fix(memory): honest memory_forget reporting + lesson delete path - #1132
Merged
rohitg00 merged 8 commits intoAug 2, 2026
Merged
Conversation
Calling mem::forget with a lesson id (lsn_*) deleted a nonexistent key
from the memories keyspace, counted it, and reported success. Guard the
delete, index cleanup, and counter on the kv.get result, matching the
mem::governance-delete pattern, so nonexistent ids return
{ success: true, deleted: 0 } with no audit row. Closes rohitg00#1120.
Register mem::lesson-delete to set deleted: true on a lesson, mirroring the lesson-strengthen existence guard and audit pattern. Read paths already filter !l.deleted, and re-saving deleted content creates a fresh lesson. Adds lesson_delete to the audit operation union.
Wire mem::lesson-delete through the MCP tool registry and dispatch case (memory_lesson_delete) and a POST /agentmemory/lessons/delete REST route with 400 for a missing lessonId and 404 for a nonexistent lesson.
Adds memory_lesson_delete to the registry, so update every count surface: tool-count test, README badge and prose, AGENTS.md stats, INSTALL_FOR_AGENTS.md, plugin manifests and docs, and the two code comments this change makes stale. REST endpoint count goes 128 to 129 for the new /agentmemory/lessons/delete route.
Cast the lesson-delete trigger result once instead of twice inline, and restore the lastDecayedAt incremental-delta decay test that was dropped when the lesson-delete describe block was added.
Review fixes: the lesson-delete REST route now returns the repo-standard
{ error: 'lesson not found' } body on 404 instead of the function-shaped
{ success: false } payload, matching api::memory-by-id. Regenerated the
autogen MCP and REST skill references so memory_lesson_delete and the
lessons/delete route appear in the tables with accurate counts.
|
@SomSamantray 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe change adds soft deletion for lessons through internal, MCP, and REST interfaces. It adds audit support and regression coverage. It also corrects nonexistent-memory deletion accounting and updates MCP and REST surface documentation. ChangesLesson deletion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant RESTClient
participant memory_lesson_delete
participant api_lesson_delete
participant mem_lesson_delete
MCPClient->>memory_lesson_delete: submit lessonId
memory_lesson_delete->>mem_lesson_delete: invoke deletion
RESTClient->>api_lesson_delete: POST lessonId
api_lesson_delete->>mem_lesson_delete: invoke deletion
mem_lesson_delete-->>memory_lesson_delete: return serialized result
mem_lesson_delete-->>api_lesson_delete: return result or not found
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 3
🧹 Nitpick comments (1)
test/remember-forget-audit.test.ts (1)
125-154: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the no-op path has no cleanup side effects.
The tests verify
deleted: 0and no audit row. They do not verify thatkv.deleteor search-index cleanup is skipped. A future regression could perform those side effects and still pass these tests. Add collaborator assertions for the nonexistentmemoryIdpath.🤖 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 `@test/remember-forget-audit.test.ts` around lines 125 - 154, Extend the nonexistent-memory tests around registerRememberFunction and mem::forget to assert that kv.delete and search-index cleanup collaborators are not called for the missing memoryId. Preserve the existing deleted: 0 result and empty mem:audit assertions while covering both cleanup side effects.
🤖 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 `@src/mcp/server.ts`:
- Around line 1129-1131: Normalize lesson IDs consistently at both entry points:
in src/mcp/server.ts lines 1129-1131, trim args.lessonId once and pass the
normalized value to the mem::lesson-delete trigger; in src/triggers/api.ts lines
3156-3158, trim the request lesson ID before validating emptiness and pass only
the normalized value onward.
- Around line 1125-1127: Update the memory_lesson_delete handler’s argument
validation before accessing args.lessonId so null or otherwise invalid arguments
return status_code 400 with a validation error. Preserve the existing required,
non-blank lessonId check for valid object arguments, and ensure arguments: null
cannot reach property access or the outer 500 path.
In `@test/lessons.test.ts`:
- Around line 360-363: Remove the unmatched duplicate type assertion from each
SDK trigger result in test/lessons.test.ts at lines 360-363, 396-399, 405-408,
and 430-433, leaving one valid assertion per result so the test file parses
correctly.
---
Nitpick comments:
In `@test/remember-forget-audit.test.ts`:
- Around line 125-154: Extend the nonexistent-memory tests around
registerRememberFunction and mem::forget to assert that kv.delete and
search-index cleanup collaborators are not called for the missing memoryId.
Preserve the existing deleted: 0 result and empty mem:audit assertions while
covering both cleanup side effects.
🪄 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 Plus
Run ID: c981df68-116f-4788-99af-2c13e66e2d44
⛔ 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 (20)
AGENTS.mdINSTALL_FOR_AGENTS.mdREADME.mdplugin/.claude-plugin/plugin.jsonplugin/.codex-plugin/plugin.jsonplugin/opencode/README.mdplugin/plugin.jsonplugin/skills/agentmemory-mcp-tools/REFERENCE.mdplugin/skills/agentmemory-rest-api/REFERENCE.mdsrc/functions/lessons.tssrc/functions/remember.tssrc/index.tssrc/mcp/server.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tssrc/triggers/api.tssrc/types.tstest/lessons.test.tstest/remember-forget-audit.test.tstest/tool-count-consistency.test.ts
Address CodeRabbit review: trim lessonId once at both the MCP dispatch and REST route before triggering mem::lesson-delete (whitespace-padded ids previously 404'd or looked up raw), and extend the nonexistent- memoryId regression test to assert the no-op path performs no kv.delete and no search-index cleanup.
rohitg00
added a commit
that referenced
this pull request
Aug 2, 2026
Version trio + plugin manifests + supportedVersions + ExportData union bumped to 0.9.29; CHANGELOG entry covering everything since v0.9.28 with upgrade notes for the four visible behavior changes. Fixes the endpoint-count drift on main (130 registered routes vs docs saying 129 after #1132 landed in parallel with #1136). Project-scope parity: OpenCode plugin, Hermes plugin, Pi extension, and JSONL replay now resolve project the same way the hooks do (env override, git toplevel basename, cwd basename) instead of sending raw filesystem paths, closing #903 and #1135 and pre-empting the same bug in pi. The filesystem watcher accepts AGENTMEMORY_PROJECT_NAME with the old AGENTMEMORY_PROJECT kept as a deprecated alias, replay handles Windows-recorded paths, and OpenCode file enrichment matches the agent's lowercase tool names (the capitalized set never matched). Tests: opencode fallback expectations updated to basenames per the canonicalization, git-toplevel resolution covered with a fixture repo, new project-scope-parity suite for replay and fs-watcher.
This was referenced Aug 3, 2026
This was referenced Aug 3, 2026
berthojoris
added a commit
to berthojoris/agentmemory
that referenced
this pull request
Aug 3, 2026
`npm run skills:check` is a separate CI gate from the consistency test and fails on main for the same root cause: 5023cf3 (rohitg00#1132) registered a new endpoint without running `npm run skills:gen`, leaving the AUTOGEN:rest block in plugin/skills/agentmemory-rest-api/REFERENCE.md at 118 registered endpoints. Only the rest-api reference has a content change; the other four REFERENCE.md files regenerate byte-identical. Signed-off-by: Bertho Joris <bertho_joris@yahoo.co.id>
rohitg00
added a commit
that referenced
this pull request
Aug 9, 2026
…aces (#1141) * chore(release): v0.9.29 with project-scope parity across surfaces Version trio + plugin manifests + supportedVersions + ExportData union bumped to 0.9.29; CHANGELOG entry covering everything since v0.9.28 with upgrade notes for the four visible behavior changes. Fixes the endpoint-count drift on main (130 registered routes vs docs saying 129 after #1132 landed in parallel with #1136). Project-scope parity: OpenCode plugin, Hermes plugin, Pi extension, and JSONL replay now resolve project the same way the hooks do (env override, git toplevel basename, cwd basename) instead of sending raw filesystem paths, closing #903 and #1135 and pre-empting the same bug in pi. The filesystem watcher accepts AGENTMEMORY_PROJECT_NAME with the old AGENTMEMORY_PROJECT kept as a deprecated alias, replay handles Windows-recorded paths, and OpenCode file enrichment matches the agent's lowercase tool names (the capitalized set never matched). Tests: opencode fallback expectations updated to basenames per the canonicalization, git-toplevel resolution covered with a fixture repo, new project-scope-parity suite for replay and fs-watcher. * fix(release): review findings, git-toplevel parity, doc counts - skills generator dedupes routes on method plus path, so the REST reference lists all 130 registered routes instead of hiding the second method on ten dual-method paths (header said 119) - fs-watcher trims AGENTMEMORY_PROJECT_NAME and the deprecated alias, treating whitespace as unset, and derives the git toplevel basename when watching a subdirectory - replay resolves the git toplevel basename when the recorded cwd still exists locally (memoized per cwd), keeping the basename fallback for historical or cross-platform paths; no env override here since a bulk import spans many projects - parity tests for replay git-root resolution, watcher git-root and trim behavior - stat-tests badge updated from 1428+ to 1550+ passing * fix(cli): refuse second-instance boot over a live daemon Closes the class behind issue 1140: agentmemory consolidate (or any unrecognized word) fell through the command table into the full server boot, registering a duplicate worker on the running engine; on iii 0.11.2 the second instance's shutdown tears down the daemon's HTTP trigger routing until a full engine restart. Unknown subcommands now error with the supported list, and main() probes livez on the resolved port and refuses to boot over a live daemon, so multi-instance setups on other ports are unaffected. Verified behaviorally against the built CLI: both paths refuse with exit 1. Also from review: the watcher stamps each event with its own root's project via a per-root map (an explicit config.project still overrides for every root), and replay only accepts a non-empty string cwd from parsed JSONL so malformed entries cannot reach the filesystem probe. * test(watcher): two-repository flush events scope to their own project * chore(release): bump packages/mcp, guard it, refresh CONTRIBUTING packages/mcp was still 0.9.28 after the release bump because nothing guarded it; a consistency test now pins it to package.json. CONTRIBUTING release list corrected to the files a bump actually touches (no tracked lockfile, the two extra plugin manifests, the export test derives from VERSION now), and the subsystems table gains src/cli, integrations/pi, and the generated-manifest note. * fix(export): refuse over-frame export instead of dropping the worker Closes the availability bug in issue 1142: GET /agentmemory/export assembles the full store and returns it through sdk.trigger, so a store whose serialized export passes the engine's 16 MiB WebSocket frame (tungstenite max_frame_size, not raisable under the 0.11.2 pin) dies on the worker->engine hop, drops the worker, and 404s every endpoint for ~1s. The session collections page on maxSessions/offset but ~18 others do not, so a large store hits this at any parameter combination. A shared frame-guard measures the serialized size before returning: mem::export returns a small oversized error instead of the giant object, and api::mesh-export returns 413 (same dead-end as #890). Either way the over-frame payload never crosses the boundary, so the daemon stays up and the failure is one clean request with a hint to narrow the range. Full pagination of the non-session collections is a follow-up. Layer 1 of the fix; verified with a synthetic oversized export returning the error object (tiny) rather than the payload. * ci: collapse to a single npm install to fix Node 24/26 CI The two-step install (npm install --package-lock-only then npm ci) failed only on the Node 24/26 matrix rows: their stricter npm rejects rolldown's optional platform bindings (@rolldown/binding-android-arm64) that a --package-lock-only pass does not fully enumerate. Lockfiles are gitignored, so npm ci re-validation buys no reproducibility here. A single lenient npm install resolves and installs in one pass. * fix(mesh): scope exported memories by project like actions api::mesh-export filtered actions by ?project but returned every project's memories. On a mesh instance federating one project to a peer, the peer pulled other projects' memories (cross-project leak), and those extras could push the payload past the 16 MiB transport frame into a 413 even when the requested project's own slice fit. Memories carry the same optional project field as actions, so filter both before the frame-size guard runs. Adds a regression test asserting a project-scoped export excludes other projects' memories and that an oversized memory in another project no longer 413s the scoped request. * chore(release): credit the Antigravity native hooks adapter in 0.9.29 notes * chore(release): sweep stale 0.9.28 refs for 0.9.29 Deploy Dockerfiles/compose/render pins, AGENTS.md stats header, opencode plugin manifest, website meta snapshot, test-count claims (1,428 -> 1,596) in README/AGENTS/stat SVGs, and the missing 0.9.29 CHANGELOG compare link. * chore(release): sync stat-tests badge to 1596+ and commit bridge exec bit * refactor: trim frame-guard comments and drop issue refs from code
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
2 participants
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.
Summary
Calling
memory_forgetwith a lesson id (lsn_*) reported{deleted: 1, success: true}while deleting nothing — the lesson survived recall.mem::forgetdeleted and counted unconditionally whenever amemoryIdwas present, without checking whether a memory actually existed in the memories keyspace. It now guards the delete, index cleanup, and counter on thekv.getresult (matchingmem::governance-delete), so a nonexistent or lesson id honestly returns{success: true, deleted: 0}with no audit row.Lessons also had no delete path at all: the
Lessontype carries adeletedflag honored by save/recall/list/decay-sweep, but nothing ever set it. This addsmem::lesson-delete(soft-delete by id, mirroring thelesson-strengthenguard and audit pattern), exposed as amemory_lesson_deleteMCP tool and aPOST /agentmemory/lessons/deleteREST route (400 for a missing id, 404 for a nonexistent lesson). Re-saving the content of a deleted lesson creates a fresh lesson, giving users a repair path for malformed fingerprint-id lessons.Fixes #1120. Also resolves the deletion half of #945 (no way to delete/cleanup lessons).
Changes
mem::forgetmemoryId branch: existence-guarded delete/count, matchingmem::governance-deletemem::lesson-deletesoft-delete function +lesson_deleteaudit operationmemory_lesson_deleteMCP tool (registry + server dispatch)api::lesson-deleteREST routeTesting
test/remember-forget-audit.test.ts: regression cases for nonexistent/lessonmemoryIdreturningdeleted: 0with no audit rowtest/lessons.test.ts:mem::lesson-deletesoft-delete, recall/list exclusion, not-found for already-deleted/nonexistent, re-save creates fresh lesson, audit rowtest/tool-count-consistency.test.ts:EXPECTED_TOOL_COUNT53 → 54npm test: 1,432 passing; the 8 failures are pre-existing environment-dependent tests (missing@huggingface/transformerspackage, git-repo-state tests) present on the clean baselinenpm run build: succeedsnpm run skills:check: passes (15 skills)Notes
The
mem::forgetobservation/session branches retain their unconditional shape; that keyspace family is tracked separately in #833. Soft-deleted lessons are excluded from recall/list; tombstone pruning, replay-merge resurrection, and export/import handling of the deleted flag are follow-up work.Post-Deploy Monitoring & Validation
No additional operational monitoring required — this is internal function/tool work with unit-level coverage; the observable contract change is the honest
deleted: 0for nonexistent ids.Summary by CodeRabbit