Skip to content

feat: Bitwarden secret resolver and async register#398

Open
Coke1120 wants to merge 3 commits intoCortexReach:masterfrom
Coke1120:feat/bitwarden-secret-resolver
Open

feat: Bitwarden secret resolver and async register#398
Coke1120 wants to merge 3 commits intoCortexReach:masterfrom
Coke1120:feat/bitwarden-secret-resolver

Conversation

@Coke1120
Copy link
Copy Markdown

Summary

  • Add src/secret-resolver.ts supporting ${ENV_VAR} and bws://<secret-id> Bitwarden Secrets Manager refs for embedding, rerank, and LLM API keys
  • Make plugin.register() async to support async secret resolution
  • Update openclaw.plugin.json docs to advertise bws:// support on all API key fields
  • Fix all tests to await plugin.register()

Test plan

  • node --test test/secret-resolver.test.mjs — verify Bitwarden and env-var resolution paths
  • npm test — full test suite passes with async register

Split from #349.

🤖 Generated with Claude Code

Add src/secret-resolver.ts supporting ${ENV_VAR} and bws://<secret-id>
Bitwarden CLI secret references for embedding, rerank, and LLM API keys.
Make plugin register() async to support async secret resolution.
Update openclaw.plugin.json docs to advertise bws:// support.
Fix all tests to await plugin.register().

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
With async register() now awaited, selfImprovement defaults to enabled
and registers command:new before the sessionMemory assertion runs.
Explicitly disable it in the base test config to isolate the assertion.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@AliceLJY AliceLJY left a comment

Choose a reason for hiding this comment

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

Clean implementation. execFile (not exec) avoids shell injection, execFileImpl injection enables proper unit testing, URL-based bws:// ref parsing is well-structured.

Note: PR #399 (Anthropic distill) depends on this PR — it includes all changes from this branch. Please merge #398 first, then #399.

@rwmjhb
Copy link
Copy Markdown
Collaborator

rwmjhb commented Mar 30, 2026

Review: REQUEST-CHANGES

The Bitwarden secret resolver addresses a real operational gap (issue #349). Two issues need fixing before merge:

Must fix:

  1. Async register() breaks synchronous callers — The OpenClaw loader does not await the returned promise, but CLI/hook registration now happens only after await resolveSecretValues(). Hooks and tools are undefined until the promise settles. Test files at test/session-summary-before-reset.test.mjs:104 and :144 also have unawaited plugin.register(api) calls that observe partially initialized state.

  2. Test suite failsselfImprovement: { enabled: false } was added to only one of three test config blocks in plugin-manifest-regression.mjs. The sessionDefaultApi and sessionEnabledApi fixtures still omit it, so command:new gets registered by the default-on selfImprovement feature and breaks the hook-absence assertions.

Worth considering (not blocking):

  • Resolved Bitwarden secrets are passed through resolveEnvVars() again in src/embedder.ts:422-424. If a real API key contains the literal substring ${...}, it will be re-interpreted as an env var template and throw. Bitwarden-resolved values should be treated as opaque.
  • The Bitwarden access token is passed as --access-token CLI argument to bws secret get, making it visible in process listings. Consider using an env var or stdin pipe instead.
  • Branch is behind main (stale_base=true) — please rebase.

…t resolution

Make register() synchronous again (OpenClaw loader does not await it).
All hook/tool registrations happen immediately; embedder, retriever, and
smartExtractor are initialized in a fire-and-forget initPromise that
resolves async secrets. Every hook that uses these awaits initPromise
before proceeding. register() returns initPromise so awaiting callers
(tests, host implementations that do support async) can still wait.

Also fix:
- bws access token passed via BWS_ACCESS_TOKEN env var instead of
  --access-token CLI arg to avoid exposure in process listings
- embedder double-resolveEnvVars: skip expansion if key lacks ${}
- selfImprovement: { enabled: false } in sessionDefaultApi,
  sessionEnabledApi, and session-summary harness to isolate assertions
  from the now-default-true selfImprovement feature

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants