diff --git a/hermes_cli/models.py b/hermes_cli/models.py index 02718d33..19a036b9 100644 --- a/hermes_cli/models.py +++ b/hermes_cli/models.py @@ -1706,12 +1706,18 @@ def _resolve_copilot_catalog_api_key() -> str: 1. ``resolve_api_key_provider_credentials("copilot")`` — env vars (``COPILOT_GITHUB_TOKEN`` / ``GH_TOKEN`` / ``GITHUB_TOKEN``) plus the ``gh auth token`` CLI fallback. - 2. ``read_credential_pool("copilot")`` — OAuth ``access_token`` saved - in ``auth.json`` by ``hermes auth add copilot`` (device-code flow). + 2. ``read_credential_pool("copilot")`` — a token (typically a + ``gho_*`` from device-code login, or a fine-grained PAT) stored in + ``auth.json`` under ``credential_pool.copilot[]``. The pool is + populated by ``hermes auth add copilot`` and by ``_seed_from_env`` + when the env var is set in ``~/.hermes/.env``. - Without (2), users whose only Copilot credential is the OAuth token - Hermes itself stores see the ``/model`` picker fall back to a stale - hardcoded list because the live catalog fetch silently 401s. + Without (2), users whose only Copilot credential is in the pool see + the ``/model`` picker fall back to a stale hardcoded list because the + live catalog fetch silently 401s. To avoid wedging on a malformed pool + entry, each candidate is exchanged via ``exchange_copilot_token`` — + only entries that actually exchange successfully are returned, so a + later valid entry is reachable when an earlier one is unsupported. """ try: from hermes_cli.auth import resolve_api_key_provider_credentials @@ -1726,7 +1732,7 @@ def _resolve_copilot_catalog_api_key() -> str: try: from hermes_cli.auth import read_credential_pool from hermes_cli.copilot_auth import ( - get_copilot_api_token, + exchange_copilot_token, validate_copilot_token, ) @@ -1739,7 +1745,12 @@ def _resolve_copilot_catalog_api_key() -> str: valid, _ = validate_copilot_token(raw) if not valid: continue - return get_copilot_api_token(raw) + try: + api_token, _expires_at = exchange_copilot_token(raw) + except Exception: + continue + if api_token: + return api_token except Exception: pass diff --git a/tests/hermes_cli/test_copilot_catalog_oauth_fallback.py b/tests/hermes_cli/test_copilot_catalog_oauth_fallback.py index 7e364990..be383b23 100644 --- a/tests/hermes_cli/test_copilot_catalog_oauth_fallback.py +++ b/tests/hermes_cli/test_copilot_catalog_oauth_fallback.py @@ -1,11 +1,13 @@ """Catalog-API-key fallback for the Copilot ``/model`` picker. -Regression for #16708: when the user's only Copilot credential is the -OAuth ``access_token`` saved in ``auth.json`` (the device-code flow that -``hermes auth add copilot`` itself produces), the picker was silently -dropping back to a stale hardcoded list because -``_resolve_copilot_catalog_api_key`` only consulted env vars / ``gh -auth token`` and never read the credential pool. +Regression for #16708: when the user's only Copilot credential is a +``gho_*`` token (typically obtained via device-code login) stored in +``auth.json`` under ``credential_pool.copilot[]`` — placed there by +``hermes auth add copilot`` or by ``_seed_from_env`` when the env var +is set in ``~/.hermes/.env`` — the picker was silently dropping back to +a stale hardcoded list because ``_resolve_copilot_catalog_api_key`` +only consulted env vars / ``gh auth token`` and never read the +credential pool. """ from unittest.mock import patch @@ -26,7 +28,7 @@ class TestCopilotCatalogApiKeyResolution: mock_pool.assert_not_called() def test_falls_back_to_pool_oauth_token(self): - """Empty env → walk credential_pool.copilot[] for OAuth access_token.""" + """Empty env → walk credential_pool.copilot[] for an OAuth access_token.""" with patch( "hermes_cli.auth.resolve_api_key_provider_credentials", return_value={"api_key": ""}, @@ -34,10 +36,10 @@ class TestCopilotCatalogApiKeyResolution: "hermes_cli.auth.read_credential_pool", return_value=[{"access_token": "gho_abc123"}], ), patch( - "hermes_cli.copilot_auth.get_copilot_api_token", - return_value="exchanged-tid_xyz", + "hermes_cli.copilot_auth.exchange_copilot_token", + return_value=("tid_exchanged_xyz", 1234567890.0), ): - assert _resolve_copilot_catalog_api_key() == "exchanged-tid_xyz" + assert _resolve_copilot_catalog_api_key() == "tid_exchanged_xyz" def test_falls_back_when_env_resolution_raises(self): """Env path raising an exception still falls through to the pool.""" @@ -48,10 +50,10 @@ class TestCopilotCatalogApiKeyResolution: "hermes_cli.auth.read_credential_pool", return_value=[{"access_token": "gho_xyz"}], ), patch( - "hermes_cli.copilot_auth.get_copilot_api_token", - return_value="exchanged-tid_xyz", + "hermes_cli.copilot_auth.exchange_copilot_token", + return_value=("tid_exchanged_xyz", 1234567890.0), ): - assert _resolve_copilot_catalog_api_key() == "exchanged-tid_xyz" + assert _resolve_copilot_catalog_api_key() == "tid_exchanged_xyz" def test_skips_classic_pat_in_pool(self): """Classic PATs (``ghp_…``) are unsupported by the Copilot API — skip them.""" @@ -62,12 +64,12 @@ class TestCopilotCatalogApiKeyResolution: "hermes_cli.auth.read_credential_pool", return_value=[{"access_token": "ghp_classic_pat"}], ), patch( - "hermes_cli.copilot_auth.get_copilot_api_token", + "hermes_cli.copilot_auth.exchange_copilot_token", ) as mock_exchange: assert _resolve_copilot_catalog_api_key() == "" mock_exchange.assert_not_called() - def test_skips_invalid_pool_entries(self): + def test_skips_invalid_pool_entries_until_first_exchangeable(self): """Non-dict entries and entries without an ``access_token`` are skipped.""" with patch( "hermes_cli.auth.resolve_api_key_provider_credentials", @@ -82,12 +84,56 @@ class TestCopilotCatalogApiKeyResolution: {"access_token": "gho_should_not_reach"}, ], ), patch( - "hermes_cli.copilot_auth.get_copilot_api_token", - return_value="exchanged-from-first", + "hermes_cli.copilot_auth.exchange_copilot_token", + return_value=("tid_from_first", 1234567890.0), ) as mock_exchange: - assert _resolve_copilot_catalog_api_key() == "exchanged-from-first" + assert _resolve_copilot_catalog_api_key() == "tid_from_first" mock_exchange.assert_called_once_with("gho_first_real_token") + def test_skips_pool_entry_that_fails_to_exchange(self): + """If the first entry won't exchange, try the next — an unsupported pool[0] + must not wedge a later valid entry (Copilot review #16868 finding).""" + attempts: list[str] = [] + + def fake_exchange(raw_token: str): + attempts.append(raw_token) + if raw_token == "gho_unsupported_account": + raise ValueError("Copilot token exchange failed: HTTP 401") + return ("tid_from_second", 1234567890.0) + + with patch( + "hermes_cli.auth.resolve_api_key_provider_credentials", + return_value={"api_key": ""}, + ), patch( + "hermes_cli.auth.read_credential_pool", + return_value=[ + {"access_token": "gho_unsupported_account"}, + {"access_token": "gho_valid_token"}, + ], + ), patch( + "hermes_cli.copilot_auth.exchange_copilot_token", + side_effect=fake_exchange, + ): + assert _resolve_copilot_catalog_api_key() == "tid_from_second" + assert attempts == ["gho_unsupported_account", "gho_valid_token"] + + def test_all_pool_entries_fail_exchange_returns_empty(self): + """All exchanges fail → return "" so the caller falls back to curated.""" + with patch( + "hermes_cli.auth.resolve_api_key_provider_credentials", + return_value={"api_key": ""}, + ), patch( + "hermes_cli.auth.read_credential_pool", + return_value=[ + {"access_token": "gho_expired_a"}, + {"access_token": "gho_expired_b"}, + ], + ), patch( + "hermes_cli.copilot_auth.exchange_copilot_token", + side_effect=ValueError("Copilot token exchange failed"), + ): + assert _resolve_copilot_catalog_api_key() == "" + def test_returns_empty_string_when_no_credentials_anywhere(self): """No env, no pool → empty string (caller falls back to curated list).""" with patch(