diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 6da0369bc..62abf2ed3 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -5,14 +5,14 @@ }, "metadata": { "description": "Headroom marketplace for Claude Code and GitHub Copilot CLI plugins.", - "version": "0.36.4" + "version": "0.36.5" }, "plugins": [ { "name": "headroom", "source": "./plugins/headroom-agent-hooks", "description": "Headroom startup hooks for Claude Code and GitHub Copilot CLI.", - "version": "0.36.4", + "version": "0.36.5", "author": { "name": "Headroom Contributors", "url": "https://github.com/chopratejas/headroom" diff --git a/.github/plugin/marketplace.json b/.github/plugin/marketplace.json index 6da0369bc..62abf2ed3 100644 --- a/.github/plugin/marketplace.json +++ b/.github/plugin/marketplace.json @@ -5,14 +5,14 @@ }, "metadata": { "description": "Headroom marketplace for Claude Code and GitHub Copilot CLI plugins.", - "version": "0.36.4" + "version": "0.36.5" }, "plugins": [ { "name": "headroom", "source": "./plugins/headroom-agent-hooks", "description": "Headroom startup hooks for Claude Code and GitHub Copilot CLI.", - "version": "0.36.4", + "version": "0.36.5", "author": { "name": "Headroom Contributors", "url": "https://github.com/chopratejas/headroom" diff --git a/.release-please-manifest.json b/.release-please-manifest.json index 00a26c097..28c17198a 100644 --- a/.release-please-manifest.json +++ b/.release-please-manifest.json @@ -1,3 +1,3 @@ { - ".": "0.36.4" + ".": "0.36.5" } diff --git a/.releasemetadata b/.releasemetadata index 2d731b82c..df07a0b5a 100644 --- a/.releasemetadata +++ b/.releasemetadata @@ -1,10 +1,10 @@ { - "version": "0.36.4", + "version": "0.36.5", "packages": { - "pypi": "0.36.4", - "npm-sdk": "0.36.4", - "npm-openclaw": "0.36.4", - "npm-opencode": "0.36.4", - "agent-hooks-plugin": "0.36.4" + "pypi": "0.36.5", + "npm-sdk": "0.36.5", + "npm-openclaw": "0.36.5", + "npm-opencode": "0.36.5", + "agent-hooks-plugin": "0.36.5" } } diff --git a/CHANGELOG.md b/CHANGELOG.md index cf7a97113..89a8f36fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -284,6 +284,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **code:** fix two `CodeAwareCompressor` AST-reassembly bugs: an exported JS/TS function or class (`export function foo() {`) produced a duplicated `export export` keyword and invalid syntax, because line-based node slicing (used to preserve indentation) pulled in the preceding `export` sibling's text on top of the `export_statement` handler's own prefix reconstruction. Separately, in every supported language, a doc comment immediately above a top-level function, class, or type was detached from its declaration during extraction and re-emitted in a cluster at the end of the compressed output instead of staying attached to what it documents. - * **proxy:** Buffered upstream responses containing a `server_tool_use` (or any other unrecognized Anthropic content block) no longer turn a fully-generated response into an HTTP 502. `StreamingMixin._response_to_sse` raised `ValueError` on unknown block types after the entire upstream generation had already been buffered, so a slow-but-successful response failed and the client retried the whole multi-minute request. Unknown blocks are now emitted verbatim in `content_block_start` (following the existing redacted_thinking` pattern), so `server_tool_use`, `server_tool_result`, `mcp_tool_use`, and future block types round-trip ([#1806](https://github.com/headroomlabs-ai/headroom/issues/1806)). +## [0.36.5](https://github.com/headroomlabs-ai/headroom/compare/v0.36.4...v0.36.5) (2026-08-22) + + +### Bug Fixes + +* **codex:** detect ChatGPT auth from id_token claims so wrap/init emit requires_openai_auth ([#3212](https://github.com/headroomlabs-ai/headroom/issues/3212)) ([2f81fa5](https://github.com/headroomlabs-ai/headroom/commit/2f81fa5931ddf233b908103f25c614b5a6b7e33b)) +* **doctor:** report project-scoped Claude routing instead of a false negative ([#3213](https://github.com/headroomlabs-ai/headroom/issues/3213)) ([8f3e33a](https://github.com/headroomlabs-ai/headroom/commit/8f3e33a00ea377403497b600841c03344d0c1cd8)) + ## [0.36.4](https://github.com/headroomlabs-ai/headroom/compare/v0.36.3...v0.36.4) (2026-08-22) diff --git a/docs/content/docs/mcp.mdx b/docs/content/docs/mcp.mdx index af6302825..7fe121ff8 100644 --- a/docs/content/docs/mcp.mdx +++ b/docs/content/docs/mcp.mdx @@ -187,6 +187,8 @@ Install Headroom so it's globally on PATH — `uv tool install "headroom-ai[mcp] ## Architecture +For user-managed Serena drift, run `headroom mcp reconcile` to inspect the current recommendation. Add `--adopt` only when you want Headroom to replace the Serena entry. + ### MCP only (no proxy) The LLM calls `headroom_compress` on demand. Compression happens locally in the MCP process. Originals are stored in a local `CompressionStore` with 1-hour TTL. diff --git a/docs/content/docs/troubleshooting.mdx b/docs/content/docs/troubleshooting.mdx index 22ef73ae8..5b796b371 100644 --- a/docs/content/docs/troubleshooting.mdx +++ b/docs/content/docs/troubleshooting.mdx @@ -223,6 +223,18 @@ See [issue #746](https://github.com/headroomlabs-ai/headroom/issues/746) for the `ENABLE_TOOL_SEARCH` is unaffected and can stay enabled for context-window savings while routing through Headroom. +## Server-managed settings unavailable through custom ANTHROPIC_BASE_URL + +**Symptom**: Settings pushed from **Admin Settings > Claude Code > Managed settings** in the claude.ai console (server-managed settings) don't apply to sessions running through Headroom, even though they apply fine without the proxy. + +**Cause**: This is a Claude-side gate, not a Headroom limitation. Per Anthropic's docs, server-managed settings require a direct connection to `api.anthropic.com`; if `ANTHROPIC_BASE_URL` is set to any non-default host — which is exactly what wrapping via Headroom does — Claude Code skips the settings fetch entirely for that session. The request never reaches Headroom, so there is no endpoint for Headroom to implement or proxy. + +This is separate from the OS-level `managed-settings.json` file (macOS `/Library/Application Support/ClaudeCode/`, Linux `/etc/claude-code/`, Windows `C:\Program Files\ClaudeCode\`): that file is read straight from local disk at startup and is unaffected by `ANTHROPIC_BASE_URL` or Headroom. If that file isn't taking effect, the cause is unrelated to proxying (path, permissions, or JSON syntax) — check `claude --debug-file ` and search the log for `Remote settings`. + +**Fix**: None available on the Headroom side — this is an intentional Anthropic security boundary (a proxy in the path could otherwise forge org policy). If your org relies on server-managed settings, deploy the same policy as [endpoint-managed settings](https://code.claude.com/docs/en/settings#settings-files) (MDM profile, Windows registry, or a local `managed-settings.json`) instead, since those are read locally and unaffected by proxying. + +See [Server-managed settings platform availability](https://code.claude.com/docs/en/server-managed-settings#platform-availability) and [issue #3074](https://github.com/headroomlabs-ai/headroom/issues/3074). + ## Compression Too Aggressive **Symptom**: LLM responses are missing information that was in tool outputs. diff --git a/headroom/cache/semantic.py b/headroom/cache/semantic.py index 0ea2effba..946fada32 100644 --- a/headroom/cache/semantic.py +++ b/headroom/cache/semantic.py @@ -160,8 +160,18 @@ class SemanticCache: self._hits += 1 return entry - # Try semantic similarity if we have embedding function - if self._embedding_fn: + # Try semantic similarity if we have embedding function. + # + # Only for a NON-EMPTY query: the query is the last user message, and in + # agent/tool traffic the overwhelming majority of turns are tool_result + # continuations whose extracted query is "" (no text block). Embedding + # matching on "" makes every such turn ~identical to every other (a real + # sentence embedder maps "" to a fixed non-zero vector), so an empty + # query would false-hit and serve one conversation's response to an + # unrelated one — precisely the cross-context collision the messages_hash + # key is chosen to avoid. An empty query may still hit via the exact + # messages_hash above, which is context-complete and safe. + if self._embedding_fn and query.strip(): query_embedding = self._embedding_fn(query) best_match, best_similarity = self._find_similar(query_embedding) @@ -208,9 +218,13 @@ class SemanticCache: while key not in self._cache and len(self._cache) >= self.config.max_entries: self._evict_oldest() - # Generate embedding if available + # Generate embedding if available — but never for an empty/blank query. + # A stored empty-query entry with an embedding would be a false-match + # target for the semantic get() path; leaving its embedding empty makes + # _find_similar skip it (it ignores entries with no embedding), so an + # empty-query entry is reachable only by its exact messages_hash. embedding: list[float] = [] - if self._embedding_fn: + if self._embedding_fn and query.strip(): embedding = self._embedding_fn(query) now = time.time() diff --git a/headroom/cli/doctor.py b/headroom/cli/doctor.py index cd5d4839a..4187bd1a2 100644 --- a/headroom/cli/doctor.py +++ b/headroom/cli/doctor.py @@ -15,7 +15,7 @@ import json import os import re import sys -from collections.abc import Callable, Mapping +from collections.abc import Callable, Mapping, Sequence from dataclasses import asdict, dataclass from datetime import datetime from pathlib import Path @@ -149,47 +149,76 @@ def check_version_drift(livez: dict[str, Any] | None, installed: str) -> CheckRe ) -def check_claude_routing(settings_path: Path, port: int) -> CheckResult: - """Is Claude Code configured to route through the proxy?""" +def _claude_base_url_in(path: Path) -> tuple[str, CheckResult | None]: + """Read ``env.ANTHROPIC_BASE_URL`` from one Claude settings file. + + Returns ``(base_url, error)``. A parse problem comes back as a WARN so the + caller surfaces it verbatim instead of skipping the file and reporting the + misleading "not routed". + """ name = "claude" - if not settings_path.exists(): - return CheckResult( - name=name, - status=WARN, - summary="not routed (no ~/.claude/settings.json)", - hint="wrap it: headroom wrap claude", - ) try: - payload = json.loads(settings_path.read_text(encoding="utf-8")) + payload = json.loads(path.read_text(encoding="utf-8")) except (OSError, ValueError) as exc: - return CheckResult( - name=name, - status=WARN, - summary=f"could not parse {settings_path}: {exc}", - ) + return "", CheckResult(name=name, status=WARN, summary=f"could not parse {path}: {exc}") # `json.loads` succeeds on valid non-object JSON (e.g. `[]`, `null`, `42`), # which a hand-edited or reset settings file can contain. `.get` on a # non-dict raises AttributeError, and it is not one of the caught parse # errors above, so it would crash the very command run to diagnose the # broken config. Treat a non-object like an unparseable file. if not isinstance(payload, dict): - return CheckResult( + return "", CheckResult( name=name, status=WARN, - summary=f"could not parse {settings_path}: not a JSON object", + summary=f"could not parse {path}: not a JSON object", ) - base_url = "" env_block = payload.get("env") if isinstance(env_block, dict): - base_url = str(env_block.get("ANTHROPIC_BASE_URL", "") or "") - if not base_url: + return str(env_block.get("ANTHROPIC_BASE_URL", "") or ""), None + return "", None + + +def check_claude_routing( + settings_path: Path, + port: int, + project_settings_paths: Sequence[Path] | None = None, +) -> CheckResult: + """Is Claude Code configured to route through the proxy? + + Claude Code layers project settings over user settings, and `headroom init + claude` without --global writes the project-scoped + ``.claude/settings.local.json``. Reading only ``~/.claude/settings.json`` + reported "not routed" for sessions that demonstrably were -- confirmed by + `ps eww` on the live process and by active compression on it (#3205). + Candidates are consulted in Claude's own precedence order, and the summary + names the file that supplied the routing so the scope is never ambiguous. + """ + name = "claude" + candidates = [*(project_settings_paths or []), settings_path] + existing = [path for path in candidates if path.exists()] + if not existing: return CheckResult( name=name, status=WARN, - summary="not routed (no ANTHROPIC_BASE_URL in settings env)", + summary="not routed (no ~/.claude/settings.json)", hint="wrap it: headroom wrap claude", ) - return _classify_routing_url(name, base_url, port, source=str(settings_path)) + first_error: CheckResult | None = None + for candidate in existing: + base_url, error = _claude_base_url_in(candidate) + if error is not None: + first_error = first_error or error + continue + if base_url: + return _classify_routing_url(name, base_url, port, source=str(candidate)) + if first_error is not None: + return first_error + return CheckResult( + name=name, + status=WARN, + summary="not routed (no ANTHROPIC_BASE_URL in settings env)", + hint="wrap it: headroom wrap claude", + ) def check_claude_auth_conflict( @@ -404,9 +433,39 @@ def check_codex_routing(config_path: Path, port: int) -> CheckResult: summary=f"routed to port {match.group(1)}, but doctor probed port {port}", hint=f"re-run with: headroom doctor --port {match.group(1)}", ) + # Routed, but Codex may still attach no credentials. A ChatGPT-OAuth user + # needs `requires_openai_auth = true` in the provider block or Codex sends + # no Authorization header at all and every request 401s with "Missing + # bearer" (#3206). That failure is invisible from here -- the proxy is up, + # the block is present -- so this check is the only place it can surface. + if _codex_block_missing_openai_auth(text, config_path): + return CheckResult( + name=name, + status=WARN, + summary="routed, but Codex will send no Authorization (missing requires_openai_auth)", + hint="re-run: headroom wrap codex (or headroom init codex) to rewrite the block", + ) return CheckResult(name=name, status=PASS, summary=f"routed ({config_path})") +def _codex_block_missing_openai_auth(text: str, config_path: Path) -> bool: + """ChatGPT-OAuth Codex routed without ``requires_openai_auth`` (#3206).""" + start = text.find("[model_providers.headroom]") + if start == -1: + return False + rest = text[start + len("[model_providers.headroom]") :] + end = rest.find("\n[") + block = rest if end == -1 else rest[:end] + if "requires_openai_auth" in block: + return False + try: + from headroom.providers.codex.install import codex_uses_chatgpt_auth + + return codex_uses_chatgpt_auth(config_path.parent / "auth.json") + except Exception: # pragma: no cover - never let a doctor check crash + return False + + def check_shell_env(environ: Mapping[str, str], port: int) -> CheckResult: """Is the *current shell* pointed at the proxy for ad-hoc runs?""" name = "shell env" @@ -653,7 +712,11 @@ def doctor(port: int, emit_json: bool) -> None: checks = [ check_proxy_liveness(livez, base_url), check_version_drift(livez, installed), - check_claude_routing(claude_settings_path(), port), + check_claude_routing( + claude_settings_path(), + port, + [project_local_claude_settings, project_claude_settings], + ), check_wrap_marker_staleness(project_local_claude_settings), check_codex_routing(codex_config_path(), port), check_shell_env(os.environ, port), diff --git a/headroom/cli/learn.py b/headroom/cli/learn.py index b17a339ec..6ea29f2ad 100644 --- a/headroom/cli/learn.py +++ b/headroom/cli/learn.py @@ -225,6 +225,7 @@ def learn( total_projects = 0 total_failures = 0 total_recommendations = 0 + total_analysis_failures = 0 matched_projects = 0 available_projects: list[tuple[str, Path]] = [] @@ -299,6 +300,12 @@ def learn( f"Failures: {result_data.total_failures} ({result_data.failure_rate:.1%})" ) + analysis_error = getattr(result_data, "analysis_error", None) + if analysis_error: + total_analysis_failures += 1 + click.echo(f" Analysis failed: {analysis_error}", err=True) + continue + if result_data.failure_rate == 0 and not result_data.recommendations: click.echo(" No failures or patterns found.") continue @@ -350,6 +357,9 @@ def learn( f"{total_recommendations} recommendations" ) + if total_analysis_failures: + raise SystemExit(1) + def _make_llm_judge(model: str) -> Any: """Build an LLM judge callable for verbosity, or None if unavailable. diff --git a/headroom/cli/mcp.py b/headroom/cli/mcp.py index 7563822bb..40e8c25ca 100644 --- a/headroom/cli/mcp.py +++ b/headroom/cli/mcp.py @@ -216,6 +216,52 @@ def mcp_uninstall() -> None: click.echo("Headroom MCP is not configured. Nothing to uninstall.") +@mcp.command("reconcile") +@click.option("--adopt", is_flag=True, help="Replace only the Serena entry with Headroom's spec.") +def mcp_reconcile(adopt: bool) -> None: + """Inspect or explicitly reconcile a user-managed Serena MCP entry.""" + from headroom.mcp_registry import ( + CLAUDE_SERENA_CONTEXT, + ClaudeConfigMutationError, + ClaudeRegistrar, + RegisterStatus, + build_serena_spec, + ) + from headroom.mcp_registry.ledger import ( + LedgerMutationError, + record_install, + validate_ledger_for_mutation, + ) + + registrar = ClaudeRegistrar() + if not registrar.detect(): + raise click.ClickException("claude is not detected") + recommended = build_serena_spec(CLAUDE_SERENA_CONTEXT) + observed = registrar.get_server("serena") + + if adopt: + try: + registrar.validate_configs_for_mutation() + validate_ledger_for_mutation() + except (ClaudeConfigMutationError, LedgerMutationError) as exc: + raise click.ClickException(str(exc)) from exc + if adopt: + result = registrar.register_server(recommended, force=True) + if result.status not in (RegisterStatus.REGISTERED, RegisterStatus.ALREADY): + raise click.ClickException(result.detail or "could not adopt Serena configuration") + record_install("claude", recommended) + click.echo( + "Adopted Headroom's Serena configuration for Claude; unrelated config preserved." + ) + return + + click.echo("Serena reconciliation for Claude") + click.echo(f" observed: {'absent' if observed is None else 'present'}") + click.echo(f" recommendation: {recommended.command} {' '.join(recommended.args)}") + if observed is not None and observed != recommended: + click.echo(" action: use --adopt to replace it") + + @mcp.command("status") def mcp_status() -> None: """Check Headroom MCP configuration status. diff --git a/headroom/cli/wrap.py b/headroom/cli/wrap.py index 638d9e56a..966d150ac 100644 --- a/headroom/cli/wrap.py +++ b/headroom/cli/wrap.py @@ -32,11 +32,11 @@ import subprocess import sys import time import urllib.parse -from collections.abc import Callable +from collections.abc import Callable, Mapping from contextlib import contextmanager from functools import wraps from pathlib import Path -from typing import Any, cast +from typing import Any, NamedTuple, cast from headroom._subprocess import pid_alive, run @@ -1192,6 +1192,235 @@ def _wrap_marker_path(settings_path: Path) -> Path: return settings_path.parent / ".headroom_wrap_marker.json" +def _wrap_owners_path(settings_path: Path) -> Path: + """Sidecar recording which live wrap sessions own each settings env key. + + Separate from ``.headroom_wrap_marker.json`` on purpose: that marker + describes a single writer and is consumed by doctor, unwrap and the + staleness checks. Concurrency ownership is additive state, so it lives in + its own file rather than changing a shape those readers depend on. + """ + return settings_path.parent / ".headroom_wrap_owners.json" + + +def _wrap_settings_lock(settings_path: Path) -> Any: + """Serialize settings read-modify-write across concurrent wrap sessions. + + Writing the proxy URL into ``settings.local.json`` is a read-modify-write, + and several ``headroom wrap`` sessions in one project run it concurrently. + The write itself is atomic, so the file never tears -- but without this the + updates are still lost against each other (#3205). + """ + from contextlib import nullcontext + + lock_path = settings_path.parent / ".headroom_wrap_settings.lock" + try: + lock_path.parent.mkdir(parents=True, exist_ok=True) + lock_file = open(lock_path, "a+b") # noqa: SIM115 + except OSError: + # Matches _proxy_start_lock: a workspace that cannot hold lock state is + # degraded, not unusable. + return nullcontext() + return _locked_file(lock_file) + + +@contextmanager +def _locked_file(lock_file: Any) -> Any: + """Hold an exclusive OS lock on an already-open file for the block. + + Shared by ``_proxy_start_lock`` and ``_wrap_settings_lock`` -- the two + differ only in which file they lock, and an OS-lock dance duplicated per + call site is one place for the platform branches to drift apart. + """ + with lock_file: + if sys.platform == "win32": + import msvcrt + + # msvcrt.locking operates on bytes from the current file position. + lock_file.seek(0) + if lock_file.read(1) == b"": + lock_file.seek(0) + lock_file.write(b"0") + lock_file.flush() + lock_file.seek(0) + # LK_LOCK has implementation-dependent retry limits, and a holder + # may legitimately take longer than that (a proxy loading ML + # components), so use the non-blocking primitive in a loop. + while True: + try: + msvcrt.locking(lock_file.fileno(), msvcrt.LK_NBLCK, 1) + break + except OSError: + time.sleep(0.05) + try: + yield + finally: + lock_file.seek(0) + msvcrt.locking(lock_file.fileno(), msvcrt.LK_UNLCK, 1) + else: + import fcntl + + fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) + try: + yield + finally: + fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) + + +def _read_wrap_owners(settings_path: Path) -> dict[str, Any]: + try: + rec = json.loads(_read_text(_wrap_owners_path(settings_path))) + except (OSError, ValueError): + return {} + return rec if isinstance(rec, dict) else {} + + +def _write_wrap_owners(settings_path: Path, owners: dict[str, Any]) -> None: + target = _wrap_owners_path(settings_path) + try: + if not owners: + target.unlink(missing_ok=True) + return + _write_text(target, json.dumps(owners, indent=2) + "\n") + except OSError: + pass + + +def _live_holders(entry: Any, *, dead_ports: frozenset[int] = frozenset()) -> list[dict[str, Any]]: + """Holders in *entry* whose process is still provably alive. + + Reuses the same conservative liveness the proxy-client markers use: a PID + that is gone, or that is now provably a different process, is dropped. Any + uncertainty keeps the holder, because dropping a live owner is what causes + a running session to be unrouted. + + ``dead_ports`` additionally drops holders whose proxy port the caller has + *proven* dead. A wrapper process outlives its proxy after a hard reboot or + SIGKILL of the proxy alone, and such a holder routes nothing; left in place + it would block the #2221 self-heal from clearing a base_url that now points + at nothing. + """ + if not isinstance(entry, dict): + return [] + holders = entry.get("holders") + if not isinstance(holders, list): + return [] + live: list[dict[str, Any]] = [] + for holder in holders: + if not isinstance(holder, dict): + continue + pid = holder.get("pid") + if not isinstance(pid, int) or not _pid_alive(pid): + continue + if _identity_mismatch(holder.get("start_src"), holder.get("start_time"), pid): + continue + port = holder.get("port") + if isinstance(port, int) and port in dead_ports: + continue + live.append(holder) + return live + + +def _self_holder(port: int | None) -> dict[str, Any]: + ident = _proc_identity(os.getpid()) + return { + "pid": os.getpid(), + "start_src": ident[0] if ident else None, + "start_time": ident[1] if ident else None, + "port": port, + } + + +def _claim_wrap_key( + settings_path: Path, + key: str, + current_value: str | None, + *, + port: int | None = None, +) -> None: + """Register this process as an owner of *key*, recording the true original. + + The first live owner records ``original``; later owners inherit it and are + flagged ``inherited`` so their exit knows the value they happened to + observe was not the pre-wrap one. Without that, a second wrap session + captures the *first session's* proxy URL as the value to restore, and puts + a dead proxy back into the file on exit (#3205). + """ + owners = _read_wrap_owners(settings_path) + entry = owners.get(key) + live = _live_holders(entry) + inherited = bool(live) and isinstance(entry, dict) and "original" in entry + original = entry.get("original") if inherited and isinstance(entry, dict) else current_value + me = _self_holder(port) + me["inherited"] = inherited + live = [h for h in live if h.get("pid") != me["pid"]] + live.append(me) + owners[key] = {"original": original, "holders": live} + _write_wrap_owners(settings_path, owners) + + +class _KeyRelease(NamedTuple): + """Outcome of dropping this process's claim on a settings env key.""" + + should_restore: bool + original: str | None + trust_caller: bool + survivor: dict[str, Any] | None + + +def _release_wrap_key( + settings_path: Path, + key: str, + *, + force: bool = False, + dead_ports: frozenset[int] = frozenset(), +) -> _KeyRelease: + """Drop this process's claim on *key*. + + ``should_restore`` is False while another live wrap session still owns the + key -- restoring then silently unroutes a running session. ``force`` is for + ``unwrap``, where the user is explicitly asking for their settings back: + every claim is dropped and the restore happens regardless. + + ``trust_caller`` says whether the caller's remembered ``previous`` is its + own first-hand observation of the pre-wrap value. True when there is no + owner record at all (unwrap of a pre-upgrade session, and the legacy + callers that pass the value directly), and when this process founded the + record. False for an inheriting holder -- it remembers the *first + session's* proxy URL, so honouring it writes a dead proxy back, the exact + bug #3205 is about -- and false for a caller with no claim of its own, + whose marker-derived value is second-hand where the record is not. + + ``survivor`` is a still-live holder the caller can re-point the + single-slot wrap marker at, so an exiting session does not take the + surviving one's #2221 self-heal record with it. + """ + owners = _read_wrap_owners(settings_path) + entry = owners.get(key) + if not isinstance(entry, dict): + return _KeyRelease(True, None, True, None) + me = os.getpid() + remaining = [h for h in _live_holders(entry, dead_ports=dead_ports) if h.get("pid") != me] + original = entry.get("original") + # Look this process's own claim up in the raw holder list, never the + # liveness-filtered one: the caller is by definition running, and its claim + # is what says whether the value it remembers is first-hand. + raw = entry.get("holders") + mine = ( + next((h for h in raw if isinstance(h, dict) and h.get("pid") == me), None) + if isinstance(raw, list) + else None + ) + trust_caller = mine is not None and not mine.get("inherited") + if remaining and not force: + owners[key] = {"original": original, "holders": remaining} + _write_wrap_owners(settings_path, owners) + return _KeyRelease(False, original, trust_caller, remaining[0]) + owners.pop(key, None) + _write_wrap_owners(settings_path, owners) + return _KeyRelease(True, original, trust_caller, None) + + def _write_wrap_marker(settings_path: Path, *, port: int, key: str, previous: str | None) -> None: """Best-effort record of which (pid, port, key) wrote the base_url entry. @@ -1214,6 +1443,53 @@ def _write_wrap_marker(settings_path: Path, *, port: int, key: str, previous: st pass +def _rehome_wrap_marker( + settings_path: Path, + *, + key: str, + survivor: dict[str, Any] | None, + original: str | None, +) -> None: + """Hand this session's wrap marker to a session that is still running. + + The marker has one slot and the last writer wins it. When that writer exits + while a sibling still owns the key, leaving the marker describes a dead + process, and deleting it strips the survivor of the #2221 dead-proxy + self-heal record. Rewrite it to describe the survivor instead, carrying the + owner record's ``original`` as the value to restore -- the marker's own + ``previous`` may be an earlier session's proxy URL (#3205). + + Only ever touches a marker this process wrote; a sibling's marker is + already accurate. + """ + marker_path = _wrap_marker_path(settings_path) + marker = _read_wrap_marker(settings_path) + if marker is None or marker.get("key") != key or marker.get("pid") != os.getpid(): + return + port = survivor.get("port") if survivor is not None else None + try: + if survivor is None or not isinstance(port, int): + # No survivor to hand it to, or one whose port we never recorded: + # a marker without a usable port is worse than none. + marker_path.unlink(missing_ok=True) + return + _write_text( + marker_path, + json.dumps( + { + "pid": survivor.get("pid"), + "start_src": survivor.get("start_src"), + "start_time": survivor.get("start_time"), + "port": port, + "key": key, + "previous": original, + } + ), + ) + except OSError: + pass + + def _read_wrap_marker(settings_path: Path) -> dict[str, Any] | None: marker = _wrap_marker_path(settings_path) try: @@ -1337,7 +1613,15 @@ def _check_and_clear_dead_wrap_marker(settings_path: Path, *, key: str) -> str | f"running (issue #2221); restoring prior value", err=True, ) - _restore_claude_wrap_base_url(previous, settings_path=settings_path, _key_override=key) + _restore_claude_wrap_base_url( + previous, + settings_path=settings_path, + _key_override=key, + # The wrapper process can outlive its proxy (the proxy alone was + # SIGKILLed). Its ownership claim would otherwise veto this restore and + # leave the base_url pointing at a port proven dead just above (#3205). + dead_ports=frozenset({port}) if isinstance(port, int) else frozenset(), + ) return previous @@ -1503,16 +1787,21 @@ def _write_claude_wrap_base_url( detected and self-healed (issue #1768). """ path = settings_path or (Path.cwd() / ".claude" / "settings.local.json") - payload = _read_settings_for_write(path) - env_map = dict(payload.get("env") or {}) if isinstance(payload.get("env"), dict) else {} key = _claude_wrap_base_url_env_key(foundry_mode=foundry_mode, vertex_mode=vertex_mode) - previous = env_map.get(key) - env_map[key] = proxy_url - payload["env"] = env_map path.parent.mkdir(parents=True, exist_ok=True) - _write_text(path, json.dumps(payload, indent=2) + "\n") - if port is not None: - _write_wrap_marker(path, port=port, key=key, previous=previous) + with _wrap_settings_lock(path): + payload = _read_settings_for_write(path) + env_map = dict(payload.get("env") or {}) if isinstance(payload.get("env"), dict) else {} + previous = env_map.get(key) + # Claim before writing, so the recorded original is the value that was + # there before *any* wrap session touched it -- not the previous + # session's proxy URL (#3205). + _claim_wrap_key(path, key, previous, port=port) + env_map[key] = proxy_url + payload["env"] = env_map + _write_text(path, json.dumps(payload, indent=2) + "\n") + if port is not None: + _write_wrap_marker(path, port=port, key=key, previous=previous) return previous @@ -1525,13 +1814,15 @@ def _write_claude_wrap_tool_search(value: str, *, settings_path: Path | None = N process, and is restored transactionally when the wrap session exits. """ path = settings_path or (Path.cwd() / ".claude" / "settings.local.json") - payload = _read_settings_for_write(path) - env_map = dict(payload.get("env") or {}) if isinstance(payload.get("env"), dict) else {} - previous = env_map.get(_TOOL_SEARCH_ENV) - env_map[_TOOL_SEARCH_ENV] = value - payload["env"] = env_map path.parent.mkdir(parents=True, exist_ok=True) - _write_text(path, json.dumps(payload, indent=2) + "\n") + with _wrap_settings_lock(path): + payload = _read_settings_for_write(path) + env_map = dict(payload.get("env") or {}) if isinstance(payload.get("env"), dict) else {} + previous = env_map.get(_TOOL_SEARCH_ENV) + _claim_wrap_key(path, _TOOL_SEARCH_ENV, previous) + env_map[_TOOL_SEARCH_ENV] = value + payload["env"] = env_map + _write_text(path, json.dumps(payload, indent=2) + "\n") return previous @@ -1553,6 +1844,8 @@ def _restore_claude_wrap_base_url( vertex_mode: bool = False, settings_path: Path | None = None, _key_override: str | None = None, + force: bool = False, + dead_ports: frozenset[int] = frozenset(), ) -> None: """Restore (or remove) the env key written by _write_claude_wrap_base_url. @@ -1561,40 +1854,63 @@ def _restore_claude_wrap_base_url( ``previous`` is None the key is removed; when it has a value it is restored — preserving any URL the project already had set. Also clears this key's sidecar wrap marker, if any (issue #1768). + + Concurrency (#3205): while another live wrap session still owns the key, + this is a no-op — restoring underneath a running session unroutes it. Set + ``force`` when the user has explicitly asked for their settings back + (``unwrap``), and ``dead_ports`` to name proxy ports already proven dead so + holders that outlived their proxy stop counting as live. """ path = settings_path or (Path.cwd() / ".claude" / "settings.local.json") key = _key_override or _claude_wrap_base_url_env_key( foundry_mode=foundry_mode, vertex_mode=vertex_mode ) - if not path.exists(): - _clear_wrap_marker(path, key=key) - return - try: - payload = json.loads(_read_text(path)) - except (OSError, json.JSONDecodeError): - return - if not isinstance(payload, dict): - return - env_map = payload.get("env") - if not isinstance(env_map, dict): - return - if previous is None: - if key not in env_map: + with _wrap_settings_lock(path): + # Another live wrap session in this project may still be using the key. + # Restoring underneath it silently unroutes a running session -- traffic + # bypasses the proxy with no error anywhere (#3205). + release = _release_wrap_key(path, key, force=force, dead_ports=dead_ports) + if not release.should_restore: + # The value stays, but this session's marker must not linger + # describing a process that is gone: hand the slot to a survivor. + _rehome_wrap_marker(path, key=key, survivor=release.survivor, original=release.original) + return + # The owner record holds the value from before *any* wrap session wrote. + # Prefer the caller's own value only when the caller observed it + # first-hand; a session that started second remembers the first + # session's (now dead) proxy URL, and so does the marker an unwrap or a + # self-heal reads it from. + restore_to = previous if release.trust_caller else release.original + + if not path.exists(): _clear_wrap_marker(path, key=key) return - del env_map[key] - if env_map: - payload["env"] = env_map + try: + payload = json.loads(_read_text(path)) + except (OSError, json.JSONDecodeError): + return + if not isinstance(payload, dict): + return + env_map = payload.get("env") + if not isinstance(env_map, dict): + return + if restore_to is None: + if key not in env_map: + _clear_wrap_marker(path, key=key) + return + del env_map[key] + if env_map: + payload["env"] = env_map + else: + payload.pop("env", None) else: - payload.pop("env", None) - else: - env_map[key] = previous - payload["env"] = env_map - if payload: - _write_text(path, json.dumps(payload, indent=2) + "\n") - else: - path.unlink(missing_ok=True) - _clear_wrap_marker(path, key=key) + env_map[key] = restore_to + payload["env"] = env_map + if payload: + _write_text(path, json.dumps(payload, indent=2) + "\n") + else: + path.unlink(missing_ok=True) + _clear_wrap_marker(path, key=key) def _setup_headroom_mcp( @@ -2018,17 +2334,18 @@ def _setup_serena_mcp( spec = build_serena_spec(context) result = registrar.register_server(spec, force=force) + owned_drift = ( + result.status == RegisterStatus.MISMATCH + and not force + and headroom_installed_matching(registrar.name, registrar.get_server("serena")) + ) # Migrate a stale Headroom-installed entry. register_server won't overwrite # a differing spec without force, so an older Headroom Serena entry would # otherwise persist across re-wraps. Force-update it only when the ledger # proves Headroom installed the entry that's currently on disk — never a # user-managed Serena. - if ( - result.status == RegisterStatus.MISMATCH - and not force - and headroom_installed_matching(registrar.name, registrar.get_server("serena")) - ): + if result.status == RegisterStatus.MISMATCH and not force and owned_drift: result = registrar.register_server(spec, force=True) if result.status == RegisterStatus.REGISTERED: click.echo(" Serena MCP: migrated previously-installed entry to current spec") @@ -2041,7 +2358,13 @@ def _setup_serena_mcp( result, label="Serena MCP", verbose=verbose, - overwrite_hint="update or remove the existing serena MCP entry, then rerun headroom wrap", + overwrite_hint=( + "run headroom wrap again" + if owned_drift + else "run headroom mcp reconcile --adopt" + if registrar.name == "claude" + else "update or remove the existing serena MCP entry, then rerun headroom wrap" + ), restart_hint=f"restart {registrar.display_name} if it was already running", ) if line is not None: @@ -3620,6 +3943,20 @@ def _copilot_default_wire_api_for_model(model: str | None) -> str: return _copilot_default_wire_api_for_model_impl(model) +def _build_copilot_native_launch_env( + *, port: int, environ: dict[str, str], project: str | None +) -> tuple[dict[str, str], list[str]]: + from headroom.providers.copilot.wrap import build_native_launch_env + + return build_native_launch_env(port=port, environ=environ, project=project) + + +def _native_api_url_supported(*, environ: Mapping[str, str] | None = None) -> bool | None: + from headroom.providers.copilot.wrap import native_api_url_supported + + return native_api_url_supported(environ=environ) + + def _should_use_copilot_oauth( *, backend: str | None, @@ -4097,39 +4434,8 @@ def _proxy_start_lock(port: int) -> Any: # environment. yield return - with lock_file: - if sys.platform == "win32": - import msvcrt - - # msvcrt.locking operates on bytes from the current file position. - lock_file.seek(0) - if lock_file.read(1) == b"": - lock_file.seek(0) - lock_file.write(b"0") - lock_file.flush() - lock_file.seek(0) - # LK_LOCK has implementation-dependent retry limits. A proxy may - # legitimately take longer than that to load ML components, so - # use the non-blocking primitive in a loop instead. - while True: - try: - msvcrt.locking(lock_file.fileno(), msvcrt.LK_NBLCK, 1) - break - except OSError: - time.sleep(0.05) - try: - yield - finally: - lock_file.seek(0) - msvcrt.locking(lock_file.fileno(), msvcrt.LK_UNLCK, 1) - else: - import fcntl - - fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) - try: - yield - finally: - fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) + with _locked_file(lock_file): + yield @wraps(_ensure_proxy_unlocked) @@ -4323,6 +4629,20 @@ def _ignore_child_sigint(signum: int | None = None, frame: Any = None) -> None: return None +def _exit_on_signal(signum: int | None = None, frame: Any = None) -> None: + """Unwind on SIGTERM/SIGHUP so the ``finally`` block actually runs. + + Registering ``cleanup`` itself as the handler did not achieve what its call + site documented. A Python signal handler that returns normally does not + unwind the stack -- under PEP 475 the interrupted ``waitpid`` is simply + retried -- so the ``finally`` that restores ``settings.local.json`` never + ran, while the handler had already terminated the proxy underneath a child + that was still alive. Raising SystemExit reverses that: the settings are + restored and cleanup runs exactly once, from ``finally`` (#3205). + """ + raise SystemExit(128 + int(signum or 0)) + + def _launch_tool( binary: str, args: tuple, @@ -4340,6 +4660,7 @@ def _launch_tool( anyllm_provider: str | None = None, region: str | None = None, openai_api_url: str | None = None, + anthropic_api_url: str | None = None, copilot_api_token: str | None = None, copilot_refresh_oauth_token: str | None = None, copilot_api_token_expires_at: float | None = None, @@ -4354,7 +4675,7 @@ def _launch_tool( port_holder: list[int] = [port] cleanup = _make_cleanup(proxy_holder, port_holder) signal.signal(signal.SIGINT, _ignore_child_sigint) - signal.signal(signal.SIGTERM, cleanup) + signal.signal(signal.SIGTERM, _exit_on_signal) try: click.echo() @@ -4376,6 +4697,7 @@ def _launch_tool( anyllm_provider=anyllm_provider, region=region, openai_api_url=openai_api_url, + anthropic_api_url=anthropic_api_url, copilot_api_token=copilot_api_token, copilot_refresh_oauth_token=copilot_refresh_oauth_token, copilot_api_token_expires_at=copilot_api_token_expires_at, @@ -4386,7 +4708,7 @@ def _launch_tool( port_holder[0] = actual_port _push_runtime_env(actual_port, no_proxy) - # If port fell back, update env URLs to point at the actual port + # If port fell back, update environment URLs to point at the actual port. if actual_port != port: for k, v in dict(env).items(): env[k] = v.replace(f"127.0.0.1:{port}", f"127.0.0.1:{actual_port}") @@ -4826,11 +5148,11 @@ def claude( ) cleanup = _make_cleanup(proxy_holder, port_holder) signal.signal(signal.SIGINT, _ignore_child_sigint) - signal.signal(signal.SIGTERM, cleanup) + signal.signal(signal.SIGTERM, _exit_on_signal) if hasattr(signal, "SIGHUP"): # Terminal close / tmux kill-session sends SIGHUP, not SIGTERM — without # this, the finally block's base_url restore never runs (issue #1768). - signal.signal(signal.SIGHUP, cleanup) + signal.signal(signal.SIGHUP, _exit_on_signal) # Memory sync BEFORE proxy startup — sync headroom DB ↔ Claude's files if memory: @@ -4932,11 +5254,11 @@ def claude( click.echo(" Skipping MCP retrieve tool (--no-mcp)") # Coding-task compressor: Serena (retires any legacy tokensave entry). - from headroom.mcp_registry import ClaudeRegistrar + from headroom.mcp_registry import CLAUDE_SERENA_CONTEXT, ClaudeRegistrar _setup_coding_compressor( ClaudeRegistrar(), - serena_context="claude-code", + serena_context=CLAUDE_SERENA_CONTEXT, serena=serena, no_serena=no_serena, no_tokensave=no_tokensave, @@ -5215,6 +5537,10 @@ def unwrap_claude( foundry_mode=_foundry, vertex_mode=_vertex, settings_path=_unwrap_settings_path, + # unwrap is the user asking for their settings back, so it drops + # every wrap session's claim rather than deferring to a live + # sibling and silently doing nothing (#3205). + force=True, ) # Issue #2238: unwrap restores settings.local.json, but a proxy URL that was @@ -5297,6 +5623,14 @@ def _require_copilot_subscription_resolution() -> CopilotSubscriptionTokenResolu ), ) @click.option("--memory", is_flag=True, help="Enable persistent cross-session memory") +@click.option( + "--native", + is_flag=True, + help=( + "Route Copilot's own GitHub-authenticated API through Headroom instead of " + "the single-model BYOK override. Keeps native model aliases and /model switching." + ), +) @click.option("--verbose", "-v", is_flag=True, help="Verbose output") @click.argument("copilot_args", nargs=-1, type=click.UNPROCESSED) def copilot( @@ -5309,6 +5643,7 @@ def copilot( wire_api: str | None, subscription: bool, memory: bool, + native: bool, verbose: bool, copilot_args: tuple[str, ...], ) -> None: @@ -5343,6 +5678,7 @@ def copilot( ) raise SystemExit(1) + explicit_subscription = subscription effective_backend = backend or os.environ.get("HEADROOM_BACKEND") if _check_proxy(port): running_backend = _detect_running_proxy_backend(port) @@ -5353,6 +5689,17 @@ def copilot( ) effective_backend = running_backend or effective_backend + if native: + subscription = True + if provider_type == "anthropic": + raise click.ClickException( + "--native does not use the BYOK provider override; drop --provider-type anthropic." + ) + if wire_api is not None: + raise click.ClickException( + "--native selects the wire per request; drop the BYOK-only --wire-api option." + ) + effective_provider_type = _resolve_copilot_provider_type(effective_backend, provider_type) if subscription: if effective_backend not in (None, "", "anthropic"): @@ -5380,12 +5727,22 @@ def copilot( copilot_api_token_expires_at: float | None = None client_bearer: str | None = None subscription_resolution: CopilotSubscriptionTokenResolution | None = None - if _should_use_copilot_oauth( + anthropic_api_url: str | None = None + use_copilot_oauth = _should_use_copilot_oauth( backend=effective_backend, provider_type=provider_type, env=env, force_subscription=subscription, - ): + ) + # Without a provider key, the old implicit OAuth lane still configured + # Copilot as a one-model BYOK client. Native aliases (and runtime /model + # switches) were then forwarded literally and rejected by GitHub (#1910). + # Explicit --subscription remains on its existing fixed-wire behavior; + # implicit GitHub OAuth uses Copilot's own routing automatically. + if use_copilot_oauth and not explicit_subscription: + native = True + + if use_copilot_oauth: if subscription: subscription_resolution = _require_copilot_subscription_resolution() client_bearer = subscription_resolution.token @@ -5398,7 +5755,35 @@ def copilot( "GITHUB_COPILOT_TOKEN / GITHUB_COPILOT_GITHUB_TOKEN." ) - selected_model = _copilot_model_from_args(copilot_args, env) + if native: + openai_api_url = ( + subscription_resolution.api_url + if subscription_resolution is not None + else resolve_copilot_api_url(client_bearer) + ) + env, env_vars_display = _build_copilot_native_launch_env( + port=port, + environ=env, + project=_project_name_from_cwd(), + ) + env["GITHUB_COPILOT_API_URL"] = openai_api_url + env["OPENAI_TARGET_API_URL"] = openai_api_url + env["ANTHROPIC_TARGET_API_URL"] = openai_api_url + anthropic_api_url = openai_api_url + copilot_proxy_token = client_bearer + if subscription_resolution is not None: + copilot_refresh_oauth_token = subscription_resolution.refresh_oauth_token + copilot_api_token_expires_at = subscription_resolution.api_token_expires_at + support = _native_api_url_supported(environ=os.environ) + if support is False: + raise click.ClickException( + "This Copilot CLI build does not reference COPILOT_API_URL; refusing " + "a native launch that could silently bypass Headroom." + ) + if support is None and verbose: + click.echo(" Note: could not verify this Copilot CLI's COPILOT_API_URL support.") + else: + selected_model = _copilot_model_from_args(copilot_args, env) # ``--model auto`` is a Copilot-internal routing token that the BYOK # API rejects with ``400 The requested model is not supported``. In @@ -5406,7 +5791,7 @@ def copilot( # Copilot's own native auto-selection works fine — we just need to # strip the ``--model auto`` flag before launch so Copilot doesn't # forward it to the provider endpoint. - if _is_auto_model(selected_model): + if not native and _is_auto_model(selected_model): copilot_args = _strip_auto_model_args(copilot_args) selected_model = None click.echo( @@ -5415,57 +5800,58 @@ def copilot( "automatic model selection." ) - env_wire_api = env.get("COPILOT_PROVIDER_WIRE_API") - effective_wire_api = wire_api or ( - env_wire_api - if env_wire_api in {"completions", "responses"} - else _copilot_default_wire_api_for_model(selected_model) - ) - env["COPILOT_PROVIDER_TYPE"] = "openai" - # Per-project savings: the Copilot CLI cannot send custom headers, so - # the project rides as a /p/ base-URL prefix the proxy strips. - env["COPILOT_PROVIDER_BASE_URL"] = _with_project_prefix( - f"http://127.0.0.1:{port}/v1", _project_name_from_cwd() - ) - env["COPILOT_PROVIDER_WIRE_API"] = effective_wire_api - env["COPILOT_PROVIDER_BEARER_TOKEN"] = client_bearer - env["GITHUB_COPILOT_USE_TOKEN_EXCHANGE"] = "false" - env.pop("COPILOT_PROVIDER_API_KEY", None) - # Hand the exact token we resolved (and, for --subscription, validated - # against GitHub) to the proxy explicitly via copilot_proxy_token below. - # The proxy pins it as GITHUB_COPILOT_API_TOKEN, so upstream auth is - # deterministic instead of the proxy re-running unvalidated discovery - # (read_cached_oauth_token returns the *first* candidate, which may not - # be the one the wrapper approved → environment-dependent 401s). Passing - # it as a launch argument — rather than mutating this process's global - # os.environ — keeps the token off shared state and out of unrelated - # code paths. - copilot_proxy_token = client_bearer - if subscription_resolution is not None: - copilot_refresh_oauth_token = subscription_resolution.refresh_oauth_token - copilot_api_token_expires_at = subscription_resolution.api_token_expires_at - env_vars_display = [ - "COPILOT_PROVIDER_TYPE=openai", - f"COPILOT_PROVIDER_BASE_URL={env['COPILOT_PROVIDER_BASE_URL']}", - f"COPILOT_PROVIDER_WIRE_API={effective_wire_api}", - ( - "COPILOT_AUTH_MODE=github-subscription-experimental" - if subscription - else "COPILOT_AUTH_MODE=github-oauth" - ), - ] - # Non-subscription OAuth keeps upstream's generic-host policy from - # #610. Subscription mode can use the endpoint returned by the Copilot - # token exchange, which is how Business accounts advertise their API - # host without requiring users to configure it manually. - openai_api_url = ( - subscription_resolution.api_url - if subscription_resolution is not None - else resolve_copilot_api_url(client_bearer) - ) - env["GITHUB_COPILOT_API_URL"] = openai_api_url - env["OPENAI_TARGET_API_URL"] = openai_api_url - env_vars_display.append(f"COPILOT_PROVIDER_API_URL={openai_api_url}") + if not native: + env_wire_api = env.get("COPILOT_PROVIDER_WIRE_API") + effective_wire_api = wire_api or ( + env_wire_api + if env_wire_api in {"completions", "responses"} + else _copilot_default_wire_api_for_model(selected_model) + ) + env["COPILOT_PROVIDER_TYPE"] = "openai" + # Per-project savings: the Copilot CLI cannot send custom headers, so + # the project rides as a /p/ base-URL prefix the proxy strips. + env["COPILOT_PROVIDER_BASE_URL"] = _with_project_prefix( + f"http://127.0.0.1:{port}/v1", _project_name_from_cwd() + ) + env["COPILOT_PROVIDER_WIRE_API"] = effective_wire_api + env["COPILOT_PROVIDER_BEARER_TOKEN"] = client_bearer + env["GITHUB_COPILOT_USE_TOKEN_EXCHANGE"] = "false" + env.pop("COPILOT_PROVIDER_API_KEY", None) + # Hand the exact token we resolved (and, for --subscription, validated + # against GitHub) to the proxy explicitly via copilot_proxy_token below. + # The proxy pins it as GITHUB_COPILOT_API_TOKEN, so upstream auth is + # deterministic instead of the proxy re-running unvalidated discovery + # (read_cached_oauth_token returns the *first* candidate, which may not + # be the one the wrapper approved → environment-dependent 401s). Passing + # it as a launch argument — rather than mutating this process's global + # os.environ — keeps the token off shared state and out of unrelated + # code paths. + copilot_proxy_token = client_bearer + if subscription_resolution is not None: + copilot_refresh_oauth_token = subscription_resolution.refresh_oauth_token + copilot_api_token_expires_at = subscription_resolution.api_token_expires_at + env_vars_display = [ + "COPILOT_PROVIDER_TYPE=openai", + f"COPILOT_PROVIDER_BASE_URL={env['COPILOT_PROVIDER_BASE_URL']}", + f"COPILOT_PROVIDER_WIRE_API={effective_wire_api}", + ( + "COPILOT_AUTH_MODE=github-subscription-experimental" + if subscription + else "COPILOT_AUTH_MODE=github-oauth" + ), + ] + # Non-subscription OAuth keeps upstream's generic-host policy from + # #610. Subscription mode can use the endpoint returned by the Copilot + # token exchange, which is how Business accounts advertise their API + # host without requiring users to configure it manually. + openai_api_url = ( + subscription_resolution.api_url + if subscription_resolution is not None + else resolve_copilot_api_url(client_bearer) + ) + env["GITHUB_COPILOT_API_URL"] = openai_api_url + env["OPENAI_TARGET_API_URL"] = openai_api_url + env_vars_display.append(f"COPILOT_PROVIDER_API_URL={openai_api_url}") else: env, env_vars_display = _build_copilot_launch_env( port=port, @@ -5488,7 +5874,7 @@ def copilot( ) raise SystemExit(1) - if not subscription and not _copilot_model_configured(copilot_args, env): + if not subscription and not native and not _copilot_model_configured(copilot_args, env): # Distinguish between "--model auto" (wrong model for BYOK) and # genuinely missing model (no --model flag at all). raw_model = _copilot_model_from_args(copilot_args, env) @@ -5525,6 +5911,7 @@ def copilot( anyllm_provider=anyllm_provider, region=region, openai_api_url=openai_api_url, + anthropic_api_url=anthropic_api_url, copilot_api_token=copilot_proxy_token, copilot_refresh_oauth_token=copilot_refresh_oauth_token, copilot_api_token_expires_at=copilot_api_token_expires_at, @@ -6295,9 +6682,10 @@ def kimi( """Launch Kimi CLI through Headroom proxy. \b - Sets KIMI_BASE_URL to route Kimi's OpenAI-compatible /chat/completions - traffic through Headroom. Kimi's own OAuth bearer is forwarded upstream, - so no extra login is required — run `kimi` once to authenticate first. + Sets KIMI_CODE_BASE_URL for managed Kimi Code and KIMI_BASE_URL for legacy + kimi-cli to route OpenAI-compatible /chat/completions traffic through + Headroom. Managed Kimi Code needs one `/login` after the proxy URL changes + so its OAuth slot matches that URL; legacy kimi-cli keeps its existing login. \b Examples: @@ -6315,9 +6703,20 @@ def kimi( click.echo("Install Kimi CLI: https://github.com/MoonshotAI/kimi-cli") raise SystemExit(1) - env, env_vars_display = _build_kimi_launch_env( - port, os.environ, project=_project_name_from_cwd() - ) + project = _project_name_from_cwd() + env, env_vars_display = _build_kimi_launch_env(port, os.environ, project=project) + + def configure_kimi_launch( + actual_port: int, + current_args: tuple, + current_env: dict[str, str], + current_display: list[str], + ) -> tuple[tuple, dict[str, str], list[str]]: + del current_display + updated_env, updated_display = _build_kimi_launch_env( + actual_port, current_env, project=project + ) + return current_args, updated_env, updated_display _launch_tool( binary=kimi_bin, @@ -6332,6 +6731,7 @@ def kimi( agent_type="kimi", code_graph=code_graph, openai_api_url=kimi_api_url, + configure_launch=configure_kimi_launch, ) diff --git a/headroom/config.py b/headroom/config.py index 123cc9e93..b1dc607eb 100644 --- a/headroom/config.py +++ b/headroom/config.py @@ -228,6 +228,9 @@ DEFAULT_EXCLUDE_TOOLS: frozenset[str] = frozenset( "WebSearch", "WebFetch", "headroom_retrieve", + # Copilot CLI's file-read tool (its `Read` equivalent): raw file bytes + # the model byte-patches against. + "view", # Lowercase variants for case-insensitive matching "read", "glob", @@ -253,6 +256,10 @@ DEFAULT_VERBATIM_EXCLUDE_TOOLS: frozenset[str] = frozenset( "web_search", "web_fetch", "headroom_retrieve", + # `view` (Copilot CLI file read) must stay BYTE-EXACT: the model produces + # line/byte-precise edits against it, and even "lossless" JSON rewrites + # or cross-turn dedup folds break old_str matching and force re-reads. + "view", } ) diff --git a/headroom/copilot_auth.py b/headroom/copilot_auth.py index 2603577b5..fd9a5504c 100644 --- a/headroom/copilot_auth.py +++ b/headroom/copilot_auth.py @@ -25,6 +25,7 @@ from headroom import paths from headroom._subprocess import run from headroom.copilot_linux_secret import read_copilot_oauth_token as read_linux_secret_token from headroom.copilot_macos_keychain import read_copilot_oauth_token as read_macos_keychain_token +from headroom.proxy import ssl_context as proxy_ssl_context logger = logging.getLogger(__name__) @@ -76,6 +77,15 @@ _OAUTH_TOKEN_KEYS = ( _EXPIRY_KEYS = ("expires_at", "expiresAt", "expiry", "expires") +def _urlopen(request: urllib_request.Request, *, timeout: float) -> Any: + """Open a GitHub request with Headroom's configured corporate trust roots.""" + + context = proxy_ssl_context.build_urlopen_context() + if context is not None: + return urllib_request.urlopen(request, timeout=timeout, context=context) + return urllib_request.urlopen(request, timeout=timeout) + + @dataclass(frozen=True) class CopilotAPIToken: """Short-lived API token exchanged from a GitHub OAuth token.""" @@ -662,7 +672,7 @@ def start_copilot_device_authorization( }, method="POST", ) - with urllib_request.urlopen(request, timeout=timeout) as response: + with _urlopen(request, timeout=timeout) as response: payload = json.loads(response.read().decode("utf-8", errors="replace")) if not isinstance(payload, dict): raise RuntimeError("GitHub device authorization returned an invalid response.") @@ -700,7 +710,7 @@ def poll_copilot_device_authorization( }, method="POST", ) - with urllib_request.urlopen(request, timeout=timeout) as response: + with _urlopen(request, timeout=timeout) as response: payload = json.loads(response.read().decode("utf-8", errors="replace")) if not isinstance(payload, dict): raise RuntimeError("GitHub device authorization returned an invalid response.") @@ -1341,7 +1351,7 @@ def _fetch_copilot_user_info(token: str) -> dict[str, Any] | None: headers = _copilot_token_exchange_headers(token) request = urllib_request.Request(_user_info_url(), headers=headers, method="GET") try: - with urllib_request.urlopen(request, timeout=10.0) as response: + with _urlopen(request, timeout=10.0) as response: payload = json.loads(response.read().decode("utf-8")) except Exception as exc: logger.debug("Unable to resolve Copilot API URL from user info: %s", exc) @@ -1457,7 +1467,7 @@ class CopilotTokenProvider: def _exchange_token_sync(headers: dict[str, str]) -> dict[str, Any]: request = urllib_request.Request(_token_exchange_url(), headers=headers, method="GET") try: - with urllib_request.urlopen(request, timeout=10.0) as response: + with _urlopen(request, timeout=10.0) as response: payload = json.loads(response.read().decode("utf-8")) if not isinstance(payload, dict): return {} diff --git a/headroom/learn/analyzer.py b/headroom/learn/analyzer.py index 670b5c2b6..5fdb44a0e 100644 --- a/headroom/learn/analyzer.py +++ b/headroom/learn/analyzer.py @@ -56,7 +56,7 @@ _MAX_DIGEST_TOKENS = 80_000 # Budget for the digest (leave room for prompt + ou _CLI_BACKENDS: list[tuple[str, str, list[str]]] = [ ("claude", "claude-cli", ["claude", "-p", "--output-format", "stream-json", "--verbose"]), ("gemini", "gemini-cli", ["gemini", "-p"]), - ("codex", "codex-cli", ["codex", "exec"]), + ("codex", "codex-cli", ["codex", "exec", "--skip-git-repo-check"]), ] # Set of valid CLI model identifiers, derived from _CLI_BACKENDS. @@ -202,7 +202,9 @@ class SessionAnalyzer: result.recommendations.sort(key=lambda r: r.estimated_tokens_saved, reverse=True) except Exception as e: logger.warning("LLM analysis failed: %s", e) - # Return result with stats but no recommendations + # Preserve the stats so multi-project runs can continue, but retain + # the failure so the CLI cannot report an empty result as success. + result.analysis_error = str(e) or type(e).__name__ return result diff --git a/headroom/learn/models.py b/headroom/learn/models.py index 5af68211e..6852f38f8 100644 --- a/headroom/learn/models.py +++ b/headroom/learn/models.py @@ -174,6 +174,7 @@ class AnalysisResult: total_calls: int = 0 total_failures: int = 0 recommendations: list[Recommendation] = field(default_factory=list) + analysis_error: str | None = None @property def failure_rate(self) -> float: diff --git a/headroom/mcp_registry/__init__.py b/headroom/mcp_registry/__init__.py index 5f1ff4f8f..f19ad1ba7 100644 --- a/headroom/mcp_registry/__init__.py +++ b/headroom/mcp_registry/__init__.py @@ -14,11 +14,12 @@ without changing the calling code. from __future__ import annotations from .base import MCPRegistrar, RegisterResult, RegisterStatus, ServerSpec -from .claude import ClaudeRegistrar +from .claude import ClaudeConfigMutationError, ClaudeRegistrar from .codex import CodexRegistrar from .display import any_succeeded, format_result, format_results from .grok import GrokRegistrar from .install import ( + CLAUDE_SERENA_CONTEXT, DEFAULT_PROXY_URL, build_headroom_spec, build_serena_spec, @@ -30,6 +31,8 @@ from .server_json import build_server_json, render_server_json __all__ = [ "DEFAULT_PROXY_URL", + "CLAUDE_SERENA_CONTEXT", + "ClaudeConfigMutationError", "ClaudeRegistrar", "CodexRegistrar", "GrokRegistrar", diff --git a/headroom/mcp_registry/claude.py b/headroom/mcp_registry/claude.py index 7acbc02fb..de7ee308c 100644 --- a/headroom/mcp_registry/claude.py +++ b/headroom/mcp_registry/claude.py @@ -26,6 +26,10 @@ from .base import MCPRegistrar, RegisterResult, RegisterStatus, ServerSpec logger = logging.getLogger(__name__) +class ClaudeConfigMutationError(ValueError): + """Raised when a Claude config cannot be safely changed.""" + + class ClaudeRegistrar(MCPRegistrar): """Register MCP servers with Claude Code.""" @@ -84,6 +88,34 @@ class ClaudeRegistrar(MCPRegistrar): return entry return None + def validate_configs_for_mutation(self) -> None: + """Validate every Claude config root before an explicit mutation.""" + seen: set[Path] = set() + for config_path in (self._modern_config, self._legacy_config): + if config_path in seen or not config_path.exists(): + continue + seen.add(config_path) + try: + raw = config_path.read_text(encoding="utf-8") + except OSError as exc: + raise ClaudeConfigMutationError( + f"could not read Claude config {config_path}: {exc}" + ) from exc + try: + config = json.loads(raw) + except json.JSONDecodeError as exc: + raise ClaudeConfigMutationError( + f"Claude config {config_path} is not valid JSON; refusing to mutate" + ) from exc + if not isinstance(config, dict): + raise ClaudeConfigMutationError( + f"Claude config {config_path} must contain a JSON object" + ) + if "mcpServers" in config and not isinstance(config["mcpServers"], dict): + raise ClaudeConfigMutationError( + f"Claude config {config_path} has a non-object mcpServers; refusing to mutate" + ) + def register_server(self, spec: ServerSpec, *, force: bool = False) -> RegisterResult: existing = self.get_server(spec.name) if existing is not None: diff --git a/headroom/mcp_registry/install.py b/headroom/mcp_registry/install.py index e550f6ded..4d1fbeb72 100644 --- a/headroom/mcp_registry/install.py +++ b/headroom/mcp_registry/install.py @@ -14,6 +14,7 @@ from .opencode import OpencodeRegistrar #: Default proxy URL used when none is given. DEFAULT_PROXY_URL = "http://127.0.0.1:8787" +CLAUDE_SERENA_CONTEXT = "claude-code" def get_all_registrars() -> list[MCPRegistrar]: diff --git a/headroom/mcp_registry/ledger.py b/headroom/mcp_registry/ledger.py index 6bb0160e4..e802227e7 100644 --- a/headroom/mcp_registry/ledger.py +++ b/headroom/mcp_registry/ledger.py @@ -21,6 +21,10 @@ from .base import ServerSpec _LEDGER_FILE = "mcp_installs.json" +class LedgerMutationError(ValueError): + """Raised when a ledger cannot be safely updated.""" + + def ledger_path() -> Path: """Return the Headroom MCP install ledger path.""" return paths.workspace_dir() / _LEDGER_FILE @@ -41,9 +45,17 @@ def spec_fingerprint(spec: ServerSpec) -> str: def record_install(agent: str, spec: ServerSpec, *, path: Path | None = None) -> None: """Record that Headroom installed ``spec`` for ``agent``.""" ledger_file = path or ledger_path() + # Automatic installs must recover from a stale or damaged ledger. The + # explicit reconcile route performs strict validation before config writes. data = _read_ledger(ledger_file) - agents = data.setdefault("agents", {}) - agent_entry = agents.setdefault(agent, {}) + agents = data.get("agents") + if not isinstance(agents, dict): + agents = {} + data["agents"] = agents + agent_entry = agents.get(agent) + if not isinstance(agent_entry, dict): + agent_entry = {} + agents[agent] = agent_entry agent_entry[spec.name] = { "fingerprint": spec_fingerprint(spec), "installed_at": datetime.now(timezone.utc).isoformat(), @@ -89,16 +101,45 @@ def headroom_installed_matching( return entry.get("fingerprint") == spec_fingerprint(current_spec) -def _read_ledger(path: Path) -> dict[str, Any]: +def validate_ledger_for_mutation(path: Path | None = None) -> None: + """Reject malformed ledger structure before a config mutation.""" + _read_ledger(path or ledger_path(), for_mutation=True) + + +def _read_ledger(path: Path, *, for_mutation: bool = False) -> dict[str, Any]: try: raw = path.read_text(encoding="utf-8") - except OSError: + except FileNotFoundError: + return {} + except OSError as exc: + if for_mutation: + raise LedgerMutationError(f"MCP install ledger is unreadable: {path}") from exc return {} try: data = json.loads(raw) - except json.JSONDecodeError: + except json.JSONDecodeError as exc: + if for_mutation: + raise LedgerMutationError(f"MCP install ledger is invalid JSON: {path}") from exc return {} - return data if isinstance(data, dict) else {} + if not isinstance(data, dict): + if for_mutation: + raise LedgerMutationError("MCP install ledger must contain a JSON object") + return {} + if for_mutation: + for section in ("agents",): + section_data = data.get(section) + if not isinstance(section_data, dict) or any( + not isinstance(agent_entry, dict) + or any( + not isinstance(server_entry, dict) + or not isinstance(server_entry.get("fingerprint"), str) + or not isinstance(server_entry.get("installed_at"), str) + for server_entry in agent_entry.values() + ) + for agent_entry in section_data.values() + ): + raise LedgerMutationError(f"MCP install ledger section {section!r} is malformed") + return data def _write_ledger(path: Path, data: dict[str, Any]) -> None: diff --git a/headroom/memory/adapters/sqlite_graph.py b/headroom/memory/adapters/sqlite_graph.py index 2c2ac432c..8c74bde6f 100644 --- a/headroom/memory/adapters/sqlite_graph.py +++ b/headroom/memory/adapters/sqlite_graph.py @@ -14,6 +14,7 @@ This is a drop-in replacement for InMemoryGraphStore that: from __future__ import annotations import json +import logging import sqlite3 from collections import deque from datetime import datetime @@ -26,6 +27,8 @@ from .graph_models import Entity, Relationship, RelationshipDirection, Subgraph if TYPE_CHECKING: from ..tracker import ComponentStats +logger = logging.getLogger(__name__) + class SQLiteGraphStore: """SQLite-based graph store implementing the GraphStore protocol. @@ -165,19 +168,31 @@ class SQLiteGraphStore: "metadata": json.dumps(entity.metadata), } - def _row_to_entity(self, row: sqlite3.Row) -> Entity: - """Convert database row to Entity object.""" - return Entity( - id=row["id"], - user_id=row["user_id"], - name=row["name"], - entity_type=row["entity_type"], - description=row["description"], - properties=json.loads(row["properties"]), - created_at=datetime.fromisoformat(row["created_at"]), - updated_at=datetime.fromisoformat(row["updated_at"]), - metadata=json.loads(row["metadata"]), - ) + def _row_to_entity(self, row: sqlite3.Row) -> Entity | None: + """Convert a database row to an Entity, or None if the row is corrupt. + + ``properties``/``metadata`` (JSON) and ``created_at``/``updated_at`` + (ISO timestamps) are parsed from stored text. A single unparseable row — + from a partial write, a manual edit, or a bad migration — must not abort + an entire multi-row scan (``query_subgraph``, neighbour expansion): one + corrupt edge would otherwise make an unrelated part of the graph + unqueryable. Skip the bad row instead. + """ + try: + return Entity( + id=row["id"], + user_id=row["user_id"], + name=row["name"], + entity_type=row["entity_type"], + description=row["description"], + properties=json.loads(row["properties"]), + created_at=datetime.fromisoformat(row["created_at"]), + updated_at=datetime.fromisoformat(row["updated_at"]), + metadata=json.loads(row["metadata"]), + ) + except (ValueError, TypeError, KeyError) as exc: + logger.warning("skipping corrupt entity row %r: %s", row["id"], exc) + return None def _relationship_to_row(self, relationship: Relationship) -> dict[str, Any]: """Convert Relationship object to row dict for insertion.""" @@ -193,19 +208,29 @@ class SQLiteGraphStore: "metadata": json.dumps(relationship.metadata), } - def _row_to_relationship(self, row: sqlite3.Row) -> Relationship: - """Convert database row to Relationship object.""" - return Relationship( - id=row["id"], - user_id=row["user_id"], - source_id=row["source_id"], - target_id=row["target_id"], - relation_type=row["relation_type"], - weight=row["weight"], - properties=json.loads(row["properties"]), - created_at=datetime.fromisoformat(row["created_at"]), - metadata=json.loads(row["metadata"]), - ) + def _row_to_relationship(self, row: sqlite3.Row) -> Relationship | None: + """Convert a database row to a Relationship, or None if the row is corrupt. + + Same contract as :meth:`_row_to_entity`: a single unparseable relationship + row (bad ``properties``/``metadata`` JSON or ``created_at`` timestamp) must + not abort a whole ``get_relationships`` / ``query_subgraph`` scan and take + unrelated edges down with it. Skip the bad row instead. + """ + try: + return Relationship( + id=row["id"], + user_id=row["user_id"], + source_id=row["source_id"], + target_id=row["target_id"], + relation_type=row["relation_type"], + weight=row["weight"], + properties=json.loads(row["properties"]), + created_at=datetime.fromisoformat(row["created_at"]), + metadata=json.loads(row["metadata"]), + ) + except (ValueError, TypeError, KeyError) as exc: + logger.warning("skipping corrupt relationship row %r: %s", row["id"], exc) + return None # ========================================================================= # Entity Operations @@ -373,7 +398,9 @@ class SQLiteGraphStore: params, ) - return [self._row_to_relationship(row) for row in cursor] + return [ + rel for row in cursor if (rel := self._row_to_relationship(row)) is not None + ] async def delete_relationship(self, relationship_id: str) -> bool: """Delete a single relationship. @@ -436,9 +463,12 @@ class SQLiteGraphStore: ) row = cursor.fetchone() if row is not None: + entity = self._row_to_entity(row) + if entity is None: + continue queue.append((entity_id, 0)) visited.add(entity_id) - collected_entities[entity_id] = self._row_to_entity(row) + collected_entities[entity_id] = entity # BFS traversal while queue: @@ -470,6 +500,8 @@ class SQLiteGraphStore: for rel_row in cursor: rel = self._row_to_relationship(rel_row) + if rel is None: + continue # Add relationship collected_relationships[rel.id] = rel @@ -496,8 +528,11 @@ class SQLiteGraphStore: ) neighbor_row = neighbor_cursor.fetchone() if neighbor_row is not None: + neighbor = self._row_to_entity(neighbor_row) + if neighbor is None: + continue visited.add(neighbor_id) - collected_entities[neighbor_id] = self._row_to_entity(neighbor_row) + collected_entities[neighbor_id] = neighbor queue.append((neighbor_id, depth + 1)) return Subgraph( @@ -651,7 +686,9 @@ class SQLiteGraphStore: "SELECT * FROM entities WHERE user_id = ?", (user_id,), ) - return [self._row_to_entity(row) for row in cursor] + return [ + entity for row in cursor if (entity := self._row_to_entity(row)) is not None + ] async def clear(self) -> None: """Clear all data from the store.""" diff --git a/headroom/providers/codex/install.py b/headroom/providers/codex/install.py index b5da67833..234ef70fc 100644 --- a/headroom/providers/codex/install.py +++ b/headroom/providers/codex/install.py @@ -2,11 +2,13 @@ from __future__ import annotations +import base64 import json import os import re import subprocess from pathlib import Path +from typing import Any try: import tomllib @@ -99,10 +101,47 @@ def codex_uses_chatgpt_auth(auth_path: Path) -> bool: tokens = data.get("tokens") if isinstance(tokens, dict): account_id = tokens.get("account_id") - return isinstance(account_id, str) and bool(account_id.strip()) + if isinstance(account_id, str) and account_id.strip(): + return True + return _id_token_carries_chatgpt_account(tokens.get("id_token")) return False +def _id_token_carries_chatgpt_account(raw: Any) -> bool: + """Whether an ``id_token`` carries the ChatGPT account claim (#3206). + + Newer Codex releases can write an ``auth.json`` with neither ``auth_mode`` + nor a top-level ``tokens.account_id``; the account identity lives only in + the ``id_token`` claims. Those configs then read as API-key mode, so + ``requires_openai_auth`` is omitted, Codex attaches no Authorization + header, and every request 401s with "Missing bearer". + + The payload is decoded, not verified. This is a local config file the user + already owns, and the result only decides which key we write into their own + ``config.toml`` -- nothing is authenticated or authorised on the strength + of it. An API-key user has no ChatGPT id_token, so this cannot resurrect + the forced-OAuth-login regression in #406. + """ + if not isinstance(raw, str): + return False + parts = raw.split(".") + if len(parts) != 3: + return False + payload = parts[1] + payload += "=" * (-len(payload) % 4) + try: + claims = json.loads(base64.urlsafe_b64decode(payload.encode("ascii"))) + except Exception: + return False + if not isinstance(claims, dict): + return False + auth_claim = claims.get("https://api.openai.com/auth") + if not isinstance(auth_claim, dict): + return False + account_id = auth_claim.get("chatgpt_account_id") + return isinstance(account_id, str) and bool(account_id.strip()) + + def build_provider_section( *, port: int, diff --git a/headroom/providers/copilot/wrap.py b/headroom/providers/copilot/wrap.py index db1610a3b..e73ec3110 100644 --- a/headroom/providers/copilot/wrap.py +++ b/headroom/providers/copilot/wrap.py @@ -156,6 +156,73 @@ def provider_key_source(provider_type: str) -> str: return "ANTHROPIC_API_KEY" if provider_type == "anthropic" else "OPENAI_API_KEY" +COPILOT_NATIVE_API_URL_ENV = "COPILOT_API_URL" + +# Any survivor keeps Copilot in its single-model BYOK lane, defeating native +# model routing while making the launch look superficially successful. +COPILOT_BYOK_ENV_VARS: tuple[str, ...] = ( + "COPILOT_PROVIDER_BASE_URL", + "COPILOT_PROVIDER_TYPE", + "COPILOT_PROVIDER_API_KEY", + "COPILOT_PROVIDER_BEARER_TOKEN", + "COPILOT_PROVIDER_WIRE_API", + "COPILOT_PROVIDER_TRANSPORT", + "COPILOT_PROVIDER_AZURE_API_VERSION", + "COPILOT_PROVIDER_MODEL_ID", + "COPILOT_PROVIDER_WIRE_MODEL", + "COPILOT_PROVIDER_MODEL_LIMITS_ID", + "COPILOT_PROVIDER_MAX_PROMPT_TOKENS", + "COPILOT_PROVIDER_MAX_OUTPUT_TOKENS", + "COPILOT_PROVIDER_HEADERS", +) + + +def build_native_launch_env( + *, + port: int, + environ: Mapping[str, str] | None = None, + project: str | None = None, +) -> tuple[dict[str, str], list[str]]: + """Redirect Copilot's native API surface through Headroom, not BYOK.""" + env = dict(environ if environ is not None else os.environ) + base_url = with_project_prefix(f"http://127.0.0.1:{port}", project) + env[COPILOT_NATIVE_API_URL_ENV] = base_url + for variable in COPILOT_BYOK_ENV_VARS: + env.pop(variable, None) + return env, [ + f"{COPILOT_NATIVE_API_URL_ENV}={base_url}", + "COPILOT_AUTH_MODE=github-native", + ] + + +def native_api_url_supported(*, environ: Mapping[str, str] | None = None) -> bool | None: + """Best-effort tri-state probe for the CLI's native API URL override.""" + env = environ if environ is not None else os.environ + local = env.get("LOCALAPPDATA") or env.get("HOME") or os.path.expanduser("~") + roots = ( + os.path.join(local, "copilot", "pkg"), + os.path.join(os.path.expanduser("~"), ".local", "share", "copilot", "pkg"), + ) + found_bundle = False + for root in roots: + if not os.path.isdir(root): + continue + for dirpath, _dirnames, filenames in os.walk(root): + if "app.js" not in filenames: + continue + found_bundle = True + try: + with open( + os.path.join(dirpath, "app.js"), encoding="utf-8", errors="replace" + ) as bundle: + while chunk := bundle.read(1 << 20): + if COPILOT_NATIVE_API_URL_ENV in chunk: + return True + except OSError: + continue + return False if found_bundle else None + + def build_launch_env( *, port: int, diff --git a/headroom/providers/kimi/runtime.py b/headroom/providers/kimi/runtime.py index fa2a30678..1d89e7a8f 100644 --- a/headroom/providers/kimi/runtime.py +++ b/headroom/providers/kimi/runtime.py @@ -18,11 +18,13 @@ def build_launch_env( Kimi CLI (``kimi`` / ``kimi-cli``) talks to its managed coding endpoint with an OpenAI-compatible ``/chat/completions`` client (``kosong``'s ``Kimi`` - provider wraps ``AsyncOpenAI``). Its base URL is overridable via the - ``KIMI_BASE_URL`` environment variable, so we point it at the local proxy. + provider wraps ``AsyncOpenAI``). Its base URL is overridable via + ``KIMI_CODE_BASE_URL`` for the managed client and ``KIMI_BASE_URL`` for + legacy clients, so both point at the local proxy. The proxy forwards the request — including Kimi's own OAuth ``Authorization`` - bearer (passthrough auth mode) — to the real upstream configured by - ``--openai-api-url`` (``https://api.kimi.com/coding/v1``). + bearer after the managed client completes its proxy-scoped ``/login`` — to + the real upstream configured by ``--openai-api-url`` + (``https://api.kimi.com/coding/v1``). ``project`` (the wrap launch directory) is encoded as a ``/p/`` base-URL prefix because the Kimi base-URL override cannot carry custom @@ -30,5 +32,9 @@ def build_launch_env( """ env = dict(environ or os.environ) base_url = with_project_prefix(codex_proxy_base_url(port), project) + env["KIMI_CODE_BASE_URL"] = base_url env["KIMI_BASE_URL"] = base_url - return env, [f"KIMI_BASE_URL={base_url}"] + return env, [ + f"KIMI_CODE_BASE_URL={base_url}", + f"KIMI_BASE_URL={base_url}", + ] diff --git a/headroom/proxy/handlers/openai.py b/headroom/proxy/handlers/openai.py index cb2f70766..fd928d110 100644 --- a/headroom/proxy/handlers/openai.py +++ b/headroom/proxy/handlers/openai.py @@ -2116,6 +2116,45 @@ class OpenAIHandlerMixin: if is_tool_excluded(fn_name, DEFAULT_VERBATIM_EXCLUDE_TOOLS) } + # Read protection (HEADROOM_PROTECT_READS) — parity with the + # chat/Anthropic path (ContentRouter.apply). Output of a file-READ + # command (cat/nl/sed -n/head/tail/…) must stay verbatim: the agent + # byte-patches against it, and lossy reads caused re-reads / + # turn-inflation + resolve loss on SWE-bench. The Responses wire carries + # the producing command in two shapes, both normalized by the shared + # _tool_call_command_text helper: + # - function_call.arguments (Copilot bash, Codex exec_command, …) + # - local_shell_call.action (native Responses shell; argv or string) + # Content is gated per-output by _read_output_should_be_protected so + # confidently non-code DATA reads (lockfiles, JSON, logs, search) stay + # compressible, exactly like the chat path. + from headroom.transforms.content_router import ( + _is_read_command, + _read_output_should_be_protected, + _tool_call_command_text, + read_protection_enabled, + ) + + read_command_by_call_id: dict[str, str] = {} + if read_protection_enabled(): + for item in items: + if not isinstance(item, dict): + continue + item_type = item.get("type") + if item_type == "function_call": + command = _tool_call_command_text(item.get("arguments")) + elif item_type == "local_shell_call": + command = _tool_call_command_text(item.get("action")) + else: + continue + call_id = item.get("call_id") + if command and isinstance(call_id, str) and call_id and _is_read_command(command): + read_command_by_call_id[call_id] = command + # Outputs protected by read-command detection. Also unioned into the + # cross-turn dedup protection set below: a [↑…] fold of a read would + # break the exact-bytes contract just like lossy compression would. + read_protected_call_ids: set[str] = set() + timing_sink: dict[str, float] = timing if timing is not None else {} def _add_timing(name: str, started_at: float) -> None: @@ -2159,6 +2198,24 @@ class OpenAIHandlerMixin: } ) continue + if isinstance(call_id, str) and call_id in read_command_by_call_id: + # Finalize by CONTENT (same gate as ContentRouter.apply): + # protect unless the output is confidently non-code DATA. + if _read_output_should_be_protected(_responses_part_text(item.get("output"))): + read_protected_call_ids.add(call_id) + if debug_enabled: + extraction_debug.append( + { + "index": idx, + "eligible": False, + "reason": "read_command_protected", + "item_type": item_type, + "call_id": call_id, + "command": read_command_by_call_id[call_id], + "item": item, + } + ) + continue if isinstance(call_id, str) and call_id in excluded_call_ids: if call_id in verbatim_excluded_call_ids: if debug_enabled: @@ -2180,6 +2237,7 @@ class OpenAIHandlerMixin: # Note: when output is a content-part array, fold each text part # individually using ("output_part", index) slots to preserve the # array structure (non-text parts like images are left untouched). + excluded_folded = False raw_output = item.get("output") if isinstance(raw_output, list): for pidx, part in enumerate(raw_output): @@ -2191,6 +2249,7 @@ class OpenAIHandlerMixin: part_text = part["text"] pf = router._lossless_compact_excluded(part_text) if pf is not None: + excluded_folded = True lossless_excluded.append( (idx, ("output_part", pidx), pf[0], part_text) ) @@ -2198,6 +2257,7 @@ class OpenAIHandlerMixin: excl_out = _responses_part_text(raw_output) fold = router._lossless_compact_excluded(excl_out) if excl_out else None if fold is not None: + excluded_folded = True lossless_excluded.append((idx, ("output", None), fold[0], excl_out)) if debug_enabled: extraction_debug.append( @@ -2206,7 +2266,7 @@ class OpenAIHandlerMixin: "eligible": False, "reason": ( "exclude_tools_lossless_fold" - if fold is not None + if excluded_folded else "exclude_tools_protected" ), "item_type": item_type, @@ -2622,7 +2682,7 @@ class OpenAIHandlerMixin: updated_items, self.OPENAI_RESPONSES_OUTPUT_TYPES, tokenizer.count_text, - protected_call_ids=verbatim_excluded_call_ids, + protected_call_ids=verbatim_excluded_call_ids | read_protected_call_ids, ) if dd_folded: modified = True diff --git a/headroom/proxy/outcome.py b/headroom/proxy/outcome.py index c2b78b1dc..ba94feee7 100644 --- a/headroom/proxy/outcome.py +++ b/headroom/proxy/outcome.py @@ -25,6 +25,7 @@ actually reports. from __future__ import annotations +import asyncio import logging from dataclasses import dataclass, field from datetime import datetime, timezone @@ -455,10 +456,17 @@ async def emit_request_outcome(handler: Any, outcome: RequestOutcome) -> None: from headroom.proxy.output_savings import get_recorder _rec = get_recorder() - _rec.record_from_labels(outcome.transforms_applied, outcome.output_tokens) - output_tokens_saved_est = _rec.estimate_request_savings( - outcome.transforms_applied, outcome.output_tokens - ) + + def _record_and_estimate() -> int: + _rec.record_from_labels(outcome.transforms_applied, outcome.output_tokens) + return _rec.estimate_request_savings( + outcome.transforms_applied, outcome.output_tokens + ) + + # Both calls take the recorder lock, and the every-Nth record also + # does a full read-modify-write of the ledger file — run them + # together off the event loop (#18) so a slow flush can't stall it. + output_tokens_saved_est = await asyncio.to_thread(_record_and_estimate) except Exception: # pragma: no cover - defensive pass diff --git a/headroom/proxy/output_savings.py b/headroom/proxy/output_savings.py index b0ee6baf3..5fe317cb0 100644 --- a/headroom/proxy/output_savings.py +++ b/headroom/proxy/output_savings.py @@ -39,6 +39,7 @@ Pure module: no I/O except explicit ``load``/``save``. from __future__ import annotations import json +import logging import math from dataclasses import asdict, dataclass, field from typing import Any @@ -68,6 +69,8 @@ from .output_savings_policy import ( stratum_label as stratum_label, ) +logger = logging.getLogger(__name__) + @dataclass class _Accum: @@ -328,9 +331,13 @@ class SavingsLedger: def save(self, path: Any) -> None: from pathlib import Path + from headroom import fsutil + p = Path(path) p.parent.mkdir(parents=True, exist_ok=True) - p.write_text(json.dumps(self.to_dict(), separators=(",", ":"))) + # fsutil.write_text is atomic (temp file + os.replace), so a crash + # mid-write cannot truncate the ledger already on disk (#18). + fsutil.write_text(p, json.dumps(self.to_dict(), separators=(",", ":"))) @classmethod def load(cls, path: Any) -> SavingsLedger: @@ -341,7 +348,11 @@ class SavingsLedger: return cls() try: return cls.from_dict(json.loads(p.read_text())) - except (json.JSONDecodeError, ValueError, OSError): + except (json.JSONDecodeError, ValueError, OSError) as exc: + # Fail open (empty ledger), but surface the loss — silently + # swallowing a corrupt file made lost history indistinguishable + # from no history yet (#18). + logger.warning("output-savings ledger %s unreadable, starting empty: %s", p, exc) return cls() diff --git a/headroom/proxy/ssl_context.py b/headroom/proxy/ssl_context.py index 7ecba7c31..058c70d1f 100644 --- a/headroom/proxy/ssl_context.py +++ b/headroom/proxy/ssl_context.py @@ -188,6 +188,19 @@ def build_httpx_verify() -> ssl.SSLContext | bool: return True +def build_urlopen_context() -> ssl.SSLContext | None: + """Return Headroom's configured TLS context for ``urllib.request.urlopen``. + + ``urlopen`` already handles Python's default trust configuration when no + explicit context is passed. Return only a custom context here so callers + retain that default while sharing Headroom's corporate CA and strict-mode + handling when it is configured. + """ + + verify = build_httpx_verify() + return verify if isinstance(verify, ssl.SSLContext) else None + + def apply_global_tls_relaxation() -> bool: """Strip ``VERIFY_X509_STRICT`` from urllib3's context builder when opted in. diff --git a/headroom/transforms/content_detector.py b/headroom/transforms/content_detector.py index 5e364fed6..bb8325e42 100644 --- a/headroom/transforms/content_detector.py +++ b/headroom/transforms/content_detector.py @@ -457,6 +457,23 @@ def _try_detect_html(content: str) -> DetectionResult | None: ) +def _is_search_result_line(line: str) -> bool: + """True when a line looks like ``path:line:content`` grep output. + + The bare ``^[^\\s:]+:\\d+:`` shape also matches ISO-8601 timestamps + (``…T09:57:59…``) and XML-ish wrappers harnesses prepend to user turns + (Copilot CLI's ``…`` line), which misroutes prose to + the SearchCompressor — and that compressor keeps only matching lines, + deleting the rest. So the pre-colon segment must additionally look like + a file path: no angle brackets and no ``=`` (rules out markup tags and + ``key=value:12:`` log lines). + """ + if not _SEARCH_RESULT_PATTERN.match(line): + return False + prefix = line.split(":", 1)[0] + return "<" not in prefix and ">" not in prefix and "=" not in prefix + + def _try_detect_search(content: str) -> DetectionResult | None: """Try to detect grep/ripgrep search results.""" lines = content.split("\n")[:100] # Check first 100 lines @@ -465,10 +482,16 @@ def _try_detect_search(content: str) -> DetectionResult | None: matching_lines = 0 for line in lines: - if line.strip() and _SEARCH_RESULT_PATTERN.match(line): + if line.strip() and _is_search_result_line(line): matching_lines += 1 - if matching_lines == 0: + # Absolute floor: a single coincidental `word:digits:` line (a timestamp, + # a URL, a time literal inside prose) must not classify a whole payload as + # search results — the SearchCompressor drops every non-matching line, so + # a false positive is data loss. A genuine one-line grep result loses + # nothing by staying uncompressed: all of its lines match, so the + # compressor would have kept it verbatim anyway. + if matching_lines < 2: return None # Calculate confidence based on proportion of matching lines diff --git a/headroom/transforms/content_router.py b/headroom/transforms/content_router.py index 6f1fba323..090408ab5 100644 --- a/headroom/transforms/content_router.py +++ b/headroom/transforms/content_router.py @@ -532,6 +532,20 @@ def _tool_call_args_text(raw: Any) -> str: return " ".join(text.split())[:300] +def read_protection_enabled() -> bool: + """True when HEADROOM_PROTECT_READS opts into byte-exact file-read protection. + + Shared by every request path (chat/Anthropic ``ContentRouter.apply`` and the + OpenAI Responses units path) so the flag means the same thing everywhere. + """ + return os.environ.get("HEADROOM_PROTECT_READS", "0").strip().lower() not in ( + "0", + "", + "false", + "no", + ) + + def _tool_call_command_text(raw: Any) -> str: """Extract the raw shell command from a tool call's args, if present. @@ -4827,12 +4841,7 @@ class ContentRouter(Transform): # Type-specific by design: grep/test/ls output stays compressible, so the # cache-mode delta still compresses whenever the newest turn is NOT a read. self._protect_read_tool_ids = set() - if os.environ.get("HEADROOM_PROTECT_READS", "0").strip().lower() not in ( - "0", - "", - "false", - "no", - ): + if read_protection_enabled(): # Use _tool_call_commands (the parsed shell command), NOT # _tool_call_args (a compact free-text blob that, for OpenAI-style # JSON-string args, is the raw ``{"command": ...}`` JSON — on which @@ -4854,12 +4863,7 @@ class ContentRouter(Transform): # cat/sed/head code reads are protected on ANY model/harness, not just # those that emit tool-call/tool_result blocks. self._protect_read_msg_indices: set[int] = set() - if os.environ.get("HEADROOM_PROTECT_READS", "0").strip().lower() not in ( - "0", - "", - "false", - "no", - ): + if read_protection_enabled(): for _idx, _m in enumerate(messages): if _m.get("role") != "user": continue diff --git a/plugins/headroom-agent-hooks/.claude-plugin/plugin.json b/plugins/headroom-agent-hooks/.claude-plugin/plugin.json index f646bcf47..6ac2d6875 100644 --- a/plugins/headroom-agent-hooks/.claude-plugin/plugin.json +++ b/plugins/headroom-agent-hooks/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "headroom", - "version": "0.36.4", + "version": "0.36.5", "description": "Headroom startup hooks for Claude Code and GitHub Copilot CLI.", "author": { "name": "Headroom Contributors", diff --git a/plugins/headroom-agent-hooks/.github/plugin/plugin.json b/plugins/headroom-agent-hooks/.github/plugin/plugin.json index 34a703b5b..67948772f 100644 --- a/plugins/headroom-agent-hooks/.github/plugin/plugin.json +++ b/plugins/headroom-agent-hooks/.github/plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "headroom", - "version": "0.36.4", + "version": "0.36.5", "description": "Headroom startup hooks for Claude Code and GitHub Copilot CLI.", "author": { "name": "Headroom Contributors", diff --git a/plugins/openclaw/package.json b/plugins/openclaw/package.json index 41f5dd1fd..76a0ad3a0 100644 --- a/plugins/openclaw/package.json +++ b/plugins/openclaw/package.json @@ -1,6 +1,6 @@ { "name": "headroom-openclaw", - "version": "0.36.4", + "version": "0.36.5", "description": "Headroom context compression plugin for OpenClaw — 70-90% token savings with zero LLM calls", "type": "module", "main": "./dist/index.js", diff --git a/plugins/opencode/package.json b/plugins/opencode/package.json index ef071bb06..674432586 100644 --- a/plugins/opencode/package.json +++ b/plugins/opencode/package.json @@ -1,6 +1,6 @@ { "name": "headroom-opencode", - "version": "0.36.4", + "version": "0.36.5", "description": "Headroom proxy integration plugin for OpenCode - routes LLM traffic through the Headroom proxy for token compression", "type": "module", "main": "./dist/index.js", diff --git a/pyproject.toml b/pyproject.toml index 9003173b0..3e930bc2d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "maturin" [project] name = "headroom-ai" -version = "0.36.4" +version = "0.36.5" description = "The Context Optimization Layer for LLM Applications - Cut costs by 50-90%" readme = "README.md" license = "Apache-2.0" diff --git a/sdk/typescript/package.json b/sdk/typescript/package.json index 6bcd9173e..9a1341b22 100644 --- a/sdk/typescript/package.json +++ b/sdk/typescript/package.json @@ -1,6 +1,6 @@ { "name": "headroom-ai", - "version": "0.36.4", + "version": "0.36.5", "description": "Compress LLM context. Save tokens. Fit more into every request.", "type": "module", "main": "./dist/index.cjs", diff --git a/server.json b/server.json index 70bc9d8a0..2b72896a6 100644 --- a/server.json +++ b/server.json @@ -9,13 +9,13 @@ "source": "github", "id": "1129940957" }, - "version": "0.36.4", + "version": "0.36.5", "packages": [ { "registryType": "pypi", "registryBaseUrl": "https://pypi.org", "identifier": "headroom-ai", - "version": "0.36.4", + "version": "0.36.5", "runtimeHint": "uvx", "runtimeArguments": [ { diff --git a/tests/fixtures/headroom-issue-3054.json b/tests/fixtures/headroom-issue-3054.json new file mode 100644 index 000000000..e5eb4f12b --- /dev/null +++ b/tests/fixtures/headroom-issue-3054.json @@ -0,0 +1,26 @@ +{ + "issue": 3054, + "url": "https://github.com/headroomlabs-ai/headroom/issues/3054", + "old_serena_args": [ + "--from", + "git+https://github.com/oraios/serena", + "serena", + "start-mcp-server", + "--project-from-cwd", + "--context", + "claude-code", + "--open-web-dashboard", + "False" + ], + "recommended_serena_args": [ + "--from", + "serena-agent", + "serena", + "start-mcp-server", + "--project-from-cwd", + "--context", + "claude-code", + "--open-web-dashboard", + "False" + ] +} diff --git a/tests/test_cache/test_semantic.py b/tests/test_cache/test_semantic.py index c24d261f9..f5923f1cb 100644 --- a/tests/test_cache/test_semantic.py +++ b/tests/test_cache/test_semantic.py @@ -208,6 +208,36 @@ class TestSemanticCache: entry = cache.get("What time is it?") assert entry is None + def test_empty_query_never_semantic_matches(self): + """An empty extracted query must not trigger cross-context false hits. + + The query is the last user message; in agent/tool traffic most turns are + tool_result continuations whose extracted query is "". A real embedder + maps "" to a fixed non-zero vector, so without a guard every empty-query + turn would be ~identical to every other and serve one conversation's + response to an unrelated one. An empty query may only ever hit via the + exact messages_hash (which is context-complete). + """ + + def const_embedding(text: str) -> list[float]: + # Realistic: a non-zero, identical vector for every input (incl. ""). + return [0.5, 0.5, 0.5] + + config = SemanticCacheConfig(similarity_threshold=0.9) + cache = SemanticCache(config, embedding_fn=const_embedding) + + # Conversation A: an empty-query turn (unique full-context hash). + cache.put("", "response-A", messages_hash="ctxA") + + # Conversation B: a different empty-query turn — must NOT get A's answer. + assert cache.get("", messages_hash="ctxB") is None + # Its own exact hash still works. + assert cache.get("", messages_hash="ctxA").response == "response-A" + + # A whitespace-only query is treated the same as empty. + cache.put(" \n\t", "response-C", messages_hash="ctxC") + assert cache.get(" ", messages_hash="ctxD") is None + class TestSemanticCacheLayer: """Test SemanticCacheLayer functionality.""" diff --git a/tests/test_cli/test_mcp_reconcile.py b/tests/test_cli/test_mcp_reconcile.py new file mode 100644 index 000000000..519aa0591 --- /dev/null +++ b/tests/test_cli/test_mcp_reconcile.py @@ -0,0 +1,291 @@ +from __future__ import annotations + +import json +import os +import sys +from pathlib import Path + +import pytest +from click.testing import CliRunner + +from headroom.cli.main import main +from headroom.mcp_registry import ClaudeRegistrar, build_serena_spec +from headroom.mcp_registry.ledger import headroom_installed_matching + +FIXTURE = Path(__file__).parents[1] / "fixtures" / "headroom-issue-3054.json" + + +def _setup(monkeypatch, tmp_path: Path): + config = tmp_path / ".claude.json" + config.write_text( + json.dumps( + { + "oauthAccount": {"email": "user@example.com"}, + "mcpServers": { + "serena": { + "command": "uvx", + "args": json.loads(FIXTURE.read_text())["old_serena_args"], + }, + "other": {"command": "other", "args": []}, + }, + "projects": {"/repo": {"trust": True}}, + } + ) + ) + registrar = ClaudeRegistrar(claude_cli=None, home_dir=tmp_path) + monkeypatch.setattr("headroom.mcp_registry.ClaudeRegistrar", lambda: registrar) + ledger = tmp_path / "ledger.json" + monkeypatch.setattr("headroom.mcp_registry.ledger.ledger_path", lambda: ledger) + return config, ledger + + +def test_issue_fixture_reconcile_is_base_fail_head_pass(monkeypatch, tmp_path: Path): + config, _ = _setup(monkeypatch, tmp_path) + fixture = json.loads(FIXTURE.read_text()) + recommended = build_serena_spec("claude-code") + assert list(recommended.args) == fixture["recommended_serena_args"] + assert CliRunner().invoke(main, ["mcp", "reconcile"]).exit_code == 0 + adopted = CliRunner().invoke(main, ["mcp", "reconcile", "--adopt"]) + assert adopted.exit_code == 0, adopted.output + assert json.loads(config.read_text())["mcpServers"]["serena"]["args"] == list(recommended.args) + + +def test_read_only_preserves_config_and_ledger_bytes_and_mtimes(monkeypatch, tmp_path: Path): + config, ledger = _setup(monkeypatch, tmp_path) + ledger.write_text("not json") + before = ( + config.read_bytes(), + ledger.read_bytes(), + os.stat(config).st_mtime_ns, + os.stat(ledger).st_mtime_ns, + ) + result = CliRunner().invoke(main, ["mcp", "reconcile"]) + assert result.exit_code == 0, result.output + after = ( + config.read_bytes(), + ledger.read_bytes(), + os.stat(config).st_mtime_ns, + os.stat(ledger).st_mtime_ns, + ) + assert after == before + assert "--adopt" in result.output + + +def test_adopt_preserves_unrelated_config_and_records_ownership(monkeypatch, tmp_path: Path): + config, ledger = _setup(monkeypatch, tmp_path) + result = CliRunner().invoke(main, ["mcp", "reconcile", "--adopt"]) + assert result.exit_code == 0, result.output + data = json.loads(config.read_text()) + assert data["oauthAccount"] == {"email": "user@example.com"} + assert data["projects"] == {"/repo": {"trust": True}} + assert data["mcpServers"]["other"] == {"command": "other", "args": []} + assert data["mcpServers"]["serena"]["args"] == list(build_serena_spec("claude-code").args) + assert json.loads(ledger.read_text())["agents"]["claude"]["serena"]["fingerprint"] + + +@pytest.mark.parametrize( + "contents", + [ + "not json", + "[]", + '{"agents": null}', + '{"agents": []}', + '{"agents": {"claude": null}}', + '{"agents": {"claude": []}}', + '{"agents": {"claude": {"serena": null}}}', + ], +) +def test_malformed_ledger_blocks_adopt_before_config_write( + monkeypatch, tmp_path: Path, contents: str +): + config, ledger = _setup(monkeypatch, tmp_path) + before = config.read_bytes() + ledger.write_text(contents) + result = CliRunner().invoke(main, ["mcp", "reconcile", "--adopt"]) + assert result.exit_code != 0 + assert "ledger" in result.output.lower() + assert config.read_bytes() == before + + +def test_corrupt_ledger_is_tolerated_by_read_only(monkeypatch, tmp_path: Path): + _, ledger = _setup(monkeypatch, tmp_path) + ledger.write_text('{"agents": []}') + result = CliRunner().invoke(main, ["mcp", "reconcile"]) + assert result.exit_code == 0, result.output + + +def test_reconcile_rejects_absent_claude(monkeypatch, tmp_path: Path): + _, _ = _setup(monkeypatch, tmp_path) + registrar = ClaudeRegistrar(claude_cli=None, home_dir=tmp_path) + monkeypatch.setattr(registrar, "detect", lambda: False) + monkeypatch.setattr("headroom.mcp_registry.ClaudeRegistrar", lambda: registrar) + + result = CliRunner().invoke(main, ["mcp", "reconcile", "--adopt"]) + + assert result.exit_code != 0 + assert "claude is not detected" in result.output + + +def test_reconcile_adopt_preserves_malformed_config(monkeypatch, tmp_path: Path): + config, _ = _setup(monkeypatch, tmp_path) + config.write_text("not json") + before = config.read_bytes() + + result = CliRunner().invoke(main, ["mcp", "reconcile", "--adopt"]) + + assert result.exit_code != 0 + assert config.read_bytes() == before + + +def test_adopt_rejects_malformed_modern_before_touching_valid_legacy(monkeypatch, tmp_path: Path): + modern = tmp_path / ".claude.json" + legacy = tmp_path / ".claude" / "mcp.json" + legacy.parent.mkdir() + modern.write_text("not json") + legacy.write_text( + json.dumps( + { + "mcpServers": { + "serena": {"command": "uvx", "args": ["--from", "user"]}, + "other": {"command": "other"}, + } + } + ) + ) + registrar = ClaudeRegistrar(claude_cli=None, home_dir=tmp_path) + monkeypatch.setattr("headroom.mcp_registry.ClaudeRegistrar", lambda: registrar) + ledger = tmp_path / "ledger.json" + monkeypatch.setattr("headroom.mcp_registry.ledger.ledger_path", lambda: ledger) + before = (modern.read_bytes(), legacy.read_bytes()) + + result = CliRunner().invoke(main, ["mcp", "reconcile", "--adopt"]) + + assert result.exit_code != 0 + assert "not valid JSON" in result.output + assert (modern.read_bytes(), legacy.read_bytes()) == before + + +def test_adopt_rejects_non_dict_mcp_servers_in_legacy_root(monkeypatch, tmp_path: Path): + modern, _ = _setup(monkeypatch, tmp_path) + legacy = tmp_path / ".claude" / "mcp.json" + legacy.parent.mkdir() + legacy.write_text(json.dumps({"mcpServers": []})) + before = (modern.read_bytes(), legacy.read_bytes()) + + result = CliRunner().invoke(main, ["mcp", "reconcile", "--adopt"]) + + assert result.exit_code != 0 + assert "non-object mcpServers" in result.output + assert (modern.read_bytes(), legacy.read_bytes()) == before + + +def test_unreadable_ledger_blocks_adopt_without_partial_mutation(monkeypatch, tmp_path: Path): + config, ledger = _setup(monkeypatch, tmp_path) + ledger.write_text(json.dumps({"agents": {}})) + before = (config.read_bytes(), ledger.read_bytes()) + original_read_text = Path.read_text + + def unreadable(path: Path, *args, **kwargs): + if path == ledger: + raise PermissionError("test unreadable ledger") + return original_read_text(path, *args, **kwargs) + + monkeypatch.setattr(Path, "read_text", unreadable) + + result = CliRunner().invoke(main, ["mcp", "reconcile", "--adopt"]) + + assert result.exit_code != 0 + assert "unreadable" in result.output + assert (config.read_bytes(), ledger.read_bytes()) == before + + +@pytest.mark.parametrize("state", ["absent", "matching", "user-drift", "headroom-drift"]) +@pytest.mark.parametrize("adopt", [False, True]) +def test_reconcile_state_matrix(monkeypatch, tmp_path: Path, state: str, adopt: bool): + config, ledger = _setup(monkeypatch, tmp_path) + data = json.loads(config.read_text()) + recommended = build_serena_spec("claude-code") + owned_spec = None + if state == "absent": + del data["mcpServers"]["serena"] + elif state == "matching": + data["mcpServers"]["serena"] = { + "command": recommended.command, + "args": list(recommended.args), + } + elif state == "user-drift": + data["mcpServers"]["serena"]["args"] = ["--from", "user-managed"] + elif state == "headroom-drift": + from headroom.mcp_registry.ledger import record_install + + stale = build_serena_spec("claude-code") + stale.args = ("--from", "headroom-installed-old") + owned_spec = stale + data["mcpServers"]["serena"] = { + "command": stale.command, + "args": list(stale.args), + } + record_install("claude", stale, path=ledger) + config.write_text(json.dumps(data)) + if owned_spec is not None: + assert headroom_installed_matching("claude", owned_spec, path=ledger) + result = CliRunner().invoke(main, ["mcp", "reconcile"] + (["--adopt"] if adopt else [])) + assert result.exit_code == 0, result.output + observed = json.loads(config.read_text())["mcpServers"].get("serena") + ownership = observed is not None and headroom_installed_matching( + "claude", + build_serena_spec("claude-code") if observed["args"] == list(recommended.args) else None, + path=ledger, + ) + if adopt: + assert observed == { + "command": recommended.command, + "args": list(recommended.args), + } + assert ownership + assert "Adopted Headroom" in result.output + elif state == "headroom-drift": + assert observed["args"] == ["--from", "headroom-installed-old"] + assert headroom_installed_matching("claude", owned_spec, path=ledger) + assert ownership is False + assert "observed: present" in result.output + else: + assert not ownership + assert "Serena reconciliation for Claude" in result.output + + +def test_only_adopt_is_a_reconcile_mutation(monkeypatch, tmp_path: Path): + _setup(monkeypatch, tmp_path) + result = CliRunner().invoke(main, ["mcp", "reconcile", "--help"]) + assert result.exit_code == 0 + assert "--adopt" in result.output + for option in ("--acknowledge", "--clear", "--agent", "--server"): + assert option not in result.output + + +def test_ordinary_install_does_not_adopt_serena(monkeypatch, tmp_path: Path): + config, _ = _setup(monkeypatch, tmp_path) + before = config.read_bytes() + monkeypatch.setitem(sys.modules, "mcp", object()) + registrar = ClaudeRegistrar(claude_cli=None, home_dir=tmp_path) + monkeypatch.setattr("headroom.mcp_registry.install.get_all_registrars", lambda: [registrar]) + result = CliRunner().invoke(main, ["mcp", "install", "--agent", "claude"]) + assert result.exit_code == 0, result.output + after = json.loads(config.read_text()) + before_data = json.loads(before) + assert after["mcpServers"]["serena"] == before_data["mcpServers"]["serena"] + assert after["mcpServers"]["headroom"]["args"] == ["mcp", "serve"] + assert "mcp reconcile --adopt" not in result.output + + +def test_mcp_install_force_preserves_user_managed_serena(monkeypatch, tmp_path: Path): + config, _ = _setup(monkeypatch, tmp_path) + before = json.loads(config.read_text())["mcpServers"]["serena"] + monkeypatch.setitem(sys.modules, "mcp", object()) + registrar = ClaudeRegistrar(claude_cli=None, home_dir=tmp_path) + monkeypatch.setattr("headroom.mcp_registry.install.get_all_registrars", lambda: [registrar]) + + result = CliRunner().invoke(main, ["mcp", "install", "--agent", "claude", "--force"]) + + assert result.exit_code == 0, result.output + assert json.loads(config.read_text())["mcpServers"]["serena"] == before diff --git a/tests/test_cli/test_serena_reconcile.py b/tests/test_cli/test_serena_reconcile.py new file mode 100644 index 000000000..f65cbea1a --- /dev/null +++ b/tests/test_cli/test_serena_reconcile.py @@ -0,0 +1,124 @@ +from __future__ import annotations + +from pathlib import Path + +from headroom.cli import wrap as wrap_cli +from headroom.mcp_registry import build_serena_spec +from headroom.mcp_registry.base import RegisterResult, RegisterStatus, ServerSpec +from headroom.mcp_registry.ledger import headroom_installed_matching, record_install + + +class _Registrar: + display_name = "Claude Code" + + def __init__(self, current: ServerSpec | None, *, name: str = "claude"): + self.name = name + self.current = current + self.force_calls: list[bool] = [] + + def detect(self) -> bool: + return True + + def get_server(self, name: str) -> ServerSpec | None: + return self.current if name == "serena" else None + + def register_server(self, spec: ServerSpec, *, force: bool = False) -> RegisterResult: + self.force_calls.append(force) + if self.current == spec: + return RegisterResult(RegisterStatus.ALREADY, "matches") + if self.current is not None and not force: + return RegisterResult(RegisterStatus.MISMATCH, "different") + self.current = spec + return RegisterResult(RegisterStatus.REGISTERED, "updated") + + +def _quiet(monkeypatch): + monkeypatch.setattr(wrap_cli, "_ensure_serena_dashboard_disabled", lambda **kwargs: None) + monkeypatch.setattr(wrap_cli, "_inject_serena_instructions", lambda *args, **kwargs: None) + monkeypatch.setattr(wrap_cli, "_serena_project_skip_reason", lambda root: "test") + monkeypatch.setattr(wrap_cli, "_index_serena_project", lambda **kwargs: None) + monkeypatch.setattr(wrap_cli.shutil, "which", lambda name: "uvx" if name == "uvx" else None) + + +def test_automatic_wrap_migrates_owned_drift_and_recurs_to_noop( + monkeypatch, tmp_path: Path, capsys +): + _quiet(monkeypatch) + monkeypatch.setattr( + "headroom.mcp_registry.ledger.ledger_path", lambda: tmp_path / "ledger.json" + ) + stale = ServerSpec("serena", "uvx", ("--from", "old")) + record_install("claude", stale) + registrar = _Registrar(stale) + wrap_cli._setup_serena_mcp(registrar, context="claude-code", verbose=True) + assert registrar.current == build_serena_spec("claude-code") + assert registrar.force_calls == [False, True] + assert headroom_installed_matching("claude", registrar.current) + capsys.readouterr() + wrap_cli._setup_serena_mcp(registrar, context="claude-code", verbose=True) + assert registrar.force_calls == [False, True, False] + + +def test_automatic_wrap_owned_drift_suggests_rerun_wrap(monkeypatch, tmp_path: Path, capsys): + _quiet(monkeypatch) + monkeypatch.setattr( + "headroom.mcp_registry.ledger.ledger_path", lambda: tmp_path / "ledger.json" + ) + stale = ServerSpec("serena", "uvx", ("--from", "old")) + record_install("claude", stale) + + class _FailedMigrationRegistrar(_Registrar): + def register_server(self, spec, *, force=False): + if force: + self.force_calls.append(force) + return RegisterResult(RegisterStatus.MISMATCH, "still different") + return super().register_server(spec, force=force) + + wrap_cli._setup_serena_mcp( + _FailedMigrationRegistrar(stale), context="claude-code", verbose=True + ) + + output = capsys.readouterr().out + assert "run headroom wrap again" in output + assert "mcp reconcile --adopt" not in output + + +def test_automatic_wrap_preserves_user_managed_warning(monkeypatch, tmp_path: Path, capsys): + _quiet(monkeypatch) + monkeypatch.setattr( + "headroom.mcp_registry.ledger.ledger_path", lambda: tmp_path / "ledger.json" + ) + user = ServerSpec("serena", "uvx", ("--from", "user")) + registrar = _Registrar(user) + wrap_cli._setup_serena_mcp(registrar, context="claude-code", verbose=True) + assert registrar.current == user + assert registrar.force_calls == [False] + assert "existing config differs" in capsys.readouterr().out + + +def test_automatic_wrap_recovers_from_malformed_ledger(monkeypatch, tmp_path: Path): + _quiet(monkeypatch) + ledger = tmp_path / "ledger.json" + ledger.write_text("not json") + monkeypatch.setattr("headroom.mcp_registry.ledger.ledger_path", lambda: ledger) + registrar = _Registrar(None) + + wrap_cli._setup_serena_mcp(registrar, context="claude-code", verbose=True) + + current = registrar.get_server("serena") + assert current == build_serena_spec("claude-code") + assert headroom_installed_matching("claude", current) + + +def test_non_claude_wrap_keeps_usable_remediation_hint(monkeypatch, tmp_path: Path, capsys): + _quiet(monkeypatch) + monkeypatch.setattr( + "headroom.mcp_registry.ledger.ledger_path", lambda: tmp_path / "ledger.json" + ) + registrar = _Registrar(ServerSpec("serena", "uvx", ("--from", "user")), name="codex") + + wrap_cli._setup_serena_mcp(registrar, context="codex", verbose=True) + + output = capsys.readouterr().out + assert "update or remove the existing serena MCP entry" in output + assert "mcp reconcile --adopt" not in output diff --git a/tests/test_cli/test_unwrap_claude.py b/tests/test_cli/test_unwrap_claude.py index a562e05a3..e3b688196 100644 --- a/tests/test_cli/test_unwrap_claude.py +++ b/tests/test_cli/test_unwrap_claude.py @@ -243,18 +243,24 @@ def test_unwrap_claude_restores_all_base_url_modes(runner: CliRunner) -> None: "foundry_mode": False, "vertex_mode": False, "settings_path": settings_path, + # unwrap is the user asking for their settings back, so it drops + # every wrap session's ownership claim instead of deferring to a + # live sibling and silently doing nothing (#3205). + "force": True, }, { "previous": None, "foundry_mode": True, "vertex_mode": False, "settings_path": settings_path, + "force": True, }, { "previous": None, "foundry_mode": False, "vertex_mode": True, "settings_path": settings_path, + "force": True, }, ] diff --git a/tests/test_cli/test_wrap_copilot.py b/tests/test_cli/test_wrap_copilot.py index 98f261f09..9772b3bdb 100644 --- a/tests/test_cli/test_wrap_copilot.py +++ b/tests/test_cli/test_wrap_copilot.py @@ -266,17 +266,16 @@ def test_wrap_copilot_prefers_existing_oauth_session( assert result.exit_code == 0, result.output env = captured["env"] assert isinstance(env, dict) - assert env["COPILOT_PROVIDER_TYPE"] == "openai" - assert env["COPILOT_PROVIDER_BASE_URL"] == ( - f"http://127.0.0.1:8787{_expected_project_prefix()}/v1" - ) - assert env["COPILOT_PROVIDER_WIRE_API"] == "completions" - assert env["COPILOT_PROVIDER_BEARER_TOKEN"] == "gho-existing" + assert env["COPILOT_API_URL"] == f"http://127.0.0.1:8787{_expected_project_prefix()}" + assert "COPILOT_PROVIDER_TYPE" not in env + assert "COPILOT_PROVIDER_BASE_URL" not in env + assert "COPILOT_PROVIDER_WIRE_API" not in env + assert "COPILOT_PROVIDER_BEARER_TOKEN" not in env assert env["GITHUB_COPILOT_API_URL"] == DEFAULT_API_URL assert env["OPENAI_TARGET_API_URL"] == DEFAULT_API_URL assert "COPILOT_PROVIDER_API_KEY" not in env assert captured["openai_api_url"] == DEFAULT_API_URL - assert f"COPILOT_PROVIDER_API_URL={DEFAULT_API_URL}" in captured["env_vars_display"] + assert "COPILOT_AUTH_MODE=github-native" in captured["env_vars_display"] @pytest.mark.parametrize( @@ -293,7 +292,7 @@ def test_wrap_copilot_oauth_defaults_wire_api_for_selected_model( model: str, expected_wire_api: str, ) -> None: - """OAuth sessions use the same model-aware wire API default as subscriptions.""" + """Implicit OAuth leaves wire selection to Copilot's native router.""" _wrap_cli, main = wrap_modules _clear_copilot_env(monkeypatch) captured: dict[str, object] = {} @@ -315,8 +314,8 @@ def test_wrap_copilot_oauth_defaults_wire_api_for_selected_model( assert result.exit_code == 0, result.output env = captured["env"] assert isinstance(env, dict) - assert env["COPILOT_PROVIDER_WIRE_API"] == expected_wire_api - assert f"COPILOT_PROVIDER_WIRE_API={expected_wire_api}" in captured["env_vars_display"] + assert "COPILOT_PROVIDER_WIRE_API" not in env + assert env["COPILOT_API_URL"].startswith("http://127.0.0.1:8787") @pytest.mark.parametrize("wire_api", ["completions", "responses"]) @@ -348,7 +347,8 @@ def test_wrap_copilot_oauth_honors_existing_wire_api( assert result.exit_code == 0, result.output env = captured["env"] assert isinstance(env, dict) - assert env["COPILOT_PROVIDER_WIRE_API"] == wire_api + assert "COPILOT_PROVIDER_WIRE_API" not in env + assert env["COPILOT_API_URL"].startswith("http://127.0.0.1:8787") def test_wrap_copilot_subscription_uses_github_auth_without_provider_key( @@ -869,7 +869,8 @@ def test_wrap_copilot_oauth_keeps_generic_endpoint_when_account_advertised( assert result.exit_code == 0, result.output env = captured["env"] assert isinstance(env, dict) - assert env["COPILOT_PROVIDER_BEARER_TOKEN"] == "gho-oauth" + assert "COPILOT_PROVIDER_BEARER_TOKEN" not in env + assert env["COPILOT_API_URL"].startswith("http://127.0.0.1:8787") assert captured["openai_api_url"] == DEFAULT_API_URL assert env["OPENAI_TARGET_API_URL"] == DEFAULT_API_URL assert env["GITHUB_COPILOT_API_URL"] == DEFAULT_API_URL diff --git a/tests/test_cli/test_wrap_kimi.py b/tests/test_cli/test_wrap_kimi.py index 01f94b45c..05ce3cc61 100644 --- a/tests/test_cli/test_wrap_kimi.py +++ b/tests/test_cli/test_wrap_kimi.py @@ -2,6 +2,8 @@ from __future__ import annotations +import os +import sys from pathlib import Path from typing import Any from unittest.mock import patch @@ -18,6 +20,68 @@ def runner() -> CliRunner: return CliRunner() +def test_managed_route_reproduction( + runner: CliRunner, + capfd: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + """The production launcher passes the managed endpoint to an exact-contract child.""" + direct = "https://api.kimi.com/coding/v1" + monkeypatch.setenv("KIMI_BASE_URL", direct) + monkeypatch.setenv("KIMI_TEST_UNRELATED", "preserved") + monkeypatch.setattr(wrap_mod, "_project_name_from_cwd", lambda: "repo") + captured: dict[str, Any] = {} + + def fake_launch_tool(**kwargs: Any) -> None: # noqa: ANN003 + captured.update(kwargs) + + with patch.object(wrap_mod.shutil, "which", return_value=sys.executable): + with patch.object(wrap_mod, "_launch_tool", side_effect=fake_launch_tool): + result = runner.invoke(main, ["wrap", "kimi", "--port", "8787"]) + + assert result.exit_code == 0, result.output + env = captured["env"] + display = captured["env_vars_display"] + configure_launch = captured["configure_launch"] + child_result = tmp_path / "kimi-child.txt" + child = ( + "import os, sys; from pathlib import Path; Path(r'" + f"{child_result}" + "').write_text('CHILD|' + os.environ['KIMI_CODE_BASE_URL'] + '|' + " + "os.environ['KIMI_BASE_URL'] + '|' + os.environ['KIMI_TEST_UNRELATED'] + '|' + sys.argv[1])" + ) + + with ( + patch.object(wrap_mod, "_make_cleanup", return_value=lambda: None), + patch.object(wrap_mod.signal, "signal"), + patch.object(wrap_mod, "_register_proxy_client"), + patch.object(wrap_mod, "_ensure_proxy", return_value=(None, 9001)), + patch.object(wrap_mod, "_unregister_proxy_client"), + patch.object(wrap_mod, "_push_runtime_env"), + patch.object(wrap_mod, "_configure_quiet_cli_env", return_value=[]), + ): + with pytest.raises(SystemExit) as raised: + wrap_mod._launch_tool( + binary=os.fspath(Path(sys.executable)), + args=("-c", child, "child-arg"), + env=env, + port=8787, + no_proxy=False, + tool_label="KIMI", + env_vars_display=display, + configure_launch=configure_launch, + ) + + assert raised.value.code == 0 + output = result.output + capfd.readouterr().out + expected = "http://127.0.0.1:9001/p/repo/v1" + assert f"KIMI_CODE_BASE_URL={expected}" in output + assert f"KIMI_BASE_URL={expected}" in output + assert child_result.read_text() == f"CHILD|{expected}|{expected}|preserved|child-arg" + assert direct not in output + + def test_wrap_kimi_launch( runner: CliRunner, tmp_path: Path, @@ -46,15 +110,17 @@ def test_wrap_kimi_launch( assert captured["agent_type"] == "kimi" assert captured["args"] == ("-m", "kimi-for-coding") assert captured["openai_api_url"] == "https://api.kimi.com/coding/v1" + assert callable(captured["configure_launch"]) + assert env["KIMI_CODE_BASE_URL"] == "http://127.0.0.1:9000/v1" assert env["KIMI_BASE_URL"] == "http://127.0.0.1:9000/v1" -def test_wrap_kimi_with_project_name( +def test_project_name( runner: CliRunner, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: - """Project name is encoded in KIMI_BASE_URL when run from a project directory.""" + """Project name is encoded in both Kimi endpoint variables.""" project_dir = tmp_path / "my-project" project_dir.mkdir() monkeypatch.chdir(project_dir) @@ -71,6 +137,7 @@ def test_wrap_kimi_with_project_name( assert result.exit_code == 0, result.output env = captured["env"] + assert env["KIMI_CODE_BASE_URL"] == "http://127.0.0.1:7000/p/my-project/v1" assert env["KIMI_BASE_URL"] == "http://127.0.0.1:7000/p/my-project/v1" @@ -117,12 +184,12 @@ def test_wrap_kimi_not_found( assert "https://github.com/MoonshotAI/kimi-cli" in result.output -def test_wrap_kimi_custom_port( +def test_port_fallback( runner: CliRunner, tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: - """Custom --port is passed to _launch_tool and appears in KIMI_BASE_URL.""" + """Custom --port is passed to _launch_tool and appears in both URLs.""" monkeypatch.chdir(tmp_path) monkeypatch.delenv("HEADROOM_CONTEXT_TOOL", raising=False) @@ -138,9 +205,48 @@ def test_wrap_kimi_custom_port( assert result.exit_code == 0, result.output assert captured["port"] == 9999 + assert captured["env"]["KIMI_CODE_BASE_URL"] == "http://127.0.0.1:9999/v1" assert captured["env"]["KIMI_BASE_URL"] == "http://127.0.0.1:9999/v1" +def test_non_kimi_fallback_display_is_unchanged( + capfd: pytest.CaptureFixture[str], tmp_path: Path +) -> None: + env = {**os.environ, "OTHER_BASE_URL": "http://127.0.0.1:8787/v1"} + display = ["OTHER_BASE_URL=http://127.0.0.1:8787/v1"] + child_result = tmp_path / "other-child.txt" + child = ( + "import os; from pathlib import Path; Path(r'" + f"{child_result}" + "').write_text('CHILD|' + os.environ['OTHER_BASE_URL'])" + ) + + with ( + patch.object(wrap_mod, "_make_cleanup", return_value=lambda: None), + patch.object(wrap_mod.signal, "signal"), + patch.object(wrap_mod, "_register_proxy_client"), + patch.object(wrap_mod, "_ensure_proxy", return_value=(None, 9001)), + patch.object(wrap_mod, "_unregister_proxy_client"), + patch.object(wrap_mod, "_push_runtime_env"), + patch.object(wrap_mod, "_configure_quiet_cli_env", return_value=[]), + ): + with pytest.raises(SystemExit) as raised: + wrap_mod._launch_tool( + binary=os.fspath(Path(sys.executable)), + args=("-c", child), + env=env, + port=8787, + no_proxy=False, + tool_label="OTHER", + env_vars_display=display, + ) + + assert raised.value.code == 0 + output = capfd.readouterr().out + assert "OTHER_BASE_URL=http://127.0.0.1:8787/v1" in output + assert child_result.read_text() == "CHILD|http://127.0.0.1:9001/v1" + + def test_wrap_kimi_custom_api_url( runner: CliRunner, tmp_path: Path, diff --git a/tests/test_cli/test_wrap_stale_marker.py b/tests/test_cli/test_wrap_stale_marker.py index 966c551b2..6fb50fa08 100644 --- a/tests/test_cli/test_wrap_stale_marker.py +++ b/tests/test_cli/test_wrap_stale_marker.py @@ -1,8 +1,11 @@ from __future__ import annotations import json +import signal from pathlib import Path +import pytest + from headroom.cli import doctor as doctor_cli from headroom.cli import wrap as wrap_cli @@ -49,4 +52,19 @@ def test_claude_command_registers_sighup_next_to_sigterm() -> None: src = inspect.getsource(wrap_cli.claude.callback) assert 'hasattr(signal, "SIGHUP")' in src - assert "signal.signal(signal.SIGHUP, cleanup)" in src + assert "signal.signal(signal.SIGHUP, _exit_on_signal)" in src + assert "signal.signal(signal.SIGTERM, _exit_on_signal)" in src + + +def test_signal_handler_unwinds_so_the_restore_can_run() -> None: + """Registering `cleanup` directly never achieved what #1768 wanted. + + A Python signal handler that returns normally does not unwind the stack -- + under PEP 475 the interrupted `waitpid` is simply retried -- so the finally + block that restores settings.local.json never ran, while the handler had + already torn the proxy down under a live child. The handler must raise. + """ + with pytest.raises(SystemExit) as excinfo: + wrap_cli._exit_on_signal(signal.SIGHUP, None) + + assert excinfo.value.code == 128 + signal.SIGHUP diff --git a/tests/test_cli_doctor.py b/tests/test_cli_doctor.py index 8695af976..30adbeb51 100644 --- a/tests/test_cli_doctor.py +++ b/tests/test_cli_doctor.py @@ -375,6 +375,85 @@ class TestClaudeRemoteControlGate: assert result.status == PASS +class TestClaudeRoutingScope: + """Project-scoped routing must not read as "not routed" (#3205). + + `headroom init claude` without --global writes + `.claude/settings.local.json`. Reading only `~/.claude/settings.json` + reported not-routed for sessions that were genuinely routed and actively + compressing, which sent one team hand-checking `ps eww` on every session. + """ + + @staticmethod + def _settings(path, base_url): # noqa: ANN001, ANN205 + path.parent.mkdir(parents=True, exist_ok=True) + body = {"env": {"ANTHROPIC_BASE_URL": base_url}} if base_url else {"env": {}} + path.write_text(json.dumps(body), encoding="utf-8") + return path + + def test_project_local_settings_count_as_routed(self, tmp_path): + user = tmp_path / "user" / "settings.json" + project = self._settings( + tmp_path / "proj" / ".claude" / "settings.local.json", "http://127.0.0.1:8787" + ) + + result = check_claude_routing(user, 8787, [project]) + + assert result.status == PASS + assert "settings.local.json" in result.summary or "settings.local.json" in str(result) + + def test_project_settings_json_counts_as_routed(self, tmp_path): + user = tmp_path / "user" / "settings.json" + project = self._settings( + tmp_path / "proj" / ".claude" / "settings.json", "http://127.0.0.1:8787" + ) + + assert check_claude_routing(user, 8787, [project]).status == PASS + + def test_project_scope_takes_precedence_over_user_scope(self, tmp_path): + """Claude layers project over user, so the reported port follows suit.""" + user = self._settings(tmp_path / "user" / "settings.json", "http://127.0.0.1:9999") + project = self._settings( + tmp_path / "proj" / ".claude" / "settings.local.json", "http://127.0.0.1:8787" + ) + + assert check_claude_routing(user, 8787, [project]).status == PASS + + def test_falls_back_to_user_scope_when_project_has_no_base_url(self, tmp_path): + user = self._settings(tmp_path / "user" / "settings.json", "http://127.0.0.1:8787") + project = self._settings(tmp_path / "proj" / ".claude" / "settings.local.json", "") + + assert check_claude_routing(user, 8787, [project]).status == PASS + + def test_still_warns_when_nothing_routes(self, tmp_path): + user = self._settings(tmp_path / "user" / "settings.json", "") + project = self._settings(tmp_path / "proj" / ".claude" / "settings.local.json", "") + + assert check_claude_routing(user, 8787, [project]).status == WARN + + def test_missing_project_file_is_skipped_not_fatal(self, tmp_path): + user = self._settings(tmp_path / "user" / "settings.json", "http://127.0.0.1:8787") + absent = tmp_path / "proj" / ".claude" / "settings.local.json" + + assert check_claude_routing(user, 8787, [absent]).status == PASS + + def test_unparseable_project_file_surfaces_rather_than_reporting_not_routed(self, tmp_path): + project = tmp_path / "proj" / ".claude" / "settings.local.json" + project.parent.mkdir(parents=True, exist_ok=True) + project.write_text("{not json", encoding="utf-8") + user = tmp_path / "user" / "settings.json" + + result = check_claude_routing(user, 8787, [project]) + + assert result.status == WARN + assert "could not parse" in result.summary + + def test_no_project_paths_preserves_original_behaviour(self, tmp_path): + user = self._settings(tmp_path / "user" / "settings.json", "http://127.0.0.1:8787") + + assert check_claude_routing(user, 8787).status == PASS + + class TestCodexRouting: def test_missing_file_warns(self, tmp_path): assert check_codex_routing(tmp_path / "config.toml", 8787).status == WARN @@ -409,6 +488,56 @@ class TestCodexRouting: path.write_bytes(b"\xff\xfe garbage \x00") assert check_codex_routing(path, 8787).status == WARN + # -- requires_openai_auth (#3206) ------------------------------------ + # Codex attaches no Authorization header to a custom provider unless the + # block carries requires_openai_auth. A ChatGPT-OAuth user then 401s on + # every request with "Missing bearer" while doctor reported green -- the + # reason one report went 15h before anyone could see the cause. + + @staticmethod + def _routed(tmp_path, *, requires_auth: bool): + path = tmp_path / "config.toml" + block = ( + "[model_providers.headroom]\n" + 'base_url = "http://127.0.0.1:8787/v1"\n' + "supports_websockets = true\n" + ) + if requires_auth: + block += "requires_openai_auth = true\n" + path.write_text(block, encoding="utf-8") + return path + + @staticmethod + def _chatgpt_auth(tmp_path): + (tmp_path / "auth.json").write_text('{"auth_mode": "chatgpt"}', encoding="utf-8") + + def test_chatgpt_auth_without_requires_openai_auth_warns(self, tmp_path): + path = self._routed(tmp_path, requires_auth=False) + self._chatgpt_auth(tmp_path) + + result = check_codex_routing(path, 8787) + + assert result.status == WARN + assert "Authorization" in result.summary + + def test_chatgpt_auth_with_requires_openai_auth_passes(self, tmp_path): + path = self._routed(tmp_path, requires_auth=True) + self._chatgpt_auth(tmp_path) + + assert check_codex_routing(path, 8787).status == PASS + + def test_api_key_user_without_requires_openai_auth_still_passes(self, tmp_path): + """API-key users must not be nagged -- the flag would break them (#406).""" + path = self._routed(tmp_path, requires_auth=False) + (tmp_path / "auth.json").write_text('{"OPENAI_API_KEY": "sk-test"}', encoding="utf-8") + + assert check_codex_routing(path, 8787).status == PASS + + def test_no_auth_json_does_not_warn(self, tmp_path): + path = self._routed(tmp_path, requires_auth=False) + + assert check_codex_routing(path, 8787).status == PASS + class TestShellEnv: def test_unset_warns(self): diff --git a/tests/test_cli_learn.py b/tests/test_cli_learn.py index 0142e8fc9..cdb92e123 100644 --- a/tests/test_cli_learn.py +++ b/tests/test_cli_learn.py @@ -370,6 +370,35 @@ def test_learn_handles_empty_sessions_and_no_pattern_outputs( assert "No actionable patterns found." in result.output +def test_learn_surfaces_analysis_failure_and_exits_nonzero( + monkeypatch: pytest.MonkeyPatch, runner: CliRunner, tmp_path: Path +) -> None: + project = SimpleNamespace(name="broken", project_path=tmp_path / "broken") + plugin = FakePlugin("codex", "Codex", [project]) + + class FailingAnalyzer(FakeAnalyzer): + def analyze(self, project, sessions): # noqa: ANN001, ANN201 + self.calls.append((project, sessions)) + return SimpleNamespace( + total_sessions=1, + total_calls=3, + total_failures=1, + failure_rate=1 / 3, + recommendations=[], + analysis_error="codex CLI failed (exit 1): Not inside a trusted directory", + ) + + monkeypatch.setattr("headroom.learn.analyzer._detect_default_model", lambda: "codex-cli") + monkeypatch.setattr("headroom.learn.registry.get_plugin", lambda name: plugin) + monkeypatch.setattr("headroom.learn.analyzer.SessionAnalyzer", FailingAnalyzer) + + result = runner.invoke(main, ["learn", "--agent", "codex", "--all"]) + + assert result.exit_code == 1 + assert "Analysis failed: codex CLI failed (exit 1)" in result.output + assert "No actionable patterns found." not in result.output + + def test_learn_main_only_flag_threads_to_scanner( monkeypatch: pytest.MonkeyPatch, runner: CliRunner, tmp_path: Path ) -> None: diff --git a/tests/test_copilot_auth.py b/tests/test_copilot_auth.py index dd377c987..797b70568 100644 --- a/tests/test_copilot_auth.py +++ b/tests/test_copilot_auth.py @@ -10,6 +10,7 @@ from urllib import error as urllib_error import pytest from headroom import copilot_auth +from headroom.proxy import ssl_context def test_device_authorization_uses_form_encoded_request(monkeypatch: pytest.MonkeyPatch) -> None: @@ -1615,3 +1616,36 @@ def test_exchange_token_sync_returns_payload_on_success(monkeypatch: pytest.Monk ) assert result == payload + + +def test_exchange_token_sync_uses_configured_corporate_tls_context( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Copilot refresh must use the same corporate trust config as upstream I/O.""" + payload = {"token": "copilot-api", "expires_at": int(time.time()) + 3600} + tls_context = object() + captured: dict[str, object] = {} + + class FakeResponse: + def read(self) -> bytes: + return json.dumps(payload).encode() + + def __enter__(self): + return self + + def __exit__(self, *args): + pass + + def fake_urlopen(*args, **kwargs): # noqa: ANN002, ANN003, ANN202 + captured.update(kwargs) + return FakeResponse() + + monkeypatch.setattr(ssl_context, "build_urlopen_context", lambda: tls_context) + monkeypatch.setattr(copilot_auth.urllib_request, "urlopen", fake_urlopen) + + result = copilot_auth.CopilotTokenProvider._exchange_token_sync( + {"Authorization": "Bearer gho_test"} # noqa: S105 + ) + + assert result == payload + assert captured["context"] is tls_context diff --git a/tests/test_copilot_native_mode.py b/tests/test_copilot_native_mode.py new file mode 100644 index 000000000..72a7d7f77 --- /dev/null +++ b/tests/test_copilot_native_mode.py @@ -0,0 +1,148 @@ +from __future__ import annotations + +import pytest +from click.testing import CliRunner + +from headroom.providers.copilot.wrap import ( + COPILOT_BYOK_ENV_VARS, + COPILOT_NATIVE_API_URL_ENV, + build_launch_env, + build_native_launch_env, + native_api_url_supported, +) + + +def test_native_env_redirects_api_and_clears_all_byok_state() -> None: + seeded = dict.fromkeys(COPILOT_BYOK_ENV_VARS, "stale") + seeded["UNRELATED"] = "preserved" + env, _ = build_native_launch_env(port=8890, environ=seeded, project="repo name") + + assert env[COPILOT_NATIVE_API_URL_ENV] == "http://127.0.0.1:8890/p/repo%20name" + assert env["UNRELATED"] == "preserved" + assert not any(variable in env for variable in COPILOT_BYOK_ENV_VARS) + + +def test_byok_builder_remains_disjoint_from_native_mode() -> None: + env, _ = build_launch_env( + port=8787, + provider_type="openai", + wire_api="responses", + environ={}, + ) + assert env["COPILOT_PROVIDER_BASE_URL"] == "http://127.0.0.1:8787/v1" + assert env["COPILOT_PROVIDER_WIRE_API"] == "responses" + assert COPILOT_NATIVE_API_URL_ENV not in env + + +def test_native_support_probe_distinguishes_unknown_and_unsupported(tmp_path) -> None: + local = tmp_path / "local" + assert native_api_url_supported(environ={"LOCALAPPDATA": str(local)}) is None + + bundle = local / "copilot" / "pkg" / "platform" / "1.0" / "app.js" + bundle.parent.mkdir(parents=True) + bundle.write_text("no override here", encoding="utf-8") + assert native_api_url_supported(environ={"LOCALAPPDATA": str(local)}) is False + + bundle.write_text("process.env.COPILOT_API_URL", encoding="utf-8") + assert native_api_url_supported(environ={"LOCALAPPDATA": str(local)}) is True + + +def test_native_support_probe_skips_unreadable_bundle(monkeypatch, tmp_path) -> None: + local = tmp_path / "local" + bundle = local / "copilot" / "pkg" / "platform" / "1.0" / "app.js" + bundle.parent.mkdir(parents=True) + bundle.write_text("process.env.COPILOT_API_URL", encoding="utf-8") + + def _unreadable(*_args, **_kwargs): + raise OSError("synthetic unreadable bundle") + + monkeypatch.setattr("builtins.open", _unreadable) + assert native_api_url_supported(environ={"LOCALAPPDATA": str(local)}) is False + + +def _invoke_native( + monkeypatch: pytest.MonkeyPatch, + extra: list[str] | None = None, + *, + support: bool | None = True, +): + from headroom.cli import wrap as wrap_mod + from headroom.cli.main import main + + captured: dict[str, object] = {} + + class Resolution: + token = "copilot-token" + api_url = "https://api.business.githubcopilot.com" + refresh_oauth_token = "refresh-token" + api_token_expires_at = 123.0 + + monkeypatch.setattr(wrap_mod.shutil, "which", lambda _name: "/usr/bin/copilot") + monkeypatch.setattr(wrap_mod, "_check_proxy", lambda _port: False) + monkeypatch.setattr(wrap_mod, "_require_copilot_subscription_resolution", lambda: Resolution()) + monkeypatch.setattr(wrap_mod, "_native_api_url_supported", lambda **_kwargs: support) + monkeypatch.setattr(wrap_mod, "_launch_tool", lambda **kwargs: captured.update(kwargs)) + result = CliRunner().invoke( + main, + ["wrap", "copilot", "--native", "--port", "8890", *(extra or [])], + ) + return result, captured + + +def test_implicit_oauth_uses_native_routing_without_flag(monkeypatch) -> None: + from headroom.cli import wrap as wrap_mod + from headroom.cli.main import main + + captured: dict[str, object] = {} + monkeypatch.setattr(wrap_mod.shutil, "which", lambda _name: "/usr/bin/copilot") + monkeypatch.setattr(wrap_mod, "_check_proxy", lambda _port: False) + monkeypatch.setattr(wrap_mod, "has_oauth_auth", lambda: True) + monkeypatch.setattr(wrap_mod, "resolve_client_bearer_token", lambda: "oauth-token") + monkeypatch.setattr( + wrap_mod, "resolve_copilot_api_url", lambda _token: "https://api.githubcopilot.com" + ) + monkeypatch.setattr(wrap_mod, "_native_api_url_supported", lambda **_kwargs: True) + monkeypatch.setattr(wrap_mod, "_launch_tool", lambda **kwargs: captured.update(kwargs)) + + result = CliRunner().invoke( + main, + ["wrap", "copilot", "--port", "8890", "--", "--model", "claude-sonnet-5"], + ) + + assert result.exit_code == 0, result.output + env = captured["env"] + assert isinstance(env, dict) + assert COPILOT_NATIVE_API_URL_ENV in env + assert not any(variable in env for variable in COPILOT_BYOK_ENV_VARS) + + +def test_native_cli_routes_both_protocols_to_tenant_host(monkeypatch) -> None: + result, captured = _invoke_native(monkeypatch) + assert result.exit_code == 0, result.output + assert captured["openai_api_url"] == "https://api.business.githubcopilot.com" + assert captured["anthropic_api_url"] == "https://api.business.githubcopilot.com" + env = captured["env"] + assert isinstance(env, dict) + assert COPILOT_NATIVE_API_URL_ENV in env + assert not any(variable in env for variable in COPILOT_BYOK_ENV_VARS) + + +@pytest.mark.parametrize("extra", [["--wire-api", "responses"], ["--provider-type", "anthropic"]]) +def test_native_cli_rejects_byok_only_options(monkeypatch, extra) -> None: + result, captured = _invoke_native(monkeypatch, extra) + assert result.exit_code != 0 + assert not captured + + +def test_native_cli_refuses_known_unsupported_bundle(monkeypatch) -> None: + result, captured = _invoke_native(monkeypatch, support=False) + assert result.exit_code != 0 + assert "COPILOT_API_URL" in result.output + assert not captured + + +def test_native_cli_reports_unknown_support_in_verbose_mode(monkeypatch) -> None: + result, captured = _invoke_native(monkeypatch, ["--verbose"], support=None) + assert result.exit_code == 0, result.output + assert "could not verify" in result.output + assert captured diff --git a/tests/test_integrations/agno/test_model.py b/tests/test_integrations/agno/test_model.py index 26545996c..a3fe45c40 100644 --- a/tests/test_integrations/agno/test_model.py +++ b/tests/test_integrations/agno/test_model.py @@ -19,6 +19,11 @@ try: AGNO_AVAILABLE = True except ImportError: AGNO_AVAILABLE = False +else: + try: # agno < 3: the per-message usage dataclass lived at agno.models.metrics + from agno.models.metrics import Metrics + except ImportError: # agno >= 3 moved it to agno.metrics, renamed MessageMetrics + from agno.metrics import MessageMetrics as Metrics from headroom import HeadroomConfig, HeadroomMode @@ -26,6 +31,24 @@ from headroom import HeadroomConfig, HeadroomMode pytestmark = pytest.mark.skipif(not AGNO_AVAILABLE, reason="Agno not installed") +def _response_usage(input_tokens: int, output_tokens: int, total_tokens: int): + """Build response usage across Agno 2.x and 3.x module layouts.""" + + try: + from agno.metrics import MessageMetrics + + metrics_type = MessageMetrics + except ImportError: + from agno.models.metrics import Metrics + + metrics_type = Metrics + return metrics_type( + input_tokens=input_tokens, + output_tokens=output_tokens, + total_tokens=total_tokens, + ) + + @pytest.fixture def mock_agno_model(): """Create a mock Agno model (OpenAIChat-like).""" @@ -50,17 +73,11 @@ def mock_agno_model(): # Mock invoke method (returns ModelResponse for Agno's response() loop) def mock_invoke(messages, **kwargs): - from agno.models.metrics import Metrics - # Create a proper ModelResponse that Agno's response() can process return ModelResponse( role="assistant", content="Hello! I'm a mock response.", - response_usage=Metrics( - input_tokens=10, - output_tokens=5, - total_tokens=15, - ), + response_usage=_response_usage(10, 5, 15), ) mock.invoke = MagicMock(side_effect=mock_invoke) @@ -73,16 +90,10 @@ def mock_agno_model(): # Mock invoke_stream for streaming def mock_invoke_stream(messages, **kwargs): - from agno.models.metrics import Metrics - yield ModelResponse( role="assistant", content="Streaming...", - response_usage=Metrics( - input_tokens=10, - output_tokens=5, - total_tokens=15, - ), + response_usage=_response_usage(10, 5, 15), ) mock.invoke_stream = MagicMock(side_effect=mock_invoke_stream) diff --git a/tests/test_learn/test_analyzer.py b/tests/test_learn/test_analyzer.py index e271d2226..69308f4e3 100644 --- a/tests/test_learn/test_analyzer.py +++ b/tests/test_learn/test_analyzer.py @@ -473,6 +473,7 @@ class TestSessionAnalyzer: assert result.total_calls == 1 assert result.total_failures == 1 assert result.recommendations == [] + assert result.analysis_error == "API key not set" @patch("headroom.learn.analyzer._call_llm") def test_passes_events_to_digest(self, mock_call_llm: MagicMock): @@ -865,7 +866,7 @@ class TestCallCliLlm: result = _call_cli_llm("test digest", "codex-cli") assert result == {"context_file_rules": [], "memory_file_rules": []} cmd = mock_run.call_args[0][0] - assert cmd == ["codex", "exec"] + assert cmd == ["codex", "exec", "--skip-git-repo-check"] @patch("headroom.learn.analyzer.subprocess.run") def test_gemini_cli_uses_p_flag(self, mock_run: MagicMock): diff --git a/tests/test_mcp_registry/test_ledger.py b/tests/test_mcp_registry/test_ledger.py index 73100a32b..aa7ff08dc 100644 --- a/tests/test_mcp_registry/test_ledger.py +++ b/tests/test_mcp_registry/test_ledger.py @@ -1,53 +1,97 @@ from __future__ import annotations +import json + +import pytest + +import headroom.mcp_registry.ledger as ledger_module from headroom.mcp_registry.base import ServerSpec from headroom.mcp_registry.ledger import ( + LedgerMutationError, clear_install, headroom_installed_matching, record_install, spec_fingerprint, + validate_ledger_for_mutation, ) def _spec(command: str = "uvx") -> ServerSpec: - return ServerSpec( - name="serena", - command=command, - args=("--from", "git+https://github.com/oraios/serena", "serena"), - ) + return ServerSpec("serena", command, ("--from", "serena-agent", "serena")) -def test_ledger_records_matching_install(tmp_path): +def test_ledger_records_and_clears_matching_install(tmp_path): ledger = tmp_path / "mcp_installs.json" spec = _spec() + record_install("claude", spec, path=ledger) + assert headroom_installed_matching("claude", spec, path=ledger) + clear_install("claude", "serena", path=ledger) + assert not headroom_installed_matching("claude", spec, path=ledger) + + +def test_spec_fingerprint_is_stable_for_env_order(): + a = ServerSpec("serena", "uvx", env={"B": "2", "A": "1"}) + b = ServerSpec("serena", "uvx", env={"A": "1", "B": "2"}) + assert spec_fingerprint(a) == spec_fingerprint(b) + + +@pytest.mark.parametrize( + "value", + [ + "not json", + [], + {"agents": None}, + {"agents": []}, + {"agents": {"claude": None}}, + {"agents": {"claude": []}}, + {"agents": {"claude": {"serena": None}}}, + {"agents": {"claude": {"serena": {"fingerprint": "only"}}}}, + ], +) +def test_mutation_preflight_rejects_unsafe_shapes(tmp_path, value): + ledger = tmp_path / "mcp_installs.json" + ledger.write_text(value if isinstance(value, str) else json.dumps(value)) + with pytest.raises(LedgerMutationError): + validate_ledger_for_mutation(ledger) + + +def test_mutation_preflight_rejects_unreadable_ledger(monkeypatch, tmp_path): + ledger = tmp_path / "mcp_installs.json" + ledger.write_text('{"agents": {}}') + original_read_text = ledger_module.Path.read_text + + def unreadable(path, *args, **kwargs): + if path == ledger: + raise PermissionError("test unreadable ledger") + return original_read_text(path, *args, **kwargs) + + monkeypatch.setattr(ledger_module.Path, "read_text", unreadable) + + with pytest.raises(LedgerMutationError, match="unreadable"): + validate_ledger_for_mutation(ledger) + + +def test_read_matching_tolerates_corrupt_ledger(tmp_path): + ledger = tmp_path / "mcp_installs.json" + ledger.write_text("not json") + assert not headroom_installed_matching("claude", _spec(), path=ledger) + + +def test_record_install_recovers_from_corrupt_ledger(tmp_path): + ledger = tmp_path / "mcp_installs.json" + ledger.write_text("not json") + spec = _spec() record_install("claude", spec, path=ledger) - assert headroom_installed_matching("claude", spec, path=ledger) is True + assert headroom_installed_matching("claude", spec, path=ledger) -def test_ledger_rejects_changed_spec(tmp_path): +@pytest.mark.parametrize("contents", ['{"agents": null}', '{"agents": {"claude": null}}']) +def test_record_install_recovers_from_unsafe_ledger_shape(tmp_path, contents): ledger = tmp_path / "mcp_installs.json" + ledger.write_text(contents) record_install("claude", _spec(), path=ledger) - assert ( - headroom_installed_matching("claude", _spec(command="/custom/serena"), path=ledger) is False - ) - - -def test_clear_install_removes_entry(tmp_path): - ledger = tmp_path / "mcp_installs.json" - spec = _spec() - record_install("claude", spec, path=ledger) - - clear_install("claude", "serena", path=ledger) - - assert headroom_installed_matching("claude", spec, path=ledger) is False - - -def test_spec_fingerprint_stable_for_env_order(): - a = ServerSpec(name="serena", command="uvx", env={"B": "2", "A": "1"}) - b = ServerSpec(name="serena", command="uvx", env={"A": "1", "B": "2"}) - - assert spec_fingerprint(a) == spec_fingerprint(b) + assert headroom_installed_matching("claude", _spec(), path=ledger) diff --git a/tests/test_openai_responses_read_protection.py b/tests/test_openai_responses_read_protection.py new file mode 100644 index 000000000..36ed3691d --- /dev/null +++ b/tests/test_openai_responses_read_protection.py @@ -0,0 +1,531 @@ +"""Regression tests: file reads over the OpenAI Responses API path must stay verbatim. + +Copilot CLI (and other Responses-native harnesses) read files two ways: + +1. A first-class ``view`` tool (the Copilot equivalent of Claude Code's ``Read``) + whose output is raw file content the model will byte-patch against. +2. Shell reads through ``bash`` (``cat``/``nl``/``sed -n`` …), which the + chat/Anthropic path protects via ``HEADROOM_PROTECT_READS`` read-command + detection in ``ContentRouter``. + +The Responses compression-units path historically protected neither: only +``DEFAULT_EXCLUDE_TOOLS`` names were honored, and ``HEADROOM_PROTECT_READS`` +was never consulted. Lossy (Kompress) compression of a fresh file read garbles +exactly the bytes the model needs for line-precise edits, forcing re-reads +(turn inflation) — the harm read protection exists to prevent. +""" + +from __future__ import annotations + +from types import MethodType, SimpleNamespace + +from headroom.proxy.handlers.openai import OpenAIHandlerMixin +from headroom.transforms.content_router import ( + CompressionStrategy, + ContentRouter, + RouterCompressionResult, +) + + +class TokenCounter: + def count_text(self, text: str) -> int: + return len(text.split()) + + +def _handler_with_router(router: ContentRouter) -> OpenAIHandlerMixin: + handler = OpenAIHandlerMixin() + handler.openai_pipeline = SimpleNamespace(transforms=[router]) + handler.openai_provider = SimpleNamespace( + get_token_counter=lambda _model: TokenCounter(), + ) + return handler + + +def _lossy_router() -> ContentRouter: + """Router whose compress() always 'lossy-compresses' any candidate it sees.""" + + router = ContentRouter() + + def compress(self, content: str, **_kwargs): + return RouterCompressionResult( + compressed="kept words", + original=content, + strategy_used=CompressionStrategy.KOMPRESS, + ) + + router.compress = MethodType(compress, router) + return router + + +def _run(handler: OpenAIHandlerMixin, payload: dict): + return handler._compress_openai_responses_live_text_units_with_router( + payload, + model="gpt-5", + request_id="req_read_protection", + ) + + +_FILE_CONTENT = "\n".join( + f"## Section {i}\nSome roadmap prose line {i} with enough words to matter" for i in range(90) +) + +_NL_OUTPUT = "\n".join( + f"{i}\tline {i} of the roadmap file with a handful of words in it" for i in range(1, 110) +) + + +def test_responses_view_tool_read_stays_verbatim(): + """Copilot's `view` tool returns raw file bytes: never lossy-compress them.""" + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_view", + "name": "view", + "arguments": '{"path": "/repo/ROADMAP.md"}', + }, + { + "type": "function_call_output", + "call_id": "call_view", + "output": _FILE_CONTENT, + }, + ], + } + + new_payload, _modified, _saved, _t, _u, _s, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == _FILE_CONTENT + + +def test_responses_bash_read_command_stays_verbatim_when_protect_reads(monkeypatch): + """HEADROOM_PROTECT_READS=1 must cover bash file reads on the Responses path too.""" + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_bash", + "name": "bash", + "arguments": ('{"command": "nl -ba .overlay/ROADMAP.md | sed -n \'1,75p\'"}'), + }, + { + "type": "function_call_output", + "call_id": "call_bash", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, _modified, _saved, _t, _u, _s, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == _NL_OUTPUT + + +def test_responses_excluded_read_tool_stays_verbatim_control(): + """Control: Claude-style `Read` outputs are already protected today.""" + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_read", + "name": "Read", + "arguments": '{"file_path": "/repo/ROADMAP.md"}', + }, + { + "type": "function_call_output", + "call_id": "call_read", + "output": _FILE_CONTENT, + }, + ], + } + + new_payload, _modified, _saved, _t, _u, _s, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == _FILE_CONTENT + + +def test_responses_bash_read_compresses_when_protect_reads_disabled(monkeypatch): + """Control: with HEADROOM_PROTECT_READS unset/0, bash reads stay compressible.""" + monkeypatch.delenv("HEADROOM_PROTECT_READS", raising=False) + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_bash", + "name": "bash", + "arguments": '{"command": "cat src/main.py"}', + }, + { + "type": "function_call_output", + "call_id": "call_bash", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert modified is True + assert new_payload["input"][1]["output"] == "kept words" + + +def test_responses_non_read_bash_command_still_compresses(monkeypatch): + """Protection is type-specific: test/build/search output stays compressible.""" + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_test", + "name": "bash", + "arguments": '{"command": "uv run pytest tests/ -q"}', + }, + { + "type": "function_call_output", + "call_id": "call_test", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert modified is True + assert new_payload["input"][1]["output"] == "kept words" + + +def test_responses_lockfile_read_stays_compressible(monkeypatch): + """Lockfiles are tool-regenerated, never byte-patched: the command-level + carve-out keeps `cat uv.lock` compressible even with protection on.""" + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_lock", + "name": "bash", + "arguments": '{"command": "cat uv.lock"}', + }, + { + "type": "function_call_output", + "call_id": "call_lock", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert modified is True + assert new_payload["input"][1]["output"] == "kept words" + + +def test_responses_local_shell_call_read_stays_verbatim(monkeypatch): + """Codex native shell: local_shell_call.action.command (argv) read protected.""" + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "local_shell_call", + "call_id": "call_lsc", + "action": {"type": "exec", "command": ["nl", "-ba", "ROADMAP.md"]}, + }, + { + "type": "local_shell_call_output", + "call_id": "call_lsc", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, _modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == _NL_OUTPUT + + +def test_responses_view_output_content_part_array_stays_verbatim(): + """`view` output shaped as a content-part array is protected byte-exactly, + including non-text parts.""" + handler = _handler_with_router(_lossy_router()) + parts = [ + {"type": "output_text", "text": _FILE_CONTENT}, + {"type": "refusal", "refusal": "n/a"}, + ] + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_view", + "name": "view", + "arguments": '{"path": "/repo/ROADMAP.md"}', + }, + { + "type": "function_call_output", + "call_id": "call_view", + "output": parts, + }, + ], + } + + new_payload, _modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == parts + + +def test_responses_view_json_shaped_output_stays_byte_exact(): + """Even JSON-shaped `view` output is verbatim: the byte-exact contract beats + the lossless JSON minification other excluded tools accept.""" + handler = _handler_with_router(_lossy_router()) + pretty_json = "\n".join( + ["{"] + [f' "key_{i}": {i},' for i in range(120)] + [' "end": true', "}"] + ) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_view", + "name": "view", + "arguments": '{"path": "/repo/data.json"}', + }, + { + "type": "function_call_output", + "call_id": "call_view", + "output": pretty_json, + }, + ], + } + + new_payload, _modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == pretty_json + + +def test_responses_malformed_arguments_do_not_break_extraction(monkeypatch): + """Malformed function_call arguments yield no command -> normal compression.""" + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_bad", + "name": "bash", + "arguments": "{not json at all", + }, + { + "type": "function_call_output", + "call_id": "call_bad", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert modified is True + assert new_payload["input"][1]["output"] == "kept words" + + +def test_responses_protected_read_survives_cross_turn_dedup(monkeypatch): + """A repeated protected read must not be replaced by a [↑…] dedup pointer.""" + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + router = _lossy_router() + router._cross_turn_dedup_enabled = True + handler = _handler_with_router(router) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_r1", + "name": "bash", + "arguments": '{"command": "nl -ba ROADMAP.md"}', + }, + { + "type": "function_call_output", + "call_id": "call_r1", + "output": _NL_OUTPUT, + }, + { + "type": "function_call", + "call_id": "call_r2", + "name": "bash", + "arguments": '{"command": "nl -ba ROADMAP.md"}', + }, + { + "type": "function_call_output", + "call_id": "call_r2", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, _modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == _NL_OUTPUT + assert new_payload["input"][3]["output"] == _NL_OUTPUT + + +def test_responses_debug_path_with_excluded_list_output(monkeypatch): + """Regression: debug logging over an excluded tool's content-part output must + not raise (latent unbound `fold` variable in the list branch).""" + from headroom.proxy.handlers import openai as openai_handler + + monkeypatch.setattr(openai_handler, "_log_codex_compression_debug", lambda *a, **k: None) + handler = _handler_with_router(_lossy_router()) + parts = [{"type": "output_text", "text": _FILE_CONTENT}] + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_read", + "name": "Read", + "arguments": '{"file_path": "/repo/ROADMAP.md"}', + }, + { + "type": "function_call_output", + "call_id": "call_read", + "output": parts, + }, + ], + } + + new_payload, _modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == parts + + +def test_responses_read_command_with_releasable_json_output_compresses(monkeypatch): + """Content gate: a read command whose output is confidently DATA (JSON array) + is released to compression even with HEADROOM_PROTECT_READS=1.""" + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + handler = _handler_with_router(_lossy_router()) + json_output = "[" + ",".join(f'{{"line": {i}, "text": "value {i}"}}' for i in range(60)) + "]" + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_json", + "name": "bash", + "arguments": '{"command": "cat data.json"}', + }, + { + "type": "function_call_output", + "call_id": "call_json", + "output": json_output, + }, + ], + } + + new_payload, modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert modified is True + assert new_payload["input"][1]["output"] == "kept words" + + +def test_responses_local_shell_call_string_command_read_stays_verbatim(monkeypatch): + """local_shell_call with a string (not argv) command is also covered.""" + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "local_shell_call", + "call_id": "call_lsc_str", + "action": {"type": "exec", "command": "cat src/app.py"}, + }, + { + "type": "local_shell_call_output", + "call_id": "call_lsc_str", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, _modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == _NL_OUTPUT + + +def test_responses_debug_path_with_read_protected_output(monkeypatch): + """Debug logging over a read-protected output records and does not raise.""" + from headroom.proxy.handlers import openai as openai_handler + + monkeypatch.setattr(openai_handler, "_log_codex_compression_debug", lambda *a, **k: None) + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + { + "type": "function_call", + "call_id": "call_dbg", + "name": "bash", + "arguments": '{"command": "nl -ba ROADMAP.md"}', + }, + { + "type": "function_call_output", + "call_id": "call_dbg", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, _modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert new_payload["input"][1]["output"] == _NL_OUTPUT + + +def test_responses_read_scan_tolerates_non_dict_and_missing_call_id(monkeypatch): + """The producer scan must skip non-dict items and calls without a string + call_id without breaking normal compression.""" + monkeypatch.setenv("HEADROOM_PROTECT_READS", "1") + handler = _handler_with_router(_lossy_router()) + payload = { + "model": "gpt-5", + "input": [ + "a bare string item", + { + "type": "function_call", + "name": "bash", + "arguments": '{"command": "cat src/app.py"}', + }, + { + "type": "function_call", + "call_id": 42, + "name": "bash", + "arguments": '{"command": "cat src/app.py"}', + }, + { + "type": "function_call_output", + "call_id": "call_x", + "output": _NL_OUTPUT, + }, + ], + } + + new_payload, modified, _s, _t, _u, _c, _a = _run(handler, payload) + + assert modified is True + assert new_payload["input"][0] == "a bare string item" + assert new_payload["input"][3]["output"] == "kept words" diff --git a/tests/test_output_savings.py b/tests/test_output_savings.py index e9d67a826..f62bc3e37 100644 --- a/tests/test_output_savings.py +++ b/tests/test_output_savings.py @@ -389,12 +389,7 @@ class TestRecorderBaselineReload: @staticmethod def _key() -> str: - return stratum_key( - turn_kind="code", - input_tokens=8000, - model="claude-opus-4-8", - has_tools=True, - ) + return SAMPLE_KEY def test_adopts_baseline_learned_after_start(self, tmp_path): path = str(tmp_path / "output_savings.json") @@ -482,3 +477,99 @@ class TestRecorderBaselineReload: relearned.save(path) assert recorder.estimate().baseline_tokens > baseline_tokens_v1 + + +# --------------------------------------------------------------------------- +# flush durability + event-loop safety +# --------------------------------------------------------------------------- + +# Deterministic stratum key shared by the recorder tests below. +SAMPLE_KEY = stratum_key( + turn_kind="code", + input_tokens=8000, + model="claude-opus-4-8", + has_tools=True, +) + + +class TestFlushDurability: + def test_crash_mid_write_leaves_previous_ledger_intact(self, tmp_path, monkeypatch): + import headroom.fsutil + + path = str(tmp_path / "output_savings.json") + key = SAMPLE_KEY + + recorder = SavingsRecorder(path, flush_every=1) + recorder.record_from_labels([stratum_label("treatment", key)], 200) + recorder.flush() + assert SavingsLedger.load(path).treatment[key].n == 1 + + def _die_before_rename(*args, **kwargs): + raise OSError(5, "simulated crash before rename") + + monkeypatch.setattr(headroom.fsutil.os, "replace", _die_before_rename) + recorder.record_from_labels([stratum_label("treatment", key)], 210) + recorder.flush() # OSError swallowed by the recorder — fail-open by design + + # The pre-crash sample must survive and no temp residue may be left + # behind: a failed save may not corrupt or clutter the ledger. + assert SavingsLedger.load(path).treatment[key].n == 1 + assert not list(tmp_path.glob("*.tmp")) + + def test_corrupt_ledger_warns_and_starts_empty(self, tmp_path, caplog): + import logging + + path = tmp_path / "output_savings.json" + path.write_text("{not json") + + with caplog.at_level(logging.WARNING): + SavingsRecorder(str(path)) + + assert caplog.records, "corrupt ledger was swallowed silently" + + def test_emit_request_outcome_flushes_off_the_loop_thread(self, tmp_path, monkeypatch): + import asyncio + import threading + + from headroom.proxy.outcome import RequestOutcome, emit_request_outcome + + path = str(tmp_path / "output_savings.json") + recorder = SavingsRecorder(path, flush_every=1) + monkeypatch.setattr("headroom.proxy.output_savings.get_recorder", lambda: recorder) + + saved_on_threads = [] + real_save = SavingsLedger.save + + def _spy_save(self, save_path): + saved_on_threads.append(threading.get_ident()) + real_save(self, save_path) + + monkeypatch.setattr(SavingsLedger, "save", _spy_save) + + class _Metrics: + async def record_request(self, **kwargs): + pass + + class _Handler: + def __init__(self): + self.metrics = _Metrics() + self.cost_tracker = None + self.logger = None + + outcome = RequestOutcome( + request_id="req-shaper", + provider="openai", + model="gpt-5", + status_code=200, + original_tokens=100, + optimized_tokens=80, + output_tokens=50, + tokens_saved=20, + attempted_input_tokens=100, + transforms_applied=(stratum_label("treatment", SAMPLE_KEY),), + ) + asyncio.run(emit_request_outcome(_Handler(), outcome)) + + loop_thread = threading.get_ident() + assert saved_on_threads, "flush never ran" + assert all(t != loop_thread for t in saved_on_threads) diff --git a/tests/test_provider_codex_install.py b/tests/test_provider_codex_install.py index 05283762f..30e75bb27 100644 --- a/tests/test_provider_codex_install.py +++ b/tests/test_provider_codex_install.py @@ -85,3 +85,104 @@ def test_codex_provider_section_supports_custom_markers() -> None: assert section.endswith("# --- end ---\n") assert 'base_url = "http://127.0.0.1:9100/v1"' in section assert 'env_key = "OPENAI_API_KEY"' not in section + + +# --------------------------------------------------------------------------- +# ChatGPT-auth detection from the id_token claims (#3206) +# +# Newer Codex releases can write an auth.json with neither `auth_mode` nor a +# top-level `tokens.account_id`; the account identity lives only in the +# id_token claims. Those configs read as API-key mode, so requires_openai_auth +# is omitted, Codex attaches no Authorization header, and every request 401s +# with "Missing bearer" -- silently, with doctor reporting green. +# --------------------------------------------------------------------------- + + +def _unsigned_jwt(claims: dict[str, object]) -> str: + import base64 + import json as _json + + def seg(raw: bytes) -> str: + return base64.urlsafe_b64encode(raw).decode("ascii").rstrip("=") + + header = seg(b'{"alg":"none"}') + payload = seg(_json.dumps(claims).encode("utf-8")) + return ".".join((header, payload, "sig")) + + +_CHATGPT_CLAIMS: dict[str, object] = { + "https://api.openai.com/auth": { + "chatgpt_account_id": "1a155430-5551-47f4-9c7b-aeab7983f24a", + "chatgpt_plan_type": "pro", + } +} + + +def _write_auth(tmp_path, document: dict[str, object]): # noqa: ANN001, ANN202 + import json as _json + + path = tmp_path / "auth.json" + path.write_text(_json.dumps(document), encoding="utf-8") + return path + + +def test_chatgpt_auth_detected_from_id_token_claims_alone(tmp_path) -> None: + """The #3206 shape: no auth_mode, no tokens.account_id, only the JWT.""" + path = _write_auth(tmp_path, {"tokens": {"id_token": _unsigned_jwt(_CHATGPT_CLAIMS)}}) + + assert codex_uses_chatgpt_auth(path) is True + + +def test_explicit_api_key_mode_still_wins_over_a_chatgpt_id_token(tmp_path) -> None: + """Guards the #406 regression: API-key users must not get forced OAuth.""" + path = _write_auth( + tmp_path, + {"auth_mode": "apikey", "tokens": {"id_token": _unsigned_jwt(_CHATGPT_CLAIMS)}}, + ) + + assert codex_uses_chatgpt_auth(path) is False + + +def test_api_key_config_without_tokens_is_not_chatgpt(tmp_path) -> None: + path = _write_auth(tmp_path, {"OPENAI_API_KEY": "sk-test"}) + + assert codex_uses_chatgpt_auth(path) is False + + +def test_id_token_without_the_chatgpt_claim_is_not_chatgpt(tmp_path) -> None: + path = _write_auth(tmp_path, {"tokens": {"id_token": _unsigned_jwt({"sub": "user"})}}) + + assert codex_uses_chatgpt_auth(path) is False + + +def test_malformed_id_token_is_not_chatgpt(tmp_path) -> None: + for bogus in ("not-a-jwt", "a.b", "a.!!!not-base64!!!.c", ""): + path = _write_auth(tmp_path, {"tokens": {"id_token": bogus}}) + assert codex_uses_chatgpt_auth(path) is False, bogus + + +def test_blank_chatgpt_account_id_is_not_chatgpt(tmp_path) -> None: + claims = {"https://api.openai.com/auth": {"chatgpt_account_id": " "}} + path = _write_auth(tmp_path, {"tokens": {"id_token": _unsigned_jwt(claims)}}) + + assert codex_uses_chatgpt_auth(path) is False + + +def test_legacy_account_id_still_detected(tmp_path) -> None: + path = _write_auth(tmp_path, {"tokens": {"account_id": "acct-123"}}) + + assert codex_uses_chatgpt_auth(path) is True + + +def test_provider_block_emits_requires_openai_auth_for_the_new_shape(tmp_path) -> None: + """End of the chain: the JWT-only shape must produce the key Codex needs.""" + path = _write_auth(tmp_path, {"tokens": {"id_token": _unsigned_jwt(_CHATGPT_CLAIMS)}}) + + block = build_provider_section( + port=8787, + name="Headroom", + include_markers=False, + requires_openai_auth=codex_uses_chatgpt_auth(path), + ) + + assert "requires_openai_auth = true" in block diff --git a/tests/test_sqlite_graph_store.py b/tests/test_sqlite_graph_store.py index 9fb5a90b0..b3db37e62 100644 --- a/tests/test_sqlite_graph_store.py +++ b/tests/test_sqlite_graph_store.py @@ -689,6 +689,49 @@ class TestSQLiteGraphStoreEdgeCases: assert len(subgraph.entities) == 0 assert len(subgraph.relationships) == 0 + @pytest.mark.asyncio + async def test_one_corrupt_row_does_not_abort_a_multi_row_scan(self, tmp_path): + """A single unparseable row must not take down an entire query. + + Regression: ``_row_to_relationship`` / ``_row_to_entity`` parsed stored + JSON/timestamps with no guard, so one corrupt row (partial write, manual + edit, bad migration) raised inside the row loop and aborted the whole + ``query_subgraph`` / ``get_relationships`` scan — taking unrelated, + perfectly good edges down with it. The corrupt row is now skipped. + """ + import sqlite3 + + store = SQLiteGraphStore(db_path=str(tmp_path / "graph.db")) + a = Entity(user_id="u", name="A", entity_type="n") + b = Entity(user_id="u", name="B", entity_type="n") + c = Entity(user_id="u", name="C", entity_type="n") + for entity in (a, b, c): + await store.add_entity(entity) + await store.add_relationship( + Relationship(user_id="u", source_id=a.id, target_id=b.id, relation_type="e") + ) + await store.add_relationship( + Relationship(user_id="u", source_id=a.id, target_id=c.id, relation_type="e") + ) + + # Corrupt the A->B relationship row's properties JSON out-of-band. + con = sqlite3.connect(str(store.db_path)) + con.execute("UPDATE relationships SET properties = '{oops' WHERE target_id = ?", (b.id,)) + con.commit() + con.close() + + # get_relationships returns the one good edge instead of raising. + rels = await store.get_relationships(a.id) + assert len(rels) == 1 + assert rels[0].target_id == c.id + + # query_subgraph completes, skipping the corrupt edge and its node. + subgraph = await store.query_subgraph( + [a.id], max_hops=1, direction=RelationshipDirection.OUTGOING + ) + assert {e.name for e in subgraph.entities} == {"A", "C"} + assert len(subgraph.relationships) == 1 + @pytest.mark.asyncio async def test_entity_with_special_characters(self, store): """Test entity names with special characters.""" diff --git a/tests/test_transforms_content_detection.py b/tests/test_transforms_content_detection.py index 1201c2a60..e74776292 100644 --- a/tests/test_transforms_content_detection.py +++ b/tests/test_transforms_content_detection.py @@ -170,6 +170,49 @@ def test_search_detection_uses_match_ratio() -> None: assert _try_detect_search("\n\n") is None +def test_search_detection_rejects_datetime_prefixed_user_message() -> None: + """Regression: wrap-copilot ate one-line interactive prompts (2026-08-23). + + Copilot CLI prepends ``…`` to every + interactive user turn. The ISO-8601 ``T09:57:59`` matched the grep + ``file:line:`` pattern, so a datetime + one-line prompt classified as + SEARCH_RESULTS (1 match / 2 lines = 50% ≥ 30%) and the SearchCompressor + deleted the prompt line — the model received only the timestamp. + """ + incident = ( + "2026-08-23T09:57:59.792+02:00\n" + "\n" + "Please update the PR desc and check .overlay/ for hints." + ) + assert _try_detect_search(incident) is None + assert detect_content_type(incident).content_type is not ContentType.SEARCH_RESULTS + + +def test_search_detection_requires_two_matching_lines() -> None: + """A single coincidental ``word:digits:`` line must not classify prose.""" + assert _try_detect_search("src/foo.py:12:def foo():") is None + assert ( + _try_detect_search( + "Meeting at 09:30:00 tomorrow.\nBring the reports.\nDo not forget coffee." + ) + is None + ) + # Two genuine grep lines still classify. + two = "src/foo.py:12:def foo():\nsrc/bar.py:34: foo()" + result = _try_detect_search(two) + assert result is not None + assert result.content_type is ContentType.SEARCH_RESULTS + + +def test_search_detection_rejects_tag_like_and_key_value_prefixes() -> None: + """Markup / key=value lines are not file paths even with ``:\\d+:`` inside.""" + assert ( + _try_detect_search('started\nstopped') + is None + ) + assert _try_detect_search("timeout=30:12:retried\ntimeout=31:12:retried") is None + + def test_log_detection_prefers_build_output_patterns() -> None: log_output = "\n".join( [ diff --git a/tests/test_transforms_content_router.py b/tests/test_transforms_content_router.py index 4f76c6386..74ce9b5c9 100644 --- a/tests/test_transforms_content_router.py +++ b/tests/test_transforms_content_router.py @@ -1785,3 +1785,23 @@ def test_detect_content_overrides_html_misroute_for_grep_and_logs( "" ) assert _detect_content(html).content_type is ContentType.HTML + + +def test_datetime_prefixed_user_prompt_survives_router() -> None: + """Regression (2026-08-23): interactive wrap-copilot prompts were deleted. + + Copilot CLI prepends ``…`` to every + interactive user turn; the ISO timestamp matched the grep ``file:line:`` + detector, the one-line prompt classified as SEARCH_RESULTS, and + SearchCompressor kept only the datetime line — the model received no + request and answered "How can I help you today?". The router must never + route this shape to the search line-filter and must keep the prose. + """ + prompt = ( + "2026-08-23T09:57:59.792+02:00\n" + "\n" + "Please update the PR desc and check .overlay/ for hints." + ) + result = ContentRouter().compress(prompt) + assert result.strategy_used is not CompressionStrategy.SEARCH + assert "Please update the PR desc" in result.compressed diff --git a/tests/test_wrap_concurrent_settings.py b/tests/test_wrap_concurrent_settings.py new file mode 100644 index 000000000..f0ecfe76e --- /dev/null +++ b/tests/test_wrap_concurrent_settings.py @@ -0,0 +1,254 @@ +"""Concurrent `headroom wrap` sessions sharing one project's settings (#3205). + +`wrap claude` writes ANTHROPIC_BASE_URL into `.claude/settings.local.json` and +restores it on exit. Several sessions in one project run that read-modify-write +concurrently. The write is atomic so the file never tears, but the updates were +still lost against each other: + + * the first session's exit deleted the key while the others were still + running -- they silently stopped routing through the proxy, kept working, + and lost every byte of compression with no error anywhere; and + * a session that started second remembered the *first* session's proxy URL as + "the original", so its exit wrote a dead proxy back into the file, which + every later session in that project then failed to connect to. +""" + +from __future__ import annotations + +import json +from pathlib import Path +from unittest import mock + +import pytest + +from headroom.cli import wrap as W + + +@pytest.fixture +def settings(tmp_path: Path) -> Path: + path = tmp_path / ".claude" / "settings.local.json" + path.parent.mkdir(parents=True) + path.write_text(json.dumps({"env": {"FOO": "bar"}}), encoding="utf-8") + return path + + +def _env(path: Path) -> dict: + return json.loads(path.read_text(encoding="utf-8")).get("env", {}) if path.exists() else {} + + +class _Sessions: + """Drive several wrap sessions with distinct, controllable PIDs.""" + + def __init__(self, *pids: int) -> None: + self.live = set(pids) + + def __enter__(self) -> _Sessions: + self._patches = [ + mock.patch.object(W, "_pid_alive", lambda pid: pid in self.live), + mock.patch.object(W, "_identity_mismatch", lambda *a: False), + ] + for p in self._patches: + p.start() + return self + + def __exit__(self, *exc: object) -> None: + for p in self._patches: + p.stop() + + def launch(self, pid: int, url: str, path: Path, port: int | None = None) -> str | None: + with mock.patch("os.getpid", lambda: pid): + return W._write_claude_wrap_base_url(url, settings_path=path, port=port) + + def exit(self, pid: int, previous: str | None, path: Path) -> None: + self.live.discard(pid) + with mock.patch("os.getpid", lambda: pid): + W._restore_claude_wrap_base_url(previous, settings_path=path) + + def crash(self, pid: int) -> None: + """Vanish without running cleanup (SIGKILL, hard reboot).""" + self.live.discard(pid) + + +def test_first_session_exiting_leaves_the_others_routed(settings: Path) -> None: + """The reported symptom: sessions silently stop routing when a sibling exits.""" + with _Sessions(1001, 1002) as s: + a = s.launch(1001, "http://127.0.0.1:8787", settings) + s.launch(1002, "http://127.0.0.1:8788", settings) + + s.exit(1001, a, settings) + + assert "ANTHROPIC_BASE_URL" in _env(settings), "surviving session was unrouted" + + +def test_last_session_out_restores_the_true_original(settings: Path) -> None: + """A later session must not restore an earlier session's dead proxy URL.""" + with _Sessions(1001, 1002) as s: + a = s.launch(1001, "http://127.0.0.1:8787", settings) + b = s.launch(1002, "http://127.0.0.1:8788", settings) + + s.exit(1001, a, settings) + s.exit(1002, b, settings) + + assert _env(settings) == {"FOO": "bar"}, "stale proxy URL left behind" + + +def test_a_pre_existing_user_base_url_survives_the_whole_cycle(settings: Path) -> None: + """A URL the project already had is restored, not deleted.""" + settings.write_text( + json.dumps({"env": {"ANTHROPIC_BASE_URL": "http://user-proxy:1234"}}), encoding="utf-8" + ) + with _Sessions(1001, 1002) as s: + a = s.launch(1001, "http://127.0.0.1:8787", settings) + b = s.launch(1002, "http://127.0.0.1:8788", settings) + s.exit(1001, a, settings) + s.exit(1002, b, settings) + + assert _env(settings)["ANTHROPIC_BASE_URL"] == "http://user-proxy:1234" + + +def test_three_sessions_any_exit_order(settings: Path) -> None: + for order in ([1001, 1002, 1003], [1003, 1001, 1002], [1002, 1003, 1001]): + settings.write_text(json.dumps({"env": {"FOO": "bar"}}), encoding="utf-8") + with _Sessions(*order) as s: + prev = { + pid: s.launch(pid, f"http://127.0.0.1:{8787 + i}", settings) + for i, pid in enumerate(order) + } + for pid in order[:-1]: + s.exit(pid, prev[pid], settings) + assert "ANTHROPIC_BASE_URL" in _env(settings), f"unrouted early in {order}" + s.exit(order[-1], prev[order[-1]], settings) + assert _env(settings) == {"FOO": "bar"}, f"residue after {order}" + + +def test_a_crashed_session_does_not_wedge_the_key(settings: Path) -> None: + """A SIGKILLed session never releases; its claim must be pruned as dead.""" + with _Sessions(1001, 1002) as s: + s.launch(1001, "http://127.0.0.1:8787", settings) + b = s.launch(1002, "http://127.0.0.1:8788", settings) + + s.crash(1001) + s.exit(1002, b, settings) + + assert _env(settings) == {"FOO": "bar"} + assert not W._wrap_owners_path(settings).exists() + + +def test_single_session_behaviour_is_unchanged(settings: Path) -> None: + with _Sessions(1001) as s: + a = s.launch(1001, "http://127.0.0.1:8787", settings) + assert _env(settings)["ANTHROPIC_BASE_URL"] == "http://127.0.0.1:8787" + s.exit(1001, a, settings) + + assert _env(settings) == {"FOO": "bar"} + + +def test_restore_without_an_owner_record_still_honours_the_caller(settings: Path) -> None: + """unwrap and legacy sessions pass the previous value directly.""" + settings.write_text( + json.dumps({"env": {"ANTHROPIC_BASE_URL": "http://127.0.0.1:8787"}}), encoding="utf-8" + ) + assert not W._wrap_owners_path(settings).exists() + + W._restore_claude_wrap_base_url("http://legacy:9999", settings_path=settings) + + assert _env(settings)["ANTHROPIC_BASE_URL"] == "http://legacy:9999" + + +def test_tool_search_key_is_tracked_independently(settings: Path) -> None: + """Ownership is per key -- the tool-search entry has the same race.""" + with _Sessions(1001, 1002) as s: + with mock.patch("os.getpid", lambda: 1001): + a = W._write_claude_wrap_tool_search("auto", settings_path=settings) + with mock.patch("os.getpid", lambda: 1002): + W._write_claude_wrap_tool_search("auto", settings_path=settings) + + s.live.discard(1001) + with mock.patch("os.getpid", lambda: 1001): + W._restore_claude_wrap_tool_search(a, settings_path=settings) + + assert W._TOOL_SEARCH_ENV in _env(settings), "surviving session lost tool-search" + + +def test_exit_on_signal_unwinds_so_finally_can_run() -> None: + """`cleanup` as the handler never unwound; the settings restore never ran.""" + with pytest.raises(SystemExit) as excinfo: + W._exit_on_signal(15, None) + + assert excinfo.value.code == 143 + + +def test_unwrap_forces_the_restore_past_a_live_session(settings: Path) -> None: + """`unwrap` is the user asking for their settings back -- it must not no-op. + + Deferring to a live sibling is right for a session exiting on its own, but + unwrap deferring means the command prints success while leaving the proxy + URL in the file. + """ + with _Sessions(1001) as s: + s.launch(1001, "http://127.0.0.1:8787", settings) + + with mock.patch("os.getpid", lambda: 2002): + W._restore_claude_wrap_base_url(None, settings_path=settings, force=True) + + assert _env(settings) == {"FOO": "bar"}, "unwrap left the proxy URL behind" + assert not W._wrap_owners_path(settings).exists(), "unwrap left ownership state behind" + + +def test_unwrap_restores_the_true_original_not_the_marker_value(settings: Path) -> None: + """A caller with no claim of its own trusts the record over its marker. + + The single-slot marker is won by the *last* writer, whose `previous` is the + first session's proxy URL -- restoring that is the #3205 bug via unwrap. + """ + settings.write_text( + json.dumps({"env": {"ANTHROPIC_BASE_URL": "http://user-proxy:1234"}}), encoding="utf-8" + ) + with _Sessions(1001, 1002) as s: + s.launch(1001, "http://127.0.0.1:8787", settings, port=8787) + s.launch(1002, "http://127.0.0.1:8788", settings, port=8788) + + with mock.patch("os.getpid", lambda: 2002): + W._restore_claude_wrap_base_url( + "http://127.0.0.1:8787", settings_path=settings, force=True + ) + + assert _env(settings)["ANTHROPIC_BASE_URL"] == "http://user-proxy:1234" + + +def test_a_holder_that_outlived_its_proxy_cannot_veto_the_selfheal(settings: Path) -> None: + """#2221: a wrapper PID can outlive its proxy; its claim must not block.""" + with _Sessions(1001) as s: + s.launch(1001, "http://127.0.0.1:8787", settings, port=8787) + + # PID 1001 is still alive, but port 8787 has been proven dead. + W._restore_claude_wrap_base_url(None, settings_path=settings, dead_ports=frozenset({8787})) + + assert _env(settings) == {"FOO": "bar"}, "dead proxy URL survived the self-heal" + + +def test_exiting_session_hands_its_marker_to_a_survivor(settings: Path) -> None: + """The marker has one slot; the leaver must not strand or hijack it.""" + with _Sessions(1001, 1002) as s: + s.launch(1001, "http://127.0.0.1:8787", settings, port=8787) + b = s.launch(1002, "http://127.0.0.1:8788", settings, port=8788) + + marker = W._read_wrap_marker(settings) + assert marker is not None and marker["pid"] == 1002, "last writer owns the marker" + + s.exit(1002, b, settings) + + marker = W._read_wrap_marker(settings) + assert marker is not None, "survivor lost its #2221 self-heal record" + assert marker["pid"] == 1001, "marker still describes the exited session" + assert marker["port"] == 8787 + assert marker["previous"] is None, "marker must carry the true original" + + +def test_the_founding_session_still_honours_an_explicit_previous(settings: Path) -> None: + """A sole writer observed the pre-wrap value first-hand; do not override it.""" + with _Sessions(1001) as s: + s.launch(1001, "http://127.0.0.1:8787", settings) + s.exit(1001, "https://existing-gateway.example.com/v1", settings) + + assert _env(settings)["ANTHROPIC_BASE_URL"] == "https://existing-gateway.example.com/v1"