fix(agent): strip connection-bound message ids on Copilot Responses replay - #32725
Closed
briandevans wants to merge 2 commits into
Closed
fix(agent): strip connection-bound message ids on Copilot Responses replay#32725briandevans wants to merge 2 commits into
briandevans wants to merge 2 commits into
Conversation
…eplay Hermes was permanently breaking multi-turn Copilot sessions whenever the backend "connection" rotated between turns. ``_chat_messages_to_responses_input`` replays prior assistant ``codex_message_items`` with their server-assigned ``id`` field intact. GitHub Copilot's ``/responses`` endpoint binds those ids to a backend connection that does not survive credential-pool rotation, gateway restart, or routine load-balancer churn — replaying a stale id returns ``HTTP 401 "input item ID does not belong to this connection"`` and poisons the session because every subsequent turn re-sends the same bad ids. Add an ``is_github_responses`` flag to ``_chat_messages_to_responses_input`` and wire it through ``ResponsesApiTransport``'s ``convert_messages`` and ``build_kwargs``. When the flag is set, the replay omits the connection- bound ``id`` while keeping ``content``, ``phase`` and ``status`` so prefix-cache hits and multi-turn coherence still work. Native Codex and xAI Responses keep the existing id-replay behaviour. Fixes NousResearch#32716.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds GitHub Copilot /responses compatibility by stripping connection-bound assistant message item IDs during replay to avoid 401 session poisoning.
Changes:
- Add
is_github_responsesflag plumbed through the Codex transport into_chat_messages_to_responses_input. - Strip replayed assistant message item
idfields whenis_github_responses=True, preservingphaseandcontent. - Add a unit test verifying ID stripping for GitHub Responses while keeping IDs for other transports.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/run_agent/test_run_agent_codex_responses.py | Adds regression test ensuring GitHub Responses replays omit connection-bound message item IDs. |
| agent/transports/codex.py | Plumbs is_github_responses through transport conversion/build kwargs into the adapter. |
| agent/codex_responses_adapter.py | Implements GitHub-specific stripping of id on replayed assistant message items and documents rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+274
to
+275
| are preserved so multi-turn coherence and prefix-cache opportunities | ||
| survive the strip. |
| return _chat_messages_to_responses_input( | ||
| messages, | ||
| is_xai_responses=bool(kwargs.get("is_xai_responses")), | ||
| is_github_responses=bool(kwargs.get("is_github_responses")), |
Comment on lines
+1855
to
+1858
| replay_copilot = next(item for item in items_copilot if item.get("type") == "message") | ||
| assert "id" not in replay_copilot, ( | ||
| "Copilot ``/responses`` replay must omit the connection-bound id" | ||
| ) |
alt-glitch
added
type/bug
- codex_responses_adapter: clarify docstring — the Copilot replay intentionally trades the id-keyed prefix-cache shortcut for session survivability, since that shortcut is the failure mechanism here. Multi-turn coherence still survives via replayed content/phase/status. - transports/codex: tighten ``is_xai_responses`` / ``is_github_responses`` kwarg handling in ``ResponsesApiTransport.convert_messages`` to a strict ``is True`` check. ``bool(kwargs.get(...))`` would treat any truthy string (e.g. an env-piped ``"false"``) as enabling the backend-specific replay branch and silently change semantics. - tests/run_agent/test_run_agent_codex_responses: replace the bare ``next(...)`` over the replayed message items with an explicit ``assert message_items`` + indexed read, so a regression that drops the message item entirely fails with a clear assertion message instead of an opaque ``StopIteration``.
Contributor
Author
|
@copilot All three findings addressed in commit 6b7ff1d:
|
Contributor
Author
|
Closing to keep the queue focused — 33 days idle with no maintainer pickup. Happy to reopen if the Copilot Responses replay id-stripping fix is still wanted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
When using the GitHub Copilot provider with a model that routes to the Responses API (e.g.
gpt-5.5), Hermes can permanently break a multi-turn session by replaying assistantcodex_message_itemsids that were minted under a different "connection." The Copilot backend rejects the request withHTTP 401 "input item ID does not belong to this connection". Every subsequent turn re-sends the same poisoned ids and gets the same 401 back, making the session unrecoverable.The bug is in
agent/codex_responses_adapter.py::_chat_messages_to_responses_input— when a prior assistant turn contained server-assigned message-item ids, we replay the full item including itsidfield. Copilot binds that id to a backend connection that does not survive credential-pool rotation, gateway restart, or routine GitHub-side load-balancer churn between turns.This PR adds an
is_github_responsesflag to_chat_messages_to_responses_inputand wires it throughResponsesApiTransport.convert_messagesandResponsesApiTransport.build_kwargs(the only two call sites). When the flag is set, the replay omits the connection-boundidwhile keepingcontent,phase, andstatusso prefix-cache hits and multi-turn coherence still work. Native Codex (is_github_responses=False, default) and xAI Responses keep the existing id-replay behaviour — those backends rely on the id for cache lookups and do not have the connection-binding constraint.Related Issue
Fixes #32716
Type of Change
Changes Made
agent/codex_responses_adapter.py— addis_github_responseskeyword argument to_chat_messages_to_responses_input; skip theidfield on replayedcodex_message_itemswhen the flag is set; document the connection-binding rationale.agent/transports/codex.py— forwardis_github_responsesto the adapter at both call sites (convert_messagesandbuild_kwargs). The transport already accepts the flag from upstream callers; this propagates it the last hop into the converter.tests/run_agent/test_run_agent_codex_responses.py— addtest_chat_messages_to_responses_input_strips_message_id_for_github_responsesexercising both the new flag (id stripped, phase/content preserved) and the default behaviour (id kept) so the regression guard catches future drift.How to Test
copilotprovider withgpt-5.5(anything matched by_should_use_copilot_responses_api), take a few multi-turn steps so the gateway captures assistantcodex_message_itemswith server ids, then trigger a connection rotation (credential-pool rotation, gateway restart with a differentghaccount, or organic load-balancer churn in a long session). The next/responsescall previously returned 401; with this fix the request is sent without the connection-bound id and succeeds._chat_messages_to_responses_inputandcodex_message_itemsregression tests including the new one).Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
docs/, docstrings) — docstring on_chat_messages_to_responses_inputupdated to explain the new flag and its rationalecli-config.yaml.exampleif I added/changed config keys — N/A (no new config keys)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/AScreenshots / Logs
The issue body has a complete trace and reproduction.
Related / Positioning
Distinct from prior Responses-id fixes:
store=False(reasoning items already strip unconditionally in this code path).No competing PR on this code path; closed-PR scan returned no prior briandevans PR on the Copilot connection-binding error.