fix(api): hoist message-name omit/validate before tools passthrough - #623
fix(api): hoist message-name omit/validate before tools passthrough#623cursor[bot] wants to merge 12 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.
OpenAI-style tool descriptions are at most 1024 characters. Over-long descriptions fail closed with named invalid_tools so SDKs never believe a truncated description was accepted.
SDK optional participant name blanks ("" / whitespace) are omit-equivalent
like JSON null. Non-string, over-long, and invalid charset names remain
fail-closed with invalid_message_name.
Blank SDK name defaults are popped in place so omit is real on both orchestration and tools paths. Non-string, over-long, and invalid charset names fail closed with invalid_message_name before proxy. 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.
Unique tip is sound — do not merge the stack yet
3f3f8ef does what the title claims. _omit_or_validate_message_name pops None / "" / whitespace in place, including Unicode whitespace via str.strip(). _validate_chat_message_name runs at server.py:3609 on the same body that proxy_completion forwards at :3647. Non-string, over-long, charset-invalid, and non-blank role=tool names fail closed with invalid_message_name on both orchestration and tools/passthrough paths (including response_format / tool_choice triggers).
Helper tests prove the pop. Tools-path HTTP tests prove the 400 cases. Docs (docs/rest_api_design.md, docs/architecture.md) and the APA 7th OpenAI chat-completion cite in docs/papers/README.md match the omit/fail-closed contract.
Next action for the landing reviewer
- Prefer this hoist over #614. Do not merge #614.
- Independent non-author APPROVE is still required (reviewer
seonghobaeis already requested). - Do not merge this 147-file honesty stack onto
mainin parallel with #621, #624, #625, #628, or #629. - Pending GitHub Checks are not a product blocker. A concrete failing job is.
Residual (non-blocking)
- Tools-path HTTP 200 cases still prove accept, not omit —
_mock_rawdoes not echomessages. - Charset predicate is
str.isalnum()while docs/error text say[a-zA-Z0-9_-]. _validate_chat_message_nameskips non-list / non-dictmessages(pre-existing tools-path shape gap). Do not widen this PR to fix that.
Do not open a third empty-name PR. Land this unique tip after an independent APPROVE, then rebase dependents onto the merge result.
Sent by Cursor Automation: Fix Issues
| "tools": _TOOLS, | ||
| }, | ||
| ) | ||
| assert status == 200, (value, body) |
There was a problem hiding this comment.
HTTP 200 here still only proves accept. _mock_raw echoes model / tools / tool_choice / sampling keys, not messages, so this test cannot see whether blank name was popped from the proxied body.
Helper tests already prove the in-place pop. If you want omit-honest HTTP coverage, echo messages from the mock and assert "name" not in body["echo"]["messages"][0] on these 200 cases.
Do not treat this as a tools-path smuggle. The hoist at server.py:3609 is sound.
| "tools": _TOOLS, | ||
| }, | ||
| ) | ||
| assert status == 200, body |
There was a problem hiding this comment.
Same accept≠omit gap for blank name on role=tool. Status 200 is necessary; it is not sufficient to prove the key was stripped before proxy_completion.
Keep the 400 cases (123, 65-char, bad name!, ZWSP, non-blank tool-role name). Those already fail closed with invalid_message_name.
| "invalid_message_name", | ||
| "message name must be at most 64 characters", | ||
| ) | ||
| if not all(ch.isalnum() or ch in "_-" for ch in msg_name): |
There was a problem hiding this comment.
str.isalnum() is wider than the documented [a-zA-Z0-9_-] class and the error string on the next lines. Unicode letters such as café are accepted and forwarded. This predicate was hoisted as-is from #614 — not a new tools-path hole.
Pick one contract: re.fullmatch(r"[A-Za-z0-9_-]+", msg_name) to match the docs, or change the docs/error text to Unicode alphanumeric plus _/-. Add a café case either way. Do not widen this PR to re-validate full message shape.
![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 head3f3f8ef22d886eba39d2fe3d4a0b8cf10101f687. -
Head SHA:
3f3f8ef22d886eba39d2fe3d4a0b8cf10101f687 -
Workflow run: 32138303634
-
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 head3f3f8ef22d886eba39d2fe3d4a0b8cf10101f687. -
Head SHA:
3f3f8ef22d886eba39d2fe3d4a0b8cf10101f687 -
Workflow run: 32148163640
-
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
namenull/empty/whitespace as omit-equivalent (same as fix(api): treat chat message name empty/whitespace as omit #6144f84700)._omit_or_validate_message_name/_validate_chat_message_namebefore toolsproxy_completionso non-string, over-long, and charset-invalid names cannot smuggle on the tools path._validate_messagesomit assertions. Docs:docs/rest_api_design.md,docs/papers/README.md(APA 7th OpenAI chat-completion citation),docs/architecture.md.Why this PR
#614 unique tip is sound on the orchestration helper, but tools passthrough still returned 200 for
name: 123/ 65-char / bad charset. Buyers must getinvalid_message_nameon both paths.Test plan
python3 tests/test_message_name_empty_omit_http_honesty.pypython3 tests/test_message_name_tools_passthrough_http_honesty.pypython3 tests/test_chat_message_name_http_honesty.pypython3 tests/test_message_name_null_noop_http_honesty.pypython3 tests/test_paper_contracts.pypython3 tests/test_api_contract.pypython3 tests/test_conventions.pypython3 tests/test_self_check.pyLanding
Prefer this hoist over merging #614 as a 142-file stack. Independent non-author APPROVE required. Do not merge in parallel with other honesty-stack PRs onto
main.