Handle headless signals as graceful cancellation#98
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| export const SCHEMA_VERSION = "1" | ||
|
|
||
| export type SessionErrorReason = "rate_limit" | "auth" | "timeout" | "oom" | "provider" | "unknown" | ||
| export type SessionErrorReason = |
There was a problem hiding this comment.
🟠 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"| const share = await Session.share(opts.sessionID) | ||
| return { data: { share } } | ||
| }, | ||
| abort(opts: any) { |
There was a problem hiding this comment.
🟠 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) {| @@ -784,6 +785,32 @@ export const RunCommand = cmd({ | |||
| permissions: PermissionNext.merge(agentInfo.permission, rules), | |||
| }) | |||
There was a problem hiding this comment.
🟠 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) => {| } | ||
| state.current = signal | ||
| process.exitCode = signal.code | ||
| state.timer = setTimeout(() => process.exit(signal.code), grace) |
There was a problem hiding this comment.
🟠 Grace timer hard-exits without emitting session_complete.
| state.timer = setTimeout(() => process.exit(signal.code), grace) | |
| Have the grace-timer callback emit a terminal `session_complete` (or invoke a registered onGraceExpired hook) before `process.exit`, OR have run.ts race `await loopDone` against a pre-timeout that emits `session_complete` and flushes before delegating to the hard exit. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/signals.ts:42):
Problem: Grace timer hard-exits without emitting session_complete
Detail: The grace timer `setTimeout(() => process.exit(signal.code), grace)` is a hard fallback. In run.ts the signal handler emits `session_error` but NOT `session_complete` — `session_complete` is only produced when `loopDone` resolves naturally after a successful abort. If `sdk.session.abort` is slow, unacked, or throws (see related finding on `cancel()`), `loopDone` never resolves within the 5s grace window; the timer fires `process.exit` and the NDJSON stream ends after `session_error` with no `session_complete`. This violates the documented contract in EVENTS.md ("Emitted once when the session ends (success or failure)"). The new test `an expired grace period causes hard termination` explicitly asserts only `["session_error"]` is emitted, confirming `session_complete` is skipped by design on this path. Downstream alerting that waits on `session_complete` will hang or treat the run as still-active.
Suggested fix: Have the grace-timer callback emit a terminal `session_complete` (or invoke a registered onGraceExpired hook) before `process.exit`, OR have run.ts race `await loopDone` against a pre-timeout that emits `session_complete` and flushes before delegating to the hard exit.
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 grace timer setTimeout(() => process.exit(signal.code), grace) is a hard fallback. In run.ts the signal handler emits session_error but NOT session_complete — session_complete is only produced when loopDone resolves naturally after a successful abort. If sdk.session.abort is slow, unacked, or throws (see related finding on cancel()), loopDone never resolves within the 5s grace window; the timer fires process.exit and the NDJSON stream ends after session_error with no session_complete. This violates the documented contract in EVENTS.md ("Emitted once when the session ends (success or failure)"). The new test an expired grace period causes hard termination explicitly asserts only ["session_error"] is emitted, confirming session_complete is skipped by design on this path. Downstream alerting that waits on session_complete will hang or treat the run as still-active.
state.current = signal
process.exitCode = signal.code
state.timer = setTimeout(() => process.exit(signal.code), grace)
cancel(signal)
state.resolve?.(signal)| cancel() | ||
| }) | ||
|
|
||
| function flush() { |
There was a problem hiding this comment.
🟡 flush() empty-string write may not drain stdout on Bun.
| function flush() { | |
| Drain via a real write of a known non-empty sentinel byte, or capture the boolean return of `process.stdout.write` and, when false, await the `'drain'` event (`process.stdout.once("drain", resolve)`) — the canonical Node drain pattern that Bun also honors. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:807-810):
Problem: flush() empty-string write may not drain stdout on Bun
Detail: `flush()` uses `process.stdout.write("", () => resolve())` to wait for prior NDJSON writes to drain. Empty-string write semantics differ across runtimes: on Node TTY stdout, `write` is effectively synchronous and the callback fires on nextTick regardless of pending data; on Node pipe stdout, an empty chunk is queued behind prior writes so drain order is preserved. This repo targets Bun (tests invoke `bun run`), where `process.stdout.write` is a re-implementation and there is no documented guarantee that an empty-string write blocks the callback until prior buffered data is handed to the OS. The `await flush()` before `return` (and before `using` dispose) may therefore resolve before `session_error`/`session_complete` bytes leave the process, risking truncated NDJSON output when the caller exits immediately after.
Suggested fix: Drain via a real write of a known non-empty sentinel byte, or capture the boolean return of `process.stdout.write` and, when false, await the `'drain'` event (`process.stdout.once("drain", resolve)`) — the canonical Node drain pattern that Bun also honors.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
flush() uses process.stdout.write("", () => resolve()) to wait for prior NDJSON writes to drain. Empty-string write semantics differ across runtimes: on Node TTY stdout, write is effectively synchronous and the callback fires on nextTick regardless of pending data; on Node pipe stdout, an empty chunk is queued behind prior writes so drain order is preserved. This repo targets Bun (tests invoke bun run), where process.stdout.write is a re-implementation and there is no documented guarantee that an empty-string write blocks the callback until prior buffered data is handed to the OS. The await flush() before return (and before using dispose) may therefore resolve before session_error/session_complete bytes leave the process, risking truncated NDJSON output when the caller exits immediately after.
function flush() {
if (!control.current) return Promise.resolve()
return new Promise<void>((resolve) => {
process.stdout.write("", () => resolve())
})
}| @@ -784,6 +785,32 @@ export const RunCommand = cmd({ | |||
| permissions: PermissionNext.merge(agentInfo.permission, rules), | |||
| }) | |||
There was a problem hiding this comment.
🟡 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)
})
}| console.error(e) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 session_complete.error mixes provider and signal text.
| 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()
})
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 4 · 🟡 3 · ⚪ 0 · 0/7 resolved
🤖 Fix all 7 open findings with your agent📋 Out-of-diff findings (7)
Reviewed 6 files · 0 inline · view all 7 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Review finding judgments and remediationJudged against original review head
Verification
One automated re-review round is already running for |
| } | ||
|
|
||
| function emit(type: string, data: Record<string, unknown>) { | ||
| if (args.format === "json") { |
There was a problem hiding this comment.
🟠 flush() awaits a write callback that hides stdout errors.
| if (args.format === "json") { | |
| Chain promises instead of reassigning (`output = output.then(() => writeChunk(...))`) so any rejection propagates to `await flush()` and into the expire onRejected branch; and register `process.stdout.on('error', (e) => Log.Default.error('stdout write failed'))` once at module load so EPIPE does not crash or silently lose the terminal events. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:471-475):
Problem: flush() awaits a write callback that hides stdout errors
Detail: The new `emit` wraps `process.stdout.write` in a promise resolved by the write callback, and the signal expire path does `await flush()` (returning `output`) before `process.exit(signal.code)`. The `writable.write` callback only signals that the chunk was handed to the kernel -- it does NOT propagate write errors (EPIPE / EIO), which surface via stdout's 'error' event, for which no listener is registered in this scope. When the consumer closes the pipe early (`aictrl run --format json | head`, parent crash, closed PTY), `await flush()` resolves "successfully" even though the `session_error`/`session_complete` lines were never delivered. The process then exits 130/143 reporting a graceful cancellation that downstream orchestrators never observed, defeating the purpose of the structured event. Additionally, reassigning `output = new Promise(...)` per emit (instead of chaining) means a rejection from an earlier write is dropped rather than propagated to `await flush()`. If `JSON.stringify` or `write` ever throws synchronously inside the executor (destroyed stream), `output` becomes a rejected promise that nothing `.catch`es synchronously, raising an unhandledRejection during signal handling.
Suggested fix: Chain promises instead of reassigning (`output = output.then(() => writeChunk(...))`) so any rejection propagates to `await flush()` and into the expire onRejected branch; and register `process.stdout.on('error', (e) => Log.Default.error('stdout write failed'))` once at module load so EPIPE does not crash or silently lose the terminal events.
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 emit wraps process.stdout.write in a promise resolved by the write callback, and the signal expire path does await flush() (returning output) before process.exit(signal.code). The writable.write callback only signals that the chunk was handed to the kernel -- it does NOT propagate write errors (EPIPE / EIO), which surface via stdout's 'error' event, for which no listener is registered in this scope. When the consumer closes the pipe early (aictrl run --format json | head, parent crash, closed PTY), await flush() resolves "successfully" even though the session_error/session_complete lines were never delivered. The process then exits 130/143 reporting a graceful cancellation that downstream orchestrators never observed, defeating the purpose of the structured event. Additionally, reassigning output = new Promise(...) per emit (instead of chaining) means a rejection from an earlier write is dropped rather than propagated to await flush(). If JSON.stringify or write ever throws synchronously inside the executor (destroyed stream), output becomes a rejected promise that nothing .catches synchronously, raising an unhandledRejection during signal handling.
function emit(type: string, data: Record<string, unknown>) {
if (args.format === "json") {
output = new Promise<void>((resolve) => {
process.stdout.write(JSON.stringify({ type, timestamp: Date.now(), sessionID, ...data }) + EOL, () =>
resolve(),
)
})
return true| }) | ||
| complete(error ?? classified.message) | ||
| console.error(e) | ||
| process.exitCode = 1 |
There was a problem hiding this comment.
🟠 Signal exit code overwritten to 1 if abort rejects.
| process.exitCode = 1 | |
| Gate the fallback on the absence of a signal in BOTH catch handlers: `if (!control.current) process.exitCode = 1` (and wrap `console.error(e)` in the same guard) so an intentional cancellation does not overwrite the 130/143 exit code or print a misleading stack. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:881-882):
Problem: Signal exit code overwritten to 1 if abort rejects
Detail: Both rejection handlers unconditionally run `process.exitCode = 1` (loopDone.catch at ~L881, and the symmetric promptResult.catch at ~L935). When a SIGINT/SIGTERM fires, the signal handler sets `process.exitCode = 130/143` and calls `abort()` -> `sdk.session.abort` -> `SessionPrompt.cancel(sessionID)`. If that cancellation propagates as a rejection of `loop()` or `promptResult` (a normal way for cancellation to surface), the `.catch` runs AFTER the handler already set 130/143 and clobbers it back to 1. The early-return path `if (control.current) { await loopDone; await flush(); return }` then returns, the `using control` dispose clears the grace timer so `expire` never fires, and the process exits with code 1 -- not the conventional 130/143 that is this PR's stated contract. The new integration test does NOT catch this: the mock `/abort` returns `Response.json(true)` + an idle `session.status`, so `loop()` resolves cleanly; any real SDK abort that surfaces as a rejection exposes the bug. There is no `control.current` guard in either catch. Secondary noise: `console.error(e)` in the same handlers would also dump the raw cancellation error to stderr after an intentional Ctrl+C.
Suggested fix: Gate the fallback on the absence of a signal in BOTH catch handlers: `if (!control.current) process.exitCode = 1` (and wrap `console.error(e)` in the same guard) so an intentional cancellation does not overwrite the 130/143 exit code or print a misleading stack.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Both rejection handlers unconditionally run process.exitCode = 1 (loopDone.catch at ~L881, and the symmetric promptResult.catch at ~L935). When a SIGINT/SIGTERM fires, the signal handler sets process.exitCode = 130/143 and calls abort() -> sdk.session.abort -> SessionPrompt.cancel(sessionID). If that cancellation propagates as a rejection of loop() or promptResult (a normal way for cancellation to surface), the .catch runs AFTER the handler already set 130/143 and clobbers it back to 1. The early-return path if (control.current) { await loopDone; await flush(); return } then returns, the using control dispose clears the grace timer so expire never fires, and the process exits with code 1 -- not the conventional 130/143 that is this PR's stated contract. The new integration test does NOT catch this: the mock /abort returns Response.json(true) + an idle session.status, so loop() resolves cleanly; any real SDK abort that surfaces as a rejection exposes the bug. There is no control.current guard in either catch. Secondary noise: console.error(e) in the same handlers would also dump the raw cancellation error to stderr after an intentional Ctrl+C.
complete(error ?? classified.message)
console.error(e)
process.exitCode = 1
})
if (control.current) {
await loopDone
await flush()
return
}| @@ -104,8 +104,8 @@ Emitted immediately before `session_complete` when the session terminates abnorm | |||
| } | |||
There was a problem hiding this comment.
🟡 New enum values added to required field without version bump.
| } | |
| 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.
| parts: [...files, { type: "text", text: message }], | ||
| }) | ||
| } | ||
| if (control.current) abort() |
There was a problem hiding this comment.
🟡 abort() may fire twice (signal handler + post-prompt).
| if (control.current) abort() | |
| Guard with a closed-over flag: `let aborted = false; function abort() { if (aborted) return; aborted = true; cancel(() => sdk.session.abort({ sessionID }), () => Log.Default.error("session abort failed")); }`. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:909):
Problem: abort() may fire twice (signal handler + post-prompt)
Detail: `abort()` (which calls `cancel(() => sdk.session.abort({ sessionID }), ...)`) is invoked from the signal `stop` callback the moment a signal arrives, AND again at `if (control.current) abort()` immediately after `sdk.session.prompt`/message is dispatched (L909). When a signal lands while a prompt is in flight, `sdk.session.abort` / `SessionPrompt.cancel` is therefore invoked twice. Correctness depends entirely on `SessionPrompt.cancel` being idempotent; if it rejects on the second invocation, the rejection is swallowed by the `fail` handler that only logs 'session abort failed', leaving an ambiguous abort state that can race with the loop's catch path. The `sessionErrorEmitted` guard protects the event, but the abort side-effect is unguarded.
Suggested fix: Guard with a closed-over flag: `let aborted = false; function abort() { if (aborted) return; aborted = true; cancel(() => sdk.session.abort({ sessionID }), () => Log.Default.error("session abort failed")); }`.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
abort() (which calls cancel(() => sdk.session.abort({ sessionID }), ...)) is invoked from the signal stop callback the moment a signal arrives, AND again at if (control.current) abort() immediately after sdk.session.prompt/message is dispatched (L909). When a signal lands while a prompt is in flight, sdk.session.abort / SessionPrompt.cancel is therefore invoked twice. Correctness depends entirely on SessionPrompt.cancel being idempotent; if it rejects on the second invocation, the rejection is swallowed by the fail handler that only logs 'session abort failed', leaving an ambiguous abort state that can race with the loop's catch path. The sessionErrorEmitted guard protects the event, but the abort side-effect is unguarded.
| } | ||
| state.current = signal | ||
| process.exitCode = signal.code | ||
| state.timer = setTimeout(() => { |
There was a problem hiding this comment.
🟡 expire continuation can observe advanced state.current.
| 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.
| }, | ||
| } as const satisfies Record<Signals.Info["name"], Signals.Info> | ||
|
|
||
| export function cancel(run: () => unknown, fail: () => void) { |
There was a problem hiding this comment.
⚪ `cancel` export name collides with SessionPrompt.cancel.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/signals.ts:25-27):
Problem: `cancel` export name collides with SessionPrompt.cancel
Detail: `export function cancel(run, fail)` in signals.ts is a generic async-runner-with-error-swallowing wrapper, not a cancellation primitive -- the actual cancellation is performed by the `run` callback (which calls `sdk.session.abort` -> `SessionPrompt.cancel`). The sibling `SessionPrompt.cancel` already exists with a different meaning. In run.ts both are in scope (`import { cancel } from "../signals"` alongside `sdk.session.abort` / `SessionPrompt.cancel`), so a reader seeing a bare `cancel(...)` call must check the import to disambiguate. A self-descriptive name would avoid the semantic clash.
Suggested fix: Rename to a self-descriptive name such as `runSwallowingErrors`, `safeRun`, or `attempt`, and update the call site in run.ts.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
export function cancel(run, fail) in signals.ts is a generic async-runner-with-error-swallowing wrapper, not a cancellation primitive -- the actual cancellation is performed by the run callback (which calls sdk.session.abort -> SessionPrompt.cancel). The sibling SessionPrompt.cancel already exists with a different meaning. In run.ts both are in scope (import { cancel } from "../signals" alongside sdk.session.abort / SessionPrompt.cancel), so a reader seeing a bare cancel(...) call must check the import to disambiguate. A self-descriptive name would avoid the semantic clash.
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 2 · 🟡 3 · ⚪ 1 · 0/6 resolved
🤖 Fix all 6 open findings with your agent📋 Out-of-diff findings (6)
Reviewed 7 files · 0 inline · view all 6 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Final bounded review responseJudged against round-two head
Verification
No merge performed and no further review loop requested. |
0ec7d0f to
e97241e
Compare
| error: message, | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 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 })
}| @@ -961,6 +963,9 @@ export const RunCommand = cmd({ | |||
| const share = await Session.share(opts.sessionID) | |||
| return { data: { share } } | |||
| }, | |||
There was a problem hiding this comment.
🟡 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) {| } | ||
|
|
||
| using control = signals(interrupt, 5_000, expire) | ||
|
|
There was a problem hiding this comment.
🟡 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
}| complete(error ?? signal.message) | ||
| await Shutdown.flush() | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 abort() no-op if signal beats RPC handler wiring.
| 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"),
)
}| } | ||
| if (!control.current) process.exitCode = 1 | ||
| const classified = classifySessionError(props.error) | ||
| // Structured session_error is emitted for the primary session only. |
There was a problem hiding this comment.
⚪ session_error vs error channel rationale lost.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/src/cli/cmd/run.ts:701-703):
Problem: session_error vs error channel rationale lost
Detail: The loop's session.error handler previously carried a multi-line comment explaining WHY `session_error` and the legacy `error` event are intentionally distinct channels ("session_error is for structured telemetry/CI consumers, 'error' is the legacy observable for raw error pass-through"). The replacement comment keeps the "primary-only vs both" fact but drops the audience/rationale, which is exactly the context future editors need to avoid collapsing the two channels. The rest of the file preserves rationale-style comments (e.g. the `process.exitCode` block immediately above), so this is inconsistent with the surrounding comment density.
Suggested fix: Restore the audience note, e.g. `// Structured session_error is emitted for the primary session only — it is the telemetry/CI channel. The legacy "error" event below is the raw pass-through for both primary and child-session failures.`
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 loop's session.error handler previously carried a multi-line comment explaining WHY session_error and the legacy error event are intentionally distinct channels ("session_error is for structured telemetry/CI consumers, 'error' is the legacy observable for raw error pass-through"). The replacement comment keeps the "primary-only vs both" fact but drops the audience/rationale, which is exactly the context future editors need to avoid collapsing the two channels. The rest of the file preserves rationale-style comments (e.g. the process.exitCode block immediately above), so this is inconsistent with the surrounding comment density.
if (!control.current) process.exitCode = 1
const classified = classifySessionError(props.error)
// Structured session_error is emitted for the primary session only.
// The legacy "error" event below remains the raw pass-through for
// both primary and child-session failures.
report(classified.reason, classified.code, classified.message)| const errIdx = source.indexOf('emit("session_error"') | ||
| expect(errIdx).toBeGreaterThan(-1) | ||
| expect(errIdx).toBeLessThan(source.lastIndexOf('emit("session_complete"')) | ||
| expect(source.slice(errIdx).indexOf("complete(")).toBeGreaterThan(-1) |
There was a problem hiding this comment.
⚪ v1 schema ordering assertion was weakened by the refactor.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #98, packages/cli/test/cli/run-schema-v1.test.ts:42-43):
Problem: v1 schema ordering assertion was weakened by the refactor
Detail: The old assertion `expect(errIdx).toBeLessThan(source.lastIndexOf('emit("session_complete"'))` enforced that the first `emit("session_error")` precedes the last `emit("session_complete")` in source — a structural proxy for the runtime ordering guarantee. The replacement `expect(source.slice(errIdx).indexOf("complete(")).toBeGreaterThan(-1)` only checks that some `complete(` call textually follows the session_error emit site, which is trivially true because `fail()` calls `complete(error)` after `report(...)`. The new assertion no longer protects against regressions where `session_complete` is emitted before `session_error` at the source level; that invariant is now covered only by the heavier integration tests in run-signal-cancellation.test.ts. Consider keeping a stricter source-ordering check alongside the new idempotency check.
Suggested fix: Also assert relative ordering of the emit sites, e.g. locate `emit("session_error"` inside `report()` and `emit("session_complete"` inside `complete()` and assert error-index < complete-index, rather than just that `complete(` appears later in the file.
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 old assertion expect(errIdx).toBeLessThan(source.lastIndexOf('emit("session_complete"')) enforced that the first emit("session_error") precedes the last emit("session_complete") in source — a structural proxy for the runtime ordering guarantee. The replacement expect(source.slice(errIdx).indexOf("complete(")).toBeGreaterThan(-1) only checks that some complete( call textually follows the session_error emit site, which is trivially true because fail() calls complete(error) after report(...). The new assertion no longer protects against regressions where session_complete is emitted before session_error at the source level; that invariant is now covered only by the heavier integration tests in run-signal-cancellation.test.ts. Consider keeping a stricter source-ordering check alongside the new idempotency check.
const source = await Bun.file(RUN_SRC).text()
const errIdx = source.indexOf('emit("session_error"')
expect(errIdx).toBeGreaterThan(-1)
expect(source.slice(errIdx).indexOf("complete(")).toBeGreaterThan(-1)
expect(source).toContain("if (sessionCompleteEmitted) return")
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 3 · ⚪ 2 · 0/6 resolved
🤖 Fix all 6 open findings with your agent📋 Out-of-diff findings (6)
Reviewed 11 files · 0 inline · view all 6 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
1 participant
Closes #94
Intent
Translate SIGINT and SIGTERM during headless runs into bounded, observable cancellation instead of abrupt, ambiguous termination, while keeping the command lifecycle straightforward to read and maintain.
Expected outcomes
Expected impact
Shells, supervisors, and automation receive consistent exit codes and terminal events for interrupted work, reducing sessions that appear stuck or successful after cancellation. Signal handling remains scoped to an active run, and future changes have one clear location for reporting, completion, abort, interruption, expiry, and failure behavior.
Summary
interrupted/terminatedsession errors before the normalsession_completepathVerification
bun testfrompackages/cli— 1,347 passed, 7 skipped, 0 failedbun run typecheckfrompackages/clibun run typecheck— 6 tasks successfulgit diff --checkRisk
Signal handlers are installed only after a real session starts and are scoped to that run. SIGKILL and supervisors that provide no grace period remain outside the guarantee. The branch is rebased onto the shared stdout/shutdown implementation already on
main; an unexpected flush error records a failure without replacing an existing signal exit code.