Code Review

Directly conducts finder jobs against diffs. Handles input parsing, context gathering, finder-job lifecycle, and result synthesis.

Premises (apply to orchestrator AND finder jobs)

These two premises are non-negotiable. They are forwarded through every finder-job prompt and govern every decision in this skill.

1. Post-change state — The working directory reflects the post-change state of the target ref. PR mode achieves this by checking out the PR head into a dedicated linked worktree (see Step 0). Non-PR modes (branch comparison, auto-detect) achieve this by verifying HEAD-match + clean-tree on the current working directory (also Step 0). Either way: read code freely from the working directory — the diff is the delta, the working directory is the result. Do not pretend the file system is read-only or stuck at base.

2. No diff-only review — A diff is a delta. The unit of review is the system the diff produces. Always trace dependencies, callers, callees, interfaces, configurations, and runtime context across files. If you cannot explain how the changed code behaves end-to-end against the surrounding system, you have not reviewed it.

These premises must be reflected in the finder-job prompt — see Step 4. Review is static-only: do not run tests, builds, linters, formatters, migrations, or other project execution. The job lifecycle commands (start, collect, status, results, clean, and usage-summary) are allowed.

Input Modes

# PR (number or URL)
/code-review pr 123
/code-review pr https://github.com/org/repo/pull/123

# Branch comparison
/code-review main feature/auth

# Auto-detect (current branch vs origin/main or origin/master)
/code-review

Caller-supplied scope contract

When project_context contains a complete valid [SCOPE_CONTRACT]/[/SCOPE_CONTRACT] JSON envelope, apply it regardless of artifact filename or gate. It contains outcome, verification_surface, constraints, boundaries, non_goals, stories, and scope_contract_sha256; parse it before deriving requirements and preserve it unchanged through finder dispatch, direct candidate verification, cards, and the artifact. A delimiter appearing without a valid pair, or a request explicitly requiring a contract that omits or damages it, is INCONCLUSIVE; never fall back to ordinary review. A request with no contract retains ordinary behavior. stories are caller-provided approved requirement entries and workflow state is caller-owned.

The contract and confirmed stories define authorization. Artifacts, codebase analogs, review comments, and derived expected-items may explain approved behavior; they cannot add acceptance criteria, revise boundaries/non-goals, or authorize collateral repairs. Tracing surrounding code is required evidence gathering, not permission to repair everything reached.

Finders suppress scenarios fully explained by a declared non-goal before generating candidates. For every generated candidate, the reviewer directly decides scope before quality verdict, including LOW cleanup and documentation findings. OUT_OF_SCOPE reporting applies to generated candidates subsequently excluded by the reviewer; it does not require searching for unrelated work. The reviewer checks both the symptom and a concrete proposed remedy. A real CSV defect can authorize a local CSV correction without authorizing a generic export framework. A pre-existing legacy billing defect remains unrelated even when severe. A change-introduced regression may authorize minimal restoration/rollback of the prior behavior; it never authorizes enhancement of an excluded subsystem. If restoring an approved invariant needs new excluded behavior or a new capability, return UNKNOWN for a user scope decision instead of expanding the specification.

Each finding must carry reviewer-authored fields:

{"scope":"IN_SCOPE | OUT_OF_SCOPE | UNKNOWN","scope_evidence":{"basis":"requirement | regression | non_goal | unrelated | uncertain","reference":"<contract key or confirmed story id>","rationale":"<specific symptom, change causality, and remedy boundary>"}}

reference identifies outcome, verification_surface, constraints, boundaries, non_goals, or a confirmed story id. IN_SCOPE requires basis requirement or regression and evidence that the remedy fits authorization; OUT_OF_SCOPE requires non_goal or unrelated; UNKNOWN requires uncertain. Severity and confidence do not establish scope. Report IN_SCOPE, OUT_OF_SCOPE, and UNKNOWN independently with their evidence, including CONFIRMED and PLAUSIBLE quality verdicts. Do not convert a scope result into a repair, adjudication, completion, budget, or approval decision; those decisions belong to the caller.

Do vs Delegate Decision Matrix

Action	YOU Do	DELEGATE
Requirements 3-question gate	Yes	-
Diff range determination & git	Yes	-
Findings synthesis (rank/class verified findings)	Yes	-
Individual candidate judgment inline (Phase 2)	Yes	-
Individual finder review	NEVER	configured finder CLIs through direct jobs
Code modification	NEVER	(forbidden entirely)

The reviewer directly verifies candidates in every mode, including reviews with a scope contract.

Role Separation

Your role as orchestrator:

• Start the direct finder job with a diff command string (the job fans out the configured angle finders)
• Judge each deduped candidate inline in Phase 2 (code and scope evidence → verdict + enrichment); enrich kept findings directly
• Synthesize the kept findings into a ranked findings report (text only)
• Rank the verified findings (no merge verdict — this review reports, it does not gate)

NOT your role:

• Modifying any source files
• Running a general raw git diff command (the Step 3 candidate-scoped integrity exception is the only text-inspection carve-out)

Context Budget

Allowed in orchestrator context:

• ["git", "diff", range, "--numstat", "-z", "--find-renames"] output
• ["git", "diff", range, "--stat"] output
• ["git", "diff", range, "--name-only", "-z", "--no-renames"] output
• ["git", "diff", range, "--numstat", "-z", "--no-renames"] output
• ["git", "diff", range, "--name-status", "-z", "--find-renames"] output
• --no-renames name-only/numstat output is inventory/accounting only
• ["git", "log", range, "--oneline"] output
• CLAUDE.md file content
• chunk-reviewer results (candidate findings)
• Phase 2 inline judgment output (reasoning, verdicts, enriched findings for every candidate)
• Code reading via Read/Grep for Phase 2 inline candidate judgment

The orchestrator never inspects, loads, or displays diff text as general raw-diff review input. The candidate-scoped diff inspection exception in Step 3 is for integrity judgment only: for each candidate derived file, stream its complete candidate-scoped diff to the prescribed out-of-band digest/byte-count sink and compare only bounded integrity evidence with authored source/generator evidence. Its diff result is not forwarded to a finder prompt, candidate aggregation, or general orchestrator context. This exception does not permit project tests, builds, linters, formatters, migrations, or other project execution, and it does not relax the ban on general raw diff text. The separate prescribed binary git diff --no-ext-diff --binary ... stdout byte stream flows directly to SHA-256 outside model context for diffFingerprint and an out-of-band byte-count sink; stderr is excluded and a nonzero exit aborts. Finder jobs execute the review diff from the prompt.

Step 0: Input Parsing

Environment setup runs first — resolve the range and, in PR mode, check out the post-change code into the worktree before any code-reading step (intent acquisition, the derived-context sub-step, chunk-review). Every downstream step reads the working directory, so the working directory must already hold the post-change state.

Determine range and setup for subsequent steps:

Input	Setup	Range
pr <number or URL>	Fetch and check out PR ref into the worktree (see below)	origin/<baseRefName>...pr-<number>
<base> <target>	Verify HEAD is <target> via git rev-parse --abbrev-ref HEAD; verify clean tree via git status --porcelain -uno. Abort if mismatch or dirty.	<base>...<target>
(none)	Detect default branch (origin/main or origin/master). Verify HEAD is the target branch via git rev-parse --abbrev-ref HEAD; verify clean tree via git status --porcelain -uno. Abort if mismatch or dirty.	<default>...HEAD

PR Mode: Worktree Checkout (per Premise 1)

This skill assumes the orchestrator is already running inside a worktree dedicated to this review (the caller is responsible for creating the worktree). Therefore: fetch the PR ref AND check it out. The working directory must reflect the post-change state of the PR so that all subsequent code reading (Phase 2 verification, chunk-reviewer Step 2) sees the actual code under review.

PR ID extraction rule: When the user supplies a URL (https://github.com/<org>/<repo>/pull/<N>), extract numeric <N> from the trailing path segment and substitute it for <number> before entering the bash block below. Substituting the URL itself makes git fetch origin pull/<URL>/head fail with an invalid refspec and git checkout -B pr-<URL> fail with an invalid branch name.

set -euo pipefail

# 0. Safety guards (Premise 1 enforcement) — abort BEFORE any state change
# -uno: untracked files are preserved by checkout; only check tracked modifications
if [ -n "$(git status --porcelain -uno)" ]; then
  echo "Error: working directory has uncommitted changes — refusing to checkout over the user's work" >&2
  exit 1
fi
# Distinguish primary repo from linked worktree.
# In a linked worktree, --git-dir points inside .git/worktrees/<wt>,
# while --git-common-dir points to the shared .git directory; they differ.
# In a primary clone they are equal — refuse so we never checkout over the user's main work tree.
if [ "$(git rev-parse --git-dir 2>/dev/null)" = "$(git rev-parse --git-common-dir 2>/dev/null)" ]; then
  echo "Error: refusing to run in primary repo — create a dedicated linked worktree first (Premise 1). Hint: 'git worktree add ../review-pr-<N> -b review/pr-<N>'" >&2
  exit 1
fi

# 1. Get base branch name (abort if gh fails or returns empty)
BASE_REF=$(gh pr view <number> --json baseRefName --jq '.baseRefName')
if [ -z "$BASE_REF" ]; then
  echo "Error: gh pr view returned empty baseRefName — aborting before any fetch" >&2
  exit 1
fi

# 2. Fetch base branch first, then PR ref last so FETCH_HEAD points to PR head
#    (rerun-safe — force-push on the PR is picked up on re-review)
git fetch origin "${BASE_REF}"
git fetch origin pull/<number>/head

# 3. Reset local pr-<N> to the freshly fetched PR head (FETCH_HEAD) and check it out
#    so the working directory matches the PR state
git checkout -B pr-<number> FETCH_HEAD

If the working directory is dirty (uncommitted changes) or the caller is not in a worktree, abort and report — do not silently checkout over the user's work. The worktree premise is the safety net; without it, the safety net is gone.

All range formats use three-dot syntax (A...B), which is equivalent to git diff $(git merge-base A B)..B. This shows only changes introduced by the target since the common ancestor — not changes on the base branch. This prevents false positives when origin/main has moved ahead after branching.

All subsequent steps use {range} from this table. All subsequent commands that receive range or path values must use argv-safe arguments; if Bash is required, quote each dynamic value as a separate argument. After checkout, code reading via Read/Grep/Glob reflects the post-change state, which is the intended behavior — diff shows the delta, the working directory shows the result.

Before constructing the review range, verify each raw base and target endpoint with the exact argv ["git", "rev-parse", "--verify", "--end-of-options", "<ref>^{commit}"]. Abort and report on non-zero exit or empty stdout; never treat it as an empty diff. Construct <baseSha>...<targetSha> from verified commit IDs only and use it for every subsequent diff and log command. argv separation alone does not prevent Git option parsing; --end-of-options is required.

Untrusted path rendering

Untrusted path rendering contract: Use the same strict escaped JSON/structured representation for every untrusted report path, including binary paths, derived artifact paths, pre-existing paths, and finding locations. Encode each path as a JSON string, escaping every JSON control character, including newline, plus backslash and double quote; additionally encode backtick, <, >, &, U+2028, and U+2029 as \u escapes. Preserve the encoded value only in an explicitly marked untrusted-data JSON array or structured JSON field. Use this representation whenever a path is surfaced; never raw Markdown/backtick prose, raw file:line, headings, fences, or shell commands. Decoded paths never enter prose, raw output templates, or command strings.

Early Exit

After the range is resolved and (PR mode) the checkout is done, before proceeding to Step 1:

1. Run ["git", "diff", range, "--stat"] (using the range determined above)
2. If empty diff: report "No changes detected (between and )" and exit immediately without collecting this manifest
3. If the stat is non-empty, before deciding whether it is binary-only or reporting any binary-only result, collect a fresh completeChangedFileManifest with ["git", "diff", range, "--name-only", "-z", "--no-renames"] and parse its stdout as NUL-delimited paths. Step 2 has not run yet; do not reference or reuse a Step 2 manifest. If the manifest/stat identifies a binary-only diff (if the diff is binary-only), enumerate every binary changed path under Out of Scope as an explicitly marked untrusted-data strict escaped JSON array of strings (one entry per path) under the Untrusted path rendering contract; do not render a raw path or file:line prose, state that no finder has run and no finder job is dispatched, and then exit

Early Exit runs before intent acquisition on purpose — an empty diff exits immediately without manifest collection; after a non-empty stat, a fresh complete manifest is collected and parsed NUL-safely before binary-only determination/reporting, then binary paths are reported under Out of Scope before any finder runs and the review exits without dispatching a finder job.

Step 1: Intent and Context Acquisition

Intent acquisition is non-negotiable. Either intent is confirmed (from artifacts, interview, or both), or the user explicitly defers to a code-quality-only review. There is no third option — proceeding without intent and without explicit deferral is forbidden. Reviewing without intent produces wrong severities, missed scope creep, and false positives born from misunderstanding the author's goal.

Acquisition order

1. PR/branch artifacts — PR title, description, labels, commit history, code review comments and threads
2. Linked references (recursive) — every link found in the artifacts above, followed transitively until the trail ends
3. Codebase signals — CLAUDE.md, README, ADRs, related history in changed paths
4. User interview — only for what the artifacts cannot reveal

Acquire all reachable references

PR descriptions, commits, and comments routinely link to richer context (issue trackers, design docs, chat threads). Follow every link recursively — a linked ticket may itself link to a doc which links to a discussion thread; keep following until the trail ends.

Do not name specific tools. Use whatever fetch capability the environment provides for each link type. If a link cannot be fetched directly (no credential, no MCP for that platform, network unreachable), do not skip it — mark it for the user interview step.

Sources to consult per input mode:

Input mode	Sources
PR	gh pr view --json title,body,labels,comments,reviews, gh pr view --comments, linked issues, gh issue view <n>, every external link found in the chain, commit messages on the PR branch
Branch comparison	Commit messages, branch name conventions, any linked tickets discovered in commits, related issues
Auto-detect	Recent commit messages on HEAD, any linked tickets found there

User interview — only for what artifacts cannot reveal

After exhausting fetchable sources, ask the user about:

• Intent — what problem is this PR solving and why was this approach chosen
• Alternatives — what was considered and rejected, and why
• Constraints — deadlines, dependencies, compatibility commitments, hidden requirements
• Concerns — known risks, untested paths, areas the author is uncertain about

DO NOT interview the user about codebase facts (file locations, patterns, architecture, who calls what). Use Read/Grep/Glob and the explore agent for those — they are reachable from the working directory.

Intent Block Gate (hard exit condition)

Before exiting Step 1, the state must be one of:

State	Action
Intent confirmed — author's goal, approach, and constraints are understood from artifacts and/or interview	Proceed to Step 2
User explicit deferral — user says "skip", "그냥 리뷰해줘", "없어", "code quality only", or unambiguous equivalent	Set {REQUIREMENTS} = "N/A — code-quality-only review (user deferred)" and proceed
Non-interactive dispatch (completion-gate) — the dispatch prompt itself carries a supplied artifact destination alongside a 5-slot intent payload (what_was_implemented/description/requirements/project_context/non_goals)	Treat as Intent confirmed (non-interactive, no user interview) and proceed to Step 2. Acquisition steps 1-3 (PR/branch artifacts, linked references, codebase signals) still run — they backfill any slot whose value is the (none provided) marker. Only step 4 (user interview) is replaced by the payload. The destination is opaque; do not infer workflow from its basename.
Neither — artifacts thin and user not yet asked, OR user gave vague answers without explicit deferral	BLOCK. Do not proceed. Continue interview until one of the two states above is reached.

There is no "I tried hard enough, just review" path. The block IS the safety mechanism.

A fresh code-reviewer agent has no ambient session to check for an active artifact path — the non-interactive discriminator above is prompt-borne: whether the dispatch prompt includes the supplied destination, not whether a session-scoped artifact happens to exist. The destination is opaque and may use any basename. When the signal is absent, the main-session interactive gate above (Neither → BLOCK) is unchanged.

Vague answer refinement

When the user gives a vague answer that is not an explicit deferral, refine ONCE with a specific follow-up:

Include the deferral option in every follow-up so the user can opt out without already knowing the phrases "skip / 그냥 리뷰해줘 / 없어".

User says	Follow-up
"대충 있어" / "뭐 좀 있긴 한데"	"어디서 찾을 수 있나요? 링크나 문서 위치를 알려주세요. (답하기 어려우면 'skip'으로 코드 품질만 리뷰 가능)"
"그냥 성능 개선이야"	"어떤 지표를 개선하려 했나요? (latency, throughput, memory 등 — 답하기 어려우면 'skip'으로 코드 품질만 리뷰 가능)"
"여러 가지 고쳤어"	"가장 중요한 1-2개만 알려주세요. 나머지는 코드에서 식별하겠습니다. (답하기 어려우면 'skip'으로 코드 품질만 리뷰 가능)"

If refinement still yields a vague answer, surface the block explicitly to the user:

"의도를 명확히 잡기 어렵습니다. 둘 중 하나를 선택해주세요: (1) [구체적 질문]에 답하여 의도 확정, 또는 (2) 'skip / 코드 품질만 리뷰' 명시적 deferral. 둘 중 하나를 명시하기 전까지 리뷰는 시작하지 않습니다."

This is not adversarial — it is refusing to silently produce a worse review.

Question discipline

Situation	Method
2-4 structured choices (review scope, focus areas)	AskUserQuestion tool
Free-form / subjective (intent, alternatives, constraints, concerns)	Plain text question

One question per message. Never bundle. Wait for the answer before the next question.

Question quality — every question must include either a specific anchor (a summary the user can correct) or a default action in parentheses (so progress is possible without an answer):

BAD	GOOD
"요구사항이 있나요?"	"PR 본문과 연결된 이슈에서 [요약]을 추출했습니다. 보완할 부분이 있나요?"
"어떤 부분을 볼까요?"	"23개 파일이 변경됐습니다. 집중할 영역이 있나요? (없으면 전체 리뷰)"

Project Context

Include project context when interpolating the chunk-reviewer prompt template in Step 4. Describe what kind of software this is, who uses it, how it runs, and what depends on it — based on CLAUDE.md, README.md, and the artifacts gathered above.

If available context is insufficient to characterize the project, ask the user once: "What kind of software is this? (e.g., personal CLI tool, internal team service, public-facing API, shared library, etc.)"

Step 1 Exit Condition

Proceed to Step 2 only when the Intent Block Gate state is Intent confirmed or User explicit deferral. Any other state → continue at Step 1.

Bounded derived context (derived expected-items)

Scope-contract exception: Keep the frozen requirements unchanged. Derive only supporting hypotheses about behavior already authorized by a contract key or confirmed story; label them as evidence and keep them separate from the authoritative requirements. An analog, general best practice, or review-derived expectation alone cannot become a requirement-gap or authorize new work. The unconditional rules below apply to other review modes.

By this point {REQUIREMENTS} has settled — via interview, the deferral sentinel, or the completion-gate payload. This sub-step adds one more thing to it: bounded derived context, a codebase-grounded prediction of expected-items, kept distinct from intent acquisition above. "Intent acquisition is non-negotiable" (the Step 1 charter) means received, stated author intent is authoritative; this sub-step instead generates a hypothesis from the codebase's own "Codebase signals" (acquisition step 3) — it does not receive stated intent, it infers from what the codebase already does.

Mirror the same reasoning shape the regression and cleanup finder angles use: name a thing the codebase already establishes, then check whether the change re-establishes it. Here: name a same-role analog already in the codebase, then check whether the change wires the new addition into it the same way. Derive an expected-item only through this named-necessity gate:

State	Condition	Action
Grounded + necessity-named	A citable codebase analog exists with a concrete file:line, AND a concrete runtime consequence of the item's absence can be named	Keep — emit the derived item
Uncertain	Only one of the two holds, or either is fuzzy	Drop
Neither	No citable analog, no nameable consequence	never invent

For each kept item, emit one bullet carrying four fields — self-labeling for provenance, so the downstream requirement-gap finding needs no new field to explain where it came from:

• Analog (file:line) — the existing code whose role the missing item should mirror
• Why same role — why the analog and the missing item play the same structural role
• Expected item absent here — what the analog implies should exist in the changed code, and doesn't
• Runtime consequence of its absence — what breaks, silently or loudly, if it stays missing

Phrase the bullet itself like "Codebase analog at file:line implies <wiring>; absent here" — never "a requirement you stated is absent." The label must stay honest: a same-role analog implies wiring that is absent here, not a stated requirement that is absent.