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
Comments
- Comments describe the code and explain why; they are not narration.
- No incidental history or defensive justification — strip comments that explain
today's bug fix, argue for the code's correctness, or narrate how the code
used to work. The comment should help a future maintainer, not relitigate
the change that introduced it.
- No restating volatile facts from the surrounding code (a count of subclasses,
a list of variants/enum cases, the call sites of a function). These pin the
comment to a moment in time and rot quickly — follow DRY.
- No ASCII-art banners or box-drawing section dividers. They add visual noise
without conveying anything a plain one-line comment does not.
- No editorializing — strip frustration, jokes, hedging-as-mood, and
exaggeration, keeping any underlying fact.
- No pointers to throwaway docs, tickets, or plan phases —
agent_docs/...
paths, REQ-* requirement IDs, or implementation-plan phase references.
The comment must stand on its own.
- No real people's names in comments, docstrings, or test strings. Reword
(e.g. "check with the team"), and use a generic placeholder (
dev, foo,
alice) for usernames in /Users/<name>/ paths or example branch names.
Error handling
- No bare
except: or unconditional except Exception:
- Errors carry context (custom exception types where appropriate)
- No silently swallowed errors (
except: pass etc.)
- User-facing errors are actionable
- External I/O has timeouts and reasonable retry/backoff where appropriate
Security & secrets
- No secrets, tokens, API keys, or credentials in code or configs
- Input validated at trust boundaries
- Auth/permission checks present where required
- No untrusted input fed to subprocess/eval/SQL/template without sanitization
- Sensitive values not logged
Type safety
- Type hints on new/changed public functions
- No new
Any types unless justified
- Pydantic models for structured data, not ad-hoc dicts
- No new Pyrefly errors; suppressions justified with a comment
- For frontend: TypeScript types up to date; if
ElementIDs changed, run
just generate-api
Backwards compatibility
- API/schema/config changes are documented and have migration paths
- Persisted data formats: forward-compatible reads if format changes
- Public CLI flags / env vars: deprecation considered before removal
- Frontend↔backend contract changes shipped together
Frontend issues (for .tsx changes)
Required: Read docs/development/review/react.md (generic React rules),
docs/development/review/sculptor.md (Sculptor-specific frontend conventions),
docs/development/review/design.md (design-system usage, existing patterns, UI copy),
and docs/development/review/file_structure.md (file placement, feature layout,
naming — also applies to frontend .ts changes) in full with the Read tool, and
apply every rule in them. The rules cover effects,
state, refs, render purity, performance, props, lists, backend data hooks,
Jotai atom usage, component-level invariants, design-token and component-reuse
conventions, UI copy, code placement, and naming — detailed enough that
summarizing them here would lose
information, so this section intentionally does not list them. If you find
yourself reviewing a .tsx change without having opened all four docs in the
current session, stop and read them.
Beyond the docs: IconButton in a Flex uses gap="2" (per CLAUDE.md).
Integration test issues (for changes under sculptor/tests/integration/)
Required: Read docs/development/review/integration_tests.md in full with the Read
tool and apply every rule in it. The rules cover Playwright assertion
patterns, test isolation, and POM usage — they're detailed enough that
summarizing them here would lose information, so this section intentionally
does not list them. If you find yourself reviewing an integration test change
without having opened that doc in the current session, stop and read it.
Style guide & ratchets
- All imports at top of file (no inline imports unless
# noqa: E402)
- No relative imports
- No nested/inline function definitions (except simple lambdas)
- Boolean variables prefixed with
is_/has_/should_/etc.
- No
num_ prefix (use count_ or _idx suffix)
- Early-exit pattern preferred over deep nesting
- No magic numbers — use named constants
- No mutable default arguments
- Comments follow the Comments category above
- Run
just ratchets if you suspect any counts changed; flag any increases
Git hygiene
- Commit messages explain the "why"
- Commits are atomic — one logical change each
- No unrelated changes mixed in
Public-facing text (commit messages & PR/MR description)
Commit messages and the PR/MR title and description are published to a public
repository — review them as permanently, world-readable artifacts. Read the
commit messages in the range (git log <base>..<head>) as well as the stated
goal (the PR/MR body, when one was provided). Flag any of the following:
- CRITICAL — secrets, tokens, credentials, or private keys in a commit
message or PR/MR body.
- PII — real people's full names, personal email addresses, individual
usernames, or local filesystem paths embedding a username (
/Users/<name>/).
These should be reworded to roles or generic placeholders (dev, <user>).
- Internal-only leakage — internal hostnames/URLs, internal
service/tool/infra names, internal dashboards, customer names or
customer-specific data, or verbatim private ticket/chat discussion. A bare
ticket ID reference (e.g.
SCU-1447) is fine; pasting the ticket's private
contents is not.
- Security-sensitive detail — exploit steps or descriptions of internal
security mechanisms written in a way that would aid an attacker. Security
fixes should be described at a safe altitude, not as a reproduction recipe.
- Candid internal commentary — business strategy, unreleased plans,
speculation about competitors, or remarks about individuals.
This is the same standard as the Comments category, extended from code
comments to the commit/PR prose. See also CLAUDE.md, "Public Visibility:
Commit Messages and PR Descriptions".
1---2name: code-review-checklist3description: Review a set of code changes against Sculptor's review categories and produce a markdown findings table. Use when reviewing code (your own or others').4---56# Code Review Checklist78Review a set of code changes against the categories below and produce a9markdown findings table.1011## Inputs1213- **Working directory** — where to run `git` commands. Default: the current repo.14- **Diff range** — preferred form is `<base>...<head>` (three dots). Default if15 not specified: `git diff origin/main...HEAD`.16- **Stated goal** (optional) — a description of what the change is meant to17 accomplish (e.g. an MR/PR description, a spec, a ticket). Used for the18 "Consistency with stated goal" category. Skip that category if no goal was19 provided.2021## Steps2223### 1. Read the stated goal (if any)2425Read it before reading the diff so the goal is fresh while you're reviewing.2627### 2. Read the diff2829Run `git diff <base>...<head>` in the working directory and read the full30output. For very large diffs, also list changed files31(`git diff --stat <base>...<head>`) and prioritize files most relevant to the32stated goal.3334Also read the commit messages in the range (`git log <base>..<head>`) — they35are part of what you review (see "Public-facing text" and "Git hygiene").3637### 3. Walk every category3839For each category below, look for issues in the diff. Be exhaustive — a single40file can have multiple issues of the same type. Don't skip categories that41look empty at a glance; spend a moment confirming.4243For frontend (`.tsx`) changes, you MUST read `docs/development/review/react.md`44(generic React rules), `docs/development/review/sculptor.md` (Sculptor-specific45conventions: backend data hooks, Jotai atoms, component invariants),46`docs/development/review/design.md` (design-system usage, existing patterns,47UI copy), and `docs/development/review/file_structure.md` (file placement,48feature layout, naming) in full and apply every rule in them. Do not skip this49step. Do not rely on summaries elsewhere in this checklist or on prior50knowledge — the docs are the source of truth and are updated independently.51The file-structure doc also applies to frontend `.ts` changes (new files,52moves, renames).5354For integration test changes (under `sculptor/tests/integration/`), you MUST55read the file `docs/development/review/integration_tests.md` in full and apply every rule56in it. Same reasoning as above.5758### 4. Produce findings broken down by category5960Output one section per category in the order they appear under "Categories"61below. Every category must have a section in your output — including ones62where you found no issues. This forces you to confirm you actually reviewed63each category rather than letting empty ones slip past unnoticed.6465For each category, write a heading with the category name, then either prose66findings or an explicit "no issues" line.6768Write each finding as a short paragraph, not as a table row. Lead with the69severity tag in bold, then the file path and line numbers, then a one- to70three-sentence explanation of what's wrong and why it matters. Multiple71findings in the same category go as separate paragraphs, blank-line separated.7273Example output:7475```76### Correctness7778**HIGH** — `path/foo.py:42-48`. `bar()` is called with `None` when `x` is79empty, and `bar` does not guard against that. This will raise an80`AttributeError` at runtime on the empty-list path, which the new tests81don't cover.8283**MEDIUM** — `path/baz.py:101`. The loop runs `range(len(items) - 1)`, so84the last item is silently skipped. Likely an off-by-one introduced when the85slice was removed in this change.8687### Consistency with stated goal8889No stated goal provided — section skipped.9091### Test coverage9293No issues found.94```9596Each section must be one of:97- One or more prose findings (each a paragraph led by a bolded severity tag), OR98- The literal line `No issues found.`, OR99- For "Consistency with stated goal" only, when no goal was provided:100 `No stated goal provided — section skipped.`101102Do NOT collapse multiple empty categories into a single "no issues" note.103Do NOT omit a category section. If you do, it signals you didn't review it.104Do NOT use markdown tables — prose only.105106Severity:107- **CRITICAL** — crashes, data loss, security holes, silently wrong behavior108- **HIGH** — likely bugs, broken edge cases, missing tests for risky paths,109 breaking API/schema changes110- **MEDIUM** — maintainability issues, scope creep, style guide violations,111 unclear code that will lead to bugs112- **LOW** — nits, minor cleanup, naming, comments113114### 5. Summarize115116After the per-category sections, write a short summary (2–4 bullets):117118- Whether the change accomplishes the stated goal (if a goal was provided)119- Top 1–3 things to address before merging120- Anything that should block the change121- "No issues found" is a valid summary if the diff is clean122123## Categories124125### Correctness126- Logic errors: off-by-one, wrong operator, inverted conditions127- Edge cases: `None`/empty/zero/negative inputs, boundary values128- Race conditions, ordering bugs in async/concurrent code129- Resource lifecycle: file handles, sockets, subscriptions, listeners,130 subprocesses cleaned up on all paths (including error paths)131- State invariants: any path that leaves an object in an inconsistent state132133### Consistency with stated goal134- Does the implementation match the stated intent?135- Are there functional changes not mentioned in the goal (scope creep)?136- Is anything described in the goal missing from the diff?137- Are there obvious follow-ups the goal implies but the diff doesn't deliver?138- **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.139140### Test coverage141- New behavior has tests142- Bug fixes include a regression test that fails without the fix143- Tests are focused — one logical assertion per test144- Critical paths covered: error paths and edge cases, not just the happy path145- No tests skipped/`xfail`/disabled without justification146- No flaky patterns introduced (sleeps, real network, time-of-day deps)147148### Proof of work completeness149This 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.150151- **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.152- **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.153- **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.154- **Root cause and Fix blocks MUST be substantive.** Single-sentence "fixed the bug" entries are not acceptable; flag any block under two sentences.155- **Repro steps MUST be exact.** Vague repro steps ("open the app, click around") that a reviewer cannot follow verbatim are not acceptable.156157### Dead code & leftover artifacts158- Unused imports, variables, functions, types, components159- Commented-out code160- New `TODO`/`FIXME`/`XXX` comments without an owner or ticket161- Debug statements: `console.log`, `print(...)`, `pdb`, `breakpoint()`,162 `debugger`163- Unused feature flags, config knobs, env vars164- Stale code paths replaced by the change but not deleted165166### Comments167- Comments describe the code and explain *why*; they are not narration.168- No incidental history or defensive justification — strip comments that explain169 today's bug fix, argue for the code's correctness, or narrate how the code170 used to work. The comment should help a future maintainer, not relitigate171 the change that introduced it.172- No restating volatile facts from the surrounding code (a count of subclasses,173 a list of variants/enum cases, the call sites of a function). These pin the174 comment to a moment in time and rot quickly — follow DRY.175- No ASCII-art banners or box-drawing section dividers. They add visual noise176 without conveying anything a plain one-line comment does not.177- No editorializing — strip frustration, jokes, hedging-as-mood, and178 exaggeration, keeping any underlying fact.179- No pointers to throwaway docs, tickets, or plan phases — `agent_docs/...`180 paths, `REQ-*` requirement IDs, or implementation-plan phase references.181 The comment must stand on its own.182- No real people's names in comments, docstrings, or test strings. Reword183 (e.g. "check with the team"), and use a generic placeholder (`dev`, `foo`,184 `alice`) for usernames in `/Users/<name>/` paths or example branch names.185186### Error handling187- No bare `except:` or unconditional `except Exception:`188- Errors carry context (custom exception types where appropriate)189- No silently swallowed errors (`except: pass` etc.)190- User-facing errors are actionable191- External I/O has timeouts and reasonable retry/backoff where appropriate192193### Security & secrets194- No secrets, tokens, API keys, or credentials in code or configs195- Input validated at trust boundaries196- Auth/permission checks present where required197- No untrusted input fed to subprocess/eval/SQL/template without sanitization198- Sensitive values not logged199200### Type safety201- Type hints on new/changed public functions202- No new `Any` types unless justified203- Pydantic models for structured data, not ad-hoc dicts204- No new Pyrefly errors; suppressions justified with a comment205- For frontend: TypeScript types up to date; if `ElementIDs` changed, run206 `just generate-api`207208### Backwards compatibility209- API/schema/config changes are documented and have migration paths210- Persisted data formats: forward-compatible reads if format changes211- Public CLI flags / env vars: deprecation considered before removal212- Frontend↔backend contract changes shipped together213214### Frontend issues (for `.tsx` changes)215**Required:** Read `docs/development/review/react.md` (generic React rules),216`docs/development/review/sculptor.md` (Sculptor-specific frontend conventions),217`docs/development/review/design.md` (design-system usage, existing patterns, UI copy),218and `docs/development/review/file_structure.md` (file placement, feature layout,219naming — also applies to frontend `.ts` changes) in full with the Read tool, and220apply every rule in them. The rules cover effects,221state, refs, render purity, performance, props, lists, backend data hooks,222Jotai atom usage, component-level invariants, design-token and component-reuse223conventions, UI copy, code placement, and naming — detailed enough that224summarizing them here would lose225information, so this section intentionally does not list them. If you find226yourself reviewing a `.tsx` change without having opened all four docs in the227current session, stop and read them.228229Beyond the docs: `IconButton` in a `Flex` uses `gap="2"` (per CLAUDE.md).230231### Integration test issues (for changes under `sculptor/tests/integration/`)232**Required:** Read `docs/development/review/integration_tests.md` in full with the Read233tool and apply every rule in it. The rules cover Playwright assertion234patterns, test isolation, and POM usage — they're detailed enough that235summarizing them here would lose information, so this section intentionally236does not list them. If you find yourself reviewing an integration test change237without having opened that doc in the current session, stop and read it.238239### Style guide & ratchets240- All imports at top of file (no inline imports unless `# noqa: E402`)241- No relative imports242- No nested/inline function definitions (except simple lambdas)243- Boolean variables prefixed with `is_`/`has_`/`should_`/etc.244- No `num_` prefix (use `count_` or `_idx` suffix)245- Early-exit pattern preferred over deep nesting246- No magic numbers — use named constants247- No mutable default arguments248- Comments follow the **Comments** category above249- Run `just ratchets` if you suspect any counts changed; flag any increases250251### Git hygiene252- Commit messages explain the "why"253- Commits are atomic — one logical change each254- No unrelated changes mixed in255256### Public-facing text (commit messages & PR/MR description)257Commit messages and the PR/MR title and description are published to a public258repository — review them as **permanently, world-readable** artifacts. Read the259commit messages in the range (`git log <base>..<head>`) as well as the stated260goal (the PR/MR body, when one was provided). Flag any of the following:261262- **CRITICAL** — secrets, tokens, credentials, or private keys in a commit263 message or PR/MR body.264- **PII** — real people's full names, personal email addresses, individual265 usernames, or local filesystem paths embedding a username (`/Users/<name>/`).266 These should be reworded to roles or generic placeholders (`dev`, `<user>`).267- **Internal-only leakage** — internal hostnames/URLs, internal268 service/tool/infra names, internal dashboards, customer names or269 customer-specific data, or verbatim private ticket/chat discussion. A bare270 ticket ID reference (e.g. `SCU-1447`) is fine; pasting the ticket's private271 contents is not.272- **Security-sensitive detail** — exploit steps or descriptions of internal273 security mechanisms written in a way that would aid an attacker. Security274 fixes should be described at a safe altitude, not as a reproduction recipe.275- **Candid internal commentary** — business strategy, unreleased plans,276 speculation about competitors, or remarks about individuals.277278This is the same standard as the **Comments** category, extended from code279comments to the commit/PR prose. See also CLAUDE.md, "Public Visibility:280Commit Messages and PR Descriptions".
imbue-ai