Convention Review

convention-review

Review a diff against the rules this repo actually wrote down, not against conventions you remember from other codebases. Every finding must be traceable to a line you read in a rule doc during this review, or to a plain correctness argument. That is the whole point: the skill stays correct as the repo's docs evolve, and it is safe to hand to a team whose conventions you've never seen.

The one rule that outranks everything else: if a repo has no rule doc, say so and review on correctness only. Never invent a convention, never import one from another project, never soften "the docs don't say" into "the docs probably mean".

0. Scope the diff (don't review the whole repo)

Decide what to review, in this order:

1. The user named a PR / branch / file(s) → use that.
2. Else the working-tree + staged diff: git -C <repo> diff HEAD, git -C <repo> diff --staged, plus untracked files from git -C <repo> status --short.
3. Else the branch delta vs the default branch: git -C <repo> diff <default-branch>...HEAD.

Review only changed hunks and the code they touch — not untouched files. State which scope you picked.

1. Detect which repo/package the diff touches — load THAT repo's rules

Changed paths tell you where you are. A diff can span several packages in a monorepo, or several repos in a workspace; each one has its own rules. For every one touched, read its rule sources now:

Rules for this step:

• Read them in this session. A rule you didn't open is a rule you can't cite.
• The docs win over this file. If a doc contradicts what you expected, the doc is right.
• Missing docs → say which ones are missing, and downgrade the review to correctness only.
• Docs that contradict each other → report the contradiction as a finding; don't pick a side silently.

2. Review axes — in this order (blast radius first)

a. Contract / API-boundary changes — highest blast radius

Anything crossing a boundary that another package, repo, or client consumes: a public function signature, an exported type, an HTTP route or its response shape, a DB schema or migration, an event/message payload, a generated client, a config key, an env var. These break code that isn't in the diff, so they come first.

For each boundary change ask:

• Who consumes this shape, and is the consumer in this diff too? If not, the change is a break.
• Does the repo have a codegen or schema-sync step that must be re-run after this change? Was it?
• Does the actual serialized shape match what the consumer expects — field names, casing, nullability, units? Verify the wire shape, not the source declaration; re-shaping between the two is common and invisible in the type.
• Is a new surface registered wherever registration is required (export list, route table, generated output, schema file)? A surface that exists but isn't registered silently doesn't exist.

b. Repo conventions — every finding cites a doc section you read

Walk the diff against the rules you loaded in step 1. Cite them as CONVENTIONS.md §<section> — "<the rule, short quote>". Typical territory (check what this repo actually says, never assume the list):

• module/layer boundaries and allowed import direction
• file, folder, and symbol naming
• where types/DTOs/enums/constants are allowed to live
• banned patterns (import styles, escape hatches, untyped values, auto-generated artifacts)
• error handling and logging shape
• config/env declaration requirements
• generated code that must never be hand-edited

If a diff pattern feels wrong but no doc forbids it, it is not a convention finding. Either argue it as correctness or drop it.

c. Plain correctness — independent of any doc

null/undefined and error paths, off-by-one and pagination bounds, await/promise handling, race conditions, wrong field or casing on the wire, guards and permission checks, resource cleanup, silently swallowed failures. Prefer bugs that produce a concrete wrong output over style opinions.

d. Pattern-era check (avoid false positives)

A repo mid-refactor holds two generations of the same pattern. Before flagging a file as "inconsistent", confirm which era that file belongs to — the neighbouring files and the rule docs decide, not the older pattern you saw first.

3. Run the repo's real gates — evidence, not assertions

Find the gate commands in the repo's own docs or package.json scripts. Run only what the change needs, and paste the actual output:

pnpm lint <changed files>          # or npm run lint / the repo's linter
<your typecheck command>           # e.g. npm run typecheck
npm test <changed spec>            # only if logic changed

• Never claim a gate passed without showing its output.
• A gate you can't run → say which, and why.
• Use the repo's configured invocation. Don't override its flags, worker counts, or memory settings to make it pass.

4. Report — ranked, actionable, honest

Most-severe first. One line per finding:

severity · <repo>/<file>:<line> — one-line defect → concrete fix (cite the section or the argument)

End with a one-line verdict: clean to mark done, or N blockers / M should-fix left. If nothing survives the bar, say so plainly — inventing findings to look thorough is a failure mode, not thoroughness. Report; don't auto-fix or commit unless the user asked for fixes.

5. Say what the gates can't see

Green gates are a floor, not proof. Type-checkers and linters cannot see a drifting layout, a clipped panel, copy that states something false, a loading state that doesn't resemble its page, or a workflow that now takes two clicks where it took one. Every one of those ships past a green build.

So before you say "clean to mark done", add one line naming the area this diff touches and the risk it creates in the running app — appended to the repo's testing inbox/ledger if it keeps one (follow that file's format), otherwise stated in your reply. If behaviour genuinely needs checking now, say so instead of implying the gates covered it.