ahmido
b511b08371
Some checks failed
PR Fast Feedback / fast-feedback (pull_request) Failing after 1m0s
This PR removes the legacy "acknowledged" status compatibility for findings and unifies the canonical operation types (e.g., transitioning from baseline_capture to baseline.capture). It includes updated tests, models, and services to reflect these changes. Co-authored-by: Ahmed Darrazi <ahmed.darrazi@live.de> Reviewed-on: #296
129 lines
7.7 KiB
Markdown
129 lines
7.7 KiB
Markdown
# Research — Remove Legacy Acknowledged Finding Status Compatibility
|
|
|
|
**Date**: 2026-04-29
|
|
**Spec**: [spec.md](spec.md)
|
|
|
|
This document records the repo-grounded planning decisions for the acknowledged-compatibility cleanup slice. All decisions assume the current pre-production LEAN-001 posture.
|
|
|
|
## Decision 1 — Remove acknowledged semantics at shared seams, not through page-local relabeling
|
|
|
|
**Decision**: Delete productive `acknowledged` compatibility from the shared findings seams that currently define status truth, query truth, badge vocabulary, filter vocabulary, workflow eligibility, and capability language. Do not treat this as a page-local label replacement.
|
|
|
|
**Rationale**:
|
|
- The drift is cross-surface today: `Finding::STATUS_ACKNOWLEDGED`, `Finding::openStatusesForQuery()`, `FilterOptionCatalog`, `BadgeCatalog`, `FindingWorkflowService`, `Capabilities::TENANT_FINDINGS_ACKNOWLEDGE`, and multiple summary consumers all preserve the old vocabulary.
|
|
- A list-only or badge-only rename would leave summary counts, disabled helper text, and RBAC wording inconsistent.
|
|
- XCUT-001 requires converging on the existing shared path instead of adding local exceptions.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/app/Models/Finding.php`
|
|
- `apps/platform/app/Services/Findings/FindingWorkflowService.php`
|
|
- `apps/platform/app/Support/Filament/FilterOptionCatalog.php`
|
|
- `apps/platform/app/Support/Badges/Domains/FindingStatusBadge.php`
|
|
- `apps/platform/app/Support/Auth/Capabilities.php`
|
|
- `apps/platform/app/Services/Auth/RoleCapabilityMap.php`
|
|
|
|
**Alternatives considered**:
|
|
- Relabel `acknowledged` to `triaged` only on findings pages.
|
|
- Rejected: shared queries, summaries, and capability guidance would still preserve conflicting truth.
|
|
- Keep a read-side compatibility mapper indefinitely.
|
|
- Rejected: LEAN-001 forbids preserving a pre-production legacy branch without a current-release need.
|
|
|
|
## Decision 2 — Treat stale acknowledged rows and columns as pre-production residue, not as a runtime compatibility contract
|
|
|
|
**Decision**: Keep the cleanup code-only by default. Remove productive semantics first and do not add migration shims, fallback readers, or preserved UI labels just because `acknowledged_at` or `acknowledged_by_user_id` columns still exist locally.
|
|
|
|
**Rationale**:
|
|
- The repo is explicitly pre-production, and LEAN-001 prefers replacement or deletion over historical compatibility behavior.
|
|
- The current problem is active workflow semantics in code and tests, not an unavoidable database constraint.
|
|
- The narrowest correct implementation is to stop writing, querying, and presenting `acknowledged` as current findings truth.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/app/Models/Finding.php`
|
|
- `.specify/memory/constitution.md` (LEAN-001, PERSIST-001)
|
|
- `docs/product/spec-candidates.md`
|
|
|
|
**Alternatives considered**:
|
|
- Add a migration or fallback reader now.
|
|
- Rejected: widens scope into persistence work not justified by current release truth.
|
|
- Preserve `legacy acknowledged` UI labels until later.
|
|
- Rejected: keeps the removed semantics productized.
|
|
|
|
## Decision 3 — Keep findings-derived review, report, and support-diagnostic consumers in scope where they surface current open-work truth
|
|
|
|
**Decision**: Include review/report and support-diagnostic consumers in this cleanup only where they derive current findings-open counts, disclosure text, or issue grouping from the same shared status helpers as canonical summaries.
|
|
|
|
**Rationale**:
|
|
- Repo truth shows `TenantReviewSectionFactory` and `SupportDiagnosticBundleBuilder` still depend on acknowledged-aware status logic.
|
|
- Leaving those consumers out would preserve productive status drift even if the findings register and canonical `/admin` summaries were cleaned.
|
|
- This remains bounded because the slice is limited to findings-derived open-work semantics, not broader review-pack, evidence, or diagnostic redesign.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/app/Services/TenantReviews/TenantReviewSectionFactory.php`
|
|
- `apps/platform/app/Support/SupportDiagnostics/SupportDiagnosticBundleBuilder.php`
|
|
- `apps/platform/app/Support/CustomerHealth/WorkspaceHealthSummaryQuery.php`
|
|
- `apps/platform/app/Support/GovernanceInbox/GovernanceInboxSectionBuilder.php`
|
|
|
|
**Alternatives considered**:
|
|
- Defer review/report and diagnostics entirely.
|
|
- Rejected: current productive consumers would still present split workflow truth.
|
|
- Broaden into review-pack or diagnostic domain redesign.
|
|
- Rejected: outside the smallest cleanup slice.
|
|
|
|
## Decision 4 — Keep non-finding acknowledgement domains explicitly out of scope
|
|
|
|
**Decision**: Do not rename or remove acknowledgement semantics outside the findings domain, including verification-check acknowledgement, onboarding-verification acknowledgement, and restore impact acknowledgement.
|
|
|
|
**Rationale**:
|
|
- Those domains carry different user intent and do not prove that findings status compatibility must remain.
|
|
- Mixing them into this slice would widen terminology cleanup into unrelated workflows.
|
|
- The spec already depends on maintaining bounded ownership and avoiding accidental cross-domain churn.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/app/Services/Verification/VerificationCheckAcknowledgementService.php`
|
|
- `apps/platform/app/Filament/Support/VerificationReportViewer.php`
|
|
- `apps/platform/app/Filament/Pages/Workspaces/ManagedTenantOnboardingWizard.php`
|
|
- `apps/platform/app/Filament/Resources/RestoreRunResource.php`
|
|
|
|
**Alternatives considered**:
|
|
- Normalize every `acknowledge*` term in the repo at once.
|
|
- Rejected: too broad and not required for the findings cleanup to be correct.
|
|
|
|
## Decision 5 — Keep validation in focused Feature, Unit, and retained guard lanes only
|
|
|
|
**Decision**: Prove the cleanup with focused findings workflow tests, focused summary-consumer tests, focused capability cleanup tests, and the already-retained heavy-governance guard coverage. Do not add browser coverage.
|
|
|
|
**Rationale**:
|
|
- The business risk is shared-seam drift, not browser choreography or async execution.
|
|
- The repo already has meaningful findings, generator, summary, and guard test families that can be narrowed or rewritten.
|
|
- TEST-GOV-001 prefers the smallest proving lane mix that guards business truth.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/tests/Feature/PermissionPosture/PermissionPostureFindingGeneratorTest.php`
|
|
- `apps/platform/tests/Feature/EntraAdminRoles/EntraAdminRolesFindingGeneratorTest.php`
|
|
- `apps/platform/tests/Feature/Findings/FindingsIntakeQueueTest.php`
|
|
- `apps/platform/tests/Feature/Guards/NoAdHocStatusBadgesTest.php`
|
|
- `apps/platform/tests/Feature/Guards/FilamentTableStandardsGuardTest.php`
|
|
|
|
**Alternatives considered**:
|
|
- Add browser smoke coverage.
|
|
- Rejected: low additional value for this cleanup.
|
|
- Preserve broad acknowledged-compatibility fixture families.
|
|
- Rejected: would keep the removed semantics alive in the suite.
|
|
|
|
## Decision 6 — Remove the stale findings capability alias instead of translating it forever
|
|
|
|
**Decision**: Delete `tenant_findings.acknowledge` from the canonical capability registry and role mappings, and converge disabled helper text and authorization expectations on the surviving findings permissions.
|
|
|
|
**Rationale**:
|
|
- The acknowledged alias keeps RBAC language inconsistent with the canonical triage action.
|
|
- Capability drift is part of the user-visible problem in this slice, not a separate concern.
|
|
- RBAC-UX requires server-side truth to stay on the canonical capability set, not parallel aliases.
|
|
|
|
**Evidence**:
|
|
- `apps/platform/app/Support/Auth/Capabilities.php`
|
|
- `apps/platform/app/Services/Auth/RoleCapabilityMap.php`
|
|
- `apps/platform/app/Policies/FindingPolicy.php`
|
|
|
|
**Alternatives considered**:
|
|
- Keep the alias as an undocumented backward-compatibility seam.
|
|
- Rejected: preserves the exact semantics blocker this feature is intended to remove. |