fix(wrap/claude): bind _wrap_settings_path before the try (#2126)

## 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 <mxjerrett@gmail.com>
Co-authored-by: Tejas Chopra <chopratejas@gmail.com>
This commit is contained in:
Abhay Singh 2026-07-14 21:32:39 +05:30 committed by GitHub
parent d236b27c60
commit faed4dcfe7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 78 additions and 1 deletions

View file

@ -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.

View file

@ -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(

View file

@ -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