Skip to content

fix(compression): keep tool_result blocks first when aging inserts an annotation - #12919

Closed
ntdat812 wants to merge 2 commits into
diegosouzapw:release/v3.8.51from
ntdat812:fix/compression-aging-toolresult-order-12890
Closed

ntdat812 wants to merge 2 commits into
diegosouzapw:release/v3.8.51from
ntdat812:fix/compression-aging-toolresult-order-12890

Conversation

@ntdat812

@ntdat812 ntdat812 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Closes #12890

Root cause

When the compression pipeline ages (or aggressively rewrites) a user message whose content is a lone tool_result block and has no text block of its own, replaceTextContent() fell into its if (!replaced) branch and prepended a fresh {type:"text"} block:

// open-sse/services/compression/messageContent.ts (before)
if (!replaced) {
  return { ...msg, content: [{ type: "text", text: newText }, ...msg.content] };
}

For a message like [{type:"tool_result", ...}] this produces ["text", "tool_result"]. The Anthropic Messages API requires tool_result blocks to come first in the message that immediately follows a tool_use, so upstream rejects the request:

[400] messages.N: `tool_use` ids were found without `tool_result` blocks immediately after:
toolu_… Each `tool_use` block must have a corresponding `tool_result` block in the next message.

Both callers hit this branch — progressiveAging.ts (fullSummary tier tagging for user messages) and aggressive.ts (replaceContent) — so once a conversation crosses the aging threshold it fails on every subsequent turn (the reporter saw ~110 occurrences in a day).

Call site Reaches !replaced prepend when…
progressiveAging.ts → tagAged("fullSummary", …) user message content is a lone tool_result (no text block)
aggressive.ts → replaceContent() same shape, aggressive mode

Fix

Only the empty-of-text branch changes. When the message carries a tool_result block, the annotation is appended after the existing content; messages without a tool_result keep the previous prepend behavior (the annotation still leads, unchanged):

if (!replaced) {
  const textBlock = { type: "text", text: newText };
  const hasToolResult = msg.content.some(
    (part) => !!part && typeof part === "object" && (part as TextBlock).type === "tool_result"
  );
  return {
    ...msg,
    content: hasToolResult ? [...msg.content, textBlock] : [textBlock, ...msg.content],
  };
}

Scoping the change to messages that actually contain a tool_result keeps the existing ordering for every other shape (plain text, image-only, multi-text), so no other compression path shifts.

Tests

New regression file tests/unit/compression/tool-result-order-12890.test.ts (4 cases), next to the sibling tests/unit/compression/aggressive-fidelity.test.ts which already covers Anthropic tool_result compression:

  • lone tool_result → text block appended, tool_result stays at index 0
  • two tool_result blocks → both survive, no text block precedes them
  • no tool_result (image-only) → prepend behavior unchanged
  • end-to-end: aging a lone-tool_result user message across the fullSummary threshold keeps tool_result before any text block

Base-revert proof — with the source change stashed out and only the test present on the base commit:

# tests 4  # pass 1  # fail 3

The 3 ordering cases fail on base (the "unchanged behavior" case passes). With the fix:

# tests 4  # pass 4  # fail 0

Commands run

# base (fix reverted, test only)
$ node --import tsx/esm --test tests/unit/compression/tool-result-order-12890.test.ts
# tests 4  # pass 1  # fail 3

# with fix
$ node --import tsx/esm --test tests/unit/compression/tool-result-order-12890.test.ts
# tests 4  # pass 4  # fail 0

# siblings (no regression)
$ node --import tsx/esm --test tests/unit/compression/aggressive-fidelity.test.ts \
    tests/unit/compression-aggressive-spare-last-user.test.ts \
    tests/unit/codex-responses-passthrough-strip-3317.test.ts
# tests 16  # pass 16  # fail 0

$ npx eslint open-sse/services/compression/messageContent.ts tests/unit/compression/tool-result-order-12890.test.ts
# clean
$ npx prettier --check <same files>   # clean

Not verified

I did not reproduce the original upstream 400 against a live Anthropic endpoint — the fix and its proof are at the replaceTextContent() unit boundary plus the applyAging() integration path, matching the block ordering the issue traced. I did not run the full suite / coverage gate or test:vitest; I ran the changed test plus its immediate siblings. The base branch is currently red independent of this change — ⚠️ base-red inherited: #12732 — so compare CI failure sets against the base rather than reading a red check as this PR's fault.

… annotation

replaceTextContent() prepended a {type:"text"} block to messages that had no
text block of their own, producing ["text","tool_result"] for a user message
whose only content was a tool_result. The Anthropic Messages API requires
tool_result blocks to come first in the message following a tool_use, so
upstream rejected it with 400 "tool_use ids were found without tool_result
blocks immediately after".

When the message carries a tool_result block, append the annotation after the
existing content instead of prepending it; messages without a tool_result keep
the prior prepend behavior.

Closes diegosouzapw#12890
diegosouzapw pushed a commit that referenced this pull request Sep 10, 2026
 (#12925)

Rebased onto the tip and completed, per the maintainer's call to finish the wiring rather than merge the capability alone.

What changed since your version:

The tip had already cleared the TS2554 by deleting the 16th argument, leaving a comment that the highWaterMark stays at the helper default. So the base-red you found is gone, but the 64 KB #12179 asked for was still not applied and your new parameter had no caller. glm.ts now passes it, which is what turns the capability into the fix.

Your test file also hung the runner: every stream createSSEStream builds arms a 10s idle watchdog via setInterval in start, and nothing cancelled them, so node:test waited on a non-empty event loop long after the assertions passed. Cancelling each readable in an after hook runs the cancel handler that clears the timer — the file now reports in about 7 seconds. Worth knowing for future stream tests.

Your five assertions are unchanged and all pass. Reading the writable's desiredSize to measure the queue budget the stream was actually built with, rather than standing in for it, is the detail that makes this testable at all — and the 0-budget case pinning `??` against `||` is the kind of thing that silently rots otherwise.

Thank you also for separating your own red checks from the base's and reporting what you found there. That is how #12919's identical failures got explained instead of chased.
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for the clear root-cause write-up — the diagnosis and fix here are correct, but the
identical fix already shipped via #12920 (merged 2026-09-11, same messageContent.ts function,
same append-when-tool_result / prepend-otherwise behavior, just refactored into an exported
isToolResultBlock() helper). Closing this one as a duplicate rather than merging a second
implementation of the same fix — appreciate you catching and reporting #12890 independently.

Triage note: this is the review recommendation — the close itself happens only after the maintainer's per-PR sign-off (and, where a superseding PR is named, after it has landed). Nothing is being closed by this comment.

@diegosouzapw

Copy link
Copy Markdown
Owner

Obrigado — o diagnóstico estava certo, mas o fix já entrou por outro caminho. Verifiquei contra o tip atual:

$ git log --oneline origin/release/v3.8.51 --grep="12920"
e1a1290 fix(compression): keep tool_result blocks first when aging annotates a turn (#12920)

A #12920 mergeou em 11/09 com o comportamento idêntico (append quando é tool_result, prepend caso contrário), no mesmo arquivo e na mesma função — open-sse/services/compression/messageContent.ts — refatorado num helper exportado isToolResultBlock(), que confirmei presente no tip.

Fechando como subsumida. O crédito pelo diagnóstico do ordenamento é seu.

muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…gosouzapw#12179 (diegosouzapw#12925)

Rebased onto the tip and completed, per the maintainer's call to finish the wiring rather than merge the capability alone.

What changed since your version:

The tip had already cleared the TS2554 by deleting the 16th argument, leaving a comment that the highWaterMark stays at the helper default. So the base-red you found is gone, but the 64 KB diegosouzapw#12179 asked for was still not applied and your new parameter had no caller. glm.ts now passes it, which is what turns the capability into the fix.

Your test file also hung the runner: every stream createSSEStream builds arms a 10s idle watchdog via setInterval in start, and nothing cancelled them, so node:test waited on a non-empty event loop long after the assertions passed. Cancelling each readable in an after hook runs the cancel handler that clears the timer — the file now reports in about 7 seconds. Worth knowing for future stream tests.

Your five assertions are unchanged and all pass. Reading the writable's desiredSize to measure the queue budget the stream was actually built with, rather than standing in for it, is the detail that makes this testable at all — and the 0-budget case pinning `??` against `||` is the kind of thing that silently rots otherwise.

Thank you also for separating your own red checks from the base's and reporting what you found there. That is how diegosouzapw#12919's identical failures got explained instead of chased.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(backend): Compression aging inserts text block before tool_result → Anthropic 400 "tool_use ids without tool_result"

2 participants