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
2630 commits 410 branches 169 tags 142 MiB
Commit graph

5 commits

Author SHA1 Message Date
Abhay Singh
3bb02f8f75
fix(transforms/smart_crusher): don't crash on a tool call with a null function (#2232) 
## Description

A tool call whose `function` field is explicitly `null` crashes
SmartCrusher's per-request context extraction.

`_extract_context_from_messages` (called at the top of `apply()`) walks
recent assistant tool calls:

```python
for tc in msg.get("tool_calls", []):
    if isinstance(tc, dict):
        func = tc.get("function", {})
        args = func.get("arguments", "")
```

`dict.get("function", {})` only substitutes `{}` when the key is
**missing**. When the key is present but `null` — `{"id": "1", "type":
"function", "function": null}`, which clients emit for a partial or
streamed tool call — `func` is `None`, and `None.get("arguments")`
raises `AttributeError`. That propagates out of
`_extract_context_from_messages` and crashes `apply()` for the entire
request, so the request either errors or has to fail open to
uncompressed with a logged traceback.

The sibling `_build_tool_name_index` in the same file already guards
this exact shape with `(tc.get("function") or {})` — this call site just
wasn't updated to match.

## Fix

Use the same null-safe form:

```python
func = tc.get("function") or {}
```

`None` (and any other falsy value) now collapses to `{}`, the null tool
call contributes no context, and extraction continues to the next call.

Closes #

## 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

- `headroom/transforms/smart_crusher.py`: `tc.get("function", {})` →
`tc.get("function") or {}` in `_extract_context_from_messages`.
- `tests/test_transforms/test_smart_crusher_bugs.py`: new test asserting
a `{"function": null}` tool call doesn't crash extraction and later
calls are still read.
- `CHANGELOG.md`: Bug Fixes entry.

## Testing

- [ ] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [x] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [ ] Manual testing performed

### Test Output

```text
$ uvx ruff@0.15.17 check headroom/transforms/smart_crusher.py tests/test_transforms/test_smart_crusher_bugs.py
All checks passed!
$ uvx mypy@1.20.2 --ignore-missing-imports headroom/transforms/smart_crusher.py
Success: no issues found in 1 source file
```

## Real Behavior Proof

- Environment: Windows 11, Python 3.12, `uvx ruff@0.15.17` / `uvx
mypy@1.20.2`. A full `pytest` OOM-kills this box (ML stack import), so I
reproduced the extraction loop with a dependency-free script and left
the full pytest to CI.
- Exact command / steps: ran an assistant message with tool calls
`[{"function": null}, {"function": {"arguments": "keep-me"}}]` through
the OLD `get("function", {})` loop and the NEW `get("function") or {}`
loop.
- Observed result: OLD raises `AttributeError` on the null function; NEW
skips it and returns `"keep-me"` from the following call.
- Not tested: a live proxy request carrying a null-function tool call;
full local `pytest` deferred to CI (OOM).

## 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
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] New and existing unit tests pass locally with my changes
- [x] I have updated the CHANGELOG.md if applicable

## Additional Notes

The "unit tests pass locally" box is unchecked because the full suite
imports the ML stack, which I can't run here. The new test uses the
existing `_make_crusher` helper in
`tests/test_transforms/test_smart_crusher_bugs.py`, so it runs under the
normal CI pytest job; behaviour is additionally verified by the
standalone proof above.

---------

Co-authored-by: JerrettDavis <mxjerrett@gmail.com>
 2026-08-11 23:39:37 -05:00
Nadia Ujovich
7c93c50c2c
Marker-free lossless_only mode + gate opaque-blob CCR markers behind enable_ccr_marker (#1129) 
## Description

`enable_ccr_marker` only gated the **row-drop sentinel** path. The
**opaque-blob** path still emitted `<<ccr:HASH,kind,size>>` markers
unconditionally whenever a string cell exceeded `opaque_min_bytes`
(256), so **no configuration could produce a fully marker-free prompt**.
Any `<<ccr:>>` marker is a promise that the full payload lives in the
CCR store and must be fetched back via a retrieval tool call — there was
no way to get compression without that round-trip dependency.

**Rebased on #1130 (merged).** That PR fixed the opaque-blob gate at the
classifier (`ClassifyConfig.emit_opaque_markers`, driven by
`enable_ccr_marker`) and closed #1091. This branch originally carried
its own equivalent gating commit; that commit is now **redundant and has
been dropped** — `classifier.rs` here is identical to upstream. What
remains is the **net-new** work that is **not** in #1130:

- **Strict `lossless_only` mode** — keeps lossless tabular compaction,
but routes every path that would need a CCR marker (row-drop sentinel
**and** opaque-blob offload) to leave content uncompacted instead, so
output is always marker-free **and** byte-recoverable.
- **Python parity** — `lossless_only` exposed across both config
dataclasses, a `SmartCrusher` kwarg, and a per-call `crush(...,
lossless_only=)` override.
- **`HEADROOM_LOSSLESS_ONLY` env var** — wires the mode through to the
proxy runtime so real agents can use it.

The #1130 opaque gate is consumed here through a single centralized
helper (`opaque_markers_enabled() = enable_ccr_marker &&
!lossless_only`) used by **all four** `ClassifyConfig` construction
sites.

## Type of Change

- [x] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update
- [ ] Performance improvement
- [x] Code refactoring (no functional changes)

## Changes Made

- **`feat(smart_crusher)`** — Add `lossless_only` (default `false`):
keeps lossless tabular compaction but routes every marker-requiring path
(row-drop sentinel + opaque-blob offload) to leave content uncompacted
instead. Exposed across the Rust core, PyO3 bridge, both Python config
dataclasses, a `SmartCrusher` kwarg, a per-call `crush(...,
lossless_only=)` override, and `smart_crush_tool_output`. Includes a
`debug_assert` documenting the load-bearing invariant (a `lossless_only`
crusher must never reach the CCR store write).
- **`refactor(smart_crusher)`** — Extract
`SmartCrusherConfig::opaque_markers_enabled()` as the single source of
truth for `enable_ccr_marker && !lossless_only`, consumed by **all
four** `ClassifyConfig` sites: the compaction-stage builder,
`with_compaction_format`, the top-level `process_string` path (Rust
core), and the PyO3 `compact_document_json` document-compactor path. No
site derives the gate inline anymore, so they cannot drift.
- **`feat(proxy)`** — Expose the mode via `HEADROOM_LOSSLESS_ONLY`:
`ContentRouterConfig.smart_crusher_lossless_only` →
`_get_smart_crusher`; the proxy reads the env var and sets it on the
live router config. Previously reachable only via the Python API, never
through the proxy.

## Testing

- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check` on changed files)
- [ ] Type checking passes (`mypy headroom`) — not run (see Additional
Notes)
- [x] New tests added for new functionality
- [x] Manual testing performed (proxy env-var seam, end-to-end — see
Real Behavior Proof)

### Test Output

```text
### RUST  (cargo test -p headroom-core --lib smart_crusher)
test result: ok. 323 passed; 0 failed; 0 ignored; 0 measured; 516 filtered out

### PYTEST  (test_smart_crusher_bugs.py + test_agent_savings.py + test_smart_crusher_toin_attachment.py)
45 passed

### RUFF  (changed files)
All checks passed!

### FMT + CLIPPY  (cargo fmt --check && cargo clippy --workspace --lib)
clean — no warnings
```

New/affected tests: `enable_ccr_marker_false_suppresses_opaque_markers`,
`lossless_only_leaves_array_uncompacted_instead_of_dropping`,
`lossless_only_inlines_opaque_blobs_when_table_ships`,
`lossless_only_never_writes_to_ccr_store` (Rust);
`TestLosslessOnlyMode`,
`test_router_lossless_only_flag_reaches_crusher`,
`test_router_lossless_only_defaults_off` (Python). Coexists green with
#1130's `long_string_stays_scalar_when_opaque_markers_disabled` (Rust)
and `test_smart_crusher_toin_attachment.py` (Python). The Python
`TestOpaqueMarkerGate` from the dropped gating commit was removed as
redundant with #1130's coverage.

## Real Behavior Proof

### Proxy env-var seam — end-to-end (this revision)

The one path with no automated coverage was `server.py` reading
`HEADROOM_LOSSLESS_ONLY` from the environment and threading it into the
live router config. Verified end-to-end by instantiating the **real**
`HeadroomProxy`, pulling the `ContentRouter` out of its pipeline, and
crushing a 50-row array with >256B opaque cells through the real Rust
crusher:

| | `HEADROOM_LOSSLESS_ONLY=1` | env unset (default) |
|---|---|---|
| `crusher._lossless_only` | **True** | **False** |
| output contains `<<ccr:` | **No** (marker-free) | Yes (normal lossy) |
| byte-recoverable (round-trips to original JSON) | **Yes** | No (rows
offloaded) |

This confirms the full chain `os.environ["HEADROOM_LOSSLESS_ONLY"]` →
`server.py` → `ContentRouterConfig.smart_crusher_lossless_only` →
`content_router.py` → `crusher_config.lossless_only` → Rust crusher. The
default column proves strict mode genuinely changes behavior (not a
no-op) and that the default path is unchanged.

### Prior live-traffic run

- Environment: Headroom proxy in front of a real agent (Hermes) routed
to NVIDIA NIM (OpenAI-compatible upstream). Isolated config dir;
`OPENAI_TARGET_API_URL=https://integrate.api.nvidia.com/v1`,
`HEADROOM_LOSSLESS_ONLY=1`. Confirmed with `ss` that all LLM traffic
flowed agent → proxy → upstream with no direct bypass.
- Exact command / steps: Start the proxy with `python -m
headroom.proxy.server --host 127.0.0.1 --port 8787`; point the agent's
LLM base_url at `http://127.0.0.1:8787/v1`; run a real chat plus a
`search_files`-style task; read `/stats` and `/v1/retrieve/stats`; then
toggle `HEADROOM_LOSSLESS_ONLY` and repeat for the comparison.
- Observed result: With 150K+ tokens of real traffic processed,
`lossless_only` kept the CCR store empty (`entry_count: 0`) and emitted
zero markers. A synthetic before/after with opaque (>256B) cells
produced 12 `<<ccr:>>` markers in default mode and 0 under
`lossless_only`, with output round-tripping to the original JSON
structure.
- Not tested: A live `lossless_only`-vs-markers contrast on real agent
traffic. The SmartCrusher offload path never engaged on this agent's
tool outputs (`total_compressions: 0`; CCR store stayed at `entry_count:
0` even after a broad codebase search), and compression stayed marginal
(~0.2–0.4%) in both modes. The agent's tool results don't match the
crushable-array profile the offload paths target, so the marker path is
never exercised in that integration. Why SmartCrusher barely engages
with this agent's outputs is a separate integration question (output
format / routing / size thresholds), out of scope for this change.

## 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
(config docstrings updated in-tree; no separate docs)
- [x] My changes generate no new warnings
- [x] I have added tests that prove my feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] I have updated the CHANGELOG.md if applicable — N/A

## Additional Notes

- Rebased on top of merged #1130; the now-redundant opaque-blob gating
commit was dropped, so this PR is purely the `lossless_only` feature +
proxy wiring on top of #1130's gate.
- `mypy headroom` was not run in this environment; happy to add the
result if CI requires it.
- Default behavior is fully preserved: `enable_ccr_marker` defaults to
`true`, `lossless_only` defaults to `false`, and
`HEADROOM_LOSSLESS_ONLY` unset is a no-op.
 2026-06-23 12:52:15 -05:00
chopratejas
c765c53bf8 feat(rust): retire python smart_crusher, ship rust-only via pyo3 
Stage 3c.1b step 2 + cleanup. The python `SmartCrusher` (3669 lines)
is replaced by a thin pyo3-backed shim (~290 lines) that delegates
every byte to `headroom._core.SmartCrusher` (built from
`crates/headroom-py`, landed in the previous commit). There is no
python implementation and no env-var fallback — the wheel is a hard
import.

Why now: parity was already proven across 17 fixtures + the python-
side bridge test (1+17 in `test_smart_crusher_rust_parity.py`).
Keeping a shadow python impl behind a flag is a permanent maintenance
cost with no operational benefit. Stage 3c.1b deletes ~3380 lines of
python parser/scorer/analyzer/orchestrator code; the rust crate has
its own coverage (388 unit tests + property tests in headroom-core).

Surface preserved (drop-in for every production caller):
- `headroom.transforms.smart_crusher.SmartCrusher` — same class name,
  same `__init__(config, relevance_config, scorer, ccr_config)`
  signature (the latter three are accepted for source-compat and
  silently dropped — rust port keeps those subsystems disabled in
  Stage 3c.1, they re-attach in Stage 3c.2).
- `SmartCrusherConfig` and `CrushResult` dataclasses kept as python
  dataclasses (callers use `asdict()` / dataclass matching on them).
- `crush(content, query, bias)`, `_smart_crush_content(content, ...)`,
  `apply(messages, tokenizer, **kwargs)`, and
  `_extract_context_from_messages(messages)` all preserved.
- `smart_crush_tool_output(content, config, ccr_config)` thin wrapper.

The transform-protocol `apply()` orchestration stays python (message
walking, digest-marker insertion, token counting); only the per-
message compression call delegates to rust.

Removed:
- Python parser / planner / scorer / analyzer / classifier (~3380 lines).
- Internal helpers `_classify_array`, `_detect_sequential_pattern`,
  `_detect_rare_status_values`, `_detect_items_by_learned_semantics`,
  `_percentile_linear`, `_compute_k_split`, `_crush_number_array`,
  `_process_value`, etc. — rust crate has parallel coverage.
- `SmartAnalyzer`, `ArrayType`, `CompressionStrategy`,
  `extract_query_anchors` — internals; not used by any production
  caller (only tests probed them).

Tests deleted (probed deleted internals — same precedent as Stage 3b):
- `tests/test_transforms/test_smart_crusher.py` (40 tests)
- `tests/test_transforms/test_universal_json_crush.py` (45)
- `tests/test_transforms/test_anchor_selector.py` (49)
- `tests/test_toin_field_learning.py` (21)
- `tests/test_crushability.py` (20)

Tests trimmed (removed methods/classes that probe deferred subsystems
— scorer injection, CCR marker injection, TOIN feedback recording —
all of which re-attach in Stage 3c.2):
- `tests/test_transforms/test_smart_crusher_bugs.py`:
  TestNumberArraySchemaPreservation, TestStage3c1BugFixes.
- `tests/test_relevance.py`: 2 scorer-injection tests.
- `tests/test_ccr.py`: TestSmartCrusherCCRIntegration class +
  test_custom_marker_template.
- `tests/test_toin_integration.py`: TestTOINIntegration +
  TestStoreToTOINHash classes.
- `tests/test_critical_fixes.py`: TestSmartCrusherTOINIntegration +
  test_full_feedback_loop.
- `tests/test_acceptance.py::TestQueryAnchorExtraction`: dropped the
  `extract_query_anchors` probe; kept the end-to-end "Alice
  preserved" assertion.

Bug fixes from Stage 3c.1 (#1 percentile linear interp, #2 zero-
padded sequential, #3 rare-status pareto, #4 k-split overshoot) are
pinned by the rust crate and the parity fixtures
(`tests/parity/fixtures/smart_crusher/`).

Tests:
- 517 passed in the smart_crusher-adjacent file set
  (test_transforms/, test_relevance*, test_ccr, test_toin_integration,
  test_quality_retention, test_acceptance, test_critical_fixes).
- 18 in `test_smart_crusher_rust_parity.py` (1 sanity + 17 fixtures).
- 388 rust unit tests still green.

One stale-error-message regex in `test_relevance_extra.py` updated
from "requires sentence-transformers" → "requires fastembed".
 2026-04-27 00:52:21 -07:00
chopratejas
c829dfa539 fix(python+rust): smart_crusher bugs #1, #2, #3, #4 + sorted iteration 
Lockstep fixes for the four known bugs in headroom/transforms/smart_crusher.py
plus the field-iteration ordering parity fix. Both languages now agree
byte-for-byte on the affected code paths — prerequisite for parity
fixtures landing next.

Bug #1 — percentile off-by-one (Python line 2844 + Rust crushers.rs)
Replaces integer-division indexing with linear-interpolation
percentile (numpy "linear" method). New _percentile_linear helper
shared by both languages: index = q * (n - 1), interpolate between
floor and ceil.

Bug #2 — zero-padded string IDs misclassified as sequential
Track had_non_string_numeric flag; if every parseable value came
from a string (no actual int/float), return False (categorical, not
sequential). Pre-fix: int("001") loses zero-padding and fakes a
sequential pattern.

Bug #3 — rare-status detection cardinality cap
Cardinality cap raised from 10 to 50. Single-dominant check
replaced with Pareto top-K: smallest K such that top-K covers >=80%
of items. If K <= 5, items NOT in top-K are outliers. Catches
bimodal distributions like 60×INFO + 25×WARN + 15 distinct error
codes.

Bug #4 — k-split overshoot when k_total=1
Clamp after the floored fractions: k_first=min(k_first, k_total),
k_last=min(k_last, max(0, k_total - k_first)). No-op for the
common case k_total >= 2.

Field iteration ordering (Python line 1049)
`for key in all_keys` → `for key in sorted(all_keys)`. Set
iteration is non-deterministic across PYTHONHASHSEED; downstream
short-circuits in _select_strategy and _detect_pattern would pick
different fields between runs. Rust uses BTreeMap (sorted ASCII);
sorting Python locks both languages to the same iteration order.

Verification:
- 56 Python tests pass (51 existing + 5 new lockstep tests under
  TestStage3c1BugFixes class).
- 382 Rust tests pass (rust bug #1 documentation test replaced
  with two new "fixed behavior" tests).
- Clippy clean.

Status: all four bugs are now fixed in BOTH languages. Parity
fixtures can be recorded against post-fix Python and asserted
byte-equal against Rust. That's the next commit.
 2026-04-26 23:08:04 -07:00
chopratejas
14415dbbb5 Fix SmartCrusher bugs: schema violation, race condition, thread safety, recursion 
- Number array compression no longer mixes types (string summary was
  prepended to numeric array, violating schema-preserving guarantee).
  Statistics now go in the strategy string instead.
- Replace instance-level _current_field_semantics with threading.local()
  to prevent cross-thread contamination in concurrent crushes.
- Add lock to module-level _within_compressor lazy init (was unprotected).
- Add _MAX_PROCESS_DEPTH=50 guard to _process_value to prevent
  RecursionError on deeply nested JSON.
- Remove dead expression (unused stats.max_val - stats.min_val).
- Fix all UP038 isinstance(x, (A, B)) -> isinstance(x, A | B) across file.
- Add 11 regression tests covering all fixes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
 2026-04-14 21:16:13 -07:00