--- name: spec-code-review description: "Structured code review for bugs, regressions, tests, and standards. Report-only by default; apply fixes only when the current user or upstream caller explicitly requests review-and-fix. mode:agent is always report-only." argument-hint: "[mode:agent] [base:] [plan:] [task-pack: task: task-context:] [blank to review current branch, or provide PR link]" --- # Code Review Reviews code changes against intent, tests, standards, and risk lenses. When reviewer dispatch is explicitly authorized and callable, it uses selected personas and merges structured findings; otherwise it performs an honest inline report-only review with degraded coverage. ## Workflow Contract Summary - **输入：** 当前分支、显式 base、远端 branch/PR，及可选 plan/task-pack 上下文。 - **输出：** 默认 report-only 的结构化 findings、actionable queue、coverage、验证限制与 run artifact；`mode:agent` 返回 JSON。 - **硬出口：** scope/base 不可解析、task context 漂移或越界、错误 checkout、必需独立覆盖不可用时不得给出完整 merge-ready 声明。 - **权威：** source/diff/test/log 提供事实，reviewers 判断语义；review、local mutation、commit 与 landing 是四个独立授权面。 - **消费者：** 人工 reviewer、`spec-work`、caller-owned shipping/LFG 流程与后续 residual gate。 ## When to Use - Before creating a PR - After completing a task during iterative implementation - When feedback is needed on any code changes - Can be invoked standalone - Can run inside larger workflows; use `mode:agent` when the caller needs JSON instead of markdown tables ## Scenario Capability Follows `docs/contracts/workflows/scenario-capability-matrix.md`. Overrides: high-risk - `foreign-residual-workspace` -> `blocked-action-required`: stop before reviewer dispatch that depends on suspect local artifacts, checkout mutation, root-cause-like review claims, or PR-ready verdicts until the named cleanup/init action runs or the user explicitly accepts degraded evidence. - optional external-tool evidence unavailable -> `fallback-only`: use bounded direct source, diff, test, log, and user-provided evidence; disclose missing reviewer/tool coverage and do not claim unconfirmed impact or full review coverage. - `non-git-build-workspace` coverage gaps -> `partial`: review only the selected repo/inspected build surfaces and directly inspect uncovered modules before claiming they are unaffected. ## Argument Parsing Parse the invocation arguments supplied by the current host for optional tokens. Strip only recognized tokens while preserving quoted paths, Windows drive paths, URLs, and the order of the remainder before interpreting it as a PR number, GitHub URL, or branch name. | Token | Example | Effect | |-------|---------|--------| | `mode:agent` | `mode:agent` | **Report-only**: return **JSON** instead of markdown tables and skip the Stage 5c apply (the caller applies). Does not change reviewer selection, merge logic, or scope rules (see Output format) | | `mode:headless` | `mode:headless` | **Deprecated alias** for `mode:agent` | | `mode:report-only` | `mode:report-only` | **Deprecated — ignored.** Former no-artifacts mode; default behavior is review-only without checkout | | `base:` | `base:abc1234` or `base:origin/main` | Diff base on the **current checkout** (explicit; skips auto base detection) | | `plan:` | `plan:docs/plans/2026-03-25-001-feat-foo-plan.md` | Plan file for requirements verification (explicit). Supports markdown and HTML unified plans. | | `task-pack:` | `task-pack:docs/tasks/example-tasks.md` | Local task-pack source for a bounded task review. Requires `task:`, `task-context:`, `mode:agent`, and `base:`. | | `task:` | `task:T003` | Task Card ID to review from the paired task pack. | | `task-context:` | `task-context:` | Caller-captured `spec-code-review-task-context/v1` facts for digest, pre-task state, and task delta attribution. | | `depth:full` | `depth:full` | **Force the full reviewer roster** — skip the Stage 3c small-diff lite path so every always-on persona runs regardless of diff size. Use when a deep/thorough review is explicitly requested (the one escalation signal Stage 3c cannot infer from the diff). Does not change conditional selection, merge, or scope. | | `depth:auto` | `depth:auto` | **Default** — self-right-size via Stage 3c (lite roster for trivial, low-risk, code-only diffs; full roster otherwise). | | `grouping:auto` | `grouping:auto` | **Default** — build thematic triage groups when findings span distinct concerns (Stage 5 step 9b) | | `grouping:off` | `grouping:off` | Suppress triage groups: no Triage Groups section, empty `triage_groups` in JSON | | `grouping:always` | `grouping:always` | Always build triage groups, even for small reviews | **Grouping is presentation, not a mode.** The `grouping:` tokens change how the finding set is organized for triage — never reviewer selection, merge logic, scope rules, or the Stage 5c apply decision. **Mode alias:** `mode:headless` normalizes to `mode:agent`. `mode:agent` + `mode:headless` is not a conflict. Path-valued tokens split only at their first `:` and may be host-quoted so paths with spaces remain one argument. Preserve the decoded path exactly; do not split it again on whitespace. Windows drive letters inside the value and POSIX paths are data, not extra tokens. **Conflicting arguments:** Stop without dispatching reviewers when: - Multiple incompatible scope selectors appear together (e.g. `base:` **and** a PR number/branch target — `base:` means "review the current checkout against this base") - Multiple distinct `mode:` tokens other than the `mode:agent`/`mode:headless` alias pair - Multiple distinct `grouping:` tokens (e.g. `grouping:off` **and** `grouping:always`) - task-pack and task tokens must appear together, exactly once, with exactly one `task-context:` token - any task token appears without `mode:agent` or without `base:`, or task context is combined with a PR number/URL/branch target Deprecated `mode:autofix` is **not** a conflict — ignore the token and proceed with the normal flow (see below). Emit a one-line failure reason. In `mode:agent`, return JSON: `{"status":"failed","reason":"..."}`. ### Phase 0a: Freeze effective mode before any tool call Normalize mode before interpreting intent or running any repository command. This is an exit gate, not a later presentation choice: ```yaml effective_mode: default | agent source_mutation_gate: open | closed artifact_write_gate: review-artifacts-only ``` When `mode:agent` or its `mode:headless` alias is present, set `effective_mode: agent`, `mutation_policy: report-only`, and `source_mutation_gate: closed`. In this mode, adjacent fix/apply wording is intent data, not mutation authority. Do not call project-writing tools, `apply_patch`, edit/write APIs, `git apply`, `git restore`, `git checkout`, formatters with write flags, or any command that can rewrite reviewed source. The only permitted writes are run-scoped review artifacts and deterministic evidence under the exact artifact roots owned below. Generate the run ID and resolve `REVIEW_ARTIFACT_DIR` here, before scope inspection. Use the OS-native temporary-directory rules in Stage 4 and reuse the same canonical path for the entire run. Failure to create an artifact directory does not open the source mutation gate; `mode:agent` continues report-only with an in-band limitation. ### Phase 0: Resolve mutation, commit, and dispatch policy Before scope detection, derive three independent run-local facts from the current user request and any visible upstream handoff: ```yaml mutation_policy: report-only | apply-fixes commit_authorization: authorized | missing worker_dispatch_authorization: authorized | missing capability_probe: not_applicable | attempted | unavailable worker_dispatch_capability: available | missing | unknown worker_context_isolation: isolated | inherited | unknown worker_model_override: supported | unsupported | unknown worker_bounded_parallelism: supported | unsupported | unknown ``` - Ordinary requests to "review", "check", "audit", or run `spec-code-review` use `mutation_policy: report-only`. - Use `mutation_policy: apply-fixes` only when the current user or upstream caller explicitly says review-and-fix, review and fix, apply fixes, or equivalent. `mode:agent` always forces report-only even when adjacent text asks to apply. - `apply-fixes` authorizes only bounded local review-owned edits. It does not authorize commit, push, PR creation/update, tickets, or unrelated cleanup. - Set `commit_authorization: authorized` only when commit creation is separately explicit. Without commit authorization, return a verified uncommitted review-fix set. - Set `worker_dispatch_authorization: authorized` only when the current user/upstream explicitly requests subagents, personas, delegated reviewers, multi-agent review, or parallel review. A review invocation, task context, plan, available primitive, or permission setting is not dispatch authorization. Missing authorization forbids schema discovery and fixes `capability_probe: not_applicable` + `worker_dispatch_capability: unknown`. Only after authorization may the current-session registry/schema be consumed as `provider_untrusted` evidence; confirmed absence records `subagent_capability_missing`, while unavailable/incomplete/ambiguous discovery records `worker_capability_unproven`. Normalize every path as `worker_dispatch_outcome`; use only live facts for isolation, model override, parallelism, permission, capacity, output, and mutation claims. If review repo scope is ambiguous in a parent multi-repo workspace, stop before local diff claims or apply and return a failure reason naming the required selected child repo/current checkout; do not open a blocking prompt. Read-only PR-remote scope may proceed from explicit PR metadata without choosing a sibling checkout. Generated runtime mirrors remain out of source-fix scope. ## Operating principles Same review pipeline for default and `mode:agent`: - **Report-only by default; never land.** Never push, open PRs, or file tickets in any mode. Ordinary default review and every `mode:agent` run report findings only. Stage 5c runs solely for default mode with `mutation_policy: apply-fixes`; commit remains a separate authorization gate. - **Agent mode never mutates.** In **`mode:agent`** it never mutates the tree, regardless of adjacent apply/fix wording. - **No blocking prompts.** Never use `AskUserQuestion`, `request_user_input`, or other blocking question tools. Infer intent, plan, and scope from explicit tokens, git state, PR metadata, and conversation. Note uncertainty in Coverage or the verdict — do not stop to ask. - **Explicit mutations only.** Never run `gh pr checkout`, `git checkout`, `git switch`, or similar branch-switch commands. Passing a PR number, URL, or branch name selects **review scope**, not permission to mutate the working tree. To review local uncommitted work on a feature branch, check out that branch yourself (or stay on it) and pass `base:` or no target. - **Smart defaults.** Untracked files: review tracked changes only and list excluded paths in Coverage. Plan: use `plan:` when passed; otherwise discover conservatively from PR body or branch keywords. Weak advisory P2/P3 from testing/maintainability alone: demote to `testing_gaps` / `residual_risks` per Stage 5. - **Report outcomes, not machinery.** What you show the user is about the review: what's being examined (the PR/branch), which coverage is included and the one-line reason for each conditional lens, the independent cross-model pass and which model runs it, and the findings. Keep the skill's internals out of user-facing text — model-tier assignments, raw scope-mode codenames (`local-aligned`/`pr-remote`), staging the diff to disk, loading persona files, parallel-dispatch bookkeeping, and step-by-step narration of your own setup. Name what the user would recognize (a PR number, a reviewer's concern, a peer model), not the plumbing. This governs *what* you surface and suppress; it does not script the wording — use your own voice. ## Anti-Rationalization Red Flags | 红旗念头 | 停下来做什么 | | --- | --- | | 「看着没问题，跳过应有的证伪」 | 按当前风险运行被触发的 adversarial/validator/direct fact check；未运行的独立视角必须在 Coverage 降级。 | | 「这条 finding 大概成立」 | 回到 source、diff、test、log 或 artifact 核对；provider/advisory evidence 不能升级为 confirmed。 | | 「口头说一下 residual 就行」 | 产出结构化 finding、Actionable Findings、Coverage 与 durable limitation，让下游不依赖会话记忆。 | 这是注意力提醒,不是 gate,也不替代 LLM 判断;最终是否停下、如何处理仍由你按当前证据决定。 ## Output format | Invocation | Deliverable | |------------|-------------| | **Default** | Markdown report (pipe-delimited finding tables where useful) + Actionable Findings summary; optional Applied section only for explicit `mutation_policy: apply-fixes` | | **`mode:agent`** | One JSON object (see ### JSON output format below) + artifacts under the returned concrete `artifact_path` when writable | `mode:agent` is **report-only**: it skips the Stage 5c apply (the caller applies) and serializes findings as JSON instead of markdown. It does not change reviewer selection, merge logic, or scope rules — the JSON is the deterministic contract for programmatic and cross-harness callers (Codex and other harnesses). The default markdown is the human view; keep it ASCII-safe (pipe tables, `->` not middot `·`, no box-drawing) so it degrades gracefully across terminals. ## Quick Review Short-Circuit If the invocation arguments indicate the user wants a quick, fast, or light code review — and **`mode:agent` is not active** — do not dispatch the multi-agent flow. **Announce the chosen path** before any other work (Quick review vs Multi-agent review). Skip this announcement when `mode:agent` is active. Sequence: 1. **Run the harness's built-in code review.** Forward any review target after stripping tokens. Then stop — do not dispatch the multi-agent pipeline. 2. **Exemption:** If no built-in review exists, continue into the full multi-agent review. 3. **`mode:agent` bypasses this short-circuit** — always run the full multi-agent review and return JSON. **Deprecated:** `mode:autofix` is no longer supported. If passed, ignore the token; it does not create apply authorization. The run stays report-only unless the current user/upstream independently and explicitly requested review-and-fix, and `mode:agent` remains report-only regardless. ## Severity Scale All reviewers use P0-P3: | Level | Meaning | Action | |-------|---------|--------| | **P0** | Critical breakage, exploitable vulnerability, data loss/corruption | Must fix before merge | | **P1** | High-impact defect likely hit in normal usage, breaking contract | Should fix | | **P2** | Moderate issue with meaningful downside (edge case, perf regression, maintainability trap) | Fix if straightforward | | **P3** | Low-impact, narrow scope, minor improvement | User's discretion | ## Action Routing Severity answers **urgency**. `autofix_class` and `owner` are **signal** describing follow-up shape for callers — not apply permission or an apply gate. Ordinary/default review reports. Only explicit `mutation_policy: apply-fixes` can enter Stage 5c, and `mode:agent` never mutates. See `references/action-class-rubric.md` for persona guidance. | `autofix_class` | Default owner | Meaning | |-----------------|---------------|---------| | `gated_auto` | `downstream-resolver` or `human` | Concrete `suggested_fix` proposed; caller applies after judgment | | `manual` | `downstream-resolver` or `human` | Actionable work needing design input or handoff | | `advisory` | `human` or `release` | Report-only — learnings, rollout notes, residual risk | Routing rules: - **Synthesis owns the final route.** Persona-provided routing metadata is input, not the last word. - **Choose the more conservative route on disagreement.** A merged finding may move from `gated_auto` to `manual`, but never widen without stronger evidence. - **Reject `safe_auto` and `review-fixer` if present** — drop the finding or remap to `gated_auto` / `downstream-resolver` during synthesis. - **`requires_verification: true` means any caller-applied fix needs targeted tests or follow-up validation.** ## Reviewers 14 reviewer personas in layered conditionals, plus spec-first local prompt assets. Quick roster with one-line triggers below; the persona catalog in `references/persona-catalog.md` (read it at Stage 3) has the full per-persona selection criteria and spawn gates. Each selected reviewer is a generic subagent seeded with a local prompt file from `references/personas/`; do not dispatch standalone agents by type/name. **Always-on (full review):** local prompt assets `correctness-reviewer`, `testing-reviewer`, `maintainability-reviewer`, `project-standards-reviewer`, plus spec-first local prompt assets `agent-native-reviewer` and `learnings-researcher`. (Stage 3c may reduce this set to a lite roster for trivial, low-risk diffs.) **Cross-cutting conditional (per diff):** - `security-reviewer` — auth、tenant/resource authorization、untrusted model/tool/web output、dangerous sinks、reachable dependency risk - `performance-reviewer` — DB queries, data transforms, caching, async - `api-contract-reviewer` — routes, serializers, type signatures, versioning - `frontend-quality-reviewer` — user-visible state, semantic/a11y, focus/contrast, responsive and presentation/data boundaries - `data-migration-reviewer` — migration files / schema dumps / backfills (see spawn gate in Stage 3) - `reliability-reviewer` — error handling, retries, timeouts, background jobs - `adversarial-reviewer` — >=50 changed code lines, or auth / payments / data mutations / external APIs, or a **silent-pass verification mechanism** (CI/CD gating logic, merge-blocking checks, build/deploy steps, coverage/lint gates, or test infrastructure/mocks that could mask production) regardless of size. When selected, a **cross-model adversarial pass** (Stage 4) additionally runs the same brief through a different model family via a peer CLI — additive, non-blocking - `previous-comments-reviewer` — PR with existing review comments (PR-only, comment-gated) **Stack-specific conditional (per diff):** `julik-frontend-races-reviewer` (Stimulus/Turbo, DOM events, async UI) and `swift-ios-reviewer` (Swift/SwiftUI/UIKit, entitlements, Core Data, `.pbxproj`). **spec-first conditional (migration-specific):** local prompt asset `deployment-verification-agent` — deployment checklist + rollback when the migration gate applies and the change is risky. ## Review Scope A full review spawns generic subagents for all 4 always-on personas plus the 2 spec-first always-on local prompt assets, then adds whichever cross-cutting and stack-specific conditionals fit the diff (Stage 3c can collapse this to a lite roster for trivial, low-risk diffs). The model naturally right-sizes: a small config change triggers 0 conditionals = 6 reviewers. A Rails auth feature might trigger security + reliability + adversarial = 9 reviewers. ## Protected Artifacts The following paths are spec-first pipeline artifacts and must never be flagged for deletion, removal, or gitignore by any reviewer: - `docs/brainstorms/*` -- legacy requirements documents created by older spec-brainstorm versions - `docs/plans/*.{md,html}` -- unified plan artifacts created by spec-brainstorm or spec-plan (decision artifacts; execution progress is derived from git, not stored in plan bodies) - `docs/solutions/*.md` -- solution documents created during the pipeline If a reviewer flags any file in these directories for cleanup or removal, discard that finding during synthesis. ## Plan Requirements Completeness When a plan is provided via `plan:` or discovered from PR/branch context, classify readiness before checking completeness: - Unified artifact: metadata includes `artifact_contract: spec-unified-plan/v1`. - `artifact_readiness: requirements-only` can inform product intent, but it must not trigger implementation-unit completeness findings. Report that the artifact was not implementation-ready if the diff appears to implement it. - `artifact_readiness: implementation-ready` is eligible for full requirements and U-ID completeness checks. - Invalid progress-like readiness values (`active`, `in_progress`, `completed`, `done`) are contract errors. - Legacy plan: use the existing completeness checks. Extract requirements from these shapes, in order: 1. Unified `Product Contract` -> `### Requirements` 2. Legacy top-level `## Requirements` 3. Legacy `## Requirements Trace` For unified implementation-ready plans, also extract U-IDs from `## Implementation Units` and compare against PR body/branch context when available. Do not require every Product Contract R-ID to map one-to-one to a single U-ID; verify that implemented U-IDs cite the relevant R/F/AE/KTD IDs and that no claimed U-ID is missing from the plan. Task-mode exception: completeness scope is the selected Task Card, its `source_unit`/`requirement_refs`, and the bounded task delta only. Do not flag other plan requirements or U-IDs as unaddressed during an early task review; the final full review owns whole-plan completeness. ## How to Run ### Stage 1: Determine scope Compute the diff range, file list, and diff. Minimize permission prompts by combining into as few commands as possible. #### Task-scoped `mode:agent` intake When `task-pack:`, `task:`, and `task-context:` are present, run this branch before the ordinary `base:` fast path. This is a bounded review of one Task Card inside the current checkout. It remains report-only: task context never enables Stage 5c, checkout mutation, commit, push, PR, or ticket creation. The caller-owned context file must be local, readable, inside the caller's authorized work-run/artifact root, and shaped as: ```json { "schema_version": "spec-code-review-task-context/v1", "task_pack_digest": "sha256:<64-hex>", "source_plan": "docs/plans/...", "source_plan_section_titles": ["### U6. ...", "### Interface Contracts"], "work_run_base": "", "pre_task_dirty_files": ["repo/relative/path"], "pre_task_untracked_files": ["repo/relative/path"], "pre_task_file_facts": [ { "path": "repo/relative/path", "base_content_sha256": "sha256:<64-hex> | absent", "pre_task_content_sha256": "sha256:<64-hex> | absent" } ], "task_delta_files": [ { "path": "repo/relative/path", "change_kind": "modified | added | deleted | renamed", "old_path": "repo/relative/old-path | null", "current_content_sha256": "sha256:<64-hex> | absent" } ], "task_owned_untracked_files": ["repo/relative/new-path"] } ``` These are caller-captured run facts, not workflow state. The caller must capture the pre-task facts before task mutation; the reviewer must not reconstruct or invent them after the fact. Normalize all paths to repo-relative POSIX paths, reject absolute paths, `..` escape, generated runtime mirrors, secret-path matches, duplicate/conflicting entries, and rename endpoints outside the task's declared/allowed surface. Validate before reviewer dispatch: 1. Read the task pack, find exactly one `Task Pack Contract` JSON block, and select exactly one Task Card whose `task_id` equals `task:`. An unknown task_id, unreadable pack, duplicate task ID, or malformed contract returns `status: failed` without dispatch. 2. Hash the current task-pack file bytes as SHA-256 and compare them with `task_pack_digest`. Digest drift returns `status: failed`, `required_gate_eligible: false`, and reason `task-pack-digest-drift`; do not silently review a different pack. 3. Confirm task-pack `source_plan`, optional explicit `plan:`, context `source_plan`, and `work_run_base`/`base:` agree. A mismatch is a failed scope contract, not inferred intent. 4. Read the Task Card's `files`, `expected_side_effects`, `review_gate`, and `review_focus`. The declared files plus narrowly bounded expected side effects are the allowed task surface. `review_focus` guides reviewers but never suppresses correctness findings. 5. Require a pre-task file fact for every task delta path and both rename endpoints. Re-hash every current delta file and compare it with `current_content_sha256` (`absent` for deletion). Missing, drifting, or contradictory attribution returns `status: degraded`, reason `task-scope-unattributed`, and `required_gate_eligible: false`; a required review must not pass on an invented or concurrently changed task diff. ### Task-Scoped Live Plan Context `source_plan_section_titles` is an optional producer label list of exact visible section headings relevant to the selected Task Card. It transports only the current plan path plus section titles: never plan body bytes, hashes, byte offsets, anchors, access-control claims, or a second context schema. When task context provides readable `source_plan` and one or more titles, reviewers re-read that same live file in the current checkout and use only the named sections as plan context. The producer does not attest that the plan is unchanged and the reviewer does not reconstruct a same-session hash check. Ordinary bounded heading lookup is enough; do not introduce an anchor parser. When the path is missing/unreadable, a title is absent, or section labels are absent, retain the attributed task diff review but set `plan_context_mode: diff-only` and record respectively `task-plan-unreadable`, `task-plan-section-unreadable`, or `task-plan-section-hints-missing`. Do not claim plan-aware coverage, do not invent omitted requirements, and do not fail the task solely because the additive plan context is unavailable. Build one review bundle with per-file provenance: - **`exact-file`**: `base_content_sha256` equals `pre_task_content_sha256`, including `absent == absent`. The file had no pre-task divergence from the work-run baseline, so its current base-to-working-tree patch is attributable to this task. - **`cumulative-file`**: the hashes differ. Include the file's full current diff against `base:` and disclose that it also contains pre-existing dirty work or earlier task changes. Do not call it an isolated task diff; reviewers may mark unrelated hunks `pre_existing`. - **Task-owned new file**: the path is in `task_delta_files` and `task_owned_untracked_files`, was `absent` at base and pre-task, exists now, and is declared/allowed. Include a full-addition patch or bounded full content generated without assuming POSIX `/dev/null`; do not apply the standalone rule that excludes all untracked files. - **Pre-existing untracked**: paths listed in `pre_task_untracked_files` stay excluded and disclosed when untouched. If the task delta claims one, attribution is degraded and the required gate is ineligible because no base-to-pre-task boundary exists. - **Scope expansion**: a delta or task-owned new file outside declared `files` and bounded `expected_side_effects` returns `status: failed`, reason `task-scope-expansion`, and names the paths. Do not broaden review scope to legitimize the mutation. - **Unattributed current file**: a changed/untracked path claimed by neither pre-task facts nor task delta returns degraded `task-scope-unattributed`; do not hide it or call the task clean. Set per-file isolation and one aggregate `task_diff_isolation`: `exact-file` when every included file is exact, `cumulative-file` when every included file is cumulative, `mixed` when both are fully attributed, and `degraded` when any required fact or file attribution is missing. `cumulative-file`/`mixed` are honest bounded review scopes with limitations; only `degraded` or failed scope makes `required_gate_eligible: false` before findings are considered. For task mode, `BASE` remains the work-run baseline used for file diffs and reviewer context. It is not a pre-task snapshot. Set `FILES` to the attributed task bundle, `DIFF` to the labeled per-file patches/content, and `UNTRACKED` to two separate lists: excluded pre-existing untracked files and included task-owned untracked files. Pass the Task Card, observed/expected pack digest, source plan, `source_plan_section_titles`, `plan_context_mode`, declared files, delta files, file isolation map, aggregate isolation, review focus, and limitations to every reviewer and validator. Requirements completeness is limited to this Task Card's cited source refs; final branch/plan completeness remains the Phase 3 full review's job. **If `base:` argument is provided (fast path):** The caller already knows the diff base. Skip all base-branch detection, remote resolution, and merge-base computation. Use the provided value directly: ``` BASE_ARG="{base_arg}" BASE=$(git merge-base HEAD "$BASE_ARG" 2>/dev/null) || BASE="$BASE_ARG" ``` Then produce the same output as the other paths: ``` echo "BASE:$BASE" && echo "FILES:" && git diff --name-only $BASE && echo "DIFF:" && git diff -U10 $BASE && echo "UNTRACKED:" && git ls-files --others --exclude-standard ``` This path works with any ref — a SHA, `origin/main`, a branch name. Callers reviewing the current checkout should pass explicit `base:` when auto-detection is unnecessary. **Do not combine `base:` with a PR number or branch target.** If both are present, stop with an error: "Cannot use `base:` with a PR number or branch target — `base:` implies the current checkout is already the correct branch. Pass `base:` alone, or pass the target alone and let scope detection resolve the base." **If a PR number or GitHub URL is provided as an argument:** Do **not** check out the PR branch. Scope comes from GitHub read APIs plus optional local alignment when HEAD already matches the PR head branch. **Skip-condition pre-check.** Before scope detection, run a PR-state probe: ``` gh pr view --json state,title,body,files ``` Apply skip rules in order: - `state` is `CLOSED` or `MERGED` -> stop with reason `PR is closed/merged; not reviewing.` - **Trivial-PR judgment**: perform this conservative judgment in the orchestrator inline from the PR title, body, and changed file paths; this pre-gate check does not dispatch a sub-agent. Ask: "Is this an automated or trivial PR that does not warrant a code review? Consider dependency lock-file or manifest-only bumps, automated release commits, and chore version increments with no substantive code changes." When in doubt, answer no — false negatives (skipped reviews that should run) are more costly than false positives (an unnecessary review). If the judgment returns yes: stop with reason `PR appears to be a trivial automated PR; not reviewing. Run without a PR argument to review the current branch, or pass base: if review is intended.` When any skip rule fires, stop without dispatching reviewers. **Default mode:** emit the reason as plain text. **`mode:agent`:** emit JSON only — `{"status":"skipped","reason":""}` — so programmatic callers can parse the outcome. **Standalone**, **`base:`**, and **branch-remote** paths are unaffected. **Draft PRs are reviewed normally.** If no skip rule fires, fetch PR metadata **without checkout**: ``` gh pr view --json title,body,baseRefName,headRefName,headRefOid,isCrossRepository,url,files,reviews,comments --jq '{title, body, baseRefName, headRefName, headRefOid, isCrossRepository, url, files: [.files[].path], hasPriorComments: ((.reviews | map(select(.state != "APPROVED" or .body != "")) | length) > 0 or (.comments | length) > 0)}' ``` Set `BASE:` to `pr:` (logical marker — not a git SHA). Set `UNTRACKED:` from `git ls-files --others --exclude-standard` on the **current** checkout (usually empty during PR-remote review). **PR scope mode.** Classify as **`local-aligned`** only when **all** of these hold; otherwise use **`pr-remote`**. A matching branch name alone is not enough — a fork PR or a stale local branch can share a name with the PR head while pointing at unrelated code, and trusting the name would diff and inspect the wrong tree. 1. `git rev-parse --abbrev-ref HEAD` equals `headRefName`. 2. The PR is **not** cross-repository (`isCrossRepository` is false). 3. The PR head commit is contained in the local checkout: `git merge-base --is-ancestor HEAD` exits 0. This confirms the working tree actually carries the PR head (allowing unpushed local fixes layered on top) rather than an unrelated same-named branch. - **`local-aligned`** — all three checks pass. Local Read/Grep/git blame against workspace files are valid for PR changed paths. - **`pr-remote`** — any check fails. The working tree is **not** the PR head; workspace file contents for changed paths may be stale or unrelated. **Diff by scope mode** (do not mix remote and local diffs — contradictory hunks cause false positives): - **`local-aligned`:** Resolve `` from `baseRefName` (fetch if needed). Compute `BASE=$(git merge-base HEAD )`, then set `FILES:` from `git diff --name-only $BASE` and `DIFF:` from `git diff -U10 $BASE` (includes committed, staged, and unstaged changes on the PR branch). Do **not** call `gh pr diff` or append remote hunks — when unpushed fixes exist, the local tree is canonical. Note in Coverage: `scope: local-aligned (PR; local tree diff)`. - **`pr-remote`:** Set `FILES:` from the PR `files` array. Set `DIFF:` from `gh pr diff --color=never`. If `gh pr diff` fails, stop with an actionable error — do not fall back to checkout. When **`pr-remote`**, before Stage 4: 1. Best-effort fetch PR head without checkout: `git fetch --no-tags origin :refs/review/pr--head` (substitute PR number from metadata). 2. When fetch succeeds, set `PR_HEAD_REF=refs/review/pr--head` for reviewers and validators. When fetch fails, omit `PR_HEAD_REF` and note in Coverage — reviewers must rely on diff hunks only. 3. Best-effort fetch the PR base without checkout: `git fetch --no-tags origin `. When it succeeds, resolve a concrete ref with `git rev-parse FETCH_HEAD` and set `PR_BASE_REF` to that SHA — a **real git base ref** reviewers and validators use for file-level git diffs (e.g. `data-migration-reviewer` runs `git diff -- db/schema.rb`/`structure.sql`). The `pr:` logical marker in `BASE:` stays the scope marker; `PR_BASE_REF` is the diffable base. When the fetch fails, omit `PR_BASE_REF` and note in Coverage — schema-drift and other git-diff checks fall back to diff hunks only and must **not** assume `main`. 4. Include `pr-remote` and, when set, `...` and `...` in the Stage 4 review context bundle. Reviewers and Stage 5b validators in **`pr-remote`** mode must **not** Read/Grep workspace paths for files in `FILES:`. Inspect via `git show :` when `PR_HEAD_REF` is set, otherwise use only the provided diff hunks. **`local-aligned`** uses normal workspace inspection. **If a branch name is provided as an argument:** Substitute the provided branch name as ``. Do **not** check out ``. If `git rev-parse --abbrev-ref HEAD` equals ``, use the **standalone (current branch)** path below — same tree, explicit branch name; do not use remote-only diff. Otherwise diff the remote/local ref **without checkout**: 1. Try `gh pr view --json baseRefName,url,headRefName` — if a PR exists, prefer the **PR number/URL path** above (same remote diff rules). 2. Else resolve `` as `origin/` or `` after `git fetch --no-tags origin ` when needed. 3. Resolve default base branch (same logic as standalone). Compute `BASE=$(git merge-base )` and `git diff -U10 $BASE `. 4. If `` cannot be resolved locally, stop: "Cannot diff branch `` without checkout. Check out that branch, pass its open PR URL/number, or review the current branch with `base:`." On success for remote branch diff, set **branch-remote scope**. The working tree is **not** ``. Include `branch-remote` and `` in the Stage 4 review context bundle. Reviewers and Stage 5b validators must **not** Read/Grep workspace paths for files in `FILES:`. Inspect via `git show :` or diff hunks only. Produce: ``` echo "BASE:$BASE" && echo "FILES:" && git diff --name-only $BASE && echo "DIFF:" && git diff -U10 $BASE && echo "UNTRACKED:" && git ls-files --others --exclude-standard ``` **If no argument (standalone on current branch):** Apply the same base-detection logic as branch mode above, using the current branch (i.e., `gh pr view --json baseRefName,url` with no argument defaults to the current branch). If no base can be resolved, **stop**. Do not fall back to `git diff HEAD` — a standalone review without the base would only show uncommitted changes and silently miss all committed work on the branch. On success, produce the diff: ``` echo "BASE:$BASE" && echo "FILES:" && git diff --name-only $BASE && echo "DIFF:" && git diff -U10 $BASE && echo "UNTRACKED:" && git ls-files --others --exclude-standard ``` Using `git diff $BASE` (without `..HEAD`) diffs the merge-base against the working tree, which includes committed, staged, and unstaged changes together. **Untracked file handling:** Outside task mode, always inspect `UNTRACKED:`. Untracked paths are out of scope unless staged. When non-empty, list excluded files in Coverage and continue on tracked changes only — never stop or prompt. Task mode uses the caller-captured pre-task/task-owned classification above instead of this blanket exclusion. ### Stage 1a: Freeze the reviewed local scope For `mode:agent` and every report-only review whose reviewed tree is local (`base:`, standalone, `local-aligned`, or task-scoped current checkout), freeze the diff before semantic review. Set `SKILL_DIR` and run: ```bash SCOPE_SNAPSHOT="$REVIEW_ARTIFACT_DIR/scope-snapshot.json" SCOPE_ARGS=(--base "$DIFF_A" --snapshot-out "$SCOPE_SNAPSHOT") [ -n "${DIFF_B:-}" ] && SCOPE_ARGS+=(--head "$DIFF_B") bash "$SKILL_DIR/scripts/run-python.sh" "$SKILL_DIR/scripts/review-scope.py" "${SCOPE_ARGS[@]}" ``` The returned `changed_files`, `files_changed`, and `diff_sha256` are immutable scope facts for the rest of the run. `mode:agent` JSON must use the frozen `files_changed`; never recompute it after tests or inspection. For task mode, the task-attributed bundle remains the semantic review scope, while the whole local base-to-working-tree snapshot is only the mutation detector. Remote-only PR/branch review has no reviewed local tree and records the guard as not applicable. If the helper, snapshot write, or artifact root is unavailable, keep `source_mutation_gate: closed`, record `mutation_guard_unavailable`, and do not emit a successful/complete machine handoff. Do not replace the deterministic snapshot with remembered prose or a later `git diff`. ### Stage 1b: Compute scope signals (cheap, deterministic) Derive deterministic signals from the resolved diff once, so reviewer selection (Stage 3) and the small-diff fast path (Stage 3c) do not each re-reason over the whole diff. **These signals only ever shrink the roster via Stage 3c, and that gate fails closed (Stage 3c) — so any failure here (unresolved base, count failure, an uncounted file type) must surface as `UNKNOWN`/non-zero `UNCOUNTED_FILES`, never as a silent `0` that reads as "trivial."** **Set `DIFF_A`/`DIFF_B` to the two endpoints to diff, by Stage 1 scope mode:** - **`local-aligned` / standalone / `base:`** — `DIFF_A="$BASE"` (a real SHA/ref), `DIFF_B` empty (diffs base vs working tree). - **`pr-remote` / `branch-remote`** — `DIFF_A=`, `DIFF_B=` (or ``) — the **fetched** refs from Stage 1. Do **not** model-count from hunks (it drifts per host/model). If either ref was not fetched, skip the block and emit `EXEC_LINES:UNKNOWN` + `UNCOUNTED_FILES:1` so Stage 3c forces the full roster. - **Task-scoped `mode:agent`** — compute signals from the attributed task `FILES`/`DIFF` bundle only, including task-owned full-addition files. Never count the entire base-to-working-tree diff. If any task file is degraded/unattributed or cannot be counted consistently, emit `EXEC_LINES:UNKNOWN` + `UNCOUNTED_FILES:1` so the full roster runs and the coverage limitation remains visible. For every non-task scope, set `SKILL_DIR` to this Skill's runtime directory and run the facts helper through its portable Python wrapper: ```bash SCOPE_ARGS=(--base "$DIFF_A") [ -n "${DIFF_B:-}" ] && SCOPE_ARGS+=(--head "$DIFF_B") bash "$SKILL_DIR/scripts/run-python.sh" "$SKILL_DIR/scripts/review-scope.py" "${SCOPE_ARGS[@]}" ``` Consume only its JSON facts (`status`, endpoints, `exec_lines`, `uncounted_files`, `changed_files`, path `signals`, test/agent-surface flags, fixed `docs/solutions` corpus presence, and `lite_eligible`). The helper does not select personas or decide semantic risk. A non-`complete` status, malformed output, invocation failure, or changed-file mismatch becomes `EXEC_LINES:UNKNOWN` plus `UNCOUNTED_FILES:1`; never reconstruct a favorable zero. Task-scoped mode continues to compute from its attributed bundle and uses the same fail-closed fields because the helper intentionally has no authority to reinterpret caller-owned task attribution. `EXEC_LINES` counts changed executable lines (added + removed, counted code extensions only — so a modified line counts as 2; the Stage 3c `<40` threshold is in add+delete units). `EXEC_LINES:UNKNOWN` means the base was unresolved — treat as non-trivial. `UNCOUNTED_FILES` is the count of changed files outside the code set (skill `.md`, JSON schemas, `.sh`, config, CI, lockfiles, unknown extensions) — **spec-first's own product surface is mostly uncounted, which is exactly why Stage 3c must fail closed on it.** The `SIGNALS` list is **path heuristics, not selection decisions**: Stage 3 still applies judgment and adds the matching conditional persona only when the runtime concern is real. Content-based risk (auth, payments, data mutation) is **not** path-derivable — read it from the diff in Stage 3 as before; it also disqualifies the Stage 3c fast path regardless of line count. ### Stage 1c: Dispatch gate and inline fallback After scope/diff/task-context resolution, enforce the Phase 0 dispatch policy before profile derivation, persona loading, team announcements, validators, or cross-model work. - If `worker_dispatch_authorization: missing`, select the bounded inline report-only path, set `status: degraded` and `coverage.dispatch_reason_code: dispatch_authorization_missing`. - If authorization is present but confirmed current-session capability is missing, use the same path with `subagent_capability_missing`; if the probe is unavailable or cannot establish a unique eligible candidate, use `worker_capability_unproven`. - On this path, continue only through Stage 2 intent discovery and Stage 2b plan/task completeness context when applicable, then perform the inline pass, run the mandatory Stage 5e mutation check, and go to Stage 6. Skip Stage 2c, Stages 3/3b/3c, persona/validator/cross-model dispatch, and Stage 5/5b/5c. No persona prompt may be represented as independently executed. - Resolve the Stage 4 Run ID/artifact-directory setup before synthesis even though no reviewer is dispatched. If the directory is unavailable, keep the complete result in band with `artifact_path: null` and `artifact_write_status: unavailable`. Inline fallback output contract: 1. Inspect the entire resolved diff or attributed task bundle once with direct correctness, testing, project-standards, scope, and plan/task-completeness checks. These are orchestrator checks, not executed personas. 2. Normalize every surviving issue directly into the final finding fields, assign stable `#` values in Stage 5 ordering, set `reviewers: ["inline-fallback"]`, and derive `actionable_findings` from the normal routing fields. Do not emit a preliminary fast-pass block or claim validation/cross-reviewer agreement. 3. Set `reviewers: ["inline-fallback"]`, `coverage.dispatch_reason_code` to the concrete fallback reason, and record independent/validator/cross-model coverage as not run. When no targeted command ran, use the Stage 5d `no-targeted-command-executed` evidence shape. 4. Return `status: degraded` and `verdict: Not ready` even when no issue is found; single-model bounded coverage cannot close merge readiness. In task mode also set `required_gate_eligible: false`. The explicit Quick Review Short-Circuit is separate: when the user asked for a quick/light review and the harness has a built-in report-only reviewer, use that path as requested rather than labelling it a failed multi-agent run. ### Stage 2: Intent discovery Understand what the change is trying to accomplish. The source of intent depends on which Stage 1 path was taken: **PR/URL mode:** Use the PR title, body, and linked issues from `gh pr view` metadata. Supplement with commit messages from the PR if the body is sparse. **Branch mode:** Run `git log --oneline ${BASE}..` using the resolved merge-base and resolved branch ref from Stage 1. Use `` (the resolved `origin/` or fetched ref), not the raw `` argument — a remote-only branch has no matching local ref, so the raw name would fail or read a stale same-named local branch. **Standalone (current branch):** Run: ``` echo "BRANCH:" && git rev-parse --abbrev-ref HEAD && echo "COMMITS:" && git log --oneline ${BASE}..HEAD ``` Combined with conversation context (plan section summary, PR description), write a 2-3 line intent summary: ``` Intent: Simplify tax calculation by replacing the multi-tier rate lookup with a flat-rate computation. Must not regress edge cases in tax-exempt handling. ``` Pass this to every reviewer in their spawn prompt. Intent shapes *how hard each reviewer looks*, not which reviewers are selected. **When intent is ambiguous:** Infer from branch name, commits, PR title/body, diff, `plan:`, and conversation. Write the best-effort intent summary and note uncertainty in Coverage — never block on a clarifying question. ### Stage 2b: Plan discovery (requirements verification) Locate the plan document so Stage 6 can verify requirements completeness. Check these sources in priority order — stop at the first hit: 1. **`plan:` argument.** If the caller passed a plan path, use it directly. Read the file to confirm it exists. 2. **PR body.** If PR metadata was fetched in Stage 1, scan the body for paths matching `docs/plans/*.{md,html}` (unified plans may be markdown or HTML). If exactly one match is found and the file exists, use it as `plan_source: explicit`. If multiple plan paths appear, treat as ambiguous — demote to `plan_source: inferred` for the most recent match that exists on disk, or skip if none exist or none clearly relate to the PR title/intent. Always verify the selected file exists before using it — stale or copied plan links in PR descriptions are common. 3. **Auto-discover.** Extract 2-3 keywords from the branch name (e.g., `feat/onboarding-skill` -> `onboarding`, `skill`). Glob `docs/plans/*` and filter filenames containing those keywords. If exactly one match, use it. If multiple matches or the match looks ambiguous (e.g., generic keywords like `review`, `fix`, `update` that could hit many plans), **skip auto-discovery** — a wrong plan is worse than no plan. If zero matches, skip. **Confidence tagging:** Record how the plan was found: - `plan:` argument -> `plan_source: explicit` (high confidence) - Single unambiguous PR body match -> `plan_source: explicit` (high confidence) - Multiple/ambiguous PR body matches -> `plan_source: inferred` (lower confidence) - Auto-discover with single unambiguous match -> `plan_source: inferred` (lower confidence) If a plan is found, classify readiness before extraction (see "Plan Requirements Completeness" above): for a unified plan read the metadata/header first, and treat a requirements-only artifact as product intent only — it must not drive implementation-unit completeness findings. Then read its **Requirements** in this order — unified `Product Contract` -> `### Requirements`, then legacy top-level `## Requirements`, then legacy `## Requirements Trace` — and the R-IDs (R1, R2, etc.) listed there, plus **Implementation Units** (current numeric subsections such as `### U1.`, `### U2.`, or `### Unit 1:` under `## Implementation Units`; legacy bullet or checkbox unit entries under that section also count). For HTML unified plans the same section names and R-/U-IDs appear as visible headings/anchors — match on the section name, ignoring HTML wrapper tags. Store the extracted requirements list and `plan_source` for Stage 6. In task mode, retain only the selected Task Card's cited source refs for this review and set `completeness_scope: selected-task`; when `source_plan_section_titles` are present, use the Task-Scoped Live Plan Context branch above rather than injecting a plan body or asserting freshness. Do not compare the bounded task delta with unrelated U-IDs. Do not block the review if no plan is found — requirements verification is additive, not required. ### Stage 2c: Resolve current-tree orientation Derive one run-local stack/conventions orientation from the tree actually under review so reviewer selection and the non-standards reviewers (`correctness`, `testing`, `maintainability`) share the same current facts. For `local-aligned`, standalone, and `base:` scope, record the current git identity and dirty state, then read current manifests, layout, and active instructions. For `pr-remote` or `branch-remote`, derive only from the fetched reviewed refs/diff; never substitute the local checkout's profile. Do not persist or reuse this orientation across runs, branches, or worktrees. Include a compact orientation slice plus direct source refs in the Stage 4 review context bundle passed to every reviewer **except** `project-standards` and `learnings-researcher`. This is orientation only and never replaces a fresh read. The `project-standards-reviewer` still reads the actual root and subdirectory `AGENTS.md`/`CLAUDE.md` standards files through the Stage 3b path list, and `learnings-researcher` re-globs `docs/solutions/` per run. If the reviewed tree or a required source cannot be read, record the exact degraded fact and narrow reviewer claims; do not silently fall back to a profile from another source identity. ### Stage 3: Select reviewers Read the diff and file list from Stage 1, and the `SIGNALS` / `EXEC_LINES` from Stage 1b. The 4 always-on personas and 2 spec-first always-on local prompt assets are automatic. Read `references/persona-catalog.md` from this skill's directory now — it carries the full per-persona selection criteria and spawn gates the one-line roster above only summarizes. For each cross-cutting and stack-specific conditional persona in that catalog, decide whether the diff warrants it. This is agent judgment, not keyword matching — a `SIGNALS` hit (`migrations`, `frontend`, `api`, `swift-ios`) is a *prompt* to consider the matching persona, not an instruction to spawn it; confirm the runtime concern is real in the diff before adding it, and add content-gated personas (`security`, `reliability`, `adversarial`) from the diff as before since those are not path-derivable. **Security selection boundary.** Select `security` when the diff introduces or changes an agent/model/tool/web-content trust boundary, tenant/resource authorization, credentials, or a reachable dangerous sink such as shell, path, SQL, server-side URL, template evaluation, or privileged tool action. A schema-only compatibility drift belongs to `api-contract`; an unreachable dependency advisory or generic hardening idea is not a security finding. Require a concrete attack path before escalating a security concern, and do not use file extension or dependency-name presence as a substitute for that judgment. **Frontend-quality selection boundary.** Select the internal `frontend-quality` persona when the diff changes a user-visible route, form, navigation, component public behavior, async loading/error/empty/permission/retry state, semantic HTML/ARIA, keyboard/focus, contrast, layout, responsive breakpoint, or motion behavior. This is a semantic activation judgment, not an extension test: CSS-only changes that affect contrast/focus/layout/responsive/motion activate it; backend-only, docs-only, type-only, fixture-only, and token-value-only changes that do not affect those visible semantics do not. It reviews current diff quality, not browser field behavior; timing/race findings remain `julik-frontend-races`, unsafe rendering remains `security`, test proof remains `testing`, and structural complexity remains `maintainability`. **File-type awareness for conditional selection:** Instruction-prose files (Markdown skill definitions, JSON schemas, config files) are product code but do not benefit from runtime-focused reviewers. The adversarial reviewer's techniques (race conditions, cascade failures, abuse cases) target executable code behavior. For diffs that only change instruction-prose files, skip adversarial unless the prose describes auth, payment, or data-mutation behavior, or the change is itself a silent-pass verification mechanism (next paragraph — a CI/CD workflow is a config file but still gets the adversarial lens). Count only executable code lines toward line-count thresholds. **Silent-pass verification mechanisms — adversarial fires on the guard itself.** When the change *is* a verification mechanism — CI/CD gating logic, merge-blocking checks, build/deploy steps, coverage/lint gates, or test infrastructure/mocks that could mask production — its risk isn't blast radius, it's fidelity: it can go green while the real thing is red, so the exact "can this false-pass?" lens must run. Select `adversarial` (and therefore the Stage-4 cross-model pass) for such a change regardless of changed-line count and independent of the auth/data heuristics. The selection question: "If this mechanism is wrong, does it fail loudly or silently pass? A silent-pass guard gets the adversarial + cross-model lens regardless of size." Scope guard: this fires on the *mechanism* (gating/CI/build/deploy/harness changes), not on ordinary per-feature test assertions — a unit test asserting business logic is the `testing` reviewer's job, not adversarial's. **`previous-comments` is PR-only AND comment-gated.** Only select this persona when both conditions hold: 1. Stage 1 gathered PR metadata (PR number or URL was provided as an argument, or `gh pr view` returned metadata for the current branch). 2. `hasPriorComments` from Stage 1 is true (the PR has at least one review submission or issue comment). Skip it for standalone branch reviews with no associated PR, and skip it for PRs with no prior feedback yet -- there is nothing for the persona to verify, and a spawned subagent that returns empty findings still costs the full subagent startup overhead (persona spec, diff, schema, plus its own gh calls). Stack-specific personas are additive when runtime behavior warrants them. A Hotwire UI change may warrant `julik-frontend-races`; a TypeScript API diff may warrant `api-contract` and `reliability`. **`data-migration` spawn gate.** Select `data-migration-reviewer` only when the diff includes at least one migration or schema artifact: `db/migrate/*`, `db/schema.rb`, `db/structure.sql`, Alembic/Flyway/Liquibase migration paths, or explicit backfill/data-transform scripts (rake tasks, one-off data migration classes). **Do not spawn** for model-only changes, query-only refactors, serializers/controllers that reference columns without a migration or schema dump in the diff, or migration tests alone. For `deployment-verification-agent`, use the same migration-artifact gate when the change is risky (destructive DDL, backfills, NOT NULL without default, column renames/drops)。只有 orchestrator 能应用该 gate；worker prompt 的适用范围说明不构成 self-invocation，也不能把普通 data-processing change 扩张成 activation signal。Only when both conditions pass, add `deployment-verification-agent` to `selected_local_prompt_assets`; otherwise do not add it. Record an applicability reason that names both the migration/schema artifact path and the concrete risky operation. Announce the team before spawning, as a user-facing summary: name the always-on reviewers plainly, and for each conditional reviewer give the one-line reason it was added (the real concern, not the keyword that matched). Do **not** put model-tier labels (`[session model]`/`[mid-tier]`) or scope-mode codenames in this announce — those are internal. Still *decide* each reviewer's tier here and keep it in your own working notes (Stage 4 applies it at dispatch as a correctness guarantee); just keep it out of this user-facing summary. If the cross-model adversarial pass will run (adversarial selected + `local-aligned`/standalone scope), resolve its peer now via the cross-model reference's host/preflight steps and surface it **as its own prominent line that names the peer** (the peer CLI, plus its model if cheaply known) — the headline is that a second, genuinely independent model is also reviewing. Keep it with the team, not buried after it, and honor that reference's host gating (interactive hosts only). If it won't run, omit it. Illustrative shape only — match your own voice, do not copy verbatim: ``` Reviewing PR #1234 (against main) Also running an independent adversarial pass via a different model: Codex. Reviewers: - correctness, testing, maintainability, project-standards, agent-native, learnings (always) - security — new endpoint accepts a user-provided redirect URL - data-migration — adds migration 20260303_add_index_to_orders ``` This is progress reporting, not a blocking confirmation. ### Stage 3b: Discover project standards paths Before spawning sub-agents, find the file paths (not contents) of all relevant standards files for the `project-standards` persona. Use the native file-search/glob tool to locate: 1. Use the native file-search tool (e.g., Glob in Claude Code) to find all `**/CLAUDE.md` and `**/AGENTS.md` in the repo. 2. Filter to those whose directory is an ancestor of at least one changed file. A standards file governs all files below it (e.g., `AGENTS.md` at the repo root applies to the whole checkout, while `skills/AGENTS.md` would apply to everything under `skills/`). Pass the resulting path list to the `project-standards` persona inside a `` block in its review context (see Stage 4). The persona reads the files itself, targeting only the sections relevant to the changed file types. This keeps the orchestrator's work cheap (path discovery only) and avoids bloating the subagent prompt with content the reviewer may not fully need. ### Stage 3c: Small-diff fast path (reduce the roster for trivial, low-risk diffs) **`depth:full` hard-disables this gate** — when that token was passed, skip Stage 3c entirely and run the full roster (the caller explicitly asked for a deep review; size no longer matters). **This gate fails closed: it only ever fires for a positive count of low-risk application code, and every uncertainty resolves to the full roster.** Collapse to a lite roster only when **all** of these hold: - `EXEC_LINES` from Stage 1b is a **number** (not `UNKNOWN`) and **between 1 and 39**, AND - `UNCOUNTED_FILES` from Stage 1b is **0** — i.e. *every* changed file is counted application code. Any uncounted file (skill `.md`, JSON schema, `.sh`/CI/config, lockfile, unknown extension) disqualifies the lite path. This guard is load-bearing on **mixed** diffs the line count alone would pass: 15 exec lines of application code plus two `.md` skill files reads `EXEC_LINES:15` with `UNCOUNTED_FILES:2`, and the uncounted files force the **full** roster. (This plugin's own product surface — `skills/**`, `SKILL.md`, `references/**`, schemas — is uncounted.) AND - Stage 1b `SIGNALS` is **empty** (no `migrations`, `frontend`, `api`, or `swift-ios`), AND - No content-based risk read from the diff in Stage 3 (auth, payments, data mutation, external API, secrets/permissions, deserialization, crypto, concurrency/background jobs, filesystem/process execution), AND - No conditional persona was selected in Stage 3. `EXEC_LINES:UNKNOWN` (unresolved base) or `UNCOUNTED_FILES > 0` are **hard disqualifiers** — never lite. A pure code diff that also touches one `.md` runs the full roster; that conservatism is the point. **Lite roster:** the inline fast pass (Stage 4) plus `correctness-reviewer` and `project-standards-reviewer` only — skip `testing`, `maintainability`, `agent-native`, and `learnings`. Announce the reduction plainly (e.g. "Small diff (28 exec lines, code-only, no risk signals) — running a lite review: fast pass + correctness + standards.") and note it in Coverage. **Do not collapse** when any gate condition fails — the gate keys on risk, not size alone (a 12-line auth change still needs the full roster). When in doubt (signals ambiguous, risk unclear, count `UNKNOWN`), run the full roster. ### Stage 4: Spawn sub-agents #### Inline fast pass (emit before dispatch) To surface findings in seconds, **in the same turn that fires the parallel dispatch** the orchestrator does a quick first-principles scan of the diff it already holds — emit the fast-pass block as text, then make the Agent calls, so it adds no wall-clock to the fan-out. Scan only for **high-signal, obvious** issues a careful first read catches: data/SQL safety, injection (shell/SQL/LLM-output trust boundary), broken control flow, a missing `await`/unhandled promise, a swapped argument or off-by-one, an enum/status added without updating its sibling switch, a null deref on a value the diff makes reachable. Do **not** do deep analysis, read beyond the diff (except a quick Grep for enum completeness), or chase subtle concerns. Quote the verbatim motivating line for each, same bar as a persona finding. Present these under a clearly preliminary header (e.g. `### Fast pass (preliminary — deep review in progress)`) as a short list of `severity — file:line — what`, with one line stating they are unverified and will be deduplicated into the final report. Do **not** assign stable `#` numbers here. The fast pass enters Stage 5 as a pseudo-reviewer named `fast-pass`, with two hard constraints because it is the orchestrator's own read, **not** an independent reviewer (it shares the session model and its blind spots with the orchestrator and the session-model personas): - **Cap every `fast-pass` finding at anchor 50.** At anchor 50 it surfaces on its own only when P0 (P0+50 survives the gate); otherwise it reaches the actionable tier only by deduping onto an independent persona finding that carries its own ≥75 anchor. - **`fast-pass` never counts toward cross-reviewer promotion** (Stage 5 step 3). A `fast-pass`+persona fingerprint match is noted in the Reviewer column but does **not** bump the anchor — only independent reviewers corroborate. Do not feed `fast-pass` candidates into the persona or validator prompts — those agents review the raw diff independently, and seeding them would manufacture the false agreement this cap exists to prevent. If the fast pass finds nothing obvious, emit one line saying so and proceed; never block dispatch on it. When Stage 3c selected the lite roster, the fast pass still runs. **Reconcile the preliminary block in the final report.** A preliminary fast-pass item that did not survive (deduped away, demoted at the gate, or dropped by validation) must be accounted for, not left dangling — add a one-line "Preliminary fast-pass items withdrawn: ()" note so a user who saw a scary preliminary finding learns it was cleared. Mark any final finding that survived from `fast-pass` alone (no persona corroboration) so its weaker provenance is visible. **`mode:agent`:** do **not** emit the preliminary block — that mode's response must be a single raw JSON object with nothing before it. Still run the scan internally and seed its findings into Stage 5 dedup as `fast-pass`. #### Model tiering Three reviewers inherit the session model with no override: `correctness-reviewer`, `security-reviewer`, and `adversarial-reviewer`. These perform the highest-stakes analysis — logic bugs, security vulnerabilities, adversarial failure scenarios — and should run at whatever capability level the user has configured. If the user is on Opus, these get Opus. All other persona workers and spec-first local prompt assets request the balanced mid-tier only when `worker_model_override: supported`; otherwise they inherit and disclose the model-selection limitation. See the Spawning subsection below for the dispatch-time rule. The orchestrator (this skill) also inherits the session model; it handles intent discovery, reviewer selection, finding merge/dedup, and synthesis. #### Run ID Reuse the unique run identifier and concrete artifact directory established by Phase 0a. This identity scopes all agent artifact files and the post-review run artifact to one concrete directory; never create a second run directory during dispatch. Resolve `REVIEW_ARTIFACT_DIR` exactly once with an OS-native temp primitive, then reuse that exact absolute path everywhere. Prefer the runtime API (`os.tmpdir()` in Node.js; an equivalent host temp API otherwise), which already respects platform conventions such as `$TMPDIR` on POSIX and `%TEMP%` on Windows. Create a run-scoped child such as `spec-first/spec-code-review/`, canonicalize it, verify it is writable, and never reconstruct it later from a hard-coded root or `run_id` alone. A host-provided writable run-local temp directory is an acceptable fallback. If no writable artifact directory can be created, continue report-only with the complete in-band reviewer/merge JSON when available, set `artifact_path: null`, `artifact_write_status: unavailable`, and record limitation `review-artifact-write-unavailable`. Do not re-run reviewers merely to recover an artifact. A task review with complete in-band coverage may still report findings, but any durable handoff must first materialize a sanitized repo-local copy or carry a structured summary with the limitation. Pass `{run_id}` and the concrete `{review_artifact_dir}` to every persona and validator. When artifact writing is available, each persona writes its full analysis to `{review_artifact_dir}/{reviewer_name}.json`. **Large shared context — pass paths, not contents.** The diff and file list go to every reviewer and validator. When inlining them into each subagent prompt would be wasteful (many files / a big diff), write them once into the run dir (e.g. `full.diff`, `files.txt`) and pass those **paths** in the diff / changed-files slots instead of inline content — the subagent and validator templates instruct the child to Read a staged path. Inline a small diff directly. #### Spawning Omit the `mode` parameter when dispatching sub-agents so the user's configured permission settings apply. Do not pass `mode: "auto"`. Permission settings govern whether a call may execute; they are not dispatch authorization. **Dispatch authorization is separate from task/review scope and mutation policy.** A `mode:agent` invocation, task context, branch, plan, `mutation_policy: apply-fixes`, or visible tool does not authorize reviewer dispatch. Require `worker_dispatch_authorization: authorized` from explicit user/upstream wording, then inspect the current-session schema through the semantic eligibility contract. - Missing authorization: do not spawn reviewers, validators, or cross-model processes. Run a bounded inline report-only pass over the resolved diff/task bundle, return `status: degraded`, set `coverage.dispatch_reason_code: dispatch_authorization_missing`, and list only `inline-fallback` in executed coverage. - Authorization present but capability missing: use the same inline report-only `status: degraded` fallback with `coverage.dispatch_reason_code: subagent_capability_missing`. Use `worker_capability_unproven` instead when the discovery surface is unavailable, incomplete, or ambiguous. - The fallback never enters Stage 5c even if apply was requested. Do not claim persona, independent, parallel, validator, or cross-model coverage. In task mode set `coverage.task_scope.required_gate_eligible: false`; outside task mode do not emit `Ready to merge` from degraded single-model coverage. This preserves review signal without fabricating a roster or closing a gate that required independent coverage. **Model override at dispatch time — this is a correctness guarantee, not cosmetics.** Omitting the override on a top-tier parent session (e.g. Opus) silently runs that reviewer at the expensive tier — the regression this prevents. The tier is a deterministic function of the persona, so as you select reviewers in Stage 3, **record each reviewer's tier in an internal working list** — that list is your external memory (the role the old printed `[session model]`/`[mid-tier]` labels served) and it must exist and be honored even though it is no longer rendered in the user-facing announce: - **Session model** (no override; inherits the session model) — `correctness-reviewer`, `security-reviewer`, and `adversarial-reviewer` only. - **Mid-tier** — every other persona and spec-first local prompt asset: request the balanced mid-tier only when `worker_model_override: supported` and the current schema exposes an explicit selector. Otherwise omit the override, inherit the parent model, and record `model_override_unsupported` or `model_override_unknown`; task wording alone does not select a different model. Apply this on **every** dispatched reviewer packet. A missed supported override is a silent cost-and-quality regression, so treat the internal tier list as load-bearing — moving it out of the user-facing output removed the *display*, not the discipline. **Bounded parallel dispatch.** Respect the current harness's active-subagent limit without hard-coding a number. Keep the selected reviewers in a deterministic queue, dispatch up to the known/accepted capacity, and fill freed slots as reviewers complete. If the harness exposes no limit, start optimistically and learn it from the first capacity response. Treat active-agent/thread/concurrency-limit spawn errors as backpressure, not reviewer failure: leave that reviewer queued, wait for any active reviewer to finish, then retry. If no reviewer from this run has been accepted yet, do not wait on an empty active set: retry after a short bounded delay, and after repeated zero-capacity responses proceed with a user-visible degraded/no-subagent review path instead of waiting forever. Do not shrink the roster, ask the user, or record a reviewer as failed for capacity backpressure. Record a reviewer as failed only after a successful dispatch times out/fails, or when dispatch fails for a non-capacity reason. Before assembling any spawn prompt, read these three files from this skill's directory now — they define the dispatch shape and the JSON contract every subagent needs, and you cannot construct a valid spawn without them: `references/subagent-template.md`, `references/diff-scope.md`, and `references/findings-schema.json`. For each selected reviewer, read the corresponding local prompt asset from `references/personas/.md` and spawn a generic subagent using the subagent template. Do not use `subagent_type`, typed `Agent` names, or platform-level spec-first local prompt asset registration. Each persona subagent receives: 1. Their persona file content (identity, failure modes, calibration, suppress conditions) 2. Shared diff-scope rules from `references/diff-scope.md` 3. The JSON output contract from `references/findings-schema.json` 4. PR metadata: title, body, and URL when reviewing a PR (empty string otherwise). Passed in a `` block so reviewers can verify code against stated intent 5. Review context: intent summary, file list, diff, scope mode (`local-aligned` | `pr-remote` | `branch-remote`), and remote head ref (`PR_HEAD_REF` or ``) when set 6. Run ID and reviewer name for the artifact file path 7. **For `project-standards` only:** the standards file path list from Stage 3b, wrapped in a `` block appended to the review context 8. **For `data-migration` only:** the resolved review base ref from Stage 1 (`BASE:` marker), wrapped in `` inside the review context so schema drift checks never assume `main` 9. **For task mode:** the selected Task Card, `review_focus`, expected/observed task-pack digest, source plan, `source_plan_section_titles`, `plan_context_mode`, work-run base, declared and delta files, included task-owned/excluded pre-existing untracked files, per-file isolation, aggregate `task_diff_isolation`, `required_gate_eligible`, and limitations. With `plan_context_mode: live-plan`, reviewers re-read only the named current sections; with `diff-only`, they do not claim plan-aware coverage. Reviewers inspect only this bundle and must not reinterpret a cumulative file as an isolated task patch. Persona sub-agents are **read-only** with respect to the project: they review and return structured JSON. They do not edit project files or propose refactors. The one permitted write is saving their full analysis under the exact caller-provided `REVIEW_ARTIFACT_DIR` when it is available. Read-only here means **non-mutating**, not "no shell access." Reviewer sub-agents may use non-mutating inspection commands when needed to gather evidence or verify scope, including read-oriented `git` / `gh` usage such as `git diff`, `git show`, `git blame`, `git log`, and `gh pr view`. In **`pr-remote`** or **`branch-remote`** scope (see Stage 1), inspect changed files via `git show :` or diff hunks — do not Read/Grep workspace paths for files in scope. They must not edit project files, change branches, commit, push, create PRs, or otherwise mutate the checkout or repository state. Each persona sub-agent writes full JSON (all schema fields) to `{review_artifact_dir}/{reviewer_name}.json` when artifact writing is available and returns compact JSON with merge-tier fields only: ```json { "reviewer": "security", "findings": [ { "title": "User-supplied ID in account lookup without ownership check", "severity": "P0", "file": "orders_controller.rb", "line": 42, "confidence": 100, "autofix_class": "gated_auto", "owner": "downstream-resolver", "requires_verification": true, "pre_existing": false, "suggested_fix": "Add current_user.owns?(account) guard before lookup", "first_evidence": "orders_controller.rb:42 -- account = Account.find(params[:account_id])" } ], "residual_risks": [...], "testing_gaps": [...] } ``` `first_evidence` is the **one** detail-tier field promoted into the compact return: the verbatim motivating line with `file:line` that the quote-the-line gate requires. It is **mandatory for every finding at anchor 75 or 100** (the gate is unenforceable without it in-band, since the rest of `evidence` lives only in the artifact). Omit it only for anchor-50 findings. Stage 5 drops/demotes any 75/100 finding missing it; Stage 5b uses it for the validator-skip check. Keep it to the single triggering line, not the full `evidence` array — the array stays in the artifact. The artifact file **must** carry the full detail-tier fields (`why_it_matters`, `evidence`); the compact *return* omits all detail-tier fields **except `first_evidence`**, but writing the compact shape to the artifact (a common reviewer slip) silently strips the detail Coverage and the keyed detail lines depend on. However review context is delivered — inlined, or staged to disk for a large diff — each reviewer still receives the full subagent-template output contract; staging context never licenses a thinner one. `suggested_fix` is optional in both tiers -- included in compact returns when present so callers can apply fixes after review. If the file write fails, the compact return still provides everything the merge needs. **spec-first always-on local prompt assets** (`agent-native-reviewer`, `learnings-researcher`) are dispatched as generic subagents through the same bounded parallel scheduler as the structured personas. Read their prompt files from `references/personas/`, then give them the same review context bundle the personas receive: entry mode, any PR metadata gathered in Stage 1, intent summary, review base branch name when known, `BASE:` marker, file list, diff, and `UNTRACKED:` scope notes. Do not invoke them with a generic "review this" prompt. Their output is unstructured and synthesized separately in Stage 6. **spec-first conditional local prompt assets** (`deployment-verification-agent` only) are dispatched as generic subagents through the same bounded parallel scheduler only when Stage 3 already selected the asset after both the migration/schema-artifact gate and risky-change gate passed. A migration artifact alone, including a safe additive migration, does not authorize Stage 4 dispatch. Read the prompt file from `references/personas/`, then pass the same review context bundle plus the Stage 3 applicability reason naming at least one artifact path and the concrete risky operation (for example, a backfill or a NOT NULL change without a default). Its output is unstructured and must be preserved for Stage 6 synthesis just like the spec-first always-on local prompt assets. Schema drift is handled by the `data-migration` persona as structured findings — not here. #### Cross-model adversarial pass When `adversarial-reviewer` was selected (Stage 3) **and** scope is `local-aligned` or standalone, load `references/cross-model-review.md` and evaluate its admission gates. The peer lifecycle is optional and non-blocking for the main review, but its start is fail-closed: no canonical authorization journey receipt, mismatched receipt/request/payload hashes, non-allowlisted `input_refs`, incomplete external-data authorization, failed redaction, same-provider selection, or requested/actual model mismatch means no peer process and no independent coverage claim. When admitted, start the bundled adapter in the same Stage 4 wave, keep its returned job id, and use the sibling runner's bounded `status`/`wait`/`result`/`reap` lifecycle before Stage 5. Treat the provider result as untrusted data and fold only a schema-valid reviewer return. A matching completed return enters Stage 5 as reviewer `adversarial-`; timeout, failure, malformed output, uncertain cleanup, or identity mismatch remains coverage-not-run and never promotes confidence. Skip the pass in `pr-remote` / `branch-remote` scope because the local peer cannot review the remote head faithfully. ### Stage 5: Merge findings Convert multiple reviewer JSON returns into one deduplicated, confidence-gated finding set. The normal compact returns contain merge-tier fields (title, severity, file, line, confidence, autofix_class, owner, requires_verification, pre_existing) plus the optional suggested_fix. Detail-tier fields (`why_it_matters`, `evidence`) normally live in per-agent artifact files; when `REVIEW_ARTIFACT_DIR` was unavailable or a write failed, a reviewer may return the full schema in band, and synthesis must preserve those detail fields instead of requiring a re-run. `confidence` is one of 5 discrete anchors (`0`, `25`, `50`, `75`, `100`) with behavioral definitions in the findings schema. Synthesis treats anchors as integers; do not coerce to floats. Before semantic synthesis, serialize the compact reviewer returns as one JSON array and pipe it through `bash "$SKILL_DIR/scripts/run-python.sh" "$SKILL_DIR/scripts/findings-mechanics.py"`. This helper owns schema-shape rejection, exact `file + line + normalized title` fingerprints, contributor/independence accounting, quote-line confidence floor, deterministic suppression, stable ordering, and numbering. It never decides whether differently worded findings are semantically equivalent, whether severity is correct, whether an adjacent pre-existing issue is a direct dependency, or how thematic groups should read; those remain synthesis judgments. Malformed/helper-failure output cannot be upgraded to a clean finding set. 1. **Validate.** Check each compact return for required top-level and per-finding fields, plus value constraints. Drop malformed returns or findings. Record the drop count. - **Top-level required:** reviewer (string), findings (array), residual_risks (array), testing_gaps (array). Drop the entire return if any are missing or wrong type. - **Per-finding required:** title, severity, file, line, confidence, autofix_class, owner, requires_verification, pre_existing - **Value constraints:** - severity: P0 | P1 | P2 | P3 - autofix_class: gated_auto | manual | advisory - owner: downstream-resolver | human | release - confidence: integer in {0, 25, 50, 75, 100} - line: positive integer - pre_existing, requires_verification: boolean - **Quote-the-line gate (enforced here).** Any finding at anchor **75 or 100** must carry a non-empty `first_evidence` (the verbatim motivating line with `file:line`). A 75/100 finding missing `first_evidence` is **demoted to anchor 50** (record the demotion count for Coverage) — it then faces the normal anchor-50 fate in the confidence gate (dropped unless P0 or routed to a soft bucket). - Do not validate against the full schema here -- the full schema (including why_it_matters and evidence) applies to the artifact files on disk, not the compact returns. 2. **Deduplicate.** Accept the helper's exact fingerprint merges first. Then use semantic judgment for differently worded findings or nearby lines that describe the same failure; record why they are equivalent rather than asking the script to guess. Keep highest severity/anchor and note every contributing reviewer. Dedup runs over the full validated set (including anchor 50) so cross-reviewer promotion in step 3 can lift matching anchor-50 findings into the actionable tier. 3. **Cross-reviewer agreement.** When 2+ independent reviewers flag the same issue (same fingerprint), promote the merged finding by one anchor step: `50 -> 75`, `75 -> 100`, `100 -> 100`. Note the agreement in the Reviewer column of the output (e.g., "security, correctness"). **Promotion never bypasses the quote-the-line gate (step 1).** A finding may sit at anchor 75 or 100 — whether originally or via this promotion — only if the merged finding carries `first_evidence`. If no contributing reviewer supplied it (e.g. two reviewers reported the same finding without the quote and step 1 demoted both to 50), cap the promotion at 50: agreement corroborates that the issue is *real*, but the quoted line is what licenses *high confidence*, and two un-quoted findings must not combine into a quote-free 75. When at least one contributor supplied `first_evidence`, the merged finding inherits it (step 2 keeps it) and promotes normally. The cross-model `adversarial-` return counts as an independent reviewer here; agreement between it and the in-process `adversarial` persona is the strongest signal in the set (different model families, separate processes) — render it as `adversarial, adversarial-`. The Stage 4 `fast-pass` pseudo-reviewer is the orchestrator's own read, **not** independent, so it **never counts toward this promotion** (and is capped at anchor 50): a `fast-pass`+persona fingerprint match is noted in the Reviewer column (e.g. "correctness, fast-pass") but does **not** bump the anchor — the persona's own independent anchor carries the finding. A `fast-pass`-only finding stays at anchor 50 (surfacing solo only when P0). 4. **Separate pre-existing.** Pull out findings with `pre_existing: true` into a separate list. **Exception — the change depends on it:** a pre-existing or adjacent-file gap that the change under review *relies on for its own correctness* stays in the primary findings, not the pre-existing list. The bar is a direct dependency — the new code is wrong, unsafe, or unverified *because of* this gap. Merely sitting nearby, or the repo being better without it, does not qualify: if the change would be correct regardless of the gap, it stays pre-existing. Do not widen this into general repo cleanup ("fix the whole repo"). 5. **Resolve disagreements.** When reviewers flag the same code region but disagree on severity, autofix_class, or owner, annotate the Reviewer column with the disagreement (e.g., "security (P0), correctness (P1) -- kept P0"). 6. **Normalize routing.** For each merged finding, set the final `autofix_class`, `owner`, and `requires_verification`. If reviewers disagree, keep the more conservative route. Remap any legacy `safe_auto` or `review-fixer` to `gated_auto` / `downstream-resolver`. 6b. **Mode-aware demotion of weak general-quality findings.** Some persona output is real signal but does not warrant primary-findings attention. Reroute it to the existing soft buckets so the primary findings table stays focused on actionable issues. A finding qualifies for demotion when **all** of these hold: - Severity is P2 or P3 (P0 and P1 always stay in primary findings) - `autofix_class` is `advisory` (concrete-fix findings stay in primary) - **All** contributing reviewers are `testing` or `maintainability` — if any other persona also flagged this finding, cross-reviewer corroboration is present and the finding stays in primary findings regardless of its severity or advisory status (expand the weak-signal list later only with evidence) When a finding qualifies: - Move demoted findings out of the primary set. If the contributing reviewer is `testing`, append ` -- ` to `testing_gaps`. If `maintainability`, append to `residual_risks`. Use title-only lines (compact return omits `why_it_matters`). Record the demotion count for Coverage. 7. **Confidence gate.** After dedup, promotion, and demotion have shaped the primary set, suppress remaining findings below anchor 75. Exception: P0 findings at anchor 50+ survive the gate -- critical-but-uncertain issues must not be silently dropped. Record the suppressed count by anchor (so Coverage can report "N findings suppressed at anchor 50, M at anchor 25"). The gate runs late deliberately: anchor-50 findings need a chance to be promoted by step 3 (cross-reviewer corroboration) or rerouted by step 6b (mode-aware demotion to soft buckets) before any drop decision. 8. **Partition the work.** Build two sets: - actionable queue: `gated_auto` or `manual` findings whose owner is `downstream-resolver` (hand off to caller) - report-only queue: `advisory` findings plus anything owned by `human` or `release` 9. **Sort and number.** Order by severity (P0 first) -> anchor (descending) -> file path -> line number, then assign monotonically increasing `#` values across the full primary finding set in that sorted order. Do not restart numbering inside each severity table, triage group, or autofix/routing bucket. If later sections repeat a finding (for example Actionable Findings), reuse the same stable `#` so users and downstream workflows can reference findings by `#` across the report and caller handoff. 9b. **Build thematic triage groups.** After stable `#` values exist, group related findings so the reader can triage themes instead of items. This is distinct from deduplication: dedupe answers "are these the same finding?", grouping answers "are these distinct findings that should be understood or resolved together?". Groups never merge findings into a synthetic finding and never change a finding's severity, confidence, route, owner, or stable `#`. Groups span the **full primary finding set** — both actionable and report-only findings — so they organize the whole report, not just the apply queue. - **`grouping:off`:** skip this step. - **`grouping:auto` (default):** build groups when findings span distinct concerns — the trigger is distinct concerns, not item count (mirroring how plan Requirements group by capability). Skip only when all findings are genuinely about the same thing; prefer no groups over decorative single-item groups. - **`grouping:always`:** always build groups; use single-finding groups only when no meaningful multi-finding grouping exists. - **Grouping signals:** shared root cause, affected subsystem, user-facing failure mode, overlapping fix path, dependency ordering, or repeated symptoms of one design choice. - **Group shape:** short title, the included stable finding `#`s, one-line context, preferred resolution, and why — when one fix path resolves several findings, name it and say which finding to handle first. - **Ordering:** order groups by the highest-severity finding they contain, then by lowest stable `#`. A finding appears in at most one group; leave genuinely unrelated findings ungrouped. 10. **Collect coverage data.** Union residual_risks and testing_gaps across reviewers. 11. **Preserve spec-first local-prompt artifacts.** Keep the learnings, agent-native, and deployment-verification outputs alongside the merged finding set. Do not drop unstructured output just because it does not match the persona JSON schema. Schema drift from `data-migration` is already in the merged finding set. ### Stage 5b: Validation pass (optional quality gate) Independent verification gate. Spawn one validator sub-agent per surviving finding using `references/validator-template.md`. Findings the validator rejects are dropped; confirmed findings flow through unchanged. **When this stage runs:** After Stage 5 whenever at least one finding survives — skip only when zero survive. When more than 15 survive, do **not** skip the stage; validate per the budget cap in step 2. The default method is the per-finding validator wave (steps below); a surviving **P2/P3 finding at anchor 100** may instead be validated by direct first-party verification (see below). Same rule for default and `mode:agent`. **Steps:** 1. **Select findings to validate.** All survivors of Stage 5. 2. **Apply dispatch budget cap.** If the selected set exceeds 15 findings, validate the highest-severity 15 (P0 first, then P1, then P2, then P3, breaking ties by anchor descending), dropping only from the P2/P3 tail. **Never drop a P0 or P1 from validation** — if P0/P1 findings alone exceed 15, raise the cap to include all of them. Record the over-budget count (the dropped P2/P3 tail) for the Coverage section. 3. **Spawn validators with bounded parallelism.** One sub-agent per finding, dispatched independently using the validator template and the same bounded scheduler from Stage 4. Each validator receives: - The finding's title, severity, file, line, suggested_fix, original reviewer name, and confidence anchor - `why_it_matters` when available — loaded from `{review_artifact_dir}/{reviewer_name}.json`; omit when `artifact_path` is null, the file is absent, or the artifact write failed. The validator proceeds without it, using the diff and cited code directly. - The full diff - When task mode is active, the same task-scope bundle and file-isolation labels supplied to reviewers; validators must validate the finding against the cited task bundle rather than the full working-tree diff - The scope mode and remote head ref, mirroring the Stage 4 reviewer bundle: inject `<pr-scope-mode>local-aligned | pr-remote | branch-remote</pr-scope-mode>` and, when set, `<pr-head-ref>...</pr-head-ref>` or `<branch-head-ref>...</branch-head-ref>`. The validator template defaults to local-aligned workspace inspection when these are absent, so omitting them in `pr-remote`/`branch-remote` makes validators verify findings against the stale working tree — dropping valid findings or confirming false ones on the wrong tree. - Inspection access scoped by mode: in `local-aligned`, Read/Grep/git blame the cited code, callers, guards, framework defaults, and history; in `pr-remote`/`branch-remote`, inspect via `git show <remote-head-ref>:<path>` or the provided diff hunks only — do not Read/Grep workspace paths for files in scope. 4. **Collect verdicts.** Each validator returns `{ "validated": true | false, "reason": "<one sentence>" }`. - `validated: true` -> finding survives unchanged into Stage 6 - `validated: false` -> finding is dropped; record the validator's reason in Coverage - Validator **infrastructure** failure (timeout, dispatch error, malformed JSON — not a `validated:false` verdict): for **P2/P3**, drop the finding with reason "validator failed" (conservative bias). For **P0/P1**, do **not** drop on infra failure — keep the finding and mark its validation **degraded** (note in Coverage). A transient validator failure must never silently remove a critical/high finding; a genuine `validated:false` rejection above still drops at any severity. 5. **Use mid-tier model for validators.** Same platform model class the mid-tier persona reviewers use; omit the override if the model name is unknown. Validators are read-only — same constraints as persona reviewers. They may use non-mutating inspection commands (Read, Grep, Glob, git blame, gh). 6. **Record metrics for Coverage.** Total dispatched, validated true count, validated false count (with reasons), infra failures (and any P0/P1 kept-on-failure as degraded), and over-budget drops. 7. **Prune triage groups after drops.** When validation dropped any finding, rebuild or prune `triage_groups` from the validated set: a group must never reference a `#` that was rejected or dropped. Remove groups left with fewer than two findings under `grouping:auto`; under `grouping:always`, keep them as single-finding groups only when still meaningful. **Orchestrator direct verification.** When a finding hinges on a fact the orchestrator can check cheaply and authoritatively — a pinned dependency's source, a wiring/config fact in this repo, a build tag — verify it directly with single-purpose native tools (Read/Grep/Glob, one git command at a time), never chained or error-suppressed shell. Fold confirmed facts into synthesis. Whether it can *replace* the independent validator turns on a single distinction: the orchestrator is **not** an independent second opinion (it synthesized these findings), so direct verification catches a wrong **fact** but not the orchestrator's own **bias**. Independence adds nothing to a mechanically-checkable fact and everything to a judgment call: - **P0/P1, any anchor:** the per-finding validator wave is **required**; direct verification only *complements* it, never replaces it. - **P2/P3 at anchor 100, MECHANICAL only** — the finding is true by inspection of the code itself: a compile/type error, a definitive logic bug, or a standards violation with a quotable rule, where the quoted line *entails* the conclusion with no interpretation. Direct verification **may stand in for** the wave. **Shortcut:** confirm the finding's `first_evidence` line (1) actually exists at the cited `file:line` in the diff/cited file, AND (2) on its own substantiates the claim (the quote *is* the bug, not merely adjacent to it). When both hold, no validator subagent is needed. **A quote proves the line exists, not that the conclusion follows** — so this shortcut is barred for any finding whose truth depends on runtime behavior or cross-file reasoning even at anchor 100: security (SSRF/IDOR/authz), concurrency/races, perf, or anything touching auth/contracts. Those keep the independent validator regardless of anchor. - **P2/P3 at anchor 75** (judgment call — "will affect users," not airtight): the independent wave is **required** — this is exactly where a fresh second opinion filters false positives, and the orchestrator cannot supply that for its own findings. **Why per-finding bounded dispatch (not batched):** Independence is the point. A single batched validator looking at all findings together pattern-matches across them and recreates the persona-bias problem. Per-finding dispatch preserves fresh context while the scheduler respects harness limits. ### Stage 5c: Act on findings (explicit apply only) **Skip entirely in `mode:agent`, `mutation_policy: report-only`, and every inline degraded dispatch fallback.** The caller owns apply in agent mode. Ordinary default review is also report-only. Enter this stage only when default mode has explicit `mutation_policy: apply-fixes` and full reviewed-tree scope is known. Read `references/apply-findings.md` only after all three admission conditions pass: default mode, explicit `mutation_policy: apply-fixes`, and a non-degraded local reviewed-tree scope. Do not load that reference in `mode:agent`, report-only, remote-only, or inline fallback runs. The reference contains apply/verification/commit details but grants no authority by itself. ### Stage 5d: Structured targeted verification evidence Only when a targeted verification command actually ran in this review may the review create command evidence. Persona findings, validator verdicts, cross-model agreement, quoted source lines, inferred test gaps, and a reviewer's natural-language “tests passed” statement are semantic review evidence, not command evidence; never serialize them as a passed check. When no targeted command ran, do not call either helper. Set `coverage.verification_evidence` to `status: not-produced`, `run_summary_ref: null`, `closeout_verdict: not-run`, `reason_code: no-targeted-command-executed`, and include any relevant limitations. This is an honest absence of command evidence, not a failed review. When one or more targeted commands actually ran—such as an orchestrator-owned mechanical fact check that executed a test/build command, or post-Stage-5c verification after explicitly authorized fixes—capture their real exit codes and bounded secret-stripped logs under `.spec-first/workflows/spec-code-review/<workspace-slug>/<run-id>/logs/`. Evidence writes are limited to repo-local workflow artifacts; they do not authorize source changes, apply, commit, push, PR, or ticket actions. If the invocation or target repo forbids artifact writes, keep the result in band and use a degraded `artifact-write-not-authorized` limitation instead of claiming a durable ref. Record only those actual command outcomes: ```bash spec-first internal verification-run-summary record \ --workflow spec-code-review \ --input <verification-run-summary-input.json> \ --run-id <run-id> \ --target-repo <repo-root> \ --json ``` Then construct validation claims from `verification-run-summary:<check-id>` refs and validate them: ```bash spec-first internal honest-closeout validate \ --input <honest-closeout-claims.json> \ --target-repo <repo-root> \ --json ``` Do not cherry-pick a passing targeted check to hide another failed/not-run check in the same summary. Preserve `run_summary_ref`, `overall`, `overall_reason_code`, per-claim reasons, and limitations in `coverage.verification_evidence`. `spec-code-review` does not own or emit the durable spec-work run artifact; a `spec-work` caller may later materialize the review JSON/summary into its own run directory. ### Stage 5e: Enforce the report-only mutation gate Before synthesis, every local `mode:agent`, report-only, and inline-fallback run must compare the current tree with the Stage 1a snapshot: ```bash bash "$SKILL_DIR/scripts/run-python.sh" \ "$SKILL_DIR/scripts/review-scope.py" \ --verify-snapshot "$SCOPE_SNAPSHOT" ``` If the result is `mutation_detected: false`, continue and keep using the frozen scope facts. If it reports `reviewer_mutation_detected`, do not revert, hide, reinterpret, or validate the mutation as a successful fix. In `mode:agent`, return one raw failure JSON with `status: failed`, `reason_code: reviewer_mutation_detected`, `mutated_paths`, the frozen scope, and the already discovered findings when available. In default report-only mode, stop with the same reason and paths. A reviewer-caused mutation invalidates any test result obtained after the mutation and blocks completion claims. If the snapshot cannot be read or rechecked, return a degraded/failed handoff with `mutation_guard_unavailable`; absence of the deterministic guard is never evidence that the tree stayed unchanged. Explicit default-mode Stage 5c apply runs are excluded because their mutation is separately admitted, scoped, and verified. ### Stage 6: Synthesize and present Assemble the final report. **Default:** human-readable markdown. **`mode:agent`:** skip markdown and emit JSON (see ### JSON output format) — the structured fields are how a downstream agent consumes the review. Put `---` before the verdict in markdown mode. **Before writing, load `references/review-output-template.md` and mirror its section skeleton** — that file is the canonical skeleton for *which sections appear and in what order*; its example shows one good rendering, not the only permitted layout. The direction below is the always-loaded fallback so it survives a long session even if the template was not reloaded. **Presentation direction — optimize for the reader's next action (goal + considerations, not a fixed layout).** The report is *acted on*: by a human deciding what to fix and whether to merge, or by a downstream agent applying fixes. Shape it so that action is fast and well-founded. - **Per finding, make four things unambiguous** (in whatever layout reads clearest): *what & where* — one scannable line, the symptom + `file:line`, not the mechanism; *why it matters* — what breaks or who's hit, never a restatement of the code; *what response it needs* — this varies by finding type: a bug states its fix, a **design call** presents the options and the tradeoff without forcing one answer, a coverage gap names the test and precedent to mirror, a residual risk is marked informational, an already-applied item gives what changed and how it was verified; *how sure* — confidence, and whether it was corroborated (cross-reviewer / cross-model agreement is the strongest signal — say so). - **Let the shape serve the finding type; stay consistent within a section.** A terse table, a short keyed block, or a compact list are all fine — pick what reads clearest for that content. Consistency *within* a section matters; a single global shape does not. - **Group by the unit of work or decision, not just severity.** Severity orders urgency; it does not tell the actor what *kind* of action a finding needs. Surface the split: **decisions a human must make** (design calls, ambiguous semantics) vs **mechanical work that can just be done** (tests, dedup, concrete fixes) vs **informational** (residual risks) — an agent clears the mechanical work and must stop at decisions. Group findings sharing a root cause or one fix (the Triage Groups) and name the order/dependency ("decide X once -> resolves #1 and #7; do #1 first"); the unit of work is often a group, not a finding. - **Detail is earned by enabling the next action, not by demonstrating thoroughness.** Cover *every* finding — completeness is non-negotiable — but say each in the least that lets the consumer act. **Do not paste file contents or re-print the diff**; it is already in the repo/PR — cite `file:line` and spend words only on what the diff can't show (why it breaks, the fix, the repro). This governs *expression, never coverage*: never drop a finding or its why/fix to be shorter, and match weight to weight (a nit is one line; a P1 design call earns room). - **The bottom is the most-read screen — make the closing self-sufficient.** In long output the reader's viewport lands at the end, so the **Verdict and Actionable list must stand alone without scrolling**: the verdict plus the single most important thing to do, then the prioritized actionable list where each item already carries severity, `file:line`, the terse what, and its response-type. The itemized findings above are drill-down evidence, referenced by stable `#`. **Hard constraints (non-negotiable; everything above is judgment):** - **ASCII-safe only — no box-drawing or per-item horizontal-rule separators (`────`, `———`), no Unicode arrows or middot (`·`); use `->`.** These break across terminals and violate repo convention. (The single report-level `---` before the verdict is fine.) - **Stable `#` numbering from Stage 5** — never re-derive per section; reuse the same `#` everywhere a finding appears. A multi-file applied fix is one row with one `#`, never duplicated. - **If you use a markdown table, escape literal `|` in cells as `\|`** so a pipe inside a title/regex/cache-key example doesn't split the row. - **The Verdict and Actionable list are present, last, and self-sufficient.** This is satisfied by the closing, not the section skeleton: the Verdict is the final report section, immediately followed by the post-report prioritized Actionable recap (default mode — see *Emit actionable findings summary* below). The in-report `Actionable Findings` section keeps its skeleton position (5) as the detailed table; the recap is the self-sufficient last word the reader sees without scrolling. (If for some layout you cannot emit the recap, move the Actionable list itself to just after the Verdict.) 1. **Header.** Scope, intent, mode, resolved `mutation_policy`, commit authorization/status when applicable, scenario/dispatch limitations, and reviewer team with per-conditional justifications. 2. **Applied (explicit apply only).** When Stage 5c ran under `mutation_policy: apply-fixes`, list applied fixes first — before the findings — with `#`, file, fix, reviewer, validation outcome, and commit authorization/status. Omit this section in `mode:agent`, report-only/default reviews, degraded inline fallbacks, and when nothing was applied. Applied findings appear here, not in the severity tables. 2b. **Triage Groups.** When finalized `triage_groups` exist (post-validation, post-apply — Stage 5b step 7 / Stage 5c), render a `### Triage Groups` section before the findings as a compact table (`| Group | Findings | Context | Preferred Resolution | Why |`) — a table fits this content well. The `Findings` cell lists the stable `#`s it covers; the resolution names the order/dependency. **Mark whether each group is an apply-queue or a decision-gate** (so an automated fixer applies the mechanical groups and stops at the design calls). Every referenced `#` must appear in the findings below; groups supplement the findings, never replace them. Omit the section when `grouping:off` is active or no groups survived. In `mode:agent` this section is carried by the `triage_groups` JSON field instead. 3. **Findings.** Grouped by severity (`### P0 -- Critical`, `### P1 -- High`, `### P2 -- Moderate`, `### P3 -- Low`), rendered per the per-finding direction above and consistent within the section. Surface the decision-vs-mechanical split where it helps the actor (flag the design calls). Omit empty severity levels. Finding numbers come from the stable assignment in Stage 5 -- never re-derive them per severity section or triage group. 4. **Requirements Completeness.** Include only when a plan was found in Stage 2b. Outside task mode, for each requirement (R1, R2, etc.) and implementation unit in the plan, report whether corresponding work appears in the diff. In task mode, report only the selected Task Card's source refs and label the section `selected-task`; never create missing-work findings for unrelated plan units. Use a simple checklist: met / not addressed / partially addressed. Routing depends on `plan_source`: - **`explicit`** (caller-provided or PR body): Flag unaddressed requirements or implementation units as P1 findings with `autofix_class: manual`, `owner: downstream-resolver`. These enter the actionable queue. - **`inferred`** (auto-discovered): Flag unaddressed requirements or implementation units as P3 findings with `autofix_class: advisory`, `owner: human`. These stay in the report only — no autonomous follow-up. An inferred plan match is a hint, not a contract. Omit this section entirely when no plan was found — do not mention the absence of a plan. 5. **Actionable Findings.** Include when the actionable queue is non-empty — findings the caller should address (`gated_auto` / `manual` with `downstream-resolver`), plus anything Stage 5c chose not to apply. In default mode, findings already applied appear in the Applied section, not here. 6. **Pre-existing.** Separate section, does not count toward verdict. 7. **Learnings & Past Solutions.** Surface `learnings-researcher` local-prompt results: if past solutions are relevant, flag them as "Known Pattern" with links to docs/solutions/ files. 8. **Agent-Native Gaps.** Surface `agent-native-reviewer` local-prompt results. Omit section if no gaps found. 9. **Deployment Notes.** If the `deployment-verification-agent` local prompt ran, surface the key Go/No-Go items: blocking pre-deploy checks, the most important verification queries, rollback caveats, and monitoring focus areas. Keep the checklist actionable rather than dropping it into Coverage. Schema drift appears in the findings tables as `data-migration` P1 rows — do not add a separate Schema Drift section. 10. **Coverage.** Applied count (when Stage 5c ran), suppressed count by anchor (e.g., "N findings suppressed at anchor 50, M at anchor 25"), mode-aware demotion count, validator drop count and reasons (when Stage 5b ran), any P0/P1 with degraded validation (kept on validator infra failure), validator over-budget drops (when the 15-cap fired), residual risks, testing gaps, failed/timed-out reviewers, inferred-intent uncertainty, and the Stage 5d `verification_evidence` object when applicable. When the Stage 3c lite roster ran, state it and the reduced reviewer set (so the narrower coverage is visible). When the Stage 5b direct-verification shortcut skipped validators for anchor-100 findings, note how many were verified by quote rather than by an independent validator subagent. When the Stage 5 quote-the-line gate demoted any 75/100 finding for missing `first_evidence`, record that count. **Removable surface (only when deletion-oriented maintainability findings exist):** one line giving the approximate net lines/files those findings would remove if applied (e.g., "Removable surface: ~120 lines / 2 files across findings #4, #7"). This is a dead-weight signal, **not** a reduction target — never lower the bar for a finding or invent deletions to grow the number, and omit the line entirely when no finding proposes a deletion. 11. **Verdict.** Ready to merge / Ready with fixes / Not ready. Fix order if applicable. When an `explicit` plan has unaddressed requirements or implementation units in the active completeness scope, the verdict must reflect it — a full PR review that's code-clean but missing planned requirements is "Not ready" unless the omission is intentional. Task mode's active scope is only the selected Task Card; unrelated future/dependent units do not affect its verdict. When an `inferred` plan has unaddressed requirements or implementation units, note it in the verdict reasoning but do not block on it alone. Do not include time estimates. **Final check before delivering (default only).** Verify the hard constraints, not a layout: no box-drawing / per-item horizontal-rule separators (`────`), no Unicode arrows or middot (`·`) anywhere; stable `#`s consistent across sections; literal `|` escaped (`\|`) in any table cell; and **the closing stands alone** — a reader seeing only the last screen gets the verdict and the prioritized actionable list, each item carrying its severity, `file:line`, terse what, and response-type. Re-render anything that fails. Skip when `mode:agent` is active. ### JSON output format (`mode:agent` only) Emit **one raw JSON object** as the primary response — a single bare JSON value, **no markdown code fence**. A leading ```` ```json ```` fence makes the response start with backticks and breaks naive `JSON.parse` consumers, so never wrap it. When `REVIEW_ARTIFACT_DIR` is available, also write the same payload to `{review_artifact_dir}/review.json`. `mode:agent` does not apply fixes — the caller does — so there is no `applied_fixes` field; the handoff is `actionable_findings`. Applied work surfaces only in the default-mode markdown Applied section (Stage 5c/6). Minimum shape: ```json { "status": "complete", "verdict": "Ready to merge | Ready with fixes | Not ready", "mutation_policy": "report-only | apply-fixes", "commit_authorization": "authorized | missing", "scope": { "base": "<merge-base sha, pr:NNN marker, or base: ref>", "branch": "<current branch name>", "head_sha": "<git rev-parse HEAD>", "pr_url": "<url or null>", "files_changed": 0 }, "intent": "<2-3 line summary>", "intent_confidence": "explicit | inferred | uncertain", "reviewers": ["correctness", "security"], "findings": [], "actionable_findings": [], "triage_groups": [], "pre_existing_findings": [], "requirements_completeness": null, "learnings": [], "agent_native_gaps": [], "deployment_notes": [], "residual_risks": [], "testing_gaps": [], "coverage": { "task_scope": null, "dispatch_reason_code": null, "scenario_capability": "full | bounded | partial | fallback-only | blocked-action-required", "verification_evidence": { "status": "recorded | not-produced | degraded", "run_summary_ref": "<repo-relative ref or null>", "closeout_verdict": "verified | degraded | unsupported | not-run", "reason_code": "<structured reason code>", "checks": [], "limitations": [] } }, "artifact_path": "<absolute path or null>", "artifact_write_status": "written | unavailable | partial", "run_id": "<run-id>" } ``` When task context is active, `coverage.task_scope` is an object with `task_id`, `task_pack`, `expected_task_pack_digest`, `observed_task_pack_digest`, `source_plan`, `source_plan_section_titles`, `plan_context_mode`, `work_run_base`, `declared_files`, `task_delta_files`, `task_owned_untracked_files`, `pre_task_dirty_files`, `pre_task_untracked_files`, `pre_task_overlap`, `review_focus`, per-file isolation, aggregate `task_diff_isolation`, `required_gate_eligible`, and `limitations`. Outside task mode it remains `null`. `coverage.verification_evidence` is always present in `mode:agent`. It reports a repo-relative `run_summary_ref` and `closeout_verdict` only when Stage 5d recorded commands this review actually executed. Pure review judgment uses `status: not-produced` with `reason_code: no-targeted-command-executed`; never turn persona/validator coverage into a fake check. Each object in `findings` uses the merged finding fields: `#`, `title`, `severity`, `file`, `line`, `confidence`, `autofix_class`, `owner`, `requires_verification`, `pre_existing`, `suggested_fix`, `first_evidence`, `why_it_matters`, `evidence`, `reviewers`. `actionable_findings` lists the `gated_auto` / `manual` + `downstream-resolver` subset with the same fields plus stable `#`. Each object in `triage_groups` carries `{ "title", "findings": [<stable #s>], "context", "preferred_resolution", "why" }` — the finalized groups from Stage 5 step 9b after Stage 5b pruning. Every referenced `#` must exist in `findings` (the full set) — **not** necessarily in `actionable_findings`. Groups are a triage **lens over all findings, not an apply queue**: a group (and its `preferred_resolution` ordering) can reference advisory or `human`/`release`-owned findings that the caller must not apply. So a caller batching related fixes by theme must first intersect each group's `findings` with `actionable_findings` and act only on that subset — the apply handoff stays `actionable_findings`, never `triage_groups`. Empty array when `grouping:off` is active or no groups were built. On failure before review completes, set `"status": "failed"` and `"reason": "<one sentence>"`. When all reviewers fail, use `"status": "degraded"` with a reason. When a PR skip rule fires (closed/merged/trivial), use `"status": "skipped"` with the skip reason. Do not emit markdown tables when `mode:agent` is active. ## Quality Gates Before delivering the review, verify: 1. **Every finding is actionable.** Re-read each finding. If it says "consider", "might want to", or "could be improved" without a concrete fix, rewrite it with a specific action. Vague findings waste engineering time. 2. **No false positives from skimming.** For each finding, verify the surrounding code was actually read. Check that the "bug" isn't handled elsewhere in the same function, that the "unused import" isn't used in a type annotation, that the "missing null check" isn't guarded by the caller. 3. **Severity is calibrated.** A style nit is never P0. A SQL injection is never P3. Re-check every severity assignment. 4. **Line numbers are accurate.** Verify each cited line number against the file content. A finding pointing to the wrong line is worse than no finding. 5. **Protected artifacts are respected.** Discard any findings that recommend deleting or gitignoring files in `docs/brainstorms/`, `docs/plans/`, or `docs/solutions/`. 6. **Findings don't duplicate linter output.** Don't flag things the project's linter/formatter would catch (missing semicolons, wrong indentation). Focus on semantic issues. ## Language-Aware Conditionals Stack-specific reviewers fire only when the diff touches runtime behavior they specialize in (async UI races, iOS/Swift lifecycle) — never mechanically from file extensions alone; the trigger is meaningful changed behavior in that stack's runtime domain. Structural quality (complexity deletion, 1k-line regressions, type-boundary leaks) lives in the always-on `maintainability-reviewer`; do not spawn extra reviewers for language conventions, philosophy, or "strict bar" passes. ## After Review After Stage 6, stop. Never push, open PRs, or file tickets from this skill. Ordinary default review and every `mode:agent` run mutate nothing. Only explicit `mutation_policy: apply-fixes` may have changed the checkout, and only separate commit authorization may have created a commit. Callers use the report/artifact to apply fixes, record residuals, or perform separately authorized landing work. ### Emit actionable findings summary (default mode only) After Stage 6 **in default mode**, emit a compact **Actionable Findings** summary for callers: - List each actionable finding (`gated_auto` or `manual` with `downstream-resolver`) with stable `#`, severity, file:line, title, `autofix_class`, whether `suggested_fix` is present, and `confidence`. - Include the returned `artifact_path` when one was written; when null, state the artifact limitation and keep the in-band summary complete. - When the actionable queue is empty, state `Actionable findings: none.` explicitly. In `mode:agent` do **not** emit this markdown summary — the actionable findings are carried solely by the `actionable_findings` field of the JSON object. Emit nothing after the JSON object, so the response stays a single parseable JSON value. Do not run post-review triage (no per-finding walk-through, bulk ticket filing, or routing questions). The report and summary are the complete handoff. ### Mode-specific completion | Mode | After Stage 6 + actionable summary | |------|-----------------------------------| | **Default** | Markdown tables + Actionable Findings summary. | | **`mode:agent`** | JSON object + `review.json` in run artifact dir. | Do not offer push/PR/create-branch next steps from this skill. #### Run artifacts When `REVIEW_ARTIFACT_DIR` is available, write run artifacts under that exact returned `artifact_path`: - synthesized findings - actionable findings list - advisory outputs - per-agent `{reviewer_name}.json` from Stage 4 - `report.md` — the rendered markdown report exactly as presented to the user (default mode only), so format and numbering stay auditable after the run `metadata.json` minimum fields: ```json { "run_id": "<run-id>", "branch": "<git branch --show-current at dispatch time>", "head_sha": "<git rev-parse HEAD at dispatch time>", "verdict": "<Ready to merge | Ready with fixes | Not ready>", "completed_at": "<ISO 8601 UTC timestamp>" } ``` Capture `branch` and `head_sha` at dispatch time as the reviewed snapshot. If explicit Stage 5c fixes later change `HEAD` or the tree, record the post-apply state separately; do not rewrite the reviewed snapshot. ## Fallback If review dispatch is authorized but parallelism is unavailable, run authorized reviewers sequentially. If the platform caps active concurrency, use bounded queueing rather than treating backpressure as reviewer failure. If dispatch authorization or callable capability is missing, use the inline report-only degraded fallback in Stage 4; do not pretend sequential persona dispatch occurred. --- ## References Every reference lives in this skill's directory and loads **on demand at the stage that needs it** — none is `@`-inlined, because all of them are late-sequence and inlining would carry their full weight through the orchestrator's many early-stage turns and subagent dispatches. Each stage below already names the file to read; this is the maintainer index. Do not reintroduce `@` includes here. | Reference | Load at | Purpose | |-----------|---------|---------| | `references/persona-catalog.md` | Stage 3 | Full per-persona selection criteria and spawn gates | | `references/subagent-template.md` | Stage 4 | Dispatch shape for every persona subagent | | `references/diff-scope.md` | Stage 4 | Shared diff-scope rules passed to each subagent | | `references/findings-schema.json` | Stage 4 | JSON output contract passed to each subagent | | `references/cross-model-review.md` | Stage 4 (only when the cross-model adversarial pass runs) | Host self-identification + peer-CLI shell-out | | `references/action-class-rubric.md` | Action Routing (as needed) | Persona guidance for `autofix_class` | | `references/apply-findings.md` | Stage 5c (admitted default apply-only runs) | Apply, verification, and commit boundaries kept out of report-only context | | `references/review-output-template.md` | Stage 6 | Canonical section skeleton for the report | Selected reviewer prompt assets live under `references/personas/`. Read only the prompt files selected for the current review.