feat(plugin): generate the Claude plugin tree from the monorepo (WALM-669) - #964
harrymove-ctrl wants to merge 1 commit into
Conversation
|
@ducnmm ready for review. This is the generator for the new Mysten plugin repo Five things worth your attention: 1. The security-relevant part — please check this hardest. Thanos's finding 4 cites 2. It stacks on #961. 3. 14 markdown files move into the monorepo (commands, skills/setup, docs/usage, README). That 4. The generated CI asserts the inverse of the old gate. The old 5. No comments in the new code, per Harry's standing rule — which departs from the header-comment Honest note on CI: the generated |
|
Ticket correction: this PR is WALM-669, not WALM-634 — title updated. WALM-634 is "Transfer walrus-memory-mcp-plugin repo to MystenLabs", an admin/permissions task assigned to @ducnmm whose acceptance criteria are all about someone at Mysten running a GitHub org transfer. It contains no code work and this PR does none of what it asks. I picked it originally because it was the nearest-sounding ticket, which was wrong. What this PR actually maps to:
WALM-634's premise is also now stale and I have corrected it separately: Thanos created a new repo ( No code change here — bookkeeping only. |
|
@ducnmm CI is 21/21 green on this and it is ready whenever you have a slot. Quick re-orientation since the ticket changed under it: this is now WALM-669, not WALM-634. WALM-634 is a repo-configuration task assigned to you and this PR does none of it — I retitled and left a comment above explaining the mapping. The one part I would most like a second opinion on is the structural call: 14 markdown files move into The security-relevant half is the injection: Thanos's finding 4 names Still true and worth remembering at merge time: this stacks on #961, so the byte-identical copy of |
ducnmm
left a comment
There was a problem hiding this comment.
Summary
The generator is the right shape for WALM-669: walk packages/mcp/plugin/ plus the overlay, substitute policy/pin from one module, refuse CODEOWNERS/collisions/forbidden paths, and --check --against names missing/extra/differing files. The SKILL.md snippet now carries both rules. The emitted check-plugin-tree.mjs asserts the WALM-640 node ${PLUGIN_ROOT}/scripts/<launcher>.mjs shape rather than npx. No current correctness bug in the generated tree; the holes below are CI completeness and rebase leftovers (#959/#961).
Issue counts by severity
- bugs: 0
- suggestions: 5
- nits: 1
| assert.ok(!paths.includes(".github/CODEOWNERS"), "CODEOWNERS belongs to the published repository"); | ||
|
|
||
| for (const target of paths.filter((name) => name.endsWith(".md"))) { | ||
| assert.doesNotMatch( |
There was a problem hiding this comment.
[suggestion] This job only asserts placeholders are gone. Deleting __MEMWAL_SECRET_POLICY__ from commands/remember.md (or the CLAUDE.md span in skills/setup/SKILL.md) still produces a tree with no leftover __MEMWAL_*__ tokens, so this loop and --check --against of a just-generated tree both pass. The emitted check-plugin-tree.mjs would catch a missing secret block, but MemWal CI does not run it, and MystenLabs/walrus-memory-claude-plugin is still empty.
Suggestion: After generating, import SECRET_EXCLUSION_RULES / AUTO_SAVE_OPT_IN_RULE from the generated scripts/lib/memory-policy.mjs and assert they appear in commands/remember.md, commands/analyze.md, skills/setup/SKILL.md (including the <!-- memwal:start --> span) and README.md. Also exec the generated .github/scripts/check-plugin-tree.mjs with cwd set to the output tree (after the #959 rebase).
| /\.tgz$/, | ||
| ]; | ||
|
|
||
| const REQUIRED = [ |
There was a problem hiding this comment.
[suggestion] REQUIRED is the completeness list --check claims to enforce, but it omits files the walk still copies today: scripts/install_codex_hooks.mjs and scripts/lib/hook-template.mjs. Deleting either still yields a "complete" tree; docs/usage/codex.md Option B would then tell users to run a script that is not there. After rebase onto #959/#961 the same list will also omit scripts/launch_mcp.mjs and scripts/lib/mcp-launch.mjs, so --check will not notice if the WALM-640 launcher is removed from the source tree.
Suggestion: Make REQUIRED the full emitted set (or derive it from the walk and a small denylist). On rebase, include the trusted launcher and its helper explicitly.
| ".codex-mcp.json": "PLUGIN_ROOT", | ||
| ".cursor-mcp.json": "CURSOR_PLUGIN_ROOT", | ||
| })) { | ||
| if (!existsSync(file)) continue; |
There was a problem hiding this comment.
[suggestion] Missing .codex-mcp.json / .cursor-mcp.json is continue, not a failure, so those two WALM-640 configs are optional. commands/analyze.md is in the policy loop but not the required-files list, and that loop also continues on absence. Placeholder and SECRET_EXCLUSION_RULES checks cover only four markdown files (SETUP.md and docs/usage/* can carry a leftover __MEMWAL_*__ after a hand-edit). AUTO_SAVE_OPT_IN_RULE is never imported or asserted.
Suggestion: Require the three MCP configs and commands/analyze.md. Fail closed on missing files. Scan the whole tree for __MEMWAL_*__. Assert AUTO_SAVE_OPT_IN_RULE on skills/setup/SKILL.md and README.md, specifically inside the <!-- memwal:start --> … <!-- memwal:end --> span.
|
|
||
| If `$ARGUMENTS` is empty, ask the user what they want remembered. | ||
|
|
||
| __MEMWAL_SECRET_POLICY__ |
There was a problem hiding this comment.
[suggestion] WALM-669 says both SECRET_EXCLUSION_RULES and AUTO_SAVE_OPT_IN_RULE are injected into commands/remember.md, commands/analyze.md, and skills/setup/SKILL.md. Only __MEMWAL_SECRET_POLICY__ is in the two slash commands. That is probably the right product call — a /remember invocation is an explicit request — but then auto-save still needs a real assertion on SKILL.md / README, which are the surfaces that outlive uninstall.
Suggestion: Keep SECRET-only on the explicit-save commands if that is intentional, and say so. Assert AUTO_SAVE_OPT_IN_RULE on skills/setup/SKILL.md (especially the paste-into-CLAUDE.md block) and README.md. Do not put the opt-in rule under README's "What is never saved" heading; it is a standing user choice, not an exclusion list.
| ] | ||
| ], | ||
| "mcpServers": "./.mcp.json", | ||
| "skills": "./skills/", |
There was a problem hiding this comment.
[suggestion] These pointers are redundant with Claude Code's defaults. Claude's plugin spec adds skills paths to the default skills/ scan rather than replacing it, so "skills": "./skills/" can register the setup skill twice. "commands": "./commands/" replaces the default scan with the same directory (harmless but unnecessary). check-plugin-tree.mjs then requires those keys to exist, which locks in the redundant pointers.
Suggestion: Drop the four pointers and have check-plugin-tree.mjs exist-check the default layout. If a pointer is kept, do not set skills to the default directory. Run claude plugin validate . --strict against a generated tree before merge.
|
|
||
| Walrus Memory gives Claude Code durable, user-owned memory. This plugin packages the published [`@mysten-incubation/memwal-mcp`](https://www.npmjs.com/package/@mysten-incubation/memwal-mcp) server with setup guidance, slash commands, and lifecycle hooks. | ||
|
|
||
| This tree is generated from [`MystenLabs/MemWal/packages/mcp/plugin`](https://github.com/MystenLabs/MemWal/tree/dev/packages/mcp/plugin) by `scripts/build-claude-plugin.mjs`. Do not hand-edit a published copy: change it in MemWal and regenerate, or the fix will be lost at the next build. |
There was a problem hiding this comment.
[nit] The "source of truth" link is hardcoded to MemWal/tree/dev/packages/mcp/plugin. A generation from main or a release tag will still advertise dev.
Suggestion: Point at /tree/HEAD/packages/mcp/plugin or at the repo root without a branch, or substitute a placeholder the generator fills from the build ref.
…-634) The published plugin repo was hand-maintained, so three of the six findings from the 17 Sep review were still sitting in it after being fixed here. scripts/build-claude-plugin.mjs emits the whole publishable tree from packages/mcp/plugin plus a small plugin-repo overlay, so nothing in the published repo is ever edited by hand again. The plugin assets that had no monorepo home — commands, the setup skill, docs/usage and the README — move under packages/mcp/plugin and become the source of truth. The secret-exclusion and auto-save rules are injected into commands/remember.md, commands/analyze.md, skills/setup/SKILL.md and the README from scripts/lib/memory-policy.mjs, so the markdown cannot state a weaker policy than the hooks and the relayer.
44b1ea9 to
21aab02
Compare
|
Rebased onto current
|
The published Claude plugin repo is hand-maintained, so fixes landed here never reach it.
scripts/build-claude-plugin.mjsemits the whole publishable tree — 40 files — frompackages/mcp/plugin/plus a smallpackages/mcp/plugin-repo/overlay, and--checkre-runs the generation to prove it is reproducible and complete. The plugin assets that had no monorepo home (commands, the setup skill,docs/usage/, README) move underpackages/mcp/plugin/and become the source of truth;LICENSEis taken from the repo root, which is already byte-identical. The secret-exclusion and auto-save rules are injected intocommands/remember.md,commands/analyze.md,skills/setup/SKILL.mdand the README fromscripts/lib/memory-policy.mjs, so those files cannot state a weaker policy than the hooks and the relayer.Evidence: the three files review finding 4 cites exist only in the old repo, and they carry their own weaker wording:
Three shared files are already stale in the published repo against
dev:scripts/lib/hook-io.mjs(WALM-644),scripts/install_codex_hooks.mjs(WALM-641),scripts/lib/signals.mjs. Itstest/signals.test.mjsstill testsdetectRecall/detectRemember, which no longer exist.Impact: nobody hits this at runtime today — the plugin repo has not been transferred or listed yet. It is the mechanism that keeps the shipped artifact from drifting again, and without the policy injection the artifact would ship the always-on automatic-memory instructions with no secret-exclusion rule, which is finding 4 unfixed in the thing users install.
The emitted
.github/workflows/ci.ymldeliberately does not carry the old repo'sserver.command === 'npx'gate, which #959 would fail. It asserts the inverse:.mcp.json/.codex-mcp.json/.cursor-mcp.jsonmust start the server asnode ${<CLIENT>_PLUGIN_ROOT}/scripts/<launcher>.mjs, that launcher must exist in the tree, the pin resolved the waypinnedVersion()resolves it must be published on npm, and the four markdown surfaces must carrySECRET_EXCLUSION_RULESverbatim. The generator does not re-derive or re-verify the npm pin: it readsmcpPackageVersion ?? versionfromplugin/plugin.jsonexactly likepinnedVersion(), and existence checking stays inscripts/check-mcp-pin.mjs(#959) and in the emitted repo's own CI.Ordering. This stacks on #961:
packages/mcp/plugin/scripts/lib/memory-policy.mjsis copied byte-identical from that branch because the generator needs it anddevhas no shared policy yet. Merge #961 first and drop this copy in the rebase. It should also land after #959 — run today, the emitted tree fails its owncheck-plugin-tree.mjson all three MCP configs (stillnpx) and on the pin0.0.14, which npm does not carry.Test:
scripts/build-claude-plugin.test.mjs, wired into themetamatrix intest.yml. One test: generating twice is byte-identical, the tree contains the expected files,.github/CODEOWNERSis never emitted, no__MEMWAL_*__placeholder survives in any markdown, and--check --againstthe emitted tree exits 0.