Preserve audit logs when a connection is deleted - #15
Merged
Conversation
Decouple SigningLog from the connection lifecycle (Option B): the connection FK becomes nullable with ON DELETE SET NULL, and userId / connectionName / clientPubkey are denormalized onto each log so an orphaned record stays scoped to its user and readable in the dashboard. A new user FK (ON DELETE CASCADE) keeps user-deletion purging logs. Dashboard activity + stats now scope by the denormalized userId instead of the connection relation, so deleted-connection logs no longer vanish from the feed. Tests: - unit: getDashboardActivity scopes by userId (regression guard) - DB integration (RUN_DB_TESTS=1): SET NULL retention + user-delete cascade - CI: new `integration` job runs migrate deploy (full chain), a schema/migration drift check, and the integration tests against a migration-built Postgres
…tests Panel-review follow-ups: - stats: connectionNames now unions live connection names with the names preserved on signing_logs, so a deleted connection (logs orphaned with connectionId=null) stays selectable in the dashboard filter. - unit: assert every signingLog count/groupBy in getDashboardStats scopes by the denormalized userId (guards against a revert to connection.userId silently dropping orphaned logs from charts). - integration: prove orphaned logs are still aggregated into dashboard stats and the deleted name remains in connectionNames after delete.
dsbaars
commented
Jun 24, 2026
dsbaars
left a comment
Owner
Author
There was a problem hiding this comment.
Multi-engine panel review — checked-out branch @ 05a857d
Independent review by four engines (each read the real code on the branch; cursor-agent and GLM also reproduced the migration chain + integration tests against a throwaway Postgres).
| Engine | Verdict |
|---|---|
| Claude (primary) | ✅ PASS |
| Sonnet 4.6 | ✅ PASS |
| cursor-agent (auto) | ✅ PASS |
| opencode / GLM 5.2 | ✅ PASS |
Verified correct (with evidence)
- Migration is atomic & correct — backfill runs before
SET NOT NULL; all source columns areNOT NULLonBunkerConnection, so no nulls slip through; FK swap toSET NULL+ new user-cascade FK + index matchschema.prisma;prisma migrate diffreports no drift (reproduced live by 3 engines). Prisma runs the migration in a single transaction on Postgres, so a concurrent insert cannot interleave betweenADD COLUMNandSET NOT NULL— there is no mid-migration rollback race. - Scoping change is complete — every query that previously scoped
SigningLogvia theconnectionrelation (Prisma calls + all three$queryRawblocks) now scopes by the denormalizeduserId. No call site missed → orphaned logs neither vanish nor leak. - Single writer (
bunker-rpc.handler.ts) sets all required fields from the server-derived connection row; TypeScript enforces them. Tenancy holds: reads scope by JWTreq.user.sub;userIdis written fromconnection.userId, never client input. - Integration tests prove
ON DELETE SET NULLretention + user-delete cascade at the DB level.
Findings & resolution
- [was P2] Filter dropdown excluded deleted-connection names — RESOLVED in
05a857d:connectionNamesnow unions live connection names with the names preserved onsigning_logs, so a deleted connection stays filterable. - [was P2 / coverage] stats path under-tested — RESOLVED in
05a857d: a unit test now assertsuserId-scoping on everycount/groupByingetDashboardStats(guards against a silent revert), and an integration test proves orphaned logs stay aggregated into stats and the deleted name stays inconnectionNames. - [P3]
getLogsForConnectionscopes byconnectionIdafter an ownership pre-check — defence-in-depth only; pre-existing. - [LOW]
connectionNameis frozen at write time (a rename won't rewrite historical logs) — intentional audit immutability. - [LOW] a deleted connection's per-connection logs endpoint 404s — orphaned history is dashboard-only by design.
- [nit, pre-existing]
LogResult.DENIEDis never written (denied actions log asERROR) — not touched by this PR.
Bottom line
Correct, complete, and well-tested. The central safety claim was verified live by three independent engines plus the primary pass. The two substantive follow-ups are fixed in 05a857d. Verdict: APPROVE (posted as a comment — GitHub blocks self-approval since the reviewer is the PR author).
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
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.
What
Audit logs (
SigningLog) currently cascade-delete with their connection. This decouples them (Option B) so the signing history survives connection deletion.SigningLog.connectionId→ nullable, FKON DELETE CASCADE→SET NULLuserId/connectionName/clientPubkeyonto each log so an orphaned record stays scoped + readableON DELETE CASCADE) so deleting a user still purges their logsuserIdinstead of the connection relation (otherwise orphaned logs vanish from the feed)Tests
getDashboardActivityscopes byuserId(regression guard)RUN_DB_TESTS=1): provesSET NULLretention + user-delete cascade against a real Postgresintegrationjob runsmigrate deploy(full chain from scratch), a schema/migration drift check, and the integration tests against a migration-built PostgresNotes