Skip to content

fix: support env-backed keys for custom providers - #5551

Open
Elizabeth1979 wants to merge 1 commit into
NousResearch:mainfrom
Elizabeth1979:fix/custom-provider-api-key-env
Open

fix: support env-backed keys for custom providers#5551
Elizabeth1979 wants to merge 1 commit into
NousResearch:mainfrom
Elizabeth1979:fix/custom-provider-api-key-env

Conversation

@Elizabeth1979

Copy link
Copy Markdown

Summary

  • seed custom provider credential pools from api_key_env and api_key_env_vars
  • keep existing inline api_key behavior unchanged
  • add regression tests for both env-backed config paths

Why

Custom OpenAI-compatible providers in custom_providers currently seed credentials from inline api_key, but not from env-backed key fields. That causes false "invalid API key" failures even when the configured environment variable is present and direct API calls work.

Test Plan

  • python -m pytest tests/test_credential_pool.py -q -o addopts=''
  • Verified regression tests for api_key_env and api_key_env_vars

@alt-glitch alt-glitch added type/bug Something isn't working area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists labels May 1, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to #4674 (closed, same feature) and #9315 (custom provider API key override bug). This PR adds regression tests for both env-backed config paths.

@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to #4674 and #9315

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing this to custom-pool seeding. The gap is still present on current main: _seed_custom_pool() only reads inline api_key at agent/credential_pool.py:2281-2302, while named-provider runtime resolution checks the pool before falling back to direct key_env resolution at hermes_cli/runtime_provider.py:979-1016.

Problems

  • The patch checks api_key_env but misses canonical key_env. Current config normalization maps api_key_env to key_env (hermes_cli/config.py:4688-4692), and the documented named-custom-provider form uses key_env (website/docs/integrations/providers.md:1183-1194).
  • Direct os.getenv() bypasses the credential pool's dotenv-preferred and scoped lookup (agent/credential_pool.py:2101-2114, 2199-2205).
  • The test target has moved: current coverage is tests/agent/test_credential_pool.py, not tests/test_credential_pool.py.

Suggested changes

  • Seed from canonical key_env, preserving api_key_env only as a compatibility alias, and use the existing dotenv-aware helper.
  • Add regression coverage in tests/agent/test_credential_pool.py for canonical key_env and a profile .env value.

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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026

@GottZ GottZ 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.

This was generated by AI during triage.

Summary

Two PRs are associated with this complex, but only #5551 addresses the reported custom-provider credential-seeding failure: it adds environment-backed key lookup, whereas #5580 implements delegated-child credential leasing and does not touch that root cause.

Related pull requests

  • #5551 related — (+78/-2) — keep open, but revise before merge: the diff extends _seed_custom_pool() to read api_key_env and api_key_env_vars, directly targeting the missing env-backed custom-provider credentials while preserving inline api_key. The contributor keep_open review identifies material gaps in the current patch: it misses canonical key_env, bypasses the existing dotenv-aware lookup with os.getenv(), and places regression coverage in the obsolete test path.
  • #5580 [closed] related — (+309/-6) — unrelated merged reference: the diff adds soft credential leases and child-agent pool binding for concurrent delegation, not environment-backed custom-provider pool seeding. It remains relevant only as credential-pool history and was subsequently merged via #5748 with the unrelated tool-name restoration change removed.

Suggested consolidation

Merge #5551 after addressing the contributor review by supporting canonical key_env with api_key_env as a compatibility alias, using the existing scoped dotenv-aware lookup, and moving regression coverage to tests/agent/test_credential_pool.py. #5580 is not a duplicate and requires no action here because its distinct delegation-leasing work was already merged via #5748.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 21 kB of PR diffs, 4 kB of issue/PR text, 2 kB of discussion (4 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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 P2 Medium — degraded but workaround exists 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.

4 participants