From 8f867e46222239a45136df8f5a05515897077bbf Mon Sep 17 00:00:00 2001 From: Abhay Singh Date: Tue, 14 Jul 2026 21:49:58 +0530 Subject: [PATCH] fix(install): guard non-dict health config in 'install status' (#2150) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `headroom install status` crashes with an `AttributeError` when the probed health endpoint returns a non-dict `config`. ```python if payload and isinstance(payload, dict): click.echo(f"Health URL: {manifest.health_url.replace('/readyz', '/health')}") click.echo(f"Backend: {payload.get('config', {}).get('backend', manifest.backend)}") ``` `payload` is guarded as a dict, but `payload['config']` is not. `dict.get('config', {})` only substitutes the `{}` default when the key is **absent** — a present-but-non-dict `config` (`null`, a string, a list) is returned as-is, and the chained `.get('backend', ...)` then raises `AttributeError`, crashing the command with a raw traceback. Reachability: the Headroom proxy normally returns `config` as an object, so this bites when `install status` probes a port that a different or older service is occupying (which can emit `config: null` or a non-object), or a build that emits `config: null`. The correctly-guarded sibling already exists in the codebase — `wrap.py`'s `_proxy_health_config` does `config = payload.get("config"); return config if isinstance(config, dict) else None`. ## Fix Guard the `config` value with `isinstance(config, dict)` before the `.get('backend', ...)` lookup, mirroring `_proxy_health_config`. A non-dict (or missing) `config` falls back to the manifest's backend. 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/cli/install.py`: `install status` guards `config` with `isinstance(config, dict)` before reading `backend`. - `tests/test_cli/test_install_cli.py`: add `test_install_status_survives_non_dict_config` (health payload with `config: null` must not crash; backend falls back to the manifest). - `CHANGELOG.md`: Bug Fixes entry. ## Testing - [ ] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [ ] 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/cli/install.py tests/test_cli/test_install_cli.py All checks passed! $ python -m py_compile headroom/cli/install.py tests/test_cli/test_install_cli.py OK ``` ## Real Behavior Proof - Environment: Windows 11, Python 3.12, `uvx ruff@0.15.17`. Importing `headroom` pulls in the torch/transformers stack and a full `pytest` gets OOM-killed on this box, so I verified the access with a dependency-free script that replicates the old vs guarded lookup, and left the full pytest (including the new CLI test) to CI. - Exact command / steps: ran the old `payload.get('config', {}).get('backend', ...)` and the new guarded lookup against `config` values of `null`, a string, a list, a proper object, and a missing key. - Observed result: the old lookup raises `AttributeError` for every non-dict `config`; the new lookup falls back to the manifest backend for those and returns the real backend for a proper object (and the missing-key case is unchanged). The new CLI test drives `install status` with `probe_json` returning `{"config": null}` and asserts a clean exit with the manifest backend. - Not tested: a live foreign service occupying the port; 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 - [x] I have updated the CHANGELOG.md if applicable ## Additional Notes The "unit tests pass locally" and "type checking" boxes are unchecked because the full suite imports the ML stack, which I can't run in this environment; the change adds an `isinstance` guard mirroring an existing sibling, verified by the standalone proof and a new CLI test that reuses the file's existing `install status` mocking harness. Co-authored-by: JerrettDavis Co-authored-by: Tejas Chopra --- CHANGELOG.md | 1 + headroom/cli/install.py | 10 +++++++++- tests/test_cli/test_install_cli.py | 27 +++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2dde03b58..d1713314e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -118,6 +118,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * **proxy:** run a cold-start fast pass before background-compression deferral so byte-identical freeze doesn't lock sessions to the uncompressed transcript. Since #1850, a session's provider-cached prefix is frozen in whatever form its cold start forwarded; deferring the WHOLE pipeline (`HEADROOM_BACKGROUND_COMPRESSION=1`, frozen=0, ≥50k tokens) therefore cached the raw transcript and forfeited the session's compression savings for its lifetime — including sub-second lossless wins like `read_lifecycle` stale-read drops, observed in the field as sessions permanently stuck at 0 savings. The deferral branch now runs the pipeline synchronously with the new `skip_kompress=True` kwarg (everything except the Kompress ML stage — the only stage that can blow the request budget per #1171) under a bounded fast-pass budget (`HEADROOM_COLD_START_FAST_PASS_TIMEOUT_SECONDS`, default 10s), forwards the pruned form, and defers only Kompress to the background job (tagged `deferred:kompress_background`). Fail-open: on fast-pass timeout/error the request forwards uncompressed exactly as before. Units routed to Kompress under `skip_kompress` take the same fallback as when the model isn't ready. * **init/codex:** don't overwrite the user's `hooks.json`. `_ensure_codex_hooks` wrote a fresh payload containing only Headroom's two hooks, wholesale-replacing `~/.codex/hooks.json` — so any user-managed Codex hooks (and other top-level keys) were silently destroyed on `headroom init codex`. It now read-merges: existing entries are preserved, Headroom's are deduped on the `headroom-init-codex` marker and appended, matching `_ensure_claude_hooks` / `_ensure_copilot_hooks`. * **ccr:** detect `read_lifecycle` stale/superseded markers in the retrieve-tool injector so they stay redeemable. Those markers (`[Read content stale: … Retrieve original: hash=]`) store the original bytes in the CCR store under a valid hash, but none of `CCRToolInjector`'s patterns matched them — every pattern required the word "compressed" or the `< 0`, so a genuinely free model — where `_estimate_compression_savings_usd` correctly returns `0.0` — was treated as "unpriced" and fell through to the blended fallback rate, writing phantom cost-avoided into the JSONL ledger and surfacing it in `headroom savings`. The ledger now trusts the estimate verbatim for known models (it already falls back internally for models litellm can't price and returns `0.0` for free ones), fixing the same defect at this call site that was already fixed inside the helper. * **init/codex:** stop `headroom init codex` from deleting per-profile provider settings. `_ensure_codex_provider` removed the root-level `model_provider`/`openai_base_url` (which init owns) with a multiline regex that matched those keys in **every** table, so a user's `[profiles.*]` overrides (e.g. `[profiles.work] model_provider = "azure"`) were silently stripped and those profiles fell through to the injected `"headroom"` default — config corruption. The strip is now scoped to the document root (everything before the first table header), so per-profile overrides are preserved while init still replaces a root-level assignment. diff --git a/headroom/cli/install.py b/headroom/cli/install.py index 8ba43ca52..77aa18ab3 100644 --- a/headroom/cli/install.py +++ b/headroom/cli/install.py @@ -337,7 +337,15 @@ def install_status(profile: str) -> None: click.echo(f"Healthy: {'yes' if probe_ready(manifest.health_url) else 'no'}") if payload and isinstance(payload, dict): click.echo(f"Health URL: {manifest.health_url.replace('/readyz', '/health')}") - click.echo(f"Backend: {payload.get('config', {}).get('backend', manifest.backend)}") + # `config` may be a non-dict (null / string / list) if a different or + # older service is answering on the port. `payload.get('config', {})` + # only defaults on a MISSING key, so a present-but-non-dict value would + # reach `.get('backend', ...)` and crash with AttributeError. Guard on + # isinstance, mirroring wrap.py's _proxy_health_config. + config = payload.get("config") + if not isinstance(config, dict): + config = {} + click.echo(f"Backend: {config.get('backend', manifest.backend)}") @install.command("start") diff --git a/tests/test_cli/test_install_cli.py b/tests/test_cli/test_install_cli.py index f52cc0b85..95c05046f 100644 --- a/tests/test_cli/test_install_cli.py +++ b/tests/test_cli/test_install_cli.py @@ -137,6 +137,33 @@ def test_install_status_includes_backend_from_health_probe(monkeypatch) -> None: assert "Backend: anthropic" in result.output +def test_install_status_survives_non_dict_config(monkeypatch) -> None: + """A health payload whose `config` is a non-dict (e.g. a different service + answering on the port returns config: null) must not crash the command.""" + runner = CliRunner() + + class Manifest: + profile = "default" + preset = "persistent-service" + runtime_kind = "python" + supervisor_kind = "service" + scope = "user" + port = 8787 + backend = "anthropic" + health_url = "http://127.0.0.1:8787/readyz" + + monkeypatch.setattr("headroom.cli.install.load_manifest", lambda profile: Manifest()) + monkeypatch.setattr("headroom.cli.install.runtime_status", lambda manifest: "running") + monkeypatch.setattr("headroom.cli.install.probe_ready", lambda url: True) + monkeypatch.setattr("headroom.cli.install.probe_json", lambda url: {"config": None}) + + result = runner.invoke(main, ["install", "status"]) + + # No AttributeError; Backend falls back to the manifest value. + assert result.exit_code == 0, result.output + assert "Backend: anthropic" in result.output + + def test_install_restart_uses_internal_helpers(monkeypatch) -> None: runner = CliRunner() calls: list[str] = []