/review-quality

name: review-quality
description: Review quality framework for the work-to-review transition gate. Guides verification of plan alignment, test quality, and code simplification before marking implementation complete. Referenced by schema guidance fields during review-phase note filling. Use when filling review-checklist notes or when asked to review completed implementation work.
user-invocable: false

Review Quality Framework

This skill defines what a reviewer must verify before implementation work advances to completion. It applies whether the reviewer is the orchestrator directly or a delegated subagent.

The review gate exists because implementation agents optimize for getting things working, not for verifying they built the right thing. Without a structured review checkpoint, planned work gets silently dropped, tests get written to pass rather than to verify, and unnecessary complexity accumulates. The review is where these failure modes get caught.

**Critical separation of concerns:** The reviewer must not be the same agent that wrote the code or the tests. An agent reviewing its own work will rationalize rather than evaluate. The reviewer reads, runs, and reports — it never fixes. If issues are found, they go back to the implementation agent for resolution.

This is the same principle the `needs-test-author` trait applies one step earlier, to test authorship itself — the test author must not be the implementer, for the same rationalize-not-evaluate reason. Verifying that separation actually held (see "Independence verification" in Area 3) is this rule applied to test authorship, not an optional extra.

---

Getting Started

The reviewer is given an MCP item ID. Use MCP tools and codebase access to gather what you need — do not expect context to be pre-loaded for you.

1. **Load the item's notes** — `query_notes(itemId=..., includeBody=true)` to retrieve the planning note and `implementation-notes`. The planning note's key depends on the item's schema: `feature-summary` (feature-implementation), `task-scope` (feature-task), or `diagnosis` (bug-fix). 2. **Read the changed files** — use the implementation notes to identify which files were modified, then read them directly. Review the actual code, not just summaries. 3. **Run the test suite** — execute the project's test command and capture the results. Do not assume tests pass because the implementation agent said they did. 4. **For items carrying the `needs-test-author` trait** — load the trait's `test-plan` and `test-manifest` notes via `query_notes(operation="list", itemId=..., includeBody=true)`, which carries the test author's own commit SHA range field, and obtain the orchestrator-provided per-child SHA table (Pre-SHA/Post-SHA/Test-Pre-SHA/ Test-Post-SHA) plus any declared orchestrator fixture-repair commit SHAs from the review handoff, for cross-checking. A trait-bearing item with no `test-plan` note is a blocking issue on its own — do not proceed to the Area 3 independence verification until the note exists.

If the planning note (feature-summary / task-scope / diagnosis) or implementation notes are missing, the review cannot proceed. Report this as a blocking issue.

Before reporting any file or artifact as missing, or any behavior as broken, verify with a direct check — Read the exact expected path, an exact-path Glob, or a reproduction — rather than inferring absence from one plausible directory or from a prior bug's pattern. Two review false-positives reached verdicts this way before being caught downstream.

---

Review Areas

These four areas form the minimum review. Each one catches a different class of failure. If the review surfaces additional concerns, include them — this is a floor, not a ceiling.

1. Build State Verification

Establish the actual state of the tests before anything else. Everything downstream depends on it — but the reviewer does not run the suite. The whole suite is the orchestrator's, run once per wave (dispatch contract, "Who runs gradle"); a reviewer re-running it adds minutes of Gradle per item and no information.

Read the recorded build state: the Review scoping slot's "Build state reviewers rely on" line, the orchestrator's `session-tracking` note, or the `test-manifest` VERIFY line (which cites scoped classes, not a suite count). Record the count, the pass/fail breakdown and the SHA it was taken at.

**If no build state is recorded, or it is for an older SHA than the diff under review:** that is a blocking finding — say so and stop; do not run the suite to fill the gap.

**If a recorded failure is listed:** Document every failure — test name, assertion message, and the file where the test lives. Do not attempt to fix failures. Do not speculate about whether failures are pre-existing or new. Test failures are a blocking issue — the item cannot advance with a failing test suite.

**If you doubt one specific result** (a red-proof line, a mapped test's claimed red): re-run only the test classes in question, scoped (`--tests`), never the whole suite, and record what you ran.

**If the recorded state is green and current:** Record it and move on.

2. Plan Alignment

Compare what was built against the planning note (feature-summary / task-scope / diagnosis). The goal is to catch drift in both directions — work that was planned but not done, and work that was done but not planned.

**Check each acceptance criterion.** Walk through the acceptance criteria from the planning note one by one. For each criterion, identify the specific code change that satisfies it. If a criterion has no corresponding implementation, flag it — either the work is incomplete or the criterion was intentionally descoped (which should appear in the implementation notes).

**Check for unplanned changes.** Review the changed files for modifications that don't trace back to any acceptance criterion. Unplanned changes aren't automatically wrong — sometimes implementat