fix(config): honor key_env for custom provider - #43704
Open
Theagentvikram wants to merge 1 commit into
Open
Theagentvikram wants to merge 1 commit into
Theagentvikram wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for resolving API keys for custom OpenRouter-compatible endpoints from a model-configured env var (e.g. model.key_env), and verifies the behavior with a new regression test.
Changes:
- Read
key_env/api_key_envfrommodel_cfgand use the referenced env var value as an API key candidate for custom endpoints. - Add a test ensuring
model.key_envis honored for a configured custom endpoint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/hermes_cli/test_runtime_provider_resolution.py | Adds a regression test for model.key_env-based API key resolution on custom endpoints. |
| hermes_cli/runtime_provider.py | Extends _resolve_openrouter_runtime to resolve API keys via model_cfg.key_env / model_cfg.api_key_env. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+782
to
+788
| cfg_key_env_api_key = "" | ||
| for k in ("key_env", "api_key_env"): | ||
| env_name = model_cfg.get(k) | ||
| if isinstance(env_name, str) and env_name.strip(): | ||
| cfg_key_env_api_key = os.getenv(env_name.strip(), "").strip() | ||
| if cfg_key_env_api_key: | ||
| break |
Comment on lines
864
to
870
| api_key_candidates = [ | ||
| explicit_api_key, | ||
| (cfg_api_key if use_config_base_url else ""), | ||
| (cfg_key_env_api_key if use_config_base_url else ""), | ||
| (os.getenv("OLLAMA_API_KEY") if _is_ollama_url else ""), | ||
| (os.getenv("OPENAI_API_KEY") if (_is_openai_url or _is_openai_azure) else ""), | ||
| (os.getenv("OPENROUTER_API_KEY") if _is_openrouter_url else ""), |
Comment on lines
+603
to
+606
| monkeypatch.delenv("OPENAI_BASE_URL", raising=False) | ||
| monkeypatch.delenv("OPENAI_API_KEY", raising=False) | ||
| monkeypatch.delenv("OPENROUTER_API_KEY", raising=False) | ||
| monkeypatch.setenv("MY_CUSTOM_API_KEY", "env-custom-key") |
Comment on lines
+782
to
+788
| cfg_key_env_api_key = "" | ||
| for k in ("key_env", "api_key_env"): | ||
| env_name = model_cfg.get(k) | ||
| if isinstance(env_name, str) and env_name.strip(): | ||
| cfg_key_env_api_key = os.getenv(env_name.strip(), "").strip() | ||
| if cfg_key_env_api_key: | ||
| break |
alt-glitch
added
type/bug
teknium1
reviewed
Jul 14, 2026
teknium1
left a comment
teknium1
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the focused regression fix. The current generic bare-custom path still has the reported gap: hermes_cli/runtime_provider.py:1139-1150 has no candidate derived from model.key_env or model.api_key_env.
Problems
- The new
os.getenv()onhermes_cli/runtime_provider.py:788bypasses_getenv(), whose current contract is profile-scoped, fail-closed credential access (hermes_cli/runtime_provider.py:49-59). Use_getenv()for this credential read. - The patch does not cover the direct-alias custom path:
hermes_cli/runtime_provider.py:951-964still has no model env-hint candidate and can returnno-key-required.
Suggested changes
- Centralize scoped resolution of the model env hint and use it in both custom resolution paths, with regression tests for each.
Automated hermes-sweeper review.
| if isinstance(env_name, str) and env_name.strip(): | ||
| cfg_key_env_api_key = os.getenv(env_name.strip(), "").strip() | ||
| if cfg_key_env_api_key: | ||
| break |
Collaborator
There was a problem hiding this comment.
Use the module's _getenv() accessor here rather than os.getenv(). Current main defines _getenv() as the profile-scoped, fail-closed credential reader (hermes_cli/runtime_provider.py:49-59); bypassing it can read an unscoped secret under multiplexing.
teknium1
added
sweeper:risk-security-boundary
This was referenced Aug 3, 2026
Open
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #43586
Tests