# Convention Review

> Review a diff against THIS repo's OWN written rules — CONVENTIONS.md / CLAUDE.md / CONTRIBUTING.md read at runtime, never a memorized rule set — plus correctness, before calling work done. Reports most-severe first with file:line, the section actually read, and a concrete fix. Triggers on review, soát/soát lại, check diff, "review giúp", "check convention", code review, "trước khi done", or after finishing a feature/bugfix.

- Skill: `acevinh/convention-review` (Agent Skill)
- Install (CLI): `npx skillmds@latest add acevinh/convention-review`
- Raw SKILL.md: https://api.skillmd.com/api/skills/acevinh/convention-review/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: acevinh (https://skillmd.com/u/acevinh)
- Updated: 2026-09-22
- Page: https://skillmd.com/skills/acevinh/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**:

| Source | What you take from it |
| --- | --- |
| `<repo>/CONVENTIONS.md` | the full spec — folder layout, naming, import rules, banned patterns |
| `<repo>/CLAUDE.md` | the short "must-not-break" list, usually the highest-value rules |
| `<repo>/CONTRIBUTING.md` | contribution/gate rules when the other two are absent |
| the workspace/root equivalent | anything cross-package: shared contracts, wire formats, codegen |

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)`

| Severity | Means |
| --- | --- |
| 🔴 **Blocker** | breaks a contract/consumer, a real correctness bug, or violates a hard "never do X" rule |
| 🟠 **Should-fix** | convention violation with real cost — will be paid for later |
| 🟡 **Nit** | style/naming; list briefly, never pad the report with these |

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.