feat: audit group AI budget mutations#25374
Conversation
Docs preview📖 View docs preview for |
b8727b0 to
053eb55
Compare
053eb55 to
3e37b1f
Compare
|
/coder-agents-review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (13 reviewers). DEREM-1 verified fixed. One new P3, two Nits.
The audit wiring is well-executed: correct resource type, correct org-scoping through the parent group, and the migration is clean (ADD VALUE IF NOT EXISTS, no table locks). Pariston investigated whether budget changes should fold into the Group audit instead of a separate resource type, traced the AIProviderKey precedent (same pattern: child table, separate endpoints, own resource type), and concluded the approach is proportional. Hisoka pulled every thread and found nothing hidden. 8 of 13 reviewers returned no findings.
"I tried to build a case against this PR's premises and could not." (Pariston)
Process note: the initial commit shipped audit instrumentation with zero test code. The test was added only after R1 flagged the gap. This worked, but audit features should ship with audit tests from the start.
🤖 This review was automatically generated with Coder Agents.
|
|
||
| // Upsert (create-or-update) emits an AuditActionWrite entry. | ||
| base := len(auditor.AuditLogs()) | ||
| _, err = adminClient.UpsertGroupAIBudget(ctx, group.ID, codersdk.UpsertGroupAIBudgetRequest{ |
There was a problem hiding this comment.
P3 [DEREM-3] The upsert audit test only exercises the create-from-nothing path. The update path's diff capture is untested.
"You can delete
aReq.Old = oldBudget(line 766) and this test still passes. The audit log is still emitted becauseaReq.Newis non-nil. But the diff for an update (e.g., $500 to $50) would silently become{}, which is the exact scenario the PR description calls out." (Bisky)
The handler at aibridge.go:760-766 fetches the existing budget to populate aReq.Old for before/after diffs. Since the test only creates from nothing (GetGroupAIBudget returns sql.ErrNoRows, aReq.Old stays zero-value), that fetch path is dead weight in the test. A regression removing the Old-state capture would be invisible.
Sketch: after the first upsert, call UpsertGroupAIBudget again with a different SpendLimitMicros and assert auditor.AuditLogs() grew by one more. This exercises the GetGroupAIBudget fetch that populates Old. (Bisky P3, Chopper P3)
🤖
There was a problem hiding this comment.
A regression removing the Old-state capture would be invisible.
There is no way to properly test diff with MockAuditor, so there is no way to catch this regression.
I can add test with real enterprise auditor, but I don't think it worth it. Also pattern in the code base to use mockAuditor.
Relates to https://linear.app/codercom/issue/AIGOV-284/add-group-budgets-table-and-crud-api
Adds audit-log support for
group_ai_budgetmutations. Without it, an admin could silently lower a spend limit from$500to$50or delete a budget entirely, with no record of who performed the action.Both write (
create-or-update) and delete actions now produce audit log entries, including before/after diffs forspend_limit_micros.Depends on #25203.