feat(llm): thread per-tool reasoning through provider/tool-call path

[panosAthDBX]

Summary

First split PR from #361: core reasoning-summary plumbing in the LLM/provider layer.

This PR is intentionally scoped to provider/types + LLM reasoning integration, matching requested split direction:

• src/llm/provider.rs
• src/llm/reasoning.rs
• src/llm/rig_adapter.rs
• related LLM adapters/callers that construct ToolCall

Changes

• Extend ToolCall with reasoning: String.
• Add deterministic fallback/normalization helpers:
  • DEFAULT_TOOL_RATIONALE
  • normalize_tool_reasoning(...)
• Thread per-tool reasoning through Reasoning::select_tools and RespondResult::ToolCalls construction.
• Ensure adapters/providers populate normalized reasoning when upstream omits it.
• Add tests for normalization and reasoning threading behavior.
• Add minimal compatibility test updates where ToolCall is constructed directly.

Why this split

Everything downstream (dispatcher/session, worker streaming, web UI reasoning surfaces) depends on this type-level and provider-level contract. Landing this first reduces risk and makes follow-up PRs mechanical and reviewable.

Testing

Ran locally before opening:

• cargo clippy --all --all-features
• cargo test

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request integrates per-tool reasoning into the core LLM and provider layers. By extending the ToolCall structure and implementing robust normalization and threading, it ensures that a rationale for tool selection is consistently available. This foundational change prepares the system for richer downstream features that can leverage this reasoning, while also improving system stability by handling potential panics from external LLM providers more gracefully.

Highlights

• Enhanced ToolCall Structure: The ToolCall struct now includes a reasoning: String field, allowing LLMs to provide a rationale for selecting a specific tool.
• Reasoning Normalization and Fallback: Introduced DEFAULT_TOOL_RATIONALE and normalize_tool_reasoning to ensure a consistent, non-empty reasoning string for all tool calls, providing a fallback when providers don't explicitly supply one.
• Reasoning Threading in LLM Providers: Per-tool reasoning is now threaded through the Reasoning::select_tools and RespondResult::ToolCalls construction paths, ensuring it's captured and propagated.
• LLM Adapter Compatibility: LLM adapters and providers have been updated to populate the new reasoning field, utilizing the normalization helper where necessary.
• Robust Panic Handling for Rig-Core: Implemented a panic suppression mechanism for the rig-core OpenAI provider to convert specific panics (e.g., usage underflow) into LlmErrors, preventing process crashes.
• Comprehensive Testing: New tests have been added to verify the correct behavior of reasoning normalization, threading, and the panic suppression logic.

Changelog

• src/agent/dispatcher.rs
  • Updated ToolCall instantiations in tests to include the new reasoning field with a normalized default.
• src/channels/web/openai_compat.rs
  • Imported normalize_tool_reasoning for use in tool call conversions.
  • Modified ToolCall constructions to assign a normalized empty string to the reasoning field.
• src/llm/mod.rs
  • Exported DEFAULT_TOOL_RATIONALE and normalize_tool_reasoning from the provider module.
• src/llm/nearai_chat.rs
  • Imported normalize_tool_reasoning for use in tool call handling.
  • Added the reasoning field to ToolCall constructions, normalizing empty reasoning strings.
• src/llm/provider.rs
  • Defined DEFAULT_TOOL_RATIONALE as a constant for default tool reasoning.
  • Implemented normalize_tool_reasoning function to trim and provide a fallback for empty reasoning strings.
  • Added a reasoning: String field to the ToolCall struct.
  • Included new unit tests for normalize_tool_reasoning to verify its behavior with empty and non-empty inputs.
• src/llm/reasoning.rs
  • Imported normalize_tool_reasoning for consistent reasoning handling.
  • Modified select_tools to apply normalize_tool_reasoning to the reasoning field of ToolSelection.
  • Updated RespondResult::ToolCalls construction to map and normalize the reasoning field for each ToolCall.
  • Added new test cases (test_toolcall_reasoning_threaded_from_selection, test_toolcall_reasoning_falls_back_when_empty) to validate reasoning propagation and fallback.
• src/llm/rig_adapter.rs
  • Imported futures::FutureExt and modules for panic handling (std::panic, std::sync::Once, std::sync::atomic).
  • Imported normalize_tool_reasoning for tool call processing.
  • Implemented helper functions (panic_payload_str, map_completion_panic, should_suppress_rig_openai_usage_panic_details, should_suppress_rig_openai_usage_panic) for robust panic handling.
  • Introduced install_rig_panic_suppression_hook_once, RIG_PANIC_SUPPRESSION_DEPTH, and RigPanicSuppressionGuard to manage global panic hook and suppression depth.
  • Created run_completion_guarded to wrap LLM completion calls, catching and mapping panics to LlmErrors.
  • Updated extract_response to include the reasoning field in IronToolCall with normalization.
  • Modified complete and complete_with_tools methods to use run_completion_guarded for safer execution.
  • Added new unit tests for panic handling logic, including test_map_completion_panic_*, test_should_suppress_rig_openai_usage_panic_*, and test_rig_panic_suppression_guard_refcount_balances.
• tests/openai_compat_integration.rs
  • Updated ToolCall construction in the MockLlmProvider to include a reasoning field.

Activity

• The author has run cargo clippy --all --all-features locally to ensure code quality and adherence to lints.
• The author has run cargo test locally to verify that all tests pass and the changes behave as expected.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request successfully threads per-tool reasoning through the LLM provider and tool-calling pathways, a significant enhancement for tool-use transparency. The changes are well-implemented, with consistent use of a normalization function for reasoning strings and good test coverage for the new logic. A notable addition is the robust panic handling mechanism in the rig_adapter, which improves application stability by catching and managing panics from the underlying rig-core dependency. While this is a valuable improvement, one part of its implementation could be more resilient, as detailed in the specific comment.

[gemini-code-assist]

The current implementation for detecting the panic location is brittle as it relies on a hardcoded Unix-style path separator (/). This check will fail on Windows environments where the path separator is \. To make this logic more robust and cross-platform, I suggest normalizing the path separators to a consistent format before performing the string containment check.

Suggested change

	fn should_suppress_rig_openai_usage_panic_details(
	message: Option<&str>,
	location_file: Option<&str>,
	) -> bool {
	let Some(message) = message else {
	return false;
	};
	if !message.contains("attempt to subtract with overflow") {
	return false;
	}
	let Some(file) = location_file else {
	return false;
	};
	file.contains("/src/providers/openai/completion/mod.rs") && file.contains("/rig-core-")
	}
	fn should_suppress_rig_openai_usage_panic_details(
	message: Option<&str>,
	location_file: Option<&str>,
	) -> bool {
	let Some(message) = message else {
	return false;
	};
	if !message.contains("attempt to subtract with overflow") {
	return false;
	}
	let Some(file) = location_file else {
	return false;
	};
	file.replace('\\', "/").contains("src/providers/openai/completion/mod.rs") && file.contains("rig-core-")
	}

[ilblackdragon]

Code Review

Overview

This PR extends ToolCall with a reasoning: String field so each tool call carries its own rationale (instead of sharing the response-level content). It adds a normalize_tool_reasoning() helper that falls back to a default string when providers don't supply reasoning. Additionally, it introduces a panic-catching wrapper around rig-core completions to handle a known upstream panic in rig-core's OpenAI usage tracking.

Two distinct concerns are bundled here: (1) per-tool reasoning plumbing, and (2) rig-core panic suppression. These should ideally be separate PRs, but it's not a blocker.

---

Positives

• Clean, well-scoped type change to ToolCall with consistent propagation
• Good test coverage for the new normalization logic and reasoning threading
• The StaticToolCompletionProvider mock is well-designed for testing Reasoning::select_tools
• Proper use of normalize_tool_reasoning() at every ToolCall construction site

---

Issues & Suggestions

1. Global panic hook is risky (High concern)

The install_rig_panic_suppression_hook_once() function replaces the global panic hook. This is a process-wide side effect with several problems:

• Race condition with other hooks: The warning comment acknowledges this but doesn't mitigate it. If any other component (tracing-subscriber panic layer, test harness, etc.) also sets a hook, behavior is unpredictable.
• Ordering::Relaxed on the depth counter: In should_suppress_rig_openai_usage_panic, the depth check uses Relaxed ordering. Since panic hooks can run on any thread, this should use Ordering::Acquire/Release (or at minimum SeqCst) to ensure the hook sees the updated depth from the thread that entered the guard.
• Scope creep: This panic suppression is a workaround for a bug in rig-core. Consider filing an upstream issue and pinning to a fixed version instead. If the workaround must stay, it deserves its own module/file and PR.

2. AssertUnwindSafe on async futures (Medium concern)

let response = AssertUnwindSafe(fut)
    .catch_unwind()
    .await

The // SAFETY comment claims no mutable aliases are retained, but AssertUnwindSafe on an arbitrary CompletionModel::completion future is a strong assertion. If the model implementation holds &mut self or interior mutability across .await points, a caught panic could leave the model in an inconsistent state. The RigAdapter would then be reused on the next call with potentially corrupted internal state.

Suggestion: At minimum, document that RigAdapter instances should be considered poisoned after a caught panic, or wrap the model in an Option that gets taken on panic.

3. Unnecessary allocation on every tool call (Low concern)

normalize_tool_reasoning("") allocates a new String from DEFAULT_TOOL_RATIONALE every time. Since most providers don't supply reasoning yet, this happens on virtually every tool call.

Suggestion: Consider using Cow<'static, str> as the return type, or store reasoning as Option<String> on ToolCall (where None means "use default"). This avoids cloning the static string repeatedly and is more idiomatic for "maybe has a value" semantics.

4. Behavioral change in select_tools (Medium concern)

Previously, ToolSelection::reasoning was set to the response-level content (the model's overall thinking). Now it's set to per-tool reasoning from tool_call.reasoning. But since all current providers populate reasoning as "", every selection will now get DEFAULT_TOOL_RATIONALE instead of the model's actual thinking text.

This is a regression in reasoning quality for the select_tools path until providers actually populate per-tool reasoning. The old behavior (using response.content) provided more useful context.

Suggestion: Fall back to response.content when per-tool reasoning is empty, rather than the generic default string:

reasoning: if tool_call.reasoning.trim().is_empty() {
    reasoning_from_content.clone()  // preserve old behavior
} else {
    tool_call.reasoning
}

5. Consider extracting panic suppression (Nit)

Once, AtomicUsize, and the suppression machinery add non-trivial complexity to what was described as a "reasoning plumbing" PR. Consider extracting to a separate module like src/llm/rig_panic_guard.rs.

---

Summary

Area	Assessment
Correctness	The reasoning field plumbing is correct. The panic suppression works but has ordering and safety concerns.
Conventions	Follows project patterns. Test coverage is good.
Performance	Minor: unnecessary string allocations on every tool call.
Security	No concerns.
Risk	Medium — the global panic hook and AssertUnwindSafe on async futures are the main risks. The behavioral change in select_tools reasoning is a subtle regression.

Recommendation: Address items #1 (memory ordering) and #4 (reasoning fallback regression) before merging. Consider splitting the panic suppression into a separate PR.

[panosAthDBX]

Implemented all ilblackdragon review recommendations in follow-up commit 2bab285 (ported from local split/worker-orchestrator-streaming-v2):

• removed global panic-hook suppression from rig adapter and switched to per-adapter poisoning on panic

• tightened panic handling semantics around AssertUnwindSafe by refusing reuse after panic

• changed normalize_tool_reasoning to Cow to avoid fallback allocation churn

• restored select_tools reasoning quality by falling back to response content when tool rationale is empty

• updated tests and call sites accordingly

• Validation run locally: cargo fmt, cargo clippy --all --benches --tests --examples --all-features, cargo test, cargo check --no-default-features --features libsql, cargo check --all-features.

[ilblackdragon]

Code Review

Overview

This PR adds a reasoning: String field to ToolCall so each tool call carries its own rationale, with normalize_tool_reasoning() ensuring a non-empty fallback. The field is threaded through Reasoning::select_tools, RespondResult::ToolCalls, and all adapter/provider construction sites. A second, unrelated change catches panics from a known rig-core bug in the OpenAI usage tracker.

---

Key Issues

1. Behavioral regression in select_tools (High)

src/llm/reasoning.rs:372-379 — Previously ToolSelection::reasoning used response.content (the model's actual thinking). Now it uses normalize_tool_reasoning(&tool_call.reasoning), which resolves to "Tool selected to satisfy the current subtask." for every provider since none populate per-tool reasoning yet.

This is a downgrade — callers that consumed ToolSelection::reasoning now get a useless static string instead of the model's chain-of-thought. Suggestion: fall back to response.content when per-tool reasoning is empty:

let fallback_reasoning = response.content.unwrap_or_default();
// ...
reasoning: if tool_call.reasoning.trim().is_empty() {
    fallback_reasoning.clone()
} else {
    tool_call.reasoning.trim().to_string()
},

2. Global panic hook is a footgun (High)

src/llm/rig_adapter.rs:399-462 — install_rig_panic_suppression_hook_once() replaces the global panic hook. Concerns:

• Ordering::Relaxed on RIG_PANIC_SUPPRESSION_DEPTH is incorrect. The hook runs on whatever thread panicked; Relaxed doesn't guarantee visibility of the fetch_add from the calling thread. Use Acquire/Release at minimum.
• Hook ordering fragility — any subsequent set_hook call silently removes this suppression.
• Scope: This is a workaround for a specific rig-core bug. Is there an upstream issue filed? If rig-core ≥ 0.31 fixes it, a version bump is cleaner. If the workaround is necessary, it should live in its own module (src/llm/rig_panic_guard.rs), not inline in the adapter.

3. AssertUnwindSafe on async futures (Medium)

src/llm/rig_adapter.rs:489-499 — Wrapping an arbitrary CompletionModel future in AssertUnwindSafe is technically unsound if the model holds &mut state across .await points. After a caught panic, the RigAdapter is reused with potentially corrupted model state.

Consider marking the adapter as poisoned after a caught panic, or documenting this as a known risk scoped to specific model implementations.

4. Unnecessary allocations (Low)

normalize_tool_reasoning("") allocates a new String on every tool call, and currently all providers pass "". Consider Cow<'static, str> or Option<String> on ToolCall to avoid this.

---

Positives

• Clean, consistent propagation of the new field to all ToolCall construction sites
• Good test coverage: StaticToolCompletionProvider mock, normalization unit tests, threading integration tests
• Thorough test updates — no missed construction sites

Nits

• tests/openai_compat_integration.rs:95 uses a raw string instead of normalize_tool_reasoning(...), inconsistent with the rest of the PR
• The panic suppression machinery (futures::FutureExt, Once, AtomicUsize, guard struct) adds significant complexity to a "reasoning plumbing" PR — consider splitting into a separate PR

---

Recommendation

Fix the select_tools reasoning regression (#1) and memory ordering (#2) before merge. Consider splitting panic suppression into a separate PR.

[zmanian]

Review: feat(llm): thread per-tool reasoning through provider/tool-call path

The type-level change and normalization approach are sound. normalize_tool_reasoning with DEFAULT_TOOL_RATIONALE fallback is clean, and the test coverage for the new helpers and the select_tools / respond threading is solid.

However, this PR will not compile as-is. Adding reasoning: String to ToolCall is a breaking struct change, and several production call sites that construct ToolCall were not updated:

Missing reasoning field (compilation errors)

Production providers (same layer as the files in this PR):

• src/llm/bedrock.rs ~line 518 -- ToolCall in extract_response equivalent
• src/llm/anthropic_oauth.rs ~line 550 -- ToolCall construction in response parsing

Agent/worker layer (production code):

• src/agent/session.rs ~line 349 -- rebuild_messages() constructs ToolCall from turn history
• src/agent/thread_ops.rs ~line 1660 -- tool call reconstruction from stored JSON
• src/worker/job.rs ~line 871 and ~line 1325 (selections_to_tool_calls) -- job worker tool dispatch

Test code (also won't compile):

• src/agent/agentic_loop.rs ~line 388
• src/agent/session.rs test blocks (~lines 1191, 1221, 1286)
• src/worker/job.rs ~line 871 test
• src/llm/bedrock.rs test blocks (~lines 755, 760, 798, 821, 985, 990)
• src/llm/nearai_chat.rs has some in-PR fixes but check ~lines 1456, 1505, 2125 as well

Rig panic suppression: out of scope

The run_completion_guarded / install_rig_panic_suppression_hook_once addition (~120 lines) is unrelated to the per-tool reasoning feature. It catches a known rig-core overflow panic and converts it to LlmError. While potentially useful, it:

1. Replaces the global panic hook, which affects the entire process -- this deserves its own PR with dedicated review and testing.
2. Uses AssertUnwindSafe across an async boundary, which needs careful scrutiny beyond a "reasoning threading" PR.
3. The // SAFETY comment is appreciated but AssertUnwindSafe on a future is not a soundness concern -- it's a logical correctness concern (are invariants maintained after partial execution?). The comment should address that.

Recommend splitting this into its own PR.

Minor

• In tests/openai_compat_integration.rs, the mock uses a raw string "tool selected for integration test coverage" instead of normalize_tool_reasoning(...). This is fine functionally but inconsistent with the pattern established elsewhere in the PR. Consider using the normalizer for consistency, or at least noting that non-empty strings pass through unchanged.

Verdict

The core design (field on ToolCall, normalization helper, threading through reasoning/provider) is good. But the PR needs to update ALL ToolCall construction sites to compile. Run cargo check across the full workspace to confirm. The panic suppression should be split out.

[ilblackdragon]

This has been implemented in #1513 which covers the full feature end-to-end:

• ToolCall.reasoning: Option<String> (using Option instead of mandatory String to avoid the compilation issue)
• Reasoning populated from LLM response, threaded through session model, persisted to DB, and surfaced via REPL, HTTP, and SSE/WS
• Both chat and worker job paths covered
• /reasoning [N|all] command added

All 3500+ tests pass with zero clippy warnings. Thanks for starting this work, @panosAthDBX!