From 838c5234a877d4cf96f9e914cfd39d6d6addb211 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Sun, 5 Jul 2026 17:03:33 -0400 Subject: [PATCH] fix(transforms): normalize diff compressor context (#1801) ## Description Unified diff content could skip compression when the router reached the DIFF strategy with no question context. `DiffCompressor.compress()` defaulted omitted context to an empty string, but explicit `None` still crossed into the Rust boundary and raised before any compression result could be produced. The router also had a DEBUG-only crash path because it measured `len(context)` before DIFF dispatch. This normalizes `None` at the router entry and at the DIFF wrapper boundary so direct and routed diff compression both send a string context to Rust. Closes #1798. ## 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 - Normalize `None` context to `""` before router debug logging and compression dispatch. - Normalize `None` context to `""` again before calling the Rust diff compressor. - Add regressions for explicit `None`, omitted context, non-empty context preservation, and DEBUG-enabled router DIFF dispatch. - Keep DIFF fallback behavior unchanged so patch-shaped content is not routed through a lossy fallback. ## Testing - [x] Unit tests pass (`uv run pytest tests/test_transforms/test_diff_compressor.py tests/test_transforms/test_content_router.py -q`) - [x] Linting passes (`uv run ruff check headroom/transforms/diff_compressor.py headroom/transforms/content_router.py tests/test_transforms/test_diff_compressor.py tests/test_transforms/test_content_router.py`) - [ ] Type checking passes (`uv run mypy headroom`) - [x] New tests added for new functionality when applicable - [x] Manual testing performed ### Test Output ```text uv run pytest tests/test_transforms/test_diff_compressor.py tests/test_transforms/test_content_router.py -q 86 passed in 3.09s uv run pytest tests/test_transforms/test_content_router.py -q 55 passed in 2.84s uv run ruff check headroom/transforms/diff_compressor.py headroom/transforms/content_router.py tests/test_transforms/test_diff_compressor.py tests/test_transforms/test_content_router.py All checks passed! ``` ## Real Behavior Proof - Environment: Windows, Python through the project `uv` environment. - Exact command / steps: run the new DIFF context regressions against base and head. - Observed result: base fails explicit `None` at the fake Rust boundary with `AssertionError: Rust diff compressor received None context`; head passes explicit `None`, omitted context, non-empty context, and DEBUG-enabled router dispatch. - Not tested: native Rust internals beyond the Python wrapper boundary. ## 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 - [x] 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 - [x] I have updated the CHANGELOG.md if applicable ## Additional Notes No changelog entry is needed for this narrow wrapper and router bug fix. Type checking was not part of the focused local validation for this Python-only change. --- headroom/transforms/content_router.py | 1 + headroom/transforms/diff_compressor.py | 2 +- tests/test_transforms/test_content_router.py | 18 +++++++ tests/test_transforms/test_diff_compressor.py | 50 +++++++++++++++++++ 4 files changed, 70 insertions(+), 1 deletion(-) diff --git a/headroom/transforms/content_router.py b/headroom/transforms/content_router.py index 4a4bbc8d0..a6c0ab411 100644 --- a/headroom/transforms/content_router.py +++ b/headroom/transforms/content_router.py @@ -1449,6 +1449,7 @@ class ContentRouter(Transform): Returns: RouterCompressionResult with compressed content and routing metadata. """ + context = context or "" debug_enabled = logger.isEnabledFor(logging.DEBUG) request_debug = ( { diff --git a/headroom/transforms/diff_compressor.py b/headroom/transforms/diff_compressor.py index 95885f651..4d1e2984e 100644 --- a/headroom/transforms/diff_compressor.py +++ b/headroom/transforms/diff_compressor.py @@ -103,7 +103,7 @@ class DiffCompressor: ) def compress(self, content: str, context: str = "") -> DiffCompressionResult: - r = self._rust.compress(content, context) + r = self._rust.compress(content, context or "") cache_key: str | None = r.cache_key if cache_key is not None: # Mirror log_compressor.py + search_compressor.py: when the diff --git a/tests/test_transforms/test_content_router.py b/tests/test_transforms/test_content_router.py index 7d53e584a..16c3e7f39 100644 --- a/tests/test_transforms/test_content_router.py +++ b/tests/test_transforms/test_content_router.py @@ -9,6 +9,7 @@ Comprehensive tests covering: """ import json +import logging from types import SimpleNamespace import pytest @@ -472,6 +473,23 @@ class TestContentRouter: assert result.original == content assert result.strategy_used is not None + def test_compress_diff_accepts_none_context_with_debug(self, router, caplog): + """None context is normalized before debug logging and compressor dispatch.""" + + class FakeDiffCompressor: + def compress(self, content, context): + assert context == "" + return SimpleNamespace(compressed="diff summary") + + diff = "diff --git a/file.py b/file.py\n@@ -1 +1 @@\n-old\n+new\n" + router._diff_compressor = FakeDiffCompressor() + + caplog.set_level(logging.DEBUG, logger="headroom.transforms.content_router") + result = router.compress(diff, context=None) + + assert result.compressed == "diff summary" + assert result.strategy_used == CompressionStrategy.DIFF + def test_name_property(self, router): """Router has correct name.""" assert router.name == "content_router" diff --git a/tests/test_transforms/test_diff_compressor.py b/tests/test_transforms/test_diff_compressor.py index 522434767..8490d1536 100644 --- a/tests/test_transforms/test_diff_compressor.py +++ b/tests/test_transforms/test_diff_compressor.py @@ -24,6 +24,30 @@ from headroom.transforms.diff_compressor import ( ) +def _fake_diff_result(compressed: str = "compressed") -> DiffCompressionResult: + return DiffCompressionResult( + compressed=compressed, + original_line_count=1, + compressed_line_count=1, + files_affected=1, + additions=0, + deletions=0, + hunks_kept=1, + hunks_removed=0, + ) + + +class _FakeRustDiffCompressor: + def __init__(self) -> None: + self.contexts: list[str] = [] + + def compress(self, content: str, context: str): + if context is None: + raise AssertionError("Rust diff compressor received None context") + self.contexts.append(context) + return _fake_diff_result(content) + + class TestContextReduction: """Tests for context line reduction.""" @@ -374,6 +398,32 @@ class TestEdgeCases: assert result.compressed is not None +class TestContextNormalization: + """Tests for the Python-to-Rust diff compressor boundary.""" + + def test_none_and_omitted_context_become_empty_string(self) -> None: + compressor = object.__new__(DiffCompressor) + fake_rust = _FakeRustDiffCompressor() + compressor._rust = fake_rust + + diff = "diff --git a/file.py b/file.py\n--- a/file.py\n+++ b/file.py\n" + + compressor.compress(diff, context=None) + compressor.compress(diff) + + assert fake_rust.contexts == ["", ""] + + def test_non_empty_context_passes_through_unchanged(self) -> None: + compressor = object.__new__(DiffCompressor) + fake_rust = _FakeRustDiffCompressor() + compressor._rust = fake_rust + + diff = "diff --git a/file.py b/file.py\n--- a/file.py\n+++ b/file.py\n" + compressor.compress(diff, context="question context") + + assert fake_rust.contexts == ["question context"] + + class TestConfigOptions: """Tests for configuration options."""