fix(api): cap tool.function.description at 1024 characters fail-closed - #602
fix(api): cap tool.function.description at 1024 characters fail-closed#602seonghobae wants to merge 10 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.
|
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 141 files, which is 41 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 (141)
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.
Unique-tip review (55d7a45 only)
Scope is the unique commit on #595 (5dace0f), not the 141-file honesty stack vs main. CodeRabbit CLI 0.7.3 auth login --agent timed out on the browser callback; the GitHub CodeRabbit check skipped this PR (141 files over the 100-file limit). This is a unique-tip review, not a CodeRabbit result.
Strengths
- The cap is a real fail-closed check in
_validate_chat_tools(contextual_orchestrator/server.py:2631-2637):isinstance(description, str) and len(description) > 1024raisesRequestError(400, "invalid_tools", "each tool.function.description must be at most 1024 characters"). The named code isinvalid_tools, notunknown_fields. - Boundary is correct: 1024 is accepted (
>not>=). Chat HTTP tests intests/test_tool_description_length_http_honesty.pyprintedokon this SHA. /v1/responsesuses the same validator (server.py:4141-4142). Live probe on this tip: Responsesdescription×1024 → 200; ×1025 → 400invalid_toolswith the 1024 message. Chat-shaped Responses is a live reject, not claimed-only.- Legacy Completions and
functionsdo not need a parallel length check. Live probe: Completions + over-long description → 400invalid_tools(tools unsupported on/v1/completions); chatfunctions+ over-long description → 400invalid_functions(migrate totools). - Placement is after the parent null/type skip, so
description: nullstill follows the #595 type-check path and is not treated as over-long.
Critical
- Do not merge this 141-file stack onto
main. This PR is a re-ship of the honesty stack plus one 7-line check. Inherited merge-blockers from the #595 substrate remain (including accept-not-omit for JSON-nulldescription/parameters/strict). The landing vehicle is the current honesty tip, not this head.
Important
- Unique-tip tests cover chat only. Responses is implementation-covered and live-verified here, but
tests/test_tool_description_length_http_honesty.pynever hits/v1/responses. Add 1024-accept / 1025-reject Responses cases when landing so the shared path cannot regress. - Cherry-pick is not a clean apply onto #601 (
7e9a339) or the current tip #606 (6372c25). Those heads replaced the inline description type-check with_omit_null_tool_function_field. Insert thelen > 1024check after that helper (null is popped, soget("description")is then a string or absent). - The unique commit adds no docs.
docs/on this substrate does not mention the 1024 contract. When landing, add one sentence next to the existing tools-path honesty notes indocs/rest_api_design.md/ README so the claimed OpenAI-style cap is buyer-visible.
Minor
- Accept test asserts status 200 only; it does not echo that the 1024-character description was forwarded untruncated. There is no truncate path in this hunk, so this is coverage polish, not a live-honesty hole.
- Current
openai/openai-openapiFunctionObject.descriptionistype: stringwith nomaxLength: 1024(name still documents 64). 1024 is a conservative fail-closed cap, consistent with the PR claim, not a byte-for-byte copy of the live OpenAPI schema. - Completions / legacy
functionscorrectly fail at the surface (invalid_toolsunsupported /invalid_functions). No second length check is required unless those surfaces start accepting function tools.
Recommendations
- Do not merge #602. Do not open a third description-length stack PR.
- Port the 7-line check (plus Responses HTTP cases and one docs sentence) onto the current honesty tip. Template asked about #601: the hunk is Yes to cherry-pick, with the omit-helper adapt above. Concurrent stack state: #601 is absorbed; prefer #606 (
6372c25) over extending #601 or #608 in parallel. - After porting, keep
invalid_toolsand the exact 1024/1025 boundaries.
Assessment
| Question | Verdict |
|---|---|
| Unique tip real (live reject) vs claimed-only? | Real. Chat tests pass; Responses live 1025 → 400 invalid_tools. |
| Responses path covered? | Yes in code (shared _validate_chat_tools). No in unique-tip tests. Live-verified this run. |
| Ready to merge the STACK? | No. |
| Ready to cherry-pick unique tip onto #601? | Yes (adapt after _omit_null_tool_function_field). Prefer landing on #606, not a parallel #601 merge. |
Do not approve or merge from this review.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Unique tip is real. Do not merge this 141-file stack.
Next action: keep #602 open only as a cherry-pick source. Port the 7-line len(description) > 1024 check (plus Responses HTTP cases and one docs sentence) onto the current honesty tip that already has _omit_null_tool_function_field (≥ #603 / the #606–#608 family). Do not add more commits here, and do not open another 141-file description-length PR.
CodeRabbit GitHub skipped this PR (141 files over the 100-file limit). CodeRabbit CLI 0.7.3 is installed here but not authenticated. This is a unique-tip review from source + live HTTP, not a CodeRabbit result.
Unique tip (55d7a45 vs #595 5dace0f) — claimed vs live
| Claim | Live on this SHA |
|---|---|
Cap tools[].function.description at 1024, fail-closed invalid_tools |
Real. _validate_chat_tools (server.py ~2631–2637) uses > so 1024 is accepted. |
Chat HTTP 1024 → 200, 1025 → 400 invalid_tools |
Real. python3 tests/test_tool_description_length_http_honesty.py printed ok. Live chat 1025 → 400 named invalid_tools (not unknown_fields). |
| SDKs never see a truncated description | Real on accept. Chat 1024 echo still contains the full 1024-character string. |
| Responses covered | Real in code (same _validate_chat_tools at the Responses tools gate). Live /v1/responses 1024 → 200, 1025 → 400 invalid_tools. Missing from unique-tip tests (chat only). |
| Docs / APA contract | Absent. No docs/ mention of the 1024 cap on this substrate. |
Completions + over-long tools correctly dies at the unsupported-tools surface (invalid_tools). Legacy functions dies at invalid_functions (migrate to tools). No second length check is required on those surfaces.
The 1024 bound is a conservative fail-closed cap (OpenAI-style name already caps at 64 in this validator). Current openai/openai-openapi FunctionObject.description is type: string without maxLength: 1024 — keep the named reject, and write that as a gateway contract when landing, not as a byte-for-byte OpenAPI copy.
Inherited merge-blockers still live on this head
This PR is #595 + 7 lines. Live tools-path probes on 55d7a45 still return 200:
- empty / missing
messages role: developer- message
weight: 0.5andprefix: true - omitted
model stream: true→ JSON 200 (not SSE, not fail-closed)stream_options: {unknown_flag: null}seed: 1,temperature: 99description/parameters/strictJSON-null are accept-not-omit: echo still containsdescription: null,parameters: null,strict: null(same hole as #595; the real omit-pop is #603e58cb2f)
Do not merge #582–#602 current heads as parallel stacks. Independent non-author APPROVE + Full unit / Semgrep are still required on the landing vehicle. This automation will not approve or merge.
Landing
- Cherry-pick is not a clean apply onto heads that already replaced the inline description type-check with
_omit_null_tool_function_field. Insertlen > 1024after that helper (null is popped, soget("description")is then a string or absent). - Add
/v1/responses1024-accept / 1025-reject HTTP cases so the shared path cannot regress. - Add one buyer-visible sentence next to the existing tools-path honesty notes (
docs/rest_api_design.md/ README) stating the 1024 fail-closed contract. - Prefer extending the current honesty tip over landing this stack or opening a third description-length PR.
Ready to merge this stack? No.
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 head55d7a45786fef60064e491b470b4bb469f34167c. -
Head SHA:
55d7a45786fef60064e491b470b4bb469f34167c -
Workflow run: 32095092728
-
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.descriptionat 1024 characters (OpenAI-style).invalid_tools.Test plan
tests/test_tool_description_length_http_honesty.pypython -m pytest tests -q(978 passed)