From 16c638bc211ecc6d1768bbe36e0c12971996e104 Mon Sep 17 00:00:00 2001 From: Ralf Escher <47742057+ralf-escher@users.noreply.github.com> Date: Mon, 29 Jun 2026 00:26:38 +0200 Subject: [PATCH] fix: recover persistent proxy feature checks and reject non-Copilot exchange URL (#1465) ## Description This PR fixes two related reliability issues in Copilot wrap/subscription flows: 1. Recovered persistent proxy instances could be reused too early, before validating requested feature-sensitive config (especially `openai_api_url`), which could lead to wrong upstream routing. 2. Subscription token-exchange payloads could provide a non-Copilot API URL; this is now rejected and we safely fall back to user-info/default Copilot endpoint resolution. Related: #488 ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - Updated persistent proxy recover path to: - continue into feature checks when feature-sensitive options are requested - restart persistent deployment when config is missing/mismatched after recovery - keep historical fast return for plain recover-only calls - Hardened subscription exchange URL resolution: - accept exchange `api_url` only when it is a Copilot host - log warning and fall back when non-Copilot host is provided - Added regression tests for: - recovered persistent proxy feature mismatch and config-unavailable restart behavior - non-Copilot exchange host rejection with/without user-info fallback ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [ ] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output ```text $ python -m pytest -q tests/test_copilot_auth.py tests/test_cli/test_wrap_persistent.py ============================= test session starts ============================= platform win32 -- Python 3.12.8, pytest-9.1.1, pluggy-1.6.0 rootdir: C:\Users\ralf.escher\Documents\headroom collected 82 items tests\test_copilot_auth.py ............................................. [ 54%] ........... [ 68%] tests\test_cli\test_wrap_persistent.py .......................... [100%] ============================= 82 passed in 1.60s ============================== ``` ## Real Behavior Proof - Environment: - Windows - Python 3.12.8 - Local Headroom branch with this patch - Copilot subscription route through local proxy - Exact command / steps: 1. Start local proxy and run Copilot wrap in subscription mode. 2. Execute chat-completions requests through proxy. 3. Inspect runtime proxy logs for outbound target and inbound status. 4. Run focused regression tests: - `python -m pytest -q tests/test_copilot_auth.py tests/test_cli/test_wrap_persistent.py` - Observed result: - Outbound requests routed to Copilot business host: - `path=https://api.business.githubcopilot.com/chat/completions` - Successful proxy responses observed: - `path=/v1/chat/completions status=200` - Model activity logged during successful requests: - `PERF model=gpt-4.1 ...` - Regression tests pass (`82 passed`), covering both fixes. - Not tested: - Full repository test suite - Full lint/typecheck across entire project - Non-Windows runtime verification in this run ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] I have updated the CHANGELOG.md if applicable ## Additional Notes - This PR intentionally excludes incidental local edits to `.github/copilot-instructions.md`. - Scope is limited to this bug fix and regression coverage; linked as related work to #488. --- headroom/cli/wrap.py | 103 +++++++++- headroom/copilot_auth.py | 7 +- tests/test_cli/test_wrap_persistent.py | 255 +++++++++++++++++++++++++ tests/test_copilot_auth.py | 36 ++++ 4 files changed, 391 insertions(+), 10 deletions(-) diff --git a/headroom/cli/wrap.py b/headroom/cli/wrap.py index b7ff1cd36..8f3a020cb 100644 --- a/headroom/cli/wrap.py +++ b/headroom/cli/wrap.py @@ -2608,15 +2608,100 @@ def _ensure_proxy( f"Persistent deployment '{manifest.profile}' on port {port} " f"is running stale Headroom {running_version} and could not be restarted." ) - click.echo(f" Proxy already running on port {port}") - click.echo(f" Dashboard: http://127.0.0.1:{port}/dashboard") - return None - if helpers._recover_persistent_proxy(port): - return None - if helpers._check_proxy(port): - raise click.ClickException( - f"Persistent deployment '{manifest.profile}' on port {port} is not healthy." - ) + # Check if the running proxy has the features we need. + # Without this, a persistent deployment started for one use case + # (e.g. --backend anthropic) would be silently reused for another + # (e.g. --subscription --provider-type openai) causing auth failures. + running_config = helpers._proxy_health_config(health_payload) + if running_config is None: + running_config = helpers._query_proxy_config(port) + if running_config is not None: + missing = [] + if memory and not running_config.get("memory"): + missing.append("memory") + if learn and not running_config.get("learn"): + missing.append("learn") + if code_graph and not running_config.get("code_graph"): + missing.append("code_graph") + if openai_api_url: + running_openai_url = _normalize_proxy_api_url( + running_config.get("openai_api_url") + ) + requested_openai_url = _normalize_proxy_api_url(openai_api_url) + if running_openai_url != requested_openai_url: + missing.append("openai-api-url") + if not missing: + click.echo(f" Proxy already running on port {port}") + click.echo(f" Dashboard: http://127.0.0.1:{port}/dashboard") + return None + # Features mismatch or config unavailable — fall through to + # the non-persistent path which handles proxy restart. + else: + if helpers._recover_persistent_proxy(port): + # If the caller requested feature-sensitive config (e.g. + # openai_api_url for Copilot subscription), continue into + # the shared running-proxy checks below so mismatch-driven + # restart logic can run. For plain recover-only calls, + # preserve the historical fast return. + if not any((memory, learn, code_graph, openai_api_url)): + return None + if not helpers._check_proxy(port): + return None + + # A freshly recovered persistent proxy may not expose + # a full config payload yet. In feature-sensitive flows + # (e.g. Copilot subscription), treat missing or mismatched + # config as restart-required and refresh the persistent + # deployment directly instead of silently reusing it. + health_payload = helpers._query_proxy_health(port) + running_config = helpers._proxy_health_config(health_payload) + if running_config is None: + running_config = helpers._query_proxy_config(port) + + if running_config is None: + click.echo( + f" Recovered persistent deployment '{manifest.profile}' " + "did not expose config; restarting with requested features..." + ) + if helpers._restart_persistent_proxy(manifest, port): + return None + raise click.ClickException( + f"Persistent deployment '{manifest.profile}' on port {port} " + "could not be restarted after recovery." + ) + + missing = [] + if memory and not running_config.get("memory"): + missing.append("memory") + if learn and not running_config.get("learn"): + missing.append("learn") + if code_graph and not running_config.get("code_graph"): + missing.append("code-graph") + if openai_api_url: + running_openai_url = _normalize_proxy_api_url( + running_config.get("openai_api_url") + ) + requested_openai_url = _normalize_proxy_api_url(openai_api_url) + if running_openai_url != requested_openai_url: + missing.append("openai-api-url") + + if missing: + flags_str = ", ".join(f"--{f}" for f in missing) + click.echo( + f" Recovered persistent deployment '{manifest.profile}' is missing: " + f"{flags_str}; restarting..." + ) + if helpers._restart_persistent_proxy(manifest, port): + return None + raise click.ClickException( + f"Persistent deployment '{manifest.profile}' on port {port} " + "could not be restarted with requested features." + ) + return None + elif helpers._check_proxy(port): + raise click.ClickException( + f"Persistent deployment '{manifest.profile}' on port {port} is not healthy." + ) click.echo( f" Warning: persistent deployment '{manifest.profile}' on port {port} " "is stale; starting a fresh proxy instead." diff --git a/headroom/copilot_auth.py b/headroom/copilot_auth.py index 49cd7d6ec..6484ed533 100644 --- a/headroom/copilot_auth.py +++ b/headroom/copilot_auth.py @@ -779,7 +779,12 @@ def _api_url_from_exchange_payload(payload: dict[str, Any], *, oauth_token: str) api_url = _api_url_from_payload(payload) if api_url: - return api_url + if is_copilot_api_url(api_url): + return api_url + logger.warning( + "Ignoring non-Copilot API URL from token exchange payload: %s", + api_url, + ) return _subscription_api_url_from_user_info(oauth_token) diff --git a/tests/test_cli/test_wrap_persistent.py b/tests/test_cli/test_wrap_persistent.py index e440dc8db..de5a3951d 100644 --- a/tests/test_cli/test_wrap_persistent.py +++ b/tests/test_cli/test_wrap_persistent.py @@ -559,3 +559,258 @@ def test_ensure_proxy_restarts_for_flags_when_no_other_wrapper(monkeypatch) -> N assert result is None assert calls[0] == ("kill", 12345, 8787) assert calls[1][0] == "start" + + +def test_ensure_proxy_restarts_persistent_deployment_for_feature_mismatch(monkeypatch) -> None: + """Persistent deployment should restart when requested features differ from running config.""" + calls: list[object] = [] + health = { + "version": wrap_cli._HEADROOM_VERSION, + "runtime": {"websocket_sessions": {"active_sessions": 0, "active_relay_tasks": 0}}, + "config": { + "pid": 12345, + "memory": False, + "learn": False, + "code_graph": False, + "openai_api_url": None, + }, + } + + monkeypatch.setattr(wrap_cli, "_find_persistent_manifest", lambda port: _Manifest()) + monkeypatch.setattr("headroom.install.health.probe_ready", lambda url: True) + monkeypatch.setattr(wrap_cli, "_query_proxy_health", lambda port: health) + # Persistent proxy is running, so _check_proxy returns True + monkeypatch.setattr(wrap_cli, "_check_proxy", lambda port: True) + monkeypatch.setattr(wrap_cli, "_port_bind_error", lambda port: None) + monkeypatch.setattr( + wrap_cli, + "_kill_proxy_by_pid", + lambda pid, port: calls.append(("kill", pid, port)) or True, + ) + monkeypatch.setattr( + wrap_cli, + "_start_proxy", + lambda *args, **kwargs: calls.append(("start", args, kwargs)), + ) + + # Request openai_api_url that differs from running config (None) + result = wrap_cli._ensure_proxy( + 8787, + False, + openai_api_url="https://api.githubcopilot.com", + ) + + assert result is None + # Proxy should be killed and restarted due to openai_api_url mismatch + assert calls[0] == ("kill", 12345, 8787) + assert calls[1][0] == "start" + assert calls[1][2]["openai_api_url"] == "https://api.githubcopilot.com" + + +def test_ensure_proxy_restarts_persistent_deployment_for_memory_mismatch(monkeypatch) -> None: + """Persistent deployment should restart when memory is requested but not enabled.""" + calls: list[object] = [] + health = { + "version": wrap_cli._HEADROOM_VERSION, + "runtime": {"websocket_sessions": {"active_sessions": 0, "active_relay_tasks": 0}}, + "config": { + "pid": 12345, + "memory": False, + "learn": False, + "code_graph": False, + "openai_api_url": None, + }, + } + + monkeypatch.setattr(wrap_cli, "_find_persistent_manifest", lambda port: _Manifest()) + monkeypatch.setattr("headroom.install.health.probe_ready", lambda url: True) + monkeypatch.setattr(wrap_cli, "_query_proxy_health", lambda port: health) + # Persistent proxy is running, so _check_proxy returns True + monkeypatch.setattr(wrap_cli, "_check_proxy", lambda port: True) + monkeypatch.setattr(wrap_cli, "_port_bind_error", lambda port: None) + monkeypatch.setattr( + wrap_cli, + "_kill_proxy_by_pid", + lambda pid, port: calls.append(("kill", pid, port)) or True, + ) + monkeypatch.setattr( + wrap_cli, + "_start_proxy", + lambda *args, **kwargs: calls.append(("start", args, kwargs)), + ) + + # Request memory that differs from running config (False) + result = wrap_cli._ensure_proxy(8787, False, memory=True) + + assert result is None + # Proxy should be killed and restarted due to memory mismatch + assert calls[0] == ("kill", 12345, 8787) + assert calls[1][0] == "start" + + +def test_ensure_proxy_restarts_recovered_persistent_for_openai_api_url_mismatch( + monkeypatch, +) -> None: + calls: list[object] = [] + health = { + "version": wrap_cli._HEADROOM_VERSION, + "runtime": {"websocket_sessions": {"active_sessions": 0, "active_relay_tasks": 0}}, + "config": { + "pid": 12345, + "memory": False, + "learn": False, + "code_graph": False, + "openai_api_url": None, + }, + } + + monkeypatch.setattr(wrap_cli, "_find_persistent_manifest", lambda port: _Manifest()) + monkeypatch.setattr("headroom.install.health.probe_ready", lambda url: False) + monkeypatch.setattr(wrap_cli, "_recover_persistent_proxy", lambda port: True) + monkeypatch.setattr(wrap_cli, "_check_proxy", lambda port: True) + monkeypatch.setattr(wrap_cli, "_query_proxy_health", lambda port: health) + monkeypatch.setattr( + wrap_cli, + "_restart_persistent_proxy", + lambda manifest, port: calls.append(("restart", manifest.profile, port)) or True, + ) + monkeypatch.setattr( + wrap_cli, + "_start_proxy", + lambda *args, **kwargs: (_ for _ in ()).throw( + AssertionError("ephemeral proxy should not start") + ), + ) + + result = wrap_cli._ensure_proxy( + 8787, + False, + openai_api_url="https://api.business.githubcopilot.com", + ) + + assert result is None + assert calls == [("restart", "default", 8787)] + + +def test_ensure_proxy_restarts_recovered_persistent_when_config_unavailable(monkeypatch) -> None: + calls: list[object] = [] + + monkeypatch.setattr(wrap_cli, "_find_persistent_manifest", lambda port: _Manifest()) + monkeypatch.setattr("headroom.install.health.probe_ready", lambda url: False) + monkeypatch.setattr(wrap_cli, "_recover_persistent_proxy", lambda port: True) + monkeypatch.setattr(wrap_cli, "_check_proxy", lambda port: True) + monkeypatch.setattr(wrap_cli, "_query_proxy_health", lambda port: {"version": "x"}) + monkeypatch.setattr(wrap_cli, "_query_proxy_config", lambda port: None) + monkeypatch.setattr( + wrap_cli, + "_restart_persistent_proxy", + lambda manifest, port: calls.append(("restart", manifest.profile, port)) or True, + ) + monkeypatch.setattr( + wrap_cli, + "_start_proxy", + lambda *args, **kwargs: (_ for _ in ()).throw( + AssertionError("ephemeral proxy should not start") + ), + ) + + result = wrap_cli._ensure_proxy( + 8787, + False, + openai_api_url="https://api.business.githubcopilot.com", + ) + + assert result is None + assert calls == [("restart", "default", 8787)] + + +def test_ensure_proxy_reuses_persistent_deployment_when_features_match(monkeypatch) -> None: + """Persistent deployment should be reused when all requested features match.""" + health = { + "version": wrap_cli._HEADROOM_VERSION, + "runtime": {"websocket_sessions": {"active_sessions": 0, "active_relay_tasks": 0}}, + "config": { + "pid": 12345, + "memory": True, + "learn": False, + "code_graph": False, + "openai_api_url": "https://api.githubcopilot.com", + }, + } + + monkeypatch.setattr(wrap_cli, "_find_persistent_manifest", lambda port: _Manifest()) + monkeypatch.setattr("headroom.install.health.probe_ready", lambda url: True) + monkeypatch.setattr(wrap_cli, "_query_proxy_health", lambda port: health) + monkeypatch.setattr( + wrap_cli, + "_restart_persistent_proxy", + lambda *args, **kwargs: (_ for _ in ()).throw( + AssertionError("should not restart when features match") + ), + ) + monkeypatch.setattr( + wrap_cli, + "_start_proxy", + lambda *args, **kwargs: (_ for _ in ()).throw( + AssertionError("should not start ephemeral proxy when features match") + ), + ) + + # Request same features as running config + result = wrap_cli._ensure_proxy( + 8787, + False, + memory=True, + openai_api_url="https://api.githubcopilot.com", + ) + + assert result is None + + +def test_ensure_proxy_recovered_persistent_deployment_checks_feature_mismatch(monkeypatch) -> None: + """Recovered persistent deployments must still restart on feature mismatch. + + Regression guard for the recover path: when wrap requests a different + openai_api_url (Copilot subscription), do not early-return right after + recover; run the shared mismatch checks and restart if needed. + """ + + calls: list[object] = [] + health = { + "version": wrap_cli._HEADROOM_VERSION, + "runtime": {"websocket_sessions": {"active_sessions": 0, "active_relay_tasks": 0}}, + "config": { + "pid": "12345", + "memory": False, + "learn": False, + "code_graph": False, + "openai_api_url": None, + }, + } + + monkeypatch.setattr(wrap_cli, "_find_persistent_manifest", lambda port: _Manifest()) + monkeypatch.setattr("headroom.install.health.probe_ready", lambda url: False) + monkeypatch.setattr(wrap_cli, "_recover_persistent_proxy", lambda port: True) + monkeypatch.setattr(wrap_cli, "_check_proxy", lambda port: True) + monkeypatch.setattr(wrap_cli, "_query_proxy_health", lambda port: health) + monkeypatch.setattr( + wrap_cli, + "_restart_persistent_proxy", + lambda manifest, port: calls.append(("restart", manifest.profile, port)) or True, + ) + monkeypatch.setattr( + wrap_cli, + "_start_proxy", + lambda *args, **kwargs: (_ for _ in ()).throw( + AssertionError("ephemeral proxy should not start") + ), + ) + + result = wrap_cli._ensure_proxy( + 8787, + False, + openai_api_url="https://api.githubcopilot.com", + ) + + assert result is None + assert calls == [("restart", "default", 8787)] diff --git a/tests/test_copilot_auth.py b/tests/test_copilot_auth.py index c1395a722..389c53776 100644 --- a/tests/test_copilot_auth.py +++ b/tests/test_copilot_auth.py @@ -250,6 +250,42 @@ def test_resolve_subscription_exchange_uses_cloud_enterprise_advertised_api( assert copilot_auth._token_exchange_url() == "https://api.github.com/copilot_internal/v2/token" +def test_api_url_from_exchange_payload_rejects_non_copilot_host( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.delenv("GITHUB_COPILOT_API_URL", raising=False) + monkeypatch.delenv("GITHUB_COPILOT_ENTERPRISE_URL", raising=False) + monkeypatch.delenv("GITHUB_COPILOT_ENTERPRISE_DOMAIN", raising=False) + monkeypatch.setattr( + copilot_auth, + "_fetch_copilot_user_info", + lambda _token: {"endpoints": {"api": "https://api.business.githubcopilot.com"}}, + ) + + resolved = copilot_auth._api_url_from_exchange_payload( + {"endpoints": {"api": "https://api.openai.com/v1"}}, + oauth_token="gho-oauth", + ) + + assert resolved == "https://api.business.githubcopilot.com" + + +def test_api_url_from_exchange_payload_rejects_non_copilot_host_without_user_info( + monkeypatch: pytest.MonkeyPatch, +) -> None: + monkeypatch.delenv("GITHUB_COPILOT_API_URL", raising=False) + monkeypatch.delenv("GITHUB_COPILOT_ENTERPRISE_URL", raising=False) + monkeypatch.delenv("GITHUB_COPILOT_ENTERPRISE_DOMAIN", raising=False) + monkeypatch.setattr(copilot_auth, "_fetch_copilot_user_info", lambda _token: None) + + resolved = copilot_auth._api_url_from_exchange_payload( + {"endpoints": {"api": "https://api.openai.com/v1"}}, + oauth_token="gho-oauth", + ) + + assert resolved == copilot_auth.DEFAULT_API_URL + + def test_enterprise_domain_routes_token_exchange_and_user_info_together( monkeypatch: pytest.MonkeyPatch, ) -> None: