From 1f18d5980972fc7b2091ca0be5318d06c4edfa79 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Sun, 21 Jun 2026 13:07:29 -0400 Subject: [PATCH] fix(proxy): preserve byte-faithful Anthropic tool forwarding (#1222) ## Description Anthropic tool sorting was rewriting `tools` lists even when canonical order was already present, which forced a mutation path that bypassed byte-faithful forwarding and reduced prefix-cache hit stability on repeated `/v1/messages` turns. Closes #1042 ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - changed Anthropic tool canonicalization so already-canonical tool arrays are not rewritten in both single-request and batch paths - preserved byte-faithful forwarding for no-op single-request canonical tool-order requests by avoiding unnecessary `body["tools"]` reassignment - kept canonicalization behavior intact for out-of-order tool arrays - added one true regression proof for the PRE_SEND empty-tools path, plus forward-coverage tests for canonical no-op and real-sort-mutation request bodies in `test_proxy_byte_faithful_forwarding.py` - updated `CHANGELOG.md` to document the prefix-cache fix ## Testing - [x] Unit tests pass (`uv run pytest tests/test_proxy_handler_helpers.py tests/test_proxy_byte_faithful_forwarding.py -x -v`) - [x] Linting passes (`uv run ruff check headroom/proxy/handlers/anthropic.py tests/test_proxy_handler_helpers.py tests/test_proxy_byte_faithful_forwarding.py`) - [ ] Type checking passes (`uv run mypy headroom`) or explain N/A truthfully - [x] New tests added for new functionality when applicable - [ ] Manual testing performed ### Test Output ```text uv run pytest tests/test_proxy_handler_helpers.py tests/test_proxy_byte_faithful_forwarding.py -x -v 56 passed in 12.42s uv run ruff check headroom/proxy/handlers/anthropic.py tests/test_proxy_handler_helpers.py tests/test_proxy_byte_faithful_forwarding.py All checks passed! uv run ruff format headroom/proxy/handlers/anthropic.py tests/test_proxy_handler_helpers.py tests/test_proxy_byte_faithful_forwarding.py --check 3 files already formatted ``` ## Real Behavior Proof - Environment: Windows, local proxy test environment, Anthropic request-forwarding path - Exact command / steps: post `/v1/messages` requests with canonical tool order and then intentionally unsorted tool order through the `TestClient` path with the no-optimize app variant - Observed result: the PRE_SEND empty-tools runtime path now keeps `body_mutated` false where base would mark the request mutated, canonical-order tool payloads still preserve exact inbound bytes end-to-end, and unsorted tool-order payloads are still canonicalized as expected. - Batch coverage: no dedicated runtime batch regression test was added; batch no-op/mutation correctness is addressed through the same compare-and-assign pattern on both batch canonical-sort call sites. - Not tested: full-suite behavior outside the touched Anthropic forwarding regression surface ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [ ] New and existing unit tests pass locally with my changes - [x] I have updated the CHANGELOG.md if applicable ## Screenshots (if applicable) Not applicable. ## Additional Notes The important boundary is not whether tool arrays can be sorted. The real contract is whether a no-op canonicalization should count as a mutation. This change keeps canonicalization for real reorder cases and restores byte-faithful forwarding for already-canonical requests. --- CHANGELOG.md | 1 + headroom/proxy/handlers/anthropic.py | 32 +++- tests/test_issue_728_empty_tools_injection.py | 32 ++++ tests/test_proxy_byte_faithful_forwarding.py | 151 +++++++++++++++++- 4 files changed, 205 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd8af08c0..3f0632809 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * **proxy:** route Codex OAuth image generation and edit requests through the ChatGPT Codex image backend, while preserving OpenAI API-key image passthrough ([#1215](https://github.com/chopratejas/headroom/pull/1215)). * **proxy:** enable SSO credential resolution in the native Bedrock route via the `aws-config` `sso` feature flag, making the credential chain match what `docs/bedrock.md` already documented ([#999](https://github.com/chopratejas/headroom/pull/999)). * **proxy:** route native Bedrock `/model/{id}/converse` requests to the upstream Converse endpoint instead of the hard-coded `/invoke` action — the non-streaming handler now resolves the action from the inbound path, matching the streaming handler ([#999](https://github.com/chopratejas/headroom/pull/999)). +* **proxy:** preserve byte-faithful `/v1/messages` forwarding when Anthropic tool arrays are already canonical, and only canonicalize-and-mutate tool lists when sorting changes ordering ([#1042](https://github.com/chopratejas/headroom/issues/1042)). * **ccr:** make retrieval store TTL configurable with `HEADROOM_CCR_TTL_SECONDS`, expose the effective TTL in `/v1/retrieve/stats`, and distinguish expired retrievals from missing hashes. * **proxy:** add native Bedrock `/model/{id}/converse-stream` route and forward it through the existing streaming EventStream/SSE pipeline. * **wrap (codex):** fix `headroom wrap codex` producing a `config.toml` with duplicate top-level `model_provider` / `openai_base_url` keys (TOML-spec error) when the user had already configured their own provider. The injector now rewrites pre-existing top-level `model_provider` and `openai_base_url` lines in place — the previous value is kept in a `# was: …` trailing comment — instead of unconditionally prepending a duplicate, so `codex` can start against the proxy. The pre-wrap snapshot mechanism continues to byte-for-byte restore the original file on `headroom unwrap codex`. diff --git a/headroom/proxy/handlers/anthropic.py b/headroom/proxy/handlers/anthropic.py index e930e96bf..19ac622fa 100644 --- a/headroom/proxy/handlers/anthropic.py +++ b/headroom/proxy/handlers/anthropic.py @@ -1671,8 +1671,11 @@ class AnthropicHandlerMixin: # Update body body["messages"] = optimized_messages if tools or _original_tools is not None: - tools = self._sort_tools_deterministically(tools) - body["tools"] = tools + sorted_tools = self._sort_tools_deterministically(tools) + if sorted_tools != tools: + tools = sorted_tools + if tools != _original_tools: + body["tools"] = tools presend_event = self.pipeline_extensions.emit( PipelineStage.PRE_SEND, @@ -1690,8 +1693,14 @@ class AnthropicHandlerMixin: optimized_messages = presend_event.messages body["messages"] = optimized_messages if presend_event.tools is not None: - tools = self._sort_tools_deterministically(presend_event.tools) - body["tools"] = tools + sorted_tools = self._sort_tools_deterministically(presend_event.tools) + if sorted_tools != presend_event.tools: + tools = sorted_tools + else: + tools = presend_event.tools + if tools or body.get("tools") is not None: + if tools != body.get("tools"): + body["tools"] = tools if presend_event.headers is not None: headers = presend_event.headers if presend_event.messages is not previous_presend_messages: @@ -2646,9 +2655,11 @@ class AnthropicHandlerMixin: custom_id = batch_req.get("custom_id", "") params = batch_req.get("params", {}) canonical_params = dict(params) - canonical_tools = canonical_params.get("tools") - if canonical_tools is not None: - canonical_params["tools"] = self._sort_tools_deterministically(canonical_tools) + original_tools = canonical_params.get("tools") + if original_tools is not None: + sorted_tools = self._sort_tools_deterministically(original_tools) + if sorted_tools != original_tools: + canonical_params["tools"] = sorted_tools messages = params.get("messages", []) original_messages = copy.deepcopy(messages) model = params.get("model", "unknown") @@ -2725,7 +2736,12 @@ class AnthropicHandlerMixin: # Create compressed batch request compressed_params = {**params, "messages": optimized_messages} if tools is not None: - compressed_params["tools"] = self._sort_tools_deterministically(tools) + sorted_tools = self._sort_tools_deterministically(tools) + if sorted_tools != tools: + tools = sorted_tools + if tools or original_tools is not None: + if tools != original_tools: + compressed_params["tools"] = tools compressed_requests.append( { "custom_id": custom_id, diff --git a/tests/test_issue_728_empty_tools_injection.py b/tests/test_issue_728_empty_tools_injection.py index fe08a9ed1..93b2d7f5f 100644 --- a/tests/test_issue_728_empty_tools_injection.py +++ b/tests/test_issue_728_empty_tools_injection.py @@ -16,6 +16,7 @@ from __future__ import annotations import pytest from headroom.ccr.tool_injection import CCR_TOOL_NAME +from headroom.proxy.handlers.anthropic import AnthropicHandlerMixin from headroom.proxy.helpers import ( _reset_session_ccr_tracker_for_test, apply_session_sticky_ccr_tool, @@ -39,6 +40,29 @@ def _should_set_body_tools(tools: list | None, original_tools: list | None) -> b return bool(tools or original_tools is not None) +def _sort_tools(tools: list | None) -> list | None: + return AnthropicHandlerMixin._sort_tools_deterministically(tools) + + +def _should_set_body_tools_after_sort(tools: list | None, original_tools: list | None) -> bool: + """Mirror fixed logic when candidate tools may already be sorted.""" + if not _should_set_body_tools(tools, original_tools): + return False + sorted_tools = _sort_tools(tools) + if sorted_tools != tools: + tools = sorted_tools + return tools != original_tools + + +def _legacy_should_set_body_tools_after_sort( + tools: list | None, original_tools: list | None +) -> bool: + """Older comparator that only wrote when sorting reordered.""" + if not _should_set_body_tools(tools, original_tools): + return False + return _sort_tools(tools) != tools + + class TestHandlerGuardCondition: """Verify the guard condition that decides whether to write body['tools'].""" @@ -78,6 +102,14 @@ class TestHandlerGuardCondition: assert _should_set_body_tools(tools_after_helpers, original_tools) + def test_sorted_replacement_reaches_body(self): + """A sorted replacement that differs from payload still needs to be written.""" + original_tools = [{"name": "zeta"}, {"name": "alpha"}] + tools_after_helpers = [{"name": "alpha"}, {"name": "zeta"}] + + assert not _legacy_should_set_body_tools_after_sort(tools_after_helpers, original_tools) + assert _should_set_body_tools_after_sort(tools_after_helpers, original_tools) + # --------------------------------------------------------------------------- # apply_session_sticky_ccr_tool behaviour with no existing tools diff --git a/tests/test_proxy_byte_faithful_forwarding.py b/tests/test_proxy_byte_faithful_forwarding.py index 3d20e6cc4..d8c0eb87e 100644 --- a/tests/test_proxy_byte_faithful_forwarding.py +++ b/tests/test_proxy_byte_faithful_forwarding.py @@ -25,11 +25,9 @@ from unittest.mock import AsyncMock import httpx import pytest - -pytest.importorskip("fastapi") - from fastapi.testclient import TestClient +from headroom.pipeline import PipelineStage from headroom.proxy.helpers import ( BodyMutationTracker, append_text_to_latest_user_chat_message, @@ -40,6 +38,8 @@ from headroom.proxy.helpers import ( ) from headroom.proxy.server import ProxyConfig, create_app +pytest.importorskip("fastapi") + # --------------------------------------------------------------------------- # Unit tests for serializer + tracker # --------------------------------------------------------------------------- @@ -301,6 +301,13 @@ class _FakePrefixTracker: return None +class _SortedEmptyToolsPreSendExtension: + def on_pipeline_event(self, event): # noqa: ANN001 + if event.stage is PipelineStage.PRE_SEND: + event.tools = [] + return None + + def _make_no_optimize_app() -> tuple[TestClient, _CapturingTransport]: """Boot a proxy with all transforms disabled and a capturing transport.""" config = ProxyConfig( @@ -411,6 +418,144 @@ def test_compression_off_numeric_precision_preserved() -> None: assert upstream == inbound_bytes +# Forward coverage only; the PRE_SEND case below is the base-fails proof for this fix. +def test_anthropic_tools_canonical_order_preserves_byte_faithful_request() -> None: + client, transport = _make_no_optimize_app() + inbound_dict = { + "model": "claude-sonnet-4-6", + "max_tokens": 64, + "messages": [{"role": "user", "content": "plan test"}], + "tools": [ + {"name": "alpha"}, + {"name": "zeta", "description": "later"}, + ], + } + inbound_bytes = serialize_body_canonical(inbound_dict) + + response = client.post( + "/v1/messages", + headers={ + "x-api-key": "test-key", + "anthropic-version": "2023-06-01", + "content-type": "application/json", + }, + content=inbound_bytes, + ) + assert response.status_code == 200, response.text + upstream = transport.captured_body or b"" + assert upstream == inbound_bytes, ( + f"Expected byte-faithful passthrough for canonical tools; upstream={upstream!r}" + ) + + +def test_anthropic_tools_unsorted_reordered_and_canonicalized() -> None: + client, transport = _make_no_optimize_app() + inbound_dict = { + "model": "claude-sonnet-4-6", + "max_tokens": 64, + "messages": [{"role": "user", "content": "plan test"}], + "tools": [ + {"name": "zeta", "description": "later"}, + {"name": "alpha"}, + ], + } + expected_dict = { + **inbound_dict, + "tools": [ + inbound_dict["tools"][1], + inbound_dict["tools"][0], + ], + } + inbound_bytes = serialize_body_canonical(inbound_dict) + expected_bytes = serialize_body_canonical(expected_dict) + + response = client.post( + "/v1/messages", + headers={ + "x-api-key": "test-key", + "anthropic-version": "2023-06-01", + "content-type": "application/json", + }, + content=inbound_bytes, + ) + assert response.status_code == 200, response.text + upstream = transport.captured_body or b"" + assert upstream == expected_bytes + assert upstream != inbound_bytes + + +def test_anthropic_presend_sorted_empty_tools_keeps_body_unmutated() -> None: + inbound_dict = { + "model": "claude-sonnet-4-6", + "max_tokens": 64, + "messages": [{"role": "user", "content": "plan test"}], + } + inbound_bytes = serialize_body_canonical(inbound_dict) + + config = ProxyConfig( + optimize=False, + cache_enabled=False, + rate_limit_enabled=False, + cost_tracking_enabled=False, + log_requests=False, + ccr_inject_tool=False, + ccr_handle_responses=False, + ccr_context_tracking=False, + image_optimize=False, + pipeline_extensions=[_SortedEmptyToolsPreSendExtension()], + discover_pipeline_extensions=False, + ) + app = create_app(config) + client = TestClient(app) + + captured: dict[str, object] = {} + + async def _fake_retry( + method: str, # noqa: ARG001 + url: str, # noqa: ARG001 + headers: dict[str, str], # noqa: ARG001 + body: dict[str, object], # noqa: ARG001 + body_mutated: bool, + mutation_reasons: list[str], + **kwargs: object, # noqa: ANN003 + ) -> httpx.Response: # noqa: ANN201 + captured["body_mutated"] = body_mutated + captured["mutation_reasons"] = mutation_reasons + captured["body"] = body + return httpx.Response( + 200, + json={ + "id": "msg_1", + "type": "message", + "role": "assistant", + "content": [{"type": "text", "text": "ok"}], + "usage": { + "input_tokens": 10, + "output_tokens": 3, + "cache_read_input_tokens": 0, + "cache_creation_input_tokens": 0, + }, + }, + ) + + app.state.proxy._retry_request = _fake_retry # type: ignore[assignment] + response = client.post( + "/v1/messages", + headers={ + "x-api-key": "test-key", + "anthropic-version": "2023-06-01", + "content-type": "application/json", + }, + content=inbound_bytes, + ) + assert response.status_code == 200, response.text + assert captured["body_mutated"] is False + assert captured["mutation_reasons"] == [] + forwarded = captured["body"] + assert isinstance(forwarded, dict) + assert "tools" not in forwarded + + def test_legacy_json_kwarg_mode_yields_drifted_bytes( monkeypatch: pytest.MonkeyPatch, ) -> None: