fix(api): treat tool.function description/parameters null as omit - #595
fix(api): treat tool.function description/parameters null as omit#595seonghobae wants to merge 9 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.
…or Responses parallel true SDK optional defaults often send function.strict and json_schema.strict as null — treat as omit rather than type errors. Align Responses parallel_tool_calls=true with chat by requiring a non-empty tools array.
SDK optional defaults often send description and parameters as JSON null. Treat null as omit rather than type errors; non-null non-string/object values remain fail-closed with invalid_tools.
|
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. |
|
Important Review skippedToo many files! This PR contains 140 files, which is 40 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (140)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Verdict: COMMENT
Would be REQUEST_CHANGES if GitHub allowed that event on an author-owned PR. Do not merge #595 onto main.
Merge-safe? no
The unique tip is accept-not-omit: description: null / parameters: null skip the type check and stay on the body that proxy_completion forwards. Mock 200 hides provider-hop rejection. The 140-file stacked diff also still carries the inherited tools-passthrough and stream_options unknown-null fail-opens.
Scope reviewed: unique tip 5dace0f only (2 files, +202/−2) plus live HTTP probes of claimed omit behavior and known inherited residuals at HEAD. Stack vs main (6841b71) is 9 commits / 140 files; that inherited surface is merge-unsafe even where this tip is locally correct. CodeRabbit CLI was not installed in this environment (command not found); this review is from the tip diff, validators, honesty tests, and live mock HTTP.
Unique tip: claimed vs real
| Claim | Evidence at 5dace0f |
|---|---|
tools[].function.description: null and parameters: null are omit-equivalent |
Partial. Validators no longer 400. They do not pop the keys. Live mock echo.tools still contains description: null, parameters: null, and (inherited) strict: null on both /v1/chat/completions and /v1/responses. |
Non-null wrong types stay fail-closed with named invalid_tools |
Real. description: 123 and parameters: "not-an-object" return 400 invalid_tools (chat test + live Responses description: 123). |
| HTTP honesty tests cover the contract | Status-only. tests/test_tool_description_parameters_null_noop_http_honesty.py printed ok. Every accept case asserts status == 200 and never inspects echo. |
_validate_chat_tools mutates nothing. Its return value is discarded at contextual_orchestrator/server.py:3566 (chat) and :4135 (Responses). TaskOrchestrator.proxy_completion copies body minus orchestration-only keys and ModelClient._mock_raw echoes tools verbatim (orchestrator.py:450-454, :967-976). That is fail-open to real providers: several OpenAI-compatible backends reject parameters: null / description: null as non-objects.
No docs/CHANGELOG/README update in the tip. docs/rest_api_design.md and README.md still do not mention tool description/parameters null-as-omit (or tools honesty at all).
Concrete bugs (HEAD lines)
contextual_orchestrator/server.py:2612-2619—parameters is not Noneskips the object check but leavesparameters: nullonfunction. Not omit-real.contextual_orchestrator/server.py:2621-2630— same fordescription: null.contextual_orchestrator/server.py:2581-2589(inherited #590 tip, still at this HEAD) —strict: nullaccepted, not popped. No_normalize_optional_strict_flagon this stack (512d5b8is not included).contextual_orchestrator/server.py:3566/:4135—_validate_chat_tools(body)return discarded; originalbodyis proxied.tests/test_tool_description_parameters_null_noop_http_honesty.py:75,:100,:134— 200-only asserts. The combined-null case even sendsstrict: Noneand would pass while echo still carries all three nulls.
Missing tests
- Mock-echo (or equivalent) assertion that null
description/parameters/strictkeys are absent after validation — the omit-real contract this series used on later #590 repair. - Responses-path accept + echo for the same nulls (validator is shared; forwarding bug is shared; untested).
- Docs/API-contract coverage if this repo still requires
docs/*.mdupdates for honesty behavior. - No coverage that
{unknown_flag: null}/ tools+prefix / tools+developer stay fail-closed — those are inherited, but this PR advertises a merge ontomain.
Inherited merge-blockers still at HEAD (live HTTP on this process)
| Probe | Result | Contrast |
|---|---|---|
tools + prefix: true |
200 | same message without tools → 400 invalid_message_prefix |
tools + weight: 0.5 |
200 | without tools → 400 invalid_message_weight |
tools + role: developer |
200 | without tools → 400 invalid_message_role |
stream_options: {unknown_flag: null} |
200 | nulls dropped before unknown-key check (server.py:1074-1077) |
tools + that stream_options |
200 | same |
Responses-native {type,name,parameters,strict} |
400 invalid_tools |
chat-shaped type+function still required |
Cause: _validate_messages (prefix/weight/developer) runs only after the PASSTHROUGH_TRIGGER_KEYS early-return. Pre-passthrough helpers (_validate_chat_message_known_fields, _validate_chat_message_audio_function_call) do not hoist those value checks. weight/prefix are in ALLOWED_MESSAGE_KEYS, so they pass the known-field gate.
Recommended next successor slice
Do not merge this 140-file stack. Do not open another competing honesty-stack PR.
Prefer the current honesty vehicle (#597, built on #592's unknown-null + message-honesty hoist). If this tip is still wanted, add a thin commit there that:
- Pops
description/parameters/strictwhen JSON-null (in-place on thefunctiondict, same pattern as #590's later_normalize_optional_strict_flag). - Asserts omit-real via mock
echo.toolson chat and Responses. - Leaves non-null wrong types on
invalid_tools. - Documents the omit in
docs/rest_api_design.md(and README if that surface lists chat tools).
Independent non-author APPROVE is still required on whatever lands. Do not self-approve.
Inline comment targets (unique-tip RIGHT-side lines)
Not posted as GitHub inline comments: this PR diff is +23325 lines and the review API rejects inlines past ~20k. Use these if a later thin PR re-opens the hunks:
contextual_orchestrator/server.py:2614—if parameters is not None and not isinstance(parameters, dict):— accept-not-omit; popNonebefore proxy.contextual_orchestrator/server.py:2623—and function.get("description") is not None— same for description.tests/test_tool_description_parameters_null_noop_http_honesty.py:75—assert status == 200, body— also assert"description" not in echo["tools"][0]["function"].tests/test_tool_description_parameters_null_noop_http_honesty.py:100— same forparameters.tests/test_tool_description_parameters_null_noop_http_honesty.py:134— combined-null +strict: None; assert all three keys absent in echo.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: COMMENT
Do not merge #595 at 5dace0f. The unique tip is accept-not-omit, not omit-real.
Unique tip vs claimed contract
tools[].function.description: null and parameters: null no longer 400. The keys stay on the body that proxy_completion forwards. Mock 200 hides provider-hop rejection: live echo.tools still contains description: null, parameters: null, and inherited strict: null on chat and Responses. Non-null wrong types correctly stay invalid_tools.
_validate_chat_tools does not pop. Its return is discarded. Tests assert status == 200 only.
Inherited merge-blockers still at this head
This 140-file stack does not include #592's repairs. Live HTTP on 5dace0f still returns 200 for tools+prefix: true, tools+weight: 0.5, tools+developer, and stream_options: {unknown_flag: null}.
Successor
Prefer #603 (e58cb2f on the #592 substrate). That tip pops JSON-null description / parameters / strict in place and asserts omit-real via mock echo.tools on chat and Responses. Keep #595 closed or superseded once #603 is independently approved.
Independent non-author APPROVE is still required on whatever lands. Do not self-approve.
Next action
Review and land #603 for this slice. Do not add another competing honesty-stack PR for the same null-field accept.
Sent by Cursor Automation: Fix Issues
![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 head5dace0fb504fd6c291f9923847fdef500ccb5b88. -
Head SHA:
5dace0fb504fd6c291f9923847fdef500ccb5b88 -
Workflow run: 32086390944
-
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"]
|
1 participant
Summary
tools[].function.description: nullandparameters: nullare omit-equivalent (SDK optional defaults).invalid_tools.Test plan
tests/test_tool_description_parameters_null_noop_http_honesty.pypython -m pytest tests -q(976 passed)