feat(budget): add user path budget management#276
Merged
SantiagoDePolonia merged 10 commits intomainfrom Apr 27, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds first-class budget enforcement: config/env surface, budget domain/service and stores (SQLite/Postgres/Mongo), admin REST/dashboard UI, request-time enforcement wired into server/batch/passthrough flows, workflow feature gating, audit enrichment, and extensive tests. Changes
sequenceDiagram
participant Client
participant Server
participant Enforcer
participant Service
participant Store
participant DB
Client->>Server: incoming request (with user_path, workflow)
Server->>Enforcer: enforceBudget(ctx)
Enforcer->>Service: Check(ctx, userPath, now)
Service->>Store: SumUsageCost / ListBudgets
Store->>DB: query usage & budgets
DB-->>Store: aggregated results
Store-->>Service: spent, hasUsage, budgets
Service-->>Enforcer: allowed / ExceededError
alt Exceeded
Enforcer-->>Server: budget exceeded -> budget_check_error
Server-->>Client: 429 Too Many Requests + Retry-After
else Allowed
Server->>Server: continue processing request
Server-->>Client: 200 OK
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
# Conflicts: # config/config.go # internal/admin/dashboard/static/js/modules/dashboard-layout.test.cjs # internal/admin/dashboard/templates/layout.html # internal/admin/dashboard/templates/sidebar.html
Greptile SummaryThis PR introduces a complete per-user-path budget management system: YAML/env-var configuration, SQLite/PostgreSQL/MongoDB persistence, a
Confidence Score: 4/5Safe to merge after addressing the default-enabled breaking-change risk for deployments with usage tracking disabled. One P1 finding: the opt-out-required default for BudgetsEnabled=true is a silent breaking change for any deployment with USAGE_ENABLED=false. The rest of the implementation is sound — calendar math, transaction safety in SQL stores, path matching, and lifecycle wiring are all correct. The MongoDB non-atomicity and env-var underscore ambiguity are P2 and do not affect correctness for the common case. config/config.go (default Enabled=true + SET_BUDGET_* underscore ambiguity), internal/budget/store_mongodb.go (non-atomic ReplaceConfigBudgets) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Server as Server (handlers)
participant BudgetSvc as budget.Service
participant Store as budget.Store (SQL/Mongo)
participant UsageDB as usage table
Client->>Server: POST /v1/chat/completions
Server->>BudgetSvc: Check(ctx, userPath, now)
BudgetSvc->>BudgetSvc: read cached budgets (RLock)
BudgetSvc->>Store: SumUsageCost(userPath, periodStart, now)
Store->>UsageDB: SELECT SUM(total_cost) WHERE path LIKE ...
UsageDB-->>Store: spent float64
Store-->>BudgetSvc: spent, hasUsage
alt spent >= amount
BudgetSvc-->>Server: ExceededError (429)
Server-->>Client: 429 budget_exceeded
else
BudgetSvc-->>Server: nil
Server->>Server: dispatch to LLM provider
Server-->>Client: 200 response
end
note over Client,BudgetSvc: Admin reset flow
Client->>Server: POST /admin/api/v1/budgets/reset-one
Server->>BudgetSvc: ResetBudget(ctx, path, period, now)
BudgetSvc->>Store: UPDATE budgets SET last_reset_at=now
BudgetSvc->>Store: ListBudgets + GetSettings (Refresh)
BudgetSvc-->>Server: ok
Server-->>Client: 200 budgetListResponse
Reviews (1): Last reviewed commit: "fix(budgets): require usage for budget e..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1004
to
1006
| Enabled: true, | ||
| }, | ||
| Metrics: MetricsConfig{ |
There was a problem hiding this comment.
BudgetsConfig.Enabled defaults to true. Any existing deployment that has explicitly disabled usage tracking (USAGE_ENABLED=false or usage.enabled: false) will fail to start after upgrading with:
"budgets require usage tracking to be enabled because spend limits are evaluated from usage cost records"
Those users must add BUDGETS_ENABLED=false (or budgets.enabled: false) to restore previous behavior. The failure is silent from the outside — the process exits on startup — and the fix is not obvious without reading the new config docs.
Comment on lines
+117
to
+141
| func (s *MongoDBStore) ReplaceConfigBudgets(ctx context.Context, budgets []Budget) error { | ||
| budgets, err := normalizeBudgetsForUpsert(budgets) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for i := range budgets { | ||
| budgets[i].Source = "config" | ||
| } | ||
|
|
||
| filter := bson.D{{Key: "source", Value: "config"}} | ||
| if len(budgets) > 0 { | ||
| keep := make(bson.A, 0, len(budgets)) | ||
| for _, budget := range budgets { | ||
| keep = append(keep, bson.D{ | ||
| {Key: "user_path", Value: budget.UserPath}, | ||
| {Key: "period_seconds", Value: budget.PeriodSeconds}, | ||
| }) | ||
| } | ||
| filter = append(filter, bson.E{Key: "$nor", Value: keep}) | ||
| } | ||
| if _, err := s.budgets.DeleteMany(ctx, filter); err != nil { | ||
| return fmt.Errorf("delete old config budgets: %w", err) | ||
| } | ||
| return s.UpsertBudgets(ctx, budgets) | ||
| } |
There was a problem hiding this comment.
ReplaceConfigBudgets is not atomic in MongoDB
The DeleteMany and subsequent UpsertBudgets calls are separate operations with no session/transaction wrapping them. SQLite and PostgreSQL implementations wrap this in a transaction. If the process crashes or the context is cancelled between the two calls, the source = 'config' budgets are permanently wiped from the database; they would only be restored on the next server restart (when seedConfiguredBudgets reruns). On a replica set this can be wrapped with a session that uses WithTransaction.
Comment on lines
+1284
to
+1300
| } | ||
| cfg.Budgets.UserPaths = append(cfg.Budgets.UserPaths, BudgetUserPathConfig{ | ||
| Path: path, | ||
| Limits: limits, | ||
| }) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func budgetEnvPath(suffix string) string { | ||
| suffix = strings.Trim(strings.ToLower(strings.TrimSpace(suffix)), "_") | ||
| if suffix == "" { | ||
| return "/" | ||
| } | ||
| segments := make([]string, 0) | ||
| for _, part := range strings.Split(suffix, "_") { | ||
| part = strings.TrimSpace(part) |
There was a problem hiding this comment.
SET_BUDGET_* env vars
budgetEnvPath splits the suffix on _ to build the user-path, so SET_BUDGET_TEAM_A produces /team/a, not /team_a. There is no way to target a single path segment that itself contains an underscore (e.g. user_123). Since user paths are freely defined by admins, this silent mis-mapping could configure budgets against the wrong path tree without any error. Consider documenting this constraint explicitly, or choosing a different delimiter (e.g. double-underscore __ as separator vs single underscore in a segment).
Comment on lines
+43
to
+54
|
|
||
| func budgetCheckError(err error) error { | ||
| var exceeded *budget.ExceededError | ||
| if errors.As(err, &exceeded) { | ||
| message := exceeded.Error() | ||
| if message == "" { | ||
| message = "budget exceeded" | ||
| } | ||
| return core.NewRateLimitError("budget", message).WithCode("budget_exceeded") | ||
| } | ||
| return core.NewProviderError("budget", http.StatusServiceUnavailable, fmt.Sprintf("budget check failed: %v", err), err). | ||
| WithCode("budget_check_failed") |
There was a problem hiding this comment.
Retry-After header on budget-exceeded (429) response
When ExceededError is returned the handler maps it to NewRateLimitError, which produces an HTTP 429. RFC 6585 §4 recommends including a Retry-After header so clients know when the budget period resets. The CheckResult.PeriodEnd field is already computed in evaluateBudget and returned in the ExceededError.Result — it could be surfaced in the header to avoid clients polling blindly.
![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: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/server/error_support.go (1)
18-33: 🧹 Nitpick | 🔵 TrivialHelper looks good; minor consistency nit on the fallback branch.
gatewayErrorCodecorrectly guards both nil receiver and nilCode. The fallbackcore.NewProviderError(...)branch on line 24 omits the third argument; since that error has noCodeset it would resolve to""either way, so this is purely a stylistic asymmetry. Consider passinggatewayErrorCode(gatewayErr)there too for symmetry, or leave as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/error_support.go` around lines 18 - 33, The fallback branch should include the same error code parameter for symmetry: update the auditlog.EnrichEntryWithError call in the fallback branch (after core.NewProviderError(...) and logHandledError(c, gatewayErr)) to pass gatewayErrorCode(gatewayErr) as the fourth argument so it matches the earlier call pattern (functions involved: gatewayErrorCode, core.NewProviderError, logHandledError, auditlog.EnrichEntryWithError).internal/workflows/types.go (1)
74-101:⚠️ Potential issue | 🟠 MajorCanonicalize
BudgetlikeFallbackto keepworkflow_hashstable for semantically equivalent payloads.
canonicalize()setsFallbackto*truewhen nil precisely so that two payloads with the same effective behavior produce the same marshaled bytes (and therefore the sameworkflow_hashinnormalizePayload).Budgetwas added with the same nil-defaults-to-true semantics inruntimeFeatures()but is not canonicalized, so a payload withbudgetomitted and one with"budget": truewill:
- evaluate to identical
core.WorkflowFeatures.Budgetat runtime, but- hash to different
workflow_hashvalues.This breaks dedup/idempotency for callers (YAML loaders, env-driven seeders, dashboard upserts) that emit one form vs. the other for the same intent.
♻️ Suggested fix
func (f FeatureFlags) canonicalize() FeatureFlags { - if f.Fallback != nil { - return f + if f.Fallback == nil { + fallbackEnabled := true + f.Fallback = &fallbackEnabled + } + if f.Budget == nil { + budgetEnabled := true + f.Budget = &budgetEnabled } - fallbackEnabled := true - f.Fallback = &fallbackEnabled return f }Then
runtimeFeatures()can drop the localbudgetresolution and use*f.Budgetdirectly, mirroring howFallbackis consumed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/workflows/types.go` around lines 74 - 101, The canonicalization currently sets Fallback to true when nil but not Budget, causing semantically identical payloads (omitted budget vs "budget": true) to hash differently; update FeatureFlags.canonicalize() to also set Budget to true when f.Budget == nil, then simplify runtimeFeatures() to use *f.Budget directly (like Fallback) so core.WorkflowFeatures.Budget is derived consistently and workflow_hash/normalizePayload remain stable for equivalent payloads.internal/auditlog/middleware.go (1)
581-600: 🧹 Nitpick | 🔵 TrivialMinor: drop empty
errorCodeinstead of assigning a trimmed empty string.If a caller passes a whitespace-only
errorCode, this still overwritesentry.Data.ErrorCodewith"". Today that's invisible (the JSON/BSON tag hasomitempty), but it silently clobbers a previously-set non-empty value ifEnrichEntryWithErroris ever called twice on the same entry. A guard keeps the "trim → ignore empty" symmetry already used elsewhere in this file (e.g.EnrichEntryWithUserPath,EnrichEntryWithAuthKeyID).♻️ Suggested fix
- if len(errorCode) > 0 { - entry.Data.ErrorCode = strings.TrimSpace(errorCode[0]) - } + if len(errorCode) > 0 { + if code := strings.TrimSpace(errorCode[0]); code != "" { + entry.Data.ErrorCode = code + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/middleware.go` around lines 581 - 600, EnrichEntryWithError currently trims the provided errorCode but assigns it even when empty, which can overwrite a prior non-empty entry.Data.ErrorCode; update the function (EnrichEntryWithError) to trim the first errorCode and only set entry.Data.ErrorCode when the trimmed value is non-empty (preserving existing value otherwise), keeping the existing guards for entry and entry.Data and the current behavior for ErrorType and ErrorMessage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/config.go`:
- Around line 1316-1326: The map iteration over values (map[string]float64)
produces a non-deterministic order for BudgetLimitConfig entries, causing
cfg.Budgets.UserPaths[i].Limits and seedConfiguredBudgets tests to be unstable;
fix by collecting the map keys from values, sort them (use sort.Strings), then
iterate over the sorted keys to build limits (creating BudgetLimitConfig{Period:
key, Amount: values[key]}) before returning; update imports to include "sort".
In `@docs/advanced/admin-endpoints.mdx`:
- Around line 140-143: Clarify that the PUT, DELETE, and reset-one admin
endpoints require the budget's composite identifier including user_path as well
as the period (either period or period_seconds); update the sentence mentioning
`period`/`period_seconds` to explicitly state that budgets are identified by
`(user_path, period)` (or `(user_path, period_seconds)`), and/or add a short
note referencing the `/features/budgets` request shape so readers don't assume
these operations are global per period.
In `@docs/advanced/configuration.mdx`:
- Around line 123-126: Expand the `SET_BUDGET_<PATH>` docs to explicitly state
the suffix→path transformation (the `<PATH>` suffix is lowercased and each
underscore becomes a slash, so `SET_BUDGET_TEAM_ALPHA_SERVICE` →
`/team/alpha/service` and `SET_BUDGET_TEAM_ALPHA` → `/team/alpha`), list the
supported periods as `hourly`, `daily`, `weekly`, and `monthly`, and clarify
that `SET_BUDGET_=` refers to a literal env var named `SET_BUDGET_` (valid per
POSIX but sometimes rejected by certain shells/orchestrators such as some
Kubernetes validators); also mention this follows the same `<SUFFIX>` rules used
by the `<PROVIDER>_<SUFFIX>_API_KEY` examples below for consistency.
- Around line 122-123: The docs and config defaults introduce a breaking change
because buildDefaultConfig() currently sets BUDGETS_ENABLED=true while
USAGE_ENABLED may be false, causing startup validation failures; fix by making
BUDGETS_ENABLED default to false in buildDefaultConfig() (opt-in) and update the
docs table entry for `BUDGETS_ENABLED` accordingly, or alternatively modify the
validation logic that enforces "budgets require usage tracking" to soft-disable
budgets with a warning (use process logger) when USAGE_ENABLED=false instead of
throwing; also add a clear migration note to the release notes/changelog
describing the dependency between BUDGETS_ENABLED and USAGE_ENABLED and the
chosen migration behavior.
In `@docs/features/budgets.mdx`:
- Around line 85-92: Add a short clarifying sentence directly above or below the
periods table explaining that the Seconds column is an internal period
identifier (e.g., `monthly` = `2592000`) and that standard periods other than
`period_seconds` are evaluated using the configured reset-anchor logic (clamped
to month length), so `monthly` is calendar-aware and not a fixed 30‑day window;
reference the `monthly` identifier, `period_seconds`, and the reset-anchor
behavior when editing the table caption or adjacent paragraph.
- Around line 75-83: Update the docs for the SET_BUDGET_* env-var mapping to
explicitly state the underscore-encoding limitation: explain how the suffix of
SET_BUDGET_ is parsed into a slash-separated path (e.g. SET_BUDGET_TEAM_ALPHA ->
/team/alpha), and either document the escape mechanism if one exists (e.g.
double-underscore or casing rules) or add a clear note that there is no escape
and env-var-seeded budgets cannot represent path segments that contain
underscores (so paths like /team_alpha are unsupported), and recommend
configuring those budgets via YAML or the dashboard as the fallback.
In `@docs/openapi.json`:
- Around line 4395-4400: The OpenAPI security scheme "BearerAuth" is defined
incorrectly as type "apiKey"; update the "securitySchemes" entry for
"BearerAuth" to use HTTP bearer semantics by changing type to "http", adding
scheme: "bearer" and optionally bearerFormat: "JWT" so tooling (Swagger UI and
generated clients) will automatically prepend "Bearer " to Authorization
headers; locate the "BearerAuth" object under "securitySchemes" in the OpenAPI
document and replace the apiKey definition accordingly.
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 3676-3693: The hard-coded brown foregrounds in
.budget-period-label-daily and .budget-period-label-hourly (and the dashed
variant .budget-period-label-custom if applicable) produce insufficient contrast
in dark theme; update these selectors to route their color through the theme
text variable (e.g., use color-mix(..., <brown> X%, var(--text)) for the color
and for the background mix with var(--bg)) or add theme-specific overrides like
the existing .budget-period-label-monthly [data-theme="light"] block so that in
dark mode the label text uses a color-mix against var(--text) (or a separate
darker/light variant) to meet WCAG contrast requirements while preserving
current light-theme appearance.
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 558-563: The budgets page fetch is being queued twice:
dashboardDataFetches() already adds fetchBudgetsPage() when this.page ===
"budgets", and this block pushes it again; remove the duplicate push here so
fetchBudgetsPage() is only invoked once, or add a guard to check whether
requests already includes fetchBudgetsPage() before pushing; update the block
around this.page === "budgets" / requests.push(this.fetchBudgetsPage()) to rely
on dashboardDataFetches() (and reference methods
refreshDashboardDataAfterRuntimeRefresh, dashboardDataFetches, and
fetchBudgetsPage when making the change).
- Around line 196-198: The code currently triggers fetchBudgetsPage twice on
direct navigation because init() calls _applyRoute() which immediately calls
fetchBudgetsPage(), and then fetchAll()/dashboardDataFetches() queues it again;
fix by deduplicating the fetch: either remove the immediate fetchBudgetsPage()
call in _applyRoute() and let dashboardDataFetches() drive page fetches, or add
a guard/flag (e.g., this.isFetchingBudgets or this.budgetsLoaded) checked by
both fetchBudgetsPage() and dashboardDataFetches() to skip scheduling/starting a
fetch if one is in-flight or already completed; apply same dedupe pattern to the
similar branch at lines referenced (462-467).
In `@internal/admin/dashboard/static/js/modules/budgets.js`:
- Around line 730-745: normalizeBudgetSettings currently converts blank strings
to 0 and lets fractional numbers through; update numberValue inside
normalizeBudgetSettings to treat empty string (payload[key] === '') as "use
fallback" and to only accept integer values—coerce valid numeric input to an
integer (e.g. via Number.isFinite(parsed) && Number.isInteger(parsed) ?
Math.trunc(parsed) : fallback) so fields like monthly_reset_day,
daily_reset_hour, weekly_reset_minute, etc. never get blank or fractional values
in the payload.
In `@internal/admin/dashboard/templates/page-settings.html`:
- Around line 196-201: The Escape key handler for the budget reset modal should
be guarded by the auth dialog state so it doesn't close the budget-reset dialog
while an auth dialog is stacked; update the handler on the element that uses
budgetResetDialogOpen and closeBudgetResetDialog() (the `@keydown.escape.window`
binding) to first check that authDialogOpen is false before calling
closeBudgetResetDialog(), e.g., ensure the expression requires !authDialogOpen
&& budgetResetDialogOpen before closing, or move the check into a small wrapper
method that checks authDialogOpen and budgetResetDialogOpen then calls
closeBudgetResetDialog().
In `@internal/admin/handler_budgets_test.go`:
- Around line 114-214: Add tests in internal/admin/handler_budgets_test.go that
exercise the new BudgetSettings, UpdateBudgetSettings, and ResetBudgets paths:
create a test handler with an adminBudgetStore, then call h.BudgetSettings and
assert default settings are returned; call h.UpdateBudgetSettings with valid and
invalid JSON bodies and assert store.settings updated and proper HTTP status
codes; and add tests for h.ResetBudgets that exercise both the confirmation path
(body {"confirm":"reset"}) which should clear/reset store state and return 200,
and the non-confirmation/error path which should return a 400/expected error;
use existing patterns in TestBudgetEndpoints* (echo.New, httptest.NewRequest,
check rec.Code, decode response) and reference adminBudgetStore fields (e.g.,
settings, reset state) to verify side effects.
In `@internal/admin/handler.go`:
- Around line 1182-1202: The handler ResetBudgets currently returns a dynamic
map (map[string]any) for the successful response; replace this with a concrete
response struct (e.g., type resetBudgetsResponse struct { Status string
`json:"status"` }) and return an instance of that struct from ResetBudgets
instead of map[string]any; update any OpenAPI comment if necessary to reference
the concrete response type and ensure imports/definitions (resetBudgetsRequest,
resetBudgetsResponse) are declared near the handler so the JSON schema and
type-safety are preserved.
- Around line 1052-1059: The current handler collapses any error from
h.budgets.UpsertBudgets into core.NewInvalidRequestError and returns via
handleError, which misclassifies backend/store failures as 400s; change the
error handling in the UpsertBudgets call to distinguish validation/user errors
from service/store errors by inspecting the returned error (or using error
types/markers from the budgets/store package) and: if the error represents
invalid input, return handleError(core.NewInvalidRequestError(...)), otherwise
wrap/return a service-level error (e.g. core.NewServiceError or an equivalent
5xx error) so DB timeouts/resets produce 5xx responses; apply the same fix to
the other similar blocks that call h.budgets.UpsertBudgets / RemoveBudget and
currently use core.NewInvalidRequestError.
In `@internal/budget/factory.go`:
- Around line 106-132: Remove the early-return that skips ReplaceConfigBudgets
when cfg.UserPaths is empty: in seedConfiguredBudgets, stop checking
len(cfg.UserPaths) == 0 and only guard on service == nil so ReplaceConfigBudgets
is always called (even with an empty budgets slice) to allow it to prune stale
config-sourced rows; keep the rest of the function logic (normalizing user
paths, building budgets slice, calling service.ReplaceConfigBudgets(ctx,
budgets)) unchanged.
In `@internal/budget/service_test.go`:
- Around line 70-147: Tests never exercise the "no usage" branch (the boolean
return from SumUsageCost / fakeStore.sum that indicates hasUsage), so add a unit
test that uses fakeStore with budgets (e.g., Budget{UserPath:"/team",...}) and a
sum implementation that returns (0, false, nil) to simulate no usage and assert
that Service.Check / CheckWithResults do not treat the budget as exceeded and do
not call enforcement logic; reference the fakeStore.sum function and the
Service.Check and Service.CheckWithResults paths (and the hasUsage boolean from
SumUsageCost) so the test verifies enforcement is only applied when
hasUsage==true.
In `@internal/budget/settings_helpers.go`:
- Around line 29-58: The function applySettingValue currently parses the value
before checking the key, causing unknown keys with non-integer values to produce
parse errors; change the logic so the switch on strings.TrimSpace(key) (in
applySettingValue) runs first and only for matching cases call strconv.Atoi on
strings.TrimSpace(value) and assign to the appropriate Settings field
(DailyResetHour, DailyResetMinute, WeeklyResetWeekday, WeeklyResetHour,
WeeklyResetMinute, MonthlyResetDay, MonthlyResetHour, MonthlyResetMinute); for
unknown keys return nil and for parse failures return the existing fmt.Errorf
error.
In `@internal/budget/store_mongodb.go`:
- Around line 126-140: The current code deletes all "config" budgets via
s.budgets.DeleteMany and then calls s.UpsertBudgets, which can leave Mongo in a
partially-updated state if upserts fail; wrap the delete+upsert work in a single
Mongo transaction (start a client session, use session.WithTransaction) and
perform the DeleteMany and the subsequent upserts/inserts inside that
transaction so they either all commit or all abort, or alternatively perform a
single atomic replacement by writing the new set to a temp collection and using
a rename/replace under transaction; modify the code that calls
s.budgets.DeleteMany and s.UpsertBudgets to run inside the transaction (use the
same ctx/session for s.UpsertBudgets or inline the upsert logic so it uses the
transaction session).
- Around line 237-247: The aggregation pipeline in SumUsageCost() builds a
computed field _gomodel_budget_user_path with $addFields and regex-matches it,
preventing use of an index on the persisted user_path and hurting performance;
instead, normalize user_path at write time (trim and default to "/") in the code
path that inserts/updates usage documents and remove the $addFields step from
the pipeline so the match filters the persisted user_path directly (replace
references to _gomodel_budget_user_path with user_path in the pipeline variable
and queries in store_mongodb.go).
- Around line 177-193: SaveSettings currently upserts each key individually
using s.settings.UpdateOne (via settingsKeyValues), which can leave partial
state on failure; fix it by executing those upserts inside a MongoDB
transaction: start a client session, call session.WithTransaction (or
StartTransaction/CommitTransaction) and perform the same UpdateOne calls for
each key within the transaction callback, returning any error so the transaction
aborts on failure; keep ValidateSettings and settings.UpdatedAt logic and use
the same filter/update objects but run them inside the transaction so all keys
are persisted atomically.
In `@internal/budget/types_test.go`:
- Around line 8-44: The test TestPeriodBoundsUsesConfiguredAnchors only checks
one fixed now and misses boundary and edge cases; convert it into a table-driven
test that iterates cases calling PeriodBounds(now, period, settings) with varied
now values and settings to cover: now exactly at the daily/weekly/monthly anchor
(to assert inclusivity rules), weekly cases on the anchor weekday just before
and just after the anchor time, and monthly with MonthlyResetDay=31 evaluated in
short months (Feb 28/29 and Apr/Jun) to ensure clamping; use the existing
Settings struct and PeriodDailySeconds/PeriodWeeklySeconds/PeriodMonthlySeconds
identifiers and assert expected start/end for each case.
In `@internal/core/workflow_test.go`:
- Around line 14-31: Add a positive test case for
TestWorkflowFeaturesApplyUpperBound_DisablesBudgetWhenUsageDisabled (or convert
to a small table-driven test) to assert the complementary behavior: when base
WorkflowFeatures and the upper-bound WorkflowFeatures both have Usage=true and
Budget=true, calling ApplyUpperBound must leave Budget true; locate the existing
test that references WorkflowFeatures and ApplyUpperBound and add an additional
subtest/case that constructs features :=
WorkflowFeatures{Usage:true,Budget:true}.ApplyUpperBound(WorkflowFeatures{Usage:true,Budget:true})
and asserts features.Usage and features.Budget are both true to prevent
regressions that always clear Budget.
In `@internal/server/budget_support_test.go`:
- Around line 65-88: Add a positive-path unit test for batchBudgetEnforcer that
mirrors TestBatchBudgetEnforcerUsesResolvedWorkflow but sets
core.ResolvedWorkflowPolicy.Features.Budget to true and asserts the budget
checker is invoked once: create countingBudgetChecker, get enforcer via
batchBudgetEnforcer(checker), build ctx with core.WithWorkflow(...
Features{Usage:true, Budget:true}), call enforcer(ctx) and fail on error, then
assert checker.calls == 1 (and fail if not). Ensure the new test function is
named e.g. TestBatchBudgetEnforcerInvokesCheckerWhenEnabled and follows the same
structure as the existing test.
In `@internal/server/http.go`:
- Around line 329-335: The routes for budget upsert/delete currently expose PUT
/budgets -> cfg.AdminHandler.UpsertBudget and DELETE /budgets ->
cfg.AdminHandler.DeleteBudget which accept the compound (user_path, period) via
JSON body; change them to path-scoped routes (e.g. PUT
/budgets/:user_path/:period and DELETE /budgets/:user_path/:period) so they
match other admin endpoints and avoid bodies on DELETE; URL-encode user_path in
clients and keep the request body only for the numeric limit (amount) on
UpsertBudget; update the handler signatures (UpsertBudget, DeleteBudget) to read
params from the URL (user_path, period) instead of the request body and
validate/parse them accordingly.
In `@internal/server/translated_inference_service.go`:
- Around line 85-87: The budget enforcement is being bypassed on cache hits
because enforceBudget is called inside dispatchChatCompletion/dispatchResponses
via the dispatch closure and handleWithCache (responseCache.HandleRequest) may
return without invoking dispatch; update the code to either (a) hoist the
enforceBudget call up into handleTranslatedJSON before the cache lookup so
budget is always checked, or (b) add an explicit comment in
handleWithCache/handleTranslatedJSON documenting that bypassing enforceBudget on
cache hits is intentional (cache hits incur no provider cost), and ensure
dispatchChatCompletion/dispatchResponses still call enforceBudget when dispatch
runs; reference enforceBudget, dispatchChatCompletion, dispatchResponses,
handleWithCache, handleTranslatedJSON, and responseCache.HandleRequest to locate
the spots to change.
---
Outside diff comments:
In `@internal/auditlog/middleware.go`:
- Around line 581-600: EnrichEntryWithError currently trims the provided
errorCode but assigns it even when empty, which can overwrite a prior non-empty
entry.Data.ErrorCode; update the function (EnrichEntryWithError) to trim the
first errorCode and only set entry.Data.ErrorCode when the trimmed value is
non-empty (preserving existing value otherwise), keeping the existing guards for
entry and entry.Data and the current behavior for ErrorType and ErrorMessage.
In `@internal/server/error_support.go`:
- Around line 18-33: The fallback branch should include the same error code
parameter for symmetry: update the auditlog.EnrichEntryWithError call in the
fallback branch (after core.NewProviderError(...) and logHandledError(c,
gatewayErr)) to pass gatewayErrorCode(gatewayErr) as the fourth argument so it
matches the earlier call pattern (functions involved: gatewayErrorCode,
core.NewProviderError, logHandledError, auditlog.EnrichEntryWithError).
In `@internal/workflows/types.go`:
- Around line 74-101: The canonicalization currently sets Fallback to true when
nil but not Budget, causing semantically identical payloads (omitted budget vs
"budget": true) to hash differently; update FeatureFlags.canonicalize() to also
set Budget to true when f.Budget == nil, then simplify runtimeFeatures() to use
*f.Budget directly (like Fallback) so core.WorkflowFeatures.Budget is derived
consistently and workflow_hash/normalizePayload remain stable for equivalent
payloads.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 4f723acd-8440-4479-8731-cc4f999fd9a5
📒 Files selected for processing (63)
config/config.example.yamlconfig/config.goconfig/config_test.godocs/advanced/admin-endpoints.mdxdocs/advanced/config-yaml.mdxdocs/advanced/configuration.mdxdocs/advanced/workflows.mdxdocs/docs.jsondocs/features/budgets.mdxdocs/features/user-path.mdxdocs/getting-started/quickstart.mdxdocs/openapi.jsoninternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/budgets.jsinternal/admin/dashboard/static/js/modules/budgets.test.jsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.jsinternal/admin/dashboard/static/js/modules/timezone.jsinternal/admin/dashboard/static/js/modules/timezone.test.jsinternal/admin/dashboard/static/js/modules/workflows-layout.test.jsinternal/admin/dashboard/static/js/modules/workflows.jsinternal/admin/dashboard/static/js/modules/workflows.test.jsinternal/admin/dashboard/templates/index.htmlinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/page-budgets.htmlinternal/admin/dashboard/templates/page-settings.htmlinternal/admin/dashboard/templates/page-workflows.htmlinternal/admin/dashboard/templates/sidebar.htmlinternal/admin/dashboard/templates/workflow-chart.htmlinternal/admin/handler.gointernal/admin/handler_budgets_test.gointernal/admin/handler_test.gointernal/app/app.gointernal/app/app_test.gointernal/auditlog/auditlog.gointernal/auditlog/middleware.gointernal/budget/factory.gointernal/budget/service.gointernal/budget/service_test.gointernal/budget/settings_helpers.gointernal/budget/store.gointernal/budget/store_mongodb.gointernal/budget/store_postgresql.gointernal/budget/store_sqlite.gointernal/budget/store_sqlite_test.gointernal/budget/types.gointernal/budget/types_test.gointernal/core/workflow.gointernal/core/workflow_test.gointernal/gateway/batch_orchestrator.gointernal/gateway/batch_orchestrator_test.gointernal/server/budget_support.gointernal/server/budget_support_test.gointernal/server/error_support.gointernal/server/error_support_test.gointernal/server/handlers.gointernal/server/http.gointernal/server/internal_chat_completion_executor.gointernal/server/native_batch_service.gointernal/server/passthrough_service.gointernal/server/translated_inference_service.gointernal/workflows/types.gointernal/workflows/types_test.go
Comment on lines
+329
to
+335
| adminAPI.GET("/budgets", cfg.AdminHandler.ListBudgets) | ||
| adminAPI.PUT("/budgets", cfg.AdminHandler.UpsertBudget) | ||
| adminAPI.DELETE("/budgets", cfg.AdminHandler.DeleteBudget) | ||
| adminAPI.GET("/budgets/settings", cfg.AdminHandler.BudgetSettings) | ||
| adminAPI.PUT("/budgets/settings", cfg.AdminHandler.UpdateBudgetSettings) | ||
| adminAPI.POST("/budgets/reset-one", cfg.AdminHandler.ResetBudget) | ||
| adminAPI.POST("/budgets/reset", cfg.AdminHandler.ResetBudgets) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
API surface inconsistency — PUT/DELETE /budgets use a JSON-body identifier while peer admin resources use path-scoped keys.
/aliases/:name, /model-overrides/:selector, /guardrails/:name, and /workflows/:id all use path-scoped PUT/DELETE. /budgets instead overloads PUT /budgets (upsert) and DELETE /budgets (delete) on a request body. This works, but:
- It complicates HTTP cache/proxy handling (DELETE with a body is allowed but discouraged by RFC 9110 §9.3.5).
- It diverges from the existing admin REST style, hurting discoverability.
If the compound (user_path, period) key is the blocker, consider URL-encoding the user path and using something like PUT /budgets/:user_path/:period and DELETE /budgets/:user_path/:period (or a query-string form), keeping the body for the limit amount only. Not a blocker, but worth a second look before this surface ships.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/http.go` around lines 329 - 335, The routes for budget
upsert/delete currently expose PUT /budgets -> cfg.AdminHandler.UpsertBudget and
DELETE /budgets -> cfg.AdminHandler.DeleteBudget which accept the compound
(user_path, period) via JSON body; change them to path-scoped routes (e.g. PUT
/budgets/:user_path/:period and DELETE /budgets/:user_path/:period) so they
match other admin endpoints and avoid bodies on DELETE; URL-encode user_path in
clients and keep the request body only for the numeric limit (amount) on
UpsertBudget; update the handler signatures (UpsertBudget, DeleteBudget) to read
params from the URL (user_path, period) instead of the request body and
validate/parse them accordingly.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
internal/admin/dashboard/static/js/dashboard.js (1)
223-225:⚠️ Potential issue | 🟡 MinorBudget page still double-fetches on first load.
On direct navigation to
/admin/dashboard/budgets,init()calls_applyRoute()beforefetchAll(), so Lines 223-225 startfetchBudgetsPage()immediately and Lines 489-494 queue it again throughdashboardDataFetches(). That still races two identical requests and can flicker the loading/error state.Also applies to: 489-494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/js/dashboard.js` around lines 223 - 225, The budget page double-fetch occurs because _applyRoute() calls fetchBudgetsPage() immediately while fetchAll()/dashboardDataFetches() also queues the same fetch, causing two concurrent requests; fix by removing the immediate call from _applyRoute() (the if (page === "budgets" && typeof this.fetchBudgetsPage === "function") block) and rely on fetchAll()/dashboardDataFetches() to schedule fetchBudgetsPage(), or alternatively add a guard inside fetchBudgetsPage() or dashboardDataFetches() to detect an already-enqueued/in-flight budgets fetch (e.g., check a boolean flag or an existing promise) and skip scheduling a duplicate—update code around init(), _applyRoute(), fetchAll(), dashboardDataFetches(), and fetchBudgetsPage() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 7: Update the CHANGELOG migration note about budgets to explicitly show
the default environment variable values and provide a minimal example: mention
defaults USAGE_ENABLED=false and BUDGETS_ENABLED=true, explain that when
USAGE_ENABLED is false the budget subsystem is disabled (behavior implemented in
internal/app/app.go around the budget/usage checks), and add a short bash
snippet showing how to enable both (set USAGE_ENABLED=true and
BUDGETS_ENABLED=true). Keep the text concise, split into 2–3 short sentences or
bullets (default values, observed behavior, example), and ensure it references
the conditions checked in internal/app/app.go.
In `@config/config.go`:
- Around line 1303-1328: The current applyBudgetEnv appends BudgetUserPathConfig
entries to cfg.Budgets.UserPaths, causing duplicate logical paths when an env
var overrides a YAML entry; update applyBudgetEnv so that for each env-derived
path (budgetEnvPath) you first check cfg.Budgets.UserPaths for an existing
BudgetUserPathConfig with the same Path and either replace it (remove the old
entry and append the new one) or, alternatively, detect duplicate (user_path,
period) keys across existing limits and the parsed limits from
parseBudgetEnvLimits and return an error; reference applyBudgetEnv,
cfg.Budgets.UserPaths, BudgetUserPathConfig, budgetEnvPath and
parseBudgetEnvLimits when making the change.
In `@docs/openapi.json`:
- Around line 4675-4681: The OpenAPI schema for admin.upsertBudgetRequest
currently leaves the amount property optional; update the
admin.upsertBudgetRequest object to require amount by adding a required array
that includes "amount" so generated clients and server validation enforce that
PUT /admin/api/v1/budgets/{user_path}/{period} cannot accept an empty body;
locate the admin.upsertBudgetRequest definition and add required: ["amount"]
alongside the existing properties.
In `@internal/admin/dashboard/static/js/modules/budgets.test.cjs`:
- Around line 351-365: Add a test for the happy path of resetBudgets: set
module.budgetResetConfirmation to the correct typed string, replace module.fetch
with a spy that verifies it was called with a POST and returns a successful
Response-like object (e.g. { ok: true }), keep or stub module.requestOptions and
module.handleFetchResponse as needed, call await module.resetBudgets(), and then
assert that module.budgetSettingsError is empty, module.budgetResetLoading is
false, the fetch spy was invoked (with POST), and that any refresh/notification
behavior the module triggers after success occurred (verify by stubbing or
spying on the module method it calls to refresh data or show a success notice).
Ensure you reference resetBudgets, module.fetch, requestOptions,
handleFetchResponse, budgetResetConfirmation, budgetSettingsError, and
budgetResetLoading when adding the assertions.
In `@internal/admin/handler.go`:
- Around line 1031-1044: ListBudgets and ResetBudgets are creating provider
errors inline with core.NewProviderError("budgets", ...) instead of using the
existing budgetServiceError helper; replace those inline error constructions
with calls to budgetServiceError(ctxErr) (i.e., return handleError(c,
budgetServiceError(err)) or equivalent) so both endpoints funnel budget service
failures through the single budgetServiceError wrapper (refer to
Handler.ListBudgets and Handler.ResetBudgets and the budgets.Statuses /
budgets.Reset usages to locate the spots).
In `@internal/budget/factory.go`:
- Around line 116-129: The loop that builds budgets from entry.Limits must
eagerly fail when a period can't be resolved: if limit.PeriodSeconds <= 0 and
PeriodSeconds(limit.Period) returns !ok, return an error (or propagate one) from
the factory function with contextual details (include userPath and limit.Period
or limit index) instead of appending a Budget with PeriodSeconds=0; update the
code around the for loop that constructs Budget entries (references:
entry.Limits, PeriodSeconds(limit.Period), Budget{UserPath, PeriodSeconds,
Amount, Source}) so callers (currently ReplaceConfigBudgets/NormalizeBudget)
receive a clear, immediate message about the invalid period value.
In `@internal/budget/store_mongodb.go`:
- Around line 130-145: The transaction call using session.WithTransaction can
fail on standalone mongod; modify the transaction wrapper in the Mongo budget
store to detect transaction-capability errors (use the driver helper
IsTransientTransactionError or inspect the error text from
session.WithTransaction) and if detected: log a clear warning and fall back to
calling replaceConfigBudgets (and similarly SaveSettings) non-transactionally
instead of returning an error; ensure you reference and wrap calls to
replaceConfigBudgets and SaveSettings so they can run both inside the
transaction callback and directly as a fallback, and keep the original error
returned for unexpected failures.
In `@internal/server/budget_support_test.go`:
- Around line 117-152: Add a new test (e.g.,
TestBudgetCheckFailedResponseMapping) that covers the non-Exceeded branch by
calling budgetCheckError with a generic/failure budget error (not
budget.ExceededError), passing its result to handleError, and asserting the
response is NOT the 429 path and instead contains the generic
budget_check_failed mapping (inspect response body JSON for the
"budget_check_failed" error/type string and that the HTTP status differs from
http.StatusTooManyRequests); reference budgetCheckError and handleError in
internal/server/budget_support.go to locate the mapping logic.
In `@internal/server/budget_support.go`:
- Around line 62-63: The client-facing error currently includes raw backend text
via fmt.Sprintf("budget check failed: %v", err); change the NewProviderError
call in budget_support.go (the core.NewProviderError(...) followed by
.WithCode("budget_check_failed")) to use a fixed, generic 503 message (e.g.
"budget check failed") while still passing the original err as the wrapped cause
to preserve logs/debugging; keep the WithCode("budget_check_failed") call
unchanged.
In `@internal/usage/cache_type_test.go`:
- Around line 24-37: Add table-driven tests for normalizedUsageEntryForStorage
to cover the fallback-to-root and no-op cloning behaviors: create cases where
UsageEntry.UserPath triggers core.NormalizeUserPath to error (e.g., contains
".." or ":" or yields empty) and assert the returned UserPath is "/" and the
original entry is not mutated; also add cases where UserPath is already
canonical and CacheType/ProviderName are unchanged and assert the function
returns the same pointer (no clone) and no mutation occurs. Use the existing
UsageEntry struct and call normalizedUsageEntryForStorage in each subtest,
comparing pointer identity and field values, and include cases that vary
CacheType/ProviderName to ensure cloning happens when they differ.
---
Duplicate comments:
In `@internal/admin/dashboard/static/js/dashboard.js`:
- Around line 223-225: The budget page double-fetch occurs because _applyRoute()
calls fetchBudgetsPage() immediately while fetchAll()/dashboardDataFetches()
also queues the same fetch, causing two concurrent requests; fix by removing the
immediate call from _applyRoute() (the if (page === "budgets" && typeof
this.fetchBudgetsPage === "function") block) and rely on
fetchAll()/dashboardDataFetches() to schedule fetchBudgetsPage(), or
alternatively add a guard inside fetchBudgetsPage() or dashboardDataFetches() to
detect an already-enqueued/in-flight budgets fetch (e.g., check a boolean flag
or an existing promise) and skip scheduling a duplicate—update code around
init(), _applyRoute(), fetchAll(), dashboardDataFetches(), and
fetchBudgetsPage() accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c7801de8-e245-4872-94d8-c67f47d15aa8
📒 Files selected for processing (41)
CHANGELOG.mdconfig/config.example.yamlconfig/config.goconfig/config_test.godocs/advanced/admin-endpoints.mdxdocs/advanced/config-yaml.mdxdocs/advanced/configuration.mdxdocs/features/budgets.mdxdocs/openapi.jsoninternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/budgets.jsinternal/admin/dashboard/static/js/modules/budgets.test.cjsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.cjsinternal/admin/dashboard/static/js/modules/timezone.test.cjsinternal/admin/dashboard/static/js/modules/workflows-layout.test.cjsinternal/admin/dashboard/static/js/modules/workflows.test.cjsinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/page-settings.htmlinternal/admin/dashboard/templates/sidebar.htmlinternal/admin/handler.gointernal/admin/handler_budgets_test.gointernal/admin/handler_test.gointernal/app/app.gointernal/auditlog/middleware.gointernal/budget/factory.gointernal/budget/service.gointernal/budget/service_test.gointernal/budget/settings_helpers.gointernal/budget/store_mongodb.gointernal/budget/types_test.gointernal/core/workflow_test.gointernal/server/budget_support.gointernal/server/budget_support_test.gointernal/server/error_support.gointernal/server/http.gointernal/server/translated_inference_service.gointernal/usage/cache_type.gointernal/usage/cache_type_test.gointernal/workflows/types.gointernal/workflows/types_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/admin/dashboard/static/js/modules/workflows.js (1)
562-575:⚠️ Potential issue | 🟠 MajorPreserve stored feature flags when hydrating the editor.
workflowSourceFeatures()applies the current runtime caps before returning flags. If an existing workflow is opened whileUSAGE_ENABLEDorBUDGETS_ENABLEDis off, Line 574 writes that maskedfalsevalue intoworkflowForm.features.budget; the next save then persists the disabled state viabuildWorkflowRequest(), even if the user never touched the toggle. Hydrate the form fromworkflow.workflow_payload.featuresand reserveworkflowApplyGlobalFeatureCaps()for preview/effective-state rendering.💡 Proposed fix
- const features = this.workflowSourceFeatures(workflow); + const features = this.workflowNormalizedFeatures( + workflow && workflow.workflow_payload && workflow.workflow_payload.features + ); const guardrails = this.workflowSourceGuardrails(workflow); this.workflowForm = { scope_provider: this.workflowScopeProviderValue(workflow.scope), scope_model: String(workflow.scope && workflow.scope.scope_model || ''), scope_user_path: String(workflow.scope && workflow.scope.scope_user_path || ''), name: String(workflow.name || ''), description: String(workflow.description || ''), features: { cache: !!features.cache, audit: !!features.audit, usage: !!features.usage, budget: !!features.budget, guardrails: !!features.guardrails, fallback: !!features.fallback },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/js/modules/workflows.js` around lines 562 - 575, The form hydration currently uses workflowSourceFeatures(workflow), which applies runtime global caps and can overwrite stored flags (e.g., workflowForm.features.budget) with masked false values; instead, read the saved feature flags from workflow.workflow_payload.features (falling back to workflowSourceFeatures(workflow) only if payload.features is missing) when populating workflowForm.features and workflowForm.guardrails, and keep workflowApplyGlobalFeatureCaps()/workflowSourceFeatures for preview/effective-state rendering only so saves persist the original stored flags unless the user explicitly toggles them.
♻️ Duplicate comments (1)
internal/usage/cache_type_test.go (1)
24-37: 🧹 Nitpick | 🔵 TrivialOptional: fold this single-case test into the table-driven suite below.
TestNormalizedUsageEntryForStorageNormalizesUserPathexercises a scenario (team/alpha→/team/alpha, with a clone and unchanged input) that fits naturally as another row inTestNormalizedUsageEntryForStorageUserPathFallbackAndCloneBehavior. Consolidating avoids two test functions covering overlapping invariants and keeps theUserPathcases discoverable in one place.♻️ Suggested consolidation
-func TestNormalizedUsageEntryForStorageNormalizesUserPath(t *testing.T) { - entry := &UsageEntry{ - ID: "usage-1", - UserPath: " team/alpha ", - } - - got := normalizedUsageEntryForStorage(entry) - if got.UserPath != "/team/alpha" { - t.Fatalf("normalized UserPath = %q, want /team/alpha", got.UserPath) - } - if entry.UserPath != " team/alpha " { - t.Fatalf("input UserPath mutated to %q", entry.UserPath) - } -} -And add a row to the table in
TestNormalizedUsageEntryForStorageUserPathFallbackAndCloneBehavior:{ name: "trims whitespace and prepends slash", entry: UsageEntry{ID: "usage-0", UserPath: " team/alpha "}, wantUserPath: "/team/alpha", },As per coding guidelines: "Prefer ... table-driven tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/usage/cache_type_test.go` around lines 24 - 37, Remove the standalone TestNormalizedUsageEntryForStorageNormalizesUserPath and instead add its case to the table in TestNormalizedUsageEntryForStorageUserPathFallbackAndCloneBehavior: add a row named "trims whitespace and prepends slash" with entry UsageEntry{ID: "usage-0", UserPath: " team/alpha "}, wantUserPath "/team/alpha"; ensure the table-driven test still calls normalizedUsageEntryForStorage and verifies both the returned object's UserPath and that the original entry.UserPath remains unchanged, referencing the UsageEntry type and normalizedUsageEntryForStorage function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 9-12: Add a blank line before and after the fenced code block that
sits under the list item so the markdown fence is not nested directly beneath
the list marker; locate the fenced example under the "Minimal enablement:" list
item and insert one empty line above the opening ``` and one empty line below
the closing ``` to satisfy markdownlint.
In `@config/config.go`:
- Around line 1415-1439: validateBudgetConfig resolves PeriodSeconds but doesn't
reject duplicate logical budget keys; update validateBudgetConfig to detect and
reject duplicates after normalization by tracking seen keys per user
path/period. For each entry in cfg.UserPaths and each limit in entry.Limits,
determine the final period seconds (use limit.PeriodSeconds if >0, otherwise
call budgetPeriodSeconds(limit.Period) and set
cfg.UserPaths[pathIdx].Limits[limitIdx].PeriodSeconds = seconds as currently
done), then build a unique key like trimmed entry.Path + ":" + seconds and check
a map[string]struct{} (or map[string]map[int]struct{}) for prior occurrence; if
the key already exists return an error indicating duplicate budgets for that
path/period. Ensure you still validate amount with math.IsNaN/IsInf and only add
the key after successful validation/normalization.
In `@docs/openapi.json`:
- Around line 4646-4674: The handler currently decodes requests into
budget.Settings (plain ints) and calls SaveSettings, so omitted JSON fields
become zeros and overwrite existing values; fix by changing the request handling
to support patch semantics: create a dedicated request type with pointer fields
(e.g., UpdateBudgetSettingsRequest with *int fields) or use json.RawMessage to
detect presence, unmarshal the incoming body into that type, call the current
settings getter (e.g., GetSettings), update only fields that are non-nil in the
request, then call SaveSettings with the merged result; alternatively, if you
want full-replace semantics, update the admin.updateBudgetSettingsRequest
OpenAPI schema to mark all fields required and validate that the handler
enforces all fields are present before calling SaveSettings.
In `@internal/admin/handler.go`:
- Around line 1064-1067: The request decoding for the budget endpoints currently
uses json.NewDecoder(c.Request().Body).Decode(&req) which diverges from the rest
of this handler; replace those decoders with Echo's c.Bind(&req) wherever you
see upsertBudgetRequest decoding (e.g., in UpsertBudget and the other budget
handlers UpdateBudgetSettings, ResetBudget, ResetBudgets and the occurrences
around lines referenced) so the code calls c.Bind(&req) and preserves the
existing error handling via handleError/core.NewInvalidRequestError; apply the
same change to the other mentioned occurrences (1145-1148, 1185-1188, 1219-1222)
to keep content-type negotiation and middleware behavior consistent.
In `@internal/budget/service.go`:
- Around line 118-130: SaveSettings currently discards the successfully
persisted snapshot if s.Refresh fails; change Service.SaveSettings so that after
calling s.store.SaveSettings(ctx, settings) it always returns the saved value to
the caller even if s.Refresh(ctx) returns an error — call s.Refresh, capture any
refresh error, but return the persisted saved result (Settings) and surface the
refresh error by wrapping it (or logging it) instead of returning an empty
Settings; refer to Service.SaveSettings, s.store.SaveSettings, and s.Refresh to
locate and update the logic.
- Around line 198-234: The results slice in CheckWithResults is allocated with
zero capacity causing repeated growth; pre-allocate it to len(budgets) to avoid
allocations on the hot path. Update the initialization of results in
CheckWithResults (the results variable defined just before the for loop over
budgets) to use make([]CheckResult, 0, len(budgets)) so capacity equals the
upper bound of matching budgets and existing logic appending results remains
unchanged.
In `@internal/budget/store_mongodb_test.go`:
- Around line 12-75: The tests only cover helper logic and miss exercising the
Mongo-backed persistence paths in internal/budget/store_mongodb.go; add
end-to-end table-driven tests that instantiate a real (or in-memory/test) Mongo
collection and exercise UpsertBudgets, ReplaceConfigBudgets, ListBudgets and the
reset-anchor preservation paths to validate request translation, response
normalization, error handling, default configuration, and provider-specific
parameter mapping; for each function (UpsertBudgets, ReplaceConfigBudgets,
ListBudgets) include cases for normal success, idempotent/upsert behavior,
conflict/error from Mongo, and legacy root-row handling, then assert DB state
and returned values match expectations and that reset-anchor is preserved across
operations.
In `@internal/budget/store_mongodb.go`:
- Around line 33-50: The compound unique index created in
store.budgets.Indexes().CreateMany already covers prefix/equality queries on
user_path, so remove the redundant standalone mongo.IndexModel with Keys:
bson.D{{Key: "user_path", Value: 1}} from the CreateMany call to avoid extra
write amplification during upserts; keep the compound index {user_path,
period_seconds} and the {period_seconds:1} index (and leave the settings index
creation unchanged).
- Around line 75-104: Replace the per-item UpdateOne loop in
upsertNormalizedBudgets with a single BulkWrite: build a slice of
mongo.WriteModel using mongo.NewUpdateOneModel() for each budget, call
SetFilter(bson.D{{"user_path", budget.UserPath}, {"period_seconds",
budget.PeriodSeconds}}), SetUpdate(...) with the same $set and $setOnInsert
documents you already construct, and SetUpsert(true); then call
s.budgets.BulkWrite(ctx, models) (after early-return on empty budgets) and
return a wrapped error on failure similar to the existing fmt.Errorf("upsert
budget %s/%d: %w", ...). Keep normalizeBudgetsForUpsert and behavior unchanged,
only replace the loop in upsertNormalizedBudgets with the BulkWrite approach.
- Around line 272-296: The isMongoTransactionCapabilityError function currently
relies on fragile message substring checks; update it to prefer structured
driver checks first: use errors.As to detect mongo.CommandError and call
HasErrorCode(20) (IllegalOperation) for the "transaction numbers only allowed on
replica set" case, and use errors.As to detect mongo.LabeledError and call
HasErrorLabel("TransientTransactionError") for label-based detection; only fall
back to minimal, well-scoped string checks if neither structured check applies,
and remove broad phrase matching in capabilityPhrases so the logic centers on
CommandError.HasErrorCode and LabeledError.HasErrorLabel.
- Around line 409-414: Update mongoUncachedUsageMatch to include the missing nil
check for the cache_type field so it matches the behavior of
mongoCacheModeFilter in the usage reader: add an additional bson.D entry
checking for cache_type equal to nil (Value: nil) into the $or array alongside
the existing $exists:false and empty-string checks in the
mongoUncachedUsageMatch function.
In `@internal/budget/store_postgresql.go`:
- Around line 89-128: UpsertBudgets currently duplicates the INSERT...ON
CONFLICT loop found in upsertPostgreSQLBudgets; refactor UpsertBudgets to
normalize budgets, begin a transaction (with defer tx.Rollback), then call the
existing helper upsertPostgreSQLBudgets (or the helper used by
ReplaceConfigBudgets) to perform the loop, and finally commit the
transaction—preserving the same error wrapping and nil/empty checks so the SQL
and bind order are maintained in one place.
- Around line 53-60: Remove the redundant single-column index creation `CREATE
INDEX IF NOT EXISTS idx_budgets_user_path ON budgets(user_path)` from the slice
that is iterated with pool.Exec; the primary key on (user_path, period_seconds)
already covers queries on user_path so only keep the
`idx_budgets_period_seconds` entry. Update the index creation loop in the same
block (where pool.Exec is called) to only attempt creating the period_seconds
index and leave the existing error handling unchanged.
- Around line 253-259: In ResetAllBudgets, avoid calling at.UTC() twice: compute
utc := at.UTC() once, use utc.Unix() for both bind parameters when calling
s.pool.Exec (same pattern as ResetBudget), so update the s.pool.Exec call to
pass the precomputed unix timestamp variable instead of calling at.UTC() twice;
ensure the function still returns the formatted error on failure.
- Around line 261-280: SumUsageCost uses an expression-based normalized
user_path filter (via usagePathMatchesBudgetExpr) which is not covered by the
existing plain btree idx_usage_user_path; add a matching expression index to
make that lookup sargable by creating an index with
COALESCE(NULLIF(TRIM(user_path), ''), '/') for PostgreSQL and the equivalent
expression for SQLite. Update internal/usage/store_postgresql.go to include a
CREATE INDEX IF NOT EXISTS idx_usage_user_path_normalized ON
usage(COALESCE(NULLIF(TRIM(user_path), ''), '/')) in the index setup routine,
and mirror an appropriate expression/index statement into
internal/usage/store_sqlite.go so the SumUsageCost path (and
usagePathMatchesBudgetExpr) can use the index.
In `@internal/budget/store_sqlite.go`:
- Around line 168-207: ReplaceConfigBudgets currently causes manual rows to
become SourceConfig because upsertSQLiteBudgets's ON CONFLICT DO UPDATE sets
source = excluded.source; change upsertSQLiteBudgets so it does not overwrite an
existing non-config row: either remove source from the DO UPDATE list or add a
WHERE clause on the DO UPDATE that only applies updates when the existing row
already has source = SourceConfig (e.g., "DO UPDATE SET ... WHERE budgets.source
= ?"). Update the function upsertSQLiteBudgets and its SQL/arguments accordingly
so collisions preserve SourceManual unless the existing row is already
SourceConfig.
In `@internal/budget/types.go`:
- Around line 232-239: anchoredMonthStart's use of now.AddDate(0, -1, 0) can
normalize months incorrectly for day-31 anchors; change the previous-month
computation to anchor at day 1 before subtracting a month to avoid overflow
normalization. Specifically, in anchoredMonthStart replace prev :=
now.AddDate(0, -1, 0) with constructing a time for the first day of the current
month (preserving location/time-of-day if needed), subtract one month from that
(e.g., prevFirst := time.Date(now.Year(), now.Month(), 1, now.Hour(),
now.Minute(), now.Second(), now.Nanosecond(), now.Location()); prev :=
prevFirst.AddDate(0, -1, 0)), then call monthAnchor(prev.Year(), prev.Month(),
day, hour, minute) so monthAnchor computes the true previous-month anchor
(function anchoredMonthStart and monthAnchor are the locations to update).
In `@internal/server/base_path.go`:
- Around line 37-42: When req.URL.RawPath is non-empty but
strippedRawPath(req.URL.RawPath, basePath) returns an empty string (indicating
decoded fallback failed) do not silently set urlCopy.RawPath = "" and continue;
instead detect this case after computing strippedRaw (the result of
strippedRawPath) and short-circuit the request with a 400 Bad Request (or at
minimum log a debug/error) so downstream handlers/audit see the original client
encoding is invalid. Update the code around urlCopy / cloned.RequestURI to check
if req.URL.RawPath != "" && strippedRaw == "" and then return an HTTP 400 (or
log and abort) rather than assigning an empty RawPath and building a RequestURI
from strippedPath.
- Around line 82-104: stripRawPathByDecodedBase currently validates using
stripBasePath(decoded, basePath) but then slices rawPath by segment counts
(rawParts/baseParts), which breaks when a single raw segment contains
percent-encoded slashes; update stripRawPathByDecodedBase to not rely on segment
counts: either (A) iterate the rawPath bytes in parallel with the decoded prefix
(use url.PathUnescape on progressively consumed raw bytes or decode runes) and
stop consuming raw bytes once you have consumed decoded bytes equal to
len(config.NormalizeBasePath(basePath)), then return the remaining raw suffix,
or (B) detect any percent-encoded '/' in rawParts[1:len(baseParts)+1] and fail
early (return "") so we don’t produce a misaligned suffix; keep references to
stripBasePath(decoded, basePath), config.NormalizeBasePath(basePath), rawParts
and baseParts so the change is applied in stripRawPathByDecodedBase.
---
Outside diff comments:
In `@internal/admin/dashboard/static/js/modules/workflows.js`:
- Around line 562-575: The form hydration currently uses
workflowSourceFeatures(workflow), which applies runtime global caps and can
overwrite stored flags (e.g., workflowForm.features.budget) with masked false
values; instead, read the saved feature flags from
workflow.workflow_payload.features (falling back to
workflowSourceFeatures(workflow) only if payload.features is missing) when
populating workflowForm.features and workflowForm.guardrails, and keep
workflowApplyGlobalFeatureCaps()/workflowSourceFeatures for
preview/effective-state rendering only so saves persist the original stored
flags unless the user explicitly toggles them.
---
Duplicate comments:
In `@internal/usage/cache_type_test.go`:
- Around line 24-37: Remove the standalone
TestNormalizedUsageEntryForStorageNormalizesUserPath and instead add its case to
the table in TestNormalizedUsageEntryForStorageUserPathFallbackAndCloneBehavior:
add a row named "trims whitespace and prepends slash" with entry UsageEntry{ID:
"usage-0", UserPath: " team/alpha "}, wantUserPath "/team/alpha"; ensure the
table-driven test still calls normalizedUsageEntryForStorage and verifies both
the returned object's UserPath and that the original entry.UserPath remains
unchanged, referencing the UsageEntry type and normalizedUsageEntryForStorage
function.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 66a8d749-69c0-4b54-88cb-cc6245994ee1
📒 Files selected for processing (25)
CHANGELOG.mdconfig/config.goconfig/config_test.godocs/openapi.jsoninternal/admin/dashboard/static/js/modules/budgets.test.cjsinternal/admin/dashboard/static/js/modules/workflows.jsinternal/admin/dashboard/static/js/modules/workflows.test.cjsinternal/admin/handler.gointernal/admin/handler_budgets_test.gointernal/budget/factory.gointernal/budget/service.gointernal/budget/service_test.gointernal/budget/store.gointernal/budget/store_mongodb.gointernal/budget/store_mongodb_test.gointernal/budget/store_postgresql.gointernal/budget/store_sqlite.gointernal/budget/store_sqlite_test.gointernal/budget/types.gointernal/budget/types_test.gointernal/server/base_path.gointernal/server/budget_support.gointernal/server/budget_support_test.gointernal/server/http_test.gointernal/usage/cache_type_test.go
Comment on lines
+261
to
+280
| func (s *PostgreSQLStore) SumUsageCost(ctx context.Context, userPath string, start, end time.Time) (float64, bool, error) { | ||
| userPath, err := NormalizeUserPath(userPath) | ||
| if err != nil { | ||
| return 0, false, err | ||
| } | ||
| userPathExpr := usagePathMatchesBudgetExpr("user_path") | ||
| query := `SELECT SUM(total_cost) FROM "usage" | ||
| WHERE timestamp >= $1 | ||
| AND timestamp < $2 | ||
| AND (` + userPathExpr + ` = $3 OR ` + userPathExpr + ` LIKE $4 ESCAPE '\') | ||
| AND (cache_type IS NULL OR cache_type = '')` | ||
| var total *float64 | ||
| if err := s.pool.QueryRow(ctx, query, start.UTC(), end.UTC(), userPath, usagePathLikePattern(userPath)).Scan(&total); err != nil { | ||
| return 0, false, fmt.Errorf("sum usage cost: %w", err) | ||
| } | ||
| if total == nil { | ||
| return 0, false, nil | ||
| } | ||
| return *total, true, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the path-matching helpers and any usage-table index creation.
rg -nP --type=go -C3 '\b(usagePathMatchesBudgetExpr|usagePathLikePattern)\b'
echo "--- index creation on usage(user_path) ---"
rg -nP --type=go -C2 'CREATE\s+INDEX[^;]*\busage\b[^;]*user_path|idx_usage_user_path'Repository: ENTERPILOT/GoModel
Length of output: 3791
🏁 Script executed:
rg -nP --type=go 'CREATE\s+INDEX.*ON\s+usage\(' --multiline -A3Repository: ENTERPILOT/GoModel
Length of output: 2364
🏁 Script executed:
rg -nP --type=go 'COALESCE.*NULLIF.*TRIM' -B3 -A3Repository: ENTERPILOT/GoModel
Length of output: 1541
Create an expression index to make the normalized user_path lookup sargable.
SumUsageCost wraps user_path in COALESCE(NULLIF(TRIM(user_path), ''), '/') for the filter condition, but the existing idx_usage_user_path is a plain btree index on the raw column and cannot optimize expression-based lookups. Add an expression index matching the expression:
CREATE INDEX IF NOT EXISTS idx_usage_user_path_normalized
ON usage(COALESCE(NULLIF(TRIM(user_path), ''), '/'))Add this to both internal/usage/store_postgresql.go (PostgreSQL indexes) and internal/usage/store_sqlite.go (SQLite indexes) to ensure the budget enforcement hot path can use index acceleration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/budget/store_postgresql.go` around lines 261 - 280, SumUsageCost
uses an expression-based normalized user_path filter (via
usagePathMatchesBudgetExpr) which is not covered by the existing plain btree
idx_usage_user_path; add a matching expression index to make that lookup
sargable by creating an index with COALESCE(NULLIF(TRIM(user_path), ''), '/')
for PostgreSQL and the equivalent expression for SQLite. Update
internal/usage/store_postgresql.go to include a CREATE INDEX IF NOT EXISTS
idx_usage_user_path_normalized ON usage(COALESCE(NULLIF(TRIM(user_path), ''),
'/')) in the index setup routine, and mirror an appropriate expression/index
statement into internal/usage/store_sqlite.go so the SumUsageCost path (and
usagePathMatchesBudgetExpr) can use the index.
Comment on lines
+168
to
+207
| func (s *SQLiteStore) ReplaceConfigBudgets(ctx context.Context, budgets []Budget) error { | ||
| budgets, err := normalizeBudgetsForUpsert(budgets) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for i := range budgets { | ||
| budgets[i].Source = SourceConfig | ||
| } | ||
|
|
||
| tx, err := s.db.BeginTx(ctx, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("begin config budget replace: %w", err) | ||
| } | ||
| defer tx.Rollback() //nolint:errcheck | ||
|
|
||
| if len(budgets) == 0 { | ||
| if _, err := tx.ExecContext(ctx, `DELETE FROM budgets WHERE source = ?`, SourceConfig); err != nil { | ||
| return fmt.Errorf("delete old config budgets: %w", err) | ||
| } | ||
| } else { | ||
| conditions := make([]string, 0, len(budgets)) | ||
| args := make([]any, 0, 1+len(budgets)*2) | ||
| args = append(args, SourceConfig) | ||
| for _, budget := range budgets { | ||
| conditions = append(conditions, `(user_path = ? AND period_seconds = ?)`) | ||
| args = append(args, budget.UserPath, budget.PeriodSeconds) | ||
| } | ||
| query := `DELETE FROM budgets WHERE source = ? AND NOT (` + strings.Join(conditions, " OR ") + `)` | ||
| if _, err := tx.ExecContext(ctx, query, args...); err != nil { | ||
| return fmt.Errorf("delete old config budgets: %w", err) | ||
| } | ||
| } | ||
| if err := upsertSQLiteBudgets(ctx, tx, budgets); err != nil { | ||
| return err | ||
| } | ||
| if err := tx.Commit(); err != nil { | ||
| return fmt.Errorf("commit config budget replace: %w", err) | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for tests that establish overlap precedence between config and manual budgets
rg -nP -C5 'SourceConfig|SourceManual|ReplaceConfigBudgets' --type=go -g '*_test.go'
# And surface any documentation on this contract
rg -nP -C2 'config-?source|manual budget|source\s*=\s*config' README.md docs/ 2>/dev/nullRepository: ENTERPILOT/GoModel
Length of output: 8953
🏁 Script executed:
# First, let's look at the upsertSQLiteBudgets function to confirm the ON CONFLICT behavior
rg -nP -A20 'func upsertSQLiteBudgets' --type=go internal/budget/store_sqlite.goRepository: ENTERPILOT/GoModel
Length of output: 788
🏁 Script executed:
# Check if there's any test that actually creates a config/manual collision on the same key
rg -nP -B5 -A15 'SourceConfig.*SourceManual|SourceManual.*SourceConfig' --type=go -g '*_test.go' internal/budget/Repository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Search for any comments in code about source precedence or collision handling
rg -nP -C3 'precedence|collision|conflict.*source|source.*conflict' --type=go internal/budget/Repository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Look for documentation on budget sources or config precedence
find . -type f \( -name "*.md" -o -name "*.txt" \) | xargs grep -l -i "budget\|source" 2>/dev/null | head -5Repository: ENTERPILOT/GoModel
Length of output: 227
🏁 Script executed:
# Check for comments in the store_sqlite.go file around ReplaceConfigBudgets
sed -n '150,210p' internal/budget/store_sqlite.go | cat -nRepository: ENTERPILOT/GoModel
Length of output: 2470
🏁 Script executed:
# Look at the Budget struct definition and any comments
rg -nP -B3 -A10 'type Budget struct' --type=go internal/budget/Repository: ENTERPILOT/GoModel
Length of output: 1115
🏁 Script executed:
# Search README for budget-related documentation
rg -nP -C5 'Budget|budget' README.md | head -100Repository: ENTERPILOT/GoModel
Length of output: 869
🏁 Script executed:
# Check if there's any information about config source precedence
rg -nP -C3 'SourceConfig|SourceManual|source.*precedence|config.*precedence' README.md docs/Repository: ENTERPILOT/GoModel
Length of output: 44
🏁 Script executed:
# Let's see if the test file has any other tests related to collision scenarios
rg -nP 'func Test.*Budget' --type=go internal/budget/store_sqlite_test.goRepository: ENTERPILOT/GoModel
Length of output: 148
ReplaceConfigBudgets silently changes manual budgets to source = 'config' when keys collide.
When a config budget and manual budget share the same (user_path, period_seconds) key, the DELETE ... AND NOT (...) correctly skips the manual row, but upsertSQLiteBudgets then executes ON CONFLICT DO UPDATE SET source = excluded.source with the config source, converting the manual budget to config-sourced. The manual budget will be deleted on the next config replace if the YAML/env entry is removed without manual deletion.
No test or documentation establishes whether config should win on overlap. Either clarify this as intended behavior in the budget docs/dashboard, or preserve SourceManual on collision (e.g., omit source from the DO UPDATE clause or skip upsert for pre-existing non-config rows).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/budget/store_sqlite.go` around lines 168 - 207, ReplaceConfigBudgets
currently causes manual rows to become SourceConfig because
upsertSQLiteBudgets's ON CONFLICT DO UPDATE sets source = excluded.source;
change upsertSQLiteBudgets so it does not overwrite an existing non-config row:
either remove source from the DO UPDATE list or add a WHERE clause on the DO
UPDATE that only applies updates when the existing row already has source =
SourceConfig (e.g., "DO UPDATE SET ... WHERE budgets.source = ?"). Update the
function upsertSQLiteBudgets and its SQL/arguments accordingly so collisions
preserve SourceManual unless the existing row is already SourceConfig.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/admin/dashboard/static/js/modules/workflows.js (1)
562-593:⚠️ Potential issue | 🟡 MinorHydration switch from
workflowSourceFeatures→ rawworkflow_payload.featureschanges masking semantics.Previously the form was populated from
workflowSourceFeatures(workflow), which applies global runtime caps viaworkflowApplyGlobalFeatureCaps. The new branch usesworkflowNormalizedFeatures(workflow.workflow_payload.features), which does not apply global caps. Editing a workflow whose persisted features include e.g.usage: truewhileUSAGE_ENABLED=offwill now display the toggles as enabled in the form.buildWorkflowRequestlater re-applies caps so the submitted payload is still bounded, but the user sees a UI state that disagrees with runtime visibility.If that divergence is intentional (preserve persisted state for edits even when globally disabled), worth a short comment near the new branch documenting it; otherwise consider routing the hydrated raw features through
workflowApplyGlobalFeatureCaps(then preservefallbackseparately, asworkflowSourceFeaturesalready does).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/js/modules/workflows.js` around lines 562 - 593, The hydration now uses workflowNormalizedFeatures(workflow.workflow_payload.features) which skips global runtime caps and can show toggles (e.g., usage) as enabled even when USAGE_ENABLED is off; change the branch so the hydrated features are passed through workflowApplyGlobalFeatureCaps(...) (preserving fallback separately as workflowSourceFeatures does) before assigning to this.workflowForm.features, or if the intent is to keep persisted state, add a short comment next to the new branch explaining that workflowNormalizedFeatures is intentionally used to preserve persisted flags despite runtime caps; reference workflowNormalizedFeatures, workflowSourceFeatures, and workflowApplyGlobalFeatureCaps to locate and update the code.internal/admin/dashboard/static/js/modules/workflows.test.cjs (1)
868-921: 🧹 Nitpick | 🔵 TrivialHydration test:
workflowSourceFeatures/workflowSourceGuardrailsmocks are now unreachable here.Because
openWorkflowCreatewas changed to preferworkflow.workflow_payload.features/.guardrailsover the source helpers when present, the mocks at lines 870–880 are never invoked by this test (the saved payload exists). The assertions still correctly validate the new code path (usage: falsefrom payload,budget: truedefaulted viaworkflowNormalizedFeatures,fallback: truedefaulted,guardrails: [{ ref: 'wrong-source', step: 10 }]from payload — not thepolicy-system, step: 30from the mock).Consider either removing the mocks (they no longer guard against regression on this path) or splitting into a second test that exercises the fallback branch (workflow without
workflow_payload, whereworkflowSourceFeatures/workflowSourceGuardrailsare used). As-is, the mocks read like setup that influences the result but actually don't.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/admin/dashboard/static/js/modules/workflows.test.cjs` around lines 868 - 921, Test setup includes mocks for workflowSourceFeatures and workflowSourceGuardrails that are never used because openWorkflowCreate now prefers workflow.workflow_payload; remove the misleading mocks or split into two tests: keep this test as-is but delete the workflowSourceFeatures/workflowSourceGuardrails setup lines (and any unused focusWorkflowForm stub) so it only validates payload hydration, and add a new test that calls openWorkflowCreate with a workflow lacking workflow_payload to assert that workflowSourceFeatures and workflowSourceGuardrails are invoked and their results used to populate workflowForm/features and workflowForm/guardrails.
♻️ Duplicate comments (1)
config/config.go (1)
1326-1336:⚠️ Potential issue | 🟠 MajorNormalize paths before deciding which YAML entry the env override replaces.
This replacement check still compares raw strings. If YAML defines the same logical path in a non-canonical form, like
team/alphaor/team/alpha/,SET_BUDGET_TEAM__ALPHA=...will not replace it here; both entries survive untilvalidateBudgetConfignormalizes them and the load fails with a duplicate-budget error instead of honoring env-overrides-YAML precedence. Compare normalized paths on both sides before filteringcfg.Budgets.UserPaths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/config.go` around lines 1326 - 1336, Normalize the paths before comparing so env-derived entries override YAML equivalents: when building the new BudgetUserPathConfig entry and when iterating cfg.Budgets.UserPaths, run the same normalization used in validateBudgetConfig (or a shared helper) on entry.Path and existing.Path and compare those normalized values instead of the raw strings; create the entry with the normalized Path and use normalizedExisting != normalizedEntry in the loop that builds replaced before assigning cfg.Budgets.UserPaths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/static/js/modules/workflows.js`:
- Around line 1400-1440: Add a short clarifying comment above the
workflowNestedErrorCode function explaining that it only attempts to JSON.parse
string values that begin with '{' or '[', that trimming already handles leading
whitespace, and that quoted/other non-object JSON shapes are intentionally
ignored by this helper; reference the function name workflowNestedErrorCode and
the workflowEntryErrorCode caller so reviewers know where the limitation
applies.
In `@internal/admin/handler.go`:
- Around line 1176-1189: The handler currently lets an empty or missing
user_path default to "/" because budget.NormalizeUserPath("") returns "/", so
before calling budget.NormalizeUserPath in the reset-one endpoint (where
resetBudgetRequest is bound), add an explicit non-empty check on req.UserPath
and return a 400 via handleError(core.NewInvalidRequestError(...)) if it's
blank; then proceed to call budget.NormalizeUserPath(req.UserPath) and continue
with h.budgets.ResetBudget as before. Ensure the error message clearly states
that user_path is required and include the original err when creating the
NewInvalidRequestError.
In `@internal/budget/service_test.go`:
- Around line 47-52: The fakeStore.GetSettings currently treats a zero-value
s.settings as "not saved" and returns DefaultSettings(), which masks
intentionally saved Settings{}; modify fakeStore to track whether settings were
actually saved (e.g., add a savedSettings bool set to true in SaveSettings) and
change GetSettings to return DefaultSettings() only when savedSettings is false,
otherwise return s.settings (even if it's the zero value); update references to
fakeStore.GetSettings and SaveSettings accordingly.
In `@internal/server/http_test.go`:
- Around line 384-417: Update TestBasePathRejectsInvalidRawPathPrefix to also
assert the response body contains the exact error string emitted by
stripBasePathMiddleware (e.g. "invalid encoded request path") so the error
contract is pinned; after calling srv.ServeHTTP(rec, req) in each subtest check
rec.Body.String() (or use strings.Contains/require.Contains) and fail the test
if the body does not contain that message, keeping the existing status code
assertion unchanged.
---
Outside diff comments:
In `@internal/admin/dashboard/static/js/modules/workflows.js`:
- Around line 562-593: The hydration now uses
workflowNormalizedFeatures(workflow.workflow_payload.features) which skips
global runtime caps and can show toggles (e.g., usage) as enabled even when
USAGE_ENABLED is off; change the branch so the hydrated features are passed
through workflowApplyGlobalFeatureCaps(...) (preserving fallback separately as
workflowSourceFeatures does) before assigning to this.workflowForm.features, or
if the intent is to keep persisted state, add a short comment next to the new
branch explaining that workflowNormalizedFeatures is intentionally used to
preserve persisted flags despite runtime caps; reference
workflowNormalizedFeatures, workflowSourceFeatures, and
workflowApplyGlobalFeatureCaps to locate and update the code.
In `@internal/admin/dashboard/static/js/modules/workflows.test.cjs`:
- Around line 868-921: Test setup includes mocks for workflowSourceFeatures and
workflowSourceGuardrails that are never used because openWorkflowCreate now
prefers workflow.workflow_payload; remove the misleading mocks or split into two
tests: keep this test as-is but delete the
workflowSourceFeatures/workflowSourceGuardrails setup lines (and any unused
focusWorkflowForm stub) so it only validates payload hydration, and add a new
test that calls openWorkflowCreate with a workflow lacking workflow_payload to
assert that workflowSourceFeatures and workflowSourceGuardrails are invoked and
their results used to populate workflowForm/features and
workflowForm/guardrails.
---
Duplicate comments:
In `@config/config.go`:
- Around line 1326-1336: Normalize the paths before comparing so env-derived
entries override YAML equivalents: when building the new BudgetUserPathConfig
entry and when iterating cfg.Budgets.UserPaths, run the same normalization used
in validateBudgetConfig (or a shared helper) on entry.Path and existing.Path and
compare those normalized values instead of the raw strings; create the entry
with the normalized Path and use normalizedExisting != normalizedEntry in the
loop that builds replaced before assigning cfg.Budgets.UserPaths.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b0609491-2131-40ce-9e67-83fd53810ea6
📒 Files selected for processing (21)
CHANGELOG.mdconfig/config.goconfig/config_test.gointernal/admin/dashboard/static/js/modules/workflows.jsinternal/admin/dashboard/static/js/modules/workflows.test.cjsinternal/admin/handler.gointernal/admin/handler_budgets_test.gointernal/budget/service.gointernal/budget/service_test.gointernal/budget/store_mongodb.gointernal/budget/store_mongodb_test.gointernal/budget/store_postgresql.gointernal/budget/store_sqlite.gointernal/budget/store_sqlite_test.gointernal/budget/types.gointernal/budget/types_test.gointernal/server/base_path.gointernal/server/http_test.gointernal/usage/cache_type_test.gointernal/usage/store_postgresql.gointernal/usage/store_sqlite.go
Comment on lines
+1400
to
+1440
| workflowEntryErrorCode(entry) { | ||
| const data = entry && entry.data && typeof entry.data === 'object' && !Array.isArray(entry.data) | ||
| ? entry.data | ||
| : {}; | ||
| const direct = String(data.error_code || data.errorCode || '').trim(); | ||
| if (direct) return direct; | ||
| return this.workflowNestedErrorCode(data.response_body); | ||
| }, | ||
|
|
||
| workflowNestedErrorCode(value, depth = 0) { | ||
| if (depth > 4 || value === null || value === undefined) { | ||
| return ''; | ||
| } | ||
| if (typeof value === 'string') { | ||
| const trimmed = value.trim(); | ||
| if (!trimmed || (trimmed[0] !== '{' && trimmed[0] !== '[')) { | ||
| return ''; | ||
| } | ||
| try { | ||
| return this.workflowNestedErrorCode(JSON.parse(trimmed), depth + 1); | ||
| } catch (_) { | ||
| return ''; | ||
| } | ||
| } | ||
| if (Array.isArray(value)) { | ||
| for (const item of value) { | ||
| const code = this.workflowNestedErrorCode(item, depth + 1); | ||
| if (code) return code; | ||
| } | ||
| return ''; | ||
| } | ||
| if (typeof value !== 'object') { | ||
| return ''; | ||
| } | ||
| const code = String(value.code || '').trim(); | ||
| if (code) return code; | ||
| if (value.error !== undefined) { | ||
| return this.workflowNestedErrorCode(value.error, depth + 1); | ||
| } | ||
| return ''; | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
workflowNestedErrorCode recursion: minor robustness considerations.
A few things worth tightening on this new helper:
- JSON-string detection is too narrow. Line 1415 only attempts
JSON.parsewhen the trimmed string starts with{or[. If a backend ever serializesresponse_bodywith a leading whitespace-or-BOM-stripped wrapper, or as a quoted JSON object string"\"{...}\"", the parse path is skipped. Trimming already handles whitespace, so this is fine for the current backend shape (gatewayErrorWithResponseHeaderswrites a JSON object), but it's worth a brief comment that only object/array payloads are inspected. - Error precedence. When
value.codeis empty butvalue.errorexists, the current order returns''via the empty-codebranch only after also checkingvalue.error— actually re-reading: line 1434 checkscode, returns if truthy; otherwise line 1436 recurses intovalue.error. That's correct. ✅ - Default-parameter syntax (
depth = 0) is fine for the dashboard target (modern browsers / Alpine.js context); no concern.
No functional bug — the helper correctly extracts budget_exceeded from both data.error_code (top-level) and nested data.response_body shapes (string-encoded or object), which matches the backend code path in internal/server/budget_support.go (WithCode("budget_exceeded")).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/dashboard/static/js/modules/workflows.js` around lines 1400 -
1440, Add a short clarifying comment above the workflowNestedErrorCode function
explaining that it only attempts to JSON.parse string values that begin with '{'
or '[', that trimming already handles leading whitespace, and that quoted/other
non-object JSON shapes are intentionally ignored by this helper; reference the
function name workflowNestedErrorCode and the workflowEntryErrorCode caller so
reviewers know where the limitation applies.
Comment on lines
+1176
to
+1189
| var req resetBudgetRequest | ||
| if err := c.Bind(&req); err != nil { | ||
| return handleError(c, core.NewInvalidRequestError("invalid request body: "+err.Error(), err)) | ||
| } | ||
| periodSeconds, err := budgetRequestPeriodSeconds(req.Period, req.PeriodSeconds) | ||
| if err != nil { | ||
| return handleError(c, core.NewInvalidRequestError(err.Error(), err)) | ||
| } | ||
| userPath, err := budget.NormalizeUserPath(req.UserPath) | ||
| if err != nil { | ||
| return handleError(c, core.NewInvalidRequestError(err.Error(), err)) | ||
| } | ||
| if err := h.budgets.ResetBudget(c.Request().Context(), userPath, periodSeconds, time.Now().UTC()); err != nil { | ||
| return handleError(c, budgetServiceError("failed to reset budget", err)) |
Contributor
There was a problem hiding this comment.
Reject missing user_path on reset-one instead of defaulting it to /.
budget.NormalizeUserPath("") resolves to /, so a request body that omits or blanks user_path will reset the root budget rather than fail validation. That makes malformed input mutate the wrong budget key. Add an explicit non-empty check before normalization and return 400 when user_path is missing.
💡 Minimal fix
var req resetBudgetRequest
if err := c.Bind(&req); err != nil {
return handleError(c, core.NewInvalidRequestError("invalid request body: "+err.Error(), err))
}
+ if strings.TrimSpace(req.UserPath) == "" {
+ return handleError(c, core.NewInvalidRequestError("user_path is required", nil))
+ }
periodSeconds, err := budgetRequestPeriodSeconds(req.Period, req.PeriodSeconds)
if err != nil {
return handleError(c, core.NewInvalidRequestError(err.Error(), err))
}
userPath, err := budget.NormalizeUserPath(req.UserPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/admin/handler.go` around lines 1176 - 1189, The handler currently
lets an empty or missing user_path default to "/" because
budget.NormalizeUserPath("") returns "/", so before calling
budget.NormalizeUserPath in the reset-one endpoint (where resetBudgetRequest is
bound), add an explicit non-empty check on req.UserPath and return a 400 via
handleError(core.NewInvalidRequestError(...)) if it's blank; then proceed to
call budget.NormalizeUserPath(req.UserPath) and continue with
h.budgets.ResetBudget as before. Ensure the error message clearly states that
user_path is required and include the original err when creating the
NewInvalidRequestError.
Comment on lines
+47
to
+52
| func (s *fakeStore) GetSettings(context.Context) (Settings, error) { | ||
| if s.settings == (Settings{}) { | ||
| return DefaultSettings(), nil | ||
| } | ||
| return s.settings, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
fakeStore.GetSettings masks zero-value settings as DefaultSettings().
If a test ever saves an explicit zero Settings{} snapshot and then re-reads it, this branch silently swaps it back to DefaultSettings(), which can hide bugs in code paths that depend on persisting/reading the exact saved value. Consider using a separate saved bool flag (set in SaveSettings) to decide whether to return DefaultSettings() instead of comparing against the zero value.
♻️ Proposed fix
type fakeStore struct {
budgets []Budget
settings Settings
+ saved bool
listErr error
sum func(userPath string, start, end time.Time) (float64, bool, error)
...
func (s *fakeStore) GetSettings(context.Context) (Settings, error) {
- if s.settings == (Settings{}) {
+ if !s.saved {
return DefaultSettings(), nil
}
return s.settings, nil
}
func (s *fakeStore) SaveSettings(_ context.Context, settings Settings) (Settings, error) {
s.settings = settings
+ s.saved = true
return settings, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/budget/service_test.go` around lines 47 - 52, The
fakeStore.GetSettings currently treats a zero-value s.settings as "not saved"
and returns DefaultSettings(), which masks intentionally saved Settings{};
modify fakeStore to track whether settings were actually saved (e.g., add a
savedSettings bool set to true in SaveSettings) and change GetSettings to return
DefaultSettings() only when savedSettings is false, otherwise return s.settings
(even if it's the zero value); update references to fakeStore.GetSettings and
SaveSettings accordingly.
Comment on lines
+384
to
+417
| func TestBasePathRejectsInvalidRawPathPrefix(t *testing.T) { | ||
| srv := New(&mockProvider{}, &Config{BasePath: "/g"}) | ||
| srv.echo.PUT("/admin/api/v1/budgets/:user_path/:period", func(c *echo.Context) error { | ||
| return c.NoContent(http.StatusOK) | ||
| }) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| rawPath string | ||
| }{ | ||
| { | ||
| name: "decoded raw path does not match base path", | ||
| rawPath: "/%2Fg/admin/api/v1/budgets/%2F/86400", | ||
| }, | ||
| { | ||
| name: "encoded slash in raw base path segment", | ||
| rawPath: "/%67%2Fadmin/api/v1/budgets/%2F/86400", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| req := httptest.NewRequest(http.MethodPut, "/g/admin/api/v1/budgets/%2F/86400", nil) | ||
| req.URL.RawPath = tt.rawPath | ||
| rec := httptest.NewRecorder() | ||
|
|
||
| srv.ServeHTTP(rec, req) | ||
|
|
||
| if rec.Code != http.StatusBadRequest { | ||
| t.Fatalf("status = %d, want %d; body=%s", rec.Code, http.StatusBadRequest, rec.Body.String()) | ||
| } | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: also assert the 400 body to lock the error contract.
The rejection cases correctly confirm that an invalid RawPath short-circuits routing with 400, but a future change that surfaces a generic 400 from another middleware (or replaces the message) wouldn't be caught. Consider asserting the body contains "invalid encoded request path" so the contract emitted by stripBasePathMiddleware is pinned down.
♻️ Suggested tightening
if rec.Code != http.StatusBadRequest {
t.Fatalf("status = %d, want %d; body=%s", rec.Code, http.StatusBadRequest, rec.Body.String())
}
+ if !strings.Contains(rec.Body.String(), "invalid encoded request path") {
+ t.Fatalf("body = %q, want substring %q", rec.Body.String(), "invalid encoded request path")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/http_test.go` around lines 384 - 417, Update
TestBasePathRejectsInvalidRawPathPrefix to also assert the response body
contains the exact error string emitted by stripBasePathMiddleware (e.g.
"invalid encoded request path") so the error contract is pinned; after calling
srv.ServeHTTP(rec, req) in each subtest check rec.Body.String() (or use
strings.Contains/require.Contains) and fail the test if the body does not
contain that message, keeping the existing status code assertion unchanged.
This was referenced Apr 27, 2026
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
Tests
Summary by CodeRabbit
New Features
Admin API
Documentation