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), and
docs/development/review/design.md (design-system usage, existing patterns,
UI copy) 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.
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), and
docs/development/review/design.md (design-system usage, existing patterns, UI copy)
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, and UI copy — 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 three 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".
Source: imbue-ai/sculptor — distributed by TomeVault.
1---2name: imbue-ai-sculptor-sculptor3description: Code Review Checklist4---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), and46`docs/development/review/design.md` (design-system usage, existing patterns,47UI copy) in full and apply every rule in them. Do not skip this step. Do not48rely on summaries elsewhere in this checklist or on prior knowledge — the docs49are the source of truth and are updated independently.5051For integration test changes (under `sculptor/tests/integration/`), you MUST52read the file `docs/development/review/integration_tests.md` in full and apply every rule53in it. Same reasoning as above.5455### 4. Produce findings broken down by category5657Output one section per category in the order they appear under "Categories"58below. Every category must have a section in your output — including ones59where you found no issues. This forces you to confirm you actually reviewed60each category rather than letting empty ones slip past unnoticed.6162For each category, write a heading with the category name, then either prose63findings or an explicit "no issues" line.6465Write each finding as a short paragraph, not as a table row. Lead with the66severity tag in bold, then the file path and line numbers, then a one- to67three-sentence explanation of what's wrong and why it matters. Multiple68findings in the same category go as separate paragraphs, blank-line separated.6970Example output:7172```73### Correctness7475**HIGH** — `path/foo.py:42-48`. `bar()` is called with `None` when `x` is76empty, and `bar` does not guard against that. This will raise an77`AttributeError` at runtime on the empty-list path, which the new tests78don't cover.7980**MEDIUM** — `path/baz.py:101`. The loop runs `range(len(items) - 1)`, so81the last item is silently skipped. Likely an off-by-one introduced when the82slice was removed in this change.8384### Consistency with stated goal8586No stated goal provided — section skipped.8788### Test coverage8990No issues found.91```9293Each section must be one of:94- One or more prose findings (each a paragraph led by a bolded severity tag), OR95- The literal line `No issues found.`, OR96- For "Consistency with stated goal" only, when no goal was provided:97 `No stated goal provided — section skipped.`9899Do NOT collapse multiple empty categories into a single "no issues" note.100Do NOT omit a category section. If you do, it signals you didn't review it.101Do NOT use markdown tables — prose only.102103Severity:104- **CRITICAL** — crashes, data loss, security holes, silently wrong behavior105- **HIGH** — likely bugs, broken edge cases, missing tests for risky paths,106 breaking API/schema changes107- **MEDIUM** — maintainability issues, scope creep, style guide violations,108 unclear code that will lead to bugs109- **LOW** — nits, minor cleanup, naming, comments110111### 5. Summarize112113After the per-category sections, write a short summary (2–4 bullets):114115- Whether the change accomplishes the stated goal (if a goal was provided)116- Top 1–3 things to address before merging117- Anything that should block the change118- "No issues found" is a valid summary if the diff is clean119120## Categories121122### Correctness123- Logic errors: off-by-one, wrong operator, inverted conditions124- Edge cases: `None`/empty/zero/negative inputs, boundary values125- Race conditions, ordering bugs in async/concurrent code126- Resource lifecycle: file handles, sockets, subscriptions, listeners,127 subprocesses cleaned up on all paths (including error paths)128- State invariants: any path that leaves an object in an inconsistent state129130### Consistency with stated goal131- Does the implementation match the stated intent?132- Are there functional changes not mentioned in the goal (scope creep)?133- Is anything described in the goal missing from the diff?134- Are there obvious follow-ups the goal implies but the diff doesn't deliver?135- **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.136137### Test coverage138- New behavior has tests139- Bug fixes include a regression test that fails without the fix140- Tests are focused — one logical assertion per test141- Critical paths covered: error paths and edge cases, not just the happy path142- No tests skipped/`xfail`/disabled without justification143- No flaky patterns introduced (sleeps, real network, time-of-day deps)144145### Proof of work completeness146This 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.147148- **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.149- **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.150- **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.151- **Root cause and Fix blocks MUST be substantive.** Single-sentence "fixed the bug" entries are not acceptable; flag any block under two sentences.152- **Repro steps MUST be exact.** Vague repro steps ("open the app, click around") that a reviewer cannot follow verbatim are not acceptable.153154### Dead code & leftover artifacts155- Unused imports, variables, functions, types, components156- Commented-out code157- New `TODO`/`FIXME`/`XXX` comments without an owner or ticket158- Debug statements: `console.log`, `print(...)`, `pdb`, `breakpoint()`,159 `debugger`160- Unused feature flags, config knobs, env vars161- Stale code paths replaced by the change but not deleted162163### Comments164- Comments describe the code and explain *why*; they are not narration.165- No incidental history or defensive justification — strip comments that explain166 today's bug fix, argue for the code's correctness, or narrate how the code167 used to work. The comment should help a future maintainer, not relitigate168 the change that introduced it.169- No restating volatile facts from the surrounding code (a count of subclasses,170 a list of variants/enum cases, the call sites of a function). These pin the171 comment to a moment in time and rot quickly — follow DRY.172- No ASCII-art banners or box-drawing section dividers. They add visual noise173 without conveying anything a plain one-line comment does not.174- No editorializing — strip frustration, jokes, hedging-as-mood, and175 exaggeration, keeping any underlying fact.176- No pointers to throwaway docs, tickets, or plan phases — `agent_docs/...`177 paths, `REQ-*` requirement IDs, or implementation-plan phase references.178 The comment must stand on its own.179- No real people's names in comments, docstrings, or test strings. Reword180 (e.g. "check with the team"), and use a generic placeholder (`dev`, `foo`,181 `alice`) for usernames in `/Users/<name>/` paths or example branch names.182183### Error handling184- No bare `except:` or unconditional `except Exception:`185- Errors carry context (custom exception types where appropriate)186- No silently swallowed errors (`except: pass` etc.)187- User-facing errors are actionable188- External I/O has timeouts and reasonable retry/backoff where appropriate189190### Security & secrets191- No secrets, tokens, API keys, or credentials in code or configs192- Input validated at trust boundaries193- Auth/permission checks present where required194- No untrusted input fed to subprocess/eval/SQL/template without sanitization195- Sensitive values not logged196197### Type safety198- Type hints on new/changed public functions199- No new `Any` types unless justified200- Pydantic models for structured data, not ad-hoc dicts201- No new Pyrefly errors; suppressions justified with a comment202- For frontend: TypeScript types up to date; if `ElementIDs` changed, run203 `just generate-api`204205### Backwards compatibility206- API/schema/config changes are documented and have migration paths207- Persisted data formats: forward-compatible reads if format changes208- Public CLI flags / env vars: deprecation considered before removal209- Frontend↔backend contract changes shipped together210211### Frontend issues (for `.tsx` changes)212**Required:** Read `docs/development/review/react.md` (generic React rules),213`docs/development/review/sculptor.md` (Sculptor-specific frontend conventions), and214`docs/development/review/design.md` (design-system usage, existing patterns, UI copy)215in full with the Read tool, and apply every rule in them. The rules cover effects,216state, refs, render purity, performance, props, lists, backend data hooks,217Jotai atom usage, component-level invariants, design-token and component-reuse218conventions, and UI copy — detailed enough that summarizing them here would lose219information, so this section intentionally does not list them. If you find220yourself reviewing a `.tsx` change without having opened all three docs in the221current session, stop and read them.222223Beyond the docs: `IconButton` in a `Flex` uses `gap="2"` (per CLAUDE.md).224225### Integration test issues (for changes under `sculptor/tests/integration/`)226**Required:** Read `docs/development/review/integration_tests.md` in full with the Read227tool and apply every rule in it. The rules cover Playwright assertion228patterns, test isolation, and POM usage — they're detailed enough that229summarizing them here would lose information, so this section intentionally230does not list them. If you find yourself reviewing an integration test change231without having opened that doc in the current session, stop and read it.232233### Style guide & ratchets234- All imports at top of file (no inline imports unless `# noqa: E402`)235- No relative imports236- No nested/inline function definitions (except simple lambdas)237- Boolean variables prefixed with `is_`/`has_`/`should_`/etc.238- No `num_` prefix (use `count_` or `_idx` suffix)239- Early-exit pattern preferred over deep nesting240- No magic numbers — use named constants241- No mutable default arguments242- Comments follow the **Comments** category above243- Run `just ratchets` if you suspect any counts changed; flag any increases244245### Git hygiene246- Commit messages explain the "why"247- Commits are atomic — one logical change each248- No unrelated changes mixed in249250### Public-facing text (commit messages & PR/MR description)251Commit messages and the PR/MR title and description are published to a public252repository — review them as **permanently, world-readable** artifacts. Read the253commit messages in the range (`git log <base>..<head>`) as well as the stated254goal (the PR/MR body, when one was provided). Flag any of the following:255256- **CRITICAL** — secrets, tokens, credentials, or private keys in a commit257 message or PR/MR body.258- **PII** — real people's full names, personal email addresses, individual259 usernames, or local filesystem paths embedding a username (`/Users/<name>/`).260 These should be reworded to roles or generic placeholders (`dev`, `<user>`).261- **Internal-only leakage** — internal hostnames/URLs, internal262 service/tool/infra names, internal dashboards, customer names or263 customer-specific data, or verbatim private ticket/chat discussion. A bare264 ticket ID reference (e.g. `SCU-1447`) is fine; pasting the ticket's private265 contents is not.266- **Security-sensitive detail** — exploit steps or descriptions of internal267 security mechanisms written in a way that would aid an attacker. Security268 fixes should be described at a safe altitude, not as a reproduction recipe.269- **Candid internal commentary** — business strategy, unreleased plans,270 speculation about competitors, or remarks about individuals.271272This is the same standard as the **Comments** category, extended from code273comments to the commit/PR prose. See also CLAUDE.md, "Public Visibility:274Commit Messages and PR Descriptions".275276---277> Source: [imbue-ai/sculptor](https://github.com/imbue-ai/sculptor) — distributed by [TomeVault](https://tomevault.io).278<!-- tomevault:4.0:skill_md:2026-07-04 -->
tomevault-io