Add review-loop: automated review/triage/fix cycle on a branch

[mattmoran56]

Note on the branch name: the branch is named claude/add-colorful-theme-IW1IX for historical reasons — the contents are the review-loop feature, not a theme. Worth renaming before merge.

Summary

A new orchestrator that drives a Claude review (diff vs. base) → triage (one sub-agent per finding) → fix (apply, commit, push) cycle on a session's branch. The loop stops on the first of:

• Convergence — N consecutive rounds with zero actionable triage decisions (default 2)
• Iteration cap — hard limit on rounds (default 5)
• Cost cap — total USD spent across all sub-sessions (default $5)
• Manual cancel from the Review Loop tab

All four are user-configurable per workspace, with per-project overrides in Settings → Review Loop (including a toggle that hides the toolbar button and prevents the loop from running for that scope).

After the loop ends, any skip / defer triage decisions are summarised in a single sticky comment on the open PR (using a hidden marker so subsequent rounds update the same comment instead of re-posting), giving reviewers a record of what was knowingly left undone and why.

How it surfaces

• A default-visible Review Loop toolbar button (seeded once via a stable id so re-removal sticks). Hidden when the per-project toggle is off.
• A new Review Loop core tab in the session workspace showing status pill, current phase, cumulative cost, per-round triage decisions, and a per-round log.
• Three new app actions usable as custom-button commands: review-loop:start, review-loop:cancel, review-loop:toggle-tab.

Screenshots

Review Loop tab — mid-loop (round 2 in triage):

Review Loop tab — converged after 2 clean rounds:

Settings — workspace defaults + per-project overrides:

Architecture

Concern	Where
Orchestrator (phases, stop conditions, cancel, sticky PR comment)	src/main/services/review-loop.service.ts
IPC handlers + persistence (electron-store)	src/main/ipc/review-loop.ipc.ts
Preload bridge + state-update event subscription	src/preload/index.ts
Renderer state (settings, per-session loop state, effective-config helper)	src/renderer/stores/reviewLoopStore.ts
Tab UI (status, rounds, triage decisions, log)	src/renderer/components/review-loop/ReviewLoopPanel.tsx
Settings section	src/renderer/components/settings/ReviewLoopSettings.tsx
Built-in toolbar button (seeded once) + per-project filter	src/renderer/stores/buttonStore.ts

Each round writes its findings to <worktree>/.crucible/review-loop/round-N-{issues,triage}.json so phase agents can communicate via files. Cost is read from the Claude CLI's --output-format json envelope (total_cost_usd).

Caveats worth flagging on review

• The spawned claude runs with --dangerously-skip-permissions — necessary for the unattended loop, but a real footgun. We may want a first-time confirmation per project.
• Couldn't run end-to-end (no claude binary in the build sandbox), so the prompts and JSON parsing are unverified. First real run on macOS may need prompt tweaks if Claude doesn't reliably emit the expected JSON.
• The sticky comment requires prNumber. If the loop runs on a branch without an open PR, skipped items aren't surfaced anywhere outside the tab.

Test plan

• Toolbar button appears by default; hides when per-project toggle is flipped off
• Starting the loop switches to the Review Loop tab and shows the running status pill
• Cancel button stops the in-flight claude subprocess and finalises the state as cancelled
• Each phase writes the expected JSON file; subsequent phases read it back correctly
• Cost cap stops the loop once cumulativeCostUsd ≥ costCapUsd
• Iteration cap stops the loop after maxIterations rounds
• Convergence stops the loop after consecutiveCleanRounds consecutive zero-fix rounds
• Sticky PR comment is posted on first stop and updated (not duplicated) on subsequent runs
• Workspace-default and per-project settings round-trip through electron-store and survive restart
• Build passes (npm run build) and Storybook stories render the running/completed/settings panels correctly

https://claude.ai/code/session_013V6ENheXA8v4fVfF6MYwjC

---

Generated by Claude Code

[mattmoran56]

Review Loop — issues left open

Generated by the Crucible Code review loop on branch claude/add-colorful-theme-IW1IX (base: main).

The loop ran for 2 rounds and chose not to fix the following 7 items:

↷ Skipped: Cancel mid-phase leaves loop stuck in 'running' state — src/main/services/review-loop.service.ts:205

Reason: Not a real bug in current code: runLoop already checks loop.cancelled after each phase and calls finalize(loop,'cancelled') before the early return, so the cancel path does finalize the state. The described stuck-running scenario does not occur.

Original finding

When the user cancels while a phase is in flight, cancelReviewLoop sets loop.cancelled=true and SIGTERMs the child. runClaude resolves with {ok:false,error:'cancelled'}. runReviewPhase / runTriagePhase / runFixPhase then check if (loop.cancelled) return false BEFORE the error branch, returning false without calling finalize(). Back in runLoop, if (!reviewOk) return // finalize already called by phase exits with no finalize call — yet finalize was never invoked on the cancel path. The loop coroutine returns and state.status remains 'running' forever; the UI Cancel button keeps showing, future state updates never arrive, and isReviewLoopActive(sessionId) keeps returning true so the user can't restart. Either call finalize(loop,'cancelled') in each phase's cancellation branch or check loop.cancelled in runLoop before the (!reviewOk) early return.

↷ Skipped: runClaude doc comment falsely claims it streams text to the renderer — src/main/services/review-loop.service.ts:371

Reason: False positive: the implementation now uses --output-format stream-json with incremental NDJSON parsing, pushTranscript calls, and throttled emitState updates, so the JSDoc accurately describes the live progress streaming. The 'only buffers final JSON' claim no longer matches the code.

Original finding

The JSDoc on runClaude says it 'Streams text to the renderer as log lines so users can watch progress live.' The implementation only buffers stdout in stdoutBuf, parses one final JSON object on exit, and never sends incremental progress to the renderer (no emitState in the stdout/stderr handlers). Misleading documentation that will confuse maintainers — either implement streaming or drop the claim.

⏭ Deferred: No timeout / watchdog on claude subprocess — src/main/services/review-loop.service.ts:374

Reason: Real hardening concern but out of scope for this PR. The feature is functional with manual cancel and per-round caps; a per-phase wall-clock watchdog that SIGTERMs and finalizes with 'error' is a follow-up improvement once the core review-loop cycle is merged.

Original finding

The spawned claude subprocess has no timeout. The cost cap and iteration cap are only checked between rounds, and the cancel path is the only way to abort an in-flight phase. A hung claude (network stall, infinite tool loop, etc.) keeps the loop pinned indefinitely, the cost cap never fires, and the user must click Cancel manually — which itself has the bug above. A reasonable per-phase wall-clock timeout that SIGTERMs the child and finalizes with 'error' would bound worst-case cost and cleanly recover.

⏭ Deferred: mainWindow reference is set once and never refreshed — src/main/services/review-loop.service.ts:33

Reason: Window recreation does not occur in practice today — the app quits on window-all-closed, so the stale-reference path is unreachable. Pattern matches other services in the codebase. Real concern only if a future refactor introduces window recreation; defer until that lifecycle is on the roadmap.

Original finding

setReviewLoopWindow is called from registerReviewLoopHandlers exactly once at startup. If the BrowserWindow is ever destroyed and recreated (e.g. on macOS reopen-from-dock or any future window-recreate flow), the module-level mainWindow still points at the destroyed window and emitState short-circuits via isDestroyed(). All in-flight review-loop progress events are then silently dropped — the renderer's UI freezes on the last state it saw and only updates on manual refreshState calls. Either re-bind on window creation or use BrowserWindow.getAllWindows() to broadcast.

⏭ Deferred: Review and triage phases inherit --dangerously-skip-permissions despite being read-only — src/main/services/review-loop.service.ts:364

Reason: Real defense-in-depth concern, but attack surface is low: the loop runs against the user's own checked-out branch where they presumably trust the diff. Constraining review/triage via --allowedTools is a worthwhile follow-up but not blocking for the initial feature ship.

Original finding

runClaude is shared by all three phases and always passes --dangerously-skip-permissions. Only the fix phase actually needs write/exec permissions; review and triage just need to read the diff and write one structured JSON file each. Granting full skip-permissions to those phases broadens the blast radius for any prompt-injection content that lands in the diff (e.g. a malicious test fixture telling claude to run a command). A least-privilege design would constrain the read phases via --allowedTools (Bash for git diff, Read, Glob, Grep, Write to a single path) instead of skip-permissions.

↷ Skipped: Branch and base-branch names are interpolated into prompts without escaping — src/main/services/review-loop.service.ts:224

Reason: Branch names come from local git refs, not user input fields, and git refname rules already forbid most problematic characters in normal flows. Branch names are only spliced into prompt text and markdown — not into shell commands — so this is at most a minor prompt-confusion risk on a deliberately crafted ref. Not worth introducing sanitization in this PR.

Original finding

buildReviewPrompt, buildTriagePrompt, and buildFixPrompt all use template-literal interpolation on o.branch and o.baseBranch. Git refnames don't allow most shell-special chars but they DO allow backticks, dollar signs, and slashes; a branch like foobar or foo${x} produces a malformed prompt to claude (and in the case of the fix prompt, would interleave into the literal git diff instructions). The orchestrator should at minimum reject (or sanitize) ref names containing backticks, ${, and quotes before splicing them into prompts.

⏭ Deferred: Built-in review-loop button uses a localStorage 'seeded' flag that diverges from on-disk state — src/renderer/stores/buttonStore.ts:79

Reason: Buttons live in electron-store (main process) while the seeded flag lives in renderer localStorage, creating a divergence risk. Low probability today (no settings-sync feature exists), but the flag should move into the on-disk button store before this becomes harder to migrate. Defer to a follow-up.

Original finding

seedBuiltInButtons writes 'codecrucible.builtin-buttons.seeded'='1' to localStorage as a one-shot 'we already added it' flag, then never re-seeds. But the button itself is persisted in the main-process button store, not in localStorage. If a user clears Application Data or uses Crucible on a different machine with synced settings, localStorage is empty and the button gets re-added even if the user had deliberately removed it. Conversely, if the user clears the on-disk button store but keeps localStorage, the built-in button is gone forever. The 'seeded' bit should live alongside the buttons (in the same persisted store), not in a separate browser-storage location.