fix(auth): resolve base_url_env_var via get_env_value everywhere (closes #18757) - #18948
Closed
Sanjays2402 wants to merge 1 commit into
Closed
fix(auth): resolve base_url_env_var via get_env_value everywhere (closes #18757)#18948Sanjays2402 wants to merge 1 commit into
Sanjays2402 wants to merge 1 commit into
Conversation
Closes NousResearch#18757. resolve_api_key_provider_credentials() and friends used os.getenv() to read `base_url_env_var`, which never consults ~/.hermes/.env. Result: providers configured solely via the dotenv file (e.g. Xiaomi with a custom `token-plan-cn.xiaomimimo.com` endpoint) silently fell back to the registry default and returned 401s on auxiliary tasks. The same os.getenv() vs get_env_value() pattern was fixed previously in NousResearch#16101 and NousResearch#17434; this is the third pass for base_url_env_var. PR NousResearch#17246 attempted this fix but only patched 2 of the 6 affected spots and notably missed runtime_provider.py despite claiming to fix it. This commit closes the remaining four: * hermes_cli/auth.py - get_api_key_provider_status (api_key path) [PR NousResearch#17246 hit] - get_external_process_provider_status (Copilot ACP) [missed] - resolve_api_key_provider_credentials [PR NousResearch#17246 hit] - resolve_external_process_provider_credentials [missed] * hermes_cli/runtime_provider.py - resolve_runtime_provider api_key branch [missed] * hermes_cli/model_switch.py - _refresh_curated_models built-in endpoint dedup [missed] get_env_value() preserves shell-export precedence: os.environ wins when the variable is exported, so existing deployments are unaffected. The dotenv file is consulted only as a fallback, matching how API keys have always been resolved. Adds tests/hermes_cli/test_base_url_dotenv_resolution.py covering all four fixed paths plus a guard test that shell exports still take priority.
alt-glitch
added
type/bug
Collaborator
1 similar comment
Collaborator
This was referenced May 4, 2026
Closed
3 tasks
teknium1
reviewed
Jul 12, 2026
teknium1
left a comment
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for tracing the dotenv/base-URL inconsistency across the original resolver paths. The bug remains on current main: hermes_cli/auth.py:6204, :6236, :6391, and :6449, plus hermes_cli/model_switch.py:1560, still bypass dotenv lookup.
Problems
- The
runtime_provider.pyhunk needs a semantic update before salvage. Current main uses profile-scoped_getenv()athermes_cli/runtime_provider.py:1464;get_env_value()directly readsos.environ(hermes_cli/config.py:7735) and would bypass the fail-closed profile secret scope documented inagent/secret_scope.py:130-155. - A current sibling is omitted:
hermes_cli/models.py:2571-2573fingerprintsbase_url_env_varonly fromos.environ, despite that fingerprint representing values the provider reads. - Current API-key resolution deliberately prefers dotenv via
get_env_value_prefer_dotenv()(hermes_cli/auth.py:586-592), so the proposed shell-wins regression expectation needs an explicit policy decision.
Suggested changes
- Apply the dotenv-aware, scope-safe lookup to the current resolver locations and the model-cache fingerprint; update the tests to use
invalidate_env_cache()rather than the stale private cache name.
Automated hermes-sweeper review.
| # custom base URLs stored only in the dotenv file (e.g. Xiaomi) | ||
| # are honoured here too. See issue #18757. | ||
| from hermes_cli.config import get_env_value | ||
| env_url = (get_env_value(pconfig.base_url_env_var) or "").strip().rstrip("/") |
Contributor
There was a problem hiding this comment.
Current main now uses profile-scoped _getenv() for this read. When salvaging, preserve agent.secret_scope isolation rather than replacing it with get_env_value(), which directly reads os.environ and can bypass the fail-closed multiplexed-profile boundary.
teknium1
added
sweeper:risk-compatibility
3 tasks
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
3 participants
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.
Closes #18757
resolve_api_key_provider_credentials()and friends readbase_url_env_varviaos.getenv(), which never consults~/.hermes/.env. A user who sets, e.g.,XIAOMI_BASE_URL=https://token-plan-cn.xiaomimimo.com/v1only in the dotenv file silently falls back to the registry default and gets 401s on auxiliary tasks. API keys are read correctly viaget_env_value()— base URLs are not. This is the same bug class fixed for API keys in #16101 and for TTS/STT in #17434.Why this PR rather than #17246
#17246 (diff) targets the same issue but only patches 2 of 6 buggy spots, and despite the PR description claiming
runtime_provider.pywas "aligned", the diff doesn't touch that file at all.hermes_cli/auth.pyget_api_key_provider_statushermes_cli/auth.pyresolve_api_key_provider_credentialshermes_cli/auth.pyget_external_process_provider_status(Copilot ACP)hermes_cli/auth.pyresolve_external_process_provider_credentialshermes_cli/runtime_provider.pyresolve_runtime_provider(api_key branch)hermes_cli/model_switch.py_refresh_curated_modelsbuiltin endpoint dedup#17246 also bundles 5 unrelated fixes (cron, env precedence, moonshot detection, gateway lock, etc.) — this PR is scoped solely to #18757 so it can land without dragging the rest along.
Behavior
get_env_value()already preserves shell-export precedence —os.environwins when the variable is exported, dotenv is the fallback. Existing deployments are unaffected; users who relied on the dotenv file finally get the right endpoint.Tests
New
tests/hermes_cli/test_base_url_dotenv_resolution.py(7 tests):resolve_api_key_provider_credentials/get_api_key_provider_statusread base URL from~/.hermes/.env(Xiaomi)resolve_external_process_provider_credentials/*_statusread base URL from~/.hermes/.env(Copilot ACP)resolve_runtime_providerhonours dotenv on the api_key branchmodel_switchdedup helper is wired throughget_env_valueRun on the full intersection of test files touching the changed modules — full green, no regressions.
Files changed
hermes_cli/auth.py— top-levelget_env_valueimport + 4 call siteshermes_cli/runtime_provider.py— 1 call sitehermes_cli/model_switch.py— 1 call site (withos.environfallback for safety)tests/hermes_cli/test_base_url_dotenv_resolution.py— newCloses #18757.