fix(api): fail-closed mode and orchestration-trace on tools passthrough - #613
fix(api): fail-closed mode and orchestration-trace on tools passthrough#613cursor[bot] wants to merge 17 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>
Call the orchestration message, max_tokens, attribution, and routing validators before proxy_completion so a tools or response_format body cannot bill a completion with no prompt, crash on a non-object entry, or silently drop unknown spend/routing keys. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Passthrough has no batch job plane. Reject routing.channel=batch and latency_tolerant=true before proxy_completion so a tool-calling body cannot bill a silent sync completion. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Hoist the orchestration request-knob validators before proxy_completion so a tools or response_format body cannot bill a completion that silently drops seed, stop, n>1, logprobs, logit_bias, or out-of-range penalties. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
stream_chat defaulted to 0.2 even after the HTTP path wrote default_temperature from the request. A streamed invoice summary at temperature=0.8 no longer silently falls back to 0.2. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
… proxy SDK optional defaults serialize omitted fields as JSON null. Accepting those keys without popping them is not omit-equivalent: proxy_completion forwards the body and several providers reject a null JSON Schema object. Pop the keys in place, keep non-null wrong types on invalid_tools, and assert omit-real via mock echo on chat and Responses. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Hoist mode/orchestration and include_orchestration_trace before proxy_completion. tools + mode=explode and include_orchestration_trace="yes" were 200 on #601. mode=conduct and include_orchestration_trace=true also fail closed — this path has no Conductor workflow or trusted-trace plane. 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.
Review (#613 unique 7e9a339...37f1224)
CodeRabbit CLI 0.7.3 is installed in this environment but coderabbit auth login --agent failed (automatic_login_failed / browser auth required). This assessment is from a senior review of the unique range, not a CodeRabbit CLI run.
The advertised #601 hole is closed: _validate_chat_passthrough_orchestration_controls runs on the tools/response_format early-return before proxy_completion. Live HTTP cases cover explode, orchestration=explode, include_orchestration_trace="yes", true, mode=conduct, response_format+explode, auto+false, and null. Named errors and buyer next-action copy are in place. /v1/responses is not a silent hole — those knobs are not in ALLOWED_RESPONSES_KEYS.
Two residuals remain on this head. Do not merge #613. Prefer the successor that reuses the orchestration or chain and adds the missing live cases.
Strengths
- Fail-closed placement is correct: billed
chat.completioncan no longer ignore these knobs. - Reuses
_validate_modeand the existinginvalid_mode/invalid_include_orchestration_tracecodes. tests/test_passthrough_mode_trace_http_honesty.pyis live loopback HTTP. The #601 RED case (tools+mode=explode→ 200) is asserted as 400 /invalid_mode/ nochoices.- Docs cite Conductor (Nielsen et al., 2025) and TRINITY (Xu et al., 2025) with cite+link+summary.
Important
- Whitespace-only
modeis omit on passthrough,invalid_modeon orchestration. The strip-skip (not candidate.strip()→ continue) billstools+mode: " ". Orchestration usesbody.get("orchestration") or body.get("orchestration_mode") or body.get("mode") or "auto"— a space-only string is truthy and 400s. That breaks “same named errors as the orchestration path.” mode=routehas no live 200/choicescase on the tools path. Docs and themode=conducterror advertiserouteas an omit-equivalent no-op. A regression that 400srouteon passthrough would not be caught.
Next action
Independent non-author APPROVE + Full unit/Semgrep are still required on the successor. Do not merge #601 or #613 in parallel. Do not self-approve.
Sent by Cursor Automation: Fix Issues
| candidate = body.get(key) | ||
| if candidate is None: | ||
| continue | ||
| if isinstance(candidate, str) and not candidate.strip(): |
There was a problem hiding this comment.
Whitespace-only mode / orchestration / orchestration_mode is treated as omit (not candidate.strip() → continue), so tools + mode: " " still bills a completion.
The orchestration path does not strip. It does body.get("orchestration") or body.get("orchestration_mode") or body.get("mode") or "auto" and then _validate_mode. A space-only string is truthy, so the same body is 400 invalid_mode without tools.
That breaks “same named errors as the orchestration path.” Drop the strip-skip and reuse that or chain. "" is already omit-equivalent via or (falsy). Add a live HTTP case: tools + mode: " " → 400 invalid_mode, no choices.
|
|
||
|
|
||
| def test_http_chat_tools_accepts_mode_auto_and_false_trace() -> None: | ||
| """Honest no-ops: ``mode=auto`` and ``include_orchestration_trace=false``.""" |
There was a problem hiding this comment.
The only honest no-op on the tools path sends mode=auto. Docs and the mode=conduct error advertise mode=route as an omit-equivalent no-op. There is no live POST with tools + mode=route (or omitted mode) asserting 200 + choices.
The reject branch is if mode == "conduct", so route should pass today — a regression that 400s route on passthrough would not be caught. Add test_http_chat_tools_accepts_mode_route and wire it in __main__.
![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 head37f1224ec2217d7754dc8925ca72f7efb9a9b6e3. -
Head SHA:
37f1224ec2217d7754dc8925ca72f7efb9a9b6e3 -
Workflow run: 32138280390
-
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 head37f1224ec2217d7754dc8925ca72f7efb9a9b6e3. -
Head SHA:
37f1224ec2217d7754dc8925ca72f7efb9a9b6e3 -
Workflow run: 32148137865
-
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
Summary
Successor to #601 at
7e9a339. Unique commit37f1224.#601 closed seed/stop/n/logprobs on the tools path. Live HTTP on that head:
tools+mode=explodestill billed achat.completionwhile the mode was ignored. The same fail-open applied toorchestration=explode,include_orchestration_trace="yes",include_orchestration_trace=true(stripped, no trace), andmode=conduct(buyer asked for a Conductor workflow, got a single-worker proxy)._validate_chat_passthrough_orchestration_controlsbeforeproxy_completionso those knobs use the same named errors as orchestration.tests/test_passthrough_mode_trace_http_honesty.py(RED on fix(api): fail-closed seed/stop/n/logprobs on tools passthrough #601: tools +mode=explodewas 200).docs/rest_api_design.md, README, CHANGELOG,docs/architecture.md,docs/papers/README.md(APA: Nielsen et al., 2025; Xu et al., 2025; OpenAI, 2024).Buyer next action: omit
mode=conductandinclude_orchestration_trace=trueon tool-calling requests; usemode=autoor omit mode; omit the trace flag or set itfalse. Keep omittingseed,stop,n>1, andlogprobs.Do not merge #589, #591, #592, #594, #596, #597, #599, #600, #601, #603-as-stack, or #605 in parallel. Prefer this head. Merge only after Full unit + Semgrep are green and an independent non-author review lands.
Test plan
python3 tests/test_passthrough_mode_trace_http_honesty.py(RED on fix(api): fail-closed seed/stop/n/logprobs on tools passthrough #601, GREEN here)python3 tests/test_passthrough_request_knobs_http_honesty.pypython3 tests/test_include_orchestration_trace_null_noop_http_honesty.pypython3 tests/test_chat_orchestration_mode_http_honesty.pypython3 tests/test_chat_include_orchestration_trace_http_honesty.pypython3 tests/test_tool_description_parameters_null_noop_http_honesty.pypython3 tests/test_stream_chat_request_sampling_http_honesty.pypython3 tests/test_paper_contracts.pytest_self_check.pytest_conventions.pytest_api_contract.pytest_product_planning_contract.pyDocs
mode=conductand trusted traces cannot silently drop on the tools path.References