fix: plugin auto-wires MCP + sandbox-safe skills (#139)#141
Conversation
Two fixes reported in #139: 1. The Claude Code plugin (rohitg00/agentmemory) only shipped hooks and skills; it did not auto-register any MCP server. Users had to configure the @agentmemory/mcp stdio server separately, which the README didn't make clear. Add plugin/.mcp.json so /plugin install agentmemory@agentmemory wires up the MCP server automatically when the plugin is enabled. 2. The `recall` and `session-history` skills used inline bash pre-execution blocks (`!$(curl -s ... ${VAR:-default})`) with shell expansion, which Claude Code's sandbox rejects by pattern match with "Contains expansion" — no way to bypass except for Claude to reconstruct the curl call manually, as the reporter had to work around. Rewrite all four plugin skills (recall / remember / forget / session-history) as pure prompts that tell Claude to use the MCP tools directly: memory_smart_search, memory_save, memory_sessions, memory_governance_delete. No bash, no sandbox issues, no shell escaping. Skills also run faster — no curl subprocess fork on every invocation. Also tighten the README Claude Code install snippet to note that the plugin now registers hooks + skills AND auto-wires the MCP server in one step. Bumps to 0.8.9. 707 tests still pass (no behavioral changes to server code; only plugin assets).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRelease 0.8.9 bumps package versions, auto-wires an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClaudeCode
participant MCP as "@agentmemory/mcp"
participant KV as "kvInstance (storage)"
User->>ClaudeCode: invoke skill (recall/remember/forget/session-history)
ClaudeCode->>MCP: call tool (memory_smart_search / memory_save / memory_governance_delete / memory_sessions) with args
MCP->>KV: query or update (search, save, delete, list sessions)
KV-->>MCP: return results/ack
MCP-->>ClaudeCode: return JSON payload (matches / saved id / deleted report / sessions)
ClaudeCode-->>User: formatted skill response (grouped by session / confirmation / error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
plugin/skills/forget/SKILL.md (1)
16-18: Use explicit field names for session deletion arguments.Line 18 (“in the same argument”) is ambiguous. Please name the exact session field expected by
memory_governance_deleteto reduce mis-calls from the skill prompt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin/skills/forget/SKILL.md` around lines 16 - 18, Update the documentation for memory deletion to explicitly name the session field expected by memory_governance_delete (e.g., use sessionIds: [<sessionId>, ...]) instead of the ambiguous “in the same argument”; mention the exact parameter name memory_governance_delete expects (sessionIds) and show the two clear usage examples: memoryIds: [<observationId>, ...] for specific observations and sessionIds: [<sessionId>, ...] to drop whole sessions so callers use the correct field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 14: Replace the escaped nested backticks in the CHANGELOG entry (the
snippet shown as !\`curl $(...) ${VAR:-default}\`) with proper Markdown-safe
code formatting to satisfy MD038; for example, change it to a simple inline code
span `!curl $(...) ${VAR:-default}` or convert that fragment into a fenced code
block so you no longer need backslash-escaped backticks; update the text that
mentions the `recall` and `session-history` skills accordingly.
In `@plugin/.mcp.json`:
- Around line 4-5: The MCP standalone implementation is missing tool handlers so
calls from the forget/recall skills fail; update src/mcp/standalone.ts to add
the missing tool names to the IMPLEMENTED_TOOLS allowlist and implement their
handler cases (or forward them to the MCP tool registry): add
"memory_governance_delete" and "memory_smart_search" to IMPLEMENTED_TOOLS and
provide corresponding handling logic (or a registry proxy) that performs the
same operations used by the forget and recall skills, ensuring the tool names
and payload shapes match what those skills expect.
In `@plugin/skills/remember/SKILL.md`:
- Around line 18-20: The memory_save handler currently expects comma-separated
strings for `concepts` and `files` but the SKILL.md describes them as arrays;
update the `memory_save` implementation to accept and correctly handle both
shapes: if `concepts` or `files` is already an array, use it as-is (allow empty
arrays), and if it's a string, split on commas and trim entries; ensure the
fields are not dropped when arrays are passed and adjust any validation in
`memory_save` to accept Array<string> as well as string input.
In `@plugin/skills/session-history/SKILL.md`:
- Line 7: The SKILL doc references a `limit: 20` argument for the
memory_sessions MCP tool, but the `memory_sessions` tool schema/handler does not
accept or consume a `limit` parameter; either add support for it or remove the
mention from SKILL.md. To fix: either (A) implement `limit` in the tool by
updating the MCP tool schema (in the .mcp.json entry for memory_sessions) to
include an optional numeric `limit` field and modify the memory_sessions handler
(e.g., memory_sessions handler / memorySessionsHandler function) to read that
argument, apply a default of 20 when absent, and use it to bound the returned
sessions; or (B) remove the `limit: 20` instruction from SKILL.md so the doc
matches current behavior. Ensure you update any tests/docs that assume the new
behavior.
---
Nitpick comments:
In `@plugin/skills/forget/SKILL.md`:
- Around line 16-18: Update the documentation for memory deletion to explicitly
name the session field expected by memory_governance_delete (e.g., use
sessionIds: [<sessionId>, ...]) instead of the ambiguous “in the same argument”;
mention the exact parameter name memory_governance_delete expects (sessionIds)
and show the two clear usage examples: memoryIds: [<observationId>, ...] for
specific observations and sessionIds: [<sessionId>, ...] to drop whole sessions
so callers use the correct field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8742282a-912f-4415-9687-502d640f26af
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
CHANGELOG.mdREADME.mdpackage.jsonpackages/mcp/package.jsonplugin/.claude-plugin/plugin.jsonplugin/.mcp.jsonplugin/skills/forget/SKILL.mdplugin/skills/recall/SKILL.mdplugin/skills/remember/SKILL.mdplugin/skills/session-history/SKILL.mdsrc/functions/export-import.tssrc/types.tssrc/version.tstest/export-import.test.ts
| - `concepts` — the extracted concept list | ||
| - `files` — the extracted file list (empty array if none apply) | ||
| 5. Confirm to the user that the memory was saved and show the concepts you tagged so they know what terms will retrieve it later. |
There was a problem hiding this comment.
memory_save argument shape is mismatched for concepts/files.
Line 18 and Line 19 describe list/array inputs, but memory_save currently parses concepts and files as comma-separated strings. Passing arrays will silently drop both fields.
💡 Suggested wording fix
- - `concepts` — the extracted concept list
- - `files` — the extracted file list (empty array if none apply)
+ - `concepts` — comma-separated concepts string (example: `jwt-refresh-rotation,token-reuse-detection`)
+ - `files` — comma-separated file paths string (empty string if none apply)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `concepts` — the extracted concept list | |
| - `files` — the extracted file list (empty array if none apply) | |
| 5. Confirm to the user that the memory was saved and show the concepts you tagged so they know what terms will retrieve it later. | |
| - `concepts` — comma-separated concepts string (example: `jwt-refresh-rotation,token-reuse-detection`) | |
| - `files` — comma-separated file paths string (empty string if none apply) | |
| 5. Confirm to the user that the memory was saved and show the concepts you tagged so they know what terms will retrieve it later. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/skills/remember/SKILL.md` around lines 18 - 20, The memory_save
handler currently expects comma-separated strings for `concepts` and `files` but
the SKILL.md describes them as arrays; update the `memory_save` implementation
to accept and correctly handle both shapes: if `concepts` or `files` is already
an array, use it as-is (allow empty arrays), and if it's a string, split on
commas and trim entries; ensure the fields are not dropped when arrays are
passed and adjust any validation in `memory_save` to accept Array<string> as
well as string input.
| --- | ||
|
|
||
| Fetch recent session history from agentmemory: | ||
| Fetch recent session history using the `memory_sessions` MCP tool (provided by the agentmemory server that this plugin wires up automatically via `.mcp.json`). Pass `limit: 20` to get a meaningful window. |
There was a problem hiding this comment.
limit: 20 is documented but not supported by memory_sessions.
Line 7 asks for limit: 20, but current tool schema/handler does not consume a limit argument. Either implement limit in memory_sessions or remove that instruction from this skill.
✏️ Minimal doc-side fix
-Fetch recent session history using the `memory_sessions` MCP tool (provided by the agentmemory server that this plugin wires up automatically via `.mcp.json`). Pass `limit: 20` to get a meaningful window.
+Fetch recent session history using the `memory_sessions` MCP tool (provided by the agentmemory server that this plugin wires up automatically via `.mcp.json`).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Fetch recent session history using the `memory_sessions` MCP tool (provided by the agentmemory server that this plugin wires up automatically via `.mcp.json`). Pass `limit: 20` to get a meaningful window. | |
| Fetch recent session history using the `memory_sessions` MCP tool (provided by the agentmemory server that this plugin wires up automatically via `.mcp.json`). |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugin/skills/session-history/SKILL.md` at line 7, The SKILL doc references a
`limit: 20` argument for the memory_sessions MCP tool, but the `memory_sessions`
tool schema/handler does not accept or consume a `limit` parameter; either add
support for it or remove the mention from SKILL.md. To fix: either (A) implement
`limit` in the tool by updating the MCP tool schema (in the .mcp.json entry for
memory_sessions) to include an optional numeric `limit` field and modify the
memory_sessions handler (e.g., memory_sessions handler / memorySessionsHandler
function) to read that argument, apply a default of 20 when absent, and use it
to bound the returned sessions; or (B) remove the `limit: 20` instruction from
SKILL.md so the doc matches current behavior. Ensure you update any tests/docs
that assume the new behavior.
CodeRabbit flagged that the rewritten plugin skills reference
memory_smart_search and memory_governance_delete, but the
standalone MCP shim (@agentmemory/mcp) only exposed 5 tools:
memory_save, memory_recall, memory_sessions, memory_export,
memory_audit. At runtime the skills would have thrown
"Unknown tool" the first time they ran.
Add the missing handlers to src/mcp/standalone.ts:
- memory_smart_search — aliases memory_recall, falling back to
substring search over the in-memory KV store. The standalone
shim has no engine behind it, so BM25/vector/graph aren't
available, but the substring match is good enough for plugin
usage on small personal memory stores.
- memory_governance_delete — deletes memories by id. Accepts
memoryIds as an array or a comma-separated string, returns
{deleted, requested, reason}. Throws on missing/empty input,
silently skips unknown ids.
Also normalize memory_save concepts/files: they now accept both
arrays (what the new skills send) and CSV strings (what the old
skills and legacy clients send). New normalizeList() helper
centralizes the logic.
Fix memory_sessions to honour a limit arg (default 20) — it was
previously returning every session in the store.
Update the forget skill wording per CodeRabbit's nit: make it
explicit that memory_governance_delete only takes memoryIds, and
that deleting a whole session means collecting all memory IDs
from that session via search first.
Adds 8 regression tests to test/mcp-standalone.test.ts covering:
- memory_save with array concepts/files
- memory_save with CSV concepts/files (legacy path)
- memory_smart_search substring fallback
- memory_sessions limit
- memory_governance_delete array path
- memory_governance_delete CSV path
- memory_governance_delete missing/empty validation
- memory_governance_delete unknown-id skip
Full suite: 715 passing (was 707 + 8 new).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/standalone.ts (1)
78-94:⚠️ Potential issue | 🟠 MajorTighten
memory_smart_searchbefore using it in the new forget flow.This fallback only searches
title/content, so/forget <session-id>and/forget <file-path>won't find the memories the skill now promises to delete. It also treats a missing query as"", which makesincludes("")return the first N memories and can surface unrelated IDs in a destructive workflow.Suggested fix
case "memory_recall": case "memory_smart_search": { - const query = (args.query as string)?.toLowerCase() || ""; + const rawQuery = args.query; + if (typeof rawQuery !== "string" || !rawQuery.trim()) { + throw new Error("query is required"); + } + const query = rawQuery.toLowerCase(); const limit = (args.limit as number) || 10; const all = await kvInstance.list<Record<string, unknown>>("mem:memories"); const results = all .filter((m) => { - const text = `${m.title} ${m.content}`.toLowerCase(); + const text = [ + typeof m.title === "string" ? m.title : "", + typeof m.content === "string" ? m.content : "", + Array.isArray(m.files) ? m.files.join(" ") : "", + Array.isArray(m.sessionIds) ? m.sessionIds.join(" ") : "", + ] + .join(" ") + .toLowerCase(); return text.includes(query); }) .slice(0, limit);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/standalone.ts` around lines 78 - 94, The fallback for the "memory_smart_search"/"memory_recall" case is too broad and treats a missing query as "" (which matches everything); change the handler around args.query/args.limit and kvInstance.list so it (1) requires a non-empty query string (reject or return empty results if query is blank), (2) if the query looks like a session id or file path (detect patterns you expect), match against memory.sessionId, memory.sourcePath, or the memory id fields in the objects returned by kvInstance.list<Record<string, unknown>>("mem:memories"), and (3) otherwise perform the existing substring search on `${m.title} ${m.content}`; also ensure limit (args.limit) is respected safely (default and bounds-check) before slicing the filtered results. Reference symbols: memory_smart_search / memory_recall case block, args.query, args.limit, kvInstance.list, and the memory objects' sessionId/sourcePath/id/title/content fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin/skills/forget/SKILL.md`:
- Line 23: Update the troubleshooting text in SKILL.md to stop directing users
to check localhost:3111 and instead instruct them to verify that the plugin was
enabled and that the auto-wired stdio MCP shim started by checking that the
.mcp.json server successfully launched (and relevant logs/output for the MCP
shim), and clarify that the skill relies on the stdio MCP shim rather than a
manually-run agentmemory engine.
---
Outside diff comments:
In `@src/mcp/standalone.ts`:
- Around line 78-94: The fallback for the "memory_smart_search"/"memory_recall"
case is too broad and treats a missing query as "" (which matches everything);
change the handler around args.query/args.limit and kvInstance.list so it (1)
requires a non-empty query string (reject or return empty results if query is
blank), (2) if the query looks like a session id or file path (detect patterns
you expect), match against memory.sessionId, memory.sourcePath, or the memory id
fields in the objects returned by kvInstance.list<Record<string,
unknown>>("mem:memories"), and (3) otherwise perform the existing substring
search on `${m.title} ${m.content}`; also ensure limit (args.limit) is respected
safely (default and bounds-check) before slicing the filtered results. Reference
symbols: memory_smart_search / memory_recall case block, args.query, args.limit,
kvInstance.list, and the memory objects' sessionId/sourcePath/id/title/content
fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cfbb324-8f5d-4fef-853f-ad25027312bc
📒 Files selected for processing (4)
CHANGELOG.mdplugin/skills/forget/SKILL.mdsrc/mcp/standalone.tstest/mcp-standalone.test.ts
✅ Files skipped from review due to trivial changes (2)
- test/mcp-standalone.test.ts
- CHANGELOG.md
…139) CodeRabbit round 2: 1. src/mcp/standalone.ts memory_smart_search accepted '' as a query and used .includes('') which matches every memory. In the forget flow this would surface unrelated memory IDs for deletion. Now throws 'query is required' on missing/empty/ whitespace-only input. Also broadens the search corpus from just title+content to include files, concepts, sessionIds, and id — so users can /forget by file path, concept name, or session ID, as the skill docs already promise. 2. All four skill files (recall, remember, forget, session-history) said 'check the agentmemory engine is running on localhost:3111' when tools are missing. Wrong troubleshooting path: the stdio MCP shim spawned by Claude Code via .mcp.json has no REST API, no port, no localhost. The correct checks are /plugin list, /mcp, and restarting Claude Code after install. Updated all four skills. Adds 2 new regression tests to test/mcp-standalone.test.ts: - memory_smart_search rejects empty/whitespace/missing query - memory_smart_search finds memories by file, concept, or id Full suite: 717 passing.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/standalone.ts (1)
54-55:⚠️ Potential issue | 🟡 MinorHarden
contenttype validation before calling.trim().Casting
args.contentto string can still allow non-string values through and produce runtime method errors instead of a clean validation error.💡 Suggested fix
- const content = args.content as string; - if (!content?.trim()) throw new Error("content is required"); + const rawContent = args.content; + if (typeof rawContent !== "string" || !rawContent.trim()) { + throw new Error("content is required"); + } + const content = rawContent;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/standalone.ts` around lines 54 - 55, The current cast const content = args.content as string can still allow non-string values and cause a runtime error when calling content.trim(); update the validation in the function that reads args.content to first check typeof args.content === "string" (and that it's not empty after trimming) or explicitly coerce safely (e.g., check for null/undefined then String(args.content)) before calling .trim(); ensure the error thrown remains "content is required" for invalid input and reference the local variable content and args.content in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp/standalone.ts`:
- Line 95: Replace the fragile fallback "(args.limit as number) || 10" with
validated, clamped logic: parse args.limit into a number, ensure it's finite and
positive, use a DEFAULT_LIMIT (10) when invalid, then clamp to a reasonable
MAX_LIMIT (e.g., 100) and use Math.floor to force an integer; update both
occurrences of the "(args.limit as number) || 10" expression in standalone.ts so
functions/blocks that reference limit get the validated value.
- Around line 90-95: The code trims rawQuery for emptiness but then lowercases
the untrimmed value, causing mismatches; change the flow so you derive a trimmed
string first (e.g., use rawQuery.trim()) and then lowercase that trimmed value
when setting query (replace the current rawQuery.toLowerCase() assignment),
keeping the existing validation and the limit handling with args.limit as
before; update references to query (the variable used for search) to use the
trimmed+lowercased value.
---
Outside diff comments:
In `@src/mcp/standalone.ts`:
- Around line 54-55: The current cast const content = args.content as string can
still allow non-string values and cause a runtime error when calling
content.trim(); update the validation in the function that reads args.content to
first check typeof args.content === "string" (and that it's not empty after
trimming) or explicitly coerce safely (e.g., check for null/undefined then
String(args.content)) before calling .trim(); ensure the error thrown remains
"content is required" for invalid input and reference the local variable content
and args.content in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b45d7351-1480-4bdf-bea6-8d8f6d12f2de
📒 Files selected for processing (6)
plugin/skills/forget/SKILL.mdplugin/skills/recall/SKILL.mdplugin/skills/remember/SKILL.mdplugin/skills/session-history/SKILL.mdsrc/mcp/standalone.tstest/mcp-standalone.test.ts
✅ Files skipped from review due to trivial changes (1)
- plugin/skills/forget/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- plugin/skills/recall/SKILL.md
- test/mcp-standalone.test.ts
- plugin/skills/session-history/SKILL.md
CodeRabbit round 3:
1. memory_save cast args.content as string and called .trim(),
which throws a runtime TypeError if content is a number, object,
null, etc. Replace the cast with a proper typeof guard that
returns the clean 'content is required' error for any non-string
input.
2. memory_smart_search trimmed rawQuery for emptiness but then
lowercased the *untrimmed* value, so a query like ' bcrypt '
would fail substring matches with trailing whitespace. Trim
first, then lowercase.
3. (args.limit as number) || 10 is fragile — it treats 0 and NaN
the same, silently accepts negative/Infinity values, and has no
upper bound so a malicious or buggy caller could request a
million memories in one slice(). Add parseLimit() helper that:
- rejects non-number/non-string types (booleans, objects, arrays)
- rejects NaN, Infinity, and non-positive values
- clamps to MAX_LIMIT=100 via Math.min
- Math.floor()s to force integer
- falls back to a per-call default (10 for search, 20 for
sessions, 50 for audit)
Applied to all three limit sites: memory_smart_search,
memory_sessions, memory_audit.
Adds 2 regression tests:
- memory_save rejects 42 / {} / [] / null / undefined / true without
crashing on .trim()
- parseLimit falls back on bogus values and clamps 99999 -> 100
Full suite: 719 passing (was 717 + 2 new).
Closes #139.
@stefanfaur reported two bugs that surface together on a fresh `/plugin install agentmemory@agentmemory` install:
Both fixes land together because they share the same architectural cleanup: ship the MCP server with the plugin, and have the skills use MCP tools directly instead of curling the REST API.
What changed
1. `plugin/.mcp.json` (new)
Declares the stdio MCP server so Claude Code auto-starts it when the plugin is enabled:
```json
{
"mcpServers": {
"agentmemory": {
"command": "npx",
"args": ["-y", "@agentmemory/mcp"]
}
}
}
```
Claude Code plugins auto-start MCP servers declared in `.mcp.json` (documented in the Claude Code plugin spec). No user action after install.
2. All four skills rewritten as MCP-tool prompts
Zero bash. Zero shell expansion. Zero sandbox errors. The skills also run faster because they no longer fork a curl subprocess on every invocation.
3. README Claude Code install snippet
Now says explicitly that `/plugin install agentmemory` registers hooks + skills and auto-wires the MCP server via `.mcp.json` in one step.
Test plan
Release
Bumps to 0.8.9 (main package + `@agentmemory/mcp` shim). Shipping right after this merges.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores