Reviewkit

reviewkit

Review code an AI agent just wrote, for the failure modes that are specific to AI-generated changes. A generic "find bugs" pass misses the three things agents get wrong most: writing code that is correct in a vacuum but wrong for this repo, padding a change with plausible-looking cruft nobody asked for, and quietly leaving part of the job undone. reviewkit runs those checks first, then a correctness pass, and reports findings ranked by severity. It does not fix anything. Fixing is the human's call, or a handoff to an implement-style skill.

This is a reviewer, not an editor. It reads the change and judges it; it never edits source. Its one optional artifact is a review report the user can save to feed a PR description.

Review with fresh eyes

The reviewer should not be the agent that wrote the change. reviewkit usually fires in the same session that just produced the code, which means the reviewer arrives carrying every rationalization it made while writing: the shortcut it already justified, the edge case it already decided didn't matter. That is the single biggest way this review turns into a rubber stamp.

So when you have a subagent tool, delegate the passes to a fresh subagent: hand it the diff, the stated intent, and the passes below, and let it report back with no memory of the implementation session. Review its findings, then present them. When no subagent tool is available, run the passes yourself but say plainly in the report that this was a self-review, because the user needs to know how much weight the verdict carries. Never quietly self-review a change you just wrote.

The one thing that survives either mode: judge the code that is actually on disk, not the code you remember intending to write.

When this fires

After an agent (or the user) finishes a chunk of work and wants it judged before it ships: "review this", "review my changes", "review the diff", "check this code", "self-review before I commit", "/reviewkit", or a bare "does this look right" after a coding session. It reviews uncommitted work or a branch's diff, not a line-by-line audit of the whole codebase.

It is distinct from a generic correctness linter: reviewkit leads with convention-fit and agent-slop passes that a bug-focused review skips. If the user only wants correctness bugs, say so and run just that pass; don't pad the report with the other two.

Procedure

1. Pick the review target

Ground the review in an actual diff, because reviewing from memory is worthless. Detect the target from git state, then state your pick and let the user override:

• Uncommitted changes present (git status --porcelain is non-empty) → review the working tree: git diff HEAD (include staged with git diff --staged). This is the default after a fresh coding session. git diff does not show untracked files, so a brand-new file an agent never staged is invisible to it, and a whole new module silently escaping review is the worst possible miss. List them with git status --porcelain (the ?? entries) or git ls-files --others --exclude-standard, and Read each one in full as part of the change: every untracked source file, no exceptions. The only untracked files you may skip are ones nobody wrote: lockfiles, build output, vendored dependencies, compiled assets, snapshots. Note them as generated, spot-check that they're actually generated rather than hand-edited, and move on.
• Clean tree, branch ahead of its base → review the branch diff. Get the base branch from gitkit, which owns that resolution. Don't assume main, and don't re-derive it here; a wrong base silently yields an empty diff or one containing half the repo's history, and both look like a real review target. Then run git diff <base>...HEAD and git log <base>..HEAD --oneline for intent.
• If the invocation names a target ("review the branch", "review my staged changes") → honor it directly, skip detection.

Say which target you chose and why in one line, then proceed. If neither applies (clean tree, no branch ahead), ask what to review rather than guessing.

Validate before reviewing. Confirm the target resolves (git rev-parse <ref> for a named base) and the diff is actually non-empty. If the ref doesn't resolve or the diff is empty, stop and say so, because reviewing a bad or empty range produces fabricated findings, not a review.

Read the diff in full before judging anything, once. The four passes below are four questions asked of one reading, not four readings; don't re-run git diff at the top of each pass. If a pass needs a detail you didn't retain, pull that hunk (git diff HEAD -- <path>), not the whole diff again. The same applies to the neighboring code you compare against in Convention-fit: read the sibling that establishes the pattern, not every file in the directory.

Note the change's stated intent, from the commit messages, the branch name, the plan or issue it references, or the user's own words, because half the review is asking "did it do what was asked, all of it, and only that?" Capture that intent concretely: it's the spec both Requirement-completeness and the scope-creep check in Agent-slop signatures measure against.

Ground rules for every pass. Apply these throughout; they are what separate a real review from noise:

• Skip what tooling already enforces. Don't report formatting, import order, or lint rules a formatter/linter/CI catches on its own. The whole value of this review is what machines miss. Spend it there.
• Every finding needs evidence. Quote the offending hunk and name what it violates: a specific line of a repo convention doc, the stated intent, a real API signature, or the concrete failing input. A finding you can't back with a quote is a guess; drop it. This is the primary guard against inventing findings.
• Hold each finding to a confidence bar before it ships. Ask what would have to be true for this to be wrong, and report it only when the answer is confirmed by the code, a reproduction, or a test run, or strongly supported with no credible innocent explanation. "Probably a problem, but I couldn't check the runtime behavior" clears the bar only if you say which part you couldn't check. Anything weaker is not a finding. It is an unverified area, and it belongs in the coverage note in Report the findings rather than in the findings list. Downgrading a hunch to "unverified" is a real result; dressing it up as a bug wastes the reader's time and burns their trust in the whole report.
• Track what you could not judge. As you go, keep a running list of areas the review didn't genuinely cover: a concurrency path you can't reason about statically, a security boundary whose auth model lives outside the diff, generated files you spot-checked rather than read, a dependency whose real API you couldn't verify offline. Silence reads as "clean," so an unexamined area must be named, not omitted.

2. Pass 1: Convention-fit

Does the change look like the rest of this repository wrote it? Agents default to generically-correct code that ignores local idiom. For each changed file, compare against its neighbors, because the surrounding code is the spec:

• Naming & structure. Do new names, file layout, and module boundaries match sibling files? An agent that names a helper getUserData in a repo full of fetch_user stands out.
• Established patterns. Does the repo already have a way to do this (an HTTP client, an error type, a logging helper, a test factory) that the change reinvents instead of reusing? Grep for prior art before accepting a new abstraction.
• Idiom & style. Error handling, async style, imports, formatting conventions the linter doesn't catch. Match what's there, not what's "best practice" in the abstract.
• Dependencies. Did it add a library for something the repo already solves, or that the project's conventions forbid? Check the manifest and existing imports.
• Stated conventions. If the repo documents its conventions (a CLAUDE.md, an agent-guide file, a CONTRIBUTING.md, or a style guide), read it and hold the change to it. A documented repo rule always overrides a general "best practice."

Named smells that show up here (Fowler's vocabulary, so use the label when it fits, since it's sharper than a paragraph): Mysterious Name (unclear naming), Shotgun Surgery (one logical change smeared across many files), Divergent Change (one module edited for several unrelated reasons).

3. Pass 2: Agent-slop signatures

Hunt the tells of machine-generated code, the padding that looks productive but earns its keep nowhere:

• Over-engineering. Abstraction for a single caller, config knobs nothing sets, layers of indirection a direct call would replace, "future-proofing" for requirements that don't exist.
• Dead & unreachable code. Helpers never called, branches that can't execute, exports nothing imports, variables assigned and never read.
• Hallucinated or wrong APIs. Calls to methods, flags, or fields that don't exist on the real type; a plausible-sounding function from the wrong library version. Verify against the actual dependency, not the model's guess.
• Redundant comments. Narration that restates the code (// increment i), docstrings that add nothing, commented-out code left behind.
• Scope creep. Changes outside what was asked: unrelated refactors, reformatting untouched lines, drive-by renames, version bumps nobody requested. Flag anything the stated intent doesn't justify.
• Fake robustness. try/catch that swallows errors, defaults that hide failures, tests that assert nothing or are tautological, TODOs standing in for real handling.

Named smells that show up here: Speculative Generality (abstraction for needs that don't exist, so delete it and inline until a real second caller appears), Duplicated Code (the same logic pasted across hunks instead of shared), Primitive Obsession (a bare string/int standing in for a domain concept), Middle Man / Message Chains (layers that only delegate, or a.b().c().d() navigation). Speculative Generality especially is the signature agent smell.

4. Pass 3: Requirement-completeness

Agents under-deliver as often as they over-deliver: they stub a branch, skip an edge of the ask, or solve the easy 80% and leave the rest silently unfinished. Measure the change against the intent captured in Pick the review target, meaning the plan, issue, or user's words, and report the gaps:

• Missing requirements. Something the ask called for that the diff doesn't do at all.
• Partial implementation. A requirement handled for the happy path only, one case of several, or the interface without the behavior.
• Stubs & placeholders left in. TODO/FIXME/throw new Error("not implemented")/pass/empty handlers presented as if the work were done.
• Wrong interpretation. The change does something, but not the thing that was asked; it solved a nearby, easier problem.

If there's no captured intent to measure against (no plan, issue, or clear request), say so and skip this pass rather than inventing a spec. Don't guess at requirements the user never stated.

5. Pass 4: Correctness

Now the classic review, on what survives the earlier passes:

• Logic errors. Off-by-one, inverted conditions, wrong operator, mishandled return values.
• Edge cases. Empty/null/missing input, boundary values, unicode, large payloads, the error path as well as the happy path.
• State & concurrency. Race conditions, mutation of shared state, ordering assumptions, idempotency, resource cleanup (files, connections, locks).
• Security. Injection, missing authz/ownership checks, secrets in code or logs, unsafe deserialization, unvalidated input crossing a trust boundary.
• Tests. Do the changed tests actually exercise the change, and do they pass? Run the repo's test command if one is obvious and cheap; report what you ran and what happened.

A passing test suite is not evidence of a tested change, because agents write tests that pass because they assert nothing that could fail. Hold each new or changed test to these:

• Would it fail against a broken implementation? Mentally invert the logic it covers, or break a boundary value. If the test still passes, it's decoration. This is the single sharpest test-quality question.
• Does it assert observable behavior, or does it re-implement the production logic in the assertion and compare the code to itself?
• Is the scenario visible in the test, or buried in setup/fixtures/mocks so thoroughly that the test proves the mock works and nothing else?
• Does it cover the failure paths the change introduced, not just the happy path the feature demo walks?
• Does it prove the stated requirement, or an easier neighbor of it?

6. Report the findings

Print the review inline, findings ranked by severity so the reader triages at a glance. Tag each:

• 🔴 Blocker. Must fix before this ships; a real bug, security hole, missing requirement, or broken convention that will bite.
• 🟡 Should-fix. A genuine problem worth addressing, not release-blocking.
• 🟢 Nit. Polish, style, minor slop; take it or leave it.

For each finding give, in this order: the location as file:line (clickable), which pass caught it (convention / slop / completeness / correctness), a quote of the offending hunk and what it violates (the convention doc line, the stated requirement, the real API, or the failing input, per the evidence ground rules in Pick the review target), what's wrong in one sentence, and the concrete fix. If a pass found nothing, say so; a clean pass is a real result. Never invent findings to look thorough. If you can't quote the evidence, it's not a finding. An empty report on a clean diff is the honest outcome.

Structure the report as: the verdict line, the findings most severe first, then the coverage note.

The verdict follows from the findings; it is not a vibe. Pick it mechanically so a reader can trust it without re-reading the list:

• needs-work. One or more 🔴 Blockers.
• ready-with-fixes. No Blockers, but at least one 🟡 Should-fix.
• ready. Nothing above 🟢 Nits. Nits never hold up a change.

Never soften a verdict because the change is mostly good or the author worked hard on it; never harden one to look rigorous. If a Blocker is real, the change needs work even if everything else is excellent.

Close with the coverage note: one short paragraph naming what this review did not verify, meaning the unverified areas collected during the passes, plus whether you ran the tests and whether this was a fresh-eyes review or a self-review (per Review with fresh eyes). A reader who knows the concurrency path went unexamined can go look; a reader who assumes it was covered cannot. When a gap is serious enough that a real problem could be hiding in it (an unreviewed security boundary, a migration nobody can validate here), say the change is blocked on outside review rather than issuing a verdict the evidence doesn't support.

Do not edit source or apply fixes. If the user wants the fixes made, hand off: they run an implement-style skill, or fix by hand and re-run reviewkit.

7. Optional: save the report

After showing the review, offer to save it (don't save unprompted). If the user wants a durable copy, e.g. to paste into a PR description, write it to docs/reviews/review-<branch-or-feature-slug>-YYYY-MM-DD.md, using a short lowercase kebab-case slug and the review's ISO creation date (for example, review-auth-refactor-2026-07-23.md). Keep that date stable if the same report is edited. For a genuine same-day collision between distinct reviews, make the slug more specific; only as a last resort insert a sequence immediately before the date (review-auth-refactor-02-2026-07-23.md). Create docs/reviews/ if needed. Keep the saved file identical to what you printed, with a one-line header noting the date and the reviewed range. If there's no filesystem, skip this step and leave the inline report as the deliverable.

Notes

• Read-only by contract. reviewkit runs git and read/search commands to understand the change, and at most the repo's own test command to check correctness. It never edits source, never commits, never pushes. Its only write is the optional report file in Optional: save the report, and only when the user asks for it.
• Skill is in the tool list for gitkit, and for nothing else. reviewkit calls gitkit to resolve the base ref when it reviews a branch diff, because a wrong base yields an empty diff or half the repo's history, and both look like a real review target. The implement-style skill named in the findings hand-off is routed to, never invoked.
• Scale to the diff. A one-line fix gets a quick pass-through and a one-line verdict; a large feature branch gets the full treatment. Don't pad a small change with ceremony.
• Not a substitute for tests or CI. It's a judgment pass on top of them, tuned for how agent-written code fails. Report what the automated gates already cover as covered; spend the review on what they miss. This is the same reason the review-target ground rules skip tooling-enforced rules.
• No shell or git? Ask the user to paste the diff and the original ask, then run the four passes on what they provide and print the report as a codeblock for them to save themselves.