fix(api): land tools knobs, omit-real tools, and KV host allowlist - #608
fix(api): land tools knobs, omit-real tools, and KV host allowlist#608cursor[bot] wants to merge 16 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>
Stop reading CONTEXTUAL_ORCHESTRATOR_ALLOWED_PROVIDER_HOSTS in ModelClient._validate_provider. The allowlist lives in provider_egress.allowed_provider_hosts; env is bootstrap transport via seed_provider_egress_from_environ only. 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 (COMMENT only)
Unique slices on 5901b79 are sound: KV provider_egress.allowed_provider_hosts is request-time only, JSON-null tool.function fields are popped before proxy, and tools-path seed/stop/n/logprobs/batch fail closed. Do not treat this as a merge vehicle.
Buyer next action: do not merge #608 in parallel with the later landing tips. Prefer:
- #616 for allowlist + trusted-trace email
- #613 / #624 for tools-path
mode/include_orchestration_trace - #615 for streamed
top_p/ penalties - #606 / #620 for SSE tools proxy
- #621 for gateway Bearer tokens in the credential KV
Independent non-author APPROVE plus Full unit / Semgrep is still required on whichever tip you land. This automation will not approve or merge.
Residuals on this head (already owned elsewhere — do not re-implement here):
tools+mode=explode/include_orchestration_tracestill 200 because passthrough returns before_validate_mode.stream_chatstill dropstop_p/ penalties after the temperature fix.- Allowlist seed empty-check + write is not under
_runtime_lock;_parse_host_allowlisthas no fuzz target yet. - Process-local
InMemoryConfigStoreonly; empty KV means no extra hostname filter.
Next unique product gap after those landings is issue #568 (role reasoning_effort_profile + equal-budget ablation), not another honesty-stack or allowlist PR.
Sent by Cursor Automation: Fix Issues
| ) | ||
| # Explicit JSON null on trigger keys is omit-equivalent (SDK optional | ||
| # defaults) — do not force single-agent passthrough for null-only keys. | ||
| if any( |
There was a problem hiding this comment.
| @@ -330,7 +368,7 @@ def stream_chat(self, agent: ModelAgent, messages: list[ChatMessage], temperatur | |||
| payload = { # pragma: no cover | |||
There was a problem hiding this comment.
stream_chat now copies request temperature, but still omits top_p / presence_penalty / frequency_penalty that chat() applies at 295–300. A streamed invoice at top_p=0.1 silently uses the provider default.
Cherry-pick the #615 nucleus/penalty hoist. Do not open a third stream-sampling PR from this stack.
| This is the only allowed ``os.environ`` read for the host allowlist. | ||
| Request-time validation must call :func:`allowed_provider_hosts`. | ||
| """ | ||
| existing = get_runtime_config(PROVIDER_EGRESS_CATEGORY, ALLOWED_PROVIDER_HOSTS_KEY, None) |
There was a problem hiding this comment.
Bootstrap seed is correct (env only when the KV key is empty). Residual: the empty-check and write are not under _runtime_lock, so concurrent main() + serve() can both observe empty and widen the allowlist.
Hold the lock across the check+write on the #616 allowlist tip. Add a Hypothesis/fuzz/targets.py seam for _parse_host_allowlist there — do not open another ALLOWED_PROVIDER_HOSTS PR.
![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 head5901b7962be8e472953b2467f603d2f7802c722b. -
Head SHA:
5901b7962be8e472953b2467f603d2f7802c722b -
Workflow run: 32138260950
-
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 head5901b7962be8e472953b2467f603d2f7802c722b. -
Head SHA:
5901b7962be8e472953b2467f603d2f7802c722b -
Workflow run: 32148116418
-
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
Landing vehicle for the tools-passthrough honesty stack plus the remaining request-time env allowlist deviation.
Superset of #601 (
18e6263) with two unique commits on this branch:e58cb2f: pop JSON-nulltools[].function.description/parameters/strictbeforeproxy_completionso SDK defaults are omit-real.a50911f:ModelClient._validate_providerreadsprovider_egress.allowed_provider_hostsfrom the process KV.CONTEXTUAL_ORCHESTRATOR_ALLOWED_PROVIDER_HOSTSis bootstrap transport viaseed_provider_egress_from_environonly.Live HTTP on this tip (
5901b79):tools+ emptymessages→ 400invalid_messagetools+seed: 1/stop/n: 2→ 400 named errors (the fix(api): fail-closed empty messages and spend keys on tools passthrough #597 residual)tools+routing.channel=batch→ 400invalid_routingtools+ nulldescription/strict→ 200 and those keys are absent from the echoed provider bodyLocal:
test_chat_messages_array_tools_passthrough_http_honesty.py,test_passthrough_request_knobs_http_honesty.py,test_tool_description_parameters_null_noop_http_honesty.py,test_provider_host_allowlist_kv.py,test_security_hardening.py, paper/self-check/conventions/api-contract →ok.Do not merge #589, #592, #594, #596, #597, #598, #599, #600, #601, #603, or #605 in parallel. Prefer this head.
Buyer next action: send a non-empty
messagesarray of objects; omitstream(or setfalse) on tool-calling requests; always send a poolmodel; omitrouting.channel=batch,latency_tolerant=true,seed,stop,n>1, andlogprobswithtools. Leave unuseddescription/parameters/strictomitted or JSONnull. Seedprovider_egress.allowed_provider_hostsin the KV (or setCONTEXTUAL_ORCHESTRATOR_ALLOWED_PROVIDER_HOSTSat process start so bootstrap can copy it once).APA 7th: OpenAI (2024); Bray (2017); Joint Task Force (2020) NIST SP 800-53 Rev. 5 SC-7; ISO/IEC 27001:2022 A.8.20.
Merge only after Full unit + Semgrep are green and an independent non-author review lands. This automation does not approve or merge.
Test plan
python3 tests/test_chat_messages_array_tools_passthrough_http_honesty.pypython3 tests/test_passthrough_request_knobs_http_honesty.pypython3 tests/test_tool_description_parameters_null_noop_http_honesty.pypython3 tests/test_provider_host_allowlist_kv.pypython3 tests/test_security_hardening.pypython3 tests/test_paper_contracts.pytest_self_check.pytest_conventions.pytest_api_contract.pyDocs
CHANGELOG.md[Unreleased] records the fail-closed knobs, omit-real tools, and KV allowlist with buyer next actions.docs/rest_api_design.md, README,docs/architecture.md,docs/kv-credentials.md, anddocs/papers/README.mdcarry the same contract plus APA 7th citations.