Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughSummary by CodeRabbit
WalkthroughReplaces single optional team budget with a multi-budget model: DB adds Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI:<br/>TeamDialog
participant API as API:<br/>createTeam
participant DB as DB:<br/>governance_teams / governance_budgets
UI->>API: POST /teams {name, budgets: [{maxLimit, resetDuration, calendarAligned}]}
API->>API: Validate budgets (non-negative, parse durations, unique resetDuration)
API->>DB: INSERT TableTeam
API->>DB: INSERT TableBudget (team_id FK) for each budget
DB->>DB: Enforce FK CASCADE
DB-->>API: Created team + budgets
API-->>UI: 201 Created {team, budgets: [...]}
sequenceDiagram
participant UI as UI:<br/>TeamDialog
participant API as API:<br/>updateTeam
participant DB as DB
participant Store as InMemoryStore
UI->>API: PATCH /teams/:id {budgets: [{resetDuration, maxLimit, calendarAligned}, ...]}
API->>DB: SELECT team WITH Budgets[]
API->>API: Validate & reconcile by resetDuration
API->>DB: UPDATE matched budgets
API->>DB: INSERT new budgets
API->>DB: DELETE budgets not present in request
API->>Store: UpdateTeamInMemory (reconcile budgets by ID, preserve usage)
API-->>UI: 200 OK {team, budgets: [...]}
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
|
This was referenced Apr 19, 2026
Merged
Merged
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
Confidence Score: 4/5Safe to merge with the in-memory calendar-align reset bug addressed; the DB write is correct and all other paths are clean. One P1 defect: the calendar-align usage reset is correctly persisted to the DB but is immediately undone by UpdateTeamInMemory's unconditional live-usage preservation, leaving in-memory governance checks seeing pre-reset spend for up to the duration of the budget window. All other categories (schema, migration backfill, duplicate validation, cascade delete, UI stability fixes) look correct. plugins/governance/store.go — UpdateTeamInMemory budget reconciliation loop Important Files Changed
Reviews (4): Last reviewed commit: "team budget db structure refactor" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/governance.go (1)
1450-1485:⚠️ Potential issue | 🟠 MajorValidation errors return HTTP 500 instead of 400.
Budget validation errors (e.g., negative max_limit, invalid reset_duration, duplicate duration) are returned as plain
fmt.Errorf, but the error handler at lines 1481-1484 doesn't distinguish them from internal errors—all failures return HTTP 500.This differs from the VK handler pattern (lines 658-666) which wraps validation errors in
badRequestErrorand checks witherrors.Asto return 400.🐛 Proposed fix
// Create owned multi-budgets; enforce unique reset_duration per team seenDurations := make(map[string]bool) for _, b := range req.Budgets { if b.MaxLimit < 0 { - return fmt.Errorf("budget max_limit cannot be negative: %.2f", b.MaxLimit) + return &badRequestError{err: fmt.Errorf("budget max_limit cannot be negative: %.2f", b.MaxLimit)} } if _, err := configstoreTables.ParseDuration(b.ResetDuration); err != nil { - return fmt.Errorf("invalid reset duration format: %s", b.ResetDuration) + return &badRequestError{err: fmt.Errorf("invalid reset duration format: %s", b.ResetDuration)} } if seenDurations[b.ResetDuration] { - return fmt.Errorf("duplicate reset_duration in budgets: %s", b.ResetDuration) + return &badRequestError{err: fmt.Errorf("duplicate reset_duration in budgets: %s", b.ResetDuration)} } // ... }And update the error handler:
}); err != nil { + var badReqErr *badRequestError + if errors.As(err, &badReqErr) { + SendError(ctx, 400, err.Error()) + return + } logger.Error("failed to create team: %v", err) SendError(ctx, 500, "failed to create team") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 1450 - 1485, Validation failures in the budgets loop (checks around seenDurations, MaxLimit, ParseDuration, and validateBudget) currently return plain errors which the outer handler treats as 500; change each validation return to wrap the error in the existing badRequestError type (e.g., return badRequestError{fmt.Errorf(...)} or badRequestError{err}) for the cases in the req.Budgets loop (negative MaxLimit, invalid ResetDuration parse, duplicate ResetDuration, and validateBudget failures) and leave calls that fail due to h.configStore.CreateBudget as plain/internal errors; then ensure the outer error handling already present uses errors.As to detect badRequestError and return HTTP 400 while other errors remain HTTP 500.
🧹 Nitpick comments (4)
tests/governance/teambudget_test.go (1)
23-26: Add one true multi-budget team scenario.This only preserves the old path via a one-item
Budgetsslice. The new risk in this stack is multi-budget ordering/replacement/enforcement, so it would be good to add at least one case with two team budgets (for example, different reset durations) and assert the read/update behavior on both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/teambudget_test.go` around lines 23 - 26, The test currently exercises only a single-item Budgets slice (BudgetRequest) so it doesn’t cover multi-budget behavior; add a new test case in teambudget_test.go that constructs a team with two BudgetRequest entries (e.g., different MaxLimit and different ResetDuration values), then assert that both budgets are persisted, that reads return both in the expected order/identity, and that updates/replacements to one budget do not incorrectly affect the other; locate the test setup using the Budgets: []BudgetRequest{...} block and extend it with a second BudgetRequest and corresponding assertions for read/update/enforcement behavior.tests/governance/e2e_test.go (1)
1089-1092: Exercise the delete path with more than one team budget.This cascade test still creates a single team budget and only checks that the VK keeps working afterward. For a one-to-many team-budget refactor, that misses the failure mode where extra budget rows survive the team delete. Please create two budgets here and assert they are both removed or unlinked from the in-memory budgets store after
DELETE /api/governance/teams/:id.As per coding guidelines, "
**: always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/e2e_test.go` around lines 1089 - 1092, The test currently creates a single BudgetRequest; change the Budgets slice to include two BudgetRequest entries (e.g., two distinct BudgetRequest structs) and after calling the DELETE /api/governance/teams/:id endpoint assert that both budget records are removed or unlinked from the in-memory budgets store (the test's budgets store / map used by the handler) — locate the test harness that constructs Budgets: []BudgetRequest{...} and the in-memory budgets store reference (e.g., the variable or helper that represents stored budgets) and add assertions that neither of the two created budgets exist after the DELETE (or that the association to the deleted team is cleared).ui/app/workspace/governance/views/teamDialog.tsx (2)
460-465: Using array index as key for dynamic list.Budget rows are added/removed dynamically, so using
key={idx}can cause React to incorrectly associate component state (e.g., input values, focus) with the wrong row when the list changes. Since budgets don't have stable IDs until persisted, consider generating a client-side ID when adding rows.♻️ Suggested approach
interface TeamBudgetRow { + clientId: string; // stable key for React rendering maxLimit: number | undefined; resetDuration: string; calendarAligned: boolean; }Then in
addBudgetRow:const addBudgetRow = () => { setFormData((prev) => ({ ...prev, budgets: [ ...prev.budgets, - { maxLimit: undefined, resetDuration: "1M", calendarAligned: false }, + { clientId: crypto.randomUUID(), maxLimit: undefined, resetDuration: "1M", calendarAligned: false }, ], })); };And in the render:
-{formData.budgets.map((row, idx) => ( - <div key={idx} ...> +{formData.budgets.map((row, idx) => ( + <div key={row.clientId} ...>As per coding guidelines: "Use stable, unique keys in lists; never use array index as key unless unavoidable."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/governance/views/teamDialog.tsx` around lines 460 - 465, The budget list is using unstable keys (key={idx}) which can break React state when rows are added/removed; change the row model in formData.budgets to include a stable client-side id (e.g., id or _cid) when new rows are created (update addBudgetRow to assign a UUID/timestamp/unique counter), then render using that id as the key and in data-testid (replace team-budget-row-${idx} with team-budget-row-${row.id}) and update any code that relied on numeric index to find rows to use the id instead (references: formData.budgets, addBudgetRow, and the JSX that renders team-budget-row-{idx}).
543-559: Consider extracting nested ternary to a helper function.The nested ternary for determining the period label is hard to follow. A small helper would improve readability:
♻️ Suggested refactor
const getPeriodLabel = (resetDuration: string | undefined): string => { switch (resetDuration) { case "1d": return "day"; case "1w": return "week"; case "1M": return "month"; case "1Y": return "year"; default: return "period"; } };Then use it in the JSX:
-{pendingCalendarAlignIdx !== null && -formData.budgets[pendingCalendarAlignIdx]?.resetDuration === - "1d" - ? "day" - : pendingCalendarAlignIdx !== null && - formData.budgets[pendingCalendarAlignIdx] - ?.resetDuration === "1w" - ? "week" - ... +{getPeriodLabel( + pendingCalendarAlignIdx !== null + ? formData.budgets[pendingCalendarAlignIdx]?.resetDuration + : undefined +)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/workspace/governance/views/teamDialog.tsx` around lines 543 - 559, Extract the nested ternary that computes the period label into a small helper function (e.g., getPeriodLabel(resetDuration: string | undefined): string) and use it in the JSX instead of the long inline expression; read the reset value from formData.budgets[pendingCalendarAlignIdx]?.resetDuration and pass that to getPeriodLabel, then render the returned string. Ensure the helper covers "1d"→"day", "1w"→"week", "1M"→"month", "1Y"→"year", and defaults to "period".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@framework/configstore/migrations.go`:
- Around line 6118-6136: The migration migrationAddTeamBudgetsToBudgetsTable
backfilled team_id from governance_teams.budget_id but did not recompute teams'
config_hash (GenerateTeamHash uses sorted budget IDs), leaving hashes stale;
after the backfill and after dropping the legacy column, query teams that now
have budgets (join governance_budgets to governance_teams to get distinct
governance_teams.id), preload their Budgets into tables.TableTeam, call
GenerateTeamHash(team) for each, and update the team's config_hash via
tx.Model(&team).Update("config_hash", hash), returning errors on any failure so
the migration fails safely.
In `@framework/configstore/rdb.go`:
- Line 2592: preloadCustomerRelations() is still only Preloading "Teams" so
calls like GetCustomer, GetCustomersPaginated, and GetVirtualKey will return
Customer.Teams without their newly added Budgets; update
preloadCustomerRelations() to also Preload("Teams.Budgets") (or
Preload("Customer.Teams").Preload("Customer.Teams.Budgets") depending on how
it's wired) so the nested Team budget data is hydrated consistently with the new
direct getters that now Preload("Budgets").
In `@framework/configstore/tables/team.go`:
- Line 19: The tests expect a legacy budget_id field but the Team model now
exposes Budgets []TableBudget; add backward-compatible behavior by emitting a
budget_id in JSON (e.g., implement a custom MarshalJSON on the Team struct) that
picks a sensible value (for example the first Budgets[0].ID or nil when empty)
so existing tests reading teamData["budget_id"] continue to work; update the
MarshalJSON to include all original Team fields plus "budget_id" derived from
Budgets, or alternatively update the three tests to read from Budgets[0].ID
(tests: inmemorysync_test.go, configupdatesync_test.go,
advancedscenarios_test.go) if you prefer migrating tests.
In `@plugins/governance/store.go`:
- Around line 313-316: The code is mutating the shared slice backing array when
hydrating team.Budgets (see team.Budgets, gs.budgets.Load, and the clone created
via clone := *team in GetGovernanceData); fix by deep-copying the slice before
modifying it: create a new slice and copy the elements (e.g., newBudgets :=
make([]configstoreTables.TableBudget, len(team.Budgets)); copy(newBudgets,
team.Budgets)), assign that slice to the clone (clone.Budgets = newBudgets),
then perform the per-index replacement using the liveBudget lookup so the
original cached gs.teams entry is not mutated and races are avoided.
In `@tests/governance/configupdatesync_test.go`:
- Around line 700-703: The test is asserting usage preservation against the
pre-update budget row (budgetID) even though the update now sends Budgets:
[]BudgetRequest (no stable id) and reconciliation may replace the row; update
the test to re-query the team's post-update budgets after the update call (e.g.,
fetch the team's budgets collection and locate the surviving/new budget entry)
and then perform the usage-preservation assertions against that budget's ID/row
instead of the original budgetID; ensure you reference the Budgets/BudgetRequest
payload and replace direct assertions that poll budgetID with assertions that
use the freshly retrieved budget from the team's budgets collection.
In `@tests/governance/test_utils.go`:
- Around line 282-283: The Budgets field currently has type []BudgetRequest
which makes an empty slice indistinguishable from omitted when marshaled; change
its type to *[]BudgetRequest (pointer to slice) so callers can use nil to omit
the field and &[]BudgetRequest{} to send an explicit empty array, i.e., update
the struct field declaration for Budgets from "Budgets []BudgetRequest
`json:\"budgets,omitempty\"`" to "Budgets *[]BudgetRequest
`json:\"budgets,omitempty\"`" and adjust any helpers/tests that construct this
struct to pass nil or &[]BudgetRequest{} as needed.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 1568-1634: The updateTeam multi-budget reconciliation currently
returns raw fmt.Errorf validation errors (e.g., for negative MaxLimit, invalid
ResetDuration, duplicate durations, and validateBudget failures) which the
global error handler maps to HTTP 500; change these to return a client
validation error type so they produce HTTP 400 like createTeam. Replace the
plain fmt.Errorf returns inside updateTeam (the checks over req.Budgets, the
ParseDuration error, duplicate-duration check, and validateBudget error returns
when updating/creating budgets) to return the same bad-request/validation error
type used by createTeam (or wrap them with a sentinel ErrBadRequest /
NewBadRequestError) and ensure the global error handler recognizes that type and
returns 400; keep the existing calls to
h.configStore.UpdateBudget/CreateBudget/DeleteBudget and preserve the
reconciledBudgets logic.
In `@ui/app/workspace/governance/views/teamDialog.tsx`:
- Around line 165-174: handleCalendarAlignedChange currently looks up
existingBudget by index (team?.budgets?.[idx]) which can diverge from the form
row order; instead derive a stable key from the form row (e.g., read
resetDuration from formData.budgets[idx]) and find the persisted budget via
team.budgets.find(b => b.reset_duration === resetDuration) (or fallback to
matching an id if present). Then keep the same logic: if checked && isEditing &&
foundBudget && !foundBudget.calendar_aligned call
setPendingCalendarAlignIdx(idx) else call updateBudgetRow(idx, {
calendarAligned: checked }); update references to formData.budgets,
team.budgets, handleCalendarAlignedChange, updateBudgetRow, and
setPendingCalendarAlignIdx accordingly.
In `@ui/app/workspace/governance/views/teamsTable.tsx`:
- Around line 225-265: The multi-budget renderer treats budgets with b.max_limit
<= 0 as $0.00 and shows a 0% Progress bar; change the rendering inside the
teamBudgets.map to detect b.max_limit <= 0 and instead render an explicit
"Unlimited" (or "No limit") label and omit the Progress component (or replace it
with a neutral indicator), using the same Tooltip/TooltipContent but showing
formatCurrency(b.current_usage) with "Unlimited" for the cap; update the
conditional around Progress and the displayed cap text (references: teamBudgets,
b.max_limit, b.current_usage, Progress, Tooltip/TooltipContent, formatCurrency,
formatResetDuration) so unlimited/unset budgets are not shown as $0.00 or a 0%
bar.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 1450-1485: Validation failures in the budgets loop (checks around
seenDurations, MaxLimit, ParseDuration, and validateBudget) currently return
plain errors which the outer handler treats as 500; change each validation
return to wrap the error in the existing badRequestError type (e.g., return
badRequestError{fmt.Errorf(...)} or badRequestError{err}) for the cases in the
req.Budgets loop (negative MaxLimit, invalid ResetDuration parse, duplicate
ResetDuration, and validateBudget failures) and leave calls that fail due to
h.configStore.CreateBudget as plain/internal errors; then ensure the outer error
handling already present uses errors.As to detect badRequestError and return
HTTP 400 while other errors remain HTTP 500.
---
Nitpick comments:
In `@tests/governance/e2e_test.go`:
- Around line 1089-1092: The test currently creates a single BudgetRequest;
change the Budgets slice to include two BudgetRequest entries (e.g., two
distinct BudgetRequest structs) and after calling the DELETE
/api/governance/teams/:id endpoint assert that both budget records are removed
or unlinked from the in-memory budgets store (the test's budgets store / map
used by the handler) — locate the test harness that constructs Budgets:
[]BudgetRequest{...} and the in-memory budgets store reference (e.g., the
variable or helper that represents stored budgets) and add assertions that
neither of the two created budgets exist after the DELETE (or that the
association to the deleted team is cleared).
In `@tests/governance/teambudget_test.go`:
- Around line 23-26: The test currently exercises only a single-item Budgets
slice (BudgetRequest) so it doesn’t cover multi-budget behavior; add a new test
case in teambudget_test.go that constructs a team with two BudgetRequest entries
(e.g., different MaxLimit and different ResetDuration values), then assert that
both budgets are persisted, that reads return both in the expected
order/identity, and that updates/replacements to one budget do not incorrectly
affect the other; locate the test setup using the Budgets: []BudgetRequest{...}
block and extend it with a second BudgetRequest and corresponding assertions for
read/update/enforcement behavior.
In `@ui/app/workspace/governance/views/teamDialog.tsx`:
- Around line 460-465: The budget list is using unstable keys (key={idx}) which
can break React state when rows are added/removed; change the row model in
formData.budgets to include a stable client-side id (e.g., id or _cid) when new
rows are created (update addBudgetRow to assign a UUID/timestamp/unique
counter), then render using that id as the key and in data-testid (replace
team-budget-row-${idx} with team-budget-row-${row.id}) and update any code that
relied on numeric index to find rows to use the id instead (references:
formData.budgets, addBudgetRow, and the JSX that renders team-budget-row-{idx}).
- Around line 543-559: Extract the nested ternary that computes the period label
into a small helper function (e.g., getPeriodLabel(resetDuration: string |
undefined): string) and use it in the JSX instead of the long inline expression;
read the reset value from
formData.budgets[pendingCalendarAlignIdx]?.resetDuration and pass that to
getPeriodLabel, then render the returned string. Ensure the helper covers
"1d"→"day", "1w"→"week", "1M"→"month", "1Y"→"year", and defaults to "period".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b608ec2b-ae51-425a-a255-c1ce7c1c24cc
📒 Files selected for processing (20)
framework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/budget.goframework/configstore/tables/team.goplugins/governance/store.goplugins/governance/test_utils.gotests/governance/advancedscenarios_test.gotests/governance/configupdatesync_test.gotests/governance/customerbudget_test.gotests/governance/e2e_test.gotests/governance/edgecases_test.gotests/governance/inmemorysync_test.gotests/governance/teambudget_test.gotests/governance/test_utils.gotransports/bifrost-http/handlers/governance.goui/app/workspace/governance/views/teamDialog.tsxui/app/workspace/governance/views/teamsTable.tsxui/components/sidebar.tsxui/lib/types/governance.ts
akshaydeo
force-pushed
the
04-20-team_budget_db_structure_refactor
branch
from
7d13d7e to
34346ea
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/governance.go (1)
1704-1721:⚠️ Potential issue | 🔴 CriticalFail the update when
ReloadTeamfails.After the transaction commits,
ReloadTeamis only logged and the handler still returns 200 with the stale team payload. That leaves DB state and the in-memory governance state diverged while telling the client the update succeeded. This path should return HTTP 500, consistent with the rest of the governance update flows.Based on learnings, "if the database update succeeds but the in-memory GovernanceManager reload fails, respond with HTTP 500 to the client rather than signaling success."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 1704 - 1721, The handler currently logs errors from h.governanceManager.ReloadTeam and continues to SendJSON a 200 response with a stale team; change this so that if ReloadTeam(ctx, team.ID) returns an error you call logger.Error(...) and then SendError(ctx, 500, "Failed to reload team") (or a similar 500 response) and return, instead of falling back to preloadedTeam and sending a success; update the block around ReloadTeam to fail the request on error (referencing ReloadTeam, governanceManager, logger.Error, SendError, and SendJSON).
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/governance.go (1)
1472-1476:⚠️ Potential issue | 🟠 MajorWrap
validateBudgeterrors as bad requests here too.The new
< 0/ duplicate-duration checks correctly returnbadRequestError, butvalidateBudget(...)is still returned raw in both create/update paths. A request likemax_limit: 0will therefore fall through these handlers as HTTP 500 instead of HTTP 400. Please wrap thosevalidateBudgetfailures before returning.🐛 Minimal fix
- if err := validateBudget(&budget); err != nil { - return err - } + if err := validateBudget(&budget); err != nil { + return &badRequestError{err: err} + }Apply the same change to the reconciled-budget update path as well.
Also applies to: 1482-1485, 1603-1607, 1704-1707
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/governance.go` around lines 1472 - 1476, The validateBudget(...) call currently returns raw errors causing 500s; wrap its error in a bad request before returning (e.g., return badRequestError(err) or badRequestErrorf with context) wherever validateBudget is used in the create/update handlers—specifically around the validateBudget call preceding h.configStore.CreateBudget(ctx, &budget, tx) and the analogous validateBudget call before the reconciled-budget update—so that invalid input (like max_limit: 0) yields HTTP 400; apply the same wrapping change to the other occurrences noted (the other create/update/reconciled-budget paths).
🧹 Nitpick comments (1)
plugins/governance/test_utils.go (1)
175-175: Preferbifrost.Ptr(...)for pointer creation consistency in test builders.Functionally correct as-is; this is just a consistency tweak with existing repository conventions.
Based on learnings: In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically.♻️ Suggested tweak
- budget.TeamID = &team.ID + budget.TeamID = bifrost.Ptr(team.ID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/test_utils.go` at line 175, Replace the direct address-of usage for team.ID with the repository convention bifrost.Ptr(...) so change the assignment setting budget.TeamID = &team.ID to use budget.TeamID = bifrost.Ptr(team.ID); ensure the bifrost package is imported in the file if not already present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/workspace/governance/views/teamDialog.tsx`:
- Around line 61-66: The form budget rows use array indices as React keys and
match persisted budgets by resetDuration which leads to DOM state reuse and
incorrect find() matches when resetDuration duplicates; update the TeamBudgetRow
type to include a stable client-side id (e.g., id: string), generate a unique id
when creating new rows in the form (UUID or incremented stable id), replace
key={idx} with key={row.id} wherever form rows are rendered, and change any
lookup logic that matches persisted budgets (the find() used during
edit/display) to use the row.id to find the correct persisted budget (or map
persisted budgets by this id) so calendarAligned warnings and state remain
correctly associated with each row.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 1704-1721: The handler currently logs errors from
h.governanceManager.ReloadTeam and continues to SendJSON a 200 response with a
stale team; change this so that if ReloadTeam(ctx, team.ID) returns an error you
call logger.Error(...) and then SendError(ctx, 500, "Failed to reload team") (or
a similar 500 response) and return, instead of falling back to preloadedTeam and
sending a success; update the block around ReloadTeam to fail the request on
error (referencing ReloadTeam, governanceManager, logger.Error, SendError, and
SendJSON).
---
Duplicate comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 1472-1476: The validateBudget(...) call currently returns raw
errors causing 500s; wrap its error in a bad request before returning (e.g.,
return badRequestError(err) or badRequestErrorf with context) wherever
validateBudget is used in the create/update handlers—specifically around the
validateBudget call preceding h.configStore.CreateBudget(ctx, &budget, tx) and
the analogous validateBudget call before the reconciled-budget update—so that
invalid input (like max_limit: 0) yields HTTP 400; apply the same wrapping
change to the other occurrences noted (the other create/update/reconciled-budget
paths).
---
Nitpick comments:
In `@plugins/governance/test_utils.go`:
- Line 175: Replace the direct address-of usage for team.ID with the repository
convention bifrost.Ptr(...) so change the assignment setting budget.TeamID =
&team.ID to use budget.TeamID = bifrost.Ptr(team.ID); ensure the bifrost package
is imported in the file if not already present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a608dfff-1d13-457d-a600-5a78729972ea
📒 Files selected for processing (20)
framework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/budget.goframework/configstore/tables/team.goplugins/governance/store.goplugins/governance/test_utils.gotests/governance/advancedscenarios_test.gotests/governance/configupdatesync_test.gotests/governance/customerbudget_test.gotests/governance/e2e_test.gotests/governance/edgecases_test.gotests/governance/inmemorysync_test.gotests/governance/teambudget_test.gotests/governance/test_utils.gotransports/bifrost-http/handlers/governance.goui/app/workspace/governance/views/teamDialog.tsxui/app/workspace/governance/views/teamsTable.tsxui/components/sidebar.tsxui/lib/types/governance.ts
✅ Files skipped from review due to trivial changes (3)
- tests/governance/edgecases_test.go
- tests/governance/customerbudget_test.go
- ui/components/sidebar.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/app/workspace/governance/views/teamsTable.tsx
- framework/configstore/tables/budget.go
- ui/lib/types/governance.ts
- tests/governance/e2e_test.go
- tests/governance/teambudget_test.go
- tests/governance/advancedscenarios_test.go
- tests/governance/test_utils.go
- tests/governance/inmemorysync_test.go
- plugins/governance/store.go
akshaydeo
force-pushed
the
04-20-team_budget_db_structure_refactor
branch
from
34346ea to
c8bf1f6
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plugins/governance/test_utils.go (1)
175-176: Preferbifrost.Ptr(team.ID)in this fixture.That keeps the test helper consistent with the repo-wide pointer-helper convention and avoids tying the copied budget row back to the parent struct field.
Based on learnings: prefer using `bifrost.Ptr()` to create pointers instead of the address operator (`&`) even in test utilities.♻️ Suggested change
- budget.TeamID = &team.ID + budget.TeamID = bifrost.Ptr(team.ID) team.Budgets = []configstoreTables.TableBudget{*budget}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/governance/test_utils.go` around lines 175 - 176, Replace the address operator use with the repo pointer helper: set budget.TeamID = bifrost.Ptr(team.ID) instead of budget.TeamID = &team.ID (leaving team.Budgets assignment unchanged); ensure the bifrost package is imported in this test_utils.go file so bifrost.Ptr is available.tests/governance/test_utils.go (1)
243-247: Keep the team test helpers fully aligned with the live API.The live team handlers still accept
rate_limit, and their budget reconciliation paths readcalendar_alignedfrom each budget. These helpers still can’t express either field, so typed governance tests can’t cover those branches without dropping down to ad-hoc JSON. The same gap exists onUpdateTeamRequest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/governance/test_utils.go` around lines 243 - 247, The test helpers CreateTeamRequest and UpdateTeamRequest lack the live-API fields used by handlers: add a RateLimit field to both team request types (matching the API’s rate_limit shape) and add a CalendarAligned bool field to the BudgetRequest type (used by budget reconciliation); update any JSON tags to "rate_limit,omitempty" and "calendar_aligned,omitempty" respectively so tests can construct requests that exercise the live handlers' branches. Ensure you reference the types CreateTeamRequest, UpdateTeamRequest, and BudgetRequest when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/governance/test_utils.go`:
- Around line 175-176: Replace the address operator use with the repo pointer
helper: set budget.TeamID = bifrost.Ptr(team.ID) instead of budget.TeamID =
&team.ID (leaving team.Budgets assignment unchanged); ensure the bifrost package
is imported in this test_utils.go file so bifrost.Ptr is available.
In `@tests/governance/test_utils.go`:
- Around line 243-247: The test helpers CreateTeamRequest and UpdateTeamRequest
lack the live-API fields used by handlers: add a RateLimit field to both team
request types (matching the API’s rate_limit shape) and add a CalendarAligned
bool field to the BudgetRequest type (used by budget reconciliation); update any
JSON tags to "rate_limit,omitempty" and "calendar_aligned,omitempty"
respectively so tests can construct requests that exercise the live handlers'
branches. Ensure you reference the types CreateTeamRequest, UpdateTeamRequest,
and BudgetRequest when making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a22e9ba-6acb-48be-b70f-1312970d2b0e
📒 Files selected for processing (20)
framework/configstore/clientconfig.goframework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/tables/budget.goframework/configstore/tables/team.goplugins/governance/store.goplugins/governance/test_utils.gotests/governance/advancedscenarios_test.gotests/governance/configupdatesync_test.gotests/governance/customerbudget_test.gotests/governance/e2e_test.gotests/governance/edgecases_test.gotests/governance/inmemorysync_test.gotests/governance/teambudget_test.gotests/governance/test_utils.gotransports/bifrost-http/handlers/governance.goui/app/workspace/governance/views/teamDialog.tsxui/app/workspace/governance/views/teamsTable.tsxui/components/sidebar.tsxui/lib/types/governance.ts
✅ Files skipped from review due to trivial changes (7)
- tests/governance/customerbudget_test.go
- tests/governance/edgecases_test.go
- tests/governance/advancedscenarios_test.go
- framework/configstore/migrations.go
- ui/components/sidebar.tsx
- tests/governance/inmemorysync_test.go
- tests/governance/e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/governance/teambudget_test.go
- framework/configstore/clientconfig.go
- tests/governance/configupdatesync_test.go
- framework/configstore/tables/budget.go
- framework/configstore/rdb.go
- transports/bifrost-http/handlers/governance.go
- ui/app/workspace/governance/views/teamDialog.tsx
18 tasks
Contributor
Author
Merge activity
|
akshaydeo
changed the base branch from
04-19-team_budgets
to
graphite-base/2841
akshaydeo
dismissed
coderabbitai[bot]’s stale review
The base branch was changed.
akshaydeo
force-pushed
the
04-20-team_budget_db_structure_refactor
branch
from
c8bf1f6 to
df5fbbd
Compare
Comment on lines
+2432
to
2436
| if live, exists := gs.budgets.Load(b.ID); exists && live != nil { | ||
| if lb, ok := live.(*configstoreTables.TableBudget); ok && lb != nil { | ||
| b.CurrentUsage = lb.CurrentUsage | ||
| b.LastReset = lb.LastReset | ||
| } |
Contributor
There was a problem hiding this comment.
UpdateTeamInMemory unconditionally replaces b.CurrentUsage and b.LastReset with the live in-memory values for every matching budget. This means the explicit CurrentUsage = 0 / LastReset = GetCalendarPeriodStart(...) written to the DB during a false → true calendar-align transition is immediately overridden when ReloadTeam calls this function.
Concrete failure scenario: a monthly budget created on Jan 10 with $50 usage; admin enables calendar-alignment on Jan 25. After ReloadTeam, the in-memory budget still holds CurrentUsage = $50 and LastReset = Jan 10. ResetExpiredBudgetsInMemory then computes GetCalendarPeriodStart("1M", now) = Jan 1 — which is NOT later than LastReset = Jan 10 — so no in-memory reset fires until Feb 1. Governance enforcement will see $50 spend for the remainder of January despite the user confirming an immediate reset.
A minimal fix is to skip the live-usage preservation when the incoming CurrentUsage is zero and CalendarAligned is true (i.e., a deliberate reset just happened):
if live, exists := gs.budgets.Load(b.ID); exists && live != nil {
if lb, ok := live.(*configstoreTables.TableBudget); ok && lb != nil {
// Preserve live usage/reset only when the DB value wasn't
// deliberately zeroed (e.g. calendar-align reset).
if !(b.CalendarAligned && b.CurrentUsage == 0 && b.LastReset.After(lb.LastReset)) {
b.CurrentUsage = lb.CurrentUsage
b.LastReset = lb.LastReset
}
}
}Note also that budgetBaselines map[string]float64 (the third parameter) is declared but never read anywhere in this function — it appears to be dead code left over from an earlier design.
dominictayloruk
pushed a commit
to dominictayloruk/bifrost
that referenced
this pull request
Apr 21, 2026
## Summary
Teams previously supported a single budget via a `budget_id` foreign key on `governance_teams`. This PR migrates team budgets to the same multi-budget ownership model already used by virtual keys and provider configs, where budgets are owned via a `team_id` FK on `governance_budgets`. Teams can now have multiple budgets with different reset intervals (e.g., a daily cap and a monthly cap enforced simultaneously).
## Changes
- **Schema**: Removed `budget_id` from `governance_teams`; added `team_id` to `governance_budgets` with a `CASCADE` delete constraint. `TableTeam.Budget *TableBudget` is replaced by `TableTeam.Budgets []TableBudget`.
- **Migration** (`migrationAddTeamBudgetsToBudgetsTable`): Adds the `team_id` column and index, creates the FK constraint, backfills existing team budgets from the legacy `budget_id` column, then drops `budget_id` from `governance_teams`. Includes a SQLite workaround (foreign key pragma + single connection) matching the pattern used in `migrationAddMultiBudgetTables`.
- **Budget ownership validation**: `TableBudget.BeforeSave` now counts all three owner FK fields (`TeamID`, `VirtualKeyID`, `ProviderConfigID`) and rejects any budget with more than one owner.
- **Config hash**: `GenerateTeamHash` now sorts and serializes all budget IDs rather than a single `budget_id`, preventing hash flips from slice ordering on reload.
- **HTTP handlers**: `CreateTeamRequest` and `UpdateTeamRequest` replace the single `budget`/`UpdateBudgetRequest` field with a `budgets []CreateBudgetRequest` slice. Create inserts the team row first (satisfying the FK), then creates child budgets. Update reconciles by `reset_duration`: matching durations update in place (preserving usage), new durations create new budget rows, and removed durations delete the orphaned rows. Duplicate `reset_duration` values within a single request are rejected.
- **In-memory governance store**: All budget load, store, update, and delete paths in `LocalGovernanceStore` are updated to iterate over `team.Budgets` instead of dereferencing a single `team.Budget` pointer. `UpdateTeamInMemory` reconciles the live budget map by ID, evicting budgets that are no longer associated with the team.
- **`DeleteTeam`**: Budget deletion is now handled entirely by the database cascade; the explicit budget delete inside the transaction is removed.
- **UI**: The team dialog replaces the single budget field with a dynamic list of budget rows, each independently configurable with a max limit, reset duration, and calendar alignment toggle. The calendar-align confirmation dialog is now index-aware. The teams table renders a progress bar per budget. `Team` type drops `budget_id`/`budget` in favor of `budgets?: Budget[]`. All API request types are updated accordingly.
- **Tests**: All governance integration tests updated to use the `budgets: []BudgetRequest{{...}}` array shape.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [x] UI (React)
- [ ] Docs
## How to test
```sh
# Core/Transports
go test ./framework/configstore/... ./plugins/governance/... ./tests/governance/...
# UI
cd ui
pnpm i
pnpm build
```
1. Create a team with two budgets (e.g., `1d` and `1M`) via `POST /api/governance/teams` and confirm both rows appear in `governance_budgets` with the correct `team_id`.
2. Update the team replacing one budget duration; confirm the old budget row is deleted and the new one is created, while the unchanged budget retains its `current_usage`.
3. Delete the team; confirm all associated budget rows are removed via cascade without explicit deletes.
4. Upgrade from a previous schema version; confirm the migration backfills `team_id` on existing budget rows and drops `budget_id` from `governance_teams`.
5. In the UI, open the team dialog, add multiple budget rows with distinct reset durations, save, and verify each budget appears in the current usage section and the teams table.
## Breaking changes
- [x] Yes
- [ ] No
The `budget` / `budget_id` fields are removed from the `Team` object in both the API response and the UI type definitions. Callers reading `team.budget` or `team.budget_id` must migrate to `team.budgets[]`. The `budget` field on `CreateTeamRequest` and `UpdateTeamRequest` is replaced by `budgets[]`. The database schema change is handled automatically by the included migration.
## Related issues
## Security considerations
No new authentication or authorization surfaces are introduced. Budget ownership validation in `BeforeSave` prevents a budget from being assigned to multiple owners simultaneously.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
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
2 participants
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.
Summary
Teams previously supported a single budget via a
budget_idforeign key ongovernance_teams. This PR migrates team budgets to the same multi-budget ownership model already used by virtual keys and provider configs, where budgets are owned via ateam_idFK ongovernance_budgets. Teams can now have multiple budgets with different reset intervals (e.g., a daily cap and a monthly cap enforced simultaneously).Changes
budget_idfromgovernance_teams; addedteam_idtogovernance_budgetswith aCASCADEdelete constraint.TableTeam.Budget *TableBudgetis replaced byTableTeam.Budgets []TableBudget.migrationAddTeamBudgetsToBudgetsTable): Adds theteam_idcolumn and index, creates the FK constraint, backfills existing team budgets from the legacybudget_idcolumn, then dropsbudget_idfromgovernance_teams. Includes a SQLite workaround (foreign key pragma + single connection) matching the pattern used inmigrationAddMultiBudgetTables.TableBudget.BeforeSavenow counts all three owner FK fields (TeamID,VirtualKeyID,ProviderConfigID) and rejects any budget with more than one owner.GenerateTeamHashnow sorts and serializes all budget IDs rather than a singlebudget_id, preventing hash flips from slice ordering on reload.CreateTeamRequestandUpdateTeamRequestreplace the singlebudget/UpdateBudgetRequestfield with abudgets []CreateBudgetRequestslice. Create inserts the team row first (satisfying the FK), then creates child budgets. Update reconciles byreset_duration: matching durations update in place (preserving usage), new durations create new budget rows, and removed durations delete the orphaned rows. Duplicatereset_durationvalues within a single request are rejected.LocalGovernanceStoreare updated to iterate overteam.Budgetsinstead of dereferencing a singleteam.Budgetpointer.UpdateTeamInMemoryreconciles the live budget map by ID, evicting budgets that are no longer associated with the team.DeleteTeam: Budget deletion is now handled entirely by the database cascade; the explicit budget delete inside the transaction is removed.Teamtype dropsbudget_id/budgetin favor ofbudgets?: Budget[]. All API request types are updated accordingly.budgets: []BudgetRequest{{...}}array shape.Type of change
Affected areas
How to test
1dand1M) viaPOST /api/governance/teamsand confirm both rows appear ingovernance_budgetswith the correctteam_id.current_usage.team_idon existing budget rows and dropsbudget_idfromgovernance_teams.Breaking changes
The
budget/budget_idfields are removed from theTeamobject in both the API response and the UI type definitions. Callers readingteam.budgetorteam.budget_idmust migrate toteam.budgets[]. Thebudgetfield onCreateTeamRequestandUpdateTeamRequestis replaced bybudgets[]. The database schema change is handled automatically by the included migration.Related issues
Security considerations
No new authentication or authorization surfaces are introduced. Budget ownership validation in
BeforeSaveprevents a budget from being assigned to multiple owners simultaneously.Checklist
docs/contributing/README.mdand followed the guidelines