test fixes - #5569
Merged
Merged
test fixes#5569
Conversation
|
|
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 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 (6)
📝 WalkthroughWalkthroughThe changes preserve additional OpenAI response fields, update GPT-oss role expectations, stabilize a Sidekiq race test, revise MCP authentication expectations, and add governance budget override schema and mock-store support. ChangesResponses serialization
Sidekiq race test
MCP authentication test
Budget override support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
akshaydeo
marked this pull request as ready for review
July 27, 2026 04:03
![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: 3
🤖 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 `@core/providers/openai/responses_test.go`:
- Around line 559-560: Update the gpt-oss “preserves message when Content
already exists” fixture to set expectRoleDropped true for its typeless reasoning
item with a role. Adjust the role assertion logic in the response conversion
tests so it runs for both Content == nil and existing-content paths, not only
the summary-conversion branch, matching the stripping behavior in the responses
converter.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 784-788: The mock budget update flow around SetOverrideAt must not
retain the caller-supplied IsCalendarAligned when validation fails. Preserve the
original flag and restore it if SetOverrideAt returns an error, or perform
validation on a copy and commit the flag only after success, matching
RDBConfigStore.UpdateBudgetOverride behavior.
In `@transports/config.schema.json`:
- Around line 517-541: Update the override schema containing override_amount,
override_mode, override_cycles_remaining, override_cycles_total, and
override_anchor_reset to add allOf/if/then constraints matching
TableBudget.validateOverride: empty mode requires all override fields to be zero
or absent, cycles requires positive amount and cycle counts with a valid anchor,
and forever requires a positive amount with zero/absent cycle fields and no
anchor. Do not encode the cross-field constraint that override_cycles_total must
be at least override_cycles_remaining, leaving that validation to runtime.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3099d3cd-b404-4d6a-8844-fb40c1226438
📒 Files selected for processing (6)
core/providers/openai/responses_test.gocore/schemas/responses.goframework/sidekiq/sidekiq_test.gotransports/bifrost-http/handlers/mcpserver_auth_test.gotransports/bifrost-http/lib/config_test.gotransports/config.schema.json
akshaydeo
force-pushed
the
07-27-test_fixes
branch
from
July 27, 2026 04:53
e19a097 to
ca3bde2
Compare
Contributor
Author
Merge activity
|
This was referenced Jul 27, 2026
akhsaul
pushed a commit
to akhsaul/bifrost
that referenced
this pull request
Aug 27, 2026
## Summary This PR fixes several correctness issues across the responses pipeline, Sidekiq runner contention, MCP server auth routing, and budget override handling. The core theme is ensuring that data decoded through the raw-preserved fast path is re-emitted verbatim rather than reconstructed from partial struct fields, and that identity/credential precedence rules are enforced rather than rejected as conflicts. ## Changes - **`ResponsesMessage` marshal/unmarshal (`core/schemas/responses.go`):** - `MarshalJSON` now short-circuits to return `rawPreserved` bytes verbatim when they are present, preventing silent data loss (e.g. `id`, `status`, `call_id`, per-tool type discriminators) that caused OpenAI to reject re-submitted items. - `UnmarshalJSON` now surfaces the `execution` field into `ResponsesToolMessage.Execution` and the `tools` array into `ToolSearchOutputTools` as raw JSON when decoding through the fast path, so downstream consumers can read them without triggering a full re-decode. - **Responses test (`core/providers/openai/responses_test.go`):** - Added `expectRoleDropped` flag to the test table to correctly assert that a typeless reasoning item has its `role` stripped rather than preserved, matching the OpenAI API contract. - **Sidekiq runner race test (`framework/sidekiq/sidekiq_test.go`):** - Introduced `testRunnerWithID` to build runners with explicit IDs. `TestRunnerRaceSingleWinner` now uses distinct runner IDs (`runner-A`, `runner-B`) so that `staleAfter` is non-zero and the loser cannot immediately reclaim the winner's in-flight job, eliminating a spurious double-execution race. - **MCP server auth (`transports/bifrost-http/handlers/mcpserver_auth_test.go`):** - Changed the "stamped user id with a header VK is rejected as conflicting" test to assert that the stamped user's representative VK wins and the header VK is silently ignored, rather than returning an error. Conflict resolution is the responsibility of upstream SCIM inference middleware, not `getMCPServerForRequest`. - **Mock config store (`transports/bifrost-http/lib/config_test.go`):** - Implemented `UpdateBudgetOverride` on `MockConfigStore` to mirror the RDB store's behaviour: applies only override columns, leaves usage and base config untouched, and returns `ErrNotFound` for unknown IDs. - **Config schema (`transports/config.schema.json`):** - Added `override_amount`, `override_mode`, `override_cycles_remaining`, `override_cycles_total`, and `override_anchor_reset` fields to the budget schema so override state is expressible in config files. ## Type of change - [x] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... go test ./core/providers/openai/... go test ./framework/sidekiq/... go test ./transports/bifrost-http/... ``` Validate that: - A round-tripped `ResponsesMessage` with `rawPreserved` bytes re-emits the original JSON byte-for-byte. - `TestRunnerRaceSingleWinner` passes consistently without the job running twice. - `getMCPServerForRequest` returns the user's representative VK server when both a stamped user ID and a header VK are present. - `UpdateBudgetOverride` on the mock store returns `ErrNotFound` for unknown IDs and correctly updates only override fields for known ones. ## Breaking changes - [x] No ## Security considerations The MCP auth change ensures a header-supplied virtual key cannot redirect or escalate an already-authenticated user to a different key. The stamped identity always wins, which is the intended security boundary. ## 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
occcat
pushed a commit
to occcat/bifrost
that referenced
this pull request
Sep 2, 2026
## Summary This PR fixes several correctness issues across the responses pipeline, Sidekiq runner contention, MCP server auth routing, and budget override handling. The core theme is ensuring that data decoded through the raw-preserved fast path is re-emitted verbatim rather than reconstructed from partial struct fields, and that identity/credential precedence rules are enforced rather than rejected as conflicts. ## Changes - **`ResponsesMessage` marshal/unmarshal (`core/schemas/responses.go`):** - `MarshalJSON` now short-circuits to return `rawPreserved` bytes verbatim when they are present, preventing silent data loss (e.g. `id`, `status`, `call_id`, per-tool type discriminators) that caused OpenAI to reject re-submitted items. - `UnmarshalJSON` now surfaces the `execution` field into `ResponsesToolMessage.Execution` and the `tools` array into `ToolSearchOutputTools` as raw JSON when decoding through the fast path, so downstream consumers can read them without triggering a full re-decode. - **Responses test (`core/providers/openai/responses_test.go`):** - Added `expectRoleDropped` flag to the test table to correctly assert that a typeless reasoning item has its `role` stripped rather than preserved, matching the OpenAI API contract. - **Sidekiq runner race test (`framework/sidekiq/sidekiq_test.go`):** - Introduced `testRunnerWithID` to build runners with explicit IDs. `TestRunnerRaceSingleWinner` now uses distinct runner IDs (`runner-A`, `runner-B`) so that `staleAfter` is non-zero and the loser cannot immediately reclaim the winner's in-flight job, eliminating a spurious double-execution race. - **MCP server auth (`transports/bifrost-http/handlers/mcpserver_auth_test.go`):** - Changed the "stamped user id with a header VK is rejected as conflicting" test to assert that the stamped user's representative VK wins and the header VK is silently ignored, rather than returning an error. Conflict resolution is the responsibility of upstream SCIM inference middleware, not `getMCPServerForRequest`. - **Mock config store (`transports/bifrost-http/lib/config_test.go`):** - Implemented `UpdateBudgetOverride` on `MockConfigStore` to mirror the RDB store's behaviour: applies only override columns, leaves usage and base config untouched, and returns `ErrNotFound` for unknown IDs. - **Config schema (`transports/config.schema.json`):** - Added `override_amount`, `override_mode`, `override_cycles_remaining`, `override_cycles_total`, and `override_anchor_reset` fields to the budget schema so override state is expressible in config files. ## Type of change - [x] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [x] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh go test ./core/schemas/... go test ./core/providers/openai/... go test ./framework/sidekiq/... go test ./transports/bifrost-http/... ``` Validate that: - A round-tripped `ResponsesMessage` with `rawPreserved` bytes re-emits the original JSON byte-for-byte. - `TestRunnerRaceSingleWinner` passes consistently without the job running twice. - `getMCPServerForRequest` returns the user's representative VK server when both a stamped user ID and a header VK are present. - `UpdateBudgetOverride` on the mock store returns `ErrNotFound` for unknown IDs and correctly updates only override fields for known ones. ## Breaking changes - [x] No ## Security considerations The MCP auth change ensures a header-supplied virtual key cannot redirect or escalate an already-authenticated user to a different key. The stamped identity always wins, which is the intended security boundary. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [ ] I updated documentation where needed - [x] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes several correctness issues across the responses pipeline, Sidekiq runner contention, MCP server auth routing, and budget override handling. The core theme is ensuring that data decoded through the raw-preserved fast path is re-emitted verbatim rather than reconstructed from partial struct fields, and that identity/credential precedence rules are enforced rather than rejected as conflicts.
Changes
ResponsesMessagemarshal/unmarshal (core/schemas/responses.go):MarshalJSONnow short-circuits to returnrawPreservedbytes verbatim when they are present, preventing silent data loss (e.g.id,status,call_id, per-tool type discriminators) that caused OpenAI to reject re-submitted items.UnmarshalJSONnow surfaces theexecutionfield intoResponsesToolMessage.Executionand thetoolsarray intoToolSearchOutputToolsas raw JSON when decoding through the fast path, so downstream consumers can read them without triggering a full re-decode.Responses test (
core/providers/openai/responses_test.go):expectRoleDroppedflag to the test table to correctly assert that a typeless reasoning item has itsrolestripped rather than preserved, matching the OpenAI API contract.Sidekiq runner race test (
framework/sidekiq/sidekiq_test.go):testRunnerWithIDto build runners with explicit IDs.TestRunnerRaceSingleWinnernow uses distinct runner IDs (runner-A,runner-B) so thatstaleAfteris non-zero and the loser cannot immediately reclaim the winner's in-flight job, eliminating a spurious double-execution race.MCP server auth (
transports/bifrost-http/handlers/mcpserver_auth_test.go):getMCPServerForRequest.Mock config store (
transports/bifrost-http/lib/config_test.go):UpdateBudgetOverrideonMockConfigStoreto mirror the RDB store's behaviour: applies only override columns, leaves usage and base config untouched, and returnsErrNotFoundfor unknown IDs.Config schema (
transports/config.schema.json):override_amount,override_mode,override_cycles_remaining,override_cycles_total, andoverride_anchor_resetfields to the budget schema so override state is expressible in config files.Type of change
Affected areas
How to test
Validate that:
ResponsesMessagewithrawPreservedbytes re-emits the original JSON byte-for-byte.TestRunnerRaceSingleWinnerpasses consistently without the job running twice.getMCPServerForRequestreturns the user's representative VK server when both a stamped user ID and a header VK are present.UpdateBudgetOverrideon the mock store returnsErrNotFoundfor unknown IDs and correctly updates only override fields for known ones.Breaking changes
Security considerations
The MCP auth change ensures a header-supplied virtual key cannot redirect or escalate an already-authenticated user to a different key. The stamped identity always wins, which is the intended security boundary.
Checklist
docs/contributing/README.mdand followed the guidelines