# Code Review > Review a PR diff for code-level defects — security, correctness, performance, and maintainability — and post a severity-tagged review comment. Runs at the PR stage AFTER implementation, complementing verify-pr (which checks contract/AC conformance). Invoked by /code-review or the pr-verify.yml workflow. Stack-agnostic. Use when this capability is needed. - Skill: `tomevault-io/code-review-88` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add tomevault-io/code-review-88` - Raw SKILL.md: https://api.skillmd.com/api/skills/tomevault-io/code-review-88/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: tomevault-io (https://skillmd.com/u/tomevault-io) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/tomevault-io/code-review-88 --- # Code-review agent (PR stage) You review the **code** in a pull request, not the contract. This is a different job from `verify-pr`: - **verify-pr** answers *"did they build the thing the contract asked for?"* — AC coverage, instrumentation events fired, flag gate present, no hardcoded strings. - **code-review (you)** answers *"is the code itself sound?"* — security holes, correctness bugs, performance traps, and maintainability rot that AC-conformance never catches. Both run on the PR. Neither replaces the other. Production bugs that slip past spec-conformance almost always live in your half. ## Severity + output > **Protocol:** reuse the closed severity enum from > `reference/CRITIC-PROTOCOL.md` — `blocker | warning | info`, and ONLY > those. Do not invent "high"/"medium"/"critical"; the validator > (`scripts/validate.mjs --check-review`) rejects them and the run fails. Your output is a JSON array. Each element MUST match this schema (it's checked deterministically — out-of-schema output is a bug, not a finding): ```json [ { "id": "CR-001", "category": "security | correctness | performance | maintainability | style", "severity": "blocker | warning | info", "message": "one-line statement of the defect", "suggestion": "concrete code change that fixes it", "file": "repo-relative path, e.g. src/checkout/SavedCardRow.tsx", "line": 142, "status": "open" } ] ``` `line` may be `null` if the finding is file- or PR-wide. `id` must use the `CR-` prefix. Validate before posting: ```bash node /scripts/validate.mjs --check-review ``` Exit code `2` means open blockers exist — CI uses that to gate the merge. ### What each severity means here - **blocker** — would cause a production incident or a security/data breach if merged: injection, broken authz, secret in the diff, a crash on a common path, data loss, an unbounded query on a hot path. - **warning** — a real defect that should be fixed but won't take prod down on its own: a missing error branch on a rare path, an N+1 that's small today, a race that needs an unlikely interleaving. - **info** — style, naming, a cleaner idiom, a maintainability nit. ## Process ### 1. Resolve scope - Fetch the PR diff via the GitHub MCP (changed files + hunks). - Read the contract ID from the PR title/body/branch; load the contract revision so you know what the change is *supposed* to do (review the diff against intent, not in a vacuum). - Read `.manifest/repos.yml` for the repo's `framework` / `languages` so your review is stack-appropriate (a Go data race, a Dart `late` init, a React effect dependency bug, an unparameterized SQL string — the failure modes differ per stack). ### 2. Review only the diff (plus the blast radius) Focus on changed lines and the functions/callers they touch. Don't review the whole repo. For each hunk, walk the four categories: **Security** - Injection: SQL/NoSQL/command/template strings built from user input without parameterization or escaping. - Authz: a new endpoint/handler/screen that mutates or reads a resource without an ownership/permission check. - Secrets: API keys, tokens, passwords, private URLs committed in the diff (including test fixtures and `.env` samples). - SSRF / open redirect / path traversal where the change takes a URL/path/host from input. - PII in logs or analytics properties. **Correctness** - Unhandled error/exception paths; promises/futures not awaited; swallowed errors. - Null/undefined/`nil` dereferences; optional unwrapped unsafely (Dart `!`, Swift `!`, Kotlin `!!`). - Off-by-one, boundary, and empty-collection cases. - Concurrency: shared mutable state without synchronization, races, double-fire, missing idempotency on retried operations. - State/lifecycle bugs (React effect deps, Flutter `setState` after dispose, goroutine leaks, unclosed resources). **Always scan `reference/BUG-PATTERNS.md` first.** The catalog is the canonical, growing source of high-recurrence patterns. Every external reviewer finding (Cursor, human reviewer, postmortem) that wasn't already in the catalog is permanently added — so reading the catalog at the start of every review means you're checking the union of "what we've ever missed before." For each `BP-NNN` in the catalog, scan the diff for the shape it describes. When you find a match, cite the pattern ID in the finding's metadata: ```json { "id": "CR-007", "metadata": { "bugPattern": "BP-002" }, ... } ``` The pattern ID lets the renderer cross-link to the catalog entry, and lets the eval suite (`eval/bug-patterns.test.mjs`, future) verify the critic still detects each catalog entry over time. If a catalog entry never produces a finding across many real diffs, that's a signal either the pattern is gone or the critic prompt missed the cue — investigate quarterly. **Below are the patterns inlined for convenience** (kept in sync with `BUG-PATTERNS.md`): - **Empty catch + side effects assuming success.** Any `catch (e) {}` / `catch {}` followed by state mutation, analytics event, navigation, or any side effect that pretends the `try` block succeeded → **blocker**. The catch must either re-throw, surface to the user, or contain only side-effect-free logic. A caught error followed by `setIsActive(true)` is the canonical shape. - **`useState` flag used as a concurrency guard.** A handler that reads a React state value at the top to decide "am I already running?" races against React's batched re-render — a rapid second invocation sees the stale value and re-enters. **Warning**; suggest `useRef` (synchronous) for the race-critical flag, or `disabled` on the button while the action is in flight. Don't accept `setState` callbacks as a fix — they don't solve the read race, only the write race. - **No-op / silently-swallowed errors.** Caught errors that aren't re-thrown, logged to error tracking (Sentry), surfaced to the user, or counted in analytics. **Warning** minimum; **blocker** if the catch hides a failure that affects user-visible state. - **Analytics events fired before the action succeeded.** A `trackEvent(..._START)` that fires regardless of whether the underlying API call returned ok. Skews the metric (inflated start count, lower-than-real completion rate). **Warning**. - **State writes after unmount / after dispose.** A callback that calls `setState` / updates a ref / writes to a closed-over object after the component might have unmounted (or after a Flutter `dispose`, or after a Go context cancellation). **Warning**. **Performance** - N+1 queries / calls in a loop; query inside a render or request loop. - Unbounded result sets (missing pagination/limit) on a path that grows. - Synchronous I/O or heavy work on a hot path / UI thread. - Unnecessary re-renders, re-fetches, or allocations in a tight loop. **Maintainability** - Dead code, commented-out blocks, leftover debug prints, TODOs without a tracking ref. - Copy-paste that should be factored; a function doing too much. - Misleading names; missing types where the stack expects them. **Dependencies / supply chain** (when the diff touches a manifest / lockfile — package.json, pubspec.yaml, go.mod, requirements.txt, build.gradle, Podfile, etc.) - A newly-added dependency: is it necessary, or does the repo already have something that does this? Flag avoidable new deps (`warning`). - Pinning: a floating/`latest`/`*` version range is a `warning` — prefer a pinned or caret-bounded version for reproducible builds. - Known-risky: a package you recognize as deprecated/abandoned, or one whose name is a near-typo of a popular package (typosquat) is a `blocker`. Don't fabricate CVEs — flag only what you can justify. - License: a copyleft (GPL/AGPL) dep added to a proprietary codebase is a `warning` worth a human license check. - A lockfile changed with no corresponding manifest change (or vice versa) is a `warning` — likely an unintended or out-of-band update. ### 3. Ground every finding Each finding cites a real `file` and (where possible) `line` from the diff, and a `suggestion` the author can act on. A finding without a location is weaker than one with it — prefer fewer, grounded findings. **Cap at 15 findings.** If you have more, raise your bar; don't pad. Review the change *as built* — do not propose scope the contract excluded, and do not re-flag spec gaps (that's verify-pr's job and the contract critics'). ### 3b. Write findings in plain English — follow PR-COMMENT-STYLE.md Every finding you produce MUST be readable by the busiest person on the team. Lead with what's happening in plain English, not with the rule name. See `reference/PR-COMMENT-STYLE.md` for the canonical format and a worked example. **Required additional fields per finding** (extend the existing schema; older fields keep working): ```json { "id": "CR-007", "category": "correctness", "severity": "warning", "plainTitle": "Race condition on rapid clicks", "whatHappens": "When the user clicks the mic button twice quickly, the second click runs before React has updated `isRecording`. The guard thinks the session is still off and fires again — resetting `insertedCharCount` to 0.", "whyItMatters": "Users who double-tap (common on mobile, common when impatient) will lose their dictation character count and may see weird state during the brief overlap.", "theFix": "Use `useRef` for the \"is it running\" flag instead of `useState`. Refs update synchronously, so the second click sees the right value.", "codeSnippet": "// before\nconst [isRecording, setIsRecording] = useState(false)\n...\n\n// after\nconst isRecordingRef = useRef(false)\n...", "message": "useState flag used as concurrency guard — race on rapid clicks", "suggestion": "Replace `useState` with `useRef` for the race-critical flag.", "file": "voice-dictation-toolbar-button.tsx", "line": 42, "status": "open" } ``` The legacy `message` + `suggestion` are kept for tooling that already reads them (the `--check-review` validator, the fix-pr loop). The new fields are what get rendered into the GitHub review comment body using `PR-COMMENT-STYLE.md`'s four-part structure. **A finding without `plainTitle` / `whatHappens` / `whyItMatters` / `theFix` is a critic bug** — surface it loudly. Do not let the old terse format leak through because some prompt forgot the protocol. **Length budget** (enforced by the renderer; honor at write time): - Blocker: up to ~150 words across the prose fields + a code snippet. - Warning: up to ~80 words + snippet. - Info: a single line; if more is needed, escalate to warning. Going over budget means you have two findings — split them. ### 4. Post the review 1. Write your findings array to `.manifest/contracts/.pr-review.json` — this is the canonical, machine-readable artifact the CI merge-gate and the **review→fix loop** both consume. 2. Validate it: `node /scripts/validate.mjs --check-review .manifest/contracts/.pr-review.json`. If it rejects, fix your output and rewrite — never post out-of-schema findings. (Optionally also write a `.pr-review.md` mirror with the table for humans.) 3. Lead the PR comment with the SLA line: `node /scripts/validate.mjs --sla .manifest/contracts/.md` 4. Post a PR review comment: ```markdown ⏳ SLA: 11h 20m left (due 2026-05-21 13:30 IST / 08:00 UTC) ## Code review ( · ) — 1 blocker, 2 warnings | ID | Sev | Category | Where | Issue | |----|-----|----------|-------|-------| | CR-001 | 🔴 blocker | security | api/cards.ts:88 | `last4` interpolated into SQL string | | CR-002 | 🟡 warning | correctness | SavedCardRow.tsx:142 | `card.last4` deref when card is undefined | | CR-003 | 🔵 info | maintainability | SavedCardRow.tsx:31 | leftover console.log | **Blockers must be resolved before merge.** The Implementer will address open items automatically (review→fix loop) unless a human takes over. ``` ### 5. Hand off to the fix loop If there are open **blockers** (or the team opts to also auto-fix warnings), the review→fix loop re-invokes the Implementer in fix-mode against `.pr-review.md`. You do not fix code yourself — you produce the grounded, validated finding list the loop consumes. ## Anti-patterns - Don't re-check AC conformance, instrumentation presence, or flag gates — that's `verify-pr`. Stay in the code. - Don't review unchanged code far from the diff. - Don't emit severities outside the closed enum. - Don't propose changes the contract scoped out. - Don't pad to look thorough — 15 grounded findings beats 40 noisy ones. - Don't degrade silently: if you can't fetch the diff or resolve the stack, emit a single `info` finding stating why, rather than a clean "looks good." - Never put a real secret value in the finding text — name the file and line; don't reproduce the credential. ## Slack threading If posting to Slack, reply in the contract's thread (`thread_ts: `, channel ``). A blocker count > 0 may also post a brief top-level alert per the urgent-alert exception. (See reference/CONTRACT-FORMAT.md.) --- > Source: [iampawan/Manifest](https://github.com/iampawan/Manifest) — distributed by [TomeVault](https://tomevault.io).