feat: add privacy-safe audit receipts#617
Conversation
Signed-off-by: Caio Ribeiro <caio.ribeiro.clw@gmail.com>
|
@caioribeiroclw-pixel is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
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)
📝 WalkthroughWalkthroughAdds buildAuditReceipt() (HMAC-SHA256 receipts) and integrates optional receipt mode into MCP server, standalone MCP (proxy/local), and REST GET /agentmemory/audit; updates tool schema, tests, and README. ChangesAudit Receipt Feature
Sequence DiagramsequenceDiagram
participant Client
participant Handler
participant buildAuditReceipt
participant Receipt
Client->>Handler: request with ?receipt=true
Handler->>Handler: query audit entries
Handler->>buildAuditReceipt: entries list
buildAuditReceipt->>buildAuditReceipt: HMAC-SHA256 hash ids & extract detail keys
buildAuditReceipt->>Receipt: produce versioned receipt with metadata
Handler->>Client: { receipt: Receipt, success: true }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 2
🧹 Nitpick comments (1)
test/audit.test.ts (1)
124-139: ⚡ Quick winAdd explicit
auditIdHashassertions to lock the privacy contract.The new test validates target/user hashes, but not hashed audit ID behavior, which is part of the receipt contract.
Proposed test additions
expect(receipt.entries[0].targetCount).toBe(1); + expect(receipt.entries[0].auditIdHash).toMatch(/^sha256:/); expect(receipt.entries[0].targetIdHashes[0]).toMatch(/^sha256:/); @@ expect(serialized).not.toContain("user-private-email@example.com"); + expect(serialized).not.toContain(entry.id);🤖 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/audit.test.ts` around lines 124 - 139, The test is missing assertions for the hashed audit ID; update the assertions around the created receipt (variable receipt) to assert that the audit ID is present and hashed (e.g., expect(receipt.auditIdHash).toMatch(/^sha256:/) or the plural field used by the implementation), and add a serialized exclusion asserting the raw audit ID string is not exported (e.g., expect(serialized).not.toContain(rawAuditId)). Locate and modify the assertions near the existing receipt and serialized checks to include these auditIdHash format and non-export checks so the privacy contract is locked.
🤖 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/functions/audit.ts`:
- Around line 60-62: The current digest function uses an unhashed SHA-256 which
is vulnerable to dictionary attacks; replace it with an HMAC using a secret key:
change digest to use crypto.createHmac("sha256",
key).update(value).digest("hex") and prefix with "sha256:" as before, and load
the HMAC key from a secure config or environment variable (fail fast if
missing). Update any other usages in this file (the locations referenced around
lines 79-89 that call digest) to continue using digest unchanged; ensure the
secret is injected once (e.g., module init or config.getSecret) rather than
passed ad-hoc to callers.
- Around line 64-67: safeDetailKeys currently assumes details is an object and
calls Object.keys(details), which throws for null or non-object legacy values;
update safeDetailKeys to first verify details is a non-null object (and not an
array/primitive) before calling Object.keys, returning [] otherwise so malformed
runtime/legacy audit rows can't break receipt generation; reference the
safeDetailKeys function and replace the single truthy check with a robust type
check (e.g., typeof details === 'object' && details !== null &&
!Array.isArray(details)) before sorting keys.
---
Nitpick comments:
In `@test/audit.test.ts`:
- Around line 124-139: The test is missing assertions for the hashed audit ID;
update the assertions around the created receipt (variable receipt) to assert
that the audit ID is present and hashed (e.g.,
expect(receipt.auditIdHash).toMatch(/^sha256:/) or the plural field used by the
implementation), and add a serialized exclusion asserting the raw audit ID
string is not exported (e.g., expect(serialized).not.toContain(rawAuditId)).
Locate and modify the assertions near the existing receipt and serialized checks
to include these auditIdHash format and non-export checks so the privacy
contract is locked.
🪄 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: fe951c37-ba8b-46e6-a6ae-7374202ab98a
📒 Files selected for processing (7)
README.mdsrc/functions/audit.tssrc/mcp/server.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tssrc/triggers/api.tstest/audit.test.ts
|
cool to see audit receipts being added here. a few things that might be useful based on what we learned building a similar receipt format:
we built an open-source receipt format for AI agents that handles all of this (Ed25519 signing, JCS canonicalization, tamper detection, hash-linked chains): github.com/arian-gogani/nobulex happy to share more if any of this is useful. |
|
Updated the PR in
Checks run locally: npm run build
npm test -- --run test/audit.test.ts
npm test
git diff --check |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/audit.test.ts (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
AGENTMEMORY_RECEIPT_HMAC_KEYafter this test.This mutates process-global state and leaves the key set for later tests in the same worker, which can hide failures in paths that should error when the key is missing.
Proposed fix
-import { describe, it, expect, beforeEach, vi } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; @@ describe("Audit Functions", () => { let kv: ReturnType<typeof mockKV>; + let previousReceiptHmacKey: string | undefined; beforeEach(() => { kv = mockKV(); + previousReceiptHmacKey = process.env.AGENTMEMORY_RECEIPT_HMAC_KEY; + }); + + afterEach(() => { + if (previousReceiptHmacKey === undefined) { + delete process.env.AGENTMEMORY_RECEIPT_HMAC_KEY; + return; + } + process.env.AGENTMEMORY_RECEIPT_HMAC_KEY = previousReceiptHmacKey; });Also applies to: 30-35, 121-123
🤖 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/audit.test.ts` at line 1, The test mutates the process-wide AGENTMEMORY_RECEIPT_HMAC_KEY env var and fails to restore it, so save the original value at test start and restore it after the test (use beforeEach to capture const _orig = process.env.AGENTMEMORY_RECEIPT_HMAC_KEY and afterEach to set process.env.AGENTMEMORY_RECEIPT_HMAC_KEY = _orig or delete it if undefined); update the tests in test/audit.test.ts that set process.env.AGENTMEMORY_RECEIPT_HMAC_KEY (references: the test blocks that assign this env var) to use this save/restore pattern to avoid leaking state between tests.
🧹 Nitpick comments (1)
test/audit.test.ts (1)
106-142: ⚡ Quick winAdd a regression case for malformed legacy
details.This PR hardens
safeDetailKeys()fornull/non-object audit rows, but the new test only covers the happy path. One malformed-entry case here would keep that fix from regressing.🤖 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/audit.test.ts` around lines 106 - 142, Add a regression case inside the "buildAuditReceipt hashes ids and omits raw details" test (or as a new it-block nearby) that constructs an audit entry with a malformed legacy details value (e.g., details: null or details: "string") using recordAudit or by creating a minimal entry object, then call buildAuditReceipt([entry]) and assert it does not throw and that the resulting receipt.entries[0].detailKeys is handled safely (empty or only valid keys), privacy flags remain correct, and the serialized receipt does not contain the raw malformed details; this ensures safeDetailKeys() remains robust for null/non-object audit rows.
🤖 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.
Outside diff comments:
In `@test/audit.test.ts`:
- Line 1: The test mutates the process-wide AGENTMEMORY_RECEIPT_HMAC_KEY env var
and fails to restore it, so save the original value at test start and restore it
after the test (use beforeEach to capture const _orig =
process.env.AGENTMEMORY_RECEIPT_HMAC_KEY and afterEach to set
process.env.AGENTMEMORY_RECEIPT_HMAC_KEY = _orig or delete it if undefined);
update the tests in test/audit.test.ts that set
process.env.AGENTMEMORY_RECEIPT_HMAC_KEY (references: the test blocks that
assign this env var) to use this save/restore pattern to avoid leaking state
between tests.
---
Nitpick comments:
In `@test/audit.test.ts`:
- Around line 106-142: Add a regression case inside the "buildAuditReceipt
hashes ids and omits raw details" test (or as a new it-block nearby) that
constructs an audit entry with a malformed legacy details value (e.g., details:
null or details: "string") using recordAudit or by creating a minimal entry
object, then call buildAuditReceipt([entry]) and assert it does not throw and
that the resulting receipt.entries[0].detailKeys is handled safely (empty or
only valid keys), privacy flags remain correct, and the serialized receipt does
not contain the raw malformed details; this ensures safeDetailKeys() remains
robust for null/non-object audit rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14bbf957-b7a2-45e4-b827-4d62f00b2c1f
📒 Files selected for processing (3)
README.mdsrc/functions/audit.tstest/audit.test.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
|
Followed up on the latest review in |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/audit.test.ts (1)
117-174: ⚡ Quick winConsider adding a test for the missing HMAC key error case.
The current tests always set
AGENTMEMORY_RECEIPT_HMAC_KEYbefore callingbuildAuditReceipt(). Per the implementation context,buildAuditReceipt()throws an error when the environment variable is missing. Adding a test that verifies this error behavior would improve coverage and document the contract.🧪 Proposed test case
it("buildAuditReceipt throws when HMAC key is missing", async () => { const entry = await recordAudit(kv as never, "observe", "mem::test", ["mem_1"], {}); delete process.env.AGENTMEMORY_RECEIPT_HMAC_KEY; expect(() => buildAuditReceipt([entry])).toThrow( "AGENTMEMORY_RECEIPT_HMAC_KEY is required to generate audit receipts" ); });Do you want me to help refine this test case or open a tracking issue?
🤖 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/audit.test.ts` around lines 117 - 174, Add a test that verifies buildAuditReceipt throws when the AGENTMEMORY_RECEIPT_HMAC_KEY environment variable is missing: create an audit entry with recordAudit (e.g., recordAudit(..., "observe", "mem::test", ["mem_1"], {})), delete process.env.AGENTMEMORY_RECEIPT_HMAC_KEY, call buildAuditReceipt([entry]) inside an expect(...).toThrow asserting the exact error message ("AGENTMEMORY_RECEIPT_HMAC_KEY is required to generate audit receipts"), and ensure you restore or isolate the env var after the test to avoid cross-test pollution.
🤖 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.
Nitpick comments:
In `@test/audit.test.ts`:
- Around line 117-174: Add a test that verifies buildAuditReceipt throws when
the AGENTMEMORY_RECEIPT_HMAC_KEY environment variable is missing: create an
audit entry with recordAudit (e.g., recordAudit(..., "observe", "mem::test",
["mem_1"], {})), delete process.env.AGENTMEMORY_RECEIPT_HMAC_KEY, call
buildAuditReceipt([entry]) inside an expect(...).toThrow asserting the exact
error message ("AGENTMEMORY_RECEIPT_HMAC_KEY is required to generate audit
receipts"), and ensure you restore or isolate the env var after the test to
avoid cross-test pollution.
|
Followed up on the latest review in |
|
Thanks, this is useful. I tightened this PR in the opposite direction from plain SHA-256: predictable audit/user/target ids now use I agree with your broader point on canonicalization/signatures. My read is:
So I’m intentionally not expanding scope here, but I’m going to track canonicalization/signing as the next step if this lands or if maintainers ask for stronger verification. Nobulex looks relevant for that layer — especially the receipt chain / tamper-evidence side. |
2 participants
What
Adds an opt-in privacy-safe audit receipt format for existing audit trails.
buildAuditReceipt()converts audit entries toagentmemory.audit.receipt.v1targetIds, rawdetails, or rawuserIdGET /agentmemory/audit?receipt=truememory_auditwithreceipt: trueWhy
Persistent memory tools need a shareable proof that memory operations happened without leaking the memory content, paths, raw ids, prompts, or user identifiers. The normal audit endpoint remains unchanged; the receipt mode is for safe handoff/debug/audit artifacts.
Verification
npm run buildnpm test -- --run test/audit.test.tsnpm test(1097 tests passed)git diff --checkNote:
npm installneeded--legacy-peer-depslocally because the current dependency tree has a peer constraint conflict between@anthropic-ai/sdk@0.39.0and@anthropic-ai/claude-agent-sdk.Summary by CodeRabbit
New Features
receipt=trueto receive stable hashed identifiers, counts and metadata instead of raw sensitive values (supported by API endpoints, MCP tools, proxy and local modes).Documentation
receiptparameter, receipt format, and requirement for a configured HMAC key for stable receipts.Tests