feat(installations): add status audit log#13
Conversation
Test & Lint Summary
Tests: 68 passed, 0 failed, 0 skipped |
Test & Lint Summary
Tests: 68 passed, 0 failed, 0 skipped |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
Implementation matches the spec (status clear-on-whitespace, append-only audit log with 24h idempotency, 1000-entry retention, newest-first pagination, and correct auth split between app identity vs org members).
Left a few minor follow-ups:
- add DB-level constraint for
level - consider avoiding
hashtextadvisory-lock collisions - add unit tests for audit-log page token format
- strengthen the status-clear test to assert presence semantics
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if resp.GetInstallation().GetStatus() != "" { |
There was a problem hiding this comment.
[minor] This assertion uses GetStatus() which can’t distinguish an unset optional field from an explicitly empty string. Since the spec says whitespace clears the field (store NULL), it’d be better to assert the field is actually unset, e.g. if resp.GetInstallation().Status != nil { ... }.
| return entries, nextToken, nil | ||
| } | ||
|
|
||
| func encodeAuditLogPageToken(entry InstallationAuditLogEntry) string { |
There was a problem hiding this comment.
[minor] Can we add store-level unit tests for the audit-log page token (round-trip + invalid token cases), similar to pagination_test.go? This would protect against accidental changes to the token format.
| id UUID PRIMARY KEY DEFAULT uuid_generate_v4(), | ||
| installation_id UUID NOT NULL REFERENCES app_installations (id) ON DELETE CASCADE, | ||
| message TEXT NOT NULL, | ||
| level TEXT NOT NULL, |
There was a problem hiding this comment.
[minor] Consider adding a CHECK constraint for installation_audit_log_entries.level (e.g., level IN ('info','warning','error')). The converter currently panics on unknown levels, so it’d be good to enforce the invariant at the DB boundary too.
| _ = tx.Rollback(ctx) | ||
| }() | ||
|
|
||
| if _, err := tx.Exec(ctx, "SELECT pg_advisory_xact_lock(hashtext($1))", input.InstallationID.String()); err != nil { |
There was a problem hiding this comment.
[minor] Locking: advisory locks here make sense for idempotency/retention, but hashtext(uuid.String()) is only 32-bit and can collide across installations. Consider instead locking the installation row itself (SELECT 1 FROM app_installations WHERE id=$1 FOR UPDATE) or using a stronger lock key derivation to avoid cross-installation contention.
3 participants
Summary
Testing
Issue