fix(openai): always emit required fields on Responses API input items#115
fix(openai): always emit required fields on Responses API input items#115clintecker merged 2 commits intomainfrom
Conversation
Closes #114. The OpenAI Responses API input array is a discriminated union keyed by the item's `type` field. Tracker modeled it as a single struct with `omitempty` on every field, which stripped required fields whenever the value happened to be an empty string: - function_call_output with an empty tool result dropped `output` (required: string | array) - function_call with empty arguments dropped `arguments` (required: string) and `name` (required: string) OpenAI's own endpoint tolerated these, so the bug lay dormant. OpenRouter validates with a strict Zod schema matching the documented spec and rejected the requests with `invalid_prompt` / `invalid_union` errors. Symptoms: random-looking failures on GLM, Qwen, Kimi (models OpenRouter routes through its /v1/responses proxy), consistent per-model on resume because the bad item is replayed from conversation history. The fix replaces omitempty-on-everything with a MarshalJSON method that emits only the fields valid for each discriminator, with required fields always present. Unmarshal tags retained for round-trip test reads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
llm/openai/translate.go (1)
89-95: Fail fast on unsupportedopenaiInput.TypevaluesThe
defaultbranch currently treats any unknown non-emptyTypeas a role-message item, which can silently produce malformed union payloads. Return an explicit error whenTypeis non-empty but not recognized.Proposed patch
import ( "encoding/json" + "fmt" "strings" @@ default: - // Role-based message (user / assistant / system / developer). + // Role-based message (user / assistant / system / developer). + if i.Type != "" { + return nil, fmt.Errorf("openai: unsupported input type %q", i.Type) + } return json.Marshal(struct { Role string `json:"role"` Content string `json:"content"` }{i.Role, i.Content}) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@llm/openai/translate.go` around lines 89 - 95, The default branch in the switch that marshals openai input items currently assumes unknown types are role-based messages and silently produces bad payloads; update that default case to fail fast: if i.Type is non-empty return an explicit error (e.g. fmt.Errorf("unsupported openai input type: %q", i.Type)) instead of json.Marshal, ensuring the enclosing function (the marshal/convert function in llm/openai/translate.go that handles i.Type, Role, Content) returns an error up the call stack; keep the existing behavior only for empty i.Type (preserve the role-based json.Marshal for the empty-type case).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@llm/openai/translate_test.go`:
- Around line 319-321: The test currently only checks for presence of
"arguments" in the fc map but must assert it's a string (empty string is
allowed) not e.g. null; replace the presence check on fc["arguments"] with a
type assertion like v, ok := fc["arguments"].(string) and call t.Error (or
t.Fatalf) if !ok, optionally keeping an additional assertion for expected empty
string value when required; update the assertion around the fc variable in
translate_test.go to ensure the "arguments" value is a string.
---
Nitpick comments:
In `@llm/openai/translate.go`:
- Around line 89-95: The default branch in the switch that marshals openai input
items currently assumes unknown types are role-based messages and silently
produces bad payloads; update that default case to fail fast: if i.Type is
non-empty return an explicit error (e.g. fmt.Errorf("unsupported openai input
type: %q", i.Type)) instead of json.Marshal, ensuring the enclosing function
(the marshal/convert function in llm/openai/translate.go that handles i.Type,
Role, Content) returns an error up the call stack; keep the existing behavior
only for empty i.Type (preserve the role-based json.Marshal for the empty-type
case).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c92a6ac-9966-44c3-81fc-d3c0a7510f20
📒 Files selected for processing (3)
CHANGELOG.mdllm/openai/translate.gollm/openai/translate_test.go
There was a problem hiding this comment.
Pull request overview
Fixes strict OpenAI Responses API request validation failures (notably via OpenRouter) by ensuring discriminated-union input items always serialize the required fields even when values are empty strings, preventing invalid_prompt / invalid_union errors when replaying conversation history.
Changes:
- Implemented
openaiInput.MarshalJSONto emit only fields valid for eachtypediscriminator and to always include required fields (noomitemptystripping). - Added regression tests covering empty tool outputs (
output: "") and empty tool-call arguments (arguments: ""). - Documented the fix and its impact in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
llm/openai/translate.go |
Adds custom JSON marshaling for openaiInput to ensure required discriminator fields are always emitted. |
llm/openai/translate_test.go |
Adds regression tests ensuring empty-string required fields are preserved in serialized request JSON. |
CHANGELOG.md |
Records the bug, symptoms (OpenRouter strict validation), and the serialization fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| switch i.Type { | ||
| case "function_call": | ||
| return json.Marshal(struct { | ||
| Type string `json:"type"` | ||
| CallID string `json:"call_id"` | ||
| Name string `json:"name"` | ||
| Arguments string `json:"arguments"` | ||
| }{i.Type, i.CallID, i.Name, i.Arguments}) | ||
| case "function_call_output": | ||
| return json.Marshal(struct { | ||
| Type string `json:"type"` | ||
| CallID string `json:"call_id"` | ||
| Output string `json:"output"` | ||
| }{i.Type, i.CallID, i.Output}) | ||
| default: | ||
| // Role-based message (user / assistant / system / developer). | ||
| return json.Marshal(struct { | ||
| Role string `json:"role"` | ||
| Content string `json:"content"` | ||
| }{i.Role, i.Content}) | ||
| } |
There was a problem hiding this comment.
MarshalJSON treats any unknown Type the same as a role-based message (it falls into default and drops the type field). Given the comment that an empty Type means role-message, it would be safer to handle case "" for role-messages and return an error for any other unexpected discriminator to avoid silently emitting an invalid item shape if Type is ever set incorrectly.
| }{i.Type, i.CallID, i.Name, i.Arguments}) | ||
| case "function_call_output": | ||
| return json.Marshal(struct { | ||
| Type string `json:"type"` | ||
| CallID string `json:"call_id"` | ||
| Output string `json:"output"` | ||
| }{i.Type, i.CallID, i.Output}) | ||
| default: | ||
| // Role-based message (user / assistant / system / developer). | ||
| return json.Marshal(struct { | ||
| Role string `json:"role"` | ||
| Content string `json:"content"` | ||
| }{i.Role, i.Content}) |
There was a problem hiding this comment.
In MarshalJSON, the anonymous struct literals are initialized using positional fields (e.g., }{i.Type, i.CallID, ...}). This is brittle if the struct definition changes and makes reviews harder; prefer keyed fields (Type: i.Type, etc.) to prevent accidental field-order bugs.
| }{i.Type, i.CallID, i.Name, i.Arguments}) | |
| case "function_call_output": | |
| return json.Marshal(struct { | |
| Type string `json:"type"` | |
| CallID string `json:"call_id"` | |
| Output string `json:"output"` | |
| }{i.Type, i.CallID, i.Output}) | |
| default: | |
| // Role-based message (user / assistant / system / developer). | |
| return json.Marshal(struct { | |
| Role string `json:"role"` | |
| Content string `json:"content"` | |
| }{i.Role, i.Content}) | |
| }{ | |
| Type: i.Type, | |
| CallID: i.CallID, | |
| Name: i.Name, | |
| Arguments: i.Arguments, | |
| }) | |
| case "function_call_output": | |
| return json.Marshal(struct { | |
| Type string `json:"type"` | |
| CallID string `json:"call_id"` | |
| Output string `json:"output"` | |
| }{ | |
| Type: i.Type, | |
| CallID: i.CallID, | |
| Output: i.Output, | |
| }) | |
| default: | |
| // Role-based message (user / assistant / system / developer). | |
| return json.Marshal(struct { | |
| Role string `json:"role"` | |
| Content string `json:"content"` | |
| }{ | |
| Role: i.Role, | |
| Content: i.Content, | |
| }) |
| input := raw["input"].([]any) | ||
| fco := input[1].(map[string]any) | ||
| if fco["type"] != "function_call_output" { |
There was a problem hiding this comment.
This test uses unchecked type assertions (raw["input"].([]any), input[1].(map[string]any)) which will panic on failure and make the test output less actionable. Prefer asserting the type with an ok check (and also validating len(input) before indexing) so failures report via t.Fatalf instead of a panic.
| input := raw["input"].([]any) | ||
| fc := input[0].(map[string]any) |
There was a problem hiding this comment.
Same as above: this test uses unchecked type assertions (raw["input"].([]any), input[0].(map[string]any)) which can panic. Add ok checks and a length assertion so test failures are reported cleanly.
| input := raw["input"].([]any) | |
| fc := input[0].(map[string]any) | |
| input, ok := raw["input"].([]any) | |
| if !ok { | |
| t.Fatalf("expected input to be []any, got %T", raw["input"]) | |
| } | |
| if len(input) == 0 { | |
| t.Fatal("expected input to contain at least one item") | |
| } | |
| fc, ok := input[0].(map[string]any) | |
| if !ok { | |
| t.Fatalf("expected first input item to be map[string]any, got %T", input[0]) | |
| } |
| req := &llm.Request{ | ||
| Model: "openai/gpt-4.1", | ||
| Messages: []llm.Message{ |
There was a problem hiding this comment.
The new tests set req.Model to "openai/gpt-4.1", while other tests in this file use bare model IDs (e.g. "gpt-4.1", "gpt-5.4"). Consider keeping the model string consistent within these unit tests unless the prefix is required for this specific code path, to avoid confusion about what translateRequest expects.
| req := &llm.Request{ | ||
| Model: "openai/gpt-4.1", | ||
| Messages: []llm.Message{ |
There was a problem hiding this comment.
Same consistency note here: Model: "openai/gpt-4.1" differs from the bare model IDs used in the other translate_test.go cases. Aligning the model string format across tests will make intent clearer.
- MarshalJSON: fail fast on unknown Type rather than silent role-message
fallback. Adding a new item shape should be a test failure, not a
wire-format bug. (Copilot + CodeRabbit)
- MarshalJSON: use keyed anonymous-struct field literals. (Copilot)
- Tests: type-check map assertions with ok + length checks so failures
report via t.Fatalf instead of panicking. (Copilot)
- Tests: assert arguments is a string (empty "" OK, null/undefined not).
(CodeRabbit)
- Tests: use bare model IDs ("gpt-4.1") consistent with sibling tests,
not the OpenRouter-prefixed form. (Copilot)
- Tests: add regression for unknown-Type error path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #114.
Diagnosis
The OpenAI Responses API input array is a discriminated union keyed by each item's
typefield:function_call_outputrequiresoutput: string | arrayfunction_callrequiresname: stringandarguments: stringroleandcontentTracker modeled this as a single Go struct with
omitemptyon every field. The serializer dropped required fields whenever the value happened to be an empty string — e.g. a tool that returned `""` produced:```json
{"type": "function_call_output", "call_id": "call_xyz"}
```
OpenAI's endpoint accepted the loose shape, so the bug was latent. OpenRouter validates with a strict Zod schema matching the published spec and rejected the requests with `invalid_prompt` / `invalid_union` errors. Symptoms matched the report: random-looking failures only on OpenRouter-proxied models (GLM, Qwen, Kimi), deterministic per-model on resume (the bad item replays from conversation history), different failure location when the model changes.
Fix
Replace `omitempty`-on-every-field with a `MarshalJSON` method that emits only the fields valid per discriminator, with required fields always present (empty strings kept, undefined never produced). Keeps the single builder struct so translation code is unchanged; retains unmarshal tags for round-trip test reads.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests