Repository navigation
fix(providers): carry tool-call history into the gemini-web prompt - #14374
diegosouzapw merged 6 commits into
Conversation
buildGeminiToolPrompt() kept only the system messages plus the last user turn, so the assistant's tool call and its role:"tool" result never reached Gemini Web. The model had no evidence the tool had run and re-issued the same call under fresh ids until the client's tool-loop limit stopped it (cheaper-inference#14368). Render the prior turns under the same "Previous conversation:" labels the no-tools path already uses, and keep the single-turn output byte-identical so the cheaper-inference#7286 guards still hold. Closes cheaper-inference#14368
|
Thanks for picking this up. I checked the current head against the exact post-tool request from #14368, and this PR still drops the tool call and result in the standard OpenAI agent loop. The normal continuation request ends with the tool result and has no new user turn: However, the new builder finds the last effectiveMessages.slice(0, lastUserIdx)Both The new regression test does not catch this because it adds another user message after the tool result: That is a later follow-up question, not the automatic model continuation after a tool execution. Could you add the exact no-trailing-user regression and adjust the boundary accordingly? At minimum this should pass: const prompt = buildGeminiToolPrompt([
{ role: "system", content: "TOOL CONTRACT" },
{ role: "user", content: "What is in the catalog?" },
{
role: "assistant",
content: null,
tool_calls: [
{
id: "call_1",
type: "function",
function: { name: "lookup_catalog", arguments: "{}" },
},
],
},
{
role: "tool",
tool_call_id: "call_1",
content: '["Item A","Item B"]',
},
]);
assert.match(prompt, /lookup_catalog\(\{\}\)/);
assert.match(prompt, /Tool result \[call_1\]/);
assert.match(prompt, /Item A/);A robust split probably needs to distinguish two cases:
There are also parts of #14368 this PR deliberately does not cover: an incomplete |
buildGeminiToolPrompt() rendered effectiveMessages.slice(0, lastUserIdx), so a tool round that ends the conversation — system, user, assistant(tool_calls), tool — dropped the call and its result, the exact evidence cheaper-inference#14368 was about. Skip only the current ask instead and render everything else in index order; the single-turn shapes cheaper-inference#7286 pins stay byte-identical.
|
@formilw your repro is exact, and you are right about both halves. Fixed in 1a0ac58.
The boundary is now the current ask itself — render every message except Measured with
The new test is your snippet plus two ordering assertions: the call line precedes its |
|
@diegosouzapw could you approve the workflows for |
|
Thanks — the bug is real and the rendering format you chose is right (labeled I replicated your function body and fed it the exact array from the issue's Steps to Reproduce — Could you extend the slice to include the assistant/tool turns after |
|
The Re-ran it just now by slicing the function verbatim out of so neither Both smaller asks are done: the description now says |
|
@.babysit/or_14374_reply.md |
|
Reposting the intended reply: comment #5837403165 in this thread is an unsubstituted placeholder path ( @diegosouzapw your read was right: const lastUserIdx = effectiveMessages.map((m) => m.role).lastIndexOf("user");
...
for (const [index, m] of effectiveMessages.entries()) {
if (index === lastUserIdx) continue; // rendered below as the current askI re-ran your literal array (
Both branches are covered in Your two smaller asks are already in the tree: the description says On validation, you are still right and nothing has changed since: |
|
Correction first: my comment #5837403165 in this thread landed as the literal 27-character text All four asks are on the branch now — head The real defect you found. You were right that my slice was wrong: The test you asked for.
Body. One thing I have to flag rather than claim: nothing has validated these two test cases yet. Re-checked at 03:55Z today, |
827306f
into
cheaper-inference:release/v3.8.51
Summary
buildGeminiToolPrompt()now renders the turns before the current one — including the assistant's tool call and the matchingrole: "tool"result — instead of keeping only the system text plus the last user message. Refs #14368.Motivation
gemini-web has no native message array: everything is flattened into one text box, so that prompt string is the only history the model ever sees. In tool mode the builder dropped it, keeping every system message (the client's own instructions plus the synthetic tool contract appended last by
prepareToolMessages(), per #13380) and the final user message only.The reporter's trace is exact and reproduces: with the client sending
user -> assistant.tool_calls -> tool -> user, the text typed into Gemini Web contained the tool contract and the new ask but neither the call nor its result, so Gemini had no evidence the tool had run and re-issued the same call under fresh ids — five times in that session, ending in a truncated tool block returned to the client as ordinary assistant text. This diverged from the no-tools path, which already emits a labeled transcript viabuildGeminiPrompt().What changed
open-sse/executors/gemini-web.ts, insidebuildGeminiToolPrompt()only:User: .../Assistant: ...for text turns,Assistant requested tool [id]: name(arguments)for eachtool_callsentry, andTool result [tool_call_id]: ...for tool turns;Previous conversation:with the current ask underCurrent user message:, the same labels the no-tools builder already uses, so the two paths now agree;tool_calls/tool_call_idinstead of casting, and adds noany(the file's two pre-existingno-explicit-anyhits are unchanged and still frozen atconfig/quality/eslint-suppressions.json:112-116).Rendered for the issue's own history, after the change:
How it was found and tested
New test
tests/unit/gemini-web-tool-history-14368.test.ts, using the same import style astests/unit/gemini-web-tool-calling-7286.test.ts(the pure builder, no Playwright mock needed). Two cases: the issue's suggested regression #1 (call and result present, in order, tied by the call id, with the contract intact), and the single-turn shapes asserted as exact strings so the fix cannot change the case #7286 guards.Base:
release/v3.8.51at06f1df9d7(the repo default and highest activerelease/v*), developed in its own worktree under.claude/worktrees/per Hard Rule #19 with.husky/_copied in so the gates actually run.Deliberately not done
prepareToolMessages()(open-sse/translator/webTools.ts), shared by the other web-cookie executors (the feat(providers): tool calling for claude-web / gemini-web — extend the webTools emulation shim (#2820 never covered these two) #7286 test note counts eleven); changing it here would only affect gemini-web and would duplicate the contract's wording. The missing input the model needs is the transcript, and that is what this fixes.GeminiWebExecutor.execute()) drive a Playwright-backed browser path; this PR keeps to the pure-function regression that pins the actual defect. Happy to add the executor-level pair if a maintainer wants that coverage in this change.Migrations, feature flags, generated artifacts
A changelog fragment was added to this branch after I re-read CONTRIBUTING.md line 409: user-facing changes ship a file under the changelog directory rather than editing CHANGELOG.md, so this PR now carries one. It is the only addition beyond the fix and its test.
None. One exported pure function plus its test file.
CI-only validation still pending
npm run typecheck:core,npm run lint,npm run test:unit,npm run test:vitest, the coverage ratchet and the ecosystem jobs run on the PR; locally I ran every gemini-web suite plus the focused lint and format gates above.