mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(install): guard non-dict health config in 'install status' (#2150)
## 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 <mxjerrett@gmail.com>
Co-authored-by: Tejas Chopra <chopratejas@gmail.com>
This commit is contained in:
parent
0cddac632d
commit
8f867e4622
3 changed files with 37 additions and 1 deletions
|
|
@ -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=<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 `<<ccr:` form. So on a frozen-prefix turn (both `read_lifecycle` and prefix freezing are on by default) the injector reported no compressed content, the `headroom_retrieve` tool was not injected, and the model was handed a marker advertising `Retrieve original: hash=X` with no tool to redeem it — silent data loss for stale reads, where retrieval is the only way to recover the original-at-read-time content (the exact case the #1006 guard exists to prevent). Added a pattern matching the load-bearing `Retrieve original: hash=` phrase, aligning the injector with the sibling `read_maturation` marker that was already (incidentally) detected.
|
||||
* **install:** don't crash `headroom install status` when the health payload's `config` is a non-dict. The command did `payload.get('config', {}).get('backend', manifest.backend)`, but `dict.get(..., {})` only defaults on a *missing* key — a present-but-non-dict `config` (`null`, a string, a list, e.g. when a different or older service is answering on the port) reached the chained `.get('backend', ...)` and raised `AttributeError`, crashing the command with a raw traceback. The value is now guarded with `isinstance(config, dict)` before the lookup, mirroring `wrap.py`'s `_proxy_health_config`, so it falls back to the manifest's backend.
|
||||
* **telemetry:** only advance the usage-report baseline after a confirmed 200. `UsageReporter._report_usage` sends usage as a delta against the last snapshot, but it called `_snapshot_metrics()` (and advanced `_last_report_time`) unconditionally after the POST — including when the send returned non-200 or raised. So a report that failed to reach the cloud (which the module is explicitly designed to tolerate) still rebased the baseline, permanently dropping that window's requests/tokens from usage-based billing/quota; the next report started from the advanced baseline and never re-included them. The baseline now advances only on a 200, so a failed send leaves the window intact for the next report to retry.
|
||||
* **savings:** stop the durable savings ledger from billing free (0-priced) models at the `$3/M` fallback. `estimate_cost_usd` guarded the litellm estimate with `if priced > 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.
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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] = []
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue