feat(mcp): expose memory_forget tool so observations and sessions can actually be deleted - #842
Closed
serhiizghama wants to merge 3 commits into
Closed
feat(mcp): expose memory_forget tool so observations and sessions can actually be deleted#842serhiizghama wants to merge 3 commits into
serhiizghama wants to merge 3 commits into
Conversation
|
@serhiizghama is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. 📝 WalkthroughWalkthroughThis PR implements a new ChangesMemory Forget MCP Tool and Deletion Semantics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
README.md (1)
892-918:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign MCP section labels with the new 54-tool total.
The top of the MCP section says 54 tools, but the subsection labels still say
53 Toolsand53 total. Update those labels to avoid contradictory docs.As per coding guidelines, when adding/removing MCP tools, README MCP tool counts must be updated consistently (search for “MCP tools”).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 892 - 918, Update the MCP tool count labels to match the new 54-tool total: change the "### 53 Tools" heading to "### 54 Tools" and update the "<summary>Extended tools (53 total — set AGENTMEMORY_TOOLS=all)</summary>" text to reflect "54 total" (and search for any other occurrences of "53 Tools", "53 total", or "MCP tools" in the README and replace them with "54"/"54 total" to keep counts consistent).Sources: Coding guidelines, Learnings
src/functions/remember.ts (1)
182-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix over-counting in
mem::forgetdeletion results and audit details.The new breakdown fields at Line 261-Line 267 can report deletions that never happened in KV, because counters are incremented unconditionally in Line 182-Line 193 and Line 200-Line 214 even when records are absent. That makes
deleted,memoriesDeleted,observationsDeleted, and audit details inaccurate.Suggested fix
if (data.memoryId) { const mem = await kv.get<Memory>(KV.memories, data.memoryId); - await kv.delete(KV.memories, data.memoryId); - if (mem?.imageRef) { - await decrementImageRef(kv, sdk, mem.imageRef); - } - await deleteAccessLog(kv, data.memoryId); - getSearchIndex().remove(data.memoryId); - vectorIndexRemove(data.memoryId); - deletedMemoryIds.push(data.memoryId); - deleted++; + if (mem) { + await kv.delete(KV.memories, data.memoryId); + if (mem.imageRef) { + await decrementImageRef(kv, sdk, mem.imageRef); + } + await deleteAccessLog(kv, data.memoryId); + getSearchIndex().remove(data.memoryId); + vectorIndexRemove(data.memoryId); + deletedMemoryIds.push(data.memoryId); + deleted++; + } } @@ for (const obsId of data.observationIds) { const obs = await kv.get<{ imageData?: string; imageRef?: string }>( KV.observations(data.sessionId), obsId, ); - await kv.delete(KV.observations(data.sessionId), obsId); - if (obs?.imageData) await decrementImageRef(kv, sdk, obs.imageData); - if (obs?.imageRef && obs.imageRef !== obs.imageData) { - await decrementImageRef(kv, sdk, obs.imageRef); - } - getSearchIndex().remove(obsId); - vectorIndexRemove(obsId); - deletedObservationIds.push(obsId); - deleted++; + if (obs) { + await kv.delete(KV.observations(data.sessionId), obsId); + if (obs.imageData) await decrementImageRef(kv, sdk, obs.imageData); + if (obs.imageRef && obs.imageRef !== obs.imageData) { + await decrementImageRef(kv, sdk, obs.imageRef); + } + getSearchIndex().remove(obsId); + vectorIndexRemove(obsId); + deletedObservationIds.push(obsId); + deleted++; + } } }Also applies to: 261-267
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/remember.ts` around lines 182 - 214, The code increments deletion counters and appends IDs (deleted, deletedMemoryIds, deletedObservationIds, memoriesDeleted, observationsDeleted) even when KV entries don't exist; change the logic in the mem deletion block (where you call kv.get<Memory>(KV.memories, data.memoryId), decrementImageRef, deleteAccessLog, getSearchIndex().remove, vectorIndexRemove) to only perform delete, index removal, ref-decrementing, push to deletedMemoryIds and increment deleted and memoriesDeleted if mem is truthy (exists); likewise, in the observations loop (where you call kv.get(..., obsId), kv.delete, decrementImageRef, getSearchIndex().remove, vectorIndexRemove) only perform kv.delete side-effects, push obsId into deletedObservationIds, and increment deleted and observationsDeleted when obs was found, and adjust the imageRef vs imageData decrement logic to run only when obs exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin/skills/forget/SKILL.md`:
- Around line 20-23: The SKILL.md text for memory_governance_delete is
inaccurate: update the description for the memory_governance_delete call
(referenced by name) to state that the runtime expects a comma-separated string
for memoryIds (not an array) and that the backend default reason is "manual
deletion" (not "plugin skill request"); update any examples or parameter notes
in SKILL.md that reference memoryIds or reason to match this runtime contract.
In `@src/mcp/server.ts`:
- Around line 624-643: The handler currently normalizes malformed observationIds
to [] which gets omitted from the mem::forget payload and can cause
whole-session deletion; update the validation around
asNonEmptyString(args.sessionId), asNonEmptyString(args.memoryId) and
parseCsvList(args.observationIds) so that if args.observationIds is present you
validate its raw type (typeof args.observationIds === "string"), parse it with
parseCsvList, then ensure the result is a non-empty array of non-empty strings
(each entry typeof === "string" && entry.trim().length>0); if this validation
fails return a 400 with an explicit error like "malformed observationIds" and do
not call sdk.trigger({ function_id: "mem::forget", ...}) without observationIds
— only include observationIds in the trigger payload when they passed
validation.
---
Outside diff comments:
In `@README.md`:
- Around line 892-918: Update the MCP tool count labels to match the new 54-tool
total: change the "### 53 Tools" heading to "### 54 Tools" and update the
"<summary>Extended tools (53 total — set AGENTMEMORY_TOOLS=all)</summary>" text
to reflect "54 total" (and search for any other occurrences of "53 Tools", "53
total", or "MCP tools" in the README and replace them with "54"/"54 total" to
keep counts consistent).
In `@src/functions/remember.ts`:
- Around line 182-214: The code increments deletion counters and appends IDs
(deleted, deletedMemoryIds, deletedObservationIds, memoriesDeleted,
observationsDeleted) even when KV entries don't exist; change the logic in the
mem deletion block (where you call kv.get<Memory>(KV.memories, data.memoryId),
decrementImageRef, deleteAccessLog, getSearchIndex().remove, vectorIndexRemove)
to only perform delete, index removal, ref-decrementing, push to
deletedMemoryIds and increment deleted and memoriesDeleted if mem is truthy
(exists); likewise, in the observations loop (where you call kv.get(..., obsId),
kv.delete, decrementImageRef, getSearchIndex().remove, vectorIndexRemove) only
perform kv.delete side-effects, push obsId into deletedObservationIds, and
increment deleted and observationsDeleted when obs was found, and adjust the
imageRef vs imageData decrement logic to run only when obs exists.
🪄 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: 991419c0-cef4-4d0c-8250-f9a4b4c4f95c
⛔ Files ignored due to path filters (2)
assets/tags/light/stat-tools.svgis excluded by!**/*.svgassets/tags/stat-tools.svgis excluded by!**/*.svg
📒 Files selected for processing (15)
AGENTS.mdREADME.mdplugin/.claude-plugin/plugin.jsonplugin/.codex-plugin/plugin.jsonplugin/opencode/README.mdplugin/opencode/agentmemory-capture.tsplugin/plugin.jsonplugin/skills/forget/SKILL.mdsrc/functions/governance.tssrc/functions/remember.tssrc/mcp/server.tssrc/mcp/tools-registry.tstest/governance.test.tstest/mcp-forget-tool.test.tstest/mcp-standalone.test.ts
Comment on lines
+20
to
23
| - **Saved memories** (`mem_*` IDs) → call `memory_governance_delete` with: | ||
| - `memoryIds: [<id>, ...]` — an array (or comma-separated string) of memory IDs | ||
| - `reason: "<short reason>"` — optional, defaults to `"plugin skill request"` | ||
|
|
Contributor
There was a problem hiding this comment.
Align governance-delete instructions with the actual runtime contract.
This text currently says memoryIds can be an array and that reason defaults to "plugin skill request". In current runtime behavior, MCP memory_governance_delete expects a comma-separated string in this path, and backend default reason is "manual deletion".
Proposed wording fix
- - **Saved memories** (`mem_*` IDs) → call `memory_governance_delete` with:
- - `memoryIds: [<id>, ...]` — an array (or comma-separated string) of memory IDs
- - `reason: "<short reason>"` — optional, defaults to `"plugin skill request"`
+ - **Saved memories** (`mem_*` IDs) → call `memory_governance_delete` with:
+ - `memoryIds: "<id_1>,<id_2>"` — comma-separated memory IDs
+ - `reason: "<short reason>"` — optional, defaults to `"manual deletion"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugin/skills/forget/SKILL.md` around lines 20 - 23, The SKILL.md text for
memory_governance_delete is inaccurate: update the description for the
memory_governance_delete call (referenced by name) to state that the runtime
expects a comma-separated string for memoryIds (not an array) and that the
backend default reason is "manual deletion" (not "plugin skill request"); update
any examples or parameter notes in SKILL.md that reference memoryIds or reason
to match this runtime contract.
Comment on lines
+624
to
+643
| const sessionId = asNonEmptyString(args.sessionId); | ||
| const memoryId = asNonEmptyString(args.memoryId); | ||
| const observationIds = parseCsvList(args.observationIds); | ||
| if (observationIds.length > 0 && !sessionId) { | ||
| return { | ||
| status_code: 400, | ||
| body: { error: "observationIds requires sessionId" }, | ||
| }; | ||
| } | ||
| if (!sessionId && !memoryId) { | ||
| return { | ||
| status_code: 400, | ||
| body: { error: "sessionId or memoryId is required" }, | ||
| }; | ||
| } | ||
| try { | ||
| const result = await sdk.trigger({ function_id: "mem::forget", payload: { | ||
| ...(sessionId ? { sessionId } : {}), | ||
| ...(observationIds.length > 0 ? { observationIds } : {}), | ||
| ...(memoryId ? { memoryId } : {}), |
Contributor
There was a problem hiding this comment.
Reject malformed observationIds to avoid accidental full-session deletion.
At Line 626, malformed observationIds (e.g., non-string array entries or empty CSV payload) are normalized to [], and then omitted from the trigger payload. With a sessionId, that can unintentionally invoke whole-session deletion semantics in mem::forget instead of targeted observation deletion.
Proposed fix
case "memory_forget": {
const sessionId = asNonEmptyString(args.sessionId);
const memoryId = asNonEmptyString(args.memoryId);
- const observationIds = parseCsvList(args.observationIds);
+ const hasObservationIds = args.observationIds !== undefined;
+ if (
+ hasObservationIds &&
+ typeof args.observationIds !== "string" &&
+ !Array.isArray(args.observationIds)
+ ) {
+ return {
+ status_code: 400,
+ body: { error: "observationIds must be a comma-separated string or string[]" },
+ };
+ }
+ if (
+ Array.isArray(args.observationIds) &&
+ args.observationIds.some((v) => typeof v !== "string" || !v.trim())
+ ) {
+ return {
+ status_code: 400,
+ body: { error: "observationIds array must contain non-empty strings" },
+ };
+ }
+ const observationIds = parseCsvList(args.observationIds);
+ if (hasObservationIds && observationIds.length === 0) {
+ return {
+ status_code: 400,
+ body: { error: "observationIds must contain at least one ID" },
+ };
+ }
if (observationIds.length > 0 && !sessionId) {
return {
status_code: 400,
body: { error: "observationIds requires sessionId" },
};As per coding guidelines: MCP tool handlers must validate arguments with typeof checks and parse CSV args in a controlled way before triggering functions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp/server.ts` around lines 624 - 643, The handler currently normalizes
malformed observationIds to [] which gets omitted from the mem::forget payload
and can cause whole-session deletion; update the validation around
asNonEmptyString(args.sessionId), asNonEmptyString(args.memoryId) and
parseCsvList(args.observationIds) so that if args.observationIds is present you
validate its raw type (typeof args.observationIds === "string"), parse it with
parseCsvList, then ensure the result is a non-empty array of non-empty strings
(each entry typeof === "string" && entry.trim().length>0); if this validation
fails return a 400 with an explicit error like "malformed observationIds" and do
not call sdk.trigger({ function_id: "mem::forget", ...}) without observationIds
— only include observationIds in the trigger payload when they passed
validation.
Source: Coding guidelines
Contributor
Author
|
Closing — it's gone stale and picked up conflicts in the meantime. Can reopen if it's still useful. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #833
Problem
The
forgetskill (and any MCP-only client) has no way to delete observations or sessions:memory_governance_deleteonly reads/writes theKV.memoriesstore. Observation IDs (obs_*) never exist there, so the skill's documented happy path is a guaranteed no-op.{ "deleted": 0, "success": true }for that no-op, so users were told their data was removed while the.binfiles undermem:obs:<sessionId>stayed fully intact.mem::forgethandles memories, per-session observation lists, and whole-session deletion (record + summary) while keeping the BM25/vector indexes and audit log consistent — but it was only reachable over REST (POST /agentmemory/forget), not via MCP.Solution
memory_forgetMCP tool mapping tomem::forget(registry +mcp::tools::callcase). AcceptsmemoryId, orsessionIdwith optional comma-separated/arrayobservationIds; baresessionIddeletes all of the session's observations plus the session record and summary, matching the REST behavior. Validation mirrors the REST route (sessionId or memoryId is required) plus a specific error forobservationIdswithoutsessionId.mem::forgetnow returns its breakdown (memoriesDeleted,observationsDeleted,sessionDeleted) — it already tracked these for the audit row; returning them lets callers confirm to the user what was actually removed.mem::governance-deletestops reporting no-ops as success: IDs that don't exist in the memories store are returned in anotFoundarray andsuccessis onlytruewhen every requested ID was deleted. The audit row recordsnotFoundtoo.forgetskill + OpenCode instructions rewritten to route observation/session deletion throughmemory_forgetand keepmemory_governance_deletefor saved memories (mem_*), removing the incorrect "deletes by memory ID only" workaround that never worked.Tool count badges/docs/manifests bumped 53 → 54 to keep
test/consistency.test.tsgreen.Testing
test/mcp-forget-tool.test.ts: registry listing, validation errors, payload forwarding for all three deletion shapes (CSV + arrayobservationIds), and the error path.test/governance.test.ts: non-existent IDs now expectsuccess: false+notFound; added partial-delete and all-deleted cases.npm run buildclean.Summary by CodeRabbit
New Features
Documentation