Repository navigation
fix(session): drop assistant messages with no provider content (empty-assistant-guard) - #53575
Closed
mradwankhalil wants to merge 24 commits into
Closed
mradwankhalil wants to merge 24 commits into
mradwankhalil wants to merge 24 commits into
Conversation
prompt.ts passed `parentID: lastUser.id` when creating a compaction summary. When a prompt arrived between the compaction marker and the summary, the summary was orphaned: the read-side gate only honours a summary whose parent resolves to a user message carrying the compaction part, so compaction re-fired indefinitely and the marker's model (how a plugin-supplied pin arrives) was discarded in favour of the session model. One symptom or the other, depending on timing. Anchor the summary to the marker instead: - pass the marker id (task.messageID) as the summary's parentID - hard-reject a marker that already carries a completed summary - cut history at the marker index, so a prompt that arrived after the marker stays live instead of being summarized and replayed - split the resume user id from the marker identity - suppress the synthetic autocontinue while a real prompt is waiting - drop the forced tail; a missing tail_start_id is a valid full compaction, not a signal to force one - label the model source in logs (configured-compaction-agent vs request-marker) so a working pin is distinguishable from a dead one
…pipeline The fix was uncommitted working-tree changes on fix/compaction-anchor, so any checkout would have silently destroyed it. Adds: local-fixes/manifest.json the fix, its markers, build and stage config local-fixes/apply.mjs upgrades to a newer v1.x tag and re-applies the fix local-fixes/verify.mjs cheap pre-flight on the staged binary local-fixes/patches/compaction-anchor.patch fallback when a cherry-pick conflicts fixes:apply / fixes:verify npm scripts The fix commit is immutable: every upgrade builds on a fresh build/<version> branch cut from the target tag, so an upgrade can never rewrite or lose the fix, and a failed upgrade restores the original branch untouched.
- the test gate matched the literal "0 fail" and so rejected passing runs; it now reads the numeric counts and fails only when failed>0 or passed==0 - preflight used `!x === 0`, which is always false, so an explicitly named tag was never fetched before being checked
OpenChamber resolves its opencode binary at startup as
process.env.OPENCODE_BINARY || searchPathFor('opencode')
so the environment variable is the only durable pin. settings.json's
opencodeBinary is UI-managed state: OpenChamber drops the key on every
settings rewrite (observed twice on 2026-09-25 at 18:57:50 and 19:10:49)
and never reads it at startup.
On the 2026-09-25 restart the app therefore came up on its bundled
1.18.31 with the compaction fix silently absent, despite pin-opencode.ps1
having written and verified the settings key.
- pin-opencode.ps1: sets the User-scope OPENCODE_BINARY (primary, and the
only thing that matters), broadcasts WM_SETTINGCHANGE so newly launched
apps inherit it, and writes the settings key only as cosmetic best-effort
while OpenChamber is closed.
- verify.mjs [6]: checks the environment variable. It previously reported
'settings.json -> staged binary' as ok, which was a pin that did not
exist. settings.json is now reported as cosmetic and never trusted.
- manifest.json: stage.pinMechanism documents the real mechanism;
stage.settingsKeyNote records that the key is dropped on rewrite.
Verified: fixes:verify [6] reports 'OPENCODE_BINARY (User) -> staged
binary'; managed-opencode/50192.json records opencode-patched.exe at
1.18.32 after restart.
Every claim in it was verified against the actual patch before committing: - prompt.ts: parentID lastUser.id -> task.messageID (patch lines 172/178) - diffstat 3 files, +230 -11 - message-v2.ts:534 (the forced-tail premise was false) - 9/9 rule replay, 37-hour runaway, +156 test lines, 59 pass / 1 skip / 0 fail It separates the fix from the circuit-breaker gap deliberately: the absence of any guard against repeated compaction is a different defect and deserves its own change, not to be smuggled into this one. It also states plainly that the model-resolution expression is byte-identical to upstream, so a reviewer does not suspect the patch of changing model selection.
…n agent (compaction-model-override)
…ore tree (config Settings + resolveRef seam, not in the v1 build)
…string (identifier was minified away)
…n-scope narrowing)
…is the overflow fallback (compaction-pin-precedence)
…ify the actually-running binary (launch-hijack hardening)
github-actions
Bot
added
the
needs:compliance
Contributor
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 72 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
Contributor
|
The following comment was made by an LLM, it may be inaccurate: |
…-assistant-guard) Strict providers (Kimi) reject assistant messages with no content with 400 "must not be empty", permanently wedging the session: every retry persists another error husk and the session can never send again. - Emit an assistant message only when it contributes provider content: non-empty text, a tool call, non-empty reasoning, or same-model reasoning carrying a replayable signed/redacted payload (Anthropic signed-thinking replay preserved; dropped after a model switch). - Drop empty step-split blocks before conversion so a non-empty sibling step can no longer ship an empty assistant turn. - Guard tolerates undefined text (no .trim() crash). Tests: message-v2.fix5.test.ts (11 cases) + existing suite 39/39. Upstream: anomalyco#53571 (PR anomalyco#53575).
mradwankhalil
force-pushed
the
fix/compaction-anchor
branch
from
October 6, 2026 17:50
18e2b88 to
122ce94
Compare
Contributor
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 72-hour window. Feel free to open a new pull request that follows our guidelines. |
github-actions
Bot
removed
the
needs:compliance
Contributor
Author
|
#53571/#53575 appear process-closed rather than technically reviewed: the closure came from automation, the PR has no REST reviews or inline comments, and its recorded standards/compliance checks passed. Against the pinned PR head, the focused fixtures pass 11/11 and the existing message-v2 suite passes 39/39; the combined run was 50 pass with one teardown failure, so this is not a clean full-suite result. The PR also contains 55 files, including unrelated handoff/patch artifacts. Would a maintainer prefer a narrow, template-compliant refile with only the intended source and tests? |
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.
Fixes #53571
Problem
Strict providers (Kimi's coding API) reject assistant messages with no content (
400 "must not be empty"), permanently wedging the session — every retry persists another failed-reply husk and the session can never send again. Trigger: any reply stream that dies before producing content (app quit mid-generation, provider error mid-turn).Two conversion gaps in
toModelMessagesEffect:parts.length > 0— a message with onlystep-startor empty-text parts converts to an empty provider message.step-startinto separate provider messages — an empty sibling step still serializes ascontent: ""even when another step has text.Fix
ProviderTransformreplays signature-bearing reasoning even with empty text; it is dropped after a model switch).step-startmarker) are dropped before conversion, so a non-empty sibling step can no longer ship an empty assistant turn.Tests
11 new fixture cases (
message-v2.fix5.test.ts): step-start-only, empty/whitespace text, signed reasoning retention, redacted reasoning retention, model-switch drop, empty sibling step, empty first/middle/last steps, zero-part passthrough, plus a no-empty-assistant guarantee on converted output. Existingmessage-v2suite: 39/39.Adversarially reviewed: tool pending/running states preserved (Anthropic tool_use pairing), extracted-media injection path preserved, error-filter ordering unchanged, whitespace-only dropped,
undefined textno longer crashes the guard.