feat: add OpenResponses 2026-04-24 API support - #245
Closed
matdev83 wants to merge 3 commits into
Closed
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 441 files, which is 341 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (113)
📒 Files selected for processing (441)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return `{ | ||
| "id": "resp_provider_1", | ||
| "object": "response", | ||
| "created_at": ` + itoa64(created) + `, |
| func openAIResponsesCompatSSE(created int64) string { | ||
| txt := `provider-mode-ok` | ||
| return "event: response.created\n" + | ||
| "data: " + `{"type":"response.created","sequence_number":1,"response":{"id":"resp_provider_stream","object":"response","created_at":` + itoa64(created) + `,"status":"in_progress","model":"gpt-4o-mini","output":[]}}` + "\n\n" + |
| "event: response.output_item.done\n" + | ||
| "data: " + `{"type":"response.output_item.done","sequence_number":7,"output_index":0,"item":{"type":"message","id":"msg_provider_1","status":"completed","role":"assistant","content":[{"type":"output_text","text":"` + txt + `"}]}}` + "\n\n" + | ||
| "event: response.completed\n" + | ||
| "data: " + `{"type":"response.completed","sequence_number":8,"response":{"id":"resp_provider_stream","object":"response","created_at":` + itoa64(created) + `,"status":"completed","model":"gpt-4o-mini","output":[{"type":"message","id":"msg_provider_1","status":"completed","role":"assistant","content":[{"type":"output_text","text":"` + txt + `"}]}]}}` + "\n\n" + |
| return `{ | ||
| "id": "resp_harness_1", | ||
| "object": "response", | ||
| "created_at": ` + itoa64(created) + `, |
| "id": "msg_harness_out", | ||
| "status": "completed", | ||
| "role": "assistant", | ||
| "content": [{"type": "output_text", "text": ` + string(txt) + `}] |
| func openResponsesRichSSE(text string, created int64) string { | ||
| txt, _ := json.Marshal(text) | ||
| return "event: response.created\n" + | ||
| "data: " + `{"type":"response.created","sequence_number":1,"response":{"id":"resp_harness_stream","object":"response","created_at":` + itoa64(created) + `,"status":"in_progress","model":"gpt-4o-mini","output":[]}}` + "\n\n" + |
| "event: response.output_item.done\n" + | ||
| "data: " + `{"type":"response.output_item.done","sequence_number":7,"output_index":0,"item":{"type":"message","id":"msg_harness_1","status":"completed","role":"assistant","content":[{"type":"output_text","text":` + string(txt) + `}]}}` + "\n\n" + | ||
| "event: response.completed\n" + | ||
| "data: " + `{"type":"response.completed","sequence_number":8,"response":{"id":"resp_harness_stream","object":"response","created_at":` + itoa64(created) + `,"status":"completed","model":"gpt-4o-mini","output":[{"type":"message","id":"msg_harness_1","status":"completed","role":"assistant","content":[{"type":"output_text","text":` + string(txt) + `}]}]}}` + "\n\n" + |
| } | ||
|
|
||
| func deriveRecordRequirements(rec ContinuationRecord) lipapi.ProtocolRequirements { | ||
| items := make([]lipapi.Item, 0, len(rec.InputItems)+len(rec.OutputItems)) |
| } | ||
|
|
||
| func deriveRecordRequirements(rec ContinuationRecord) lipapi.ProtocolRequirements { | ||
| items := make([]lipapi.Item, 0, len(rec.InputItems)+len(rec.OutputItems)) |
matdev83
commented
Aug 2, 2026
matdev83
left a comment
matdev83
left a comment
Owner
Author
There was a problem hiding this comment.
Merge-blocking review findings
I found several compatibility/correctness issues that should be resolved before merge:
- The OpenRouter and NVIDIA cells in the claimed 5×9 matrix do not instantiate those connectors; they instantiate the generic
custom-openai-responses-compatiblebackend under a different ID. The matrix therefore does not exercise connector-specific mapping, headers, capabilities, errors, credentials, or inventory. - The production OpenResponses codec omits standard file/video content variants and coerces valid prefixed content extensions into text, losing their discriminator and structured payload.
- Standard request controls are incomplete: create rejects
instructions, while compact explicitly accepts and ignoresinstructions/prompt_cache_key. The pinned schema contains additional standard controls not represented byWireResponseParam; passing the 17 official scenarios does not prove full 2026-04-24 request compatibility. - The generic backend advertises documents/annotations by default although its request encoder rejects those content kinds, so routing admission can accept a candidate that fails only inside the backend adapter.
- Continuation item-count arithmetic uses unchecked
len(a)+len(b)both before bounds checks and as amakecapacity. This can overflow and bypass the intended materialization limits or panic on a malformed durable record.
Separately, the submitted head is not CI-green: the main CI workflow fails the Repo hygiene job because the new release files are not fully approved in .release-files. Backend plugin cross-platform is still in progress. Please make the release manifest complete and rerun the required gates after addressing the findings.
I did not rerun the repository locally in this environment; this review is based on the submitted head, targeted source inspection, the merged SDD, and GitHub Actions evidence.
| _ = d.Close() | ||
| tb.Fatalf("harness: provider-mode config: %v", err) | ||
| } | ||
| be, err := openaicompat.BuildCompatible(backendID, "custom-openai-responses-compatible", n, primaryOrigin.Client(), openaicompat.FlavorResponses, providerModeTransportCaps()) |
Owner
Author
There was a problem hiding this comment.
[P1] Exercise the actual OpenRouter/NVIDIA connectors in these matrix cells. backendID is only used as an identity here; the constructed backend is always the generic custom-openai-responses-compatible implementation. Consequently every row scenario labeled openrouter or nvidia bypasses the connector-specific request mapping, attribution/auth headers, capability declarations, error handling, inventory, and credential behavior. This cannot serve as evidence for the required OpenResponses→OpenRouter/NVIDIA cells. Please instantiate the real connectors (through the plugin host if needed), or classify these as separate generic provider-mode cells and add actual connector conformance coverage.
| "output_text": true, | ||
| "text": true, | ||
| "input_image": true, | ||
| "refusal": true, |
Owner
Author
There was a problem hiding this comment.
[P1] Implement the pinned standard file/video content variants instead of rejecting them. The checked-in 2026-04-24 schema includes input_file and input_video, the wire type already has file_url/video_url, and the canonical model has ContentPartFileRef/ContentPartVideoRef; however this discriminator table admits only text/image/refusal. Such requests are therefore rejected as unknown, despite the PR claiming the pinned API and document/video compatibility. The same codec later stringifies valid prefixed custom content parts, which loses extension identity and structured data. Add typed lossless mappings (and a bounded opaque content representation where required) with direct-emulator and full-path tests; do not coerce structured variants to text.
| // separately by compactUnsupportedControls. Metadata is deliberately excluded: | ||
| // it maps to Call.Session.Metadata end-to-end. | ||
| var createUnsupportedControls = []unsupportedControl{ | ||
| {"instructions", func(p *proto.WireResponseParam) bool { return p.Instructions != nil }}, |
Owner
Author
There was a problem hiding this comment.
[P1] Preserve standard OpenResponses request controls instead of shrinking the advertised API. instructions is a standard 2026-04-24 field and the approved design explicitly includes canonical Instructions/request controls, but every non-null value is rejected here. Other standard schema fields (include, presence/frequency penalties, stream_options, top_logprobs, etc.) are absent from WireResponseParam and become unknown-field failures. On compact, the code explicitly accepts instructions and prompt_cache_key while ignoring them, which is silent semantic loss. Please map the supported controls into canonical fields/residual requirements and forward/project them safely; any intentionally unsupported field must be consistently rejected and documented, not accepted-and-dropped.
| lipapi.CapabilityStreaming, | ||
| lipapi.CapabilityTools, | ||
| lipapi.CapabilityVision, | ||
| lipapi.CapabilityDocuments, |
Owner
Author
There was a problem hiding this comment.
[P1] Do not advertise capabilities that the request mapper cannot encode. The default profile claims CapabilityDocuments (and later CapabilityAnnotations), while representableContentPartKind accepts only text, image, and refusal. A file or annotation request can therefore pass routing capability admission and fail only when this backend builds the request. That breaks honest capability negotiation and can distort failover selection. Either implement the corresponding wire mappings first or remove these capabilities from the defaults; add an admission test proving every advertised default capability reaches the reference backend without ErrUnrepresentable.
| if total > bounds.MaxMaterializedBytes { | ||
| return MaterializedTrajectory{}, ErrMaterializedSizeExceeded | ||
| } | ||
| recordItems := len(rec.InputItems) + len(rec.OutputItems) |
Owner
Author
There was a problem hiding this comment.
[P1] Use checked addition for record item counts. len(rec.InputItems)+len(rec.OutputItems) can overflow int before this bounds comparison, making recordItems negative and bypassing MaxMaterializedItems. The same unchecked sum is later used as the capacity in deriveRecordRequirements, where it can panic. Durable continuation records are an input boundary, so validate each length and perform a checked sum (for example if len(input) > max-len(output)) before both accounting and allocation; add a malformed-record regression test.
Owner
Author
|
Superseded by #246. The replacement PR contains the same verified feature plus the Advanced Security remediations, and uses a single clean commit so CI's per-commit release-manifest check can pass without force-rewriting this published branch. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Main integration
Verification
Platform note