diff --git a/internal/llmadapters/pi_rpc.go b/internal/llmadapters/pi_rpc.go index dc8c0de1..b319938f 100644 --- a/internal/llmadapters/pi_rpc.go +++ b/internal/llmadapters/pi_rpc.go @@ -52,11 +52,17 @@ func NewPiRPCAdapter(opts PiRPCOptions) *PiRPCAdapter { if factory == nil { factory = defaultScratchDir } + // Same defaulting as the subprocess adapters: an unbounded task can hang a + // whole review on one stuck worker. Negative disables the bound explicitly. + timeout := opts.Timeout + if timeout == 0 { + timeout = defaultLLMTaskTimeout + } return &PiRPCAdapter{ command: command, commandArgsPrefix: append([]string(nil), opts.commandArgsPrefix...), env: append([]string(nil), opts.Env...), - timeout: opts.Timeout, + timeout: timeout, scratchDirFactory: factory, fastModeModels: append([]string(nil), opts.FastModeModels...), } diff --git a/internal/llmadapters/subprocess.go b/internal/llmadapters/subprocess.go index 88737ecc..929056e8 100644 --- a/internal/llmadapters/subprocess.go +++ b/internal/llmadapters/subprocess.go @@ -29,8 +29,13 @@ var ( // SubprocessOptions configures subprocess LLM adapters. type SubprocessOptions struct { - Command string - Env []string + Command string + Env []string + // Timeout bounds a single LLM task (one subprocess run, or one background + // job launch + result wait). Zero applies defaultLLMTaskTimeout — without a + // deadline, one hung CLI worker or never-terminal background job hangs the + // entire review until something external kills it. Negative disables the + // bound explicitly. Timeout time.Duration ScratchDirFactory ScratchDirFactory AllowBestEffortNoTools bool @@ -38,6 +43,13 @@ type SubprocessOptions struct { commandArgsPrefix []string } +// defaultLLMTaskTimeout bounds a single LLM task when the caller does not set +// SubprocessOptions.Timeout. Healthy reviewer tasks finish in a few minutes; +// ten is generous headroom for slow models while still converting "one worker +// hung" from an indefinite review stall into a timely, clearly-attributed task +// failure. Mirrors defaultAPIClientTimeout on the API adapter. +const defaultLLMTaskTimeout = 10 * time.Minute + type subprocessKind string const ( @@ -104,12 +116,16 @@ func newSubprocessAdapter(kind subprocessKind, defaultCommand string, opts Subpr if factory == nil { factory = defaultScratchDir } + timeout := opts.Timeout + if timeout == 0 { + timeout = defaultLLMTaskTimeout + } return &SubprocessAdapter{ kind: kind, command: command, commandArgsPrefix: append([]string(nil), opts.commandArgsPrefix...), env: append([]string(nil), opts.Env...), - timeout: opts.Timeout, + timeout: timeout, scratchDirFactory: factory, allowBestEffortNoTools: opts.AllowBestEffortNoTools, fastModeModels: append([]string(nil), opts.FastModeModels...), diff --git a/internal/llmadapters/subprocess_test.go b/internal/llmadapters/subprocess_test.go index 05f5541b..615c3872 100644 --- a/internal/llmadapters/subprocess_test.go +++ b/internal/llmadapters/subprocess_test.go @@ -1815,3 +1815,29 @@ func eventually(t *testing.T, timeout time.Duration, condition func() bool) { } t.Fatalf("condition was not met within %s", timeout) } + +func TestSubprocessAdapterDefaultsTaskTimeout(t *testing.T) { + // Without a bound, one hung CLI worker or never-terminal background job + // hangs the entire review indefinitely — the caller in app/runtime.go sets + // no Timeout, so the constructor must default it. + a := NewClaudeCLIAdapter(SubprocessOptions{}) + if a.timeout != defaultLLMTaskTimeout { + t.Fatalf("timeout = %v, want default %v", a.timeout, defaultLLMTaskTimeout) + } + + explicit := NewCodexCLIAdapter(SubprocessOptions{Timeout: 3 * time.Minute}) + if explicit.timeout != 3*time.Minute { + t.Fatalf("explicit timeout = %v, want 3m", explicit.timeout) + } + + // Negative opts out: LaunchProcess only applies a deadline when timeout > 0. + unbounded := NewClaudeCLIAdapter(SubprocessOptions{Timeout: -1}) + if unbounded.timeout >= 0 { + t.Fatalf("negative timeout should be preserved as opt-out, got %v", unbounded.timeout) + } + + pi := NewPiRPCAdapter(PiRPCOptions{}) + if pi.timeout != defaultLLMTaskTimeout { + t.Fatalf("pi timeout = %v, want default %v", pi.timeout, defaultLLMTaskTimeout) + } +}