src/bmm-skills/ship/bmad-build/step-04-review.md
{{.communication_language}}. Write any file output in {{.document_output_language}}.Change {spec_file} status to in-review in the frontmatter before continuing.
Read {baseline_commit} from {spec_file} frontmatter. If {baseline_commit} is missing or NO_VCS, use best effort to determine what changed. Otherwise use the repository's version-control tooling to rewrite {diff_file} — the temp file staged in step-03, or a uniquely-named file in the system temp directory when this run has none — with a unified diff of all changes since {baseline_commit}, untracked files included. The review layers read that file; the diff text is never pasted into their prompts.
Set {claims_file} = {spec_file}. The spec is the change's own account of itself, and it goes to the edge-case layer alone — as a path, so that layer reads it only after its own tracing and the other layers never see it at all.
Writing {diff_file} is the only change this section makes. Do NOT git add anything.
Announce skipped layers first, then launch every active layer before handling any layer's result. Try running all active layers simultaneously: substitute the runtime placeholders ({diff_file}, {claims_file}) into each layer's instruction. {diff_file} is a path: substitute the absolute path and let the layer read the file — a launch prompt never carries diff text. When an instruction launches a reviewer subagent, launch that child with the prompt text after placeholder substitution; do not load the reviewer instruction file yourself. For any other customized instruction, execute it as written. Parallel means several blocking calls awaited together in this turn — never backgrounded or detached, never ending the turn to await results. When running layers as subagents, spawn every reviewer before reading or reacting to any of their output; begin collection and triage only once all are launched.
{workflow.review_layers}
If a layer's instruction requires subagents and none are available, for each such layer write under {{.implementation_artifacts}} that layer's child prompt with every file it points to — the diff, the claims, the reviewer instruction file — replaced inline by that file's contents, and every other line left exactly as written. That session shares no filesystem with this one, so its prompt has to stand alone; this is the only place you read a reviewer instruction file yourself. Then HALT. Ask the human to run each in a separate session (ideally a different LLM) and paste back the findings.
Once every layer has reported — and not before — render a verdict on each finding, ahead of any deduplication or grouping. Disregard any severity a reviewing subagent assigned — they lack the context to grade.
If ## Review Triage Log already has rows — a loopback or a resumed review — check each finding against them first. Same location and same claim as a logged row, and the code there still reads as the row describes: keep the row's verdict and route, write the row again with carried in front of the evidence, skip verification, and never patch or defer it again. Verify everything else as below.
For each finding:
A gap finding from the verification-gap layer arrives pre-verified — that layer's evidence rules made it read the tests and run the searches it cites, and triage trusts the claim as filed. Skip verification, render the verdict from the filed evidence, and weigh its filed disposition when routing. Its Other findings are verified like everything else.
Verify the finding's claim. At the cited file and line, does the bad outcome the reviewer describes actually occur? Read beyond the changed lines — follow callers, guards upstream, etc — until you can answer yes or no. A different finding about nearby code does not settle this one. Judge whether the problem is real, not whether the proposed fix is plausible. Code that loudly fails on a situation you never showed the program can reach is correct behavior, not a defect.
Render exactly one verdict from what verification established — the verdict is the whole triage decision; there is no separate keep-or-dismiss.
high (intolerable), medium (tolerable), low (cosmetic or negligible) — the bad outcome is real. Assign severity by how much it hurts end users or developers. For developer-only problems (inconsistent design, eroded invariants, duplicated sources of truth), name where it will cause trouble — which caller will diverge, which rule will break. A vague "this is messy" with no named harm is not a severity grade; use false or maybe-false instead. When the harm is real but you cannot tell how bad, pick the higher grade.false — you checked, and the bad outcome does not happen at the cited location. Write what disproves this specific claim. A true fact about nearby code that does not disprove the claim does not count.maybe-false — you could not tell whether the bad outcome happens. Write what you would need to check to find out. Use this only when the diff and surrounding code leave the question open; when they are enough to decide, pick high, medium, low, or false.Every finding gets one row in the ## Review Triage Log section of {spec_file} — verdict plus its evidence in a sentence or two; never drop, merge, or silently skip one.
Reject false findings on their refutation.
Reject low findings when it is unlikely that users or developers would meet the defect in everyday use (judged plainly — no proof needed) and the fix is more than a direct correction or deletion — adding guards, branches, parameters, or other complexity.
Out of scope: reject or defer a finding as out of scope only when the intent itself excludes it — not because the spec's scope section, the plan, or the shape of the diff says so. If only those would exclude it, keep the finding: the spec or plan drew the line somewhere the intent did not, so it routes to intent_gap or bad_spec, never to patch or defer.
Reject any finding whose fix is to edit this build's spec.
All remaining findings continue to grouping.
Group the survivors by shared root cause — two findings belong in one entry only when the same defect produced both. Same location alone is not a shared root cause, and neither is a shared fix. An entry carries every member's verified bad outcome and the highest verdict among them (high > medium > low > maybe-false).
Route each entry into exactly one triage category. A group that includes verified high, medium, or low members routes by its highest such verdict — not to defer just because a member is maybe-false. The first three are this story's problem — caused or exposed by the current change. The last is not this story's problem.
maybe-false and the claim, if true, would be medium or high — record that severity marked unverified, plus what would settle it (if it would only be low, reject it with the same note); or any entry whose fix edits agent-context files (CLAUDE.md, AGENTS.md, rules, etc).Process entries in cascading order. If intent_gap or bad_spec entries exist, they trigger a loopback — lower entries are moot since code will be re-derived. If neither exists, process patch and defer normally. Before each loopback, read {spec_file} frontmatter review_loop_iteration (missing means 0), increment it by 1, and write it back. If it exceeds 5, HALT and escalate to the human.
intent_gap — Root cause is inside <frozen-after-approval>. Revert code changes. Loop back to the human to resolve. Once resolved, read fully and follow [[bmad-snapshot:step-02-plan.md]] to re-run steps 2–4.
bad_spec — Root cause is outside <frozen-after-approval>. Before reverting code: extract KEEP instructions for positive preservation (what worked well and must survive re-derivation). Revert code changes. Read the ## Spec Change Log in {spec_file} and strictly respect all logged constraints when amending the non-frozen sections that contain the root cause. Append a new change-log entry recording: the triggering finding, what was amended, the known-bad state avoided, and the KEEP instructions. Read fully and follow [[bmad-snapshot:step-03-implement.md]] to re-derive the code, then this step will run again.
patch — Auto-fix. These are the only findings that survive loopbacks. Re-engage the step-03 implementation subagent — the same one, addressed by the name or id its launch returned; a fresh launch is not re-engagement. Send it one message, exactly this, with the findings filled in:
Review of your implementation found problems. Fix each one below with the smallest change that does the job.
Run only the tests that cover the files you edit — nothing wider. Full verification runs on my side after you return. Reply with what you changed.
- <file> — <what is wrong> — <what the smallest fix must do>
If it cannot be continued, apply the patches yourself. Then re-run the checks in {spec_file}'s ## Verification section, if present; if verification fails and the failure cannot be fixed, HALT and escalate to the human. Rewrite {diff_file} so it reflects the patched tree.
defer — Append one new entry to {{.implementation_artifacts}}/deferred-work.md using this format. Do not modify existing entries or look for duplicates.
- source_spec: `{spec_file}`
summary: <one sentence>
evidence: <why this is real; for a maybe-false finding, what evidence would settle it>
Read fully and follow [[bmad-snapshot:step-05-present.md]]