From 18e5680be33d6881bfa3fdd9e0fdd32324ec8af7 Mon Sep 17 00:00:00 2001 From: Abhay Singh Date: Mon, 13 Jul 2026 03:00:31 +0530 Subject: [PATCH] fix(install): only validate requested targets on the manual path (#1659) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- CHANGELOG.md | 1 + headroom/install/planner.py | 23 +++++++++------- tests/test_install/test_planner.py | 43 +++++++++++++++++++++++++++++- 3 files changed, 57 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a9b3b8aa..cb41d6292 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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)). diff --git a/headroom/install/planner.py b/headroom/install/planner.py index d2f6e9cce..df5557f8e 100644 --- a/headroom/install/planner.py +++ b/headroom/install/planner.py @@ -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: diff --git a/tests/test_install/test_planner.py b/tests/test_install/test_planner.py index 1eac07d5e..37ee39016 100644 --- a/tests/test_install/test_planner.py +++ b/tests/test_install/test_planner.py @@ -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, + )