fix(copilot): fall back to credential_pool OAuth access_token for /model picker (#16708)#16868
Closed
briandevans wants to merge 2 commits into
Closed
Conversation
…del picker (NousResearch#16708) Users whose only Copilot credential is the OAuth `access_token` saved by `hermes auth add copilot` (device-code flow) saw the `/model` picker drop back to a stale hardcoded list. Reason: `_resolve_copilot_catalog_api_key` only consulted env vars (`COPILOT_GITHUB_TOKEN` / `GH_TOKEN` / `GITHUB_TOKEN`) and the `gh auth token` CLI fallback, never the credential pool that Hermes's own login flow writes into `auth.json`. With no token, the live catalog fetch silently 401s and the picker hides current models (claude-opus-4.7, claude-sonnet-4.6, gpt-5.5, grok-code-fast-1) — even though `/model <id>` works fine because runtime inference reads the pool through a different code path. Mirror the Codex catalog resolver pattern: env-var first (unchanged), then walk `read_credential_pool("copilot")` for the first entry with a supported `access_token` (`gho_*` / `github_pat_*` / `ghu_*`). Run it through `get_copilot_api_token()` so the catalog request uses the same exchanged token the runtime path uses. Classic PATs (`ghp_*`) are still rejected up-front via `validate_copilot_token` since the Copilot API doesn't accept them. Strictly additive: env still wins, and a missing/locked auth.json (or any exception during pool read) still returns "" so the caller falls through to the curated catalog. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes Copilot /model picker live-catalog discovery when the only available Copilot credential is stored in auth.json’s credential pool, by adding a fallback path to reuse pooled access_token entries (and exchange them via the Copilot token exchanger) after the existing env/gh auth token resolution misses.
Changes:
- Updated
hermes_cli.models._resolve_copilot_catalog_api_key()to fall back from env/gh-resolved credentials toread_credential_pool("copilot")and exchange the first acceptable pooled token viaget_copilot_api_token(). - Added a focused regression test suite covering env-wins, pool fallback behavior, classic PAT rejection, invalid entry skipping, and failure modes (env raises, pool read raises).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
hermes_cli/models.py |
Adds credential-pool fallback for Copilot catalog token resolution and exchanges pooled tokens for Copilot API use. |
tests/hermes_cli/test_copilot_catalog_oauth_fallback.py |
New regression tests for Copilot catalog token resolution fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1704
to
+1709
| 2. ``read_credential_pool("copilot")`` — OAuth ``access_token`` saved | ||
| in ``auth.json`` by ``hermes auth add copilot`` (device-code flow). | ||
|
|
||
| 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. |
There was a problem hiding this comment.
The docstring says the OAuth access_token is saved in auth.json by hermes auth add copilot via a device-code flow, but hermes auth add for copilot currently just adds an API-key credential (it doesn’t run the device-code flow). Consider rewording to avoid implying auth add copilot performs OAuth login; it can store a gho_* token, but it doesn’t mint it.
Suggested change
| 2. ``read_credential_pool("copilot")`` — OAuth ``access_token`` saved | |
| in ``auth.json`` by ``hermes auth add copilot`` (device-code flow). | |
| 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. | |
| 2. ``read_credential_pool("copilot")`` — a previously stored GitHub | |
| OAuth ``access_token`` from ``auth.json`` (for example a ``gho_*`` | |
| token), if one is present. | |
| Without (2), users whose only Copilot credential is a stored OAuth token | |
| can see the ``/model`` picker fall back to a stale hardcoded list | |
| because the live catalog fetch silently 401s. |
Comment on lines
+1731
to
+1737
| raw = str(entry.get("access_token") or "").strip() | ||
| if not raw: | ||
| continue | ||
| valid, _ = validate_copilot_token(raw) | ||
| if not valid: | ||
| continue | ||
| return get_copilot_api_token(raw) |
There was a problem hiding this comment.
The pool-walk treats any non-empty token as “valid” except ghp_* because validate_copilot_token() currently only rejects classic PATs/empty strings. That means a malformed/unsupported token in credential_pool.copilot could be selected first and prevent reaching a later valid gho_*/github_pat_*/ghu_* entry. Consider tightening the acceptance check for pool entries (e.g., enforce supported prefixes or strengthen validate_copilot_token) and add a regression test where the first pool token is unsupported but a later one is valid.
Comment on lines
+3
to
+6
| 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 |
There was a problem hiding this comment.
This module docstring states hermes auth add copilot “itself produces” the OAuth token via device-code flow, but that command path currently adds an API-key credential and doesn’t run OAuth login. Consider rephrasing to “a gho_* token (typically obtained via device-code login) stored in auth.json” to avoid confusing readers about where the token is minted.
Suggested change
| 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 | |
| Regression for #16708: when the user's only Copilot credential is a | |
| ``gho_*`` OAuth ``access_token`` saved in ``auth.json`` (typically | |
| obtained via device-code login), the picker was silently dropping back | |
| to a stale hardcoded list because |
…l catalog tokens Address Copilot review on NousResearch#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>
Contributor
Author
|
@copilot All three findings addressed in commit Finding 1 + 3 (docstring inaccuracy — Finding 2 (a malformed pool[0] could wedge a later valid pool[1]): Real concern. Two new tests cover the change:
Full focused suite is now 9 tests; adjacent suite ( |
Contributor
Author
|
CI audit — all 24 Failure set is the same one observable on other open PRs against |
teknium1
pushed a commit
that referenced
this pull request
Apr 28, 2026
…l catalog tokens 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>
teknium1
pushed a commit
that referenced
this pull request
Apr 28, 2026
…l catalog tokens 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>
Contributor
|
Merged via PR #16901. Your commits were cherry-picked onto current main with your authorship preserved in git log — both commits show as yours. Thanks for the fix (and the follow-up catching the skip-and-try-next edge case)! |
5 tasks
cluricaun28
referenced
this pull request
in cluricaun28/Logos
Apr 28, 2026
…l catalog tokens
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>
ulasbilgen
pushed a commit
to ulasbilgen/hermes-adhd-agent
that referenced
this pull request
May 1, 2026
…l catalog tokens Address Copilot review on NousResearch#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>
donald131
pushed a commit
to donald131/hermes-agent
that referenced
this pull request
May 2, 2026
…l catalog tokens Address Copilot review on NousResearch#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>
02356abc
pushed a commit
to 02356abc/hermes-agent
that referenced
this pull request
May 14, 2026
…l catalog tokens Address Copilot review on NousResearch#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>
dannyJ848
pushed a commit
to dannyJ848/hermes-agent
that referenced
this pull request
May 17, 2026
…l catalog tokens Address Copilot review on NousResearch#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>
gweeteve
pushed a commit
to gweeteve/hermes-agent
that referenced
this pull request
Jun 2, 2026
…l catalog tokens Address Copilot review on NousResearch#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>
Egavasyug
pushed a commit
to Egavasyug/hermes-agent
that referenced
this pull request
Jun 10, 2026
…l catalog tokens Address Copilot review on NousResearch#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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_resolve_copilot_catalog_api_keynow falls back to the OAuthaccess_tokensaved inauth.jsonbyhermes auth add copilot(device-code flow) when env vars andgh auth tokenare unset.""and let the picker fall through to the curated list.hermes_cli/models.py:1791).The bug
Users whose only Copilot credential is the OAuth token Hermes itself stores see the
/modelpicker drop back to a stale hardcoded list.provider_model_ids(\"copilot\")calls_resolve_copilot_catalog_api_key(), which only checksCOPILOT_GITHUB_TOKEN/GH_TOKEN/GITHUB_TOKENandgh auth token. With no token,_fetch_github_modelssilently 401s and the picker hides current models (claude-opus-4.7,claude-sonnet-4.6,gpt-5.5,grok-code-fast-1)./model <id>works fine because runtime inference readsauth.jsonthrough a different code path — only the catalog fetch is broken.The fix
hermes_cli/models.py::_resolve_copilot_catalog_api_key:resolve_api_key_provider_credentials(\"copilot\")first (existing behaviour).read_credential_pool(\"copilot\")for the first entry with a supported access token (gho_*/github_pat_*/ghu_*). Reject classic PATs (ghp_*) up-front viavalidate_copilot_tokensince the Copilot API doesn't accept them.get_copilot_api_token()so catalog requests use the same exchanged token runtime inference uses.Strictly additive: env still wins; non-dict entries, empty
access_tokens, or a raisingread_credential_poolall return\"\"so the caller falls through to the curated catalog (existing behaviour).Test plan
tests/hermes_cli/test_copilot_catalog_oauth_fallback.py(7 tests) covering: env-wins-over-pool, pool-fallback success, env-raises-still-falls-back, classic-PAT rejected, non-dict / empty / first-valid-wins iteration, no-credentials returns\"\", pool-read-raises returns\"\".tests/hermes_cli/test_model_validation.py,test_copilot_in_model_list.py,test_copilot_auth.py,test_copilot_token_exchange.py— all 115 pass.hermes_cli/models.pyto baseline and reran the new suite — 3 of 7 fail (test_falls_back_to_pool_oauth_token,test_falls_back_when_env_resolution_raises,test_skips_invalid_pool_entries), proving the tests exercise the new path. With the fix restored, all 7 pass.Related
/modelpicker should fall back to OAuth access_token from auth.json #16708.hermes_cli/models.py:1791(env first, thenresolve_codex_runtime_credentials()readingauth.json).