mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(compress): don't mutate the caller's CompressConfig via kwargs (#2134)
## Description `compress(messages, config=my_cfg, protect_recent=0, target_ratio=0.2)` used to write those kwarg values onto the caller's `my_cfg` object — so a shared per-agent `CompressConfig` was silently rewritten every time a call passed a single override. The next call that did NOT override that field then saw the previous request's value instead of the original default. Copy the config once at entry with `dataclasses.replace` before applying kwarg overrides (and before the savings-profile pass, which also mutates in place). Existing behavior for callers that pass **only** kwargs, or **only** a config, is unchanged. Issue #2133 has the root-cause walkthrough. Closes #2133 ## 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/compress.py`: copy the incoming `CompressConfig` once at entry with `dataclasses.replace` before applying kwarg overrides, so the caller's object is no longer mutated. The savings-profile branch already did a defensive `replace(cfg)`; that copy is now hoisted up front so both the kwarg and profile paths share the same guarantee. - `tests/test_compress_api.py`: added `test_kwargs_do_not_mutate_caller_config`, which fails on unpatched `main` and passes on this branch, covering the previously broken kwarg leg. - `CHANGELOG.md`: noted the fix. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [ ] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output ```text $ uv run pytest tests/test_compress_api.py -q ................. [100%] 17 passed in 2.88s $ uv run ruff check headroom/compress.py tests/test_compress_api.py All checks passed! $ uv run ruff format --check headroom/compress.py tests/test_compress_api.py 2 files already formatted ``` ## Real Behavior Proof - Environment: macOS 25.4 (Darwin arm64), Python 3.12.13, `uv 0.11.28`, branch `fix/compress-mutates-caller-config`, model `claude-sonnet-4-5-20250929` used for token counting. - Exact command / steps: build `c = CompressConfig(protect_recent=4, target_ratio=0.8)`, call `compress(msgs, model="claude-sonnet-4-5-20250929", config=c, protect_recent=0, target_ratio=0.2)` on a 3000-char user message, then read `c.protect_recent` and `c.target_ratio` back (full snippet run via `uv run python <<'PY' ... PY` — see the code block below). - Observed result: before the patch, `c.protect_recent` became `0` and `c.target_ratio` became `0.2` (caller's config silently rewritten). After the patch, `c.protect_recent` stays `4` and `c.target_ratio` stays `0.8`; caller's config unchanged. `uv run pytest tests/test_compress_api.py` reports 17 passed including the new `test_kwargs_do_not_mutate_caller_config` case. - Not tested: end-to-end proxy path with `savings_profile` set (the pre-fix code already did a defensive `replace(cfg)` on that branch, so the profile leg was safe; this change hoists that copy up front and the added unit test covers the kwarg leg that was broken — I did not spin up the proxy to reconfirm the profile branch end-to-end). No concurrent-caller / threading regression test was added — the fix removes the mutation entirely which sidesteps the race, but there is no explicit multi-thread reproducer. ### Reproducer **Before the patch (unpatched `main`)** ```text before: protect_recent=4, target_ratio=0.8 after : protect_recent=0, target_ratio=0.2 # <-- caller's cfg silently rewritten caller's config MUTATED ``` **After the patch (this branch)** ```text $ uv run python <<'PY' from headroom.compress import compress, CompressConfig c = CompressConfig(protect_recent=4, target_ratio=0.8) print(f"before: protect_recent={c.protect_recent}, target_ratio={c.target_ratio}") msgs = [{"role":"user","content":"x"*3000}] compress(msgs, model="claude-sonnet-4-5-20250929", config=c, protect_recent=0, target_ratio=0.2) print(f"after : protect_recent={c.protect_recent}, target_ratio={c.target_ratio}") print("caller's config", "unchanged" if (c.protect_recent, c.target_ratio) == (4, 0.8) else "MUTATED") PY before: protect_recent=4, target_ratio=0.8 after : protect_recent=4, target_ratio=0.8 caller's config unchanged ``` ## 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: no user-facing doc covers the CompressConfig / kwargs contract; see Additional Notes --> - [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 - **Documentation checklist item** — left unchecked as N/A. The behavior being fixed is internal to `headroom.compress.compress()`; the mutation contract of `CompressConfig` + kwargs is not covered in any user-facing doc (`wiki/compression.md`, `wiki/text-compression.md`, `wiki/image-compression.md`, and `docs/content/docs/shared-context.mdx` document a different / higher-level API surface). The `CHANGELOG.md` entry is the appropriate place for this fix. - **`mypy headroom` checklist item** — left unchecked because I did not run it in this workflow; the change is a two-line refactor within a well-typed function and no signatures moved. - The prior body's `## Summary`, `## Test plan`, and `## Real behavior proof` sections were reorganized into the six template-required headings so the PR-governance check passes. All technical content (root-cause, before/after reproducer, and test output) is preserved above; no code changes were made in this update. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Tejas Chopra <chopratejas@gmail.com>
This commit is contained in:
parent
af7385a298
commit
ecdcf13f3f
3 changed files with 42 additions and 4 deletions
|
|
@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
## Unreleased
|
||||
|
||||
### Fixed
|
||||
- **compress:** stop mutating the caller's `CompressConfig`. `compress(config=my_cfg, protect_recent=0, target_ratio=0.2)` used to write those kwargs onto `my_cfg`, so a shared per-agent config was silently rewritten by every request that overrode a single option.
|
||||
- **paths:** reject `.`, `..`, and NUL as plugin names so `plugin_config_dir` / `plugin_workspace_dir` cannot resolve outside the `plugins/` sandbox. Previously `plugin_config_dir("..")` returned the entire config root and `plugin_workspace_dir("..")` returned the workspace root (savings ledger, memory DB, license cache, logs).
|
||||
- **backends/litellm:** drop tool names over 64 chars before calling Bedrock Converse (`send_message` and `stream_message`), instead of letting the whole request 401. The Bedrock Converse API hard-rejects any tool name past that length, and Claude Code includes every globally-added claude.ai MCP connector tool in every request, even ones the user hasn't enabled locally, so a single oversized connector name broke every call through this backend. Only the `bedrock` provider filters; other providers forward tool names unfiltered.
|
||||
- **memory:** annotate `_EMBEDDER_CACHE` as `dict[tuple[str, str, str], Embedder]` to match the 3-element key (backend, model, ollama_base_url). The stale 2-tuple annotation made `mypy headroom` fail on `main`, which broke the `lint` CI job on every open PR.
|
||||
|
|
|
|||
|
|
@ -202,14 +202,17 @@ def compress(
|
|||
if not messages or not optimize:
|
||||
return CompressResult(messages=messages)
|
||||
|
||||
# Build config from explicit config + kwargs
|
||||
cfg = config or CompressConfig()
|
||||
# Build config from explicit config + kwargs. ``replace(config)`` up front
|
||||
# so kwargs overrides and any savings-profile pass never mutate the
|
||||
# caller's long-lived ``CompressConfig`` — a shared per-agent config being
|
||||
# silently rewritten by every request that overrode a single option is the
|
||||
# scenario this guards against.
|
||||
cfg = replace(config) if config is not None else CompressConfig()
|
||||
config_fields = {f.name for f in cfg.__dataclass_fields__.values()}
|
||||
for key, value in kwargs.items():
|
||||
if key in config_fields:
|
||||
setattr(cfg, key, value)
|
||||
if cfg.savings_profile:
|
||||
cfg = replace(cfg)
|
||||
apply_agent_savings_profile(cfg, cfg.savings_profile)
|
||||
|
||||
pipeline = _get_pipeline()
|
||||
|
|
|
|||
|
|
@ -1,10 +1,11 @@
|
|||
"""Tests for the one-function compress() API and integrations."""
|
||||
|
||||
import json
|
||||
from dataclasses import replace as _dc_replace
|
||||
|
||||
import pytest
|
||||
|
||||
from headroom.compress import CompressResult, compress
|
||||
from headroom.compress import CompressConfig, CompressResult, compress
|
||||
from headroom.hooks import CompressionHooks
|
||||
|
||||
try:
|
||||
|
|
@ -97,6 +98,39 @@ class TestCompressFunction:
|
|||
assert result.messages is messages
|
||||
assert result.tokens_saved == 0
|
||||
|
||||
def test_kwargs_do_not_mutate_caller_config(self):
|
||||
"""kwargs must not smuggle their values onto the caller's CompressConfig.
|
||||
|
||||
Regression: ``compress`` did ``cfg = config or CompressConfig()`` and
|
||||
then ``setattr(cfg, key, value)`` for every matching kwarg — so a caller
|
||||
who passed ``config=my_cfg, protect_recent=0`` came back to find their
|
||||
long-lived ``my_cfg`` silently rewritten. A shared, per-agent config
|
||||
was corrupted by every request that overrode a single option.
|
||||
"""
|
||||
|
||||
big_data = json.dumps([{"id": i, "status": "active"} for i in range(200)])
|
||||
messages = [
|
||||
{"role": "user", "content": "analyze"},
|
||||
{"role": "tool", "content": big_data, "tool_call_id": "c1"},
|
||||
]
|
||||
cfg = CompressConfig(protect_recent=4, target_ratio=0.8)
|
||||
snapshot = _dc_replace(cfg)
|
||||
|
||||
compress(
|
||||
messages,
|
||||
model="claude-sonnet-4-5-20250929",
|
||||
config=cfg,
|
||||
protect_recent=0,
|
||||
target_ratio=0.2,
|
||||
)
|
||||
|
||||
assert cfg.protect_recent == snapshot.protect_recent, (
|
||||
"compress() mutated caller's config.protect_recent via kwargs"
|
||||
)
|
||||
assert cfg.target_ratio == snapshot.target_ratio, (
|
||||
"compress() mutated caller's config.target_ratio via kwargs"
|
||||
)
|
||||
|
||||
def test_with_custom_hooks(self):
|
||||
"""Hooks are called when provided."""
|
||||
calls = []
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue