fix(review): restore the judge's 20m effective bound + stale help (trail findings) · Entire
fix(review): restore the judge's 20m effective bound + stale help (trail findings)
50698ad→main· peyton-alt·1w ago·3 files·+18 added/-11 removed
Trail findings on this branch caught two regressions the reviews missed:
The judge's effective no-flag bound silently dropped 20m -> 5m: on main the --timeout flag default (20m) always flowed into SynthesisSink.ProviderTimeout, so the 5m defaultSynthesisProviderTimeout was nearly dead code on this path; with the flag default now 0, the 5m fallback became live and tightened the judge 4x — against this PR's own thesis, and a judge timeout discards the whole multi-reviewer run with no verdict. The synthesis default is now 20m, preserving the prior effective bound while keeping the judge always bounded.
The command's Long help still promised 'default 20m; 0 disables both bounds' — both claims stale. Rewritten to the new contract: no reviewer default, judge separately bounded and never uncapped.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Sessions
01KX0PWPRKDPWNYPN3PWMT2YQBView transcript
Changes
3
cmd/entire/cli/review
Mcmd.go+7/-5
Msynthesis_sink.go+6/-2
Msynthesis_sink_test.go+5/-4
133 unmodified lines
134
135
136
137
138
139
140
141
137
138
139
140
141
142
143
144
145
146
133 unmodified lines
--models list the models each agent advertises (optionally --agent NAME)
--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 reviewer may run before it's cancelled and marked
failed; also bounds the consolidating judge, whose timeout or
error fails the review with no verdict (default 20m; 0 disables
both bounds). A timed-out reviewer's siblings and the judge
still proceed.
--timeout DUR optional hard cap on each reviewer before it's cancelled and
marked failed. No default — reviewers run until they finish,
like a directly-invoked skill. A positive value also bounds
the consolidating judge, which otherwise keeps its own 20m
default (the judge is never unbounded; its timeout or error
fails the review with no verdict). A timed-out reviewer's
siblings and the judge still 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,
Mcmd/entire/cli/review/cmd.go+7/-5
89 unmodified lines
90
91
92
93
94
93
94
95
96
97
98
99
100
101
89 unmodified lines
// defaultSynthesisProviderTimeout bounds the judge's single consolidation call
// when SynthesisSink.ProviderTimeout is unset. The judge reads every reviewer's
// report and writes the combined verdict in one text-generation call, which
// regularly needs more than the original 2m, so the default is 5m.
const defaultSynthesisProviderTimeout = 5 * time.Minute
// regularly needs more than the original 2m. 20m matches the judge's previous
// effective bound: before the reviewer default was dropped, the --timeout flag
// default (20m) always flowed into ProviderTimeout on the no-flag path, so
// keeping 5m here would have silently tightened the judge 4x — and a judge
// timeout discards an entire multi-reviewer run with no verdict.
const defaultSynthesisProviderTimeout = 20 * time.Minute
// AgentEvent is a no-op; SynthesisSink only acts in RunFinished.
func (SynthesisSink) AgentEvent(_ string, _ reviewtypes.Event) {}
Mcmd/entire/cli/review/synthesis_sink.go+6/-2
311 unmodified lines
312
313
314
315
315
316
317
318
319
7 unmodified lines
327
328
329
329
330
331
331
332
332
333
334
335
336
311 unmodified lines
// TestSynthesisSink_DefaultProviderTimeoutValue pins the judge's default
// deadline (~5m) when ProviderTimeout is unset, so an accidental change to
// deadline (~20m, the flag default's previous effective bound) when
// ProviderTimeout is unset, so an accidental change to
// defaultSynthesisProviderTimeout is caught rather than passing silently.
func TestSynthesisSink_DefaultProviderTimeoutValue(t *testing.T) {
t.Parallel()
7 unmodified lines
if !provider.hadDeadline {
t.Fatal("unset ProviderTimeout must apply the default deadline")
}
// The default is 5m; allow generous slack for scheduling between context
// The default is 20m; allow generous slack for scheduling between context
// creation and the provider reading the deadline.
if provider.remaining < 4*time.Minute || provider.remaining > 5*time.Minute {
t.Fatalf("default deadline remaining = %v, want ~5m", provider.remaining)
}
if provider.remaining < 19*time.Minute || provider.remaining > 20*time.Minute {
t.Fatalf("default deadline remaining = %v, want ~20m", provider.remaining)
}
}
Mcmd/entire/cli/review/synthesis_sink_test.go+5/-4