Code Review

Code Review

What this skill is - and isn't

This skill reviews and reports. It does not implement fixes. If a fix is obvious and one-line, mention it in the comment as a suggestion - but don't apply it. Hand back to vd:cook / vd:fix for the actual work.

Codex runtime: this file uses Claude-Code tooling (AskUserQuestion, Task(Explore) subagents). Under codex exec use the self-contained codex-review.md in this skill dir instead; per-site fallbacks below also apply.

Modes

Auto-detect from arguments. Ambiguous or empty → AskUserQuestion (Claude Code; plain-text question elsewhere).

Flags:

• --dry-run - print the review payload, do NOT post to GitHub (PR mode only)
• --post - opposite default; force posting even if other flags would skip
• --no-inline - skip inline comments, post only the top-level summary
• --auto - non-interactive, default answers, no prompts
• --ultra - adversarial review via a dynamic workflow: every finding is independently refuted before it ships (see Ultra mode). Higher token cost; use for high-stakes diffs.
• --cross-model - add reviewers from other model families and weigh findings by agreement (see Cross-model mode). Combine with --ultra only for release-critical diffs.
• --refactor - local reuse / composition / slop lens. Never posts. See references/refactor.md. Combine with --fix / --save. Detect from "refactor review", "is this slop", "does this fit the codebase".

--refactor is a two-axis pass when paired with a landing review: run the reuse/slop lens locally first, then the default PR pass. Do not mix the two outputs in one GitHub review.

Hard rules

1. One review = one push to GitHub. Don't dribble out comments across multiple gh calls. Build the full payload, then submit a single POST /pulls/:n/reviews. The author gets one notification, not 12.
2. Inline beats top-level. If a finding maps to a specific file:line, it MUST go inline. Top-level summary is for context, verdict, and orphan concerns that don't anchor to a hunk.
3. No critical claim without evidence. "Critical" means "merge would cause incident / data loss / security hole." If you can't point to the failure mode, downgrade to Important.
4. Encouraging tone, firm content. Lead with what works when it's true. Be direct about problems. Never sarcastic. Never "as an AI…". The author is a peer.
5. Read the file, not just the hunk. Diff context lies. Open the file at the changed lines and 30 lines around to understand surrounding state before commenting.
6. Intent vs mistake. Review whether the change achieves the stated intent, not whether the intent is right. A finding that challenges a deliberate product decision, a named tradeoff, or two valid designs is **Question:**, never Critical/Important, and must not flip the verdict to Request changes. Relay it; let the author/user answer.
7. Stay in the diff. Comment on paths this change touches. Adjacent cleanup is out of scope unless this diff introduces a merge-blocking defect there.

Review voice (the style guide)

These are the conventions for every comment this skill writes. Reference example: careernowbrands/cnb-data-contract#124.

Severity prefix on every finding

Comment shape

**<Severity prefix>**: <one-sentence problem statement>.

<1-3 sentences of evidence: what breaks, when, why it matters. Include the
actual symptom - not just "may cause issues">.

<Optional: suggested change, in fenced block or inline code>.

<Optional: question to author if the fix path is unclear>.

Do:

• Open with the problem, not the praise. (Praise goes in the summary.)
• Quote the offending fragment in fenced code when correcting a specific value.
• Name the failure mode explicitly: "alert silently won't fire", "PII leaks to logs on retry", "locks the table for the duration of the migration".
• Suggest a concrete alternative when you have one: CURRENT_DATE - 1 not "use the right date".
• Cross-link related comments when the issues compound: "combined with the threshold on L81…".

Don't:

• Don't pad with hedges. "It might possibly be a small concern that perhaps…" → "This will fail when X."
• Don't restate the diff. The author already wrote the code.
• Don't moralize ("you should always…"). State the consequence: "this breaks X."
• Don't use emojis or icons (severity badges/PNGs) unless the repo's review conventions already use them. Plain **Severity:** ships everywhere.
• Don't comment on every line. If a file has 5 issues, the worst 2 are usually enough. The author will find the rest when they fix those.

Top-level summary shape

Keep under 8 lines. Structure:

<One sentence on what the PR does + what works>.

<If CI is failing: Critical: CI <job> is red (link/run-id). Block merge until fixed:
exact command to debug>.

<One line pointing at inline comments>: "Inline comments cover the
<correctness / design / perf> concerns."

<Verdict - see below>.

Verdict line - exactly one of:

• Approve - no Critical, no Important. Suggestions OK.
• Request changes - at least one Critical or Important. List the topics.
• Comment - Only Suggestions / Questions / Nits. Author decides.

Tone calibration

• Encouraging when it's earned. "Solid coverage on the happy path" / "Nice catch on the parameterized query - easy to get wrong." Don't fake-praise.
• Direct on real problems. "This deadlocks under concurrent writes." Don't soften with "maybe consider perhaps."
• Curious on unknowns. "What's the intended behavior when X is null? The current branch silently drops it."
• Never condescending. Never "obviously" / "clearly" / "any junior dev would know" / "I'm surprised this passed".

PR mode - the polished path

This is the path that produces the GitHub artifact. Treat it as the primary mode.

1. Fetch context

PR=$1   # accepts "#123", "123", or full URL - normalize to number
gh pr view  "$PR" --json title,body,author,baseRefName,headRefName,files,additions,deletions,state,reviewDecision,headRefOid
gh pr diff  "$PR"
# Exit 8 = pending (retry), not "no checks". Print the table; ignore the rc.
gh pr checks "$PR" || true

Capture headRefOid - every inline comment's commit_id MUST equal this so comments stay anchored if the author force-pushes mid-review.

2. Decide the scope of effort

3. Build the review payload

Collect findings as you go into this structure (memory only - don't write a file):

{
  "commit_id": "<headRefOid>",
  "event":     "APPROVE | REQUEST_CHANGES | COMMENT",
  "body":      "<top-level summary, per the shape above>",
  "comments": [
    {
      "path":      "contracts/constraints/snowflake/five9_old_lead_recency_alert.yaml",
      "line":      44,
      "side":      "RIGHT",
      "body":      "**Important - CURRENT_DATE timing**: ..."
    }
  ]
}

line rules:

• line is the line number in the file at commit_id (the new version for additions, the old version with "side": "LEFT" for deletions).
• For a multi-line comment, use start_line + line. Both on the same side.
• If the targeted line is unchanged context (not in the diff), GitHub rejects the comment. In that case, anchor to the nearest changed line and reference the real line in the body: "Re. L44 (unchanged): …".

4. Post the review

Default behavior:

• PR mode + no --dry-run → post via single gh api call (see below).
• PR mode + --dry-run → print the JSON payload, exit, do not call the API.
• Non-PR modes → print the findings as a markdown report. Never post.
• GitHub rejects APPROVE reviews from the PR author. If the authenticated user is the author and the verdict is Approve, submit the same body as COMMENT and say a different reviewer is required for branch-protection approval.

gh api \
  -X POST \
  "repos/$OWNER/$REPO/pulls/$PR_NUM/reviews" \
  --input - <<< "$PAYLOAD_JSON"

If the call returns 422 with Pull request review thread line must be part of the diff, the comment anchored to an unchanged line. Move it to the nearest diff line and retry - once. Never auto-retry more than once; surface the error to the user.

5. Confirm

After posting, print:

Posted review to PR #<n> as <event>:
  • <count> inline comments
  • Verdict: <event>
  • URL: <html_url from API response>

Ultra mode - adversarial workflow

--ultra swaps the single-context review pass for a dynamic Workflow (Claude Code's Workflow tool) that structurally removes self-preferential bias: the agent that finds an issue is never the one that confirms it. Reach for it when a wrong call is expensive (security-sensitive change, large refactor, release diff) - not for a routine 50-line PR.

How it runs. The template is workflows/adversarial-review.js - read it, adapt DIMENSIONS / votes to the diff, then invoke the Workflow tool with args:

{ "pr": 123, "votes": 3 }        // or { "diffCmd": "git diff", "votes": 3 }

It composes two patterns:

1. Review (fan-out) - one agent per dimension (correctness, security, reliability, performance, api, tests), each with its own clean context. They map onto the Checklist below, so coverage doesn't degrade the way a single long pass does (no "addressed 20 of 50" laziness).
2. Verify (adversarial) - each candidate finding faces votes independent refuters prompted to kill it. Majority-refute drops the finding. Only survivors come back.

Then post as normal. The workflow returns { confirmed, dropped }. Map confirmed into the same review payload (§3) and post the one review (§4) with the usual severity prefixes and voice. Mention the filter in the summary: "Adversarial pass: N findings confirmed, M refuted and dropped." --dry-run still prints instead of posting.

Portability & cost. The Workflow tool is Claude Code-only - in another runtime, fall back to the standard pass and say so. Ultra spends materially more tokens (≈ dimensions × findings × votes agents); the default non-ultra path remains correct for everyday reviews.

Cross-model mode

--ultra removes self-preferential bias; --cross-model removes family bias - models share blind spots with their own outputs and priors, so a second family catches what the first cannot. Adapted from cursor/plugins pstack interrogate (MIT).

How it runs.

1. State the change's intent in one paragraph (from the PR body, commits, or the user). Reviewers challenge whether the work achieves the intent, not whether the intent is right.
2. Run the standard pass here, then send the same diff + intent + the Checklist to one or two reviewers on different model families - codex exec / gemini / opencode run, whichever is on PATH. No personas: the adversarial signal comes from model diversity. A family that is unavailable is reported as skipped, never simulated.
3. Synthesize with lead judgment - you are a pragmatic senior engineer with the full context, not a neutral aggregator. Dedupe, then bucket every finding: act on (correctness/security given the actual goals), consider (legitimate, cost unclear), noted (valid, not actionable now), dismissed (wrong or missing context - say why). Tag each with the families that raised it: 2+ families = high signal; a lone-family finding is read and weighed, never auto-promoted.
4. Post the one review as normal (§4). Summary names the panel: "Cross-model pass: A+B agreed on N, lone-family M, dismissed K."

Cost. One extra full-diff pass per family. Use for security-sensitive, migration, or release diffs - not routine PRs.

Non-PR modes (quick reference)

--pending (local, pre-commit)

git diff + git diff --cached. Apply the same severity/voice rubric. Output to stdout as a markdown report. The author runs this themselves before pushing.

Commit hash

git show <sha>. Same review, no post target. Useful for reviewing someone else's commit after the fact.

codebase / codebase parallel

Out of scope for the polished PR path. Spawn Task(Explore) subagents per top-level dir (no subagents → scan each dir inline, sequentially); each returns a findings list with file:line:severity:body. Synthesize into a single markdown report. Write to the injected Reports: path. Filename: code-review-<date>-<slug>.md.

Checklist (apply to every diff)

This is the always-on lens. Repo-specific rules (i18n, SQL store conventions, mobile UI…) live in the project's CLAUDE.md or docs/code-standards.md - read those first and layer them on top of this list.

Correctness

• Off-by-one, nil/null deref, missing error handling, swallowed errors.
• Race conditions, goroutine leaks, channel ownership, unprotected shared state.
• Edge cases on inputs the author probably didn't test (empty, zero, max, unicode, timezone, DST).

Security

• Injection - SQL, command, template, XSS. Look for string concat or fmt.Sprintf into queries/shells/HTML.
• Hardcoded secrets, tokens, API keys. Anything resembling base64 or hex of ≥32 chars.
• Auth scoping. userID vs tenantID vs sessionID - never trust the wrong one.
• SSRF, path traversal, open redirect on any URL/path read from input.

Reliability

• Retries without idempotency keys.
• Unbounded queues, channels, slices, caches.
• Timeouts: any network call without an explicit timeout is a finding.
• Migrations: any schema change must be reversible OR the PR must call out the irreversibility.

Performance (only if there's evidence, not vibes)

• N+1 queries - loop over IDs each issuing a DB call.
• Full table scans on hot paths - verify WHERE/JOIN/ORDER BY against existing indexes (check migrations).
• Allocations in hot loops (Go: profile-guided only; don't pre-optimize).
• Framework AI-slop: useEffect for data already in render scope, missing key/useMemo on heavy lists, SELECT * then filter in app code, await-in-loop where a batch call exists. Real, common, cheap to flag.

Dependencies

• New dependency for something the stdlib or an existing dep already does → ask "why not the one we have?"
• A heavy/transitive-heavy package for a few lines of logic → flag (supply-chain surface + bundle/binary weight).
• Version bumps that cross a major, or lockfile churn unrelated to the PR's purpose → call out.
• Unpinned/latest or a brand-new low-adoption package on a critical path → security finding, not a nit.

API surface & breakage

• Renamed/removed exports → downstream breaks.
• Changed response shape / status codes → client breaks.
• Config-file schema change → ops breaks. Needs a migration note or backwards-compat shim.

Testing

• New code paths covered? Edge cases tested, not just happy path?
• Tests touching real DB or mocks - match the repo's existing convention. Don't mix.
• Are there tests that would catch the bug if the author re-introduced it tomorrow? If no, ask for one.

Change shape

• Refactor + new behavior in one PR → ask to split. A reviewer can't tell a behavior change from a move when they're tangled, and a bad refactor hides inside the feature diff.
• Dead/zombie code: a function/flag/branch the diff stops calling but leaves behind → flag and ask (it may be load-bearing elsewhere - don't assert "remove it"). Same for commented-out blocks.

Project conventions

• Read CLAUDE.md, docs/code-standards.md, docs/system-architecture.md if present. Apply repo-specific rules (i18n keys in 3 locales, h-dvh not h-screen, parameterized SQL, etc.).
• Calibrate: this is a review, not a rewrite. If the code is correct, safe, and readable, ship it - don't manufacture findings to look thorough. "Different from how I'd write it" is not a finding. Review the change against its stated intent; do not reject a named tradeoff because you'd have chosen differently.

CI handling

When gh pr checks shows failures:

1. Identify the failing job and its run ID.
2. The summary's first finding is **Critical:** CI <job> is red ([run-id](url)).
3. Include the exact command for the author to inspect: gh run view <run-id> --log-failed.
4. If the failure cause is obvious from the diff (e.g. lint, type error, missing import), name it in the comment body. Don't make the author hunt.
5. Verdict is at least Request changes while CI is red. No exceptions.

When the answer is "approve"

It's a real verdict, not a participation trophy. Use it when:

• No Critical, no Important findings.
• Suggestions/Nits are fine to include.
• Tests cover the change.
• CI is green (or only flakes the repo is known to tolerate).

Approval body - keep it short:

Approved. <one sentence on what shipped well>.

<Optional: 1-2 suggestion comments inline>.

Anti-patterns (don't do these)

• Comment dump. 40 comments on a 200-line diff. Pick the worst 5-10.
• "LGTM 🚀". Empty approvals teach nothing and erode trust in your reviews. Either approve with a specific reason, or don't approve.
• Ghost suggestions. "Consider refactoring this." → useless. Either propose the refactor with code, or drop the comment.
• Re-reviewing on every push. If the author pushed a 3-line fix to address your Critical, look at those 3 lines - don't re-review the whole PR.
• Hidden assumptions. "This should use the foo pattern." → name the file, name the pattern, link the prior art.
• Reviewer as designer. Using extra models or Important findings to pick a product path. That's a Question to the user.

Common rationalizations to catch (in the code, and in yourself)

Workflow position

vd:cook  →  vd:code-review (this skill)  →  vd:ship
                                             (or vd:fix if changes requested)

Also fires standalone when the user invokes on a teammate's PR or to review the local branch before vd:ship.