feat: Add ToolResultReviewer trait for critiquing open-ended tool results

[Phoenix500526]

• I have looked for existing issues (including closed) about this

Feature Request

Add a ToolResultReviewer capability that allows critiquing tool execution results in the agentic loop, enabling smaller specialized models to guide the main LLM toward better tool usage.

Motivation

Some tools are open-ended - there are multiple ways to achieve the same goal, but some approaches are better than others. Say terminal, rag, web search or something like that

Example: Terminal tool for viewing directory structure

• Approach A: Use ls to list subdirectories, then execute ls on each subdirectory one by one
• Approach B: Use tree to get the complete directory structure in one call

From a human perspective, tree is clearly better - it's more efficient and reduces the number of tool calls.

The main LLM can tell when a tool call succeeds or fails, but it cannot evaluate whether a successful approach was optimal. A reviewer powered by a smaller, specialized model with up-to-date domain knowledge can:

1. Guide better tool usage: Suggest more efficient approaches (e.g., "use tree instead of multiple ls calls")
2. Save resources: Reduce unnecessary tool calls and token usage
3. Provide domain expertise: Smaller models can be fine-tuned or prompted with specific business knowledge

Proposal

Add ToolResultReviewer trait to rig-core:

 pub trait ToolResultReviewer: Clone + WasmCompatSend + WasmCompatSync {
     fn critique(
         &self,
         tool_name: &str,
         tool_call_id: Option<String>,
         args: &str,
         result: &str,
         cancel_sig: CancelSignal,
     ) -> impl Future<Output = String> + WasmCompatSend {
         let result = result.to_string();
         async { result }
     }
 }

Key design decisions:

• critique() returns String directly - the implementer controls the final output format (important for JSON tool results)
• Only called on successful tool executions - failures should be passed directly to the LLM
• Attached via with_reviewer() builder method on PromptRequest

Alternatives

Instead of a separate reviewer trait, each tool could implement its own critique logic internally.

Drawbacks:

• Code duplication: Open-ended tools like Terminal, RAG, and Web Search all need similar critique functionality
• Tight coupling: Mixes critique concerns with tool execution logic
• Less flexible: Cannot easily swap or configure different critique strategies

The ToolResultReviewer trait provides a clean separation of concerns and allows reuse across multiple tools.

[linear]

RIG-1112

[joshua-mo-143]

Hi @Phoenix500526, thanks for opening an issue!

I am not sure this is in scope at the moment. We might need to refactor the regular agent pattern into a state machine for it to accommodate something like this (and then we can set the machine state from prompt hooks as required, rather than having to very carefully think about what to do with this).

[Phoenix500526]

Thanks for the feedback @joshua-mo-143

A few thoughts:

1. Why not merge into PromptHook directly: I actually considered this initially. However, PromptHook methods are currently observer-only - they don't interact with the LLM. ToolResultReviewer.critique() needs to return a modified result back to the LLM, which is fundamentally different. I'm afraid that merging these two concepts might dilute the abstraction and confuse existing users. Also, while the critique feature is useful, achieving optimal results likely requires a fine-tuned model with domain knowledge. The observer pattern in PromptHook is more general-purpose. Perhaps we can start with this separation, gather feedback, and let that inform future architectural evolution.

2. Upcoming changes to on_tool_call: I'm also working on modifying PromptHook.on_tool_call to return Result<(), String> instead of (). This enables pre-execution interception for permission requests (per ACP, permissions need to be bound to tool_call_id). Currently, permission checks must happen inside the Tool::call method, but the Tool trait doesn't receive tool_call_id, creating a mismatch. If we decide to merge ToolResultReviewer into PromptHook later, I can do it together with the on_tool_call changes. I'll file another issue to describe my solution.

[joshua-mo-143]

1. Why not merge into PromptHook directly: I actually considered this initially. However, `PromptHook` methods are currently observer-only - they don't interact with the LLM. `ToolResultReviewer.critique()` needs to return a modified result back to the LLM, which is fundamentally different. I'm afraid that merging these two concepts might dilute the abstraction and confuse existing users. Also, while the critique feature is useful, achieving optimal results likely requires a fine-tuned model with domain knowledge. The observer pattern in `PromptHook` is more general-purpose. Perhaps we can start with this separation, gather feedback, and let that inform future architectural evolution.

This is likely out of scope of the original agent loop for now until we can figure out the best way to re-architect it.

2. Upcoming changes to `on_tool_call`: I'm also working on modifying `PromptHook.on_tool_call` to return `Result<(), String>` instead of `()`. This enables pre-execution interception for permission requests (per ACP, permissions need to be bound to `tool_call_id`). Currently, permission checks must happen inside the `Tool::call` method, but the `Tool` trait doesn't receive `tool_call_id`, creating a mismatch. If we decide to merge `ToolResultReviewer` into `PromptHook` later, I can do it together with the `on_tool_call` changes. I'll file another issue to describe my solution.

Is this intended to terminate the agent loop early? Because if it is, returning a String error from the prompt hook is not the right way to do it. It would be much easier to add a termination reason field to the cancel signal (ie, a OnceLock), then return that rather than having to change the type signature. We should try to avoid breaking user space where possible.

[Phoenix500526]

Is this intended to terminate the agent loop early? Because if it is, returning a String error from the prompt hook is not the right way to do it. It would be much easier to add a termination reason field to the cancel signal (ie, a OnceLock), then return that rather than having to change the type signature. We should try to avoid breaking user space where possible.

This is not intended to terminate the agent loop. The use case is:

1. LLM may attempt to execute dangerous/unwanted commands due to hallucination or other reasons
2. We need to intercept and reject that specific tool call (not terminate the loop)
3. Return a message to the LLM like "This command is not allowed, please try a different approach"
4. LLM continues and tries alternative approaches

Only if the LLM repeatedly makes bad attempts would we use Cancel Signal to terminate the loop.

So the semantics of Result<(), String> would be:

• Ok(()) → allow tool execution
• Err(reason) → reject this tool call, send reason back to LLM as the tool result

This is different from termination - the loop continues, but the tool doesn't execute and the LLM gets feedback to adjust its approach.