126 lines
7.5 KiB
Markdown
126 lines
7.5 KiB
Markdown
# Research — Enforce Creation-Time Finding Invariants
|
|
|
|
**Date**: 2026-04-29
|
|
**Spec**: [spec.md](spec.md)
|
|
|
|
This document records the repo-grounded planning decisions for the creation-time findings hardening slice after Specs 253 and 254. All decisions assume the current pre-production LEAN-001 posture.
|
|
|
|
## Decision 1 — Scope the feature to the three active finding writers that currently persist `Finding` records
|
|
|
|
**Decision**: Treat baseline compare drift, Entra admin roles, and permission posture as the full active writer set for this feature.
|
|
|
|
**Rationale**:
|
|
- Repo search shows only five direct `Finding` creation sites in app code: one `new Finding` path in `CompareBaselineToTenantJob` and four `Finding::create()` sites split between Entra admin roles and permission posture.
|
|
- No other shipped service or job currently persists `Finding` records directly, so widening the slice would be speculative rather than repo-driven.
|
|
- This keeps the hardening aligned with the spec's stated bounded scope and avoids inventing a new writer registry.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/app/Jobs/CompareBaselineToTenantJob.php`
|
|
- `apps/platform/app/Services/EntraAdminRoles/EntraAdminRolesFindingGenerator.php`
|
|
- `apps/platform/app/Services/PermissionPosture/PermissionPostureFindingGenerator.php`
|
|
|
|
**Alternatives considered**:
|
|
- Widen to every findings consumer or downstream summary surface.
|
|
- Rejected: those surfaces consume findings truth but do not create it.
|
|
- Add a speculative "all writers" registry now.
|
|
- Rejected: violates ABSTR-001 because three concrete paths are already directly visible.
|
|
|
|
## Decision 2 — Enforce lifecycle readiness in the same write path, not through a later repair pass
|
|
|
|
**Decision**: Require each in-scope writer to create or refresh lifecycle-ready findings inside the same code path that persists or updates the record.
|
|
|
|
**Rationale**:
|
|
- Spec 253 removes runtime backfill surfaces and this feature explicitly exists to prevent reintroducing that repair dependency.
|
|
- Current code already initializes lifecycle fields on new creates and updates some fields inline on repeated observations; that makes write-path hardening the narrowest correct implementation.
|
|
- Downstream findings pages, inboxes, and intake queues already assume findings are ready for immediate use.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/app/Jobs/CompareBaselineToTenantJob.php`
|
|
- `apps/platform/app/Filament/Resources/FindingResource.php`
|
|
- `apps/platform/app/Filament/Pages/Findings/MyFindingsInbox.php`
|
|
|
|
**Alternatives considered**:
|
|
- Reintroduce a maintenance action or backfill command.
|
|
- Rejected: directly conflicts with the cleanup direction from Spec 253.
|
|
- Add a deploy-time or queue-time repair hook.
|
|
- Rejected: widens scope and hides invariant ownership.
|
|
|
|
## Decision 3 — Preserve `FindingWorkflowService::reopenBySystem()` as the only shared reopen path
|
|
|
|
**Decision**: Keep terminal-to-reopened mutation on `FindingWorkflowService::reopenBySystem()` and treat open-record lifecycle normalization as the actual planning gap.
|
|
|
|
**Rationale**:
|
|
- `reopenBySystem()` already validates terminal-status eligibility, recalculates SLA or due state, clears resolved or closed markers, writes audit context, and dispatches the reopened alert notification.
|
|
- Bypassing it would create a second reopen dialect and risk inconsistent audit or notification semantics.
|
|
- The repo gap is not reopened-state ownership; it is that current open-record repair is still distributed across per-family `observeFinding()` logic and currently emphasizes seen-history more than full lifecycle readiness.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/app/Services/Findings/FindingWorkflowService.php`
|
|
- `apps/platform/app/Jobs/CompareBaselineToTenantJob.php`
|
|
- `apps/platform/app/Services/EntraAdminRoles/EntraAdminRolesFindingGenerator.php`
|
|
- `apps/platform/app/Services/PermissionPosture/PermissionPostureFindingGenerator.php`
|
|
|
|
**Alternatives considered**:
|
|
- Reopen findings directly inside each writer.
|
|
- Rejected: duplicates side effects and weakens audit consistency.
|
|
- Create a new generic lifecycle orchestration framework.
|
|
- Rejected: too broad for three known writers.
|
|
|
|
## Decision 4 — Keep recurrence identity family-owned and preserve each writer's current double-count boundary
|
|
|
|
**Decision**: Keep the existing recurrence identity and observation boundary per family instead of forcing one synthetic cross-domain algorithm.
|
|
|
|
**Rationale**:
|
|
- Baseline compare already uses `recurrence_key` plus `fingerprint` with `current_operation_run_id` to suppress duplicate `times_seen` increments for the same compare run.
|
|
- Entra admin roles and permission posture use later `observedAt` comparisons to advance seen history.
|
|
- The operator need is one canonical finding identity per issue family, not one universal recurrence engine.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/app/Jobs/CompareBaselineToTenantJob.php`
|
|
- `apps/platform/app/Services/EntraAdminRoles/EntraAdminRolesFindingGenerator.php`
|
|
- `apps/platform/app/Services/PermissionPosture/PermissionPostureFindingGenerator.php`
|
|
|
|
**Alternatives considered**:
|
|
- Normalize all writers onto a single recurrence service.
|
|
- Rejected: would add abstraction without current-release need.
|
|
- Count every repeated observation the same way across all writers.
|
|
- Rejected: risks breaking baseline retry semantics.
|
|
|
|
## Decision 5 — The current proof gap is inline repair of incomplete lifecycle fields on existing findings
|
|
|
|
**Decision**: Plan for explicit regression proof that existing open findings with missing lifecycle fields are repaired inline on active paths, especially for `sla_days` and `due_at`.
|
|
|
|
**Rationale**:
|
|
- Existing tests already prove creation readiness, idempotence, and reopen behavior in all three families.
|
|
- Repo code also already repairs `first_seen_at`, `last_seen_at`, and `times_seen` inline when existing findings are re-observed.
|
|
- What is not yet clearly owned as one invariant is the repair of incomplete lifecycle fields such as missing due-state data on existing findings encountered through active writers.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/tests/Feature/Baselines/BaselineCompareFindingsTest.php`
|
|
- `apps/platform/tests/Feature/EntraAdminRoles/EntraAdminRolesFindingGeneratorTest.php`
|
|
- `apps/platform/tests/Feature/PermissionPosture/PermissionPostureFindingGeneratorTest.php`
|
|
|
|
**Alternatives considered**:
|
|
- Rely on current creation and reopen tests only.
|
|
- Rejected: leaves FR-255-007 partially implied.
|
|
- Add a new browser or broad workflow suite.
|
|
- Rejected: too expensive for a write-path invariant gap.
|
|
|
|
## Decision 6 — Keep schema and DB constraints out of the slice unless they become an explicit stop condition
|
|
|
|
**Decision**: Keep the default plan app-code-only. Any database-level constraint or migration-based enforcement is a bounded follow-up candidate or an explicit stop condition, not part of this feature by default.
|
|
|
|
**Rationale**:
|
|
- The repo is pre-production and LEAN-001 favors direct replacement over compatibility layers.
|
|
- The current code already has the necessary domain seams to harden write-time behavior without changing the schema.
|
|
- Folding a constraint into this feature would silently broaden it from write-path hardening into data rollout and compatibility review.
|
|
|
|
**Evidence**:
|
|
- `.specify/memory/constitution.md`
|
|
- `specs/255-enforce-finding-creation-invariants/spec.md`
|
|
|
|
**Alternatives considered**:
|
|
- Add `NOT NULL` or check constraints now.
|
|
- Rejected: outside the smallest bounded slice.
|
|
- Keep the option undefined.
|
|
- Rejected: the plan must name the stop condition explicitly so task generation stays bounded. |