fix(copilot): exchange token in env seeding, catalog-aware API mode, ACP guard - #74377
fix(copilot): exchange token in env seeding, catalog-aware API mode, ACP guard#74377khpawan wants to merge 1 commit into
Conversation
…ACP guard
Four related Copilot failures, all reproducible against a live account.
1. Credential pool seeded the raw GitHub token (intermittent HTTP 403)
load_pool("copilot") runs _seed_from_singletons then _seed_from_env
against the same env:COPILOT_GITHUB_TOKEN source key. The first
correctly exchanges the raw ghu_/gho_ token for a short-lived tid=
Copilot token and records the endpoint the exchange advertises
(business/enterprise accounts get a dedicated proxy). The second then
overwrote that entry with the raw token plus the generic
api.githubcopilot.com URL.
GitHub rejects that pairing intermittently: measured ~1 in 3 requests
returning 403 with the raw token, versus 10/10 success with the
exchanged one. auth.json looked healthy, so the failure presented as
random Copilot outages.
_seed_from_env now exchanges for copilot, mirroring the existing
kimi-coding / zai base-URL hooks. The exchange is memoised in
copilot_auth._jwt_cache, so it adds no round-trip. An explicit
COPILOT_API_BASE_URL still wins, and a failed exchange still degrades
to the previous raw-token behaviour.
2. Responses-only non-GPT models routed to /chat/completions (HTTP 400)
copilot_model_api_mode classified models by the ^gpt-N name pattern
only. Copilot also ships responses-only models under other vendor
prefixes: grok-4.5 advertises exactly ["/responses"], and calling
/chat/completions returns 400 "not accessible via the
/chat/completions endpoint".
The docstring already promised a supported_endpoints fallback that was
never implemented; this implements it. The upgrade is conservative --
only when /responses is present and /chat/completions is absent -- so
dual-endpoint models (Claude, gpt-5-mini) keep their current routing.
A 1-hour catalog cache mirrors the existing _copilot_context_cache
convention so the check doesn't add a /models call per client build.
3. resolve_provider_client bypassed that decision
It called the name-pattern helper directly, so fix 2 never applied to
the main client-construction path. It now uses copilot_model_api_mode.
The lookup is resolved before the per-request headers are built so the
catalog request cannot interleave with header construction.
4. ACP providers upgraded into a Responses call they cannot serve
CopilotACPClient speaks chat-shaped JSON-RPC over a spawned subprocess
and exposes only .chat -- it has no .responses attribute. A gpt-5.x
model reached over copilot-acp matched the "GPT-5 -> Responses API"
rule and died with:
AttributeError: 'CopilotACPClient' object has no attribute 'responses'
agent_init.py guarded the primary path by provider/base_url, but
_try_activate_fallback in chat_completion_helpers recomputes the mode
from scratch, so a Copilot ACP entry configured as a fallback still
crashed (and its explicit api_mode: chat_completions was ignored).
The rule now lives in the shared
AIAgent._provider_model_requires_responses_api helper, which covers
both call sites, plus an acp:// / acp+tcp:// branch in the fallback
path.
Testing
15 new behavior-contract tests across three files, each verified to
fail without its corresponding fix. Full tests/agent and
tests/hermes_cli sweep: 17202 passed.
Verified end-to-end against a live enterprise Copilot account:
grok-4.5 and gpt-5.6-sol route through CodexAuxiliaryClient,
claude-sonnet-5 and gpt-5-mini stay on the plain client, and
copilot-acp/gpt-5.6-sol completes over CopilotACPClient.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the Copilot credential and transport failures. The current-main premises are present, but two routing details need revision before this is safe to salvage.
Problems
agent/chat_completion_helpers.py:1856-1863resolves fallback mode throughAIAgent._provider_model_requires_responses_api(). Currentrun_agent.py:1532-1540uses only the GPT-name Copilot predicate. This PR adds the ACP exclusion, but a regular non-GPT Copilot fallback that advertises only/responsesstill becomeschat_completions.- The proposed
_cached_github_model_catalog(api_key=...)is one global cache, althoughfetch_github_model_catalog()returns a catalog “for this account” (hermes_cli/models.py:3355-3387). A catalog must not be reused across credentials without account/credential scoping. - The new ACP regression test calls the helper directly rather than exercising the fallback activation path described in the PR.
Suggested changes
- Route fallback Copilot mode through the same catalog-aware resolver and add a non-GPT
/responses-only fallback test. - Key the catalog cache by a non-secret credential fingerprint, and test sequential distinct credentials.
- Add an ACP fallback activation test asserting
chat_completions.
Automated hermes-sweeper review.
|
|
||
| def test_copilot_acp_rejects_every_gpt5_variant(self): | ||
| for model in ("gpt-5", "gpt-5-mini", "gpt-5.6-sol", "gpt-6-future"): | ||
| assert requires_responses(model, provider="copilot-acp") is False, model |
There was a problem hiding this comment.
This checks the helper only. Please add a try_activate_fallback() regression test: the reported failure is the fallback path recomputing API mode after resolve_provider_client() has returned the ACP client.
|
Independent reproduction, live Enterprise Copilot account Hit this in production (Hermes at Same account, same moment:
Easy to misdiagnose: the pool persists only Mechanism, Applying only item 1 restored service:
The review findings above all concern items 2–3. Item 1 is independently reproducible and sufficient on its own — happy to open a narrow credential-seeding-only PR if useful. |
What does this PR do?
Fixes four related GitHub Copilot failures, all reproduced and verified against a live enterprise Copilot account. Three of them are independent of any model choice — they made Copilot look intermittently or completely broken.
The headline one is a credential-seeding order bug that caused intermittent HTTP 403s on every Copilot request.
1.
_seed_from_envoverwrote the exchanged token with the raw one → intermittent 403load_pool("copilot")runs two seeders in sequence against the sameenv:COPILOT_GITHUB_TOKENsource key:_seed_from_singletonscorrectly exchanges the rawghu_/gho_GitHub token for a short-livedtid=…Copilot token, and records the endpoint the exchange advertises (business/enterprise accounts get a dedicated proxy, e.g.api.enterprise.githubcopilot.com)._seed_from_envthen upserts the same key — and clobbered that with the raw GitHub token plus the genericapi.githubcopilot.com.GitHub rejects that pairing intermittently. Measured on a live enterprise account:
tid=+ account endpointghu_+api.githubcopilot.comBecause the persisted entry stores only a fingerprint and re-hydrates from env,
auth.jsonlooked healthy while requests failed at random._seed_from_envnow exchanges forcopilot, mirroring the existingkimi-coding/zaibase-URL hooks in the same loop. The exchange is memoised incopilot_auth._jwt_cache, so it adds no round-trip. An explicitCOPILOT_API_BASE_URLstill wins, and a failed exchange still degrades to the previous raw-token behaviour.2. Responses-only non-GPT models routed to
/chat/completions→ HTTP 400copilot_model_api_modeclassified models by the^gpt-Nname pattern only. Copilot also ships responses-only models under other vendor prefixes —grok-4.5advertises exactly["/responses"]— so they gotchat_completionsand returned400 "not accessible via the /chat/completions endpoint".The docstring already promised a
supported_endpointsfallback that was never implemented; this implements it, conservatively: upgrade only when/responsesis present and/chat/completionsis absent, so dual-endpoint models (Claude,gpt-5-mini) keep their current routing. A 1-hour catalog cache mirrors the existing_copilot_context_cacheconvention.3.
resolve_provider_clientbypassed that decisionIt called the name-pattern helper directly, so fix 2 never reached the main client-construction path. It now uses
copilot_model_api_mode, resolved before the per-request headers are built so the catalog request cannot interleave with header construction.4. ACP providers upgraded into a Responses call they cannot serve
CopilotACPClientspeaks chat-shaped JSON-RPC over a spawned subprocess and exposes only.chat— there is no.responsesattribute. Agpt-5.xmodel reached overcopilot-acpmatched the "GPT-5 → Responses API" rule and died with:agent_init.pyguards the primary path by provider/base_url, but_try_activate_fallbackinchat_completion_helpers.pyrecomputes the mode from scratch — so a Copilot ACP entry configured as a fallback still crashed, and its explicitapi_mode: chat_completionswas ignored. Per AGENTS.md ("fix the whole bug class including sibling call paths"), the rule now lives in the sharedAIAgent._provider_model_requires_responses_apihelper, which covers both call sites, plus anacp:///acp+tcp://branch in the fallback path.Relationship to existing PRs
I searched open PRs before filing. Flagging the overlap explicitly so maintainers can dedupe:
fix(models): route Responses-only Copilot models to the Responses API) proposes the samesupported_endpointsfallback as my fix 2, formai-code-1-flash-picker. Open since 2026-07-05. If fix(models): route Responses-only Copilot models to the Responses API #58835 lands first I'll happily drop my fix 2 and rebase — fix 3 then simply consumes their version. Fixes 1, 3 and 4 are independent of it.copilot_model_api_modefor Claude/Grok catalog routing; overlapping area, different scope (gatewaytarget_modelthreading).run_agent/gateway. This PR fixes the earlier root cause: the pool entry is seeded wrong in the first place, in_seed_from_env. They are complementary, not duplicates.Related Issue
Fixes #
Type of Change
Changes Made
agent/credential_pool.py—_seed_from_envexchanges the raw GitHub token forcopilotand uses the account endpoint from the exchange.hermes_cli/models.py— implement the documentedsupported_endpointsfallback incopilot_model_api_mode; add_cached_github_model_catalog(1h TTL).agent/auxiliary_client.py—resolve_provider_clientusescopilot_model_api_mode; decision resolved before header construction.run_agent.py—_provider_model_requires_responses_apireturnsFalseforcopilot-acp.agent/chat_completion_helpers.py— fallback activation pinschat_completionsforacp:///acp+tcp://.How to Test
Bug 1 (403s) — before/after, no mocks:
Then issue ~10
/modelsrequests with the pool credential: before the fix roughly a third return 403; after, 10/10 return 200.auth.jsonself-repairs on the nextload_pool().Bugs 2–4 — live routing:
Expected after the fix (all four complete a real turn):
Tests:
Each new test was verified to fail without its corresponding fix (reverting each hunk individually turns the relevant assertions red), so none are tautological.
Checklist
Code
tests/agent+tests/hermes_cli: 17202 passed. (The 3 unrelated failures observed under 40-worker load —test_kanban_db_init,test_relay_shared_metrics,test_early_recovery— reproduce on unpatchedmainin the same environment or pass when re-run serially.)Documentation & Housekeeping
copilot_model_api_modedocstring already described thesupported_endpointsfallback this PR implementsScreenshots / Logs
Original failures from
~/.hermes/logs/errors.log:All three are silent after this change.