fix(audit): add audit coverage for mem::forget + document policy#162
Merged
fix(audit): add audit coverage for mem::forget + document policy#162
Conversation
Addresses issue #125's policy question. Decision adopted (hybrid of Options A and C): - EVERY structural deletion must call recordAudit. Silent deletes are forbidden. - Scoped deletions (governance-delete, forget) emit one audit row per call with targetIds populated. - Bulk sweeps (retention-evict, evict, auto-forget) emit one batched audit row per invocation with targetIds listing every removed id, to avoid flooding the log during routine sweeps. Implementation gap closed: mem::forget previously deleted memories, observations, sessions, and summaries without ever calling recordAudit. Added a single batched audit row per invocation with sessionId, counts by type, and sessionDeleted flag. Policy comment added to src/functions/audit.ts so new deletion sites have an explicit precedent instead of discovering it via review cycles. Test coverage (test/remember-forget-audit.test.ts): - Single-memory forget: one audit row, targetIds=[memId], forget op. - Whole-session forget: one audit row batching all observation ids, sessionDeleted=true, deleted count sums memories+observations+2. - No-op forget (neither memoryId nor sessionId): no audit row. All 769 existing tests still pass.
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds an audit policy comment to Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as SDK (trigger)
participant Func as mem::forget
participant KV as KV Store
participant Audit as recordAudit -> KV[mem:audit]
SDK->>Func: invoke mem::forget(payload)
Func->>KV: read session/memory/observation keys
alt deletions found
Func->>KV: delete memory/observation/session keys
Func->>Audit: recordAudit(operation="forget", targetIds, details)
Audit->>KV: write mem:audit entry
else no deletions
Func-->>SDK: return { deleted: 0 }
end
Func-->>SDK: return { success: true, deleted }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/functions/remember.ts (1)
130-183:⚠️ Potential issue | 🟠 MajorBuild and audit a verified deletion plan before mutating KV.
Line 168 writes the audit row after Lines 130-163 have already deleted data, so an audit write failure leaves an unaudited structural deletion. The counters also track delete attempts, not confirmed rows:
{ sessionId: "missing" }still setssessionDeleted=true, incrementsdeletedby 2, and emits an audit row. First read/plan the existing rows to delete, include the actual structural targets/counts, callrecordAudit, then perform thekv.delete(...)calls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/functions/remember.ts` around lines 130 - 183, Currently deletion calls (KV.memories, deleteAccessLog, KV.observations, KV.sessions, KV.summaries) happen before writing the audit via recordAudit, and counters reflect attempted deletes rather than verified existing targets. First scan/read the KV namespaces to build a verified deletion plan: check existence of memoryId via KV.memories and access log via deleteAccessLog, list KV.observations(sessionId) to collect actual observation ids, and check existence of KV.sessions and KV.summaries to determine session deletion; populate deletedMemoryIds, deletedObservationIds, deletedSession, and deleted counts from these reads only. Call recordAudit(kv, "forget", "mem::forget", [...deletedMemoryIds, ...deletedObservationIds], { sessionId, deleted, memoriesDeleted, observationsDeleted, sessionDeleted, reason: "user-initiated forget" }) to log the planned structural deletions, and only after a successful audit write perform the kv.delete(...) and deleteAccessLog(...) operations using the same verified id lists.
🧹 Nitpick comments (2)
test/remember-forget-audit.test.ts (2)
1-12: Mockiii-sdkin this unit test.
remember.tsimportsTriggerActionfromiii-sdk, so this suite currently loads the real SDK while testing local forget behavior. Add a minimal module mock before importingregisterRememberFunction.Suggested mock
vi.mock("../src/state/keyed-mutex.js", () => ({ withKeyedLock: <T>(_key: string, fn: () => Promise<T>) => fn(), })); + +vi.mock("iii-sdk", () => ({ + TriggerAction: { Void: vi.fn(() => ({ type: "void" })) }, +})); import { registerRememberFunction } from "../src/functions/remember.js";As per coding guidelines,
test/**/*.test.ts:Mock pattern for tests: use vi.mock("iii-sdk") with mock sdk.trigger, kv.get/set/list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/remember-forget-audit.test.ts` around lines 1 - 12, The test loads the real iii-sdk because remember.ts imports TriggerAction; add a vi.mock("iii-sdk", () => ({ sdk: { trigger: vi.fn() }, kv: { get: vi.fn(), set: vi.fn(), list: vi.fn() }, TriggerAction: { /* minimal shape if used by remember.ts */ } })) call at the top of test/remember-forget-audit.test.ts before importing registerRememberFunction so the test uses the mocked iii-sdk (mock sdk.trigger and kv.get/set/list) instead of the real module; keep the mock placement above the line that imports registerRememberFunction.
106-118: Cover missing IDs as no-op forgets too.This test only covers undefined identifiers. Add cases for
{ memoryId: "missing" }and{ sessionId: "missing" }; those exercise the delete branches and would catch false positive audit rows/counts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/remember-forget-audit.test.ts` around lines 106 - 118, Add two additional test cases to test/remember-forget-audit.test.ts that call sdk.trigger for the "mem::forget" function with payloads { memoryId: "missing" } and { sessionId: "missing" } (keeping the other id undefined) to ensure deletes of non-existent keys are treated as no-ops and do not emit audit rows; after each trigger, call kv.list("mem:audit") and assert length remains 0. Reuse the existing mockSdk(), mockKV(), and registerRememberFunction(sdk, kv) setup so the new cases exercise the same delete branches in the mem::forget handler and will catch false-positive audit rows/counts.
🤖 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/functions/audit.ts`:
- Around line 24-27: The comment for the operation field incorrectly groups
"mem::auto-forget" as user-initiated; update the wording in
src/functions/audit.ts so "mem::forget" remains labeled as user-initiated while
"mem::auto-forget" is described as an automatic sweep (not user-initiated), and
ensure the doc references AuditEntry["operation"] and any guidance about
choosing the correct reason/audit shape so callers won't copy the wrong
operation.
---
Outside diff comments:
In `@src/functions/remember.ts`:
- Around line 130-183: Currently deletion calls (KV.memories, deleteAccessLog,
KV.observations, KV.sessions, KV.summaries) happen before writing the audit via
recordAudit, and counters reflect attempted deletes rather than verified
existing targets. First scan/read the KV namespaces to build a verified deletion
plan: check existence of memoryId via KV.memories and access log via
deleteAccessLog, list KV.observations(sessionId) to collect actual observation
ids, and check existence of KV.sessions and KV.summaries to determine session
deletion; populate deletedMemoryIds, deletedObservationIds, deletedSession, and
deleted counts from these reads only. Call recordAudit(kv, "forget",
"mem::forget", [...deletedMemoryIds, ...deletedObservationIds], { sessionId,
deleted, memoriesDeleted, observationsDeleted, sessionDeleted, reason:
"user-initiated forget" }) to log the planned structural deletions, and only
after a successful audit write perform the kv.delete(...) and
deleteAccessLog(...) operations using the same verified id lists.
---
Nitpick comments:
In `@test/remember-forget-audit.test.ts`:
- Around line 1-12: The test loads the real iii-sdk because remember.ts imports
TriggerAction; add a vi.mock("iii-sdk", () => ({ sdk: { trigger: vi.fn() }, kv:
{ get: vi.fn(), set: vi.fn(), list: vi.fn() }, TriggerAction: { /* minimal shape
if used by remember.ts */ } })) call at the top of
test/remember-forget-audit.test.ts before importing registerRememberFunction so
the test uses the mocked iii-sdk (mock sdk.trigger and kv.get/set/list) instead
of the real module; keep the mock placement above the line that imports
registerRememberFunction.
- Around line 106-118: Add two additional test cases to
test/remember-forget-audit.test.ts that call sdk.trigger for the "mem::forget"
function with payloads { memoryId: "missing" } and { sessionId: "missing" }
(keeping the other id undefined) to ensure deletes of non-existent keys are
treated as no-ops and do not emit audit rows; after each trigger, call
kv.list("mem:audit") and assert length remains 0. Reuse the existing mockSdk(),
mockKV(), and registerRememberFunction(sdk, kv) setup so the new cases exercise
the same delete branches in the mem::forget handler and will catch
false-positive audit rows/counts.
🪄 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: f72a9490-79ba-4029-86b6-21d19bea2cf5
📒 Files selected for processing (3)
src/functions/audit.tssrc/functions/remember.tstest/remember-forget-audit.test.ts
rohitg00
added a commit
that referenced
this pull request
Apr 18, 2026
Bump version + ship CHANGELOG covering everything that merged since v0.8.13: - #118 security advisory drafts for v0.8.2 CVEs - #132 semantic eviction routing + batched retention audit - #157 iii console docs + vendored screenshots in README - #160 (#158) health gated on RSS floor - #161 (#159) standalone MCP proxies to the running server - #162 (#125) mem::forget audit coverage + policy doc - #163 (#62) @agentmemory/fs-watcher filesystem connector - #164 Next.js website (website/ root, ship to Vercel) Version bumps (8 files): - package.json / package-lock.json (top + packages['']) - plugin/.claude-plugin/plugin.json - packages/mcp/package.json (self + ~0.9.0 dep pin) - src/version.ts (union extended, assigned 0.9.0) - src/types.ts (ExportData.version union) - src/functions/export-import.ts (supportedVersions set) - test/export-import.test.ts (export assertion) Tests: 777 passing. Build clean.
Merged
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 #125.
Decision
Adopt a hybrid of the proposed options:
recordAudit. Silent deletes are forbidden.targetIdspopulated.targetIdslisting every removed id, to avoid flooding the log during routine sweeps.This matches what just landed in #132 (retention-evict batched audit) and what evict.ts and auto-forget.ts already do for scoped cases, so no churn to those files — the codebase already points at this shape.
Gap closed
mem::forgetdeleted memories, observations, sessions, and summaries without anyrecordAuditcall. User-initiated forget is a scoped deletion and should always leave a trail. Added one batched audit row per invocation withsessionId, counts by type,sessionDeletedflag, andreason: "user-initiated forget".Policy doc
Added a top-of-file comment block to
src/functions/audit.tsspelling out the two shapes and the operation-field convention. New deletion sites now have an explicit precedent.Tests
test/remember-forget-audit.test.ts:targetIds=[memId],operation=forget.targetIds,sessionDeleted=true,deletedcount sums everything.memoryIdandsessionId) → no audit row emitted.All 769 existing tests still pass.
Summary by CodeRabbit
New Features
Documentation
Tests