mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
27d6f8e2a7
## Description Closes #1091. SmartCrusher's array compaction is lossless-first, but the **opaque-blob** substitution path emitted `<<ccr:HASH,string,KB>>` markers **unconditionally** — it did not honor `enable_ccr_marker` / `inject_retrieval_marker`, which gate only the lossy **row-drop** path. As the issue notes, the consequence was that *no configuration produced guaranteed-lossless, marker-free output*: any array with a single string cell over `opaque_min_bytes` (256B default) still emitted a CCR marker, forcing a retrieval round-trip for consumers that need verbatim output. **Root cause:** the row-drop path is gated (`crusher.rs` — `if dropped_count > 0 && self.config.enable_ccr_marker`), but opaque classification in `compaction/classifier.rs` keyed purely on byte length, with no reference to the flag, and both emit sites (`walker.rs`, `crusher.rs`) then produced a marker. **Fix:** thread the gate into classification. `ClassifyConfig` gains an `emit_opaque_markers` field (default `true`); when `false`, a long string is classified `Scalar` (kept verbatim) instead of `Opaque`, so no marker is emitted and nothing is written to the CCR store anywhere downstream. The flag is set from `enable_ccr_marker` at both `ClassifyConfig` construction sites in `crusher.rs`. > Design note: gating at the classifier (rather than at marker-emit time) is the single complete fix — it covers all three emit paths (walker inline-substitution, the crusher string path, and the compactor `OpaqueRef`→formatter path, which no longer has the original string by the time it formats). One consequence: with markers **off**, an array dominated by unique long-string cells now falls through to a conservative passthrough (`skip:unique_entities_no_signal`) instead of a lossy opaque table — still lossless and marker-free, which is the point of disabling markers. If you'd rather preserve structural table compaction with the blob inlined verbatim, that's a larger change at the emit + compactor layers; happy to take it that direction if preferred. Default behavior (`enable_ccr_marker=true`) is unchanged. ## 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 - `crates/headroom-core/src/transforms/smart_crusher/compaction/classifier.rs`: add `emit_opaque_markers: bool` (default `true`) to `ClassifyConfig`; in `classify_cell`, keep long strings `Scalar` when it is `false`. New unit test `long_string_stays_scalar_when_opaque_markers_disabled`. - `crates/headroom-core/src/transforms/smart_crusher/crusher.rs`: set `classify.emit_opaque_markers = config.enable_ccr_marker` at both `ClassifyConfig` construction sites (the `CompactConfig` builder and the standalone string path). - `tests/test_smart_crusher_toin_attachment.py`: regression test pinning both directions — markers ON ⇒ opaque marker present (input really triggers the path); markers OFF ⇒ no marker, blob verbatim. ## Testing - [x] Unit tests pass (`pytest`) - [x] Rust tests pass (`cargo test`) - [x] Linting passes (`ruff check .`, `cargo fmt --check`, `cargo clippy -- -D warnings`) - [ ] Type checking (`mypy headroom`) — N/A (no headroom/ Python source changed) - [x] New tests added ### Test Output ```text # Rust $ cargo test -p headroom-core --lib smart_crusher test result: ok. 319 passed; 0 failed (incl. new: ...classifier::tests::long_string_stays_scalar_when_opaque_markers_disabled ... ok) $ cargo fmt --check && cargo clippy --workspace -- -D warnings ok # Python (after `uv pip install -e .` to rebuild the Rust core) $ pytest tests/test_smart_crusher_toin_attachment.py tests/test_transforms/ tests/test_ccr_row_drop_store_bridge.py 289 passed, 35 skipped # Full suite is green except the 5 pre-existing caplog logging-isolation # flakes that are unrelated to this change and fixed separately in #1117. ``` ## Real Behavior Proof - Environment: macOS, Python 3.13.3, Rust core rebuilt via `uv pip install -e .`. - Exact command / steps: crush a 60-row array whose rows carry a distinct >256B `blob` string, with `inject_retrieval_marker` ON then OFF. - Observed result: with `inject_retrieval_marker` OFF (after this fix) the crushed output contains NO `<<ccr:` marker and the original `sentinel5_…` blob survives verbatim; before the fix the same input still emitted `<<ccr:…,string,407B>>` (the bug); with markers ON behavior is unchanged. Concretely: - markers ON → `strategy=lossless:table`, output contains `<<ccr:…,string,407B>>` (blob replaced). - markers OFF (before fix) → `lossless:table` **still emitted `<<ccr:…>>`** (the bug). - markers OFF (after fix) → no `<<ccr:` marker, the original `sentinel5_…` blob present verbatim. - Not tested: behavior under CI's sharded jobs specifically; fix is deterministic and config-gated. ## 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 - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective - [x] New and existing unit tests pass locally with my changes - [ ] I have updated the CHANGELOG.md — N/A 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|---|---|---|
| .. | ||
| headroom-core | ||
| headroom-parity | ||
| headroom-proxy | ||
| headroom-py | ||