fix(api): fail-closed role, content, and name on tools passthrough - #586
fix(api): fail-closed role, content, and name on tools passthrough#586cursor[bot] wants to merge 10 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>
|
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. |
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>
There was a problem hiding this comment.
Stale comment
The #584 fail-open on
weight/prefix/refusal/annotationsis closed here, and Completionsmax_tool_calls, omit-pop, role/content/name, and thread-local sampling all hold. Local honesty + sampling tests passed.Do not merge
28ef9d2yet. Tools passthrough still accepts a chat completion without a valid messages array. Live at this head:tools+messages: []/ omitted / JSONnull/ non-list all return 200 and are proxied. Without tools, empty messages correctly 400invalid_message. A buyer sendingtoolsfrom an SDK can therefore get a billed completion with no prompt.Next action: require a non-empty
messagesarray before the passthrough early-return (same named error as_validate_messages) and add HTTP cases with nonemptytools. Follow-up branch from this head will land that repair.Sent by Cursor Automation: Fix Issues
Missing, empty, or non-list messages skipped every pre-passthrough check and still reached proxy_completion when tools or response_format forced the early-return. Reject those bodies with invalid_message, and cover response_format+weight plus unknown tool_calls.function keys on the tools path. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
There was a problem hiding this comment.
Stale comment
The empty-messages fail-open flagged on
28ef9d2is closed atafd8208.Live HTTP at this head:
tools+ omitted /[]/ JSONnull/ non-listmessagesall return 400invalid_message. Developer role, empty user content, multimodalinput_audio, and invalidnamealso 400 on the tools path. Thread-localrequest_samplingkeeps concurrent knobs on the calling thread. Claimed honesty tests passed locally:
tests/test_message_role_content_name_tools_passthrough_http_honesty.pytests/test_model_client_request_sampling_thread_isolation.pytests/test_message_weight_http_honesty.pytests/test_tool_calls_entry_keys_http_honesty.pytests/test_chat_developer_multimodal_content_http_honesty.pyDo not merge #582–#585. This head is the stacked repair for that message-honesty slice.
Still fail-open on the same early-return (live at this head; non-tools path is already 400 / SSE):
toolsorresponse_format+stream=true→ 200 JSON (proxy_completionforcesstream=false). Without tools,stream=truecorrectly returns SSE. An OpenAI SDK tool-calling client that streams will parse a completion object as a broken stream.tools+ omittedmodel→ 200 and a silent pool pick. Without tools this is 400invalid_model.tools+stream_options.include_usage=true→ 200. Without tools this is 400invalid_stream_options.tools+temperature=99→ 200. Without tools this is 400invalid_temperature.
seed/stop/nstaying 200 on passthrough is consistent withOPENAI_PASSTHROUGH_PARAM_KEYS(forward to one provider). Do not hoist those rejects.Next action: before the passthrough early-return, reject
stream=truewith namedinvalid_stream(SSE passthrough is still a follow-up), requiremodel+ pool membership, run_validate_chat_stream_options, and range-checktemperature/top_p. Independent non-author APPROVE still required. Product gates: Full unit + Semgrep.Sent by Cursor Automation: Fix Issues
There was a problem hiding this comment.
Verdict: COMMENT on 28ef9d2 (cannot REQUEST_CHANGES on cursor[bot] own PR)
The unique commits close the holes this PR claims. Live HTTP at this SHA:
| Prior #585 fail-open | This head |
|---|---|
tools + role: developer |
400 invalid_message_role |
| tools + empty user content | 400 invalid_message_content |
tools + input_audio parts |
400 invalid_message_content |
tools + empty / tool-role name |
400 invalid_message_name |
Shared ModelClient sampling defaults |
Isolated for chat() |
Do not merge #582–#585. Do not merge this head either.
Blocking
- Empty
messagesstill fail-open on tools passthrough (_validate_chat_message_content_and_namereturns whenmessagesis not a list, and is a no-op on[])._validate_messagesrequires a non-empty array, but it runs only after the tools/response_format return. Live: tools +[]/ omitted /null/ non-list → 200chat.completionwithprompt_tokens: 0. stream_chatdropstop_p/ penalties.chat()applies temperature,top_p, both penalties, andmax_tokensfrom thread-local overrides. Route streaming still ignorestop_p/ penalties.
Next action
Prefer #594 (7a16460): _require_chat_messages_array before passthrough, HTTP cases for empty/omitted/null/non-list + tools and response_format, and stream_chat sharing _effective_request_sampling with chat. Independent non-author APPROVE still required after product gates (Full unit + Semgrep).
CodeRabbit CLI 0.7.3 is installed; coderabbit auth login --agent timed out waiting for a browser.
Sent by Cursor Automation: Fix Issues
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 headafd8208f225fe1d6c6907075635177a8a7317f0d. -
Head SHA:
afd8208f225fe1d6c6907075635177a8a7317f0d -
Workflow run: 32071253960
-
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"]
|


Summary
developerrole, empty user/system content, multimodal content-part shape, participantname, and missing/empty/messagesnow run before the passthrough early-return._normalize_chat_message_content,_validate_one_chat_message_name) keep the orchestration path and the tools path on the same fail-closed rules.ModelClient.request_samplingso concurrentThreadingHTTPServerworkers cannot observe another request'stemperature/top_p/ penalties /max_tokens.response_format+weight, unknowntool_calls.functionkeys + tools, and missing/empty/non-objectmessages+ tools.This branch is the mergeable repair of #585 (
cbd0420) and therefore of #582/#583/#584. Do not merge those heads.Test plan
python3 tests/test_message_role_content_name_tools_passthrough_http_honesty.pypython3 tests/test_model_client_request_sampling_thread_isolation.pypython3 tests/test_message_weight_http_honesty.pypython3 tests/test_tool_calls_entry_keys_http_honesty.pypython3 tests/test_chat_developer_multimodal_content_http_honesty.pypython3 tests/test_api_contract.pyProduct gates only: Full unit + Semgrep (Strix ignored). Independent non-author APPROVE still required.