Skip to content

fix(run_agent): refresh Copilot token and base_url together during credential rotation - #61754

Open
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-61746
Open

fix(run_agent): refresh Copilot token and base_url together during credential rotation#61754
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-61746

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a Copilot credential rotation bug where a valid Enterprise credential could be marked exhausted after a single 403 error, even though it succeeds on a fresh turn.

Root cause: when rotating to a Copilot pool entry, _swap_credential() trusted the entry's persisted base_url (which might still carry the old public default https://api.githubcopilot.com). The subsequent request sent the Enterprise API token to the public endpoint, received HTTP 403, and the credential was marked exhausted. A new agent turn succeeds because normal initialization performs a live token exchange and receives the account-specific Enterprise endpoint.

This change makes Copilot credential rotation perform a live token exchange at rotation time, ensuring the API token and base_url are updated together from a single exchange result before rebuilding the client.

Related Issue

Fixes #61746

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • run_agent.py::_swap_credential(): For provider == "copilot", call get_copilot_api_token() to refresh both the API token and base_url from a live exchange before updating the client.
  • tests/agent/test_credential_pool_routing.py::TestCopilotCredentialRefresh: Add 3 regression tests for the Copilot credential rotation behavior.

How to Test

  1. Run the Copilot-specific tests: pytest tests/agent/test_credential_pool_routing.py::TestCopilotCredentialRefresh -v. Expected result: All 3 tests pass.
  2. Run the full credential pool routing test suite: pytest tests/agent/test_credential_pool_routing.py -v. Expected result: All 13 tests pass.
  3. Manual verification with a Copilot Enterprise credential: Configure two Copilot Enterprise credential pool entries where the first entry has a stale public base_url (e.g., https://api.githubcopilot.com) while the second entry is fresh. Trigger credential rotation (e.g., by sending a malformed request that causes a 403). Observed result: The rotated credential now fetches both the API token and base_url from a live token exchange, so the request goes to the Enterprise endpoint (https://api.enterprise.githubcopilot.com) and succeeds with HTTP 200 instead of failing with HTTP 403 on the stale public endpoint.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/agent/test_credential_pool_routing.py and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.2

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…edential rotation

When rotating to a Copilot credential pool entry, perform a live token
exchange to fetch both the API token and base_url from a single
exchange result. This prevents pairing a valid Enterprise credential
with a stale public endpoint (https://api.githubcopilot.com), which
causes HTTP 403 and incorrectly marks the credential exhausted.

Fixes NousResearch#61746
Add regression tests for NousResearch#61746:
- test_copilot_swap_refreshes_token_and_base_url: verifies that Copilot rotation
  fetches both token and base_url from a live exchange
- test_copilot_swap_falls_back_on_exchange_failure: verifies fallback behavior
  when the exchange fails
- test_non_copilot_swap_does_not_call_exchange: verifies non-Copilot providers
  are unaffected
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint provider/copilot GitHub Copilot (ACP + Chat) area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists labels Jul 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Core fix for #61746. Complementary/competing with #61757 (which builds on this to also cover the already-exchanged-token endpoint path and the gateway/run.py 403 diagnostic). Two overlapping OPEN PRs for the same issue — maintainer picks/merges (they cover distinct sub-paths of the rotation flow).

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting the current credential-rotation site; the stale-entry premise is still present in run_agent.py:4505-4532.

Problems

  • The added get_copilot_api_token(entry.access_token) call receives an already-exchanged API token in normal Copilot pool operation: seeding stores api_token in access_token (agent/credential_pool.py:1931-1949).
  • When that re-exchange fails, get_copilot_api_token() catches the error and returns (raw_token, None) rather than raising (hermes_cli/copilot_auth.py:415-434). The new assignment would therefore clear runtime_base, not fall back to the selected entry as the comment claims.
  • The added failure test mocks an exception from the wrapper, so it does not exercise that real fail-soft tuple contract.

Suggested changes

  • Handle raw and already-exchanged Copilot tokens separately, preserving or deriving the selected endpoint for an exchanged token; preserve the existing runtime endpoint when resolution supplies none.
  • Add regression coverage for the helper returning (existing_api_token, None).

This is an automated hermes-sweeper review.

Comment thread run_agent.py
from hermes_cli.copilot_auth import get_copilot_api_token
runtime_key, runtime_base = get_copilot_api_token(entry.access_token or "")
except Exception:
# Fall back to the existing entry values if exchange fails.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

entry.access_token is normally already the exchanged Copilot API token (agent/credential_pool.py:1931-1949). Re-exchange failure is swallowed by get_copilot_api_token() and returns (same_token, None) (hermes_cli/copilot_auth.py:415-434), so this assignment clears the selected endpoint instead of reaching the except fallback. Preserve/derive the selected endpoint for exchanged tokens.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 11, 2026
@DeamonDev888

Copy link
Copy Markdown

This is a critical fix that #62467's unified resolver handles at the architectural level.

What this PR fixes: Copilot token refresh doesn't update base_url together with the token, causing stale endpoint usage during credential rotation.

How #62467 prevents this class of bug:

The unified resolver agent/auth.py::resolve_provider_credentials() always reads the current entry state (Step 1) before resolving. It doesn't cache resolved credentials across rotation cycles. When Copilot's OAuth token refreshes and entry.runtime_base_url updates, the next call to the resolver picks up the new values automatically.

Additionally, the resolver's source labeling (ResolvedCredential.source) makes it clear whether the URL came from the OAuth refresh, the original pool entry, or the registry default — so if a stale URL leaks through, the diagnostic logs immediately show where it came from.

Copilot is covered under Category C (generic API-key with OAuth awareness) in the unified resolver. If this PR's token-refresh fix merges, the resolver will consume the refreshed base_url + token pair transparently.

Complementary — both fixes are needed for fully correct Copilot Enterprise support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists provider/copilot GitHub Copilot (ACP + Chat) sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Copilot credential rotation reuses stale public endpoint for Enterprise accounts

4 participants