mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(smart_crusher): close audit watch list — CCR.enabled + custom scorer
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.
This commit is contained in:
chopratejas
parent
d0a4bc2d44
commit
6cceea0766
3 changed files with 98 additions and 28 deletions
|
|
@ -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 `<<ccr:HASH>>` 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 `<<ccr:HASH>>` 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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 "<<ccr:" in result.compressed
|
||||
|
||||
|
||||
def test_ccr_enabled_false_suppresses_markers_in_output():
|
||||
"""`CCRConfig.enabled=False` is the master kill-switch and must
|
||||
behave the same as `inject_retrieval_marker=False`: no marker text,
|
||||
no sentinel key, no CCR store write. The audit found this knob was
|
||||
silently ignored — both flags now collapse to the same Rust-side
|
||||
`enable_ccr_marker=False` gate.
|
||||
|
||||
Storing a payload under `enabled=False` would also be a surprise
|
||||
side effect: the user explicitly opted out of CCR.
|
||||
"""
|
||||
from headroom.config import CCRConfig
|
||||
|
||||
crusher = SmartCrusher(
|
||||
SmartCrusherConfig(),
|
||||
# Note: inject_retrieval_marker stays True — we want to prove
|
||||
# `enabled=False` alone is enough to suppress.
|
||||
ccr_config=CCRConfig(enabled=False, inject_retrieval_marker=True),
|
||||
)
|
||||
payload = _bigger_array(60)
|
||||
result = crusher.crush(payload, query="", bias=1.0)
|
||||
|
||||
if result.strategy == "passthrough":
|
||||
pytest.skip("payload didn't trigger compression — bump the size")
|
||||
|
||||
assert "<<ccr:" not in result.compressed, f"expected no marker, got: {result.compressed!r}"
|
||||
assert "_ccr_dropped" not in result.compressed
|
||||
|
||||
|
||||
# ─── Custom scorer / relevance_config override ─────────────────────────
|
||||
|
||||
|
||||
def test_custom_scorer_arg_raises_not_implemented():
|
||||
"""The Rust port doesn't support custom scorers yet. Silently
|
||||
dropping a user-supplied scorer would be a textbook silent
|
||||
fallback (the user's scoring logic gets ignored, compression
|
||||
looks fine but is wrong). Fail loud instead.
|
||||
"""
|
||||
|
||||
class FakeScorer:
|
||||
pass
|
||||
|
||||
with pytest.raises(NotImplementedError, match="relevance_config.*scorer"):
|
||||
SmartCrusher(SmartCrusherConfig(), scorer=FakeScorer())
|
||||
|
||||
|
||||
def test_custom_relevance_config_arg_raises_not_implemented():
|
||||
"""Same fail-loud contract for `relevance_config`."""
|
||||
with pytest.raises(NotImplementedError, match="relevance_config.*scorer"):
|
||||
SmartCrusher(SmartCrusherConfig(), relevance_config={"alpha": 0.7})
|
||||
|
||||
|
||||
def test_default_construction_still_works():
|
||||
"""Sanity: the audit fail-loud only triggers when the user passes
|
||||
one of the unsupported args. Default `SmartCrusher()` still
|
||||
constructs fine."""
|
||||
SmartCrusher(SmartCrusherConfig()) # no raise
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue