feat(protocols): implement P7 ToolChoice variant coverage

[slin1237]

Summary

Implements audit task P7: widen ToolChoice from 3 variants to the 8 required by the OpenAI Responses API spec (Options, Types, Function, AllowedTools, Mcp, Custom, ApplyPatch, Shell). Adds single-value enum tag types to prevent cross-variant collision under #[serde(untagged)], and widens the Function variant to carry both flat (name) and nested (function) wire shapes for bidirectional spec compat.

What changed

Commits on branch (main..HEAD):

• a1a84aa3 — feat(protocols): implement P7 ToolChoice variant coverage (substance)
• 80099675 — style(protocols): rustfmt remediation for P7 (cycle 2) (fmt-only follow-up)

Diff totals: 8 files, +398 / -56.

• crates/protocols/src/common.rs (+319 / most lines): expanded ToolChoice from 3 → 8 spec variants; added 6 single-value enum tag types (FunctionToolChoiceTag, AllowedToolsToolChoiceTag, BuiltInToolChoiceType, McpToolChoiceTag, CustomToolChoiceTag, ApplyPatchToolChoiceTag, ShellToolChoiceTag) to prevent untagged-match collisions; added ToolChoice::function_nested() constructor and ToolChoice::function_name() accessor; 11 serde round-trip tests covering every new variant + the cross-variant collision regression case (tool_choice_mcp_does_not_collide_with_function).
• crates/protocols/src/chat.rs (+ / -): widened Function-variant validation to accept either wire shape via the new accessor; dropped now-unused FunctionChoice import.
• crates/protocols/src/responses.rs (+ / -): catch-all arm for new variants at the two exhaustive match tool_choice sites.
• model_gateway/src/routers/grpc/harmony/stages/preparation.rs (+13 / -): Ok(None) arm for the new variants (they don't drive harmony structural-tag generation).
• model_gateway/src/routers/grpc/regular/streaming.rs (+ / -): adapted two ToolChoice::Function { function, .. } destructuring sites to function_name() accessor.
• model_gateway/src/routers/grpc/utils/chat_utils.rs (+ / -): same accessor adaptation.
• model_gateway/src/routers/grpc/utils/message_utils.rs (+ / -): replaced one constructor with ToolChoice::function_nested(name).
• model_gateway/tests/spec/chat_completion.rs (+ / -): corrected six tests that constructed AllowedTools { tool_type: "function" } (a pre-existing typo hidden by the old permissive String field; the new enum-tagged type surfaced it) → AllowedToolsToolChoiceTag::AllowedTools; two Function constructors migrated to the new helper.

Why

OpenAI Responses API spec (see .claude/_audit/openai-responses-api-spec.md:409-425) defines 8 ToolChoice variants with strict per-variant field sets. smg previously modeled only 3 (Value, Function, AllowedTools); spec-valid clients sending {"type": "file_search"} as tool_choice (spec-valid ToolChoiceTypes) silently failed today, same for ToolChoiceMcp, ToolChoiceCustom, ToolChoiceApplyPatch, ToolChoiceShell. This PR closes the gap.

Verification

• cargo +nightly fmt --all -- --check silent (isolated CARGO_TARGET_DIR=/tmp/p7-c2-lead-target to avoid worktree cache collision)
• cargo clippy -p openai-protocol -- -D warnings clean
• cargo test -p openai-protocol → 93/0 (66 + 8 + 18 + 1 doc, all cycle-1 plus fmt delta)
• cargo test -p smg --test spec_test → 93/0, 1 ignored
• cargo test -p openai-protocol tool_choice → 22/22 (incl. regression anchors tool_choice_mcp_does_not_collide_with_function, tool_choice_function_nested_roundtrip)
• Tech Lead cycle-1 hand-built 20/20 spec fixtures (all 8 variants individually, plus Function flat/nested and Allowed shapes) — all byte-identical
• Cycle-2 fmt delta verified semantic-neutral line-by-line (no symbol rename, no literal change, no control-flow edit)
• function_nested helper has 2+ prod callsites; function_name has 6+ prod callsites → §7 "no new helper used by one callsite" passes
• Test-file change (chat_completion.rs) verified compile-forced (not unrelated cleanup) — the old String field hid a typo that the new typed enum surfaces at compile time
• Codex review: unavailable (harness skill permission; Tech Lead proceeded solo per playbook §8 fallback after cycle-1 spec fixtures + cycle-2 fmt delta both clean)

Blast radius

8 files, classified:

• Protocol schema (3 files — new types + accessor + widened variant): common.rs, chat.rs, responses.rs.
• Compile-forced call-site adaptation (4 files — accessor migration + exhaustive-match arms + spec-test fixture): routers/grpc (3), tests/spec/chat_completion.rs (1).
• Justification for the test-file edit: the pre-edit test constructed AllowedTools { tool_type: "function" } — a permissive-String bug that the old schema hid; the new typed enum tag (AllowedToolsToolChoiceTag::AllowedTools) would fail to compile with the old literal string. Documenting as forced, not cleanup.

Out of scope

• Explicit 400 invalid_tool_choice for unknown type strings — deferred to P5 (silent-swallow removal task) per audit guidance; #[serde(untagged)] kept here.
• T6/T7/T8 tool registration (separate audit tasks).

Refs: audit task P7 · .claude/_audit/responses-api-gap-audit.md

Summary by CodeRabbit

Release Notes

• New Features

  • Expanded tool selection options in the Responses API with enhanced support for granular control over tool availability across different scenarios, including function-specific selection and allowed tools configuration.

• Improvements

  • Strengthened validation of tool selection configurations for more reliable handling across all supported tool types.

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Introduced a new ResponsesToolChoice API specific to the Responses protocol with comprehensive enums for various tool choice configurations (none, auto, required, and object variants for function, allowed_tools, mcp, custom, apply_patch, shell, and hosted tools). Updated ResponsesRequest.tool_choice from the shared ToolChoice type and added projection logic to map to Chat API representation. These changes propagate through harmony preparation, router conversion, and test layers.

Changes

Cohort / File(s)	Summary
Protocol Definition crates/protocols/src/responses.rs	Introduced new ResponsesToolChoice enum with 8 variants, ToolChoiceOptions, and tool-specific tag enums. Added validation logic for tool existence and to_chat_tool_choice() projection method. Updated ResponsesRequest.tool_choice field type from Option<ToolChoice> to Option<ResponsesToolChoice>.
Harmony Preparation model_gateway/src/routers/grpc/harmony/stages/preparation.rs	Projects request.tool_choice to Chat format via to_chat_tool_choice() and uses the result for tool filtering and constraint generation instead of the original Responses variant.
Responses Conversion Layer model_gateway/src/routers/grpc/harmony/responses/common.rs, model_gateway/src/routers/grpc/regular/responses/conversions.rs	Updated to use ResponsesToolChoice::Options(ToolChoiceOptions::Auto) for tool selection and invoke to_chat_tool_choice() projection during request transformation.
Test Updates model_gateway/tests/spec/chat_completion.rs, model_gateway/tests/api/responses_api_test.rs, model_gateway/tests/spec/responses.rs	Updated test imports and ResponsesRequest.tool_choice construction from ToolChoice/ToolChoiceValue to ResponsesToolChoice/ToolChoiceOptions variants.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• refactor(e2e): reorganize Harmony tests, add validation, remove unused gateway args #796: Modifies Harmony preparation stage tool choice and constraint handling; related to how tool_choice is processed in the preparation flow.
• feat(harmony): support response_format in Chat Completions #791: Updates Harmony chat preparation flow's constraint generation logic; addresses tool constraint production alongside the main PR's tool_choice projection changes.

Suggested reviewers

• CatherineSue
• key4ng

Poem

🐰 A new ResponsesToolChoice hops into view,
With enums so clever and projections so true,
From Responses to Chat, the types dance with glee,
Tool filtering flows through each harmony spree! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title "feat(protocols): implement P7 ToolChoice variant coverage" directly and specifically summarizes the main objective: implementing expanded ToolChoice variant coverage (from 3 to 8 variants) as part of the P7 audit task. It is concise, clear, and accurately reflects the primary change across all modified files.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/audit-p7-tool-choice

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/protocols/src/responses.rs (1)

1077-1141: ⚠️ Potential issue | 🟠 Major

Expand existence checks beyond function refs.

Line 1077 onward only cross-checks function selections. A request can still force a non-function tool that is missing from request.tools—for example via ToolChoice::Mcp, a hosted ToolChoice::Types, or non-function entries inside AllowedTools—and this validator will accept it. That turns simple typos/mismatches into late routing failures instead of a request-time validation error.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/protocols/src/responses.rs` around lines 1077 - 1141, The validator
currently only checks function references (uses function_tool_names and
ToolReference::Function) so non-function tool refs (e.g., ToolChoice::Mcp,
ToolChoice::Types, hosted tools, or non-function entries inside AllowedTools)
can slip through; update the validation to also verify existence for every tool
reference type: in the AllowedTools loop check all ToolReference variants (not
just Function) against the canonical tool name set (the same set used for
function_tool_names or a unified tools_name_set), and add existence checks for
ToolChoice::Mcp, ToolChoice::Types, ToolChoice::Custom, ToolChoice::ApplyPatch,
and ToolChoice::Shell paths where they include tool identifiers, returning a
ValidationError (same style as existing errors like
tool_choice_tool_not_found/tool_choice_invalid_mode) when a referenced tool
name/id is not present. Ensure you reference the enums/variants ToolChoice,
ToolReference, function_tool_names (or a unified tools_name_set), allowed_tools,
and ValidationError when making the changes.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/protocols/src/chat.rs`:
- Around line 523-531: The validation wrongly treats all non-`none` ToolChoice
variants as requiring a `tools` array; update the check in the function that
calls tool_choice_requires_tools so it does not mark ToolChoice::Types,
ToolChoice::Mcp, ToolChoice::Custom, ToolChoice::ApplyPatch, or
ToolChoice::Shell as requiring tools. Concretely, adjust the predicate used by
tool_choice_requires_tools (or the call site before the validation) to return
false for those five variants so they pass through to the upstream provider when
no request-scoped `tools` are present, leaving other variants (including
function-invoking ones) to still require the `tools` array.

In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 1183-1198: The code currently calls
choice.function_name().unwrap_or_default() which turns a missing function name
into an empty string and allows generate_tool_call_id and add_choice_tool_name
to proceed with an invalid seed; instead, detect when function_name() is None
and fail closed: do not default to "", return/propagate an error or skip
emitting this tool-choice chunk. Update the block handling ToolChoice::Function
(the choice variable, fn_name extraction, utils::generate_tool_call_id and
ChatCompletionStreamResponse::add_choice_tool_name calls) to explicitly handle
None (e.g., early-return Err or skip with a clear log) rather than using
unwrap_or_default().

In `@model_gateway/tests/spec/chat_completion.rs`:
- Around line 176-177: Add a test case that covers the flat ToolChoice::Function
shape (top-level "name" field, not nested "function.name") so the
parser/validator exercise the flat Responses-style path; locate the pattern
match handling Some(choice @ ToolChoice::Function { .. }) in
model_gateway/tests/spec/chat_completion.rs (around the existing assertions at
the current match arms) and add an input example where the choice payload uses
the flat "name":"my_function" form, then assert choice.function_name() ==
Some("my_function") (do the same for the similar block around lines 283-313).

---

Outside diff comments:
In `@crates/protocols/src/responses.rs`:
- Around line 1077-1141: The validator currently only checks function references
(uses function_tool_names and ToolReference::Function) so non-function tool refs
(e.g., ToolChoice::Mcp, ToolChoice::Types, hosted tools, or non-function entries
inside AllowedTools) can slip through; update the validation to also verify
existence for every tool reference type: in the AllowedTools loop check all
ToolReference variants (not just Function) against the canonical tool name set
(the same set used for function_tool_names or a unified tools_name_set), and add
existence checks for ToolChoice::Mcp, ToolChoice::Types, ToolChoice::Custom,
ToolChoice::ApplyPatch, and ToolChoice::Shell paths where they include tool
identifiers, returning a ValidationError (same style as existing errors like
tool_choice_tool_not_found/tool_choice_invalid_mode) when a referenced tool
name/id is not present. Ensure you reference the enums/variants ToolChoice,
ToolReference, function_tool_names (or a unified tools_name_set), allowed_tools,
and ValidationError when making the changes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 11d263e4-777a-4c6d-9376-85afb45386b0

📥 Commits

Reviewing files that changed from the base of the PR and between 3fce285 and faee4fb.

📒 Files selected for processing (8)

• crates/protocols/src/chat.rs
• crates/protocols/src/common.rs
• crates/protocols/src/responses.rs
• model_gateway/src/routers/grpc/harmony/stages/preparation.rs
• model_gateway/src/routers/grpc/regular/streaming.rs
• model_gateway/src/routers/grpc/utils/chat_utils.rs
• model_gateway/src/routers/grpc/utils/message_utils.rs
• model_gateway/tests/spec/chat_completion.rs

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (2)

model_gateway/tests/spec/chat_completion.rs (1)

176-177: 🧹 Nitpick | 🔵 Trivial

Consider adding a test for the flat ToolChoice::Function wire shape.

The current tests only exercise the nested helper path (ToolChoice::function_nested). Since the validator now accepts both "name" (flat Responses-style) and "function.name" (nested), a regression in the flat form would not be caught by this suite.

Adding a test that deserializes {"type": "function", "name": "my_function"} and asserts choice.function_name() == Some("my_function") would provide coverage for both wire shapes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@model_gateway/tests/spec/chat_completion.rs` around lines 176 - 177, Add a
new unit test that deserializes the flat wire shape for ToolChoice::Function
(e.g. JSON {"type":"function","name":"my_function"}) and asserts that
choice.function_name() == Some("my_function"); place it alongside the existing
test that exercises ToolChoice::function_nested so both the nested
("function.name") and flat ("name") forms are covered, using the same
deserialization helper and match arm that checks Some(choice @
ToolChoice::Function { .. }) as in the current test.

model_gateway/src/routers/grpc/regular/streaming.rs (1)

1183-1197: ⚠️ Potential issue | 🟡 Minor

Don't default missing function names to an empty string.

Line 1184 uses choice.function_name().unwrap_or_default(), which silently converts a malformed ToolChoice::Function (missing both name and function.name) into an empty string. This allows the code path to proceed with an invalid tool name and generate a tool-call ID from an empty seed, rather than failing closed.

Consider returning early or logging a warning when function_name() returns None:

Suggested fix

         if let Some(choice @ ToolChoice::Function { .. }) = tool_choice {
-            let fn_name = choice.function_name().unwrap_or_default();
+            let Some(fn_name) = choice.function_name() else {
+                debug_assert!(false, "ToolChoice::Function missing name");
+                return chunks;
+            };
             let is_first_call = !has_tool_calls.contains_key(&index);
 
             if is_first_call {
                 // First chunk: send name and id
                 has_tool_calls.insert(index, true);
 
                 let tool_call_id =
-                    utils::generate_tool_call_id(model, fn_name, 0, history_tool_calls_count);
+                    utils::generate_tool_call_id(model, &fn_name, 0, history_tool_calls_count);
 
                 chunks.push(
                     ChatCompletionStreamResponse::builder(request_id, model)
                         .created(created)
-                        .add_choice_tool_name(index, tool_call_id, fn_name.to_string())
+                        .add_choice_tool_name(index, tool_call_id, fn_name.to_string())
                         .maybe_system_fingerprint(system_fingerprint)
                         .build(),
                 );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 1183 -
1197, Do not silently default a missing function name to "" in the
ToolChoice::Function branch: replace the unwrap_or_default() call on
choice.function_name() with an explicit check for None and bail out (or log and
skip) so you never call utils::generate_tool_call_id or
ChatCompletionStreamResponse::add_choice_tool_name with an empty name; in the
block handling is_first_call (where has_tool_calls, index, tool_call_id,
ChatCompletionStreamResponse::builder and add_choice_tool_name are used) return
early or emit an error chunk when function_name() is None to ensure
invalid/malformed ToolChoice::Function values fail closed.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/protocols/src/responses.rs`:
- Around line 1229-1235: The new match arms in ToolChoice (ToolChoice::Types,
ToolChoice::Mcp, ToolChoice::Custom, ToolChoice::ApplyPatch, ToolChoice::Shell)
skip validation and allow tool_choice values that aren't present in the provided
tools list; update the validation logic where tool_choice is matched to check
that the selected variant actually corresponds to an entry in tools (e.g., for
Types ensure the referenced type exists in tools, for Mcp match the server_label
against an MCP tool in tools, for Custom/ApplyPatch/Shell ensure a tool with
that identifier/type is present) and return an appropriate validation error when
no matching tool is found instead of falling through to the empty arm.

In `@model_gateway/src/routers/grpc/harmony/stages/preparation.rs`:
- Around line 336-343: The match arm that currently returns Ok(None) for
ToolChoice::Types, ToolChoice::Mcp, ToolChoice::Custom, ToolChoice::ApplyPatch
and ToolChoice::Shell must not silently drop a forced non-function tool
selection; instead detect these variants in the Harmony preparation path and
return a validation/error (not Ok(None)) so the gRPC router fails fast rather
than converting a forced tool choice into unconstrained generation. Locate the
match over ToolChoice in preparation.rs (the arm with ToolChoice::Types |
ToolChoice::Mcp | ToolChoice::Custom | ToolChoice::ApplyPatch |
ToolChoice::Shell) and replace the Ok(None) behavior with an explicit Err
variant (or a Result::Err with a clear validation error) that includes context
about the unsupported/non-function tool_choice in this path.

In `@model_gateway/src/routers/grpc/utils/chat_utils.rs`:
- Around line 431-440: The code currently uses unwrap_or_default() on
choice.function_name() in parse_json_schema_response which can create empty
function names and invalid IDs (e.g., functions.:0); change the logic in the
Some(choice @ ToolChoice::Function { .. }) branch to explicitly handle a missing
function name by failing closed: if choice.function_name() is None, return an
Err or skip creating the ToolCall rather than using an empty string. Update the
block that builds ToolCall/FunctionCallResponse (references: fn_name,
generate_tool_call_id, FunctionCallResponse) to only proceed when a non-empty
function name is present and propagate an appropriate error or skip result when
it is missing.

---

Duplicate comments:
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 1183-1197: Do not silently default a missing function name to ""
in the ToolChoice::Function branch: replace the unwrap_or_default() call on
choice.function_name() with an explicit check for None and bail out (or log and
skip) so you never call utils::generate_tool_call_id or
ChatCompletionStreamResponse::add_choice_tool_name with an empty name; in the
block handling is_first_call (where has_tool_calls, index, tool_call_id,
ChatCompletionStreamResponse::builder and add_choice_tool_name are used) return
early or emit an error chunk when function_name() is None to ensure
invalid/malformed ToolChoice::Function values fail closed.

In `@model_gateway/tests/spec/chat_completion.rs`:
- Around line 176-177: Add a new unit test that deserializes the flat wire shape
for ToolChoice::Function (e.g. JSON {"type":"function","name":"my_function"})
and asserts that choice.function_name() == Some("my_function"); place it
alongside the existing test that exercises ToolChoice::function_nested so both
the nested ("function.name") and flat ("name") forms are covered, using the same
deserialization helper and match arm that checks Some(choice @
ToolChoice::Function { .. }) as in the current test.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0b5fcad4-df51-4b32-a935-afbd92cf4f2b

📥 Commits

Reviewing files that changed from the base of the PR and between faee4fb and 6d4d513.

📒 Files selected for processing (8)

• crates/protocols/src/chat.rs
• crates/protocols/src/common.rs
• crates/protocols/src/responses.rs
• model_gateway/src/routers/grpc/harmony/stages/preparation.rs
• model_gateway/src/routers/grpc/regular/streaming.rs
• model_gateway/src/routers/grpc/utils/chat_utils.rs
• model_gateway/src/routers/grpc/utils/message_utils.rs
• model_gateway/tests/spec/chat_completion.rs

[coderabbitai]

♻️ Duplicate comments (2)

model_gateway/src/routers/grpc/regular/streaming.rs (1)

1183-1198: ⚠️ Potential issue | 🟡 Minor

Fail closed when ToolChoice::Function has no name.

Line 1184 turns a malformed function choice into "", which then flows into generate_tool_call_id() and add_choice_tool_name(...) as an invalid tool name.

Suggested direction

-        if let Some(choice @ ToolChoice::Function { .. }) = tool_choice {
-            let fn_name = choice.function_name().unwrap_or_default();
+        if let Some(choice @ ToolChoice::Function { .. }) = tool_choice {
+            let Some(fn_name) = choice.function_name() else {
+                debug_assert!(false, "ToolChoice::Function missing name");
+                return chunks;
+            };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@model_gateway/src/routers/grpc/regular/streaming.rs` around lines 1183 -
1198, The code currently converts a missing function name into an empty string
via choice.function_name().unwrap_or_default(), which then gets passed into
utils::generate_tool_call_id and add_choice_tool_name; instead, treat a
missing/empty function name as an error/invalid tool choice and bail out early:
replace the unwrap_or_default usage with a check that calls
choice.function_name() and if None or empty then skip creating the tool-call
chunk (or return an Err) so you never call utils::generate_tool_call_id or
ChatCompletionStreamResponse::add_choice_tool_name with an invalid name; update
the logic around ToolChoice::Function, has_tool_calls.insert(index, true),
generate_tool_call_id, and add_choice_tool_name to only run when a valid
non-empty fn_name is present.

model_gateway/src/routers/grpc/harmony/stages/preparation.rs (1)

336-343: ⚠️ Potential issue | 🟠 Major

Reject unsupported forced non-function tool_choice variants here.

Lines 339-343 currently accept a forced Types/Mcp/Custom/ApplyPatch/Shell request but emit no Harmony constraint, so the request silently degrades into unconstrained generation.

Suggested direction

-            ToolChoice::Types { .. }
-            | ToolChoice::Mcp { .. }
-            | ToolChoice::Custom { .. }
-            | ToolChoice::ApplyPatch { .. }
-            | ToolChoice::Shell { .. } => Ok(None),
+            ToolChoice::Types { .. }
+            | ToolChoice::Mcp { .. }
+            | ToolChoice::Custom { .. }
+            | ToolChoice::ApplyPatch { .. }
+            | ToolChoice::Shell { .. } => Err(Box::new(error::bad_request(
+                "unsupported_tool_choice",
+                "Harmony currently supports only function/required/allowed_tools tool_choice variants",
+            ))),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@model_gateway/src/routers/grpc/harmony/stages/preparation.rs` around lines
336 - 343, The match arm currently treating forced non-function tool choices
(ToolChoice::Types, ToolChoice::Mcp, ToolChoice::Custom, ToolChoice::ApplyPatch,
ToolChoice::Shell) as Ok(None) must instead reject them with an error; update
the match block so these variants return an Err containing a descriptive error
(e.g., invalid_argument/UnsupportedToolChoice or the module's existing
Preparation/validation error type) explaining that forced non-function
tool_choice variants are not supported, and ensure the error uses the same error
construction pattern used elsewhere in this file/function so callers get a
proper failure instead of silent unconstrained generation.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@model_gateway/src/routers/grpc/harmony/stages/preparation.rs`:
- Around line 336-343: The match arm currently treating forced non-function tool
choices (ToolChoice::Types, ToolChoice::Mcp, ToolChoice::Custom,
ToolChoice::ApplyPatch, ToolChoice::Shell) as Ok(None) must instead reject them
with an error; update the match block so these variants return an Err containing
a descriptive error (e.g., invalid_argument/UnsupportedToolChoice or the
module's existing Preparation/validation error type) explaining that forced
non-function tool_choice variants are not supported, and ensure the error uses
the same error construction pattern used elsewhere in this file/function so
callers get a proper failure instead of silent unconstrained generation.

In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 1183-1198: The code currently converts a missing function name
into an empty string via choice.function_name().unwrap_or_default(), which then
gets passed into utils::generate_tool_call_id and add_choice_tool_name; instead,
treat a missing/empty function name as an error/invalid tool choice and bail out
early: replace the unwrap_or_default usage with a check that calls
choice.function_name() and if None or empty then skip creating the tool-call
chunk (or return an Err) so you never call utils::generate_tool_call_id or
ChatCompletionStreamResponse::add_choice_tool_name with an invalid name; update
the logic around ToolChoice::Function, has_tool_calls.insert(index, true),
generate_tool_call_id, and add_choice_tool_name to only run when a valid
non-empty fn_name is present.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 97900599-e13c-46a7-81f6-cae0c466c96d

📥 Commits

Reviewing files that changed from the base of the PR and between 6d4d513 and 89acffd.

📒 Files selected for processing (8)

• crates/protocols/src/chat.rs
• crates/protocols/src/common.rs
• crates/protocols/src/responses.rs
• model_gateway/src/routers/grpc/harmony/stages/preparation.rs
• model_gateway/src/routers/grpc/regular/streaming.rs
• model_gateway/src/routers/grpc/utils/chat_utils.rs
• model_gateway/src/routers/grpc/utils/message_utils.rs
• model_gateway/tests/spec/chat_completion.rs

[claude]

Incremental review (synchronize) — reviewed all changes from origin/main...HEAD.

No new issues found beyond what existing review comments already cover. The core changes are well-designed:

• Type-safe tag enums (FunctionToolChoiceTag, BuiltInToolChoiceType, etc.) correctly prevent #[serde(untagged)] variant collisions — each variant's type field is locked to a single-value enum, so JSON payloads can't accidentally match the wrong arm.
• function_name() accessor cleanly abstracts over both Responses-API flat (name) and Chat-Completions nested (function.name) wire shapes, with correct priority (flat wins).
• Round-trip tests in common.rs cover all 8 spec variants plus the collision regression guard — good coverage.
• Validation properly rejects Function variants missing both name and function.name with a clear tool_choice_function_name_missing error.

0 🔴 Important · 0 🟡 Nit · 0 🟣 Pre-existing

[mergify]

Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch:

git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (1)

crates/protocols/src/responses.rs (1)

1498-1506: ⚠️ Potential issue | 🟠 Major

Non-function tool_choice variants still skip existence validation.

Function and function refs inside AllowedTools are checked against request.tools, but Types, Mcp, Custom, ApplyPatch, and Shell all bypass validation even when no matching tool/server is present. That lets impossible requests through instead of failing at request validation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/protocols/src/responses.rs` around lines 1498 - 1506, The match arm
for ResponsesToolChoice variants (Options, Types, Mcp, Custom, ApplyPatch,
Shell) currently does nothing, allowing types/MCP/custom/apply_patch/shell
choices to bypass existence checks against request.tools; update the validation
logic in the ResponsesToolChoice handling (the match over ResponsesToolChoice)
to perform the same existence verification used for Function/FunctionRef within
AllowedTools: for Types/Mcp/Custom/ApplyPatch/Shell, ensure the referenced tool
name/server exists in request.tools (or the MCP server registry) and return a
validation error if not present, so impossible tool choices fail during request
validation rather than at routing time.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/protocols/src/responses.rs`:
- Around line 248-254: The projection currently collapses explicit
Responses-only selections (Self::Types, Self::Mcp, Self::Custom,
Self::ApplyPatch, Self::Shell) into
ChatToolChoice::Value(ChatToolChoiceValue::Auto), which loses the original hard
selection; change the projection logic in the conversion function in
responses.rs so these variants are preserved (for example by adding a distinct
ChatToolChoice variant or encoding the original variant into the ChatToolChoice
value) or explicitly return an error/reject when the chat/gRPC representation
cannot express the exact selection; update the match arm handling Self::Types |
Self::Mcp | Self::Custom | Self::ApplyPatch | Self::Shell to emit that preserved
representation (or an Err) instead of ChatToolChoiceValue::Auto.
- Around line 206-213: The current serialize_to_string function double-encodes
ResponsesToolChoice by calling serde_json::to_string and returning a JSON
string; change the model and serialization so ResponsesResponse.tool_choice is a
typed optional field (Option<ResponsesToolChoice> or Option<serde_json::Value>)
and stop producing a JSON string. Replace usages of serialize_to_string to pass
the typed Option directly (or convert to serde_json::Value via
serde_json::to_value) so the final serializer emits the union/object shape
instead of a quoted JSON string; remove or repurpose serialize_to_string
accordingly and ensure ResponsesToolChoice derives/implements Serialize so the
response serializes natively.

---

Duplicate comments:
In `@crates/protocols/src/responses.rs`:
- Around line 1498-1506: The match arm for ResponsesToolChoice variants
(Options, Types, Mcp, Custom, ApplyPatch, Shell) currently does nothing,
allowing types/MCP/custom/apply_patch/shell choices to bypass existence checks
against request.tools; update the validation logic in the ResponsesToolChoice
handling (the match over ResponsesToolChoice) to perform the same existence
verification used for Function/FunctionRef within AllowedTools: for
Types/Mcp/Custom/ApplyPatch/Shell, ensure the referenced tool name/server exists
in request.tools (or the MCP server registry) and return a validation error if
not present, so impossible tool choices fail during request validation rather
than at routing time.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2bcf569d-2cac-4d59-9344-ec00224d1712

📥 Commits

Reviewing files that changed from the base of the PR and between 89acffd and 3d8b4ac.

📒 Files selected for processing (7)

• crates/protocols/src/responses.rs
• model_gateway/src/routers/grpc/harmony/responses/common.rs
• model_gateway/src/routers/grpc/harmony/stages/preparation.rs
• model_gateway/src/routers/grpc/regular/responses/conversions.rs
• model_gateway/tests/api/responses_api_test.rs
• model_gateway/tests/spec/chat_completion.rs
• model_gateway/tests/spec/responses.rs

[coderabbitai]

Only repository collaborators, contributors, or members can run CodeRabbit commands.

[claude]

🟡 Nit: function_name() on a Function variant always returns Some (the custom Deserialize impl guarantees name: String is populated). The if let Some guard introduces a dead else branch that would silently skip the "function not found in tools" check if function_name() ever returned None.

The previous code destructured name directly from the variant and had no way to skip the check. Consider matching the payload directly to preserve that unconditional guarantee:

Suggested change

	if let Some(name) = tool_choice.function_name() {
	if let Some(name) = tool_choice.function_name() {

→

            let name = tool_choice
                .function_name()
                .expect("Function variant always carries a name");

Not a bug today, but the old code was structurally unable to skip validation whereas this version relies on a runtime invariant.