fix(auxiliary_client): treat Copilot 400 model_not_supported as a refreshable stale credential - #51313
Conversation
…tale-credential A long-lived process (Council daemon, multi-hour agent) caches a Copilot+Claude client whose bearer token's entitlement later rotates. Copilot then returns '400 model_not_supported' instead of a clean 401, and the existing auth-refresh path (gated on _is_auth_error, which only matched 401) never fired — so the cached client looped on the doomed request indefinitely. Two gaps fixed: - _is_stale_copilot_credential_error(): classify 400 + model_not_supported as a refreshable stale-credential signal (scoped to copilot at the call site, so a genuinely wrong model name on another provider never triggers a refresh loop). - _refresh_provider_credentials() gained a 'copilot' branch (re-resolves the freshest token via hermes_cli.copilot_auth.resolve_copilot_token + evicts the cached client); it previously handled codex/nous/anthropic/xai-oauth but not copilot, even though the main agent has _try_refresh_copilot_client_credentials. The auth-refresh gate now fires on _is_auth_error OR (provider==copilot AND stale-credential-400), single-shot retry, no loop risk. Tests: tests/agent/test_stale_copilot_credential.py (7 cases). Existing auxiliary_client suite (225) unaffected.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Handles Copilot's non-standard error: when a long-lived process holds a cached client whose bearer token entitlement rotates, Copilot returns 400 model_not_supported instead of 401. Adds _is_stale_copilot_credential_error to detect this pattern, scoped narrowly to copilot provider only. Single-shot retry (no loop risk). Includes tests.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the Copilot stale-credential path. The premise remains relevant: current main only sends _is_auth_error() failures into the credential-refresh paths (agent/auxiliary_client.py:6755-6785, 7303-7332), while model_not_supported is currently handled as a generic capability mismatch/fallback (agent/auxiliary_client.py:3135-3186, 6866-6897).
Problems
- The proposed
_norm_provider = _normalize_aux_provider(resolved_provider)gate excludesauto. Current main intentionally supportsauto → copilotrefresh by resolving the concrete provider from the selected client's base URL (agent/auxiliary_client.py:3573-3595; regression test attests/agent/test_auxiliary_client.py:3509-3539). - The diff changes only synchronous
call_llm; the parallel async refresh path remains atagent/auxiliary_client.py:7303-7332. - Current main already has a stronger Copilot refresh branch from
f69e3aadf(agent/auxiliary_client.py:3504-3518), which invalidates the exchanged JWT cache and forces a new exchange. The PR's older branch should not replace it.
Suggested changes
- Salvage the narrow 400 classifier against the concrete provider returned by
_auth_refresh_provider_for_route(), apply it to sync and async paths, and add real retry-path tests rather than a mirrored gate.
Automated hermes-sweeper review.
| # rotated, Copilot returns ``400 model_not_supported`` instead of a clean | ||
| # 401. Treat that stale-credential 400 as auth-class too, but ONLY for the | ||
| # copilot provider (so a genuinely wrong model name on another provider is | ||
| # never mistaken for a refreshable auth error). The retry below is |
There was a problem hiding this comment.
resolved_provider can remain auto after Copilot is selected. Current main resolves the concrete auth provider from the selected client's base URL; classify this 400 against that route provider so auto-routed Copilot also refreshes.
| # single-shot, so there is no loop risk. | ||
| _norm_provider = _normalize_aux_provider(resolved_provider) | ||
| _is_refreshable_auth = ( | ||
| _is_auth_error(first_err) |
There was a problem hiding this comment.
Please mirror this recovery in async_call_llm; current main has a separate async auth-refresh gate, so this sync-only change leaves asynchronous auxiliary calls on the old behavior.
…oken 401 Copilot degrades in two related ways that both abort a turn as non-retryable and only clear on a gateway restart (a cold process re-runs the token exchange): 1. HTTP 400 model_not_available_for_integrator / model_not_supported — a raw/degraded token routes to the restricted copilot-language-server integrator whose allowlist omits enterprise-only models (e.g. claude-opus-4.8). Because it is a 400 (not 401), the existing 401 refresh path never fired. Prevented (retry-with-backoff exchange + on-disk JWT persistence + header guard at the client chokepoint) and self-healed at runtime (single-shot forced re-exchange + client rebuild + retry before fallback). 2. HTTP 401 'IDE token expired: unauthorized: token expired' — the short-TTL *exchanged* IDE token expires mid-turn. The clean-401 path DID fire and call _try_refresh_copilot_client_credentials(), but that method only re-resolved the stable raw ghu_ token and rebuilt the client — it never evicted the cached exchanged JWT or forced a fresh exchange, so the retry put the SAME expired token back on the wire, 401'd again, and the single-shot guard aborted the turn. Fix: force a fresh IDE-token exchange (evict cached JWT via evict_cached_exchanged_token + re-mint via get_copilot_api_token) before the client rebuild, mirroring the merged auxiliary-path recovery (NousResearch#59837) and the 400 recovery in this same PR. Graceful fallback to the resolved token if the exchange endpoint is unreachable; picks up the enterprise base_url on re-exchange. Brings main-loop clean-401 recovery to parity with the merged auxiliary path (NousResearch#59837), using the newer on-disk-aware evict helper. Companion context: NousResearch#58743 (this PR, expanded), NousResearch#51313, NousResearch#63204 (which assumed the 401 path already recovered — it reached the method but the method was too weak). Tests: exchange retry/persist round-trip, restart-blip disk reuse, stale-cred 400 classifier, 400 recovery, and 3 new 401 cases (fresh exchanged token on the wire; network-blip fallback to resolved token). 58 copilot tests green on current main.
…oken 401 Copilot degrades in two related ways that both abort a turn as non-retryable and only clear on a gateway restart (a cold process re-runs the token exchange): 1. HTTP 400 model_not_available_for_integrator / model_not_supported — a raw/degraded token routes to the restricted copilot-language-server integrator whose allowlist omits enterprise-only models (e.g. claude-opus-4.8). Because it is a 400 (not 401), the existing 401 refresh path never fired. Prevented (retry-with-backoff exchange + on-disk JWT persistence + header guard at the client chokepoint) and self-healed at runtime (single-shot forced re-exchange + client rebuild + retry before fallback). 2. HTTP 401 'IDE token expired: unauthorized: token expired' — the short-TTL *exchanged* IDE token expires mid-turn. The clean-401 path DID fire and call _try_refresh_copilot_client_credentials(), but that method only re-resolved the stable raw ghu_ token and rebuilt the client — it never evicted the cached exchanged JWT or forced a fresh exchange, so the retry put the SAME expired token back on the wire, 401'd again, and the single-shot guard aborted the turn. Fix: force a fresh IDE-token exchange (evict cached JWT via evict_cached_exchanged_token + re-mint via get_copilot_api_token) before the client rebuild, mirroring the merged auxiliary-path recovery (#59837) and the 400 recovery in this same PR. Graceful fallback to the resolved token if the exchange endpoint is unreachable; picks up the enterprise base_url on re-exchange. Brings main-loop clean-401 recovery to parity with the merged auxiliary path (#59837), using the newer on-disk-aware evict helper. Companion context: #58743 (this PR, expanded), #51313, #63204 (which assumed the 401 path already recovered — it reached the method but the method was too weak). Tests: exchange retry/persist round-trip, restart-blip disk reuse, stale-cred 400 classifier, 400 recovery, and 3 new 401 cases (fresh exchanged token on the wire; network-blip fallback to resolved token). 58 copilot tests green on current main.
…oken 401 Copilot degrades in two related ways that both abort a turn as non-retryable and only clear on a gateway restart (a cold process re-runs the token exchange): 1. HTTP 400 model_not_available_for_integrator / model_not_supported — a raw/degraded token routes to the restricted copilot-language-server integrator whose allowlist omits enterprise-only models (e.g. claude-opus-4.8). Because it is a 400 (not 401), the existing 401 refresh path never fired. Prevented (retry-with-backoff exchange + on-disk JWT persistence + header guard at the client chokepoint) and self-healed at runtime (single-shot forced re-exchange + client rebuild + retry before fallback). 2. HTTP 401 'IDE token expired: unauthorized: token expired' — the short-TTL *exchanged* IDE token expires mid-turn. The clean-401 path DID fire and call _try_refresh_copilot_client_credentials(), but that method only re-resolved the stable raw ghu_ token and rebuilt the client — it never evicted the cached exchanged JWT or forced a fresh exchange, so the retry put the SAME expired token back on the wire, 401'd again, and the single-shot guard aborted the turn. Fix: force a fresh IDE-token exchange (evict cached JWT via evict_cached_exchanged_token + re-mint via get_copilot_api_token) before the client rebuild, mirroring the merged auxiliary-path recovery (NousResearch#59837) and the 400 recovery in this same PR. Graceful fallback to the resolved token if the exchange endpoint is unreachable; picks up the enterprise base_url on re-exchange. Brings main-loop clean-401 recovery to parity with the merged auxiliary path (NousResearch#59837), using the newer on-disk-aware evict helper. Companion context: NousResearch#58743 (this PR, expanded), NousResearch#51313, NousResearch#63204 (which assumed the 401 path already recovered — it reached the method but the method was too weak). Tests: exchange retry/persist round-trip, restart-blip disk reuse, stale-cred 400 classifier, 400 recovery, and 3 new 401 cases (fresh exchanged token on the wire; network-blip fallback to resolved token). 58 copilot tests green on current main.
|
Closing as implemented on |
What this fixes
A long-lived process (for example a daemon that holds a cached client, or any multi-hour agent) builds a Copilot+Claude client with a bearer token captured at startup. When GitHub later rotates that account's Copilot entitlement, the cached token's request starts coming back as
400 model_not_supportedeven though the model name is perfectly valid and the same model succeeds on a freshly resolved token.The existing auth-refresh path is gated on
_is_auth_error, which only matches401. So this stale-credential400slipped straight past the refresh+evict+retry logic, and the cached client looped on a doomed request indefinitely.The fix
Two narrow additions to
auxiliary_client.py:_is_stale_copilot_credential_error()classifies a400whose body carries themodel_not_supportedmarker as a refreshable stale-credential signal. It's deliberately narrow (status must be 400 and the body must carry the marker) so a genuinely wrong model name isn't mistaken for an auth problem._refresh_provider_credentials()gains acopilotbranch that re-resolves the freshest token viahermes_cli.copilot_auth.resolve_copilot_tokenand evicts the cached client. It previously handled codex/nous/anthropic/xai-oauth but not copilot.The refresh gate in
call_llmnow fires on_is_auth_error(...)or (provider == copilotand the stale-credential 400). The copilot scoping means a wrong-model 400 on any other provider can never trigger a refresh loop, and the retry is single-shot, so there's no loop risk.Tests
tests/agent/test_stale_copilot_credential.py(7 cases): the copilot stale-credential 400 is detected; a clean 401 is not misclassified as one; an unrelated 400 (e.g. a bad temperature) is not; the refresh gate fires for the copilot 400 and still fires for a real 401; a wrong-model 400 on a non-copilot provider does not trigger refresh; theautoprovider is excluded. The existingauxiliary_clientsuite is unaffected.