fix(core): harden canonical tool execution semantics

Author: gold-silver-copper

CHANGELOG.md

@@ -15,8 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Dynamic tools: `ToolDyn` is removed from the public API; use `DynamicTool` for runtime-defined tools. Rig's erased dispatch trait is private. Typed tools use `Tool::NAME` as their sole identity; runtime-named agents convert explicitly with `Agent::into_tool()`.
   - Registration vocabulary: `AgentBuilder::tools(Vec<DynamicTool>)` becomes `dynamic_tools`, while retrieval-backed `dynamic_tools(sample, index, toolset)` becomes `retrieved_tools`. On `ToolSetBuilder`, `runtime_tool(DynamicTool)` becomes `dynamic_tool`, and the former embedding-backed `dynamic_tool(ToolEmbedding)` becomes `retrieved_tool`.
   - Results and errors: replace `ToolError`, `ToolFailure`, `ToolFailureKind`, `ToolReturn`, `ToolReturnOutcome`, `ToolExecutionResult`, and `ToolOutcome` with `ToolExecutionError`, `ToolErrorKind`, and the read-only `ToolResult` observed by hooks.
-  - Model presentation: serializable outputs convert once into `ToolOutput`; strings remain literal text, structured values remain JSON, and multimodal tools return explicit `ToolOutput::Content`. Result hooks now rewrite `ToolOutput`, provider adapters render JSON only at their terminal wire boundary, mixed user/tool-result blocks retain order, and Rig never reparses strings to infer rich content.
-  - Error privacy: operator-facing error messages reach the model only when explicitly supplied with `with_model_feedback`; otherwise Rig emits stable kind-specific feedback. MCP responses preserve ordered supported text/image content, retain unsupported and future blocks as typed JSON, and attach raw `CallToolResult`, `structuredContent`, and response metadata to `ToolContext`. MCP list installation and refresh are atomic and ownership-aware, so stale handlers cannot replace or remove newer registrations.
+  - Model presentation: serializable outputs convert once into canonical `ToolOutput` content blocks; strings remain literal text, explicit `serde_json::Value` values remain JSON, and multimodal tools use `ToolOutput::content` / `ToolOutput::one` or return typed `ToolResultContent` directly. Result hooks now rewrite `ToolOutput`, provider adapters render JSON only at their terminal wire boundary, mixed user/tool-result blocks retain order, and Rig never reparses strings to infer rich content.
+  - Error presentation: detailed execution-error messages remain model-visible by default so argument, validation, and custom tool errors are actionable. Use `redact_model_feedback` when diagnostics contain secrets, `with_model_feedback` for replacement text, or `with_model_output` for JSON/multimodal feedback. MCP responses preserve ordered supported text/image content, retain unsupported and future blocks as typed JSON, and attach raw `CallToolResult`, `structuredContent`, and response metadata to `ToolContext`. MCP list installation and refresh are atomic and ownership-aware, so stale handlers cannot replace or remove newer registrations.
   - Dispatch: replace `ToolSet::{call, call_with_extensions, call_structured}` with `ToolSet::execute`; replace `ToolServerHandle::{call_tool, call_tool_with_extensions, call_tool_structured}` with `ToolServerHandle::execute`.
   - Registration and definitions: `ToolSet` is the single ordered registry and records whether each tool is always advertised or retrieval-only. `ToolSet::{get_tool_definitions, documents}` are now synchronous and infallible, `ToolServerHandle` registration/removal methods no longer return an artificial `Result`, and the obsolete `ToolSetError` is removed.
   - Hooks: replace `AgentHook::on_event`, `StepEvent`, and `Flow` with the event-specific `AgentHook` methods and their corresponding action types (`CompletionCallAction`, `ToolCallAction`, `ToolResultAction`, `InvalidToolCallAction`, and `ObservationAction`). Invalid-tool hooks return `None` to defer; every explicit action, including `Fail`, is terminal for that hook stack.

crates/rig-core/CHANGELOG.md

@@ -15,8 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Dynamic tools: `ToolDyn` is removed from the public API; use `DynamicTool` for runtime-defined tools. Rig's erased dispatch trait is private. Typed tools use `Tool::NAME` as their sole identity; runtime-named agents convert explicitly with `Agent::into_tool()`.
   - Registration vocabulary: `AgentBuilder::tools(Vec<DynamicTool>)` becomes `dynamic_tools`, while retrieval-backed `dynamic_tools(sample, index, toolset)` becomes `retrieved_tools`. On `ToolSetBuilder`, `runtime_tool(DynamicTool)` becomes `dynamic_tool`, and the former embedding-backed `dynamic_tool(ToolEmbedding)` becomes `retrieved_tool`.
   - Results and errors: replace `ToolError`, `ToolFailure`, `ToolFailureKind`, `ToolReturn`, `ToolReturnOutcome`, `ToolExecutionResult`, and `ToolOutcome` with `ToolExecutionError`, `ToolErrorKind`, and the read-only `ToolResult` observed by hooks.
-  - Model presentation: serializable outputs convert once into `ToolOutput`; strings remain literal text, structured values remain JSON, and multimodal tools return explicit `ToolOutput::Content`. Result hooks now rewrite `ToolOutput`, provider adapters render JSON only at their terminal wire boundary, mixed user/tool-result blocks retain order, and Rig never reparses strings to infer rich content.
-  - Error privacy: operator-facing error messages reach the model only when explicitly supplied with `with_model_feedback`; otherwise Rig emits stable kind-specific feedback. MCP responses preserve ordered supported text/image content, retain unsupported and future blocks as typed JSON, and attach raw `CallToolResult`, `structuredContent`, and response metadata to `ToolContext`. MCP list installation and refresh are atomic and ownership-aware, so stale handlers cannot replace or remove newer registrations.
+  - Model presentation: serializable outputs convert once into canonical `ToolOutput` content blocks; strings remain literal text, explicit `serde_json::Value` values remain JSON, and multimodal tools use `ToolOutput::content` / `ToolOutput::one` or return typed `ToolResultContent` directly. Result hooks now rewrite `ToolOutput`, provider adapters render JSON only at their terminal wire boundary, mixed user/tool-result blocks retain order, and Rig never reparses strings to infer rich content.
+  - Error presentation: detailed execution-error messages remain model-visible by default so argument, validation, and custom tool errors are actionable. Use `redact_model_feedback` when diagnostics contain secrets, `with_model_feedback` for replacement text, or `with_model_output` for JSON/multimodal feedback. MCP responses preserve ordered supported text/image content, retain unsupported and future blocks as typed JSON, and attach raw `CallToolResult`, `structuredContent`, and response metadata to `ToolContext`. MCP list installation and refresh are atomic and ownership-aware, so stale handlers cannot replace or remove newer registrations.
   - Dispatch: replace `ToolSet::{call, call_with_extensions, call_structured}` with `ToolSet::execute`; replace `ToolServerHandle::{call_tool, call_tool_with_extensions, call_tool_structured}` with `ToolServerHandle::execute`.
   - Registration and definitions: `ToolSet` is the single ordered registry and records whether each tool is always advertised or retrieval-only. `ToolSet::{get_tool_definitions, documents}` are now synchronous and infallible, `ToolServerHandle` registration/removal methods no longer return an artificial `Result`, and the obsolete `ToolSetError` is removed.
   - Hooks: replace `AgentHook::on_event`, `StepEvent`, and `Flow` with the event-specific `AgentHook` methods and their corresponding action types (`CompletionCallAction`, `ToolCallAction`, `ToolResultAction`, `InvalidToolCallAction`, and `ObservationAction`). Invalid-tool hooks return `None` to defer; every explicit action, including `Fail`, is terminal for that hook stack.

crates/rig-core/src/agent/completion.rs

@@ -11,7 +11,7 @@ use crate::{
     json_utils,
     message::ToolChoice,
     streaming::{StreamingChat, StreamingCompletion, StreamingPrompt},
-    tool::server::ToolServerHandle,
+    tool::server::{ToolRegistrySnapshot, ToolServerHandle},
     vector_store::{VectorStoreError, request::VectorSearchRequest},
     wasm_compat::WasmCompatSend,
 };
@@ -33,6 +33,8 @@ pub type DynamicContextStore = Arc<
 /// to the provider for this turn.
 pub(crate) struct PreparedCompletionRequest<M: CompletionModel> {
     pub(crate) builder: CompletionRequestBuilder<M>,
+    /// Exact implementations behind this turn's provider definitions.
+    pub(crate) tool_snapshot: Arc<ToolRegistrySnapshot>,
     pub(crate) executable_tool_names: BTreeSet<String>,
     pub(crate) allowed_tool_names: BTreeSet<String>,
     /// When Tool output mode is active, the name of the synthetic output tool
@@ -325,7 +327,7 @@ pub(crate) async fn build_prepared_completion_request<M: CompletionModel>(
     // Fetch dynamic (RAG) documents and the real executable tool set first, so we
     // can resolve the output mode (which depends on whether tools exist) before
     // building the preamble and request.
-    let (mut tooldefs, fetched_context): (Vec<crate::completion::ToolDefinition>, Vec<Document>) =
+    let (mut tool_snapshot, fetched_context): (ToolRegistrySnapshot, Vec<Document>) =
         match &rag_text {
             Some(text) => {
                 let search_futures = dynamic_context.iter().map(|(num_sample, index)| {
@@ -366,21 +368,25 @@ pub(crate) async fn build_prepared_completion_request<M: CompletionModel>(
                     .flatten()
                     .collect();
 
-                let tooldefs = tool_server_handle
-                    .get_tool_defs(Some(text.to_string()))
+                let tool_snapshot = tool_server_handle
+                    .snapshot_tool_defs(Some(text.to_string()))
                     .await
                     .map_err(|_| {
                         CompletionError::RequestError("Failed to get tool definitions".into())
                     })?;
 
-                (tooldefs, fetched_context)
+                (tool_snapshot, fetched_context)
             }
             None => {
-                let tooldefs = tool_server_handle.get_tool_defs(None).await.map_err(|_| {
-                    CompletionError::RequestError("Failed to get tool definitions".into())
-                })?;
-
-                (tooldefs, Vec::new())
+                let tool_snapshot =
+                    tool_server_handle
+                        .snapshot_tool_defs(None)
+                        .await
+                        .map_err(|_| {
+                            CompletionError::RequestError("Failed to get tool definitions".into())
+                        })?;
+
+                (tool_snapshot, Vec::new())
             }
         };
 
@@ -392,8 +398,13 @@ pub(crate) async fn build_prepared_completion_request<M: CompletionModel>(
     // narrowed set could commit a name that collides once the filter lifts.
     // Without a filter the full set equals `executable_tool_names` below, so we
     // skip the extra allocation and reuse that.
-    let pre_filter_tool_names: Option<BTreeSet<String>> =
-        active_tools.map(|_| tooldefs.iter().map(|tool| tool.name.clone()).collect());
+    let pre_filter_tool_names: Option<BTreeSet<String>> = active_tools.map(|_| {
+        tool_snapshot
+            .definitions()
+            .iter()
+            .map(|tool| tool.name.clone())
+            .collect()
+    });
 
     // Apply a per-turn `active_tools` allow-list (from a `CompletionCall` hook):
     // narrow the advertised tool set to the named tools BEFORE computing the
@@ -403,20 +414,25 @@ pub(crate) async fn build_prepared_completion_request<M: CompletionModel>(
     // allow-list. A name that isn't available this turn is a hook bug, surfaced
     // as a request error (mirroring `ToolChoice::Specific`'s contract).
     if let Some(allow) = active_tools {
-        if let Some(missing) = allow
-            .iter()
-            .find(|name| !tooldefs.iter().any(|tool| &tool.name == *name))
-        {
+        if let Some(missing) = allow.iter().find(|name| {
+            !tool_snapshot
+                .definitions()
+                .iter()
+                .any(|tool| &tool.name == *name)
+        }) {
             return Err(CompletionError::RequestError(
                 format!(
                     "active_tools requested tool `{missing}`, which is not available this turn"
                 )
                 .into(),
             ));
         }
-        tooldefs.retain(|tool| allow.iter().any(|name| name == &tool.name));
+        let allowed: BTreeSet<String> = allow.iter().cloned().collect();
+        tool_snapshot.retain_names(&allowed);
     }
 
+    let mut tooldefs = tool_snapshot.definitions().to_vec();
+
     // Executable tools are the real tool-server tools, computed BEFORE any
     // synthetic output tool is appended.
     let executable_tool_names: BTreeSet<String> =
@@ -604,6 +620,7 @@ pub(crate) async fn build_prepared_completion_request<M: CompletionModel>(
 
     Ok(PreparedCompletionRequest {
         builder: completion_request,
+        tool_snapshot: Arc::new(tool_snapshot),
         executable_tool_names,
         allowed_tool_names,
         output_tool_name,

crates/rig-core/src/agent/hook.rs

@@ -1,8 +1,25 @@
 //! Event-specific hooks for observing and steering an agent run.
 //!
-//! Each lifecycle method returns an action type containing only actions that
-//! event can honor. Request patches merge, tool argument/result rewrites chain,
-//! and stop actions short-circuit in registration order.
+//! [`AgentHook`] replaces the old universal event/action pair with one lifecycle
+//! method and one action type per event. Unsupported combinations are therefore
+//! rejected by the compiler instead of being interpreted at runtime.
+//!
+//! Hooks run in registration order through [`HookStack`]. Completion-call
+//! [`RequestPatch`] values accumulate and merge; tool-call argument rewrites and
+//! tool-result presentation rewrites chain into later hooks. Nested stacks obey
+//! the same rules as flat stacks, including preserving an argument rewrite when
+//! an inner stack later skips or stops. Every stop action short-circuits the
+//! remaining hooks for that event.
+//!
+//! Register observe-only hooks before steering hooks when every observation is
+//! required: a steering stop intentionally prevents later observers from
+//! running. Tool-result rewrites change only the model-visible `presentation`;
+//! [`ToolResultEvent::raw_result`] and its [`ToolResultEvent::tool_context`]
+//! remain the execution truth used by policy and telemetry.
+//!
+//! Blocking and streaming agents share the same request, tool-call, and
+//! tool-result resolution path. Streaming adds delta-specific observations, but
+//! shared lifecycle actions have identical semantics on both surfaces.
 
 use std::collections::HashMap;
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -1107,7 +1124,7 @@ impl<M: CompletionModel> HookStack<M> {
     ) -> (ToolCallAction, Option<serde_json::Value>) {
         let mut effective = None;
         for hook in &self.hooks {
-            let rewritten = effective.as_ref().map(json_utils::value_to_json_string);
+            let rewritten = effective.as_ref().map(json_utils::serialize_json_value);
             let current = ToolCall {
                 args: rewritten.as_deref().unwrap_or(event.args),
                 ..event
@@ -1420,7 +1437,7 @@ mod tests {
             *seen.lock().unwrap(),
             vec![
                 (
-                    "tool execution timed out".into(),
+                    "raw failure".into(),
                     ToolErrorKind::Timeout,
                     "request-metadata".into()
                 ),
@@ -1431,7 +1448,7 @@ mod tests {
                 ),
             ]
         );
-        assert_eq!(raw.output().as_text(), Some("tool execution timed out"));
+        assert_eq!(raw.output().as_text(), Some("raw failure"));
         assert_eq!(
             context.result::<String>().map(String::as_str),
             Some("request-metadata")
@@ -2013,6 +2030,43 @@ mod migrated_tests {
         assert_eq!(calls.load(Ordering::Relaxed), 1);
     }
 
+    #[tokio::test]
+    async fn string_rewrite_is_json_encoded_for_later_hook_in_same_stack() {
+        let spy = ArgsSpy::default();
+        let replacement = Value::String("sanitized".into());
+        let mut stack = HookStack::<M>::new();
+        stack.push(RewriteHook(replacement.clone()));
+        stack.push(spy.clone());
+
+        let (action, salvaged) = resolve(&stack).await;
+
+        assert_eq!(action, ToolCallAction::rewrite(replacement.clone()));
+        assert_eq!(salvaged, None);
+        assert_eq!(
+            spy.0.lock().unwrap().as_slice(),
+            [serde_json::to_string(&replacement).unwrap()]
+        );
+    }
+
+    #[tokio::test]
+    async fn string_rewrite_is_json_encoded_for_hook_in_nested_stack() {
+        let spy = ArgsSpy::default();
+        let replacement = Value::String("sanitized".into());
+        let inner = HookStack::<M>::with(spy.clone());
+        let mut outer = HookStack::<M>::new();
+        outer.push(RewriteHook(replacement.clone()));
+        outer.push(inner);
+
+        let (action, salvaged) = resolve(&outer).await;
+
+        assert_eq!(action, ToolCallAction::rewrite(replacement.clone()));
+        assert_eq!(salvaged, None);
+        assert_eq!(
+            spy.0.lock().unwrap().as_slice(),
+            [serde_json::to_string(&replacement).unwrap()]
+        );
+    }
+
     #[tokio::test]
     async fn nested_rewrite_then_skip_preserves_rewrite() {
         let mut inner = HookStack::<M>::new();