fix(ws): stop replaying a turn after output was emitted

[iceteaSA]

Fixes #88.

The rate-limit path already refused to retry once output had reached the consumer — its comment names the harm exactly: "side-effecting tools, and double-bill — so end the turn WITHOUT a retry." Five other failure paths never consulted emitted, so a failure arriving after partial output could still surface a retryable marker and replay the turn: duplicated text, tool calls run twice, the turn billed twice.

Branched from 9bf8f4c, independent of #87.

The rule

ResponseStreamError extends APICallError with isRetryable: true, and OpenCode's retry loop converts retryable APICallErrors into SessionRetry attempts. So the gate is: before output a failure may surface a retryable marker and reroute; after output it must fail visibly and non-retryably.

invalidateTransport implements the split, and fail(error, connectionError) separates the two channels — the consumer gets the gated error, the pool still gets a ResponseStreamError for its own bookkeeping.

Sites gated

site	before	after
idle timeout	retryable	gated
socket error	retryable	gated
early close	retryable	gated
unexpected binary frame	retryable	gated
throwing onRetryableTerminal callback	retryable	gated
wrapped provider error (408/409/429/500/503)	retryable	plain Error, APICallError as cause

The callback path mattered most in practice: the connection-limit callback is a throwing path, so it was reachable in normal operation.

Post-output failures deliberately still count toward streamRetries — the socket genuinely failed, and a successful terminal response resets the counter.

Deliberately NOT gated

Eleven sites were enumerated; six stay ungated on purpose:

• Pre-output admission and rate-limit failures — these should reroute.
• Post-output rate limits — already handled by the existing closeCompleted() policy.
• Synchronous socket.send failure — production callbacks never return a replacement socket, so this send always precedes output.
• Abort — already plain and non-retryable.
• Ordinary terminal frames and cancellation — expose no retry marker.

Why not closeCompleted()

The obvious fix is wrong. closeCompleted() enqueues data: [DONE] and closes normally, so a turn that died halfway would look complete — trading duplication for silent truncation, which is harder to detect.

Tests

1071 pass / 0 fail (1064 on 9bf8f4c), tsc clean. ws.ts moves 48 lines.

Reviewed independently by a different model family, which found two of the five sites we had missed — the binary-frame and throwing-callback paths, plus the wrapped-error bypass — by enumerating every consumer-visible error construction rather than reviewing only the sites we named. Those were proven with live probes before being fixed.

Every new test was mutation-checked behaviourally (keep the API, flip only the gate); each fails with a concrete assertion that a retryable marker was received, and no red was a TypeError or a hang. Pre-output rerouting was separately verified as intact — a pre-output wrapped 503 still yields APICallError / isRetryable: true, and the inverse mutation fails.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Stop replaying a WebSocket turn after genuine output (text/tool/function) reaches the client to avoid duplicated text, tool re-runs, and double billing. Previously, post-output transport/wrapped provider errors could surface as retryable and trigger a replay; now they fail visibly and non-retryably.

• Gate retryability on emitted output via a new emittedOutput flag and invalidateTransport; lifecycle frames (response.created/response.in_progress) do not count as output.
• Apply the gate to idle timeout, socket error, early close, unexpected binary frame, thrown onRetryableTerminal, and wrapped provider errors (408/409/429/500/503). Before output (including after lifecycle-only frames), failures remain retryable; after output, the consumer sees a plain Error while the pool records a ResponseStreamError.
• Rate limits: admission-time and pre-output (including post-lifecycle) remain retryable and call onRateLimitReached; after output, they end the turn without retry. APICallError is produced only if no output was emitted (after output it is provided as the cause of a plain Error).

^{Written for commit 4fb45d6. Summary will update on new commits.}

[cubic-dev-ai]

All reported issues were addressed across 2 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[iceteaSA]

Updated to 192c003 (force-push; previous head cea3215 preserved on our side).

Cubic caught that the gate was too COARSE, and chasing it found two more sites than it named.

emitted flips on the first enqueued frame — normally response.created — so the entire window between a response starting and its first real token was treated as "output already delivered". Every failure in that window became non-retryable, killing reroutes that were completely safe. That is where connection failures cluster, so it was a real capability loss, and it was our own over-correction.

A separate emittedOutput flag now drives replay decisions. emitted keeps its original meaning for the three consumers that want it.

Chasing that turned up the same defect in the two rate-limit paths: admission classification and the mid-stream split. Proven by probe — response.created → response.failed(rate_limit) closed normally instead of rejecting retryably, and response.created → 429 stayed retryable but never called onRateLimitReached, so a retry could select the same exhausted account. A reroute that silently fails to mark the account is worse than no reroute.

That is three rounds of the same defect class on this branch: a retry decision keyed on the wrong signal. So we stopped patching sites and audited all 10 that decide retry, reroute, replay, or mark. Two changed, eight verified correct, no further instances.

Classification was validated against the pool rather than by frame name: response.created and response.in_progress commit no continuation state (updateContinuation runs only from onComplete, finalized-call recording only on response.output_item.done, onTerminal uninvoked). Everything ambiguous — output-item, content-part, unknown, terminal — is classified conservatively as output, because a wrong call that way costs a reroute while the reverse duplicates output and re-runs tools.

Gate: 1074 pass / 0 fail (1064 on 9bf8f4c), tsc clean. Re-reviewed after each round; final APPROVE 0 must / 0 should, with every site mutated back independently and each reddening only its own test behaviourally.

[ualtinok]

Verified the bug against main: onError and onClose at ws.ts:540-556 call invalidate(new ResponseStreamError(...)) with no reference to the emission flag, while the rate-limit path deliberately checks it. So a socket drop after partial output produced a retryable error, OpenCode replayed the turn, and any tool call already dispatched ran twice. That is the no-replay invariant this plugin has held since 0.3.3, applied to one failure path and not the others.

Your two-flag split is the right shape. emitted keeps meaning "anything reached the consumer" for the idle/first-event bookkeeping, while emittedOutput gates replay and excludes response.created and response.in_progress. Separating them is what lets the gate be conservative without freezing the early-reroute window shut.

On the open cubic P2

It argues the gate should key on user-visible text rather than any non-control frame, since a drop after response.output_item.done currently blocks a reroute that would have been safe.

I disagree with the direction, and the reason matters more than the verdict: user-visible text is not the hazard the invariant exists to prevent. Duplicate text is the cosmetic half. The expensive half is a tool call that already dispatched — re-running a side-effecting tool, and paying for it twice. response.output_item.done carrying a function_call is precisely the frame after which replay is unsafe, and it produces no text at all. Gating on text would reopen the case the gate was built for.

The residual cost you are accepting is real and worth stating plainly in the code: a transport failure in the window after the first output item but before anything the user would notice now ends the turn instead of rerouting. That is the conservative direction — it loses a reroute, it never double-charges. Given the choice between "occasionally fail to reroute" and "occasionally re-run a side-effecting tool", the first is obviously right, but a future reader will hit that comment and wonder, so let the comment answer them.

If you want the reroute window back without weakening the guarantee, the discriminator is dispatch rather than visibility: a frame that OpenCode cannot have acted on yet is safe to replay. That needs checking against how OpenCode's parser dispatches tool calls, and is a bigger change than this PR should carry — worth an issue rather than a revision here.

Gate is green on my machine: 1074 pass / 0 fail, typecheck clean.

Blocking only on the comment. Add the note about what the conservative gate gives up and I will merge this.

[iceteaSA]

Comment added — 4fb45d6. 1074 pass / 0 fail, typecheck clean.

Agreed on the cubic P2, and your reason is better than the one I would have given. I had been justifying the conservative classification by risk asymmetry — over-gating costs a reroute, under-gating costs a double charge — which is true but leaves the impression we were being cautious about a fuzzy boundary. Your framing is sharper: the boundary is not fuzzy at all. response.output_item.done carrying a function_call emits no text and is precisely the frame after which replay is unsafe, so text-gating does not trade safety for reroutes — it misses the expensive case outright. That is the version worth having in the file, so it is the version I wrote:

// Gated on generated output rather than user-visible text: a
// `response.output_item.done` carrying a function_call produces no text at
// all, yet it is exactly the point after which a replay would re-run a
// side-effecting tool and bill for it twice. Duplicate text is the cheap
// half of the hazard; a re-dispatched tool call is the expensive one.
//
// What that conservatism costs: a transport failure in the window after the
// first output item but before anything the user would notice now ends the
// turn instead of rerouting. We give up a reroute rather than risk a double
// charge. Narrowing this further needs a dispatch-based discriminator (has
// OpenCode acted on the frame yet?), not a visibility-based one.

The last line points at your dispatch-vs-visibility discriminator so a future reader finds the real answer rather than re-deriving the text-gating idea and reopening the case. Happy to file that as an issue — it needs checking against how OpenCode's parser dispatches tool calls, which is outside this PR.

On calibration: your read matches ours. These came out of an automated sweep, and we filed only the three we had verified by hand out of 44 raw findings — the aggregate severity ordering was not trustworthy (one HIGH turned out to require an operator to configure fallback on a success status, i.e. misconfiguration rather than a defect). Treating them as claims to test rather than a queue to burn down is exactly right, and it is why the other ~38 are sitting unfiled until someone verifies them.

[ualtinok]

Verified at 4fb45d63 — comment-only delta, 11 added lines, no production logic touched. 1074 pass / 0 fail, typecheck clean.

The comment says the right thing, and the closing line pointing at a dispatch-based discriminator is the part that earns its place: it stops the next reader re-deriving text-gating and reopening the case. Please do file that issue.

Approved. Merging.

One thing I hit while gating this that is worth reporting separately, because it is not yours and it is not a regression:

bun test src/tests/integration.test.ts -t "file-exhausted fallback with an empty process quota cache"
(fail) ... [554ms]

That test fails when run alone under a -t filter, and passes when the file or the full suite runs. It does so on main too, at 9bf8f4c, so it predates this PR. It is order-dependent — it needs state a sibling test establishes first.

Not a production defect, but it is the shape that hides one: a test that only passes with its siblings cannot be trusted to gate the behaviour it names, and anyone bisecting with -t gets a false positive. This repository has shipped tests that looked like coverage and gated nothing before, which is why you mutation-test — same family. I will file it rather than hang it on this PR.

On calibration: filing 3 verified out of 44 raw findings is the right ratio to have applied, and the HIGH-that-needed-a-misconfiguration is a good example of why the severity number cannot be read as a queue position. Worth keeping the other ~38 unfiled until someone tests them — an unverified finding costs a reviewer more than it saves.

[iceteaSA]

Filed as #105, with the text-gating rejection written up as a constraint rather than a footnote — so whoever picks it up cannot re-derive the idea without first meeting the function_call-emits-no-text objection. It also states the two questions that decide whether it is implementable at all (is there an observable pre-dispatch point, and is it stable enough to depend on), and says plainly that closing it in favour of the current gate is an acceptable outcome if either answer is no.

On the order-dependent test — I cannot reproduce it, which changes its shape

Ran your exact command on Linux at 9bf8f4c, five times:

bun test src/tests/integration.test.ts -t "file-exhausted fallback with an empty process quota cache"
run1: 1 pass 0 fail
run2: 1 pass 0 fail
run3: 1 pass 0 fail
run4: 1 pass 0 fail
run5: 1 pass 0 fail

So it is not deterministically order-dependent — it is environment-dependent, which is a worse shape than the one you described and worth capturing in your issue.

Two things I checked that narrow it:

It is not sidebar-state leakage. The test lives at integration.test.ts:4978, inside describe('integration: active fallback routing') (line 2538), whose beforeEach at 2545 sets OPENCODE_OPENAI_AUTH_SIDEBAR_STATE_FILE at 2553. -t filtering still runs enclosing hooks, so the isolated path is set even standalone. The bunfig.toml preload (setup-env.ts) is a second layer under that, and its comment says it exists specifically so fire-and-forget writes never reach the operator's live default.

Worth noting the hazard that preload defends against is real on my machine: /tmp/opencode-openai-auth/sidebar-state.json exists and was written minutes ago by the running plugin. So a test that escaped isolation would read live operator state — which is exactly the failure mode that would differ between your box and mine.

That leaves module-level process state rather than file state as the remaining candidate — a memoized settings object, or an in-process quota cache that a sibling test's body resets and a hook does not. Since the test's precondition is literally "an empty process quota cache", anything that leaves that cache non-empty across a filtered run would flip it.

I agree with your framing entirely: a test that only passes alongside its siblings cannot gate the behaviour it names, and it hands a false positive to anyone bisecting with -t. Same family as the vacuous-test problem, which is why it is worth a real issue rather than a note.

Happy to take it if useful — I have the reproduction environment for the negative case, which is half of pinning it. Your call whether it lands on your side since you have the failing box.