Skip to content

Drop tool results orphaned by trimMessagesToFitTokenLimit at the removal boundary - #1292

Open
nordicnode wants to merge 2 commits into
CodebuffAI:mainfrom
nordicnode:fix/trim-orphaned-tool-results
Open

Drop tool results orphaned by trimMessagesToFitTokenLimit at the removal boundary#1292
nordicnode wants to merge 2 commits into
CodebuffAI:mainfrom
nordicnode:fix/trim-orphaned-tool-results

Conversation

@nordicnode

Copy link
Copy Markdown

Problem & Context

trimMessagesToFitTokenLimit removes 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 its role: 'tool' results. The surviving tool message then reaches the provider without its call and the whole step fails with tool_call_id does not exist (observed on the openai-compatible lane; consumed via getMessagesSubset by find-files/request-files-prompt.ts:201,271).

Reproduction (pre-fix, against the real module):

messages = [
  userMessage('please write the file'),
  assistantMessage({ content: [{ type: 'tool-call', toolCallId: 'c1', toolName: 'write_file', input: { content: 'x'.repeat(4000) } }] }),
  { role: 'tool', toolName: 'write_file', toolCallId: 'c1', content: jsonToolResult('ok') },
  assistantMessage('done'),
]
trimMessagesToFitTokenLimit({ messages, systemTokens: 0, maxTotalTokens: 600, ... })
→ output roles: ["user", "user", "tool", "assistant"]   // "tool" has no preceding call → provider 400

dropUnansweredToolCalls at 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 surviving role: 'tool' message whose toolCallId was 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. providerExecuted calls are excluded from both sets, mirroring the pairing semantics of dropUnansweredToolCalls.

Architecture & Conventions Conformance

  • Adheres to Dependency Injection (contracts defined in common/src/types/contracts/, no module monkey patching) — pure function change, logger injected as before
  • Terminal commands use terminalCommandBroker (no direct spawn or TUI-process bypass) — no process execution touched
  • Environment hygiene respected (getCliEnv() for CLI, getSdkEnv() for SDK, no forbidden getProcessEnv() imports) — no env access added
  • Freebuff mode compatibility (IS_FREEBUFF preserved, no paid features introduced) — product-agnostic bug fix
  • Imports ordered and explicit (import type used for types) — no import changes

Scope Verification

  • All modified files are within allowed public directories: packages/agent-runtime/
  • NO modifications to web/, freebuff/web/, packages/internal/, packages/billing/, packages/bigquery/, or packages/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 writable HOME; the sandboxed checkout has a read-only home directory)
  • Unit / integration tests added or updated in affected package
  • Anti-flake principles observed (no sleeps, no ports, no filesystem, no timers; the budget-sweep test asserts a structural invariant rather than fragile token arithmetic)

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, providerExecuted asymmetry, 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 is providerExecuted-related; input never mutated; no new failures.

Verification Output / Log Snippet

$ bun test packages/agent-runtime/src/util/__tests__/messages.test.ts
 32 pass
 0 fail
 92 expect() calls
Ran 32 tests across 1 file. [454.00ms]

$ bun run --cwd packages/agent-runtime test
 597 pass / 5 fail  (identical to main baseline: MCP schema ×2 = #1259, find-files WIP ×1-2 — not this change)

$ bun run --cwd packages/agent-runtime typecheck
 0 new errors (2 agents-graveyard = #1202; 1 from uncommitted find-files WIP, not this branch's files)

$ bun cli/scripts/smoke-binary.ts cli/bin/freebuff
 smoke-binary: tree-sitter init OK.
 smoke-binary: OK (matched /\x1b\[\?1049h/, exit code null, 9176 bytes captured, attempt 1/3).

New tests (all against the production module, no local reimplementations):

  1. drops a tool result whose call was removed at the boundary — the reported 400 case; also asserts the final 'done' assistant message survives
  2. drops tool results orphaned after a kept keepDuringTruncation message — removal runs are not pure prefixes when keepDuringTruncation messages sit mid-run, so the orphan can appear anywhere, not just leading the kept run
  3. keeps tool results whose call survives the trim (no-trim passthrough)
  4. leaves pre-existing orphans in an already-malformed history untouched (scoping guard, under an active trim)
  5. keeps the invariant across a sweep of budgets (six budgets × two call pairs, structural assertion)

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.
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.

1 participant