governance: quarterly budget windows with configurable fiscal start - #5997
Conversation
|
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds quarterly budget durations with configurable fiscal quarter starts. It updates calendar calculations, persistence, API validation, governance reconciliation, reset behavior, rate-limit exclusions, compatibility hashing, and tests. ChangesQuarterly budget reset flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GovernanceHandlers
participant TableBudget
participant CalendarHelpers
participant Storage
Client->>GovernanceHandlers: Submit budget with ResetConfig
GovernanceHandlers->>TableBudget: Validate and apply ResetConfig
GovernanceHandlers->>CalendarHelpers: Calculate fiscal-quarter boundary
CalendarHelpers-->>GovernanceHandlers: Return LastReset
GovernanceHandlers->>Storage: Persist or reconcile budget
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
transports/bifrost-http/handlers/governance.go (1)
348-350: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
ResetConfigin legacy budget updatesA
reset_config-only legacybudgetrequest currently satisfiesisBudgetRemovalRequest, so it deletes the existing budget. Updating that predicate alone is insufficient becausecoerceLegacyBudgetdoes not copyResetConfigintoCreateBudgetRequest. ExcludeResetConfigfrom removal detection, propagate it during coercion, and add a regression test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/bifrost-http/handlers/governance.go` around lines 348 - 350, Update isBudgetRemovalRequest to require ResetConfig to be nil, so reset_config-only requests are not treated as removals; update coerceLegacyBudget to copy ResetConfig into CreateBudgetRequest, and add a regression test covering preservation of reset_config in legacy budget updates.plugins/governance/store.go (1)
2068-2086: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument the
Qrate-limit compatibility change.
"1Q"is accepted as a positive 90-day duration. ForIsCalendarAligned == true, this commit changes it from a rolling reset to a calendar-quarter reset. Confirm deployed rows cannot contain"Q"; otherwise add release notes and upgrade guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/governance/store.go` around lines 2068 - 2086, The rateLimitResetTarget change makes calendar-aligned “1Q” durations use calendar-quarter resets, so document this compatibility change in the project’s release notes and upgrade guidance. Explicitly state that “1Q” remains a valid positive 90-day duration, now resets on calendar quarters when IsCalendarAligned is true, and confirm deployed rows cannot contain “Q” (or provide migration guidance if they can).
🧹 Nitpick comments (2)
transports/config.schema.json (1)
482-494: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider enforcing the quarterly-only rule in the schema, not only in the description.
The description says
reset_configis valid only whenreset_durationis quarterly. The Go validator enforces this and returns "reset_config is only valid on a quarterly reset duration". The schema does not, soconfig.jsonvalidation passes and the failure surfaces later at load time.The budget item already carries an
allOfblock for theoverride_moderules, so a matching conditional fits the existing structure. Areset_durationpattern check for theQsuffix would move the failure to the earliest point.This is optional. The runtime already fails closed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transports/config.schema.json` around lines 482 - 494, Update the budget item schema’s existing allOf validation structure to add a conditional requiring reset_duration to match the quarterly “1Q” form whenever reset_config is present. Preserve the current reset_config property definitions and ensure non-quarterly configurations containing reset_config fail schema validation.Source: Path instructions
framework/configstore/tables/budget_test.go (1)
241-249: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the SQLite test pool to one connection.
Call
db.DB()andSetMaxOpenConns(1)beforeAutoMigrate; otherwise a second pooled connection sees an empty:memory:database.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/configstore/tables/budget_test.go` around lines 241 - 249, Update setupBudgetTestDB to retrieve the underlying SQL database via db.DB() and set its maximum open connections to 1 before AutoMigrate, handling any returned error consistently with the existing require checks.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/configstore/budget_test.go`:
- Around line 94-110: Extend the GenerateBudgetHash test with a non-nil
ResetConfig whose QuarterStartMonth is 0, representing an omitted
quarter_start_month value. Generate its hash and assert it equals januaryHash,
while preserving the existing nil-config and explicit-January assertions.
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 269-276: Align the framework dependencies used by the governance
modules with a release exposing BudgetResetConfig, QuarterStartNotApplicable,
and the three-argument GetCalendarPeriodStart API: update transports/go.mod and
plugins/governance/go.mod, or add temporary local replace directives if
unreleased. Run go mod tidy in both modules; the affected code is
transports/bifrost-http/handlers/governance.go around UpdateBudgetRequest and
plugins/governance/store.go around its calendar-period usage.
In `@transports/config.schema.json`:
- Around line 486-491: Update the quarter_start_month schema definition to
accept 0 as a valid explicit unset value, changing its minimum constraint while
preserving the 1–12 upper bound. Keep the description’s “omitted or 0 means
January” wording and ensure it remains consistent with validateBudget and
BudgetResetConfig.QuarterStart().
---
Outside diff comments:
In `@plugins/governance/store.go`:
- Around line 2068-2086: The rateLimitResetTarget change makes calendar-aligned
“1Q” durations use calendar-quarter resets, so document this compatibility
change in the project’s release notes and upgrade guidance. Explicitly state
that “1Q” remains a valid positive 90-day duration, now resets on calendar
quarters when IsCalendarAligned is true, and confirm deployed rows cannot
contain “Q” (or provide migration guidance if they can).
In `@transports/bifrost-http/handlers/governance.go`:
- Around line 348-350: Update isBudgetRemovalRequest to require ResetConfig to
be nil, so reset_config-only requests are not treated as removals; update
coerceLegacyBudget to copy ResetConfig into CreateBudgetRequest, and add a
regression test covering preservation of reset_config in legacy budget updates.
---
Nitpick comments:
In `@framework/configstore/tables/budget_test.go`:
- Around line 241-249: Update setupBudgetTestDB to retrieve the underlying SQL
database via db.DB() and set its maximum open connections to 1 before
AutoMigrate, handling any returned error consistently with the existing require
checks.
In `@transports/config.schema.json`:
- Around line 482-494: Update the budget item schema’s existing allOf validation
structure to add a conditional requiring reset_duration to match the quarterly
“1Q” form whenever reset_config is present. Preserve the current reset_config
property definitions and ensure non-quarterly configurations containing
reset_config fail schema validation.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5cc39f3f-3a45-401a-bf13-6ebc5aeaa22f
📒 Files selected for processing (14)
framework/configstore/budget_test.goframework/configstore/budgetresetconfighash_test.goframework/configstore/rdb_test.goframework/configstore/tables/budget.goframework/configstore/tables/budget_test.goframework/configstore/tables/budgetresetconfig_test.goframework/configstore/tables/budgetwindow_test.goframework/configstore/tables/utils.goplugins/governance/budgetcycle_test.goplugins/governance/ratelimitreset_test.goplugins/governance/store.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.gotransports/config.schema.json
💤 Files with no reviewable changes (2)
- framework/configstore/tables/budgetresetconfig_test.go
- framework/configstore/budgetresetconfighash_test.go
df5aa2b to
2377591
Compare
fc21eb0 to
eb925a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/config.schema.json`:
- Around line 482-493: Restrict the reset_config schema definition in
transports/config.schema.json:482-493 to configurations whose reset_duration is
exactly "1Q" by adding the requested allOf conditional. Update
transports/schema_test/config_schema_test.go:1079-1121 to parameterize
budgetConfig by duration and add coverage rejecting reset_config with "1M";
retain acceptance for quarterly budgets.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b11d605-3d73-455e-a74a-ce17c13a7d04
📒 Files selected for processing (3)
framework/configstore/budget_test.gotransports/config.schema.jsontransports/schema_test/config_schema_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- framework/configstore/budget_test.go
eb925a0 to
a2315b7
Compare
2377591 to
0c4b974
Compare
0c4b974 to
436fb8e
Compare
a2315b7 to
2bdb827
Compare
Merge activity
|
The base branch was changed.
2bdb827 to
1dd593f
Compare
…aximhq#5997) ## Summary Adds support for configurable fiscal quarter (`1Q`) budget reset windows. Previously, budgets could only reset on sub-day, daily, weekly, monthly, or yearly boundaries. This change introduces a `1Q` duration suffix and an optional `reset_config.quarter_start_month` field that lets operators define which month opens Q1, enabling non-calendar fiscal years (e.g. April in the UK, October for the US federal government, February for custom fiscal calendars). ## Changes - `GetCalendarPeriodStart` and `CountCalendarPeriods` now accept an explicit `quarterStart time.Month` parameter. All call sites that carry no fiscal calendar (rate limits, tests using non-quarterly durations) pass the new `QuarterStartNotApplicable` constant, which is `time.January`. The parameter is required rather than optional so that adding a budget call site is a compile error until the budget's own definition is threaded through. - `quarterStartAt` implements the boundary arithmetic in absolute months (`year*12 + month`) so the year boundary needs no special case. A fiscal year opening in November correctly places January in a quarter that began the previous calendar year. - `IsCalendarAlignableDuration` now includes the `Q` suffix alongside `d`, `w`, `M`, and `Y`. - `BudgetResetConfig.QuarterStart()` is extracted onto the config struct itself so the reconciler can compare an old definition against a new one through the same normalisation, preventing a spurious re-snap when an unset config and an explicit January are compared. - `TableBudget.QuarterStartMonth()` delegates to `ResetConfig.QuarterStart()` and remains safe on a nil receiver. - `newBudgetFromRequest` replaces five near-identical struct literals across the reconcilers. A dropped field in a literal is invisible at the API layer; a missing assignment in one constructor is caught in one place. - `applyResetConfigToExistingBudget` moves `LastReset` onto the new fiscal boundary when the quarter definition actually changes. It compares normalised months so that switching between an unset config and an explicit January does not trigger a re-snap. - `budgetLastReset` now takes the full `*TableBudget` rather than a bare duration string so a quarterly window snaps to the budget's own fiscal quarter. - `CreateBudgetRequest` and `UpdateBudgetRequest` gain a `ResetConfig` field, and `validateBudget` rejects a quarter definition on a non-quarterly duration and a `quarter_start_month` outside 1–12. - `config.schema.json` documents `1Q` as a valid `reset_duration` and adds the `reset_config` object with `quarter_start_month`. - `budgetresetconfighash_test.go` and `budgetresetconfig_test.go` are consolidated into `budget_test.go` and `budget_test.go` (tables package) respectively, removing the separate files. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./framework/configstore/... go test ./framework/configstore/tables/... go test ./plugins/governance/... go test ./transports/bifrost-http/handlers/... ``` Key scenarios to verify: - A `1Q` budget with `reset_config.quarter_start_month: 4` resets on April 1, July 1, October 1, and January 1. - A `1Q` budget with `reset_config.quarter_start_month: 2` resets on February 1, May 1, August 1, and November 1. - A `1Q` budget with no `reset_config` behaves identically to one with `quarter_start_month: 1`. - Changing `quarter_start_month` on an existing budget moves `LastReset` to the new boundary and does not mark the budget as immediately due. - A `reset_config` on a `1M` budget is rejected with a validation error. - Non-quarterly budgets (existing deployments) produce the same hash before and after the upgrade. ## Breaking changes - [x] Yes - [ ] No `GetCalendarPeriodStart` and `CountCalendarPeriods` have a new required `quarterStart time.Month` parameter. Any code outside this repository calling these functions directly must be updated to pass `tables.QuarterStartNotApplicable` (or the budget's own `QuarterStartMonth()`) at each call site. ## Related issues Closes maximhq#4851 (perpetually-due budget regression when a quarterly duration falls through `GetCalendarPeriodStart` to returning `now`). ## Security considerations No authentication, secrets, or PII changes. The `quarter_start_month` field is operator-supplied configuration stored in the governance database; it is validated to the range 1–12 before persistence. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Adds support for configurable fiscal quarter (
1Q) budget reset windows. Previously, budgets could only reset on sub-day, daily, weekly, monthly, or yearly boundaries. This change introduces a1Qduration suffix and an optionalreset_config.quarter_start_monthfield that lets operators define which month opens Q1, enabling non-calendar fiscal years (e.g. April in the UK, October for the US federal government, February for custom fiscal calendars).Changes
GetCalendarPeriodStartandCountCalendarPeriodsnow accept an explicitquarterStart time.Monthparameter. All call sites that carry no fiscal calendar (rate limits, tests using non-quarterly durations) pass the newQuarterStartNotApplicableconstant, which istime.January. The parameter is required rather than optional so that adding a budget call site is a compile error until the budget's own definition is threaded through.quarterStartAtimplements the boundary arithmetic in absolute months (year*12 + month) so the year boundary needs no special case. A fiscal year opening in November correctly places January in a quarter that began the previous calendar year.IsCalendarAlignableDurationnow includes theQsuffix alongsided,w,M, andY.BudgetResetConfig.QuarterStart()is extracted onto the config struct itself so the reconciler can compare an old definition against a new one through the same normalisation, preventing a spurious re-snap when an unset config and an explicit January are compared.TableBudget.QuarterStartMonth()delegates toResetConfig.QuarterStart()and remains safe on a nil receiver.newBudgetFromRequestreplaces five near-identical struct literals across the reconcilers. A dropped field in a literal is invisible at the API layer; a missing assignment in one constructor is caught in one place.applyResetConfigToExistingBudgetmovesLastResetonto the new fiscal boundary when the quarter definition actually changes. It compares normalised months so that switching between an unset config and an explicit January does not trigger a re-snap.budgetLastResetnow takes the full*TableBudgetrather than a bare duration string so a quarterly window snaps to the budget's own fiscal quarter.CreateBudgetRequestandUpdateBudgetRequestgain aResetConfigfield, andvalidateBudgetrejects a quarter definition on a non-quarterly duration and aquarter_start_monthoutside 1–12.config.schema.jsondocuments1Qas a validreset_durationand adds thereset_configobject withquarter_start_month.budgetresetconfighash_test.goandbudgetresetconfig_test.goare consolidated intobudget_test.goandbudget_test.go(tables package) respectively, removing the separate files.Type of change
Affected areas
How to test
Key scenarios to verify:
1Qbudget withreset_config.quarter_start_month: 4resets on April 1, July 1, October 1, and January 1.1Qbudget withreset_config.quarter_start_month: 2resets on February 1, May 1, August 1, and November 1.1Qbudget with noreset_configbehaves identically to one withquarter_start_month: 1.quarter_start_monthon an existing budget movesLastResetto the new boundary and does not mark the budget as immediately due.reset_configon a1Mbudget is rejected with a validation error.Breaking changes
GetCalendarPeriodStartandCountCalendarPeriodshave a new requiredquarterStart time.Monthparameter. Any code outside this repository calling these functions directly must be updated to passtables.QuarterStartNotApplicable(or the budget's ownQuarterStartMonth()) at each call site.Related issues
Closes #4851 (perpetually-due budget regression when a quarterly duration falls through
GetCalendarPeriodStartto returningnow).Security considerations
No authentication, secrets, or PII changes. The
quarter_start_monthfield is operator-supplied configuration stored in the governance database; it is validated to the range 1–12 before persistence.Checklist
docs/contributing/README.mdand followed the guidelines