-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Drop tool results orphaned by trimMessagesToFitTokenLimit at the removal boundary - #1292
Drop tool results orphaned by trimMessagesToFitTokenLimit at the removal boundary #1292nordicnode wants to merge 3 commits into
Conversation
The removal run in trimMessagesToFitTokenLimit stops as soon as the token budget is met, which can land between an assistant tool-call and its role:'tool' result. The surviving tool message then reaches the provider without its call and the step fails with 'tool_call_id does not exist' — observed on the find-files request path via getMessagesSubset. After the removal loop, drop results whose call this trim removed; results whose call never existed in the input history pass through unchanged so pre-existing orphans are not silently rewritten, and providerExecuted calls are excluded to mirror the pairing semantics of dropUnansweredToolCalls.
codebuff-team
commented
Sep 7, 2026
Good find and a tight fix. The root cause — trimMessagesToFitTokenLimit's removal boundary can land between a tool-call and its role: 'tool' result, producing a history that providers reject with tool_call_id does not exist — is real and the fix in messages.ts addresses it directly without touching unrelated logic. The five tests in messages.test.ts cover the core case, the keepDuringTruncation interaction, the no-op case when nothing is removed, the deliberate non-goal (pre-existing malformed histories are left alone), and a budget sweep, which is the right level of coverage for a change to core trimming logic.
One readability nit, not blocking: the variable removedCallIds is actually populated from all call ids in the original messages (not just the ones removed by the trim), and is only narrowed to "actually removed" by subtracting survivingCallIds afterward. A name like inputCallIds would make the two-set diff obvious at a glance; as written a future reader has to trace the loop to realize it isn't what the name implies.
Worth double-checking before porting: behavior when a single assistant message carries multiple tool-calls and only some of their results get orphaned — the per-callId matching looks correct from reading it, but it'd be good to see an explicit test for that multi-call-per-message case since it's a plausible real shape from parallel tool calls.
Scope is clean (packages/agent-runtime only), no forbidden paths touched, and the PR doesn't overreach beyond the stated bug.
Addresses PR CodebuffAI#1292 review: the set is populated from all call ids in the input history and narrowed by diffing against survivingCallIds, so inputCallIds makes the two-set diff obvious. Adds explicit tests for the parallel-tool-call shape (one assistant message, several calls): orphaned results are dropped per toolCallId while a generous budget keeps every result of a surviving multi-call message.
nordicnode
commented
Sep 7, 2026
Thanks — both addressed in bce9434:
- Rename:
removedCallIds→inputCallIds, so the two-set diff (inputCallIdsminussurvivingCallIds) reads at a glance. - Multi-call-per-message test: added
drops only the orphaned results when one assistant message carries multiple tool calls— one assistant message with parallel callsc1/c2, akeepDuringTruncationsteer message between the results, tight budget. Asserts the no-orphan invariant, that neither orphaned result survives, and that the kept steer + final reply do. Plus a positive control (keeps every result of a multi-call assistant message that survives the trim) proving per-callId matching doesn't overcorrect when the multi-call message survives.
Red/green proof for the new multi-call test: against the true pre-fix file (git show 441947ed9^:packages/agent-runtime/src/util/messages.ts, zero occurrences of either set name) it fails 1/1; with the fix the full file is 34 pass / 0 fail under bun x bun@1.3.14. Package typecheck unchanged from the baseline noted in the body (×ばつ agents-graveyard = #1202, ×ばつ uncommitted find-files WIP; no new errors).
Problem & Context
trimMessagesToFitTokenLimitremoves a contiguous run of oldest messages until the token budget is met — and the run can stop exactly between an assistant message carrying tool-calls and itsrole: 'tool'results. The surviving tool message then reaches the provider without its call and the whole step fails withtool_call_id does not exist(observed on the openai-compatible lane; consumed viagetMessagesSubsetbyfind-files/request-files-prompt.ts:201,271).Reproduction (pre-fix, against the real module):
dropUnansweredToolCallsat the conversion chokepoint intentionally handles only the mirror case (calls whose results are gone) — orphaned results were the gap.Changes Made
packages/agent-runtime/src/util/messages.ts(trimMessagesToFitTokenLimit): after the removal loop, drop any survivingrole: 'tool'message whosetoolCallIdwas present in the input history but is not provided by any surviving assistant message. +38 lines, no other behavior touched.packages/agent-runtime/src/util/__tests__/messages.test.ts: five regression/behavior-preservation tests importing the real production function.Deliberate scoping: only results orphaned by this trim are dropped (their call existed in the input and was removed). Orphans already present in a malformed input history pass through unchanged, as before — this fix does not silently rewrite histories it did not break.
providerExecutedcalls are excluded from both sets, mirroring the pairing semantics ofdropUnansweredToolCalls.Architecture & Conventions Conformance
common/src/types/contracts/, no module monkey patching) — pure function change, logger injected as beforeterminalCommandBroker(no directspawnor TUI-process bypass) — no process execution touchedgetCliEnv()for CLI,getSdkEnv()for SDK, no forbiddengetProcessEnv()imports) — no env access addedIS_FREEBUFFpreserved, no paid features introduced) — product-agnostic bug fiximport typeused for types) — no import changesScope Verification
packages/agent-runtime/web/,freebuff/web/,packages/internal/,packages/billing/,packages/bigquery/, orpackages/build-tools/Testing & Verification
bun run build:sdk(passed cleanly)bun run build:freebuff(passed cleanly)bun cli/scripts/smoke-binary.ts cli/bin/freebuff(passed cleanly — with the CI env var set and a writableHOME; the sandboxed checkout has a read-only home directory)Tests were proven to catch the bug: with the fix stashed, 3 of the new tests fail against the unfixed source (boundary orphan, keepDuringTruncation-interleaved orphan, budget sweep); with the fix applied, all 32 tests in the file pass. Fix and tests were additionally verified by an independent adversarial pass (targeted attacks over duplicate call ids, multi-call assistant messages,
providerExecutedasymmetry, keepDuringTruncation interaction, input-mutation checks, early-return path, plus a seeded property fuzz ≥ 3000 histories): every surviving tool result either has its call, was a pre-existing orphan, or isproviderExecuted-related; input never mutated; no new failures.Verification Output / Log Snippet
New tests (all against the production module, no local reimplementations):
drops a tool result whose call was removed at the boundary— the reported 400 case; also asserts the final'done'assistant message survivesdrops tool results orphaned after a kept keepDuringTruncation message— removal runs are not pure prefixes whenkeepDuringTruncationmessages sit mid-run, so the orphan can appear anywhere, not just leading the kept runkeeps tool results whose call survives the trim(no-trim passthrough)leaves pre-existing orphans in an already-malformed history untouched(scoping guard, under an active trim)keeps the invariant across a sweep of budgets(six budgets ×ばつ two call pairs, structural assertion)