mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
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.
This commit is contained in:
parent
d24a3f8425
commit
838c5234a8
4 changed files with 70 additions and 1 deletions
|
|
@ -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 = (
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue