molecule-ai/hermes-agent
35
0
Fork 0
Code Issues 2 Pull Requests 4 Actions 9 Packages Projects Releases Wiki Activity

fix(copilot): require successful exchange when walking credential_pool catalog tokens

Browse Source 
Address Copilot review on #16868:

1. Tighten pool iteration. ``validate_copilot_token`` only rejects empty
   strings and classic PATs (``ghp_*``); a malformed/unsupported ``gho_*``
   token at ``credential_pool.copilot[0]`` would pass the gate and short-
   circuit the loop, hiding a later valid entry. Switch to calling
   ``exchange_copilot_token`` directly: only entries that actually exchange
   into a live Copilot API token are returned. Bad/expired entries fall
   through to the next, and an exhausted pool returns ``""`` so the picker
   falls back to the curated list (existing behaviour).

2. Reword the docstring + test module docstring to describe the pool seed
   path accurately — ``hermes auth add copilot`` adds an api-key-typed
   credential whose ``access_token`` field stores the pasted token, and
   ``_seed_from_env`` mirrors ``COPILOT_GITHUB_TOKEN`` from
   ``~/.hermes/.env`` into the pool. The previous wording implied
   ``auth add copilot`` itself ran the device-code flow, which it does
   not (the device-code flow lives in ``hermes model``).

Two new tests cover the iteration change:
  - ``test_skips_pool_entry_that_fails_to_exchange`` — pool[0] raises,
    pool[1] succeeds, picker uses pool[1].
  - ``test_all_pool_entries_fail_exchange_returns_empty`` — every entry
    raises, return ``""``.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
briandevans 2026-04-28 00:42:37 -07:00 committed by Teknium
parent fdfe40a48b
commit 66a05e44d6
2 changed files with 82 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
hermes_cli/models.py
View File

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

82
tests/hermes_cli/test_copilot_catalog_oauth_fallback.py
View File

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