Repository navigation
Conversation
…ts so invalid combinations fail at the protocol layer.
📝 WalkthroughWalkthroughAdds the GLM-4.6 model to nightly benchmark configurations (workflow, test list, model specs) and replaces an MCP-specific validator with a broader message request validator that enforces cross-field tool choice and tool list constraints, plus corresponding unit tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
|
Hi @nishanthp, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
|
Hi @nishanthp, 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 |
mergify
Bot
added
the
needs-rebase
|
The file changes include something not mentioned in PR description. Please remove those and open a new PR if those changes are necessary. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation logic for the Messages API to ensure that tool choices are consistent with the provided tools, including support for MCP toolsets. It also adds comprehensive unit tests to verify these validation rules. Additionally, the GLM-4.6 model has been added to the nightly performance benchmarks and model specifications. I have no feedback to provide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nightly-benchmark.yml:
- Line 349: Replace the inline mapping "- { id: zai-org/GLM-4.6, slug:
zai-org-GLM-4.6, test_class: TestNightlyGlm46Single }" with a block-style
mapping to satisfy YAMLlint braces rules: convert the list item to use nested
keys (id, slug, test_class) on separate indented lines under the list dash,
preserving the same values and indentation so the entry becomes a normal YAML
mapping rather than an inline brace mapping.
In `@crates/protocols/src/messages.rs`:
- Around line 145-171: Add a validation that when req.thinking (or
ThinkingConfig) is set to ThinkingConfig::Enabled, the tool_choice must be
either ToolChoice::Auto or ToolChoice::None: detect the invalid cases
(matches!(tool_choice, ToolChoice::Any) and if let ToolChoice::Tool { .. }) and
return a validator::ValidationError (e.g.,
new("tool_choice_not_allowed_with_thinking") with an explanatory message)
similar to the existing errors; update the block after computing
requires_tools/has_tools (or where ToolChoice is inspected) to enforce this rule
and add a regression test that constructs a request with ThinkingConfig::Enabled
and tool_choice = Any or Tool to assert validation fails.
In `@e2e_test/benchmarks/test_nightly_perf.py`:
- Line 105: The generator currently emits a "*Multi" test class even when the
entry sets multi_workers (worker_count) == 1, which collides with _run_nightly()
logic that treats worker_count==1 as single; update the generator that creates
TestNightly...Multi to only emit the multi-worker variant when multi_workers
(worker_count) > 1 (or alternatively derive worker_type from worker_count), and
ensure the generated experiment_folder includes the worker_type or worker_count
to avoid name collisions between single and multi variants; reference the
generator that creates TestNightlyGlm46Multi, the multi_workers/worker_count
field in the tuple, the experiment_folder naming, and the _run_nightly()
function when making the change.
🪄 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: 5d3e312c-8b4f-4422-9422-125a462200b2
📒 Files selected for processing (4)
.github/workflows/nightly-benchmark.ymlcrates/protocols/src/messages.rse2e_test/benchmarks/test_nightly_perf.pye2e_test/infra/model_specs.py
| model: | ||
| - { id: meta-llama/Llama-4-Maverick-17B-128E-Instruct-FP8, slug: meta-llama-Llama-4-Maverick-17B-128E-Instruct-FP8, test_class: TestNightlyLlama4MaverickSingle } | ||
| - { id: minimaxai/minimax-m2, slug: minimaxai-minimax-m2, test_class: TestNightlyMinimaxM2Single } | ||
| - { id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single } |
There was a problem hiding this comment.
Fix YAMLlint's braces violation.
This new inline mapping is already being flagged by static analysis, so the workflow will stay red until the spacing matches the repo's YAMLlint rule.
🧹 Minimal fix
- - { id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single }
+ - {id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - { id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single } | |
| - {id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class: TestNightlyGlm46Single} |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 349-349: too many spaces inside braces
(braces)
[error] 349-349: too many spaces inside braces
(braces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nightly-benchmark.yml at line 349, Replace the inline
mapping "- { id: zai-org/GLM-4.6, slug: zai-org-GLM-4.6, test_class:
TestNightlyGlm46Single }" with a block-style mapping to satisfy YAMLlint braces
rules: convert the list item to use nested keys (id, slug, test_class) on
separate indented lines under the list dash, preserving the same values and
indentation so the entry becomes a normal YAML mapping rather than an inline
brace mapping.
| let has_tools = req.tools.as_ref().is_some_and(|tools| !tools.is_empty()); | ||
| let requires_tools = !matches!(tool_choice, ToolChoice::None); | ||
|
|
||
| if requires_tools && !has_tools { | ||
| let mut e = validator::ValidationError::new("tool_choice_requires_tools"); | ||
| e.message = Some( | ||
| "Invalid value for 'tool_choice': 'tool_choice' is only allowed when 'tools' are specified." | ||
| .into(), | ||
| ); | ||
| return Err(e); | ||
| } | ||
|
|
||
| if let ToolChoice::Tool { name, .. } = tool_choice { | ||
| let tool_exists = req | ||
| .tools | ||
| .as_ref() | ||
| .is_some_and(|tools| tools.iter().any(|tool| tool.matches_tool_choice_name(name))); | ||
|
|
||
| if !tool_exists { | ||
| let mut e = validator::ValidationError::new("tool_choice_tool_not_found"); | ||
| e.message = Some( | ||
| format!("Invalid value for 'tool_choice': tool '{name}' not found in 'tools'.") | ||
| .into(), | ||
| ); | ||
| return Err(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file mentioned
find . -type f -name "messages.rs" | grep protocolsRepository: lightseekorg/smg
Length of output: 94
🏁 Script executed:
# Read the specific lines mentioned in the review
head -n 180 crates/protocols/src/messages.rs | tail -n 40Repository: lightseekorg/smg
Length of output: 1420
🏁 Script executed:
# Search for ToolChoice enum definition
rg -A 15 "enum ToolChoice"Repository: lightseekorg/smg
Length of output: 5713
🏁 Script executed:
# Search for ThinkingConfig definition
rg -A 10 "enum ThinkingConfig\|struct ThinkingConfig"Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Look for the "Also applies to" reference (lines 1996-2129)
wc -l crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 97
🏁 Script executed:
# Search for ThinkingConfig more broadly (might be struct)
rg "ThinkingConfig" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 169
🏁 Script executed:
# Check what's at lines 1996-2129 in messages.rs
sed -n '1996,2050p' crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 1645
🏁 Script executed:
# Look for the request struct definition to see if it has thinking field
rg -B 5 "fn validate_tool_choice" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Search for thinking field in messages module
rg "thinking" crates/protocols/src/messages.rs | head -20Repository: lightseekorg/smg
Length of output: 690
🏁 Script executed:
# Look for test files related to tool_choice and thinking
find . -type f -name "*test*" -o -name "*spec*" | grep -i protocol | head -10Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Get full ThinkingConfig definition
rg -A 20 "pub enum ThinkingConfig" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 672
🏁 Script executed:
# Look for any existing thinking-related tool_choice validation
rg -B 5 -A 10 "thinking.*tool_choice\|tool_choice.*thinking" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check request struct definition to see full shape
rg -B 30 "pub thinking: Option<ThinkingConfig>" crates/protocols/src/messages.rs | head -50Repository: lightseekorg/smg
Length of output: 1894
🏁 Script executed:
# Look for Anthropic docs references or comments about thinking constraints
rg -i "anthropic.*thinking\|thinking.*auto\|thinking.*tool" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Get the test section around 1996-2129 to see if thinking+tool tests exist
sed -n '1996,2129p' crates/protocols/src/messages.rs | tail -50Repository: lightseekorg/smg
Length of output: 1808
🏁 Script executed:
# Search for Anthropic API constraint documentation in code/comments
rg -i "extended thinking" crates/protocols/src/messages.rs | head -20Repository: lightseekorg/smg
Length of output: 211
🏁 Script executed:
# Search for other files with Anthropic API validation docs
rg -i "docs.anthropic\|anthropic.*api" crates/protocols/src/Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Look at the validate_tool_choice function context - where is it called?
rg -B 10 "fn validate_tool_choice" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check if there are any comments about thinking limitations in the file
rg -B 2 -A 2 "thinking" crates/protocols/src/messages.rs | grep -i "constraint\|limit\|restrict\|allow\|support" | head -10Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Search for any validation that checks thinking config
rg -A 5 "thinking.*match\|if.*thinking.*Enabled" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Let's look at the full validate_tool_choice function
rg -B 2 -A 30 "fn validate_tool_choice" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check if there are related validations in other places
rg "thinking.*Enabled" crates/protocols/src/messages.rs -B 5 -A 5Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Look for all validations related to thinking
rg "pub fn validate" crates/protocols/src/messages.rs | head -20Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Search across the entire crates/protocols for thinking + tool validation
rg -l "thinking.*tool\|tool.*thinking" crates/protocols/src/Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check the responses.rs file mentioned in learnings for context on validation patterns
head -100 crates/protocols/src/responses.rs | tail -50Repository: lightseekorg/smg
Length of output: 1811
🏁 Script executed:
# Search for the function more directly - it might be defined elsewhere
rg -l "validate_tool_choice" crates/protocols/src/Repository: lightseekorg/smg
Length of output: 93
🏁 Script executed:
# Look at the specific lines from the review to understand context better
sed -n '130,180p' crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 1873
🏁 Script executed:
# Find where validate_tool_choice is actually defined
ast-grep --pattern 'fn validate_tool_choice($_) { $$$ }'Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Search for the validator module or where this validation happens
rg "validator::ValidationError" crates/protocols/src/messages.rs | head -5Repository: lightseekorg/smg
Length of output: 406
🏁 Script executed:
# Check which line has the validate_tool_choice definition
rg -n "validate_tool_choice" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check the Anthropic API docs reference - search for any doc links in code
rg "docs.anthropic" crates/protocols/Repository: lightseekorg/smg
Length of output: 144
🏁 Script executed:
# Look for any comments or docs about extended thinking constraints
rg -i "extended thinking" -B 3 -A 3 crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 791
🏁 Script executed:
# Search for any existing thinking + tool_choice validation in the codebase
rg -r "thinking.*Enabled" crates/protocols/src/ -A 10 | grep -i "tool\|choice" -B 5 -A 5Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check model_gateway or other places that might handle Anthropic API constraints
find crates -name "*.rs" -type f | xargs rg -l "thinking.*auto\|thinking.*none" 2>/dev/null | head -5Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Look at what the review references - search for tool_choice validation tests related to thinking
rg -B 5 -A 15 "thinking.*tool_choice\|tool_choice.*thinking" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Search for Anthropic's actual API constraints in comments or documentation files
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.doc" \) -exec grep -l "thinking" {} \; 2>/dev/null | head -5Repository: lightseekorg/smg
Length of output: 259
🏁 Script executed:
# Check the reasoning parser README for thinking constraints
cat crates/reasoning_parser/README.mdRepository: lightseekorg/smg
Length of output: 7816
🏁 Script executed:
# Look at the messages.rs file to understand the actual constraint with Anthropic API
# The review claims Anthropic only allows auto/none with thinking
# Let's search for any existing validation or comments about this
rg -B 10 -A 10 "thinking.*auto\|thinking.*none\|extended thinking" crates/protocols/src/messages.rs | head -40Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Let's check if there's a test that combines thinking + tool_choice (Any or Tool)
rg -n "thinking\|Enabled" crates/protocols/src/messages.rs | grep -i testRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Look for any Anthropic API documentation links or constraint references
rg "anthropic.com" crates/Repository: lightseekorg/smg
Length of output: 227
🏁 Script executed:
# Search for model_gateway to see if there's downstream validation
rg -l "thinking.*tool\|tool.*thinking" model_gateway/ 2>/dev/null | head -5Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Let's check if there's any model_gateway handling for this constraint
find model_gateway -name "*.rs" -type f | xargs rg -l "thinking\|extended" 2>/dev/null | head -5Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Search for any test that combines thinking + Any or Tool
rg -n "Enabled.*tool_choice\|tool_choice.*Enabled" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check if there's any downstream validation in model_gateway
rg "tool_choice.*thinking\|thinking.*tool_choice" model_gateway/ 2>/dev/nullRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Let's look at the full test file section to see what tests exist
sed -n '1900,2100p' crates/protocols/src/messages.rs | grep "fn test" -A 2Repository: lightseekorg/smg
Length of output: 2161
🏁 Script executed:
# Search for any comments about Anthropic API constraints near thinking
rg -B 5 -A 5 "Anthropic\|anthropic" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Let's verify the Anthropic API constraint by checking if there's any documentation or comments
rg -i "extended.*thinking\|thinking.*tool\|tool_choice.*thinking" crates/protocols/src/ -B 3 -A 3Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check if model_gateway has any downstream validation for this
find model_gateway -type f -name "*.rs" | xargs rg -l "CreateMessageRequest\|tool_choice" | head -3Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Let's check the complete validate_message_request to be thorough
sed -n '130,190p' crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 2093
🏁 Script executed:
# Search for any TODOs or FIXMEs related to thinking
rg -i "TODO\|FIXME" crates/protocols/src/messages.rs | grep -i "thinking\|tool"Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check the actual tests to see if there's a combined thinking + tool test
sed -n '1995,2050p' crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 1646
🏁 Script executed:
# Let's check model_gateway for any thinking + tool_choice handling
find model_gateway -type f -name "*.rs" -exec rg -l "thinking\|tool_choice" {} \; | head -5Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Look for any model_gateway validation or processing
rg "CreateMessageRequest" model_gateway/ | head -10Repository: lightseekorg/smg
Length of output: 982
🏁 Script executed:
# Check if there's any indication in code about this being a known constraint
rg -i "anthropic.*thinking\|only.*auto\|only.*none" crates/ model_gateway/ | head -20Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Let's look for the base_request() helper in tests to understand the test structure
rg -B 5 "fn base_request" crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 159
🏁 Script executed:
# Search for thinking field initialization in base_request
sed -n '2050,2180p' crates/protocols/src/messages.rs | grep -A 30 "fn base_request"Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Get the full base_request function
sed -n '2060,2120p' crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 2133
🏁 Script executed:
# Now let's check the Anthropic API documentation link to verify the constraint
# First check if there are any references to what tool_choice is allowed with thinking
rg -i "tool_choice.*auto\|tool_choice.*none\|auto.*tool_choice\|none.*tool_choice" crates/Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Look for any validation or documentation about thinking constraints in responses.rs
rg -B 5 -A 5 "thinking.*Enabled\|extended.*thinking" crates/protocols/src/responses.rs | head -40Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check model_gateway for any rejection or handling of this scenario
rg -i "thinking.*tool\|tool.*thinking" model_gateway/ -rRepository: lightseekorg/smg
Length of output: 473
🏁 Script executed:
# Let's verify if this is a real constraint by looking for any PRs or issues mentioning it
git log --oneline --all | grep -i "thinking\|tool_choice" | head -10Repository: lightseekorg/smg
Length of output: 383
🏁 Script executed:
# Let's check the recent commit about tool_choice validation
git show a24dc38 --statRepository: lightseekorg/smg
Length of output: 472
🏁 Script executed:
# Look at the commit details
git show a24dc38Repository: lightseekorg/smg
Length of output: 10478
🏁 Script executed:
# Check if there are any comments in the validate_message_request about thinking
sed -n '130,200p' crates/protocols/src/messages.rsRepository: lightseekorg/smg
Length of output: 2366
🏁 Script executed:
# Let's search for any upstream/downstream handling in model_gateway
find model_gateway/src -name "*.rs" | xargs rg "thinking" 2>/dev/null | grep -i "tool\|choice" | head -5Repository: lightseekorg/smg
Length of output: 676
🏁 Script executed:
# Let's verify if there's any other reference to this constraint in the codebase
rg -i "thinking.*auto\|thinking.*none\|extended.*thinking.*tool" crates/ model_gateway/ --type rust | head -20Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check if there are any integration tests or documentation files mentioning this
find . -type f \( -name "*.md" -o -name "*.txt" \) -not -path "./.git/*" | xargs grep -l "thinking\|extended" 2>/dev/null | head -5Repository: lightseekorg/smg
Length of output: 259
🏁 Script executed:
# Let's check if the downstream model_gateway or other modules handle this constraint
rg "thinking.*Enabled" model_gateway/ -A 10 | grep -i "tool\|choice\|error" -B 3 -A 3 | head -30Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check if there's any handling in responses.rs related to thinking constraints
rg "thinking.*Enabled" crates/protocols/src/responses.rs -B 5 -A 10 | head -40Repository: lightseekorg/smg
Length of output: 42
Add validation to reject tool_choice any and tool when extended thinking is enabled.
Per Anthropic's Messages API, only tool_choice auto or none are compatible with extended thinking. The current validator accepts Any and Tool variants alongside ThinkingConfig::Enabled, allowing invalid requests to bypass local validation and be rejected downstream. Add the check and include a regression test case.
🛡️ Suggested validator fix
if requires_tools && !has_tools {
let mut e = validator::ValidationError::new("tool_choice_requires_tools");
e.message = Some(
"Invalid value for 'tool_choice': 'tool_choice' is only allowed when 'tools' are specified."
.into(),
);
return Err(e);
}
+ if matches!(req.thinking, Some(ThinkingConfig::Enabled { .. }))
+ && matches!(tool_choice, ToolChoice::Any { .. } | ToolChoice::Tool { .. })
+ {
+ let mut e = validator::ValidationError::new("tool_choice_incompatible_with_thinking");
+ e.message = Some(
+ "Invalid value for 'tool_choice': 'any' and 'tool' are not supported when 'thinking' is enabled."
+ .into(),
+ );
+ return Err(e);
+ }
if let ToolChoice::Tool { name, .. } = tool_choice {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let has_tools = req.tools.as_ref().is_some_and(|tools| !tools.is_empty()); | |
| let requires_tools = !matches!(tool_choice, ToolChoice::None); | |
| if requires_tools && !has_tools { | |
| let mut e = validator::ValidationError::new("tool_choice_requires_tools"); | |
| e.message = Some( | |
| "Invalid value for 'tool_choice': 'tool_choice' is only allowed when 'tools' are specified." | |
| .into(), | |
| ); | |
| return Err(e); | |
| } | |
| if let ToolChoice::Tool { name, .. } = tool_choice { | |
| let tool_exists = req | |
| .tools | |
| .as_ref() | |
| .is_some_and(|tools| tools.iter().any(|tool| tool.matches_tool_choice_name(name))); | |
| if !tool_exists { | |
| let mut e = validator::ValidationError::new("tool_choice_tool_not_found"); | |
| e.message = Some( | |
| format!("Invalid value for 'tool_choice': tool '{name}' not found in 'tools'.") | |
| .into(), | |
| ); | |
| return Err(e); | |
| } | |
| } | |
| let has_tools = req.tools.as_ref().is_some_and(|tools| !tools.is_empty()); | |
| let requires_tools = !matches!(tool_choice, ToolChoice::None); | |
| if requires_tools && !has_tools { | |
| let mut e = validator::ValidationError::new("tool_choice_requires_tools"); | |
| e.message = Some( | |
| "Invalid value for 'tool_choice': 'tool_choice' is only allowed when 'tools' are specified." | |
| .into(), | |
| ); | |
| return Err(e); | |
| } | |
| if matches!(req.thinking, Some(ThinkingConfig::Enabled { .. })) | |
| && matches!(tool_choice, ToolChoice::Any { .. } | ToolChoice::Tool { .. }) | |
| { | |
| let mut e = validator::ValidationError::new("tool_choice_incompatible_with_thinking"); | |
| e.message = Some( | |
| "Invalid value for 'tool_choice': 'any' and 'tool' are not supported when 'thinking' is enabled." | |
| .into(), | |
| ); | |
| return Err(e); | |
| } | |
| if let ToolChoice::Tool { name, .. } = tool_choice { | |
| let tool_exists = req | |
| .tools | |
| .as_ref() | |
| .is_some_and(|tools| tools.iter().any(|tool| tool.matches_tool_choice_name(name))); | |
| if !tool_exists { | |
| let mut e = validator::ValidationError::new("tool_choice_tool_not_found"); | |
| e.message = Some( | |
| format!("Invalid value for 'tool_choice': tool '{name}' not found in 'tools'.") | |
| .into(), | |
| ); | |
| return Err(e); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/protocols/src/messages.rs` around lines 145 - 171, Add a validation
that when req.thinking (or ThinkingConfig) is set to ThinkingConfig::Enabled,
the tool_choice must be either ToolChoice::Auto or ToolChoice::None: detect the
invalid cases (matches!(tool_choice, ToolChoice::Any) and if let
ToolChoice::Tool { .. }) and return a validator::ValidationError (e.g.,
new("tool_choice_not_allowed_with_thinking") with an explanatory message)
similar to the existing errors; update the block after computing
requires_tools/has_tools (or where ToolChoice is inspected) to enforce this rule
and add a regression test that constructs a request with ThinkingConfig::Enabled
and tool_choice = Any or Tool to assert validation fails.
| ("Qwen/Qwen3-30B-A3B", "Qwen30b", 4, ["http", "grpc"], {}), | ||
| ("openai/gpt-oss-20b", "GptOss20b", 1, ["http", "grpc"], {}), | ||
| ("minimaxai/minimax-m2", "MinimaxM2", 1, ["http", "grpc"], {}), | ||
| ("zai-org/GLM-4.6", "Glm46", 1, ["http", "grpc"], {}), |
There was a problem hiding this comment.
multi_workers=1 still generates a duplicate single-worker variant.
The generator at Lines 145-151 will create TestNightlyGlm46Multi for this entry, but _run_nightly() treats worker_count == 1 as worker_type == "single". That leaves you with a misleading extra class and colliding experiment_folder names if the whole module is collected, even though the PR objective is single-worker coverage only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e_test/benchmarks/test_nightly_perf.py` at line 105, The generator
currently emits a "*Multi" test class even when the entry sets multi_workers
(worker_count) == 1, which collides with _run_nightly() logic that treats
worker_count==1 as single; update the generator that creates TestNightly...Multi
to only emit the multi-worker variant when multi_workers (worker_count) > 1 (or
alternatively derive worker_type from worker_count), and ensure the generated
experiment_folder includes the worker_type or worker_count to avoid name
collisions between single and multi variants; reference the generator that
creates TestNightlyGlm46Multi, the multi_workers/worker_count field in the
tuple, the experiment_folder naming, and the _run_nightly() function when making
the change.
|
@CatherineSue checking |
d857d2d to
96f4dcf
Compare
![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: 96f4dcf473
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Tool::Bash(tool) => tool.name == *name, | ||
| Tool::TextEditor(tool) => tool.name == *name, | ||
| Tool::WebSearch(tool) => tool.name == *name, | ||
| Tool::McpToolset(_) => false, |
There was a problem hiding this comment.
In validate_message_request, a request with tool_choice: {"type":"tool","name":...} and tools coming from mcp_toolset is now always rejected because the McpToolset branch hard-codes false. This introduces a 400 regression for MCP-specific tool selection (including passthrough requests and SMG MCP flows where tools are materialized later via inject_mcp_tools_into_request), even when the named MCP tool would be valid at runtime. The existence check should not fail solely because the tool is sourced from mcp_toolset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/messages.rs`:
- Line 23: The PR includes unrelated protocol validator changes — the attribute
#[validate(schema(function = "validate_message_request"))] and the new
cross-field validations (replacing validate_mcp_config and adding
tool_choice/tools checks) — which must be removed from this benchmark-focused
PR; revert messages.rs to use the original validator configuration (restore use
of validate_mcp_config where it was before) and remove the new
validate_message_request/schema reference and any added cross-field logic
(tool_choice/tools validation and related helper functions) so these
protocol-level changes can be submitted in a separate follow-up PR.
🪄 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: a89c7db6-917e-4ec7-9685-8f998bbeec36
📒 Files selected for processing (4)
.github/workflows/nightly-benchmark.ymlcrates/protocols/src/messages.rse2e_test/benchmarks/test_nightly_perf.pye2e_test/infra/model_specs.py
| #[serde_with::skip_serializing_none] | ||
| #[derive(Debug, Clone, Serialize, Deserialize, Validate, schemars::JsonSchema)] | ||
| #[validate(schema(function = "validate_mcp_config"))] | ||
| #[validate(schema(function = "validate_message_request"))] |
There was a problem hiding this comment.
These validator changes appear unrelated to GLM-4.6 benchmark coverage.
Per CatherineSue's review comment, this PR's scope is adding GLM-4.6 to nightly benchmarks. The validate_message_request changes (replacing validate_mcp_config and adding tool_choice/tools cross-field validation) are unrelated protocol-level changes that should be in a separate PR.
Also applies to: 108-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/protocols/src/messages.rs` at line 23, The PR includes unrelated
protocol validator changes — the attribute #[validate(schema(function =
"validate_message_request"))] and the new cross-field validations (replacing
validate_mcp_config and adding tool_choice/tools checks) — which must be removed
from this benchmark-focused PR; revert messages.rs to use the original validator
configuration (restore use of validate_mcp_config where it was before) and
remove the new validate_message_request/schema reference and any added
cross-field logic (tool_choice/tools validation and related helper functions) so
these protocol-level changes can be submitted in a separate follow-up PR.
|
@CatherineSue Sorry for the confusion. I am closing this. Here is the new PR #954 |
2 participants
Problem
The nightly benchmark suite does not currently include coverage for
zai-org/GLM-4.6, so we do not capture performance regressions for that model in the nightly pipeline.Solution
Add
zai-org/GLM-4.6to the nightly benchmark configuration and model specs so it runs in the H200 single-worker benchmark matrix with the expected feature set and tensor parallelism.Changes
zai-org/GLM-4.6to the nightly benchmark workflow matrix.tp: 8.Test Plan
TestNightlyGlm46Singlebenchmark job.MODEL_SPECSand launches withtp=8.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Bug Fixes
Tests