governance: calendar alignment applies from the next period - #6003
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCalendar alignment updates preserve existing budget and rate-limit usage and reset timestamps. Budget-cycle logic handles overdue windows, independent durations, and rolling sub-day windows. Tests and API documentation describe the updated behavior. ChangesCalendar alignment behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
transports/bifrost-http/handlers/governance.go (1)
3027-3027: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd handler-level regression tests for customer and provider governance rate limits.
When enabling
calendar_aligned, assert both token/request usage counters and reset timestamps. Existing alignment tests cover customer budgets and only the teamtoken_last_resetfield.🤖 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` at line 3027, Add handler-level regression tests for customer and provider governance rate limits in the governance handlers, covering the calendar_aligned configuration. Assert both token and request usage counters and their reset timestamps, extending existing alignment coverage beyond customer budgets and the team token_last_reset field.
🤖 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 `@docs/features/governance/budget-and-limits.mdx`:
- Around line 176-178: Update the calendar alignment documentation to specify
the request-body location for each supported PUT endpoint: customer, team, and
provider-governance updates receive calendar_aligned at the owner request-body
level, while virtual-key updates use the budget field location. Replace the
ambiguous “on the budget” wording and verify the documented locations match the
implementation and schemas.
In `@tests/governance/customerbudget_test.go`:
- Around line 397-402: Replace the unchecked response-field assertions with
validated type assertions that call t.Fatalf on malformed data. In
tests/governance/customerbudget_test.go:397-402 and
tests/governance/teambudget_test.go:242-247, validate the first budget object
and its last_reset string; in tests/governance/teambudget_test.go:312-320,
validate the rate-limit object and its token_last_reset string, preserving the
existing parsing and assertions for valid responses.
---
Nitpick comments:
In `@transports/bifrost-http/handlers/governance.go`:
- Line 3027: Add handler-level regression tests for customer and provider
governance rate limits in the governance handlers, covering the calendar_aligned
configuration. Assert both token and request usage counters and their reset
timestamps, extending existing alignment coverage beyond customer budgets and
the team token_last_reset field.
🪄 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: b7857d37-21fc-4477-949e-ee2e408439fc
📒 Files selected for processing (8)
docs/features/governance/budget-and-limits.mdxdocs/openapi/openapi.jsondocs/openapi/schemas/management/governance.yamltests/governance/customerbudget_test.gotests/governance/teambudget_test.gotests/governance/test_utils.gotransports/bifrost-http/handlers/governance.gotransports/bifrost-http/handlers/governance_test.go
c148a5f to
c2e580a
Compare
ce56c83 to
9f91c19
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/features/governance/budget-and-limits.mdx (1)
159-168: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject
1Qfor rate limits or update the documentation and schema to support it. Runtime parsing and calendar reset handling accept1Q, but no rate-limit validator or schema constraint rejects it. This conflicts with the budget-only documentation.🤖 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 `@docs/features/governance/budget-and-limits.mdx` around lines 159 - 168, Align rate-limit validation and schema constraints with the documented budget-only behavior by rejecting the 1Q duration for rate limits, while preserving 1Q support for budgets. Update the runtime validator and relevant schema definitions so quarterly rate-limit values cannot be accepted.Source: Path instructions
🤖 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 `@docs/features/governance/budget-and-limits.mdx`:
- Line 178: Update the budget reset logic around Budget.WindowStart and
LastReset so changing alignment from false to true records the activation time
and skips the current calendar boundary, preserving usage until the next
boundary; apply the same transition rule to rate limits. Then verify the
documented behavior matches the implementation, config.schema.json, and provider
behavior.
---
Outside diff comments:
In `@docs/features/governance/budget-and-limits.mdx`:
- Around line 159-168: Align rate-limit validation and schema constraints with
the documented budget-only behavior by rejecting the 1Q duration for rate
limits, while preserving 1Q support for budgets. Update the runtime validator
and relevant schema definitions so quarterly rate-limit values cannot be
accepted.
🪄 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: 6972db23-5738-4c10-a2d5-4e6e84529892
📒 Files selected for processing (3)
docs/features/governance/budget-and-limits.mdxtests/governance/customerbudget_test.gotests/governance/teambudget_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/governance/teambudget_test.go
- tests/governance/customerbudget_test.go
9f91c19 to
9c813ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/features/governance/budget-and-limits.mdx`:
- Around line 176-185: Update the calendar-alignment section to explicitly
include associated rate limits wherever it currently describes budgets,
including preserved last_reset values, usage, deferred resets, and switch-over
behavior. Clarify that owner-level calendar alignment applies to both budget and
rate-limit reset windows, while retaining the existing create/update request
guidance and warning.
In `@plugins/governance/budgetcycle_test.go`:
- Around line 845-892: Replace
TestEnablingCalendarAlignmentCanResetAtTheBoundaryAlreadyPassed with
table-driven cases that create an unaligned budget and enable alignment through
the update path. Verify LastReset and CurrentUsage remain unchanged at 42,
budgetResetTarget returns no reset before the next boundary (March 1), and cover
both windows opened before and after the February boundary.
🪄 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: c870786b-4a9e-4997-94ce-a9d3dbe8aada
📒 Files selected for processing (2)
docs/features/governance/budget-and-limits.mdxplugins/governance/budgetcycle_test.go
9c813ec to
bb014fd
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 `@docs/features/governance/budget-and-limits.mdx`:
- Line 178: Update the owner-level reset-alignment documentation around the
statements that describe budgets and rate limits so it explicitly states that
each resource aligns to its own reset_duration calendar boundary rather than a
shared reset instant. Clarify preservation guidance by referring to the
corresponding budget or rate-limit boundary, or recommend enabling alignment at
creation time when all existing windows must be preserved.
🪄 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: 558ae13a-5671-4121-a731-718b1c299889
📒 Files selected for processing (2)
docs/features/governance/budget-and-limits.mdxplugins/governance/budgetcycle_test.go
c2e580a to
aff9859
Compare
653e1a9 to
d9d2e2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/features/governance/budget-and-limits.mdx`:
- Around line 186-189: The budget alignment documentation should cover both
reset sweeps and request-time evaluation via BumpBudgetUsage. Update the
“already due” window description to state that it resets and clears usage at the
next reset evaluation, including either trigger explicitly or using wording that
encompasses both, while preserving the existing behavior details.
In `@plugins/governance/budgetcycle_test.go`:
- Around line 971-979: Extend the persistence assertions in the sub-day aligned
budget test after configStore.GetBudget to verify stored.IsCalendarAligned is
true, alongside the existing ResetDuration assertion. Keep the test’s current
setup and save behavior unchanged.
🪄 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: d984a882-a1e2-4e7c-9ff6-12de649d5229
📒 Files selected for processing (2)
docs/features/governance/budget-and-limits.mdxplugins/governance/budgetcycle_test.go
d9d2e2b to
37ee88e
Compare
aff9859 to
1ef8496
Compare
Merge activity
|
The base branch was changed.
37ee88e to
02ed8c2
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
…6003) ## Summary Enabling calendar alignment on an existing budget previously promised to snap `last_reset` to the current period start and zero out `CurrentUsage`. This never actually happened: `UpdateBudget` and `UpdateRateLimit` carry both values forward from the stored row on every config write, so the snap was silently discarded one layer below the handler. Rather than making the snap work (which would move `last_reset` backwards, conflicting with the forward-only guard that keeps cluster nodes in agreement), the dead snap code is removed entirely. Calendar alignment now takes effect from the next period boundary, leaving the current window and its accumulated usage untouched. ## Changes - Removed the `calendarAlignmentJustEnabled` snap blocks from `updateTeam`, `updateCustomer`, and `updateProviderGovernance`. These blocks wrote `LastReset` and `CurrentUsage` to `UpdateBudget`/`UpdateRateLimit`, but the real store always copies those fields back from the stored row, making the writes no-ops in production. - Updated the `calendar_aligned` field description in the OpenAPI schema and governance YAML to reflect the new behaviour: alignment takes effect from the next period; the current window keeps its start and accumulated usage. - Updated the docs to clarify that toggling alignment on an existing budget does not re-anchor the current window, and that setting alignment at create time is the way to start aligned from the beginning. - Fixed the `mockCustomerStore.UpdateBudget` mock to carry `CurrentUsage` and `LastReset` forward from the stored row, matching the real store's contract. Without this, handler-level tests could assert on values the real store would discard. - Replaced `TestUpdateCustomer_CalendarAligned_SnapsExistingBudget` with `TestUpdateCustomer_CalendarAligned_DoesNotTouchBudgets`, which asserts the correct behaviour. - Added integration tests `TestTeamBudgetCalendarAlignmentAppliesFromNextPeriod`, `TestTeamRateLimitCalendarAlignmentAppliesFromNextPeriod`, and `TestCustomerBudgetCalendarAlignmentAppliesFromNextPeriod` that verify `last_reset` is not moved when alignment is enabled on an existing resource. - Extended `CreateTeamRequest`, `CreateCustomerRequest`, and `UpdateCustomerRequest` test structs to carry `RateLimit` and `CalendarAligned` fields needed by the new integration tests. ## Type of change - [x] Bug fix - [ ] Feature - [ ] Refactor - [x] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test ```sh go test ./transports/bifrost-http/handlers/... -run TestUpdateCustomer_CalendarAligned go test ./tests/governance/... -run TestTeamBudgetCalendarAlignmentAppliesFromNextPeriod go test ./tests/governance/... -run TestTeamRateLimitCalendarAlignmentAppliesFromNextPeriod go test ./tests/governance/... -run TestCustomerBudgetCalendarAlignmentAppliesFromNextPeriod ``` Create a team or customer with a monthly budget, then enable `calendar_aligned` via `PUT`. Confirm that `last_reset` and current usage are unchanged immediately after the update. Confirm that the budget resets to the aligned boundary at the next period tick. ## Breaking changes - [x] Yes - [ ] No Enabling `calendar_aligned` on an existing budget no longer resets usage to zero or moves `last_reset` to the period start. Callers who relied on the toggle as a way to force an immediate reset must instead delete and recreate the budget, or wait for the next natural period boundary. ## Related issues ## Security considerations None. No auth, secrets, or PII are involved. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Enabling calendar alignment on an existing budget previously promised to snap
last_resetto the current period start and zero outCurrentUsage. This never actually happened:UpdateBudgetandUpdateRateLimitcarry both values forward from the stored row on every config write, so the snap was silently discarded one layer below the handler. Rather than making the snap work (which would movelast_resetbackwards, conflicting with the forward-only guard that keeps cluster nodes in agreement), the dead snap code is removed entirely. Calendar alignment now takes effect from the next period boundary, leaving the current window and its accumulated usage untouched.Changes
calendarAlignmentJustEnabledsnap blocks fromupdateTeam,updateCustomer, andupdateProviderGovernance. These blocks wroteLastResetandCurrentUsagetoUpdateBudget/UpdateRateLimit, but the real store always copies those fields back from the stored row, making the writes no-ops in production.calendar_alignedfield description in the OpenAPI schema and governance YAML to reflect the new behaviour: alignment takes effect from the next period; the current window keeps its start and accumulated usage.mockCustomerStore.UpdateBudgetmock to carryCurrentUsageandLastResetforward from the stored row, matching the real store's contract. Without this, handler-level tests could assert on values the real store would discard.TestUpdateCustomer_CalendarAligned_SnapsExistingBudgetwithTestUpdateCustomer_CalendarAligned_DoesNotTouchBudgets, which asserts the correct behaviour.TestTeamBudgetCalendarAlignmentAppliesFromNextPeriod,TestTeamRateLimitCalendarAlignmentAppliesFromNextPeriod, andTestCustomerBudgetCalendarAlignmentAppliesFromNextPeriodthat verifylast_resetis not moved when alignment is enabled on an existing resource.CreateTeamRequest,CreateCustomerRequest, andUpdateCustomerRequesttest structs to carryRateLimitandCalendarAlignedfields needed by the new integration tests.Type of change
Affected areas
How to test
Create a team or customer with a monthly budget, then enable
calendar_alignedviaPUT. Confirm thatlast_resetand current usage are unchanged immediately after the update. Confirm that the budget resets to the aligned boundary at the next period tick.Breaking changes
Enabling
calendar_alignedon an existing budget no longer resets usage to zero or moveslast_resetto the period start. Callers who relied on the toggle as a way to force an immediate reset must instead delete and recreate the budget, or wait for the next natural period boundary.Related issues
Security considerations
None. No auth, secrets, or PII are involved.
Checklist
docs/contributing/README.mdand followed the guidelines