fix(run): preserve terminal usage provenance#97
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| }) | ||
| const contextUsed = tokens.input + tokens.cache.read + tokens.cache.write | ||
| const context = buildContextWindow(contextLimit, contextUsed) | ||
| const contextLimit = args.attach |
There was a problem hiding this comment.
🟠 args.attach nulls contextLimit; undocumented behavior change.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/src/cli/cmd/run.ts:528-531):
Problem: args.attach nulls contextLimit; undocumented behavior change
Detail: The new `args.attach ? null : Provider.getModel(...)` short-circuit forces `context: null` for every attached run, even when the model is locally registered (claude-sonnet, glm-4.7, gpt-4) and `info.tokens` is fully reported. The pre-existing `.catch(Provider.ModelNotFoundError -> null)` already handles the genuinely-unregistered case, making the explicit `args.attach` branch redundant for that purpose. EVENTS.md's `context` section (updated in this same PR) enumerates the null cases as only "model limit not known" or "usage is missing" — attach mode is a third, unenumerated case, so the code contradicts the doc. The PR title and description ("preserve terminal usage provenance") do not mention any context-window behavior change, suggesting this is either unjustified scope creep or an unreviewed edit.
Suggested fix: Either drop the `args.attach ? null :` short-circuit and rely on the existing `ModelNotFoundError -> null` catch to return null only when the model is genuinely unregistered, or — if the attach-mode null is intentional (e.g. the local registry cannot vouch for the remote model) — document it as a third null case in EVENTS.md's `context` section and add a code comment explaining why.
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 args.attach ? null : Provider.getModel(...) short-circuit forces context: null for every attached run, even when the model is locally registered (claude-sonnet, glm-4.7, gpt-4) and info.tokens is fully reported. The pre-existing .catch(Provider.ModelNotFoundError -> null) already handles the genuinely-unregistered case, making the explicit args.attach branch redundant for that purpose. EVENTS.md's context section (updated in this same PR) enumerates the null cases as only "model limit not known" or "usage is missing" — attach mode is a third, unenumerated case, so the code contradicts the doc. The PR title and description ("preserve terminal usage provenance") do not mention any context-window behavior change, suggesting this is either unjustified scope creep or an unreviewed edit.
const contextLimit = args.attach
? null
: await Provider.getModel(info.providerID, info.modelID)
.then((m) => m.limit.context)
.catch((e) => {
if (e instanceof Provider.ModelNotFoundError) return null
throw e
})| } | ||
| delete s[sessionID] | ||
| SessionStatus.set(sessionID, { type: "idle" }) | ||
| // The loop's deferred cleanup publishes idle after the terminal assistant |
There was a problem hiding this comment.
🟠 cancel() drops idle; external callers deadlock consumers.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/src/session/prompt.ts:274-276):
Problem: cancel() drops idle; external callers deadlock consumers
Detail: The exported `SessionPrompt.cancel()` previously published `SessionStatus.set(sessionID, { type: "idle" })`. This PR removes that line and relocates idle publication to the single `using _ = defer(...)` scope inside the prompt loop. `cancel` is an exported symbol imported by run.ts, github.ts, session/index.ts, revert.ts, and tool/task.ts. Any caller of `cancel()` outside that one defer scope (e.g. `Instance.state()`'s dispose handler, signal handlers, session lifecycle cleanup) now silently drops the idle status. Headless SSE consumers that gate stop on `session.status: idle` — the exact pattern this PR hardens — will block indefinitely waiting for an idle event that never arrives. The added tests do not catch this: the mock SSE server in run-message-terminal.test.ts injects `session.status: idle` itself, and prompt-reliability.test.ts only asserts source-text ordering within the one defer scope.
Suggested fix: Either keep `SessionStatus.set(sessionID, { type: "idle" })` inside `cancel()` (the defer then becomes a harmless redundant superset that still publishes idle after the terminal update), or audit every caller of `cancel()` — including `Instance.state()`'s dispose handler and any abort/signal path — and ensure each publishes idle or routes through the defer. At minimum, add a behavioral test that calling `cancel(sessionID)` directly results in a `session.status: idle` event being observable.
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 exported SessionPrompt.cancel() previously published SessionStatus.set(sessionID, { type: "idle" }). This PR removes that line and relocates idle publication to the single using _ = defer(...) scope inside the prompt loop. cancel is an exported symbol imported by run.ts, github.ts, session/index.ts, revert.ts, and tool/task.ts. Any caller of cancel() outside that one defer scope (e.g. Instance.state()'s dispose handler, signal handlers, session lifecycle cleanup) now silently drops the idle status. Headless SSE consumers that gate stop on session.status: idle — the exact pattern this PR hardens — will block indefinitely waiting for an idle event that never arrives. The added tests do not catch this: the mock SSE server in run-message-terminal.test.ts injects session.status: idle itself, and prompt-reliability.test.ts only asserts source-text ordering within the one defer scope.
cb.reject(reason)
}
delete s[sessionID]
// The loop's deferred cleanup publishes idle after the terminal assistant
// update, so event consumers cannot stop before observing the aborted turn.
return
}| : { | ||
| total: | ||
| info.tokens.total ?? | ||
| info.tokens.input + info.tokens.output + info.tokens.cache.read + info.tokens.cache.write, |
There was a problem hiding this comment.
🟡 total token fallback omits the reasoning bucket.
--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -507,7 +507,7 @@
: {
total:
info.tokens.total ??
- info.tokens.input + info.tokens.output + info.tokens.cache.read + info.tokens.cache.write,
+ info.tokens.input + info.tokens.output + info.tokens.reasoning + info.tokens.cache.read + info.tokens.cache.write,
input: info.tokens.input,🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/src/cli/cmd/run.ts:510-511):
Problem: total token fallback omits the reasoning bucket
Detail: The fallback total `info.tokens.input + info.tokens.output + info.tokens.cache.read + info.tokens.cache.write` omits the `reasoning` bucket. EVENTS.md states the five token buckets are non-overlapping ("a token is counted in exactly one bucket"), so a computed total must sum all five. The fallback undercounts by `reasoning` for extended-thinking models (o1, claude-with-thinking) when replaying legacy sessions where `info.tokens.total` was never persisted. It is also inconsistent with the sibling `getUsage` total computation in session/index.ts, which uses a different fallback formula — the two computed-total paths now disagree.
Suggested fix: Add `info.tokens.reasoning` to the sum so the computed total matches the documented non-overlapping five-bucket breakdown.
Suggested patch:
--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -507,7 +507,7 @@
: {
total:
info.tokens.total ??
- info.tokens.input + info.tokens.output + info.tokens.cache.read + info.tokens.cache.write,
+ info.tokens.input + info.tokens.output + info.tokens.reasoning + info.tokens.cache.read + info.tokens.cache.write,
input: info.tokens.input,
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 fallback total info.tokens.input + info.tokens.output + info.tokens.cache.read + info.tokens.cache.write omits the reasoning bucket. EVENTS.md states the five token buckets are non-overlapping ("a token is counted in exactly one bucket"), so a computed total must sum all five. The fallback undercounts by reasoning for extended-thinking models (o1, claude-with-thinking) when replaying legacy sessions where info.tokens.total was never persisted. It is also inconsistent with the sibling getUsage total computation in session/index.ts, which uses a different fallback formula — the two computed-total paths now disagree.
: {
total:
info.tokens.total ??
info.tokens.input + info.tokens.output + info.tokens.cache.read + info.tokens.cache.write,
input: info.tokens.input,| processor.indexOf('if (needsCompaction) return "compact"'), | ||
| ) | ||
| const failure = processor.slice( | ||
| processor.indexOf("} catch (e: any) {"), |
There was a problem hiding this comment.
🟡 Source-text slice can pass failure assertion vacuously.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/test/session/prompt-reliability.test.ts:40-45):
Problem: Source-text slice can pass failure assertion vacuously
Detail: The new test slices processor.ts source text via `processor.indexOf("} catch (e: any) {")` to isolate the failure path. `indexOf` returns the FIRST match; processor.ts contains multiple catch sites (retry-loop catch and the outer failure catch), so if the first `} catch (e: any) {` in source order is not the intended failure catch, the `failure` window is wrong. Worse, if the catch syntax is ever tweaked (`} catch {` or `} catch (e) {`), `indexOf` returns -1, `slice(-1, X)` yields a near-empty string, and `expect(failure).not.toContain('type: "idle"')` passes vacuously — providing false assurance that the failure path no longer publishes idle. The same fragility applies to the `terminal` and `cleanup` slices if their anchor strings drift.
Suggested fix: Replace source-text slicing with a behavioral test (drive the processor through an error path with a fake SessionStatus recorder and assert no idle is published before updateMessage resolves). If source-text assertions must stay, anchor each slice with a unique marker, assert the window is non-empty before asserting contents, and use `lastIndexOf` or a comment sentinel to disambiguate multiple matches.
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 test slices processor.ts source text via processor.indexOf("} catch (e: any) {") to isolate the failure path. indexOf returns the FIRST match; processor.ts contains multiple catch sites (retry-loop catch and the outer failure catch), so if the first } catch (e: any) { in source order is not the intended failure catch, the failure window is wrong. Worse, if the catch syntax is ever tweaked (} catch { or } catch (e) {), indexOf returns -1, slice(-1, X) yields a near-empty string, and expect(failure).not.toContain('type: "idle"') passes vacuously — providing false assurance that the failure path no longer publishes idle. The same fragility applies to the terminal and cleanup slices if their anchor strings drift.
const failure = processor.slice(
processor.indexOf("} catch (e: any) {"),
processor.indexOf("input.assistantMessage.time.completed = Date.now()"),
)
expect(cleanup.indexOf("cancel(sessionID)")).toBeLessThan(cleanup.indexOf("SessionStatus.set(sessionID"))
expect(failure).not.toContain('type: "idle"')
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 2 · 🟡 2 · ⚪ 0 · 0/4 resolved
🤖 Fix all 4 open findings with your agent📋 Out-of-diff findings (4)
Reviewed 10 files · 0 inline · view all 4 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Remediation commit: Verification:
The push triggered one provider re-review and CI round for the new head. |
| const contextUsed = tokens.input + tokens.cache.read + tokens.cache.write | ||
| const context = buildContextWindow(contextLimit, contextUsed) | ||
| const contextLimit = await (args.attach | ||
| ? ModelsDev.get().then( |
There was a problem hiding this comment.
🟠 ModelsDev.get() in attach path has no .catch.
--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -533,7 +533,7 @@
const contextLimit = await (args.attach
? ModelsDev.get().then(
(providers) => providers[info.providerID]?.models[info.modelID]?.limit.context ?? null,
- )
+ ).catch(() => null)
: Provider.getModel(info.providerID, info.modelID)
.then((m) => m.limit.context)
.catch((e) => {🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/src/cli/cmd/run.ts:534-542):
Problem: ModelsDev.get() in attach path has no .catch
Detail: The new contextLimit ternary treats the attach and non-attach paths asymmetrically. The non-attach path catches `Provider.ModelNotFoundError` and returns null (graceful degradation when the model is not in the registry). The attach path (`ModelsDev.get().then(providers => providers[info.providerID]?.models[info.modelID]?.limit.context ?? null)`) has NO .catch at all.
`ModelsDev.get()` reads a local registry file (AICTRL_MODELS_PATH). In `--attach` mode the user is connecting to an external server and may legitimately not have a models file configured; if it is missing, unreadable, or malformed, the promise rejects. The handler awaits this directly with no outer try/catch, so the rejection propagates out of the async `message.updated` handler. Worse, `ModelsDev.get()` is typically a cached singleton — the first rejection poisons every subsequent lookup for the rest of the run, suppressing all `message_complete` emissions in attach mode. The non-attach path degrades to `context: null`; the attach path should do the same.
Suggested fix: Add a `.catch(() => null)` to the attach branch mirroring the non-attach path's graceful degradation.
Suggested patch:
--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -533,7 +533,7 @@
const contextLimit = await (args.attach
? ModelsDev.get().then(
(providers) => providers[info.providerID]?.models[info.modelID]?.limit.context ?? null,
- )
+ ).catch(() => null)
: Provider.getModel(info.providerID, info.modelID)
.then((m) => m.limit.context)
.catch((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
The new contextLimit ternary treats the attach and non-attach paths asymmetrically. The non-attach path catches Provider.ModelNotFoundError and returns null (graceful degradation when the model is not in the registry). The attach path (ModelsDev.get().then(providers => providers[info.providerID]?.models[info.modelID]?.limit.context ?? null)) has NO .catch at all.
ModelsDev.get() reads a local registry file (AICTRL_MODELS_PATH). In --attach mode the user is connecting to an external server and may legitimately not have a models file configured; if it is missing, unreadable, or malformed, the promise rejects. The handler awaits this directly with no outer try/catch, so the rejection propagates out of the async message.updated handler. Worse, ModelsDev.get() is typically a cached singleton — the first rejection poisons every subsequent lookup for the rest of the run, suppressing all message_complete emissions in attach mode. The non-attach path degrades to context: null; the attach path should do the same.
const contextLimit = await (args.attach
? ModelsDev.get().then(
(providers) => providers[info.providerID]?.models[info.modelID]?.limit.context ?? null,
)
: Provider.getModel(info.providerID, info.modelID)
.then((m) => m.limit.context)
.catch((e) => {
if (e instanceof Provider.ModelNotFoundError) return null
throw e
}))| const info = event.properties.info | ||
| if (args.format === "json") { | ||
| if (info.finish) { | ||
| if (info.sessionID === sessionID && info.time.completed !== undefined && !emitted.has(info.id)) { |
There was a problem hiding this comment.
🟡 Child-session filter silently drops message_complete events.
| if (info.sessionID === sessionID && info.time.completed !== undefined && !emitted.has(info.id)) { | |
| Either document the scoping in EVENTS.md ("message_complete is emitted only for the primary session; child-session assistant messages are not reflected"), or broaden the filter to `info.sessionID === sessionID || childSessions.has(info.sessionID)` if child telemetry is desired. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/src/cli/cmd/run.ts:499):
Problem: Child-session filter silently drops message_complete events
Detail: The emit gate changed from `if (info.finish)` to `if (info.sessionID === sessionID && info.time.completed !== undefined && !emitted.has(info.id))`. The added `info.sessionID === sessionID` clause filters out assistant messages produced by child sessions (e.g. those spawned by the task tool). Previously, any assistant message with a finish reason — including child-session messages — produced a `message_complete` event. The new test explicitly asserts `msg_child` (sessionID: 'ses_child') is filtered out, so the scoping is intentional, but EVENTS.md does not document it.
Consumers that aggregate usage/cost across the primary and child sessions (e.g. for billing or CI telemetry) will silently stop receiving child-session `message_complete` events after this PR. EVENTS.md should state that `message_complete` is scoped to the primary session only, or the filter should be broadened to tracked `childSessions`.
Suggested fix: Either document the scoping in EVENTS.md ("message_complete is emitted only for the primary session; child-session assistant messages are not reflected"), or broaden the filter to `info.sessionID === sessionID || childSessions.has(info.sessionID)` if child telemetry is desired.
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 emit gate changed from if (info.finish) to if (info.sessionID === sessionID && info.time.completed !== undefined && !emitted.has(info.id)). The added info.sessionID === sessionID clause filters out assistant messages produced by child sessions (e.g. those spawned by the task tool). Previously, any assistant message with a finish reason — including child-session messages — produced a message_complete event. The new test explicitly asserts msg_child (sessionID: 'ses_child') is filtered out, so the scoping is intentional, but EVENTS.md does not document it.
Consumers that aggregate usage/cost across the primary and child sessions (e.g. for billing or CI telemetry) will silently stop receiving child-session message_complete events after this PR. EVENTS.md should state that message_complete is scoped to the primary session only, or the filter should be broadened to tracked childSessions.
if (args.format === "json") {
if (info.sessionID === sessionID && info.time.completed !== undefined && !emitted.has(info.id)) {
emitted.add(info.id)
const usageStatus = info.usageStatus ?? (info.finish ? "reported" : "missing")| return adjustedInputTokens + outputTokens + cacheReadInputTokens + cacheWriteInputTokens | ||
| } | ||
| return input.usage.totalTokens | ||
| if (Number.isFinite(input.usage.totalTokens)) return input.usage.totalTokens |
There was a problem hiding this comment.
🟡 Partial-sum total labeled usageStatus "reported".
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/src/session/index.ts:865-867):
Problem: Partial-sum total labeled usageStatus "reported"
Detail: When `input.usage.totalTokens` is not finite but `inputTokens` and `outputTokens` are, the `total` falls back to `inputTokens + outputTokens`, silently excluding reasoning/cache.read/cache.write tokens. That sum is a derived partial total, yet the message is still tagged `usageStatus: "reported"` (because usageStatus only requires ANY one of the six token fields to be finite). The schema enum in message-v2.ts declares `"estimated"` and EVENTS.md describes it as "an explicitly estimated future source", but no code path emits it. Downstream consumers (billing, accounting, telemetry) will treat a computed partial sum as a provider-reported total. Either emit `"estimated"` when the totalTokens-absent fallback branch is taken, or omit `total` on that branch.
Suggested fix: When the `return inputTokens + outputTokens` fallback is taken, mark the result as estimated by extending usageStatus logic (e.g. `totalReported ? "reported" : "estimated"`), or omit `total` so consumers see `tokens.total === undefined` for the partial case.
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 input.usage.totalTokens is not finite but inputTokens and outputTokens are, the total falls back to inputTokens + outputTokens, silently excluding reasoning/cache.read/cache.write tokens. That sum is a derived partial total, yet the message is still tagged usageStatus: "reported" (because usageStatus only requires ANY one of the six token fields to be finite). The schema enum in message-v2.ts declares "estimated" and EVENTS.md describes it as "an explicitly estimated future source", but no code path emits it. Downstream consumers (billing, accounting, telemetry) will treat a computed partial sum as a provider-reported total. Either emit "estimated" when the totalTokens-absent fallback branch is taken, or omit total on that branch.
if (!Number.isFinite(input.usage.inputTokens) || !Number.isFinite(input.usage.outputTokens)) return
return adjustedInputTokens + outputTokens + cacheReadInputTokens + cacheWriteInputTokens
}
if (Number.isFinite(input.usage.totalTokens)) return input.usage.totalTokens
if (!Number.isFinite(input.usage.inputTokens) || !Number.isFinite(input.usage.outputTokens)) return
return inputTokens + outputTokens
})| cb.reject(reason) | ||
| } | ||
| delete s[sessionID] | ||
| if (match.loop) return false |
There was a problem hiding this comment.
🟡 cancel() no longer publishes idle for loop sessions.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/src/session/prompt.ts:277-279):
Problem: cancel() no longer publishes idle for loop sessions
Detail: `cancel()` previously always transitioned the session to `{type:"idle"}`. After this PR, when `match.loop === true` it returns `false` and skips `SessionStatus.set(sessionID, {type:"idle"})`, relying on the `defer(() => { if (cancel(sessionID)) return; SessionStatus.set(sessionID, {type:"idle"}) })` block inside `loop()` to publish idle. That return-value contract is only honored by the loop's own defer.
`cancel` is an exported symbol. Any OTHER caller that cancels a loop-based session (e.g. an external cancel API, or a signal handler that calls cancel directly rather than going through `loop()`'s scope) will receive `false` and, unless it knows to set idle itself, the session remains visibly "busy" indefinitely — which can wedge headless consumers that gate teardown on the idle status. Idle publication is now structurally coupled to a return-value convention only one caller implements.
Suggested fix: Keep idle publication in `cancel()` unconditional for external callers; let the loop's defer skip it via an option, e.g. `cancel(sessionID, { suppressIdle: true })` for the internal loop-teardown path, defaulting to publishing idle.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
cancel() previously always transitioned the session to {type:"idle"}. After this PR, when match.loop === true it returns false and skips SessionStatus.set(sessionID, {type:"idle"}), relying on the defer(() => { if (cancel(sessionID)) return; SessionStatus.set(sessionID, {type:"idle"}) }) block inside loop() to publish idle. That return-value contract is only honored by the loop's own defer.
cancel is an exported symbol. Any OTHER caller that cancels a loop-based session (e.g. an external cancel API, or a signal handler that calls cancel directly rather than going through loop()'s scope) will receive false and, unless it knows to set idle itself, the session remains visibly "busy" indefinitely — which can wedge headless consumers that gate teardown on the idle status. Idle publication is now structurally coupled to a return-value convention only one caller implements.
delete s[sessionID]
if (match.loop) return false
SessionStatus.set(sessionID, { type: "idle" })
return true
}| root: Instance.worktree, | ||
| }, | ||
| cost: 0, | ||
| usageStatus: "missing", |
There was a problem hiding this comment.
⚪ usageStatus field order diverges from Session.getUsage.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/src/session/compaction.ts:127):
Problem: usageStatus field order diverges from Session.getUsage
Detail: `Session.getUsage` returns `{ usageStatus, cost, tokens }` (usageStatus first), but the manually-constructed usage object in compaction.ts inserts `usageStatus` between `cost` and `tokens` (`{ cost: 0, usageStatus: "missing", tokens: { ... } }`). Both objects represent the same shape; aligning the field order would keep the two construction sites visually consistent and make future diffs easier to scan. Semantically equivalent.
Suggested fix: Reorder compaction.ts to `{ usageStatus: "missing", cost: 0, tokens: { ... } }` to match the getUsage return shape.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Session.getUsage returns { usageStatus, cost, tokens } (usageStatus first), but the manually-constructed usage object in compaction.ts inserts usageStatus between cost and tokens ({ cost: 0, usageStatus: "missing", tokens: { ... } }). Both objects represent the same shape; aligning the field order would keep the two construction sites visually consistent and make future diffs easier to scan. Semantically equivalent.
root: Instance.worktree,
},
cost: 0,
usageStatus: "missing",
tokens: {
output: 0,
input: 0,| const inputTokens = safe(input.usage.inputTokens ?? 0) | ||
| const outputTokens = safe(input.usage.outputTokens ?? 0) | ||
| const reasoningTokens = safe(input.usage.reasoningTokens ?? 0) | ||
|
|
There was a problem hiding this comment.
⚪ Unsafe cast on untyped provider metadata.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #97, packages/cli/src/session/index.ts:834):
Problem: Unsafe cast on untyped provider metadata
Detail: `cacheWrite` is read from provider-supplied metadata via three consecutive `@ts-expect-error` accesses (untyped external data), then handed to `safe()` with an explicit `as number | undefined` cast. The cast asserts a type the runtime never validated at the access site. `safe()` happens to reject non-numbers (via `typeof value !== "number"`), so this is not exploitable, but the cast is misleading: it papers over the fact that the value originates from untrusted provider metadata. Prefer leaving it `unknown` and letting `safe()` narrow it, or validate explicitly.
Suggested fix: Drop the cast: `const cacheWriteInputTokens = safe(typeof cacheWrite === "number" ? cacheWrite : undefined)`, or have `safe()` accept `unknown`.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
cacheWrite is read from provider-supplied metadata via three consecutive @ts-expect-error accesses (untyped external data), then handed to safe() with an explicit as number | undefined cast. The cast asserts a type the runtime never validated at the access site. safe() happens to reject non-numbers (via typeof value !== "number"), so this is not exploitable, but the cast is misleading: it papers over the fact that the value originates from untrusted provider metadata. Prefer leaving it unknown and letting safe() narrow it, or validate explicitly.
const cacheReadInputTokens = safe(input.usage.cachedInputTokens ?? 0)
const cacheWriteInputTokens = safe(cacheWrite as number | undefined)
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 |
Remediation commit: Verification:
This is the second and final bounded remediation round; no further automated-review loop will be initiated. |
|
CI remediation: the failed Commit Verification after the adjustment:
|
|
Readability revision: commit Before: the loop combined provenance fallback, missing-usage branching, legacy five-bucket total reconstruction, and payload shaping in one nested expression. After: Preserved by the existing behavioral coverage: primary-session filtering, reported zero, missing/reported provenance, partial totals remaining absent, legacy five-bucket total, attached context fallback, terminal status/deduplication, and terminal-before-idle ordering. Verification:
|
1 participant
Closes #93
Closes #45
Intent
Make terminal assistant-message telemetry trustworthy by preserving completion state, usage provenance, and terminal-before-idle ordering.
Expected outcomes
Expected impact
Billing, CI telemetry, and operational consumers can distinguish real provider usage from missing data and can stop on idle without losing the final message outcome. Child-session messages remain outside the primary-session terminal stream by design.
Summary
message_completeper primary assistant message, keyed by stablemessageIDcompleted,error, andabortedstatus plusreported,missing, and reservedestimatedusage provenancenull, and keep reported zero distinctVerification
bun test test/cli/run-message-terminal.test.ts test/session/compaction.test.ts test/session/prompt-reliability.test.ts test/cli/usage-token-breakdown.test.ts— 56 passbun test --timeout 30000— 1327 pass, 7 skipbun run typecheck— passbun turbo typecheck— 6/6 tasks pass./packages/sdk/script/build.ts— generated output unchangedgit diff --check— passRisk / compatibility
The NDJSON additions are additive for successful schema-v1 events. Failed and aborted terminal events now emit with
tokens: nullwhen provider usage is unavailable. Persisted messages written before this change inferreportedwhen they have a finish reason andmissingotherwise. The CLI does not fabricate estimated usage.