From 7ab83c5107b5183a652f566adeffc7a6ef16a8bc Mon Sep 17 00:00:00 2001 From: Ashish Patel Date: Tue, 14 Jul 2026 22:55:49 +0530 Subject: [PATCH] fix(router): stop protecting passing build/test output as error traces (#1740) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description ![Error output protection false-positive fix](https://raw.githubusercontent.com/ashishpatel26/headroom/fix/1696-error-protection-false-positive/.github/pr-images/issue-1696-error-protection-fix.svg) `content_has_strong_error_indicators()` (`headroom/transforms/error_detection.py`) protects any message/content-block from compression when it contains 2+ distinct indicator keywords (`error`, `fail`, `exception`, `traceback`, `fatal`, `panic`, `crash`). That heuristic false-positives on **passing** build/test tool output: `tsc`'s `"Found 0 errors"` plus a passing test run's `"0 failures"` trips both `error` and `fail` — 2 distinct hits — despite nothing failing. In a long JS/TS coding session this fired on nearly every request (confirmed against the `stats.json` attached to #1696), permanently protecting legitimate tool output from ever being compressed and explaining the reported 0.3% savings vs. the advertised 60-95%. Closes #1696 ## 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/transforms/error_detection.py`: strip common zero-result phrases before the 2-keyword scan in `content_has_strong_error_indicators()` — both `"N word"`/`"word N"` forms (`"0 errors"`, `"no failures"`) and `"label:value"`/`"label=value"` forms (`"Failures: 0"`, `"errors=0"`), covering `error(s)` and `fail`/`failed`/`failing`/`failure(s)` (the scan matches `fail` by substring, so all inflections needed covering). - `tests/test_error_detection.py` (new file — no prior coverage existed): 7 tests covering real error/traceback detection, single-keyword safety, `tsc`/`eslint` passing summaries, the `"0 failed"` regression a reviewer caught, label:value formats, and that a genuine second indicator elsewhere in the same blob still triggers protection. - `.github/pr-images/issue-1696-error-protection-fix.svg`: diagram explaining the mechanism (embedded above). ## Testing - [x] Unit tests pass (`pytest`) - [ ] Linting passes (`ruff check .`) — not run locally, CI `lint` check is green - [ ] Type checking passes (`mypy headroom`) — not run locally, CI is green - [x] New tests added for new functionality - [x] Manual testing performed (see Real Behavior Proof) ### Test Output ```text $ .venv/Scripts/python -m pytest tests/test_error_detection.py tests/test_transforms/test_content_router.py tests/test_transforms_content_router.py -q tests\test_error_detection.py ....... [ 7%] tests\test_transforms\test_content_router.py ........................... [ 34%] ........................... [ 62%] tests\test_transforms_content_router.py ................................ [ 94%] ..... [100%] ============================= 98 passed in 1.21s ============================== ``` ## Real Behavior Proof - Environment: Windows 11, Python 3.14.5, local venv, `headroom._core` built via `maturin develop --release` (not prebuilt in a fresh checkout) - Exact command / steps: ran the reporter's exact scenario patterns (`"Found 0 errors\nTests: 0 failures, 42 passed"`, eslint's `"0 problems (0 errors, 0 warnings)"`) through `content_has_strong_error_indicators()` directly, before and after the fix - Observed result: before → `True` (wrongly protected); after → `False` (correctly compressible). Real failure text (`Traceback... fatal error`) still returns `True` after the fix. - Not tested: have not reproduced the full KiloCode/proxy session end-to-end locally (no access to the reporter's actual traffic) — root cause was confirmed via the `stats.json` they attached to the issue, which shows `router:protected:error_output` firing on nearly every request in their session. ## 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 (N/A — internal heuristic, no user-facing docs reference it) - [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 (release-please generates this automatically from commit messages) ## Screenshots (if applicable) See the diagram embedded in Description above. ## Additional Notes **Investigation trail**: ruled out `protect_recent_reads_fraction` (proxy already overrides its `0.0` dataclass default to `0.3` in token mode, the default) before confirming the error-protection false-positive via the reporter's `stats.json`. **Response to review comments** (@AbelVM, @sparkbugz): prose mentions like `"Fix the errors in the code."` or `"console.error(...)"` contain only 1 distinct indicator keyword and were already safe under the pre-existing 2-keyword threshold — not something this PR changes. The broader concern about other CI summary formats is addressed above (label:value forms). A case like genuine prose that happens to mention *two* distinct keywords together (e.g. "there are errors and it failed") is a known limitation of a keyword-substring heuristic in general, predates this PR, and is out of scope here — downstream compressors (LogCompressor) still preserve real error lines even when a block isn't gate-protected, so the failure mode there is "slightly stricter than ideal," not data loss. **Response to @JerrettDavis's CHANGES_REQUESTED**: fixed in the follow-up commit — `"0 failed"` is now stripped (previously only `failing`/`failure(s)` were), with a regression test for the exact reproduction given. --- .../issue-1696-error-protection-fix.svg | 78 +++++++++++++++++++ headroom/transforms/error_detection.py | 32 +++++++- tests/test_error_detection.py | 58 ++++++++++++++ 3 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 .github/pr-images/issue-1696-error-protection-fix.svg create mode 100644 tests/test_error_detection.py diff --git a/.github/pr-images/issue-1696-error-protection-fix.svg b/.github/pr-images/issue-1696-error-protection-fix.svg new file mode 100644 index 000000000..c3d9915fd --- /dev/null +++ b/.github/pr-images/issue-1696-error-protection-fix.svg @@ -0,0 +1,78 @@ + + + Headroom: Error-Output Protection False-Positive Fix (#1696) + + + + + 1. THE GATE + content_router.py routes every message/content-block through + content_has_strong_error_indicators() before deciding whether + to compress it or protect it (pass through untouched). + Rule: if the text contains 2+ DISTINCT keywords from + [error, fail, exception, traceback, fatal, panic, crash] + the block is protected — assumed to be a real failure trace. + + + + + + 2. THE BUG + Passing build/test output mentions BOTH words without failing: + tsc: "Found 0 errors" + jest: "0 failures, 42 passed" + → 2 distinct keyword hits ("error" + "fail") on a CLEAN run + → wrongly protected, forever, from compression. + + + + + + 3. OBSERVED IMPACT (issue #1696 stats.json) + In a long multi-turn JS/TS coding session (KiloCode via headroom proxy), "router:protected:error_output" fired on + nearly every request. Combined with tool-output exclusion and small-block skips, only a sliver of tokens were ever + eligible for compression. + + 0.3% + actual + + 60-95% + expected + + + + + + 4. THE FIX + Strip common zero-result phrases before the keyword scan: + "0 error(s)", "no error(s)", "0 failing", + "0 failure(s)", "no failure(s)" + headroom/transforms/error_detection.py + content_has_strong_error_indicators() + → clean tool output no longer trips protection. + + + + + + 5. REAL FAILURES STILL PROTECTED + Traceback (most recent call last): + ... + ValueError: fatal error during load + "traceback" + "fatal" (+ "error") — 2+ distinct hits + outside a stripped zero-result phrase + → still protected, correctly. + + + + + + 6. TEST COVERAGE ADDED (tests/test_error_detection.py — none existed before) + ✓ real error/traceback text is still flagged ✓ single keyword mention is not flagged + ✓ passing tsc summary ("Found 0 errors") is not flagged ✓ passing eslint summary is not flagged + ✓ "0 errors" text does not mask a real second indicator elsewhere in the same blob + All 5 new tests + full existing content_router suite (86 tests total) pass. + + + Fixes GitHub issue #1696 · headroomlabs-ai/headroom + diff --git a/headroom/transforms/error_detection.py b/headroom/transforms/error_detection.py index 8b6a6cac6..545eef5cf 100644 --- a/headroom/transforms/error_detection.py +++ b/headroom/transforms/error_detection.py @@ -145,6 +145,28 @@ def content_has_error_indicators(text: str) -> bool: return bool(_rust_content_has_error_indicators(text)) +# Success-summary phrases from common build/test/lint tools that legitimately +# pair two indicator keywords (`error` + `fail`) while reporting a PASS, e.g. +# tsc's "Found 0 errors", jest's "0 failing" / "0 failures" / "0 failed", +# eslint's "0 problems (0 errors, 0 warnings)", or label:value summaries like +# "Failures: 0", "failed: 0", "Errors=0". Stripped before the keyword scan +# below so a clean JS/TS toolchain run doesn't get permanently protected from +# compression for the rest of a long coding session (issue #1696). +# +# `fail(?:ed|ing|ures?)?` covers fail/failed/failing/failure/failures — the +# keyword scan below matches the "fail" substring inside all of them, so the +# scrubber must strip all of them too, not just the forms literally named +# "failing"/"failure(s)". +_ZERO_RESULT_PATTERN = re.compile( + # "0 errors" / "no failed" — count-first forms. + r"\b(?:0|no)\s+(?:errors?|fail(?:ed|ing|ures?)?)\b" + # "Errors: 0" / "failed=0" — label:value / label=value forms used by + # other CI/test tools' summary lines. + r"|\b(?:errors?|fail(?:ed|ing|ures?)?)\s*[:=]\s*0\b", + re.IGNORECASE, +) + + def content_has_strong_error_indicators(text: str) -> bool: """Stricter triage for compression-protection gates. @@ -161,8 +183,16 @@ def content_has_strong_error_indicators(text: str) -> bool: ``crash``), while passing mentions rarely do. Misses here are safe — downstream compressors (LogCompressor) still preserve error lines. + + Before scanning, ``_ZERO_RESULT_PATTERN`` strips zero-result + summary phrases (``"0 errors"``, ``"failed: 0"``, ...) so a + passing build/test/lint run doesn't trip the two-keyword + threshold just because its PASS summary happens to mention both + "error" and "fail" at count zero (see issue #1696 — this was + firing on nearly every request in a long JS/TS coding session + and defeating compression almost entirely). """ - lowered = text.lower() + lowered = _ZERO_RESULT_PATTERN.sub(" ", text.lower()) hits = 0 for keyword in ERROR_INDICATOR_KEYWORDS: if keyword in lowered: diff --git a/tests/test_error_detection.py b/tests/test_error_detection.py new file mode 100644 index 000000000..2238e9244 --- /dev/null +++ b/tests/test_error_detection.py @@ -0,0 +1,58 @@ +"""Regression tests for the error/importance detection triage helpers.""" + +from __future__ import annotations + +from headroom.transforms.error_detection import content_has_strong_error_indicators + + +def test_real_error_output_is_detected() -> None: + text = "Traceback (most recent call last):\n ...\nValueError: fatal error during load" + assert content_has_strong_error_indicators(text) + + +def test_single_keyword_mention_is_not_flagged() -> None: + # Only one distinct indicator keyword ("error") — should not trip the + # two-keyword threshold. + text = 'Wrote error_handler.py with an "errors": [] field.' + assert not content_has_strong_error_indicators(text) + + +def test_tsc_passing_summary_is_not_flagged() -> None: + # Regression for issue #1696: a clean `tsc` run mentions both "error" + # and (via "0 failures" in a paired test run) "fail" while reporting + # success. Previously this tripped the two-keyword heuristic and got + # the message permanently protected from compression. + text = "Found 0 errors. Watching for file changes.\nTests: 0 failures, 42 passed" + assert not content_has_strong_error_indicators(text) + + +def test_eslint_passing_summary_is_not_flagged() -> None: + text = "0 problems (0 errors, 0 warnings)\nno failing tests" + assert not content_has_strong_error_indicators(text) + + +def test_zero_result_phrase_does_not_mask_a_real_second_error() -> None: + # "0 errors" is stripped, but a genuine second distinct indicator + # elsewhere in the same blob must still trigger protection. + text = "0 errors from linter, but the build crashed with a fatal signal" + assert content_has_strong_error_indicators(text) + + +def test_zero_failed_form_is_not_flagged() -> None: + # Reviewer regression (PR #1740): "0 failed" wasn't covered by the + # original pattern (only "failing"/"failure(s)"), so "failed" still + # contributed a "fail" keyword hit alongside "0 errors" and tripped + # the false positive this fix targets. + text = "Found 0 errors\nTests: 0 failed, 42 passed" + assert not content_has_strong_error_indicators(text) + + +def test_label_value_summary_formats_are_not_flagged() -> None: + # Broader CI summary formats (not just "N word" / "word N"): label:value + # and label=value pairs, in either error/fail order. + for text in ( + "Failures: 0, Errors: 0", + "failed: 0, errors: 0", + "Errors=0 Failures=0", + ): + assert not content_has_strong_error_indicators(text), text