Skip to content

fix(reflect): emit canonical OpenAI tool result messages#2766

Open
Jordan-Jarvis wants to merge 3 commits into
vectorize-io:mainfrom
Jordan-Jarvis:fix/upstream-reflect-canonical-tool-results-20260717
Open

fix(reflect): emit canonical OpenAI tool result messages#2766
Jordan-Jarvis wants to merge 3 commits into
vectorize-io:mainfrom
Jordan-Jarvis:fix/upstream-reflect-canonical-tool-results-20260717

Conversation

@Jordan-Jarvis

Copy link
Copy Markdown
Contributor

Summary

  • remove the noncanonical name field from Reflect role=tool messages
  • derive Gemini function-response names from the preceding assistant tool calls by tool_call_id
  • reject unknown tool-result IDs, duplicate IDs within a turn, and missing results
  • preserve call-ID reuse across completed turns

Why

OpenAI-compatible provider boundaries reject role=tool messages that include a name field. Gemini still requires a function name in its native FunctionResponse, so the provider adapter must derive that provider-specific field from the canonical assistant tool call instead of requiring Reflect to emit a noncanonical message shape.

Validation

  • Ruff formatting and lint passed for changed provider/Reflect files
  • 34 focused Gemini/Reflect tests passed
  • full two-file run produced 69 passing tests; three existing real-LLM fixtures could not initialize because no HINDSIGHT_API_LLM_API_KEY was configured locally
  • git diff --check passed

@Jordan-Jarvis

Copy link
Copy Markdown
Contributor Author

Additional validation from the corresponding integration branch:

  • the identical contract fix merged in Will-Augmented-Thought/hindsight as ebd1d38b04e7bcae9ddaa25db39fe16b6c6bd2b7;
  • exact-main image run 29560133277 completed successfully, including API/control-plane builds, security scanning, publication, and signature verification;
  • the broader source CI run completed 82 jobs without a failure before a newer concurrency run cancelled the remaining queued jobs.

The focused Gemini/Reflect coverage in this PR remains the direct behavioral proof: canonical OpenAI role=tool messages no longer carry name, while Gemini derives its native function name from the preceding assistant tool call by tool_call_id.

@benfrank241

Copy link
Copy Markdown
Member

Thanks @Jordan-Jarvis. Verified locally on the PR head:

  • tests/test_gemini_safety_settings.py + tests/test_reflect_agent.py: 69 passed (the 3 errors are the no-HINDSIGHT_API_LLM_API_KEY real-LLM fixtures, unrelated).
  • Blast radius is clean. Removing the noncanonical name from role=tool messages is safe across every provider: none required reading it (anthropic maps tool_call_idtool_use_id, claude_code keys off tool_call_id, and gemini now derives the name). The new hard subscripts in the Gemini adapter (msg["tool_call_id"], tc["id"], fn["name"]) are fed only by reflect/agent.py, which always emits them via _tool_call_to_dict, and reflect is the sole tool-loop caller of call_with_tools. So the change is correct and the failure cannot fire from the current caller.

Routing the merge to @nicoloboschi rather than merging directly, because this changes a provider-level message-handling contract in shared infra (gemini_llm.py): it adds five new raise ValueError invariants and switches from .get() defaults to bare subscripts, i.e. it tightens the Gemini adapter to fail-loud on any non-canonical tool pairing. That fail-loud policy is a design call worth your eye, and the actual user-facing payoff (OpenAI-compatible endpoints no longer 400 on the name field) is not directly exercised by a test that hits a real provider boundary.

No blocking concerns from me: the fix is correct, well-scoped, and lint/format clean per the PR + my run. Flagging it as a provider-contract review item. cc @nicoloboschi

@Jordan-Jarvis

Copy link
Copy Markdown
Contributor Author

Updated the branch to current upstream main and added direct OpenAI-compatible request-boundary coverage without increasing the test count.

The existing DeepSeek OpenAICompatibleLLM.call_with_tools request-capture test now asserts that the outgoing canonical tool result contains exactly role, tool_call_id, and content, and does not contain name. This covers the provider-boundary behavior called out in review while retaining the existing Reflect-producer and Gemini derivation coverage.

Validation on the updated head:

  • focused Ruff check and format check: passed
  • focused provider/Reflect suite: 77 passed
  • the three real-LLM fixtures requiring HINDSIGHT_API_LLM_API_KEY were excluded from the green focused run; an unfiltered run produced the expected 77 passes plus only those 3 configuration errors

@Jordan-Jarvis

Copy link
Copy Markdown
Contributor Author

@nicoloboschi following @benfrank241’s provider-contract review, the current head adds direct OpenAI-compatible request-boundary coverage for the canonical tool-result shape. The PR is clean, mergeable, and green; could you complete the provider-contract review when available?

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.

2 participants