fix: allow additionalProperties to be object instead of only bool #1332
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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 unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (5)📚 Learning: 2025-12-09T17:07:42.007ZApplied to files:
📚 Learning: 2025-12-29T11:54:55.836ZApplied to files:
📚 Learning: 2026-01-14T04:40:11.480ZApplied to files:
📚 Learning: 2026-01-14T13:30:28.760ZApplied to files:
📚 Learning: 2025-12-17T08:44:08.788ZApplied to files:
🧬 Code graph analysis (1)core/schemas/chatcompletions.go (2)
🔇 Additional comments (3)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@core/schemas/chatcompletions.go`:
- Around line 339-341: The docstring for AdditionalProperties.MarshalJSON is
incorrect: it references "UnmarshalJSON" and should describe the marshal
behavior; update the comment above the MarshalJSON method
(AdditionalProperties.MarshalJSON) to read that MarshalJSON implements custom
JSON marshalling for AdditionalProperties and clarify the precedence rule (if
object value exists it takes precedence, otherwise the bool value is used).
- Around line 7-8: The code currently imports "github.com/bytedance/sonic" and
calls sonic.Unmarshal/sonic.Marshal directly inside
AdditionalProperties.UnmarshalJSON and AdditionalProperties.MarshalJSON, which
bypasses the platform-abstracting wrappers and can break WASM builds; change
those calls to use the package-level Unmarshal and Marshal wrapper functions
instead, and remove the direct sonic import so the file relies on the
json_native/json_wasm abstraction; update the calls in the
AdditionalProperties.UnmarshalJSON, AdditionalProperties.MarshalJSON methods to
call Unmarshal(...) and Marshal(...) respectively.
🧹 Nitpick comments (2)
core/internal/testutil/structured_outputs.go (1)
421-425: Correct migration to the new AdditionalProperties type.The construction of
AdditionalPropertieswithBoolValuecorrectly adapts to the new structured type.Based on learnings, consider using
bifrost.Ptr()for creating the bool pointer for consistency:AdditionalProperties: &schemas.AdditionalProperties{BoolValue: bifrost.Ptr(additionalProps)},core/providers/gemini/utils.go (1)
1145-1149: Useschemas.Ptr()for pointer creation to maintain codebase consistency.The logic correctly handles both boolean and object forms of
additionalPropertiesper the JSON Schema specification. However, for consistency with the codebase conventions, prefer usingschemas.Ptr()instead of the address operator for the inner pointer values.Based on learnings, the repository prefers
schemas.Ptr()over&for creating pointers.♻️ Suggested refactor
// Extract additionalProperties if additionalProps, ok := normalizedSchemaMap["additionalProperties"].(bool); ok { - jsonSchema.AdditionalProperties = &schemas.AdditionalProperties{BoolValue: &additionalProps} + jsonSchema.AdditionalProperties = &schemas.AdditionalProperties{BoolValue: schemas.Ptr(additionalProps)} } else if additionalProps, ok := normalizedSchemaMap["additionalProperties"].(map[string]any); ok { - jsonSchema.AdditionalProperties = &schemas.AdditionalProperties{ObjectValue: &additionalProps} + jsonSchema.AdditionalProperties = &schemas.AdditionalProperties{ObjectValue: schemas.Ptr(additionalProps)} }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/internal/testutil/structured_outputs.gocore/providers/anthropic/utils.gocore/providers/cohere/utils.gocore/providers/gemini/gemini_test.gocore/providers/gemini/utils.gocore/schemas/chatcompletions.gocore/schemas/responses.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
core/schemas/responses.gocore/providers/anthropic/utils.gocore/schemas/chatcompletions.gocore/providers/gemini/utils.gocore/providers/gemini/gemini_test.gocore/internal/testutil/structured_outputs.gocore/providers/cohere/utils.go
🧠 Learnings (14)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/schemas/responses.gocore/providers/anthropic/utils.gocore/schemas/chatcompletions.gocore/providers/gemini/utils.gocore/providers/gemini/gemini_test.gocore/internal/testutil/structured_outputs.gocore/providers/cohere/utils.go
📚 Learning: 2025-12-29T11:54:55.836Z
Learnt from: akshaydeo
Repo: maximhq/bifrost PR: 1153
File: framework/configstore/rdb.go:2221-2246
Timestamp: 2025-12-29T11:54:55.836Z
Learning: In Go reviews, do not flag range-over-int patterns like for i := range n as compile-time errors, assuming Go 1.22+ semantics. Only flag actual range-capable values (slices, arrays, maps, channels, strings) and other compile-time issues. This applies to all Go files across the repository.
Applied to files:
core/schemas/responses.gocore/providers/anthropic/utils.gocore/schemas/chatcompletions.gocore/providers/gemini/utils.gocore/providers/gemini/gemini_test.gocore/internal/testutil/structured_outputs.gocore/providers/cohere/utils.go
📚 Learning: 2026-01-14T04:40:11.480Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1312
File: framework/modelcatalog/pricing.go:276-426
Timestamp: 2026-01-14T04:40:11.480Z
Learning: In the Bifrost codebase, ImageUsage and other usage types guarantee that TotalTokens is populated (computed as InputTokens + OutputTokens if providers don’t supply TotalTokens). Reviewers can rely on this invariant and should not assume TotalTokens may be missing when input/output tokens exist. When implementing tiering logic or token-based decisions, you can safely use TotalTokens without extra null/zero guards, provided you’re in a context where InputTokens and OutputTokens are present. If a branch might discard tokens, ensure the invariant is preserved or add explicit checks only where the inputs are confirmed to be valid.
Applied to files:
core/schemas/responses.gocore/providers/anthropic/utils.gocore/schemas/chatcompletions.gocore/providers/gemini/utils.gocore/providers/gemini/gemini_test.gocore/internal/testutil/structured_outputs.gocore/providers/cohere/utils.go
📚 Learning: 2026-01-14T13:30:28.760Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 1326
File: plugins/semanticcache/test_utils.go:545-559
Timestamp: 2026-01-14T13:30:28.760Z
Learning: In the maximhq/bifrost repository, prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically. Apply this consistently across all code paths, including test utilities, to improve consistency and readability. Replace occurrences of &value where a *T is expected with bifrost.Ptr(value) (or an equivalent call) and ensure the function is in scope and used correctly for the target pointer type.
Applied to files:
core/schemas/responses.gocore/providers/anthropic/utils.gocore/schemas/chatcompletions.gocore/providers/gemini/utils.gocore/providers/gemini/gemini_test.gocore/internal/testutil/structured_outputs.gocore/providers/cohere/utils.go
📚 Learning: 2025-12-19T09:26:54.961Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/utils/utils.go:1050-1051
Timestamp: 2025-12-19T09:26:54.961Z
Learning: Update streaming end-marker handling so HuggingFace is treated as a non-[DONE] provider for backends that do not emit a DONE marker (e.g., meta llama on novita). In core/providers/utils/utils.go, adjust ProviderSendsDoneMarker() (or related logic) to detect providers that may not emit DONE and avoid relying on DONE as the sole end signal. Add tests to cover both DONE-emitting and non-DONE backends, with clear documentation in code comments explaining the rationale and any fallback behavior.
Applied to files:
core/providers/anthropic/utils.gocore/providers/gemini/utils.gocore/providers/gemini/gemini_test.gocore/providers/cohere/utils.go
📚 Learning: 2026-01-10T11:27:47.535Z
Learnt from: Radheshg04
Repo: maximhq/bifrost PR: 1256
File: core/providers/openai/openai.go:2276-2385
Timestamp: 2026-01-10T11:27:47.535Z
Learning: Validate image generation requests for nil and missing prompts before dispatch. Follow the same pattern used here: core/bifrost.go validates nil/empty prompts, providerUtils.CheckContextAndGetRequestBody returns a structured error when the request converter yields nil, and apply this across all providers (including OpenAI) to avoid sending null bodies.
Applied to files:
core/providers/anthropic/utils.gocore/providers/gemini/utils.gocore/providers/gemini/gemini_test.gocore/providers/cohere/utils.go
📚 Learning: 2026-01-11T14:08:10.341Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1298
File: core/providers/anthropic/anthropic.go:682-699
Timestamp: 2026-01-11T14:08:10.341Z
Learning: In Anthroplic streaming implementations (and analogous providers), ensure that the final 'summary' chunk, which carries usage information and metadata, is emitted after all delta chunks and uses a chunk index of last_delta_index + 1. This differentiates the summary chunk from content delta chunks. Apply this convention consistently in the anthropic provider code and in similar streaming providers, and consider adding a targeted test to assert the ordering and chunk index logic.
Applied to files:
core/providers/anthropic/utils.go
📚 Learning: 2026-01-13T13:36:35.221Z
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 1319
File: core/providers/anthropic/responses.go:937-937
Timestamp: 2026-01-13T13:36:35.221Z
Learning: In core/providers/anthropic/responses.go, when handling Anthropic API streaming responses, ensure that content_block_start events include a signature field set to an empty string (e.g., contentBlock.Signature = ""). The actual signature is delivered later via signature_delta events. This behavior is per Anthropic's specification and should not be treated as an error. This guideline should apply to all Anthropic response handling files under core/providers/anthropic/ and similar go files that process streaming blocks.
Applied to files:
core/providers/anthropic/utils.go
📚 Learning: 2026-01-14T06:57:42.750Z
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 1319
File: core/providers/anthropic/types.go:248-336
Timestamp: 2026-01-14T06:57:42.750Z
Learning: For Anthropic citation types (page_location, char_location, content_block_location), ensure there is an optional string field file_id to reference uploaded files. Update the Go structs modeling these citations to include FileID *string (or string with omitempty) and document its optionality in comments, so code consuming these types can handle absence of file_id gracefully.
Applied to files:
core/providers/anthropic/utils.go
📚 Learning: 2026-01-14T06:57:42.750Z
Learnt from: TejasGhatte
Repo: maximhq/bifrost PR: 1319
File: core/providers/anthropic/types.go:248-336
Timestamp: 2026-01-14T06:57:42.750Z
Learning: In core/providers/anthropic/types.go, ensure the web_search_result_location citation type includes a string field named 'url' alongside the existing fields 'encrypted_index', 'title', and 'cited_text'. If the field is missing, add it with type string and appropriate struct tags (e.g., json and/or db tags) and update any related serialization or usage accordingly.
Applied to files:
core/providers/anthropic/utils.go
📚 Learning: 2026-01-14T10:53:44.658Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1326
File: core/providers/gemini/gemini.go:1679-1754
Timestamp: 2026-01-14T10:53:44.658Z
Learning: Validate image generation inputs in core/bifrost.go before invoking any provider handler. Ensure in all provider implementations (e.g., core/providers/gemini/gemini.go) that the request and request.Input are non-nil before use, to prevent nil dereferences and provide clear error handling. Apply this invariant broadly to all providers and add tests for nil input scenarios.
Applied to files:
core/providers/anthropic/utils.gocore/providers/gemini/utils.gocore/providers/gemini/gemini_test.gocore/providers/cohere/utils.go
📚 Learning: 2025-12-17T08:44:08.788Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1114
File: core/schemas/chatcompletions.go:224-228
Timestamp: 2025-12-17T08:44:08.788Z
Learning: In core/schemas/chatcompletions.go, ensure the schema structures mirror OpenAI's API specifications exactly. Use the valid values for fields like ChatAudioParameters.Format and ChatAudioParameters.Voice as defined by OpenAI's documentation, and avoid adding additional inline documentation or constants to maintain direct compatibility with OpenAI's API.
Applied to files:
core/schemas/chatcompletions.go
📚 Learning: 2025-12-15T10:16:21.909Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/providers/huggingface/huggingface_test.go:12-63
Timestamp: 2025-12-15T10:16:21.909Z
Learning: In provider tests under core/providers/<provider>/*_test.go, do not require or flag the use of defer for Shutdown(); instead call client.Shutdown() at the end of each test function. This pattern appears consistent across all provider tests. Apply this rule only within this path; for other tests or resources, defer may still be appropriate.
Applied to files:
core/providers/gemini/gemini_test.go
📚 Learning: 2025-12-19T08:29:20.286Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1095
File: core/internal/testutil/count_tokens.go:30-67
Timestamp: 2025-12-19T08:29:20.286Z
Learning: In core/internal/testutil test files, enforce using GetTestRetryConfigForScenario() to obtain a generic retry config, then construct a typed retry config (e.g., CountTokensRetryConfig, EmbeddingRetryConfig, TranscriptionRetryConfig) with an empty Conditions slice. Copy only MaxAttempts, BaseDelay, MaxDelay, OnRetry, and OnFinalFail from the generic config. This convention should be consistently applied across all test files in this directory.
Applied to files:
core/internal/testutil/structured_outputs.go
🧬 Code graph analysis (7)
core/schemas/responses.go (1)
core/schemas/chatcompletions.go (1)
AdditionalProperties(315-318)
core/providers/anthropic/utils.go (1)
core/schemas/chatcompletions.go (1)
AdditionalProperties(315-318)
core/schemas/chatcompletions.go (2)
core/schemas/json_native.go (2)
Unmarshal(18-20)Marshal(8-10)core/schemas/json_wasm.go (2)
Unmarshal(22-24)Marshal(8-10)
core/providers/gemini/utils.go (1)
core/schemas/chatcompletions.go (1)
AdditionalProperties(315-318)
core/providers/gemini/gemini_test.go (2)
core/schemas/chatcompletions.go (1)
AdditionalProperties(315-318)core/schemas/utils.go (1)
Ptr(14-16)
core/internal/testutil/structured_outputs.go (1)
core/schemas/chatcompletions.go (1)
AdditionalProperties(315-318)
core/providers/cohere/utils.go (1)
core/schemas/chatcompletions.go (1)
AdditionalProperties(315-318)
🔇 Additional comments (6)
core/providers/cohere/utils.go (1)
76-80: LGTM! Correct handling of additionalProperties as bool or object.The implementation correctly handles both JSON Schema forms: boolean and object. The type assertion pattern (try bool first, then map) aligns with the approach used in other providers (anthropic, gemini).
core/providers/anthropic/utils.go (1)
768-772: LGTM! Consistent with the new AdditionalProperties type.The implementation correctly handles both boolean and object forms for
additionalProperties, consistent with the changes in the cohere provider and the JSON Schema specification.core/schemas/chatcompletions.go (1)
312-337: Well-designed union type for AdditionalProperties.The struct design with
BoolValueandObjectValuepointers cleanly represents the JSON Schema specification whereadditionalPropertiescan be either a boolean or a schema object. The precedence logic (ObjectValue over BoolValue) in marshalling is sensible.core/schemas/responses.go (1)
132-140: LGTM! Consistent type update for AdditionalProperties.The field type change from
*boolto*AdditionalPropertiesaligns with the new structured type defined inchatcompletions.go, enabling both boolean and object representations ofadditionalPropertiesin the Responses API schema format.core/internal/testutil/structured_outputs.go (1)
534-539: Correct adaptation to new AdditionalProperties type.Same pattern as above - correctly constructs the new type. Consider using
bifrost.Ptr()for the inner bool pointer for consistency with codebase conventions.core/providers/gemini/gemini_test.go (1)
559-559: LGTM!The test correctly updates to the new
AdditionalPropertiesstruct type, properly wrapping the boolean value inBoolValue. This aligns with the schema changes and tests the boolean case for the responses API.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@akshaydeo Can I also have a review here and for #1331 ? Would be really nice if these go into the next release. Thanks a lot! |
|
Closing this since #1319 already contains the fix and was merged. |
1 participant
Summary
This PR allows tool parameter and jsonschema's additionalProperties to be either bool or object to make bifrost work with Claude Code's tools.
Changes
In recent versions, Claude Code built-in tools include an object in
additionalPropertiesof tool params. This makes bifrost fails to parse the request due to the internal Chat Completion request struct only allows bool foradditionalPropertiesin tool params. However, in jsonschema spec,additionalPropertiescan actually be either an object or a bool, making bifrost behavior a bug. See doc here: https://json-schema.org/understanding-json-schema/reference/object#additionalpropertiesAdditionally, I also apply the change to
additionalPropertiesinResponsesTextConfigFormatJSONSchema.This is the Claude Code tool that breaks bifrost. This tool only seems to get added when there are other MCP added as well.
{ "name": "ExitPlanMode", "description": "Use this tool when you are in plan mode and have finished writing your plan to the plan file and are ready for user approval.\n\n## How This Tool Works\n- You should have already written your plan to the plan file specified in the plan mode system message\n- This tool does NOT take the plan content as a parameter - it will read the plan from the file you wrote\n- This tool simply signals that you're done planning and ready for the user to review and approve\n- The user will see the contents of your plan file when they review it\n\n## Requesting Permissions (allowedPrompts)\nWhen calling this tool, you can request prompt-based permissions for bash commands your plan will need. These are semantic descriptions of actions, not literal commands.\n\n**How to use:**\n```json\n{\n \"allowedPrompts\": [\n { \"tool\": \"Bash\", \"prompt\": \"run tests\" },\n { \"tool\": \"Bash\", \"prompt\": \"install dependencies\" },\n { \"tool\": \"Bash\", \"prompt\": \"build the project\" }\n ]\n}\n```\n\n**Guidelines for prompts:**\n- Use semantic descriptions that capture the action's purpose, not specific commands\n- \"run tests\" matches: npm test, pytest, go test, bun test, etc.\n- \"install dependencies\" matches: npm install, pip install, cargo build, etc.\n- \"build the project\" matches: npm run build, make, cargo build, etc.\n- Keep descriptions concise but descriptive\n- Only request permissions you actually need for the plan\n- Scope permissions narrowly, like a security-conscious human would:\n - **Never combine multiple actions into one permission** - split them into separate, specific permissions (e.g. \"list pods in namespace X\", \"view logs in namespace X\")\n - Prefer \"run read-only database queries\" over \"run database queries\"\n - Prefer \"run tests in the project\" over \"run code\"\n - Add constraints like \"read-only\", \"local\", \"non-destructive\" whenever possible. If you only need read-only access, you must only request read-only access.\n - Prefer not to request overly broad permissions that would grant dangerous access, especially any access to production data or to make irrecoverable changes\n - When interacting with cloud environments, add constraints like \"in the foobar project\", \"in the baz namespace\", \"in the foo DB table\"\n - Never request broad tool access like \"run k8s commands\" - always scope to specific actions and namespaces, ideally with constraints such as read-only\n\n**Benefits:**\n- Commands matching approved prompts won't require additional permission prompts\n- The user sees the requested permissions when approving the plan\n- Permissions are session-scoped and cleared when the session ends\n\n## When to Use This Tool\nIMPORTANT: Only use this tool when the task requires planning the implementation steps of a task that requires writing code. For research tasks where you're gathering information, searching files, reading files or in general trying to understand the codebase - do NOT use this tool.\n\n## Before Using This Tool\nEnsure your plan is complete and unambiguous:\n- If you have unresolved questions about requirements or approach, use AskUserQuestion first (in earlier phases)\n- Once your plan is finalized, use THIS tool to request approval\n\n**Important:** Do NOT use AskUserQuestion to ask \"Is this plan okay?\" or \"Should I proceed?\" - that's exactly what THIS tool does. ExitPlanMode inherently requests user approval of your plan.\n\n## Examples\n\n1. Initial task: \"Search for and understand the implementation of vim mode in the codebase\" - Do not use the exit plan mode tool because you are not planning the implementation steps of a task.\n2. Initial task: \"Help me implement yank mode for vim\" - Use the exit plan mode tool after you have finished planning the implementation steps of the task.\n3. Initial task: \"Add a new feature to handle user authentication\" - If unsure about auth method (OAuth, JWT, etc.), use AskUserQuestion first, then use exit plan mode tool after clarifying the approach.\n", "input_schema": { "$schema": "https://json-schema.org/draft/2020-12/schema", "type": "object", "properties": { "allowedPrompts": { "description": "Prompt-based permissions needed to implement the plan. These describe categories of actions rather than specific commands.", "type": "array", "items": { "type": "object", "properties": { "tool": { "description": "The tool this prompt applies to", "type": "string", "enum": [ "Bash" ] }, "prompt": { "description": "Semantic description of the action, e.g. \"run tests\", \"install dependencies\"", "type": "string" } }, "required": [ "tool", "prompt" ], "additionalProperties": false } } }, "additionalProperties": {} # <- this one here } },Type of change
Affected areas
How to test
Test with Claude Code
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines