fix(xai-sanitize): deepcopy tools_for_api before in-place mutation (#27907)#34416
Merged
Conversation
…27907) The xAI tool-schema sanitizers (strip_slash_enum, strip_pattern_and_format) mutate their input in place — that's their documented contract. The two call sites (chat_completion_helpers.build_api_kwargs and the auxiliary client) were passing agent.tools straight through, so the first xAI request would permanently strip slash-containing enum constraints and pattern/format keywords from the per-agent tool registry. Effect: any subsequent non-xAI call from the same agent (auxiliary task routed to Anthropic, OpenRouter fallback, mid-session model switch) saw the already-stripped schema with no way for the user to notice from their config. Fix: deepcopy tools_for_api before sanitizing at both call sites. The slash-enum bug itself (xAI 400ing on enums with '/') was fixed earlier by #32443 (Nami4D) — that PR landed the strip but used the sanitizers directly without copying. This salvages #27907's correctness contribution (the deepcopy) while skipping its redundant parallel sanitizer (strip_xai_incompatible_enum_values is functionally equivalent to the existing strip_slash_enum) and its preflight- neutrality argument (we chose model-gated preflight in #32443). 3 new tests in tests/run_agent/test_run_agent_codex_responses.py: - strips_slash_enum_from_outgoing_request — outgoing kwargs has no slash-containing enum values (functional contract preserved). - does_not_mutate_agent_tools — headline #27907 regression. Snapshot agent.tools before build_api_kwargs, assert it survives intact after. Pre-fix this assertion would have caught the mutation. - is_idempotent_across_repeated_calls — three xAI requests in a row each strip cleanly AND don't progressively erode the source schema. 344/344 across tests/agent/test_auxiliary_client.py, tests/agent/transports/test_codex_transport.py, tests/run_agent/test_run_agent_codex_responses.py, and tests/tools/test_schema_sanitizer.py. Co-authored-by: Gabor Barany <barany.gabor@gmail.com>
Contributor
🔎 Lint report:
|
14 tasks
4 tasks
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
2 participants
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.
Summary
The xAI tool-schema sanitizers (
strip_slash_enum,strip_pattern_and_format) mutate their input in place — that's their documented contract. Two call sites (chat_completion_helpers.build_api_kwargs+ the auxiliary client) were passingagent.toolsstraight through, so the first xAI request permanently stripped slash-containing enum constraints andpattern/formatkeywords from the per-agent tool registry.Any subsequent non-xAI call from the same agent (auxiliary task routed to Anthropic, OpenRouter fallback, mid-session model switch) saw the already-stripped schema. Silent constraint loss with no way for the user to notice from their config.
Salvages the deepcopy correctness contribution from PR #27907 (@gbarany).
Background — what's already on main vs. what's missing
The slash-enum functional bug (xAI 400ing on enums with
/) was fixed earlier by PR #32443 (@Nami4D) — that landed thestrip_slash_enumintegration at the right boundaries, but used the sanitizers directly without copying. That left the mutation bug unaddressed.@gbarany's PR #27907 surfaced this in detail with a tested deepcopy fix. This salvage takes the correctness contribution and skips:
strip_xai_incompatible_enum_valueshelper (functionally redundant with the existingstrip_slash_enum).Changes
agent/chat_completion_helpers.py:587-617—_copy.deepcopy(tools_for_api)before the existingstrip_pattern_and_format+strip_slash_enumcalls.agent/auxiliary_client.py:704-722— same fix;list(tools)was only a shallow copy, the inner parameter dicts were still references.Validation
3 new tests in
tests/run_agent/test_run_agent_codex_responses.py:strips_slash_enum_from_outgoing_requestdoes_not_mutate_agent_toolsagent.toolsbefore, assert it survives intact after. Pre-fix this assertion would have caught the mutationis_idempotent_across_repeated_callsCredit
@gbarany did the careful empirical work on xAI's enum validator (mapping which values get rejected —
application/jsonrejected,text/*rejected,foo*baraccepted, etc.) AND identified the mutation issue. Salvage keeps their correctness fix and authorship; the parallel sanitizer + preflight-neutrality argument get dropped because #32443 already covered the same ground a different way.