Skip to content

feat(review): drop the default reviewer timeout — reviewers run until done - #1664

Merged
peyton-alt merged 5 commits into
mainfrom
feat/review-no-default-timeout
Jul 8, 2026
Merged

feat(review): drop the default reviewer timeout — reviewers run until done#1664
peyton-alt merged 5 commits into
mainfrom
feat/review-no-default-timeout

Conversation

@peyton-alt

@peyton-alt peyton-alt commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

https://entire.io/gh/entireio/cli/trails/784

Problem

Reviewers were killed at a wall-clock default (10m, later raised to 20m) that conflates "slow because working" with "hung". Observed live: a reviewer at ~90% completion killed with everything discarded. Plain agent invocation has no such cliff — the user is the liveness check.

Two findings shaped the fix:

  • No inactivity watchdog can replace the wall clock. Legitimate review subagents run 12+ minutes while the reviewer's parent process emits zero output (measured on a real run), so a headless child offers no liveness signal that distinguishes a quiet working subagent from a hang. Any watchdog on available signals re-creates the kill-real-work bug.
  • The duration is the task, not a hang risk. A controlled A/B on the same branch with the same two skills: plain claude session 9m57s vs entire review 9m08s. Reviews take as long as they take, wherever they run.

Change

  • --timeout default: 20m0 (none). Reviewers run until they finish, exactly like a directly-invoked skill. The flag remains as an explicit opt-in hard cap for CI/cost-bounded runs.
  • The judge keeps its own 5m default regardless — it's a single text-generation call; a new judgeTimeoutArg mapping prevents the reviewer's no-cap sentinel leaking through as "judge unbounded".
  • A stuck reviewer in an interactive run is Ctrl+C (process-group kill already handles it, fix(cli): close keyring-interrupt race in Ctrl-C signal-abort #1625-era fix); unattended liveness is the background-mode/durable-run design's job, not a kill timer's.

Testing

  • TestReviewerTimeout_NoDefaultCap, TestJudgeTimeoutArg, and the flag-resolution test updated to pin the new contract
  • mise run test (7,604) green, lint clean

🤖 Generated with Claude Code


Note

Medium Risk
Default behavior change means long or stuck reviewer processes are no longer auto-cancelled unless users pass --timeout; multi-agent reviews may run much longer, though the judge consolidation step remains bounded.

Overview
entire review no longer applies a wall-clock cap to reviewers by default (previously ~20m). Reviewers run until they finish, matching a skill invoked in a normal session; --timeout stays an opt-in hard cap for CI or cost limits, with updated flag help (default 0).

reviewerTimeout drops the implicit default: only a positive ReviewerTimeout enables context.WithTimeout; unset, zero, or the no-cap sentinel means no reviewer deadline.

The consolidating judge is handled separately via judgeTimeoutArg: an explicit positive --timeout still bounds the judge; when reviewers are uncapped, the reviewer sentinel must not disable the judge—the judge keeps its existing ~5m synthesis default.

Tests pin the no-default-cap contract, judge mapping, and flag → resolver behavior.

Reviewed by Cursor Bugbot for commit fbe6fe6. Configure here.

… done

Reviewers were killed at a wall-clock default (10m, later 20m) that
conflated 'slow because working' with 'hung'. Measured: a reviewer at
~90% completion was killed with all work discarded, and legitimate
review subagents run 12+ minutes with zero parent output — so no
inactivity watchdog can save this either (a headless child offers no
liveness signal that distinguishes a quiet working subagent from a
hang). A controlled A/B also showed entire review is not slower than
the same skills invoked directly in a session (9m57s plain vs 9m08s
through entire, same branch, same skills) — the duration is the task,
and killing it at an arbitrary ceiling only discards paid-for work.

Reviewers now run until they finish, exactly like a directly-invoked
skill. --timeout remains as an explicit opt-in hard cap (CI, cost
bounds); its default is 0/none. The judge keeps its own 5m default
regardless — it is a single text-generation call, and the reviewer
no-cap sentinel must not leak into its bound (judgeTimeoutArg).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 01KWY3M0FWWCV0SVB84CPG040G
Copilot AI review requested due to automatic review settings July 7, 2026 10:58
@peyton-alt
peyton-alt requested a review from a team as a code owner July 7, 2026 10:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the entire review command’s timeout semantics so reviewers no longer have a default wall-clock cap. The reviewer timeout flag remains available as an explicit hard limit, while the consolidating judge keeps its own bounded default unless a positive --timeout is provided.

Changes:

  • Remove the default per-reviewer timeout so reviewers run until completion unless explicitly capped via --timeout.
  • Add a judgeTimeoutArg mapping so the reviewer “no-cap” sentinel does not disable the judge timeout (judge keeps its default unless explicitly bounded).
  • Update tests and flag-resolution expectations to pin the new contract.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
cmd/entire/cli/review/run.go Drops the default reviewer timeout and treats non-positive values as “no cap”.
cmd/entire/cli/review/run_test.go Updates and adds tests to lock in the new reviewer/judge timeout contract and flag resolution.
cmd/entire/cli/review/cmd.go Changes --timeout default to 0, introduces judgeTimeoutArg, and wires judge timeout behavior accordingly.

Comment thread cmd/entire/cli/review/cmd.go Outdated
Soph
Soph previously approved these changes Jul 7, 2026
Comment thread cmd/entire/cli/review/cmd.go
Comment thread cmd/entire/cli/review/run.go Outdated
…ack)

Review feedback asked whether the two timeout helpers could collapse to
max(). Tracing every ReviewerTimeout reader shows the truth is stronger
on one side and subtler on the other:

- resolveReviewerTimeoutArg is deleted outright, not simplified. Its -1
  'disabled' sentinel was load-bearing in the old three-state world
  (positive/default-20m/disabled); with the default gone, both remaining
  consumers (reviewerTimeout, judgeTimeoutArg) use a plain > 0 check, so
  -1 and 0 were indistinguishable everywhere and the mapping did
  nothing. The --timeout flag value now flows into RunConfig unmapped.
  This also removes the stale doc comment Copilot flagged ('the flag's
  default is nonzero').

- judgeTimeoutArg stays as a named function — the point is its contract
  (the judge is one bounded call and must never be uncapped, whatever
  the reviewer setting says), not the arithmetic — but its body is now
  max(reviewerArg, 0) as suggested (builtin max; math.Max is float64).
  A negative can still arrive via an explicit '--timeout -5m'.

- types/reviewer.go's ReviewerTimeout doc still described the deleted
  defaultReviewerTimeout three-state; rewritten to the two-state
  contract.

Tests updated to pin the flag->RunConfig two-state flow directly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 01KX0NQY37TYQ2B8DF5CGBA8FJ
peyton-alt and others added 3 commits July 8, 2026 12:57
…ack)

Same shape as judgeTimeoutArg one commit earlier: the named function
stays (its doc comment carries the no-default-timeout rationale), the
two-state arithmetic becomes builtin max.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 01KX0P0XW5TXCPJY4G65B8N82V
…ail findings)

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>
Entire-Checkpoint: 01KX0PWPRKDPWNYPN3PWMT2YQB
The 'restore 20m' commit updated the constant and the Long usage block
but missed three references, leaving --help contradicting itself (flag
string said 5m, usage block said 20m) — a user bounding CI cost would
mis-estimate the judge's ceiling by 4x. Fixed the --timeout flag
registration string, the judgeTimeoutArg doc comment, and the stale
test comment.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Entire-Checkpoint: 01KX0QK2EKA17HTM2NK5Q1GYSV
@peyton-alt
peyton-alt merged commit 7ef5a0f into main Jul 8, 2026
10 checks passed
@peyton-alt
peyton-alt deleted the feat/review-no-default-timeout branch July 8, 2026 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants