akadmin/headroom
1
0
Fork 0
mirror of https://github.com/headroomlabs-ai/headroom.git synced 2026-08-27 14:17:10 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions
2696 commits 410 branches 169 tags 142 MiB
Commit graph

5 commits

Author SHA1 Message Date
Rod Boev
838c5234a8
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.
 2026-07-05 14:03:33 -07:00
chopratejas
f5f465418b feat(rust): retire python diff_compressor, ship rust-only via pyo3 
The python `DiffCompressor` is replaced by a thin pyo3-backed shim that
delegates to `headroom._core.DiffCompressor`. There is no python
implementation and no env-var fallback — the wheel is a hard import.

Why now: opt-in defaults don't drive python retirement. Byte-equal
parity was already proven across 27 fixtures (stage 3a); keeping a
shadow python impl behind a flag is a permanent maintenance cost with
no operational benefit. Stage 3b deletes ~700 lines of python parser /
scorer / formatter code; the rust crate has its own coverage.

Surface preserved:
- `headroom.transforms.diff_compressor.DiffCompressor` — same class
  name, same `__init__`, same `compress(content, context)` shape.
  Returns python `DiffCompressionResult` dataclasses so call sites that
  destructure with `asdict()` work unchanged.
- `DiffCompressorConfig` and `DiffCompressionResult` dataclasses kept.
- Sidecar `compress_with_stats(...)` exposes the rust-only
  `DiffCompressorStats` (per-file hunk drops, context lines trimmed,
  file_mode normalizations) for observability.

Removed:
- Python parser / scorer / formatter (~700 lines).
- Internal `DiffHunk` / `DiffFile` parser dataclasses (rust crate has
  parallel coverage).
- 12 tests that probed `_parse_diff` / `_score_hunks` or the deleted
  parser dataclasses. The 29 public-API tests in `test_diff_compressor.py`
  remain and now exercise the rust backend through the same import path.
- `HEADROOM_DIFF_COMPRESSOR_BACKEND` env var — no longer meaningful.

Build:
- `scripts/build_rust_extension.sh` runs `maturin develop` and symlinks
  the built `.so` into `headroom/` so `import headroom._core` resolves
  past the in-tree package shadowing the maturin overlay.
- `.gitignore` excludes the symlinks and allowlists the build script.

Tests:
- 544 transforms pass; 33 rust-vs-fixture parity tests guard the pyo3
  bridge against regressions (`tests/test_transforms/test_diff_compressor_rust_parity.py`).
- Mypy clean.
 2026-04-26 09:15:37 -07:00
chopratejas
48c13245c5 fix(diff): close ContentRouter routing gaps for merge diffs and long preambles 
User audit caught three gaps that prevented DiffCompressor from being
invoked even when the input was a real diff. These complement the four
emit-time bugs fixed in the previous commit — those fixes only kick in
once DiffCompressor receives the input. Without these gap fixes, real
merge-commit diffs and `git log -p` outputs with long commit messages
were misrouted away from DiffCompressor entirely.

# The three gaps (each fixed in Python; gap 3 also fixed in Rust)

1. Detector scan window was hardcoded to first 50 lines.
   `_try_detect_diff` in content_detector.py only inspected
   `content.split("\n")[:50]`. `git log -p` outputs commonly have
   commit messages longer than 50 lines (releases, squashed commits,
   bots), pushing the `diff --git` header out of the detection window.
   Result: input was returned with `content_type=PLAIN_TEXT` and routed
   to the text compressor, never reaching DiffCompressor. Fix: window
   widened to 500 lines.

2. Detector regex didn't recognize merge-commit headers.
   `_DIFF_HEADER_PATTERN` matched `diff --git`, `--- a/`, and the
   regular `@@ -A,B +C,D @@` hunk header. Merge-commit diffs from
   `git log -p` use `diff --combined <path>`, `diff --cc <path>`, and
   combined-diff hunk headers `@@@+`. The shared `--- a/` line still
   triggered the detector with low confidence, but only barely. Fix:
   extended the regex to recognize all four merge-shaped header forms.

3. DiffCompressor parser only matched `^diff --git`.
   Even after fixing detection, the parser's `_DIFF_GIT_PATTERN`
   wouldn't match `diff --combined` or `diff --cc`, so merge diffs
   reached DiffCompressor and were treated as one giant pre-diff blob —
   passed through unchanged after the previous PR's pre-diff
   preservation fix. Fix: added `_DIFF_COMBINED_PATTERN` and
   `_DIFF_CC_PATTERN`; `_parse_diff` starts a new file section on any
   of the three header forms. Mirrored in Rust as `is_diff_header`
   helper that checks all three regexes.

# Why this matters end-to-end

DiffCompressor's value comes from being routed to. Detection +
parser-level coverage are upstream of the compressor — without them,
the compressor never sees the input. The previous PR's four bug fixes
(rename, combined-diff hunks, no-newline marker, pre-diff content) are
correct and necessary, but for merge commits and long-preamble diffs,
they were only firing on the rare cases where the detector misclicked
into DiffCompressor anyway. With these three gaps closed, the
ContentRouter→DiffCompressor pipeline actually engages on:
- `git log -p` outputs of any commit-message length
- Merge-commit diffs (`diff --combined`, `diff --cc`)
- Combined-diff snippets (`@@@`+ hunk-only inputs)

# New fixtures (3 added to the existing 24)

- `066bc82…` — `diff --combined` merge diff (3-way)
- `5d950a94…` — `diff --cc` merge diff (alternate form)
- `66c86f64…` — long pre-diff content (60-line commit message)
  followed by a rename diff (exercises detector scan widening +
  pre-diff preservation in tandem)

Parity: total=27 matched=27 skipped=0 diffed=0.

# Tests

- Python: 4 new tests across 2 new test classes —
  `TestRoutingGapMergeDiffs` (combined / cc parser) and
  `TestRoutingGapDetectorScanWindow` (long preamble detection +
  combined-diff regex recognition).
- Rust: 2 new tests covering combined / cc parser sections.

# Verification

- 27/27 parity fixtures byte-equal.
- Python: 41/41 tests pass (was 37).
- Rust: 18/18 transforms tests; 62/62 workspace; 5/5 proptests.
- `cargo fmt --check` clean; `cargo clippy --workspace --all-targets
  -- -D warnings` clean.
 2026-04-26 09:13:08 -07:00
chopratejas
6d47a0cd00 fix(diff_compressor): four silent information-loss paths in Python AND Rust 
Audit caught four bugs that the byte-equal parity harness can't catch on
its own — both Python and Rust were faithfully emitting the buggy output.
Fixed in lockstep so parity is maintained while the underlying behavior
is now correct on inputs the existing 20 fixtures didn't exercise.

# The four bugs (each fixed in both Python and Rust)

1. Renames silently dropped from output. Parser captured `is_renamed=True`
   but the emitter never emitted ANY rename markers. Output of a rename
   looked exactly like a plain modification of the old path. Fix: capture
   `rename from` / `rename to` / `similarity index N%` / `dissimilarity
   index N%` / `copy from` / `copy to` lines in a new `rename_lines` field
   on `DiffFile`; emit them after `diff --git` in canonical git ordering.

2. Combined diff hunks (`@@@`) silently dropped. Hunk-header regex only
   matched `@@`, so 3-way merge hunks had `current_hunk` never set and
   ALL their content fell through to the no-op branch. Fix in Python:
   regex switched to `^(@@+) ... \1` (backreferences match any number of
   `@`s on each side). Fix in Rust: alternation over `@@`, `@@@`, `@@@@`
   since `regex` is RE2-based and rejects backreferences. n>3 octopus
   merges still fall through; rare in practice.

3. `\ No newline at end of file` markers can be context-trimmed away.
   Treated as ordinary "other" lines — if more than `max_context_lines`
   from a `+`/`-` change, dropped. Round-trip-breaking for patches; can
   change whether the trailing line has a newline. Fix: in
   `_reduce_context`, force-add any line starting with `\` to the keep
   set regardless of distance.

4. Pre-diff content silently dropped. Anything before the first `diff
   --git` — commit messages from `git log -p`, email headers from `git
   format-patch`, fork-and-rebase metadata — was discarded. Fix:
   `_parse_diff` now returns `(pre_diff_lines, files)`; `format_output`
   prepends pre-diff content verbatim when present.

# Hidden parity bug found during the work

`_compress_files` constructed a fresh `DiffFile` from the parsed one but
only copied a subset of the fields by name. The new `rename_lines` and
`original_*_line` fields were silently dropped here, so the parser
populated them correctly but the emitter saw an empty `rename_lines`
list. Caught by writing a real test instead of a smoke test — the smoke
test passed because it hit the no-diff-found short-circuit, not the
parser/emitter pipeline. Constructor now copies all fields explicitly.

# Parity status

- Existing 20 fixtures: still byte-equal between fixed Python and fixed
  Rust. None of them exercised the buggy paths.
- 4 NEW fixtures recorded against fixed Python, exercising each bug-fix
  path: rename, 3-way combined diff, `\ No newline` marker far from
  changes, pre-diff commit headers. All 4 byte-equal between Python and
  Rust.
- Parity harness: total=24 matched=24 skipped=0 diffed=0.

# Observability

Some normalizations remain parity-bound (file mode `100644` hardcode,
`Binary files differ` simplification). Those are surfaced in
`DiffCompressorStats::file_mode_normalizations` /
`binary_files_simplified` (Rust) and via `logger.warning` (Python's new
`_log_loss_signals` helper, called once per compress).

# Tests

- Python: 4 new test classes (11 tests) covering rename markers,
  combined diffs, no-newline preservation, pre-diff content. Edge case:
  no pre-diff content must NOT add a leading blank line.
- Rust: 4 new `bugfix_*` unit tests with the same scenarios.
- Existing Python tests calling `_parse_diff` directly were updated for
  the new `(pre_diff, files)` tuple return.

# Verification

- Python: 37/37 tests pass (was 26).
- Rust: 16/16 transforms tests; 60/60 workspace unit tests; 5/5
  proptests; 1/1 doctest.
- Parity: 24/24 byte-equal.
- `cargo fmt --check` clean; `cargo clippy --workspace --all-targets
  -- -D warnings` clean.
 2026-04-26 09:13:08 -07:00
chopratejas
5c740ea427 Add DiffCompressor and fix hnswlib SIGILL crash on CI 
DiffCompressor:
- Parse unified diff format and compress by reducing context lines
- Preserve file headers and all +/- change lines
- Score hunks by relevance (error keywords, query matches)
- Add summary line: [N files, +X -Y lines]
- Expected 30-50% savings on typical git diffs
- Wire into content router for CompressionStrategy.DIFF
- 30 tests covering parsing, compression, edge cases

hnswlib SIGILL fix:
- Move hnswlib import from module level to lazy loading
- hnswlib crashes with SIGILL (Illegal Instruction) on CPUs
  without AVX support, before Python can catch the error
- Now imports only when HNSWVectorIndex is actually used
- HNSW_AVAILABLE is checked lazily via __getattr__

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
 2026-01-31 08:09:55 -08:00