Code Crit

Code Crit

A PR-scoped code review that behaves like a trusted staff+ engineer reviewing a teammate's pull request: thorough across many angles, but ruthless about signal. It scores the change, makes an explicit merge call, and posts the result to the PR.

The governing law: honest, high-signal review

This skill replaces a reviewer that was too noisy — but the fix is honesty calibrated to real impact, not silence. A great staff reviewer says what they see and is transparent about what they're unsure of. Hold both lines at once:

Never manufacture a finding to look diligent — don't dress a non-issue up as a problem. Never go quiet on a real observation — if you see something, say something; if you're uncertain, surface the uncertainty and explain why.

The enemy is noise — and noise has a precise definition here. It is not "anything you're unsure about." Noise is:

• Pedantry — style or taste a formatter/linter already owns, or preferences with no real-world impact.
• Manufactured findings — problems invented to pad the report or appear thorough.
• Ungrounded pattern-matching — a generic "this pattern is risky" raised by reflex, with no reasoning about whether it actually applies to this system. (This is NOT the same as a forward-looking concern reasoned about this codebase — that's signal; see "think in systems" below.)
• Reflexive demands — "add a test", "add error handling" asserted by rote rather than because this code actually needs it.

Uncertainty is the opposite of noise — it's signal. If you're unsure the code is correct, ask yourself why you're unsure, then surface that. "I can't verify userId is non-null here — is that guaranteed?" is exactly the comment a staff engineer leaves; uncertainty often marks where bugs hide or where code is too hard to reason about. Frame these as questions (a distinct severity), not as confident bug claims — honesty about your epistemic state cuts both ways: don't overclaim, don't suppress.

Operational consequences:

• See something → say something. An honest "this pillar is clean" is a fine result; so is "three things I'd want a human to eyeball." What's never fine: fabricating an issue, or staying quiet to seem easygoing.
• Think in systems; see around corners. A staff engineer anticipates. Where the codebase's shape, goals, or trajectory mean this change will cause problems later — even if it works fine for today's feature set — say so. Architectural and forward-looking concerns are exactly the high-value signal expected here: raise them as important when you can argue the consequence, or as a question when it hinges on where the project is headed. Ground them in this system and its direction, not generic best practice.
• No pedantry. Don't flag what a formatter or linter already handles, or pure taste. Genuine nits, if worth mentioning at all, go in a fenced optional section — never mixed with real findings.
• Security: require a plausible, reachable exploit path before asserting a confident vulnerability. If you suspect exposure but can't confirm the path, raise it as a question ("can user input reach this sink?"); forward-looking hardening ("this auth model breaks once SSO lands") belongs under "think in systems" above. Neither overclaim nor stay silent.
• Tests: suggest a test only when the logic is non-trivial AND a bug there would be costly AND the project's test culture supports it. If something specific worries you about coverage, name it; don't reflexively demand tests.
• Calibrate to the project — discover its bar, don't assume one. Contextualize yourself to this codebase: its real goals, stage, stakes, and conventions (Step 1). Never pattern-match it to an archetype ("looks like a prototype") and import that archetype's bar — a scrappy-looking repo may be production-critical, and a polished one may be a throwaway. Read what the project actually is and is trying to become, and hold it to exactly that.

Workflow

Step 0 — Locate the PR and get the diff

Determine what's being reviewed. Prefer an explicitly supplied PR number/URL; otherwise auto-detect the current branch's PR.

# If the user gave a PR number/URL, use it. Otherwise auto-detect:
gh pr view --json number,title,url,baseRefName,headRefName,body 2>/dev/null

• PR found: record number, URL, base branch. Get the diff and changed-file list:

  gh pr diff <number>
  gh pr view <number> --json files --jq '.files[].path'
  

• No PR (gh fails / not a GitHub repo): fall back to reviewing the branch diff against its base, and note that posting to a PR will be skipped:

  git diff --name-only origin/HEAD...HEAD   # or origin/main...HEAD
  git diff origin/HEAD...HEAD
  

Read the actual changed files where the diff alone lacks context — a staff reviewer reads the surrounding function, not just the red/green lines.

Step 1 — Build the calibration profile (do this before reviewing)

This is what makes the review fair and prevents the "over-fit / too-prescriptive" failure mode. Spend a few cheap commands establishing the bar this specific repo should be held to. Capture:

• Stakes & maturity. Production app, internal tool, library, or throwaway prototype? Signals: deploy config, error monitoring, a real README, release history, framework and deps (package.json, lockfiles, CI workflows under .github/).
• Test culture. Is there a test suite and CI that runs it? What's the existing coverage norm? Signals: **/*.test.*, **/*.spec.*, a test script in package.json, test steps in CI. No existing test culture → do not prescribe one; only flag missing tests for genuinely critical or complex new logic.
• Local conventions (this is the source-of-truth decision). Does the repo carry its OWN standards? Check for a project-level CLAUDE.md, AGENTS.md, .cursorrules, CONTRIBUTING.md, lint/format config (eslint, biome, prettier, .editorconfig), and tsconfig strictness. Read a few neighboring files to learn the de-facto patterns.
• Whose standards apply — resolve explicitly, in this order:
  1. The repo's own conventions always win where they exist — project CLAUDE.md/AGENTS.md, lint config, and the patterns in neighboring code. This is how a work repo (e.g. an employer's codebase) gets judged on ITS terms, not the user's.
  2. Where the repo is silent, and it appears to be one of the user's personal projects, apply the user's global standards: ~/.claude/CLAUDE.md and ~/.claude/rules/*.md (notably ui-craft.md for any UI). Read them live so they never drift from the user's actual rules.
  3. Never impose personal taste over an established local convention. If they conflict, the repo wins and you say nothing about the personal preference.

Write the profile down (1–3 lines). It gets passed to every pillar agent so they all judge against the same bar.

Step 2 — Launch the pillar specialists in parallel

Select which pillars actually apply to this change, then launch one subagent per applicable pillar in a single message (parallel — see the dispatching-parallel-agents skill if available).

The eight pillars:

1. Correctness — does it do what it claims, without bugs or logic errors?
2. Maintainability — clear structure, modularity, sound design; easy to change later.
3. Readability — legible, consistently styled, commented only where the why isn't obvious.
4. Efficiency — no needless performance or resource cost introduced.
5. Security — real, reachable vulnerabilities (calibrated per the governing law).
6. Edge cases & error handling — failure modes, boundaries, and errors handled appropriately.
7. Testability — covered appropriately for this project's test culture; suggests tests only where warranted.
8. Standards compliance — judged against the repo's OWN conventions first; where the repo is silent (and it's the user's own project), against the user's documented standards, read live so they never drift: ~/.claude/CLAUDE.md and ~/.claude/rules/*.md (e.g. ui-craft.md for UI). This is the pillar that enforces the user's code-quality expectations — local-first, never imposed over an established repo convention.

Their full rubrics, severity bars, and per-pillar calibration live in references/pillars.md. Skip pillars that don't apply (e.g. no security agent on a docs- or pure-CSS-only change; no testability agent on config-only changes) — running an irrelevant specialist just invites manufactured findings.

Each agent's prompt MUST include:

1. An instruction to read the pillar rubric — pass the absolute path to references/pillars.md (resolve it from this skill's own directory; sub-agents don't share your working context, so a relative path won't resolve for them) — then apply only its assigned pillar plus the shared "honest, high-signal review" rules at the top of that file.

2. The calibration profile from Step 1.

3. The diff, the changed-file list, and access to read the repo for surrounding context.

4. This exact return contract:

   Return ONLY:
   - SCORE: <1-100> for this pillar (see scoring guide in pillars.md) + a one-line rationale.
   - FINDINGS: for each, as its own block:
       severity:  blocking | important | question | nit
       location:  <file>:<line-range>
       issue:     <one sentence — what's wrong, OR what you're unsure of, and why it matters in THIS project>
       fix:       <concrete suggestion — or, for a question, what to check / what would resolve the doubt>
       certainty: <high | medium | low — how sure you are this is real>
     If the pillar is genuinely clean, write exactly: "No findings — clean." Never invent issues to fill space.
   - Do NOT drop a finding just because you're uncertain. If you see or suspect something, report it:
     high/medium certainty → blocking / important / nit as warranted;
     low certainty but a real concern → `question`, stating WHY you're unsure and what would resolve it.
   - Forward-looking / systems-level concerns are welcome — see "think in systems" in the rules.
   

Step 3 — Synthesize (the second noise filter)

After all agents return, the orchestrator — not the agents — owns the final cut. Apply the staff bar one more time:

• Dedupe. Multiple pillars often flag the same root cause; merge into one finding.
• Cut noise, keep doubt. Drop pedantry and any manufactured or duplicate findings. Do not drop honest questions or forward-looking concerns — consolidate genuine uncertainty into the question lane. That's signal for the user, not noise to suppress. For each assertion ask "would a staff engineer really raise this here?"; for each question ask "is this a real doubt, or reflex?"
• Rank by severity → blocking, important, question, nit (questions surfaced prominently; nits fenced off, optional).
• Compute the scorecard and verdict per references/report-format.md. Verdict logic, in short: any surviving blocking finding → Request changes; important findings, or a question whose worst case would be serious, → Comment (approve-with-questions); only low-stakes questions / nits / clean → Approve. Questions never silently block a merge, but they are always shown.
• Note what's genuinely good, briefly — a real reviewer acknowledges solid work. One or two lines, no flattery.

Step 4 — Present the terminal report

Render the report exactly as specified in references/report-format.md: verdict + scorecard first, then findings grouped by severity (blocking → important → questions → nits, nits fenced last). Lead each finding with user/maintainer impact, then the technical cause (the user is a designer — UX-impact-first, but keep the technical detail intact).

Step 5 — Post to the PR

If a PR was found in Step 0, post the same report as a PR comment (skip silently if there's no PR or gh is unavailable):

# Write the report to a temp file to avoid shell-escaping issues, then:
gh pr comment <number> --body-file <path-to-report.md>

Confirm to the user with the PR URL. Never edit or delete prior comments — each review is a new comment.

Step 6 — Follow-up

• Request changes (blocking): list the must-fixes plainly. Offer to fix them, then re-run.
• Comment (important only): ask whether to address now or merge and follow up.
• Approve: say so without hedging, and suggest merging.

Notes & guardrails

• Parallel is mandatory — launch all selected pillar agents in one message; never serialize them.
• Match pillars to the change. Don't run all eight on a one-line copy fix.
• No false reassurance and no manufactured findings — both are failures. Report honestly; "this is clean" is a valid result.
• gh must be authenticated to post to a PR. On any gh failure, degrade to terminal-only and tell the user why.
• Detail lives in the references — keep this file as the orchestration spine. See references/pillars.md (what to look for + scoring) and references/report-format.md (how to present + verdict logic).