ahmido
f13a4ce409
Implements Spec 110 Ops‑UX Enforcement and applies the repo‑wide “enterprise” standard for operation start + dedup surfaces. Key points - Start surfaces: only ephemeral queued toast (no DB notifications for started/queued/running). - Dedup paths: canonical “already queued” toast. - Progress refresh: dispatch run-enqueued browser event so the global widget updates immediately. - Completion: exactly-once terminal DB notification on completion (per Ops‑UX contract). Tests & formatting - Full suite: 1738 passed, 8 skipped (8477 assertions). - Pint: `vendor/bin/sail bin pint --dirty --format agent` (pass). Notable change - Removed legacy `RunStatusChangedNotification` (replaced by the terminal-only completion notification policy). Co-authored-by: Ahmed Darrazi <ahmed.darrazi@live.de> Reviewed-on: #134
90 lines
4.8 KiB
Markdown
90 lines
4.8 KiB
Markdown
# 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.
|