Skip to content

fix(provider): apply reasoning_effort to ZAI providers via SetZaiThinking#3198

Open
tmchow wants to merge 4 commits intotailcallhq:mainfrom
tmchow:fix/3074-glm-zai-reasoning-effort
Open

fix(provider): apply reasoning_effort to ZAI providers via SetZaiThinking#3198
tmchow wants to merge 4 commits intotailcallhq:mainfrom
tmchow:fix/3074-glm-zai-reasoning-effort

Conversation

@tmchow
Copy link
Copy Markdown
Contributor

@tmchow tmchow commented Apr 29, 2026

Summary

Apply :reasoning-effort to z.ai providers (GLM 5.1 et al). Today the user can run :reasoning-effort none or :reasoning-effort high against GLM 5.1 and observe no behavior change because the per-effort setting is silently dropped before the request reaches the z.ai pipeline. This PR threads the user's intent through to z.ai's thinking: {type: enabled|disabled} shape.

Importance

z.ai's API only exposes thinking as on/off ({"type": "enabled"|"disabled"}), but Forge's internal model maps :reasoning-effort to Effort::None|Minimal|Low|Medium|High|XHigh|Max. Two pieces of code were dropping the user's intent:

  1. crates/forge_app/src/dto/openai/transformers/pipeline.rs:62-67 gates SetReasoningEffort on REQUESTY | GITHUB_COPILOT | deepseek | NVIDIA. ZAI is excluded by design (z.ai doesn't accept reasoning_effort), so its parallel transformer SetZaiThinking is the only path that should be touching reasoning for these providers.
  2. crates/forge_app/src/dto/openai/transformers/zai_reasoning.rs only mapped reasoning.enabled to thinking. The user's :reasoning-effort setting reaches the request as reasoning.effort with enabled: None, so SetZaiThinking did nothing and the request shipped without thinking. GLM 5.1 defaults to thinking ON, so the user got no behavior change for either none or high.

The Effort::None path additionally triggered an upstream short-circuit: Context::is_reasoning_supported() (crates/forge_domain/src/context.rs:644-647) treats Effort::None as a strong opt-out, after which DropReasoningDetails (gated at crates/forge_app/src/orch.rs:209) wiped the entire reasoning config before SetZaiThinking ever ran.

Closes #3074

Testing

  • cargo test -p forge_app zai_reasoning -> 13 tests pass (8 existing + 5 new effort-coverage tests)
  • cargo test -p forge_app drop_message_reasoning -> new transformer test passes
  • cargo test -p forge_app -> full crate suite passes
  • cargo clippy -p forge_app --all-features --all-targets -- -D warnings -> clean
  • cargo clippy --all-features --workspace -- -D clippy::string_slice -D clippy::indexing_slicing -D clippy::disallowed_methods -> clean

This contribution was developed with AI assistance.

Compound Engineering

@github-actions github-actions Bot added the type: fix Iterations on existing features or infrastructure. label Apr 29, 2026
@tmchow tmchow force-pushed the fix/3074-glm-zai-reasoning-effort branch 2 times, most recently from 785dbea to c46eb03 Compare May 5, 2026 03:17
Comment thread crates/forge_app/src/dto/openai/transformers/zai_reasoning.rs
@laststylebender14 laststylebender14 marked this pull request as draft May 5, 2026 03:32
Comment thread crates/forge_app/src/orch.rs Outdated
fn transform(&mut self, mut context: Self::Value) -> Self::Value {
context.messages.iter_mut().for_each(|message| {
if let ContextMessage::Text(text) = &mut **message {
text.reasoning_details = None;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about other reasoning fields?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I scoped this to reasoning_details to match the sibling DropReasoningDetails transformer in forge_domain (clears reasoning_details + context.reasoning only). The other reasoning-adjacent field on TextMessage is thought_signature, which only flows through dto/google/response.rs and is left alone by the sibling. Happy to clear it too if you'd prefer cross-provider parity here, just wanted to flag the scope intentionally matched the sibling before broadening.

tmchow added 2 commits May 10, 2026 16:43
@laststylebender14 asked what happens when reasoning.effort is set to
None. Add an inline comment near the .map(|_| true) line walking through
why the outer enabled becomes None and no thinking field is added, and
point at the existing test that locks the case.
@tmchow tmchow force-pushed the fix/3074-glm-zai-reasoning-effort branch from f9aeca9 to bad7354 Compare May 10, 2026 23:43
Use &self.agent.provider directly inside the method instead of passing it
through as a function argument from the caller. Addresses reviewer feedback.
@tmchow tmchow marked this pull request as ready for review May 11, 2026 00:01
@tmchow
Copy link
Copy Markdown
Contributor Author

tmchow commented May 11, 2026

@laststylebender14 thanks for the review.

you don't need to pass provider from here, you can directly access it as it's avail on self.

Fixed in 5f5f14d. Method now reads let provider_id = &self.agent.provider; internally so the body keeps the same shape.

What about other reasoning fields?

I scoped this to reasoning_details to match the sibling DropReasoningDetails transformer in forge_domain (clears reasoning_details + context.reasoning only). The other reasoning-adjacent field on TextMessage is thought_signature, which only flows through dto/google/response.rs and is left alone by the sibling. Happy to clear it too if you'd prefer cross-provider parity here, just wanted to flag the scope intentionally matched the sibling before broadening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: fix Iterations on existing features or infrastructure.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: reasoning effort command set not effect on GLM 5.1

3 participants