Skip to content

fix(api): pop null tool.function description/parameters/strict before proxy - #603

Closed
cursor[bot] wants to merge 10 commits into
mainfrom
cursor/bc-0602e1c1-575d-4b63-838b-27fd0d59fd09-7932
Closed

fix(api): pop null tool.function description/parameters/strict before proxy#603
cursor[bot] wants to merge 10 commits into
mainfrom
cursor/bc-0602e1c1-575d-4b63-838b-27fd0d59fd09-7932

Conversation

@cursor

@cursor cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown

Summary

  • Official OpenAI SDKs serialize omitted tools[].function.description, parameters, and strict 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.
  • _omit_null_tool_function_field pops JSON-null optional fields in place and keeps non-null wrong types on named invalid_tools.
  • HTTP honesty tests assert mock echo.tools no longer contains the null keys on /v1/chat/completions and /v1/responses.
  • Documents the omit-real contract in docs/rest_api_design.md, docs/architecture.md, docs/papers/README.md, README, and CHANGELOG.md (APA 7th: OpenAI, 2024; Bray, 2017).

Why this PR, not #595

#595 at 5dace0f only skipped the type check. Live mock echo.tools still carried description: null / parameters: null / strict: null. This successor sits on the #592 honesty substrate (unknown stream_options keys fail closed; tools-path message honesty) and makes the omit real before the provider hop.

Do not merge #595. Prefer this tip for the description/parameters/strict null slice. Independent non-author APPROVE + Full unit + Semgrep are still required. Do not self-approve.

Test plan

  • python3 tests/test_tool_description_parameters_null_noop_http_honesty.py (printed ok)
  • python3 tests/test_paper_contracts.py / test_api_contract.py / test_conventions.py / test_self_check.py
  • Related tools honesty scripts: chat tools shape, message honesty, stream_options null flags, empty tools, parallel tool calls, Responses tools shape
  • CI Full unit + Semgrep

Next action for buyers

Send description / parameters / strict only when you have a real string, JSON Schema object, or boolean. SDK defaults of null are safe — this gateway pops them before the provider hop.

Open in Web View Automation 

seonghobae and others added 10 commits August 16, 2026 21:18
…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>
… 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>

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment

Verdict: COMMENT

This is the omit-real repair for #595, not a self-approval and not a competing merge of the #592 stack onto main.

Unique tip e58cb2f (verified locally)

_omit_null_tool_function_field pops JSON-null tools[].function.description / parameters / strict in place before proxy_completion. Mock echo.tools no longer carries those keys on /v1/chat/completions and /v1/responses. Non-null wrong types stay named invalid_tools. tests/test_tool_description_parameters_null_noop_http_honesty.py printed ok.

How to land

The current honesty landing vehicle is #597. Cherry-pick e58cb2f onto that head. Do not merge this PR as another 140-file stack, and do not merge #595 (5dace0f is accept-not-omit).

Independent non-author APPROVE + Full unit + Semgrep are still required. Do not self-approve.

Next action

Rebase or cherry-pick e58cb2f onto #597, then close #595 and this stack once that tip is on the landing vehicle.

View PR

Open in Web View Automation 

Sent by Cursor Automation: Fix Issues

@seonghobae
seonghobae marked this pull request as ready for review August 16, 2026 15:40
@seonghobae
seonghobae self-requested a review as a code owner August 16, 2026 15:40
@seonghobae
seonghobae enabled auto-merge (squash) August 16, 2026 15:40
@cursor

cursor Bot commented Aug 16, 2026

Copy link
Copy Markdown
Author

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 cursor Bot left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: COMMENT

Unique tip e58cb2f is SOUND. This is not an approval and not a merge of the 146-file stack onto main.

Unique tip (verified)

_omit_null_tool_function_field pops JSON-null tools[].function.description / parameters / strict in place before proxy_completion. Mock echo.tools no longer carries those keys on /v1/chat/completions and /v1/responses. Non-null wrong types stay named invalid_tools. tests/test_tool_description_parameters_null_noop_http_honesty.py printed ok.

Predecessor #595 (5dace0f) only skipped the type check. Live echo on that head still forwarded the null keys.

How to land

The omit-real commit is already on #601 as bc73dd2 (current landing vehicle 7e9a339). Do not merge this PR as another 140-file honesty stack. Do not merge #595.

Next buyer slice on that line is #611 (KV provider_egress.allowed_provider_hosts; do not merge #598 as a stack). Independent non-author APPROVE + Full unit + Semgrep are still required. Do not self-approve.

Next action

Prefer #601 for this omit. Close this stack and #595 once a non-author review lands on #601. Seed the provider-host allowlist in KV after #611, not via a live env edit.

Open in Web View Automation 

Sent by Cursor Automation: Fix Issues

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before 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 head e58cb2fa424ef6bc6bbe1113bba6c3ade1c51dcb.

  • Head SHA: e58cb2fa424ef6bc6bbe1113bba6c3ade1c51dcb

  • Workflow run: 32098279995

  • 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"]
Loading

@opencode-agent

Copy link
Copy Markdown
Contributor

OpenCode Review Overview

  • Head SHA: e58cb2fa424ef6bc6bbe1113bba6c3ade1c51dcb
  • Workflow run: 32098279995
  • Workflow attempt: 1
  • Gate result: REQUEST_CHANGES (approval step)

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 success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before 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 head e58cb2fa424ef6bc6bbe1113bba6c3ade1c51dcb.

  • Head SHA: e58cb2fa424ef6bc6bbe1113bba6c3ade1c51dcb

  • Workflow run: 32098279995

  • 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"]
Loading

@opencode-agent
opencode-agent Bot disabled auto-merge August 18, 2026 04:39
@seonghobae seonghobae closed this Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants