fix(api): consolidate OpenAI API honesty and multimodal contracts - #565
fix(api): consolidate OpenAI API honesty and multimodal contracts#565seonghobae wants to merge 15 commits into
Conversation
… effort as omit
Buyer SDKs often send stream_options with only false flags, tool_choice:{},
and reasoning_effort:"" as optional defaults. Accept them as omit no-ops
while still fail-closing true stream_options flags without stream=true and
non-empty reasoning_effort. Tip honesty substrate re-ship; 849 unit pass.
SDK clients send user:null as an optional default. Treat null as omit on chat Completions, legacy Completions, Responses, embeddings, and batch embeddings. Empty/whitespace/non-string user still fail closed with invalid_user. Local full unit: 857 passed.
…l/response_format/endpoint as omit SDK clients and stringified optional controls may send empty or whitespace strings. Treat as omit no-ops on embeddings encoding_format, chat/Responses tool_choice and function_call, response_format, and batch embeddings endpoint. Non-empty unsupported values still fail closed. Local full unit: 865 passed.
SDK stringified empty controls for reasoning, Responses text, and include are treat-as-omit on chat Completions, legacy Completions, and Responses. Non-empty unsupported values still fail closed with named errors. Local full unit: 872 passed.
… as omit Legacy Completions has no tools surface. Treat SDK defaults tool_choice none/auto/empty-string/empty-object and function_call none/auto/empty-string as omit no-ops (parity with chat). Non-default controls and non-empty tools still fail closed with a chat migration path. Local full unit: 877 passed.
SDK clients may send top_logprobs:0 (no top alternatives). Treat 0 and null as omit on chat Completions and legacy Completions. Non-zero values still fail closed with invalid_top_logprobs. Local full unit: 881 passed.
…format float
Incidental whitespace around honest no-op values (" auto ", " float ")
is stripped before validation so SDK-padded strings match. Unsupported
values (flex, base64) still fail closed after strip. Local full unit:
886 passed.
…ll named honesty Treat empty-string response_format/prediction/reasoning_effort as omit on legacy Completions. Accept audio/web_search_options keys with null/empty as omit and non-empty as named invalid_* (not unknown_fields). Local full unit: 894 passed.
…ns modalities text no-op Whitespace-padded none/auto on tool_choice and function_call are omit no-ops on chat, Completions, and Responses. Completions modalities ["text"] is an honest text-only no-op; non-text modalities still fail closed. Local full unit: 901 passed.
…l names Treat empty/whitespace prediction as omit on chat and Responses. Strip modalities array items and empty-string modalities for text-only match. Strip model names on Completions/chat, Responses, and embeddings so SDK-padded pool ids resolve. Local full unit: 907 passed.
…nd as omit OpenAI reasoning_effort none disables extra reasoning — honest omit no-op on chat and Completions. Empty/whitespace store, stream, and background strings are treat-as-omit across chat Completions, Completions, and Responses store. Local full unit: 911 passed.
|
Important Review skippedToo many files! This PR contains 133 files, which is 33 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (133)
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 |
Pull request was converted to draft
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head merge blocker: request-scoped sampling and token-budget controls are implemented by mutating the shared orchestrator.client (max_output_tokens, default_temperature, default_top_p, default_presence_penalty, default_frequency_penalty) and restoring them in finally. The server is threaded, so two overlapping requests can observe or restore each other's values. A request with no override can also inherit another request's temporary override. This is cross-request execution/cost isolation failure even though single-request tests pass.
Required test-first repair:
- Add a deterministic concurrent HTTP regression using a barrier/event-controlled client and two simultaneous requests with different
temperature,top_p, penalties, and token limits; assert each provider invocation receives only its own values, a no-override request retains defaults, and exception/cancellation paths do not leak state. - Remove request-time mutation of shared
ModelClientfields. Thread an immutable request-scoped generation-options object/explicit kwargs through coordinator → orchestrator →_invoke→ client (and stream path), or use an equivalently scoped design that remains correct across worker/thread hand-offs. - Preserve role-specific policy defaults without allowing HTTP request overrides to alter another request or later orchestration steps.
- Re-run the exact-head full suite, security/SAST gates, and independent review. Keep this PR Draft until the concurrency proof is green.
Pull request was converted to draft
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head follow-up: the shared-client mutation blocker is repaired on 9cc1460f1eaead36e28097dd6c967d05f3135271.
Verified in the current diff:
- immutable
GenerationOptionsare bound through aContextVar-backedModelClient.request_options()context; - chat and streaming payloads resolve request options without changing shared client defaults;
- the HTTP handler binds max token, temperature, top-p, presence, and frequency controls for only the current request;
tests/test_request_sampling_concurrency.pyuses overlapping real HTTP requests and a controlled provider failure to prove default/override isolation and cleanup;- exact-head Tests, Security, Security Scan, Fuzz, SAST Semgrep, coverage-evidence, and OpenCode checks are terminal-success.
No unresolved review thread remains. CodeRabbit skipped the aggregate 130-file head due its 100-file limit, so this comment is not an independent approval and does not satisfy the ruleset. The remaining merge gate is a qualifying current-head non-author approval; branch protection must remain intact.
|
@opencode-agent @cwl-noema-review Please perform an independent current-head review of |
|
@opencode-agent Perform an independent exact-head review of |
…_tool_calls as omit (#571) OpenAI SDKs often send truncation=auto on /v1/responses. Without previous_response_id or conversation this gateway has no multi-turn context, so auto|disabled are honest omit-equivalent no-ops; other values stay fail-closed with invalid_truncation (not unknown_fields). Also treat empty/whitespace-string parallel_tool_calls as omit no-ops on chat, Completions, and Responses (matching store/stream empty-string SDK controls), while true without tools remains fail-closed. HTTP honesty coverage for the new contracts; cumulative tip substrate from the reasoning_effort/store/stream head.
![contextual-orchestrator-maintainer[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
#572) * fix(api): treat empty-string optional numeric/boolean controls as omit SDK clients may stringify omitted optionals as empty strings. Treat empty/whitespace temperature, top_p, max_tokens, max_completion_tokens, penalties, n, seed, logprobs, parallel_tool_calls, include_orchestration_trace, echo, best_of, dimensions, max_output_tokens, and responses stream as omit. Whitespace-only stop arrays are also omit. Local full unit: 917 passed. * chore: bootstrap deterministic empty-string honesty merge * fix(ci): configure merge identity before reconciliation * fix(ci): disambiguate both seed validators before transform * fix(ci): disambiguate duplicated stop-string validators * fix(ci): recognize cumulative parallel-tool no-op contract --------- Co-authored-by: contextual-orchestrator-maintainer[bot] <contextual-orchestrator-maintainer[bot]@users.noreply.github.com>
…at (#573) * fix(api): treat empty-string optional numeric/boolean controls as omit SDK clients may stringify omitted optionals as empty strings. Treat empty/whitespace temperature, top_p, max_tokens, max_completion_tokens, penalties, n, seed, logprobs, parallel_tool_calls, include_orchestration_trace, echo, best_of, dimensions, max_output_tokens, and responses stream as omit. Whitespace-only stop arrays are also omit. Local full unit: 917 passed. * fix(api): accept OpenAI multimodal text+image_url content parts on chat Vision callers send content-parts arrays. Shape-check and passthrough text/image_url parts; unsupported part types fail closed. Coerce part text for agent selection so list content does not 500. Tip substrate from empty-string numeric honesty. Local full unit: 921 passed. * chore: diagnose multimodal cumulative reconciliation * fix(ci): apply reviewed multimodal cumulative reconciliation --------- Co-authored-by: contextual-orchestrator-maintainer[bot] <contextual-orchestrator-maintainer[bot]@users.noreply.github.com>
seonghobae
changed the title
|
@opencode-agent @cwl-noema-review Please review exact current head |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headd192fa123b6dc368ba98e6b79ec111f71c5a08f2. -
Head SHA:
d192fa123b6dc368ba98e6b79ec111f71c5a08f2 -
Workflow run: 31998286782
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (128 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (128 files)"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (2 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (2 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (128 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (128 files)"]
R3 --> V3["targeted test run"]
|
1 participant
Status
Canonical cumulative OpenAI-compatible API-honesty head. Ready for fresh exact-head review; not merge-authorized until current protection is satisfied.
Product contract
GenerationOptionsthroughContextVarstate so concurrent requests cannot leak token, temperature, top-p, or penalty controls.textandimage_url, preserves original image content for provider calls, extracts only text for routing and complexity decisions, and rejects malformed or unsupported parts such asinput_audio.Consolidation boundary
This branch supersedes the historical single-slice API-honesty PRs #143, #232, #245–#250, #252–#258, #260–#272, #466–#562, #563, and #564 where their product contracts are present in this cumulative tree. The separate pool-scoped IDOR repair remains PR #566.
Closing a predecessor does not transfer its review or check evidence. Every predecessor-head result is historical only.
Exact identity and verification
main@6841b71935e0b7cb98fb52bcb4709cc5100c8d87;d192fa123b6dc368ba98e6b79ec111f71c5a08f2;The multimodal reconciliation was verified before stack integration by run
31943407694, job95155738033:compileallandgit diff --checkpassed;That run is implementation lineage, not a substitute for the newly generated checks on
d192fa123b6dc368ba98e6b79ec111f71c5a08f2.Merge contract
Merge only through ordinary protected integration after the unchanged exact head has:
Queued, pending, skipped, predecessor-head, author-only, local-only, model-comment, synthetic, or status-only evidence is not acceptance.