Code Review

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 <!--Document index start--> to <!--Document index end--> (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.

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:

   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:

   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:

# 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:

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:

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:

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):

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:

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/<dir>/ 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):

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):

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):

The workflow-review agents focus on .claude/, root CLAUDE.md, and plan artifacts under docs/adr/<dir>/_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/<dir>/_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/<any-dir>/_workflow/staged-workflow/(\.claude/…) is replaced by its captured .claude/… remainder; the match is anchored after the docs/adr/<dir>/ head (the <dir> 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/<dir>/ 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

## 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).