fix: propagate provider finish errors to headless runs - #113
Merged
Merged
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| } | ||
| if (status && status >= 500 && status < 600) { | ||
| return { reason: "provider", code: String(status), message } | ||
| if (name === "APIError" || (status && status >= 500 && status < 600)) { |
There was a problem hiding this comment.
🟠 APIError catch-all misclassifies 401/429 as "provider".
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #113, packages/cli/src/cli/cmd/run.errors.ts:33-34):
Problem: APIError catch-all misclassifies 401/429 as "provider"
Detail: The new `name === "APIError"` catch-all is status-blind and sits after only the message-regex branches. A thrown APIError whose status lives in data.statusCode but whose message misses the English regexes (Google's 429 "Resource has been exhausted", Anthropic's "rate_limit_error" — no space, 403 "Permission denied") now classifies as reason "provider" instead of "unknown". The new tests only use regex-friendly messages ("Rate limit exceeded", "Invalid API key"), so any reason-keyed retry/backoff, exit-code, or user-hint behavior treats real quota/auth APIErrors as generic provider failures, and data.isRetryable is ignored entirely.
Suggested fix: Before the APIError catch-all, classify by extracted status: 429 (and 408) → rate_limit/timeout, 401/403 → auth; or read err.data.statusCode and data.isRetryable inside the APIError branch instead of relying on message regexes. Add test cases with non-regex messages ("Resource has been exhausted", "rate_limit_error").
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 name === "APIError" catch-all is status-blind and sits after only the message-regex branches. A thrown APIError whose status lives in data.statusCode but whose message misses the English regexes (Google's 429 "Resource has been exhausted", Anthropic's "rate_limit_error" — no space, 403 "Permission denied") now classifies as reason "provider" instead of "unknown". The new tests only use regex-friendly messages ("Rate limit exceeded", "Invalid API key"), so any reason-keyed retry/backoff, exit-code, or user-hint behavior treats real quota/auth APIErrors as generic provider failures, and data.isRetryable is ignored entirely.
if (/heap out of memory|ENOMEM/i.test(message)) {
return { reason: "oom", message }
}
if (name === "APIError" || (status && status >= 500 && status < 600)) {
return { reason: "provider", code: status ? String(status) : undefined, message }
}
return { reason: "unknown", code: status ? String(status) : undefined, message }
}|
|
||
| Errors are attributed to the originating session. A child error alone does not change the primary session's exit status if the primary agent handles it successfully. | ||
|
|
||
| Release regression coverage: from `packages/cli`, run `bun test test/cli/run-provider-finish.test.ts test/cli/run-signal-cancellation.test.ts test/cli/classify-session-error.test.ts`. The provider fixture uses real Gemini SSE responses and the pinned SDK in a headless subprocess, including malformed function calls, content filtering, empty success, tool calls, partial output, and output limits. |
There was a problem hiding this comment.
🟡 Maintainer test instructions embedded in schema doc.
Suggested change
| Release regression coverage: from `packages/cli`, run `bun test test/cli/run-provider-finish.test.ts test/cli/run-signal-cancellation.test.ts test/cli/classify-session-error.test.ts`. The provider fixture uses real Gemini SSE responses and the pinned SDK in a headless subprocess, including malformed function calls, content filtering, empty success, tool calls, partial output, and output limits. | |
| Drop the "Release regression coverage" paragraph from EVENTS.md (keep it in the PR description or a maintainer testing doc), and consider moving the "Terminal reason semantics" table below the full message_complete field list so the `tokens` field definition stays adjacent to `usageStatus`/`finish`. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #113, EVENTS.md:229):
Problem: Maintainer test instructions embedded in schema doc
Detail: EVENTS.md is a schema reference for event consumers, but the new section ends with a "Release regression coverage" paragraph instructing maintainers to run `bun test ...`. That is PR-checklist content, references a signal-cancellation test irrelevant to finish-reason semantics, and will rot as test files are renamed. It also splits the message_complete field enumeration, separating the `tokens` field doc from `usageStatus`/`finish` by ~20 lines.
Suggested fix: Drop the "Release regression coverage" paragraph from EVENTS.md (keep it in the PR description or a maintainer testing doc), and consider moving the "Terminal reason semantics" table below the full message_complete field list so the `tokens` field definition stays adjacent to `usageStatus`/`finish`.
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 is a schema reference for event consumers, but the new section ends with a "Release regression coverage" paragraph instructing maintainers to run bun test .... That is PR-checklist content, references a signal-cancellation test irrelevant to finish-reason semantics, and will rot as test files are renamed. It also splits the message_complete field enumeration, separating the tokens field doc from usageStatus/finish by ~20 lines.
Errors are attributed to the originating session. A child error alone does not change the primary session's exit status if the primary agent handles it successfully.
Release regression coverage: from `packages/cli`, run `bun test test/cli/run-provider-finish.test.ts test/cli/run-signal-cancellation.test.ts test/cli/classify-session-error.test.ts`. The provider fixture uses real Gemini SSE responses and the pinned SDK in a headless subprocess, including malformed function calls, content filtering, empty success, tool calls, partial output, and output limits.
**`tokens`** (5-way breakdown, mirrors upstream `LLM.Usage`):
| cost: usage.cost, | ||
| }) | ||
| await Session.updateMessage(input.assistantMessage) | ||
| if (input.assistantMessage.error) { |
There was a problem hiding this comment.
🟡 Error break may kill interactive sessions.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #113, packages/cli/src/session/processor.ts:284-290):
Problem: Error break may kill interactive sessions
Detail: On a failed finish reason the code publishes Session.Event.Error and unconditionally breaks the loop. In interactive (non-headless) sessions a content-filter turn previously ended the turn and returned the prompt; now the session loop terminates, and a TUI listener for Session.Event.Error may additionally surface the error already persisted on the message (double handling). The thrown-error path goes through the catch block instead, so the two failure lifecycles this change claims to unify may still diverge (the catch path may recover/continue in interactive mode). Only the headless path is tested.
Suggested fix: Verify the interactive Session.Event.Error consumer and the catch block's recovery behavior; if interactive sessions should survive, gate the break on headless/invocation mode (or route through the same catch/failure handler a thrown nonretryable error uses). Add an interactive-mode test.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
On a failed finish reason the code publishes Session.Event.Error and unconditionally breaks the loop. In interactive (non-headless) sessions a content-filter turn previously ended the turn and returned the prompt; now the session loop terminates, and a TUI listener for Session.Event.Error may additionally surface the error already persisted on the message (double handling). The thrown-error path goes through the catch block instead, so the two failure lifecycles this change claims to unify may still diverge (the catch path may recover/continue in interactive mode). Only the headless path is tested.
cost: usage.cost,
})
await Session.updateMessage(input.assistantMessage)
if (input.assistantMessage.error) {
await Bus.publish(Session.Event.Error, {
sessionID: input.sessionID,
error: input.assistantMessage.error,
})
break
}
if (snapshot) {| // Providers can end a successful HTTP stream with a failed | ||
| // model turn. Preserve its parts and usage, but use the same | ||
| // failure lifecycle as a thrown, nonretryable provider error. | ||
| if (value.finishReason === "error" || value.finishReason === "content-filter") { |
There was a problem hiding this comment.
🟡 Finish reasons other than error/content-filter still exit 0.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #113, packages/cli/src/session/processor.ts:258):
Problem: Finish reasons other than error/content-filter still exit 0
Detail: Only "error" and "content-filter" are treated as failures. Gemini emits other failure finishReasons (RECITATION, BLOCKLIST, SPII, OTHER) that may normalize to "other"/"unknown"; those turns are still persisted as success and headless runs exit 0, so #108-style silent failures remain for a subset of blocked/malformed responses. The tests cover only MALFORMED_FUNCTION_CALL and SAFETY mappings.
Suggested fix: Verify how the provider SDK normalizes RECITATION/BLOCKLIST/OTHER; either map them into the failure set, or explicitly document the supported failure reasons in EVENTS.md and log unmapped ones. Extend tests with a RECITATION/OTHER case to pin the intended behavior.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Only "error" and "content-filter" are treated as failures. Gemini emits other failure finishReasons (RECITATION, BLOCKLIST, SPII, OTHER) that may normalize to "other"/"unknown"; those turns are still persisted as success and headless runs exit 0, so #108-style silent failures remain for a subset of blocked/malformed responses. The tests cover only MALFORMED_FUNCTION_CALL and SAFETY mappings.
// Providers can end a successful HTTP stream with a failed
// model turn. Preserve its parts and usage, but use the same
// failure lifecycle as a thrown, nonretryable provider error.
if (value.finishReason === "error" || value.finishReason === "content-filter") {
log.error("provider finish", {
sessionID: input.sessionID,
messageID: input.assistantMessage.id,
finishReason: value.finishReason,
})
input.assistantMessage.error = new MessageV2.APIError({
message:
value.finishReason === "content-filter"
? "The provider blocked the response with a content filter."
: "The provider ended the response with an error finish reason.",
isRetryable: false,
metadata: { finishReason: value.finishReason },
}).toObject()
}
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 3 · ⚪ 0 · 0/4 resolved
🤖 Fix all 4 open findings with your agent📋 Out-of-diff findings (4)
Reviewed 6 files · 0 inline · view all 4 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Contributor
Author
Review response — PR #113Verified all four structured findings against the PR head; clarified documentation and added exact provider regressions without changing runtime behavior. Issues addressed (pushed to this PR)
Review claims verified false (no change needed)
Not addressed here
Validation: 32 focused tests passed, CLI typecheck passed, Prettier and diff checks passed. MCP finding persistence is unavailable: matched 0, written 0, verified 0, failed 0, unrecorded 4. The structured verdicts are retained in this comment. |
| } | ||
| if (status && status >= 500 && status < 600) { | ||
| return { reason: "provider", code: String(status), message } | ||
| if (name === "APIError" || (status && status >= 500 && status < 600)) { |
There was a problem hiding this comment.
🟡 APIError catch-all folds statusful 4xx into "provider".
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #113, packages/cli/src/cli/cmd/run.errors.ts:33-35):
Problem: APIError catch-all folds statusful 4xx into "provider"
Detail: Adding `name === "APIError"` to the provider branch makes every APIError classify as reason "provider", including status-bearing non-5xx HTTP failures (400 invalid request, 402, 404) that previously fell through to reason "unknown". 401/403/429 are still classified correctly by the earlier status checks, but the catch-all is broader than the PR's stated need (classifying status-less persisted finish-reason errors) and changes session_error.reason telemetry semantics for pre-existing thrown 4xx provider errors consumed by CI dashboards.
Suggested fix: Tighten the condition so only status-less APIErrors (the persisted finish-reason errors) get the provider catch-all, e.g. `if ((name === "APIError" && !status) || (status && status >= 500 && status < 600))`, or key on `data?.metadata?.finishReason` instead of the error name.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Adding name === "APIError" to the provider branch makes every APIError classify as reason "provider", including status-bearing non-5xx HTTP failures (400 invalid request, 402, 404) that previously fell through to reason "unknown". 401/403/429 are still classified correctly by the earlier status checks, but the catch-all is broader than the PR's stated need (classifying status-less persisted finish-reason errors) and changes session_error.reason telemetry semantics for pre-existing thrown 4xx provider errors consumed by CI dashboards.
if (name === \"APIError\" || (status && status >= 500 && status < 600)) {\n return { reason: \"provider\", code: status ? String(status) : undefined, message }\n }\n return { reason: \"unknown\", code: status ? String(status) : undefined, message }\n}| // model turn. Preserve its parts and usage, but use the same | ||
| // failure lifecycle as a thrown, nonretryable provider error. | ||
| if (value.finishReason === "error" || value.finishReason === "content-filter") { | ||
| log.error("provider finish", { |
There was a problem hiding this comment.
🟡 "other" finish abnormal terminations still exit 0.
Suggested change
| log.error("provider finish", { | |
| Verify the pinned @ai-sdk/google maps all Gemini failure finishReasons (PROHIBITED_CONTENT, LANGUAGE, FINISH_REASON_UNSPECIFIED) to "content-filter"/"error", and consider emitting a non-fatal warning event (or failing when the turn has no output) for "other" finishes so CI can distinguish abnormal termination from STOP. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #113, packages/cli/src/session/processor.ts:259-273):
Problem: "other" finish abnormal terminations still exit 0
Detail: Only "error" and "content-filter" convert to a failed turn; a stream that terminates abnormally with a normalized "other" (Gemini OTHER, and unverified Gemini safety reasons like PROHIBITED_CONTENT/LANGUAGE depending on SDK mapping) still persists status "completed" and headless runs exit 0 — the same silent-CI-failure class issue #108 targets. The new test enshrines OTHER → exit 0, and EVENTS.md documents "other nonempty finish" as non-failure, so this is a deliberate policy — but the residual gap means some blocked/malformed responses remain indistinguishable from success in CI. Worth either verifying every Gemini failure finishReason maps to content-filter/error in the pinned SDK, or surfacing "other" terminations via a non-fatal warning event / failure when the turn produced no output.
Suggested fix: Verify the pinned @ai-sdk/google maps all Gemini failure finishReasons (PROHIBITED_CONTENT, LANGUAGE, FINISH_REASON_UNSPECIFIED) to "content-filter"/"error", and consider emitting a non-fatal warning event (or failing when the turn has no output) for "other" finishes so CI can distinguish abnormal termination from STOP.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Only "error" and "content-filter" convert to a failed turn; a stream that terminates abnormally with a normalized "other" (Gemini OTHER, and unverified Gemini safety reasons like PROHIBITED_CONTENT/LANGUAGE depending on SDK mapping) still persists status "completed" and headless runs exit 0 — the same silent-CI-failure class issue #108 targets. The new test enshrines OTHER → exit 0, and EVENTS.md documents "other nonempty finish" as non-failure, so this is a deliberate policy — but the residual gap means some blocked/malformed responses remain indistinguishable from success in CI. Worth either verifying every Gemini failure finishReason maps to content-filter/error in the pinned SDK, or surfacing "other" terminations via a non-fatal warning event / failure when the turn produced no output.
// Providers can end a successful HTTP stream with a failed\n // model turn. Preserve its parts and usage, but use the same\n // failure lifecycle as a thrown, nonretryable provider error.\n if (value.finishReason === "error" || value.finishReason === "content-filter") {\n log.error("provider finish", {\n sessionID: input.sessionID,\n messageID: input.assistantMessage.id,\n finishReason: value.finishReason,\n })| await Session.updateMessage(input.assistantMessage) | ||
| if (input.assistantMessage.error) { | ||
| await Bus.publish(Session.Event.Error, { | ||
| sessionID: input.sessionID, |
There was a problem hiding this comment.
🟡 Session.Event.Error publish awaited inconsistently.
Suggested change
| sessionID: input.sessionID, | |
| Await Bus.publish(Session.Event.Error, ...) at the max-retry branch (~L396) and the terminal catch branch (~L413) to match the new finish-step pattern, or extract a small awaited publishSessionError() helper used by all three sites. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #113, packages/cli/src/session/processor.ts:286-289):
Problem: Session.Event.Error publish awaited inconsistently
Detail: The new finish-step failure path awaits Bus.publish(Session.Event.Error) (processor.ts:286), but the two sibling sites publishing the identical event in the same function remain fire-and-forget: the max-retry branch (~L396) and the terminal catch branch (~L413) call Bus.publish without await. Since this PR's stated goal is consistent failure events and headless consumers depend on session_error flushing before idle/exit, the same event should be published the same way at all three sites; un-awaited publishes rely on incidental later awaits to flush.
Suggested fix: Await Bus.publish(Session.Event.Error, ...) at the max-retry branch (~L396) and the terminal catch branch (~L413) to match the new finish-step pattern, or extract a small awaited publishSessionError() helper used by all three sites.
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 finish-step failure path awaits Bus.publish(Session.Event.Error) (processor.ts:286), but the two sibling sites publishing the identical event in the same function remain fire-and-forget: the max-retry branch (~L396) and the terminal catch branch (~L413) call Bus.publish without await. Since this PR's stated goal is consistent failure events and headless consumers depend on session_error flushing before idle/exit, the same event should be published the same way at all three sites; un-awaited publishes rely on incidental later awaits to flush.
await Session.updateMessage(input.assistantMessage)\n if (input.assistantMessage.error) {\n await Bus.publish(Session.Event.Error, {\n sessionID: input.sessionID,\n error: input.assistantMessage.error,\n })\n break\n }\n if (snapshot) {| [401, "Unauthenticated", "auth"], | ||
| [403, "Permission denied", "auth"], | ||
| [undefined, "Stream timeout", "timeout"], | ||
| ] as const)("APIError preserves specific classifications (%s, %s)", (statusCode, message, reason) => { |
There was a problem hiding this comment.
⚪ test.each title omits expected reason placeholder.
--- a/packages/cli/test/cli/classify-session-error.test.ts
+++ b/packages/cli/test/cli/classify-session-error.test.ts
@@ -70,7 +70,7 @@
[403, "Permission denied", "auth"],
[undefined, "Stream timeout", "timeout"],
- ] as const)("APIError preserves specific classifications (%s, %s)", (statusCode, message, reason) => {
+ ] as const)("APIError preserves specific classifications (%s, %s) → %s", (statusCode, message, reason) => {
const res = classifySessionError({ name: "APIError", data: { statusCode, message, isRetryable: false } })
expect(res.reason).toBe(reason)
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #113, packages/cli/test/cli/classify-session-error.test.ts:73):
Problem: test.each title omits expected reason placeholder
Detail: The test.each title "APIError preserves specific classifications (%s, %s)" interpolates only statusCode and message; the third parameter — the expected reason, the most important value — never appears in generated test names or failure output (e.g. a failure renders as "(undefined, Stream timeout)" without "timeout").
Suggested fix: Use "APIError preserves specific classifications (%s, %s) → %s" so the expected reason appears in each generated test name.
Suggested patch:
--- a/packages/cli/test/cli/classify-session-error.test.ts
+++ b/packages/cli/test/cli/classify-session-error.test.ts
@@ -70,7 +70,7 @@
[403, "Permission denied", "auth"],
[undefined, "Stream timeout", "timeout"],
- ] as const)("APIError preserves specific classifications (%s, %s)", (statusCode, message, reason) => {
+ ] as const)("APIError preserves specific classifications (%s, %s) → %s", (statusCode, message, reason) => {
const res = classifySessionError({ name: "APIError", data: { statusCode, message, isRetryable: false } })
expect(res.reason).toBe(reason)
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 test.each title "APIError preserves specific classifications (%s, %s)" interpolates only statusCode and message; the third parameter — the expected reason, the most important value — never appears in generated test names or failure output (e.g. a failure renders as "(undefined, Stream timeout)" without "timeout").
test.each([\n [undefined, \"The provider ended the response with an error finish reason.\", \"provider\"],\n [429, \"Rate limit exceeded\", \"rate_limit\"],\n [429, \"Resource has been exhausted\", \"rate_limit\"],\n [429, \"rate_limit_error\", \"rate_limit\"],\n [401, \"Invalid API key\", \"auth\"],\n [401, \"Unauthenticated\", \"auth\"],\n [403, \"Permission denied\", \"auth\"],\n [undefined, \"Stream timeout\", \"timeout\"],\n ] as const)(\"APIError preserves specific classifications (%s, %s)\", (statusCode, message, reason) => {| content: { | ||
| role: "model", | ||
| parts: [ | ||
| { functionCall: { name: "read", args: { filePath: path.join(tmp.path, "aictrl.json") } } }, |
There was a problem hiding this comment.
⚪ fetch handler references tmp before declaration.
Suggested change
| { functionCall: { name: "read", args: { filePath: path.join(tmp.path, "aictrl.json") } } }, | |
| Move the `await using tmp = await tmpdir({...})` setup above the Bun.serve call so the fetch handler only references already-initialized bindings. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #113, packages/cli/test/cli/run-provider-finish.test.ts:37):
Problem: fetch handler references tmp before declaration
Detail: The Bun.serve fetch handler references `tmp.path` inside the tool-call branch, but `await using tmp = await tmpdir(...)` is declared ~34 lines later, after the server is already listening. It only works because no request can arrive before tmp initializes (random port, CLI spawned afterwards), but the closure-over-later-const is a fragile use-before-declare pattern — any request arriving in the setup window would throw a TDZ ReferenceError inside the handler.
Suggested fix: Move the `await using tmp = await tmpdir({...})` setup above the Bun.serve call so the fetch handler only references already-initialized bindings.
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 Bun.serve fetch handler references tmp.path inside the tool-call branch, but await using tmp = await tmpdir(...) is declared ~34 lines later, after the server is already listening. It only works because no request can arrive before tmp initializes (random port, CLI spawned afterwards), but the closure-over-later-const is a fragile use-before-declare pattern — any request arriving in the setup window would throw a TDZ ReferenceError inside the handler.
const server = Bun.serve({\n port: 0,\n fetch(): Response {\n calls++\n const chunks =\n tool && calls === 1\n ? [\n {\n candidates: [\n {\n index: 0,\n content: {\n role: \"model\",\n parts: [\n { functionCall: { name: \"read\", args: { filePath: path.join(tmp.path, \"aictrl.json\") } } },
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 3 · ⚪ 2 · 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 |
Contributor
Author
Review response — PR #113Verified the five findings from the latest review and pushed the scoped fixes in Issues addressed (pushed to this PR)
All four fixes are in commit Review claims verified false (no change needed)None in this review round. Not addressed here
Validation: 71 tests passed, CLI typecheck passed, Prettier and diff checks passed. Running the new classification and publication regressions against the previous runtime produced 5 expected failures; the fixes pass all five. Persistence is unavailable without the MCP connection: matched 0, written 0, verified 0, failed 0, unrecorded 5. The verdicts are preserved below. |
ReviewVerified the fix end-to-end: read the processor/run/classification changes, traced the headless event lifecycle, ran the full Correctness — confirmed:
Minor, non-blocking:
No bugs, security issues, or reliability regressions found. LGTM. Reviewed SHA: 00f0cd3 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #108
Intent
A provider can finish a streamed turn with a normalized
errororcontent-filterreason without throwing an exception. Previously that path could emit no session failure and exit headless mode with status 0.Expected Impact on Users
Headless consumers receive a non-zero exit and structured session error for unsuccessful provider turns. Successful empty
stopturns and existing cancellation/idle-timeout behavior remain distinct.Expected Outcomes
Implementation
errorandcontent-filterfinishes into the existing non-retryable API error lifecycle, retaining the finish reason in metadata.Scope Caveat
This does not add automatic retries or capture raw provider response diagnostics; those are separate proposals in #109 and #111.
Test Plan
Verification
Risks and Rollout
The change uses the existing error/event contract and has no migration or feature flag. Roll back the commit if a provider-specific finish reason is found to be incorrectly classified.