--- name: code-review description: "Review code, test, and workflow-machinery changes across multiple dimensions using specialized agents with triage-based selection." argument-hint: "[branch | commit-range | uncommitted | PR-number]" user-invocable: true --- ## Reading workflow files (TOC protocol) When you Read any file under `.claude/workflow/` or `.claude/skills/`, follow the protocol in `conventions.md §1.8`: 1. Read the TOC region: from `` to `` (read to the closing delimiter, not a fixed line count). If the file has no TOC region (a file whose only `## ` heading is this bootstrap block carries none, per `§1.8(d)`), read the file in full. 2. Match TOC rows where Roles contains any of your roles (or your role is `any`, or the row's Roles is `any`) AND Phases contains any of your phases (or your phase is `any`, or the row's Phases is `any`). 3. Use `Read(offset, limit)` to read only matched sections; if no row matches your role/phase, the file holds nothing for you — do not read further. Your role: orchestrator. Your phase: 3B or 3C (the review phase that invoked this skill). Inline refs you find inside workflow files carry the same `name:roles:phases` suffix; apply file-level filtering before opening: a ref matches when any of your roles is in its roles and any of your phases is in its phases, your own `any` on either axis matches every ref on that axis, and a ref whose own roles or phases is `any` matches you. Backtick-wrapped refs carry no suffix; open or skip them at your discretion. | Section | Roles | Phases | Summary | |---|---|---|---| | §Step 1: Determine what to review | orchestrator | 3B,3C | Resolve the review target from the argument or, when empty, from the branch's PR or its commits ahead of develop. | | §If `$ARGUMENTS` is provided: | orchestrator | 3B,3C | Six argument shapes: branch, commit range, last-N-commits, uncommitted, PR ref, or an unparseable input. | | §If `$ARGUMENTS` is empty: | orchestrator | 3B,3C | Fall back to the branch's open PR, then to commits ahead of develop, then ask the user when neither resolves. | | §Step 2: Detect the base branch | orchestrator | 3B,3C | Pick the comparison base: a PR's baseRefName, develop for a bare branch, or none for a range or uncommitted review. | | §Step 3: Gather the review context | orchestrator | 3B,3C | Collect changed files and commit log, and write the full diff to a unique /tmp file each agent reads on demand. | | §For branch or PR review: | orchestrator | 3B,3C | The git diff and log commands that build the changed-file list, diff file, and commit log for a branch or PR target. | | §For commit range: | orchestrator | 3B,3C | The git diff and log commands for a commit-range review target. | | §For uncommitted changes: | orchestrator | 3B,3C | The git diff commands for an uncommitted-changes target, with the no-commit-history sentinel for the log. | | §PR description (if available): | orchestrator | 3B,3C | The gh command that reads the PR body for context, when a PR is associated with the target. | | §Step 4: Filter non-reviewable files | orchestrator | 3B,3C | List the generated-code paths to skip and pass the filter note to every dispatched agent. | | §Step 5: Triage — categorize changes and select relevant agents | orchestrator | 3B,3C | Categorize each changed file, map categories to review agents, and log the triage decision before dispatch. | | §5a: Categorize each changed file | orchestrator | 3B,3C | The category table mapping file signals to domains; a file may carry several categories. | | §5b: Map categories to agents | orchestrator | 3B,3C | Launch rules for the 16 review agents across code, test, and workflow groups, keyed on detected categories. | | §5c: Log your triage decision | orchestrator | 3B,3C | Print the triage summary naming detected categories, selected agents, and skipped agents before launching. | | §5d: Edge cases | orchestrator | 3B,3C | Last-matching-row resolution for category combinations: workflow-only, docs-only, build-config, tests, mixed. | | §Step 6: Dispatch selected review agents | orchestrator | 3B,3C | Launch selected agents in parallel with a scope-filtered file list and the group-specific prompt template. | | §Prompt template — code-review and test-review groups | orchestrator | 3B,3C | The prompt body for the code and test review groups, including the PSI-over-grep tooling rule for Java symbols. | | §Prompt template — workflow-review group | orchestrator | 3B,3C | The prompt body for the workflow-review group, noting PSI does not apply to markdown, shell, and JSON files. | | §Handling missing fields | orchestrator | 3B,3C | Treat the missing-PR and uncommitted sentinels as no signal; do not infer requirements from their absence. | | §Step 7: Synthesize the results | orchestrator | 3B,3C | Map sub-agent severities, deduplicate, attribute, and summarize into one unified report, not a concatenation. | | §Handling agent failures and empty output | orchestrator | 3B,3C | Record failed reviewers, propagate preface notes, emit All clear when empty, and remove the temp diff file. | | §Output format | orchestrator | 3B,3C | The synthesized report layout: failed reviewers, assessment, blockers, should-fix, suggestions, notes, and questions. | | §Important rules | orchestrator | 3B,3C | Standing rules: gh CLI for GitHub, parallel dispatch, no self-added findings, no severity softening, large-diff abort. | Review code, test, and workflow-machinery changes across multiple dimensions by dispatching to specialized review agents and synthesizing their findings. Production code, test code, and workflow files (skills, agents, hooks, settings, prompts, CLAUDE.md, plan/design artifacts) are reviewed in one pass with triage-driven agent selection. Use `$ARGUMENTS` as the review target if provided (branch name, commit range, or "uncommitted"). ## Step 1: Determine what to review ### If `$ARGUMENTS` is provided: 1. **Branch name** (e.g., `ytdb-605-unified-edges`): Review all changes on that branch that are absent from the base branch. 2. **Commit range** (e.g., `abc123..def456`): Review that specific range. 3. **"last N commits"** (e.g., `last 3 commits`): Review `HEAD~N...HEAD`. 4. **"uncommitted"** or **"working tree"**: Review uncommitted changes (`git diff HEAD`). 5. **PR number or URL** (e.g., `#42` or `https://github.com/.../pull/42`): Fetch PR details and review its diff. 6. **None of the above** (malformed input, non-existent branch, unresolvable PR, free-form phrase): Report what you tried to parse, then ask the user for a valid target. Do not guess. ### If `$ARGUMENTS` is empty: 1. Check if the current branch has an open PR: ```bash gh pr list --head $(git branch --show-current) --json number,title,body,baseRefName,url --limit 1 ``` 2. If a PR exists, use it as the review target (the PR's base branch becomes the comparison base). 3. If no PR exists, check if the current branch differs from `develop`: ```bash git log develop..HEAD --oneline ``` 4. If there are commits ahead of `develop`, review those. 5. If the branch IS `develop` or has no commits ahead, ask the user what to review. Treat the user's reply as if it were `$ARGUMENTS` and restart Step 1. ## Step 2: Detect the base branch The base branch determines what "new changes" means: 1. If reviewing a PR: use the PR's `baseRefName` (fetched via `gh pr view`). 2. If reviewing a branch (no PR): default to `develop`. 3. If reviewing a commit range or uncommitted changes: no base branch needed. ## Step 3: Gather the review context Based on the review mode, collect the changed-file list and commit log inline, but **write the full diff to a temp file** so each agent can `Read` it on demand instead of receiving it interpolated into its prompt. This keeps per-agent prompt size bounded and removes the diff-size ceiling that inline interpolation would impose. Use a unique `/tmp` filename to avoid collisions with concurrent Claude Code agents on the same host (per the user-global rule). Generate the suffix once and reuse it for the whole dispatch: ```bash # Generate a unique suffix for this review's temp file DIFF_FILE=/tmp/claude-code-review-diff-$$.txt # or use $(uuidgen) ``` ### For branch or PR review: ```bash git diff {base}...HEAD --name-only # → CHANGED_FILES git diff {base}...HEAD > "$DIFF_FILE" # → DIFF_FILE git log {base}..HEAD --oneline # → COMMIT_LOG ``` ### For commit range: ```bash git diff {start}..{end} --name-only # → CHANGED_FILES git diff {start}..{end} > "$DIFF_FILE" # → DIFF_FILE git log {start}..{end} --oneline # → COMMIT_LOG ``` ### For uncommitted changes: ```bash git diff HEAD --name-only # → CHANGED_FILES git diff HEAD > "$DIFF_FILE" # → DIFF_FILE ``` For uncommitted changes there is no commit log; set `COMMIT_LOG` to the literal sentinel `"(uncommitted changes — no commit history)"`. ### PR description (if available): ```bash gh pr view {number} --json body --jq '.body' ``` Store the collected context: - `DIFF_FILE` — absolute path to the temp file containing the full diff (e.g., `/tmp/claude-code-review-diff-12345.txt`). Each agent reads this file via the `Read` tool. - `CHANGED_FILES` — the list of changed file paths - `COMMIT_LOG` — the commit history, or the uncommitted-changes sentinel above - `PR_DESCRIPTION` — the PR body text, or the literal string `"No PR associated with these changes."` if absent - `REVIEW_SCOPE` — human-readable description of what's being reviewed (e.g., "Branch `ytdb-605-unified-edges` vs `develop` (15 commits, 23 files)") The temp file is ephemeral — remove it at the end of Step 7 (synthesis) so orphans don't accumulate. The unique suffix prevents collision with concurrent agents while the review is in flight. ## Step 4: Filter non-reviewable files Before dispatching, note files that should be skipped: - Files under `core/src/main/java/com/jetbrains/youtrackdb/internal/core/sql/parser/` - Generated Gremlin DSL classes - Files under `generated-sources/` or `generated-test-sources/` Include this filter note in the context passed to agents. ## Step 5: Triage — categorize changes and select relevant agents Before dispatching agents, perform a quick triage pass over the **entire diff** (both production and test code) to determine which review dimensions are actually relevant. This avoids wasting time on agents that have nothing meaningful to review. ### 5a: Categorize each changed file Scan the diff and assign one or more categories to **every** changed file — production code, test code, and other files alike: | Category | Signals | |---|---| | **storage-engine** | Files in `storage/`, `cache/`, `wal/`, `StorageComponent` subclasses, page read/write logic, `DiskStorage`, `WriteCache`, `ReadCache`, `LogSequenceNumber`, double-write log | | **concurrency** | `synchronized`, `Lock`, `Atomic*`, `volatile`, `StampedLock`, `ReentrantLock`, thread pools, `ConcurrentHashMap`, `CompletableFuture`, shared mutable state, `@GuardedBy`, `ConcurrentTestHelper`, `CountDownLatch`, `CyclicBarrier` | | **index-data-structures** | Files in `index/`, B-tree, hash index, `SBTree`, `CellBTree`, histogram, `IndexEngine` | | **network-server** | Files in `server/`, `driver/`, Gremlin Server, protocol handling, TLS/SSL, authentication, session management | | **sql-query** | Files in `sql/` (excluding `parser/`), query execution, command handlers, `SELECT`/`INSERT`/`UPDATE`/`DELETE` logic | | **gremlin** | Files in `gremlin/`, traversal steps, `YTDBGraph*` classes, TinkerPop integration | | **public-api** | Files in `com.jetbrains.youtrackdb.api`, `YourTracks`, `YouTrackDB` interface | | **serialization** | Record serializers, binary format, property map encoding/decoding | | **crash-durability** | WAL operations, crash simulation, durable `StorageComponent` recovery, page corruption handling, transaction atomicity under failure, `LogSequenceNumber` manipulation, double-write log, Java `assert` statements in production code | | **configuration** | `GlobalConfiguration`, config parameters, system properties | | **tests-only** | Changes exclusively in test files with no production code changes | | **build-config** | `pom.xml`, CI workflows, Maven profiles, Docker configs | | **workflow-machinery** | Files under `.claude/` (skills, agents, hooks, scripts, settings, workflow rules, workflow prompts, output styles, docs), project root `CLAUDE.md`, all files under `docs/adr//` (plan/design artifacts in `_workflow/` and the durable `design-final.md` / `adr.md`) | | **docs-only** | Markdown documentation under `docs/` excluding `docs/adr//`, plus comments-only changes | | **other** | Files matching no category above (e.g., `.gitattributes`, miscellaneous root files). Triaged as a no-op — no agents dispatch on this category. | A file can belong to multiple categories (e.g., a lock change in storage code is both `storage-engine` and `concurrency`). Production and test files in the same domain should share the same categories. `workflow-machinery` is exclusive with `docs-only`: any file under `.claude/` or `docs/adr//` is `workflow-machinery`; anything else under `docs/` is `docs-only`. ### 5b: Map categories to agents There are **16 specialized review agents** in three groups: **Code-review agents** (review production code): | Agent | Launch when ANY of these categories are present | |---|---| | **review-code-quality** | Always launched (unless `docs-only` is the ONLY category) | | **review-bugs** | Always launched (unless `docs-only` or `build-config` are the ONLY categories) | | **review-concurrency** | `concurrency` is present on any changed file | | **review-crash-safety** | `crash-durability` | | **review-security** | `network-server`, `public-api`, `sql-query`, `serialization`, `configuration`, OR when new dependencies are added in `pom.xml` | | **review-performance** | `storage-engine`, `index-data-structures`, `concurrency`, `serialization`, `sql-query`, `gremlin` | `review-bugs` owns every defect findable by single-threaded sequential reasoning (logic, null safety, resource leaks, RID handling, state-machine / lifecycle); `review-concurrency` owns every defect whose detection needs reasoning about two or more threads interleaving (races, visibility / publication, lock-ordering / deadlock, compound-op atomicity). When `review-bugs`, reasoning sequentially, meets concurrent-looking code that `review-concurrency` was not triaged onto, it emits a one-line "concurrency triage gap" note so the orchestrator can launch `review-concurrency`. **Test-review agents** (review test quality and coverage gaps): | Agent | Launch when | |---|---| | **review-test-quality** | Always launched (unless `docs-only` or `build-config` are the ONLY categories) | | **review-test-structure** | Any test files are changed (reviews isolation, readability, setup/teardown of test code itself) | | **review-test-concurrency** | `concurrency` is present on any changed file (production or test) | | **review-test-crash-safety** | `crash-durability` | `review-test-quality` carries both the behavior sub-protocol (whether tests verify real behavior, assertion depth) and the completeness sub-protocol (corner cases, boundary conditions); it keeps both the `TB` and `TC` finding prefixes verbatim. Categories from **both** production and test code count for the test-review side — for example, if production code adds a new `synchronized` block but tests don't exercise threading, `review-test-concurrency` should still launch to flag the gap. **Workflow-review agents** (review changes to the workflow machinery itself): | Agent | Launch when | |---|---| | **review-workflow-consistency** | `workflow-machinery` is present — always launched for this group | | **review-workflow-prompt-design** | `workflow-machinery` AND any changed file matches `.claude/skills/*/SKILL.md`, `.claude/agents/*.md`, or `.claude/workflow/prompts/*.md` | | **review-workflow-instruction-completeness** | `workflow-machinery` AND any changed file matches `.claude/skills/*/SKILL.md`, `.claude/agents/*.md`, `.claude/workflow/*.md`, or `.claude/workflow/prompts/*.md` | | **review-workflow-hook-safety** | `workflow-machinery` AND any changed file matches `.claude/hooks/*.sh`, `.claude/scripts/**`, or `.claude/settings*.json` | | **review-workflow-context-budget** | `workflow-machinery` is present — always launched for this group. The agent decides whether the diff affects any of three axes (always-loaded surface, load-on-demand discipline, or instant per-operation consumption) and emits an empty findings list when none are affected. | | **review-workflow-writing-style** | `workflow-machinery` AND any changed file matches `.claude/**/*.md`, root `CLAUDE.md`, or `docs/adr/**/*.md` | The workflow-review agents focus on `.claude/`, root `CLAUDE.md`, and plan artifacts under `docs/adr//_workflow/`. They ignore Java code changes — the code-review and test-review agents handle those. **Complexity never changes which agents this step selects.** Selection is domain-only: a category is present → its agent launches, identically at every per-track complexity level. The per-track complexity tag (read by the workflow's Phase-C callers, not by this standalone skill) moves only the **rigor dial** — what terminates the Phase-C review-iteration loop. Blockers loop until clear at every complexity level. The should-fix depth scales with the tag: `low` never lets should-fix drive iteration, `medium` allows up to three iterations, `high` is uncapped. The uncapped loops terminate by no-progress detection rather than a fixed cap (see `review-iteration.md` § Limits and `track-code-review.md` § Review loop). The floor plus the domain-matched set is **never suppressed** by a low complexity; complexity only shortens or lengthens iteration, never drops a selected reviewer. The standalone `/code-review` skill takes no complexity input and always runs the domain-selected set once. ### 5c: Log your triage decision Before launching agents, output a brief triage summary so the user can see the reasoning: ``` ### Triage summary - **Categories detected**: storage-engine, concurrency, index-data-structures - **Code agents selected**: review-code-quality, review-bugs, review-concurrency, review-crash-safety, review-performance - **Test agents selected**: review-test-quality, review-test-structure, review-test-concurrency, review-test-crash-safety - **Workflow agents selected**: (none — no workflow-machinery changes) - **Agents skipped**: review-security (no network/API/SQL/config/dependency changes) ``` ### 5d: Edge cases The rules below cover combinations not handled by the per-row tables above. Where a combination matches more than one row, the **last matching row wins**. Workflow-machinery cases are listed first because workflow-only diffs are common and short-circuit the entire code/test agent dispatch. **Staged-path normalization (run first).** On a workflow-modifying plan the authored `.claude/...` edits live under `docs/adr//_workflow/staged-workflow/.claude/...` (per `§1.7`) while the live tree stays at develop's state. The Step 5b workflow-review globs name live `.claude/...` paths, so a staged path matches none of them. Three glob-gated reviewers therefore miss and fail to launch: `review-workflow-prompt-design`, `review-workflow-instruction-completeness`, and `review-workflow-hook-safety`. (`review-workflow-consistency` and `review-workflow-context-budget` always run for this group; `review-workflow-writing-style` already fires via its `docs/adr/**/*.md` glob, and since its `.claude/**/*.md` glob also matches the normalized path, it fires regardless of which form this row is evaluated against.) Before evaluating the Step 5b per-agent triggers against the workflow-machinery subset of the diff, normalize each changed path: a path matching the anchored prefix `docs/adr//_workflow/staged-workflow/(\.claude/…)` is replaced by its captured `.claude/…` remainder; the match is anchored after the `docs/adr//` head (the `` segment is variable). A path that does not match this exact anchored prefix passes through unchanged, including one that merely contains `.claude/` lower down. A staged file then evaluates exactly as its live counterpart would, and the three glob-gated reviewers launch on the staged edit. Normalization runs ahead of the Step 5b glob match only; it does not edit the globs themselves and does not change the Step 5a file-set categorization (a staged file is already `workflow-machinery` by the `docs/adr//` rule). - If **the only category present is `workflow-machinery`**: Skip all code-review and test-review agents (no Java code or tests to evaluate). Launch the workflow-review agents selected by the workflow-side rules. - If **the only categories are `docs-only` and `workflow-machinery`** (any mix): Treat as `workflow-machinery`-only — skip code-review and test-review agents, launch the workflow-review group on the `workflow-machinery` files. - If the diff **mixes `workflow-machinery` with production-code or test categories**: launch each group's agents on its in-scope files. Each group is dispatched with a pre-filtered `IN_SCOPE_FILES` list (see Step 6) so cross-contamination is bounded. - If **the only category present is `docs-only`**: Skip all agents. Just report that only end-user documentation changed and no review is needed. - If **the only category present is `build-config`**: Launch only `review-code-quality` (to check for misconfigurations) and `review-security` (to check for dependency changes). Skip all test-review and workflow-review agents. - If **the only category present is `tests-only`**: Launch `review-code-quality` and `review-bugs` (test logic can have bugs too), plus `review-concurrency` if the `concurrency` category is present, plus the full test-review set selected by the test-side rules above. - If **the only categories are `build-config` and `tests-only`** (e.g., a CI tweak plus the test it enables): Launch `review-code-quality`, `review-security`, and the full test-review set. The `tests-only` rule wins over `build-config`'s "skip test-review" because the tests are the substantive change. - If **a file matches no category** (e.g., `.gitattributes`): assign it the `other` category — it does not dispatch any agent. If `other` is the only category present, skip all agents and report that nothing reviewable changed. - If **in doubt** about whether an agent is relevant: **launch it**. False positives (an agent finding nothing) are better than false negatives (missing a real issue). ## Step 6: Dispatch selected review agents Launch the selected agents **in parallel** using the Agent tool. Each agent receives a scope-filtered context: build `IN_SCOPE_FILES` per agent group from the triage categories assigned in Step 5a. - **Code-review group**: files categorized as anything other than `docs-only`, `workflow-machinery`, or `other`. (Files categorized as `build-config` stay in scope — `review-code-quality` and `review-security` need to see `pom.xml` and CI changes per Step 5d.) - **Test-review group**: files that match the agent's individual launch rule plus any file categorized `tests-only`. - **Workflow-review group**: files categorized as `workflow-machinery`. The `IN_SCOPE_FILES` list narrows the agent's focus; the full `DIFF` is still passed so the agent can read context lines around each change. The Tooling section differs by group. Use the **code/test variant** for the code-review and test-review groups, and the **workflow variant** for the workflow-review group. ### Prompt template — code-review and test-review groups ``` Review the following changes from your specialized perspective. ## Review Scope {REVIEW_SCOPE} ## In-Scope Files {IN_SCOPE_FILES} ## PR Description {PR_DESCRIPTION} ## Commit Log {COMMIT_LOG} ## Changed Files {CHANGED_FILES} ## Skip These Files (generated code) - core/src/main/java/com/jetbrains/youtrackdb/internal/core/sql/parser/* - Any files under generated-sources/ or generated-test-sources/ - Generated Gremlin DSL classes ## Tooling Use **mcp-steroid PSI find-usages / find-implementations / type-hierarchy via `steroid_execute_code`, not grep**, for any reference-accuracy question about a Java symbol in this diff (callers/overrides/usages of a method, field, class, or annotation; whether a slot is genuinely unused; whether a renamed symbol still has stale references; for test review, which production methods a test exercises and where else they are called). Grep is acceptable for filename globs, unique string literals, and orientation reads, but the load-bearing answer behind a finding must be PSI-backed when the mcp-steroid MCP server is reachable per the SessionStart hook (`steroid_list_projects` once at the start confirms the open project matches the working tree). Fall back to grep with an explicit reference-accuracy caveat in the finding only when mcp-steroid is unreachable. See `CLAUDE.md` § MCP Steroid → "Grep vs PSI — when to switch" for the full routing rule. ## Diff The full diff is at: {DIFF_FILE} Read it with the `Read` tool before forming findings. For diffs over 2000 lines, read the file in chunks using the `offset` and `limit` parameters. Do not infer diff content from {CHANGED_FILES} alone — the file list does not show what changed inside each file. ``` ### Prompt template — workflow-review group ``` Review the following changes from your specialized perspective. ## Review Scope {REVIEW_SCOPE} ## In-Scope Files {IN_SCOPE_FILES} ## PR Description {PR_DESCRIPTION} ## Commit Log {COMMIT_LOG} ## Changed Files {CHANGED_FILES} ## Tooling PSI does not apply — these are markdown, shell, and JSON files, not Java symbols. Use `Read` on the in-scope files and on any referenced skill, agent, hook, or output-style file. Use `Grep` for "is this string mentioned anywhere else in the repo" questions. The Java-targeted mcp-steroid PSI rule does not apply to workflow-machinery review. ## Diff The full diff is at: {DIFF_FILE} Read it with the `Read` tool before forming findings. For diffs over 2000 lines, read the file in chunks using the `offset` and `limit` parameters. ``` ### Handling missing fields If `PR_DESCRIPTION` is the literal string `"No PR associated with these changes."` or `COMMIT_LOG` is the uncommitted-changes sentinel, treat the field as carrying no signal. Do not infer requirements from the absence. The 16 possible agents (launch only those selected in Step 5): **Code-review agents:** 1. **review-code-quality** — code quality, conventions, readability 2. **review-bugs** — bugs, logic errors, null safety, resource leaks, RID handling, state-machine / lifecycle (single-threaded sequential reasoning) 3. **review-concurrency** — races, visibility / publication, lock-ordering / deadlock, compound-op atomicity (multi-thread interleaving reasoning) 4. **review-crash-safety** — WAL correctness, durability, crash recovery 5. **review-security** — injection, auth, data exposure, dependencies 6. **review-performance** — algorithmic complexity, allocations, lock contention, I/O **Test-review agents:** 7. **review-test-quality** — behavior-driven quality (assertion precision, exception testing) plus completeness (corner cases, boundary conditions, test data quality) 8. **review-test-structure** — isolation, independence, readability, documentation 9. **review-test-concurrency** — concurrent behavior testing quality 10. **review-test-crash-safety** — crash/recovery test quality, production assert statements **Workflow-review agents:** 11. **review-workflow-consistency** — cross-file references, threshold sync, hook wiring, recipe paths, glossary drift 12. **review-workflow-prompt-design** — prompts-as-prompts-to-an-LLM: description discriminability, deterministic decision rules, clean-context invocation, sub-agent delegation annotations 13. **review-workflow-instruction-completeness** — branch coverage, gate resume paths, sub-agent input/output handshake, error recovery, loop termination 14. **review-workflow-hook-safety** — shell hygiene, `/tmp` collision safety, hook performance, secret hygiene, JSON schema validity 15. **review-workflow-context-budget** — always-loaded surface (descriptions, CLAUDE.md, SessionStart stdout), load-on-demand discipline, instant per-operation consumption (sub-agent delegation, output caps, `/tmp` staging, targeted reads) 16. **review-workflow-writing-style** — house-style: AI-tells, banned sentence and analysis patterns, BLUF lead, soft section length cap with template-bound exemptions, repo-anchored voice. Set `subagent_type` to the agent name. The agent's frontmatter declares its model; do not override it from the dispatch call unless the user explicitly asks for a different model. ## Step 7: Synthesize the results After all selected agents complete, produce a unified review report. Do NOT simply concatenate the outputs. Instead: 1. **Map sub-agent severities to synthesized severities.** Most sub-agents emit findings under `Critical / Recommended / Minor`, but several older code-review agents use legacy scales (`review-bugs` and `review-concurrency` share one, `review-crash-safety` and `review-security` each have their own). Translate as: - `Critical` → **blocker** (all agents) - `Recommended` → **should-fix** (most agents) - `Minor` → **suggestion** (most agents) - `Likely Issues` → **should-fix** (`review-bugs`, `review-concurrency`) - `Potential Concerns` → **suggestion** (`review-bugs`, `review-concurrency`) - `Concerning` → **should-fix** (`review-crash-safety`) - `Informational` → **suggestion** (`review-crash-safety`) - `High` → **blocker** (`review-security`) - `Medium` → **should-fix** (`review-security`) - `Low` → **suggestion** (`review-security`) Apply this mapping verbatim. The "do not soften" rule below means: do not demote a sub-agent's blocker-level finding to anything below `blocker`. The only legal override is promoting (e.g., raising a `should-fix` to `blocker` because another sub-agent's blocker-level finding on the same line escalates the severity). 2. **Deduplicate.** Findings merge when they share the same `(file, line-range, root issue)`. Different review dimensions on the same line merge into one finding listing all dimensions. Different lines do not merge. Workflow-review findings on a shell or JSON file may merge with code-review findings on the same file when they describe the same root issue. 3. **Attribute.** For each finding, indicate which review dimension(s) identified it. Use a short label (e.g., `[code-quality]`, `[bugs]`, `[concurrency]`, `[test-quality]`, `[test-crash-safety]`, `[workflow-consistency]`, `[workflow-hook-safety]`, `[workflow-writing-style]`). 4. **Summarize.** Write a brief overall assessment (2-3 sentences). Cover whichever of code, tests, and workflow machinery actually appear in the diff. Do not write about a dimension that produced no findings. ### Handling agent failures and empty output - If a sub-agent returns empty output, errors out, or times out, add it to a `### Failed reviewers` section at the top of the report with a one-line note and continue. Do not block the synthesis. - If a sub-agent emits agent-specific preface sections beyond `Critical / Recommended / Minor` (for example, `review-workflow-context-budget` emits `Always-loaded delta` and `Instant-consumption delta` tables when any axis is affected), propagate them under a `### Reviewer notes` section after the severity blocks rather than dropping them silently. These preface sections are optional: `review-workflow-context-budget` omits them in the no-impact case, and absence is not a failure. - If all selected agents return zero findings, emit an explicit `### All clear` block under the overall assessment instead of an empty report. - After the synthesized report is produced (whether with findings or `### All clear`), remove the temp diff file from Step 3: `rm "$DIFF_FILE"`. The file is no longer needed once every sub-agent has returned. ### Output format ```markdown ## Review: {REVIEW_SCOPE} ### Failed reviewers [Omit if none. One line per failed agent.] ### Overall assessment [2-3 sentences. Cover whichever of code, tests, and workflow machinery produced findings — skip the dimensions that returned clean.] ### Blockers [Must fix before merge] 1. **[Dimension]** `path/to/file.ext` (line X-Y) - **Issue**: ... - **Suggestion**: ... ### Should-fix [Should fix before merge] 1. **[Dimension]** `path/to/file.ext` (line X-Y) - **Issue**: ... - **Suggestion**: ... ### Suggestions [Recommended improvements] 1. **[Dimension]** `path/to/file.ext` (line X-Y) - **Issue**: ... - **Suggestion**: ... ### Reviewer notes [Agent-specific preface sections propagated verbatim. Omit if none.] ### Questions for the author [Clarifying questions aggregated from all reviewers. Omit if none.] ``` If a priority level has no findings, omit it entirely. If all priority levels are empty, replace them with `### All clear` and a one-line note. ## Important rules - **Always use `gh` CLI** for GitHub API calls, not WebFetch. - **All selected agents must run in parallel** — do not wait for one before launching the next. - **Only launch agents selected by the triage step** — do not launch agents for irrelevant dimensions. - **Do not add your own review findings** — only synthesize what the agents report. - **Do not soften or dismiss agent findings** — keep `Critical` at `blocker`, `Recommended` at `should-fix`, `Minor` at `suggestion`. Promotion is allowed when another sub-agent's higher-severity finding on the same line argues for it. - **If the diff is very large** (>500 files or >50,000 lines), abort and ask the user to scope the review more narrowly (by directory, module, or commit range). The file-based diff hand-off in Step 3 lifts the per-prompt limit, so the practical ceiling is the per-agent context window — only very large reviews need user-side scoping. - **Standalone command**: This command uses the same dimensional review agents as the Phase 3 workflow but with a different context structure (PR description and commit log instead of implementation plan and track file). Severity scale uses the same blocker/should-fix/suggestion levels as the workflow (see `.claude/workflow/review-iteration.md`).