Merged
Conversation
The Z.AI Coding Plan API expects model names with the 'zai/' prefix (e.g., 'zai/glm-5') rather than bare model names (e.g., 'glm-5'). Added remap_model_name_for_api() helper to handle provider-specific model name formatting. Applied to all OpenAI-compatible API call methods.
WalkthroughAdded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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)
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 |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/llm/model.rs`:
- Around line 824-832: The remap_model_name_for_api method currently
unconditionally prefixes "zai/" when provider == "zai-coding-plan", which
double-prefixes if model_name already starts with "zai/"; update
remap_model_name_for_api to be idempotent by returning model_name.clone() if it
already starts_with("zai/") and only prefixing otherwise (keep the provider
check on self.provider == "zai-coding-plan" and use self.model_name to decide
whether to add "zai/"). Ensure the function still returns
self.model_name.clone() for non-zai providers.
Co-authored-by: tembo[bot] <208362400+tembo[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/llm/model.rs (2)
757-760:call_openai_compatiblestill uses bareself.model_name— inconsistent with the other three paths.
call_openai_compatibleis the only OpenAI-compatible call path that bypassesremap_model_name_for_api(). Currently this method only handles Gemini, which doesn't need remapping, so there's no active bug. However, if any future provider is routed through this path and requires name translation, the remapping will silently be skipped.♻️ Suggested fix
- let mut body = serde_json::json!({ - "model": self.model_name, - "messages": messages, - }); + let api_model_name = self.remap_model_name_for_api(); + let mut body = serde_json::json!({ + "model": api_model_name, + "messages": messages, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 757 - 760, The JSON body construction in call_openai_compatible uses the raw self.model_name (the snippet building let mut body = serde_json::json!({ "model": self.model_name, "messages": messages, });) and thus bypasses remap_model_name_for_api; update call_openai_compatible to call self.remap_model_name_for_api() (or the equivalent method used elsewhere) and use that returned string/value for the "model" field so model name translation is applied consistently across all OpenAI-compatible paths.
1526-1566: Consider adding a unit test forremap_model_name_for_api.The test module already covers
reverse_map_tool_names. Given thatremap_model_name_for_apiis the core logic of this PR — and the idempotency edge case is worth locking in — a targeted test would be a cheap safety net.#[test] fn remap_model_name_for_api_adds_zai_prefix() { // bare model name gets prefixed // ... } #[test] fn remap_model_name_for_api_is_idempotent() { // already-prefixed model name is not double-prefixed // ... } #[test] fn remap_model_name_for_api_noop_for_other_providers() { // non-zai provider returns model_name unchanged // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 1526 - 1566, Add unit tests for remap_model_name_for_api in the existing tests module: create three tests that call remap_model_name_for_api directly and assert behavior — (1) remap_model_name_for_api_adds_zai_prefix: when provider == "zai" and model_name is "gpt-4o-mini" it returns "zai-gpt-4o-mini", (2) remap_model_name_for_api_is_idempotent: when provider == "zai" and model_name already starts with "zai-" it returns the same string unchanged, and (3) remap_model_name_for_api_noop_for_other_providers: when provider != "zai" it returns the original model_name; place them in the same mod tests block alongside reverse_map_restores_original_tool_names and use assert_eq! comparing remap_model_name_for_api(provider, model_name) to the expected string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/llm/model.rs`:
- Around line 757-760: The JSON body construction in call_openai_compatible uses
the raw self.model_name (the snippet building let mut body = serde_json::json!({
"model": self.model_name, "messages": messages, });) and thus bypasses
remap_model_name_for_api; update call_openai_compatible to call
self.remap_model_name_for_api() (or the equivalent method used elsewhere) and
use that returned string/value for the "model" field so model name translation
is applied consistently across all OpenAI-compatible paths.
- Around line 1526-1566: Add unit tests for remap_model_name_for_api in the
existing tests module: create three tests that call remap_model_name_for_api
directly and assert behavior — (1) remap_model_name_for_api_adds_zai_prefix:
when provider == "zai" and model_name is "gpt-4o-mini" it returns
"zai-gpt-4o-mini", (2) remap_model_name_for_api_is_idempotent: when provider ==
"zai" and model_name already starts with "zai-" it returns the same string
unchanged, and (3) remap_model_name_for_api_noop_for_other_providers: when
provider != "zai" it returns the original model_name; place them in the same mod
tests block alongside reverse_map_restores_original_tool_names and use
assert_eq! comparing remap_model_name_for_api(provider, model_name) to the
expected string.
|
This seems to have broken all GLM Coding Plan model routing for me |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes model routing for Z.AI Coding Plan provider. The API expects model names with the
zai/prefix (e.g.,zai/glm-5) rather than bare model names (e.g.,glm-5).Changes
remap_model_name_for_api()helper method toSpacebotModelcall_openai()call_openai_responses()call_openai_compatible_with_optional_auth()Fix
When users select
zai-coding-plan/glm-5, the system now correctly sends{\"model\": \"zai/glm-5\"}to the API instead of{\"model\": \"glm-5\"}.Affects all Z.AI Coding Plan models:
zai-coding-plan/glm-5→ API receiveszai/glm-5zai-coding-plan/glm-4.7→ API receiveszai/glm-4.7zai-coding-plan/glm-4.5-air→ API receiveszai/glm-4.5-airNote
Implementation verified against actual changes. The fix introduces a single private helper method
remap_model_name_for_api()that applies the provider-specific prefix transformation consistently across all three OpenAI-compatible API call paths insrc/llm/model.rs.Written by Tembo for commit 3eed019.