Skip to content

fix(agent): pass api_key_hint to mark_exhausted_and_rotate in credential pool recovery - #43755

Closed
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/credential-pool-api-key-hint-v2
Closed

fix(agent): pass api_key_hint to mark_exhausted_and_rotate in credential pool recovery#43755
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/credential-pool-api-key-hint-v2

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Passes api_key_hint to mark_exhausted_and_rotate() in recover_with_credential_pool(), ensuring the correct pool entry is marked as exhausted during credential rotation. Without this hint, the pool falls back to current() or _select_unlocked(), which can return the wrong (healthy) entry after a prior rotation.

Related Issue

Fixes #43747

Type of Change

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

Changes Made

  • agent/agent_runtime_helpers.py: Extract current API key from agent.api_key (with fallback to pool.current().runtime_api_key) before any rotation; pass it as api_key_hint to all 4 mark_exhausted_and_rotate call sites (billing, rate-limit pre-exhausted, rate-limit retry, auth refresh failed)
  • tests/agent/test_credential_pool_routing.py: Update helper to set agent.api_key and agent.provider/pool.provider; update assert_called_once_with to include api_key_hint; add rotate() **kwargs for forward compatibility; add test_api_key_hint_from_pool_current_when_agent_key_missing verifying the fallback path

How to Test

  1. Run pytest tests/agent/test_credential_pool_routing.py -v — all 11 tests pass
  2. Run pytest tests/run_agent/test_codex_xai_oauth_recovery.py -k "recover_with_credential_pool" -v — all 5 tests pass
  3. Inspect the diff: only 2 files changed, +59/-7 lines, no control flow changes beyond the hint extraction and forwarding

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/ -q 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

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

Code Intelligence

  • Analyzed: recover_with_credential_pool() in agent/agent_runtime_helpers.py (4 call sites to mark_exhausted_and_rotate, callers: agent error recovery path)
  • Blast radius: LOW — only affects credential pool error recovery; api_key_hint is already an optional parameter with existing None-handling
  • Related patterns: auxiliary_client.py already passes api_key_hint=hint correctly (lines 2774, 2786); this PR brings agent_runtime_helpers.py to parity

…ial pool recovery

recover_with_credential_pool() called mark_exhausted_and_rotate() without
api_key_hint, causing it to fall back to current() or _select_unlocked().
When a prior rotation left current() as None, _select_unlocked() returned
the NEXT (healthy) entry instead of the one that actually failed — marking
the wrong credential as exhausted (NousResearch#43747).

Extract the current API key from agent.api_key (or pool.current().runtime_api_key
as fallback) and pass it as api_key_hint to all 4 call sites.
@harjothkhara

Copy link
Copy Markdown
Contributor

Nice fix — and the agent.api_key-first source choice is the right call, worth calling out why since it's non-obvious: _swap_credential sets self.api_key = entry.runtime_api_key in both swap branches, but the anthropic_messages branch (run_agent.py:3899) returns before it reaches the self._client_kwargs["api_key"] = ... line that the OpenAI branch hits (run_agent.py:3905). So _client_kwargs["api_key"] can be stale while agent.api_key is always current — preferring agent.api_key keeps this correct across providers, not just openai-codex. The pool.current().runtime_api_key fallback is a sensible backstop.

One gap I'd flag before merge: the tests verify the hint is plumbed, but don't yet prove that it routes through the real pool selection logic. Both test_credential_pool_routing.py (mock rotate(..., **_kwargs)) and test_codex_xai_oauth_recovery.py (mark_exhausted_and_rotate(self, **_kwargs)) discard api_key_hint, so the assertions only confirm the value is passed through. The actual bug in #43747 lives in the selection branch of mark_exhausted_and_rotate (credential_pool.py:1391-1399, next(e for e in self._entries if e.runtime_api_key == api_key_hint)) — and that branch currently has no test exercising it against a real pool.

Suggest adding one regression against a real CredentialPool that reproduces #43747's exact shape: a multi-entry pool where current()/_select_unlocked() would return a healthy entry, but the failed key matches a different entry — then assert the failed entry is marked exhausted and the healthy one is untouched. That's the behavior the issue is actually about, and right now nothing would catch a regression in it.

Minor: for parity with the auxiliary path (auxiliary_client.py passes failed_api_key or None) and chat_completion_helpers.py (which .strip()s its hint), consider normalizing _api_key_hint to strip whitespace / coerce ""None. Low stakes since an unmatched hint just falls through to the existing behavior, but it keeps the three call sites consistent.

@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 area/auth Authentication, OAuth, credential pools provider/openai OpenAI / Codex Responses API P2 Medium — degraded but workaround exists labels Jun 10, 2026

@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 tracing the missing handoff. The current-main premise is valid: recover_with_credential_pool() still calls mark_exhausted_and_rotate() without a hint (agent/agent_runtime_helpers.py:798, :822, :846, :946), while the pool only selects the exact failed key when a hint is supplied (agent/credential_pool.py:1576-1586).

Problems

  • The new keyword will break current strict pool doubles: tests/run_agent/test_run_agent.py:5649, :5670, :5696, :5801, :5825, and :5902 define mark_exhausted_and_rotate(..., error_context=None) without api_key_hint.
  • The added tests use mocks that discard the hint, so they do not prove the reported routing guarantee through the real selector in agent/credential_pool.py:1581-1586.

Suggested changes

  • Update those current test doubles to accept and, where applicable, assert api_key_hint.
  • Add one real multi-entry CredentialPool regression where the failed key differs from the fallback/current healthy entry, then assert only the failed entry is exhausted.

Automated hermes-sweeper review.

@@ -657,7 +669,7 @@ def recover_with_credential_pool(

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.

These calls now pass api_key_hint, but current strict pool doubles in tests/run_agent/test_run_agent.py:5649, :5670, :5696, :5801, :5825, and :5902 do not accept that keyword. Update those fakes (and assert the forwarded hint where useful), otherwise their exercised recovery branches raise TypeError.

@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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
teknium1 added a commit that referenced this pull request Jul 23, 2026
…egression

Follow-up to the #43755 salvage:
- Update the strict _Pool doubles in tests/run_agent/test_run_agent.py to
  accept api_key_hint and assert it carries the agent's failed key.
- Add a real-CredentialPool regression (no mocks) proving the hint routes
  exhaustion to the entry whose key actually failed, not pool.current(),
  plus the no-hint baseline (#43747 wrong-entry marking).
@teknium1

Copy link
Copy Markdown
Contributor

Salvaged and merged in #69553 — your commit was cherry-picked onto current main with authorship preserved (rebase-merge). On top of it we updated the strict pool test doubles flagged in the earlier review and added a real-CredentialPool regression proving the hint routes exhaustion to the failed entry. Thanks for tracing the missing handoff, @liuhao1024!

randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…egression

Follow-up to the NousResearch#43755 salvage:
- Update the strict _Pool doubles in tests/run_agent/test_run_agent.py to
  accept api_key_hint and assert it carries the agent's failed key.
- Add a real-CredentialPool regression (no mocks) proving the hint routes
  exhaustion to the entry whose key actually failed, not pool.current(),
  plus the no-hint baseline (NousResearch#43747 wrong-entry marking).
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/openai OpenAI / Codex Responses API 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.

[Bug]: openai-codex credential pool marks healthy later account as usage_limit_reached; auth reset restores operation

4 participants