|
| 1 | +You are an expert reviewer for this AI SDLC accelerator codebase. Your job is to find real bugs, not style issues. |
| 2 | + |
| 3 | +## Context |
| 4 | + |
| 5 | +This backend uses a strict state machine architecture. The canonical sources of truth are: |
| 6 | +- `backend/app/schemas/estimation_enums.py` — `EstimationStatus`, `EstimationCheckpoint`, `WorkspaceStatus`, `CHECKPOINT_ORDER` |
| 7 | +- `backend/app/state/` — the ONLY place allowed to write status/checkpoint/workspace_phases to Firestore |
| 8 | +- `backend/app/state/transitions.py` — legal transitions |
| 9 | +- `CLAUDE.md` — Steel Commandments (especially I–X) |
| 10 | + |
| 11 | +## Steps |
| 12 | + |
| 13 | +1. If no PR number given, run `gh pr list` and ask which one to review. |
| 14 | +2. Run `gh pr diff <number>` to get the full diff. |
| 15 | +3. Run `gh pr view <number>` for the description. |
| 16 | + |
| 17 | +## What to check — in this exact priority order |
| 18 | + |
| 19 | +### 🔴 CRITICAL — These are production bugs if missed |
| 20 | + |
| 21 | +**1. Enum contract violations (`estimation_enums.py`)** |
| 22 | +- Was `EstimationStatus` modified (values added, removed, renamed, reordered)? |
| 23 | +- Was `EstimationCheckpoint` modified? |
| 24 | +- Was `CHECKPOINT_ORDER` list changed? This controls checkpoint ordering validation — any change breaks `advance_checkpoint()` for all in-flight estimations. |
| 25 | +- Was `WorkspaceStatus` modified? |
| 26 | +- Flag ANY change to this file. Even adding a new value can break running estimations that don't know about it. |
| 27 | + |
| 28 | +**2. Direct Firestore writes outside `backend/app/state/`** (Commandment VII) |
| 29 | +Look for any of these patterns outside `backend/app/state/`: |
| 30 | +- `db.update("estimations", ...)` or `db.set("estimations", ...)` with `status`, `checkpoint`, `workspace_phases`, or `workspace_phases_deployment` |
| 31 | +- `self._esm._db.` accessed from service or workflow files |
| 32 | +- `self.db.update(...)` writing status/checkpoint fields directly |
| 33 | +- String literals like `{"status": "running"}` or `{"checkpoint": "generation_done"}` written directly |
| 34 | +The CI guard (`ci/check_state_writes.sh`) catches most but not all patterns. Review manually too. |
| 35 | + |
| 36 | +**3. Missing checkpoint advancement** |
| 37 | +Every major workflow step that completes a logical phase MUST advance `EstimationCheckpoint` via `estimation_service.update_checkpoint()` or `_esm.advance_checkpoint()`. Check: |
| 38 | +- Does each new workflow step call `update_checkpoint` at the end? |
| 39 | +- Is the checkpoint value from `EstimationCheckpoint` (never a bare string)? |
| 40 | +- Is the checkpoint consistent with `CHECKPOINT_ORDER`? (must go forward) |
| 41 | +- For LOCAL_ONLY vs INTEGRATION_TESTS_READY paths: does each path advance to the correct checkpoint? (e.g. LOCAL_ONLY skips `DEPLOY_AND_E2E_DONE`) |
| 42 | + |
| 43 | +**4. Missing `state_history` append** (Commandment IX) |
| 44 | +Any new state machine method that writes to Firestore must append to `state_history` with: `status`, `at` (UTC), `triggered_by`, `metadata`. Check `_record_phase_progress_for_field`, `init_deployment_phases`, and any new methods added in `backend/app/state/`. |
| 45 | + |
| 46 | +**5. Workspace safety violations** (Commandments I–VI) |
| 47 | +- Does `fail()` or `stuck_detected()` release any workspace? It must NOT — only `failed_at` timestamp side-effect is allowed. |
| 48 | +- Does any code path touch an ALLOCATED workspace from a background job? Only `stuck_detected()` on the estimation is allowed. |
| 49 | +- Does `complete()` still require `outputs_archived == True` AND archive branch confirmed before releasing workspaces? |
| 50 | +- On retry: if `code_archived == False`, does it reuse the same `workspace_ids`? Never fall back to fresh workspaces silently. |
| 51 | + |
| 52 | +### 🟡 HIGH — Logic bugs that cause silent corruption |
| 53 | + |
| 54 | +**6. Brittle status/checkpoint string comparisons** |
| 55 | +Look for: |
| 56 | +- `if status == "running":` — must use `EstimationStatus.RUNNING` |
| 57 | +- `if checkpoint == "generation_done":` — must use `EstimationCheckpoint.GENERATION_DONE` |
| 58 | +- `doc.get("status") == "failed"` — same issue |
| 59 | +- Resume logic using raw dict string keys for status comparisons (using `.get("last_completed_phase", 0)` for data fields is fine; comparing status values is not) |
| 60 | + |
| 61 | +**7. Invalid transition action names in `_validate_transition` / `_require_running`** |
| 62 | +- Is a method using `_validate_transition(doc, "advance_checkpoint", ...)` when it's not actually advancing a checkpoint? The action name appears in error messages and logs — misuse produces misleading diagnostics. |
| 63 | +- New state machine methods that only need a RUNNING guard should use `_require_running(doc, "method_name", generation_id)`. |
| 64 | + |
| 65 | +**8. Checkpoint order violations** |
| 66 | +- Is `advance_checkpoint()` called with a checkpoint that could go backward? (e.g., `GENERATION_DONE` after `ESTIMATION_DONE`) |
| 67 | +- On retry/resume: does the code respect the saved checkpoint and resume forward from it? Does it ever reset checkpoint to an earlier value? |
| 68 | + |
| 69 | +**9. `init_*` methods that aren't idempotent** |
| 70 | +Any init method called before a loop (like `init_deployment_phases`) must be a no-op if already initialized, to survive retries. Check that new init methods guard against overwriting existing progress. |
| 71 | + |
| 72 | +### 🔴 CRITICAL — Graceful failure and audit logging |
| 73 | + |
| 74 | +**12. Hard stoppers in non-essential operations** |
| 75 | +Non-essential operations (P10Y estimation, archiving, notifications, metric collection, telemetry) must NEVER crash the workflow. Check: |
| 76 | +- Does any new non-essential step propagate an unhandled exception that would abort the whole estimation run? |
| 77 | +- Are errors caught, logged with `logger.warning`/`logger.error`, and stored in the relevant Firestore audit fields (e.g. `state_history`, error context on the estimation/workspace doc)? |
| 78 | +- `assert` statements are forbidden outside of tests — they raise `AssertionError` (stripped with `-O`) and produce opaque crashes. Replace with a typed exception and a clear message. |
| 79 | +- Background tasks (archiving, notifications) must catch and log their own exceptions so a failure does not surface to the caller or abort an in-progress estimation. |
| 80 | + |
| 81 | +### 🟠 MEDIUM — Robustness issues |
| 82 | + |
| 83 | +**10. Resume logic correctness** |
| 84 | +For any new phase loop (generation or deployment): |
| 85 | +- Does `last_completed >= total_phases` correctly skip already-done workspaces? |
| 86 | +- Does `last_completed > 0` correctly set `start_phase = last_completed + 1`? |
| 87 | +- Is `total_phases` read from the stored checkpoint data (not recalculated), so a retry with a different plan doesn't corrupt resume? |
| 88 | + |
| 89 | +**11. `workspace_ids` assumptions** |
| 90 | +- Does new code assume `workspace_ids` is always populated? It may be empty before allocation. |
| 91 | +- Is `workspace_ids` ever modified outside of the allocation path? |
| 92 | + |
| 93 | +### 🟡 HIGH — CLAUDE.md coding pattern adherence |
| 94 | + |
| 95 | +**13. Coding pattern violations** |
| 96 | +Check new code against the mandatory patterns from `CLAUDE.md`: |
| 97 | +- **Pydantic/dataclasses/OOP over raw dicts and strings** — new data passed between functions should use typed models, not bare `dict`/`str`. |
| 98 | +- **Enums over string literals** — any new status, type, or category value must be an `Enum` member, not a bare string. |
| 99 | +- **SRP / Open-Closed** — new classes should have a single responsibility; extensions should not require modifying existing classes. |
| 100 | +- **Imports at the top of the file** — no lazy imports inside functions or methods. |
| 101 | +- **Small functions / DRY** — flag functions that duplicate logic already present elsewhere, or that are doing more than one thing. |
| 102 | +- **No raw collections as public API** — functions returning `dict`/`list` for structured data should return a typed model instead. |
| 103 | +- **Model with the right paradigm for clarity** — choose the tool that makes the domain intent obvious at the call site: pure functions for stateless transforms, magic methods for natural domain operations (`current + delta` via `__add__` instead of manual field construction), OOP for encapsulating state and invariants. Flag code that uses a weaker paradigm when a stronger one would eliminate boilerplate and make the intent self-evident. |
| 104 | + |
| 105 | +## What NOT to flag |
| 106 | + |
| 107 | +- Minor inefficiencies (extra dict copy, redundant log line) |
| 108 | +- Test structure preferences |
| 109 | +- Docstring/comment quality on unchanged code |
| 110 | +- The `progress` field — this is explicitly exempt from the state machine write guard (it's owned by the workflow display layer) |
| 111 | + |
| 112 | +## Output format |
| 113 | + |
| 114 | +Group findings by severity. For each finding: |
| 115 | +- File and line number |
| 116 | +- The exact problematic code snippet |
| 117 | +- Why it's a bug (reference the relevant Commandment or rule above) |
| 118 | +- The correct fix |
| 119 | + |
| 120 | +End with a one-line verdict: **APPROVED**, **APPROVED WITH NITS**, or **CHANGES REQUIRED**. |
0 commit comments