Skip to content

Refresh Hermes env before cron and MCP loads - #36863

Closed
bee-san wants to merge 5 commits into
NousResearch:mainfrom
bee-san:fix/cron-secret-refresh
Closed

bee-san wants to merge 5 commits into
NousResearch:mainfrom
bee-san:fix/cron-secret-refresh

Conversation

@bee-san

@bee-san bee-san commented Jun 1, 2026

Copy link
Copy Markdown

Summary

  • Re-read Hermes .env/BSM secrets before cron jobs and MCP config expansion so rotated values are visible during per-run reloads.
  • Added regression coverage for cron reload, env-source cache reset, and MCP config interpolation order.

Tests

  • python -m pytest tests/test_bitwarden_secrets.py tests/test_env_loader_secret_sources.py -q -o 'addopts='
  • python -m pytest tests/cron/test_scheduler.py -q -o 'addopts='
  • python -m pytest tests/cron/test_cron_profile.py -q -o 'addopts='
  • python -m pytest tests/cron/test_cron_workdir.py -q -o 'addopts='
  • python -m pytest tests/hermes_cli/test_config_env_expansion.py -q -o 'addopts='
  • python -m pytest tests/hermes_cli/test_mcp_config.py tests/hermes_cli/test_mcp_tools_config.py tests/hermes_cli/test_custom_provider_model_switch.py -q -o 'addopts='
  • python -m pytest tests/tools/test_mcp_tool.py -q -o 'addopts='

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management tool/mcp MCP client and OAuth area/config Config system, migrations, profiles labels Jun 1, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Correct fix. The key change in tools/mcp_tool.py—calling load_hermes_dotenv() before load_config()—ensures env vars are refreshed before config.yaml expansion. The reset_secret_source_cache() change clears the Bitwarden cache properly. Tests verify the order of operations. No issues found.

1 similar comment
@liuhao1024

Copy link
Copy Markdown
Contributor

Correct fix. The key change in tools/mcp_tool.py—calling load_hermes_dotenv() before load_config()—ensures env vars are refreshed before config.yaml expansion. The reset_secret_source_cache() change clears the Bitwarden cache properly. Tests verify the order of operations. No issues found.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for identifying the dotenv/config ordering issue. The cron half has already landed through later work: current cron/scheduler.py:2776-2793 resets the secret-source cache and reloads Hermes dotenv, shipped in 836732f54.

Problems

  • The scheduler hunk is now superseded and the PR is conflicting against current main; it should not be reapplied over the current cron path.
  • The added MCP test mocks load_config(), so it does not exercise the real expansion at hermes_cli/config.py:7010, where stale environment values are materialized before the current reload at tools/mcp_tool.py:3766-3770.

Suggested changes

  • Salvage the MCP-only reorder: call load_hermes_dotenv() before load_config() in tools/mcp_tool.py.
  • Add a temp-HERMES_HOME regression using real config.yaml expansion and a refreshed .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 13, 2026
@bee-san
bee-san force-pushed the fix/cron-secret-refresh branch from df1cf6f to 6f2fd9a Compare July 19, 2026 18:10
@bee-san

bee-san commented Jul 19, 2026

Copy link
Copy Markdown
Author

Rebased onto current main and narrowed to the still-missing behavior at exact head 6f2fd9a92: MCP config now reloads Hermes env/BSM sources before config expansion, and refresh clears stale secret-source provenance. Current main already had the cron reload path, so that duplicate production hunk was dropped while its regression coverage was preserved. Focused validation: 228 tests passed across env-loader/MCP plus the cron regression before the final assertion portability fix; the exact three affected regressions then passed, Ruff passed, and git diff --check passed.

@alt-glitch alt-glitch added comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard and removed comp/cron Cron scheduler and job management sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Jul 19, 2026
@bee-san

bee-san commented Jul 19, 2026

Copy link
Copy Markdown
Author

Exact-head QA confirms the MCP reload ordering at 6f2fd9a is correct (including a real temp-HERMES_HOME config expansion from a refreshed synthetic .env value), and the broader focused suite passed. The rollout gate is still blocked by the BWS failure-output path this reload can exercise: _run_bws_list embeds up to 200 raw characters of backend stderr/stdout in RuntimeError, apply_bitwarden_secrets stores str(exc), and env_loader prints the resulting error. A synthetic backend marker survived unchanged. Before this rollout merges, please make failure output generic/redacted and add tests asserting backend stderr/exception, bootstrap-token, and secret-value sentinels never reach terminal/log output while startup remains non-fatal.

@bee-san bee-san left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

QA verdict at exact head 6f2fd9a92: blocked. The focused env-loader/MCP/cron suite passes (450 tests), but the required two-pass refresh behavior does not: a vault-owned value remains in os.environ after the remote key disappears. No real profile, gateway, cron, or Bitwarden state was modified.

Comment thread hermes_cli/env_loader.py
@alt-glitch alt-glitch added comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery labels Jul 19, 2026
@alt-glitch alt-glitch added comp/tui Terminal UI (ui-tui/ + tui_gateway/) needs-decision Awaiting maintainer decision before any implementation sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data and removed comp/cron Cron scheduler and job management labels Jul 19, 2026
@bee-san

bee-san commented Jul 20, 2026

Copy link
Copy Markdown
Author

Exact-head independent QA at ca3186895bd07825d2af9e12e4b690b03eb4de27: the changed/focused suite passed (570 tests) and Ruff passed, including redaction, rotation, removal, failed-source retention, shell/.env replacement, collision, cron, and MCP coverage. One refresh edge case still blocks rollout:

  • hermes_cli/env_loader.py:475-484 converts a malformed/unreadable config.yaml into {}. After reset_secret_source_cache() has moved provenance into _PENDING_SECRET_OWNERSHIP, _apply_external_secret_sources() intentionally continues on an empty config (:413-429), and apply_all() removes the matching source-owned env values before finding no enabled source to restore them. A synthetic two-pass probe produced before='known-good', after=None, and cleared provenance. This differs from the adjacent failed-fetch behavior, which retains the known-good value, and can drop live credentials during a gateway/cron reload while a config edit is malformed.

Please distinguish config parse/read failure from an intentionally empty/disabled secrets config. On parse/read failure, retain pending ownership plus current known-good values and leave the home retryable; add a regression covering valid first load → reset → malformed config → value/provenance retained without leaking parser/backend details.

GitHub's required All required checks pass context is also absent because the fork CI run ended action_required; this head is not merge-eligible yet.

@alt-glitch alt-glitch removed the needs-decision Awaiting maintainer decision before any implementation label Jul 20, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Closing with thanks — the bug halves of this have landed on main via the secrets cluster: #69056 made failed secret loads retryable (the permanent-skip half), #69053 fixed MCP stdio servers missing vault-injected vars, and cron jobs re-enter load_hermes_dotenv() per run. What remains from your PR is mid-process refresh of ROTATED values inside one long-lived process — a real feature ask, but it belongs as a small TTL knob on the orchestrator's once-per-process guard (a few lines in apply_all/env_loader), not a cli.py+gateway+tui patch. If you want to resubmit that focused version against the current agent/secret_sources/registry.py, we'd welcome it. Thanks @bee-san!

@teknium1 teknium1 closed this Jul 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery comp/tui Terminal UI (ui-tui/ + tui_gateway/) 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 tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants