mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
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.
This commit is contained in:
Ralf Escher
GitHub
parent
4db3bc91d9
commit
16c638bc21
4 changed files with 391 additions and 10 deletions
|
|
@ -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."
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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)]
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue