refactor(protocols): restore Chat/Responses ToolChoice separation invariant - #1314
Conversation
…ariant
Gemini code-assist reviewer flagged that the original over-prune lost a
real architectural invariant on `ResponsesToolChoice`: that it must NOT
share common.rs with Chat Completions's `ToolChoice`, because Chat uses
a different `Function` wire shape and does not accept
`Types`/`Mcp`/`Custom`/`ApplyPatch`/`Shell`. Merging the two would let
`/v1/chat/completions` silently accept Responses-only variants.
Restores this as a concise `NOTE:` paragraph (5 lines vs. the original
6), per the task's PRUNE rule ("shorten without deleting"). The
audit-process narrative ("deliberately does NOT live in common.rs") is
still out; only the invariant remains.
No semantic changes. Tests still pass (94/94).
Addresses: PR #1310 gemini-code-assist review comment at responses.rs:176
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA documentation note was added to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Summary
Follow-up to #1310. Restores an architectural invariant that was over-pruned in that PR: the `NOTE` explaining that `ResponsesToolChoice` must remain separate from `common::ToolChoice` because Chat Completions has different `Function` wire shape and does not accept Responses-specific variants (`Types`/`Mcp`/`Custom`/`ApplyPatch`/`Shell`).
gemini-code-assist raised this as a medium-priority concern on #1310 after that PR was already merged, flagging that losing this context could let future refactors merge the two enums and silently accept spec-invalid payloads on `/v1/chat/completions`.
Why
The original paragraph in #1310 was dropped as "deliberately" / "PR-body-in-source" prose. But it also carried a real invariant a future reader would miss. Per C2's own PRUNE rule ("shorten without deleting"), the correct outcome is a condensed NOTE that keeps the invariant and drops the audit framing.
What
One 5-line `NOTE:` block added above the `ResponsesToolChoice` derives:
```rust
/// NOTE: kept separate from `common::ToolChoice`. Chat Completions uses
/// a different `Function` wire shape (nested `{"function": {"name": ...}}`)
/// and does not accept the `Types` / `Mcp` / `Custom` / `ApplyPatch` /
/// `Shell` variants; sharing one enum would let `/v1/chat/completions`
/// silently accept spec-invalid payloads.
```
Acceptance
Refs: #1310
Summary by CodeRabbit
Note: This release contains no user-visible changes or functional updates.