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