From faed4dcfe72a329cc308057efd97fc99885173c4 Mon Sep 17 00:00:00 2001 From: Abhay Singh Date: Tue, 14 Jul 2026 21:32:39 +0530 Subject: [PATCH] fix(wrap/claude): bind _wrap_settings_path before the try (#2126) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `headroom wrap claude` crashes with an `UnboundLocalError` from its cleanup `finally` whenever the proxy fails to start, which both hides the real error and skips cleanup. `claude()` initializes its cleanup state before the `try` so the `finally` can always reference it — `proxy_holder`, `_saved_base_url`, `_settings_foundry`, `port_holder`, `_settings_vertex` are all bound up front. But `_wrap_settings_path` was the exception: it was assigned only inside the `try`, after `_ensure_proxy`: ```python try: ... proxy_holder[0], actual_port = _ensure_proxy(port, ...) # can raise ... _wrap_settings_path = Path.cwd() / ".claude" / "settings.local.json" # assigned here ... finally: _restore_claude_wrap_base_url(..., settings_path=_wrap_settings_path) # referenced here cleanup() ``` `_ensure_proxy` raises when the requested port is unavailable and the range is exhausted, or when the proxy subprocess fails to start. When it does, control jumps to the `finally`, which evaluates `settings_path=_wrap_settings_path` — a local that was never assigned — and raises `UnboundLocalError`. That replaces the real failure with a raw traceback, and because the `finally` aborts on that line, `cleanup()` never runs, so proxy cleanup and wrap-marker clearing are skipped too. ## Fix Bind `_wrap_settings_path` before the `try`, next to the other cleanup holders, so the `finally` can always reference it. The value is unchanged (the in-`try` assignment is removed since it computed the same path). 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/wrap.py`: hoist the `_wrap_settings_path` initialization to before the `try` (alongside `proxy_holder`/`_saved_base_url`/…) and drop the redundant in-`try` assignment. - `tests/test_cli/test_wrap_claude_finally_unbound.py`: new test — drive `wrap claude` with `_ensure_proxy` patched to raise and assert the `finally` completes (no `UnboundLocalError`, and both restore and cleanup ran). - `CHANGELOG.md`: Bug Fixes entry. ## Testing - [x] Unit tests pass (`uv run --extra dev pytest tests/test_cli/test_wrap_claude_finally_unbound.py -q`) - [x] Linting passes (`uvx ruff@0.15.17 check headroom/cli/wrap.py tests/test_cli/test_wrap_claude_finally_unbound.py headroom/memory/factory.py`) - [x] Type checking passes (`uvx mypy==1.20.2 headroom/memory/factory.py`) - [x] New tests added for new functionality - [ ] Manual testing performed ### Test Output ```text $ uvx ruff@0.15.17 check headroom/cli/wrap.py tests/test_cli/test_wrap_claude_finally_unbound.py All checks passed! $ python -m py_compile headroom/cli/wrap.py tests/test_cli/test_wrap_claude_finally_unbound.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 running the CLI test locally OOM-kills this box, so I verified the control flow with a dependency-free script that reproduces the try/finally with the variable assigned inside vs before the try, and left the full pytest (including the new CLI test) to CI. - Exact command / steps: ran the flow with the variable bound inside the try (old) and before the try (new), each with an early failure that fires before the in-try assignment. - Observed result: old raises `UnboundLocalError` from the finally and skips restore/cleanup; new runs the finally cleanly and lets the real `RuntimeError` propagate. The new CLI test drives `wrap claude` with `_ensure_proxy` raising and asserts no `UnboundLocalError` and that restore and cleanup both ran. - Not tested: a live proxy port-exhaustion end to end; 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 Merged current `main` to pick up the repository-wide mypy cache-key annotation fix, then verified the focused regression locally. the change hoists one assignment to before the `try` (mirroring the four sibling holders three lines above), verified by the control-flow proof and a new CLI test that reuses the same mocking pattern the existing `wrap claude` vertex tests use. Co-authored-by: JerrettDavis Co-authored-by: Tejas Chopra --- CHANGELOG.md | 1 + headroom/cli/wrap.py | 8 ++- .../test_wrap_claude_finally_unbound.py | 70 +++++++++++++++++++ 3 files changed, 78 insertions(+), 1 deletion(-) create mode 100644 tests/test_cli/test_wrap_claude_finally_unbound.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c3559494e..467829582 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -133,6 +133,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * **tokenizers:** resolve HuggingFace tokenizer names by the most-specific prefix. `get_tokenizer_name` scanned `MODEL_TO_TOKENIZER` in dict-insertion order and returned the first key the model merely starts with, so a short family key shadowed a more-specific one — `qwen2-7b-instruct` matched `qwen` before `qwen2`/`qwen2-7b` and resolved to the Qwen1 tokenizer (a different vocabulary, hence wrong token counts); `qwen2.5-*` and `deepseek-v2.x` were mis-resolved the same way. It now picks the longest matching prefix, mirroring the order-dependent-prefix guard the sibling tiktoken `get_encoding_for_model` already documents. * **pricing:** map retired `claude-3-sonnet-20240229` to a Sonnet-tier price instead of Haiku. When LiteLLM's cost DB lacks the retired model, resolution falls through to `MODEL_ALIASES`, which pointed Claude 3 Sonnet (a $3/$15-per-1M model) at `claude-3-haiku-20240307` ($0.25/$1.25) — a different tier that underpriced every cost/savings figure for that model ~12x on both input and output. It now aliases to `claude-sonnet-4-20250514`, the same-price target the other retired-Sonnet aliases already use. * **cache/semantic:** don't evict an unrelated entry when re-storing a key that is already cached. `SemanticCache.put` ran its at-capacity eviction loop before computing the entry's key, so overwriting a key that was already present (a duplicate or retried store) still evicted the LRU-oldest distinct entry even though an in-place update grows nothing. That silently dropped a live entry and turned a later lookup for it into a false cache miss. The key is now computed first and the eviction loop only runs when the key is genuinely new (mirroring `CompressionCache.store_compressed`, which deletes-then-inserts). +* **wrap/claude:** don't raise `UnboundLocalError` in the cleanup `finally` when the proxy fails to start. `claude()` referenced `_wrap_settings_path` in its `finally` block but only assigned it inside the `try`, after `_ensure_proxy` (which raises on port exhaustion or a failed proxy start). An early failure therefore made the `finally` raise `UnboundLocalError` — replacing the real error with a raw traceback and, because the `finally` aborted before `cleanup()`, skipping proxy cleanup and wrap-marker clearing. `_wrap_settings_path` is now bound before the `try` alongside the other cleanup holders (`proxy_holder`, `_saved_base_url`, …), so the `finally` is always safe. * **wrap/codex:** export the detected custom upstream base URL so Codex actually routes to it. `_inject_codex_provider_config` detects an OpenAI-compatible gateway declared in `~/.codex/config.toml` and writes an `X-Headroom-Base-Url` header mapped to `HEADROOM_CODEX_UPSTREAM_BASE_URL`, and it returns that URL for the caller to export. But `_prepare_codex_wrap_state` discarded the return and `_run_codex_wrap` never set the env var, so Codex emitted no header, the proxy fell back to `api.openai.com`, and the user's gateway key was sent to OpenAI (which rejects it). This restores the wiring a later refactor dropped: `_prepare_codex_wrap_state` now returns the URL and `_run_codex_wrap` exports it into the launch env when set (a user-provided value still wins) (regression of [#1614](https://github.com/chopratejas/headroom/issues/1614)). * **cache:** normalize embeddings before the semantic dynamic-content similarity check. `SemanticDetector` scored sentences with `np.dot` against exemplar embeddings and compared the result to `semantic_threshold` (a 0-1 cosine value), but `sentence_transformers.encode(..., convert_to_numpy=True)` does not normalize, so the dot product was an unbounded inner product (vector norms ~5-15) rather than a cosine similarity. Nearly every sentence cleared the 0.7 threshold, so the semantic tier flagged almost all text as dynamic and stripped static content, busting the cache it is meant to protect (a standalone repro scores an unrelated sentence, true cosine ~0.1, at a raw dot of ~9). Both `encode` calls now pass `normalize_embeddings=True`, matching the siblings in `prediction/feature_extractor.py` and `memory/adapters/embedders.py`, so the dot product is a true cosine in [-1, 1]. * **subscription:** cap `HeadroomContribution.efficiency_pct` at a real removal ratio so it can't exceed 100%. The numerator used `total_saved()` (which includes `tokens_saved_cache_reads`) while the denominator `raw_without_headroom()` excludes cache reads, so a contribution with large prefix-cache reads and small forwarded input reported impossible values (e.g. `tokens_submitted=100`, `tokens_saved_cache_reads=1000` → `1000.0%` on the dashboard). Cache reads are a provider-side discount on tokens that were still forwarded, not tokens Headroom removed, so the ratio now uses the sibling `compression_saved()` (compression + CLI filtering) as the numerator — bounded by its own denominator. `total_saved()` is unchanged for its other callers. diff --git a/headroom/cli/wrap.py b/headroom/cli/wrap.py index d69ac7ce3..7f54b570a 100644 --- a/headroom/cli/wrap.py +++ b/headroom/cli/wrap.py @@ -4016,6 +4016,11 @@ def claude( _settings_foundry: list[bool] = [False] port_holder: list[int] = [port] _settings_vertex: list[bool] = [False] + # Bind before the try so the finally can always reference it. It is otherwise + # only assigned inside the try (after _ensure_proxy, which can raise), so an + # early proxy-start failure would make the finally raise UnboundLocalError, + # masking the real error and skipping cleanup(). Mirrors the holders above. + _wrap_settings_path = Path.cwd() / ".claude" / "settings.local.json" cleanup = _make_cleanup(proxy_holder, port_holder) signal.signal(signal.SIGINT, _ignore_child_sigint) signal.signal(signal.SIGTERM, cleanup) @@ -4219,7 +4224,8 @@ def claude( # daemon's environment) also route through Headroom. _settings_vertex[0] = bool(use_vertex) _settings_foundry[0] = bool(foundry_upstream) and not _settings_vertex[0] - _wrap_settings_path = Path.cwd() / ".claude" / "settings.local.json" + # _wrap_settings_path is bound before the try (above) so the finally is + # always safe; the value is unchanged here. _check_and_clear_stale_wrap_marker( _wrap_settings_path, key=_claude_wrap_base_url_env_key( diff --git a/tests/test_cli/test_wrap_claude_finally_unbound.py b/tests/test_cli/test_wrap_claude_finally_unbound.py new file mode 100644 index 000000000..398717eab --- /dev/null +++ b/tests/test_cli/test_wrap_claude_finally_unbound.py @@ -0,0 +1,70 @@ +"""Regression: `wrap claude`'s finally must not raise UnboundLocalError when the +proxy fails to start before `_wrap_settings_path` is assigned inside the try.""" + +from __future__ import annotations + +import pytest +from click.testing import CliRunner + +from headroom.cli import wrap as wrap_mod +from headroom.cli.main import main + + +@pytest.fixture +def runner() -> CliRunner: + return CliRunner() + + +def test_finally_survives_early_proxy_start_failure( + runner: CliRunner, monkeypatch: pytest.MonkeyPatch +) -> None: + state = {"restore_called": False, "cleanup_called": False} + + for key in ( + "ANTHROPIC_BASE_URL", + "ANTHROPIC_VERTEX_BASE_URL", + "ANTHROPIC_FOUNDRY_BASE_URL", + "ANTHROPIC_FOUNDRY_RESOURCE", + "CLAUDE_CODE_USE_VERTEX", + "CLAUDE_CODE_USE_FOUNDRY", + "VERTEX_TARGET_API_URL", + ): + monkeypatch.delenv(key, raising=False) + + monkeypatch.setattr(wrap_mod.shutil, "which", lambda _name: "/usr/bin/claude") + monkeypatch.setattr(wrap_mod, "_register_proxy_client", lambda _port: None) + monkeypatch.setattr(wrap_mod.signal, "signal", lambda *_a, **_k: None) + monkeypatch.setattr(wrap_mod, "_push_runtime_env", lambda *_a, **_k: None) + monkeypatch.setattr(wrap_mod, "_setup_coding_compressor", lambda *_a, **_k: None) + monkeypatch.setattr(wrap_mod, "_print_telemetry_notice", lambda: None) + monkeypatch.setattr(wrap_mod, "_write_claude_wrap_base_url", lambda *_a, **_k: None) + + def _fake_make_cleanup(_holder, _port): + def _cleanup() -> None: + state["cleanup_called"] = True + + return _cleanup + + monkeypatch.setattr(wrap_mod, "_make_cleanup", _fake_make_cleanup) + + def _fake_restore(*_a, **_k) -> None: + state["restore_called"] = True + + monkeypatch.setattr(wrap_mod, "_restore_claude_wrap_base_url", _fake_restore) + + def _boom(*_a, **_k): + raise RuntimeError("proxy failed to start") + + monkeypatch.setattr(wrap_mod, "_ensure_proxy", _boom) + + result = runner.invoke( + main, + ["wrap", "claude", "--no-context-tool", "--no-mcp", "--no-tokensave", "--no-serena"], + ) + + # The finally must complete: no UnboundLocalError masking the real failure, + # and both restore and cleanup ran even though the proxy failed before + # _wrap_settings_path was assigned inside the try. + assert not isinstance(result.exception, UnboundLocalError), result.output + assert state["restore_called"] is True + assert state["cleanup_called"] is True