diff --git a/CHANGELOG.md b/CHANGELOG.md index 13911c1bb..99f6848b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,6 +102,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Bug Fixes +* **transforms/content-router:** stop a profile-derived `read_protection_window` kwarg from weakening an explicit `--protect-tool-results` guarantee. `ContentRouter.apply()` computes `read_protection_window` from `protect_recent_reads_fraction`, where `0.0` (the sentinel `--protect-tool-results` sets) means "protect all excluded-tool output regardless of conversation depth" per #1374's documented contract — but the method then unconditionally overwrote that window with a `read_protection_window` kwarg whenever one was present. `proxy_pipeline_kwargs()` supplies that kwarg on every request from the active `AgentSavingsProfile.protect_recent` (the default `coding` profile sets `protect_recent=2`), so in practice only the last 2 messages ever kept read-protection and older excluded-tool output silently fell through to lossy compression. The runtime kwarg may now only narrow the window when `protect_recent_reads_fraction > 0`; it can no longer shrink the "protect everything" guarantee set by `--protect-tool-results`. * **memory:** include the Ollama server URL in the embedder cache key so a second backend can't get an embedder bound to the wrong server. `_create_embedder` cached by `(backend, model)` only, but the Ollama embedder is constructed with `base_url=config.ollama_base_url`. Two configs in the same process that shared a backend and model but pointed at different Ollama servers (e.g. a per-project storage router) collided on one cache slot, so the second silently reused the first's embedder and embedded against the wrong host. The cache key now also includes `ollama_base_url`. * **tokenizers:** use `o200k_base` for the gpt-4.1 / gpt-4.5 / o4 families in `get_encoding_for_model`. `gpt-4.1*` and `gpt-4.5*` matched the broad `gpt-4` prefix and were encoded with `cl100k_base`, and `o4*` matched no prefix and fell through to the `cl100k_base` default — all three use `o200k_base`, so their token counts were computed with the wrong vocabulary. Added explicit `gpt-4.1`/`gpt-4.5` prefixes ahead of `gpt-4` and an `o4` prefix; `gpt-4` and `gpt-3.5` snapshots still resolve to `cl100k_base`. * **cache/ccr:** stop counting a successful eviction as a retrieval in the compression feedback learner, which inverted the learning signal. When an entry is evicted without ever being retrieved, `CompressionStore` emits a synthetic `retrieval_type="eviction_success"` event to mark that the compression was sufficient (the LLM never needed the original). `CompressionFeedback.record_retrieval` had no branch for it, so — because the type is not `"full"` — it was counted as a *search retrieval*, inflating the tool's `retrieval_rate`/`search_rate`. `get_compression_hints` reads a high retrieval rate as "compressing too aggressively" and backs off, so a compression that actually worked pushed the learner toward *less* compression (a standalone repro scores one successful eviction as a 100% retrieval rate). The event is now recognized and left out of the retrieval counters; the compression is still counted by `record_compression` at store time, so a never-retrieved entry correctly yields a low retrieval rate. Genuine retrievals are unaffected. diff --git a/headroom/transforms/content_router.py b/headroom/transforms/content_router.py index 132f5cb4c..22f06e520 100644 --- a/headroom/transforms/content_router.py +++ b/headroom/transforms/content_router.py @@ -3297,7 +3297,16 @@ class ContentRouter(Transform): else: read_protection_window = num_messages # 0.0 = protect all (old behavior) runtime_read_protection_window = kwargs.get("read_protection_window") - if runtime_read_protection_window is not None: + if ( + runtime_read_protection_window is not None + and self.config.protect_recent_reads_fraction > 0 + ): + # A profile-derived window may only narrow protection when the + # deployment hasn't explicitly opted into "protect everything" + # (protect_recent_reads_fraction == 0.0, set by --protect-tool-results). + # See #1374's documented contract: protected tool output must never + # lossy-compress "regardless of conversation depth" -- a per-request + # savings-profile kwarg must not silently weaken that. read_protection_window = max(0, int(runtime_read_protection_window)) # Adaptive compression ratio: scale with context pressure diff --git a/tests/test_content_router_exclude_tools.py b/tests/test_content_router_exclude_tools.py index de726fa74..e1fedc4ae 100644 --- a/tests/test_content_router_exclude_tools.py +++ b/tests/test_content_router_exclude_tools.py @@ -141,6 +141,79 @@ def test_bash_tool_result_passthrough_when_protected() -> None: assert "router:excluded:tool" in result.transforms_applied +# --------------------------------------------------------------------------- +# Test 4: protect_tool_results sentinel survives a profile-derived +# read_protection_window kwarg, even when the protected output is old +# --------------------------------------------------------------------------- + + +def test_protect_tool_results_survives_runtime_read_protection_window_kwarg() -> None: + """A profile-derived `read_protection_window` kwarg (e.g. from + AgentSavingsProfile.protect_recent=2, threaded in via + proxy_pipeline_kwargs()) must not shrink protection below what + protect_recent_reads_fraction == 0.0 (the --protect-tool-results + sentinel) already guarantees for the whole conversation. + + Regression test for the precedence bug: content_router.py used to apply + the runtime kwarg unconditionally, so a Bash tool_result more than + `read_protection_window` messages old fell through to lossy compression + even though --protect-tool-results promised it would never compress + "regardless of conversation depth" (see PR #1374).""" + pytest.importorskip("tiktoken") # needed for OpenAI tokenizer + + from headroom.providers import OpenAIProvider + from headroom.tokenizer import Tokenizer + + provider = OpenAIProvider() + token_counter = provider.get_token_counter("gpt-4o") + tokenizer = Tokenizer(token_counter, "gpt-4o") + + proxy = _build(protect_tool_results=frozenset({"Bash", "bash"}), mode="token") + router = _router(proxy) + + bash_output = "\n".join( + f"line {i}: some output from a bash command that is long enough to compress" + for i in range(80) + ) + messages: list[dict[str, object]] = [ + { + "role": "assistant", + "content": None, + "tool_calls": [ + { + "id": "call_bash_1", + "type": "function", + "function": {"name": "Bash", "arguments": "{}"}, + } + ], + }, + { + "role": "tool", + "tool_call_id": "call_bash_1", + "content": bash_output, + }, + ] + # Pad with enough intervening turns that the Bash tool_result above + # falls outside a read_protection_window=2 (it's ~9-10 messages from + # the end once padding is added). + for i in range(8): + messages.append({"role": "user", "content": f"follow-up turn {i}"}) + messages.append({"role": "assistant", "content": f"reply {i}"}) + + # Simulate the profile-derived kwarg the proxy threads into every + # request via proxy_pipeline_kwargs() (AgentSavingsProfile("coding") + # sets protect_recent=2). + result = router.apply(messages, tokenizer, read_protection_window=2) + + tool_msg = next(m for m in result.messages if m.get("tool_call_id") == "call_bash_1") + assert tool_msg["content"] == bash_output, ( + "Bash tool_result must stay verbatim: protect_recent_reads_fraction == 0.0 " + "(set by --protect-tool-results) must not be weakened by a profile-derived " + "read_protection_window kwarg" + ) + assert "router:excluded:tool" in result.transforms_applied + + # --------------------------------------------------------------------------- # Baseline: Bash NOT in DEFAULT_EXCLUDE_TOOLS (unchanged by this PR) # --------------------------------------------------------------------------- diff --git a/tests/test_transforms/test_content_router.py b/tests/test_transforms/test_content_router.py index 62fc9a75d..a3318d24a 100644 --- a/tests/test_transforms/test_content_router.py +++ b/tests/test_transforms/test_content_router.py @@ -1049,6 +1049,60 @@ class TestExcludeTools: assert "router:excluded:tool" not in result.transforms_applied + def test_protect_recent_reads_fraction_zero_overrides_runtime_window(self, tokenizer): + """protect_recent_reads_fraction == 0.0 (the --protect-tool-results + sentinel) means "protect all excluded-tool output forever". A + profile-derived read_protection_window kwarg must not be allowed to + shrink that back down -- regression test for the precedence bug + where the runtime kwarg unconditionally overrode this config-level + guarantee.""" + config = ContentRouterConfig( + min_section_tokens=10, + min_chars_for_block_compression=10, + exclude_tools={"Glob"}, + protect_recent_reads_fraction=0.0, + ) + router = ContentRouter(config) + + # Plain unstructured text (not grep/log/json-shaped) so + # _lossless_compact_excluded returns None and the router takes the + # bare "protect as before" branch, matching the tag this test + # asserts on. + old_tool_content = "\n".join( + f"line {i}: some output from a glob command that is long enough to compress" + for i in range(80) + ) + messages = [ + { + "role": "assistant", + "content": [ + { + "type": "tool_use", + "id": "toolu_glob_old", + "name": "Glob", + "input": {"pattern": "*.py"}, + } + ], + }, + { + "role": "user", + "content": [ + { + "type": "tool_result", + "tool_use_id": "toolu_glob_old", + "content": old_tool_content, + } + ], + }, + {"role": "assistant", "content": "ack"}, + {"role": "user", "content": "continue"}, + {"role": "assistant", "content": "ack"}, + ] + + result = router.apply(messages, tokenizer, read_protection_window=2) + + assert "router:excluded:tool" in result.transforms_applied + def test_mixed_excluded_and_non_excluded_tools(self, tokenizer): """Multiple tools in same conversation - only excluded ones pass through.""" config = ContentRouterConfig(