Civitai Review

Civitai segment review — main app (src/)

The standard pre-completion review for the main Next.js app. A segment is not done until this has run and its findings are resolved.

The conventions being enforced are the root CLAUDE.md and the docs it points at. For work in apps/moderator, apps/auth or apps/creator-studio, use svelte-review instead — these agents do not cover SvelteKit and will produce noise there.

These five lanes are pre-authorised. They do not count against a standing instruction not to spawn subagents unasked — running them needs no separate permission, and neither does chasing a lane that goes idle. That covers the review lanes only; everything else about spawning is unchanged. The pre-authorisation lives here so it travels with the skill rather than through a relay: an agent cannot tell a real global ruling from a mistaken one by reading someone's summary of it, so twice on 2026-08-19 agents correctly declined a relayed "he already said yes" and raised it instead.

This complements the built-in /code-review, which already handles generic correctness. What these five add is Civitai-specific: which service, which component, which trap.

0. Set up the review worktree

Review runs in its own throwaway worktree, detached at the head under review, and nothing is written to it — the implementer owns the tree the code was authored in.

🔴 If that head is based on an integration branch rather than main, this skill and its five agents do not exist in the tree. They are tracked in the repo, so a worktree at a commit predating their merge contains neither .claude/skills/civitai-review/ nor .claude/agents/civitai-*.md: the skill is not found and the subagent_type names do not resolve. Work stacked on a feat/… integration branch is the normal case here, not the exception, so expect this rather than treating it as a broken install. The failure is upstream of anything this file can say — by the time you could read a workaround here, you would already have found the file.

Skip this step when the head under review is based on main — the agents are already in the tree and the checkout is a no-op. This step is for integration-branch work only.

Otherwise, bring them in from main before spawning anything:

git checkout origin/main -- .claude

The tree is detached and throwaway, so a dirty .claude in it costs nothing and can never reach a PR. Do not install the agents into ~/.claude/ instead: that is a second copy free to drift from the repo's, and the repo's is the authoritative one.

⚠️ That checkout STAGES the files, so the next git checkout --detach aborts with local changes that would be overwritten — which is how you find out, halfway through moving the tree to the next review. Unstage and remove them first:

git reset -q
rm -rf .claude/agents/civitai-*.md .claude/skills/civitai-review
git checkout --detach <next-sha>
git checkout origin/main -- .claude    # re-apply, if the new head predates the skill

1. Scope the segment

Determine the base first — it is usually not main. Work stacks on a feature integration branch far more often than it sits directly on main, and diffing against main drags in every unrelated commit that landed on the integration branch. Take the base from the invoking request, or read it off the PR:

gh pr view <n> --json baseRefName -q .baseRefName

Then scope against that base, and say what it is before spawning anything:

BASE=origin/feat/<integration-branch>   # whatever the PR is actually based on
git diff --stat $BASE...HEAD -- src/
git status --short

A segment is one slice of work — a page, a router and its service, a feature. If the diff spans several unrelated slices, review them one at a time; findings from a mixed diff are hard to act on. Include uncommitted changes: the review exists to run before the commit.

Also locate the intent doc now, so the intent reviewer isn't the one to discover it's missing:

C:\Dev\Repos\work\model-share\_local\docs\plans\<feature>.md

Absolute path, from any worktree. 🔴 Never resolve _local/ relatively — it exists only in the primary worktree, and a relative write silently creates a second private copy that nobody else reads.

2. Fan out — all five at once, in one message

🔴 Launch every lane in a single message with multiple tool uses. They are independent and share no state.

A numbered list of steps reads as an instruction to run them one after another, so this is stated rather than left to inference: do not spawn them sequentially. A five-lane review that takes five times as long is the thing that makes people stop running it, and a review nobody runs finds nothing.

The first four are the code lanes — same input, four questions, launched together. civitai-intent-review launches alongside them but is different in kind: it reads the request rather than the code, and its findings stay a separate section at step 3.

Give each the same scope: the file list, what the segment is meant to do, and anything it can't recover from the code — what was deliberately left out, what a reviewer already accepted.

Give civitai-intent-review the intent doc path and any mid-project scope changes from the conversation. It is the only one whose input is not in the repo, so it is the only one you can starve by accident.

Solo mode is deliberately not offered. Running one adversarial reviewer over everything was considered and rejected in favour of five parallel lanes — the lanes hold different concrete knowledge (which service, which trap, which measurement) and one agent carrying all of it dilutes each. This is a decision, not an omission; do not helpfully collapse them.

Skip a lane only when the diff genuinely cannot contain its findings — no server code at all is a reason to skip perf. "No tests changed" is a finding for the test lane, not a reason to skip it. Say which you skipped and why.

Lanes routinely go idle without delivering. Chasing them is YOUR job, and it is the only control.

🔴 This is normal, not exceptional. Expect it every round. Measured across one project's four review rounds: 6 of 13 lanes finished, went idle, and sent nothing. In the worst round it was 4 of 4. Every one reported in full when re-pinged — re-pinging worked 8 times out of 8.

🔴 Account for every lane by name before you consolidate, and chase every silent one. Not "should". A lane that goes idle without reporting has failed, and it is indistinguishable from a lane that found nothing — which is the one failure this skill's own format actively hides, since step 3 asks you to say plainly when a lane found nothing. The consolidated report then reads as complete while missing a fifth of the review. On the run that discovered this, the vanished lane held the sharpest finding of the round.

Tell each lane in its prompt how its findings get back to you — final message text, or an explicit send for an agent whose own transcript reaches nobody. Treat that as a hint that reduces the rate, not as a control that removes your obligation.

⚠️ The agent-definition version of this rule was tried and did not work. #4011 wrote the same obligation into all five agent definitions, where it would not depend on the invoker remembering. Measured before and after: 2 of 4 lanes silent before, 4 of 4 after. The text is still in the agent files and is kept, because it costs a paragraph and may help at the margin — but it does not solve this, and the numbers are recorded here so nobody re-derives that fix and believes it worked.

Stated plainly because the mechanism is the problem, not the wording: prose in a definition cannot make delivery happen. Do not spend effort making it work harder. A structural enforcement — something in the harness that requires a report before an agent can finish — would be the real fix, and it is not something a markdown file can do. That is the open problem; this manual chase is the control until it is solved. A known-manual control beats a fixed-looking automatic one.

⚠️ And do not read "I caught the silent lanes" as evidence the system works. A run where someone already knew about this failure and watched for it proves nothing about the run where nobody does — which is every run after the one that discovered it.

3. Consolidate

Do not relay five reports. Produce three sections, in this order:

A. Does it deliver the request — civitai-intent-review's output, on its own.

Intent findings are not ranked in with code defects; they are not comparable and mixing them makes both harder to act on. "This doesn't do what was asked" and "this query is an N+1" call for different decisions from different people, and a merged list buries the first under the second. Carry the intent lane's confidence note with it — whether the doc was reviewer-authored or postdates the code changes what the whole section is worth.

B. Code findings — the four code lanes, merged into one ranked list.

• Drop what you can disprove. Read the code for anything that sounds wrong; agents produce plausible-but-false findings, and a fix applied to a non-bug is a new bug.
• Deduplicate — the same defect commonly surfaces as reuse and perf (a rewritten query that the cached-by-id-array helper already answers), or safety and test (an unguarded path with a vacuous test over it).
• Rank by consequence: money moving wrongly and content crossing a gate first, then production cost, then false confidence from a bad test, then reuse.

C. Map contribution, not for this PR — civitai-reuse-review's "Pre-existing service duplication".

It is a map contribution, not a change request; nobody is expected to fix it here. Mixing it into the fix list is how the whole list gets ignored.

Across all three sections:

🔴 Security findings stay out of the repo. Do not paste an open safety finding into a PR body, a commit message, or a file under docs/, claudedocs/ or .claude/ — this repo is public and permanent. They live in the conversation until they are fixed.

4. The fix loop

These agents report; they never patch. The loop is explicit:

1. Reviewer reports findings.
2. The implementer fixes them — a different agent or a human, not the reviewer.
3. Back to the reviewer, against the updated diff only — not a fresh read of the whole segment. Re-reviewing everything each round is slow and re-litigates settled findings. Re-run only the lanes whose findings were touched, and launch those together in one message as in step 2.
4. Iterate.

🔴 Narrowing on re-review narrows which lanes run, never what each running lane sees. Give every re-run lane the complete updated diff, not only the hunks matching its own findings.

Deciding a whole lane is irrelevant is cheap to get right — reuse has nothing to say about a one-line SQL fix. Deciding which part of a diff is relevant to a lane is the same guess the lane exists to make for us, and it is wrong in the case that matters: a fix in one lane's territory changes the code another lane's findings were about. A test lane shown only its own changed assertions reviews them against a function that no longer exists in the form it is imagining — new refusals mean new branches, and "is this test still meaningful" cannot be answered from the assertion alone.

A declined finding is a legitimate outcome, but it must come back with a reason. The reviewer then either accepts the reason or escalates to Justin. 🔴 Silent non-fixes are the failure mode here — a finding that quietly doesn't appear in the next round has not been resolved, it has been lost. Track the list across rounds and account for every item.

Ship means: no unresolved findings, and every declined finding carrying an accepted reason. Nothing weaker.

Present the consolidated list and hand it to the implementer. Ask before applying fixes yourself, unless the invoking request already said to.

When the head moves mid-review

The implementer may push while the lanes are still reading. They are asked to mail the reviewer the new SHA on every push — but do not depend on it, because a review is detached at a SHA and does not follow the PR.

🔴 Never move the worktree while lanes are running. Checking out a new head swaps files beneath agents mid-read, and the reports then describe two different commits with nothing marking which is which. Read the delta out of git instead, which touches no file in the tree:

git fetch origin <branch>
git diff <old-sha> <new-sha>
git show <new-sha>:<path>

Then re-target only the lanes whose subject those files touch — the same rule as the loop above.

Move the tree only when every lane is idle, and account for all five by name first, per step 2 — an idle lane that never reported is still holding a read you are about to invalidate. That is the safe moment and the only one. After the checkout, re-apply step 0:

git checkout --detach origin/<branch>
git checkout origin/main -- .claude    # the branch still predates the skill

Without that second line the agents disappear again mid-review, and the next fan-out fails to resolve them.

5. Close out — the implementer's step, not the reviewer's

🔴 If you ran this skill as a findings-only reviewer, stop at step 4. Everything below writes: prettier:write rewrites files in place, and a review tree is read-only by construction. Hand the consolidated list to the implementer and let them close out in the tree they own.

The rest of this step belongs to whoever is committing, per CLAUDE.md's "Before Committing":

pnpm run typecheck          # never `npx tsc`; never filter its output
pnpm run lint
pnpm run prettier:write     # uncommitted files only — never a repo-wide glob
pnpm run test:unit:run      # queued on this machine; targeted files are not

If schema.full.prisma changed: pnpm run db:check-generated, and surface that the migration must be applied manually — this repo never runs prisma migrate deploy.

⚠️ pnpm run typecheck is blind to __tests__ files, so a green typecheck does not clear the test changes. ⚠️ If a queued unit run returns suspiciously fast or empty, check the dev-server daemon is alive before believing it.

Then look at the page in a browser. Typecheck passes on plenty of pages that render blank.

Do not commit unless asked.