fix(opencode): surface truncated turns instead of ending the loop - #40142
fix(opencode): surface truncated turns instead of ending the loop#40142iceteaSA wants to merge 1 commit into
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: One related PR was found: PR #26167: fix(session): retry empty stream truncations and discard partial parts This PR appears related as it addresses truncation handling in sessions, specifically dealing with empty stream truncations and partial parts. While the scope differs from PR #40142 (which focuses on surfacing truncated turns by checking |
|
I've run into the same issue with deepseek V4 flash, when it ends with the length reason the subagent does not continue, it stalls / exits. |
Issue for this PR
Fixes #40146.
That issue is item 4 of the root-cause chain in #18108 (P1 on its checklist), filed separately because #18108 covers five interacting problems and this one is independently fixable.
Related: #38747 (V2-side truncated-stream recovery — same problem class, different layer), #29363 (the 32K
OUTPUT_TOKEN_MAXclamp that makes truncation common — deliberately out of scope here).Type of change
What does this PR do?
A turn that finishes
lengthwas classified as a normal completion. Two checks in the session loop exclude only"tool-calls"(and"unknown"), never"length", so a provider that truncated its output at the token limit ended the session exactly as though the model had chosen to stop.That is #18108's P1, filed in March:
The failure is worst when the truncated turn produced nothing usable. A subagent on an OpenAI-compatible gateway spent its entire output budget inside the reasoning channel — parts were
step-start+reasoning(129,961 chars) +step-finish, no text and no tool call,finish: "length"— and the task completed with statusokand empty output. From the caller's side that is indistinguishable from a subagent that had nothing to say. Nothing anywhere insrcbranched on"length".The change: when a turn finishes
length, branch on whether it produced anything worth continuing from.lengthOutputLengthError, stoplengthOutputLengthError, stopThree decisions worth explaining, since each has a plausible-looking alternative:
Continuation is bounded to exactly one attempt, with no counter. The bound reads
lastAssistant.finishfrom history the loop already reloads each iteration.agent.stepscould not be reused for this: it defaults toInfinity, and at the limit it only injectsMAX_STEPS_PROMPTrather than breaking — it is a nudge, not a bound. An unbounded continuation would be worse than the original bug, since each iteration costs a full output budget.Recoverability is decided by part presence, not token counts. Some gateways report reasoning inside the ordinary output count, so
tokens.reasoningreads0on exactly the turns this targets. A token-based check would be vacuous where it matters most.OutputLengthErroris reused, not newly defined. It already exists in the schema, in core, insession/message-error.ts, is already a member of the assistant error union, andacp/service.tsalready maps it tostopReason: "max_tokens". It had no producer anywhere insrc— the plumbing was built for this case and never connected. This connects it.The entry-gate change is load-bearing rather than cosmetic: the loop reloads history at the top of every iteration, so without excluding
"length"there, the reloaded state (a truncated message with no tool calls) exits before the continuation turn can run. A reviewer confirmed this independently by reverting that hunk alone.One behavioural note for reviewers: the entry gate also governs direct
loop()callers. A session whose last assistant turn finishedlengthand is resumed through thesummarizeendpoint will now take an additional provider turn where it previously stopped. For that path the caller has explicitly asked for compaction and resumption, so continuing seems right — but it is a real change and worth a second opinion.The 32K
OUTPUT_TOKEN_MAXclamp that makes truncation common in the first place is not touched here. Four PRs against that constant have been closed without merging; it is contested design territory and belongs in its own change. This PR is about not misreporting truncation when it happens.How did you verify your code works?
Three integration tests in
packages/opencode/test/session/prompt.test.ts, plus alength()finish on the test LLM server's reply builder. All three were written first and confirmed failing (0 pass / 3 fail) before any production edit.lengthturn with text, followed by astopturn — asserts exactly two provider requests and that the final message carries the completed text.MessageOutputLengthErroron both the returned and the persisted message, with the reasoning part preserved.lengthturns with a sentinel third reply queued — asserts exactly two provider requests, so an unbounded implementation fails an assertion rather than hanging.Mutation-checked in both directions: reverting the production hunks turns all three red, restoring returns them green. A cross-family reviewer independently reverted each of the two hunks separately and confirmed the bound is what makes the third test pass — with the in-loop hunk alone reverted, the loop runs three provider turns instead of two.
bun testinpackages/opencode: 3231 pass / 0 fail (baseline ondevis 3228).bun typecheckclean inpackages/opencodeandpackages/core.One honest limitation: the first test does not discriminate the in-loop hunk on its own — with that hunk reverted,
SessionProcessor.processreturns"continue"anyway and the observable outcome is identical. It documents end-to-end recovery; the in-loop logic is pinned by the other two tests.Screenshots / recordings
Not a UI change.
Checklist