Skip to content

fix: harden Bitwarden profile secret loading - #50686

Closed
vasiliyx wants to merge 2 commits into
NousResearch:mainfrom
vasiliyx:proj-secret-manager-closeout-20260622
Closed

fix: harden Bitwarden profile secret loading#50686
vasiliyx wants to merge 2 commits into
NousResearch:mainfrom
vasiliyx:proj-secret-manager-closeout-20260622

Conversation

@vasiliyx

Copy link
Copy Markdown

Summary

  • add Bitwarden Secrets Manager key_prefix / strip_prefix runtime mapping so shared projects can safely expose profile-scoped env vars
  • wire the mapping through env loading plus hermes secrets bitwarden setup/status/sync
  • document profile-prefixed Bitwarden secrets and add hermetic tests
  • include the prepared Discord voice receiver chunking config and reusable noise-gate contract tests so closeout blockers are fully resolved and the worktree is clean

Test plan

  • uv run --extra dev --extra messaging --extra acp python -m pytest tests/test_bitwarden_secrets.py tests/hermes_cli/test_secrets_bitwarden_non_tty.py tests/gateway/test_voice_command.py tests/contracts/test_noise_gate_contract.py -o 'addopts=' -q237 passed, 11 skipped
  • uv run --extra dev python -m ruff check agent/secret_sources/bitwarden.py hermes_cli/env_loader.py hermes_cli/secrets_cli.py hermes_cli/config.py tests/test_bitwarden_secrets.py plugins/platforms/discord/adapter.py tests/gateway/test_voice_command.py tests/contracts/test_noise_gate_contract.py → pass
  • git diff --check → pass
  • Required critic gate (hermes --profile critic ...) → PASS

Notes

A broader local pytest tests/ -q attempt was not a useful signal in this macOS profile: collection/run hit unrelated environment/resource issues (missing optional extras on the first attempt, then long-running suite timeout/open-file pressure after installing optional extras). Targeted changed-area tests and critic review passed.

Add Bitwarden key_prefix/strip_prefix runtime mapping so shared Secrets Manager projects can safely load only profile-scoped env vars, update CLI/status/sync/docs, and cover the behavior with hermetic tests.\n\nAlso resolves closeout blockers by including the prepared Discord voice receiver chunking config and reusable noise-gate contract test suite so the working tree is no longer half-applied.
@vasiliyx
vasiliyx requested a review from a team June 22, 2026 07:50
@alt-glitch alt-glitch added type/feature New feature or request comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have labels Jun 22, 2026

@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 the profile-scoped secret work. The capability is still absent on current main, but this branch needs a focused salvage onto the current secret-source architecture.

Problems

  • hermes_cli/env_loader.py:341-346 now calls registry.apply_all(), and BitwardenSource.fetch() returns raw secret names at agent/secret_sources/bitwarden.py:642-697. The mapping added at agent/secret_sources/bitwarden.py:690 is inside the legacy direct-apply path, so it is bypassed during normal startup on current main.
  • The Bitwarden change is bundled with channel routing, Discord voice chunking, and a noise-gate contract suite. These are independently risky surfaces and obscure review of the secret-loading behavior.

Suggested changes

  • Move mapping into BitwardenSource.fetch() before registry.apply_all() consumes result.secrets, and test the actual registry/env-loader path.
  • Split unrelated gateway, Discord, and contract-suite work. The linked #55886 identifies a competing mapping layer; choose one design before adding overlapping configuration.

Automated hermes-sweeper review.

result.error = str(exc)
return result

secrets, mapping_warnings = map_secrets_for_env(

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.

Current main no longer calls apply_bitwarden_secrets() from startup: hermes_cli/env_loader.py delegates to registry.apply_all(), which uses BitwardenSource.fetch(). Apply this mapping in BitwardenSource.fetch() before it returns result.secrets, otherwise key_prefix and strip_prefix are bypassed at runtime.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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 area/profiles Multi-profile isolation, HERMES_HOME scoping labels Jul 15, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by #69058, which landed profile-scoped secret handling at the apply_all() orchestrator (suffix aliasing + preserve_existing) so every secret source — Bitwarden, 1Password, plugins — inherits it, rather than per-source config in the Bitwarden backend. Your prefix-mapping design solved the same shared-project problem and predates that direction; the writeups in your setup/status/sync wiring were genuinely useful when scoping the orchestrator version. If explicit prefix→name mapping is still needed beyond suffix aliasing (tracked in #63088), the right shape now is a small orchestrator-level key_map — happy to review a focused PR for that. Note the Discord voice/noise-gate work bundled here should be its own PR regardless. Thanks @vasiliyx!

@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/auth Authentication, OAuth, credential pools area/profiles Multi-profile isolation, HERMES_HOME scoping comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have 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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants