fix(agent): recover zero-token empty responses - #4174
Conversation
Review verdict: MERGE_READY
Reviewed exactly head Protocol / classifier — safe
Retry / fallback lifecycle — bounded and replay-safeBare-default retry (single model, no legacy Transaction / usage accounting — no duplicatesManaged: the loop discards the transaction and splices the provisional message from context; the session receives Typed surfaces / telemetry
Verification (all run locally on the exact head)
Non-blocking observations
|
probepark
left a comment
There was a problem hiding this comment.
Thanks for this, and thanks especially for being straight in the PR body that bun check was unchecked and that the architect/critic passes did not complete — that made it obvious where to aim. BLOCK, on one issue: the trigger is too broad and reclassifies a case that dev handles correctly today.
The predicate cannot tell a transport fault from a normal empty response
packages/agent/src/agent-loop.ts adds:
function isZeroUsageEmptyStop(message: AssistantMessage): boolean {
... message.stopReason === "stop" && message.usage.totalTokens === 0
}
// then
message.stopReason = "error";
message.errorMessage = "Provider returned an empty response with zero token usage";
providerCode: EMPTY_RESPONSE_PROVIDER_CODE,stopReason === "stop" plus totalTokens === 0 is the only evidence. That set contains more than transport faults:
- Proxy/LiteLLM-style context overflow currently classifies as overflow and reaches promotion, then compaction (
agent-session.ts:~13358-13370). Under this PR it becomesempty_responseand enters retry/fallback policy instead. - Unrecognized silent refusals land in the same bucket.
The consequence is a deterministic input being replayed: a bare session makes up to four total requests by default, and managed chains allow three attempts per model before advancing. At exhaustion the bare session surfaces an error and the managed chain terminalizes — and in the overflow case the user has now lost the promotion/compaction path that would actually have fixed their turn.
So the regression is not just wasted requests; it is a working recovery being replaced by a failing one.
To be clear on what is safe: recognized Anthropic/OpenAI/Google safety stops are not affected, because those adapters emit provider_safety_stop. The gap is specifically the normal-stop empty responses that are not transport faults.
Required change: classify overflow before synthesizing empty_response, and only synthesize the transport failure from provider-specific evidence that rules out overflow and refusal. Please add a session-level regression proving a zero-token proxy overflow still invokes promotion/compaction rather than retry policy — that is the case that silently regresses.
Verification (rebased onto origin/dev 7080dace, commit a6fb60c8)
| run | result |
|---|---|
| PR-focused suites | 123 pass / 0 fail |
| neighbouring retry/safety suites | 19 pass / 0 fail |
| mutation | production present 123/0 |
Rebases cleanly. The tests you wrote do pass — the problem is the case they do not cover.
Interaction with #4169 — no conflict, but a note
I asked specifically about this because you and I are both editing #handleRetryableError. git merge-tree shows no textual conflict; the hunks are disjoint, and there is no semantic revert — #4169's auto_retry_start rejection guard still wraps this new path once both land.
One sequencing caveat: until #4169 lands, this PR adds another way to reach the existing dead-turn bug (a rejected auto_retry_start delivery leaves #retryPromise unresolved and the prompt gate held). Not your defect, and not a reason to change this PR — just worth landing #4169 first.
Fallback ordering
The fallback-transport.ts change leaves ordering and eligibility intact for other failure codes. Only the zero-token case changes classification, from context maintenance to "server" fallback, retrying the current model up to its per-entry budget before advancing. That is contained.
CHANGELOG
Currently inaccurate: it presents all zero-token empty responses as transport failures, and omits that overflow promotion/compaction no longer runs for them. Please reword once the classification is narrowed.
Also
bun check is still unchecked in the PR body. Worth running before the next push — I did not treat its absence as a finding, but it should be green before merge.
Narrow the trigger and add the overflow regression, and I think this is a good fix — the underlying symptom is real and worth handling.
Some provider streams can end with stop and no content or token usage. Treat that impossible-success shape as a typed transient failure so clean bare sessions retry within budget and managed chains discard it before fallback. Constraint: only content-empty stop responses with every usage counter at zero are reclassified Rejected: provider-specific Grok handling | the failure shape is provider-agnostic Confidence: high Scope-risk: narrow Reversibility: easy Tested: 123 focused tests; AI, agent, and coding-agent typechecks
Generic zero-token empty stops also represent proxy context overflow, where promotion or compaction is the correct recovery. Stamp empty_response only at the OpenCode Go Responses completion boundary when a completed response id has no output items or usage, and let untyped empties retain overflow precedence. Constraint: preserve generic proxy overflow promotion and compaction Rejected: generic zero-usage classifier | conflates transport anomalies with overflow and refusal Confidence: high Scope-risk: narrow Reversibility: easy Tested: 145 focused tests; AI, agent, and coding-agent typechecks
80ff2c3 to
bb6b7a7
Compare
|
Requested-change repair is pushed at
Verification: 145 focused tests pass; AI/agent/coding-agent typechecks pass; focused Biome passes. Exact diff SHA-256: |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Re-review verdict: MERGE_READY
gajae.pr-review-verdict.v1 needs-human sha256:925572b7a242efb4d9ea0486804ba3161e21beed798cb17b107f88cc3fdf4d9e reviewer:human verdict:MERGE_READY head:bb6b7a7c0a4eee0f1cd56b0d4e97e28191abfc75 evidence:focused-tests-145-pass,types-3pkgs-ai-agent-codingagent,ci-24green,prior-blocker-closed
Adversarial re-review of the exact repaired head bb6b7a7c0a4eee0f1cd56b0d4e97e28191abfc75 (base 7858b0ff63db6a3fc0cce93639a8d329ca474b2c). Review-only: no source mutation, push, merge, or release performed.
Prior blocker (probepark REQUEST_CHANGES) — verified closed at exact head
The finding was that the broad isZeroUsageEmptyStop classifier (stopReason:"stop" + totalTokens===0) reclassified proxy/LiteLLM context overflows and unrecognized silent refusals that dev handles correctly via overflow -> promotion/compaction. The repair addresses all three required changes:
- Overflow classified before empty-response synthesis.
agent-loop.tsnow computesmanagedContextOverflowfirst and converts tostopReason:"error"only under!overflow— overflow keeps precedence. - Transport failure synthesized only from provider-specific evidence. The
empty_responsetag is stamped solely by the OpenCode Go Responses adapter (isOpenCodeGoEmptyCompletedResponse): provideropencode-go, non-emptyresponseId,stopReason:"stop", empty content,nativeOutputItemCount===0, and every usage counter===0.agent-loop'sisTypedEmptyResponseStopadditionally requires theempty_responseprovider code, so no other adapter or untyped stop is reclassified. - Session regression proves untyped proxy overflow still promotes.
agent-session-context-promotion.test.ts->keeps an untyped zero-token proxy empty stop on the promotion pathasserts the model promotes to the larger-context model with zeroauto_retry_startevents. - CHANGELOG reworded to describe the narrowed contract (typed OpenCode Go retryable failure; untyped proxy empties retain overflow promotion/compaction).
Boundary attacks on the repaired trigger
- Provider/proxy/LiteLLM / context-overflow: only
opencode-gostampsempty_response; untyped empty stops (proxy/LiteLLM) retainclassifyContextOverflow's silent-overflow path -> promotion/compaction.empty_responsesits inNON_OVERFLOW_PROVIDER_CODES, so tagged messages are not misclassified as overflow. The zero-usage evidence rules out genuine context overflow (a zero-token request cannot overflow). - Legitimate empty-stop semantics (other adapters):
isTypedEmptyResponseStoprequires theempty_responseprovider code, which only the opencode-go adapter emits in production. Anthropic/OpenAI/Google and all other adapters are untouched — the applicable adapter header/affinity/overflow logic is byte-identical base->head. - Bounded retry loops:
empty_responseis not in thelegacyUnbounded(transient-only) class;attemptsUsed = #retryAttempt + 1 <= retry.maxRetriesbounds it (agent-session-resilient-retry.test.ts: capped at maxRetries,unbounded:false; no retry after visible progress or whenretry.enabled=false). - Fallback transaction: managed fallback discards the provisional empty attempt and advances the chain;
agent-session-fallback-attempt-transaction.test.tsasserts exactly 2 model calls, 1message_start, one accepted assistant message. - Content/scope guard: non-managed empty-response retry requires content-free message + clean current scope; visible/tool content or missing/dirty scope vetoes the retry.
- Reserved tool interactions: orthogonal — history neutralization runs before the empty-response conversion; the added test is a unit test of
isOpenCodeGoEmptyCompletedResponsealongside the existing reserved-tool suite. No interference. - No behavior change for other adapters: confirmed — the changed classification paths only fire for messages carrying the
empty_responseprovider code.
Verification (exact head)
- Focused suites:
agent-loop(38),model-fallback-transport-facts+openai-responses-reserved-tool-names(27),agent-session-context-promotion(10),agent-session-fallback-attempt-transaction(12),agent-session-resilient-retry(58) = 145 pass / 0 fail. - Full
packages/aiandpackages/agentsuites green except pre-existing environment artifacts (ambientANTHROPIC_BASE_URL/OPENAI_BASE_URLoverrides and network-dependent cache-affinity tests) whose exercised code is byte-identical to base — not caused by this PR. check:types+ Biome pass for packages/ai, packages/agent, packages/coding-agent.- Exact-head CI: 24 completed success / 0 failed (all affected-path checks pass; Virtual integration validation completed success).
Non-blocking observations
- OpenCode Go zero-token empty stops now shift from the generic overflow->promotion heuristic to the typed retry path. This is contained (bounded, content-free, clean-scope gated) and defensible given the zero-usage evidence; flagged for maintainers in case OpenCode Go ever swallows a genuine overflow into a zero-token empty.
- No dedicated test exercises the dirty-scope branch of the empty-response gate in isolation (shared with the first-event-timeout machinery, which has coverage).
The underlying symptom is real, the repair is narrow and correct, and the prior blocker is closed at the exact head. Approving.
probepark
left a comment
There was a problem hiding this comment.
Reviewed the stale-review delta through head bb6b7a7c0a4eee0f1cd56b0d4e97e28191abfc75. The provider-specific narrowing fixes the original broad-classifier objection, but the retry path still has three replay/event-boundary defects.
Major — the typed failure is created after message_end has already escaped
packages/agent/src/agent-loop.ts:1772-1776 changes the completed message from stop to error, but streamAssistantResponse() already emitted that same response as message_end at packages/agent/src/agent-loop.ts:2436-2450. AgentSession synchronously persists the assistant message before its first await (packages/coding-agent/src/session/agent-session.ts:4177-4210) and then emits it to TUI/extensions. Thus the observable and durable boundary reports a successful empty turn before the later mutation/retry. Extensions can act on the false success, and a crash/reload in that window retains the wrong terminal state.
Move the classification to the provider/final-response boundary before the done event is emitted, or otherwise ensure message_end is born with stopReason: "error" and the canonical error text. The new resilient-retry tests do not cover this boundary: their helper constructs an already-error terminal event (packages/coding-agent/test/agent-session-resilient-retry.test.ts:150-175). Add an assertion against the actual emitted/persisted message_end from a typed zero-token stop stream.
Major — managed fallback ignores dirty or missing attempt scopes
At packages/coding-agent/src/session/agent-session.ts:15972-15978, the empty-response clean-scope gate is restricted to !managedFallback. Managed outcomes pass their scope and cleanliness into this method (:15728-15734), but an empty response proceeds even when a context/request extension already made the attempt scope dirty. The fallback then replays on another model and can duplicate extension-observable side effects. The analogous first-event-timeout path correctly requires a clean scope for managed fallback at :15979-15985.
Apply the content/scope cleanliness gate to managed empty-response outcomes too, returning the managed terminal decision when replay is unsafe. Add a managed-chain regression with a participating context handler.
Major — later empty-response retries bypass accumulated replay-safety state
packages/coding-agent/src/session/agent-session.ts:16028-16029 makes every typed empty response bypass the bare-default replay-safety block. After the first retry, an auto_retry_start extension handler marks #hasCleanRetryReplaySafety false (:6116-6118), but the second empty response still enters another retry because canReplayEmptyResponse remains true. That reruns lifecycle side effects through the full retry budget. The existing watchdog regression at packages/coding-agent/test/agent-session-resilient-retry.test.ts:1681-1724 demonstrates the intended contract—one retry, then stop once an auto_retry_start handler participates—but the new empty-response tests do not exercise it.
Gate subsequent empty-response retries on the accumulated replay-safety state, matching the typed first-event-timeout behavior, and add the corresponding extension-handler regression.
Verification: the two isolated AI suites passed locally (27 tests, 105 assertions). The agent/coding-agent suites could not load because this review worktree resolves stale/missing workspace links (@gajae-code/ai/core, @gajae-code/utils/shell-config, and a stale external @gajae-code/ai export); I did not mutate the checkout to repair dependencies.
gajae.pr-review-verdict.v1: needs-human
|
Superseded by #4203 from the owner repair branch because this lane cannot safely push to contributor-owned #4203 preserves chulmin-dev's exact reviewed head |
Supersedes #4174 while retaining chulmin-dev contributor commits and repairs lifecycle/replay-safety boundaries.
What
Treat one provider-specific impossible-success shape as a typed
empty_responsefailure:opencode-goon the Responses API;The OpenCode Go adapter stamps the typed fact at the completion boundary. Existing policy then:
retry.maxRetries;Generic untyped zero-token empty stops retain existing context-overflow precedence and therefore continue through promotion/compaction rather than retry policy.
Why
OpenCode Go Grok 4.5 returned completed Responses objects with an id but no output items or usage during a critic run on GJC 0.12.21. GJC accepted those turns as success, allowing a load-bearing reviewer lane to stop without a verdict.
This is separate from the reserved
web_searchcollision fixed by #4106.The first revision classified every zero-token empty stop as
empty_response. Maintainer review correctly identified that proxy/LiteLLM context overflows share that shape. The repaired revision moves evidence to the OpenCode Go adapter, gives generic overflow classification precedence, and adds a session regression proving untyped proxy empties still promote instead of retrying.Testing
bun --cwd=packages/ai run check:types— passbun --cwd=packages/agent run check:types— passbun --cwd=packages/coding-agent run check:types— passGJC verdict
The prior human review was against the superseded head. This repaired exact head remains
needs-humanpending maintainer re-review.devbun checkpasses