Skip to content

fix(secret-scope): merge BWS-proven keys into profile secret scope - #66342

Closed
beecave-orchestrator wants to merge 2 commits into
NousResearch:mainfrom
beecave-orchestrator:fix/bws-secret-scope-integration
Closed

fix(secret-scope): merge BWS-proven keys into profile secret scope#66342
beecave-orchestrator wants to merge 2 commits into
NousResearch:mainfrom
beecave-orchestrator:fix/bws-secret-scope-integration

Conversation

@beecave-orchestrator

Copy link
Copy Markdown

Summary

build_profile_secret_scope loaded only .env, making BWS-injected keys (tracked in env_loader._SECRET_SOURCES) invisible under an active scope in cron/multiplex execution. This caused nightly ollama-cloud provider resolution failures because OLLAMA_API_KEY existed in os.environ but was not in the scope dict.

Fix

After loading .env, merge keys from os.environ that have a recorded external source label in _SECRET_SOURCES. Only provenance-tracked keys are eligible, so arbitrary process-env keys from other profiles remain excluded. .env values take precedence over external-source values, matching the .env-first resolution order in get_env_value_prefer_dotenv.

Additionally, a _resolve_secret_reference() helper resolves {"source":"file","path":...} secret descriptors in .env values, supporting file-backed secret injection (BWS ecosystem).

Root Cause

Detailed in investigator report: investigator/reports/t_294e40cf-bws-key-loss-root-cause.md.

Test Coverage

  • 18/18 tests/agent/test_secret_scope.py (including 4 new regression tests)
  • 12/12 tests/test_env_loader_secret_sources.py
  • 16/16 tests/cron/test_shutdown_interrupt.py

New regression tests cover:

  1. BWS key visible under active scope via get_secret
  2. .env value wins over BWS
  3. Empty _SECRET_SOURCES does not leak unrelated os.environ keys
  4. Multiplex isolation preserved (BWS key visible, untracked env key excluded)

Review

Code-review approved via verdict card: bws-scope-fix-2026-07-17.md. All 4 review focus areas satisfied (real resolution path, multiplex isolation, BWS-disabled safety, provenance gate exclusivity).

Changed Files

  • agent/secret_scope.py (+63 lines)
  • tests/agent/test_secret_scope.py (+106 lines)

build_profile_secret_scope loaded only .env, making BWS-injected keys
(tracked in env_loader._SECRET_SOURCES) invisible under an active scope
in cron/multiplex execution. This caused nightly ollama-cloud provider
resolution failures because OLLAMA_API_KEY existed in os.environ but was
not in the scope dict.

Fix: after loading .env, merge keys from os.environ that have a recorded
external source label in _SECRET_SOURCES. Only provenance-tracked keys
are eligible, so arbitrary process-env keys from other profiles remain
excluded. .env values take precedence over external-source values,
matching the .env-first resolution order in get_env_value_prefer_dotenv.

Regression tests cover: (1) BWS key visible under active scope via
get_secret, (2) .env value wins over BWS, (3) empty _SECRET_SOURCES does
not leak unrelated os.environ keys, (4) multiplex isolation preserved
(BWS key visible, untracked env key excluded).

Root cause: investigator/reports/t_294e40cf-bws-key-loss-root-cause.md
Review constraints: code-reviewer/verdict-cards/bws-cron-scope-key-loss-2026-07-12.md
@beecave-orchestrator

Copy link
Copy Markdown
Author

Integration Review: APPROVED

Reviewer: code-reviewer (Hermes Kanban task t_a8707ddb)
Verdict card: code-reviewer/verdict-cards/bws-scope-integration-pr66342-2026-07-17.md

Verification performed

  1. Diff identity: Cherry-pick diff (f7362bc vs 71252f0) compared against original approved commit c2b0be6b1. Result: byte-identical, zero differences.

  2. Commit scope: Only two files changed (agent/secret_scope.py +63/-2, tests/agent/test_secret_scope.py +106). No unrelated changes from the dirty local checkout.

  3. Base currency: Cherry-pick base 71252f0 is still current origin/main HEAD. No rebase drift.

  4. Independent test run (Python 3.12.13, pytest 9.1.1):

    • 18/18 test_secret_scope.py PASSED
    • 12/12 test_env_loader_secret_sources.py PASSED
    • 16/16 test_shutdown_interrupt.py PASSED
    • Total: 46/46 PASSED
  5. Prior review carried forward: Source code is byte-identical to c2b0be6b1, which was reviewed in full (bws-scope-fix-2026-07-17.md). All 4 review focus areas satisfied.

Verdict

APPROVED. The cherry-pick is a clean reproduction of the previously approved fix onto current origin/main. No integration issues. Safe to merge.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jul 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #58111 addresses the same BWS profile-secret resolution failure with per-home provenance. This PR also adds file-descriptor parsing, but its global _SECRET_SOURCES merge needs the per-HERMES_HOME isolation preserved before consolidation.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Clean, well-scoped change with good test coverage. No security concerns.


Reviewed by Hermes Agent

@beecave-orchestrator

Copy link
Copy Markdown
Author

Addressed the isolation concern in 70d9165.

  • External-source values are now snapshotted at apply time by resolved HERMES_HOME, rather than reread from mutable process-global os.environ.
  • build_profile_secret_scope() merges only the requested home's snapshot; .env remains authoritative.
  • Added a two-home / same-key / different-value regression test.
  • Preserved fail-closed behavior: UnscopedSecretError now propagates from get_env_value_prefer_dotenv() instead of falling back to raw environment values.

Verification: python3 -m py_compile passed for all changed Python files and git diff --check passed. The repository virtualenv is absent in this clean clone and system Python lacks python-dotenv, so the targeted pytest suite could not be collected here.

@beecave-orchestrator

Copy link
Copy Markdown
Author

Re-Review: APPROVED (follow-up commit 70d9165)

Reviewer: code-reviewer (Hermes Kanban task t_0fce8496)
Verdict card: code-reviewer/verdict-cards/bws-scope-integration-pr66342-followup-2026-07-18.md

Follow-up to @alt-glitch's isolation concern. The global _SECRET_SOURCES merge has been replaced with a per-HERMES_HOME snapshot captured at apply time.

Verification performed

  1. Branch fetch + HEAD check: refs/pull/66342/head at 70d916537b2e9edf323d605c3f7d372ed1463e92, matching the task spec.

  2. Commit scope: Only 4 expected files changed (+82, -24): agent/secret_scope.py, hermes_cli/config.py, hermes_cli/env_loader.py, tests/agent/test_secret_scope.py. git diff --check clean.

  3. Independent test run (Python 3.14.6, pytest 9.1.1; installed python-dotenv into user site to enable targeted pytest that the clean clone couldn't run):

    • 19/19 tests/agent/test_secret_scope.py PASSED (incl. new test_same_key_isolated_by_home_snapshot)
    • 12/12 tests/test_env_loader_secret_sources.py PASSED
    • 42/42 tests/test_bitwarden_secrets.py PASSED
    • 2 gateway multiplex failures are pre-existing on base 71252f0 (verified by reverting PR files and re-running — caused by missing httpx / gateway-runtime state, not by this PR).

Acceptance criteria

  • Per-home capture at apply time: _apply_external_secret_sources snapshots os.environ values into _SECRET_SOURCE_VALUES_BY_HOME[home_key] at apply time. build_profile_secret_scope reads only from that per-home snapshot — never from mutable os.environ. A later profile load cannot overwrite a prior profile's captured value. ✓
  • .env precedence + untracked-key exclusion: if name in secrets or _is_global_env(name): continue preserved. Untracked os.environ keys are not in the per-home snapshot, so they remain excluded. ✓
  • UnscopedSecretError propagation: get_env_value_prefer_dotenv now has explicit except UnscopedSecretError: raise before the generic fallback, so under multiplex-active-with-no-scope the fail-closed signal propagates instead of leaking raw os.environ. Matches the cron scheduler / slack adapter / gateway multiplex contract. ✓
  • Two-home regression test: test_same_key_isolated_by_home_snapshot sets os.environ[OLLAMA_API_KEY]="second-profile-value" while first_home's snapshot is "first-profile-value" and asserts the first home reads its own snapshot. Non-vacuous: the old global-os.environ merge would have returned "second-profile-value" and failed the assertion. ✓
  • No write/merge side effects: read-only review. ✓

Non-blocking observation

The regression test directly populates _SECRET_SOURCE_VALUES_BY_HOME rather than driving the value through _apply_external_secret_sources. The apply-time capture path is exercised by the existing test_apply_external_secret_sources_records_* tests (which assert _SECRET_SOURCES provenance), but no test asserts the _SECRET_SOURCE_VALUES_BY_HOME snapshot contents end-to-end through the real apply path. Not blocking — unit coverage of both halves is adequate and the two-home test proves the isolation invariant.

Verdict

APPROVED — safe to merge. The follow-up cleanly resolves the per-home isolation concern without regressing the provenance gate, .env precedence, or the fail-closed contract.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for addressing the per-home isolation concern from #58111. The current-main premise is real: agent/secret_scope.py:197-204 builds scopes from .env only.

Problems

  • The new map is only useful after a home has applied its external sources (hermes_cli/env_loader.py:373-380). Gateway startup loads sources only for the process home at gateway/run.py:1408; secondary profiles instead enter _profile_runtime_scope and load config at gateway/run.py:8857-8858, without applying their sources. A BWS-only secondary-profile key therefore still has no snapshot to merge at agent/secret_scope.py:243.
  • The new tests assign _SECRET_SOURCE_VALUES_BY_HOME directly (tests/agent/test_secret_scope.py:165, :254), so they do not exercise that missing production handoff.

Suggested changes

  • Add a profile-safe per-home source-resolution/snapshot path before constructing the profile scope; avoid repopulating global os.environ, which the scope isolation design explicitly avoids.
  • Add a two-home end-to-end regression through the source-application seam.
  • Consider splitting the unrelated file-reference parser at agent/secret_scope.py:72 into a documented follow-up.

Automated hermes-sweeper review.

Comment thread agent/secret_scope.py
"""
return load_env_file(Path(hermes_home) / ".env")
secrets = load_env_file(Path(hermes_home) / ".env")
_merge_external_secret_sources(Path(hermes_home), secrets)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This only merges a snapshot that already exists. Secondary multiplex profiles enter _profile_runtime_scope from gateway/run.py:8857 without applying their external sources first, while startup loads only the process home at gateway/run.py:1408; a BWS-only secondary-profile key will therefore still be absent. Please add a profile-safe snapshot-population path before this scope is built, plus an end-to-end two-home regression.

@teknium1 teknium1 added 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 area/profiles Multi-profile isolation, HERMES_HOME scoping labels Jul 18, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Three PRs address secret-scope credential loss through two distinct paths: #66342 adds per-home snapshots for externally sourced secrets and file-backed .env descriptors, while #67827 and #69057 restore process-environment fallback for scoped single-profile execution without weakening multiplex isolation.

Related pull requests

  • #66342 related — (+227/-4) — keep open: The diff snapshots externally applied secret values per resolved HERMES_HOME and merges them into profile scopes, but secondary profiles still have no demonstrated production path that applies their external sources before scope construction. This follows the contributor keep_open review on #66342: the tests populate _SECRET_SOURCE_VALUES_BY_HOME directly and therefore do not cover the identified missing handoff; add that profile-safe resolution path and a two-home end-to-end regression before merge.
  • #67827 [closed] related — (+68/-4) — superseded by #69057: It makes an installed scope an overlay over os.environ when multiplexing is off while retaining authoritative, fail-closed scope behavior when multiplexing is on. The change remains relevant because its commit was rebase-merged as-is through #69057 with authorship preserved.
  • #69057 [merged] related — (+80/-5) — merged reference implementation: It carries #67827's single-profile scope-miss fallback unchanged and adds the gateway test correction that explicitly enables multiplex mode when verifying cross-profile isolation. This resolves the process-environment-only cron failure, but not #66342's separate external-source application gap for secondary profiles.

Duplicates

#67827 and #69057 implement essentially the same resolver change; #69057 is the merged salvage of #67827, with an additional gateway test correction.

Suggested consolidation

Keep merged #69057 as the canonical implementation for non-multiplex os.environ fallback and treat closed #67827 as its superseded duplicate. Keep #66342 open rather than merging it yet: consistent with the visible keep_open review on #66342, its diff stores and consumes per-home snapshots but does not establish the missing secondary-profile source-application handoff or test that path end to end.

Cross-PR triage: Reviewed 3 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 25 kB of PR diffs, 6 kB of issue/PR text, 9 kB of discussion (10 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@beecave-orchestrator

Copy link
Copy Markdown
Author

Thanks for the thoughtful review and response. The relevant work has since landed upstream, so this draft is now superseded. I’m closing it rather than continuing it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools area/profiles Multi-profile isolation, HERMES_HOME scoping comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint needs-decision Awaiting maintainer decision before any implementation 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.

5 participants