TenantAtlas/specs/110-ops-ux-enforcement/spec.md

267 lines
18 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Feature Specification: Ops-UX Enforcement & Cleanup (Enterprise Standard Rollout)
**Feature Branch**: `110-ops-ux-enforcement`
**Created**: 2026-02-23
**Status**: Draft
**Depends On**: `specs/055-ops-ux-rollout/spec.md`
## Spec Scope Fields *(mandatory)*
- **Scope**: tenant (all operation-run-producing flows within a tenant)
- **Primary Routes**: No new routes. Affected internal flows: Inventory Sync, Backup Schedule Retention, Backup Schedule Run, Bulk Policy Export, Bulk Restore Run Force Delete, Add/Remove Policies to Backup Set, Restore Run execution.
- **Data Ownership**: `operation_runs` (tenant-scoped), `notifications` (tenant user-scoped). No schema changes required.
- **RBAC**: No new RBAC surfaces. Existing capability gates on triggering operations remain unchanged. Notification delivery is limited to the initiator user. System/scheduled runs with no initiator receive no DB notification.
*Canonical-view fields not applicable — no new views introduced.*
## User Scenarios & Testing *(mandatory)*
### User Story 1 — No Silent Completions (Priority: P1)
As a tenant admin, when I trigger any tracked operation (Inventory Sync, Retention, Backup Schedule Run), I always receive exactly one terminal DB notification upon completion, so I can audit the outcome without checking the Monitoring hub manually.
**Why this priority**: Silent completions break auditability — the core promise of the Ops-UX system. Missing terminal notifications mean admins have no persistent outcome record outside the Monitoring hub.
**Independent Test**: Trigger (or simulate) an inventory sync run to terminal state and assert exactly one `OperationRunCompleted` DB notification exists for the initiator.
**Acceptance Scenarios**:
1. **Given** an `inventory_sync` OperationRun with an initiator, **When** `InventorySyncService` transitions it to a terminal outcome, **Then** exactly one `OperationRunCompleted` DB notification is persisted for the initiator user.
2. **Given** an `apply_backup_retention` OperationRun with an initiator, **When** `ApplyBackupScheduleRetentionJob` completes (success or failure), **Then** exactly one `OperationRunCompleted` DB notification is persisted for the initiator user.
3. **Given** an OperationRun with **no initiator** (system/scheduled run), **When** the run transitions to terminal, **Then** zero DB notifications are emitted.
---
### User Story 2 — No Notification Spam (Priority: P1)
As a tenant admin, I never receive duplicate completion DB notifications for a single run, and I never receive queued/running state DB notifications for any operation.
**Why this priority**: Notification spam erodes trust in the notification surface. Even one duplicate makes admins ignore notifications — defeating the entire audit layer.
**Independent Test**: Simulate a `RunBackupScheduleJob` to completion and assert zero queued/running DB notifications exist, and exactly one terminal DB notification.
**Acceptance Scenarios**:
1. **Given** a backup schedule run job enqueued and completed, **When** all job code paths execute, **Then** zero queued/running DB notifications are persisted, and exactly one terminal `OperationRunCompleted` exists for the initiator.
2. **Given** a `BulkPolicyExportJob` that reaches terminal state via any path (success / abort / circuit-break), **Then** exactly one terminal `OperationRunCompleted` DB notification exists and zero notifications from job-level `sendToDatabase()` calls.
3. **Given** any bulk job that previously sent custom completion DB notifications (`BulkRestoreRunForceDeleteJob`, `AddPoliciesToBackupSetJob`, `RemovePoliciesFromBackupSetJob`), **When** the job completes, **Then** no job-level DB notifications are emitted.
---
### User Story 3 — Legacy Notification Removed (Priority: P1)
As a tenant admin running a restore operation, my completion feedback comes exclusively from the canonical `OperationRunCompleted` notification, not from a legacy `RunStatusChangedNotification` with inconsistent copy or link behavior.
**Why this priority**: The legacy class is an out-of-system notification that bypasses canonical delivery, creating inconsistent UX copy and a second notification channel that cannot be centrally controlled.
**Independent Test**: Confirm `RunStatusChangedNotification` class does not exist in `app/` and `ExecuteRestoreRunJob` no longer references it. Restore run completion produces exactly one `OperationRunCompleted`.
**Acceptance Scenarios**:
1. **Given** `ExecuteRestoreRunJob` completes a restore run to terminal state, **Then** no `RunStatusChangedNotification` is dispatched, and exactly one `OperationRunCompleted` DB notification is persisted for the initiator.
2. **Given** a developer searches `app/` and `tests/` for `RunStatusChangedNotification`, **Then** no results are found (class deleted, all references removed).
---
### User Story 4 — Regression Guards Enforce the Constitution (Priority: P1)
As a developer working on the repo, if I accidentally introduce a direct `$operationRun->update(['status' => ...])` outside `OperationRunService`, a CI guard test immediately fails with a clear file + snippet report so I know exactly what to fix.
**Why this priority**: Without automated guards, the enforcement degrades over time as new features are added. Guards are the only scalable way to maintain the constitution without manual code review on every PR.
**Independent Test**: Introduce a synthetic violation in a temp file, run the guard test, confirm it fails with actionable output. Remove the violation, confirm the test passes.
**Acceptance Scenarios**:
1. **Given** the codebase contains a direct `$operationRun->update(['status' => ...])` outside `OperationRunService`, **When** the guard test runs, **Then** it fails and outputs the violating file path and snippet.
2. **Given** a job file contains both an OperationRun reference and a `sendToDatabase()` call (not on the allowlist), **When** the DB-notification guard test runs, **Then** it fails with the file path.
3. **Given** any file in `app/` or `tests/` references `RunStatusChangedNotification`, **When** the legacy-class guard test runs, **Then** it fails.
4. **Given** the codebase has no violations, **When** all guard tests run, **Then** all pass green.
---
### User Story 5 — Canonical "Already Queued" Toast (Priority: P2)
As a tenant admin triggering an operation that is already queued, I receive a consistent, canonical "already queued" toast message rather than ad-hoc copy from individual feature components, so the experience is uniform across all operation types.
**Why this priority**: P2 polish — non-blocking, but addresses the last remaining ad-hoc UX copy point identified in the audit.
**Independent Test**: Trigger the "already queued" dedup path in `BackupSetPolicyPickerTable` and assert the toast uses the canonical presenter output.
**Acceptance Scenarios**:
1. **Given** an operation is already queued, **When** a user attempts to queue it again via the policy picker, **Then** a toast is shown using `OperationUxPresenter::alreadyQueuedToast(...)` with canonical copy.
---
### Edge Cases
- What happens when a job fails with an unhandled exception — does the OperationRun get stuck in a non-terminal state? *(Assumption: existing job `failed()` callback or `finally` block already transitions via service; confirm during implementation per-file.)*
- What happens when `OperationRunService` itself throws during terminal transition? *(Assumption: run stays non-terminal; pre-existing behavior outside scope of this spec.)*
- What if an OperationRun has no initiator and a job previously sent a DB notification — will removing that path cause silence? *(Confirmed acceptable: system runs are auditable via Monitoring hub only.)*
## Requirements *(mandatory)*
**Constitution alignment — OPS-UX-001**: This spec enforces the Ops-UX 3-surface contract. All status/outcome transitions must be routed through `OperationRunService`. Terminal DB notifications are sent exclusively via `OperationRunCompleted` from within the service. No job or feature code may send its own completion DB notification.
**Constitution alignment — No new Filament screens**: This spec makes no changes to Filament Resources, RelationManagers, or Pages (beyond the optional P2 presenter helper). The UI Action Matrix is not required.
**Constitution alignment — No new Graph calls / OperationRun types**: This spec modifies existing operation flows only; it does not introduce new operation types or Graph calls.
**Constitution alignment — BADGE-001**: No new status badge values introduced.
### Functional Requirements
- **FR-001**: All OperationRun `status` and `outcome` field transitions MUST go through `OperationRunService` canonical transition methods. Direct `$operationRun->update(['status' => ...])`, `$operationRun->status = ...`, `$operationRun->outcome = ...`, or bulk query updates on status/outcome are forbidden outside `OperationRunService`.
- **FR-002**: `OperationRunService` MUST emit exactly one `OperationRunCompleted` DB notification to the initiator when transitioning a run to a terminal outcome (when an initiator exists).
- **FR-003**: No job or service code outside `OperationRunService` MAY emit a DB notification representing operation completion, abort, or terminal state.
- **FR-004**: No code anywhere MAY emit a DB notification for queued or running operation states.
- **FR-005**: `RunStatusChangedNotification` MUST be deleted. No references to it may remain in `app/` or `tests/`.
- **FR-006**: `InventorySyncService` MUST transition to terminal state exclusively via `OperationRunService`, not via direct model updates.
- **FR-007**: `ApplyBackupScheduleRetentionJob` MUST transition to terminal state exclusively via `OperationRunService`.
- **FR-008**: `TenantpilotBackfillWorkspaceIds` console command MUST use the canonical transition if it transitions OperationRun status (initiator may be null; no DB notification emitted in that case).
- **FR-009**: `RunBackupScheduleJob` MUST NOT emit any queued or completion DB notifications; outcome notification is handled by `OperationRunService` terminal transition.
- **FR-010**: `BulkPolicyExportJob`, `BulkRestoreRunForceDeleteJob`, `AddPoliciesToBackupSetJob`, and `RemovePoliciesFromBackupSetJob` MUST NOT call `sendToDatabase()` for operation completion or abort feedback.
- **FR-011**: Context-only updates (e.g., updating `context`, `message`, `reason_code` fields without touching `status` or `outcome`) are permitted directly on the model outside `OperationRunService`.
- **FR-012**: Three Pest guard tests MUST exist and pass in CI:
- Guard A: Detects direct status/outcome transitions outside `OperationRunService`; reports file + snippet.
- Guard B: Detects `sendToDatabase()` calls in operation-flow jobs that also reference OperationRun; reports file path.
- Guard C: Detects any reference to `RunStatusChangedNotification` in `app/` or `tests/`.
- **FR-013** *(P2)*: `OperationUxPresenter` MUST expose an `alreadyQueuedToast(...)` static helper returning canonical copy + duration (+ optional "View run" action).
- **FR-014** *(P2)*: `BackupSetPolicyPickerTable` dedup toast MUST use `OperationUxPresenter::alreadyQueuedToast(...)`.
## Scope (Known Violations — Remediation Targets)
### Status transition bypass (direct model update — silent completion)
| File | Violation | Priority |
|------|-----------|----------|
| `app/Services/Inventory/InventorySyncService.php` | Direct `update([status/outcome])` — silent completion | P0 |
| `app/Jobs/ApplyBackupScheduleRetentionJob.php` | Direct `update([...])` — silent completion | P0 |
| `app/Console/Commands/TenantpilotBackfillWorkspaceIds.php` | Direct status update | P1 |
### Job-level DB notifications (duplicates / queued spam)
| File | Violation | Priority |
|------|-----------|----------|
| `app/Jobs/RunBackupScheduleJob.php` | Queued DB notification + custom finished notification | P0 |
| `app/Jobs/BulkPolicyExportJob.php` | Multiple `sendToDatabase()` paths | P1 |
| `app/Jobs/BulkRestoreRunForceDeleteJob.php` | Multiple `sendToDatabase()` paths | P1 |
| `app/Jobs/AddPoliciesToBackupSetJob.php` | Custom completion DB notifications | P1 |
| `app/Jobs/RemovePoliciesFromBackupSetJob.php` | Custom completion DB notifications | P1 |
### Legacy notification outside system
| File | Violation | Priority |
|------|-----------|----------|
| `app/Jobs/ExecuteRestoreRunJob.php` | References `RunStatusChangedNotification` | P0 |
| `app/Notifications/RunStatusChangedNotification.php` | Class to delete | P0 |
### Optional polish (P2)
| File | Violation | Priority |
|------|-----------|----------|
| `app/Livewire/BackupSetPolicyPickerTable.php` | Ad-hoc "already queued" toast (non-canonical copy) | P2 |
---
## Tasks
### Phase 1 — P0: Fix silent completions
- [ ] T110-001 (P0) `InventorySyncService` — replace direct terminal `update([status/outcome])` with `OperationRunService` canonical transition method
- [ ] T110-002 (P0) `ApplyBackupScheduleRetentionJob` — replace direct terminal `update([...])` with canonical transition method
- [ ] T110-003 (P1) `TenantpilotBackfillWorkspaceIds` — replace direct status update with canonical transition method (initiator may be null)
### Phase 2 — P0: Remove legacy notification
- [ ] T110-010 (P0) `ExecuteRestoreRunJob` — remove `RunStatusChangedNotification` invocation; rely on service terminal notification
- [ ] T110-011 (P0) Delete `RunStatusChangedNotification` class; assert zero references remain
### Phase 3 — P0/P1: Remove job-level DB notifications
- [ ] T110-020 (P0) `RunBackupScheduleJob` — remove queued DB notification producer
- [ ] T110-021 (P0) `RunBackupScheduleJob` — remove custom finished DB notification producer
- [ ] T110-022 (P1) `BulkPolicyExportJob` — remove all `sendToDatabase()` branches for completion/abort
- [ ] T110-023 (P1) `BulkRestoreRunForceDeleteJob` — remove all completion/abort `sendToDatabase()` branches
- [ ] T110-024 (P1) `AddPoliciesToBackupSetJob` — remove custom completion/failure DB notifications
- [ ] T110-025 (P1) `RemovePoliciesFromBackupSetJob` — remove custom completion/failure DB notifications
### Phase 4 — Guards (mandatory)
- [ ] T110-030 (P0) Pest guard: No direct OperationRun status/outcome transitions outside OperationRunService (file + snippet report; context-only updates allowed)
- [ ] T110-031 (P0) Pest guard: Jobs must not emit `sendToDatabase()` in operation flows (allowlist escape hatch if needed, minimal)
- [ ] T110-032 (P0) Pest guard: No references to `RunStatusChangedNotification` in `app/` or `tests/`
### Phase 5 — Optional polish (P2)
- [ ] T110-040 (P2) Add `OperationUxPresenter::alreadyQueuedToast(...)` canonical helper
- [ ] T110-041 (P2) Migrate `BackupSetPolicyPickerTable` dedup toast to `alreadyQueuedToast`
---
## Testing Plan (Pest)
### Guard tests (mandatory — CI enforcement layer)
- `tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php`
- `tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php`
- `tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php`
### Regression tests (mandatory for P0 fixes)
- `tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php` — exactly one `OperationRunCompleted` for initiator; none for system run
- `tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php` — exactly one `OperationRunCompleted`; no direct model transitions
- `tests/Feature/OpsUx/Regression/BackupScheduleRunNotificationTest.php` — zero queued DB notifications; exactly one terminal notification
- `tests/Feature/OpsUx/Regression/BulkJobCircuitBreakerTest.php` — circuit-breaker / abort path yields exactly one terminal `OperationRunCompleted` and zero job-level DB notifications (representative bulk job)
### Recommended test locations
- Guards: `tests/Feature/OpsUx/Constitution/`
- Regressions: `tests/Feature/OpsUx/Regression/`
## Success Criteria *(mandatory)*
### Measurable Outcomes
- **SC-001**: Every tracked operation with an initiator produces exactly one persistent DB notification upon terminal completion — zero operations complete silently.
- **SC-002**: Zero queued/running DB notifications are emitted across all operation flows.
- **SC-003**: Zero duplicate completion DB notifications for any single operation run across all exit paths (success, failure, partial, circuit-break).
- **SC-004**: `RunStatusChangedNotification` class is fully deleted — zero references remain in the codebase.
- **SC-005**: All three CI guard tests pass green on a clean codebase and fail with actionable output when a synthetic violation is introduced.
- **SC-006**: All existing OpsUx test suites and related test files pass without regression after changes.
- **SC-007** *(P2)*: All "already queued" dedup feedback paths use identical canonical copy, verifiable by a single source-of-truth presenter call.
---
## Definition of Done (DoD)
- [ ] All in-scope files no longer directly update status/outcome outside `OperationRunService`
- [ ] All in-scope jobs no longer emit completion/abort/queued DB notifications
- [ ] `RunStatusChangedNotification` deleted; zero references in `app/` and `tests/`
- [ ] Three guard tests exist and pass in CI (and fail with actionable output on synthetic violations)
- [ ] All OpsUx regression tests pass
- [ ] Full test suite green (no regressions)
- [ ] Pint formatting clean for all touched files
- [ ] `AGENTS.md` / constitution updated to reference the non-negotiable Ops-UX rule if not already present from 055 follow-up
---
## Assumptions
1. `OperationRunService` already exposes a canonical terminal transition method (from Spec 055); this spec calls it, not redeclares it.
2. The `failed()` job lifecycle callback or try/finally blocks in affected jobs already exist (or will be added) to ensure terminal transitions even on unhandled exceptions — confirmed during implementation per-file.
3. Guard tests are static analysis (filesystem grep-based) Pest tests, not runtime tests. They do not require a running application.
4. The allowlist for Guard B (job DB notifications) is intentionally minimal. Any new entry requires justification in a spec comment.
5. P2 tasks (T110-040/041) are optional and do not gate release of P0/P1 work.
---
## Rollout / PR Slicing
| PR | Tasks | Priority |
|----|-------|----------|
| PR-A | T110-001, T110-002 + regression tests (inventory sync, retention) | P0 |
| PR-B | T110-010, T110-011 + restore run tests | P0 |
| PR-C | T110-020, T110-021 + backup schedule tests | P0 |
| PR-D | T110-022T110-025 + bulk job tests | P1 |
| PR-E | T110-030, T110-031, T110-032 (guards — can land early) | P0 |
| PR-F | T110-040, T110-041 (optional polish) | P2 |