fix(passthrough): close a capped streaming turn as truncated, not as an error - #926
fix(passthrough): close a capped streaming turn as truncated, not as an error#926materemias wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The behavior change is well-scoped and backed by integration tests, with only minor observability tweaks suggested in review comments.
Pull request overview
Fixes a passthrough streaming edge case where a capped one-turn request (max_turns with turns=1) could be finalized via an SSE event: error even after meaningful content had already streamed to the client, producing a confusing “Reached maximum number of turns (1)” overlay on an otherwise-rendered answer.
Changes:
- Add a streaming-path “capped turn → truncated” close branch that emits
stop_reason: "max_tokens"+message_stop(no error frame) when content blocks were already delivered but no forwardable tool call was produced. - Preserve existing error behavior for two boundary cases: (1) only
message_startforwarded (no content blocks), and (2)tool_usestreamed but not captured (dangling call). - Add three integration test cases covering truncation and both boundary conditions.
File summaries
| File | Description |
|---|---|
| src/proxy/server.ts | Adds a streaming capped-turn truncation close path to avoid emitting an error frame after content already streamed. |
| src/tests/passthrough-early-stop-integration.test.ts | Adds integration tests validating truncation behavior and the two boundary cases that must remain errors. |
Review details
Suppressed comments (1)
src/proxy/server.ts:6161
- This telemetry record uses
sdkSessionId: resumeSessionId, which will be empty for non-resume requests; using the observedcurrentSessionId(when present) improves session-level correlation and matches the main stream success record’s pattern.
sdkSessionId: resumeSessionId,
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| requestSource, | ||
| isResume, | ||
| hasDeferredTools, | ||
| sdkSessionId: resumeSessionId, |
There was a problem hiding this comment.
Fixed in c437a6e: both capped-stream telemetry sites now use currentSessionId || resumeSessionId, matching the normal stream success path and preserving correlation for fresh requests. I also added a regression assertion for streamed result usage. Verification: bun test src/__tests__/passthrough-early-stop-integration.test.ts — 46 pass; bun run typecheck — clean.
bf60c37 to
1b46386
Compare
…an error A passthrough turn runs on a one-turn budget, so `max_turns` is its ordinary terminal state, not a fault. When it trips with tool calls captured, the streaming path already recovers them as `stop_reason:"tool_use"`. When it trips with none captured, the turn fell through to the error envelope - and the client had already rendered the turn's text, so it surfaced a finished answer with "Reached maximum number of turns (1)" stamped on it. 141 such terminations in 24h on this machine, all `reason=max_turns turns=1`, all unrecovered. The non-streaming path has degraded honestly since the budget dropped to one: content and no forwardable call is reported as `max_tokens`. Its comment already claimed the streaming path did the same. This makes that true. `max_tokens` is the choice on both paths for the same reason: it is the wire's word for a cut-off turn, so a client can continue from what it has, where an error frame is a dead end and `end_turn` is the silent-turn lie rynfar#768 exists to prevent. Nothing durable moves - with no captured calls there is no checkpoint to publish and no mapping to advance, exactly as on the throwing path. Two shapes stay on the error path, and each has a test: - A turn that forwarded only `message_start` delivered nothing to truncate. `eventsForwarded` cannot express that: it counts the envelope's own opening frame, so it is already 1 whenever `messageStartEmitted` is. The gate is `nextClientBlockIndex`, which counts only content blocks the client received - what the non-streaming branch means by `contentBlocks`. - A tool_use block on the wire with nothing captured means the hook refused those calls (forced-single overflow, duplicate abort, early-stop reversion). Closing that with `max_tokens` would dangle a call the client is told neither to run nor to drop. Verified: the new truncation test fails on the parent commit; each boundary test fails when its own clause is removed from the gate. Full suite clean.
A single-turn-capped passthrough turn that terminates `max_turns` without yielding anything — no wire event, no captured tool call — never reached the tool boundary the cap exists to stop at, so the turn bought nothing and cost the whole request. Production shape: a resumed opus[1m] stream that ran 108s, recorded 0 content blocks and 0 text events, and answered 500; the client's own identical retry succeeded. Reissue that turn once with the cap lifted, on both the stream and the non-stream path. Safe by the existing retry guards: the branch sits below `didYieldClientEvent` / `didYieldContent` and the committed priority-exposure check, so no envelope can be duplicated. Eligibility is the budget the attempt actually asked for, read off the options it built (`attemptMaxTurns === 1`) rather than parsed out of the SDK's "Reached maximum number of turns (N)" wording — that parse is optional, and an uncapped budget is a different failure that a reissue would only repeat. An operator-pinned `PASSTHROUGH_MAX_TURNS` is left alone, decided by `singleTurnCapLiftRaisesBudget`, which compares the real computation against itself instead of copying its conditions. Regressions cover the fresh and resumed shapes (the reissue keeps the resume target and takes its own fork target), the lift firing exactly once, and a pinned budget reporting the failure unchanged. The stream error log now also reports what the client received, so an error over rendered text is distinguishable from one that delivered nothing without reconstructing it from a null TTFB.
1b46386 to
514caca
Compare
|
Your The 11 failures here ( This branch predates that fix, so it is still running against the broken harness. Apologies for the delay in getting you CI signal on this — the approval was held while the flake made every result untrustworthy. |
The bug
A passthrough turn runs on a one-turn budget, so
max_turnsis its ordinary terminal state, not a fault.When it trips with tool calls captured, the streaming path already recovers them as
stop_reason: "tool_use". When it trips with none captured, the turn fell through to the error envelope - and the client had already rendered the turn's text over SSE. So the user saw a finished answer withReached maximum number of turns (1)stamped on it.141 such terminations in 24h on one machine, all
reason=max_turns turns=1, all unrecovered:The fix
The non-streaming path has degraded honestly since the budget dropped to one: content and no forwardable call is reported as
max_tokens(server.ts,passthrough.capped_turn_truncated). Its comment already claimed the streaming path did the same. This makes that true.max_tokensis the choice on both paths for the same reason: it is the wire's word for a cut-off turn, so a client can continue from what it has. An error frame is a dead end, andend_turnis the silent-turn lie #768 exists to prevent.Nothing durable moves. With no captured calls there is no checkpoint to publish and no mapping to advance - exactly as on the throwing path.
What stays on the error path
Two shapes, each with a test:
message_startforwarded. Nothing was delivered, so there is nothing to truncate.eventsForwardedcannot express this: it counts the envelope's own opening frame, so it is already 1 whenevermessageStartEmittedis. The gate isnextClientBlockIndex, which counts only content blocks the client received - what the non-streaming branch means bycontentBlocks.tool_useblock on the wire with nothing captured. That means the hook refused those calls (forced-single overflow, duplicate abort, early-stop reversion). Closing withmax_tokenswould dangle a call the client is told neither to run nor to drop.Verification
src/__tests__/passthrough-early-stop-integration.test.tsgains three cases. The truncation case fails on the parent commit; each boundary case fails when its own clause is removed from the gate.tsc --noEmitclean; full suite clean.