fix(api): fail-closed tool_calls extra keys and mode/trace on tools path - #610
fix(api): fail-closed tool_calls extra keys and mode/trace on tools path#610cursor[bot] wants to merge 15 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.
…age honesty Null include_usage/include_obfuscation stay omit-equivalent, but unknown stream_options keys no longer become no-ops just because their value is null. Weight, prefix, refusal, annotations, developer role, empty user/system content, and participant name now use the same named errors on the tools passthrough path as on orchestration. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Hoist stream, required model, stream_options, and temperature/top_p range checks before proxy_completion so a tools or response_format body cannot return a billed JSON completion when the SDK asked for SSE, or silently pick a pool model when model is omitted. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Hoist attribution and routing validation before proxy_completion so a tools or response_format body cannot bill a sync completion with an unknown spend dimension or a batch/latency_tolerant hint. Tools passthrough has no batch job plane. Buyer next action: send known sync attribution; omit routing.channel=batch and latency_tolerant=true on tool-calling requests. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Match _validate_messages: tools + user content 123 must 400 invalid_message instead of billing a sync completion. Assistant/tool JSON null stays omit-equivalent. Buyer next action: send user/system content as a non-empty string or a content-parts array. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Match the tools-path honesty contract to the invalid_message behavior so SDK clients send a string or content-parts array. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Hoist the remaining chat request knobs before proxy_completion so an OpenAI SDK tool-calling body cannot bill a sync completion for seed, stop, n>1, logprobs, logit_bias, out-of-range token/penalty values, unsupported reasoning_effort, or a non-default service_tier. Buyer next action: omit those fields on tool-calling requests; they are not applied on this gateway. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Call _validate_messages before proxy_completion so tools + [] / omitted / null / non-list / non-object messages raise invalid_message instead of billing a completion with no prompt. Buyer next action: always send a non-empty messages array of objects on tool-calling requests. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Unknown assistant tool_calls siblings and function keys, plus mode=conduct and include_orchestration_trace=true, billed a silent chat.completion on the tools proxy. Reject them with the same named errors as orchestration. 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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Buyer next action
Do not merge #610. Keep using #617 (a1823cc) as the honesty landing vehicle (SSE tools + mock tool_calls + extra tool_calls keys). Cherry-pick only the remaining fail-closed slice onto that tip: mode=conduct and include_orchestration_trace=true must 400 on tools / response_format instead of billing a silent single-agent completion.
What this head actually adds
Unique commit 009b75c (5 files, +456) is real and locally green:
python3 tests/test_tool_calls_entry_keys_http_honesty.py→okpython3 tests/test_passthrough_mode_trace_http_honesty.py→ok
Compared to the current tip:
| Slice | #610 009b75c |
#617 a1823cc |
|---|---|---|
Unknown tool_calls siblings / function keys |
fail-closed | already landed (and splits unknown_tool_call_function_fields, checks extras before name) |
mode=bogus / non-boolean trace |
fail-closed | already landed via _validate_request_mode_if_present / _validate_include_orchestration_trace_flag |
mode=conduct on tools / response_format |
400 invalid_mode |
still bills — helper accepts conduct |
include_orchestration_trace=true on that path |
400 | still bills — helper returns True and the call site discards it |
stream=true + tools |
400 invalid_stream (regresses SSE) |
SSE-proxied |
#613 (37f1224) already has the conduct/trace hoist on the older #601 substrate. Prefer that unique function over merging this 146-file parallel stack.
Do not land from here
Merging #610 would race #617 and re-break SSE tool-calling (stream=true still fail-closed on this stack). Independent non-author APPROVE + Full unit/Semgrep are still required on the landing vehicle. This automation will not self-approve or merge #610.
Preserve #610’s all-keys loop when cherry-picking: orchestration=route plus mode=conduct must still 400. #617’s or chain would keep route and hide conduct.
Sent by Cursor Automation: Fix Issues
| def _validate_chat_passthrough_gateway_knobs(body: dict[str, Any]) -> None: | ||
| """Fail-closed gateway knobs that otherwise run only after tools proxy. | ||
|
|
||
| ``mode`` / ``orchestration`` / ``orchestration_mode`` and | ||
| ``include_orchestration_trace`` are applied only on the orchestration | ||
| path. Tools / ``response_format`` proxy is single-agent and strips those | ||
| keys, so a billed ``chat.completion`` would hide that conduct/trace never | ||
| ran. ``auto`` and ``route`` are honest (passthrough is the route). | ||
| ``conduct``, unknown modes, non-boolean trace, and ``trace=true`` fail | ||
| closed with the same named errors as orchestration. | ||
| """ | ||
| for key in ("orchestration", "orchestration_mode", "mode"): | ||
| if key not in body: | ||
| continue | ||
| raw_mode = body.get(key) | ||
| if raw_mode is None or (isinstance(raw_mode, str) and not raw_mode.strip()): | ||
| continue | ||
| mode = _validate_mode(raw_mode) | ||
| if mode == "conduct": | ||
| raise RequestError( | ||
| 400, | ||
| "invalid_mode", | ||
| "mode=conduct is not supported with tools or response_format; " | ||
| "omit mode or set mode=route", | ||
| ) | ||
| if "include_orchestration_trace" not in body: | ||
| return | ||
| include_trace_raw = body.get("include_orchestration_trace") | ||
| if include_trace_raw is None or ( | ||
| isinstance(include_trace_raw, str) and not include_trace_raw.strip() | ||
| ): | ||
| return | ||
| if not isinstance(include_trace_raw, bool): | ||
| raise RequestError( | ||
| 400, | ||
| "invalid_include_orchestration_trace", | ||
| "include_orchestration_trace must be a boolean", | ||
| ) | ||
| if include_trace_raw is True: | ||
| raise RequestError( | ||
| 400, | ||
| "invalid_include_orchestration_trace", | ||
| "include_orchestration_trace=true is not supported with tools or " | ||
| "response_format; omit it or set false", | ||
| ) |
There was a problem hiding this comment.
This is the only slice still missing on #617. _validate_request_mode_if_present / _validate_include_orchestration_trace_flag already run before proxy_completion there, but they accept conduct and include_orchestration_trace=true and the call site drops the return value — so a tools body still bills a chat.completion with no workflow and no trace.
Cherry-pick this hoist onto #617. Keep the per-key loop (do not collapse to orchestration or orchestration_mode or mode) so a mixed mode=conduct + orchestration=route body cannot hide conduct. Do not merge this stack: it still 400s stream=true with tools.
| unknown_fn = sorted(set(function) - {"name", "arguments"}) | ||
| if unknown_fn: | ||
| raise RequestError( | ||
| 400, | ||
| "unknown_tool_call_fields", | ||
| "each tool_calls function accepts only name and arguments", | ||
| {"fields": unknown_fn}, | ||
| ) |
There was a problem hiding this comment.
![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 head009b75cca5a2135c1e7ee5b2643aa2a58c6487c7. -
Head SHA:
009b75cca5a2135c1e7ee5b2643aa2a58c6487c7 -
Workflow run: 32138273762
-
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 head009b75cca5a2135c1e7ee5b2643aa2a58c6487c7. -
Head SHA:
009b75cca5a2135c1e7ee5b2643aa2a58c6487c7 -
Workflow run: 32148129751
-
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"]
2 participants
Buyer next action
On
/v1/chat/completionswithtoolsorresponse_format, send assistanttool_callswith onlyid/type/function/ optionalindex. Omitmode=conductandinclude_orchestration_trace=true(or sendmode=route/include_orchestration_trace=false). Extratool_callskeys,mode=conduct, unknown modes, andinclude_orchestration_trace=true/ non-boolean values now fail closed instead of billing a silent sync completion.Why this PR exists
#596 at
7a07562closed empty/non-objectmessages, spend keys, batch routing, and request knobs on the tools path. Live HTTP on that head: tools +tool_calls[0].smuggle/function.extra_hintstill billed achat.completionand forwarded the raw object. The same fail-open applied tomode=bogus,mode=conduct,include_orchestration_trace="yes", andinclude_orchestration_trace=true.tool_callssiblings andfunctionkeys (unknown_tool_call_fields) beforeproxy_completion._validate_chat_passthrough_gateway_knobsfails closed onconduct/ unknown modes and on true / non-booleaninclude_orchestration_trace.tests/test_tool_calls_entry_keys_http_honesty.pyandtests/test_passthrough_mode_trace_http_honesty.py(RED on fix(api): fail-closed empty messages, spend keys, and request knobs on tools passthrough #596).docs/rest_api_design.mdand README tell buyers the next action (APA: OpenAI, 2024).Prefer this head over #582–#587, #589, #591–#597, and #596 for the tool_calls / mode / trace slice. #601 still has unique streamed-route temperature work — cherry-pick that onto this tip; do not merge those heads in parallel. Independent non-author APPROVE + Full unit/Semgrep required. Do not self-approve.
Test plan
python3 tests/test_tool_calls_entry_keys_http_honesty.pypython3 tests/test_passthrough_mode_trace_http_honesty.pypython3 tests/test_chat_assistant_tool_calls_http_honesty.pypython3 tests/test_chat_include_orchestration_trace_http_honesty.pypython3 tests/test_passthrough_attribution_routing_http_honesty.pypython3 tests/test_message_honesty_tools_passthrough_http_honesty.pypython3 tests/test_chat_messages_array_tools_passthrough_http_honesty.pypython3 tests/test_paper_contracts.pytest_self_check.pytest_conventions.pytest_api_contract.pyDocs
docs/rest_api_design.mdhonesty contract now includestool_callsextra keys and mode/trace before passthrough.