code-review

name

code-review

description

Review a diff, branch, or PR along three axes — Correctness (does it work, and does it step on a NodeTool landmine like cross-package imports, MsgPack framing, Zustand subscriptions, ui_primitives, packaged-Electron paths, IPC security?), Standards (does it follow the repo's documented rules, plus a Fowler smell baseline?), and Spec (does it implement what the originating issue asked for?). Runs the axes as parallel sub-agents and reports them side by side. Use when the user wants to review changes, a branch, a PR, or work before commit or merge, or asks to "review since X". For stripping AI slop from your own diff, use the unslop skill; a full pre-merge pass runs both.

Code Review

Three-axis review of the diff between HEAD and a fixed point:

• Correctness — does the change work, and does it step on a NodeTool landmine?
• Standards — does it follow this repo's documented rules?
• Spec — does it faithfully implement the originating issue or spec?

Each axis runs as a parallel sub-agent so they don't pollute each other's context. This skill scopes the diff first, then aggregates.

Division of labor with the sibling skill: unslop asks whether the change is free of AI-generated filler. Run it after this one for a full pre-merge pass.

The rules the Correctness and Standards axes cite live in AGENTS.md, docs/DEVELOPMENT_STANDARDS.md, and the area AGENTS.md files.

Process

1. Pin the fixed point

Whatever the user named is the fixed point — a commit SHA, branch, tag, main, HEAD~5. If they named none, infer from what they asked for:

• Working tree: git diff + git diff --cached, plus untracked files via git status.
• Branch or PR: git diff $(git merge-base main HEAD)...HEAD — never git diff main, which picks up drift on main. For a PR, fetch the branch first.

Capture the diff command once, and the commit list via git log <fixed-point>..HEAD --oneline. Confirm the ref resolves (git rev-parse) and the diff is non-empty before going further — a bad ref should fail here, not inside three sub-agents.

2. Map the blast radius

npm run dev:nodetool -- affected --base main --json lists the workspaces to typecheck and test, and says whether a decorator package (loads from dist/) forces npm run build:packages. Don't guess.

3. Find the spec

In order:

1. Issue references in the commit messages (#123, Closes #45).
2. A path the user passed as an argument.
3. A spec file under docs/, specs/, or .scratch/ matching the branch or feature.
4. Otherwise ask. If there is no spec, the Spec sub-agent is skipped and the report says so.

4. Find the standards sources

AGENTS.md, docs/DEVELOPMENT_STANDARDS.md, the AGENTS.md for each area the diff touches, and anything else the repo documents about how code should be written.

On top of those, the Standards axis always carries the smell baseline below, which applies even where a repo documents nothing. Two rules bind it: a documented repo standard always wins, and every smell is a labelled heuristic ("possible Feature Envy"), never a hard violation.

5. Spawn the sub-agents in parallel

One message, three Agent calls, general-purpose for each. Give every prompt the diff command, the commit list, and the path to this file (.claude/skills/code-review/SKILL.md) so the sub-agent can read the section it needs.

Correctness — "Read § Correctness checklists, § Severity, and § Quick greps in .claude/skills/code-review/SKILL.md. Read each changed function whole, and its callers — grep the symbol; a hunk that looks fine in isolation is where signature drift and broken invariants hide. For changed types in packages/protocol, check every package importing them. Report findings most-severe first; each needs a failure scenario you can name the input for, or a citable rule. Under 500 words."

Standards — "Read § Smell baseline in .claude/skills/code-review/SKILL.md, plus these standards sources: <list from step 4>. Report, per file or hunk: (a) every place the diff violates a documented standard — cite the file and rule; (b) any baseline smell — name it and quote the hunk. Documented standards override the baseline, and baseline smells are always judgement calls. Skip anything lint or typecheck enforces. Under 400 words."

Spec — "The spec is at . Report: (a) requirements the spec asked for that are missing or partial; (b) behaviour in the diff nobody asked for; (c) requirements that look implemented but implemented wrong. Quote the spec line for each finding. Under 400 words."

6. Verify

While the sub-agents run, run the targeted commands from step 2: npm run test --workspace=packages/<name> and npm run lint --workspace=packages/<name> per package (packages typecheck via build, not a typecheck script); root npm run typecheck covers web, electron, and mobile. If the diff is wide, npm run check. Report the actual output — never "should pass".

7. Aggregate

Present the reports under ## Correctness, ## Standards, and ## Spec, verbatim or lightly cleaned. Do not merge or rerank findings across axes — see Why three axes. End with one line per axis: total findings, the worst one, and the verification you ran.

Severity

Tier	Meaning	Bar
Blocker	Wrong behavior, crash, data loss, security hole, broken build	You can name the input or state that triggers it
Should-fix	Violates a written repo rule	Cite the rule
Nit	Everything else worth a sentence	Only if you found nothing bigger in that file; never pad

A finding without a failure scenario or a citable rule is not a finding. When unsure whether something is a bug, say so instead of inflating the severity.

Correctness checklists

Cross-package (any packages/ change)

• Imports: inter-package imports use @nodetool-ai/<pkg> — never ../other-pkg/ or anything containing /dist/.
• ESM extensions: relative imports need .js in compiled output. A missing extension typechecks but dies at runtime in built packages.
• Dependency direction: does the import respect protocol → config → security/auth/storage → runtime → kernel → node-sdk → base-nodes → models → agents → chat → websocket/cli? A lower package importing a higher one is a cycle waiting to happen.
• Decorator packages (base-nodes, node-sdk, fal-nodes, replicate-nodes, elevenlabs-nodes) load from dist/. A change here that isn't followed by build:packages silently tests stale code — confirm the verify step rebuilt.
• Runtime data files: anything loaded relative to import.meta.url (manifests, examples, package:// assets) breaks in the packaged Electron app, where the backend is bundled into one server.mjs. Such files must be registered in PACKAGE_RUNTIME_ASSETS (packages/config/src/package-asset-registry.ts) and loaded via loadPackageAssetJson.
• Protocol types: new message or data shapes belong in packages/protocol, not re-declared locally.
• Errors: throw new Error(...), never strings. Empty catch blocks need a comment saying why.
• WebSocket framing: WS messages are MsgPack, REST is JSON. JSON.stringify on a WS send path, or a missing msgpack decode on a receive path, is a blocker.
• Streaming nodes: genProcess must yield, not accumulate-and-return; check that backpressure-sensitive paths don't buffer unbounded.

Web (web/src/)

• Raw MUI imports outside ui_primitives/ and editor_ui/ — should-fix, migrate to primitives.
• Design tokens: hardcoded border radii, font sizes, transition strings, off-4px-grid spacing, raw z-indexes → BORDER_RADIUS, TYPOGRAPHY/var(--fontSize*), MOTION, SPACING, Z_INDEX from ui_primitives (docs/DESIGN.md).
• Zustand: a whole-store subscription (useFooStore() with no selector) re-renders on every store write. Multi-key object selectors need useShallow. getState() in a render body is a bug; in handlers and effects it's fine.
• New WebSocket(...) anywhere — use the GlobalWebSocketManager singleton.
• TanStack Query: server data via useQuery/useMutation, hierarchical keys (["workflows", id]), enabled for conditional fetches, mutations invalidate affected keys. useEffect+fetch for backend data is should-fix.
• ReactFlow: unstable references passed as nodes/edges/nodeTypes (a fresh array or object each render) tank canvas performance — hoist or memoize nodeTypes, derive nodes and edges via selectors.
• Effects: check every new useEffect dependency array against what the body reads; a stale closure over a store value or prop is a classic bug here.

Electron (electron/src/)

Non-negotiable (electron/src/AGENTS.md § Security):

• contextIsolation: true, nodeIntegration: false, no webSecurity: false, no remote-content BrowserWindow without a strict preload.
• Every new IPC handler validates its inputs — renderer input is untrusted. Channel names typed, no ipcMain.handle passing raw args into fs, child_process, or shell.openExternal without allow-listing.
• Paths resolved relative to import.meta.url or __dirname: verify against the packaged layout (§ Packaged file layout), not just dev.

Mobile (mobile/)

• mobile/ is not a root workspace. Scripts must use npm --prefix mobile ...; a diff that "standardizes" this to --workspace=mobile breaks it.
• Mobile typecheck needs packages/protocol built first.

Tests

• New behavior without a test, or changed behavior whose old test still passes unmodified — ask why.
• Vitest in packages/, Jest in web/ and electron/, files in __tests__/.
• RTL queries by role or label, userEvent, waitFor/findBy* — no setTimeout sleeps, no getByTestId where a role query works.
• A test that mocks the unit under review, or re-implements its logic in the fixture, verifies nothing.

Config, CI, docs

• package.json script or dependency changes: check the lockfile moved with it, and that the sandboxed-install caveats (AGENTS.md § Common Pitfalls) still hold.
• Changes to commands, architecture, or rules documented in AGENTS.md: the doc must move in the same PR — that's a written rule, cite it.
• Prose follows docs/WRITING_STYLE.md; flag slop words but leave the full prose pass to unslop.
• User-facing copy (marketing site, product strings, release notes, node and workflow descriptions) also follows docs/BRAND.md: outcome before mechanism, no credits/tokens as billing, no chatbot for the agent, no powered by AI where a model name fits.

Smell baseline

A fixed set of Fowler code smells (Refactoring, ch. 3) the Standards axis carries on top of whatever the repo documents. Each reads what it is → how to fix; match against the diff.

• Mysterious Name — a function, variable, or type whose name doesn't reveal what it does or holds. → rename it; if no honest name comes, the design's murky.
• Duplicated Code — the same logic shape in more than one hunk or file in the change. → extract the shared shape, call it from both.
• Feature Envy — a method that reaches into another object's data more than its own. → move the method onto the data it envies.
• Data Clumps — the same few fields or params keep travelling together, a type wanting to be born. → bundle them into one type, pass that.
• Primitive Obsession — a primitive or string standing in for a domain concept that deserves its own type. → give the concept its own small type.
• Repeated Switches — the same switch or if-cascade on the same type recurs across the change. → replace with polymorphism, or one map both sites share.
• Shotgun Surgery — one logical change forces scattered edits across many files. → gather what changes together into one module.
• Divergent Change — one file or module edited for several unrelated reasons. → split so each module changes for one reason.
• Speculative Generality — abstraction, parameters, or hooks added for needs the spec doesn't have. → delete it; inline back until a real need shows.
• Message Chains — long a.b().c().d() navigation the caller shouldn't depend on. → hide the walk behind one method on the first object.
• Middle Man — a class or function that mostly just delegates onward. → cut it, call the real target direct.
• Refused Bequest — a subclass or implementer that ignores or overrides most of what it inherits. → drop the inheritance, use composition.

Quick greps

Over the changed files, not the whole repo — pre-existing hits are out of scope.

rg "@nodetool-ai/[a-z-]+/dist"            # dist imports (always wrong)
rg "new WebSocket\("                       # bypassing GlobalWebSocketManager
rg "JSON\.(stringify|parse)" packages/websocket packages/runtime   # on WS paths only
rg "from ['\"]@mui/material" web/src       # outside ui_primitives/, editor_ui/
rg ": any\b|as any\b|as unknown as"        # strict mode escapes
rg "console\.log"                          # leftover debug output

A grep hit is a lead, not a finding — read the site before reporting.

What not to flag

• Anything npm run lint or npm run typecheck already rejects — report their output instead of duplicating it as prose findings.
• Style preferences with no backing in the repo docs. "I'd have written it differently" is not a finding.
• Pre-existing problems outside the diff. Mention once at the end if serious; don't mix them into the findings.
• Slop — comments, dead abstractions, prose filler. One pointer to unslop, not itemized findings.

Output format

Within each axis, order findings blockers → should-fix → nits. Each one:

[BLOCKER] packages/kernel/src/actor.ts:142 — `pending` is never cleared on error
When a node throws, `handleError` returns early before `this.pending.delete(id)`,
so the runner waits forever on the next `sync_mode: on_any` join.
Fix: move the delete into a `finally`.

One line of location and claim, the failure scenario, the fix. Close with what you actually ran:

Verified: npm run build --workspace=packages/kernel ✓, npm run test --workspace=packages/kernel ✓ (34 passed)

If nothing is wrong, say so plainly and list what you checked — a clean review is a valid result, not a failure to find something.

Why three axes

A change can pass one and fail another:

• Code that follows every standard and implements the wrong thing → Standards pass, Spec fail.
• Code that does exactly what the issue asked and breaks the project's conventions → Spec pass, Standards fail.
• Code that satisfies both and deadlocks the runner → Correctness fail.

Reporting them separately stops one axis from masking another.