fix(opencode): publish overflow error when recovery is abandoned, not attempted - #125
Merged
Conversation
… attempted
A provider 413 with auto-compaction on published Session.Event.Error and
then recovered: compact, retry, succeed. Every consumer of that channel
treats it as the run's outcome — CLI run exits 1, TUI shows an error
toast, the desktop app fires an OS notification, and orchestrators
tailing --format json mark the run failed — so a run that produced a
complete, correct result was reported as a failure.
The publish at processor.halt's auto-compact branch was the outlier on
three counts: it was the only publisher that does not set message.error
(the durable record disagreed with the event), the only one not followed
by idle or a throw, and nothing consumed it (compaction is triggered by
the "compact" return value). Recoverable provider errors elsewhere are
silent: retried 429s never publish, and the proactive overflow check
sets needsCompaction without an event.
Move the announcement to where failure is actually decided: compaction's
result === "compact" branch ("too large to compact even after
stripping"), which already writes the durable error but published
nothing — its error event previously arrived only as a side effect of
the halt re-entering during summary generation. Net contract:
session.error with a sessionID fires iff the outcome is affected.
Tests pin both directions: 413 -> compact -> retry exits 0 with no error
event; 413 -> compaction also overflows exits 1 with exactly one error
event carrying the clearer compaction message. Both fail without the
source change (verified by stashing it).
The recovered-413 fixture is from #123 by Magnus, which fixed the same
symptom consumer-side in run.ts; this supersedes it at the producer so
run.ts stays identical to upstream and stream consumers need no
clear-on-later-success logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
From cloud v4 production (reported via #123): a long run's context exceeds the provider limit, the provider rejects the request with HTTP 413, auto-compaction kicks in, the retry succeeds, the agent finishes the work — and the run is reported as failed. The CLI exits 1, the TUI shows an error toast, the desktop app fires an OS push notification, and cloud's worker (which tails
--format jsonstdout) marks the runrun.failed.Root cause
processor.tshalt, auto-compact branch:This publish is the outlier among all
Session.Event.Errorproducers, on three counts:message.error. The durable record says "no error"; only the ephemeral event claims one."compact"return value; nothing consumes this event to make recovery happen.And it breaks the codebase's own precedent for recoverable provider errors: retried 429/5xx never publish (
retry.tscontains zero publishes), and the proactive overflow check (processor.ts:507) setsneedsCompactionsilently. Only the reactive path announced itself — on the channel all four consumers read as "the run's outcome is failure."Fix
Move the announcement from overflow occurred to recovery abandoned.
processor.ts— delete the publish in the auto-compact branch. A 413 the system is about to compact-and-retry is the same grade as a retried 429.compaction.ts— theresult === "compact"branch ("too large to compact even after stripping media") is where overflow actually becomes an outcome. It already writes the durable failure (message.error,finish = "error") but published nothing — its error event previously arrived only as a side effect of the halt re-entering during summary generation. Publish there, with its clearer message.Net contract:
session.errorwith a sessionID fires iff the outcome is affected. No consumer changes needed anywhere — the CLI's existing latch, the TUI toast, the app notification, and cloud'sseen_errorgate all become correct as written.Relationship to #123
#123 fixed the same symptom consumer-side:
run.tscleared its latched error when a later assistant message finished cleanly. That worked for the CLI exit code, but taught one of four consumers to distrust the channel — the TUI toast, app notification, and cloud's stdout latch would all still fire failure UX on recovered runs. It also required stateful clear-on-later-success logic (with a subtle interaction: the clear actually fires on the summary message, not the recovered turn) and opened a behavioral divergence inrun.ts, which is otherwise identical to upstream here.This PR supersedes it at the producer. The recovered-413 test fixture is Magnus's from #123, credited — flipped to assert the stronger property (exit 0 and no error event on the stream).
Tests
Two tests in
run-process.test.ts, pinning both directions of the move:errorevent, recovered output present.errorevent, carrying "too large to compact". This guards the regression that a bare deletion would cause — the failed-compaction path only got its error event via the halt re-entry, so deleting the publish without adding the compaction.ts one would make failed recovery exit 0 silently.Verified both fail with the source change stashed and the tests kept:
expected exit 0, got 1expect(errors.length).toBe(1)fails (pre-fix the stream carries two overflow events and neither has the compaction message)Full
test/cli/run/run-process.test.ts: 15/15.bun typecheckclean. (test/session/compaction.test.tshas 2 failures andprocessor-effect.test.ts15, identical on cleanmain— pre-existing, unrelated.)Behavior matrix
Event.Compactedcompaction.auto: falseZone
Yellow — edits
packages/opencode/src/session/{processor,compaction}.ts, both of which already carry Yellow-zone exceptions.EXCEPTIONS.mdentry added maintainer-side. The bug is verbatim inanomalyco/opencodedev (processor.ts:616); an upstream PR with this patch is being opened in parallel, and this exception retires when it merges and syncs back.Supersedes #123.
Summary by cubic
Publish overflow errors only when recovery is abandoned. Recoverable 413s (auto-compaction then retry) no longer emit
Session.Event.Error, so successful runs report success across CLI/TUI/app/cloud.packages/opencode/src/session/processor.ts: removed error publish in the auto-compact path; only setctx.needsCompaction.packages/opencode/src/session/compaction.ts: whenresult === "compact"(cannot shrink), set durablemessage.errorandfinish = "error", then publishSession.Event.Errorwith a clear "too large to compact" message.test/cli/run/run-process.test.tsto pin both paths.Written for commit f0752f0. Summary will update on new commits.