Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| let suspended = false | ||
| try { | ||
| while (true) { | ||
| if (suspended) { |
There was a problem hiding this comment.
🟠 Idle timeout suspended forever while a tool runs.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, packages/cli/src/session/idle.ts:34-40):
Problem: Idle timeout suspended forever while a tool runs
Detail: While `suspended` is true, the loop awaits `iterator.next()` with no timer and no ceiling. Suspension is entered when the processor callback (packages/cli/src/session/processor.ts:67-79) sees a tool-call for a locally-executed tool and is only cleared by a matching tool-result/tool-error. Two consequences: (1) a tool whose execute() never resolves (hung MCP/HTTP call, dropped tool-result) produces no events, so updateSuspended never runs again and the stream never times out — the exact never-terminating-session symptom this PR fixes (#80) persists whenever the stall originates in tool execution; (2) any unpaired tool-call permanently disables the watchdog for the rest of the stream. Suspending during legitimate long tools is clearly intentional (processor-idle.test.ts test 2), but there is no wall-clock bound on suspension at all, so the protection this PR adds is best-effort rather than a true watchdog.
Suggested fix: Bound the suspension instead of disabling the watchdog: arm a generous ceiling timer in the suspended branch too (e.g. a separate AICTRL_TOOL_IDLE_TIMEOUT_MS, or a multiple of ms), or track per-tool start times and fire StreamIdleTimeoutError when the outstanding tool-call exceeds that bound.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
While suspended is true, the loop awaits iterator.next() with no timer and no ceiling. Suspension is entered when the processor callback (packages/cli/src/session/processor.ts:67-79) sees a tool-call for a locally-executed tool and is only cleared by a matching tool-result/tool-error. Two consequences: (1) a tool whose execute() never resolves (hung MCP/HTTP call, dropped tool-result) produces no events, so updateSuspended never runs again and the stream never times out — the exact never-terminating-session symptom this PR fixes (#80) persists whenever the stall originates in tool execution; (2) any unpaired tool-call permanently disables the watchdog for the rest of the stream. Suspending during legitimate long tools is clearly intentional (processor-idle.test.ts test 2), but there is no wall-clock bound on suspension at all, so the protection this PR adds is best-effort rather than a true watchdog.
let suspended = false
try {
while (true) {
if (suspended) {
const next = await iterator.next()
if (next.done) return
suspended = updateSuspended(next.value)
yield next.value
continue
}| if ( | ||
| value.type === "tool-call" && | ||
| !value.providerExecuted && | ||
| typeof streamInput.tools[value.toolName]?.execute === "function" |
There was a problem hiding this comment.
🟡 TypeError if streamInput.tools is undefined.
--- a/packages/cli/src/session/processor.ts
+++ b/packages/cli/src/session/processor.ts
@@ -68,7 +68,7 @@
(value) => {
if (
value.type === "tool-call" &&
!value.providerExecuted &&
- typeof streamInput.tools[value.toolName]?.execute === "function"
+ typeof streamInput.tools?.[value.toolName]?.execute === "function"
) {
runningTools.add(value.toolCallId)
}🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, packages/cli/src/session/processor.ts:71):
Problem: TypeError if streamInput.tools is undefined
Detail: The optional chain guards only the element access, not `streamInput.tools` itself. If the LLM.stream input is built without a `tools` property (AI SDK's tools param is optional) and a tool-call part still arrives (e.g. a provider-side/built-in tool), `streamInput.tools[value.toolName]` throws a TypeError inside the updateSuspended callback, killing the stream with an UnknownError instead of being handled by the new error mapping.
Suggested fix: Use `streamInput.tools?.[value.toolName]?.execute === "function"` so an absent tools record degrades to "not a local tool" instead of throwing.
Suggested patch:
--- a/packages/cli/src/session/processor.ts
+++ b/packages/cli/src/session/processor.ts
@@ -68,7 +68,7 @@
(value) => {
if (
value.type === "tool-call" &&
!value.providerExecuted &&
- typeof streamInput.tools[value.toolName]?.execute === "function"
+ typeof streamInput.tools?.[value.toolName]?.execute === "function"
) {
runningTools.add(value.toolCallId)
}
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The optional chain guards only the element access, not streamInput.tools itself. If the LLM.stream input is built without a tools property (AI SDK's tools param is optional) and a tool-call part still arrives (e.g. a provider-side/built-in tool), streamInput.tools[value.toolName] throws a TypeError inside the updateSuspended callback, killing the stream with an UnknownError instead of being handled by the new error mapping.
if (
value.type === "tool-call" &&
!value.providerExecuted &&
typeof streamInput.tools[value.toolName]?.execute === "function"
) {
runningTools.add(value.toolCallId)
}| } | ||
| } | ||
|
|
||
| export type StreamIdleTimeoutError = { |
There was a problem hiding this comment.
⚪ Verify generated SDK types came from codegen.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, packages/sdk/src/gen/types.gen.ts:99-106):
Problem: Verify generated SDK types came from codegen
Detail: The `src/gen/` path indicates generated output. The edit itself is shaped correctly (StreamIdleTimeoutError added to both the AssistantMessage.error and EventSessionError.properties.error unions), but if this was a hand-edit rather than the output of the repo's codegen step, the next regeneration may reorder or drop it. Worth confirming codegen was run and committing its verbatim output.
Suggested fix: Re-run the SDK codegen step from the CLI zod schemas and commit its output verbatim so the hand-applied union additions don't drift on the next regeneration.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The src/gen/ path indicates generated output. The edit itself is shaped correctly (StreamIdleTimeoutError added to both the AssistantMessage.error and EventSessionError.properties.error unions), but if this was a hand-edit rather than the output of the repo's codegen step, the next regeneration may reorder or drop it. Worth confirming codegen was run and committing its verbatim output.
export type MessageAbortedError = {
name: "MessageAbortedError"
data: {
message: string
}
}
export type StreamIdleTimeoutError = {
name: "StreamIdleTimeoutError"
data: {
message: string
timeout: number
}
}| Set `AICTRL_HEADLESS=true` in your environment to force headless behavior even in pseudo-TTYs. | ||
|
|
||
| Model streams have a five-minute idle timeout by default. Every stream event resets | ||
| the timer, so long-running responses that continue making progress are unaffected. |
There was a problem hiding this comment.
⚪ Idle-timeout doc nested under CI/CD section.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, README.md:57-62):
Problem: Idle-timeout doc nested under CI/CD section
Detail: The new `AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS` paragraph is appended to the `### CI/CD Integration` subsection, but a model-stream runtime timeout applies to every session, not CI/CD. Riding an unrelated heading makes the knob hard to discover and muddies the section's scope.
Suggested fix: Move the paragraph into its own subsection (e.g. `### Model Stream Idle Timeout`) near the other runtime/env-var documentation, keeping CI/CD Integration scoped to headless/CI behavior.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The new AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS paragraph is appended to the ### CI/CD Integration subsection, but a model-stream runtime timeout applies to every session, not CI/CD. Riding an unrelated heading makes the knob hard to discover and muddies the section's scope.
### CI/CD Integration
Set `AICTRL_HEADLESS=true` in your environment to force headless behavior even in pseudo-TTYs.
Model streams have a five-minute idle timeout by default. Every stream event resets
the timer, so long-running responses that continue making progress are unaffected.
Set `AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS` to a decimal integer of milliseconds through
2147483647 to override the timeout, or `0` to disable it. Missing, empty, negative,
fractional, non-decimal, non-numeric, or unsupported values use the 300000 ms default.
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 1 · ⚪ 2 · 0/4 resolved
🤖 Fix all 4 open findings with your agent📋 Out-of-diff findings (4)
Reviewed 10 files · 0 inline · view all 4 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Review response — PR #117Verified all four automated findings against Issues addressed (pushed to this PR)
Review claims verified false (no change needed)
Not addressed here
|
ReviewOverall this is a solid implementation: the per-event timer reset, the 2^31-1 A few reliability/behavior items worth considering: 1. Pending interactive prompts are now killed by the suspended ceiling (medium)
2. Local tool ceiling silently overrides explicit tool timeouts (medium)The bash tool accepts an explicit 3. Timeout errors are terminal, not retried (low)
4. Coverage gap: other streams not wrapped (low)
Minor
Nothing here blocks merge in my view — items 1 and 2 are the ones I'd want a deliberate decision on. Reviewed SHA: 1911854 |
| NamedError.Unknown.Schema, | ||
| OutputLengthError.Schema, | ||
| AbortedError.Schema, | ||
| StreamIdleTimeoutError.Schema, |
There was a problem hiding this comment.
🟡 New persisted error variant vs older readers.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, packages/cli/src/session/message-v2.ts:410):
Problem: New persisted error variant vs older readers
Detail: StreamIdleTimeoutError is added to the persisted AssistantMessage.error zod union and the SDK wire types (EventSessionError). Older CLI/SDK builds whose error union lacks this variant will fail to parse (or drop) a persisted assistant message saved by this version after a rollback or in mixed-version setups. Worth confirming the deserialize path degrades gracefully (e.g. falls back to NamedError.Unknown) for unknown error names.
Suggested fix: Verify the name-keyed deserializer for persisted errors falls back to an unknown-error schema for unrecognized names, so older readers can still load sessions containing StreamIdleTimeoutError.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
StreamIdleTimeoutError is added to the persisted AssistantMessage.error zod union and the SDK wire types (EventSessionError). Older CLI/SDK builds whose error union lacks this variant will fail to parse (or drop) a persisted assistant message saved by this version after a rollback or in mixed-version setups. Worth confirming the deserialize path degrades gracefully (e.g. falls back to NamedError.Unknown) for unknown error names.
| (value) => { | ||
| if ( | ||
| value.type === "tool-call" && | ||
| !value.providerExecuted && |
There was a problem hiding this comment.
🟡 Provider-executed tools can falsely hit idle timeout.
| !value.providerExecuted && | |
| Also add provider-executed tool-calls to runningTools on `tool-call` (with providerExecuted=true) and remove them on the corresponding tool-result/tool-error, so server-side execution gets the extended suspended timeout; or document that provider-side silence is intentionally bounded by the base idle timeout. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, packages/cli/src/session/processor.ts:71-72):
Problem: Provider-executed tools can falsely hit idle timeout
Detail: The updateSuspended callback only marks a tool as running when `!value.providerExecuted`, so server-side (provider-executed) tool calls are never counted as suspended. While the provider executes such a tool, the fullStream emits no events, so a provider tool running longer than the idle timeout (5 min by default — e.g. long deep-research/computer-use runs) falsely trips StreamIdleTimeoutError and aborts a healthy session. Local tools get a 12x ceiling; provider-executed tools get none. If the base timeout is meant to bound provider silence too, this deserves an explicit comment or doc note; otherwise track provider-executed tool-calls as suspended as well.
Suggested fix: Also add provider-executed tool-calls to runningTools on `tool-call` (with providerExecuted=true) and remove them on the corresponding tool-result/tool-error, so server-side execution gets the extended suspended timeout; or document that provider-side silence is intentionally bounded by the base idle timeout.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The updateSuspended callback only marks a tool as running when !value.providerExecuted, so server-side (provider-executed) tool calls are never counted as suspended. While the provider executes such a tool, the fullStream emits no events, so a provider tool running longer than the idle timeout (5 min by default — e.g. long deep-research/computer-use runs) falsely trips StreamIdleTimeoutError and aborts a healthy session. Local tools get a 12x ceiling; provider-executed tools get none. If the base timeout is meant to bound provider silence too, this deserves an explicit comment or doc note; otherwise track provider-executed tool-calls as suspended as well.
for await (const value of StreamIdle.timeout(
stream.fullStream,
Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS,
() => idle.controller.abort(),
(value) => {
if (
value.type === "tool-call" &&
!value.providerExecuted &&
typeof streamInput.tools?.[value.toolName]?.execute === "function"
) {
runningTools.add(value.toolCallId)
}
if (value.type === "tool-result" || value.type === "tool-error") {
runningTools.delete(value.toolCallId)
}
return runningTools.size > 0
},| try { | ||
| while (true) { | ||
| const timer = Promise.withResolvers<never>() | ||
| const timeout = suspended ? suspendedTimeout : ms |
There was a problem hiding this comment.
⚪ Local const `timeout` shadows generator name.
| const timeout = suspended ? suspendedTimeout : ms | |
| Rename the local to `appliedTimeout` (or similar) and use it in the setTimeout delay, the error construction, and the message. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, packages/cli/src/session/idle.ts:36):
Problem: Local const `timeout` shadows generator name
Detail: Inside `export async function* timeout<T>(...)` the loop declares `const timeout = suspended ? suspendedTimeout : ms`, shadowing the generator's own name. Harmless at runtime but invites confusion and accidental self-reference in future edits of this loop.
Suggested fix: Rename the local to `appliedTimeout` (or similar) and use it in the setTimeout delay, the error construction, and the message.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Inside export async function* timeout<T>(...) the loop declares const timeout = suspended ? suspendedTimeout : ms, shadowing the generator's own name. Harmless at runtime but invites confusion and accidental self-reference in future edits of this loop.
while (true) {\n const timer = Promise.withResolvers<never>()\n const timeout = suspended ? suspendedTimeout : ms\n const id = setTimeout(() => {\n timer.reject(\n error(\n timeout,| import { MessageV2 } from "./message-v2" | ||
|
|
||
| export namespace StreamIdle { | ||
| function error(ms: number, message = `Model stream produced no events for ${ms}ms`) { |
There was a problem hiding this comment.
⚪ Dead default message param in error helper.
--- a/packages/cli/src/session/idle.ts
+++ b/packages/cli/src/session/idle.ts
@@ -1,5 +1,5 @@
import { MessageV2 } from "./message-v2"
export namespace StreamIdle {
- function error(ms: number, message = `Model stream produced no events for ${ms}ms`) {
+ function error(ms: number, message: string) {
return new MessageV2.StreamIdleTimeoutError({
message,
timeout: ms,
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, packages/cli/src/session/idle.ts:4-9):
Problem: Dead default message param in error helper
Detail: The default value of the `message` parameter in the unexported error() helper is never used: its only call site always passes an explicit message for both the suspended and non-suspended cases. Dead default left behind by the suspended-timeout feature.
Suggested fix: Drop the unused default: `function error(ms: number, message: string)`.
Suggested patch:
--- a/packages/cli/src/session/idle.ts
+++ b/packages/cli/src/session/idle.ts
@@ -1,5 +1,5 @@
import { MessageV2 } from "./message-v2"
export namespace StreamIdle {
- function error(ms: number, message = `Model stream produced no events for ${ms}ms`) {
+ function error(ms: number, message: string) {
return new MessageV2.StreamIdleTimeoutError({
message,
timeout: ms,
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The default value of the message parameter in the unexported error() helper is never used: its only call site always passes an explicit message for both the suspended and non-suspended cases. Dead default left behind by the suspended-timeout feature.
export namespace StreamIdle {
function error(ms: number, message = `Model stream produced no events for ${ms}ms`) {
return new MessageV2.StreamIdleTimeoutError({
message,
timeout: ms,
})
}| for await (const value of stream.fullStream) { | ||
| for await (const value of StreamIdle.timeout( | ||
| stream.fullStream, | ||
| Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS, |
There was a problem hiding this comment.
⚪ Dynamic flag getter read twice for one stream.
| Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS, | |
| Capture the value once before the call: `const idleMs = Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS`, then pass `idleMs` and `Math.min(idleMs * LOCAL_TOOL_TIMEOUT_MULTIPLIER, Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MAX)`. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, packages/cli/src/session/processor.ts:66):
Problem: Dynamic flag getter read twice for one stream
Detail: Because AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS is a dynamic getter that re-reads process.env on every access, the processor evaluates it twice when wiring one stream — once for the idle timeout and once inside Math.min(... * LOCAL_TOOL_TIMEOUT_MULTIPLIER, ...). Reading it once into a local const makes the stream's configuration a single snapshot and the multiplier expression easier to read.
Suggested fix: Capture the value once before the call: `const idleMs = Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS`, then pass `idleMs` and `Math.min(idleMs * LOCAL_TOOL_TIMEOUT_MULTIPLIER, Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MAX)`.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Because AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS is a dynamic getter that re-reads process.env on every access, the processor evaluates it twice when wiring one stream — once for the idle timeout and once inside Math.min(... * LOCAL_TOOL_TIMEOUT_MULTIPLIER, ...). Reading it once into a local const makes the stream's configuration a single snapshot and the multiplier expression easier to read.
for await (const value of StreamIdle.timeout(
stream.fullStream,
Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS,
() => idle.controller.abort(),
(value) => {
if (
value.type === "tool-call" &&| }) | ||
| }) | ||
|
|
||
| describe("AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS", () => { |
There was a problem hiding this comment.
⚪ Flag env-parsing tests live in session/idle.test.ts.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #117, packages/cli/test/session/idle.test.ts:151-179):
Problem: Flag env-parsing tests live in session/idle.test.ts
Detail: The file ends with a describe block testing Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS env parsing (default, override, disable, invalid fallbacks), which is behavior of src/flag/flag.ts, not of StreamIdle. The repo's test layout mirrors source modules (e.g. src/cli/cmd/run.errors.ts -> test/cli/classify-session-error.test.ts), so flag parsing tests belong in a test/flag module, keeping the idle helper tests focused.
Suggested fix: Move the AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS env-var cases into a dedicated flag test file (e.g. packages/cli/test/flag/) next to other Flag getter tests.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The file ends with a describe block testing Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS env parsing (default, override, disable, invalid fallbacks), which is behavior of src/flag/flag.ts, not of StreamIdle. The repo's test layout mirrors source modules (e.g. src/cli/cmd/run.errors.ts -> test/cli/classify-session-error.test.ts), so flag parsing tests belong in a test/flag module, keeping the idle helper tests focused.
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 2 · ⚪ 4 · 0/6 resolved
🤖 Fix all 6 open findings with your agent📋 Out-of-diff findings (6)
Reviewed 10 files · 0 inline · view all 6 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Review response — PR #117Verified the six findings from the review of Issues addressed (pushed to this PR)
Review claims verified false (no change needed)
Not addressed here
|
1 participant
Closes #80
Intent
A provider stream can stop producing events indefinitely. The CLI needs a bounded watchdog that reports this as a distinct timeout while allowing long-running local tools to finish.
Expected Impact on Users
Stalled provider streams produce a typed timeout and non-successful headless outcome. Local executable tools are not interrupted merely because their execution exceeds the provider-event timeout.
Expected Outcomes
Implementation
StreamIdleTimeoutError.Scope Caveat
This does not change provider retry policy or capture raw provider responses.
Test Plan
Verification
Risks and Rollout
The default remains five minutes. The environment variable is capped at the maximum supported timer delay; no migration is required.