Skip to content

fix(env-loader): defer _APPLIED_HOMES.add() until after secret loading succeeds - #40614

Closed
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/applied-homes-premature-add
Closed

fix(env-loader): defer _APPLIED_HOMES.add() until after secret loading succeeds#40614
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/applied-homes-premature-add

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Moves _APPLIED_HOMES.add(home_key) from before secret loading to after all early-return guards pass, so that a failed config load does not permanently prevent retrying on subsequent calls.

Related Issue

Fixes #40597

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/env_loader.py: Removed premature _APPLIED_HOMES.add(home_key) before config loading; added it after all early-return guards (config load, enabled check, import check) pass successfully
  • tests/test_bitwarden_secrets.py: Added 2 regression tests — one verifying home is NOT marked after failed load, one verifying it IS marked after successful load

How to Test

  1. Run pytest tests/test_bitwarden_secrets.py -v — all 38 tests pass including the 2 new regression tests
  2. Run pytest tests/hermes_cli/test_env_loader.py -v — all 6 existing tests pass
  3. Manual verification: set secrets.bitwarden.enabled: true with a broken config, fix it, restart — secrets should now load on retry

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — N/A (pure Python logic, no platform-specific behavior)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

  • Analyzed: hermes_cli/env_loader.py:_apply_external_secret_sources (callers: cli.py, run_agent.py, gateway/run.py via load_hermes_dotenv)
  • Blast radius: LOW — single-line move within a well-isolated function; idempotency guard preserved; no API change
  • Related patterns: fail-closed idempotency guard (check → early return → add → do work)

…g succeeds

Move _APPLIED_HOMES.add(home_key) from before config loading to after
all early-return guards pass. Previously, if _load_secrets_config()
raised or bitwarden was disabled, the home path was permanently marked
as applied — all subsequent calls became no-ops even after the user
fixed the configuration.

Fixes NousResearch#40597
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/config Config system, migrations, profiles labels Jun 6, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the applied-home ordering bug. The premise remains present on current main: hermes_cli/env_loader.py:331 marks the home before the config guards at lines 333-343.

Problems

  • This branch targets the old direct Bitwarden path, but current main delegates through agent.secret_sources.registry.apply_all() at hermes_cli/env_loader.py:346. That path arrived in commit 2d16ec7fb; GitHub currently reports this PR as conflicting.
  • The added tests mock apply_bitwarden_secrets, so they need to be rewritten around the registry-backed wrapper to cover the current behavior.

Suggested changes

  • Salvage the guard ordering in the current wrapper and add a regression that starts with unusable config, then supplies enabled source config and confirms the second call reaches the registry path.
  • Decide explicitly whether an ApplyReport containing a source error should remain cached; current reset_secret_source_cache() is documented as the refresh mechanism after configuration changes (hermes_cli/env_loader.py:320-326).

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 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Resolved by #69056 — and an apology is owed here: you submitted this fix on June 6, well before #69056, and our duplicate sweep missed it when building the fix (we searched symptom keywords but not the issue number — a process failure on our side, now noted). Your placement of the mark-after-guards was exactly right; #69056 implements the same semantics on the SecretSource orchestrator path that superseded the apply shim your branch targeted, plus keeps fetch ERRORS marking the home so startup doesn't re-print failures 3-5x. Credit to you as first submitter for #40597. Thanks @liuhao1024, and sorry for the overlap.

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/cli CLI entry point, hermes_cli/, setup wizard 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.

Bug: _APPLIED_HOMES.add() before secret loading causes permanent skip on failure

3 participants