fix(audit): make index_persist audit entries opt-in - #1182
Conversation
Signed-off-by: Dmitrii Zhukov <dmitry0983@gmail.com>
|
@dmazhukov is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughIndex persistence auditing is now opt-in through ChangesIndex persistence audit gating
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/index-persistence.test.ts (1)
825-836: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover all accepted environment values.
auditIndexPersistEnabled()accepts trimmed, case-insensitive"1"and"true". This test covers only"1". Add cases for"true"," TRUE ", and rejected values such as"0"and"yes".🤖 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/index-persistence.test.ts` around lines 825 - 836, Expand the audit index-persistence test around auditIndexPersistEnabled() to cover accepted values "true" and whitespace-padded, case-insensitive " TRUE ", in addition to "1". Add rejected-value cases for "0" and "yes", asserting enabled values produce index_persist entries and rejected values do not.
🤖 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 `@test/index-persistence.test.ts`:
- Around line 798-806: Update the test suite’s beforeEach and afterEach hooks to
isolate AGENTMEMORY_AUDIT_INDEX_PERSIST: capture its original value before
clearing it in beforeEach, then restore that captured value in afterEach instead
of always deleting the variable. Keep fake-timer setup and teardown unchanged.
---
Nitpick comments:
In `@test/index-persistence.test.ts`:
- Around line 825-836: Expand the audit index-persistence test around
auditIndexPersistEnabled() to cover accepted values "true" and
whitespace-padded, case-insensitive " TRUE ", in addition to "1". Add
rejected-value cases for "0" and "yes", asserting enabled values produce
index_persist entries and rejected values do not.
🪄 Autofix
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: 41422e79-0301-4120-841f-d2b89a8941a9
📒 Files selected for processing (3)
plugin/skills/agentmemory-config/REFERENCE.mdsrc/state/index-persistence.tstest/index-persistence.test.ts
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| kv = mockKV(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.useRealTimers(); | ||
| delete process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reset the audit flag before each test.
If the test process starts with AGENTMEMORY_AUDIT_INDEX_PERSIST=1 or true, the default-disabled test at Lines 813-823 and the audit-off round-trip at Lines 838-850 run with auditing enabled. afterEach also deletes any value that existed before this suite. Capture and restore the original value, and clear the variable in beforeEach.
Proposed isolation fix
let kv: ReturnType<typeof mockKV>;
+ let previousAuditFlag: string | undefined;
beforeEach(() => {
+ previousAuditFlag = process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST;
+ delete process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST;
vi.useFakeTimers();
kv = mockKV();
});
afterEach(() => {
vi.useRealTimers();
- delete process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST;
+ if (previousAuditFlag === undefined) {
+ delete process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST;
+ } else {
+ process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST = previousAuditFlag;
+ }
});📝 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.
| beforeEach(() => { | |
| vi.useFakeTimers(); | |
| kv = mockKV(); | |
| }); | |
| afterEach(() => { | |
| vi.useRealTimers(); | |
| delete process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST; | |
| }); | |
| let kv: ReturnType<typeof mockKV>; | |
| let previousAuditFlag: string | undefined; | |
| beforeEach(() => { | |
| previousAuditFlag = process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST; | |
| delete process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST; | |
| vi.useFakeTimers(); | |
| kv = mockKV(); | |
| }); | |
| afterEach(() => { | |
| vi.useRealTimers(); | |
| if (previousAuditFlag === undefined) { | |
| delete process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST; | |
| } else { | |
| process.env.AGENTMEMORY_AUDIT_INDEX_PERSIST = previousAuditFlag; | |
| } | |
| }); |
🤖 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/index-persistence.test.ts` around lines 798 - 806, Update the test
suite’s beforeEach and afterEach hooks to isolate
AGENTMEMORY_AUDIT_INDEX_PERSIST: capture its original value before clearing it
in beforeEach, then restore that captured value in afterEach instead of always
deleting the variable. Keep fake-timer setup and teardown unchanged.
1 participant
Summary
Fixes #1181.
IndexPersistenceaudits its own index flushes, and those rows dominatemem:audit— 59 876of 84 028 entries (71%) on the store this was found on. They also sit outside what the log is
defined to hold: the policy at the top of
src/functions/audit.tsscopes it to structuraldeletions of user data, and an index shard write deletes none.
What this changes
auditIndexPersistence()returns early unlessAGENTMEMORY_AUDIT_INDEX_PERSISTis set to1ortrue. Everything else is untouched — shards, manifests and cleanups are writtenexactly as before, only the audit rows about them stop.
Off by default because the default should match the stated policy. The flag exists because
these rows are genuinely useful when debugging index persistence itself, which is when you
want them and the only time the volume is acceptable.
Why not retention
Retention would cap the symptom for every operation at once, which is a bigger design question
— what to keep, for how long, and whether a deletion record may ever expire. It deserves its
own issue. This change needs no such decision: the policy already exists and this code sits
outside it.
Behaviour
An install that wants the rows back sets one environment variable. An install that does not
loses nothing it was entitled to under the documented policy, and
memory_auditstops paying~2.2 s to read rows about shard writes.
Testing
test/index-persistence.test.tsgains three cases: noindex_persistrows by default, rowspresent when the flag is set, and the index still round-trips through
save()/load()withauditing off — the last one guards against gating more than the audit call.
npx vitest run --exclude test/integration.test.ts→ 1599 passed, 0 failed.npx tsc --noEmit→ 25 errors, identical tomain.npm run skills:checkandnpm run buildclean.plugin/skills/agentmemory-config/REFERENCE.mdis the generator's output for the new variable(
npm run skills:gen), not a hand edit.Summary by CodeRabbit
New Features
1ortrue.Documentation
Bug Fixes