fix(server,xai): enforce the request's tool selection after a response repair - #5376
Conversation
…tool selection The sparse-terminal repair rebuilds a terminal output from the output_item.done events it collected, and it received a budget but nothing about what the request had actually selected. The undeclared-tool guard answers a different question -- whether a name was declared -- so a request sent with tool_choice: "none", with a forced selector naming another tool, or with an allowed_tools list that excludes the call still received that call back through the repair. Read the boundary from the final outbound body, after every removal, rename and translation, and apply it to what the repair publishes. A catalog that ends up empty there authorizes no client call whatever the selector still says; an absent catalog states no boundary, exactly as it states none for the declaration guard. Keep the failure narrow and visible. Only the offending item is withheld, so the assistant text that arrived in the same turn still reaches the client instead of being discarded with it, and the withheld position is kept so the contiguity proof still covers the whole output. Because the turn no longer ended the way the upstream said it did, the reconstructed terminal is published as response.incomplete with incomplete_details.reason forbidden_tool_call rather than as a clean response.completed with a quietly shorter output. The raw stream is still forwarded untouched; policing it remains the declaration guard's job. Co-authored-by: luvs01 <luvs01@users.noreply.github.com>
xAI rejects a Responses request whose tool_choice survives a catalog the adapter had to empty, which is what happens to a cached-only web-search declaration: it is omitted rather than widened to live search, and the request then selects from a catalog it no longer has. Omit an auto or none selector once no tool remains in either the top-level catalog or additional_tools. A forced function selector is preserved: a selector this proxy cannot honor is a client input error, and the request-build path already answers it with a 400 rather than silently turning "call this tool" into "answer however you like". This is the outbound half of the same rule the response-side repair applies -- compatibility is judged on the final request and the final response, after every removal, rename and translation. The change is carried unmodified from #5350. Co-authored-by: Yeonwoo Choi <32544727+twoimo@users.noreply.github.com>
The Responses transport document described declaration enforcement and said nothing about selection, so the next person to touch this area would have read the undeclared-tool guard as the whole contract. Write down the boundary between the two questions, where the scope is read from, and why the refusal keeps the text and marks the terminal instead of finishing quietly.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ Deterministic PR hygiene checks passed. |
|
Warning Review limit reachedNext included review available in 31 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 377269aa23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : undefined, | ||
| grokClientCompatibilityEnabled | ||
| ? createGrokResponsesSparseTerminalBlockRewrite(translatorBudget) | ||
| ? createGrokResponsesSparseTerminalBlockRewrite(translatorBudget, nativeExchange.outboundRequestBody) |
There was a problem hiding this comment.
When a declared tool is excluded by tool_choice (for example, declared apply_patch with tool_choice: "none") and the provider sends its output_item.done followed by a sparse completed terminal, this rewrite emits an incomplete terminal without the call, but the parallel raw-stream inspector still reconstructs that call and passes it to rememberPassthroughResponseChecked. Because that persistence guard checks only whether the tool was declared, the refused turn enters previous_response_id replay state as completed and later continuations receive a tool call the client terminal never contained or answered. Propagate the selection refusal to inspection/persistence or apply the same scope before storing the response.
Useful? React with 👍 / 👎.
| ? item.namespace | ||
| : undefined; | ||
| if (!namespace) return [name]; | ||
| return [name, namespacedToolName(namespace, name), dottedToolName(namespace, name)]; |
There was a problem hiding this comment.
With two declared tools such as docs.search and admin.search, forcing {type: "function", namespace: "docs", name: "search"} also authorizes an emitted {namespace: "admin", name: "search"} because both callNameSpellings and selectorNameSpellings include the bare spelling search. The declaration guard permits both names because both tools are declared, so the sparse repair can reconstruct the explicitly unselected admin call. Qualified identities should match by their canonical namespace/name, with bare aliases resolved only when the catalog proves them unambiguous.
Useful? React with 👍 / 👎.
| const names = new Set(selectorNameSpellings(choice)); | ||
| return names.size > 0 ? { kind: "allow", names } : UNRESTRICTED; | ||
| } | ||
| return UNRESTRICTED; |
There was a problem hiding this comment.
When tool_choice forces a hosted tool such as {type: "web_search"} and the catalog also contains a client function, returning UNRESTRICTED allows a sparse terminal to reconstruct that function call even though the request selected web search specifically. This also disagrees with the allowed_tools branch immediately above, where a hosted-only selection correctly denies every client call. Treat a forced non-client selector as deny-all for CLIENT_EXECUTED_CALL_TYPES rather than as unrestricted.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 64 / 80이 PR은 Grok 쪽 “빈 terminal을 다시 만드는 수리”가 라인 - 라인 - 라인 - 라인 - 재구성 terminal은 호출을 빼는데 raw 라인 - PR 본문도 말했듯 non-sparse(upstream이 이미 output을 채운 terminal)에는 이 경계가 안 걸린다. sparse만 고친 범위로는 이해되지만, 같은 메인테이너의 판단이 필요한 지점 CI 빨간 compat 테스트를 “새 동작이 맞고 테스트를 고친다”로 볼지, “ 너의 추천 머지하지 말고, 먼저 이 댓글은 grok-bot이 작성했습니다 |
… removes Omitting an auto/none selector that normalization left with nothing to select keeps xAI from answering 400, but the two words are not interchangeable. auto selects from the catalog, so removing it from a request with an empty one states nothing new. none is a prohibition, and on a request whose catalog this normalizer just emptied it is the only place the turn's client-call boundary is written down. The sparse-terminal repair reads that boundary from the final outbound body, so dropping the word alone handed the reconstruction a request that authorized more than the caller did -- and nothing behind it catches that: the repair runs on the grok client surface while the declaration guard stands down whenever the provider's authMode is forward, which is what the xAI OAuth lane is. A caller who forbade every client tool could get one back inside a terminal the upstream never sent. Restate the prohibition as the explicit empty catalog. It carries the same deny-all, the request scope and the declaration guard both already read it that way, and this destination receives it unchanged whenever a caller sends one itself. auto is still dropped without inventing a catalog, because an absent catalog states no boundary. Co-authored-by: Yeonwoo Choi <32544727+twoimo@users.noreply.github.com>
1 participant
Summary
createGrokResponsesSparseTerminalBlockRewriterebuilds a terminaloutputfrom theoutput_item.doneevents it collected, and it received a translator budget but nothing about what the request had selected. The undeclared-tool guard answers a different question — whether a name was declared — sotool_choice: "none", a forced selector naming another tool, and anallowed_toolslist that excludes the call all passed it. Trigger: a Grok-compat destination answers with the durable items inoutput_item.doneand an empty terminaloutput. Before, the reconstructed terminal carried{"type":"function_call","name":"apply_patch"}for a request sent withtool_choice: "none"; after, that call is left out of the reconstruction.src/server/responses-request-tool-scope.ts. A catalog that ends up empty there authorizes no client call whatever the selector still says. An absent catalog states no boundary, exactly as it states none for the declaration guard, so a passthrough request that omitstoolsand receives a call the client understands is unaffected.response.incompletewithincomplete_details.reason: forbidden_tool_calland a bounded message naming the tool, rather than as aresponse.completedwith a quietly shorter output. The repair edits nothing but the terminal it synthesizes; the raw stream is still forwarded untouched and remains the declaration guard's to police.src/adapters/xai-web-search.ts: a selector that normalization left with nothing to select is omitted, because xAI answers that request with a 400 — which is what happens to a cached-only web-search declaration that is dropped rather than widened to live search. A forced function selector is preserved, because a selector this proxy cannot honor is a client input error the request-build path already answers with a 400.autoandnoneare not the same omission, and that distinction is this lane's change to the carried one.autoselects from the catalog, so removing it from a request with an empty catalog states nothing new.noneis a prohibition, and on a request whose catalog the same normalizer just emptied it is the only place the turn's client-call boundary is written down. Dropping the word alone left the scope above reading a body that authorized more than the caller did, and nothing behind it catches that: the repair runs on the grok client surface while the declaration guard stands down whenever the provider'sauthModeisforward, which is what the xAI OAuth lane is. The prohibition is therefore restated as the explicit empty catalog, which carries the same deny-all, which the request scope and the declaration guard both already read that way, and which this destination receives unchanged whenever a caller sends one itself.autois still dropped without inventing a catalog the caller never declared.Co-authored-bytrailer; theauto/nonesplit above is the part added here.Co-authored-bytrailer; the enforcement it added is extended so one forbidden call no longer costs the caller the accompanying text, and so a stream the repair will not reconstruct faithfully says so on the terminal.structure/transports/responses.mdrecords the split between the declaration question and the selection question, where the scope is read from, why the refusal is narrow and visible, and why one of the two omitted selectors has to be restated.Verification
test 1/4failed ontests/providers/xai/xai-web-search-compat.test.ts, where a cached-only search request normalized totool_choice: "none"and the new omission erased it, leaving an outbound body that stated no boundary at all. That is the regression the fifth bullet describes and fixes; the assertion was updated only to record the new wire shape, together with the reason.undefinedfor a request that restricts nothing, so an ordinary turn keeps the exact path it had; the repair's retain/withhold bookkeeping shares oneMAX_COMPLETED_OUTPUT_ITEMSbound and releases every charged byte through the existingreset/dispose; a withheld index participates in the contiguity proof but never fills a gap the stream actually left; and the refusal rewrites theevent:line together with the payloadtypeso the two cannot disagree. The restated catalog is the byte shape this destination already receives from a caller who sendstools: []itself, so it introduces no wire form the xAI path had not been sending.tests/responses/responses-sparse-terminal-tool-scope.test.ts(twelve cases: refusal with and without surviving text, the untouched raw stream, a permitting selector, a forced selector for another tool, an allow-list, both flattened namespaced spellings, an emptied catalog with and without a selector, an absent catalog, a selection with nothing to refuse, a gapped stream, and budget release) andtests/providers/xai/xai-empty-catalog-tool-choice.test.ts(eight cases, including the restated prohibition, the caller's ownnoneon a request with no catalog,autoinventing no catalog, and one cross-layer case that drives the normalized body through the repair and asserts the forbidden call is withheld while the assistant text survives). Both are registered inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json. The tests import the terminal type, the reason and the message builder from the source rather than restating them.Checklist