Skip to content

fix(chat-completions): coerce integer tool_call id in non-streaming normalize_response - #58562

Open
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/chat-completions-poolside-int-toolcall-id
Open

fix(chat-completions): coerce integer tool_call id in non-streaming normalize_response#58562
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/chat-completions-poolside-int-toolcall-id

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

This is a sibling follow-up to #58502 (commit 55e7986)

What does this PR do?

Coerces an integer tool_call.id to str in ChatCompletionsTransport.normalize_response so a Poolside tool turn on the non-streaming path (stream-unsupported fallback via agent._disable_streaming, ACP/MoA-no-consumer callers, gateway/health-check probes) does not persist an int id.

Left uncoerced, the assistant message's tool_calls[].id becomes a synthetic _deterministic_call_id string (because _split_responses_tool_id guards isinstance(raw_id, str) and falls through on a non-str id), while the paired tool result stores the raw int via make_tool_result_message. The two diverge, and the next replay turn is rejected by the provider with HTTP 400 ("tool_call_id did not match a preceding tool_calls"). Poolside is a first-class CANONICAL_PROVIDERS entry (CLI/TUI/desktop picker), so this path is user-reachable.

Related Issue

Sibling follow-up to #58502 (commit 55e7986). No separate issue.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/transports/chat_completions.py: in normalize_response, coerce an integer tool_call.id to str before constructing ToolCall, mirroring the existing integer finish_reason coercion.
  • tests/agent/transports/test_chat_completions.py: add test_tool_call_integer_id_coerced_to_string regression test.

How to Test

  1. uv run --with pytest --with pytest-asyncio python3 -m pytest tests/agent/transports/test_chat_completions.py -q → 83 passed.
  2. Fail-before / pass-after: the new test asserts nr.tool_calls[0].id == "24" and isinstance(... , str). With the coercion reverted it fails (assert 24 == '24'); with the fix it passes.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/agent/transports/test_chat_completions.py -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (py3.9 parse-safety verified via ast.parse(feature_version=(3,9)))

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A (pure Python type coercion)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Sibling code paths that may need the same fix: the streaming path already coerces this (chat_completion_helpers.py, #58502); no other normalize path constructs a ToolCall from a raw provider id. No further widening needed.

Copilot AI review requested due to automatic review settings July 4, 2026 23:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a provider-boundary type mismatch in the non-streaming chat-completions transport by coercing tool_call.id from int to str during response normalization, preventing tool-call ID divergence between assistant tool calls and subsequent tool-result messages (which can trigger HTTP 400 replay failures).

Changes:

  • Coerce non-streaming tool_call.id values from int → str in ChatCompletionsTransport.normalize_response.
  • Add a regression test covering Poolside’s integer tool_call.id wire shape on the non-streaming path.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
agent/transports/chat_completions.py Coerces integer tool_call.id to str during normalization to keep tool-call IDs consistent for replay.
tests/agent/transports/test_chat_completions.py Adds a regression test asserting tool_call.id is normalized to a string when provided as an integer.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists labels Jul 4, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused follow-up. The premise remains valid on current main: ChatCompletionsTransport.normalize_response() still passes raw tc.id through at agent/transports/chat_completions.py:691, although ToolCall.id is canonically str | None (agent/transports/types.py:22-35).

The proposed coercion matches the already-shipped streaming handling in agent/chat_completion_helpers.py:2472-2488. It also prevents the verified downstream mismatch: assistant-message construction replaces non-string IDs with deterministic strings (agent/chat_completion_helpers.py:1247-1260), while tool execution writes tool_call.id directly into the result message (agent/tool_executor.py:1666). The focused regression test covers the non-streaming transport boundary.

The target block and neighboring test remain present on current main; this should be straightforward to salvage despite the stale branch.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit area/streaming Streaming responses: gateway delivery, provider wire labels Jul 15, 2026
…ormalize_response

Poolside returns an integer tool_call.id on the non-streaming
ChatCompletionsTransport.normalize_response path (stream-unsupported
fallback, ACP/MoA/gateway/health-check callers). ToolCall.id is typed
str | None and downstream id-pairing assumes str: build_assistant_message
routes a non-str id through _split_responses_tool_id, which rejects it and
substitutes a synthetic _deterministic_call_id string, while
make_tool_result_message stores the raw int as tool_call_id. The assistant
tool_calls[].id (synthetic str) then diverges from the paired tool result's
tool_call_id (int), and the next replay turn is rejected by the provider
with HTTP 400 ("tool_call_id did not match a preceding tool_calls").

Coerce int -> str for the tool_call.id, mirroring the integer finish_reason
coercion 25 lines above. Sibling follow-up to 55e7986, which coerced the
integer tool_call.id only in the streaming helper.
@briandevans
briandevans force-pushed the fix/chat-completions-poolside-int-toolcall-id branch from 5d68204 to 60c5319 Compare August 5, 2026 03:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants