Code Review Checklist

Code Review Checklist

Review a set of code changes against the categories below and produce a markdown findings table.

Inputs

• Working directory — where to run git commands. Default: the current repo.
• Diff range — preferred form is <base>...<head> (three dots). Default if not specified: git diff origin/main...HEAD.
• Stated goal (optional) — a description of what the change is meant to accomplish (e.g. an MR/PR description, a spec, a ticket). Used for the "Consistency with stated goal" category. Skip that category if no goal was provided.

Steps

1. Read the stated goal (if any)

Read it before reading the diff so the goal is fresh while you're reviewing.

2. Read the diff

Run git diff <base>...<head> in the working directory and read the full output. For very large diffs, also list changed files (git diff --stat <base>...<head>) and prioritize files most relevant to the stated goal.

Also read the commit messages in the range (git log <base>..<head>) — they are part of what you review (see "Public-facing text" and "Git hygiene").

3. Walk every category

For each category below, look for issues in the diff. Be exhaustive — a single file can have multiple issues of the same type. Don't skip categories that look empty at a glance; spend a moment confirming.

For frontend (.tsx) changes, you MUST read docs/development/review/react.md (generic React rules), docs/development/review/sculptor.md (Sculptor-specific conventions: backend data hooks, Jotai atoms, component invariants), docs/development/review/design.md (design-system usage, existing patterns, UI copy), and docs/development/review/file_structure.md (file placement, feature layout, naming) in full and apply every rule in them. Do not skip this step. Do not rely on summaries elsewhere in this checklist or on prior knowledge — the docs are the source of truth and are updated independently. The file-structure doc also applies to frontend .ts changes (new files, moves, renames).

For integration test changes (under sculptor/tests/integration/), you MUST read the file docs/development/review/integration_tests.md in full and apply every rule in it. Same reasoning as above.

4. Produce findings broken down by category

Output one section per category in the order they appear under "Categories" below. Every category must have a section in your output — including ones where you found no issues. This forces you to confirm you actually reviewed each category rather than letting empty ones slip past unnoticed.

For each category, write a heading with the category name, then either prose findings or an explicit "no issues" line.

Write each finding as a short paragraph, not as a table row. Lead with the severity tag in bold, then the file path and line numbers, then a one- to three-sentence explanation of what's wrong and why it matters. Multiple findings in the same category go as separate paragraphs, blank-line separated.

Example output:

### Correctness

**HIGH** — `path/foo.py:42-48`. `bar()` is called with `None` when `x` is
empty, and `bar` does not guard against that. This will raise an
`AttributeError` at runtime on the empty-list path, which the new tests
don't cover.

**MEDIUM** — `path/baz.py:101`. The loop runs `range(len(items) - 1)`, so
the last item is silently skipped. Likely an off-by-one introduced when the
slice was removed in this change.

### Consistency with stated goal

No stated goal provided — section skipped.

### Test coverage

No issues found.

Each section must be one of:

• One or more prose findings (each a paragraph led by a bolded severity tag), OR
• The literal line No issues found., OR
• For "Consistency with stated goal" only, when no goal was provided: No stated goal provided — section skipped.

Do NOT collapse multiple empty categories into a single "no issues" note. Do NOT omit a category section. If you do, it signals you didn't review it. Do NOT use markdown tables — prose only.

Severity:

• CRITICAL — crashes, data loss, security holes, silently wrong behavior
• HIGH — likely bugs, broken edge cases, missing tests for risky paths, breaking API/schema changes
• MEDIUM — maintainability issues, scope creep, style guide violations, unclear code that will lead to bugs
• LOW — nits, minor cleanup, naming, comments

5. Summarize

After the per-category sections, write a short summary (2–4 bullets):

• Whether the change accomplishes the stated goal (if a goal was provided)
• Top 1–3 things to address before merging
• Anything that should block the change
• "No issues found" is a valid summary if the diff is clean

Categories

Correctness

• Logic errors: off-by-one, wrong operator, inverted conditions
• Edge cases: None/empty/zero/negative inputs, boundary values
• Race conditions, ordering bugs in async/concurrent code
• Resource lifecycle: file handles, sockets, subscriptions, listeners, subprocesses cleaned up on all paths (including error paths)
• State invariants: any path that leaves an object in an inconsistent state

Consistency with stated goal

• Does the implementation match the stated intent?
• Are there functional changes not mentioned in the goal (scope creep)?
• Is anything described in the goal missing from the diff?
• Are there obvious follow-ups the goal implies but the diff doesn't deliver?
• Does the fix exercise the full user flow described in the goal, not just the narrow symptom? If the goal describes a multi-step user flow (e.g. "background subagent renders as 0.0s") and the fix only addresses one moment in that flow, that's a partial fix — flag it. The bar is: a reviewer should be able to follow the original repro and confirm the whole flow works, not just that the literal symptom no longer appears.

Test coverage

• New behavior has tests
• Bug fixes include a regression test that fails without the fix
• Tests are focused — one logical assertion per test
• Critical paths covered: error paths and edge cases, not just the happy path
• No tests skipped/xfail/disabled without justification
• No flaky patterns introduced (sleeps, real network, time-of-day deps)

Proof of work completeness

This category applies when the stated goal is an MR/PR body produced by an autonomous workflow (e.g. /sculptor-workflow:fix-bug). Skip the category if no stated goal was provided, or if the stated goal is a spec/ticket rather than an MR body.

• UI-visible bugs MUST have before/after screenshots. If the diff touches any frontend file (sculptor/frontend/, any .tsx, .css, or other UI surface) and the MR body's "Before" or "After" slots lack an embedded image (e.g. <img src="..."> or a markdown image link), flag it as HIGH. Test output is not a substitute.
• Non-UI bugs MUST have failing-then-passing test output. If the diff has no UI surface and the MR body's "Before" or "After" slots lack a fenced code block with test output, flag it as HIGH.
• Test commit hashes MUST be present in the Test block. If the body says "Failing-test commit: <hash>" with a placeholder or empty hash, flag it.
• Root cause and Fix blocks MUST be substantive. Single-sentence "fixed the bug" entries are not acceptable; flag any block under two sentences.
• Repro steps MUST be exact. Vague repro steps ("open the app, click around") that a reviewer cannot follow verbatim are not acceptable.

Dead code & leftover artifacts

• Unused imports, variables, functions, types, components
• Commented-out code
• New TODO/FIXME/XXX comments without an owner or ticket
• Debug statements: console.log, print(...), pdb, breakpoint(), debugger
• Unused feature flags, config knobs, env vars
• Stale code paths replaced by the change but not deleted