feat(openai_compat): implement strict mode compatibility for third-party providers#1683
feat(openai_compat): implement strict mode compatibility for third-party providers#1683badgerbees wants to merge 8 commits intosipeed:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Implements OpenAI Structured Outputs “strict mode” handling in the openai_compat provider by conditionally sanitizing tool definitions for non-native OpenAI endpoints, and optionally injecting function.strict based on endpoint detection and a caller-provided toggle.
Changes:
- Route
toolsthrough a newfinalizeToolshelper before JSON serialization. - Add
supportsStrictMode(currently mirroring the existing OpenAI/Azure detection logic). - Add strict-mode sanitization / injection logic with an
options["strict_mode"]override.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(tools) > 0 { | ||
| requestBody["tools"] = tools | ||
| requestBody["tools"] = p.finalizeTools(tools, options) | ||
| requestBody["tool_choice"] = "auto" | ||
| } |
yinwm
left a comment
There was a problem hiding this comment.
Code Review
Overall: ✅ Ready to merge with minor suggestions
1. Logic Issue ⚠️
strict_mode=true forced on non-native providers may still cause errors
useStrict := isNative
if hasForce {
useStrict = forceStrict
}When a user sets strict_mode=true but the provider is Ollama/Groq/DeepSeek (non-native OpenAI), the code will still send strict=true in the request, which will still result in a 400 error.
Suggestion: Either log a warning or ignore the forced setting when the provider doesn't support it:
if hasForce && forceStrict && !isNative {
// slog.Warn("strict_mode=true ignored for non-OpenAI provider", "provider", p.apiBase)
useStrict = false
}2. Potential Edge Case 🔍
The ToolFunctionDefinition struct doesn't have a Strict field. This PR assumes strict is controlled via options, not already present in the input tools.
Question: Could the tools parameter already contain a strict field from upstream? If so, the current sanitization logic won't remove it since it only rebuilds with standard fields.
3. Code Quality 👍
- ✅ Clear comments explaining the performance optimization path
- ✅ Good reuse of
supportsPromptCacheKeydetection logic - ✅ Pass-through optimization avoids unnecessary memory allocation
4. Missing Items 📋
| Item | Status |
|---|---|
| Unit tests | ❌ Missing |
Documentation for strict_mode option |
❌ Not mentioned |
Edge case: forceStrict=true + non-native provider |
5. Suggested Improvement
func (p *Provider) finalizeTools(tools []ToolDefinition, options map[string]any) any {
forceStrict, hasForce := options["strict_mode"].(bool)
isNative := supportsStrictMode(p.apiBase)
// Fast path: native OpenAI without forced setting
if isNative && !hasForce {
return tools
}
// Decide whether to use strict mode
useStrict := isNative
if hasForce {
if forceStrict && !isNative {
// Non-native providers don't support strict mode, ignore user setting
// slog.Warn("strict_mode=true ignored for non-OpenAI provider", "provider", p.apiBase)
useStrict = false
} else {
useStrict = forceStrict
}
}
// ... rest unchanged
}Conclusion
The core logic is correct and solves a real compatibility issue. Recommend merging after addressing the strict_mode=true edge case (or at least adding a warning log). Unit tests would be a nice addition.
|
I've updated the branch to address the review feedback:
The implementation is now robust against unsupported field errors while maintaining flexible control for native OpenAI endpoints. |
|
@badgerbees plz fix PR Linter / tests |
what the hell there were literally no issues with it when I ran the checks hence the workflow working the first time, I've no idea what these errors meant either + I've just checked again and there were no lint error let me take a look again since all workflow failed too not just 1 |
|
huh strange all the errors are related to |
There was a problem hiding this comment.
Pull request overview
Implements OpenAI “Strict Mode” compatibility behavior in the openai_compat provider by conditionally adding/removing the strict tool flag depending on whether the configured endpoint is native OpenAI/Azure vs a third-party OpenAI-compatible API.
Changes:
- Route tool serialization through a new
finalizeToolshelper to optionally inject/stripfunction.strict. - Add strict-mode support detection (
supportsStrictMode) based on API base hostname (mirrors prompt cache support detection). - Add unit coverage for strict tool finalization behavior across native vs non-native API bases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/providers/openai_compat/provider.go | Adds strict-mode detection and tool finalization, and updates Chat() to use it. |
| pkg/providers/openai_compat/provider_test.go | Adds tests covering strict-mode tool finalization behavior and pass-through expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| res := p.finalizeTools(tools, tt.options) | ||
|
|
||
| if tt.wantSame { | ||
| if fmt.Sprintf("%p", res) != fmt.Sprintf("%p", tools) { |
|
The previous CI failure ( I've fixed this by:
|
…ility and resolve conflicts
📝 Description
Summary:
openai_compatprovider.strict: trueflag from tool definitions for non-native OpenAI providers (Ollama, vLLM, DeepSeek, Groq, etc.).Changes:
pkg/providers/openai_compat/provider.go:finalizeToolshelper to sanitize tool definitions before wire serialization.supportsStrictModeto detect native OpenAI/Azure endpoints based on the API base URL.Chat()to utilize lazy tool sanitization.🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
N/A
📚 Technical Context (Skip for Docs)
Problem Overview:
Many "OpenAI-compatible" providers (like Ollama or Groq) strictly reject unknown fields in the tool definition JSON. Specifically, OpenAI's new
"strict": trueflag for Structured Outputs causes these providers to return400 Bad Requestor422 Unprocessable Entityerrors, crashing tool calls in PicoClaw.Solution Implementation:
apiBaseis native OpenAI.strictflag by rebuilding the tool map with only standard fields.🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Test Results
Test Results:
go test ./pkg/providers/openai_compat/...☑️ Checklist