Video-Tree-TRM5/.claude/skills/subagent-driven-development/code-quality-reviewer-prompt.md

Code Quality Reviewer Prompt Template

Use this template when running the code quality review as a read-only Codex session (/codex:rescue --fresh --wait). Codex reads the diff and returns a structured critique; it does not modify code. Only run this after spec compliance review has approved the work.

This same template is reused for the final whole-implementation review at the end of the plan — just widen the scope from "this task's commits" to "every commit on this branch since plan execution started," and pass the plan itself as additional context.

---

Codex review (read-only) — pass this as the /codex:rescue --fresh --wait prompt body:

description: "Review code quality for Task N"   # or "Final whole-implementation review"
prompt: |
  You are reviewing the code quality of work that has already passed spec compliance review. The implementer was a Claude Code subagent; you are Codex, and the spec review was a prior Codex pass. You are running read-only — read the diff and report; do not edit code.

  ## What Was Built

  [Brief recap of the task — one paragraph. For the final whole-implementation review, paste the full plan summary here instead.]

  ## Working Directory

  [Absolute path of the worktree.]

  ## Commits to Review

  [Space-separated list of commit SHAs. For per-task review, just this task's SHAs. For the final review, every SHA on the branch since plan execution started.]

  ## CRITICAL: Read the Code

  Do not rely on commit messages or the implementer's notes. Read `git show <sha>` for each commit. Read the surrounding files for context. The implementer is a Claude Code subagent running on a different model from a different provider; that gives you a structural advantage over single-model review, but it does not absolve you of actually reading the diff.

  ## Your Job

  Evaluate the implementation against these dimensions. Spec compliance is already verified — do not re-litigate it.

  **Architecture and decomposition**
  - Are responsibilities cleanly separated, or are concerns tangled?
  - Are the new units (functions, modules, classes) sized appropriately, or are any growing into "god" structures?
  - Do the public interfaces between new units make sense, or are internals leaking?

  **Plan conformance**
  - Does the file structure match what the plan called for?
  - Do the type signatures and names introduced here match what later tasks in the plan will expect to consume?

  **Code quality**
  - Naming: are names clear and consistent with the rest of the codebase?
  - Error handling: are failures handled, surfaced, and logged appropriately? Any swallowed exceptions or silent fallbacks?
  - Magic numbers / strings: are constants extracted and named?
  - Duplication: is there copy-pasted logic that should be a helper?
  - Dead code: any commented-out code, unreachable branches, or unused exports?
  - Readability: can a new contributor follow the flow without help?

  **Logging and run records**
  - If the change records runtime data, does it go through the project's structured logging / `SqliteRunStore` (persisted) path rather than raw SQL or ad-hoc file writes?
  - Are persisted records carrying the expected correlation IDs (patient / session / image) so they are traceable?

  **Tests**
  - Are tests meaningful (testing behavior, not implementation detail)?
  - Are edge cases covered, or is it all happy path?
  - Are there flaky patterns (timing, ordering, hidden global state)?
  - Test names: do they describe behavior, not internals?

  **File growth and codebase health**
  - Did any file cross a reasonable size threshold in this work? If so, is the new content cohesive with what was already there, or should it have been a new file?
  - Did the implementer introduce new dependencies, new build steps, or new top-level directories? Justified?

  ## Calibration

  Only flag issues that matter. Minor stylistic preferences, formatting nits, and "I would have named it differently" are NOT blocking. Categorize what you find:

  - **Critical** — Will cause real bugs, security issues, data corruption, or break a downstream task in the plan. Must be fixed.
  - **Important** — Will cause maintenance pain, confusing behavior, or fragile code. Should be fixed.
  - **Minor** — Worth mentioning so the implementer can address it cheaply, but not blocking.

  Approval requires zero Critical and zero Important issues. Minor issues are advisory.

  ## Report Format

  Return your review in this structure:

Verdict: <APPROVED | CHANGES_REQUESTED>

Strengths: - <bullet per genuine strength — be honest, brief, and specific. If there are no real strengths, say so.>

Issues: Critical: - <bullet per critical issue with file:line reference and what's wrong> - or "none" Important: - - or "none" Minor: - - or "none"

Assessment: <One short paragraph: overall quality, whether this is solid for downstream tasks, anything cross-cutting the controller should know.>

If Verdict is `APPROVED`, the controller will mark the task complete.
If Verdict is `CHANGES_REQUESTED`, the controller will send your findings to the implementer subagent via `SendMessage` and re-run this Codex review on the result.