feat(cli): add NDJSON invocation lifecycle events#96
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| handler: async (args) => { | ||
| Invocation.phase("validation") | ||
|
|
||
| function fail(message: string, code: string) { |
There was a problem hiding this comment.
🟠 fail() process.exit races stdout NDJSON write.
| function fail(message: string, code: string) { | |
| Route `fail()` through the same flush-then-exit helper used by the top-level `finally` (await stdout `'finish'`/drain before `process.exit`), instead of calling `process.exit(1)` synchronously after `Invocation.abort()`. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/cmd/run.ts:338-342):
Problem: fail() process.exit races stdout NDJSON write
Detail: The new `fail()` helper calls `Invocation.abort()` (which writes `invocation_error` + `invocation_complete` NDJSON via `process.stdout.write`) and then immediately calls `process.exit(1)`. On a non-TTY stdout (pipe/file/CI capture), writes are async-buffered and `process.exit()` does not await the drain, so the terminal NDJSON events can be truncated or lost. This reintroduces the race at every validation failure path (`INVOCATION_INVALID_DIRECTORY`, `INVOCATION_FILE_NOT_FOUND`, `INVOCATION_EMPTY_INPUT`, `INVOCATION_INVALID_ARGUMENTS`, `INVOCATION_SESSION_CREATE_FAILED`). If a flush-then-exit helper exists for the top-level finally path, `fail()` bypasses it. Confidence is moderate because the lifecycle test does capture all three events in piped-stdout scenarios; depending on runtime (Bun may flush synchronously for small writes) the race may not manifest in practice, but the code as written does not guarantee delivery.
Suggested fix: Route `fail()` through the same flush-then-exit helper used by the top-level `finally` (await stdout `'finish'`/drain before `process.exit`), instead of calling `process.exit(1)` synchronously after `Invocation.abort()`.
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 fail() helper calls Invocation.abort() (which writes invocation_error + invocation_complete NDJSON via process.stdout.write) and then immediately calls process.exit(1). On a non-TTY stdout (pipe/file/CI capture), writes are async-buffered and process.exit() does not await the drain, so the terminal NDJSON events can be truncated or lost. This reintroduces the race at every validation failure path (INVOCATION_INVALID_DIRECTORY, INVOCATION_FILE_NOT_FOUND, INVOCATION_EMPTY_INPUT, INVOCATION_INVALID_ARGUMENTS, INVOCATION_SESSION_CREATE_FAILED). If a flush-then-exit helper exists for the top-level finally path, fail() bypasses it. Confidence is moderate because the lifecycle test does capture all three events in piped-stdout scenarios; depending on runtime (Bun may flush synchronously for small writes) the race may not manifest in practice, but the code as written does not guarantee delivery.
function fail(message: string, code: string) {
Invocation.abort(message, code)
UI.error(message)
process.exit(1)
}| sessionID = id | ||
| }, | ||
| error(error: unknown, code = "INVOCATION_FAILED") { | ||
| if (!invocationID || sessionID || failed || completed) return |
There was a problem hiding this comment.
🟠 Post-session failures report status:completed.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/invocation.ts:47-68):
Problem: Post-session failures report status:completed
Detail: `error()`'s guard `if (!invocationID || sessionID || failed || completed) return` causes it to silently no-op once `Invocation.link(sessionID)` runs in run.ts. Any later failure (share()/execute()/bootstrap-callback throws, POST /message 500, network drops, agent errors, or an unhandledRejection/uncaughtException arriving mid-session) propagates to the `catch` in headless.ts/index.ts, where `Invocation.error(e)` (and `Invocation.abort(e)` from the global handlers) early-returns because `sessionID` is truthy — so `failed` is never set. The `finally` block then calls `complete()` which emits `{ type: "invocation_complete", status: "completed", sessionID }` even though `process.exitCode = 1` was set in the catch. NDJSON consumers see a successful invocation while the shell sees a non-zero exit code. EVENTS.md hand-waves this with "Errors after a real session has been created use session_error instead", but nothing in this PR guarantees a `session_error` is emitted on every post-link failure path, and the `invocation_complete.status` field directly contradicts the actual outcome. The new lifecycle test only covers pre-link failures (`session()` returning null at L538); no test exercises a post-link failure, so the regression is unguarded.
Suggested fix: Decouple the `failed` flag from `invocation_error` emission. Either remove `|| sessionID` from the `error()` guard (so invocation_error is still emitted alongside any session_error), or add a `markFailed()` that sets `failed = true` without emitting when `sessionID` is set, and call it from the headless.ts/index.ts catch and from the unhandledRejection/uncaughtException handlers. At minimum, `complete()` must derive status from something a post-link error can affect — e.g. check `process.exitCode !== 0` or accept an explicit status override.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
error()'s guard if (!invocationID || sessionID || failed || completed) return causes it to silently no-op once Invocation.link(sessionID) runs in run.ts. Any later failure (share()/execute()/bootstrap-callback throws, POST /message 500, network drops, agent errors, or an unhandledRejection/uncaughtException arriving mid-session) propagates to the catch in headless.ts/index.ts, where Invocation.error(e) (and Invocation.abort(e) from the global handlers) early-returns because sessionID is truthy — so failed is never set. The finally block then calls complete() which emits { type: "invocation_complete", status: "completed", sessionID } even though process.exitCode = 1 was set in the catch. NDJSON consumers see a successful invocation while the shell sees a non-zero exit code. EVENTS.md hand-waves this with "Errors after a real session has been created use session_error instead", but nothing in this PR guarantees a session_error is emitted on every post-link failure path, and the invocation_complete.status field directly contradicts the actual outcome. The new lifecycle test only covers pre-link failures (session() returning null at L538); no test exercises a post-link failure, so the regression is unguarded.
error(error: unknown, code = "INVOCATION_FAILED") {
if (!invocationID || sessionID || failed || completed) return
failed = true
emit("invocation_error", {
phase,
code,
message:
error instanceof Error
? error.message
: error && typeof error === "object" && "message" in error
? String(error.message)
: String(error),
})
},
complete() {
if (!invocationID || completed) return
completed = true
emit("invocation_complete", {
status: failed ? "error" : "completed",
durationMs: Date.now() - started,
...(sessionID ? { sessionID } : {}),
})
},| emit("invocation_error", { | ||
| phase, | ||
| code, | ||
| message: |
There was a problem hiding this comment.
🟠 Raw error messages leaked to stdout NDJSON.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/invocation.ts:52-58):
Problem: Raw error messages leaked to stdout NDJSON
Detail: `Invocation.error()`/`abort()` serialize `error.message` (or `String(error)`) verbatim into the stdout NDJSON stream for every unhandledRejection, uncaughtException, parse error, and run-handler failure. Error messages from network/SDK/auth/filesystem layers routinely carry absolute paths, internal URLs, auth tokens, API keys, or env-derived values. Stdout is the machine-readable channel that consumers pipe to log aggregators and CI capture, so this widens the disclosure surface beyond the pre-existing `Log.Default.error` path (which is stderr/file-scoped). The `String(error)` else branch additionally dumps arbitrary object graphs for non-Error rejections. This is reachable from every error path that the new lifecycle instrumentation wraps.
Suggested fix: Sanitize before emitting: cap length, redact URLs/tokens/paths, or emit only the stable `code` + `phase` and keep the raw message on the existing `Log` channel. At minimum drop the `String(error)` fallback and never serialize arbitrary objects.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Invocation.error()/abort() serialize error.message (or String(error)) verbatim into the stdout NDJSON stream for every unhandledRejection, uncaughtException, parse error, and run-handler failure. Error messages from network/SDK/auth/filesystem layers routinely carry absolute paths, internal URLs, auth tokens, API keys, or env-derived values. Stdout is the machine-readable channel that consumers pipe to log aggregators and CI capture, so this widens the disclosure surface beyond the pre-existing Log.Default.error path (which is stderr/file-scoped). The String(error) else branch additionally dumps arbitrary object graphs for non-Error rejections. This is reachable from every error path that the new lifecycle instrumentation wraps.
emit("invocation_error", {
phase,
code,
message:
error instanceof Error
? error.message
: error && typeof error === "object" && "message" in error
? String(error.message)
: String(error),
})| export type InvocationPhase = "parse" | "validation" | "stdin" | "bootstrap" | "session" | ||
|
|
||
| function create(argv: string[]) { | ||
| const run = argv.indexOf("run") |
There was a problem hiding this comment.
🟡 argv indexOf('run') matches non-subcommand tokens.
| const run = argv.indexOf("run") | |
| Resolve the subcommand from the actual command tree (or require `run` to be the first non-flag argv token) instead of scanning the entire argv for an exact `run` element. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/invocation.ts:7-13):
Problem: argv indexOf('run') matches non-subcommand tokens
Detail: JSON-mode detection uses `argv.indexOf("run")` (first exact `run` token) then scans the remainder for `--format=json`. If a different subcommand accepts a positional/value equal to `run` followed elsewhere by `--format json` (e.g. `aictrl <cmd> --name run --format json`), the detector falsely activates and emits `invocation_start` / `invocation_complete` NDJSON onto an unrelated command's stdout, corrupting its parsed output. The check does not confirm `run` is in the subcommand position (first non-flag token), and any future alias/wrapper for the run subcommand would not be recognized.
Suggested fix: Resolve the subcommand from the actual command tree (or require `run` to be the first non-flag argv token) instead of scanning the entire argv for an exact `run` element.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
JSON-mode detection uses argv.indexOf("run") (first exact run token) then scans the remainder for --format=json. If a different subcommand accepts a positional/value equal to run followed elsewhere by --format json (e.g. aictrl <cmd> --name run --format json), the detector falsely activates and emits invocation_start / invocation_complete NDJSON onto an unrelated command's stdout, corrupting its parsed output. The check does not confirm run is in the subcommand position (first non-flag token), and any future alias/wrapper for the run subcommand would not be recognized.
const run = argv.indexOf("run")
const json =
run !== -1 &&
argv.slice(run + 1).some((arg, index, args) => {
if (arg === "--format=json") return true
return arg === "--format" && args[index + 1] === "json"
})| import { Database } from "./storage/db" | ||
| import { Invocation, startInvocation } from "./cli/invocation" | ||
|
|
||
| startInvocation() |
There was a problem hiding this comment.
⚪ Lifecycle wiring duplicated in index.ts and headless.ts.
| startInvocation() | |
| In `packages/cli/src/cli/invocation.ts`, export helpers such as `installInvocationProcessHandlers()`, `wireInvocationCliFail(cli)`, and `finalizeInvocation(e?)`, and call them once from each entry point. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/index.ts:34):
Problem: Lifecycle wiring duplicated in index.ts and headless.ts
Detail: The Invocation lifecycle wiring added to `index.ts` and `headless.ts` is byte-for-byte identical across six insertion points: (1) `startInvocation()` top-level call, (2) `Invocation.abort(e)` in `unhandledRejection`, (3) `Invocation.abort(e)` in `uncaughtException`, (4) `if (!Invocation.id) cli.showHelp("log")` guard, (5) `Invocation.abort(msg ?? "Invalid command line arguments", "INVOCATION_PARSE_ERROR")` before `process.exit(1)` in the yargs `.fail()` handler, (6) `Invocation.error(e)` in `catch` + `Invocation.complete()` in `finally`. The two copies can drift (e.g. the `INVOCATION_PARSE_ERROR` code or the showHelp guard condition). Rated NIT rather than MINOR because the surrounding process/yargs/try-catch scaffolding is already duplicated between these two entry points, so the new code merely follows the existing convention.
Suggested fix: In `packages/cli/src/cli/invocation.ts`, export helpers such as `installInvocationProcessHandlers()`, `wireInvocationCliFail(cli)`, and `finalizeInvocation(e?)`, and call them once from each entry point.
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 Invocation lifecycle wiring added to index.ts and headless.ts is byte-for-byte identical across six insertion points: (1) startInvocation() top-level call, (2) Invocation.abort(e) in unhandledRejection, (3) Invocation.abort(e) in uncaughtException, (4) if (!Invocation.id) cli.showHelp("log") guard, (5) Invocation.abort(msg ?? "Invalid command line arguments", "INVOCATION_PARSE_ERROR") before process.exit(1) in the yargs .fail() handler, (6) Invocation.error(e) in catch + Invocation.complete() in finally. The two copies can drift (e.g. the INVOCATION_PARSE_ERROR code or the showHelp guard condition). Rated NIT rather than MINOR because the surrounding process/yargs/try-catch scaffolding is already duplicated between these two entry points, so the new code merely follows the existing convention.
import { Invocation, startInvocation } from "./cli/invocation"
startInvocation()
process.on("unhandledRejection", (e) => {
Invocation.abort(e)
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 3 · 🟡 1 · ⚪ 1 · 0/5 resolved
🤖 Fix all 5 open findings with your agent📋 Out-of-diff findings (5)
Reviewed 7 files · 0 inline · view all 5 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Remediation commit: Verification:
The push triggered the provider re-review and CI workflows for the new head. |
| @@ -0,0 +1,118 @@ | |||
| import { EOL } from "os" | |||
There was a problem hiding this comment.
🟠 Invocation lifecycle wiring duplicated in both entrypoints.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/invocation.ts:1-118):
Problem: Invocation lifecycle wiring duplicated in both entrypoints
Detail: The PR adds near-byte-identical invocation lifecycle plumbing to BOTH packages/cli/src/headless.ts and packages/cli/src/index.ts: the `startInvocation()` call, two async `process.on("unhandledRejection"/"uncaughtException")` handlers that each do `Invocation.abort(e); Log...; await Invocation.flush(); process.exit(1)`, the same `.fail()` body (`if (!Invocation.id) cli.showHelp("log"); ... Invocation.abort(msg ?? "...", "INVOCATION_PARSE_ERROR"); if (Invocation.id) throw new Error(...); process.exit(1)`), `Invocation.error(e)` in the catch block, and `Invocation.complete(); await Invocation.flush()` in finally. This PR introduces a brand-new module (`invocation.ts`) that is the natural home for this wiring, yet the lifecycle setup is inlined into both entrypoints. The repo's own consistency rule is "Functions used across files are moved to a shared module, not duplicated"; the existing exception-handler.test.ts (also touched here, thresholds relaxed 200→250 in lockstep for both files) already has to assert the same shape twice, proving the two must be hand-synced on every future lifecycle change. Suggested fix: export helpers from invocation.ts such as `installInvocationProcessHandlers()` and `finalizeInvocation()`, and a `wrapCliFail(cli)` shim, then have both entrypoints call them.
Suggested fix: Move the shared wiring into invocation.ts: export `installInvocationProcessHandlers()` (registers the two async process handlers), `finalizeInvocation()` (complete + flush), and `wrapCliFail(cli)` for the yargs `.fail()` shim. Have headless.ts and index.ts call those instead of inlining ~14 lines each.
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 near-byte-identical invocation lifecycle plumbing to BOTH packages/cli/src/headless.ts and packages/cli/src/index.ts: the startInvocation() call, two async process.on("unhandledRejection"/"uncaughtException") handlers that each do Invocation.abort(e); Log...; await Invocation.flush(); process.exit(1), the same .fail() body (if (!Invocation.id) cli.showHelp("log"); ... Invocation.abort(msg ?? "...", "INVOCATION_PARSE_ERROR"); if (Invocation.id) throw new Error(...); process.exit(1)), Invocation.error(e) in the catch block, and Invocation.complete(); await Invocation.flush() in finally. This PR introduces a brand-new module (invocation.ts) that is the natural home for this wiring, yet the lifecycle setup is inlined into both entrypoints. The repo's own consistency rule is "Functions used across files are moved to a shared module, not duplicated"; the existing exception-handler.test.ts (also touched here, thresholds relaxed 200→250 in lockstep for both files) already has to assert the same shape twice, proving the two must be hand-synced on every future lifecycle change. Suggested fix: export helpers from invocation.ts such as installInvocationProcessHandlers() and finalizeInvocation(), and a wrapCliFail(cli) shim, then have both entrypoints call them.
// headless.ts (lines 22-37, head):
startInvocation()
process.on("unhandledRejection", async (e) => {
Invocation.abort(e)
Log.Default.error("rejection", { ... })
await Invocation.flush()
process.exit(1)
})
process.on("uncaughtException", async (e) => {
Invocation.abort(e)
Log.Default.error("exception", { ... })
await Invocation.flush()
process.exit(1)
})
// index.ts: identical block, must be hand-synced on every change.| @@ -465,7 +475,9 @@ export const RunCommand = cmd({ | |||
|
|
|||
| function emit(type: string, data: Record<string, unknown>) { | |||
| if (args.format === "json") { | |||
There was a problem hiding this comment.
🟡 Two emit() helpers diverge on schemaVersion handling.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/cmd/run.ts:477-483):
Problem: Two emit() helpers diverge on schemaVersion handling
Detail: There are now two sibling `emit(type, data)` functions building NDJSON envelopes. `invocation.ts` constructs `{type, timestamp, schemaVersion: SCHEMA_VERSION, invocationID, ...data}` and queue-serializes writes; `run.ts` constructs `{type, timestamp, invocationID: Invocation.id, sessionID, ...data}` synchronously and does NOT inject `schemaVersion` — it relies on every caller remembering to pass `schemaVersion` inside `data`. EVENTS.md (modified by this PR) states the schema is versioned via BOTH `invocation_start.schemaVersion` and `session_start.schemaVersion`, so the two emit paths are responsible for the same contract but enforce it differently: an `invocation_*` event can never forget `schemaVersion`, while a future `session_*` event added in run.ts that forgets to include `schemaVersion` in its data object will silently violate the documented schema. The duplication also means any future envelope field (cliVersion, pid, etc.) must be added in two places.
Suggested fix: Extract a single `writeEvent(type, data, ctx)` (or `buildEnvelope(type, data, ctx)`) helper — ideally on the Invocation module since it already owns the queued-write plumbing and `SCHEMA_VERSION` — and have run.ts route `session_*` events through it. This guarantees `schemaVersion` is always injected and that session events flush in order with invocation 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
There are now two sibling emit(type, data) functions building NDJSON envelopes. invocation.ts constructs {type, timestamp, schemaVersion: SCHEMA_VERSION, invocationID, ...data} and queue-serializes writes; run.ts constructs {type, timestamp, invocationID: Invocation.id, sessionID, ...data} synchronously and does NOT inject schemaVersion — it relies on every caller remembering to pass schemaVersion inside data. EVENTS.md (modified by this PR) states the schema is versioned via BOTH invocation_start.schemaVersion and session_start.schemaVersion, so the two emit paths are responsible for the same contract but enforce it differently: an invocation_* event can never forget schemaVersion, while a future session_* event added in run.ts that forgets to include schemaVersion in its data object will silently violate the documented schema. The duplication also means any future envelope field (cliVersion, pid, etc.) must be added in two places.
function emit(type: string, data: Record<string, unknown>) {
if (args.format === "json") {
process.stdout.write(
JSON.stringify({ type, timestamp: Date.now(), invocationID: Invocation.id, sessionID, ...data }) + EOL,
)
return true
}
return false| export type InvocationPhase = "parse" | "validation" | "stdin" | "bootstrap" | "session" | ||
|
|
||
| function create(argv: string[]) { | ||
| const run = (() => { |
There was a problem hiding this comment.
🟡 Argv sniffer drops lifecycle events for unknown globals.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/invocation.ts:7-25):
Problem: Argv sniffer drops lifecycle events for unknown globals
Detail: `create(argv)` re-implements argv parsing to decide whether to emit lifecycle events. The detection loop returns `arg === "run" ? index : -1` on the FIRST non-skipped token, and the skip-list only contains `--print-logs`, `--log-level`, and `--log-level=...`. Any other global option the CLI parser accepts (a short alias of `--log-level`, a future global, or any option-that-takes-a-value like a hypothetical `--config`/`--verbose`) appearing before the literal `run` token makes the loop return -1, so `invocationID` stays undefined and NO `invocation_start`/`invocation_error`/`invocation_complete` events are emitted even though the user passed `--format json`. The error is silent — consumers waiting on `invocation_complete` to detect CLI exit just hang. The skip-list is also not derived from the yargs global-option source-of-truth, so it will drift the moment a new global is added; the existing `recognizes run after global options` test only exercises `--log-level`, so a regression would not be caught. Could not verify against packages/cli/src/global.ts from the diff alone; if additional globals exist today this is a live bug rather than only a maintenance hazard.
Suggested fix: Either (a) defer detection to yargs — emit `invocation_start` after `cli.parse()` resolves the run command + format=json (and before stdin is read) — so the real parser is the single source of truth; or (b) if a pre-parse sniff must stay, make the leading-skip loop generic: skip any token starting with `--` (consuming its value when the option is registered as taking one) instead of hard-coding two flags. At minimum, derive the skip set from the same yargs global-option config and add a regression test asserting every yargs global is in the skip-list.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
create(argv) re-implements argv parsing to decide whether to emit lifecycle events. The detection loop returns arg === "run" ? index : -1 on the FIRST non-skipped token, and the skip-list only contains --print-logs, --log-level, and --log-level=.... Any other global option the CLI parser accepts (a short alias of --log-level, a future global, or any option-that-takes-a-value like a hypothetical --config/--verbose) appearing before the literal run token makes the loop return -1, so invocationID stays undefined and NO invocation_start/invocation_error/invocation_complete events are emitted even though the user passed --format json. The error is silent — consumers waiting on invocation_complete to detect CLI exit just hang. The skip-list is also not derived from the yargs global-option source-of-truth, so it will drift the moment a new global is added; the existing recognizes run after global options test only exercises --log-level, so a regression would not be caught. Could not verify against packages/cli/src/global.ts from the diff alone; if additional globals exist today this is a live bug rather than only a maintenance hazard.
const run = (() => {
for (let index = 0; index < argv.length; index++) {
const arg = argv[index]
if (arg === "--print-logs") continue
if (arg === "--log-level") {
index++
continue
}
if (arg.startsWith("--log-level=")) continue
return arg === "run" ? index : -1
}
return -1
})()| } | ||
| } | ||
|
|
||
| let current = create([]) |
There was a problem hiding this comment.
⚪ Dummy Invocation at module load is immediately replaced.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/invocation.ts:89):
Problem: Dummy Invocation at module load is immediately replaced
Detail: At module load, `let current = create([])` runs the full argv scan, allocates the `writes` promise chain, and closes over `emit`/`phase`/`link`/`error`/`complete` — all of which are immediately discarded the moment `startInvocation()` is called from headless.ts / index.ts (which happens at the top of every entrypoint before any other `Invocation.*` API is touched). Because `argv=[]` contains no `run` token, `invocationID` is undefined and every method on this dummy is a silent no-op, so the only effect is wasted allocation plus a false impression that the module is usable before `startInvocation()`. The pattern also hides bugs: a caller that fires `Invocation.error(...)` before `startInvocation()` (e.g. from a top-level await that throws during import) will silently swallow the lifecycle signal rather than fail loudly.
Suggested fix: Declare `let current: ReturnType<typeof create> | undefined` and either (a) have each `Invocation.*` method throw if `current` is unset, or (b) lazily initialize on first access. If the no-op default is intentional for test-harness safety, add a one-line comment saying so.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
At module load, let current = create([]) runs the full argv scan, allocates the writes promise chain, and closes over emit/phase/link/error/complete — all of which are immediately discarded the moment startInvocation() is called from headless.ts / index.ts (which happens at the top of every entrypoint before any other Invocation.* API is touched). Because argv=[] contains no run token, invocationID is undefined and every method on this dummy is a silent no-op, so the only effect is wasted allocation plus a false impression that the module is usable before startInvocation(). The pattern also hides bugs: a caller that fires Invocation.error(...) before startInvocation() (e.g. from a top-level await that throws during import) will silently swallow the lifecycle signal rather than fail loudly.
}
let current = create([])
export const Invocation = {
get id() {
return current.id
},
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 2 · ⚪ 1 · 0/4 resolved
🤖 Fix all 4 open findings with your agent📋 Out-of-diff findings (4)
Reviewed 7 files · 0 inline · view all 4 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Final remediation commit: Verification:
This is the second and final bounded remediation round; no further automated re-review is being requested. |
| })() | ||
| const json = | ||
| run !== -1 && | ||
| argv.slice(run + 1).some((arg, index, args) => { |
There was a problem hiding this comment.
🟠 --format=json detector ignores the `--` separator.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/invocation.ts:23-26):
Problem: --format=json detector ignores the `--` separator
Detail: The JSON-mode detector does `argv.slice(run + 1).some(arg => arg === "--format=json" || (arg === "--format" && next === "json"))` without stopping at the `--` separator that yargs uses to treat all subsequent tokens as positional message text. For an invocation like `aictrl run -- --format=json "do something"`, yargs sets `args.format` to the default (because `--format=json` is a positional after `--`), but the detector still matches the literal and generates an `invocationID`. The result: `invocation_start` and `invocation_complete` are emitted to stdout (because the invocation layer thinks it's in JSON mode), while the run-loop `emit()` in `run.ts` stays silent (because `args.format !== "json"`). Consumers of stdout see a bare invocation envelope with no session events in between, violating the deterministic-envelope goal of the PR. The author thought about positional false-positives for non-`run` subcommands (test: `does not start a run invocation for a positional token on another command`) but not for the `--` separator on `run` itself. The detector also only recognizes two flag spellings, so it can silently diverge from yargs in the other direction for any future alias.
Suggested fix: Stop the `--format` scan at the first `--` token inside the `.some()` callback:
```ts
argv.slice(run + 1).some((arg, index, args) => {
if (arg === "--") return false
if (arg === "--format=json") return true
return arg === "--format" && args[index + 1] === "json"
})
```
Better long-term: do not re-parse argv at all. Pass the parsed `args.format` value into the invocation context from the run handler so the lifecycle layer's notion of JSON mode is identical to what the run handler actually uses.
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 JSON-mode detector does argv.slice(run + 1).some(arg => arg === "--format=json" || (arg === "--format" && next === "json")) without stopping at the -- separator that yargs uses to treat all subsequent tokens as positional message text. For an invocation like aictrl run -- --format=json "do something", yargs sets args.format to the default (because --format=json is a positional after --), but the detector still matches the literal and generates an invocationID. The result: invocation_start and invocation_complete are emitted to stdout (because the invocation layer thinks it's in JSON mode), while the run-loop emit() in run.ts stays silent (because args.format !== "json"). Consumers of stdout see a bare invocation envelope with no session events in between, violating the deterministic-envelope goal of the PR. The author thought about positional false-positives for non-run subcommands (test: does not start a run invocation for a positional token on another command) but not for the -- separator on run itself. The detector also only recognizes two flag spellings, so it can silently diverge from yargs in the other direction for any future alias.
const json =
run !== -1 &&
argv.slice(run + 1).some((arg, index, args) => {
if (arg === "--format=json") return true
return arg === "--format" && args[index + 1] === "json"
})| cli.showHelp("log") | ||
| if (!Invocation.id) cli.showHelp("log") | ||
| } | ||
| if (err) throw err |
There was a problem hiding this comment.
🟠 INVOCATION_PARSE_ERROR code is unreachable in practice.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/headless.ts:130-132):
Problem: INVOCATION_PARSE_ERROR code is unreachable in practice
Detail: In the yargs `.fail()` handler, `if (err) throw err` on line 130 runs BEFORE `Invocation.abort(msg, "INVOCATION_PARSE_ERROR")` on line 131. Yargs passes a truthy `err` object alongside `msg` for every parse-error case the handler checks ("Unknown argument", "Invalid values:", etc.), so the throw fires first and control transfers to the `catch (e)` block around `await cli.parse()`. That catch calls `Invocation.error(e)` with the DEFAULT code `INVOCATION_FAILED`. As a result, the documented `INVOCATION_PARSE_ERROR` code is dead code in the common parse-error case — the emitted `invocation_error.code` is `INVOCATION_FAILED` instead. The test `reports argument parsing failure` passes only because `expectFailure` asserts on `error.phase` and `error.message` but never on `error.code`, so this regression is not caught. Any downstream consumer branching on `code === "INVOCATION_PARSE_ERROR"` will never match. The identical bug exists in `packages/cli/src/index.ts` (same handler around line 173).
Suggested fix: Call `Invocation.abort(...)` BEFORE the `if (err) throw err`, or set the code on the error before rethrowing. Example:
```ts
.fail((msg, err) => {
const isParseError =
msg?.startsWith("Unknown argument") || msg?.startsWith("Invalid values:")
if (isParseError) {
Invocation.abort(msg ?? "Invalid command line arguments", "INVOCATION_PARSE_ERROR")
if (err) throw err
if (!Invocation.id) cli.showHelp("log")
if (Invocation.id) throw new Error(msg ?? "Invalid command line arguments")
process.exit(1)
}
if (err) throw err
process.exit(1)
})
```
Apply the same fix to `packages/cli/src/index.ts`. Also add an assertion on `error.code` in the `reports argument parsing failure` test so this does not regress.
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 the yargs .fail() handler, if (err) throw err on line 130 runs BEFORE Invocation.abort(msg, "INVOCATION_PARSE_ERROR") on line 131. Yargs passes a truthy err object alongside msg for every parse-error case the handler checks ("Unknown argument", "Invalid values:", etc.), so the throw fires first and control transfers to the catch (e) block around await cli.parse(). That catch calls Invocation.error(e) with the DEFAULT code INVOCATION_FAILED. As a result, the documented INVOCATION_PARSE_ERROR code is dead code in the common parse-error case — the emitted invocation_error.code is INVOCATION_FAILED instead. The test reports argument parsing failure passes only because expectFailure asserts on error.phase and error.message but never on error.code, so this regression is not caught. Any downstream consumer branching on code === "INVOCATION_PARSE_ERROR" will never match. The identical bug exists in packages/cli/src/index.ts (same handler around line 173).
if (err) throw err
if (!Invocation.id) cli.showHelp("log")
}
if (err) throw err
Invocation.abort(msg ?? "Invalid command line arguments", "INVOCATION_PARSE_ERROR")
if (Invocation.id) throw new Error(msg ?? "Invalid command line arguments")
process.exit(1)
})| cli.showHelp("log") | ||
| if (!Invocation.id) cli.showHelp("log") | ||
| } | ||
| if (err) throw err |
There was a problem hiding this comment.
🟠 INVOCATION_PARSE_ERROR code is unreachable in practice.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/index.ts:173-175):
Problem: INVOCATION_PARSE_ERROR code is unreachable in practice
Detail: Same bug as in `packages/cli/src/headless.ts`: in the yargs `.fail()` handler, `if (err) throw err` on line 173 runs BEFORE `Invocation.abort(msg, "INVOCATION_PARSE_ERROR")` on line 174. Yargs passes a truthy `err` alongside `msg` for every parse-error case ("Unknown argument", "Invalid values:"), so the throw fires first and control transfers to the `catch (e)` block. That catch calls `Invocation.error(e)` with the DEFAULT code `INVOCATION_FAILED`. The documented `INVOCATION_PARSE_ERROR` code is therefore dead code for the common parse-error path, and consumers branching on `code === "INVOCATION_PARSE_ERROR"` will never match.
Suggested fix: Call `Invocation.abort(..., "INVOCATION_PARSE_ERROR")` BEFORE `if (err) throw err`, so the code is set on the invocation before the throw propagates to the catch block. Apply the same fix to `packages/cli/src/headless.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
Same bug as in packages/cli/src/headless.ts: in the yargs .fail() handler, if (err) throw err on line 173 runs BEFORE Invocation.abort(msg, "INVOCATION_PARSE_ERROR") on line 174. Yargs passes a truthy err alongside msg for every parse-error case ("Unknown argument", "Invalid values:"), so the throw fires first and control transfers to the catch (e) block. That catch calls Invocation.error(e) with the DEFAULT code INVOCATION_FAILED. The documented INVOCATION_PARSE_ERROR code is therefore dead code for the common parse-error path, and consumers branching on code === "INVOCATION_PARSE_ERROR" will never match.
if (err) throw err
if (!Invocation.id) cli.showHelp("log")
}
if (err) throw err
Invocation.abort(msg ?? "Invalid command line arguments", "INVOCATION_PARSE_ERROR")
if (Invocation.id) throw new Error(msg ?? "Invalid command line arguments")
process.exit(1)
})| sessionID = id | ||
| }, | ||
| error(_error: unknown, code = "INVOCATION_FAILED") { | ||
| if (!invocationID || failed || completed) return |
There was a problem hiding this comment.
🟡 abort() no-ops after complete(); late rejection misreports.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/invocation.ts:66-74):
Problem: abort() no-ops after complete(); late rejection misreports
Detail: Both `error()` and `complete()` short-circuit once `completed = true`. The `finally` block in `headless.ts` / `index.ts` runs `Invocation.complete()` → `await Invocation.flush()` → `process.exit()`. If an `unhandledRejection` (or `uncaughtException`) arrives during the `await Invocation.flush()` window — which is a real async gap where the event loop runs — the rejection handler calls `Invocation.abort(e)`, but `error()` returns early (`completed === true`) and `complete()` is also a no-op. The NDJSON stream therefore already carries `invocation_complete.status = "completed"` from the happy path, while the process actually exits with code 1 from the rejection handler. Consumers trusting the lifecycle envelope as the source of truth for invocation result get a false success in the exact error path they most need to trust. The window is narrow but reachable (any fire-and-forget promise that rejects during the flush drain will hit it).
Suggested fix: Track a `finalized` flag separately from the `completed` flag, or have `abort()` emit a corrective event when an error arrives after completion — e.g. re-emit (or revise) `invocation_complete` with `status: "error"`, or emit a dedicated `invocation_error` so the lifecycle stream always agrees with the real exit code. At minimum, document this edge case in EVENTS.md so consumers know that a late rejection after `invocation_complete` can still flip the process exit code.
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 error() and complete() short-circuit once completed = true. The finally block in headless.ts / index.ts runs Invocation.complete() → await Invocation.flush() → process.exit(). If an unhandledRejection (or uncaughtException) arrives during the await Invocation.flush() window — which is a real async gap where the event loop runs — the rejection handler calls Invocation.abort(e), but error() returns early (completed === true) and complete() is also a no-op. The NDJSON stream therefore already carries invocation_complete.status = "completed" from the happy path, while the process actually exits with code 1 from the rejection handler. Consumers trusting the lifecycle envelope as the source of truth for invocation result get a false success in the exact error path they most need to trust. The window is narrow but reachable (any fire-and-forget promise that rejects during the flush drain will hit it).
error(_error: unknown, code = "INVOCATION_FAILED") {
if (!invocationID || failed || completed) return
failed = true
if (sessionID) return
emit("invocation_error", {
phase,
code,
message: `Invocation failed during ${phase}`,
})
},| invocationID, | ||
| ...data, | ||
| }) + EOL | ||
| writes = writes.then( |
There was a problem hiding this comment.
🟡 flush() may reject; handlers await without try/catch.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/cli/invocation.ts:45-50):
Problem: flush() may reject; handlers await without try/catch
Detail: `emit()` chains writes via `writes = writes.then(() => new Promise((resolve) => process.stdout.write(line, () => resolve())))`. If `process.stdout.write` ever throws synchronously — e.g. stdout was destroyed because a downstream consumer like `head` exited early and closed the pipe — the `new Promise` executor throws, the Promise constructor converts that into a rejection, and the whole `writes` chain rejects with no `.catch()`. `flush()` then returns a rejected promise. The `unhandledRejection` / `uncaughtException` handlers in `headless.ts` and `index.ts` do `await Invocation.flush()` with no try/catch, so the handler itself throws and never reaches `process.exit(1)`, producing a fresh uncaughtException that re-enters the same handler — potentially looping or hanging instead of terminating. The window is narrow (stdout must throw synchronously rather than emit 'error'), but the failure mode is bad: the CLI cannot exit cleanly when its output stream is broken, which is exactly when a consumer is most likely to have closed it.
Suggested fix: Wrap the write in a try/catch and have the callback's err argument reject explicitly; and wrap `await Invocation.flush()` in the process error handlers with try/catch so a flush failure can never prevent `process.exit(1)`:
```ts
process.on("unhandledRejection", async (e) => {
Invocation.abort(e)
Log.Default.error("rejection", { ... })
try { await Invocation.flush() } catch {}
process.exit(1)
})
```
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
emit() chains writes via writes = writes.then(() => new Promise((resolve) => process.stdout.write(line, () => resolve()))). If process.stdout.write ever throws synchronously — e.g. stdout was destroyed because a downstream consumer like head exited early and closed the pipe — the new Promise executor throws, the Promise constructor converts that into a rejection, and the whole writes chain rejects with no .catch(). flush() then returns a rejected promise. The unhandledRejection / uncaughtException handlers in headless.ts and index.ts do await Invocation.flush() with no try/catch, so the handler itself throws and never reaches process.exit(1), producing a fresh uncaughtException that re-enters the same handler — potentially looping or hanging instead of terminating. The window is narrow (stdout must throw synchronously rather than emit 'error'), but the failure mode is bad: the CLI cannot exit cleanly when its output stream is broken, which is exactly when a consumer is most likely to have closed it.
writes = writes.then(
() =>
new Promise<void>((resolve) => {
process.stdout.write(line, () => resolve())
}),
)| import { JsonMigration } from "./storage/json-migration" | ||
| import { Database } from "./storage/db" | ||
| import { Invocation, startInvocation } from "./cli/invocation" | ||
|
|
There was a problem hiding this comment.
🟡 Lifecycle wiring duplicated in headless.ts and index.ts.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #96, packages/cli/src/headless.ts:23-34):
Problem: Lifecycle wiring duplicated in headless.ts and index.ts
Detail: This PR adds the same ~15 lines of Invocation lifecycle wiring to BOTH `headless.ts` and `index.ts`: the `import { Invocation, startInvocation }` line, the top-level `startInvocation()` call, the `Invocation.abort(e)` + `await Invocation.flush()` additions inside both `unhandledRejection` and `uncaughtException` handlers, the `Invocation.abort(..., "INVOCATION_PARSE_ERROR")` + `if (Invocation.id) throw new Error(...)` block in the yargs `.fail` handler, the `Invocation.error(e)` line in `catch`, and the `Invocation.complete()` + `await Invocation.flush()` pair in `finally`. The two files were already near-duplicates; this PR extends the byte-for-byte duplication. Any future fix to the lifecycle wiring (e.g. the INVOCATION_PARSE_ERROR bug also raised in this review) must be applied to both files in lockstep, or the two entry points will drift. The project convention (per the consistency checklist) is to move cross-file behaviour into a shared module rather than copy it.
Suggested fix: Extract the process-handler registrations, the yargs `.fail` Invocation hook, and the try/catch/finally Invocation cleanup into a single shared function exported from `invocation.ts` (e.g. `registerInvocationLifecycle(cli)`), and call it once from each entry point. The two INVOCATION_PARSE_ERROR bugs raised elsewhere in this review are a concrete example of the maintenance cost this duplication creates.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
This PR adds the same ~15 lines of Invocation lifecycle wiring to BOTH headless.ts and index.ts: the import { Invocation, startInvocation } line, the top-level startInvocation() call, the Invocation.abort(e) + await Invocation.flush() additions inside both unhandledRejection and uncaughtException handlers, the Invocation.abort(..., "INVOCATION_PARSE_ERROR") + if (Invocation.id) throw new Error(...) block in the yargs .fail handler, the Invocation.error(e) line in catch, and the Invocation.complete() + await Invocation.flush() pair in finally. The two files were already near-duplicates; this PR extends the byte-for-byte duplication. Any future fix to the lifecycle wiring (e.g. the INVOCATION_PARSE_ERROR bug also raised in this review) must be applied to both files in lockstep, or the two entry points will drift. The project convention (per the consistency checklist) is to move cross-file behaviour into a shared module rather than copy it.
import { Invocation, startInvocation } from "./cli/invocation"
startInvocation()
process.on("unhandledRejection", async (e) => {
Invocation.abort(e)
Log.Default.error("rejection", {
e: e instanceof Error ? e.message : e,
stack: e instanceof Error ? e.stack : undefined,
})
await Invocation.flush()
process.exit(1)
})
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 3 · 🟡 3 · ⚪ 0 · 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 |
Remediation commit: Verification:
This user-triggered remediation round is bounded; no further automated review loop is being requested. |
7391603 to
9928593
Compare
9928593 to
2835d9a
Compare
1 participant
Closes #90
Intent
Give every JSON-mode run a deterministic invocation envelope, including failures that happen before a real session exists.
Expected outcomes
Expected impact
Orchestrators can reliably detect that a run started and finished even when session creation never succeeds. Consumers gain stable failure classification and correlation while the schema-v1 change remains additive.
Summary
invocation_startbefore stdin, validation, and bootstrap for JSON runsinvocation_errorevents and exactly oneinvocation_completeinvocationIDwithout fabricating session IDsVerification
bun testfrompackages/cli— 1332 passed, 7 skippedbun test test/cli/invocation-lifecycle.test.ts test/cli/run-schema-v1.test.ts test/cli/run-session-error-emit.test.ts test/cli/run-event-race.test.ts test/cli/run-error-propagation.test.ts test/cli/exception-handler.test.ts— 24 passedbun run typecheckfrompackages/clibun turbo typecheck— 6 tasks passedRisk
The lifecycle starts only for explicit
run --format json/run --format=jsoninvocations. Output durability and forced-exit flushing remain scoped to #91.