Skip to content

fix(aux): resolve key_env/api_key_env in auxiliary task config - #36080

Closed
DatTheMaster wants to merge 1 commit into
NousResearch:mainfrom
DatTheMaster:fix/aux-key-env-resolution
Closed

fix(aux): resolve key_env/api_key_env in auxiliary task config#36080
DatTheMaster wants to merge 1 commit into
NousResearch:mainfrom
DatTheMaster:fix/aux-key-env-resolution

Conversation

@DatTheMaster

Copy link
Copy Markdown

Summary

_resolve_task_provider_model() reads per-task config from auxiliary.<task>.api_key but ignores the key_env and api_key_env fields — the documented way to reference an environment variable by name instead of hardcoding the key value.

Every other code path that resolves provider credentials already handles these fields:

  • resolve_provider_client()providers.<name>.key_env / api_key_env
  • agent_init.py — fallback entries key_env / api_key_env
  • chat_completion_helpers.py — fallback key_env / api_key_env
  • hermes_cli/providers.py — provider registry key_env

This PR adds the same resolution to _resolve_task_provider_model(), so that:

auxiliary:
  title_generation:
    provider: openrouter
    base_url: https://openrouter.ai/api/v1
    key_env: OPENROUTER_API_KEY

actually resolves the key from the environment, instead of silently ignoring it and sending requests with no auth (HTTP 401).

What changed

  • agent/auxiliary_client.py — After reading cfg_api_key from task config, resolve key_env / api_key_env when no explicit api_key is set. 10 lines added.
  • tests/agent/test_auxiliary_client.py — 4 new tests in TestResolveTaskProviderModelKeyEnv.

Testing

Four test cases:

  1. key_env resolves to the env var value ✓
  2. api_key_env alias resolves identically ✓
  3. Explicit api_key takes precedence over key_env
  4. Missing env var → api_key is None (graceful, not the literal var name) ✓

Notes

Previous iterations of this fix were in #28766 (closed as duplicate of #24088) and #24088 (stale branch, never merged). This PR is a clean rebase onto current main — just the 10-line fix + tests, no unrelated changes.

Closes #20139

@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/config Config system, migrations, profiles area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists labels May 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Supersedes #24088 (stale branch, same bug) and #28766 (closed as dupe of #24088, same author). Clean rebase with just the 10-line fix + tests. Related: #16256, #16308 (same _resolve_task_provider_model code path, different facets).

@mxnstrexgl mxnstrexgl left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — automated review passed. No security, quality, or test coverage issues detected.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused credential-resolution fix. Current main still reads only auxiliary.<task>.api_key in _resolve_task_provider_model() (agent/auxiliary_client.py:5895-5901), while the resulting cfg_api_key is the only configured credential propagated through its return paths (agent/auxiliary_client.py:5976-5987). The proposed key_env / api_key_env lookup matches the existing named-custom-provider convention at agent/auxiliary_client.py:4733-4735, and explicit keys are consumed by provider resolution (agent/auxiliary_client.py:4547-4558, agent/auxiliary_client.py:4890-4897).

Automated hermes-sweeper review.

@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 13, 2026
_reads the key_env or api_key_env field (the documented way to reference
an env var by name) when no explicit api_key is set, mirroring the same
pattern used by resolve_provider_client() for provider-level config.

When auxiliary.<task>.key_env: MY_API_KEY_VAR is set, the env var is
resolved at task config level inside _resolve_task_provider_model() —
without this, such references were silently ignored and the API key
resolved to None, causing auth errors downstream.

Fixes NousResearch#20139
@DatTheMaster

Copy link
Copy Markdown
Author

Conflict resolved.. happy to help

@alt-glitch alt-glitch removed 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 labels Jul 14, 2026
@DatTheMaster
DatTheMaster force-pushed the fix/aux-key-env-resolution branch from 0b36551 to 3a8109f Compare July 14, 2026 02:32
@OutThisLife

Copy link
Copy Markdown
Collaborator

Closing as superseded by #67192 (merged, salvage batch item 3 / originally #66655): _resolve_task_provider_model now resolves key_env / api_key_env when api_key isn't set directly, so auxiliary tasks (vision/compression/etc.) stop 401'ing.

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/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants