Skip to content

fix(openai-compat): stop re-sending tool results the thread already holds - #77

Merged
Enderfga merged 2 commits into
Enderfga:mainfrom
jailbirt:fix/trim-tool-results-on-live-thread
Aug 14, 2026
Merged

fix(openai-compat): stop re-sending tool results the thread already holds#77
Enderfga merged 2 commits into
Enderfga:mainfrom
jailbirt:fix/trim-tool-results-on-live-thread

Conversation

@jailbirt

@jailbirt jailbirt commented Aug 7, 2026

Copy link
Copy Markdown

Stacked on #76. The diff shown here includes that PR's commit; only the second commit belongs to this one. Happy to rebase once #76 lands or is closed.

The problem

serializeToolResults() walks the entire messages array on every call, so each hop of a tool loop re-sends the loop's whole result history.

For engines that resume a native conversation that history is already in the transcript, so the cost compounds. With a 30k-character batch per round:

hop results actually new results serialized
1 30k 30k
5 30k 150k
10 30k 300k

The prompt grows quadratically in the number of rounds while the useful content grows linearly. In our deployment this was the dominant term in long tool loops — the ones that overflow the window — and measured as the larger of the two effects we chased, ahead of the system-prompt re-send.

The change

Scope the results to the round that answers the engine's most recent assistant turn:

const scoped = latestRoundOnly ? messages.slice(messages.map((m) => m.role).lastIndexOf('assistant') + 1) : messages;

latestRoundOnly is only true when the conversation is known to hold the rest — the nativeThreadIsLive() predicate from #76. Everything else keeps the current behaviour, so the failure direction stays "send too much", never "lose context".

No per-session state: which results are new is already implied by the transcript shape the caller sends.

Why it is safe on a fresh session

The scoping is gated on sessionExists && engineHasNativeConversation(engine) && nativeThreadIsLive(...). A brand-new session — including one recreated after a restart — has no id captured, so it takes the full path and receives every result. That is exactly the case #76 exists to detect.

One ordering change

sessionExists moves a few lines up, above the extractUserMessage() call, so the extraction can see it. Safe because:

  • manager.listSessions() does not depend on the extraction.
  • In the tool-loop branch extractUserMessage() returns isNewConversation: false unconditionally, so needsCreate there reduces to !sessionExists — the same predicate the create path uses below.

Behaviour

situation before after
resumed thread, mid tool loop all results, every hop results of the latest round
resumed thread, first tool round all results all results — unchanged
session in map, no id captured all results all results — unchanged
fresh session all results all results — unchanged
claude, one-shot engines all results all results — unchanged

Tests

Five cases on the scoping, including the two boundaries that would silently lose context if the slice were wrong: a tool round with no preceding assistant turn keeps everything, and a trailing assistant turn with no results after it yields an empty block.

npm run build, npm run lint, npm run format:check clean; vitest run green at 905/905 across 58 files.

claude added 2 commits August 7, 2026 19:08
… tool reminder

The resume-turn reminder was gated on `!needsCreate`, which only says the session is
in the manager's map. That is not the same as the engine having created a
conversation: `start()` (base-oneshot-session.ts:103) resolves the cwd, assigns an
id and marks the session ready without spawning anything, and the conversation id is
captured later — codex on `thread.started` (persistent-codex-session.ts:220), agy by
harvesting the log after the first turn.

So a first send that fails before that point leaves the session in the map with no
id, and the next turn takes the resumed-thread path: it sends a short reminder
referring to tools the engine never received, with no schemas, no identity and no
history — and still answers 200.

The gate now also requires the id to be present, via a small exported predicate so
the rule is testable on its own. Once a thread is live nothing changes.

`SessionManagerLike.getStatus` had the two ids omitted from its hand-written return
type; they are added as optional, which is what they are.
…olds

`serializeToolResults()` walked the whole messages array on every hop, so a tool
loop re-sent its entire result history each time. On engines that resume a native
conversation those results are already in the transcript, which makes the prompt
grow quadratically in the number of rounds: with a 30k-character batch per round,
hop 10 carries ~300k characters the engine has already seen.

Results are now scoped to the ones that answer the engine's most recent assistant
turn, but only when the conversation is known to hold the rest — the same
`nativeThreadIsLive()` predicate the tool reminder uses. Anything that cannot be
confirmed keeps the previous behaviour of sending everything, so the failure
direction stays "send too much" rather than "lose context".

`sessionExists` moves a few lines up so the extraction can see it. That is safe:
`manager.listSessions()` does not depend on the extraction, and in the tool-loop
branch `isNewConversation` is always false, so `needsCreate` there reduces to
`!sessionExists` — the same predicate the create path uses below.
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.

3 participants