Some checks failed
Main Confidence / confidence (push) Failing after 48s
## Summary - implement the finding outcome taxonomy end-to-end with canonical resolve, close, reopen, and verification semantics - align finding UI, filters, audit metadata, review summaries, and export/read-model consumers to the shared outcome semantics - add focused Pest coverage and complete the spec artifacts for feature 231 ## Details - manual resolve is limited to the canonical `remediated` outcome - close and reopen flows now use bounded canonical reasons - trusted system clear and reopen distinguish verified-clear from verification-failed and recurrence paths - duplicate lifecycle backfill now closes findings canonically as `duplicate` - accepted-risk recording now uses the canonical `accepted_risk` reason - finding detail and list surfaces now expose terminal outcome and verification summaries - review, snapshot, and review-pack consumers now propagate the same outcome buckets ## Filament / Platform Contract - Livewire v4.0+ compatibility remains intact - provider registration is unchanged and remains in `bootstrap/providers.php` - no new globally searchable resource was introduced; `FindingResource` still has a View page and `TenantReviewResource` remains globally searchable false - lifecycle mutations still run through confirmed Filament actions with capability enforcement - no new asset family was added; the existing `filament:assets` deploy step is unchanged ## Verification - `cd apps/platform && ./vendor/bin/sail bin pint --dirty --format agent` - `cd apps/platform && ./vendor/bin/sail artisan test --compact tests/Feature/Findings/FindingWorkflowServiceTest.php tests/Feature/Findings/FindingRecurrenceTest.php tests/Feature/Findings/FindingsListFiltersTest.php tests/Feature/Filament/FindingResolvedReferencePresentationTest.php tests/Feature/Findings/FindingOutcomeSummaryReportingTest.php tests/Feature/Findings/FindingRiskGovernanceProjectionTest.php` - `cd apps/platform && ./vendor/bin/sail artisan test --compact tests/Feature/Findings tests/Feature/Filament/FindingResolvedReferencePresentationTest.php tests/Feature/Models/FindingResolvedTest.php tests/Unit/Findings/FindingWorkflowServiceTest.php` - `cd apps/platform && ./vendor/bin/sail artisan test --compact tests/Feature/TenantReview/TenantReviewExplanationSurfaceTest.php tests/Feature/TenantReview/TenantReviewRegisterTest.php tests/Feature/ReviewPack/TenantReviewDerivedReviewPackTest.php` - browser smoke: `/admin/findings/my-work` -> finding detail resolve flow -> queue regression check passed ## Notes - this commit also includes the existing `.github/agents/copilot-instructions.md` workspace change that was already present in the worktree when all changes were committed Co-authored-by: Ahmed Darrazi <ahmed.darrazi@live.de> Reviewed-on: #267
7.2 KiB
7.2 KiB
Research: Finding Outcome Taxonomy & Verification Semantics
Decision 1: Preserve the primary findings status family and derive verification meaning
- Decision: Keep the existing
Findinglifecycle statuses (new,triaged,in_progress,reopened,resolved,closed,risk_accepted) and deriveResolved pending verificationversusVerified clearedfrom bounded canonical reason keys plus existing audit history. - Rationale:
App\Models\Findingalready defines open and terminal status sets,FindingWorkflowServiceand its tests assume that matrix, and the spec explicitly forbids a new primary status just to represent verification. The narrowest viable change is to make terminal meaning derive from bounded reason families instead of expanding the workflow state machine. - Alternatives considered:
- Add a new
verifiedstatus: rejected because it widens queue semantics, transition rules, and operator routing for a problem that can remain derived. - Add a second outcome table or reporting-only register: rejected because there is no new independent source of truth or lifecycle to persist.
- Add a new
Decision 2: Reuse existing resolved_reason and closed_reason fields, and keep reopen reasons in audit metadata
- Decision: Reuse the existing
resolved_reasonandclosed_reasoncolumns as canonical stable keys, and keepreopened_reasonas structured audit metadata instead of adding a newreopened_reasoncolumn. - Rationale:
FindingWorkflowServicealready writesresolved_reason,closed_reason,system_origin, andreopened_reasoninto current record state and audit metadata. The service only needs to stop accepting arbitrary free-form text and instead validate against bounded key families. This keeps the implementation inside existing persistence truth and matches the spec's no-new-entity posture. - Alternatives considered:
- Add a new JSON outcome payload on
findings: rejected because it would create a second semantic store for data already represented by existing columns and audit events. - Keep free-form textarea input as the primary meaning: rejected because list filters, review consumers, and audit readers cannot safely depend on prose.
- Add a new JSON outcome payload on
Decision 3: Use one findings-local semantics helper rather than page-local mappings or a generic framework
- Decision: Add one findings-local helper, such as
App\Support\Findings\FindingOutcomeSemantics, to convert current finding truth into terminal-outcome labels, verification state, and report buckets. - Rationale: Multiple readers already need the same meaning:
FindingResource,FindingRiskGovernanceResolver,ReviewRegister,TenantReviewResource, and findings-derived summary or projection code. A single findings-local helper is justified because at least two real consumers already exist, while a generic governance-wide taxonomy engine would be broader than the current release truth. - Alternatives considered:
- Keep logic inside
FindingResourceonly: rejected because review and reporting consumers would immediately drift. - Build a broad cross-domain governance taxonomy framework: rejected because the current scope is bounded to findings terminal outcomes.
- Keep logic inside
Decision 4: Use canonical reason keys to make system-cleared outcomes row-visible, and keep audit history for path reconstruction
- Decision: Treat canonical resolve keys as the row-level source for pending-versus-verified meaning. Manual resolve uses a bounded operator key such as
remediated, while trusted system-clear reasons continue to use bounded system keys such asno_longer_drifting,permission_granted,permission_removed_from_registry,role_assignment_removed, andga_count_within_threshold. - Rationale:
resolveBySystem()currently writessystem_origininto audit metadata, but that audit-only flag is not enough for list filters or report buckets. Using canonical resolve keys as the current-row truth makes terminal meaning queryable without adding a new column, while the audit trail still records whether the final verified-clear state came from a direct system clear or a later confirmation after manual resolution. - Alternatives considered:
- Add a persisted
verification_statecolumn: rejected because the same meaning can be derived from the current status plus canonical reason keys. - Depend only on audit metadata for verified-clear meaning: rejected because list filters and read models should not need audit log reconstruction to classify current records.
- Add a persisted
Decision 5: Widen the system resolve path narrowly enough to confirm already resolved findings
- Decision: Allow the system-clear path to update a finding that is already
resolvedwhen the purpose is to move it from operator-declared resolution to a trusted verified-clear reason. - Rationale:
FindingWorkflowService::resolveBySystem()currently only accepts open findings, which is too narrow for the spec requirement that a previously resolved finding may later be confirmed clear by trusted evidence. The narrowest fix is to widen the system path for this exact case instead of adding a new status or a second persistence field. - Alternatives considered:
- Force a reopen-then-resolve cycle to represent verification: rejected because it would falsify the user-visible history and generate unnecessary workflow churn.
- Add a second workflow method dedicated to verification with separate persisted state: rejected because it adds more surface than the current release needs.
Decision 6: Keep risk_accepted separate from remediation and verification semantics
- Decision: Preserve
risk_acceptedas its own terminal class governed by exception validity, not as a close reason and not as a verified-clear outcome. - Rationale:
FindingRiskGovernanceResolveralready interpretsaccepted_riskand exception validity separately, and the spec requires Spec 154 semantics to remain authoritative. This means the outcome taxonomy should exposerisk_acceptedas a distinct terminal bucket while continuing to compute governance validity independently. - Alternatives considered:
- Fold
risk_acceptedinto generic closed outcomes: rejected because that would hide governance validity consequences. - Treat valid accepted risk as verified clear: rejected because risk acceptance is an administrative governance decision, not proof of remediation.
- Fold
Decision 7: Replace existing ad hoc keys in one pass instead of preserving aliases
- Decision: Replace existing ad hoc or legacy-like reason strings inside current code, factories, and backfill jobs during implementation rather than preserving alias maps.
- Rationale: The repository is still pre-production, LEAN-001 explicitly rejects compatibility shims when there is no live production data, and current code already shows several ad hoc reason strings such as
duplicate,accepted_risk, andconsolidated_duplicate. The cleanest implementation is one canonical bounded family, updated everywhere in the same slice. - Alternatives considered:
- Maintain alias translation tables for old and new reason keys: rejected because it adds avoidable compatibility machinery.
- Leave existing reasons mixed and normalize only in the UI: rejected because backend tests, report buckets, and audit semantics would remain ambiguous.