fix: add model stream idle timeout

[byapparov]

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

• Provider silence still reaches the existing timeout classification and shutdown path.
• The watchdog suspends during executable local tool calls and resumes after their result or error.
• Timeout values above the runtime timer limit fall back safely instead of wrapping to an immediate timeout.

Implementation

• Port the reviewed idle-timeout implementation and apply the provider/tool suspension fix.
• Add the bounded timer configuration and SDK error mirror for StreamIdleTimeoutError.

Scope Caveat

This does not change provider retry policy or capture raw provider responses.

Test Plan

• Provider-stall fixtures verify timeout persistence and classification.
• A local tool longer than the timeout completes before a subsequent provider stall is timed out; boundary values are covered.

Verification

• 19 focused tests passed.
• Combined headless/timeout validation passed with 26 tests.
• CLI and SDK typechecks, formatting, and diff checks passed.

Risks and Rollout

The default remains five minutes. The environment variable is capped at the maximum supported timer delay; no migration is required.

[aictrl-dev]

Code review

Verdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 1 · ⚪ 2 · 0/4 resolved

• 🟠 packages/cli/src/session/idle.ts:34-40 — Idle timeout suspended forever while a tool runs
• 🟡 packages/cli/src/session/processor.ts:71 — TypeError if streamInput.tools is undefined
• ⚪ packages/sdk/src/gen/types.gen.ts:99-106 — Verify generated SDK types came from codegen
• ⚪ README.md:57-62 — Idle-timeout doc nested under CI/CD section

🤖 Fix all 4 open findings with your agent

Fix the following code review findings on aictrl-dev/cli PR #117 (head branch).
Run the relevant tests/linters after each change.

1. packages/cli/src/session/idle.ts:34-40 — 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.
2. packages/cli/src/session/processor.ts:71 — 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.
3. packages/sdk/src/gen/types.gen.ts:99-106 — 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.
4. README.md:57-62 — 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.

📋 Out-of-diff findings (4)

Sev	Location	Finding
🟠	packages/cli/src/session/idle.ts:34-40	Idle timeout suspended forever while a tool runs
🟡	packages/cli/src/session/processor.ts:71	TypeError if streamInput.tools is undefined
⚪	packages/sdk/src/gen/types.gen.ts:99-106	Verify generated SDK types came from codegen
⚪	README.md:57-62	Idle-timeout doc nested under CI/CD section

Reviewed 10 files · 0 inline · view all 4 findings ↗

---

aictrl · AI code review for fast-moving teams · aictrl.dev

[byapparov]

Review response — PR #117

Verified all four automated findings against 1911854da4f72368bf01db4ac2d04b97bc35239e; three were fixed and one generated-file concern was disproved by the repository's current codegen target.

Issues addressed (pushed to this PR)

• Idle timeout suspended forever while a tool runs — packages/cli/src/session/idle.ts: added a bounded local-tool suspension ceiling and a hung-tool regression test (commit 1911854da4).
• TypeError if streamInput.tools is undefined — packages/cli/src/session/processor.ts: guarded the tools record before looking up an executable local tool (commit 1911854da4).
• Idle-timeout doc nested under CI/CD section — README.md: moved the runtime setting into its own subsection and documented the local-tool ceiling (commit 1911854da4).

Review claims verified false (no change needed)

• "Verify generated SDK types came from codegen" — verified false. packages/sdk/script/build.ts generates src/v2/gen; it only formats the legacy src/gen tree. The scoped legacy type mirrors the runtime StreamIdleTimeoutError schema exactly, so running current codegen cannot produce or replace this edit.

Not addressed here

• None.

[github-actions]

Review

Overall this is a solid implementation: the per-event timer reset, the 2^31-1 setTimeout clamp, the Promise.race in idle.ts (both promises get handlers attached, so no unhandled rejections), the fire-and-forget iterator.return?.() to avoid deadlocking behind a stalled next(), the StreamIdleTimeoutError case placed ahead of the generic /timeout/i regex in run.errors.ts, and the fromError case ordering in message-v2.ts are all correct. Tests cover the important paths. No security issues found.

A few reliability/behavior items worth considering:

1. Pending interactive prompts are now killed by the suspended ceiling (medium)

PermissionNext.ask / question prompts raised inside a tool's execute block the stream, so an unattended interactive session with a pending permission prompt now aborts after 12x the model timeout (1h by default) with StreamIdleTimeoutError, where it previously waited indefinitely. Note the asymmetry: the doom-loop permission prompt in the processor loop body (processor.ts:195) is not timed out (the wrapper's timer only spans iterator.next()), but prompts inside tool execute are. If that's intended, a README note would help; if not, an "awaiting permission" state could opt out of the suspended timer.

2. Local tool ceiling silently overrides explicit tool timeouts (medium)

The bash tool accepts an explicit timeout param with no upper bound (tool/bash.ts:102), so a command like timeout: 7200000 is valid per the tool's contract but will be killed at the 3600000ms ceiling (12 x 5min default) with a terminal, non-retryable error. The tool part is then marked "Tool execution aborted" and the session stops. Since the ceiling is derived from the model stream timeout, raising a tool's own timeout doesn't lift it. Consider clamping/validating tool timeouts against the ceiling, or at least documenting that AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS also caps local tool runs.

3. Timeout errors are terminal, not retried (low)

SessionRetry.retryable returns undefined for StreamIdleTimeoutError (its message isn't JSON), so a transient network stall terminates the run rather than retrying. For headless aictrl run a 5-minute stall becomes a hard failure. If that's the intent (the stable MODEL_STREAM_IDLE_TIMEOUT code suggests so), fine — just flagging that a single retry attempt might be friendlier for flaky proxies.

4. Coverage gap: other streams not wrapped (low)

agent.ts:337 iterates streamObject(...).fullStream unwrapped, and generateObject/summary/compaction calls are likewise unbounded — a stalled stream there still hangs forever. Fine to leave for a follow-up, but the README phrasing ("Model streams have a five-minute idle timeout") slightly overstates coverage.

Minor

• processor.ts:66,82 reads the Flag getter twice per attempt (ms and the Math.min ceiling); snapshotting into a local const would guarantee the two values are consistent. Behaviorally harmless today.
• processor-idle.test.ts mutates process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS globally for the duration of an end-to-end prompt; any concurrently running stream in the same process would inherit the 20-25ms timeout. Fine under bun's sequential per-file execution, just something to keep in mind if tests are ever parallelized.

Nothing here blocks merge in my view — items 1 and 2 are the ones I'd want a deliberate decision on.

Reviewed SHA: 1911854

[aictrl-dev]

Code review

Verdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 2 · ⚪ 4 · 0/6 resolved

• ⚪ packages/cli/src/session/idle.ts:4-9 — Dead default message param in error helper
• ⚪ packages/cli/src/session/idle.ts:36 — Local const `timeout` shadows generator name
• 🟡 packages/cli/src/session/message-v2.ts:410 — New persisted error variant vs older readers
• ⚪ packages/cli/src/session/processor.ts:66 — Dynamic flag getter read twice for one stream
• 🟡 packages/cli/src/session/processor.ts:71-72 — Provider-executed tools can falsely hit idle timeout
• ⚪ packages/cli/test/session/idle.test.ts:151-179 — Flag env-parsing tests live in session/idle.test.ts

🤖 Fix all 6 open findings with your agent

Fix the following code review findings on aictrl-dev/cli PR #117 (head branch).
Run the relevant tests/linters after each change.

1. packages/cli/src/session/idle.ts:4-9 — 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)`.
2. packages/cli/src/session/idle.ts:36 — 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.
3. packages/cli/src/session/message-v2.ts:410 — 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.
4. packages/cli/src/session/processor.ts:66 — 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)`.
5. packages/cli/src/session/processor.ts:71-72 — 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.
6. packages/cli/test/session/idle.test.ts:151-179 — 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.

📋 Out-of-diff findings (6)

Sev	Location	Finding
⚪	packages/cli/src/session/idle.ts:4-9	Dead default message param in error helper
⚪	packages/cli/src/session/idle.ts:36	Local const `timeout` shadows generator name
🟡	packages/cli/src/session/message-v2.ts:410	New persisted error variant vs older readers
⚪	packages/cli/src/session/processor.ts:66	Dynamic flag getter read twice for one stream
🟡	packages/cli/src/session/processor.ts:71-72	Provider-executed tools can falsely hit idle timeout
⚪	packages/cli/test/session/idle.test.ts:151-179	Flag env-parsing tests live in session/idle.test.ts

Reviewed 10 files · 0 inline · view all 6 findings ↗

---

aictrl · AI code review for fast-moving teams · aictrl.dev

[byapparov]

Review response — PR #117

Verified the six findings from the review of 1911854da4f72368bf01db4ac2d04b97bc35239e; five maintainability and timeout-policy issues were fixed, and the persisted-error compatibility claim was disproved against the actual database read path.

Issues addressed (pushed to this PR)

• Dead default message parameter — packages/cli/src/session/idle.ts: removed the unused default from the private error helper (commit ef850e7eed).
• Local timeout variable shadowed the generator name — packages/cli/src/session/idle.ts: renamed it to appliedTimeout (commit ef850e7eed).
• Dynamic flag getter read twice per stream — packages/cli/src/session/processor.ts: snapshot the configured timeout once as idleMs (commit ef850e7eed).
• Provider-executed tools could falsely hit the base idle timeout — packages/cli/src/session/processor.ts: provider-executed calls now receive the same bounded extended tool ceiling as local calls, with a regression case for each path (commit ef850e7eed).
• Flag parsing tests lived with stream-helper tests — moved the cases to packages/cli/test/flag/flag.test.ts (commit ef850e7eed).

Review claims verified false (no change needed)

• "New persisted error variant vs older readers" — verified false. MessageV2.stream() and MessageV2.get() reconstruct database rows with type casts and do not run the Assistant zod discriminated union while deserializing. Older JavaScript readers therefore retain or ignore the additional error object rather than rejecting the persisted message; SDK unions are compile-time declarations and add no runtime parser.

Not addressed here

• None.

[aictrl-dev]

Code review

Verdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 3 · ⚪ 4 · 0/8 resolved

• ⚪ EVENTS.md:173 — MODEL_STREAM_IDLE_TIMEOUT code undocumented
• ⚪ packages/cli/src/cli/cmd/run.errors.ts:27-29 — Instance path yields class name as message
• 🟡 packages/cli/src/flag/flag.ts:8 — Duplicated 2^31-1 setTimeout ceiling constant
• ⚪ packages/cli/src/flag/flag.ts:49-66 — Flag getter breaks dynamic-getter ordering
• ⚪ packages/cli/src/session/idle.ts:11-17 — Signal combining duplicates util/abort pattern
• 🟠 packages/cli/src/session/message-v2.ts:602-608 — Idle-timeout drops completed tool results from history
• 🟡 packages/cli/src/session/processor.ts:57-63 — Idle timer not armed during LLM.stream() setup
• 🟡 README.md:60 — README omits provider-executed tool timeout

🤖 Fix all 8 open findings with your agent

Fix the following code review findings on aictrl-dev/cli PR #117 (head branch).
Run the relevant tests/linters after each change.

1. EVENTS.md:173 — MODEL_STREAM_IDLE_TIMEOUT code undocumented
   Detail: The PR introduces the stable session_error code "MODEL_STREAM_IDLE_TIMEOUT" (classifySessionError) and a new persisted/wire error name StreamIdleTimeoutError, but EVENTS.md — the documented contract for JSON/headless consumers that key alerting on session_error codes — is not updated. The README documents only the env var, not the emitted code, so consumers have no way to discover the new code without reading source.
   Suggested fix: Add MODEL_STREAM_IDLE_TIMEOUT to the session_error code documentation in EVENTS.md, and note the new StreamIdleTimeoutError name in the error union if it is enumerated there.
2. packages/cli/src/cli/cmd/run.errors.ts:27-29 — Instance path yields class name as message
   Detail: NamedError subclasses call super(name), so a live StreamIdleTimeoutError instance has Error.message === "StreamIdleTimeoutError" while the human-readable text lives only in data.message. extractMessage() prefers err.message for Error instances, so if a live instance ever reaches classifySessionError (e.g. via a reject(cause) path rather than the session.error toObject() event path), the classified message would be just the class name. The currently exercised event path passes the plain {name,data} object and works, which is why the unit test passes — this is a latent trap, not a live bug.
   Suggested fix: In extractMessage, prefer data.message when present (check the {name,data} branch before the Error-instance branch), or have NamedError surface data.message as the Error message.
3. packages/cli/src/flag/flag.ts:8 — Duplicated 2^31-1 setTimeout ceiling constant
   Detail: Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MAX = 2_147_483_647 re-encodes the same fact as SessionRetry.RETRY_MAX_DELAY = 2_147_483_647 in packages/cli/src/session/retry.ts, which carries the comment "max 32-bit signed integer for setTimeout". The derivation (the setTimeout ceiling, 2^31-1) now lives in two hand-synchronized copies, and the new one omits the explanatory comment; processor.ts's Math.min(idleMs * LOCAL_TOOL_TIMEOUT_MULTIPLIER, Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MAX) mirrors retry.ts's Math.min clamp, so the two sites can drift apart silently.
   Suggested fix: Export a single shared constant (e.g. TIMER_MAX_MS = 2 ** 31 - 1 in @aictrl/util or a util/timer module) and use it in both Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MAX and SessionRetry.RETRY_MAX_DELAY; at minimum copy the "max 32-bit signed integer for setTimeout" comment onto the new constant and cross-reference the other site.
4. packages/cli/src/flag/flag.ts:49-66 — Flag getter breaks dynamic-getter ordering
   Detail: The file already has dynamic getters ordered AICTRL_DISABLE_PROJECT_CONFIG, AICTRL_CONFIG_DIR, AICTRL_CLIENT, each preceded by the same three-line comment template ("Dynamic getter for X" / "This must be evaluated at access time, not module load time," / "because ..."). The new AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS getter is inserted above all of them (directly after the namespace close) instead of after AICTRL_CLIENT, and its comment replaces the standard formula with a one-off second line, so the section no longer reads uniformly.
   Suggested fix: Move the new Object.defineProperty block below the AICTRL_CLIENT getter and align the comment with the existing template ("Dynamic getter for AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS / This must be evaluated at access time, not module load time, / because the env var may change between model streams.").
5. packages/cli/src/session/idle.ts:11-17 — Signal combining duplicates util/abort pattern
   Detail: The repo already centralizes abort/signal combining in packages/cli/src/util/abort.ts (abortAfter/abortAfterAny using AbortSignal.any, with a GC note about arrow-closure captures) and promise/timer racing in util/timeout.ts. StreamIdle.signal re-implements the AbortSignal.any combine locally in session/idle.ts rather than composing the shared helpers, so future signal-combining call sites now have two patterns to follow. The overlap is partial (abortAfterAny bakes the timeout in at creation, while StreamIdle resets per event), hence the low severity.
   Suggested fix: Either move the signal-combining helper into util/abort.ts (e.g. a combine(...signals: AbortSignal[]) next to abortAfterAny) and have StreamIdle use it, or add a brief comment in idle.ts noting why util/abort.ts's helpers don't fit the per-event reset semantics.
6. packages/cli/src/session/message-v2.ts:602-608 — Idle-timeout drops completed tool results from history
   Detail: When the new StreamIdleTimeoutError is persisted on an assistant message, toModelMessages() skips the ENTIRE message — only AbortedError with real content is exempted from the skip at lines 601-609. The idle timeout fires precisely after tool parts completed (the PR's own processor-idle.test.ts asserts tool part status "completed" together with assistant.error being StreamIdleTimeoutError), so on the next turn the model sees neither its tool calls nor their results and may re-issue non-idempotent tools (e.g. bash deploys), duplicating side effects and discarding a possibly hour-long tool's output. Previously a hung session left the message un-errored and completed tool parts were included in history (pending ones mapped to an interrupted-output error).
   Suggested fix: Extend the existing AbortedError exemption in toModelMessages() to StreamIdleTimeoutError, i.e. treat (AbortedError || StreamIdleTimeoutError) with at least one non-step-start/reasoning part as includable, letting the existing pending/running-to-output-error mapping handle interrupted tools so completed tool results stay in model history.
7. packages/cli/src/session/processor.ts:57-63 — Idle timer not armed during LLM.stream() setup
   Detail: The idle timer only arms when StreamIdle.timeout's first next() runs, i.e. AFTER `await LLM.stream(...)` resolves. That prologue awaits provider resolution (models.dev registry state + dynamic npm SDK module load), config and auth lookups, and plugin hooks — any of which can block on network/filesystem I/O with no timeout. A hang in stream setup therefore still hangs the session forever, so the all-additive wrapper only fixes the event-iteration half of the stalled-provider class the PR targets.
   Suggested fix: Race the `await LLM.stream(...)` call against the same idle budget (aborting idle.controller on expiry), or restructure so the timeout spans the whole per-attempt stream lifecycle including setup.
8. README.md:60 — README omits provider-executed tool timeout
   Detail: The new docs say "Local tool execution uses a ceiling twelve times the configured model timeout", but processor.ts suspends the idle timer for provider-executed tools too — the updateSuspended callback adds a running tool when `value.providerExecuted || typeof streamInput.tools?.[value.toolName]?.execute === "function"`, and the PR's own test parametrizes the ["local", "provider-executed"] cases. The README understates the behavior for the provider-executed path this PR explicitly introduces and tests.
   Suggested fix: Reword to "Tool execution (local or provider-executed) uses a ceiling twelve times the configured model timeout (one hour by default)." so the docs match the suspension condition in processor.ts.

📋 Out-of-diff findings (8)

Sev	Location	Finding
⚪	EVENTS.md:173	MODEL_STREAM_IDLE_TIMEOUT code undocumented
⚪	packages/cli/src/cli/cmd/run.errors.ts:27-29	Instance path yields class name as message
🟡	packages/cli/src/flag/flag.ts:8	Duplicated 2^31-1 setTimeout ceiling constant
⚪	packages/cli/src/flag/flag.ts:49-66	Flag getter breaks dynamic-getter ordering
⚪	packages/cli/src/session/idle.ts:11-17	Signal combining duplicates util/abort pattern
🟠	packages/cli/src/session/message-v2.ts:602-608	Idle-timeout drops completed tool results from history
🟡	packages/cli/src/session/processor.ts:57-63	Idle timer not armed during LLM.stream() setup
🟡	README.md:60	README omits provider-executed tool timeout

Reviewed 11 files · 0 inline · view all 8 findings ↗

---

aictrl · AI code review for fast-moving teams · aictrl.dev

[byapparov]

Review response — PR #117

Verified the eight findings from the 2026-09-29 review against the rebased head d95444e6e2: five were fixed on this branch and three maintainability claims were checked and rejected with evidence. Product decision included in this push: the model stream idle timeout is now off by default (AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS=0); users opt in with a positive millisecond value (for example 300000).

Issues addressed (pushed to this PR)

• MODEL_STREAM_IDLE_TIMEOUT code undocumented — EVENTS.md: documented that an idle timeout emits session_error with reason: "timeout" and code: "MODEL_STREAM_IDLE_TIMEOUT", and persists a StreamIdleTimeoutError on the assistant message (commit d95444e6e2).
• Instance path yields class name as message — packages/cli/src/cli/cmd/run.errors.ts: extractMessage now prefers data.message before Error.message, with a live-instance regression test (commit d95444e6e2).
• Idle-timeout drops completed tool results from history — packages/cli/src/session/message-v2.ts: extended the existing aborted-message exemption in toModelMessages() to StreamIdleTimeoutError messages with visible parts, so completed tool output stays in model history while pending/running calls still map to interrupted outputs; regression test added (commit d95444e6e2).
• Idle timer not armed during LLM.stream() setup — packages/cli/src/session/processor.ts / idle.ts: the timer now also covers LLM.stream() setup and aborts the provider signal on expiry; per-event reset and the bounded tool ceiling are unchanged; setup-stall tests added (commit d95444e6e2).
• README omits provider-executed tool timeout — README.md: states that both local and provider-executed tools use the 12x ceiling, and documents the new opt-in default and accepted range (commit d95444e6e2).

Review claims verified false (no change needed)

• "Duplicated 2^31-1 setTimeout ceiling constant" — verified false. The retry-delay clamp and the user-configurable stream-timeout bound independently derive from the JavaScript timer limit; there is no inconsistent value or behaviour, and sharing one constant would couple the retry policy to flag parsing. Boundary tests in test/flag/flag.test.ts cover the flag limit.
• "Flag getter breaks dynamic-getter ordering" — verified false. The getter is evaluated at access time with the same property descriptor as its siblings; source order carries no declared or runtime contract, and the dynamic-override test passes.
• "Signal combining duplicates util/abort pattern" — verified false. abortAfterAny arms a fixed countdown at creation, whereas the stream idle timer resets on every event and extends for tool execution, so the shared helper cannot replace the per-stream controller. Existing signal tests cover caller and local aborts.

Not addressed here

• None.

Verification: full CLI suite (1,492 passed, 7 skipped, 0 failed), bun turbo typecheck (6/6), Prettier check and SDK build all pass; each fix was mutation-checked by temporarily reverting it and confirming a focused test fails.