Review Inspect Feature Cleanup

Claude Code·dipree·3mo ago·46hr 41min·1 Checkpoint·23 file changes·+162/-2072·26.3K tokens

You are an expert code reviewer. Follow these steps:

Task: Review this change for correctness, regressions, API design, missing tests, maintainability, and user-facing behavior changes. Report only actionable findings with concrete evidence.

Scope: review the commits unique to this branch vs origin/HEAD, plus any uncommitted changes in the working tree. Ignore code outside this scope.

Checkpoint context from commits in scope:

  • 75dabf75ec5d: prompt: Merge latest main
  • 1a7f1bf3f1ec: prompt: Add a timeout. 10min default.
  • 5cbbb55f9459: prompt: Add a timeout. 10min default.
  • cb4f9fd21345: prompt: Add a timeout. 10min default.
  • 94f89f9567c7: prompt: Add a timeout. 10min default.
  • eaf38faaf53d: prompt: Add a timeout. 10min default.
  • d4bac437f615: prompt: Add a timeout. 10min default.
  • d1fe710ae227: prompt: Add a timeout. 10min default.
  • 30b78df00de8: prompt: Add a timeout. 10min default.
  • 0b9319108cb5: prompt: Add a timeout. 10min default.
  • ff9f72c12650: prompt: Add a timeout. 10min default.
  • 9babda34a47b: prompt: Add a timeout. 10min default.
  • 74c4f7c8f5a9: prompt: Add a timeout. 10min default.
  • c8c400c372be: prompt: Add a timeout. 10min default.
  • c104c98fc155: prompt: /Users/dip/Library/Application\ Support/CleanShot/media/media_V7uqzyeA1O/CleanShot\ 2026-06-17\ at\ 12.12.56@2x.png there should be no "change model" option here for review inspectors.
  • cc2e5e5a3dcc: prompt: /Users/dip/Library/Application\ Support/CleanShot/media/media_V7uqzyeA1O/CleanShot\ 2026-06-17\ at\ 12.12.56@2x.png there should be no "change model" option here for review inspectors.
  • 28dcc15e4c3b: prompt: Yes, should be in the settings or local settings, let the user decide.
  • 262d2ea09704: prompt: Yes, should be in the settings or local settings, let the user decide.
  • a84cf443023b: prompt: Yes, should be in the settings or local settings, let the user decide.
  • 4ddfc23385b7: prompt: Merge latest main
  • ... 35 more checkpoints omitted

Use entire explain <id> for full checkpoint context, or entire explain <id> --raw-transcript for raw transcripts.

In-progress session context (uncommitted): 019eca64 Pi (touched: 5 files) prompt: Not the review findings. No commits, brief list.

3mo ago·4m

I've reviewed the in-scope changes thoroughly: read the highest-risk logic (per-inspector timeout, single-writer RunMulti state, model-component matching, profile merge/persistence, codex error envelopes, synthesis sink), confirmed go build ./... is clean, and ran the review/settings/codex test packages (all green).

Review: review-profiles branch

Overview — Reworks entire review/inspect into a profile-based system: named profiles with N parallel inspectors + a single consolidating judge, stored in project/local settings; adds a per-inspector timeout, a local/trail output destination, live model enumeration, and codex error-envelope surfacing. Deletes the old migration path and most of fix.go. ~4900 LOC, exceptionally well-documented.

Findings

1. Untracked working-tree files risk being committed (actionable) Four untracked files are in scope and not gitignored:

  • .pi/extensions/entire.ts — header says "Auto-generated by entire enable --agent pi… Do not edit manually". This is a local install artifact, not a source deliverable.
  • prompt-default-tasks.md, prompt-inspector.md, prompt-synthesis-judge.md — dev scratch notes dropped at repo root (they just describe where prompt text lives in the Go source).

A git add -A would commit all four. Recommend deleting them or adding ignore rules before committing. None are referenced by the Go code.

2. Parent-cancel vs deadline race in RunMulti (minor / negligible) run_multi.go:252-255: st.timedOut overrides status to Failed after classifyStatus already returned Cancelled for a parent-cancelled ctx. If a user Ctrl+C races with an inspector's deadline firing and the deadline wins (waitErr == DeadlineExceeded), that agent reports "timed out" instead of "cancelled". Single-agent Run isn't affected (parent cancel surfaces as Canceled). Cosmetic, extremely rare — flagging for completeness, not blocking.

Verified sound

  • Timeout flag — --timeout 0 is correctly translated to the -1 disable sentinel (cmd.go:214-217); help text "(0 disables)" matches behavior. The 3-state convention (positive/zero-default/negative-disable) in inspectorTimeout is consistent.
  • Single-writer perAgentState — terminal fields routed through fanIn as a tagged marker; dispatch loop is the sole writer, post-loop reads are safe by construction. Sound.
  • OnResult/aggregateOutput — SynthesisSink is a direct sink; RunFinished (and thus OnResult) runs synchronously inside RunMulti before it returns, so the read at cmd.go:1216 is not racing the deferred tuiSink.Wait(). Correct.
  • modelComponentsMatch — boundary-aware matching with the numeric-suffix requirement correctly distinguishes version (sonnet→claude-sonnet-4-5) from variant (gpt-4o→gpt-4o-mini); the suffix-exclusion tradeoff is documented.
  • mergeReviewProfiles — pure, non-nil, no input mutation; per-profile merge semantics match the documented intent.
  • Codex --model + error envelope — failureMsg only emitted when !seenTurnComplete, so stray messages on successful runs are ignored. Correct.

No correctness regressions found in the core logic. The only thing I'd act on before committing is the four stray untracked files.

48d27b1inspect: remove dead code and collapse the synthesis path Cleanup pass over the review/inspect feature after a long iteration. Dead code: - delete the unused agent multi-picker (multipicker.go) and PromptForAgent; the multi-agent path fans out over all eligible agents, no picker - delete the unused trail.Store local-storage layer (store.go) and the ID/Priority/Type/Reviewer/Discussion/Checkpoints types it alone used - drop TrailDetailResponse and write-request fields the CLI never sends (TrailUpdateRequest.{Branch,Base,Assignees,Priority,Type}, TrailCreateRequest.{Assignees,Labels,Priority,Type}, BranchCreated) - delete EntireSettings.ReviewConfigFor (only its own test read it); the legacy Review/ReviewFixAgent/ReviewMigrationDismissed fields stay as parse-tolerance shims (the loader uses DisallowUnknownFields) Collapse the synthesis path: autoSynthesis was always true in production, so the prompted (Auto=false) branch was unreachable. Drop SynthesisSink's Auto/InputTTY/PromptYN, the legacy compose branch, realPromptYN, and the now-dead canPrompt/promptYN sink inputs. The master report now runs unconditionally in TTY and redirected output alike. Tidy: delegate top-level `trail watch` to the shared trail-review resolver (deleting the bespoke resolveTrailWatch* duplicates), make RunReviewProfileConfigPicker return only error, inline single-caller wrappers (detectScopeBaseRef, hydrateTrailReviewCommentSuggestions, reviewTrailFindingInput, saveDefaultReviewProfile), and fix flushBuffer's dead error-handling return. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 8fa27a4b7754+162/-2072