From f216e430559759f51b53eb44e76e030e6a83c80a Mon Sep 17 00:00:00 2001 From: Kessy Similien Date: Mon, 22 Jun 2026 17:05:47 -1000 Subject: [PATCH] fix(mcp): report correct savings_percent in headroom_compress (#1106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `headroom_compress` reports `savings_percent` backwards. In `_compress_content`: ```python savings_pct = ( round((1 - result.compression_ratio) * 100, 1) if result.compression_ratio < 1.0 else 0 ) ``` `compression_ratio` is already the saved fraction (`CompressResult`: "0.0 = no savings, 1.0 = 100% removed"), so `1 - compression_ratio` gives the *retained* percentage instead. A no-op comes back as 100% and a real 71% reduction as 28.8%. The `else 0` branch also zeroes out a genuine 100% result. `_Stats.record_compression` a few lines up already does it the right way (`1 - output_tokens / input_tokens`), so this is just bringing the return value in line with that. Closes # (no existing issue — found while evaluating the tool; can file one if you'd rather track it) ## 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 - `headroom/ccr/mcp_server.py`: derive `savings_percent` from `output_tokens`/`input_tokens` like `record_compression` does, so `savings_percent` and `tokens_saved` can't disagree. - `tests/test_ccr_mcp_server.py`: regression test tying `savings_percent` to the token counts, including the no-op-isn't-100% case. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [x] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output Ran the relevant checks against the changed code (borrowed the prebuilt `_core.abi3.so` from the released wheel so the checkout could import the pipeline): ```text $ ruff check headroom/ccr/mcp_server.py tests/test_ccr_mcp_server.py All checks passed! $ mypy --ignore-missing-imports headroom/ccr/mcp_server.py Success: no issues found in 1 source file $ pytest tests/test_ccr_mcp_server.py -q .... [100%] 4 passed in 1.71s ``` I ran the ccr test module and lint/type checks on the changed files, not the whole repo suite (that needs a full Rust build) — CI covers the rest. ## Real Behavior Proof - Environment: macOS, Python 3.14, checkout + prebuilt `_core` from headroom 0.26.0 - Exact command / steps: ran `compress()` on three inputs (a 40-record JSON array, an incompressible string, repeated prose) and compared the old `round((1 - compression_ratio) * 100, 1)` against the token-derived value `(1 - comp/orig) * 100`. - Observed result: the old expression returns the retained %, so 0% saved is reported as 100% and a real 71.2% reduction as 28.8%; the new value matches actual savings in every case: ```text input orig comp actual old formula new formula array(40) 497 143 71.2% 28.8% 71.2% noop 14 14 0.0% 100.0% 0.0% prose 111 111 0.0% 100.0% 0.0% ``` - Not tested: the full repo test suite and the E2E workflows (need a complete Rust build / maintainer-approved CI); only the ccr test module and lint/type checks on the changed files were run locally. ## 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 or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] I have updated the CHANGELOG.md if applicable ## Additional Notes Docs/CHANGELOG left unchecked as N/A — no user-facing doc covers this field, though I'm happy to add a CHANGELOG line if you want one. Co-authored-by: Claude Opus 4.8 Co-authored-by: JerrettDavis --- headroom/ccr/mcp_server.py | 12 +++++++++--- tests/test_ccr_mcp_server.py | 24 ++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/headroom/ccr/mcp_server.py b/headroom/ccr/mcp_server.py index d4ef58f5b..d8a8c2ed3 100644 --- a/headroom/ccr/mcp_server.py +++ b/headroom/ccr/mcp_server.py @@ -391,9 +391,15 @@ class HeadroomMCPServer: ) self._stats.record_compression(input_tokens, output_tokens, strategy) - savings_pct = ( - round((1 - result.compression_ratio) * 100, 1) if result.compression_ratio < 1.0 else 0 - ) + # Percentage of tokens removed. Derive from the same token counts used + # for ``tokens_saved`` so all three fields agree — this mirrors the + # convention in ``_Stats.record_compression`` above. The previous + # ``(1 - result.compression_ratio)`` inverted the value: since + # ``compression_ratio`` is already the *saved* fraction (see + # ``CompressResult`` in headroom/compress.py — "0.0 = no savings, 1.0 = + # 100% removed"), the old expression reported the *retained* percentage, + # e.g. a no-op (0% saved) was reported as 100%. + savings_pct = round((1 - output_tokens / input_tokens) * 100, 1) if input_tokens > 0 else 0 return { "compressed": compressed_content, diff --git a/tests/test_ccr_mcp_server.py b/tests/test_ccr_mcp_server.py index 7ecb983d0..7a353b1e2 100644 --- a/tests/test_ccr_mcp_server.py +++ b/tests/test_ccr_mcp_server.py @@ -67,6 +67,30 @@ def test_mcp_retrieves_proxy_stored_content(fresh_store) -> None: assert result["original_content"] == original +def test_compress_savings_percent_tracks_token_counts(fresh_store) -> None: + """``savings_percent`` must be the *removed* percentage derived from the + token counts — never the retained percentage. Regression for the inversion + where ``(1 - compression_ratio)`` reported a no-op (0% saved) as 100%.""" + pytest.importorskip("mcp", reason="MCP SDK required") + server = mcp_server.HeadroomMCPServer(check_proxy=False) + + # Repetitive JSON array — the shape the engine actually compresses. + content = json.dumps([{"id": i, "status": "ok", "kind": "run"} for i in range(40)]) + result = server._compress_content(content) + + orig = result["original_tokens"] + comp = result["compressed_tokens"] + expected = round((1 - comp / orig) * 100, 1) if orig > 0 else 0 + + # Reported savings agrees with the token fields (and with tokens_saved). + assert result["savings_percent"] == expected + assert 0.0 <= result["savings_percent"] <= 100.0 + if result["tokens_saved"] == 0: + assert result["savings_percent"] == 0.0 # not inverted to 100 + else: + assert result["savings_percent"] > 0.0 + + def test_mcp_retrieve_with_nonmatching_query_returns_full_content(fresh_store) -> None: """A query that matches no item above the relevance floor must still return the stored entry (it exists and is unexpired) rather than the "Content not