review agent skill

SKILL.md

name	review
description	Exhaustive, evidence-driven code review of the current branch's pull request. Surfaces every actionable issue (correctness, reliability, performance, maintainability, and architecture), walks the findings one at a time to collect a decision on each, then implements the chosen fixes with regression tests where a harness exists. Requires an open PR on the current branch. Use when the user asks to review, audit, or critique a PR.
user_invocable	true
disable-model-invocation	true
argument	Optional additional context for the review (focus areas, concerns, background). Does not change scope — the review always covers the current branch's PR.

Review

Perform an exhaustive, evidence-driven code review of the pull request for the current branch. Find all concrete issues worth acting on, from critical bugs through nits, inconsistencies, maintainability problems, and architectural concerns, then walk the user through findings one at a time, collecting a decision on each before moving to the next. Only after every finding has a decision do you implement the chosen fixes in a single batch.

Review ledger

Each PR has a persistent review ledger at memory/pr_reviews/<pr-number>.md in the current repo root. This file is the source of truth for findings, decisions, and fix outcomes across review sessions. Create memory/pr_reviews/ if it does not exist. Ledger files are intentional review history — commit them when a session produces meaningful decisions or fixes; do not gitignore them.

On every run

1. After resolving the PR number in Step 1, read memory/pr_reviews/<pr-number>.md if it exists.
2. Load all prior findings, decisions, and statuses into working memory before reviewing or presenting anything. For ledger entries with status: fixed, skipped, or withdrawn, remember the id and outcome only — do not re-triage their text unless reopening.
3. Do not re-present findings whose status is already fixed, skipped, or withdrawn unless fresh code review shows the issue is still present — in that case, add a new finding (new id) or set the existing one to status: reopened with an updated note.
4. Continue finding ids from the ledger (F1, F2, …). Never reuse or renumber ids.
5. Update the ledger after each decision (Step 5) and after each implemented fix (Step 6). Keep it on disk at the end; never delete it.
6. Resume shortcut: If any finding has status: decided and none are pending or reopened, skip Step 5 and go straight to the Step 5 recap → user confirmation → Step 6.

Ledger format

Use this structure (YAML frontmatter + markdown sections):

---
pr: 275
title: PR title from gh pr view
branch: feature/foo
base: main
repo: owner/repo
---

# PR Review #275

2 Critical, 5 Warning, 2 Nit | 6 fixed, 4 skipped, 2 pending | last session 2026-05-30

## Sessions

### 2026-05-30T14:00:00-07:00

Context: optional user argument for this session. Brief note on what happened this session.

## Findings

### F1: Short title

- **severity:** Critical | Warning | Nit
- **architecture:** true | false
- **confidence:** high | medium | low
- **location:** path/to/file.swift:42 (or area for cross-cutting issues)
- **description:** what is wrong
- **evidence:** short quote or pointer
- **impact:** user-visible or maintenance impact
- **recommendation:** preferred fix
- **alternatives:** optional; up to two
- **test_plan:** optional regression test to add/update
- **decision:** empty until decided; then the chosen action (e.g. apply recommendation, alternative, skip)
- **decision_notes:** why the user chose it; freeform notes from the session
- **status:** pending | reopened | decided | fixed | skipped | withdrawn
- **fixed_at:** ISO timestamp when fix landed; omit if not fixed
- **fix_summary:** what changed to resolve it; omit if not fixed
- **source_thread_id:** optional `PRRT_…` from `queries.sh threads` when this finding was surfaced from an open inline review thread; omit otherwise
- **source_review_id:** optional `PRR_…` from `queries.sh reviews` when the finding came from a top-level review body; omit otherwise
- **source_comment_id:** optional issue-comment id from `queries.sh comments` when the finding came from a PR conversation comment; omit otherwise

Keep YAML frontmatter to static PR metadata only (pr, title, branch, base, repo). Log sessions in ## Sessions (newest first). Update the rolling summary line under the title after each session — format: <severity counts> | <status counts> | last session <date>. Add new findings under ## Findings. Update fields in place as decisions and fixes happen.

PR feedback as context

PR review feedback is read-only context by default — the same role as prior ledger entries. Use it to understand what has already been raised, addressed, or closed on the PR. Do not reply to, resolve, or minimize on GitHub except in the narrow case documented in Close handled PR feedback below.

Intake path: run each subcommand with --all (paginate with --after when truncation says so):

~/.agents/scripts/queries.sh truncation
~/.agents/scripts/queries.sh threads --all
~/.agents/scripts/queries.sh reviews --all
~/.agents/scripts/queries.sh comments --all

--all includes resolved threads and minimized review bodies / issue comments — treat these like ledger entries with fixed/skipped/withdrawn status: load them for dedupe and history, do not re-raise unless current code regressed.

Do not use broad PR comment sources that bypass the script: gh pr view --comments, unfiltered GraphQL, or full conversation dumps.

How to use feedback:

• isResolved: true / isMinimized: true — already handled on GitHub; remember the concern and outcome, skip re-raising unless the code regressed.
• Open (isResolved: false, not minimized) — verify against current code. If already fixed in the branch, skip silently. If you disagree with the comment after verification, skip silently — do not re-raise, argue, or add a counter-finding. If still valid and your own analysis agrees, you may add a new ledger finding when not already tracked; record source_thread_id (or source_review_id / source_comment_id when the feedback is not an inline thread).
• Do not adopt review comments as a todo list; the review must stand on its own reading of the code. Surfacing a thread-linked finding in Step 5 is not adopting the whole PR comment queue — only findings you present and the user accepts may be closed on GitHub later.

Step 1: Resolve the PR

The review scope is always the current branch's PR. Do not interpret the user's argument as scope — it is supplemental context only (focus areas, concerns, background, or questions to keep in mind while reviewing).

1. Confirm the current branch with git branch --show-current.
2. Resolve the PR with gh pr view --json number,title,baseRefName,headRefName,url,isDraft,mergeable,reviewDecision,statusCheckRollup (no branch argument — uses the current branch). If no PR exists, stop and tell the user this skill only works on branches with an open PR. Record the PR number, title, base branch, and repo from this output. Note draft status, merge conflicts, failing checks, and other blockers before deep review.
3. Load the review ledger from memory/pr_reviews/<pr-number>.md if it exists; otherwise create a new ledger file with frontmatter for this PR, a rolling summary line, an empty ## Sessions section, and an empty ## Findings section.
4. Get the diff with gh pr diff (and read the changed files for context). If the hosted diff is noisy, use the local base diff such as origin/<base>...HEAD, where <base> comes from the PR metadata.
5. Read any free-form argument the user passed as additional review context — not as a scope selector.

Step 2: Gather Context

Before judging code, understand the project:

1. Read project instructions and conventions such as AGENTS.md, README, and top-level config files.
2. Check repository state with commands like git status --short, git branch --show-current, and the PR diff/log (gh pr view, gh pr diff, or the local base diff).
3. Read the changed files with surrounding context, not just the patch.
4. Note the PR title, description, and any linked issues for intent and contract expectations.
5. PR feedback (context only) — see PR feedback as context. Run truncation first; then threads --all, reviews --all, and comments --all (paginate each with --after when its *HasMore is true). Read only enough to dedupe; do not write to GitHub during intake.
6. Identify the test framework and the smallest useful test commands for the affected areas. When fixes are likely, run those tests once to confirm a green baseline before relying on them for the test-driven fixes in Step 6.
7. Apply any supplemental context from the user's argument (e.g., "pay extra attention to auth" or "we're worried about race conditions") while keeping the review scope limited to the PR diff and its related call sites.
8. Cross-check the ledger: note which findings are still pending, reopened, or decided but not yet fixed. Ignore ledger entries already fixed, skipped, or withdrawn unless the code regressed.

Step 3: Review Deeply

Review focus

Prioritize production code.

• Use tests only as context: how behavior is supposed to work, or whether production code is untested — do not nitpick test style, helpers, mocks, or coverage mechanics.
• Findings whose primary location is test-only should be rare. Report them only when the test documents incorrect expected behavior, hides a production defect, or asserts something false about the product.
• When production code and tests both change, review the production change first; mention test gaps only if they block confidence in a production finding.

Look for both targeted defects and broader design issues.

Targeted review categories (short cues, not exhaustive checklists):

• Correctness: logic errors, bad state transitions, invalid assumptions, missing edge cases, wrong API usage, data/persistence corruption, broken migrations.
• Concurrency and async: races, deadlocks, actor/thread isolation, TOCTOU, cancellation leaks, lock ordering, unsafe shared state.
• Reliability and error handling: swallowed errors, missing validation, weak retry/accounting, bad fallbacks, missing observability for important failures.
• Performance and scalability: N+1 queries, redundant expensive work, poor data structures, unbounded memory or task growth, hot-path regressions.
• Compatibility and integration: platform/version mismatches, API or schema contract drift, feature-flag mistakes, environment-specific failures.
• Dependencies, build, and release: unsafe dependency or lockfile changes, build/packaging/signing risks, missing rollout or rollback.
• Coupling and maintainability: implicit ordering, stringly-typed interfaces, duplicated logic, unclear boundaries, over-complex control flow.
• API ergonomics and foot-guns: misleading names, surprising defaults, easy-to-misuse patterns, hidden ordering requirements, weak invariants.
• Tests and tooling: missing production coverage for changed behavior, or CI/build risks that affect shipped code — not test-file style, helpers, mocks, or coverage mechanics for their own sake.
• User experience and accessibility: confusing flows, broken empty/loading/error states, focus/keyboard issues, accessibility or localization regressions.
• Documentation and contract consistency: stale comments or docs, misleading PR descriptions, tests or docs that encode a different contract than the code.
• Cleanup: dead code, unused imports, inconsistent naming, unreachable branches.

Architectural observations:

• Missing or blurred boundaries between layers.
• Modules that own too many responsibilities or live in the wrong place.
• Repeated patterns that should become a shared abstraction.
• Data paths, state models, or workflow contracts that will become painful as the codebase grows.
• Strategic simplifications that reduce risk without broad churn.

Examine changed production behavior through its call sites, data flow, and tests (tests as context only). If a suspected issue is only theoretical, verify user reachability and realistic timing before presenting it as a finding.

Step 4: Build Findings

Collect all new findings before presenting them. Skip issues already recorded in the ledger with a final status unless the code still exhibits the problem.

Do not duplicate context you already have: Skip raising a finding when the issue is already tracked in the ledger (fixed/skipped/withdrawn unless reopened) or when PR feedback (open or already resolved/minimized on GitHub) describes the same concern and current code already addresses it. PR comments are not action items.

Give each finding a severity, ranked by real impact:

1. Critical: data loss, crash, severe correctness regression, broken production path.
2. Warning: likely user-visible bug, race, reliability gap, significant performance issue, brittle contract.
3. Nit: naming, small cleanup, local duplication, minor maintainability issue.

Independently, tag a finding Architecture/Design when it concerns boundaries, responsibilities, shared abstractions, or data/workflow contracts rather than a single localized defect. This tag is orthogonal to severity — a design issue can be anything from Critical to Nit — so order findings by real impact and never demote one just because it is architectural.

Each finding must include:

• Severity, the Architecture/Design tag when it applies, a short title, and confidence (high, medium, or low) when the issue is plausible but not certain (common for concurrency, performance, and reachability arguments).
• Exact file paths and line numbers for localized issues; for architectural or cross-cutting findings, cite the relevant files, ranges, or module paths instead.
• A clear description of the broken behavior or design problem.
• Evidence from the code, with short snippets only when useful.
• The user-visible or maintenance impact.
• A recommended fix and, when there are real tradeoffs, up to two alternatives.
• A test plan when the recommended action changes behavior worth testing and a usable test harness exists: name the red-green regression test to add or update. Keep this in the finding body, not crammed into a choice label.

Be complete but honest: include every concrete new issue in the PR — bugs, inconsistencies, inefficiencies, duplication, brittle contracts, foot-guns, maintainability problems, architectural drift, missing tests, and useful cleanup — and rank them truthfully by severity rather than filtering them out. Deprioritize or omit findings that live only in test/fixture/CI files unless they indicate a production bug or a false contract about product behavior. Do not pad the review with speculative or low-value comments, and if an earlier suspected issue turns out to be false, set status: withdrawn in the ledger. If the PR contains no new actionable issues and no pending ledger items need decisions, say so plainly and stop; never invent findings to fill the review.

Write new findings to the review ledger (memory/pr_reviews/<pr-number>.md) with status: pending and empty decision fields. Also include any ledger findings still pending, reopened, or decided (not yet fixed/skipped) in the queue for Step 5.

Step 5: Present the Review — Interactively, One Finding at a Time

This step is strictly about collecting decisions, not implementing them. Do not edit code, write tests, or run fixes during Step 5. Implementation happens in Step 6, after every finding has a decision.

Present only findings whose status is pending or reopened. Always walk them one at a time, highest impact first. Never dump the full list for batch triage, and never collapse nits into a group — one finding at a time is the only mode, no matter how many findings there are.

Interactive flow

For each finding awaiting a decision, in order:

1. Show the finding in a short message: id and number (F2 — Finding 2 of 5), severity, confidence when not high, and the Architecture/Design tag if it applies, title, file path and line number (or area), the issue and its user-visible or maintenance impact, and your recommendation with a one-sentence reason. When the finding has a source_thread_id (or other source id), say so explicitly (e.g. "Matches open reviewer thread on path:line"). If the recommended action warrants a regression test, describe that test here in the finding body.
2. Ask the user to choose what to do with this finding. Use the richest interactive prompting mechanism available — if your environment supports buttoned multiple-choice prompts, use them; otherwise present a short numbered list inline and ask the user to reply with a number or a freeform answer. Whichever surface you use, keep the option labels short and always include:
   • The recommended action as the first option, clearly marked (Recommended).
   • Up to two additional concrete alternatives when real tradeoffs exist.
   • A Skip option so the user can decline this finding.
   • Make clear that the user can also respond freeform (override the options, ask for more detail, or stop).
3. Record the user's decision in the ledger: set decision, decision_notes, and status: decided (or status: skipped if they skip). Do not start fixing yet.
4. Move on to the next finding and repeat.

If the user challenges a finding, re-check the relevant code before asking for a decision — don't defend stale analysis. If the challenge holds, set status: withdrawn with decision_notes explaining why the finding was wrong; do not ask the user to skip a finding you no longer believe. If the finding still holds, update evidence and re-ask. If the user asks for more detail, expand inline before re-asking. If the user says stop, abandon the remaining findings and skip to Step 6 with whatever has been decided so far.

After every finding has a decision

Before moving to Step 6, present a short recap of the decisions collected — one line per finding (F2: <title> → <chosen action>), grouped by "Will fix" vs "Skipped". Ask for a final confirmation to proceed with implementation. Only after the user confirms, move to Step 6.

Step 6: Implement All Selected Fixes

Now implement every fix the user chose, working from the ledger. Apply accepted Architecture/Design changes first when they would subsume or reshape smaller fixes; otherwise work highest impact first (Critical → Warning → Nit). Keep each change focused on its finding; do not bundle unrelated cleanup.

For any selected fix that changes runtime behavior, data contracts, user-visible behavior, persistence, API semantics, or other logic worth testing, use red-green TDD wherever a usable test harness exists or can reasonably be created. This includes bug fixes, correctness issues, crashes, race conditions, data integrity problems, and any requested non-defect change whose new behavior should be pinned down:

1. Write or update a regression test before editing production code.
2. Run the smallest test scope that includes the regression test.
3. Confirm the test fails for the right reason: the failure must reproduce the reviewed defect, not a typo, missing import, setup error, or unrelated failure.
4. Record the relevant failure output so the fix can be verified against it.
5. Only after the red test is confirmed, edit the production code.
6. Re-run the regression test and confirm it passes.
7. Run the broader relevant test suite, formatter, linter, or type checker for the changed area.

Where a usable harness exists, do not skip the failing-first step for defects. If no harness exists or red-green is genuinely impractical for a given fix, note that explicitly and proceed rather than blocking the fix on impossible tests. If a test that should fail cannot be made to fail for the right reason, stop and explain the blocker before writing production code.

Documentation, naming, and mechanical cleanup do not need failing-first tests unless they expose behavior.

After implementing each fix, update the ledger: set status: fixed, fixed_at, and fix_summary. For skipped findings, ensure status: skipped and decision_notes capture why. Briefly note in chat which finding it resolved so the user can track progress against the recap. When all selected fixes are done, update the ledger rolling summary, then Close handled PR feedback for any fixed findings that qualify, and proceed to Step 7.

Close handled PR feedback

Close the loop on GitHub only for feedback this session explicitly tied to a ledger finding, presented in Step 5, accepted by the user, and fixed in Step 6. This is the only write path for PR comments in this skill. Mechanics, IDs, and minimize rules: ~/.agents/scripts/queries.sh closing-guide.

Qualifying findings

All must be true:

1. The finding has source_thread_id, source_review_id, or source_comment_id recorded at intake (ids from queries.sh with --all during Step 2).
2. You presented it in Step 5 and the user chose to fix it (not Skip / withdrawn).
3. The fix is on the branch (status: fixed in the ledger) — push first if the reply should reference commits on the remote.
4. The source author is jubishop or coderabbitai (same allowlist as queries.sh). Ignore all other authors.

Do not close when you skipped the finding, disagreed with the comment, fixed without Step 5 acceptance, or invented the finding without a recorded source id.

Close checklist

At the start of Step 6 (or when first linking a finding to a thread), record every source id you may close later. Only close ids on this checklist — not threads or summaries that appeared after your push.

1. queries.sh check-writes — if it fails, note in the close-out that GitHub feedback was left open; do not use the user's token.
2. For each qualifying source_thread_id: write the reply body to a file, then queries.sh thread-reply --thread … --body-file …, then queries.sh thread-resolve --thread ….
3. For qualifying source_review_id / source_comment_id from coderabbitai only: queries.sh minimize --subject … per closing-guide.
4. queries.sh open-count (paginate with --after when threadsHasMore is true).

Leave a jubishop thread open when the fix needs owner follow-up or the thread is ambiguous.

Step 7: Close Out

1. Fixed — one line per finding: id, title, what changed, tests touched; note any linked source_thread_id closed on GitHub (or skipped because bot token was missing).
2. Skipped / withdrawn — declined or withdrawn findings; no re-litigation.
3. GitHub feedback — which checklist thread ids were replied to and resolved; which coderabbitai summaries were minimized, if any; anything left open intentionally.
4. Validation — what tests, linters, formatters, or type checks ran and what did not run or failed.
5. Next steps — suggest a commit message if fixes were made; commit only if the user asks; otherwise confirm the review session is complete. Commit the ledger file when it contains meaningful decisions or fixes from this session.

Rules

• PR-only. This skill does not review staged changes, unstaged changes, arbitrary commits, file paths, or the whole repo. If there is no PR on the current branch, stop immediately.
• The user's argument is supplemental context only — never use it to narrow or widen scope beyond the PR.
• The review ledger at memory/pr_reviews/<pr-number>.md persists across sessions; read it at the start and update it throughout. Treat ledger files as intentional review history — commit them when meaningful.
• PR comments are context by default. Load feedback via ~/.agents/scripts/queries.sh with --all; do not write to GitHub during intake. Disagreement with an open comment → skip silently. The only write path is Close handled PR feedback after Step 5 acceptance and a Step 6 fix for a finding with a recorded source id.
• Respect dirty worktrees. Do not revert or overwrite unrelated user changes.
• Read enough surrounding code to avoid patch-only false positives.
• Prefer the project's existing patterns and test style.
• Keep remediation options concrete and scoped.
• Surface uncertainty clearly when evidence is incomplete.