mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(install): only validate requested targets on the manual path (#1659)
## Description
`resolve_targets()` runs the provider-scope "unsupported targets"
validation
**before** it dispatches on `provider_mode`:
```python
if scope == ConfigScope.PROVIDER.value:
unsupported = [t for t in requested if t and t not in valid]
if unsupported:
raise click.ClickException("Provider scope supports only ...; unsupported targets: ...")
if provider_mode == ALL: return [t.value for t in valid_targets] # ignores `requested`
if provider_mode == AUTO: ... # ignores `requested`
# manual: filters `requested`
```
But `all` and `auto` never consult the requested target list — only the
manual
path does. So an unsupported entry that those modes would simply ignore
instead
makes the call raise. Concretely:
```
headroom install apply --scope provider --providers all --target cursor
→ ClickException: Provider scope supports only claude, codex, openclaw, and
opencode; unsupported targets: cursor
```
...when it should just return the full provider set. (`--target` is a
click
`Choice` that accepts all 7 targets regardless of scope/mode, so this is
reachable from the CLI.) The user-scope equivalent,
`resolve_targets("all",
["cursor"])`, happily ignores `cursor` and returns all user targets — so
provider
scope is inconsistent with user scope for identical, ignored input.
Closes: no issue filed — found while auditing `install` target
resolution.
## Fix
Move the provider-scope validation so it runs only on the manual path
(the only
mode that reads `requested`). `all` returns the full provider set and
`auto`
returns detected/default targets, neither raising on an ignored
requested list.
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
## Changes Made
- `headroom/install/planner.py`: move the provider-scope "unsupported
targets" check below the `all`/`auto` dispatch, into the manual path.
- `tests/test_install/test_planner.py`: regression tests — `all` and
`auto` ignore an unsupported requested target under provider scope; the
manual path still rejects it.
- `CHANGELOG.md`: Bug Fixes entry under Unreleased.
## Testing
- [x] New tests added for the fixed behavior
(`tests/test_install/test_planner.py`)
- [x] Linting passes (`ruff check`) and formatting is clean (`ruff
format --check`)
- [ ] Full `pytest` deferred to CI (see Real Behavior Proof for the
local-OOM reason).
```text
$ uv run ruff check headroom/install/planner.py tests/test_install/test_planner.py
All checks passed!
```
## Real Behavior Proof
- Environment: Windows 11, Python 3.12.11, headroom built from this
branch. Importing `headroom` loads the torch/transformers stack and a
full `pytest` gets OOM-killed on this box, so I verified the control
flow with a dependency-free script (only stdlib) and left the full
pytest to CI.
- Exact command / steps: replicated `resolve_targets`'s control flow
(with the fix, `click.ClickException` stubbed, and stand-in target
lists) in a standalone script — no `headroom` import — and exercised
`all`/`auto`/`manual` under provider scope with an unsupported `cursor`
entry, plus the user-scope and manual-dedup regressions.
- Observed result: `all`/`auto` return the provider targets without
raising, `manual` still raises on the unsupported target, and the
pre-existing manual-dedup and user-scope behavior is unchanged:
```text
OK: all + [cursor] + provider -> provider set (no raise)
OK: auto + [cursor] + provider -> [claude, codex] (no raise)
OK: manual + [cursor] + provider -> raises (preserved)
OK: manual dedupe/filter + user-scope all unchanged
PLANNER LOGIC VERIFIED
```
- Not tested: a full `headroom install apply` end-to-end (would require
the heavy stack and a real deployment); the change is confined to pure
target-list resolution. 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; a control-flow move plus tests.
---------
Co-authored-by: JD Davis <mxjerrett@gmail.com>
This commit is contained in:
parent
560319cef4
commit
18e5680be3
3 changed files with 57 additions and 10 deletions
|
|
@ -117,6 +117,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||
* **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`).
|
||||
* **install:** stop `resolve_targets` from rejecting valid `--providers all`/`auto` installs under provider scope. The provider-scope "unsupported targets" validation ran before the mode dispatch, so `headroom install apply --scope provider --providers all --target cursor` raised `ClickException` even though `all`/`auto` ignore the requested target list entirely (user scope silently ignores the same input). The check now runs only on the manual path that actually consults the requested list.
|
||||
* **mcp/opencode:** stop the OpenCode MCP registrar from destroying an existing but unparseable `opencode.json`. `_write_entry` read the config via a helper that returns `{}` on `JSONDecodeError`, then rewrote the whole file with only `{"mcp": {...}}` — wiping the user's `theme`/`model`/`provider` and any other MCP servers (OpenCode configs are commonly JSONC / hand-edited). The write path now refuses to overwrite a present-but-invalid config and returns a `FAILED` result; absent/empty files still register fresh and valid files still merge with all other keys preserved. (Same class of fix as the Claude registrar.)
|
||||
* **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)).
|
||||
|
|
|
|||
|
|
@ -69,15 +69,6 @@ def resolve_targets(
|
|||
valid = {target.value for target in valid_targets}
|
||||
requested = [target.strip().lower() for target in requested_targets]
|
||||
|
||||
if scope == ConfigScope.PROVIDER.value:
|
||||
unsupported = [target for target in requested if target and target not in valid]
|
||||
if unsupported:
|
||||
unsupported_list = ", ".join(sorted(set(unsupported)))
|
||||
raise click.ClickException(
|
||||
"Provider scope supports only claude, codex, openclaw, and opencode; "
|
||||
f"unsupported targets: {unsupported_list}"
|
||||
)
|
||||
|
||||
if provider_mode == ProviderSelectionMode.ALL.value:
|
||||
return [target.value for target in valid_targets]
|
||||
|
||||
|
|
@ -89,6 +80,20 @@ def resolve_targets(
|
|||
*([] if scope == ConfigScope.PROVIDER.value else [ToolTarget.COPILOT.value]),
|
||||
]
|
||||
|
||||
# Manual selection is the only mode that consults `requested`, so the
|
||||
# provider-scope validation belongs here. Running it earlier rejected
|
||||
# unsupported entries that `all`/`auto` ignore entirely — e.g.
|
||||
# `install apply --scope provider --providers all --target cursor` raised
|
||||
# instead of returning the provider target set.
|
||||
if scope == ConfigScope.PROVIDER.value:
|
||||
unsupported = [target for target in requested if target and target not in valid]
|
||||
if unsupported:
|
||||
unsupported_list = ", ".join(sorted(set(unsupported)))
|
||||
raise click.ClickException(
|
||||
"Provider scope supports only claude, codex, openclaw, and opencode; "
|
||||
f"unsupported targets: {unsupported_list}"
|
||||
)
|
||||
|
||||
normalized = []
|
||||
seen: set[str] = set()
|
||||
for value in requested:
|
||||
|
|
|
|||
|
|
@ -1,7 +1,10 @@
|
|||
from __future__ import annotations
|
||||
|
||||
import click
|
||||
import pytest
|
||||
|
||||
from headroom.install.models import ConfigScope, InstallPreset, ProviderSelectionMode, ToolTarget
|
||||
from headroom.install.planner import build_manifest, resolve_targets
|
||||
from headroom.install.planner import PROVIDER_SCOPE_TARGETS, build_manifest, resolve_targets
|
||||
|
||||
|
||||
def test_resolve_targets_auto_falls_back_when_detection_empty(monkeypatch) -> None:
|
||||
|
|
@ -146,3 +149,41 @@ def test_build_manifest_persists_no_http2_override() -> None:
|
|||
|
||||
assert manifest.proxy_args.count("--no-http2") == 1
|
||||
assert "HEADROOM_HTTP2" not in manifest.base_env
|
||||
|
||||
|
||||
def test_resolve_targets_provider_scope_all_ignores_unsupported_requested() -> None:
|
||||
"""`all` mode never consults the requested list, so an unsupported entry
|
||||
like `cursor` must not make it raise — it should return the full provider
|
||||
target set (regression: this used to raise a ClickException)."""
|
||||
targets = resolve_targets(
|
||||
ProviderSelectionMode.ALL.value,
|
||||
["cursor"],
|
||||
scope=ConfigScope.PROVIDER.value,
|
||||
)
|
||||
|
||||
assert targets == [t.value for t in PROVIDER_SCOPE_TARGETS]
|
||||
|
||||
|
||||
def test_resolve_targets_provider_scope_auto_ignores_unsupported_requested(monkeypatch) -> None:
|
||||
"""`auto` mode also ignores the requested list, so an unsupported entry
|
||||
must not raise."""
|
||||
monkeypatch.setattr("headroom.install.planner.detect_targets", lambda: [])
|
||||
|
||||
targets = resolve_targets(
|
||||
ProviderSelectionMode.AUTO.value,
|
||||
["cursor"],
|
||||
scope=ConfigScope.PROVIDER.value,
|
||||
)
|
||||
|
||||
assert targets == [ToolTarget.CLAUDE.value, ToolTarget.CODEX.value]
|
||||
|
||||
|
||||
def test_resolve_targets_provider_scope_manual_rejects_unsupported() -> None:
|
||||
"""The manual path DOES consult the requested list, so an unsupported
|
||||
target under provider scope must still be rejected."""
|
||||
with pytest.raises(click.ClickException, match="cursor"):
|
||||
resolve_targets(
|
||||
ProviderSelectionMode.MANUAL.value,
|
||||
["cursor"],
|
||||
scope=ConfigScope.PROVIDER.value,
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue