fix: prompt caching issues for OpenAI chat completions and OrderedMap implementation#952
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduces OrderedMap with deterministic JSON marshaling; replaces many Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Pratham-Mishra04
force-pushed
the
11-28-fix_added_ordered_map_to_fix_prompt_caching_issue_for_openai
branch
from
71c8986 to
a7f7283
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
core/providers/bedrock/responses.go (1)
298-325: Tool schemapropertiesconversion only recognizesOrderedMap-typed valuesIn the
map[string]interface{}branch ofToBifrostResponsesRequest,params.Propertiesis now set only whenparamsMap["properties"]is a*schemas.OrderedMaporschemas.OrderedMap. If an external caller buildsInputSchema.JSONwith a plainmap[string]interface{}{ "properties": map[string]interface{}{...} }, properties will silently be dropped andparams.Propertiesleft nil.If backward compatibility with such callers matters, consider also handling a bare map and converting it, e.g.:
- // Handle both pointer and non-pointer properties - if propsPtr, ok := paramsMap["properties"].(*schemas.OrderedMap); ok { - params.Properties = propsPtr - } else if props, ok := paramsMap["properties"].(schemas.OrderedMap); ok { - params.Properties = &props - } + // Handle OrderedMap (pointer or value) and plain map[string]interface{}. + if propsPtr, ok := paramsMap["properties"].(*schemas.OrderedMap); ok { + params.Properties = propsPtr + } else if props, ok := paramsMap["properties"].(schemas.OrderedMap); ok { + params.Properties = &props + } else if propsMap, ok := paramsMap["properties"].(map[string]interface{}); ok { + om := schemas.OrderedMap(propsMap) + params.Properties = &om + }This keeps OrderedMap as the canonical type while still tolerating existing map-shaped inputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/changelog.md(1 hunks)core/internal/testutil/responses_stream.go(2 hunks)core/internal/testutil/utils.go(4 hunks)core/mcp.go(1 hunks)core/providers/bedrock/bedrock_test.go(4 hunks)core/providers/bedrock/responses.go(4 hunks)core/providers/bedrock/types.go(2 hunks)core/providers/bedrock/utils.go(1 hunks)core/providers/cohere/types.go(2 hunks)core/providers/cohere/utils.go(1 hunks)core/providers/gemini/utils.go(1 hunks)core/schemas/chatcompletions.go(2 hunks)transports/changelog.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
core/providers/gemini/utils.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(227-227)
core/providers/bedrock/utils.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(227-227)
core/providers/cohere/types.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(227-227)
core/providers/cohere/utils.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(227-227)
core/internal/testutil/utils.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(227-227)
core/providers/bedrock/responses.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(227-227)
core/providers/bedrock/types.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(227-227)
core/internal/testutil/responses_stream.go (2)
core/schemas/responses.go (2)
ResponsesStreamResponseType(1351-1351)BifrostResponsesStreamResponse(1421-1459)core/schemas/chatcompletions.go (1)
OrderedMap(227-227)
core/providers/bedrock/bedrock_test.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(227-227)
🪛 markdownlint-cli2 (0.18.1)
core/changelog.md
4-4: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🔇 Additional comments (13)
transports/changelog.md (2)
9-9: Formatting correction looks good.The doubled dash has been corrected to match the single-dash format used throughout the changelog.
10-10: New changelog entry properly documented.The prompt caching fix entry is concise, follows the existing format, and correctly references the resolved issue.
core/changelog.md (1)
4-4: Changelog entry is properly formatted.The new entry follows the existing format and correctly documents the prompt caching fix. The static analysis warning (MD037: "Spaces inside emphasis markers") appears to be a false positive—no emphasis marker spacing issues are present in this line and can be safely disregarded.
core/providers/bedrock/types.go (1)
3-4: LGTM!The migration of
AdditionalModelRequestFieldstoschemas.OrderedMapensures deterministic JSON serialization, which is essential for consistent prompt caching behavior with Bedrock.Also applies to: 51-51
core/providers/cohere/types.go (1)
165-166: LGTM!Using
schemas.OrderedMapforCohereDocument.Dataensures deterministic JSON key ordering when serializing document data.core/schemas/chatcompletions.go (2)
227-268: LGTM - Deterministic JSON serialization with sorted keys.The
OrderedMaptype with alphabetically sorted key marshaling ensures consistent JSON output for prompt caching. The implementation correctly handles nil maps and strips the trailing newline added byjson.Encoder.Encode.
219-225: I'll help you verify that all callers have been updated for thePropertiesfield type change incore/schemas/chatcompletions.go. Let me start by examining the codebase to understand the scope of this change.
<function_calls>
#!/bin/bashFirst, let's understand the ToolFunctionParameters struct and locate related code
rg -n "type ToolFunctionParameters" --type go -A 20
</function_calls><function_calls>
#!/bin/bashSearch for all usages of Properties field, especially in ToolFunctionParameters context
rg -n ".Properties\s*=" --type go -B 3 -A 1 | head -100
</function_calls><function_calls>
#!/bin/bashCheck for OrderedMap type definition and usage
rg -n "OrderedMap" --type go -B 2 -A 2 | head -80
</function_calls><function_calls>
#!/bin/bashSearch for any code that might still be using map[string]interface{} with Properties
rg -n "map[string]interface{}" --type go -B 3 -A 3 | grep -i "properties|parameter" | head -50
</function_calls>core/providers/gemini/utils.go (1)
103-122: LGTM!The return type change to
schemas.OrderedMapis compatible since it's a type alias formap[string]interface{}. The existingmap[string]interface{}returns are implicitly converted.core/mcp.go (1)
810-815: MCP tool properties → OrderedMap implementation looks correctCopying
InputSchema.Propertiesinto a freshschemas.OrderedMapand passing a*OrderedMap(or nil when empty) is safe and matches the new API; no concurrency or escaping issues here.Also applies to: 823-824
core/internal/testutil/responses_stream.go (1)
66-240: Streaming test harness and OrderedMap tool schema updates look solidThe revised streaming validation loop (timeouts, chunk limits, structured logging) is robust and defensive, and the
get_weathertool now using&schemas.OrderedMap{...}forParameters.Propertiesis consistent with the new schema type and should keep JSON serialization deterministic.Also applies to: 271-292
core/internal/testutil/utils.go (1)
108-149: Test helper tool schemas correctly migrated toOrderedMapUsing
&schemas.OrderedMap{...}forWeatherToolFunction,CalculatorToolFunction, andTimeToolFunctionmatches the productionToolFunctionParameters.Propertiestype and keeps test scaffolding in sync; theGetLionBase64Imagewhitespace tweak is a no‑op.Also applies to: 203-216
core/providers/bedrock/responses.go (1)
570-579: Using emptyOrderedMapfor inferred tool schemas is consistent with the new typeThe minimal tool definitions inferred in
extractToolsFromResponsesConversationHistorynow useParameters: &schemas.ToolFunctionParameters{ Type: "object", Properties: &schemas.OrderedMap{} }both on first creation and in the null‑schema fallback. That’s aligned with the OrderedMap migration and avoids nil maps while still representing an emptypropertiesobject.Also applies to: 591-596
core/providers/bedrock/bedrock_test.go (1)
19-24: Now I can access the repository. Let me examine the specific files and lines mentioned in the review:
<function_calls>
#!/bin/bashFirst, examine bedrock_test.go around lines 19-24
echo "=== bedrock_test.go lines 15-30 ==="
sed -n '15,30p' core/providers/bedrock/bedrock_test.go
</function_calls>Now let me check the other referenced sections:
<function_calls>
#!/bin/bashCheck lines 176-185
echo "=== bedrock_test.go lines 170-190 ==="
sed -n '170,190p' core/providers/bedrock/bedrock_test.go
echo ""Check lines 288-291
echo "=== bedrock_test.go lines 285-295 ==="
sed -n '285,295p' core/providers/bedrock/bedrock_test.go
</function_calls>Let me check the other line ranges:
<function_calls>
#!/bin/bashCheck lines 579-582
echo "=== bedrock_test.go lines 575-590 ==="
sed -n '575,590p' core/providers/bedrock/bedrock_test.go
echo ""Check lines 619-622
echo "=== bedrock_test.go lines 615-630 ==="
sed -n '615,630p' core/providers/bedrock/bedrock_test.go
</function_calls>Now let me check the critical TestToBedrockResponsesRequest_AdditionalFields test around lines 1579-1599:
<function_calls>
#!/bin/bashFirst check total lines
wc -l core/providers/bedrock/bedrock_test.go
echo ""Check lines 1575-1605
echo "=== bedrock_test.go lines 1575-1605 ==="
sed -n '1575,1605p' core/providers/bedrock/bedrock_test.go
</function_calls>Now let me examine the responses.go file to understand the ToBedrockResponsesRequest implementation:
<function_calls>
#!/bin/bashFind and examine ToBedrockResponsesRequest function
rg "func.*ToBedrockResponsesRequest" -n core/providers/bedrock/responses.go
</function_calls>Let me get the full function:
<function_calls>
#!/bin/bashGet ToBedrockResponsesRequest function from responses.go
rg "func.*ToBedrockResponsesRequest" -A 100 core/providers/bedrock/responses.go | head -120
</function_calls>Let me check what additionalModelRequestFieldPaths conversion looks like:
<function_calls>
#!/bin/bashSearch for additionalModelRequestFieldPaths handling in responses.go
rg "additionalModelRequestFieldPaths" -n -B 5 -A 5 core/providers/bedrock/responses.go
</function_calls>Let me check the types to understand the OrderedMap field definition:
<function_calls>
#!/bin/bashCheck the types file for field definitions
rg "AdditionalModelRequestFields" -n -B 2 -A 2 core/providers/bedrock/types.go
</function_calls>Let me look for the schemas package to understand OrderedMap:
<function_calls>
#!/bin/bashSearch for schemas.OrderedMap definition
rg "type OrderedMap" -n -B 2 -A 5 --type go
</function_calls>Let me check how ExtraParams is handled in the test:
<function_calls>
#!/bin/bashGet the full TestToBedrockResponsesRequest_AdditionalFields test
rg "TestToBedrockResponsesRequest_AdditionalFields" -A 50 core/providers/bedrock/bedrock_test.go
</function_calls>Let me understand the exact flow by looking at the BedrockRequest type:
<function_calls>
#!/bin/bashCheck BedrockRequest struct definition
rg "type BedrockRequest" -n -A 15 core/providers/bedrock/types.go
</function_calls>Let me see the full responses.go function that handles the conversion:
<function_calls>
#!/bin/bashGet line count and check the ToBedrockResponsesRequest function more thoroughly
wc -l core/providers/bedrock/responses.go
echo ""Search for where additionalModelRequestFieldPaths is converted
rg "additionalModelRequestFieldPaths|AdditionalModelRequestFields" -n core/providers/bedrock/responses.go
</function_calls>Let me get a larger context around that area:
<function_calls>
#!/bin/bashGet lines around the conversion of additionalModelRequestFieldPaths
sed -n '1,150p' core/providers/bedrock/responses.go | tail -80
</function_calls>Let me search for the exact handling in responses.go:
<function_calls>
#!/bin/bashFind where additionalModelRequestFieldPaths is processed
rg "ExtraParams" -n -B 3 -A 3 core/providers/bedrock/responses.go | head -60
</function_calls>Let me get the complete ToBedrockResponsesRequest function:
<function_calls>
#!/bin/bashFind the function and get all its content
line_start=$(rg "^func.*ToBedrockResponsesRequest" -n core/providers/bedrock/responses.go | cut -d: -f1)
if [ -n "$line_start" ]; thenGet next 200 lines from the function start
sed -n "${line_start},$((line_start + 200))p" core/providers/bedrock/responses.go
fi
</function_calls>Let me also check what the test actually asserts:
<function_calls>
#!/bin/bashGet full TestToBedrockResponsesRequest_AdditionalFields test
line_start=$(rg "^func.*TestToBedrockResponsesRequest_AdditionalFields" -n core/providers/bedrock/bedrock_test.go | cut -d: -f1)
if [ -n "$line_start" ]; then
sed -n "${line_start},$((line_start + 50))p" core/providers/bedrock/bedrock_test.go
fi
</function_calls>Let me check if there's a conversion function:
<function_calls>
#!/bin/bashSearch for any conversion from map to OrderedMap
rg "OrderedMap|ExtraParams" -n -B 2 -A 2 core/providers/bedrock/responses.go | head -100
</function_calls>Let me look at the complete responses.go file to understand the implementation:
<function_calls>
#!/bin/bashGet full responses.go to understand all functions
cat core/providers/bedrock/responses.go | head -200
</function_calls>Let me continue reading responses.go:
<function_calls>
#!/bin/bashGet full responses.go file
cat core/providers/bedrock/responses.go
</function_calls>Now let me check the schemas package to understand OrderedMap:
<function_calls>
#!/bin/bashFind schemas package
find . -path "/schemas/.go" -type f | head -5
</function_calls>Let me search for OrderedMap:
<function_calls>
#!/bin/bashSearch for OrderedMap type definition
rg "type OrderedMap" -n -A 10 --type go
</function_calls>Let me check the import statements in responses.go:
<function_calls>
#!/bin/bashGet imports from responses.go
head -30 core/providers/bedrock/responses.go | grep -E "^import|schemas|OrderedMap"
</function_calls>Let me look more carefully at the test assertion to understand what's happening:
<function_calls>
#!/bin/bashGet the full test including the assertion
rg "TestToBedrockResponsesRequest_AdditionalFields" -A 100 core/providers/bedrock/bedrock_test.go | head -120
</function_calls>Let me check what assertion method is used:
<function_calls>
#!/bin/bashSearch for assertion in the test
rg "TestToBedrockResponsesRequest_AdditionalFields" -A 100 core/providers/bedrock/bedrock_test.go | grep -E "assert|require|Equal|Nil"
</function_calls>Let me get line 1579 onwards from the test file directly:
<function_calls>
#!/bin/bashGet lines 1579-1650 to see the full test
sed -n '1579,1650p' core/providers/bedrock/bedrock_test.go
</function_calls>Let me verify the exact behavior by checking if there's any conversion logic:
<function_calls>
#!/bin/bashCheck if there's any map-to-OrderedMap conversion
rg "map[string]interface" core/providers/bedrock/responses.go -n -B 2 -A 2
</function_calls>Let me check the test imports to see what assertion library is used:
<function_calls>
#!/bin/bashGet imports from bedrock_test.go
head -50 core/providers/bedrock/bedrock_test.go | grep -E "^import|^[[:space:]]*""
</function_calls>Let me check if OrderedMap is even defined in schemas:
<function_calls>
#!/bin/bashFind and check schemas package
find . -name "*.go" -type f | xargs grep -l "type OrderedMap" 2>/dev/null
</function_calls>Let me check types.go for field definitions:
<function_calls>
#!/bin/bashGet types.go to see BedrockRequest structure
cat core/providers/bedrock/types.go
</function_calls>Now let me check if there's a schemas package in the bifrost repo:
<function_calls>
#!/bin/bashSearch for schemas package
find . -name "schemas" -type d
rg "package schemas" --type go
</function_calls>Let me check what schemas refers to in the imports:
<function_calls>
#!/bin/bashCheck imports in bedrock_test.go
grep -A 30 "^import" core/providers/bedrock/bedrock_test.go
</function_calls>Let me also check responses.go imports:
<function_calls>
#!/bin/bashCheck imports in responses.go
grep -A 20 "^import" core/providers/bedrock/responses.go
</function_calls>Let me search for where schemas is defined:
<function_calls>
#!/bin/bashSearch for schemas package definition across the repo
find . -name "*.go" -type f -exec grep -l "package schemas" {} ;
</function_calls>Let me check the go.mod to understand dependencies:
<function_calls>
#!/bin/bashCheck go.mod
cat go.mod | head -50
</function_calls>Let me search for OrderedMap in the go.mod dependencies:
<function_calls>
#!/bin/bashSearch entire go.mod
cat go.mod
</function_calls>Now let me check if schemas is an internal package:
<function_calls>
#!/bin/bashFind all packages in the internal directory
find . -type d -name "internal" | xargs -I {} find {} -name "*.go" -exec grep -l "package schemas" {} ;
</function_calls>Let me search more broadly:
<function_calls>
#!/bin/bashFind where OrderedMap is imported from in bedrock_test.go
rg "schemas." core/providers/bedrock/bedrock_test.go | head -5
</function_calls>Let me check the package declaration and imports of bedrock_test.go:
<function_calls>
#!/bin/bashGet package and imports from bedrock_test.go
head -60 core/providers/bedrock/bedrock_test.go
</function_calls>Let me find where schemas comes from:
<function_calls>
#!/bin/bashSearch for the full import path
rg '"[^"]*schemas"' core/providers/bedrock/bedrock_test.go
</function_calls>Let me search the entire repo for schemas:
<function_calls>
#!/bin/bashFind schemas package
find . -path ./vendor -prune -o -name "*.go" -type f -exec grep -l "package schemas" {} ;
</function_calls>Let me check if it's in an external package by looking at the full import in bedrock_test.go more carefully:
<function_calls>
#!/bin/bashGet first 100 lines of bedrock_test.go
sed -n '1,100p' core/providers/bedrock/bedrock_test.go
</function_calls>Now I see the imports. Let me look for the schemas package:
<function_calls>
#!/bin/bashFind schemas in the repo
find . -type d -name "schemas"
</function_calls>Let me look at the actual directory structure:
<function_calls>
#!/bin/bashList main directories
ls -la | grep "^d"
find . -maxdepth 2 -type d | sort
</function_calls>Now let me check the actual testProps definition by getting the relevant section from bedrock_test.go:
<function_calls>
#!/bin/bashLook at lines 1-50 more carefully to see variable declarations
sed -n '1,100p' core/providers/bedrock/bedrock_test.go | grep -A 10 "testProps"
</function_calls>Let me search for testProps definition:
<function_calls>
#!/bin/bashFind where testProps is defined
rg "testProps\s*=" -n core/providers/bedrock/bedrock_test.go | head -3
</function_calls>Let me check around those lines:
<function_calls>
#!/bin/bashLook at the actual testProps definition
sed -n '35,50p' core/providers/bedrock/bedrock_test.go
</function_calls>Let me check what the exact definition is:
<function_calls>
#!/bin/bashGet testProps with more context
sed -n '30,60p' core/providers/bedrock/bedrock_test.go
</function_calls>Now let me verify the core issue about the AdditionalFields test. Let me check the assertion again:
<function_calls>
#!/bin/bashGet the full TestToBedrockResponsesRequest_AdditionalFields test more carefully
rg "func.*TestToBedrockResponsesRequest_AdditionalFields" -n core/providers/bedrock/bedrock_test.go
</function_calls>Now let me see what's at that line:
<function_calls>
#!/bin/bashGet line number and show the test
line_num=$(rg "func.*TestToBedrockResponsesRequest_AdditionalFields" -n core/providers/bedrock/bedrock_test.go | cut -d: -f1)
echo "Test starts at line: $line_num"
sed -n "${line_num},$((line_num + 60))p" core/providers/bedrock/bedrock_test.go
</function_calls>Let me understand what ExtraParams is:
<function_calls>
#!/bin/bashSearch for ExtraParams definition/usage in the test
rg "ExtraParams" -n -B 3 -A 3 core/providers/bedrock/bedrock_test.go | grep -A 5 -B 5 "AdditionalFields"
</function_calls>Let me get a complete view of the test:
<function_calls>
#!/bin/bashShow lines 1579-1620 which should be the AdditionalFields test
sed -n '1579,1620p' core/providers/bedrock/bedrock_test.go
</function_calls>Now let me check the responses.go to see how it handles ExtraParams:
<function_calls>
#!/bin/bashSearch for where ExtraParams is used in responses.go
rg "ExtraParams|additionalModelRequestFieldPaths" -n -B 2 -A 2 core/providers/bedrock/responses.go
</function_calls>Let me get the complete responses.go file content:
<function_calls>
#!/bin/bashGet complete responses.go
wc -l core/providers/bedrock/responses.go
cat core/providers/bedrock/responses.go
</function_calls>Now let me check the types to understand BedrockRequest:
<function_calls>
#!/bin/bashGet BedrockRequest type definition
rg "type BedrockRequest struct" -A 30 core/providers/bedrock/types.go
</function_calls>Let me now check if there's any conversion logic for ExtraParams to OrderedMap:
<function_calls>
#!/bin/bashSearch for type assertions or conversions
rg "ExtraParams[" -n core/providers/bedrock/bedrock.go
</function_calls>Let me check the bedrock.go file to see how responses are converted:
<function_calls>
#!/bin/bashCheck bedrock.go for conversion logic
rg "ToBedrockResponsesRequest" -B 5 -A 15 core/providers/bedrock/bedrock.go
</function_calls>Let me look at the entire bedrock.go to understand the flow:
<function_calls>
#!/bin/bashGet bedrock.go
wc -l core/providers/bedrock/bedrock.go
head -200 core/providers/bedrock/bedrock.go
</function_calls>Let me search for how ExtraParams is actually constructed:
<function_calls>
#!/bin/bashSearch for ExtraParams assignment
rg '"additionalModelRequestFieldPaths"' -n -B 5 -A 5 core/providers/bedrock/
</function_calls>Let me check if there's a conversion utility function:
<function_calls>
#!/bin/bashSearch for any utility functions related to OrderedMap
rg "OrderedMap" -n core/providers/bedrock/*.go
</function_calls>Now I see the core issue! Let me verify the exact implementation:
<function_calls>
#!/bin/bashGet exact implementation of toOrderedMap function
rg "toOrderedMap" -n -B 2 -A 15 core/providers/bedrock/bedrock.go
</function_calls>Perfect! Now let me verify where this is called in responses.go:
<function_calls>
#!/bin/bashCheck if toOrderedMap is called for ExtraParams
rg "toOrderedMap" -n core/providers/bedrock/responses.go
</function_calls>Now let me examine the entire test assertion more carefully:
<function_calls>
#!/bin/bashGet the test assertions for AdditionalFields
sed -n '1579,1620p' core/providers/bedrock/bedrock_test.go
</function_calls>Let me understand what the test is comparing:
<function_calls>
#!/bin/bashSearch for the exact assertion comparing ExtraParams and AdditionalModelRequestFields
rg -A 30 "TestToBedrockResponsesRequest_AdditionalFields" core/providers/bedrock/bedrock_test.go | grep -B 5 -A 5 "ExtraParams|AdditionalModelRequestFields"
</function_calls>Let me get a clearer picture of how the types compare:
<function_calls>
#!/bin/bashLook at the complete test setup and assertions
sed -n '1579,1610p' core/providers/bedrock/bedrock_test.go
</function_calls>Now let me check if the test actually passes by examining imports and assertion library:
<function_calls>
#!/bin/bashCheck what assertion is being used in the test
grep -E "testify|assert.Equal|require.Equal" core/providers/bedrock/bedrock_test.go | head -3
</function_calls>Let me look at the entire AdditionalFields test section completely:
<function_calls>
#!/bin/bashGet lines 1575-1615 to see complete test
sed -n '1575,1615p' core/providers/bedrock/bedrock_test.go
</function_calls>Let me check the exact assertion in the test by looking for assert or require statements:
<function_calls>
#!/bin/bashGet the complete test with focus on assertions
sed -n '1579,1620p' core/providers/bedrock/bedrock_test.go | cat -n
</function_calls>Let me search for what assertion library and what's being tested:
<function_calls>
#!/bin/bashGet exact lines where the assertion happens
sed -n '1600,1620p' core/providers/bedrock/bedrock_test.go
</function_calls>Let me examine the full context more carefully:
<function_calls>
#!/bin/bashGet the entire AdditionalFields test function
awk '/func.*TestToBedrockResponsesRequest_AdditionalFields/,/^func/ {print NR": "$0}' core/providers/bedrock/bedrock_test.go | head -80
</function_calls>Let me use a different approach to extract the test:
<function_calls>
#!/bin/bashFind and extract the test function properly
line=$(grep -n "func.*TestToBedrockResponsesRequest_AdditionalFields" core/providers/bedrock/bedrock_test.go | cut -d: -f1)
end_line=$((line + 50))
echo "Test function from line $line to ~$end_line:"
sed -n "${line},${end_line}p" core/providers/bedrock/bedrock_test.go
</function_calls>Now let me look at the conversion in responses.go to see if ExtraParams values are converted:
<function_calls>
#!/bin/bashCheck how responses.go handles ExtraParams["additionalModelRequestFieldPaths"]
sed -n '1,100p' core/providers/bedrock/responses.go | grep -n -A 20 -B 3 "ExtraParams|toOrderedMap"
</function_calls>Let me view the full responses.go to understand the complete logic:
<function_calls>
#!/bin/bashGet complete responses.go with line numbers
cat -n core/providers/bedrock/responses.go
</function_calls>Perfect! Now I have all the information I need. Let me analyze this:
Now I have all the information needed to verify the review. Let me create the rewritten comment:
Tests correctly use
schemas.OrderedMapand conversion logic is in placeThe bedrock test file properly uses
schemas.OrderedMapfortestProps(lines 19-24) and tool parameterPropertiesthroughout, which is consistent with the schema API changes. Regarding theTestToBedrockResponsesRequest_AdditionalFieldstest concern: the test passes a plainmap[string]interface{}{ "top_k": 200 }viaExtraParams["additionalModelRequestFieldPaths"], andToBedrockResponsesRequestinresponses.go(lines 35–37) correctly converts this toschemas.OrderedMapusing thetoOrderedMap()utility function before assigning it tobedrockReq.AdditionalModelRequestFields. Legacy callers using plain maps will not lose those extra fields—the conversion is properly implemented.
Pratham-Mishra04
force-pushed
the
11-28-fix_added_ordered_map_to_fix_prompt_caching_issue_for_openai
branch
from
a7f7283 to
c7de786
Compare
Pratham-Mishra04
force-pushed
the
11-28-fix_added_ordered_map_to_fix_prompt_caching_issue_for_openai
branch
from
c7de786 to
5c34a3a
Compare
Pratham-Mishra04
force-pushed
the
11-28-fix_added_ordered_map_to_fix_prompt_caching_issue_for_openai
branch
from
5c34a3a to
a2f8784
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
core/providers/bedrock/responses.go (1)
449-452: Addresses the previous review feedback on type handling.This change correctly uses
SafeExtractOrderedMapto handle bothschemas.OrderedMapandmap[string]interface{}types, preventing silent data loss that occurred with the previous direct type assertion approach.
🧹 Nitpick comments (2)
core/changelog.md (1)
4-4: Fix markdown formatting issue.The static analysis tool flagged spaces inside emphasis markers. This appears to be a formatting inconsistency.
Apply this diff to fix the formatting:
-- fix: prompt caching issue fixes for openai chat completions +- fix: prompt caching issue fixes for openai chat completionscore/schemas/chatcompletions.go (1)
247-251: Slice mutation inconsistency with the stated design goal.The comment at line 264 states the intent to "Work on a normalized copy so we don't surprise callers by mutating om." However, this function mutates slices in-place rather than creating copies, which could cause unexpected side effects for callers holding references to the original slice.
case []interface{}: - for i := range x { - x[i] = normalizeValueToOrderedMap(x[i]) - } - return x + result := make([]interface{}, len(x)) + for i := range x { + result[i] = normalizeValueToOrderedMap(x[i]) + } + return result
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/changelog.md(1 hunks)core/internal/testutil/account.go(2 hunks)core/internal/testutil/prompt_caching.go(1 hunks)core/internal/testutil/response_validation.go(1 hunks)core/internal/testutil/responses_stream.go(2 hunks)core/internal/testutil/tests.go(1 hunks)core/internal/testutil/utils.go(4 hunks)core/mcp.go(1 hunks)core/providers/bedrock/bedrock_test.go(4 hunks)core/providers/bedrock/responses.go(4 hunks)core/providers/bedrock/types.go(2 hunks)core/providers/bedrock/utils.go(1 hunks)core/providers/cohere/types.go(2 hunks)core/providers/cohere/utils.go(1 hunks)core/providers/gemini/utils.go(1 hunks)core/providers/openai/openai_test.go(1 hunks)core/schemas/chatcompletions.go(2 hunks)core/schemas/utils.go(1 hunks)transports/changelog.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
core/schemas/utils.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/cohere/types.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/cohere/utils.go (1)
core/schemas/utils.go (1)
SafeExtractOrderedMap(527-550)
core/providers/bedrock/types.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/bedrock/utils.go (1)
core/schemas/utils.go (1)
SafeExtractOrderedMap(527-550)
core/internal/testutil/responses_stream.go (5)
core/schemas/bifrost.go (2)
ResponsesStreamRequest(92-92)BifrostStream(318-325)core/internal/testutil/test_retry_framework.go (2)
ResponsesStreamValidationResult(2129-2135)DeepCopyBifrostStream(40-47)core/schemas/responses.go (8)
ResponsesStreamResponseType(1351-1351)BifrostResponsesStreamResponse(1421-1459)ResponsesStreamResponseTypeOutputTextDelta(1369-1369)ResponsesStreamResponseTypeOutputItemAdded(1363-1363)ResponsesStreamResponseTypeContentPartAdded(1366-1366)ResponsesStreamResponseTypeCreated(1357-1357)ResponsesStreamResponseTypeCompleted(1359-1359)ResponsesStreamResponseTypeError(1418-1418)core/internal/testutil/validation_presets.go (2)
GetExpectationsForScenario(208-293)ModifyExpectationsForProvider(300-347)core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/bedrock/bedrock_test.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/openai/openai_test.go (1)
core/schemas/bifrost.go (1)
OpenAI(35-35)
core/internal/testutil/tests.go (1)
core/internal/testutil/prompt_caching.go (1)
RunPromptCachingTest(273-439)
core/providers/gemini/utils.go (2)
core/providers/gemini/types.go (1)
Schema(724-775)core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/internal/testutil/utils.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/bedrock/responses.go (2)
core/schemas/utils.go (1)
SafeExtractOrderedMap(527-550)core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
🪛 markdownlint-cli2 (0.18.1)
core/changelog.md
4-4: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🔇 Additional comments (22)
transports/changelog.md (1)
9-10: LGTM!The formatting fix and new changelog entry are appropriate and consistent with the PR objectives.
core/internal/testutil/response_validation.go (1)
520-549: LGTM! Well-structured cached tokens validation.The implementation correctly validates cached tokens percentage with:
- Proper nil checks for Usage and PromptTokensDetails
- Accurate percentage calculation
- Appropriate edge case handling for zero prompt tokens
- Clear, informative error messages
core/mcp.go (1)
810-827: LGTM! Proper OrderedMap conversion with nil handling.The conversion logic correctly:
- Checks for non-empty properties before creating OrderedMap
- Uses
maps.Copyto populate the ordered map- Assigns nil when no properties exist (appropriate behavior)
- Maintains type safety with the pointer to OrderedMap
This aligns with the PR's goal of ensuring deterministic JSON marshaling for tool parameters.
core/providers/cohere/types.go (1)
165-167: LGTM! Consistent adoption of OrderedMap.The change from
map[string]interface{}toschemas.OrderedMapfor the Data field is consistent with the PR's objective of ensuring deterministic JSON marshaling for tool and document parameters across providers.core/internal/testutil/account.go (1)
51-51: The model identifier "gpt-4.1" is correct and supported for OpenAI prompt caching. Web search confirms that "gpt-4.1" is a valid OpenAI model identifier in 2025 and explicitly supports prompt caching features, including extended 24-hour retention per OpenAI's Platform documentation. The field addition and model name are appropriate.core/providers/bedrock/utils.go (1)
48-50: LGTM! Safe extraction properly handles multiple input types.The use of
schemas.SafeExtractOrderedMapcorrectly handles bothOrderedMapandmap[string]interface{}inputs, preventing the type assertion panics flagged in previous reviews. This aligns with the PR's objective to support deterministic JSON serialization for prompt caching.core/providers/gemini/utils.go (1)
103-103: LGTM! Public API change aligns with project-wide OrderedMap adoption.The function signature change from
map[string]interface{}toschemas.OrderedMapis part of the broader effort to enable deterministic JSON serialization. SinceOrderedMapis a type alias, the underlying representation remains compatible.core/providers/bedrock/types.go (1)
51-51: LGTM! Field type change supports deterministic serialization.The change from
map[string]interface{}toschemas.OrderedMapforAdditionalModelRequestFieldsaligns with the PR objective to fix prompt caching issues. The corresponding extraction logic inutils.gousesSafeExtractOrderedMapto safely populate this field.core/providers/cohere/utils.go (1)
60-62: LGTM! Safe extraction handles multiple property types.The use of
schemas.SafeExtractOrderedMapcorrectly normalizes various input representations into anOrderedMap, addressing the type assertion issues flagged in previous reviews and supporting deterministic JSON serialization.core/internal/testutil/responses_stream.go (1)
279-289: LGTM! Test utility updated for OrderedMap properties.The change from
*map[string]interface{}to*schemas.OrderedMapfor thePropertiesfield correctly reflects the updated type definition, ensuring test tools align with the project-wide OrderedMap adoption.core/internal/testutil/utils.go (3)
111-122: LGTM! Test tool definitions updated for OrderedMap.The change from
*map[string]interface{}to*schemas.OrderedMapforWeatherToolFunction.Parameters.Propertiescorrectly reflects the updated type definition and ensures test utilities align with the project-wide OrderedMap adoption.
128-136: LGTM! Calculator tool updated consistently.The
CalculatorToolFunction.Parameters.Propertiesfield correctly uses*schemas.OrderedMap, maintaining consistency with other test tool definitions.
141-149: LGTM! Time tool updated consistently.The
TimeToolFunction.Parameters.Propertiesfield correctly uses*schemas.OrderedMap, completing the consistent update of all test tool definitions.core/providers/bedrock/bedrock_test.go (4)
19-24: LGTM! Test fixture updated for OrderedMap.The change from
map[string]interface{}toschemas.OrderedMapfortestPropscorrectly reflects the updated type definition and ensures test fixtures align with production code changes.
177-183: LGTM! Tool properties in test case use OrderedMap.The test case correctly uses
*schemas.OrderedMapfor thePropertiesfield, maintaining consistency with the updated type definition.
288-291: LGTM! AdditionalModelRequestFields updated in test.The "AllExtraParams" test case correctly uses
schemas.OrderedMapforAdditionalModelRequestFields, consistent with the type change inBedrockConverseRequest.
619-621: LGTM! Second AdditionalModelRequestFields test updated.The test case correctly uses
schemas.OrderedMapforAdditionalModelRequestFields, maintaining consistency across all test fixtures.core/schemas/chatcompletions.go (2)
218-224: LGTM on the type change for deterministic serialization.Changing
Propertiesfrom*map[string]interface{}to*OrderedMapenables consistent JSON key ordering, which is essential for prompt caching. TheOrderedMaptype alias maintains compatibility with existing code patterns while adding the custom marshaler.
259-303: Solid implementation for deterministic JSON marshaling.The approach correctly:
- Creates a normalized copy before modification
- Sorts keys alphabetically for deterministic output
- Recursively handles nested structures via
sonic.MarshalinvokingMarshalJSONon nestedOrderedMapvaluescore/internal/testutil/prompt_caching.go (1)
34-267: Tool definitions correctly use OrderedMap for consistent serialization.The tool definitions properly initialize
Propertieswith&schemas.OrderedMap{...}, ensuring deterministic JSON serialization for prompt caching. The structure aligns with the PR's goal of fixing prompt caching issues.core/providers/bedrock/responses.go (2)
308-310: Correct use of SafeExtractOrderedMap for properties extraction.Using
SafeExtractOrderedMaphere properly handles bothOrderedMapandmap[string]interface{}inputs, ensuring compatibility with various input sources.
574-576: Consistent use of OrderedMap for default tool properties.The initialization of default tool function parameters now uses
&schemas.OrderedMap{}instead of&map[string]interface{}{}, maintaining type consistency with the updatedToolFunctionParameters.Propertiesfield definition.Also applies to: 591-593
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
core/internal/testutil/prompt_caching.go (2)
285-288: Remove duplicate provider check.This check duplicates the one already performed at lines 275-278.
- if testConfig.Provider != schemas.OpenAI { - t.Logf("Prompt caching test skipped for provider %s (OpenAI-specific feature)", testConfig.Provider) - return - } - t.Run("PromptCaching", func(t *testing.T) {
371-375: Metadata uses ChatModel but request uses PromptCachingModel.The test metadata references
testConfig.ChatModelbut the actual request on line 323 usestestConfig.PromptCachingModel. This could cause confusion when debugging.TestMetadata: map[string]interface{}{ "provider": testConfig.Provider, - "model": testConfig.ChatModel, + "model": testConfig.PromptCachingModel, },core/schemas/utils.go (1)
527-550: Inconsistent nil return values for pointer cases.The function returns different values for nil pointer cases:
*map[string]interface{}nil →OrderedMap{}, false(line 540)*OrderedMapnil →nil, false(line 547)This inconsistency could cause nil pointer issues for callers expecting a non-nil empty map. Consider returning
OrderedMap{}, falseconsistently:case *OrderedMap: if v != nil { return *v, true } - return nil, false + return OrderedMap{}, false }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
core/changelog.md(1 hunks)core/internal/testutil/account.go(2 hunks)core/internal/testutil/prompt_caching.go(1 hunks)core/internal/testutil/response_validation.go(1 hunks)core/internal/testutil/responses_stream.go(2 hunks)core/internal/testutil/tests.go(1 hunks)core/internal/testutil/utils.go(4 hunks)core/mcp.go(1 hunks)core/providers/bedrock/bedrock_test.go(4 hunks)core/providers/bedrock/responses.go(4 hunks)core/providers/bedrock/types.go(2 hunks)core/providers/bedrock/utils.go(1 hunks)core/providers/cohere/types.go(2 hunks)core/providers/cohere/utils.go(1 hunks)core/providers/gemini/utils.go(1 hunks)core/providers/openai/openai_test.go(1 hunks)core/schemas/chatcompletions.go(2 hunks)core/schemas/utils.go(1 hunks)transports/changelog.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- core/providers/bedrock/responses.go
- core/providers/bedrock/bedrock_test.go
- core/providers/gemini/utils.go
- core/mcp.go
- core/internal/testutil/responses_stream.go
🧰 Additional context used
🧬 Code graph analysis (8)
core/providers/cohere/types.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/bedrock/types.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/openai/openai_test.go (1)
core/schemas/bifrost.go (1)
OpenAI(35-35)
core/providers/bedrock/utils.go (1)
core/schemas/utils.go (1)
SafeExtractOrderedMap(527-550)
core/providers/cohere/utils.go (1)
core/schemas/utils.go (1)
SafeExtractOrderedMap(527-550)
core/internal/testutil/utils.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/schemas/utils.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/internal/testutil/tests.go (1)
core/internal/testutil/prompt_caching.go (1)
RunPromptCachingTest(273-439)
🔇 Additional comments (13)
core/changelog.md (1)
4-4: LGTM!The changelog entry follows the existing format and appropriately documents the prompt caching fix.
transports/changelog.md (1)
9-10: LGTM!The format correction (removing the double dash) and the new changelog entry are appropriate.
core/internal/testutil/response_validation.go (1)
520-549: LGTM!The cached tokens percentage validation is well-structured with proper nil checks, edge case handling for zero prompt tokens, and informative error messages. The metrics collection provides good observability.
core/internal/testutil/tests.go (1)
52-52: LGTM! Consider adding PromptCaching to the test summary.The test is correctly added to the execution list. However, note that
printTestSummary(lines 66-90) doesn't include a corresponding entry for PromptCaching, so it won't appear in the summary output. This may be intentional since it's an OpenAI-specific test, but worth considering for consistency.core/providers/cohere/types.go (1)
8-8: LGTM!The migration of
CohereDocument.Datatoschemas.OrderedMapaligns with the broader codebase changes for deterministic JSON serialization. SinceOrderedMapis a type alias formap[string]interface{}, this maintains backward compatibility while enabling consistent key ordering during marshaling.Also applies to: 165-166
core/providers/bedrock/utils.go (1)
46-51: LGTM! Properly addresses the type assertion panic risk.The use of
schemas.SafeExtractOrderedMapcorrectly handles multiple input types (map[string]interface{},OrderedMap, and their pointer variants), preventing the runtime panic that would occur with a direct type assertion when callers pass a plainmap[string]interface{}.core/internal/testutil/account.go (1)
51-51: LGTM!The new
PromptCachingModelfield inComprehensiveTestConfigenables dedicated prompt caching test scenarios. The configuration is correctly set for the OpenAI provider.Also applies to: 531-531
core/providers/bedrock/types.go (1)
3-4: LGTM!The type change from
map[string]interface{}toschemas.OrderedMapforAdditionalModelRequestFieldsenables deterministic JSON serialization. This change is consistent with the corresponding safe extraction logic inutils.go.Also applies to: 51-51
core/providers/openai/openai_test.go (1)
28-28: The model namegpt-4.1is valid and supports prompt caching.The web search confirms that
gpt-4.1is a valid OpenAI model identifier with prompt caching enabled automatically. This test configuration is appropriate for testing prompt caching features.core/internal/testutil/utils.go (2)
108-149: Test helper tool definitions correctly migrated to OrderedMapSwitching Weather/Calculator/Time tool
Parameters.Propertiesto*schemas.OrderedMapaligns these helpers with the new schema type and deterministic JSON behavior. No functional regressions evident in these initializers.
209-215: Whitespace-only change around lion_base64 pathThe added blank line before
os.ReadFiledoes not affect behavior; no action needed.core/providers/cohere/utils.go (1)
59-62: More robust extraction of properties via SafeExtractOrderedMapUsing
schemas.SafeExtractOrderedMap(paramsMap["properties"])avoids brittle type assertions and correctly handles both plain maps and existingOrderedMapvalues when populatingresult.Properties. This looks correct and consistent with the OrderedMap-based schema.If you haven’t already, it’s worth adding/confirming a Cohere-focused test where
propertiesarrives as a plainmap[string]interface{}and as anOrderedMap, to ensure both paths are covered.core/schemas/chatcompletions.go (1)
226-303: OrderedMap.MarshalJSON logic otherwise looks sound for deterministic encodingBeyond the slice-mutation concern, the OrderedMap implementation is coherent:
nilmap encodes asnull, while non‑nil empty maps encode as{}.- Top‑level keys are copied into
norm, collecting a normalized tree ofOrderedMap/[]interface{}/primitives before encoding.- Keys are sorted via
sort.Strings, ensuring deterministic ordering for prompt‑caching.- Values are marshaled with
sonic.Marshal, so nestedOrderedMapinstances will use their ownMarshalJSONvia dynamic dispatch.Once the slice normalization fix is applied, this should give stable, reproducible JSON for nested tool parameter schemas.
If you see any tool schemas with deeply nested arrays of objects, it’s worth adding a regression test that inserts keys in differing orders (and with nested arrays) and asserts identical JSON to lock in this behavior.
Pratham-Mishra04
force-pushed
the
11-28-fix_added_ordered_map_to_fix_prompt_caching_issue_for_openai
branch
from
a2f8784 to
549c0d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
core/internal/testutil/prompt_caching.go (1)
346-346: Fix incorrect comment: "query 3" should be "query 2".The comment says "query 3" but the validation is for
i == 1(the second query). This was flagged in a past review.- // Create expectations - only add cached tokens validation for query 3 + // Create expectations - only add cached tokens validation for query 2 (second request)core/schemas/chatcompletions.go (1)
218-226: Type change from*map[string]interface{}to*OrderedMapis a breaking API change.While the underlying type is the same (
OrderedMap = map[string]interface{}), Go treats these as distinct types for pointer assignment. Code constructingToolFunctionParameterswithProperties: &map[string]interface{}{...}will fail to compile.The PR description states "Breaking changes: No" but this is technically incorrect. Consider either:
- Documenting this as a breaking change in the changelog
- Providing a helper function like
NewOrderedMapFromMap(m map[string]interface{}) *OrderedMap
🧹 Nitpick comments (1)
core/internal/testutil/test_retry_framework.go (1)
2252-2266: Break statement may skip empty-stream detection when timeout is found first.If a timeout error appears earlier in
allErrors, the loop breaks and theisEmptyStreamcheck is never performed. If both conditions can co-exist and you want to detect either, consider continuing the loop or separating the checks:for _, errMsg := range allErrors { lowerErr := strings.ToLower(errMsg) if strings.Contains(lowerErr, "timeout") { isTimeout = true - break + } - if strings.Contains(lowerErr, "no data") || strings.Contains(lowerErr, "without receiving") { + if !isEmptyStream && (strings.Contains(lowerErr, "no data") || strings.Contains(lowerErr, "without receiving")) { isEmptyStream = true - break } + if isTimeout && isEmptyStream { + break + } }Alternatively, if detecting either one is sufficient for the retry behavior, the current logic works but consider making the intent explicit with a comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
core/changelog.md(1 hunks)core/internal/testutil/account.go(3 hunks)core/internal/testutil/prompt_caching.go(1 hunks)core/internal/testutil/response_validation.go(1 hunks)core/internal/testutil/responses_stream.go(3 hunks)core/internal/testutil/test_retry_framework.go(1 hunks)core/internal/testutil/tests.go(2 hunks)core/internal/testutil/tool_calls_streaming.go(2 hunks)core/internal/testutil/utils.go(4 hunks)core/mcp.go(1 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/bedrock/bedrock_test.go(5 hunks)core/providers/bedrock/responses.go(5 hunks)core/providers/bedrock/types.go(2 hunks)core/providers/bedrock/utils.go(1 hunks)core/providers/cohere/types.go(2 hunks)core/providers/cohere/utils.go(1 hunks)core/providers/gemini/utils.go(1 hunks)core/providers/openai/openai_test.go(1 hunks)core/schemas/chatcompletions.go(2 hunks)core/schemas/utils.go(1 hunks)plugins/semanticcache/plugin_edge_cases_test.go(2 hunks)transports/changelog.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/providers/azure/azure_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- core/internal/testutil/tests.go
- core/mcp.go
- core/providers/bedrock/utils.go
- core/providers/bedrock/types.go
- core/providers/openai/openai_test.go
- core/providers/cohere/utils.go
- core/providers/cohere/types.go
- core/internal/testutil/utils.go
- core/schemas/utils.go
- core/providers/gemini/utils.go
- transports/changelog.md
- core/internal/testutil/account.go
🧰 Additional context used
🧬 Code graph analysis (4)
core/providers/bedrock/responses.go (2)
core/schemas/utils.go (1)
SafeExtractOrderedMap(527-550)core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/internal/testutil/responses_stream.go (4)
core/internal/testutil/test_retry_framework.go (2)
ResponsesStreamValidationResult(2129-2135)WithResponsesStreamValidationRetry(2139-2325)core/schemas/responses.go (2)
ResponsesStreamResponseType(1351-1351)BifrostResponsesStreamResponse(1421-1459)core/internal/testutil/validation_presets.go (2)
GetExpectationsForScenario(208-293)ModifyExpectationsForProvider(300-347)core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/bedrock/bedrock_test.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
plugins/semanticcache/plugin_edge_cases_test.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
🔇 Additional comments (27)
core/changelog.md (1)
4-6: LGTM!The changelog entries accurately document the key changes: the prompt caching fix, plugin versioning feature, and the breaking change to
ToolFunctionParameters.Propertiestype.core/internal/testutil/test_retry_framework.go (1)
2278-2293: Delay logic looks correct.The shorter delay for timeout/empty-stream scenarios with a 500ms minimum, falling back to exponential backoff for other cases, is a reasonable retry strategy for transient streaming issues.
core/internal/testutil/response_validation.go (1)
520-549: LGTM!The cached tokens percentage validation correctly:
- Guards against nil
UsageandPromptTokensDetails- Prevents division by zero
- Compares fractions (0.90) rather than percentages (90)
- Includes detailed metrics and error messages with actual token counts
core/providers/bedrock/bedrock_test.go (2)
19-24: LGTM!The test variable correctly uses
schemas.OrderedMaptype, aligning with the breaking change toToolFunctionParameters.Properties.
1597-1603: LGTM!The conversion from
OrderedMaptomap[string]interface{}for assertion comparison is correct sinceschemas.OrderedMapis a type alias formap[string]interface{}and this ensures the test assertions remain compatible.core/internal/testutil/prompt_caching.go (3)
34-267: LGTM!The tool definitions correctly use
schemas.OrderedMapfor thePropertiesfield, aligning with the breaking change toToolFunctionParameters.Properties. The tools are well-structured with appropriate types, descriptions, and required fields.
273-283: Provider guard looks correct.The duplicate provider check from the past review has been resolved - there's now a single provider check for OpenAI followed by the SimpleChat capability check.
413-428: LGTM!The cached tokens validation logic correctly:
- Validates on the second query (i == 1) where caching is expected
- Calculates percentage correctly as a fraction
- Uses 90% (0.90) threshold matching the expectation set in line 355
- Provides descriptive log messages with actual values
plugins/semanticcache/plugin_edge_cases_test.go (2)
127-132: LGTM! Correct migration to OrderedMap type.The test correctly uses
&schemas.OrderedMap{...}for tool function parameter properties, aligning with the type change inToolFunctionParameters.Properties.
164-169: LGTM! Consistent with the OrderedMap type migration.core/providers/bedrock/responses.go (5)
308-310: LGTM! Correct use of SafeExtractOrderedMap for property extraction.Using
SafeExtractOrderedMapcorrectly handles multiple input types (map[string]interface{},*map[string]interface{},OrderedMap,*OrderedMap), ensuring robust property extraction.
351-356: LGTM! Explicit conversion for ExtraParams.Converting
OrderedMaptomap[string]interface{}forExtraParamsis appropriate sinceExtraParamsuses the generic map type.
455-457: LGTM! Addresses the previous type assertion issue.Using
SafeExtractOrderedMapcorrectly handles bothschemas.OrderedMapandmap[string]interface{}values, resolving the previously flagged issue where plain maps were silently dropped.
579-580: LGTM! Correct OrderedMap initialization for minimal tool definitions.
596-598: LGTM! Consistent with other OrderedMap initializations.core/schemas/chatcompletions.go (2)
247-252: LGTM! Slice mutation issue has been correctly fixed.The implementation now creates a new slice (
out := make([]interface{}, len(x))) rather than mutating the input slice in-place, honoring the "normalized copy" contract stated in the comment at line 265.
260-304: LGTM! Well-implemented deterministic JSON marshaling.The
MarshalJSONimplementation correctly:
- Handles nil case (line 261-263)
- Creates a normalized copy to avoid mutating the original (line 266-269)
- Sorts keys alphabetically for deterministic output (line 276)
- Uses
sonic.Marshalfor proper JSON escaping of keys and valuesThis will ensure consistent JSON serialization for prompt caching.
core/internal/testutil/tool_calls_streaming.go (5)
382-404: LGTM! Well-structured validation-driven retry pattern.The refactor correctly uses
WithResponsesStreamValidationRetrywith:
- Proper timeout handling (200 seconds)
- Clean separation between stream reading and validation
- Appropriate use of
goto streamCompleteto break out of the select loop
406-411: Nil response returns validation failure instead of crashing.Good defensive programming - returning a validation result with an error message rather than calling
t.Fatalallows the retry framework to handle the failure appropriately.
662-669: Timeout handling correctly reports partial data state.The timeout case properly sets
ReceivedData: responseCount > 0to help the retry framework decide whether to retry.
693-706: Validation logic correctly checks all required tool call fields.The validation ensures each tool call has ID, Name, and Arguments before marking as passed.
715-723: Minor: Slice append result used correctly inline.The pattern
append(validationResult.Errors, validationResult.StreamErrors...)returns a new slice that's immediately passed tostrings.Join, which is correct usage.core/internal/testutil/responses_stream.go (5)
66-101: LGTM! Proper empty stream handling with retry support.The validation callback correctly handles the case where the stream closes without receiving any data (lines 93-99), returning
ReceivedData: falseto signal a retryable condition.
287-296: LGTM! Correct migration to OrderedMap for tool properties.The tool definition correctly uses
&schemas.OrderedMap{...}forProperties, consistent with the type change inToolFunctionParameters.
585-603: LGTM! Comprehensive lifecycle event tracking.The refactored test tracks all required streaming lifecycle events with their sequence numbers for proper ordering validation.
756-778: LGTM! Thorough event ordering validation.The validation correctly checks that streaming events occur in the expected order:
output_item.added→content_part.added→output_text.deltaoutput_text.done→content_part.done→output_item.done
805-813: LGTM! Consistent error reporting pattern.The error handling follows the same pattern as other tests, consolidating validation and stream errors before failing the test.
Pratham-Mishra04
force-pushed
the
11-28-fix_added_ordered_map_to_fix_prompt_caching_issue_for_openai
branch
from
549c0d3 to
3f7c743
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/providers/openai/openai_test.go (1)
24-39: Test configuration updates look good.The additions align with the PR objectives:
PromptCachingModel: "gpt-4.1"- Valid model for prompt caching tests (as per learnings)ReasoningModel: "o1"- Valid OpenAI reasoning model identifierBased on learnings, "gpt-4.1" is a valid OpenAI model released in April 2025 supporting prompt caching.
🧹 Nitpick comments (8)
core/changelog.md (1)
4-6: Minor formatting inconsistency with breaking change notation.Line 6 uses
[BREAKING CHANGE]:while line 1 uses**breaking change**:. Consider using consistent formatting:-- [BREAKING CHANGE]: ToolFunctionParameters.Properties is now an *OrderedMap instead of *map[string]interface{} +- **BREAKING CHANGE**: ToolFunctionParameters.Properties is now an *OrderedMap instead of *map[string]interface{}core/internal/testutil/chat_completion_stream.go (1)
343-354: Minor redundancy in post-validation checks.The checks at lines 349-354 duplicate the validation logic already present in the validation callback (lines 322-327). If
validationResult.Passedisfalse, these conditions would already be captured invalidationResult.Errors.However, this is acceptable for test clarity as it provides explicit fatal messages for specific failure modes.
If you want to reduce redundancy:
// Check validation result if !validationResult.Passed { allErrors := append(validationResult.Errors, validationResult.StreamErrors...) t.Fatalf("❌ Chat completion stream with tools validation failed after retries: %s", strings.Join(allErrors, "; ")) } - - if validationResult.ResponseCount == 0 { - t.Fatalf("❌ Should receive at least one streaming response") - } - if !validationResult.ToolCallDetected { - t.Fatalf("❌ Should detect tool calls in streaming response (received %d chunks but no tool calls)", validationResult.ResponseCount) - } t.Logf("✅ Streaming with tools test completed successfully")core/internal/testutil/test_retry_framework.go (1)
2337-2485: Consider extracting common retry logic to reduce duplication.The
WithChatStreamValidationRetryfunction closely mirrorsWithResponsesStreamValidationRetryandWithSpeechStreamValidationRetry. The core retry loop structure, error handling, timeout detection, and backoff calculation are nearly identical across these functions.Consider extracting a generic helper for stream validation retry to reduce maintenance burden. For example:
type StreamValidationResult interface { IsPassed() bool GetErrors() []string GetStreamErrors() []string } func withStreamValidationRetryGeneric[T StreamValidationResult]( t *testing.T, config TestRetryConfig, context TestRetryContext, streamName string, operation func() (chan *schemas.BifrostStream, *schemas.BifrostError), validateStream func(chan *schemas.BifrostStream) T, ) TThis is optional given the code is functional and the duplication is in test utilities.
core/internal/testutil/tool_calls_streaming.go (1)
392-719: Consider refactoring the validator function for better maintainability.The validator function is 327 lines long with significant code duplication across event handlers. The repeated patterns for extracting and logging callID, name, itemID, and arguments make the code harder to maintain.
Consider extracting helper functions:
- A function to extract and log tool call metadata from stream events
- A function to build the debug logging output for chunk fields
- Separate handler functions for each event type
Example structure:
// Helper to extract tool call metadata type toolCallMetadata struct { CallID *string Name *string ItemID *string Arguments *string } func extractToolCallMetadata(streamResp *schemas.BifrostResponsesStreamResponse) toolCallMetadata { // Common extraction logic } // Separate handler functions func handleFunctionCallArgumentsDelta(streamResp *schemas.BifrostResponsesStreamResponse, accumulator *StreamingToolCallAccumulator, t *testing.T) { metadata := extractToolCallMetadata(streamResp) logChunkFields(t, "function_call_arguments.delta", metadata) accumulator.AccumulateResponsesToolCall(metadata.CallID, metadata.Name, metadata.Arguments, metadata.ItemID) }This would reduce duplication and make each event handler easier to understand.
core/internal/testutil/account.go (1)
47-64: PromptCachingModel wiring looks consistent; consider guarding empty configs.Adding
PromptCachingModeltoComprehensiveTestConfigand setting it for the OpenAI entry ("gpt-4.1") cleanly separates cache-capable models fromChatModel. IfComprehensiveTestConfigis ever constructed outsideAllProviderConfigs, consider falling back toChatModelor skipping prompt-caching tests whenPromptCachingModelis empty to avoid sending requests with an empty model string.Also applies to: 527-563
core/schemas/chatcompletions.go (1)
216-224: OrderedMap + custom MarshalJSON provide deterministic, non-mutating encoding.The switch to
*OrderedMapforToolFunctionParameters.Propertiesand the recursivenormalizeValueToOrderedMap/MarshalJSONimplementation correctly avoid mutating caller data while giving stable, sorted-key JSON (including nested maps via the named type). You might optionally add a brief doc comment onOrderedMapexplaining the deterministic-serialization contract for future contributors.Also applies to: 226-304
core/internal/testutil/prompt_caching.go (1)
273-433: PromptCaching test flow is sound; consider model fallback and metadata tweak.The gating on OpenAI + SimpleChat, sequential two-query flow, and cached-token checks on the second query look correct and should reliably exercise prompt caching. Two small improvements:
- If
testConfig.PromptCachingModelis empty, consider falling back toChatModelor skipping the test with a clear log instead of issuing a request with an empty model.- In
retryContext.TestMetadata, you currently logmodel: testConfig.ChatModelwhile the request usesPromptCachingModel; using the caching model there would make diagnostics clearer.core/internal/testutil/responses_stream.go (1)
64-252: ResponsesStream validation + retry look robust; expectations block could be simplified.The
WithResponsesStreamValidationRetryclosure handles streaming carefully (timeouts, nil checks, chunk caps, event/sequence tracking) and combines structural + content-level checks into a singleResponsesStreamValidationResult, which is ideal for flaky providers. The smallexpectations := GetExpectationsForScenario(...); expectations = ModifyExpectationsForProvider(...)block is currently unused by the subsequent validation functions; if these helpers have no side effects, you could drop that block or explicitly wire expectations into validation to reduce dead-local code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
core/changelog.md(1 hunks)core/internal/testutil/account.go(3 hunks)core/internal/testutil/chat_completion_stream.go(1 hunks)core/internal/testutil/prompt_caching.go(1 hunks)core/internal/testutil/reasoning.go(1 hunks)core/internal/testutil/response_validation.go(1 hunks)core/internal/testutil/responses_stream.go(5 hunks)core/internal/testutil/test_retry_framework.go(2 hunks)core/internal/testutil/tests.go(2 hunks)core/internal/testutil/tool_calls_streaming.go(4 hunks)core/internal/testutil/utils.go(4 hunks)core/mcp.go(1 hunks)core/providers/azure/azure_test.go(1 hunks)core/providers/bedrock/bedrock_test.go(5 hunks)core/providers/bedrock/responses.go(5 hunks)core/providers/bedrock/types.go(2 hunks)core/providers/bedrock/utils.go(1 hunks)core/providers/cohere/types.go(2 hunks)core/providers/cohere/utils.go(1 hunks)core/providers/gemini/utils.go(1 hunks)core/providers/openai/openai_test.go(2 hunks)core/schemas/chatcompletions.go(2 hunks)core/schemas/utils.go(1 hunks)plugins/semanticcache/plugin_edge_cases_test.go(2 hunks)transports/changelog.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- core/internal/testutil/tests.go
- core/internal/testutil/utils.go
- core/providers/bedrock/utils.go
- core/providers/azure/azure_test.go
- core/providers/cohere/types.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T14:15:16.554Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 952
File: core/providers/openai/openai_test.go:28-28
Timestamp: 2025-11-28T14:15:16.554Z
Learning: "gpt-4.1" is a valid OpenAI model identifier released in April 2025, supporting prompt caching and featuring extended context windows up to 1M tokens.
Applied to files:
core/providers/openai/openai_test.go
🧬 Code graph analysis (13)
core/mcp.go (2)
core/schemas/chatcompletions.go (5)
OrderedMap(226-226)ChatTool(202-206)ChatToolTypeFunction(197-197)ChatToolFunction(209-214)ToolFunctionParameters(217-224)core/schemas/utils.go (1)
Ptr(16-18)
core/providers/gemini/utils.go (2)
core/providers/gemini/types.go (1)
Schema(724-775)core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/bedrock/types.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/internal/testutil/reasoning.go (1)
core/schemas/utils.go (1)
Ptr(16-18)
core/internal/testutil/tool_calls_streaming.go (2)
core/internal/testutil/test_retry_framework.go (2)
WithResponsesStreamValidationRetry(2139-2325)ResponsesStreamValidationResult(2129-2135)core/schemas/responses.go (5)
BifrostResponsesStreamResponse(1421-1459)ResponsesStreamResponseTypeFunctionCallArgumentsDelta(1375-1375)ResponsesStreamResponseTypeOutputItemAdded(1363-1363)ResponsesMessageTypeFunctionCall(291-291)ResponsesStreamResponseTypeFunctionCallArgumentsDone(1376-1376)
core/providers/openai/openai_test.go (1)
core/schemas/bifrost.go (1)
OpenAI(35-35)
core/schemas/chatcompletions.go (1)
core/providers/gemini/types.go (1)
Type(778-778)
core/providers/cohere/utils.go (1)
core/schemas/utils.go (1)
SafeExtractOrderedMap(527-550)
core/providers/bedrock/responses.go (2)
core/schemas/utils.go (1)
SafeExtractOrderedMap(527-550)core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/providers/bedrock/bedrock_test.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/schemas/utils.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
plugins/semanticcache/plugin_edge_cases_test.go (1)
core/schemas/chatcompletions.go (1)
OrderedMap(226-226)
core/internal/testutil/test_retry_framework.go (2)
core/schemas/bifrost.go (1)
BifrostStream(318-325)core/internal/testutil/utils.go (1)
GetErrorMessage(606-639)
🪛 markdownlint-cli2 (0.18.1)
core/changelog.md
6-6: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🔇 Additional comments (27)
transports/changelog.md (1)
9-10: LGTM!The changelog entries are correctly formatted and follow the existing conventions.
core/internal/testutil/reasoning.go (1)
46-48: LGTM!The reasoning parameters are properly configured. The commented-out
Summaryfield is documented with its intended purpose, which is acceptable for test configuration that may be re-enabled.core/schemas/utils.go (1)
527-550: LGTM!The
SafeExtractOrderedMapfunction correctly handles all expected input types and now returnsOrderedMap{}consistently for all failure paths, addressing the previous review feedback.core/internal/testutil/response_validation.go (1)
520-549: LGTM!The cached tokens percentage validation is well-implemented:
- Properly guards with nil checks for
UsageandPromptTokensDetails- Handles the edge case where
PromptTokensis 0- Uses explicit
float64casts to avoid integer division- Provides descriptive error messages with actual token counts
core/internal/testutil/chat_completion_stream.go (1)
256-341: LGTM!The refactored streaming validation with
WithChatStreamValidationRetryis well-structured:
- Clean separation of stream reading and validation
- Proper timeout handling with context
- Comprehensive error collection via
ChatStreamValidationResult- Tool call detection logic correctly inspects
delta.ToolCallscore/internal/testutil/test_retry_framework.go (2)
2252-2293: Timeout/empty stream detection for faster retry is a good addition.The logic to detect timeout and empty stream conditions and use a shorter retry delay (half of base delay, minimum 500ms) is sound. This avoids unnecessary waiting for transient errors.
Minor observation: The
isTimeoutandisEmptyStreamvariables are set but both could be true simultaneously, though onlyisTimeoutis checked first in the retry reason assignment (line 2286-2290). This is fine since both conditions warrant a faster retry.
2326-2335: ChatStreamValidationResult struct is well-structured.The struct includes
ToolCallDetectedandResponseCountfields that are specific to chat streaming validation, distinguishing it from the similarResponsesStreamValidationResultandSpeechStreamValidationResulttypes.core/mcp.go (1)
810-827: OrderedMap conversion for MCP tool properties looks correct.The implementation properly:
- Only allocates when properties exist (
len > 0)- Uses
maps.Copyfor safe copying- Returns nil pointer when no properties, preserving
omitemptybehaviorThis aligns with the broader migration to
schemas.OrderedMapfor deterministic JSON serialization.core/providers/gemini/utils.go (1)
103-122: Return type update to OrderedMap is correct.Since
schemas.OrderedMapis defined astype OrderedMap map[string]interface{}, this is a type alias change that maintains compatibility. The existingmake(map[string]interface{})calls and return statements work correctly because the underlying types are identical.core/providers/bedrock/bedrock_test.go (3)
19-25: Test variable updated to use OrderedMap.The
testPropsvariable correctly usesschemas.OrderedMapto align with the updatedToolFunctionParameters.Propertiestype.
177-184: Properties field uses OrderedMap correctly.Test input now properly uses
&schemas.OrderedMap{}matching the updated type signature.
1581-1588: OrderedMap to map conversion for test assertions is pragmatic.The explicit conversion from
OrderedMaptomap[string]interface{}ensures reliable comparison with testify'sassert.Equal. WhileOrderedMapis a type alias, this defensive approach avoids potential reflection-based comparison issues with named types.core/internal/testutil/tool_calls_streaming.go (6)
6-6: LGTM!The new imports support the validation framework:
fmtfor structured error messages andtimefor timeout handling.Also applies to: 10-10
141-142: Good bug fix for itemID mapping!The
finalKeytracking ensures that when a tool call entry is migrated from a temporary key to a callID-based key, the itemID mapping correctly points to the final storage location rather than the obsolete key.Also applies to: 150-150, 178-181
399-399: Verify the 200-second timeout is intentional.A 200-second timeout for a streaming test seems excessive. Typical streaming tests complete much faster unless working with very slow models or poor network conditions.
Consider whether a shorter timeout (e.g., 30-60 seconds) would be more appropriate, or document why such a long timeout is necessary for this specific test scenario.
402-675: LGTM!The select loop correctly handles both streaming responses and timeout scenarios, with appropriate validation result returns for each case. The
goto streamCompleteis an acceptable pattern for exiting the select loop.
677-719: LGTM!The post-stream validation is comprehensive and well-structured, checking for empty streams, missing tool calls, and incomplete tool call fields before delegating to the shared validation function.
722-729: LGTM!The validation result check properly aggregates errors from all sources and fails the test with a clear message after exhausting retries.
core/internal/testutil/account.go (1)
188-197: Azure reasoning key configuration is straightforward and isolated.The additional Azure key using
AZURE_REASONING_API_KEY/AZURE_REASONING_ENDPOINTand mapping"o1"→"o1"is well-scoped and should not affect existing Azure traffic; no issues from a test-account perspective.plugins/semanticcache/plugin_edge_cases_test.go (1)
120-171: Tests correctly migrated to OrderedMap for tool properties.Switching
Propertiesto&schemas.OrderedMap{...}matches the newToolFunctionParameters.Propertiestype and keeps the test schemas structurally identical, so compilation and behavior should remain intact.core/providers/bedrock/types.go (1)
3-3: Using schemas.OrderedMap for AdditionalModelRequestFields is appropriate.Changing
AdditionalModelRequestFieldstoschemas.OrderedMapaligns Bedrock with the new ordered-JSON behavior and with the SafeExtractOrderedMap-based conversions inresponses.go, without altering the wire format.Also applies to: 45-57
core/providers/bedrock/responses.go (2)
298-323: SafeExtractOrderedMap makes tool parameter conversion robust.Using
schemas.SafeExtractOrderedMapfor"properties"cleanly supports bothmap[string]interface{}andOrderedMapinputs when buildingToolFunctionParameters, removing prior brittle type assertions without changing behavior.
346-357: AdditionalModelRequestFields round-trip and inferred tool schemas look solid.Converting
AdditionalModelRequestFields(OrderedMap) into a plainmap[string]interface{}forExtraParams["additionalModelRequestFieldPaths"]and reading it back viaSafeExtractOrderedMapgives a consistent, type-safe round-trip. The inferred tool schemas now usingProperties: &schemas.OrderedMap{}preserve the previous “empty object” semantics while aligning with the new field type.Also applies to: 454-457, 573-599
core/providers/cohere/utils.go (1)
59-62: Properties extraction now correctly handles multiple map shapes.Replacing the direct type assertion with
schemas.SafeExtractOrderedMapensurespropertiesworks whether Cohere supplies amap[string]interface{}or anOrderedMap, and avoids silent drops or panics on mismatched types.core/internal/testutil/prompt_caching.go (1)
33-266: Prompt-caching tool schemas are consistent with the new OrderedMap parameters.The 10 function tools all use
ParameterswithType: "object",Properties: &schemas.OrderedMap{...}, and sensibleRequiredlists, matching theToolFunctionParametersshape and providing realistic schemas for exercise.core/internal/testutil/responses_stream.go (2)
283-305: Tool schema and reasoning params align with OrderedMap and reasoning streaming.The responses streaming-with-tools test’s
ResponsesToolFunction.Parametersnow usesProperties: &schemas.OrderedMap{...}, which matches the new schema type, and the reasoning stream test’sEffort: Ptr("high")is a sensible choice to exercise richer reasoning behavior without affecting other scenarios.Also applies to: 441-446
589-807: Lifecycle and structural streaming validation are comprehensive and well-integrated.The lifecycle-specific validator tracks all key events (
created,in_progress,output_item.added, content parts, text deltas/done, item done, completed) and enforces a strict ordering, feeding results back viaWithResponsesStreamValidationRetry.validateResponsesStreamingStructureandvalidateResponsesStreamingResponsecleanly separate sequence-number checks from content/provider/latency validation, giving good diagnostics while still allowing retries where appropriate.Also applies to: 823-869, 878-972
Merge activity
|
akshaydeo
deleted the
11-28-fix_added_ordered_map_to_fix_prompt_caching_issue_for_openai
branch
2 participants
Summary
Fix prompt caching issues for OpenAI chat completions and update tool function parameter handling to use ordered maps for consistent serialization.
Changes
map[string]interface{}withOrderedMaptype for tool function parameters to ensure consistent JSON serializationOrderedMapthat sorts keys alphabeticallyOrderedMaptyperesponses_stream.goType of change
Affected areas
How to test
Run the test suite to verify that the changes don't break existing functionality:
Test OpenAI chat completions with tools to verify that prompt caching works correctly.
Breaking changes
Related issues
Fixes prompt caching issues for OpenAI chat completions.
Security considerations
No security implications.
Checklist