review: single-writer RunMulti state; support disabling the inspector timeout

25c3a6f→main

dipree·1mo ago·5 files·+99 added/-57 removed

Three review notes:

- timedOut publication (high, 4th flag on this area): make perAgentState single-writer instead of relying on the WaitGroup happens-before for cross-goroutine field writes. Per-agent goroutines now send a terminal marker over fanIn carrying waitErr/finishedAt/timedOut; the dispatch loop (one goroutine) performs every perAgentState write. No cross-goroutine field sharing remains, so the post-loop reads are safe by construction. 'go test -race' clean.

- timeout disable (low): inspectorTimeout now distinguishes unset (0 -> default 10m), explicit positive, and negative (-> 0 == disabled). Run and RunMulti only wrap a deadline when timeout > 0. '--timeout 0' maps to the negative disable sentinel at the flag boundary so the RunConfig zero value still means 'use default'. Added TestInspectorTimeout.

- modelComponentsMatch off-by-one (medium): false positive — at the max offset the following component is long's last element (exists), so the span is never a suffix; the suffix window is excluded by '<'. Clarified the comment and added a boundary test (next component is the last).

Sessions

1a7f1bf3f1ecView transcript

Changes

5

• cmd/entire/cli/review

• Mcmd.go+12/-3

• Mmanifest_test.go+1

• Mrun.go+21/-8

• Mrun_multi.go+52/-46

• Mrun_test.go+13

133 unmodified lines

--profile NAME select a profile (also accepted as positional arg) --prompt TEXT add one-off per-run instructions for this invocation --timeout DUR max time each inspector may run before it's cancelled and marked failed (default 10m). Siblings and the judge proceed. marked failed (default 10m; 0 disables). Siblings and the judge proceed. --base REF scope against REF instead of mainline. Useful for stacked PRs where the base is the parent feature branch, not main. Default: first existing of origin/HEAD, origin/main,

if findings { return runReviewFindings(ctx, cmd, deps.NewSilentError) } return runReview(ctx, cmd, agentOverride, modelOverride, baseOverride, profileName, perRunPrompt, inspectTimeout, deps) // --timeout 0 disables the per-inspector bound. The RunConfig zero // value means "use the default", so translate an explicit 0 to a // negative disable sentinel. (The flag defaults to 10m, so the value is // only 0 when the user passed --timeout 0.) timeoutArg := inspectTimeout if timeoutArg == 0 { timeoutArg = -1 } return runReview(ctx, cmd, agentOverride, modelOverride, baseOverride, profileName, perRunPrompt, timeoutArg, deps) },

Mcmd/entire/cli/review/cmd.go+12/-3

778 unmodified lines

{"identical two-component ids match", "claude-sonnet", "claude-sonnet", true}, {"slash provider prefix stripped then identical", "anthropic/claude-sonnet", "claude-sonnet", true}, {"family matches across a provider component at offset", "claude-sonnet", "anthropic-claude-sonnet-4-5", true}, {"match where the next component is the last element", "sonnet-4", "claude-sonnet-4-5", true}, {"thinking-suffix-only difference matches", "claude-sonnet:high", "claude-sonnet:low", true}, {"equal-length different family does not match", "claude-sonnet", "claude-opus", false}, {"equal-length different version does not match", "claude-sonnet-4", "claude-sonnet-5", false},

Mcmd/entire/cli/review/manifest_test.go+1

37 unmodified lines

// process killed) and marked failed rather than hanging the review forever.
const defaultInspectorTimeout = 10 * time.Minute

// inspectorTimeout resolves the effective per-inspector timeout, distinguishing
// the three RunConfig.InspectorTimeout states the zero value alone can't:
//   - positive: use it.
//   - zero (unset): use defaultInspectorTimeout.
//   - negative: disabled — return 0, and callers treat 0 as "no timeout".
func inspectorTimeout(cfg reviewtypes.RunConfig) time.Duration {
if cfg.InspectorTimeout > 0 {
switch {
case cfg.InspectorTimeout > 0:
return cfg.InspectorTimeout
case cfg.InspectorTimeout < 0:
return 0
default:
return defaultInspectorTimeout
}
return defaultInspectorTimeout
}
``

Mcmd/entire/cli/review/run.go+21/-8

``
33 unmodified lines
``

Mcmd/entire/cli/review/run_multi.go+52/-46

``
671 unmodified lines
``

Mcmd/entire/cli/review/run_test.go+13