Skip to content

fix(aux): keep callable key_cmd credentials intact in all custom-provider resolution branches - #107344

Closed
SiaoZeng wants to merge 2 commits into
NousResearch:mainfrom
SiaoZeng:fix/aux-callable-key-cmd
Closed

SiaoZeng wants to merge 2 commits into
NousResearch:mainfrom
SiaoZeng:fix/aux-callable-key-cmd

Conversation

@SiaoZeng

Copy link
Copy Markdown

What does this PR do?

Fixes the key_cmd crash class in agent/auxiliary_client.py (issue #88667) at all four call sites where a custom-provider credential is assumed to be a string:

The fix passes callables through uncalled and normalises only strings, mirroring the guard the main runtime already uses when it builds its credential dict (api_key.strip() if isinstance(api_key, str) else api_key if callable(api_key) else ""). The OpenAI SDK accepts a callable api_key per-request, and the Anthropic bearer-hook path routes callables to Authorization: Bearer (pinned by TestCallableKeyGetsBearerAuth), so nothing downstream needs to change.

Related Issue

Fixes #88667

Companion to the open PRs on the same issue: #88668 (bare custom only), #102244 (derived clients), #105595 (api_key branch only). Whichever lands first, the remaining branches still crash — this PR covers the full class in one pass; overlap is additive and mergeable.

Type of Change

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

Changes Made

  • agent/auxiliary_client.py — four callable()-guard passthroughs in the custom-provider resolution branches (no behaviour change for string credentials; blank-string handling preserved, no-key-required fallback unchanged).
  • tests/agent/test_command_token_source.py — new TestExplicitCallableSurvivesCustomResolution class: three invariant tests asserting an explicit CommandTokenSource reaches _create_openai_client unchanged (not stripped, not stringified) on the bare-custom, named-custom, and main-runtime branches. RED on base per branch, all three green with the fix. Existing TestAuxiliaryResolverHonoursKeyCmd._resolve gained an optional explicit_api_key pass-through (no existing assertions touched).

How to Test

  1. Configure any custom provider with a key_cmd (e.g. key_cmd: printf minted-token), run an aux task (vision/title generation) → before: AttributeError: 'CommandTokenSource' object has no attribute 'strip' in errors.log; after: aux call succeeds with a per-request bearer.
  2. scripts/run_tests.sh tests/agent/test_command_token_source.py tests/agent/test_auxiliary_client.py → 236/236 pass.
  3. New tests are RED on base (git stash the fix, run the file): one failure per covered branch.

Checklist

Code

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 (pure Python branch logic)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

… resolution

resolve_provider_client's custom branches assumed the api_key is a str:
explicit_api_key.strip() raised AttributeError on a CommandTokenSource
(key_cmd), and str(main_runtime api_key) would send the object repr as
the bearer. vision_analyze and title generation are the observable
victims on any key_cmd custom provider; every aux call routed through
_resolve_custom_branch crashed identically.

Pass callables through uncalled (the OpenAI SDK and the Anthropic
bearer-hook path both accept them; the main runtime already builds its
credential dict with this guard). Covers bare custom, named custom,
api_key-branch overrides, and the main_runtime reuse path.

Invariant tests pin that an explicit callable reaches the client object
unchanged (not stripped, not stringified); RED on base for all three
branches. Refs NousResearch#88667; NousResearch#105595 covers the api_key-branch line alone —
this also covers the named-custom and main_runtime sites.
@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 P2 Medium — degraded but workaround exists labels Sep 10, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Related: competing fixes for #88667: #88668 (earliest, _resolve_custom_branch), #105595 (_resolve_api_key_branch), #102244 (derived clients). This PR claims to cover the _resolve_named_custom_branch site the others miss; reviewers may want to consolidate.

@SiaoZeng

Copy link
Copy Markdown
Author

Thanks for the triage note — checked against all three diffs, and the coverage picture is:

What none of them cover is _resolve_named_custom_branch: every aux call (vision, title generation, compression) against a named key_cmd provider still raises AttributeError: 'CommandTokenSource' object has no attribute 'strip' with any one of those merged alone. This PR also un-stringifies str(main_runtime.get("api_key")) at the main-runtime reuse site — the silent-401 class from #104460.

Since all four branches raise the same crash class, whichever subset PR lands first leaves the remaining ones crashing. Suggesting consolidation into this PR (it has per-branch RED tests for all four sites) — but happy to defer to whichever shape maintainers prefer: I can rebase onto any of these landing first, and would gladly fold #102244's async-credential helpers in here if the maintainers want the async layer in the same pass.

…t wrap

The OpenAI SDK stores a callable api_key as _api_key_provider and blanks
client.api_key until the first request refreshes it. _to_async_client
rebuilt AsyncOpenAI from that empty attribute, so every async auxiliary
call (vision_analyze) against a key_cmd provider sent no Authorization
header and 401d. Forward the provider as an awaitable bridge (blocking
mint via asyncio.to_thread) so the async SDK mints per request like sync.
@SiaoZeng

Copy link
Copy Markdown
Author

Independent confirmation of this fix's premises, reproduced live on main @ 04dd80a with the pinned openai==2.24.0 (no network):

  • OpenAI(api_key=<callable>) blanks .api_key to "" and stores _api_key_provider (openai/_client.py:142-147). agent/auxiliary_client.py:4329 (_to_async_client) reads only sync_client.api_key, so the AsyncOpenAI it builds carries an empty key — auth_headers == {}, i.e. async aux calls (vision, async auxiliary) against a key_cmd custom provider go out without an Authorization header. This is reachable in production via the named-custom branch (_named_custom_api_key resolves key_cmd to a callable → _create_openai_client → _route_client(async_mode=True) → _to_async_client).
  • agent/auxiliary_client.py:4667: str(main_runtime.get("api_key") or "") on a callable yields the object repr as a truthy bearer — verified: client.api_key == '<agent.command_token_source.CommandTokenSource object at 0x...>'. The branch returns before the try_fn fallback loop, so it 401s with no fallback. Note the fallback loop at 4689-4697 already handles callables correctly, so the contract is known — only these two sites leak it.
  • Test gap: no test in tests/ drives _to_async_client or the main_runtime reuse branch with a callable key (all key_cmd coverage is sync); one "callable survives async wrap" test, analogous to the existing Azure-Foundry sync test, would have caught both.

Suggest these three sites + a test be in this PR's scope; happy to see it land as-is.

@gweber

gweber commented Sep 13, 2026

Copy link
Copy Markdown

Heads-up so work isn't duplicated: #109851 fixes the same _to_async_client credential drop by moving all token-provider handling (key_cmd, Entra ID) into one helper module, agent/api_credential.py, and pointing the sibling call sites at it. Your four resolve_provider_client branches are in there too, credited in the description, with regression tests. Happy to fold anything from this PR into that one, or to close mine if maintainers prefer this approach.

@ehz0ah

ehz0ah commented Sep 17, 2026

Copy link
Copy Markdown

Blocking: named custom providers still lose their configured extra_headers. The exact-head code builds the named custom OpenAI client with only query parameters and global user headers in _named_custom_openai_wire_client. It never applies custom_entry.extra_headers. _to_async_client then rebuilds the client and also does not preserve those configured headers. I verified a named provider with key_cmd and a required route header: the callable bearer survives, but the required header is absent from both the sync and async client requests. Providers that require proxy authentication or routing headers still reject these auxiliary calls. Please normalize and apply the named entry extra_headers when building the sync client, preserve the effective configured headers in the async conversion, and add sync plus async regression coverage. Also remove the extra blank line at the end of tests/agent/test_command_token_source.py so git diff --check passes.

@alt-glitch alt-glitch added tool/vision Vision analysis and image generation area/config Config system, migrations, profiles area/compression Context compression and continuation sessions P1 High — major feature broken, no workaround sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state and removed P2 Medium — degraded but workaround exists labels Sep 17, 2026
@kshitijk4poor

Copy link
Copy Markdown

Landed as #114333 with Co-authored-by: SiaoZeng and a Credit: trailer (thank you, @SiaoZeng). Why not a cherry-pick: your _to_async_client rewrite is superseded by #113969 (merged today), the bare-custom hunk conflicts with the local-server-alias arm that landed since, and a fifth string-only site (the alias arm itself) needed the same guard — so the five sites now share one _normalize_api_key helper, with one parametrized test over the five shapes. @ehz0ah's extra_headers block is moot on current main after #113969. #114333 is armed to merge on green.

kshitijk4poor added a commit that referenced this pull request Sep 17, 2026
kshitijk4poor added a commit that referenced this pull request Sep 17, 2026
…r resolution branch (#88667)

A key_cmd (CommandTokenSource) or Entra credential is a callable, not a
string. _route_via_main_provider and the main_runtime passthrough hand it
to resolve_provider_client as explicit_api_key / main_runtime["api_key"],
where five branch sites still assumed a string: four call .strip() on it
(AttributeError: 'CommandTokenSource' object has no attribute 'strip')
and the main_runtime reuse arm wraps it in str(), so the object's repr
becomes the bearer token and the request fails with a silent 401.

One module-level helper, _explicit_key(), replaces the five inline
expressions so the callable/str/None contract lives in one place instead
of five ternaries that drift independently: a non-str callable passes
through uncalled (the client calls it per request), strings are stripped,
anything else collapses to "" so the existing `or <fallback>` chains keep
working. The async seam was already fixed the same way in #113969.

Co-authored-by: SiaoZeng <188540289+SiaoZeng@users.noreply.github.com>
Credit: #107344 @SiaoZeng (four of the five sites), #88668 @LordMelkor (first submitter, bare-custom branch), #105595 @haydster7 (api_key branch)
QuixThe2nd pushed a commit to QuixThe2nd/hermes-ide that referenced this pull request Sep 18, 2026
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 area/compression Context compression and continuation sessions area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P1 High — major feature broken, no workaround sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/vision Vision analysis and image generation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: callable api_key from key_cmd crashes custom-provider resolution with AttributeError

5 participants