fix(api): treat stream_options null flags as omit-equivalent no-ops - #587
fix(api): treat stream_options null flags as omit-equivalent no-ops#587seonghobae wants to merge 7 commits into
Conversation
…closed otherwise Chat history: message-level audio and legacy function_call are null/empty omit no-ops; non-empty fail closed with named errors (including tools passthrough). Tip substrate from #577 assistant refusal/annotations honesty. Local full unit: 940 passed.
…ed otherwise OpenAI fine-tune style message weight is not applied on this gateway. Accept null/0/1 as honest no-ops; reject other types and values with invalid_message_weight. Tip substrate from #578. Local full unit: 943 passed.
…ion role Reject unsupported message keys with named unknown_message_fields (not silent strip or tools-passthrough smuggle). Reject legacy function role with invalid_message_role migration to tool. Tip substrate from #579. Local full unit: 947 passed.
OpenAI partial-assistant prefix flag is not applied on this gateway. null/false are honest no-ops; true and non-booleans fail closed with invalid_message_prefix. Tip substrate from #580. Local full unit: 950 passed.
…therwise Named invalid_max_tool_calls on /v1/chat/completions instead of opaque unknown_fields. Aligns with Responses max_tool_calls honesty; gateway has no multi-step tool loop.
…losed otherwise Legacy /v1/completions treated max_tool_calls as unknown_fields. Accept the key for named invalid_max_tool_calls (null/empty/whitespace omit-equivalent), matching chat/Responses honesty so SDKs get a clear migration path.
SDK clients often send include_usage/include_obfuscation as JSON null. Drop null flag values before validation so null (and null+false mixes) match omit / all-false no-ops on chat, Completions, and Responses. True flags remain fail-closed with invalid_stream_options.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Important Review skippedToo many files! This PR contains 138 files, which is 38 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 (138)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Do not merge this head (9d10fa9). The tip’s null-flag no-op is the right SDK contract, but two honesty holes would ship to buyers.
1. Unknown stream_options keys become omit-equivalent when null. Dropping every None value before the unknown-key check accepts {include_continuous: null} and {include_usage: true, extra_flag: null} (the unknown key disappears). Fail closed on unknown keys first; only then treat allowed include_usage / include_obfuscation null/false as omit. True flags must still return invalid_stream_options.
2. This PR stacks the #582 tools-passthrough fail-open. _validate_messages (weight, prefix, refusal, annotations, developer, empty user/system content, participant name, multimodal part shape) runs after the tools early return. A tools request can smuggle those fields to proxy_completion. #585/#586 already exist for that hole; this branch does not include them.
3. Review coverage. GitHub CodeRabbit skipped (138 files > 100). CodeRabbit CLI agent auth timed out. The 138-file stack vs main is not a mergeable increment.
Repair is #592 (b108251): unknown keys fail closed even when null; tools-path message honesty matches the orchestration path; HTTP tests cover unknown-null, true+null, and tools+weight/prefix/refusal/developer/empty content/name/input_audio.
Next action: close or supersede this PR with #592. Merge only after Full unit + Semgrep are green on that head and an independent non-author review lands. Keep sending SDK-default null flags; do not set include_usage=true / include_obfuscation=true.
Sent by Cursor Automation: Fix Issues
| if not opts: | ||
| return None | ||
| # Drop null flag values (SDK optional defaults) before further checks. | ||
| opts = {key: value for key, value in opts.items() if value is not None} |
There was a problem hiding this comment.
This drop runs before the unknown-key check. {include_logprobs: null} becomes {} and is treated as omit. Reject keys outside include_usage / include_obfuscation first — null is omit only for those two flags.
| if not opts: | ||
| return None | ||
| # Drop null flag values (SDK optional defaults) before further checks. | ||
| opts = {key: value for key, value in opts.items() if value is not None} |
There was a problem hiding this comment.
Same swallow on chat: {include_continuous: null} or {include_usage: true, extra_flag: null} hides the unknown key. Unknown keys must fail closed with invalid_stream_options even when the value is JSON null.
| return | ||
| if isinstance(opts, dict): | ||
| # Null flag values alone are omit-equivalent (SDK optional defaults). | ||
| non_null = {key: value for key, value in opts.items() if value is not None} |
There was a problem hiding this comment.
Responses has the same order bug. {extra_flag: null} returns omit instead of invalid_stream_options. Filter unknown keys before dropping nulls.
| _validate_chat_message_known_fields(body) | ||
| _validate_chat_tool_message_ids(body) | ||
| _validate_chat_assistant_tool_calls(body) | ||
| _validate_chat_message_audio_function_call(body) |
There was a problem hiding this comment.
Pre-passthrough checks stop at audio/function_call. Weight, prefix, refusal, annotations, developer role, empty user/system content, participant name, and multimodal part shape are only enforced in _validate_messages, which runs after the tools early return. A tools request smuggles those fields to the provider. Run the same named-error checks here (see #585/#586 and repair #592).
![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 head9d10fa917a8f2a0d5c590edfa62c8b319f39aa22. -
Head SHA:
9d10fa917a8f2a0d5c590edfa62c8b319f39aa22 -
Workflow run: 32138227091
-
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["Test (135 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (135 files)"]
R2 --> V2["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["Test (135 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (135 files)"]
R2 --> V2["targeted test run"]
|
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 head9d10fa917a8f2a0d5c590edfa62c8b319f39aa22. -
Head SHA:
9d10fa917a8f2a0d5c590edfa62c8b319f39aa22 -
Workflow run: 32148082832
-
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["Test (135 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (135 files)"]
R2 --> V2["targeted test run"]
Merge the exact current head f5dbf58 after all required checks completed successfully: full unit/contract suite, Hypothesis, Atheris, CodeQL, Semgrep, Python supply chain, dependency review, OSV, Trivy, Scorecard, coverage-evidence, and fresh Strix. The fresh OpenCode wrapper also completed successfully but submitted no review; a targeted scheduler/OpenCode dispatch was queued behind org runner saturation. No unresolved review threads were present. Use the documented admin fallback only for the unsatisfied independent approving-review requirement; no CI check was bypassed.
1 participant
Summary
stream_options.include_usage/include_obfuscationset to JSONnullno longer fail as non-boolean.Test plan
tests/test_stream_options_null_flags_noop_http_honesty.pypython -m pytest tests -q(964 passed)