fix(api): fail-closed stream, model, and sampling on tools passthrough - #600
fix(api): fail-closed stream, model, and sampling on tools passthrough#600cursor[bot] wants to merge 12 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.
Validate weight/prefix/refusal/annotations before the tools early-return so SDK tool-calling bodies cannot smuggle unsupported message fields. Strip omit-equivalent max_tool_calls before provider passthrough and name-reject the field on /v1/completions. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Reject non-OpenAI keys on assistant tool_calls objects with named unknown_tool_call_fields / unknown_tool_call_function_fields. Accept optional non-negative index (null omit) for stream-assembled histories. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Developer role, empty user/system content, multimodal part shape, and participant name were only checked in _validate_messages, which the tools/response_format early-return skips. SDK tool-calling bodies could therefore proxy unsupported values. Run the same fail-closed checks before passthrough and cover the realistic tools+field HTTP cases. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Completions and chat applied temperature, top_p, penalties, and max_tokens by mutating shared ModelClient defaults. Concurrent ThreadingHTTPServer workers could observe another request's knobs. request_sampling stores overrides on threading.local and restores them on exit; chat/stream_chat read the calling thread first. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Tools/response_format still billed a completion when messages was [], omitted, null, or a non-list. Require the same non-empty array as _validate_messages before the early-return. stream_chat now reads the same thread-local sampling knobs as chat so route streaming cannot drop top_p or penalties. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Keep Fugu/TRINITY/Conductor titles for paper-contract search while recording APA 7th citations. Split the optional tool_calls index case so omit-key is actually tested instead of the previous tautology. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Empty-messages honesty still left tools/response_format billing a JSON completion when the SDK asked for SSE, omitted model, or sent out-of-range temperature/top_p. Hoist those checks before proxy_completion so the #594 tip is the mergeable request-level gate. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
There was a problem hiding this comment.
Stale comment
Review (#600 @
6f35df4)Do not merge this head in parallel with #597.
This is the #594 lineage plus
_normalize_chat_stream_flagand the stream/model/stream_options/sampling-range hoist. Local RED/GREEN:tests/test_passthrough_stream_model_http_honesty.pyfailed on #594 (tools+stream: true→ JSON 200) and passed after the hoist. Empty-messages helper andstream_chat_effective_request_samplingare kept.#597 is the designated honesty-stack tip (empty messages + spend keys + batch-routing reject on the #592 substrate). This PR does not include those #592/#597 slices.
Unique value to cherry-pick onto #597:
ModelClient._effective_request_samplingso route streaming appliestop_p/ penalties. #597 memory still listsstream_chatas dropping those knobs.Buyer next action: review/merge #597, not this parallel. On that tip, send a non-empty
messagesarray of objects and a poolmodel; omitstream(or setfalse) on tool-calling requests.Independent non-author APPROVE still required. This automation will not APPROVE.
Sent by Cursor Automation: Fix Issues
|
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. |
There was a problem hiding this comment.
Review (#600 @ 6f35df4)
Do not merge this head in parallel with #601.
Unique work is sound. _normalize_chat_stream_flag plus the stream / required pool model / stream_options / temperature / top_p hoist runs before proxy_completion. Local HTTP: tests/test_passthrough_stream_model_http_honesty.py printed ok (tools + stream: true → 400 invalid_stream; omitted/unknown model → 400 invalid_model; include_usage: true → 400 invalid_stream_options; temperature: 99 / top_p: 2 → named 400; in-range temperature: 0.2 and stream: null still 200 JSON; route stream: true without tools still SSE).
Landing vehicle is #601 (7e9a339 on cursor/bc-a9639c65-4bbc-4392-a3f8-5ed3dd135101-a2cf). That tip already has this hoist class plus empty-messages, spend keys, batch-routing reject, _validate_chat_passthrough_request_knobs, stream-route temperature, and the #603 omit-real pop. Do not merge #582–#600 / #603 / #605 as parallel stacks.
Residuals still billed on this head (tools nonempty → 200 chat.completion): seed: 1, stop, n: 5, logprobs: true, logit_bias, presence_penalty: 3, frequency_penalty: 3, max_tokens: -1 / 0, empty user, routing.channel=batch, non-object attribution, unknown reasoning_effort. Those are the #601 contract. Null tool.function.description is already 400 invalid_tools here; #601 now pops that omit-equivalent instead.
CodeRabbit CLI 0.7.3 is installed; coderabbit auth login --agent stayed unauthenticated (browser callback). Not a merge blocker. This review is live HTTP + source, not a CodeRabbit result.
Independent non-author APPROVE + Full unit / Semgrep still required. This automation will not APPROVE or merge.
Buyer next action: review and land #601, not this parallel. On that tip, send a non-empty messages array and a pool model; omit stream (or set false) on tool-calling requests until SSE tools passthrough exists; omit seed / stop / n>1 / logprobs. After #601: rebase #598 (KV provider-host allowlist), then SSE tools passthrough so the default OpenAI SDK stream=true tool-calling body no longer 400s.
Sent by Cursor Automation: Fix Issues
| stream = _normalize_chat_stream_flag(body) | ||
| if stream: | ||
| raise RequestError( | ||
| 400, | ||
| "invalid_stream", | ||
| "stream=true is not supported with tools or response_format " | ||
| "on this gateway; omit stream or set stream=false", | ||
| ) | ||
| if "stream_options" in body: | ||
| _validate_chat_stream_options(body, stream) | ||
| model_name = _validate_completions_model(body) | ||
| _require_pool_model(orchestrator, model_name) | ||
| if "temperature" in body: | ||
| _validate_completions_temperature(body) | ||
| if "top_p" in body: | ||
| _validate_completions_top_p(body) | ||
| started_at = time.perf_counter() | ||
| proxied = self._run( | ||
| lambda: orchestrator.proxy_completion(body, endpoint="chat/completions") |
There was a problem hiding this comment.
Hoist is real and the stream/model/sampling cases fail closed. The next statements still skip the route-path helpers, so tools + seed / n>1 / stop / logprobs / out-of-range penalties / max_tokens<=0 / empty user / routing.channel=batch still return 200 chat.completion on this head (probed live). Do not patch those here — #601 already calls _validate_chat_passthrough_request_knobs (and batch reject) before proxy_completion. Land that tip; cherry-pick nothing from this parallel except ModelClient._effective_request_sampling if #601 still drops stream-route top_p / penalties.
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 head6f35df445dc11e29ae9bd0817a12ccabd6719b62. -
Head SHA:
6f35df445dc11e29ae9bd0817a12ccabd6719b62 -
Workflow run: 32138251544
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
Evidence["OpenCode evidence"] --> Review["Current PR review path"]
Review --> Verify["Required checks"]
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
Evidence["OpenCode evidence"] --> Review["Current PR review path"]
Review --> Verify["Required checks"]
|
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 head6f35df445dc11e29ae9bd0817a12ccabd6719b62. -
Head SHA:
6f35df445dc11e29ae9bd0817a12ccabd6719b62 -
Workflow run: 32148106397
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
Evidence["OpenCode evidence"] --> Review["Current PR review path"]
Review --> Verify["Required checks"]


Summary
stream=true, omitted/unknownmodel,stream_options.include_usage=true, and out-of-rangetemperature/top_pnow fail closed beforeproxy_completion.stream=truewith tools orresponse_formatreturns namedinvalid_stream(SSE passthrough is still a follow-up). Setstream=falseor omitstream.modelreturns namedinvalid_modelinstead of a silent pool pick. In-rangetemperaturestill forwards on passthrough.stream_chatknob share so this head is the mergeable tip of fix(api): treat chat max_tool_calls null/empty as omit; fail-closed otherwise #582–fix(api): fail-closed empty messages before tools passthrough #594.Buyer next action: send a non-empty
messagesarray and a poolmodel; omitstream(or setfalse) on tool-calling requests. Do not setinclude_usage=true.Do not merge #582–#594. Prefer this head over #591 (missing
stream_chatknob share) and #592 (missing empty-messages helper).Test plan
python3 tests/test_passthrough_stream_model_http_honesty.py(RED on fix(api): fail-closed empty messages before tools passthrough #594: tools+stream=true was 200 JSON)python3 tests/test_message_role_content_name_tools_passthrough_http_honesty.pypython3 tests/test_model_client_request_sampling_thread_isolation.pypython3 tests/test_openai_passthrough.pytest_true_streaming.pytest_chat_stream_options_http_honesty.pypython3 tests/test_paper_contracts.pytest_conventions.pytest_api_contract.pytest_product_planning_contract.pyProduct gates only: Full unit + Semgrep (Strix ignored). Independent non-author APPROVE still required.
References