Skip to content

fix(copilot): normalize credential pool priorities by token source - #32311

Open
dandomin wants to merge 1 commit into
NousResearch:mainfrom
dandomin:fix/copilot-pool-token-precedence
Open

fix(copilot): normalize credential pool priorities by token source#32311
dandomin wants to merge 1 commit into
NousResearch:mainfrom
dandomin:fix/copilot-pool-token-precedence

Conversation

@dandomin

Copy link
Copy Markdown

Summary

_normalize_pool_priorities only handled anthropic. For Copilot, that meant pool ordering was whatever insertion order produced, so an older gh_cli token from a different GitHub account could outrank a fresh env:COPILOT_GITHUB_TOKEN obtained by hermes model — leading to confusing auth state where the picker showed one identity but the runtime used another.

Fix

Extend _normalize_pool_priorities to also normalize copilot entries, using the same source precedence that hermes_cli/copilot_auth.py:resolve_copilot_token already enforces at token resolution time:

env:COPILOT_GITHUB_TOKEN  > env:GH_TOKEN > env:GITHUB_TOKEN > gh_cli

Manual entries continue to outrank auto-seeded singletons (unchanged behavior).

Test

test_copilot_pool_priority_normalization_honors_token_source_precedence exercises the three-entry case (manual + COPILOT_GITHUB_TOKEN + gh_cli) and asserts the resulting priorities. Passes locally:

tests/agent/test_credential_pool.py .   [100%]
1 passed, 67 deselected in 0.17s

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists 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 labels May 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to #17622 — addresses the token precedence half of the same problem. #17622 bundles both token precedence and api_mode derivation; this PR isolates the credential pool normalization fix for copilot entries. Also related to #23568 (round_robin normalization skip).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the Copilot source-ordering gap. Current main still has the premise: agent/credential_pool.py:1784-1786 returns early for every provider other than Anthropic, although load_pool() invokes normalization after seeding at agent/credential_pool.py:2410-2422; Copilot's resolver already documents the requested source order at hermes_cli/copilot_auth.py:75-101.

Problems

  • The new branch would normalize on every reload. That conflicts with round_robin, which persists a rotation by moving the selected entry to the end at agent/credential_pool.py:1547-1552; the next load_pool() would restore static source ordering.
  • The added unit test exercises the helper only. It does not cover the persisted stale gh_cli row plus re-seeding path at agent/credential_pool.py:1970-1999, or verify actual pool selection.

Suggested changes

  • Preserve round-robin rotation when applying source-precedence normalization.
  • Add a load_pool("copilot") regression with a persisted gh_cli entry and a resolver-returned COPILOT_GITHUB_TOKEN, asserting the selected entry follows the documented precedence.

Automated hermes-sweeper review.

@teknium1 teknium1 added 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 sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
_normalize_pool_priorities only handled the 'anthropic' provider, so Copilot
pool entries kept their insertion-order priorities. An older gh_cli token from
a different GitHub account could outrank a freshly seeded
env:COPILOT_GITHUB_TOKEN, producing confusing auth state.

Extend the helper to also normalize 'copilot' entries using the same source
precedence that hermes_cli/copilot_auth.py:resolve_copilot_token already
enforces at token resolution time:

  env:COPILOT_GITHUB_TOKEN > env:GH_TOKEN > env:GITHUB_TOKEN > gh_cli

Manual entries continue to outrank auto-seeded singletons (unchanged).

Skip normalization entirely under the round_robin strategy. That strategy
encodes its rotation in the persisted priority order (select() moves the used
entry to the end and renumbers), so re-applying static source precedence on
every load_pool() would reset the rotation and pin the pool to a single
credential.

Tests:
  - test_copilot_pool_priority_normalization_honors_token_source_precedence
    covers the helper's ordering directly.
  - test_load_pool_copilot_env_token_outranks_persisted_gh_cli exercises the
    real load_pool("copilot") path with a persisted stale gh_cli row and a
    resolver-returned COPILOT_GITHUB_TOKEN, asserting actual pool selection.
  - test_load_pool_copilot_round_robin_rotation_is_preserved selects, reloads
    via load_pool(), and asserts the rotation advanced — it fails if the
    round_robin guard is removed.
@dandomin
dandomin force-pushed the fix/copilot-pool-token-precedence branch from 8f3b109 to e942eca Compare August 1, 2026 22:59
@dandomin

dandomin commented Aug 1, 2026

Copy link
Copy Markdown
Author

Thanks for the review — rebased onto current main and addressed both points.

1. round_robin conflict — fixed. _normalize_pool_priorities now returns early when get_pool_strategy(provider) == STRATEGY_ROUND_ROBIN. That strategy encodes its rotation in the persisted priority order (select() moves the used entry to the end and renumbers), so re-applying static source precedence on every load_pool() would reset the rotation and pin the pool to one credential.

2. Helper-only test — added two end-to-end regressions against the real load_pool("copilot") path:

  • test_load_pool_copilot_env_token_outranks_persisted_gh_cli — persisted stale gh_cli row + resolver-returned COPILOT_GITHUB_TOKEN, asserting actual pool selection follows resolve_copilot_token precedence, not persisted insertion order.
  • test_load_pool_copilot_round_robin_rotation_is_preserved — selects, reloads via load_pool(), and asserts the rotation advanced.

Worth flagging: my first version of that rotation test rotated twice within a single in-memory pool and passed even with the guard removed — the regression only surfaces across a reload, which is where normalization re-runs. The test now reloads between selections and does fail without the guard.

Verification: pytest tests/agent/test_credential_pool.py → 57 passed.

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-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

3 participants