Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion internal/llmadapters/pi_rpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,17 @@ func NewPiRPCAdapter(opts PiRPCOptions) *PiRPCAdapter {
if factory == nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

File-level note: internal/llmadapters/pi_rpc.go

PiRPCOptions.Timeout is the only exported field in PiRPCOptions whose zero/negative semantics differ from what a reader would assume (zero → 10-minute default, negative → no bound), yet the field has no doc comment while the equivalent SubprocessOptions.Timeout field carries a thorough explanation of both conventions. A caller reading only PiRPCOptions has no in-source signal that passing -1 opts out of the deadline or that omitting the field applies a default. Suggested fix: add the same explanatory comment to PiRPCOptions.Timeout as exists on SubprocessOptions.Timeout, i.e. "Zero applies defaultLLMTaskTimeout; negative disables the bound explicitly." The implementation and tests are otherwise correct.

Reply inline to this comment.

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...),
}
Expand Down
22 changes: 19 additions & 3 deletions internal/llmadapters/subprocess.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,27 @@ 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
FastModeModels []string
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 (
Expand Down Expand Up @@ -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...),
Expand Down
26 changes: 26 additions & 0 deletions internal/llmadapters/subprocess_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Loading