Conversation
Review: retry & execution outcome telemetry (#110)Overall this is well built: the new status fields are additive and optional, the free-text 1. A late session error can flip an already-recovered retry to
|
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| const classified = classifySessionError(cause) | ||
| error ??= classified.message | ||
| invocation.error(cause) | ||
| resolveRetry(sessionID, "failed") |
There was a problem hiding this comment.
🟠 Exit path mislabels cancelled retry as "failed".
--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -967,6 +967,9 @@
const classified = classifySessionError(cause)
error ??= classified.message
invocation.error(cause)
- resolveRetry(sessionID, "failed")
+ resolveRetry(
+ sessionID,
+ classified.reason === "interrupted" || classified.reason === "terminated" ? "aborted" : "failed",
+ )
report(classified.reason, classified.code, classified.message)🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #115, packages/cli/src/cli/cmd/run.ts:970):
Problem: Exit path mislabels cancelled retry as "failed"
Detail: The fatal-exit handler hardcodes resolveRetry(sessionID, "failed") immediately after computing classified = classifySessionError(cause), which can classify the cause as cancellation ("interrupted" for the new MessageAbortedError branch, or "terminated"). The sibling session.error handler maps those same reasons to "aborted", and interrupt() maps signal cancellation to "aborted"; EVENTS.md states "cancellation is `aborted`". A cancellation surfacing through the exit path therefore emits retry_complete outcome "failed" while session_error reports "interrupted", contradicting the documented contract and polluting the retry-recovery-rate metric (denominator counts "failed" but excludes "aborted"). Repro: Given a run --format json session with a pending retry and no terminal message yet, When the session terminates via the fatal-exit path with a cancellation error (MessageAbortedError), Then retry_complete outcome is "failed" even though EVENTS.md requires "aborted".
Suggested fix: Mirror the session.error mapping: resolveRetry(sessionID, classified.reason === "interrupted" || classified.reason === "terminated" ? "aborted" : "failed"), or extract a shared retryOutcomeForClassified(reason) helper used by all three call sites.
Suggested patch:
--- a/packages/cli/src/cli/cmd/run.ts
+++ b/packages/cli/src/cli/cmd/run.ts
@@ -967,6 +967,9 @@
const classified = classifySessionError(cause)
error ??= classified.message
invocation.error(cause)
- resolveRetry(sessionID, "failed")
+ resolveRetry(
+ sessionID,
+ classified.reason === "interrupted" || classified.reason === "terminated" ? "aborted" : "failed",
+ )
report(classified.reason, classified.code, classified.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
The fatal-exit handler hardcodes resolveRetry(sessionID, "failed") immediately after computing classified = classifySessionError(cause), which can classify the cause as cancellation ("interrupted" for the new MessageAbortedError branch, or "terminated"). The sibling session.error handler maps those same reasons to "aborted", and interrupt() maps signal cancellation to "aborted"; EVENTS.md states "cancellation is aborted". A cancellation surfacing through the exit path therefore emits retry_complete outcome "failed" while session_error reports "interrupted", contradicting the documented contract and polluting the retry-recovery-rate metric (denominator counts "failed" but excludes "aborted"). Repro: Given a run --format json session with a pending retry and no terminal message yet, When the session terminates via the fatal-exit path with a cancellation error (MessageAbortedError), Then retry_complete outcome is "failed" even though EVENTS.md requires "aborted".
const classified = classifySessionError(cause)
error ??= classified.message
invocation.error(cause)
resolveRetry(sessionID, "failed")
report(classified.reason, classified.code, classified.message)| } | ||
| ``` | ||
|
|
||
| Identity and dimension fields match `retry_scheduled`. `outcome` is `recovered`, `failed`, `aborted`, or `unknown`. A retry superseded by another retry is `failed`; a successful terminal assistant message is `recovered`; cancellation is `aborted`; an idle boundary without an observable terminal message is `unknown`. If the event stream ends before `retry_complete`, the attempt is censored and must remain unknown. |
There was a problem hiding this comment.
🟡 Doc: superseded retry not always "failed" in code.
| Identity and dimension fields match `retry_scheduled`. `outcome` is `recovered`, `failed`, `aborted`, or `unknown`. A retry superseded by another retry is `failed`; a successful terminal assistant message is `recovered`; cancellation is `aborted`; an idle boundary without an observable terminal message is `unknown`. If the event stream ends before `retry_complete`, the attempt is censored and must remain unknown. | |
| Qualify the EVENTS.md sentence to match the code: "A retry superseded by another retry for the same message is `failed`; supersession by a retry for a different message resolves via the prior message's observed terminal outcome." Alternatively make the code resolve unconditionally as "failed" when superseded. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #115, EVENTS.md:222):
Problem: Doc: superseded retry not always "failed" in code
Detail: EVENTS.md states unconditionally "A retry superseded by another retry is `failed`", but scheduleRetry in run.ts resolves the superseded retry as "failed" only when both retries carry the same non-null messageID; otherwise it falls back to retryOutcome(current), which can emit "recovered", "aborted", or "unknown". A pending retry for one assistant message superseded by a retry for a different message (or by an uncorrelated older-server retry) is therefore not necessarily "failed" — the PR's own test ("keeps retry outcomes scoped to their owning message across tool turns") relies on the cross-message behavior. Either the doc or the implementation is the contract; consumers following the doc will mis-aggregate the retry-recovery-rate metric.
Suggested fix: Qualify the EVENTS.md sentence to match the code: "A retry superseded by another retry for the same message is `failed`; supersession by a retry for a different message resolves via the prior message's observed terminal outcome." Alternatively make the code resolve unconditionally as "failed" when superseded.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
EVENTS.md states unconditionally "A retry superseded by another retry is failed", but scheduleRetry in run.ts resolves the superseded retry as "failed" only when both retries carry the same non-null messageID; otherwise it falls back to retryOutcome(current), which can emit "recovered", "aborted", or "unknown". A pending retry for one assistant message superseded by a retry for a different message (or by an uncorrelated older-server retry) is therefore not necessarily "failed" — the PR's own test ("keeps retry outcomes scoped to their owning message across tool turns") relies on the cross-message behavior. Either the doc or the implementation is the contract; consumers following the doc will mis-aggregate the retry-recovery-rate metric.
Identity and dimension fields match `retry_scheduled`. `outcome` is `recovered`, `failed`, `aborted`, or `unknown`. A retry superseded by another retry is `failed`; a successful terminal assistant message is `recovered`; cancellation is `aborted`; an idle boundary without an observable terminal message is `unknown`. If the event stream ends before `retry_complete`, the attempt is censored and must remain unknown.
| if (status === 429) return { reason: "rate_limit", code: "429", message } | ||
| if (status === 401 || status === 403) return { reason: "auth", code: String(status), message } | ||
| if (name === "ProviderAuthError") return { reason: "auth", code: status ? String(status) : undefined, message } | ||
| if (name === "MessageAbortedError") { |
There was a problem hiding this comment.
🟡 New "interrupted" session_error reason undocumented.
| if (name === "MessageAbortedError") { | |
| Add "interrupted" to the session_error reason list in EVENTS.md (noting it is produced by MessageAbortedError/cancellation) in the same PR that introduces the emitted value. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #115, packages/cli/src/cli/cmd/run.errors.ts:27-29):
Problem: New "interrupted" session_error reason undocumented
Detail: This PR adds reason value "interrupted" to classifySessionError, and the new integration test (run-retry-telemetry.test.ts) asserts it is emitted as session_error.reason === "interrupted" on the NDJSON stream, yet EVENTS.md is not updated anywhere in this diff — "interrupted" appears nowhere in the document. EVENTS.md is the consumer contract this PR extends; downstream consumers switching on the documented bounded session_error reason set will observe an undocumented value.
Suggested fix: Add "interrupted" to the session_error reason list in EVENTS.md (noting it is produced by MessageAbortedError/cancellation) in the same PR that introduces the emitted value.
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 reason value "interrupted" to classifySessionError, and the new integration test (run-retry-telemetry.test.ts) asserts it is emitted as session_error.reason === "interrupted" on the NDJSON stream, yet EVENTS.md is not updated anywhere in this diff — "interrupted" appears nowhere in the document. EVENTS.md is the consumer contract this PR extends; downstream consumers switching on the documented bounded session_error reason set will observe an undocumented value.
if (name === "MessageAbortedError") {\n return { reason: "interrupted", code: status ? String(status) : undefined, message }\n }| providerID: string | null | ||
| modelID: string | null | ||
| attempt: number | ||
| reason: string |
There was a problem hiding this comment.
🟡 Retry.reason typed string; status shape re-declared.
| reason: string | |
| Type `reason` as SessionRetry.Reason and have scheduleRetry accept the retry variant inferred from the status.ts zod schema (or SessionRetry.Reason-typed fields) instead of a hand-copied literal shape. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #115, packages/cli/src/cli/cmd/run.ts:553-555):
Problem: Retry.reason typed string; status shape re-declared
Detail: The bounded Reason union now lives in three mirrors (SessionRetry.Reason in retry.ts, the zod enum in status.ts, the SDK union in types.gen.ts), but run.ts types the stored/emitted Retry.reason as plain `string` and re-declares the entire session.status retry shape field-by-field in scheduleRetry's parameter instead of reusing the zod-inferred SessionStatus type. Any future field addition to status.ts must be mirrored by hand in run.ts or it silently drops out of telemetry, and the string typing discards the bounded-category guarantee EVENTS.md documents for `reason`.
Suggested fix: Type `reason` as SessionRetry.Reason and have scheduleRetry accept the retry variant inferred from the status.ts zod schema (or SessionRetry.Reason-typed fields) instead of a hand-copied literal 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
The bounded Reason union now lives in three mirrors (SessionRetry.Reason in retry.ts, the zod enum in status.ts, the SDK union in types.gen.ts), but run.ts types the stored/emitted Retry.reason as plain string and re-declares the entire session.status retry shape field-by-field in scheduleRetry's parameter instead of reusing the zod-inferred SessionStatus type. Any future field addition to status.ts must be mirrored by hand in run.ts or it silently drops out of telemetry, and the string typing discards the bounded-category guarantee EVENTS.md documents for reason.
type Retry = {\n retryID: string\n messageID: string | null\n providerID: string | null\n modelID: string | null\n attempt: number\n reason: string\n delayMs: number\n }| const status = | ||
| info.error?.name === "MessageAbortedError" ? "aborted" : info.error ? "error" : "completed" | ||
| outcomes.set(info.id, { status, finish: info.finish }) | ||
| if (emitted.has(info.id)) continue |
There was a problem hiding this comment.
🟡 continue skips loop tail for duplicate completions.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #115, packages/cli/src/cli/cmd/run.ts:648):
Problem: continue skips loop tail for duplicate completions
Detail: The original code guarded only the emission with `&& !emitted.has(info.id)`, so duplicate completed updates still fell through to the rest of the event-loop body. The new `if (emitted.has(info.id)) continue` aborts processing of the event entirely, so any statements after the assistant json block in the message.updated loop body (not visible in the diff) are now skipped for duplicate completions. If nothing meaningful follows, this is benign; if shared bookkeeping follows (control state, transcript updates), it silently regresses.
Suggested fix: Preserve the original fall-through semantics: wrap only the emission (emitted.add + emit) in `if (!emitted.has(info.id)) { ... }` so outcomes.set and any subsequent loop-body logic still run for duplicates.
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 original code guarded only the emission with && !emitted.has(info.id), so duplicate completed updates still fell through to the rest of the event-loop body. The new if (emitted.has(info.id)) continue aborts processing of the event entirely, so any statements after the assistant json block in the message.updated loop body (not visible in the diff) are now skipped for duplicate completions. If nothing meaningful follows, this is benign; if shared bookkeeping follows (control state, transcript updates), it silently regresses.
outcomes.set(info.id, { status, finish: info.finish })\n if (emitted.has(info.id)) continue\n emitted.add(info.id)\n const usage = terminalUsage(info)| reason?: string | ||
| delayMs?: number | ||
| }, | ||
| ) { |
There was a problem hiding this comment.
🟡 Legacy retry statuses cannot dedupe on replay.
| ) { | |
| For legacy statuses (no retryID), also dedupe on a stable key such as (attempt, next) or suppress re-scheduling while an unresolved legacy retry with the same attempt number exists. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #115, packages/cli/src/cli/cmd/run.ts:589):
Problem: Legacy retry statuses cannot dedupe on replay
Detail: scheduleRetry dedupes repeated retry statuses by comparing current.retryID === status.retryID. For an older server without correlation fields, the stored retryID is a locally generated UUID while status.retryID is undefined, so the comparison can never be true. Any redelivery of the same legacy retry status (SSE reconnect/replay or status re-broadcast) emits a fresh retry_scheduled plus a supersession retry_complete pair with a new retryID, double-counting retries and scheduled backoff in the documented per-retryID metrics. Correlated retries dedupe correctly; only legacy servers are affected. Repro: Given an older server (retry status without retryID) whose SSE stream reconnects with event replay, When the same retry status is delivered twice, Then two retry_scheduled/retry_complete pairs are emitted with distinct generated retryIDs.
Suggested fix: For legacy statuses (no retryID), also dedupe on a stable key such as (attempt, next) or suppress re-scheduling while an unresolved legacy retry with the same attempt number exists.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
scheduleRetry dedupes repeated retry statuses by comparing current.retryID === status.retryID. For an older server without correlation fields, the stored retryID is a locally generated UUID while status.retryID is undefined, so the comparison can never be true. Any redelivery of the same legacy retry status (SSE reconnect/replay or status re-broadcast) emits a fresh retry_scheduled plus a supersession retry_complete pair with a new retryID, double-counting retries and scheduled backoff in the documented per-retryID metrics. Correlated retries dedupe correctly; only legacy servers are affected. Repro: Given an older server (retry status without retryID) whose SSE stream reconnects with event replay, When the same retry status is delivered twice, Then two retry_scheduled/retry_complete pairs are emitted with distinct generated retryIDs.
const current = retries.get(sid)\n if (current && current.retryID === status.retryID) return\n if (current) {\n resolveRetry(\n sid,\n current.messageID && status.messageID === current.messageID ? "failed" : retryOutcome(current),\n )\n }| attempt: z.number(), | ||
| message: z.string(), | ||
| next: z.number(), | ||
| retryID: z.string().optional(), |
There was a problem hiding this comment.
🟡 Unvalidated server fields re-emitted as metric dimensions.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #115, packages/cli/src/session/status.ts:17-22):
Problem: Unvalidated server fields re-emitted as metric dimensions
Detail: The new optional fields on the SSE-parsed retry status are unconstrained: retryID/messageID/providerID/modelID are bare z.string() and delayMs is a bare z.number() with no non-negative/finite/size bounds. In --attach mode these values cross the trust boundary from the attached server and are forwarded verbatim by run.ts scheduleRetry into retry_scheduled/retry_complete NDJSON events, where EVENTS.md designates providerID/modelID as metric dimensions and delayMs as a summed quantity per distinct retryID. A buggy or compromised server can inject arbitrary high-cardinality dimension labels or negative/huge delayMs values into CI measurement pipelines. The fallback path clamps via Math.max(0, next - Date.now()), but server-supplied delayMs passes through unchecked.
Suggested fix: Constrain at the boundary in status.ts (e.g. z.string().max(256) for the ID fields, z.number().nonnegative() for delayMs), or clamp/validate in scheduleRetry before emit.
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 optional fields on the SSE-parsed retry status are unconstrained: retryID/messageID/providerID/modelID are bare z.string() and delayMs is a bare z.number() with no non-negative/finite/size bounds. In --attach mode these values cross the trust boundary from the attached server and are forwarded verbatim by run.ts scheduleRetry into retry_scheduled/retry_complete NDJSON events, where EVENTS.md designates providerID/modelID as metric dimensions and delayMs as a summed quantity per distinct retryID. A buggy or compromised server can inject arbitrary high-cardinality dimension labels or negative/huge delayMs values into CI measurement pipelines. The fallback path clamps via Math.max(0, next - Date.now()), but server-supplied delayMs passes through unchecked.
retryID: z.string().optional(),\n messageID: z.string().optional(),\n providerID: z.string().optional(),\n modelID: z.string().optional(),\n reason: z.enum(["rate_limit", "timeout", "network", "provider", "unknown"]).optional(),\n delayMs: z.number().optional(),| messageID: z.string().optional(), | ||
| providerID: z.string().optional(), | ||
| modelID: z.string().optional(), | ||
| reason: z.enum(["rate_limit", "timeout", "network", "provider", "unknown"]).optional(), |
There was a problem hiding this comment.
🟡 Closed reason enum vs documented open set.
| reason: z.enum(["rate_limit", "timeout", "network", "provider", "unknown"]).optional(), | |
| Parse leniently and narrow locally: reason: z.string().optional() (or z.enum([...]).catch("unknown")) in status.ts, then map unknown values to "unknown" in run.ts scheduleRetry, keeping SessionRetry.Reason as the emitted type. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #115, packages/cli/src/session/status.ts:21):
Problem: Closed reason enum vs documented open set
Detail: EVENTS.md's new retry_scheduled docs tell consumers to treat `reason` as an open set, and the PR's model is additive/forward-compatible fields, but the inbound parser pins a closed z.enum of five values (mirrored in the generated SDK union). A newer server emitting a new reason value fails schema validation in an older attached CLI — one of the explicit deployment shapes this PR supports via the messageID-null older-server path — turning an additive server change into a dropped/failed status parse rather than degradation to "unknown", silently losing retry telemetry.
Suggested fix: Parse leniently and narrow locally: reason: z.string().optional() (or z.enum([...]).catch("unknown")) in status.ts, then map unknown values to "unknown" in run.ts scheduleRetry, keeping SessionRetry.Reason as the emitted type.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
EVENTS.md's new retry_scheduled docs tell consumers to treat reason as an open set, and the PR's model is additive/forward-compatible fields, but the inbound parser pins a closed z.enum of five values (mirrored in the generated SDK union). A newer server emitting a new reason value fails schema validation in an older attached CLI — one of the explicit deployment shapes this PR supports via the messageID-null older-server path — turning an additive server change into a dropped/failed status parse rather than degradation to "unknown", silently losing retry telemetry.
retryID: z.string().optional(),\n messageID: z.string().optional(),\n providerID: z.string().optional(),\n modelID: z.string().optional(),\n reason: z.enum(["rate_limit", "timeout", "network", "provider", "unknown"]).optional(),\n delayMs: z.number().optional(),
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 7 · ⚪ 0 · 0/8 resolved
🤖 Fix all 8 open findings with your agent📋 Out-of-diff findings (8)
Reviewed 14 files · 0 inline · view all 8 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
1 participant
Closes #110
Intent
Headless telemetry needs stable release, retry, and terminal-outcome dimensions so consumers can measure failures and recovery without introducing a second event stream or counting terminal summaries twice.
Expected Impact on Users
NDJSON consumers receive the CLI version on every invocation envelope and correlated retry lifecycle events with bounded categorical reasons, attempt identity, delay, and explicit recovered/failed/aborted/unknown outcomes.
Expected Outcomes
Implementation
retry_scheduled/retry_completeevents.Scope Caveat
This exposes measurement inputs only. It does not add automatic recovery, raw provider diagnostics, dashboards, or product-specific review metrics.
Test Plan
Verification
Risks and Rollout
All new event fields are optional and additive. Consumers should treat unknown/censored outcomes explicitly and avoid using request or message IDs as metric labels.