fix: minimize durable audit metadata#795
Closed
BigSimmo wants to merge 12 commits into
Closed
Conversation
20260717170000_minimize_audit_log_metadata.sql conflicted with 20260717170000_registry_projection_cleanup.sql already on main. Supabase CLI uses the numeric timestamp prefix as the schema_migrations primary key, so two files sharing the same prefix cause a duplicate-key error during migration replay CI. Rename to 20260717174000 (next safe slot after 20260717173000).
…_admin (test guard)
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAudit logging now applies action-specific metadata allowlists, removes document titles, filenames, and content hashes from new entries, minimizes existing rows through a migration, and adds behavioral tests and documentation updates. ChangesAudit metadata minimization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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)
tests/audit.test.ts (1)
46-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
document_deletetest coveringstorageRemoved.The new tests cover
document_uploadanddocument_renameallowlist behavior, but there is no test fordocument_deletewithstorageRemovedmetadata. This gap is especially relevant given the type-mismatch bug flagged onsrc/lib/audit.tsLines 38-39 — a test would have caught thatstorageRemovedis always dropped because the guard checks forbooleanwhile the route sends anumber.✅ Suggested test
it("retains storageRemoved count for delete events", async () => { const client = mockClient(async () => ({ error: null })); await writeAuditLog(client, { ownerId: "owner-1", action: "document_delete", resourceId: "doc-1", metadata: { storageRemoved: 3, deletedTitle: "Patient Jane Doe" }, }); expect(client.insertSpy).toHaveBeenCalledWith( expect.objectContaining({ metadata: { storageRemoved: 3 } }), ); expect(JSON.stringify(client.insertSpy.mock.calls[0]?.[0])).not.toContain("Jane Doe"); });🤖 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 `@tests/audit.test.ts` around lines 46 - 81, Add a test alongside the existing audit metadata tests for document_delete that passes numeric storageRemoved metadata with a user-controlled deletedTitle. Assert writeAuditLog retains storageRemoved, excludes deletedTitle from the inserted metadata, and does not include the title in the serialized insert payload.
🤖 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 `@docs/privacy-impact-assessment.md`:
- Around line 196-201: Update the audit_logs row in the privacy impact
assessment to reference migration timestamp 20260717163000, matching the
minimize_audit_log_metadata migration filename and preserving the surrounding
description.
In `@src/lib/audit.ts`:
- Around line 38-39: Update the document_delete handling in minimumAuditMetadata
to accept numeric metadata.storageRemoved, matching the count produced by
cleanup in the DELETE route, and return that value instead of dropping it. Apply
the same numeric-type condition in the minimize_audit_log_metadata migration so
existing numeric values are preserved.
In `@supabase/migrations/20260717163000_minimize_audit_log_metadata.sql`:
- Around line 13-15: Align document_delete metadata handling with the numeric
cleanup.storageRemoved value: update the minimumAuditMetadata guard to accept
finite numbers, and change the migration’s jsonb_typeof check from boolean to
number so existing values are preserved during backfill.
---
Nitpick comments:
In `@tests/audit.test.ts`:
- Around line 46-81: Add a test alongside the existing audit metadata tests for
document_delete that passes numeric storageRemoved metadata with a
user-controlled deletedTitle. Assert writeAuditLog retains storageRemoved,
excludes deletedTitle from the inserted metadata, and does not include the title
in the serialized insert payload.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 984cf38d-7f70-40bb-bd3e-0fb508608f25
📒 Files selected for processing (6)
docs/privacy-impact-assessment.mdsrc/app/api/documents/[id]/route.tssrc/app/api/upload/route.tssrc/lib/audit.tssupabase/migrations/20260717163000_minimize_audit_log_metadata.sqltests/audit.test.ts
…-metadata-db17 Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
DELETE audit callers pass a removed-object count, so the allowlist and historical migration now accept number or boolean storageRemoved values. Also correct the PIA migration timestamp to 20260717163000. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
11 tasks
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #3145 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
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
20260717163000_minimize_audit_log_metadata.sqlto avoid schema_migrations timestamp collisions and keep the reassert-admin migration last.Verification
Risk and rollout
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy)Summary by CodeRabbit
Privacy Improvements
Documentation
Tests