feat: make database.Chat auditable#24485
Conversation
Wire database.Chat into the audit system so chat lifecycle events
(manual patches, background auto-archives, deletions) produce audit
log entries.
- Add 'chat' value to the resource_type enum (migration 000473).
- Register Chat in the Auditable union, AuditActionMap, and
AuditableResources with per-field tracking that ignores churny
internal fields (status, heartbeat_at, updated_at,
last_model_config_id, ...) and tracks user-meaningful fields
(title, archived, pin_order, mode, labels, mcp_server_ids, ...).
- Implement ResourceTarget/ResourceID/ResourceType/ResourceRequiresOrgID
cases for database.Chat.
- Implement auditLogIsResourceDeleted via GetChatByID (chats are
hard-deleted; missing row == deleted).
- Implement auditLogResourceLink -> /chats/{id}.
- Wire audit.InitRequest[database.Chat] into the patchChat handler
so manual updates (title, archived, pin_order, labels, workspace
binding) produce audit records.
- Introduce BackgroundSubsystemChatAutoArchive for the upcoming
dbpurge integration, and broaden the background-audit log line
from 'dormancy audit' to 'background audit'.
- Regenerate audit-logs.md, swagger, schemas.md, and typesGenerated.ts.
Part of CODAGT-200.
johnstcn
changed the title
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
The audit table registration, field-level tracking comments, and migration are thorough. The per-field ignore/track rationale in enterprise/audit/table.go is the best-annotated entry in that file.
Severity count: 1 P1, 2 P2, 3 P3, 3 Nit.
The P1 is structural: archive and pin mutations never update the local chat variable, so aReq.New captures stale state and the audit diff is always empty for those operations. Nine of fourteen reviewers raised this independently. The fix is a single GetChatByID re-fetch before aReq.New = chat.
The P2s are (a) httpapi.Is404Error conflating auth failure with deletion in auditLogIsResourceDeleted, diverging from every sibling case that uses xerrors.Is(err, sql.ErrNoRows), and (b) the test encoding that incorrect behavior while also missing any negative (deleted=false) case.
Process note: the fixup commit "address testing gap" simultaneously changed the error check from xerrors.Is(err, sql.ErrNoRows) to httpapi.Is404Error(err) and added a test for the new behavior. A commit described as closing a testing gap should test existing behavior, not introduce and ratify a behavioral change.
"A test that can't detect a stuck-true return value proves nothing about the non-deleted case." (Bisky)
🤖 This review was automatically generated with Coder Agents.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Status: APPROVED
All R1 findings addressed. The P1 (stale chat variable) is fixed via a post-mutation re-fetch. The P2s (Is404Error semantics, test gaps) are resolved cleanly. The unused constant and unreachable delete action were removed.
R2 findings are defensive improvements: 4 P3. The re-fetch fallback in patchChat (DEREM-12) and the creation path audit gap (DEREM-13) both have extremely low trigger probability (DB failure between a successful mutation and an immediate re-read on the same connection). The test gaps (DEREM-14, DEREM-15) are real but not blocking.
DEREM-9 (Nit, migration number reference): panel upheld 3/5. The factual claim stands (no other production Go file references a migration by number), but the consequence is minor. Not blocking approval.
"The migration number adds a lookup step without semantic value." (Chopper)
🤖 This review was automatically generated with Coder Agents.
|
|
||
| updated := getChat(ctx, t, client, chat.ID) | ||
| require.Empty(t, updated.PlanMode) | ||
| require.True(t, mAudit.Contains(t, database.AuditLog{ |
There was a problem hiding this comment.
P3 [DEREM-15] The mAudit.Contains assertions check metadata (Action, ResourceType, ResourceID, UserID) but MockAuditor.diff returns Map{}, so diff content is never captured. A stale-variable regression like DEREM-4 (the R1 P1) would pass all current tests. For at least one happy-path case, asserting that the audit diff is non-empty would prevent recurrence.
(Bisky)
🤖
There was a problem hiding this comment.
I looked into this, and coderdtest.New defaults its auditor to audit.NewNop(). Adding all of the required plumbing for this is out of scope.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Status: APPROVED
All R1 and R2 findings addressed. The PR is in good shape after three rounds.
One new P3 from the DEREM-13 fix: the re-read refactoring scoped updated to the audit record only, so the HTTP response at line 764 now uses the pre-re-read chat. Today both values are identical (only a join table changes in between), so the practical impact is nil, but the comment at line 751 claims the re-read is for the response.
DEREM-15 (MockAuditor diff limitation): panel accepted the author's scope-out (4/4). Pre-existing infrastructure gap affecting every auditable resource. This needs a human decision: accept permanently or file a ticket.
DEREM-9 (Nit, migration number): 3 rounds, no author action. Panel ready to move on. Not blocking.
"The code structure now depends on that normalization surviving future changes." (Meruem)
coderd/exp_chats.go:764
P3 [DEREM-16] The DEREM-13 fix (05878f3) changed the re-read from chat, err = api.Database.GetChatByID(...) to if updated, err := ...; err == nil { aReq.New = updated }. The updated variable is scoped to the if-block, so chat is never reassigned. The response here uses the original chat from CreateChat, not the re-read.
Today both values are identical because linkFilesToChat only modifies a join table, not the chat row. But the comment at lines 751-752 ("Re-read the chat so the response reflects the authoritative database state") now describes what the code used to do, not what it does.
Fix: add chat = updated alongside aReq.New = updated in the if-block at line 754, or update the comment to say the re-read serves the audit record.
(Kite P3, Meruem P3, Mafuuu Nit)
🤖
🤖 This review was automatically generated with Coder Agents.
de-bugulated |
2 participants
Wire database.Chat into the audit system so chat lifecycle events (creation, patches, etc.) produce audit log entries.
Part of CODAGT-200.