feat: add ai_user_daily_spend table and queries#26562
Conversation
This comment has been minimized.
This comment has been minimized.
0e9ce46 to
5375f15
Compare
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 5 findings (1 P2, 1 P4, 3 Nit), COMMENT. Review Finding inventoryFindings
Round logRound 1Netero + Panel. 1 P2, 0 P3, 1 P4, 3 Nit. Reviewed against 335d6bd..5375f15. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Clean, well-scoped PR. The table design is sound: composite PK covers the enforcement query pattern, the upsert serializes correctly under READ COMMITTED, UTC normalization is tested at timezone boundaries, and the test density at 85.9% (372 test LOC for 61 production LOC) is thorough. Pariston tried to build a case against this change and couldn't.
Severity count: 1 P2, 1 P4, 3 Nit.
The single P2 is a convergent finding from six reviewers: the upsert's ON CONFLICT path accepts negative cost_micros, silently reducing accumulated spend on existing rows. Since this table drives budget enforcement, a caller bug passing a negative delta would corrupt the enforcement data. The fix is a one-line Go validation.
Notes from the panel worth knowing (not posted as findings):
- The secondary index
(effective_group_id, day)has no query consumer yet; it's a sound forward investment for group-level aggregation (AIGOV-428). (Knuckle) GetUserSpendSinceauthz checksActionReadon the user, notActionReadPersonal. Any actor that can read the user can query spend in any group. For budget enforcement this is correct; for user-facing spend visibility, the API layer may need tighter scoping. (Mafuuu, Knov)- No retention/purge mechanism for
ai_user_daily_spend. At moderate scale this is manageable, but the table grows unboundedly. Worth adbpurgehandler when the callers land. (Hisoka) - The authz wrapper for
GetUserSpendSincerequires the user to exist (GetUserByID), but the table preserves rows after user deletion. Post-deletion reporting would need a separate system-level query. (Knov)
"I tried to build a case against this change and couldn't. The problem is correctly understood, the solution is proportional, and the fix is at the right level." - Pariston
🤖 This review was automatically generated with Coder Agents.
| -- The day parameter is normalized to its UTC calendar day before storage. | ||
| -- Returns the resulting row. | ||
| INSERT INTO ai_user_daily_spend (user_id, effective_group_id, day, spend_micros) | ||
| VALUES (@user_id, @effective_group_id, ((@day::timestamptz) AT TIME ZONE 'UTC')::date, @cost_micros) |
There was a problem hiding this comment.
P2 [CRF-2] The upsert's ON CONFLICT path does spend_micros = ai_user_daily_spend.spend_micros + EXCLUDED.spend_micros. CostMicros is int64 (signed). The table CHECK only fires when the result goes negative.
On INSERT (new row): negative cost_micros is rejected because the result itself is negative. Safe.
On UPDATE (existing row): if current spend_micros is 1000 and cost_micros is -500, the result is 500, which passes the CHECK. Spend is silently reduced.
The query comment says "Adds cost_micros to the spend" and the table drives budget enforcement (AIGOV-428). A caller bug passing a negative delta silently corrupts enforcement data: spend goes down, headroom goes up, budget appears unspent. No test covers negative cost_micros; all test values are non-negative.
Fix: validate CostMicros >= 0 in the Go dbauthz wrapper (or a shared helper) before calling the query. This produces a clear application error rather than relying on the schema-level floor. (Hisoka P2, Knov P2, Mafuuu P3, Bisky P3)
🤖
| -- name: UpsertUserDailySpend :one | ||
| -- Adds cost_micros to the spend for (user_id, effective_group_id, day). | ||
| -- The day parameter is normalized to its UTC calendar day before storage. | ||
| -- Returns the resulting row. |
There was a problem hiding this comment.
Nit [CRF-3] RETURNING * in the SQL and the AIUserDailySpend return type in the generated Go interface both make this visible. This line can be deleted. (Gon)
🤖
| INSERT INTO ai_user_daily_spend (user_id, effective_group_id, day, spend_micros) | ||
| VALUES (@user_id, @effective_group_id, ((@day::timestamptz) AT TIME ZONE 'UTC')::date, @cost_micros) | ||
| ON CONFLICT (user_id, effective_group_id, day) DO UPDATE SET | ||
| -- Add this call's cost to the running total for that day. |
There was a problem hiding this comment.
Nit [CRF-4] spend_micros = old + EXCLUDED.spend_micros is the standard accumulator pattern. The comment restates the arithmetic with English synonyms ("running total" = spend_micros, "this call's cost" = EXCLUDED.spend_micros). Consider deleting it, or replacing with a why-comment if the intent isn't obvious from context. (Gon)
🤖
| spend_micros = ai_user_daily_spend.spend_micros + EXCLUDED.spend_micros | ||
| RETURNING *; | ||
|
|
||
| -- name: GetUserSpendSince :one |
There was a problem hiding this comment.
Nit [CRF-1] The query filters on user_id, effective_group_id, and day >= period_start, but the name conveys only User and Since. A caller reading the name could expect spend across all groups. A name like GetUserSpendByEffectiveGroupSince would be more precise, matching the pattern of other queries in this file (GetGroupAIBudget, GetUserAIBudgetOverride). (Netero)
🤖
| }) | ||
| } | ||
|
|
||
| func TestGetUserSpendSince(t *testing.T) { |
There was a problem hiding this comment.
P4 [CRF-5] GetUserSpendSince normalizes period_start via the same ((@period_start::timestamptz) AT TIME ZONE 'UTC')::date pattern that UpsertUserDailySpend uses for day. The upsert suite tests this pattern (NormalizesNonUTCTimezones, TruncatesDayToUTCMidnight), but GetUserSpendSince does not. All PeriodStart values in these tests are UTC midnight. If someone removed the normalization from this query, all existing tests would still pass.
Risk is low because the identical SQL pattern is proven elsewhere. Adding a non-UTC PeriodStart test case (analogous to NormalizesNonUTCTimezones) would close the gap. (Bisky)
🤖
Description
Adds the spend tracking table and queries needed by AIGOV-427
(post-response accumulation) and AIGOV-428 (pre-request enforcement).
Changes
ai_user_daily_spendtable to aggregate per-user, per-effective-group AI spend by UTC day.UpsertUserDailySpendandGetUserSpendSincequeries.Closes https://linear.app/codercom/issue/AIGOV-426/add-daily-spend-table-and-queries
Note
Initially generated by Claude Opus 4.7, modified and reviewed by @ssncferreira