feat(plugins): add memlawb as an opt-in built-in plugin - #2206
beardthelion wants to merge 2 commits into
Conversation
Offers memlawb's MCP server, which gives an agent durable end-to-end-encrypted
memory, as a plugin that is off until someone turns it on. It contributes one
stdio server and nothing else: no skill, no hook, no prompting. That is the
point rather than an omission, because the memdir pipeline and team memory have
to keep working exactly as they do now.
Every value in the server's environment is a ${VAR} reference, never a literal.
The reason is specific to this codebase: a stdio child's environment is built as
{ ...subprocessEnv(), ...server env } in src/services/mcp/client.ts, and
subprocessEnv() returns the parent's own environment, so anything exported for
one MCP server is readable by all of them.
The passphrase is therefore referenced by PATH rather than by value, through
MEMLAWB_PASSPHRASE_FILE. The path still rides the same shared environment; the
secret no longer does. That narrows the exposure from "every configured MCP
server has your passphrase" to "every configured MCP server knows where it is",
which is a real improvement and is not isolation, and the plugin's comment says
so in those terms.
Only the path form is declared, and not merely because offering both would
re-open what this closes. memlawb checks MEMLAWB_PASSPHRASE for unexpanded
template text, and this host leaves an unset reference as its own literal, so a
correctly configured user with the file exported and the variable deliberately
unset would be handed template text and refused at startup. Declaring both is a
hard break of the intended setup, not a fallback.
The absence test asserts the passphrase reaches no configured server and carries
two positive controls: the service key does reach memlawb's resolved
environment, and a server that deliberately declares the passphrase does receive
it, so the assertion can fail. Its limit is recorded in the test: it reads
resolved plugin config and cannot see the spawn-time spread, so what it proves
is that this plugin does not put the secret there.
Two scenarios remain manual and are not stood in for by anything here: that
enabling the plugin leaves the memdir log and its recall working and disabling
it leaves nothing behind, and that a durable cross-machine preference lands in
memlawb while a session-log fact does not.
Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
The plugin references MEMLAWB_PASSPHRASE_FILE, which older memlawb builds do not read. Those builds refuse to start saying no passphrase is set, which points a user at their own configuration rather than at an out-of-date binary, so the description now says which version reads it. zero's equivalent already printed a minimum; this brings the two integrations in line. Signed-off-by: beardthelion <56458543+beardthelion@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions.⚙️ CodeRabbit configuration file Files:
Apply the OpenClaude maintainer review rubric from AGENTS.md.⚙️ CodeRabbit configuration file Files:
🪛 ast-grep (0.45.2)src/plugins/bundled/memlawb.test.ts[error] 31-31: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. (prototype-pollution-recursive-merge-typescript) [error] 43-46: Recursive/iterative merge copies attacker-controllable keys from a source object into a target via a computed property assignment without rejecting dangerous keys, allowing prototype pollution. Skip or block "proto", "constructor", and "prototype" keys (e.g. (prototype-pollution-recursive-merge-typescript) 🔇 Additional comments (3)
📝 WalkthroughWalkthroughAdds an opt-in bundled Memlawb plugin that runs ChangesMemlawb plugin
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This adds an opt-in Memlawb MCP plugin with passphrase-file configuration and leaves existing users unaffected unless they enable it. No current merge-readiness risk remains. 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
wow cool thanks for this @beardthelion |
2 participants
Summary
memlawbas an opt-in built-in plugin, disabled by default, contributing one stdio MCP server and nothing else: no skill, no hook, no prompting.I read
CONTRIBUTING.mdandAGENTS.mdbefore opening this.Impact
/plugingains a memlawb entry. Enabling it needs the memlawb CLI 0.1.0 or newer onPATHand four environment variables. Nothing changes for anyone who leaves it off, which is everyone by default.initBuiltinPlugins. No existing behaviour is modified.Why the passphrase is a path, not a value
This is the part worth reviewing. Every value in the server's env is a
${VAR}reference, and the passphrase is referenced as a file path viaMEMLAWB_PASSPHRASE_FILE.The reason is specific to this codebase rather than general caution.
src/services/mcp/client.tsbuilds a stdio child's environment as{ ...subprocessEnv(), ...server env }, andsubprocessEnv()returns the parent's own environment. So a passphrase exported so openclaude can expand it into memlawb's config is equally readable by every other MCP server the user runs, not just this one.A path still rides that same shared environment. The secret no longer does. That narrows the exposure from "every configured MCP server has your passphrase" to "every configured MCP server knows where the file is", which is a real improvement and is not isolation. The plugin's doc comment says exactly that rather than implying more.
Only the path form is declared, and not merely to avoid re-opening what this closes: memlawb checks
MEMLAWB_PASSPHRASEfor unexpanded template text, and this host leaves an unset reference as its own literal, so a correctly configured user (file exported, variable deliberately unset) would be handed template text and refused at startup. Declaring both is a hard break of the intended setup, not a fallback.MEMLAWB_PASSPHRASE_FILEis a memlawb-side input added for this integration, which is why the description names the minimum version. An older binary reads onlyMEMLAWB_PASSPHRASEand fails saying no passphrase is set, which points a user at their own config rather than at a stale install.Testing
Exact commands and results:
Focused tests:
bun test src/plugins/bundled/(10 pass), andbun test src/plugins/ src/utils/plugins/ src/services/mcp/(257 pass, 23 files).Verified pre-existing failures:
bun run typecheckreports three errors, insrc/tools/AgentTool/AgentTool.teammateModel.test.ts:457andsrc/utils/permissions/permissions.test.ts:695,713. I confirmed these by running the same typecheck on a cleanorigin/mainworktree, which produces the identical three. They are in files this PR does not touch.Every guard in the new test was removed once and the named test observed red: default-enabled flipped, a skill added, a hook added, the command and args changed, a second server added, the registration removed, the passphrase written as a literal, declared as both forms, and pointed at the wrong variable.
The absence test ("the passphrase reaches no configured server") carries two positive controls, because without them it passes against a check that finds nothing: the service key is present in memlawb's resolved env, and a server that deliberately declares the passphrase does receive it. Its limit is recorded in the test body: it reads resolved plugin config and cannot see the spawn-time spread, so what it proves is that this plugin does not put the secret there.
Notes
/pluginlist.MEMLAWB_PASSPHRASE_FILEis published, since the plugin is unusable before then.Summary by CodeRabbit