Skip to content
Closed
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
6 changes: 6 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ In headless mode, Aictrl automatically rejects all interactive permission reques
### 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 positive decimal integer of milliseconds to
override the timeout, or `0` to disable it. Missing, empty, negative, fractional, and
non-decimal, non-numeric, or unsafe integer values use the 300000 ms default.

## GitHub Integration

Aictrl includes a specialized GitHub agent that can be installed into your repositories to automate PR reviews, issue triage, and code generation.
Expand Down
3 changes: 3 additions & 0 deletions packages/cli/src/cli/cmd/run.errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ export function classifySessionError(err: unknown): ClassifiedSessionError {
if (status === 429) return { reason: "rate_limit", code: "429", message }
if (status === 401 || status === 403) return { reason: "auth", code: String(status), message }
if (name === "ProviderAuthError") return { reason: "auth", code: status ? String(status) : undefined, message }
if (name === "StreamIdleTimeoutError") {
return { reason: "timeout", code: "MODEL_STREAM_IDLE_TIMEOUT", message }
}
if (name === "AbortError" || /timeout/i.test(message)) {
return { reason: "timeout", code: status ? String(status) : undefined, message }
}
Expand Down
15 changes: 15 additions & 0 deletions packages/cli/src/flag/flag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ function truthy(key: string) {
}

export namespace Flag {
export const AICTRL_MODEL_STREAM_IDLE_TIMEOUT_DEFAULT = 5 * 60 * 1000
export const AICTRL_GIT_BASH_PATH = process.env["AICTRL_GIT_BASH_PATH"]
export const AICTRL_CONFIG = process.env["AICTRL_CONFIG"]
export declare const AICTRL_CONFIG_DIR: string | undefined
Expand All @@ -20,6 +21,7 @@ export namespace Flag {
export const AICTRL_FAKE_VCS = process.env["AICTRL_FAKE_VCS"]
export declare const AICTRL_CLIENT: string
export const AICTRL_ENABLE_QUESTION_TOOL = truthy("AICTRL_ENABLE_QUESTION_TOOL")
export declare const AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS: number

// Experimental
export const AICTRL_EXPERIMENTAL = truthy("AICTRL_EXPERIMENTAL")
Expand All @@ -43,6 +45,19 @@ export namespace Flag {
}
}

// Dynamic getter for AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS.
// Evaluated at access time so environment overrides take effect for each model stream.
Object.defineProperty(Flag, "AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS", {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Lazy flag getter missing rationale comment its sibling has.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #99, packages/cli/src/flag/flag.ts:48-56):

Problem: Lazy flag getter missing rationale comment its sibling has
Detail: The pre-existing `AICTRL_DISABLE_PROJECT_CONFIG` getter is preceded by a 3-line comment explaining it is defined via `Object.defineProperty` so the env var is read at access time rather than module load. The new `AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS` getter uses the same pattern (and relies on the same lazy-read semantics — the test mutates `process.env` at runtime and re-reads `Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS`), but omits the rationale comment, breaking the local convention for documenting this pattern.
Suggested fix: Add a short comment mirroring the sibling, e.g. `// Dynamic getter for AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS. Evaluated at access time, not module load, so env overrides and tests that mutate process.env take effect.`

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 pre-existing AICTRL_DISABLE_PROJECT_CONFIG getter is preceded by a 3-line comment explaining it is defined via Object.defineProperty so the env var is read at access time rather than module load. The new AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS getter uses the same pattern (and relies on the same lazy-read semantics — the test mutates process.env at runtime and re-reads Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS), but omits the rationale comment, breaking the local convention for documenting this pattern.

// Dynamic getter for AICTRL_DISABLE_PROJECT_CONFIG
// This must be evaluated at access time, not module load time,
// because external tooling may set this env var at runtime

Object.defineProperty(Flag, "AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS", {
  get() {
    const value = process.env["AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS"]
    if (value === undefined || value.trim() === "") return Flag.MODEL_STREAM_IDLE_TIMEOUT_DEFAULT
    const parsed = Number(value)
    return Number.isInteger(parsed) && parsed >= 0 ? parsed : Flag.MODEL_STREAM_IDLE_TIMEOUT_DEFAULT
  },
  enumerable: true,
  configurable: false,
})

get() {
const value = process.env["AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS"]
if (value === undefined || value.trim() === "") return Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_DEFAULT
const parsed = /^\d+$/.test(value.trim()) ? Number(value) : Number.NaN
return Number.isSafeInteger(parsed) ? parsed : Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_DEFAULT
},
enumerable: true,
configurable: false,
})

// Dynamic getter for AICTRL_DISABLE_PROJECT_CONFIG
// This must be evaluated at access time, not module load time,
// because external tooling may set this env var at runtime
Expand Down
44 changes: 44 additions & 0 deletions packages/cli/src/session/idle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { MessageV2 } from "./message-v2"

export namespace StreamIdle {
export function signal(input?: AbortSignal) {
const controller = new AbortController()
return {
controller,
signal: input ? AbortSignal.any([input, controller.signal]) : controller.signal,
}
}

export async function* timeout<T>(stream: AsyncIterable<T>, ms: number, abort: () => void) {
if (ms === 0) {
yield* stream
return
}

const iterator = stream[Symbol.asyncIterator]()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 Idle wrapper never releases inner iterator on exit.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #99, packages/cli/src/session/idle.ts:10-25):

Problem: Idle wrapper never releases inner iterator on exit
Detail: `StreamIdle.timeout` pulls `stream[Symbol.asyncIterator]()` and drives it manually with `iterator.next()`, but the generator has no `try/finally` that calls `iterator.return()` when it unwinds via the idle-timeout rejection, a propagated provider error, or a consumer `break`/`return`. The only cleanup hook is the `abort()` callback, which fires solely on the timeout path; every non-timeout early exit (and any timeout where the provider SDK does not synchronously settle the in-flight `next()` on abort) leaves the underlying HTTP socket / SSE reader and the still-pending `iterator.next()` promise dangling. This violates the async-iterator protocol that wrappers must forward `.return()` to the inner iterator, and over many sessions can leak file descriptors / sockets (slow-burn DoS). The included `idle.test.ts` does not catch it because its mock iterator exposes no `return()` method.
Suggested fix: Wrap the `while` loop in `try { ... } finally { try { await iterator.return?.() } catch {} }` so the inner iterator is released on every exit path (timeout, propagated error, early break, normal completion). Also consider awaiting `iterator.return?.()` inside the `setTimeout` handler right after `abort()` so the cleanup happens immediately on timeout rather than relying on the generator's outer finally.

Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters

StreamIdle.timeout pulls stream[Symbol.asyncIterator]() and drives it manually with iterator.next(), but the generator has no try/finally that calls iterator.return() when it unwinds via the idle-timeout rejection, a propagated provider error, or a consumer break/return. The only cleanup hook is the abort() callback, which fires solely on the timeout path; every non-timeout early exit (and any timeout where the provider SDK does not synchronously settle the in-flight next() on abort) leaves the underlying HTTP socket / SSE reader and the still-pending iterator.next() promise dangling. This violates the async-iterator protocol that wrappers must forward .return() to the inner iterator, and over many sessions can leak file descriptors / sockets (slow-burn DoS). The included idle.test.ts does not catch it because its mock iterator exposes no return() method.

    if (ms === 0) {
      yield* stream
      return
    }

    const iterator = stream[Symbol.asyncIterator]()
    while (true) {
      const timer = Promise.withResolvers<never>()
      const id = setTimeout(() => {
        timer.reject(
          new MessageV2.StreamIdleTimeoutError({
            message: `Model stream produced no events for ${ms}ms`,
            timeout: ms,
          }),
        )
        abort()
      }, ms)
      const next = await Promise.race([iterator.next(), timer.promise]).finally(() => clearTimeout(id))
      if (next.done) return
      yield next.value
    }

try {
while (true) {
const timer = Promise.withResolvers<never>()
const id = setTimeout(() => {
timer.reject(
new MessageV2.StreamIdleTimeoutError({
message: `Model stream produced no events for ${ms}ms`,
timeout: ms,
}),
)
abort()
}, ms)
const next = await Promise.race([iterator.next(), timer.promise]).finally(() => clearTimeout(id))
if (next.done) return
yield next.value
}
} finally {
// Do not await cleanup: an async generator queues return() behind an
// in-flight next(), which may never settle for the stalled stream we are
// escaping. The abort above gives cooperative providers a chance to close.
try {
iterator.return?.().catch(() => {})
} catch {}
}
}
}
10 changes: 10 additions & 0 deletions packages/cli/src/session/message-v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ import type { Provider } from "@/provider/provider"
export namespace MessageV2 {
export const OutputLengthError = NamedError.create("MessageOutputLengthError", z.object({}))
export const AbortedError = NamedError.create("MessageAbortedError", z.object({ message: z.string() }))
export const StreamIdleTimeoutError = NamedError.create(
"StreamIdleTimeoutError",
z.object({
message: z.string(),
timeout: z.number(),
}),
)
export const StructuredOutputError = NamedError.create(
"StructuredOutputError",
z.object({
Expand Down Expand Up @@ -400,6 +407,7 @@ export namespace MessageV2 {
NamedError.Unknown.Schema,
OutputLengthError.Schema,
AbortedError.Schema,
StreamIdleTimeoutError.Schema,
StructuredOutputError.Schema,
ContextOverflowError.Schema,
APIError.Schema,
Expand Down Expand Up @@ -817,6 +825,8 @@ export namespace MessageV2 {
cause: e,
},
).toObject()
case MessageV2.StreamIdleTimeoutError.isInstance(e):
return e.toObject()
case MessageV2.OutputLengthError.isInstance(e):
return e
case LoadAPIKeyError.isInstance(e):
Expand Down
14 changes: 12 additions & 2 deletions packages/cli/src/session/processor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ import type { Provider } from "@/provider/provider"
import { LLM } from "./llm"
import { Config } from "@/config/config"
import { SessionCompaction } from "./compaction"
import { StreamIdle } from "./idle"
import { PermissionNext } from "@/permission/next"
import { Question } from "@/question"
import { Flag } from "@/flag/flag"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 New imports break the file's import grouping.

Suggested change
import { Flag } from "@/flag/flag"
Move `import { StreamIdle } from \"./idle\"` next to `import { SessionCompaction } from \"./compaction\"`, and `import { Flag } from \"@/flag/flag\"` next to `import { PermissionNext } from \"@/permission/next\"`.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #99, packages/cli/src/session/processor.ts:20-21):

Problem: New imports break the file's import grouping
Detail: The existing import block in `processor.ts` groups by source kind: relative (`./compaction`), then `@/` aliases (`@/permission/next`, `@/question`), then external (`@aictrl/util/error`). The new `import { StreamIdle } from \"./idle\"` (relative) and `import { Flag } from \"@/flag/flag\"` (alias) are appended after `@aictrl/util/error`, violating that grouping.
Suggested fix: Move `import { StreamIdle } from \"./idle\"` next to `import { SessionCompaction } from \"./compaction\"`, and `import { Flag } from \"@/flag/flag\"` next to `import { PermissionNext } from \"@/permission/next\"`.

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 existing import block in processor.ts groups by source kind: relative (./compaction), then @/ aliases (@/permission/next, @/question), then external (@aictrl/util/error). The new import { StreamIdle } from \"./idle\" (relative) and import { Flag } from \"@/flag/flag\" (alias) are appended after @aictrl/util/error, violating that grouping.

import { SessionCompaction } from "./compaction"
import { PermissionNext } from "@/permission/next"
import { Question } from "@/question"
import { NamedError } from "@aictrl/util/error"
import { StreamIdle } from "./idle"
import { Flag } from "@/flag/flag"

import { NamedError } from "@aictrl/util/error"

export namespace SessionProcessor {
Expand Down Expand Up @@ -51,9 +53,17 @@ export namespace SessionProcessor {
try {
let currentText: MessageV2.TextPart | undefined
let reasoningMap: Record<string, MessageV2.ReasoningPart> = {}
const stream = await LLM.stream(streamInput)
const idle = StreamIdle.signal(streamInput.abort)
const stream = await LLM.stream({
...streamInput,
abort: idle.signal,
})

for await (const value of stream.fullStream) {
for await (const value of StreamIdle.timeout(
stream.fullStream,
Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS,
() => idle.controller.abort(),
)) {
input.abort.throwIfAborted()
switch (value.type) {
case "start":
Expand Down
13 changes: 13 additions & 0 deletions packages/cli/test/cli/classify-session-error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,19 @@ import { describe, expect, test } from "bun:test"
import { classifySessionError } from "../../src/cli/cmd/run.errors"

describe("classifySessionError (#63)", () => {
test("model stream idle timeout has a stable timeout code", () => {
expect(
classifySessionError({
name: "StreamIdleTimeoutError",
data: { message: "Model stream produced no events for 300000ms", timeout: 300000 },
}),
).toEqual({
reason: "timeout",
code: "MODEL_STREAM_IDLE_TIMEOUT",
message: "Model stream produced no events for 300000ms",
})
})

test("HTTP 429 → rate_limit", () => {
const res = classifySessionError({ status: 429, message: "Rate limit exceeded" })
expect(res.reason).toBe("rate_limit")
Expand Down
147 changes: 147 additions & 0 deletions packages/cli/test/session/idle.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
import { describe, expect, test } from "bun:test"
import { StreamIdle } from "../../src/session/idle"
import { MessageV2 } from "../../src/session/message-v2"
import { Flag } from "../../src/flag/flag"

describe("model stream idle timeout", () => {
test("uses its own abort signal when the caller signal is undefined", () => {
const idle = StreamIdle.signal()

expect(idle.signal.aborted).toBe(false)
idle.controller.abort()
expect(idle.signal.aborted).toBe(true)
})

test("combines a supplied caller signal with its timeout controller", () => {
const caller = new AbortController()
const idle = StreamIdle.signal(caller.signal)

caller.abort()
expect(idle.signal.aborted).toBe(true)
})

test("fails and aborts a stream whose next event stalls", async () => {
const pending = Promise.withResolvers<IteratorResult<string>>()
let aborted = false
const stream = {
[Symbol.asyncIterator]() {
return {
next: () => pending.promise,
}
},
}

const result = StreamIdle.timeout(stream, 10, () => {
aborted = true
})
const error = await result.next().catch((error) => error)

expect(aborted).toBe(true)
expect(MessageV2.StreamIdleTimeoutError.isInstance(error)).toBe(true)
expect(error.data).toEqual({
message: "Model stream produced no events for 10ms",
timeout: 10,
})
})

test("calls return on the inner iterator after an idle timeout", async () => {
const pending = Promise.withResolvers<IteratorResult<string>>()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 "releases iterator" test gives false confidence.

Suggested change
const pending = Promise.withResolvers<IteratorResult<string>>()
Either rename to "calls return() on the inner iterator after an idle timeout" to reflect what is actually asserted, or add a second test using a real `async function*` whose `next()` is gated on a promise that `abort()` resolves, demonstrating that release actually requires abort to propagate to the underlying stream.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #99, packages/cli/test/session/idle.test.ts:48-65):

Problem: "releases iterator" test gives false confidence
Detail: The test "releases the inner iterator after an idle timeout" uses a hand-rolled iterator whose `return()` is a plain async function — invoking it runs the body synchronously and sets `released = true`. Real iterator targets (the LLM SDK's `fullStream`, async generators in general) queue `.return()` behind an in-flight `.next()` per ECMA-262: the enqueued return-completion only runs once the pending next() settles. The production code's own comment in `idle.ts` (lines 36-38) acknowledges this ("may never settle for the stalled stream we are escaping"). With `next: () => pending.promise` (never resolves) and `abort: () => {}` (no-op), production cleanup depends entirely on `abort()` causing the provider's pending next() to settle; if the provider ignores the abort signal, `return()` never executes. The test asserts `released === true` unconditionally and so masks this production gap — a refactor that breaks abort propagation (or drops the `iterator.return?.()` call in favour of relying solely on abort) could still pass this test for the wrong reason.
Suggested fix: Either rename to "calls return() on the inner iterator after an idle timeout" to reflect what is actually asserted, or add a second test using a real `async function*` whose `next()` is gated on a promise that `abort()` resolves, demonstrating that release actually requires abort to propagate to the underlying stream.

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 test "releases the inner iterator after an idle timeout" uses a hand-rolled iterator whose return() is a plain async function — invoking it runs the body synchronously and sets released = true. Real iterator targets (the LLM SDK's fullStream, async generators in general) queue .return() behind an in-flight .next() per ECMA-262: the enqueued return-completion only runs once the pending next() settles. The production code's own comment in idle.ts (lines 36-38) acknowledges this ("may never settle for the stalled stream we are escaping"). With next: () => pending.promise (never resolves) and abort: () => {} (no-op), production cleanup depends entirely on abort() causing the provider's pending next() to settle; if the provider ignores the abort signal, return() never executes. The test asserts released === true unconditionally and so masks this production gap — a refactor that breaks abort propagation (or drops the iterator.return?.() call in favour of relying solely on abort) could still pass this test for the wrong reason.

  test("releases the inner iterator after an idle timeout", async () => {
    const pending = Promise.withResolvers<IteratorResult<string>>()
    let released = false
    const stream = {
      [Symbol.asyncIterator]() {
        return {
          next: () => pending.promise,
          async return() {
            released = true
            return { done: true as const, value: undefined }
          },
        }
      },
    }

    await StreamIdle.timeout(stream, 10, () => {}).next().catch(() => {})
    expect(released).toBe(true)
  })

let called = false
const stream = {
[Symbol.asyncIterator]() {
return {
next: () => pending.promise,
async return() {
called = true
return { done: true as const, value: undefined }
},
}
},
}

await StreamIdle.timeout(stream, 10, () => {}).next().catch(() => {})
expect(called).toBe(true)
})

test("releases the inner iterator when the consumer stops early", async () => {
let released = false
const stream = {
[Symbol.asyncIterator]() {
return {
value: 0,
async next() {
return { done: false as const, value: ++this.value }
},
async return() {
released = true
return { done: true as const, value: undefined }
},
}
},
}

for await (const _ of StreamIdle.timeout(stream, 100, () => {})) break
expect(released).toBe(true)
})

test("resets after each event instead of limiting total stream duration", async () => {
async function* stream() {
yield 1
await Bun.sleep(8)
yield 2
await Bun.sleep(8)
yield 3
}

const values: number[] = []
for await (const value of StreamIdle.timeout(stream(), 20, () => {
throw new Error("active stream should not abort")
})) {
values.push(value)
}

expect(values).toEqual([1, 2, 3])
})

test("zero disables the timeout", async () => {
async function* stream() {
await Bun.sleep(15)
yield "done"
}

const values = []
for await (const value of StreamIdle.timeout(stream(), 0, () => {
throw new Error("disabled timeout should not abort")
})) {
values.push(value)
}

expect(values).toEqual(["done"])
})
})

describe("AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS", () => {
test("supports default, override, disable, and invalid fallback", () => {
const original = process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS

try {
delete process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS
expect(Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS).toBe(300_000)
process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS = "1234"
expect(Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS).toBe(1234)
process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS = "0"
expect(Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS).toBe(0)
process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS = "invalid"
expect(Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS).toBe(300_000)
process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS = "1e3"
expect(Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS).toBe(300_000)
process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS = "0x10"
expect(Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS).toBe(300_000)
process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS = "9007199254740992"
expect(Flag.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS).toBe(300_000)
} finally {
if (original === undefined) delete process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS
else process.env.AICTRL_MODEL_STREAM_IDLE_TIMEOUT_MS = original
}
})
})
Loading