feat: Add Bitwarden secrets and Anthropic distill support#349
feat: Add Bitwarden secrets and Anthropic distill support#349Coke1120 wants to merge 5 commits intoCortexReach:masterfrom
Conversation
AliceLJY
left a comment
AliceLJY
left a comment
There was a problem hiding this comment.
整体设计清晰，测试覆盖核心路径，两个特性 scope 控制得好 👍
Must Fix
1. resolveSecretValueSync 阻塞启动
execFileSync("bws", ...) 在插件 activate() 路径上会阻塞事件循环。如果 Bitwarden CLI 慢或网络抖动，会卡住整个 OpenClaw 启动。
建议：activate() 已经返回 Promise，改用 async 版本 resolveSecretValue。或者至少加个明确的 timeout 错误提示，让用户知道卡在哪。
2. Distill worker 重复了 Bitwarden 解析逻辑
lesson-extract-worker.mjs 里的 resolveMaybeBitwardenSecret() 和 src/secret-resolver.ts 逻辑重复。Worker 是独立脚本可以理解，但至少加个注释标明两处必须同步更新，或抽成共享 ESM 模块。
Nice to have
anthropicGenerateJson（worker）和createAnthropicApiKeyClient（llm-client）独立构建 Anthropic 请求，后续可能分叉，建议加 TODO 注释- 补一个
bws://URL 解析的边缘 case 测试（无 hostname / 缺 secret ID）
修完 1-2 就可以合，辛苦了 🙏
- Change register() to async and replace all resolveSecretValueSync / resolveSecretValuesSync calls with their async counterparts so Bitwarden CLI invocations no longer block the event loop during plugin activation - Add sync comment in lesson-extract-worker.mjs flagging the duplicate Bitwarden resolution logic that must stay in sync with src/secret-resolver.ts - Add TODO comments in anthropicGenerateJson (worker) and createAnthropicApiKeyClient (llm-client) noting the two implementations may diverge and should eventually be unified - Add three edge-case tests for malformed bws:// URLs (empty hostname, empty path after prefix strip) - await plugin.register() in plugin-manifest-regression test to match the now-async register signature Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ister After making register() async, all test callsites must await it or hook registrations won't complete before assertions run. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…resolver tests Resolves the package.json conflict between branch and master: keeps secret-resolver.test.mjs (added in this branch) and adds session-summary-before-reset.test.mjs (added on master). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
@Coke1120 Thanks for the contribution. Two things before we can review: 1. Please split into two PRs — Bitwarden secret resolver and Anthropic distill are independent features. Easier to review, test, and merge separately:
2. Unrelated test file changes — This PR modifies |
|
Closing in favor of two focused PRs per reviewer feedback:
|
2 participants
Summary
src/secret-resolver.ts) for secure API key management via Bitwarden CLI or env varssrc/llm-client.ts) with distill/extraction supportcli.ts) exposing distill functionalityTest plan
test/secret-resolver.test.mjs— verify Bitwarden and env-var resolution pathstest/llm-api-key-client.test.mjs— verify Anthropic client key handlingtest/plugin-manifest-regression.mjs— verify manifest schema unchangedexamples/new-session-distill/with a real sessionCloses #348
🤖 Generated with Claude Code