Merge pull request #1664 from entireio/feat/review-no-default-timeout · Entire
Merge pull request #1664 from entireio/feat/review-no-default-timeout
7ef5a0f→main·
peyton-alt·1w ago·6 files·+96 added/-96 removed
feat(review): drop the default reviewer timeout — reviewers run until done
Changes
6
cmd/entire/cli/review
Mcmd.go+23/-24
Mrun.go+13/-22
Mrun_test.go+43/-39
Msynthesis_sink.go+6/-2
Msynthesis_sink_test.go+6/-5
types
Mreviewer.go+5/-4
133 unmodified lines
134
135
136
137
138
139
140
141
137
138
139
140
141
142
143
144
145
146
78 unmodified lines
225
226
227
226
227
228
229
230
231
232
228
229
230
231
232
233
234
235
236
14 unmodified lines
251
252
253
253
254
255
256
257
455 unmodified lines
713
714
715
715
716
717
718
719
720
721
722
723
724
716
717
718
719
720
721
722
723
724
725
726
503 unmodified lines
1230
1231
1232
1234
1233
1234
1235
1236
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,
78 unmodified lines
if findings {
return runReviewFindings(ctx, cmd, positionalArg, deps.NewSilentError)
}
// Map the flag to the RunConfig timeout convention: a non-positive
// value (the user passed --timeout 0) means "disable", encoded as the
// negative sentinel, which disables BOTH the per-reviewer bound and the
// judge's deadline. A positive value passes through and bounds both.
// (The flag's default is nonzero, so 0 only appears on --timeout 0.)
timeoutArg := resolveReviewerTimeoutArg(reviewTimeout)
return runReview(ctx, cmd, agentOverride, modelOverride, baseOverride, profileName, perRunPrompt, timeoutArg, deps)
// The flag flows through unmapped: RunConfig.ReviewerTimeout is
// two-state (positive = hard cap, anything else = no cap), so the
// default 0, an explicit --timeout 0, and a negative all mean
// "reviewers run until done". The judge derives its own bound via
// judgeTimeoutArg and is never uncapped.
return runReview(ctx, cmd, agentOverride, modelOverride, baseOverride, profileName, perRunPrompt, reviewTimeout, deps)
},
}
cmd.Flags().BoolVar(&configure, "configure", false, "set up a review profile; shows available agents and accepts --set-* flags for non-interactive config")
14 unmodified lines
cmd.Flags().StringVar(&profileOverride, "profile", "", "review profile to run (default: review_default_profile or general)")
cmd.Flags().StringVar(&perRunPrompt, "prompt", "", "one-off instructions appended to this review run")
cmd.Flags().StringVar(&baseOverride, "base", "", "git ref to scope the review against (default: origin/HEAD → origin/main → origin/master → main → master)")
cmd.Flags().DurationVar(&reviewTimeout, "timeout", defaultReviewerTimeout, "max time each reviewer may run before it is cancelled and marked failed; also bounds the consolidating judge, whose timeout or error fails the review (0 disables both)")
cmd.Flags().DurationVar(&reviewTimeout, "timeout", 0, "optional hard cap per reviewer (default: none — reviewers run until they finish, like a skill invoked directly in a session). When set, it also bounds the consolidating judge; unset, the judge keeps its own 20m default")
// The listing modes and the action modes each select a distinct command
// behavior; combining them silently runs one and drops the rest, so reject
// the combination up front with a clear cobra error.
455 unmodified lines
return names
}
// resolveReviewerTimeoutArg maps the --timeout flag value to the RunConfig
// timeout convention used by reviewerTimeout and the judge's providerContext: a
// non-positive value (the user passed --timeout 0) becomes the negative
// "disabled" sentinel; a positive value passes through unchanged. The flag's
// default is nonzero, so 0 only reaches here when the user explicitly set it.
func resolveReviewerTimeoutArg(flagValue time.Duration) time.Duration {
if flagValue <= 0 {
return -1
}
return flagValue
// judgeTimeoutArg maps the reviewer --timeout value to the judge's
// ProviderTimeout. The judge is a single text-generation call with no event
// stream, so unlike reviewers it always keeps a bound: an explicit positive
// --timeout governs it, anything else (unset, 0, or a negative like
// `--timeout -5m`) maps to 0 so the synthesis default (20m) applies — a
// reviewer-side "no cap" must never leak through as "judge unbounded".
func judgeTimeoutArg(reviewerArg time.Duration) time.Duration {
return max(reviewerArg, 0)
}
// runReview executes the main review flow.
503 unmodified lines
profileName: profileName,
task: profile.Task,
masterName: masterLabel,
judgeTimeout: timeout,
judgeTimeout: judgeTimeoutArg(timeout),
onSynthesisResult: func(result string) {
aggregateOutput = result
},
Mcmd/entire/cli/review/cmd.go+23/-24
33 unmodified lines
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
37
38
39
40
41
42
43
44
45
46
47
48
49
52
53
54
55
56
57
58
59
50
51
52
53
33 unmodified lines
return ""
}
// defaultReviewerTimeout bounds a single reviewer's run when the caller
// doesn't set RunConfig.ReviewerTimeout. A stuck agent is cancelled (its
// process killed) and marked failed rather than hanging the review forever.
//
// A full reviewer pass (read the diff, run skills, write the report) regularly
// runs past 10m, especially for the consolidating judge, so the default is 20m;
// override with --timeout (0 disables).
const defaultReviewerTimeout = 20 * time.Minute
// reviewerTimeout resolves the effective per-reviewer timeout, distinguishing
// the three RunConfig.ReviewerTimeout states the zero value alone can't:
// - positive: use it.
// - zero (unset): use defaultReviewerTimeout.
// - negative: disabled — return 0, and callers treat 0 as "no timeout".
// reviewerTimeout resolves the effective per-reviewer wall cap. There is
// deliberately NO default: reviewers run until they finish, exactly like the
// same skill invoked in a user's own session. Review time is dominated by
// long-running subagents inside the reviewer (measured: a single legitimate
// review subagent ran 12.6 minutes with zero parent output) — every
// wall-clock default we shipped killed real work at some diff size, and no
// reliable liveness signal exists for a headless child that would let a
// watchdog distinguish "working via a quiet subagent" from "hung". A stuck
// reviewer is Ctrl+C in interactive runs (process-group kill handles it);
// unattended callers that need a bound pass --timeout explicitly.
// - positive: hard cap.
// - zero or negative: no cap.
func reviewerTimeout(cfg reviewtypes.RunConfig) time.Duration {
switch {
case cfg.ReviewerTimeout > 0:
return cfg.ReviewerTimeout
case cfg.ReviewerTimeout < 0:
return 0
default:
return defaultReviewerTimeout
}
return max(cfg.ReviewerTimeout, 0)
}
var errReviewerTimeoutCause = errors.New("reviewer timeout elapsed")
Mcmd/entire/cli/review/run.go+13/-22
995 unmodified lines
996
997
998
999
1000
999
1000
1001
1002
1003
3 unmodified lines
1007
1008
1009
1010
1011
1012
1013
1014
1010
1011
1012
1013
1014
1015
1016
1016
1017
1018
1019
1020
1021
1022
1033
1027
1030
1031
1032
1034
1037
1038
1039
1040
1041
1042
1043
1044
1042
1043
1044
1045
1046
1045
1046
1047
1048
1049
1050
1051
1052
8 unmodified lines
1061
1062
1063
1061
1062
1063
1064
1065
1064
1065
1066
1067
1068
1069
1070
1067
1068
1069
1071
1072
1073
1074
1075
1072
1076
1077
1078
1079
995 unmodified lines
func TestReviewerTimeout(t *testing.T) {
t.Parallel()
if got := reviewerTimeout(reviewtypes.RunConfig{}); got != defaultReviewerTimeout {
t.Errorf("unset = %v, want default %v", got, defaultReviewerTimeout)
}
if got := reviewerTimeout(reviewtypes.RunConfig{}); got != 0 {
t.Errorf("unset = %v, want 0 (no default cap)", got)
}
if got := reviewerTimeout(reviewtypes.RunConfig{ReviewerTimeout: 5 * time.Minute}); got != 5*time.Minute {
t.Errorf("explicit = %v, want 5m", got)
}
}
// TestResolveReviewerTimeoutArg pins the --timeout flag -> RunConfig sentinel
// mapping: a non-positive flag value (the user passed --timeout 0) becomes the
// negative "disabled" sentinel that turns off both the reviewer bound and the
// judge's deadline; a positive value passes through unchanged.
func TestResolveReviewerTimeoutArg(t *testing.T) {
// TestReviewerTimeout_NoDefaultCap pins the deliberate absence of a default
// wall cap: an unset RunConfig.ReviewerTimeout means the reviewer runs until
// it finishes, like a skill invoked directly in a session. Every wall-clock
// default we shipped killed legitimate work at some diff size (reviewers
// spend 10+ minute stretches inside subagents with zero parent output).
}
Mcmd/entire/cli/review/run_test.go+43/-39
89 unmodified lines
90
91
92
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
29 unmodified lines
366
367
368
368
369
370
371
372
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)
}
}
29 unmodified lines
if !provider.hadDeadline {
t.Fatal("explicit ProviderTimeout must apply a deadline")
}
// Generous slack: the deadline should be ~1h out, far above the 5m default.
// Generous slack: the deadline should be ~1h out, far above the 20m default.
if provider.remaining < 30*time.Minute {
t.Fatalf("deadline remaining = %v, want ~1h (explicit timeout not honored, fell back to default)", provider.remaining)
}
}
Mcmd/entire/cli/review/synthesis_sink_test.go+6/-5
130 unmodified lines
131
132
133
134
135
136
137
134
135
136
137
138
139
140
141
130 unmodified lines
// ReviewerTimeout bounds how long a single reviewer may run before the
// orchestrator cancels it (its process is killed and the run is marked
// failed-by-timeout) so a stuck agent can't hang the review forever. Zero
// or negative means use the orchestrator default (defaultReviewerTimeout).
// Sibling reviewers and the judge are unaffected by one reviewer's
// timeout.
// failed-by-timeout). Positive is a hard cap; zero or negative means no
// cap — reviewers run until they finish, like a skill invoked directly
// in a session (there is deliberately no default: every wall-clock
// default shipped killed legitimate long-running work). Sibling
// reviewers and the judge are unaffected by one reviewer's timeout.
ReviewerTimeout time.Duration
// EnrichSummary optionally updates the completed run summary before sinks