mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(models): version-boundary longest-prefix match in ModelRegistry.get (#1658)
## Description
`ModelRegistry.get()` has a prefix fallback for versioned model ids. It
accepted
**any** registered name as a bare `str.startswith` prefix and returned
the
**first** match in dict-insertion order:
```python
for name, info in _MODELS.items():
if model_lower.startswith(name):
return info
```
Two concrete failures fall out of that:
- `gpt-4` is registered before `gpt-4-32k`, so `get("gpt-4-32k-0613")`
matches
`gpt-4` first and returns an **8192**-token window instead of
`gpt-4-32k`'s
**32768**.
- `gpt-4.1` / `gpt-4.5-preview` aren't registered, so they also match
`gpt-4`
and inherit its **8192**-token window — even though they're much larger,
distinct models.
`get_context_limit()` reads straight from `get()` (no LiteLLM fallback),
so both
cases make the proxy believe a nearly-empty context is almost full and
compress
far too aggressively — or reject — on requests that are actually small.
This is
silent: no error, just a wrong number driving every downstream
compression
decision for those models.
## Fix
The fallback now:
1. Only matches when the registered name ends at a **version boundary**
in the
query — the next character must be a separator (`-`, `/`, `:`, `@`, `_`)
— so
`gpt-4.1`'s `.` no longer matches `gpt-4` (it falls through to the
caller's
default instead of a wrong 8192).
2. Picks the **longest** qualifying name, so `gpt-4-32k-0613` →
`gpt-4-32k`.
Exact and alias lookups are unchanged, and boundary-separated variants
like
`gpt-4o-new-version` still resolve to `gpt-4o`.
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
## Changes Made
- `headroom/models/registry.py`: replace the first-match `startswith`
prefix loop in `ModelRegistry.get` with a
longest-prefix-at-a-version-boundary match.
- `tests/test_models.py`: add regression tests — `gpt-4-32k-0613` →
`gpt-4-32k` (32768), and `gpt-4.1`/`gpt-4.5-preview` no longer resolve
to gpt-4's 8192 window.
- `CHANGELOG.md`: Bug Fixes entry under Unreleased.
## Testing
- [x] New tests added for the fixed behavior (`tests/test_models.py`)
- [x] Linting passes (`ruff check`) and formatting is clean (`ruff
format --check`)
- [ ] Full `pytest` run deferred to CI — see Real Behavior Proof for why
I verify the logic with a dependency-free script locally.
```text
$ uv run ruff check headroom/models/registry.py tests/test_models.py
All checks passed!
$ uv run ruff format --check headroom/models/registry.py tests/test_models.py
2 files already formatted
```
## Real Behavior Proof
- Environment: Windows 11, Python 3.12.11, headroom built from this
branch (`uv sync --extra dev`). Importing `headroom` pulls in the
torch/transformers stack; a full `pytest` run exhausts memory and gets
OOM-killed on this box, so I verify the matching logic with a
dependency-free script (only stdlib) and leave the full pytest to CI.
- Exact command / steps: replicated the relevant `_MODELS` registration
order (`gpt-4o`, `gpt-4-turbo`, `gpt-4`, `gpt-4-32k`) and the new
longest-prefix-with-boundary loop in a standalone script (no `headroom`
import), then asserted the resolved context windows.
- Observed result: `gpt-4-32k-0613` resolves to 32768 (was 8192 under
first-match), `gpt-4.1`/`gpt-4.5-preview` fall through to the caller
default (no longer 8192), and `gpt-4o-new-version` / `gpt-4` /
`gpt-4-0613` resolve exactly as before:
```text
OK: gpt-4-32k-0613 -> 32768 (was 8192 under old first-prefix-wins)
OK: gpt-4.1 / gpt-4.5-preview -> default (not 8192)
OK: gpt-4o-new-version, gpt-4, gpt-4-0613 still resolve as before
REGISTRY LOGIC VERIFIED
```
- Not tested: I did not add explicit registry entries for
`gpt-4.1`/`gpt-4.5` (their real windows) — that's a data addition,
separate from this matching-logic fix; today they fall back to the
caller's default, which is honest for an unregistered model and strictly
better than the previous wrong 8192. Full local `pytest` deferred to CI
(OOM, per above).
## 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 — ran
lint + a standalone logic check; full pytest deferred to CI (local OOM,
disclosed above)
- [x] I have updated the CHANGELOG.md if applicable
## Additional Notes
- No new dependencies; pure logic change in one function plus tests.
- Found via a read-through of the registry while looking at how context
limits drive compression decisions.
This commit is contained in:
parent
48f06caca7
commit
b699bedf95
3 changed files with 40 additions and 4 deletions
|
|
@ -43,6 +43,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
* **memory/sync:** stop the Claude Code sync adapter from clobbering distinct memories that share a first line. `write_memories` derived each file name from the first line of the content only (`headroom_{slug}.md`), so two different DB memories whose first lines slugify identically wrote to the same file and the second silently overwrote the first — and because the loser never landed on disk, the next sync re-exported it, ping-ponging the pair forever. When the slug is already taken by a *different* memory (distinct `headroom_id`) the file name is now disambiguated with a content-hash suffix; an update to the same memory still rewrites its slug file in place, so existing file names are unchanged.
|
||||
* **cli/proxy:** honor `HEADROOM_MIN_TOKENS=0` / `HEADROOM_MAX_ITEMS=0`. The Click `proxy` command built these with `_get_env_int_optional(name) or 500`/`or 50`, so an explicit `0` — a legitimate value (`min_tokens_to_crush=0` means "crush every item") — was treated as falsy and silently replaced with the default. The `headroom proxy` argparse path already preserved `0` via `_get_env_int`, so the two entry points disagreed. The Click path now uses the same None-checking helper.
|
||||
* **proxy:** strip the inbound `Content-Encoding`/`Transfer-Encoding` request headers on the Anthropic `/v1/messages` and OpenAI `/v1/chat/completions` paths before forwarding upstream. `read_request_json_with_bytes` already decompresses the inbound body (zstd/gzip/deflate/br), so the bytes forwarded upstream are plain JSON — but these two handlers left the original `content-encoding` header in place, so a client (or an edge proxy like a Cloudflare Worker) that sent a compressed body got its request rejected with upstream HTTP 400 because the provider tried to decompress already-decoded JSON. The `/v1/responses` handler already carried this fix (#1542); it is now applied to the messages and chat paths too.
|
||||
* **models:** fix the model registry's prefix fallback silently returning the wrong context window. `ModelRegistry.get` accepted any registered name as a `str.startswith` prefix and returned the *first* match, so `gpt-4-32k-0613` resolved to `gpt-4` (8192) instead of `gpt-4-32k` (32768), and unregistered ids like `gpt-4.1`/`gpt-4.5` inherited `gpt-4`'s 8192-token window — making the proxy think a nearly-empty context was almost full and compress far too aggressively. The fallback now requires the registered name to end at a version boundary in the query (so `gpt-4.1` no longer matches `gpt-4`) and picks the longest qualifying name (so `gpt-4-32k-0613` → `gpt-4-32k`).
|
||||
* **proxy:** include the system prompt, tools, and the response-shaping request fields in the SemanticCache key. `_compute_key` hashed only `{model, messages}`, so two non-streaming requests with identical messages but a different top-level `system` prompt, tool set, sampling config, or output-shaping field collided on one key and the second caller was served the first's cached response — generated under different request semantics, in the default config (`cache_enabled` defaults on). The key now folds the request fields that shape generation — `temperature`/`top_p`/`top_k`/`max_tokens`/`stop`, plus OpenAI `tool_choice`/`response_format`/`parallel_tool_calls`/`seed`/`presence_penalty`/`frequency_penalty`/`logit_bias`/`n`/`logprobs`/`top_logprobs`/`reasoning_effort`/`verbosity`/`modalities` and Anthropic `thinking`/`tool_choice`/`output_config` — canonicalizing `system`/`tools` so a moved `cache_control` breakpoint does not fragment it, and the handlers snapshot the fields once at the cache read and reuse them at write so a body mutated by the pipeline cannot diverge the key. Non-streaming path only.
|
||||
* **learn (verbosity):** `--verbosity --apply --all` now aggregates the savings baseline across every project instead of overwriting it per project (last-project-wins), which previously left the output shaper with a tiny, unrepresentative baseline. The applied verbosity level comes from the project with the most samples ([#1288](https://github.com/headroomlabs-ai/headroom/pull/1288)).
|
||||
* **proxy/anthropic:** restore token-mode compression on continued Claude Code turns with a frozen prefix and deferred CCR tool injection. Token mode now runs request-side compression even when the client did not pre-register `headroom_retrieve`, relying on the existing marker-triggered injection override to keep emitted CCR markers redeemable ([#1487](https://github.com/headroomlabs-ai/headroom/issues/1487)).
|
||||
|
|
|
|||
|
|
@ -631,12 +631,29 @@ class ModelRegistry:
|
|||
if model_lower in _ALIASES:
|
||||
return _MODELS[_ALIASES[model_lower]]
|
||||
|
||||
# Prefix matching
|
||||
# Prefix matching. Two rules keep this from silently mis-resolving
|
||||
# newer or larger variants:
|
||||
# 1. The registered name must end at a version boundary in the query
|
||||
# (next char is a separator like ``-``/``/``/``:``/``@``/``_``), so
|
||||
# ``gpt-4.1`` — a distinct model, not a variant of ``gpt-4`` — does
|
||||
# not inherit gpt-4's 8192-token window (the ``.`` is not a
|
||||
# boundary, so it no longer matches).
|
||||
# 2. When several names qualify, the LONGEST wins, so
|
||||
# ``gpt-4-32k-0613`` resolves to ``gpt-4-32k`` (32768) rather than
|
||||
# the shorter ``gpt-4`` (8192) that happens to be registered first.
|
||||
best: ModelInfo | None = None
|
||||
best_len = -1
|
||||
for name, info in _MODELS.items():
|
||||
if model_lower.startswith(name):
|
||||
return info
|
||||
if not model_lower.startswith(name):
|
||||
continue
|
||||
rest = model_lower[len(name) :]
|
||||
if rest and rest[0] not in ("-", "/", ":", "@", "_"):
|
||||
continue
|
||||
if len(name) > best_len:
|
||||
best = info
|
||||
best_len = len(name)
|
||||
|
||||
return None
|
||||
return best
|
||||
|
||||
@classmethod
|
||||
def resolve(
|
||||
|
|
|
|||
|
|
@ -90,6 +90,24 @@ class TestModelRegistry:
|
|||
assert info is not None
|
||||
assert info.name == "gpt-4o"
|
||||
|
||||
def test_get_prefix_matching_prefers_longest_registered_name(self):
|
||||
"""`gpt-4-32k-0613` must resolve to `gpt-4-32k` (32768), not the
|
||||
shorter `gpt-4` (8192) that is registered first."""
|
||||
info = ModelRegistry.get("gpt-4-32k-0613")
|
||||
assert info is not None
|
||||
assert info.name == "gpt-4-32k"
|
||||
assert ModelRegistry.get_context_limit("gpt-4-32k-0613") == 32768
|
||||
|
||||
def test_get_prefix_matching_requires_version_boundary(self):
|
||||
"""`gpt-4.1`/`gpt-4.5` are distinct models, not variants of `gpt-4`.
|
||||
A `.`-separated suffix must not match `gpt-4`, so they no longer
|
||||
inherit gpt-4's 8192-token window (they fall back to the default)."""
|
||||
assert ModelRegistry.get("gpt-4.1") is None
|
||||
assert ModelRegistry.get("gpt-4.5-preview") is None
|
||||
# Not silently reported as an 8192-token model:
|
||||
assert ModelRegistry.get_context_limit("gpt-4.1") != 8192
|
||||
assert ModelRegistry.get_context_limit("gpt-4.1", default=100) == 100
|
||||
|
||||
def test_resolve_future_google_family_fallback(self):
|
||||
"""Resolve should return provider-scoped fallbacks for plausible future models."""
|
||||
with patch("headroom.models.registry.get_model_pricing", return_value=None):
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue