fix: address PR review comments for opencode provider

- Add missing 'tool' role handling in Anthropic messages endpoint
- Add missing 'tool' role handling in Gemini models endpoint
- Fix function name mapping in Gemini (use actual function name, not ToolCallID)
- Use deterministic tool call IDs in Gemini response (index-based)
- Remove APIBase requirement from factory fallback condition
- Add validation in factory_provider for empty opencode config

All review feedback from PR #1040 has been addressed.
This commit is contained in:
Joaquin Bravo 2026-03-04 16:15:45 +01:00
parent bce265f71e
commit ddd6956314
3 changed files with 49 additions and 26 deletions

View file

@ -309,7 +309,7 @@ func resolveProviderSelection(cfg *config.Config) (providerSelection, error) {
if sel.apiBase == "" { if sel.apiBase == "" {
sel.apiBase = "https://api.mistral.ai/v1" sel.apiBase = "https://api.mistral.ai/v1"
} }
case cfg.Providers.Opencode.APIKey != "" && cfg.Providers.Opencode.APIBase != "": case cfg.Providers.Opencode.APIKey != "":
sel.apiKey = cfg.Providers.Opencode.APIKey sel.apiKey = cfg.Providers.Opencode.APIKey
sel.apiBase = cfg.Providers.Opencode.APIBase sel.apiBase = cfg.Providers.Opencode.APIBase
sel.proxy = cfg.Providers.Opencode.Proxy sel.proxy = cfg.Providers.Opencode.Proxy

View file

@ -170,6 +170,9 @@ func CreateProviderFromConfig(cfg *config.ModelConfig) (LLMProvider, string, err
return provider, modelID, nil return provider, modelID, nil
case "opencode", "opencode-zen": case "opencode", "opencode-zen":
if cfg.APIKey == "" && cfg.APIBase == "" {
return nil, "", fmt.Errorf("api_key or api_base is required for opencode protocol")
}
apiBase := cfg.APIBase apiBase := cfg.APIBase
if apiBase == "" { if apiBase == "" {
apiBase = "https://opencode.ai/zen/v1" apiBase = "https://opencode.ai/zen/v1"

View file

@ -434,6 +434,18 @@ func (p *Provider) chatAnthropicMessages(
"role": "assistant", "role": "assistant",
"content": content, "content": content,
}) })
case "tool":
// Tool result - same format as user message with tool_use_id
anthropicMessages = append(anthropicMessages, map[string]any{
"role": "user",
"content": []map[string]any{
{
"type": "tool_result",
"tool_use_id": msg.ToolCallID,
"content": msg.Content,
},
},
})
} }
} }
@ -527,6 +539,8 @@ func (p *Provider) chatGeminiModels(
// Build contents from messages (Gemini format) // Build contents from messages (Gemini format)
var contents []map[string]any var contents []map[string]any
var systemParts []string var systemParts []string
// Track tool call ID to function name mapping for proper functionResponse
toolCallIDToName := make(map[string]string)
for _, msg := range messages { for _, msg := range messages {
switch msg.Role { switch msg.Role {
@ -539,35 +553,20 @@ func (p *Provider) chatGeminiModels(
systemParts = append(systemParts, msg.Content) systemParts = append(systemParts, msg.Content)
} }
case "user": case "user":
if msg.ToolCallID != "" { contents = append(contents, map[string]any{
// Tool result "role": "user",
contents = append(contents, map[string]any{ "parts": []map[string]any{
"role": "user", {"text": msg.Content},
"parts": []map[string]any{ },
{ })
"functionResponse": map[string]any{
"name": msg.ToolCallID,
"response": map[string]any{
"result": msg.Content,
},
},
},
},
})
} else {
contents = append(contents, map[string]any{
"role": "user",
"parts": []map[string]any{
{"text": msg.Content},
},
})
}
case "assistant": case "assistant":
parts := []map[string]any{} parts := []map[string]any{}
if msg.Content != "" { if msg.Content != "" {
parts = append(parts, map[string]any{"text": msg.Content}) parts = append(parts, map[string]any{"text": msg.Content})
} }
for _, tc := range msg.ToolCalls { for _, tc := range msg.ToolCalls {
// Track the mapping from tool call ID to function name
toolCallIDToName[tc.ID] = tc.Name
parts = append(parts, map[string]any{ parts = append(parts, map[string]any{
"functionCall": map[string]any{ "functionCall": map[string]any{
"name": tc.Name, "name": tc.Name,
@ -579,6 +578,27 @@ func (p *Provider) chatGeminiModels(
"role": "model", "role": "model",
"parts": parts, "parts": parts,
}) })
case "tool":
// Tool result - use mapped function name, not ToolCallID
funcName := toolCallIDToName[msg.ToolCallID]
if funcName == "" {
// Fallback: if no mapping found, this shouldn't happen in normal flow
// but we need to handle it gracefully
funcName = "unknown_function"
}
contents = append(contents, map[string]any{
"role": "user",
"parts": []map[string]any{
{
"functionResponse": map[string]any{
"name": funcName,
"response": map[string]any{
"result": msg.Content,
},
},
},
},
})
} }
} }
@ -708,13 +728,13 @@ func parseGeminiResponse(body []byte) (*LLMResponse, error) {
var content strings.Builder var content strings.Builder
var toolCalls []ToolCall var toolCalls []ToolCall
for _, part := range candidate.Content.Parts { for i, part := range candidate.Content.Parts {
if part.Text != "" { if part.Text != "" {
content.WriteString(part.Text) content.WriteString(part.Text)
} }
if part.FunctionCall != nil { if part.FunctionCall != nil {
toolCalls = append(toolCalls, ToolCall{ toolCalls = append(toolCalls, ToolCall{
ID: fmt.Sprintf("call_%s_%d", part.FunctionCall.Name, time.Now().UnixNano()), ID: fmt.Sprintf("call_%s_%d", part.FunctionCall.Name, i),
Name: part.FunctionCall.Name, Name: part.FunctionCall.Name,
Arguments: part.FunctionCall.Args, Arguments: part.FunctionCall.Args,
}) })