mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
08fce29b47
## Description `should_inject_ccr_tool` deferred CCR tool injection whenever `frozen_message_count > 0`. Because `tools` is the head of Anthropic's cache key, that dropped a tool which was already inside the provider-cached prefix and invalidated the whole prefix — in both directions (`0 → >0` removes it; `>0 → 0` on proxy restart, `/model` switch, lineage eviction or TTL lapse adds it back). On three days of local proxy logs the turns that flipped injection state carried **44.7% of all cache-write tokens at a 52.0% hit rate**, against 98.1% for non-flipping turns. The log signature is `cache_read` alternating between two values exactly 172 tokens apart — the 464-byte tool definition. This deletes the gate and calls `apply_session_sticky_ccr_tool` directly, which is **what `openai.py` already does** — the two handlers now have the same shape. Net −61 production lines, no new state, no new config flag. Fixes defect 1 of #2671. ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [x] Performance improvement ## Changes Made - `headroom/proxy/ccr_marker_policy.py` — deleted the `should_inject_ccr_tool` gate; `apply_session_sticky_ccr_tool` is now the single decision point. - `headroom/proxy/handlers/anthropic.py` — calls `apply_session_sticky_ccr_tool` directly, matching `openai.py`. - `headroom/proxy/helpers.py` — dropped the now-unused gate plumbing. - `tests/test_proxy_anthropic_cache_stability.py` — new test asserting the forwarded `tools` array is byte-identical across a `frozen 0 → >0` transition. - `tests/test_ccr_marker_policy.py` — removed the three unit tests that pinned the deleted decision (they encoded the defect). - `tests/test_proxy/test_ccr_frozen_prefix_coupling.py` — same unredeemable-marker intent, re-pinned at the sticky helper. - `tests/test_proxy/test_anthropic_ccr_deferred_injection.py` — autouse reset fixture for the process-global `SessionCcrTracker` (separate commit). - Formatting-only follow-up commit applying `ruff format` (pinned 0.15.17) to the two test files above. ### Why deleting the gate is safe `apply_session_sticky_ccr_tool` already holds the correct rule. Its four branches, in order: | # | condition | action | |---|---|---| | 1 | tool already in the incoming tool list (client/MCP pre-registered) | skip; the client's bytes win | | 2 | `session_id is None` (WS / pre-session) | per-turn flag drives it verbatim | | 3 | session has done CCR | always inject the recorded golden bytes | | 4 | fresh session, no compression this turn | **skip** | Branch 4 is the safety property: a session that has never compressed still gets no tool, so removing the gate cannot start injecting into non-CCR conversations. Branch 3 is what the gate was starving. `has_new_ccr_markers` still gates first-time injection, so markers replayed from the previously-forwarded prefix cannot trigger one. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [ ] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed The three deleted unit tests encoded the defect. Coverage moves to the property that actually matters and was previously untested: **the forwarded `tools` array must be byte-identical across a `frozen 0 → >0` transition.** That test asserts on the forwarded request body rather than on a policy function's return value; unit-testing the old policy in isolation is exactly what let a wrong-but-self-consistent decision pass. Verified failing on `upstream/main` with an assertion on the missing tool (not an `ImportError`, so it fails for the right reason). Full suite: same pre-existing unrelated failures as `upstream/main`, **zero new** (verified by running the whole suite on both revisions and diffing the failure sets). ### Test Output ```text $ uv run pytest tests/test_ccr_marker_policy.py \ tests/test_proxy/test_anthropic_ccr_deferred_injection.py \ tests/test_proxy/test_ccr_frozen_prefix_coupling.py \ tests/test_proxy_anthropic_cache_stability.py -q collected 48 items tests/test_ccr_marker_policy.py ..... [ 10%] tests/test_proxy/test_anthropic_ccr_deferred_injection.py .............. [ 39%] . [ 41%] tests/test_proxy/test_ccr_frozen_prefix_coupling.py .. [ 45%] tests/test_proxy_anthropic_cache_stability.py .......................... [100%] ======================= 48 passed, 2 warnings in 13.59s ======================== $ ruff check . All checks passed! $ ruff format --check . 1349 files already formatted ``` ## Real Behavior Proof - Environment: local macOS proxy serving live Claude Code traffic to the Anthropic API; baseline = 3 days of proxy logs on `upstream/main`, after = 5.5 hours with this change live. - Exact command / steps: ran the proxy with this branch built in, drove normal Claude Code sessions through it (including `/model` switches and proxy restarts, the two events that used to flip injection state), then parsed 235 real turns from the proxy logs with the same parser used for the baseline in #2671. - Observed result: flip turns fell from 177 (44.7% of all cache write) to 2 (1.7%); steady-state write share 1.192% → 0.867%; aggregate hit rate 86.75% → 89.21%; main conversation warm hit rate 98.1% → 97.70% (n=149). The 2 remaining "flips" have `cache_read == 0` — cold starts that the bucketing counts as a state change, not real flips. | metric | baseline | after | |---|---|---| | flip turns | 177, carrying 44.7% of all cache write | **2**, carrying **1.7%** | | main conv, warm | 98.1% | **97.70%** (n=149) | | steady-state write share | 1.192% | **0.867%** | | aggregate | 86.75% | **89.21%** | - Not tested: `mypy headroom` was not run locally for this body; the OpenAI handler path (unchanged by this PR); tracker state loss mid-session (see note below); and defect 2 of #2671 (the sub-call breakpoint), which is untouched and is now 54.9% of remaining cache write — that is why aggregate stays just under 90%. ## 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 - [ ] 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 - [x] New and existing unit tests pass locally with my changes - [x] I did **not** edit `CHANGELOG.md` ## Additional Notes **Pre-existing and unchanged here:** if the tracker loses state mid-session while the transcript still carries markers, branch 4 returns no tool and those markers are unredeemable. `upstream/main` has no recovery for that; this PR neither creates nor fixes it. See my comment on #2500, which adds a recovery path for the related dangling-reference case. **N/A checklist items:** no documentation changes — this removes an internal policy function with no user-facing surface. `mypy headroom` left unchecked because it was not run for this body; CI covers it. **Merge-order conflict with #2500 (please read before landing either):** this PR *deletes* `should_inject_ccr_tool`, which is the exact function #2500 extends with `transcript_requires_tool`. Whichever lands second needs a semantic rebase, not just a textual one — git will not flag it. If this PR lands first, #2500's recovery path should re-target `apply_session_sticky_ccr_tool` (the sticky helper now owns the decision alone) or the handler call site in `handlers/anthropic.py`. If #2500 lands first, the gate deletion here still applies but the `transcript_requires_tool` override needs to move with it. Happy to do the rebase either way — say which order you prefer.
72 lines
2.4 KiB
Python
72 lines
2.4 KiB
Python
from __future__ import annotations
|
|
|
|
from headroom.ccr.tool_injection import CCRToolInjector
|
|
from headroom.proxy.ccr_marker_policy import has_new_ccr_markers
|
|
|
|
|
|
def _hashes(*contents: str) -> list[str]:
|
|
injector = CCRToolInjector(
|
|
provider="anthropic",
|
|
inject_tool=False,
|
|
inject_system_instructions=False,
|
|
)
|
|
injector.scan_for_markers([{"role": "user", "content": content} for content in contents])
|
|
return injector.detected_hashes
|
|
|
|
|
|
def test_has_new_ccr_markers_filters_replayed_forwarded_markers() -> None:
|
|
marker = "[100 items compressed to 10. Retrieve more: hash=abc123def456abc123def456]"
|
|
|
|
assert (
|
|
has_new_ccr_markers(
|
|
current_detected_hashes=_hashes(marker),
|
|
previous_forwarded_messages=[{"role": "user", "content": marker}],
|
|
provider="anthropic",
|
|
)
|
|
is False
|
|
)
|
|
|
|
|
|
def test_source_line_span_marker_is_still_detected() -> None:
|
|
# The compressor annotates the count with a source-line span (#2586); the
|
|
# retrieval hash must still be extracted from the enriched marker.
|
|
marker = "[122 items compressed to 27 (from 5 source lines). Retrieve more: hash=c00eb437e5e5c00eb437e5e5]"
|
|
assert _hashes(marker) == ["c00eb437e5e5c00eb437e5e5"]
|
|
|
|
|
|
def test_has_new_ccr_markers_detects_hash_not_seen_in_previous_forward() -> None:
|
|
old = "[100 items compressed to 10. Retrieve more: hash=abc123def456abc123def456]"
|
|
new = "[50 items compressed to 5. Retrieve more: hash=deadbeefdeadbeefdeadbeef]"
|
|
|
|
assert (
|
|
has_new_ccr_markers(
|
|
current_detected_hashes=_hashes(old, new),
|
|
previous_forwarded_messages=[{"role": "user", "content": old}],
|
|
provider="anthropic",
|
|
)
|
|
is True
|
|
)
|
|
|
|
|
|
def test_has_new_ccr_markers_treats_missing_previous_forward_as_new() -> None:
|
|
marker = "[100 items compressed to 10. Retrieve more: hash=abc123def456abc123def456]"
|
|
|
|
assert (
|
|
has_new_ccr_markers(
|
|
current_detected_hashes=_hashes(marker),
|
|
previous_forwarded_messages=None,
|
|
provider="anthropic",
|
|
)
|
|
is True
|
|
)
|
|
|
|
|
|
def test_has_new_ccr_markers_returns_false_without_current_hashes() -> None:
|
|
assert (
|
|
has_new_ccr_markers(
|
|
current_detected_hashes=[],
|
|
previous_forwarded_messages=None,
|
|
provider="anthropic",
|
|
)
|
|
is False
|
|
)
|