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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Compatibility

- **NDJSON v1 terminal reasons are an open set** — `session_error.reason` now includes `interrupted` for `SIGINT` and `terminated` for `SIGTERM`, and `code` may contain the conventional signal-derived exit code (`130` or `143`). Schema v1 consumers should treat unknown event types, fields, and enum-like string values as forward-compatible additions.

## 0.3.2 (2026-04-11)

### Fixes
Expand Down
6 changes: 3 additions & 3 deletions EVENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ Emitted once when the session ends (success or failure).

> **Deprecated in v1.** Prefer the structured `session_error` event for new consumers. `session_complete.error` remains populated for back-compat and will be removed in a future schema version.
>
> Note: `session_error` is only emitted when the session terminates abnormally (provider/auth/rate-limit/timeout/OOM). Non-fatal errors that accumulate during an otherwise-successful run surface as individual `error` events and may also appear concatenated in `session_complete.error` without a preceding `session_error`. Alerting logic should key on `session_error`, not on `session_complete.error`.
> Note: `session_error` is only emitted when the session terminates abnormally (provider/auth/rate-limit/timeout/OOM/interruption/termination). Non-fatal errors that accumulate during an otherwise-successful run surface as individual `error` events and may also appear concatenated in `session_complete.error` without a preceding `session_error`. Alerting logic should key on `session_error`, not on `session_complete.error`.

### `session_error`

Expand All @@ -170,8 +170,8 @@ Emitted immediately before `session_complete` when the session terminates abnorm
}

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 enum values added to required field without version bump.

Suggested change
}
Either bump schemaVersion to "2" (additive but signals re-validation), or add an explicit CHANGELOG note that reason/code enum sets are open within v1 so pinned consumers have a clear re-check trigger.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, EVENTS.md:104-106):

Problem: New enum values added to required field without version bump
Detail: The PR adds two new values (`interrupted`, `terminated`) to the REQUIRED `session_error.reason` field and broadens the `code` field's semantics, yet `session_start.schemaVersion` stays `"1"`. The pre-PR v1 contract told consumers to treat unknown FIELDS as forward-compatible; this PR retroactively extends that same version's contract to also cover unknown event types and enum-like string values (line 10). A consumer that pinned to schema `"1"` under the original fields-only note and exhaustively validates `reason` against the previously-closed enum receives no version signal to re-audit their parser -- the contract changed under the same number. For a REQUIRED field with a previously-closed enum this is borderline-breaking for strict validators, even though the note update is the intended mitigation.
Suggested fix: Either bump schemaVersion to "2" (additive but signals re-validation), or add an explicit CHANGELOG note that reason/code enum sets are open within v1 so pinned consumers have a clear re-check trigger.

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 PR adds two new values (interrupted, terminated) to the REQUIRED session_error.reason field and broadens the code field's semantics, yet session_start.schemaVersion stays "1". The pre-PR v1 contract told consumers to treat unknown FIELDS as forward-compatible; this PR retroactively extends that same version's contract to also cover unknown event types and enum-like string values (line 10). A consumer that pinned to schema "1" under the original fields-only note and exhaustively validates reason against the previously-closed enum receives no version signal to re-audit their parser -- the contract changed under the same number. For a REQUIRED field with a previously-closed enum this is borderline-breaking for strict validators, even though the note update is the intended mitigation.

```

- `reason` (string, **required**) — one of `rate_limit`, `auth`, `timeout`, `oom`, `provider`, `unknown`.
- `code` (string, optional) — provider HTTP status code or error code when available.
- `reason` (string, **required**) — one of `rate_limit`, `auth`, `timeout`, `oom`, `provider`, `interrupted`, `terminated`, `unknown`. `SIGINT` produces `interrupted`; `SIGTERM` produces `terminated`. Signals are not inferred to be timeouts.
- `code` (string, optional) — provider HTTP status code, error code, or conventional signal-derived exit code (`130` for `SIGINT`, `143` for `SIGTERM`) when available.
- `message` (string, **required**) — human-readable error message.

## Message Events
Expand Down
10 changes: 9 additions & 1 deletion packages/cli/src/cli/cmd/run.errors.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
export const SCHEMA_VERSION = "1"

export type SessionErrorReason = "rate_limit" | "auth" | "timeout" | "oom" | "provider" | "unknown"
export type SessionErrorReason =

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 SessionErrorReason values not propagated to SDK/plugin.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.errors.ts:3-10):

Problem: New SessionErrorReason values not propagated to SDK/plugin
Detail: `SessionErrorReason` is widened with `"interrupted" | "terminated"`, but the diff touches no consumer outside `packages/cli`. `run.errors.ts` exports the public NDJSON event contract (`SCHEMA_VERSION` + types); the SDK and plugin packages are typical co-change partners of `run.ts` for event schemas. If either the SDK or plugin re-exports, mirrors, or generates from `SessionErrorReason`, consumers using exhaustive `switch` on `reason` will (a) fail to type-check against the widened union, and (b) see no matching `SCHEMA_VERSION` bump (still `"1"`) signaling the new discriminators. `EVENTS.md` documents the values but nothing in the diff confirms SDK/plugin types or generated OpenAPI/JSON-schema follow. Verify by grepping both packages for `SessionErrorReason` / `session_error` / `rate_limit`.
Suggested fix: Audit `packages/sdk/**` and `packages/plugin/**` for re-exports, duplicates, or generated schemas derived from `SessionErrorReason` (and the `session_error` payload). Add `"interrupted"` and `"terminated"` wherever the union is mirrored. If schema is codegen-driven from `run.errors.ts`, regenerate and commit. If the repo's versioning policy treats enum widening as breaking, bump `SCHEMA_VERSION` and document migration; otherwise add a changelog note that v1 reason is additive.

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

SessionErrorReason is widened with "interrupted" | "terminated", but the diff touches no consumer outside packages/cli. run.errors.ts exports the public NDJSON event contract (SCHEMA_VERSION + types); the SDK and plugin packages are typical co-change partners of run.ts for event schemas. If either the SDK or plugin re-exports, mirrors, or generates from SessionErrorReason, consumers using exhaustive switch on reason will (a) fail to type-check against the widened union, and (b) see no matching SCHEMA_VERSION bump (still "1") signaling the new discriminators. EVENTS.md documents the values but nothing in the diff confirms SDK/plugin types or generated OpenAPI/JSON-schema follow. Verify by grepping both packages for SessionErrorReason / session_error / rate_limit.

export const SCHEMA_VERSION = "1"

export type SessionErrorReason =
  | "rate_limit"
  | "auth"
  | "timeout"
  | "oom"
  | "provider"
  | "interrupted"
  | "terminated"
  | "unknown"

| "rate_limit"
| "auth"
| "timeout"
| "oom"
| "provider"
| "interrupted"
| "terminated"
| "unknown"

export type ClassifiedSessionError = {
reason: SessionErrorReason
Expand Down
21 changes: 21 additions & 0 deletions packages/cli/src/cli/cmd/run.terminal.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
type Emit = (type: string, data: Record<string, unknown>) => unknown

export function terminal(emit: Emit) {
const state = {
complete: false,
error: false,
}

return {
complete(data: Record<string, unknown>) {
if (state.complete) return
state.complete = true
emit("session_complete", data)
},
error(data: Record<string, unknown>) {
if (state.complete || state.error) return
state.error = true
emit("session_error", data)
},
}
}
150 changes: 78 additions & 72 deletions packages/cli/src/cli/cmd/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { pathToFileURL } from "bun"
import { UI } from "../ui"
import { cmd } from "./cmd"
import { classifySessionError, SCHEMA_VERSION } from "./run.errors"
import { terminal } from "./run.terminal"
import { buildToolCatalogItems } from "./tool-catalog"
import { withTimeout } from "../../util/timeout"
import { Flag } from "../../flag/flag"
Expand Down Expand Up @@ -37,7 +38,10 @@ import { SkillTool } from "../../tool/skill"
import { BashTool } from "../../tool/bash"
import { TodoWriteTool } from "../../tool/todo"
import { Locale } from "../../util/locale"
import { Log } from "../../util/log"
import { Shutdown } from "../shutdown"
import { Stdout } from "../stdout"
import { attempt, signals, type Signals } from "../signals"
import { createRunInvocation } from "./run.invocation"

type ToolProps<T extends Tool.Info> = {
Expand Down Expand Up @@ -534,11 +538,6 @@ export const RunCommand = cmd({

const events = await sdk.event.subscribe()
let error: string | undefined
// Guard: at most one session_error is emitted per run regardless of which
// failure path fires first (in-loop session.error handler, loopDone.catch,
// or promptResult rejection). Without this a mid-run session.error event
// can fire site 1 while promptResult/loop() also rejects and fires site 2/3.
let sessionErrorEmitted = false
const startTime = Date.now()
const childSessions = new Set<string>()
const emitted = new Set<string>()
Expand All @@ -549,6 +548,20 @@ export const RunCommand = cmd({
return n
}

// Keep every failure path on one ordered, idempotent terminal lifecycle.
const output = terminal(emit)

function complete(message = error ?? null) {
output.complete({
durationMs: Date.now() - startTime,
error: message,
})
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 report() can emit session_error after session_complete.

--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -546,7 +546,7 @@
       function report(reason: string, code: string | undefined, message: string) {
-        if (sessionErrorEmitted) return
+        if (sessionCompleteEmitted || sessionErrorEmitted) return
         sessionErrorEmitted = true
         emit("session_error", { reason, code, message })
       }
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:546-550):

Problem: report() can emit session_error after session_complete
Detail: `report()` only guards on `sessionErrorEmitted`, never on `sessionCompleteEmitted`. In the normal (non-signal) path, `loopDone = loop().then(() => complete(), fail)` runs `complete()` (emitting `session_complete`) BEFORE the trailing `await Shutdown.flush()`. The SIGINT/SIGTERM listeners installed by `using control = signals(...)` stay armed until scope exit (dispose runs on function return), so a signal arriving during that final flush invokes `interrupt()` -> `report()`, which emits `session_error` AFTER `session_complete`. This (1) violates the documented v1 invariant that `session_error` precedes `session_complete` (asserted by run-schema-v1.test.ts and promised by EVENTS.md) and (2) emits an error event for a session that already completed successfully. `complete()` was given the symmetric idempotency guard; `report()` should have the same. The same window exists on the `if (control.current)` early-return path's `await Shutdown.flush()` and the post-`Promise.race` flush.
Suggested fix: Make `report()` terminal-aware so it cannot fire after a normal completion: bail when the session already completed. Symmetrically, `interrupt()` could early-return when `sessionCompleteEmitted` is already true.

Suggested patch:
--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -546,7 +546,7 @@
       function report(reason: string, code: string | undefined, message: string) {
-        if (sessionErrorEmitted) return
+        if (sessionCompleteEmitted || sessionErrorEmitted) return
         sessionErrorEmitted = true
         emit("session_error", { reason, code, 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

report() only guards on sessionErrorEmitted, never on sessionCompleteEmitted. In the normal (non-signal) path, loopDone = loop().then(() => complete(), fail) runs complete() (emitting session_complete) BEFORE the trailing await Shutdown.flush(). The SIGINT/SIGTERM listeners installed by using control = signals(...) stay armed until scope exit (dispose runs on function return), so a signal arriving during that final flush invokes interrupt() -> report(), which emits session_error AFTER session_complete. This (1) violates the documented v1 invariant that session_error precedes session_complete (asserted by run-schema-v1.test.ts and promised by EVENTS.md) and (2) emits an error event for a session that already completed successfully. complete() was given the symmetric idempotency guard; report() should have the same. The same window exists on the if (control.current) early-return path's await Shutdown.flush() and the post-Promise.race flush.

      function complete(message = error ?? null) {
        if (sessionCompleteEmitted) return
        sessionCompleteEmitted = true
        emit("session_complete", {
          durationMs: Date.now() - startTime,
          error: message,
        })
      }

      function report(reason: string, code: string | undefined, message: string) {
        if (sessionErrorEmitted) return
        sessionErrorEmitted = true
        emit("session_error", { reason, code, message })
      }

function report(reason: string, code: string | undefined, message: string) {
output.error({ reason, code, message })
}

async function loop() {
const toggles = new Map<string, boolean>()

Expand Down Expand Up @@ -695,22 +708,13 @@ export const RunCommand = cmd({
// to a non-zero exit code so CI wrappers see the failure instead of a
// spuriously-green job. process.exitCode (not process.exit) lets the
// loop drain to session.status idle and emit session_complete first.
process.exitCode = 1
if (!control.current) process.exitCode = 1
invocation.error(props.error)
if (!sessionErrorEmitted) {
sessionErrorEmitted = true
const classified = classifySessionError(props.error)
// Structured session_error (classified reason/code/message) is emitted for the
// primary session only. The generic "error" event below fires for both primary
// and child-session failures and carries the raw error object — these are
// intentionally distinct channels: session_error is for structured telemetry/CI
// consumers, "error" is the legacy observable for raw error pass-through.
emit("session_error", {
reason: classified.reason,
code: classified.code,
message: classified.message,
})
}
const classified = classifySessionError(props.error)
// Structured session_error is the telemetry/CI channel for the
// primary session. The legacy "error" event below is the raw
// pass-through for both primary and child-session failures.
report(classified.reason, classified.code, classified.message)
}
if (emit("error", { error: props.error, sourceSessionID: props.sessionID })) continue
UI.error(err)
Expand Down Expand Up @@ -845,6 +849,48 @@ export const RunCommand = cmd({
permissions: PermissionNext.merge(agentInfo.permission, rules),
})

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 cancel() misses sync throws from sdk.session.abort.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:786-789):

Problem: cancel() misses sync throws from sdk.session.abort
Detail: `Promise.resolve(sdk.session.abort({ sessionID }))` evaluates the argument expression FIRST, then wraps the result. If `sdk.session.abort` throws synchronously — e.g. `sdk.session` has no `abort` method in non-attach mode (the abort shim is only added inside the `if (args.attach)` block at line 958), or `SessionPrompt.cancel` throws sync — the throw escapes `cancel()` entirely, propagates into the signals callback and then into `handle()` in signals.ts. Because `handle()` sets `state.current = signal` BEFORE calling `cancel(signal)`, `state.current` is set but `state.resolve?.(signal)` on the next line is never reached, so the `received` promise never resolves. Net effect: `run.ts`'s `if (control.current) { await loopDone; ... }` branch is taken, but `loopDone` may never resolve because the real abort never fired; the 5s grace timer then force-exits with no `session_complete`. The `.catch()` is dead code for the sync-throw case.
Suggested fix: function cancel() {
  Promise.resolve()
    .then(() => sdk.session.abort({ sessionID }))
    .catch((e) => {
      console.error(e)
    })
}

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

Promise.resolve(sdk.session.abort({ sessionID })) evaluates the argument expression FIRST, then wraps the result. If sdk.session.abort throws synchronously — e.g. sdk.session has no abort method in non-attach mode (the abort shim is only added inside the if (args.attach) block at line 958), or SessionPrompt.cancel throws sync — the throw escapes cancel() entirely, propagates into the signals callback and then into handle() in signals.ts. Because handle() sets state.current = signal BEFORE calling cancel(signal), state.current is set but state.resolve?.(signal) on the next line is never reached, so the received promise never resolves. Net effect: run.ts's if (control.current) { await loopDone; ... } branch is taken, but loopDone may never resolve because the real abort never fired; the 5s grace timer then force-exits with no session_complete. The .catch() is dead code for the sync-throw case.

      function cancel() {
        Promise.resolve(sdk.session.abort({ sessionID })).catch((e) => {
          console.error(e)
        })
      }

      using control = signals((signal) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Raw SDK abort error dumped to stderr via console.error.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:786-789):

Problem: Raw SDK abort error dumped to stderr via console.error
Detail: The signal-cancel path catches the rejection from `sdk.session.abort(...)` and writes the entire error object to stderr with `console.error(e)`. SDK/network errors can carry provider base URLs, request headers, redacted-but-not-safe stack traces, or environment-derived diagnostics that should not surface on the user's terminal or be captured by parent process logs. This also runs inside a signal handler, so the output is unsanitized and unstructured.
Suggested fix: Log a fixed, safe message and forward details only to a debug sink, e.g. `.catch((e) => { debug?.(e); console.error("abort failed") })`, or run the error through the existing `classifySessionError`/redaction helper before printing.

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 signal-cancel path catches the rejection from sdk.session.abort(...) and writes the entire error object to stderr with console.error(e). SDK/network errors can carry provider base URLs, request headers, redacted-but-not-safe stack traces, or environment-derived diagnostics that should not surface on the user's terminal or be captured by parent process logs. This also runs inside a signal handler, so the output is unsanitized and unstructured.

      function cancel() {
        Promise.resolve(sdk.session.abort({ sessionID })).catch((e) => {
          console.error(e)
        })
      }


let aborted = false
function abort() {
if (aborted) return
aborted = true
attempt(
() => sdk.session.abort({ sessionID }),
() => Log.Default.error("session abort failed"),
)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 session_complete.error mixes provider and signal text.

Suggested change
Either skip the error append when `sessionErrorEmitted` is already true (the prior error already owns `session_complete.error`), or emit the signal's `session_error` with precedence over the suppressed provider one.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:793-801):

Problem: session_complete.error mixes provider and signal text
Detail: When a non-fatal provider error already populated `error` and emitted `session_error` (`sessionErrorEmitted=true`), a subsequent SIGINT/SIGTERM still executes `error = error ? error + EOL + signal.message : signal.message`, appending the signal message to the existing provider error string. The `session_error` event is correctly suppressed by the guard, but `session_complete.error` ends up as `'provider error\nSession interrupted by SIGINT'` while `session_error.reason` remains `'provider'`. The two fields now disagree about why the session ended. EVENTS.md says non-fatal errors "may also appear concatenated in `session_complete.error`", but mixing a real provider error with a signal termination message in the same field while suppressing the signal's own `session_error` is surprising for consumers keying alerts on `session_complete.error` content.
Suggested fix: Either skip the error append when `sessionErrorEmitted` is already true (the prior error already owns `session_complete.error`), or emit the signal's `session_error` with precedence over the suppressed provider one.

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

When a non-fatal provider error already populated error and emitted session_error (sessionErrorEmitted=true), a subsequent SIGINT/SIGTERM still executes error = error ? error + EOL + signal.message : signal.message, appending the signal message to the existing provider error string. The session_error event is correctly suppressed by the guard, but session_complete.error ends up as 'provider error\nSession interrupted by SIGINT' while session_error.reason remains 'provider'. The two fields now disagree about why the session ended. EVENTS.md says non-fatal errors "may also appear concatenated in session_complete.error", but mixing a real provider error with a signal termination message in the same field while suppressing the signal's own session_error is surprising for consumers keying alerts on session_complete.error content.

      using control = signals((signal) => {
        error = error ? error + EOL + signal.message : signal.message
        if (!sessionErrorEmitted) {
          sessionErrorEmitted = true
          emit("session_error", {
            reason: signal.reason,
            code: String(signal.code),
            message: signal.message,
          })
        }
        cancel()
      })

function interrupt(signal: Signals.Info) {
error ??= signal.message
invocation.error(signal.message)
report(signal.reason, String(signal.code), signal.message)
abort()
}

async function expire(signal: Signals.Info) {
complete(error ?? signal.message)
await invocation.abort(signal.message)
await Shutdown.flush()
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 abort() no-op if signal beats RPC handler wiring.

Suggested change
Register the `abort` RPC handler (and ensure `sdk.session.abort` is routable) before `signals(...)` arms its listeners, or defer arming the signal listeners until the attach session object is fully constructed.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:858):

Problem: abort() no-op if signal beats RPC handler wiring
Detail: `using control = signals(interrupt, 5_000, expire)` arms the SIGINT/SIGTERM listeners at line 858, but the `abort(opts)` RPC handler (and the `sdk` object it dispatches through) is registered later in the function. A signal delivered during that setup window calls `interrupt()` -> `abort()` -> `sdk.session.abort({ sessionID })` before the handler is routable, so the RPC is unmatched/rejects and `attempt()` swallows it via `Log.Default.error("session abort failed")`. The in-flight session is then never gracefully cancelled and only gets force-exited at the 5s grace deadline. Stream shape (session_error/session_complete) and exit code are still correct due to idempotency + the grace timer, but the documented "abort the in-flight session" contract is silently skipped in this window, so any session-side cleanup hooks do not run. Depends on the exact registration order of `sdk`/the attach session object relative to `signals(...)`.
Suggested fix: Register the `abort` RPC handler (and ensure `sdk.session.abort` is routable) before `signals(...)` arms its listeners, or defer arming the signal listeners until the attach session object is fully constructed.

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

using control = signals(interrupt, 5_000, expire) arms the SIGINT/SIGTERM listeners at line 858, but the abort(opts) RPC handler (and the sdk object it dispatches through) is registered later in the function. A signal delivered during that setup window calls interrupt() -> abort() -> sdk.session.abort({ sessionID }) before the handler is routable, so the RPC is unmatched/rejects and attempt() swallows it via Log.Default.error("session abort failed"). The in-flight session is then never gracefully cancelled and only gets force-exited at the 5s grace deadline. Stream shape (session_error/session_complete) and exit code are still correct due to idempotency + the grace timer, but the documented "abort the in-flight session" contract is silently skipped in this window, so any session-side cleanup hooks do not run. Depends on the exact registration order of sdk/the attach session object relative to signals(...).

      function abort() {
        if (aborted) return
        aborted = true
        attempt(
          () => sdk.session.abort({ sessionID }),
          () => Log.Default.error("session abort failed"),
        )
      }

using control = signals(interrupt, 5_000, expire)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 fail() drops real cause when a signal arrived first.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:860-868):

Problem: fail() drops real cause when a signal arrived first
Detail: If a signal lands first, `interrupt()` sets `error = signal.message` and emits `session_error`. When the loop later rejects and `fail(cause)` runs: `report(...)` is a no-op (`sessionErrorEmitted` already true), `complete(error)` records `signal.message`, and `if (control.current) return` (line 865) skips `console.error(cause)`. The actual rejection cause therefore appears in none of `session_error`, `session_complete`, or stderr. A genuine non-signal failure that coincides with (or surfaces just after) an operator interrupt is fully masked as a clean "interrupted" run, which hurts headless CI debuggability where stderr/NDJSON is the only diagnostic channel.
Suggested fix: When `control.current` is set, still surface the cause before returning, e.g. `Log.Default.error("run failed after signal", { error: cause })`, so it is not dropped entirely.

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

If a signal lands first, interrupt() sets error = signal.message and emits session_error. When the loop later rejects and fail(cause) runs: report(...) is a no-op (sessionErrorEmitted already true), complete(error) records signal.message, and if (control.current) return (line 865) skips console.error(cause). The actual rejection cause therefore appears in none of session_error, session_complete, or stderr. A genuine non-signal failure that coincides with (or surfaces just after) an operator interrupt is fully masked as a clean "interrupted" run, which hurts headless CI debuggability where stderr/NDJSON is the only diagnostic channel.

      function fail(cause: unknown) {
        const classified = classifySessionError(cause)
        error ??= classified.message
        report(classified.reason, classified.code, classified.message)
        complete(error)
        if (control.current) return
        console.error(cause)
        process.exitCode = 1
      }

function reject(cause: unknown) {
const classified = classifySessionError(cause)
error ??= classified.message
invocation.error(cause)
report(classified.reason, classified.code, classified.message)
complete(error)
if (control.current) {
Log.Default.error("run failed after signal", {
reason: classified.reason,
code: classified.code,
})
return
}
console.error(cause)
process.exitCode = 1
}

// Emit the resolved tool catalog (builtin + MCP tools) and available
// skills before the first model turn. Enables structural detection of
// "tool was not exposed" failure modes (issue #85).
Expand Down Expand Up @@ -873,31 +919,13 @@ export const RunCommand = cmd({
})
}

const loopDone = loop()
.then(() => {
emit("session_complete", {
durationMs: Date.now() - startTime,
error: error ?? null,
})
})
.catch((e) => {
const classified = classifySessionError(e)
if (!sessionErrorEmitted) {
sessionErrorEmitted = true
emit("session_error", {
reason: classified.reason,
code: classified.code,
message: classified.message,
})
}
emit("session_complete", {
durationMs: Date.now() - startTime,
error: classified.message,
})
console.error(e)
process.exitCode = 1
invocation.error(e)
})
const loopDone = loop().then(() => complete(), reject)

if (control.current) {
await loopDone
await Shutdown.flush()
return
}

if (args.command) {
await sdk.session.command({
Expand All @@ -924,33 +952,8 @@ export const RunCommand = cmd({
// (e.g., Session.get() fails, model not found), no session.status idle event
// is emitted, so loopDone would hang forever. Racing ensures we surface
// the error and exit.
await Promise.race([
loopDone,
promptResult.then(
// If prompt resolves normally, wait for the event loop to finish
() => loopDone,
// If prompt rejects, surface the error immediately
(e) => {
const classified = classifySessionError(e)
error = error ? error + EOL + classified.message : classified.message
if (!sessionErrorEmitted) {
sessionErrorEmitted = true
emit("session_error", {
reason: classified.reason,
code: classified.code,
message: classified.message,
})
}
emit("session_complete", {
durationMs: Date.now() - startTime,
error: classified.message,
})
console.error(e)
process.exitCode = 1
invocation.error(e)
},
),
])
await Promise.race([loopDone, promptResult.then(() => loopDone, reject)])
await Shutdown.flush()
}

invocation.phase("bootstrap")
Expand Down Expand Up @@ -983,6 +986,9 @@ export const RunCommand = cmd({
const share = await Session.share(opts.sessionID)
return { data: { share } }
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 abort RPC handler forwards unvalidated opts.sessionID.

🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:965-967):

Problem: abort RPC handler forwards unvalidated opts.sessionID
Detail: The new `abort(opts: any)` handler forwards `opts.sessionID` straight to `SessionPrompt.cancel(opts.sessionID)` with no type guard or ownership check, and `opts` is typed `any`. `abort` is destructive (cancels the active prompt), so a reachable caller supplying an arbitrary session identifier could cancel a session it did not initiate. This mirrors the pre-existing unvalidated-`opts` pattern of the sibling `prompt`/`share` handlers and is almost certainly local-only dispatch today (the CLI invoking its own handlers), so the practical risk is low — but cancellation is a stronger primitive than read/share, so as defense-in-depth the handler should validate its input. If the attach endpoint is ever exposed to less-trusted callers, this becomes a cross-session DoS surface.
Suggested fix: Type the input explicitly (e.g. `abort(opts: { sessionID: string })`) and validate it before dispatching: minimal guard `if (typeof opts?.sessionID !== \"string\" || !opts.sessionID) throw new Error(\"sessionID required\")`. Preferably apply the same typing to the sibling `prompt`/`share` handlers.

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 abort(opts: any) handler forwards opts.sessionID straight to SessionPrompt.cancel(opts.sessionID) with no type guard or ownership check, and opts is typed any. abort is destructive (cancels the active prompt), so a reachable caller supplying an arbitrary session identifier could cancel a session it did not initiate. This mirrors the pre-existing unvalidated-opts pattern of the sibling prompt/share handlers and is almost certainly local-only dispatch today (the CLI invoking its own handlers), so the practical risk is low — but cancellation is a stronger primitive than read/share, so as defense-in-depth the handler should validate its input. If the attach endpoint is ever exposed to less-trusted callers, this becomes a cross-session DoS surface.

            share(opts: any) {
              const share = await Session.share(opts.sessionID)
              return { data: { share } }
            },
          abort(opts: any) {
            return SessionPrompt.cancel(opts.sessionID)
          },
          async prompt(opts: any) {

abort(opts: any) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 abort shim drops SessionPrompt.cancel returned promise.

--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -955,6 +955,7 @@
             return { data: { share } }
           },
           abort(opts: any) {
-            SessionPrompt.cancel(opts.sessionID)
+            return SessionPrompt.cancel(opts.sessionID)
           },
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:958-960):

Problem: abort shim drops SessionPrompt.cancel returned promise
Detail: The added shim `abort(opts: any) { SessionPrompt.cancel(opts.sessionID) }` has no return statement. If `SessionPrompt.cancel` returns a Promise (async cancel implementation), that promise is discarded by the shim. The caller `cancel()` does `Promise.resolve(sdk.session.abort({...})).catch(...)` — since the shim returns undefined, `Promise.resolve(undefined)` resolves immediately and the `.catch` never fires. Any async rejection from `SessionPrompt.cancel` (abort endpoint 5xx, network error) becomes an unhandled promise rejection and is silently lost from the signal-cancellation observability path. In attach mode this shim IS `sdk.session.abort`, so `cancel()`'s entire error-handling path is dead.
Suggested fix: abort(opts: any) {
  return SessionPrompt.cancel(opts.sessionID)
},

Suggested patch:
--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -955,6 +955,7 @@
             return { data: { share } }
           },
           abort(opts: any) {
-            SessionPrompt.cancel(opts.sessionID)
+            return SessionPrompt.cancel(opts.sessionID)
           },


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 added shim abort(opts: any) { SessionPrompt.cancel(opts.sessionID) } has no return statement. If SessionPrompt.cancel returns a Promise (async cancel implementation), that promise is discarded by the shim. The caller cancel() does Promise.resolve(sdk.session.abort({...})).catch(...) — since the shim returns undefined, Promise.resolve(undefined) resolves immediately and the .catch never fires. Any async rejection from SessionPrompt.cancel (abort endpoint 5xx, network error) becomes an unhandled promise rejection and is silently lost from the signal-cancellation observability path. In attach mode this shim IS sdk.session.abort, so cancel()'s entire error-handling path is dead.

            const share = await Session.share(opts.sessionID)
            return { data: { share } }
          },
          abort(opts: any) {
            SessionPrompt.cancel(opts.sessionID)
          },
          async prompt(opts: any) {

return SessionPrompt.cancel(opts.sessionID)
},
async prompt(opts: any) {
promptResult = SessionPrompt.prompt({
sessionID: opts.sessionID,
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/cli/shutdown.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export namespace Shutdown {
Log.Default.error("stdout flush failed", {
error: error instanceof Error ? error.message : error,
})
process.exitCode = 1
process.exitCode ||= 1
})
}
}
83 changes: 83 additions & 0 deletions packages/cli/src/cli/signals.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
export namespace Signals {
export type Info = {
name: "SIGINT" | "SIGTERM"
reason: "interrupted" | "terminated"
code: 130 | 143
message: string
}
}

const info = {
SIGINT: {
name: "SIGINT",
reason: "interrupted",
code: 130,
message: "Session interrupted by SIGINT",
},
SIGTERM: {
name: "SIGTERM",
reason: "terminated",
code: 143,
message: "Session terminated by SIGTERM",
},
} as const satisfies Record<Signals.Info["name"], Signals.Info>

export function attempt(run: () => unknown, fail: () => void) {
Promise.resolve().then(run).catch(fail)
}

export function signals(stop: (info: Signals.Info) => void, grace = 5_000, expire?: (info: Signals.Info) => unknown) {
const state: {
current?: Signals.Info
timer?: ReturnType<typeof setTimeout>
hard?: ReturnType<typeof setTimeout>
resolve?: (info: Signals.Info) => void
} = {}
const received = new Promise<Signals.Info>((resolve) => {
state.resolve = resolve
})

function handle(name: Signals.Info["name"]) {
const signal = info[name]
if (state.current) {
process.exit(state.current.code)
}
state.current = signal
process.exitCode = signal.code
state.timer = setTimeout(() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 expire continuation can observe advanced state.current.

Suggested change
state.timer = setTimeout(() => {
Capture the triggering signal once at the top of `handle` (already done as local `signal`) and have run.ts read the captured value rather than re-reading `control.current` after awaits; add a brief comment in `signals.ts` documenting that `state.current` is write-once.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/signals.ts:47-58):

Problem: expire continuation can observe advanced state.current
Detail: In `signals.ts`, `handle` schedules a grace timer whose async continuation (`expire?.(signal)` then `process.exit`) reads shared `state` (e.g. `state.hard`) after microtask/await boundaries. The re-entrancy guard handles a *second signal* by force-exiting synchronously, and `state.current` is write-once in practice (the only writer is `handle`, and a second call exits before mutating). However, the "first signal wins" invariant is only enforced by control flow, not by freezing the triggering signal as the source of truth for the post-await reads in run.ts (`error ?? signal.message`, `control.current`). Any future change that adds a second write path to `state.current` (or yields inside `stop` before the timer fires) would silently let the expire continuation act on a signal different from the one that started the sequence. Document/assert the write-once assumption, or capture the triggering signal once and key all continuations off it.
Suggested fix: Capture the triggering signal once at the top of `handle` (already done as local `signal`) and have run.ts read the captured value rather than re-reading `control.current` after awaits; add a brief comment in `signals.ts` documenting that `state.current` is write-once.

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

In signals.ts, handle schedules a grace timer whose async continuation (expire?.(signal) then process.exit) reads shared state (e.g. state.hard) after microtask/await boundaries. The re-entrancy guard handles a second signal by force-exiting synchronously, and state.current is write-once in practice (the only writer is handle, and a second call exits before mutating). However, the "first signal wins" invariant is only enforced by control flow, not by freezing the triggering signal as the source of truth for the post-await reads in run.ts (error ?? signal.message, control.current). Any future change that adds a second write path to state.current (or yields inside stop before the timer fires) would silently let the expire continuation act on a signal different from the one that started the sequence. Document/assert the write-once assumption, or capture the triggering signal once and key all continuations off it.

state.hard = setTimeout(() => process.exit(signal.code), 250)
Promise.resolve()
.then(() => expire?.(signal))
.then(
() => {
if (state.hard) clearTimeout(state.hard)
process.exit(signal.code)
},
() => process.exit(signal.code),
)
}, grace)
stop(signal)
state.resolve?.(signal)
}

const sigint = () => handle("SIGINT")
const sigterm = () => handle("SIGTERM")
process.on("SIGINT", sigint)
process.on("SIGTERM", sigterm)

const dispose = () => {
if (state.timer) clearTimeout(state.timer)
if (state.hard) clearTimeout(state.hard)
process.off("SIGINT", sigint)
process.off("SIGTERM", sigterm)
}

return {
received,
get current() {
return state.current
},
dispose,
[Symbol.dispose]: dispose,
}
}
1 change: 1 addition & 0 deletions packages/cli/test/cli/fixture/stdout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ if (process.argv.includes("--shutdown-error")) {
Stdout.write("")
await Stdout.flush()
process.stdout.emit("error", Object.assign(new Error("broken stdout"), { code: "EIO" }))
if (process.argv.includes("--signal-exit")) process.exitCode = 143
await Shutdown.flush()
const status = process.argv.find((arg) => arg.startsWith("--status="))?.slice("--status=".length)
if (status) await Bun.write(status, String(process.exitCode))
Expand Down
Loading