fix(cli): resolve load_hermes_dotenv home via get_hermes_home() for profile-scoped cron MCP init - #39480
Conversation
…rofile-scoped cron MCP init
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Regression fix for profile-scoped cron MCP initialization. The root cause is well-documented in both the code comments and the test docstring: load_hermes_dotenv() with no args was reading HERMES_HOME from the environment directly, reloading the root .env and stomping the profile's MCP credentials that run_job() had just loaded.
✅ Looks Good
- Fix is precise and surgically targeted at the
hermes_homeparameter branch. - New
get_hermes_home()call handles the override priority correctly. - Two regression tests added: one covers the override path, one covers the no-override fallback (preserves existing behavior).
release.pyAUTHOR_MAP update is cosmetic and correct.
💡 Suggestions
- The comment block explaining the regression is thorough — good practice.
Reviewed by Hermes Agent
…ilege errors (Fixes NousResearch#39480)
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a current profile-isolation defect. Current main still resolves parameterless dotenv loads through os.getenv(...) in hermes_cli/env_loader.py:235, while profile cron MCP discovery reaches the parameterless call in tools/mcp_tool.py:3766-3769. get_hermes_home() already gives the context-local override priority at hermes_constants.py:71-77, so the proposed resolver direction is correct.
Suggested changes
- Consolidate the overlapping platform-default regression coverage from #39982 when salvaging this focused fix; maintainer discussion on #39982 explicitly groups it with #39480.
- Keep
scripts/release.pyout of this runtime fix unless its author-map entry is needed independently.
GitHub currently marks the branch DIRTY, so this should be manually salvaged rather than merged as-is. This is an automated hermes-sweeper review.
|
|
||
| # Auto-extracted from noreply emails + manual overrides | ||
| AUTHOR_MAP = { | ||
| "drexux0@gmail.com": "Drexuxux", |
There was a problem hiding this comment.
This author-map entry is unrelated to the dotenv/profile isolation fix. Please split it out so the runtime regression salvage remains focused.
…ilege errors (Fixes NousResearch#39480)
…ilege errors (Fixes NousResearch#39480)
…ilege errors (Fixes NousResearch#39480)
…ilege errors (Fixes NousResearch#39480)
What?
Profile-scoped cron jobs set a context-local
HERMES_HOMEoverride (viaset_hermes_home_override()) while leavingos.environ["HERMES_HOME"]pointed at the scheduler root. Butload_hermes_dotenv()resolved its home fromos.getenv("HERMES_HOME", Path.home()/".hermes"), ignoring that override. So whendiscover_mcp_tools()→_load_mcp_config()calledload_hermes_dotenv()during a profile cron tick, it reloaded the root.envwithoverride=Trueand stomped the profile's freshly-loaded MCP credentials — breaking${ENV_VAR}interpolation for profile-specificmcp_servers. It also hardcoded~/.hermesinstead of the platform-native default, violating the documented "useget_hermes_home()" rule.Fix
In
load_hermes_dotenv(), when no explicithermes_homeis passed, resolve throughget_hermes_home()instead of readingos.environ["HERMES_HOME"]directly. This honors the context-local profile override (and the platform-native default), fixing all parameterless callers at the source. Explicit-path callers are unchanged.Tests
Added two regression tests in
tests/hermes_cli/test_env_loader.py:test_no_arg_load_honors_context_local_home_override— with root + profile.envdefining the same key and an active override, the profile value wins. (Verified it fails on the old code and passes with the fix.)test_no_arg_load_falls_back_to_env_home_without_override— without an override, the existingHERMES_HOMEenv behavior is preserved.Test results
tests/hermes_cli/test_env_loader.py— 8 passed (incl. 2 new)tests/cron/test_cron_profile.py— passedtests/test_env_loader_secret_sources.py— passed