diff --git a/.github/agents/copilot-instructions.md b/.github/agents/copilot-instructions.md index 27632db..2b2c6c4 100644 --- a/.github/agents/copilot-instructions.md +++ b/.github/agents/copilot-instructions.md @@ -58,8 +58,8 @@ ## Code Style PHP 8.4.15: Follow standard conventions ## Recent Changes +- 110-ops-ux-enforcement: Added PHP 8.4.x + Laravel 12, Filament v5, Livewire v4 - 109-review-pack-export: Added PHP 8.4 (Laravel 12) + Filament v5, Livewire v4, Laravel Framework v12 - 109-review-pack-export: Added [if applicable, e.g., PostgreSQL, CoreData, files or N/A] -- 109-review-pack-export: Added [if applicable, e.g., PostgreSQL, CoreData, files or N/A] diff --git a/specs/110-ops-ux-enforcement/checklists/requirements.md b/specs/110-ops-ux-enforcement/checklists/requirements.md index 73ecbd0..217eb47 100644 --- a/specs/110-ops-ux-enforcement/checklists/requirements.md +++ b/specs/110-ops-ux-enforcement/checklists/requirements.md @@ -31,7 +31,7 @@ ## Feature Readiness ## Notes -- All items pass. Spec is ready for `/speckit.plan`. -- Scope violation tables provide concrete remediation targets (11 files across 4 violation categories). -- Guard tests (FR-012) are explicitly specced as static analysis (grep-based Pest), not runtime — no application booting required. -- P2 tasks (T110-040/041) are optional and explicitly gated; they do not block P0/P1 delivery. +- All items pass. +- Spec includes the mandatory UI Action Matrix and explicitly states “no new screens” while allowing targeted start-surface cleanup. +- Remediation targets are enumerated in the spec’s “Known Violations” tables; the executable task list is the single source of truth in `tasks.md`. +- Guard tests (FR-012) are specced as static analysis (filesystem scan) with explicit allowlist, so they fail fast with actionable output. diff --git a/specs/110-ops-ux-enforcement/contracts/no-api-changes.md b/specs/110-ops-ux-enforcement/contracts/no-api-changes.md new file mode 100644 index 0000000..c7044a8 --- /dev/null +++ b/specs/110-ops-ux-enforcement/contracts/no-api-changes.md @@ -0,0 +1,9 @@ +# Contracts: No API Changes + +This feature introduces **no new HTTP endpoints**, no new API resources, and no changes to existing request/response contracts. + +Scope is limited to internal operation flow behavior: + +- OperationRun status/outcome transitions must go through `OperationRunService`. +- Queued/running DB notifications are banned. +- Terminal completion notification is initiator-only and emitted exactly once. diff --git a/specs/110-ops-ux-enforcement/data-model.md b/specs/110-ops-ux-enforcement/data-model.md new file mode 100644 index 0000000..3980cb2 --- /dev/null +++ b/specs/110-ops-ux-enforcement/data-model.md @@ -0,0 +1,39 @@ +# Phase 1 Design: Data Model (No Schema Changes) + +This feature does not introduce schema changes. It enforces consistent usage of existing entities. + +## Entity: OperationRun (`operation_runs`) + +**Ownership/scoping**: + +- Tenant-scoped operational artifact. +- Initiator user is optional (system/scheduled runs). + +**Key fields (existing)**: + +- `id` +- `workspace_id` / `tenant_id` (scoping) +- `user_id` (initiator; nullable) +- `type` (operation type string) +- `status` (`queued`/`running`/`completed`) +- `outcome` (terminal outcome; nullable until completed) +- `started_at`, `completed_at` +- `summary_counts` (JSON/array of numeric-only whitelisted keys) +- `failure_summary` (sanitized bounded array) +- `context` (additional metadata; mutable) + +**Invariants enforced by this feature**: + +- All transitions of `status` and `outcome` happen through `OperationRunService::updateRun()`. +- The only operation-related DB notification is the terminal `OperationRunCompleted`, emitted when transitioning into `completed` and only when `user_id` exists. + +## Entity: Database Notifications (`notifications`) + +**Ownership/scoping**: + +- User-scoped records (`notifiable_type=User`), used for persistent notification audit. + +**Invariants enforced by this feature**: + +- No queued/running state notifications are persisted. +- Exactly one terminal operation completion notification is persisted per OperationRun + initiator. diff --git a/specs/110-ops-ux-enforcement/plan.md b/specs/110-ops-ux-enforcement/plan.md new file mode 100644 index 0000000..e407cbf --- /dev/null +++ b/specs/110-ops-ux-enforcement/plan.md @@ -0,0 +1,157 @@ +# Implementation Plan: Ops-UX Enforcement & Cleanup (Enterprise Standard Rollout) + +**Branch**: `110-ops-ux-enforcement` | **Date**: 2026-02-23 | **Spec**: [specs/110-ops-ux-enforcement/spec.md](specs/110-ops-ux-enforcement/spec.md) +**Input**: Feature specification from `/specs/110-ops-ux-enforcement/spec.md` + +**Note**: This template is filled in by the `/speckit.plan` command. See `.specify/scripts/` for helper scripts. + +## Summary + +Enforce the Ops-UX “3-surface contract” by removing non-canonical operation-run status/outcome transitions and banning queued/running DB notifications across in-scope operation flows. + +Implementation is split into: + +- Cleanup: move all terminal transitions to `OperationRunService`, remove job-level `sendToDatabase()` completion/queued notifications, and delete the legacy `RunStatusChangedNotification`. +- Enforcement: add Pest guard tests (filesystem scans) that fail CI when these patterns reappear. +- Verification: add focused regression tests asserting “exactly one terminal `OperationRunCompleted` notification for initiator; none for system runs; and zero queued/running notifications”. + +## Technical Context + + + +**Language/Version**: PHP 8.4.x +**Primary Dependencies**: Laravel 12, Filament v5, Livewire v4 +**Storage**: PostgreSQL (Sail) +**Testing**: Pest v4 (`vendor/bin/sail artisan test --compact`) +**Target Platform**: Docker via Laravel Sail (local); Dokploy (staging/prod) +**Project Type**: Laravel monolith (Filament admin panel) +**Performance Goals**: Guard tests run in < 1s locally; overall targeted test pack < 30s +**Constraints**: + +- No schema changes and no new routes. +- Terminal DB notification is initiator-only and emitted exactly once. +- No queued/running DB notifications anywhere (including `OperationRunQueued`). +- Existing RBAC/capability gates for starting operations remain unchanged. + +**Scale/Scope**: Repo-wide enforcement via guard tests; remediation limited to enumerated violating flows in the spec. + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.* + +This spec is a cleanup/enforcement layer and does not introduce new screens, Graph calls, or operation types. + +- Inventory-first / Read-write separation: PASS (no new inventory/backups semantics; no new write UX) +- Graph contract path: PASS (no Graph call additions) +- Deterministic capabilities: PASS (no capability model changes) +- RBAC-UX / isolation: PASS (no new routes or cross-plane access) +- Run observability: PASS (this spec strengthens dedupe + terminal notifications by removing ad-hoc notifications) +- Automation: PASS (no scheduling behavior changes; only notification/transition handling) +- Data minimization: PASS (removes notification spam; no payload storage changes) +- Badge semantics: PASS (no new badges) +- Filament UI contracts: PASS (no new screens; targeted updates to existing start surfaces only). The spec includes a mandatory UI Action Matrix. +## Project Structure + +### Documentation (this feature) + +```text +specs/110-ops-ux-enforcement/ +├── plan.md # This file (/speckit.plan command output) +├── research.md # Phase 0 output (/speckit.plan command) +├── data-model.md # Phase 1 output (/speckit.plan command) +├── quickstart.md # Phase 1 output (/speckit.plan command) +├── contracts/ # Phase 1 output (/speckit.plan command) +└── tasks.md # Phase 2 output (/speckit.tasks command - NOT created by /speckit.plan) +``` + +### Source Code (repository root) + + +```text +app/ +├── Jobs/ +├── Notifications/ +├── Services/ +└── Support/ + +config/ + +database/ +├── migrations/ +└── factories/ + +resources/ +├── views/ +└── css/ + +routes/ + +tests/ +└── Feature/ +``` + +**Structure Decision**: Laravel monolith. Enforcement is implemented as: + +- Targeted production code edits under `app/` (jobs/services/notifications). +- Guard + regression tests under `tests/Feature/OpsUx/**`. + +## Complexity Tracking + +> **Fill ONLY if Constitution Check has violations that must be justified** + +| Violation | Why Needed | Simpler Alternative Rejected Because | +|-----------|------------|-------------------------------------| +| [e.g., 4th project] | [current need] | [why 3 projects insufficient] | +| [e.g., Repository pattern] | [specific problem] | [why direct DB access insufficient] | + +## Phase 0 — Outline & Research + +**Inputs**: [specs/110-ops-ux-enforcement/spec.md](specs/110-ops-ux-enforcement/spec.md), [constitution](.specify/memory/constitution.md) + +Research is captured in [specs/110-ops-ux-enforcement/research.md](specs/110-ops-ux-enforcement/research.md) and resolves: + +- Guard-test heuristics (regex/scan approach) to keep false positives low. +- Where/how queued DB notifications are emitted today, and how to disable them by default. +- Preferred test strategy for “exactly once terminal notification”. + +## Phase 1 — Design & Contracts + +Design artifacts are captured in: + +- [specs/110-ops-ux-enforcement/data-model.md](specs/110-ops-ux-enforcement/data-model.md) (no schema changes; key entity behavior) +- [specs/110-ops-ux-enforcement/contracts/](specs/110-ops-ux-enforcement/contracts/) (no new API endpoints; documented as “no contract changes”) +- [specs/110-ops-ux-enforcement/quickstart.md](specs/110-ops-ux-enforcement/quickstart.md) (how to run the focused tests) + +**Design highlights**: + +- The only terminal DB notification is `OperationRunCompleted`, emitted from `OperationRunService` when an initiator exists. +- Guard A scans `app/**/*.php` for `->update([...])` arrays that include `status` and/or `outcome` keys, excluding `OperationRunService`. +- Guard B scans `app/**/*.php` for files that both reference an OperationRun signal and emit DB notifications (`sendToDatabase(` or `->notify(`), with a strict allowlist. +- Guard C ensures the legacy notification class is fully removed. + +## Phase 2 — Execution Plan (Implementation) + +Work is implemented in small, reviewable steps: + +1. **P0 fixes (silent completions)**: replace direct terminal updates with `OperationRunService` transitions in the enumerated services/jobs. +2. **P0 fixes (notification spam)**: remove queued/running and custom completion DB notifications from the enumerated jobs. +3. **P0 fixes (legacy removal)**: delete `RunStatusChangedNotification` and remove any invocation. +4. **Enforcement**: add Pest guard tests (A/B/C) + minimal allowlist. +5. **Verification**: add focused regression tests for the key flows (inventory sync, retention, backup schedule run) proving the “exactly once terminal notification” contract. + +**Out of scope**: new UI pages, schema changes, new operation types, Graph contract changes. + +## Constitution Check (Post-Phase 1 Re-check) + +- PASS: No new routes, no Graph calls, no new Filament screens. +- PASS: Strengthens Run Observability by centralizing terminal notification emission. +- PASS: RBAC-UX/isolation unaffected (no new access paths). diff --git a/specs/110-ops-ux-enforcement/quickstart.md b/specs/110-ops-ux-enforcement/quickstart.md new file mode 100644 index 0000000..766eb29 --- /dev/null +++ b/specs/110-ops-ux-enforcement/quickstart.md @@ -0,0 +1,23 @@ +# Quickstart: Ops-UX Enforcement & Cleanup + +## Prereqs + +- Sail running: `vendor/bin/sail up -d` + +## Run the focused guard tests + +Once implemented, run the guard tests only: + +- `vendor/bin/sail artisan test --compact --filter=OpsUx` + +If the tests are split by directory as in the spec: + +- `vendor/bin/sail artisan test --compact tests/Feature/OpsUx/Constitution` + +## Run the focused regression tests + +- `vendor/bin/sail artisan test --compact tests/Feature/OpsUx/Regression` + +## Format touched files + +- `vendor/bin/sail bin pint --dirty --format agent` diff --git a/specs/110-ops-ux-enforcement/research.md b/specs/110-ops-ux-enforcement/research.md new file mode 100644 index 0000000..ea1c0cb --- /dev/null +++ b/specs/110-ops-ux-enforcement/research.md @@ -0,0 +1,89 @@ +# Phase 0 Research: Ops-UX Enforcement & Cleanup + +This research document resolves implementation uncertainties for the enforcement spec and records key decisions. + +## Decision 1 — Implement guards as Pest “architecture tests” (filesystem scan) + +**Decision**: Implement Guard A/B/C as Pest tests that scan PHP source files in `app/` (and for Guard C also `tests/`) and fail CI with actionable file + snippet output. + +**Rationale**: + +- Runs in the same CI pipeline as the rest of the test suite (no new tooling step). +- Enforcement is repo-wide and does not rely on developer discipline or code review memory. +- Can produce highly actionable failure output (file path + snippet) without external dependencies. + +**Alternatives considered**: + +- PHPStan custom rules: higher precision, but adds configuration + toolchain scope. +- `nikic/php-parser` AST scanning: high precision but adds a dependency (out of bounds per repo constraints). + +## Decision 2 — Guard A detection uses tokenizer-assisted scanning (not pure regex) + +**Decision**: Implement Guard A using PHP’s built-in tokenizer (`token_get_all()`) to detect `->update([...])` call sites and then inspect the array literal for `status` / `outcome` keys. + +**Rationale**: + +- Pure multi-line regex is brittle with nested arrays, comments, and strings. +- Tokenization gives a dependency-free way to avoid common false positives and to compute an approximate line number. +- The spec’s intended heuristic (“`->update([...])` block contains status/outcome keys”) maps naturally to tokens. + +**Alternatives considered**: + +- Regex-only `DOTALL` matching with string/comment skipping (`(*SKIP)(*F)`): simpler, but can still drift across the wrong closing bracket and become noisy. +- Manual char-by-char scanner after a simple “find start” regex: workable, but token-based is typically easier to maintain in PHP. + +**Repo evidence**: + +- Canonical status/outcome transitions live in `OperationRunService::updateRun()`. +- Violations exist in jobs/services that call `$operationRun->update([... 'status' => ..., 'outcome' => ...])` directly (enumerated in the spec). + +## Decision 3 — Guard B flags “OperationRun in scope” + “DB notification emission” + +**Decision**: Implement Guard B as a two-signal scan: + +- **OperationRun signal**: file contains any of: `use App\\Models\\OperationRun;`, `OperationRun`, `$this->operationRun`, `$operationRun`. +- **DB emission signal**: file contains either `sendToDatabase(` or `->notify(`. +- **Allowlist**: only `app/Services/OperationRunService.php` and `app/Notifications/OperationRunCompleted.php`. + +**Rationale**: + +- Enforces the constitution: completion notifications are centralized, queued/running DB notifications are banned. +- Intentionally errs on the side of flagging new ad-hoc notification producers. + +**Alternatives considered**: + +- “DB emission only” = `sendToDatabase(` and ignore `->notify(`: lower noise, but would miss notify-based DB notifications. +- Restrict scan to `app/Jobs/**` and `app/Services/**`: lower noise, but spec requires `app/**/*.php` for defense-in-depth. + +**Repo evidence (high-level)**: + +- `OperationRunService` currently emits both `OperationRunCompleted` and `OperationRunQueued` via `->notify(...)`. +- There are multiple job files with `->sendToDatabase(...)` in code paths that also handle an OperationRun. +- There are Filament resource actions that both dispatch jobs with `$operationRun` and call `->sendToDatabase(...)` for queued feedback. These are queued DB notifications and conflict with FR-004. + +## Decision 4 — Ban queued DB notifications by changing service defaults + +**Decision**: Change `OperationRunService::dispatchOrFail(..., bool $emitQueuedNotification = true)` (and any upstream helper that takes `emitQueuedNotification`) so the default is `false`, and ensure no call site opts back into queued DB notifications. + +**Rationale**: + +- Matches FR-004 / FR-012b: queued/running DB notifications are forbidden repo-wide. +- Prevents new call sites from accidentally enabling queued DB notifications by omission. + +**Alternatives considered**: + +- Delete `OperationRunQueued` entirely: higher churn; leaving the class unused is acceptable as long as it’s never emitted. +- Keep default `true` but require opt-out everywhere: violates FR-012b and is easy to regress. + +## Decision 5 — “Exactly once terminal notification” relies on `previousStatus` guard + +**Decision**: Rely on `OperationRunService::updateRun()` behavior that emits `OperationRunCompleted` only when transitioning into `Completed` from a non-completed status. + +**Rationale**: + +- Centralizes “exactly once” semantics in a single place. +- Keeps jobs/services free of notification-specific logic. + +**Alternatives considered**: + +- Add additional DB-level dedupe: unnecessary if the service is the only producer and transitions are canonical. diff --git a/specs/110-ops-ux-enforcement/spec.md b/specs/110-ops-ux-enforcement/spec.md index fbc6e9e..999d14f 100644 --- a/specs/110-ops-ux-enforcement/spec.md +++ b/specs/110-ops-ux-enforcement/spec.md @@ -5,10 +5,18 @@ # Feature Specification: Ops-UX Enforcement & Cleanup (Enterprise Standard Rollo **Status**: Draft **Depends On**: `specs/055-ops-ux-rollout/spec.md` +## Clarifications + +### Session 2026-02-23 + +- Q: For Guard A, what detection strategy should we use to catch forbidden status/outcome transitions while allowing context-only updates? → A: Scan `app/` for `->update([...])` blocks that include `status` or `outcome` keys (same call block, multi-line), excluding `app/Services/OperationRunService.php`. +- Q: Should queued/running DB notifications be allowed (e.g., `OperationRunQueued`) or is the DB notification surface terminal-only? → A: Ban queued/running DB notifications entirely (including `OperationRunQueued`); the only DB notification for operations is the terminal `OperationRunCompleted` notification. +- Q: For Guard B (DB-notification guard), what scanning scope/heuristic should we use? → A: Scan `app/**/*.php` and fail when a file both references an OperationRun (import or variable/property usage) and emits any database notification (`sendToDatabase(` or `->notify(`), except for an explicit allowlist. + ## 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. +- **Primary Routes**: No new routes. Affected internal flows: Inventory Sync, Backup Schedule Retention, Backup Schedule Run, Bulk Policy Export, Bulk Restore Run Restore, Bulk Restore Run Force Delete, Bulk Policy Unignore, Entra Group manual sync, 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. @@ -104,7 +112,18 @@ ## 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 — Filament scope**: This spec introduces **no new Filament screens**. It DOES include targeted updates to existing Filament start surfaces (Resources/Pages) to remove queued/running DB notifications and ensure start-surface feedback is toast-only. + +**UI Action Matrix (mandatory)** + +| Surface | User action | Authorization | Start surface feedback | Progress surface | Terminal surface | +|---------|-------------|---------------|------------------------|------------------|-----------------| +| `PolicyResource` (tenant context) | Trigger an operation that creates/reuses an OperationRun | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| `PolicyVersionResource` (tenant context) | Trigger an operation that creates/reuses an OperationRun | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| `BackupScheduleResource` (tenant context) | Run / queue backup schedule operation | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| `TenantResource` (tenant context) | Trigger an operation that creates/reuses an OperationRun | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| `EntraGroupResource` → `ListEntraGroups` (tenant context) | Start manual sync operation | Existing capability checks (unchanged) | Toast-only (no DB notification) + optional “View run” link | Monitoring → Operations + run detail | Exactly one `OperationRunCompleted` DB notification to initiator (service-owned) | +| Scheduled/system operations | Jobs run without an initiator | Existing schedule/lock semantics (unchanged) | N/A | Monitoring → Operations + run detail | No DB notification (initiator is null); Monitoring remains canonical | **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. @@ -115,18 +134,20 @@ ### 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-004**: No code anywhere MAY emit a DB notification for queued or running operation states. This includes `OperationRunQueued` and any other “queued” / “started” / “running” database notifications. - **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-010**: `BulkPolicyExportJob`, `BulkRestoreRunForceDeleteJob`, `BulkRestoreRunRestoreJob`, `BulkPolicyUnignoreJob`, `AddPoliciesToBackupSetJob`, and `RemovePoliciesFromBackupSetJob` MUST NOT call `sendToDatabase()` for operation completion, abort, or queued/running feedback. +- **FR-010b**: Filament start surfaces that initiate operation-run-producing flows MUST NOT persist queued/running DB notifications (including any `sendToDatabase()` “queued” notifications). Start feedback is toast-only. - **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 A: Detects direct status/outcome transitions outside `OperationRunService`; reports file + snippet. Implementation MUST scan `app/**/*.php` for `->update(` calls whose update array includes a `status` and/or `outcome` key (multi-line block match allowed), excluding `app/Services/OperationRunService.php`. Context-only updates without `status`/`outcome` MUST NOT fail this guard. + - Guard B: Detects DB-notification emissions in operation-flow code; reports file path. Implementation MUST scan `app/**/*.php` and fail when a file contains BOTH (a) an OperationRun signal (`use App\\Models\\OperationRun;` OR `OperationRun` token OR `$this->operationRun` OR `$operationRun`) AND (b) a DB-notification emission (`sendToDatabase(` OR `->notify(`). Allowed exceptions (explicit allowlist): `app/Services/OperationRunService.php`, `app/Notifications/OperationRunCompleted.php`. - Guard C: Detects any reference to `RunStatusChangedNotification` in `app/` or `tests/`. +- **FR-012b**: Operation enqueue helpers MUST NOT emit queued DB notifications by default. Any helper param like `emitQueuedNotification` MUST default to `false`, and all current call sites MUST comply. - **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(...)`. @@ -147,9 +168,21 @@ ### Job-level DB notifications (duplicates / queued spam) | `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/BulkRestoreRunRestoreJob.php` | Multiple `sendToDatabase()` paths | P1 | +| `app/Jobs/BulkPolicyUnignoreJob.php` | Multiple `sendToDatabase()` paths | P1 | | `app/Jobs/AddPoliciesToBackupSetJob.php` | Custom completion DB notifications | P1 | | `app/Jobs/RemovePoliciesFromBackupSetJob.php` | Custom completion DB notifications | P1 | +### Start-surface queued DB notifications (forbidden) + +| File | Violation | Priority | +|------|-----------|----------| +| `app/Filament/Resources/PolicyResource.php` | Queued/running DB notification emitted from a start surface | P1 | +| `app/Filament/Resources/PolicyVersionResource.php` | Queued/running DB notification emitted from a start surface | P1 | +| `app/Filament/Resources/BackupScheduleResource.php` | Queued/running DB notification emitted from a start surface | P1 | +| `app/Filament/Resources/TenantResource.php` | Queued/running DB notification emitted from a start surface | P1 | +| `app/Filament/Resources/EntraGroupResource/Pages/ListEntraGroups.php` | Queued/running DB notification emitted from a start surface | P1 | + ### Legacy notification outside system | File | Violation | Priority | @@ -165,40 +198,9 @@ ### Optional polish (P2) --- -## Tasks +## Execution Plan Reference -### 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` - ---- +The executable work breakdown for this spec lives in [specs/110-ops-ux-enforcement/tasks.md](specs/110-ops-ux-enforcement/tasks.md). The spec intentionally avoids duplicating a task list to prevent drift. ## Testing Plan (Pest) diff --git a/specs/110-ops-ux-enforcement/tasks.md b/specs/110-ops-ux-enforcement/tasks.md new file mode 100644 index 0000000..5dcc267 --- /dev/null +++ b/specs/110-ops-ux-enforcement/tasks.md @@ -0,0 +1,207 @@ +--- + +description: "Executable task list for Ops-UX Enforcement & Cleanup" + +--- + +# Tasks: Ops-UX Enforcement & Cleanup (Enterprise Standard Rollout) + +**Input**: Design documents from `/specs/110-ops-ux-enforcement/` + +- plan.md: [specs/110-ops-ux-enforcement/plan.md](specs/110-ops-ux-enforcement/plan.md) +- spec.md: [specs/110-ops-ux-enforcement/spec.md](specs/110-ops-ux-enforcement/spec.md) +- research.md: [specs/110-ops-ux-enforcement/research.md](specs/110-ops-ux-enforcement/research.md) +- data-model.md: [specs/110-ops-ux-enforcement/data-model.md](specs/110-ops-ux-enforcement/data-model.md) +- contracts/: [specs/110-ops-ux-enforcement/contracts/](specs/110-ops-ux-enforcement/contracts/) + +**Tests**: REQUIRED (Pest) — both guard tests and focused regressions. + +## Phase 1: Setup (Shared Test Infrastructure) + +**Purpose**: Create minimal shared helpers for guard tests and keep failure output consistent. + +- [ ] T001 [P] Add source scanning helper in tests/Support/OpsUx/SourceFileScanner.php + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: Cross-cutting invariants that must be true before user story work can be considered “done”. + +- [ ] T002 Update queued notification defaults in app/Services/OperationRunService.php (dispatchOrFail + enqueue helpers default emitQueuedNotification=false) +- [ ] T003 Confirm no call sites opt into queued DB notifications in app/Services/OperationRunService.php (remove/forbid emitQueuedNotification:true usage) + +**Checkpoint**: No queued/running DB notifications can be emitted by default. + +--- + +## Phase 3: User Story 1 — No Silent Completions (Priority: P1) 🎯 MVP + +**Goal**: Terminal transitions always go through `OperationRunService`, producing exactly one terminal `OperationRunCompleted` notification for initiators. + +**Independent Test**: Inventory sync + retention flow transitions to terminal and persists exactly one `OperationRunCompleted` notification for the initiator; system runs persist none. + +### Tests (write first) + +- [ ] T004 [P] [US1] Add inventory sync terminal notification regression test in tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php +- [ ] T005 [P] [US1] Add retention terminal notification regression test in tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php + +### Implementation + +- [ ] T006 [US1] Refactor terminal transition in app/Services/Inventory/InventorySyncService.php to use OperationRunService::updateRun() +- [ ] T007 [US1] Refactor terminal transition in app/Jobs/ApplyBackupScheduleRetentionJob.php to use OperationRunService::updateRun() +- [ ] T008 [US1] Refactor OperationRun status/outcome update in app/Console/Commands/TenantpilotBackfillWorkspaceIds.php to use OperationRunService::updateRun() (initiator may be null) + +**Checkpoint**: US1 regressions pass, with no silent completions. + +--- + +## Phase 4: User Story 2 — No Notification Spam (Priority: P1) + +**Goal**: Remove job-level queued/completion DB notifications and eliminate queued DB notifications from start surfaces. + +**Independent Test**: Backup schedule run + representative bulk flow complete with exactly one terminal `OperationRunCompleted` and zero queued/running DB notifications. + +### Tests (write first) + +- [ ] T009 [P] [US2] Add backup schedule run notification regression test in tests/Feature/OpsUx/Regression/BackupScheduleRunNotificationTest.php +- [ ] T010 [P] [US2] Add bulk job “abort/circuit-break” regression test in tests/Feature/OpsUx/Regression/BulkJobCircuitBreakerTest.php + +### Implementation (Jobs) + +- [ ] T011 [P] [US2] Remove queued + custom finished DB notifications in app/Jobs/RunBackupScheduleJob.php +- [ ] T012 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkPolicyExportJob.php +- [ ] T013 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkRestoreRunForceDeleteJob.php +- [ ] T029 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkRestoreRunRestoreJob.php +- [ ] T030 [P] [US2] Remove completion/abort sendToDatabase branches in app/Jobs/BulkPolicyUnignoreJob.php +- [ ] T014 [P] [US2] Remove custom completion/failure DB notifications in app/Jobs/AddPoliciesToBackupSetJob.php +- [ ] T015 [P] [US2] Remove custom completion/failure DB notifications in app/Jobs/RemovePoliciesFromBackupSetJob.php + +### Implementation (Start surfaces / Filament) + +- [ ] T016 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/PolicyResource.php (remove sendToDatabase for queued ops) +- [ ] T017 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/BackupScheduleResource.php (remove sendToDatabase for queued ops) +- [ ] T018 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/TenantResource.php (remove sendToDatabase for queued ops) +- [ ] T019 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/PolicyVersionResource.php (remove sendToDatabase for queued ops) +- [ ] T031 [P] [US2] Replace queued DB notification with toast-only queued feedback in app/Filament/Resources/EntraGroupResource/Pages/ListEntraGroups.php (remove sendToDatabase for queued ops) + +**Checkpoint**: US2 regressions pass and notifications remain terminal-only. + +--- + +## Phase 5: User Story 3 — Legacy Notification Removed (Priority: P1) + +**Goal**: Remove the out-of-system `RunStatusChangedNotification` and rely exclusively on canonical terminal `OperationRunCompleted`. + +**Independent Test**: Restore run completion produces exactly one `OperationRunCompleted` notification and there are zero references to `RunStatusChangedNotification` in `app/` and `tests/`. + +### Tests (write first) + +- [ ] T020 [P] [US3] Add restore run terminal notification regression test in tests/Feature/OpsUx/Regression/RestoreRunTerminalNotificationTest.php + +### Implementation + +- [ ] T021 [US3] Remove legacy notification invocation in app/Jobs/ExecuteRestoreRunJob.php +- [ ] T022 [US3] Delete legacy notification class app/Notifications/RunStatusChangedNotification.php + +**Checkpoint**: US3 regression passes; no legacy notification remains. + +--- + +## Phase 6: User Story 4 — Regression Guards Enforce the Constitution (Priority: P1) + +**Goal**: CI guard tests fail fast when forbidden patterns reappear. + +**Independent Test**: Guards fail with actionable output on a synthetic violation and pass on a clean codebase. + +### Guard tests + +- [ ] T023 [P] [US4] Implement Guard A in tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php (scan app/** for ->update([...]) containing status/outcome; exclude app/Services/OperationRunService.php; print snippet) +- [ ] T024 [P] [US4] Implement Guard B in tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php (scan app/** for OperationRun signal + DB notify emission; allowlist app/Services/OperationRunService.php and app/Notifications/OperationRunCompleted.php) +- [ ] T025 [P] [US4] Implement Guard C in tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php (scan app/** and tests/** for RunStatusChangedNotification) + +**Checkpoint**: Guard tests pass green and provide clear failure output. + +--- + +## Phase 7: User Story 5 — Canonical "Already Queued" Toast (Priority: P2) + +**Goal**: Dedup “already queued” messaging is canonical and consistent. + +**Independent Test**: Trigger dedup path and confirm toast uses `OperationUxPresenter::alreadyQueuedToast(...)`. + +- [ ] T026 [P] [US5] Add OperationUxPresenter::alreadyQueuedToast(...) helper in app/Support/OpsUx/OperationUxPresenter.php +- [ ] T027 [US5] Migrate dedup toast to canonical helper in app/Livewire/BackupSetPolicyPickerTable.php + +--- + +## Phase 8: Polish & Cross-Cutting Concerns + +**Purpose**: Keep docs and developer workflow aligned with final test locations. + +- [ ] T028 [P] Update quickstart commands/paths if needed in specs/110-ops-ux-enforcement/quickstart.md + +--- + +## Dependencies & Execution Order + +### User Story Completion Order + +- US1 → US2 → US3 → US4 → US5 + +Rationale: + +- US1–US3 remove known violations so the guard suite (US4) can pass on a clean codebase. +- US5 is optional polish and can land after enforcement is stable. + +### Dependency Graph + +```text +Phase 1 (Setup) ─┬─> Phase 2 (Foundational) ─┬─> US1 ─┬─> US2 ─┬─> US3 ─┬─> US4 ─┬─> US5 + │ │ │ │ │ └─> Polish + └───────────────────────────┴────────┴────────┴────────┴─────────────── +``` + +## Parallel Execution Examples + +### US1 (tests + implementation) + +```bash +T004: tests/Feature/OpsUx/Regression/InventorySyncTerminalNotificationTest.php +T005: tests/Feature/OpsUx/Regression/BackupRetentionTerminalNotificationTest.php +``` + +### US2 (jobs + start surfaces) + +```bash +T011: app/Jobs/RunBackupScheduleJob.php +T012: app/Jobs/BulkPolicyExportJob.php +T013: app/Jobs/BulkRestoreRunForceDeleteJob.php +T014: app/Jobs/AddPoliciesToBackupSetJob.php +T015: app/Jobs/RemovePoliciesFromBackupSetJob.php +T016–T019: app/Filament/Resources/*Resource.php +``` + +### US4 (guards) + +```bash +T023: tests/Feature/OpsUx/Constitution/DirectStatusTransitionGuardTest.php +T024: tests/Feature/OpsUx/Constitution/JobDbNotificationGuardTest.php +T025: tests/Feature/OpsUx/Constitution/LegacyNotificationGuardTest.php +``` + +## Implementation Strategy + +### MVP Scope (recommended) + +- Foundational (Phase 2) + US1 only. + +This yields the highest-value guarantee quickly: “no silent completions” and exactly-once terminal notifications for the most critical flows. + +### Incremental Delivery + +1. Foundational → US1 → validate +2. US2 → validate +3. US3 → validate +4. US4 guards → enforce +5. US5 polish