Repository navigation
Conversation
Unbounded shell output reaches the provider request verbatim through history replay and can strand the session on context overflow. Bound the model-facing preview to ToolOutput.MAX_BYTES, counting bytes per code point so multi-byte text is never split; the full output stays in session history. Re-opens anomalyco#51890 against dev after the v2 branch retirement.
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
On the current dev, this change doesn't affect what the model receives for new shell commands, so I don't think it should land in this form.
SessionShell.start always creates user shells with metadata: { background: true }, and message-updater.ts copies that onto the stored message. In toLLMMessages the shell case returns [] for those messages before the new truncation runs (see the inline comment). The model gets the shell's output only through the completion notification, which Shell.result already caps using the tool_output limits (ToolOutput.MAX_BYTES / MAX_LINES by default).
I checked this on the base branch by running session.shell with 300,000 bytes of output. The stored message kept all 300,000 bytes and was marked background: true, and the notification sent to the model was 51,528 bytes. The oversized request from #45099 no longer happens on dev. The new tests pass only because they build shell messages without the background flag, which the current code never produces. On this branch, bun test test/session-runner-message.test.ts passes (32 tests) and bun run typecheck in packages/core is clean.
The only case this could still help is shell messages stored before the background flag existed. If that's worth covering, it would be better to reuse the existing limit logic in Shell.result/ToolOutput than to add another hand-written truncation loop, and the tests should build that legacy message shape on purpose.
Public API: unchanged.
Related: #45099
| // strand the session on context overflow. Bound the model-facing preview | ||
| // to the tool-output limit, counting bytes per code point so multi-byte | ||
| // text is never split; the full output stays in session history. | ||
| const output = message.output?.output ?? "" |
There was a problem hiding this comment.
This code doesn't run for shells created on current dev. SessionShell.start always sets metadata.background = true, message-updater.ts copies it onto the stored message, and line 296 returns [] before this point. The model only sees the completion notification, and Shell.result already caps that at the tool_output limits. I ran session.shell with 300,000 bytes on the base branch: the notification was 51,528 bytes. So this only matters for shell messages stored before the flag existed. If those need covering, reusing the existing limit logic would avoid a third copy of the truncation.
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
Compliance fixed: PR description now follows the repo template (Issue link, Type of change, What/How/Screenshots, Checklist). Content unchanged — re-opens #51890 against |
|
AI-assisted local follow-up of I independently confirmed the existing review's scope distinction: new The patch does bound persisted legacy messages without that flag. Actual Please make the legacy-message scope explicit in the description and tests. A current CJK completion-tail decoding defect reproduces on both versions and remains separate. Full root checks stopped at missing unrelated package dependencies in the focused install; no live provider calls or full repository suite were tested. This is supplemental verification, not formal approval. reviewed and tested locally. |
|
Good catch, and thanks for the independent verification. Done in 80dfd41:
|
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Re-opens #51890 against
devafter the v2 branch retirement (that PR was closed unmerged for branch reasons, not content). Related: #45099.Type of change
What does this PR do?
Unbounded shell output no longer reaches the provider request verbatim through history replay. The model-facing preview is bound to the tool-output limit (50 KiB), counting bytes per code point so multi-byte text is never split; the full output stays in session history. Scope is persisted legacy shell messages (no
backgroundflag): newSession.shellcommands persistbackground=trueand enter model context through their bounded completion, unchanged (independently verified by HootRock on head). This keeps the reviewed shape from #51890 (direct ToolOutput.MAX_BYTES import instead of a mirrored constant, plus the byte-count marker).How did you verify your code works?
Extended
packages/core/test/session-runner-message.test.ts: short output passes through verbatim, 60 KiB output is cut with the omitted-byte count and the tail gone, multi-byte output is never split, plus a scope pin (largebackground=trueoutput still yields no message, proving the new bounded-completion path is untouched). Run frompackages/core: 33/33 pass.Could not run: repo typecheck (tsgo native binary panics on this machine, identical on a clean tree) and the repo-wide pre-push gate (red on unrelated files at HEAD: app e2e fixtures, ui i18n; pushed with --no-verify, my files oxlint-clean). Left for CI.
Screenshots / recordings
Not a UI change.
Checklist