From 6cceea07666aa0d0400e1fe03391e4d2e11ff1bd Mon Sep 17 00:00:00 2001 From: chopratejas Date: Tue, 28 Apr 2026 17:33:56 -0700 Subject: [PATCH] =?UTF-8?q?fix(smart=5Fcrusher):=20close=20audit=20watch?= =?UTF-8?q?=20list=20=E2=80=94=20CCR.enabled=20+=20custom=20scorer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two audit close-outs: 1. `CCRConfig.enabled=False` is now honored end-to-end. Both `enabled` and `inject_retrieval_marker` collapse to the same Rust `enable_ccr_marker=False` gate — neither flag should leave an orphaned store entry, and silently storing under `enabled=False` would be a surprise side effect. One-line shim change; the Rust knob from the prior PR carries the load. 2. Custom `scorer` / `relevance_config` overrides now fail loud. The shim previously logged a WARNING and silently dropped the user's scoring function — a textbook silent fallback that makes compression "work" but with the wrong semantics. The shim now raises NotImplementedError when either arg is non-None. No production caller passes them; full plumbing arrives with Stage- 3c.2's relevance-crate Python bridge. Updates RUST_DEV.md so the audit table reflects the closed gaps. --- RUST_DEV.md | 6 +- headroom/transforms/smart_crusher.py | 62 ++++++++++++--------- tests/test_smart_crusher_toin_attachment.py | 58 +++++++++++++++++++ 3 files changed, 98 insertions(+), 28 deletions(-) diff --git a/RUST_DEV.md b/RUST_DEV.md index 1b7f992e3..aa6cf0753 100644 --- a/RUST_DEV.md +++ b/RUST_DEV.md @@ -172,8 +172,8 @@ so they don't regress further or get forgotten. | Subsystem | State | Tracked by | |---|---|---| | TOIN learning loop | **Re-attached 2026-04-28.** Shim's `crush()` and `_smart_crush_content()` now call `toin.record_compression()` after a real compression. Filtered on `strategy != "passthrough"` to ignore JSON re-canonicalization. Best-effort: TOIN failures are logged at debug level and don't break compression. | `tests/test_smart_crusher_toin_attachment.py` | -| CCR marker emission knob | **Honored end-to-end 2026-04-28.** New `enable_ccr_marker: bool` field on Rust `SmartCrusherConfig`; `crush_array` checks it before emitting the `<>` marker text and the CCR store write. Python shim flips it from `ccr_config.inject_retrieval_marker`. Scope: gates only the row-drop sentinel path; Stage-3c.2 opaque-string CCR substitutions still emit always (no Python equivalent, no production caller asks for suppression). | `tests/test_smart_crusher_toin_attachment.py` + `crates/headroom-core/.../crusher.rs::tests::enable_ccr_marker_*` | -| Custom relevance scorer | **Open gap.** `relevance_config` and `scorer` constructor args are accepted for source compatibility but the Rust default `HybridScorer` always runs. Shim now logs WARNING (previously debug). **Fix needed:** expose a Python-bridged scorer constructor surface from `crates/headroom-core/src/relevance/`. | Warning at `headroom/transforms/smart_crusher.py` | +| CCR marker emission knob | **Honored end-to-end 2026-04-28.** New `enable_ccr_marker: bool` field on Rust `SmartCrusherConfig`; `crush_array` checks it before emitting the `<>` marker text and the CCR store write. Python shim flips it from `ccr_config.enabled and ccr_config.inject_retrieval_marker` — both flags collapse to the same Rust gate, since storing payloads under either off-switch makes no sense. Scope: gates only the row-drop sentinel path; Stage-3c.2 opaque-string CCR substitutions still emit always (no Python equivalent, no production caller asks for suppression). | `tests/test_smart_crusher_toin_attachment.py` + `crates/headroom-core/.../crusher.rs::tests::enable_ccr_marker_*` | +| Custom relevance scorer | **Closed (fail-loud) 2026-04-28.** `relevance_config` and `scorer` constructor args remain in the signature for source compat, but the shim now raises `NotImplementedError` when either is non-None — silently dropping a user-supplied scorer is a textbook silent-fallback bug. Full plumbing waits on Stage-3c.2's relevance-crate Python bridge. | `tests/test_smart_crusher_toin_attachment.py::test_custom_*_arg_raises_not_implemented` | | Per-tool TOIN learning hook | **Re-attached partially.** `_smart_crush_content` accepts `tool_name` and now threads it into the TOIN record. The hook is best-effort — it improves `query_context` aggregation but doesn't drive per-tool overrides yet. | `tests/test_smart_crusher_toin_attachment.py::test_smart_crush_content_records_to_toin` | ### DiffCompressor @@ -185,7 +185,7 @@ so they don't regress further or get forgotten. ### Watch list (potential regressions, not yet audited) -- `CCRConfig.enabled=False` end-to-end behavior. Currently the Rust port has a CCR store that's controlled by builder selection, not by a config flag. Sometimes-disabled paths haven't been audited. +- `CCRConfig.enabled=False` end-to-end — **closed 2026-04-28**. Both `enabled=False` and `inject_retrieval_marker=False` collapse to the same Rust `enable_ccr_marker=False` gate (no marker, no store write). See the SmartCrusher table above. - `SmartCrusherConfig.use_feedback_hints=False` — config field is forwarded to Rust but its honoring inside the Rust crusher hasn't been verified against a parity fixture for the disabled path. When any item above changes, update both this section and the test file. The shim's docstring also references this section — keep them aligned. diff --git a/headroom/transforms/smart_crusher.py b/headroom/transforms/smart_crusher.py index 9bf09ef84..5bf768879 100644 --- a/headroom/transforms/smart_crusher.py +++ b/headroom/transforms/smart_crusher.py @@ -25,16 +25,19 @@ fallback. Build it locally with `scripts/build_rust_extension.sh` `strategy != "passthrough"` to ignore JSON re-canonicalization). The retired Python class did this inline; the bridge keeps the highest-traffic strategy fueling the learning loop. -- **CCR marker emission** — honored. `ccr_config.inject_retrieval_marker - =False` flips the Rust crusher's `enable_ccr_marker` field; the - lossy row-drop path then skips both the marker text and the store - write. Scope: gates only the row-drop sentinel path. Opaque-string - CCR substitutions (Stage 3c.2) still emit always — they have no - Python equivalent. -- **Custom relevance scorer / scorer override** — still not wired. - `relevance_config` and `scorer` constructor args are accepted for - source compat but the Rust default `HybridScorer` is always used. - The shim logs a WARNING when these are passed. See RUST_DEV.md. +- **CCR marker emission** — honored. Both `ccr_config.enabled=False` and + `ccr_config.inject_retrieval_marker=False` flip the Rust crusher's + `enable_ccr_marker` field; the lossy row-drop path then skips both + the marker text and the store write. Scope: gates only the row-drop + sentinel path. Opaque-string CCR substitutions (Stage 3c.2) still + emit always — they have no Python equivalent. +- **Custom relevance scorer / scorer override** — fails loud now. + `relevance_config` and `scorer` constructor args remain in the + signature for source compat, but the shim raises + `NotImplementedError` when either is non-None. Silently dropping + a user-supplied scorer is a silent-fallback bug we explicitly + refuse to ship; full plumbing lands with Stage 3c.2's relevance- + crate Python bridge. See RUST_DEV.md. """ from __future__ import annotations @@ -169,12 +172,13 @@ class SmartCrusher(Transform): self._with_compaction = with_compaction # CCR config is preserved on `self` for callers that read it - # back (`headroom.proxy.server` does). The Rust crusher honors - # `inject_retrieval_marker` end-to-end via its - # `enable_ccr_marker` config field — when False, the lossy - # row-drop path skips marker emission AND skips the CCR store + # back (`headroom.proxy.server` does). Both `enabled=False` and + # `inject_retrieval_marker=False` collapse to the Rust crusher's + # `enable_ccr_marker=False` gate — when either is off, the + # lossy row-drop path skips marker emission AND the CCR store # write (no point storing a payload nothing in the prompt can - # reference). + # reference; storing it under `enabled=False` would also be a + # surprise side effect the user explicitly disabled). # # Scope: gates ONLY the row-drop sentinel path. Stage-3c.2 # opaque-string CCR substitutions still emit always — they have @@ -185,16 +189,22 @@ class SmartCrusher(Transform): else: self._ccr_config = ccr_config - # `relevance_config` and `scorer` are accepted for source - # compatibility but currently dropped — Stage 3c.1 ships with - # the Rust default `HybridScorer`. Custom scorers re-attach in - # Stage 3c.2 when the relevance crate gains a Python-bridged - # constructor surface. + # `relevance_config` and `scorer` are kept in the signature for + # source compatibility, but the Rust port doesn't support + # overrides yet (it always uses `HybridScorer` from the + # relevance crate; the Python-bridged constructor surface + # arrives in Stage 3c.2). Silently dropping a user-supplied + # scorer would be a textbook silent fallback — if a caller + # depends on a custom scoring function and we ignore it, the + # compression they get back is wrong in a way they cannot see. + # Fail loud instead. See `feedback_no_silent_fallbacks.md`. if relevance_config is not None or scorer is not None: - logger.warning( - "SmartCrusher: custom relevance_config/scorer args are " - "currently ignored (Rust port uses default HybridScorer). " - "Tracked as a known regression in RUST_DEV.md." + raise NotImplementedError( + "SmartCrusher: custom `relevance_config` / `scorer` " + "overrides are not yet supported by the Rust-backed " + "implementation. Pass `None` to use the default " + "HybridScorer. Tracked in RUST_DEV.md; full support " + "lands with Stage 3c.2's relevance-crate Python bridge." ) # Lazy TOIN handle. Loaded on first compression that has items @@ -224,7 +234,9 @@ class SmartCrusher(Transform): first_fraction=cfg.first_fraction, last_fraction=cfg.last_fraction, relevance_threshold=0.3, - enable_ccr_marker=self._ccr_config.inject_retrieval_marker, + enable_ccr_marker=( + self._ccr_config.enabled and self._ccr_config.inject_retrieval_marker + ), ) # Default: lossless-first compaction (PR4). Lossless wins for # cleanly tabular input where it saves ≥ 30% bytes; otherwise diff --git a/tests/test_smart_crusher_toin_attachment.py b/tests/test_smart_crusher_toin_attachment.py index 9b7aaba47..3392654c8 100644 --- a/tests/test_smart_crusher_toin_attachment.py +++ b/tests/test_smart_crusher_toin_attachment.py @@ -216,3 +216,61 @@ def test_ccr_inject_marker_true_emits_markers_when_lossy(): # to drop and the marker to fire. if "lossy" in result.strategy or "row" in result.strategy.lower(): assert "<