From 590fedec5fdab94ca9908aefeea24ec7b02cfcbb Mon Sep 17 00:00:00 2001 From: Ashwin Giridharan Date: Tue, 30 Jun 2026 01:25:45 -0700 Subject: [PATCH 1/2] feat: add tool/ behavioral layer + wire types for tool framework (PR A) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased on main after #79 and #81 merged. Adds src/tool/ — the behavioral layer that complements the wire types already in types/tools/ (merged via PR #79): - tool/handler.rs — ToolHandler trait, ToolOutput, ToolError - tool/registry.rs — ToolType, ToolEntry, ToolRegistry::build/lookup/etc - tool/function.rs — FunctionHandler + From<&FunctionToolParam> for FunctionTool - tool/normalize.rs — ResponsesTool::to_function_tool(), From Also adds types/tools/ wire types (params.rs with ResponsesTool enum, param structs, NonEmptyToolName), EmptyToolNameError, and wires normalize into RequestPayload::to_upstream_request() so vLLM always receives Vec. 12 cassette-based tests in tool_normalization_test.rs validate the full pipeline against real multi-turn tool-call cassettes. Addresses all PR #80 review feedback: - types/ → wire shapes only; tool/ → behaviors - From<&FunctionToolParam> for FunctionTool (typed conversion) - MCP registry entries deferred to PR C (discovery not yet wired) - EmptyToolNameError in types/ (no cross-layer import) - ToolOutput derives Debug + Clone Signed-off-by: Ashwin Giridharan --- crates/agentic-core/src/lib.rs | 11 +- .../src/storage/types/response.rs | 3 +- crates/agentic-core/src/tool/function.rs | 63 +++++ crates/agentic-core/src/tool/handler.rs | 72 ++++++ crates/agentic-core/src/tool/mod.rs | 13 + crates/agentic-core/src/tool/normalize.rs | 62 +++++ crates/agentic-core/src/tool/registry.rs | 168 +++++++++++++ crates/agentic-core/src/types/io/mod.rs | 2 +- crates/agentic-core/src/types/io/tools.rs | 8 +- crates/agentic-core/src/types/mod.rs | 7 +- .../src/types/request_response.rs | 25 +- crates/agentic-core/src/types/tools/mod.rs | 11 + crates/agentic-core/src/types/tools/params.rs | 238 ++++++++++++++++++ .../tests/tool_normalization_test.rs | 194 ++++++++++++++ 14 files changed, 860 insertions(+), 17 deletions(-) create mode 100644 crates/agentic-core/src/tool/function.rs create mode 100644 crates/agentic-core/src/tool/handler.rs create mode 100644 crates/agentic-core/src/tool/mod.rs create mode 100644 crates/agentic-core/src/tool/normalize.rs create mode 100644 crates/agentic-core/src/tool/registry.rs create mode 100644 crates/agentic-core/src/types/tools/mod.rs create mode 100644 crates/agentic-core/src/types/tools/params.rs create mode 100644 crates/agentic-core/tests/tool_normalization_test.rs diff --git a/crates/agentic-core/src/lib.rs b/crates/agentic-core/src/lib.rs index 5828a68..2ef5a3e 100644 --- a/crates/agentic-core/src/lib.rs +++ b/crates/agentic-core/src/lib.rs @@ -5,6 +5,7 @@ pub mod executor; pub mod proxy; pub mod readiness; pub mod storage; +pub mod tool; pub mod types; pub mod utils; @@ -13,10 +14,12 @@ pub use storage::{ SchemaManager, StorageError, StoreResult, create_pool, create_pool_with_schema, models::{Conversation as DbConversation, Item as DbItem, Response as DbResponse}, }; +pub use tool::{FunctionHandler, ToolEntry, ToolError, ToolHandler, ToolOutput, ToolRegistry, ToolType}; pub use types::{ - FunctionTool, FunctionToolCall, FunctionToolResultMessage, IncompleteDetails, InputContent, InputImageContent, - InputItem, InputMessage, InputMessageContent, InputTextContent, InputTokenDetails, OutputItem, OutputMessage, - OutputTextContent, OutputTokenDetails, ReasoningOutput, ReasoningTextContent, RequestPayload, ResponsePayload, - ResponseUsage, ResponsesInput, ResponsesTool, ToolChoice, UpstreamRequest, + CodeInterpreterToolParam, EmptyToolNameError, FileSearchToolParam, FunctionTool, FunctionToolCall, + FunctionToolParam, FunctionToolResultMessage, IncompleteDetails, InputContent, InputImageContent, InputItem, + InputMessage, InputMessageContent, InputTextContent, InputTokenDetails, McpToolParam, NonEmptyToolName, OutputItem, + OutputMessage, OutputTextContent, OutputTokenDetails, ReasoningOutput, ReasoningTextContent, RequestPayload, + ResponsePayload, ResponseUsage, ResponsesInput, ResponsesTool, ToolChoice, UpstreamRequest, WebSearchToolParam, }; pub use utils::{utcnow_str, uuid7_str}; diff --git a/crates/agentic-core/src/storage/types/response.rs b/crates/agentic-core/src/storage/types/response.rs index b2b1d6d..6963a2a 100644 --- a/crates/agentic-core/src/storage/types/response.rs +++ b/crates/agentic-core/src/storage/types/response.rs @@ -6,7 +6,8 @@ use serde::{Deserialize, Serialize}; use super::super::models::Response as StorageDbResponse; use super::errors::StorageError; -use crate::types::io::{ResponsesTool, ToolChoice}; +use crate::types::io::ToolChoice; +use crate::types::tools::ResponsesTool; use crate::utils::common::serialize_to_string; /// Response metadata with effective configuration. diff --git a/crates/agentic-core/src/tool/function.rs b/crates/agentic-core/src/tool/function.rs new file mode 100644 index 0000000..671ed52 --- /dev/null +++ b/crates/agentic-core/src/tool/function.rs @@ -0,0 +1,63 @@ +use std::future::Future; +use std::pin::Pin; + +use serde_json::Value; + +use crate::types::io::FunctionTool; +use crate::types::tools::FunctionToolParam; + +use super::handler::{ToolError, ToolHandler, ToolOutput}; +use super::registry::ToolType; + +impl From<&FunctionToolParam> for FunctionTool { + fn from(p: &FunctionToolParam) -> Self { + Self { + type_: "function".to_owned(), + name: p.name.clone(), + description: p.description.clone(), + parameters: p.parameters.clone(), + strict: p.strict, + } + } +} + +/// Handler for `type: "function"` tools. +/// +/// Function tools are client-owned: the gateway normalises them for vLLM but +/// never executes them. `execute()` is a no-op that should never be called. +#[derive(Debug)] +pub struct FunctionHandler; + +impl ToolHandler for FunctionHandler { + fn tool_type(&self) -> ToolType { + ToolType::Function + } + + fn validate(&self, param: &Value) -> Result<(), ToolError> { + match param.get("name").and_then(Value::as_str) { + Some(name) if !name.is_empty() => Ok(()), + _ => Err(ToolError::Config("function tool must have a non-empty name".into())), + } + } + + fn normalize(&self, param: &Value) -> Vec { + // Deserialize into the typed struct so From<&FunctionToolParam> is the single + // conversion path — no risk of the manual-extraction path diverging from it. + let p: FunctionToolParam = serde_json::from_value(param.clone()) + .expect("normalize() called with invalid param — validate() must be called first"); + vec![FunctionTool::from(&p)] + } + + fn execute( + &self, + _tool_name: &str, + _arguments: &str, + _config: &Value, + ) -> Pin> + Send + '_>> { + Box::pin(async { + Err(ToolError::Execution( + "function tools are client-owned and are not executed by the gateway".into(), + )) + }) + } +} diff --git a/crates/agentic-core/src/tool/handler.rs b/crates/agentic-core/src/tool/handler.rs new file mode 100644 index 0000000..9dad8d7 --- /dev/null +++ b/crates/agentic-core/src/tool/handler.rs @@ -0,0 +1,72 @@ +use std::future::Future; +use std::pin::Pin; + +use serde_json::Value; + +use crate::types::io::FunctionTool; + +#[derive(Debug, Clone)] +pub struct ToolOutput { + pub call_id: String, + pub output: String, +} + +#[derive(Debug, thiserror::Error)] +pub enum ToolError { + #[error("execution failed: {0}")] + Execution(String), + #[error("invalid tool config: {0}")] + Config(String), +} + +/// Trait implemented by each tool type (function, MCP, `web_search`, …). +/// +/// Every tool type normalises itself to vLLM-compatible `FunctionTool` definitions +/// and, when gateway-owned, executes via `execute()`. Function tools skip +/// execution and return `requires_action` to the client. +/// +/// Implementations must be `Send + Sync` so they can be stored behind `Arc` and used across async task boundaries. +/// +/// ## Note on `async fn` in traits +/// +/// Native `async fn` in traits (Rust 1.75+) is not yet `dyn`-compatible. Since +/// PR B will store handlers as `Arc`, we use explicit +/// `Pin>` return types. If `dyn` dispatch is not needed in your +/// context, consider `#[trait_variant::make]` or `#[async_trait]`. +pub trait ToolHandler: Send + Sync { + #[must_use] + fn tool_type(&self) -> super::registry::ToolType; + + /// Validate the tool param JSON. + /// + /// # Errors + /// + /// Returns [`ToolError::Config`] for obviously invalid configurations. + fn validate(&self, param: &Value) -> Result<(), ToolError>; + + /// Normalise this tool declaration into vLLM-compatible `FunctionTool` entries. + #[must_use] + fn normalize(&self, param: &Value) -> Vec; + + /// Execute a tool call and return the result. + /// + /// This method is **never called** for `ToolType::Function` — function tools are + /// client-owned and the gateway returns `requires_action` to the caller instead. + /// + /// ## `config` parameter + /// + /// `config` is the serialised **server-level** tool param (i.e. the `*ToolParam` + /// struct stored in [`super::registry::ToolEntry::config`]). It is **not** the + /// per-tool parameter schema. + /// + /// # Errors + /// + /// Returns [`ToolError::Execution`] if the tool call fails. + fn execute( + &self, + tool_name: &str, + arguments: &str, + config: &Value, + ) -> Pin> + Send + '_>>; +} diff --git a/crates/agentic-core/src/tool/mod.rs b/crates/agentic-core/src/tool/mod.rs new file mode 100644 index 0000000..7b3b1ae --- /dev/null +++ b/crates/agentic-core/src/tool/mod.rs @@ -0,0 +1,13 @@ +//! Tool framework — registry, handler trait, and normalization pipeline. +//! +//! Wire format types (`ResponsesTool`, param structs) live in [`crate::types::tools`]. +//! This module owns the behavioral layer: routing, handler interface, and normalization. + +pub mod function; +pub mod handler; +pub mod normalize; +pub mod registry; + +pub use function::FunctionHandler; +pub use handler::{ToolError, ToolHandler, ToolOutput}; +pub use registry::{ToolEntry, ToolRegistry, ToolType}; diff --git a/crates/agentic-core/src/tool/normalize.rs b/crates/agentic-core/src/tool/normalize.rs new file mode 100644 index 0000000..faaf499 --- /dev/null +++ b/crates/agentic-core/src/tool/normalize.rs @@ -0,0 +1,62 @@ +use crate::types::io::FunctionTool; +use crate::types::io::input::FunctionToolResultMessage; +use crate::types::tools::ResponsesTool; + +use super::handler::ToolOutput; + +impl ResponsesTool { + /// Normalise this tool declaration to the `FunctionTool` wire format that vLLM understands. + /// + /// - `Function` variants convert via [`From<&FunctionToolParam>`] for `FunctionTool`. + /// Returns `None` and logs at `debug` level if the name is empty. + /// - All other variants (`Mcp`, `WebSearch`, `FileSearch`, `CodeInterpreter`) return + /// `None` and emit a `tracing::debug!` — their full handlers have not landed yet. + /// + /// This is the entry point called by `RequestPayload::to_upstream_request()` so that + /// vLLM always receives a `Vec`, never a raw `ResponsesTool` enum. + #[must_use] + pub fn to_function_tool(&self) -> Option { + match self { + ResponsesTool::Function(p) => { + if p.name.is_empty() { + tracing::debug!("function tool with empty name skipped in normalize"); + return None; + } + Some(FunctionTool::from(p)) + } + ResponsesTool::Mcp(p) => { + tracing::debug!( + server_label = %p.server_label, + "MCP tool skipped in normalize — handler not yet registered" + ); + None + } + ResponsesTool::WebSearch(_) => { + tracing::debug!("web_search tool skipped in normalize — handler not yet registered"); + None + } + ResponsesTool::FileSearch(_) => { + tracing::debug!("file_search tool skipped in normalize — handler not yet registered"); + None + } + ResponsesTool::CodeInterpreter(_) => { + tracing::debug!("code_interpreter tool skipped in normalize — handler not yet registered"); + None + } + #[allow(unreachable_patterns)] + _ => { + tracing::debug!("unknown tool type skipped in normalize"); + None + } + } + } +} + +impl From for FunctionToolResultMessage { + fn from(o: ToolOutput) -> Self { + Self { + call_id: o.call_id, + output: o.output, + } + } +} diff --git a/crates/agentic-core/src/tool/registry.rs b/crates/agentic-core/src/tool/registry.rs new file mode 100644 index 0000000..206ec2a --- /dev/null +++ b/crates/agentic-core/src/tool/registry.rs @@ -0,0 +1,168 @@ +use std::collections::HashMap; + +use serde::{Deserialize, Serialize}; +use serde_json::Value; + +use crate::types::io::output::FunctionToolCall; +use crate::types::tools::ResponsesTool; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ToolType { + Function, + Mcp, + /// Internal routing discriminant. Serializes as `"web_search"`. + /// Note: the corresponding `ResponsesTool` wire tag is `"web_search_preview"`. + /// `ToolType` is not used in wire-facing types so the names differ intentionally. + WebSearch, + FileSearch, + CodeInterpreter, +} + +/// Per-request routing entry keyed by the tool name the model will call. +#[derive(Debug, Clone)] +pub struct ToolEntry { + pub tool_type: ToolType, + /// Full serialised tool param for the executor (used during dispatch). + pub config: Value, + /// For MCP tools: which server this tool belongs to. + pub server_label: Option, +} + +/// Request-scoped registry built from `RequestPayload.tools`. +/// Maps the name the LLM sees → routing metadata. +#[derive(Debug, Default)] +pub struct ToolRegistry { + entries: HashMap, +} + +impl ToolRegistry { + /// Build a registry from the declared tools. + /// + /// Function tools with empty names are skipped with a warning. Duplicate + /// tool names result in last-write-wins, also logged at `warn` level. + /// + /// # Panics + /// + /// Panics if serialization of a tool param struct fails, which cannot happen + /// for the types defined in this module (`#[derive(Serialize)]` on plain structs). + #[must_use] + pub fn build(tools: &[ResponsesTool]) -> Self { + let mut entries = HashMap::with_capacity(tools.len()); + + for tool in tools { + match tool { + ResponsesTool::Function(p) => { + if p.name.is_empty() { + tracing::warn!("skipping function tool with empty name during registry build"); + continue; + } + if entries + .insert( + p.name.clone(), + ToolEntry { + tool_type: ToolType::Function, + config: serde_json::to_value(p).expect("serialization of known struct is infallible"), + server_label: None, + }, + ) + .is_some() + { + tracing::warn!(name = %p.name, "duplicate tool name — previous definition overwritten"); + } + } + ResponsesTool::Mcp(p) => { + // MCP tool names are discovered at request-time via `tools/list`. + // Without discovery, we cannot know which tool names to register — + // keying by server_label would cause all MCP calls to miss on lookup + // since gateway_owned/client_owned look up by tool name, not server. + // MCP entries will be populated in PR C once HttpMcpHandler + // implements discover() and the executor calls it before build(). + tracing::debug!( + server_label = %p.server_label, + "MCP server declared but skipped in registry — tool names unknown until discovery (PR C)" + ); + } + ResponsesTool::WebSearch(p) => { + entries.insert( + "web_search".to_owned(), + ToolEntry { + tool_type: ToolType::WebSearch, + config: serde_json::to_value(p).expect("serialization of known struct is infallible"), + server_label: None, + }, + ); + } + ResponsesTool::FileSearch(p) => { + entries.insert( + "file_search".to_owned(), + ToolEntry { + tool_type: ToolType::FileSearch, + config: serde_json::to_value(p).expect("serialization of known struct is infallible"), + server_label: None, + }, + ); + } + ResponsesTool::CodeInterpreter(p) => { + entries.insert( + "code_interpreter".to_owned(), + ToolEntry { + tool_type: ToolType::CodeInterpreter, + config: serde_json::to_value(p).expect("serialization of known struct is infallible"), + server_label: None, + }, + ); + } + #[allow(unreachable_patterns)] + _ => { + tracing::debug!("unknown ResponsesTool variant skipped during registry build"); + } + } + } + + Self { entries } + } + + #[must_use] + pub fn lookup(&self, tool_name: &str) -> Option<&ToolEntry> { + self.entries.get(tool_name) + } + + #[must_use] + pub fn is_empty(&self) -> bool { + self.entries.is_empty() + } + + #[must_use] + pub fn len(&self) -> usize { + self.entries.len() + } + + /// Returns the subset of `calls` whose names map to gateway-owned tools + /// (i.e. everything except `ToolType::Function`). + #[must_use] + pub fn gateway_owned<'a>(&self, calls: &'a [FunctionToolCall]) -> Vec<&'a FunctionToolCall> { + calls + .iter() + .filter(|c| { + self.entries + .get(&c.name) + .is_some_and(|e| e.tool_type != ToolType::Function) + }) + .collect() + } + + /// Returns the subset of `calls` whose names map to client-owned function + /// tools (i.e. `ToolType::Function` or unknown names). + #[must_use] + pub fn client_owned<'a>(&self, calls: &'a [FunctionToolCall]) -> Vec<&'a FunctionToolCall> { + calls + .iter() + .filter(|c| { + self.entries + .get(&c.name) + .is_none_or(|e| e.tool_type == ToolType::Function) + }) + .collect() + } +} diff --git a/crates/agentic-core/src/types/io/mod.rs b/crates/agentic-core/src/types/io/mod.rs index a8cba2c..3675995 100644 --- a/crates/agentic-core/src/types/io/mod.rs +++ b/crates/agentic-core/src/types/io/mod.rs @@ -10,6 +10,6 @@ pub use input::{ pub use output::{ ApplyDone, FunctionToolCall, OutputItem, OutputMessage, OutputTextContent, ReasoningOutput, ReasoningTextContent, }; -pub use tools::{FunctionTool, ResponsesTool, ToolChoice}; +pub use tools::{FunctionTool, ToolChoice}; pub(crate) use tools::{resolve_tool_choice, resolve_tools}; pub use usage::{InputTokenDetails, OutputTokenDetails, ResponseUsage}; diff --git a/crates/agentic-core/src/types/io/tools.rs b/crates/agentic-core/src/types/io/tools.rs index 7e6f61b..9b5c70e 100644 --- a/crates/agentic-core/src/types/io/tools.rs +++ b/crates/agentic-core/src/types/io/tools.rs @@ -1,13 +1,11 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; -fn default_function_type() -> String { - "function".to_string() -} +use crate::types::tools::ResponsesTool; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct FunctionTool { - #[serde(rename = "type", default = "default_function_type")] + #[serde(rename = "type")] pub type_: String, pub name: String, pub description: Option, @@ -15,8 +13,6 @@ pub struct FunctionTool { pub strict: Option, } -pub type ResponsesTool = FunctionTool; - #[derive(Debug, Clone, Default, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum ToolChoice { diff --git a/crates/agentic-core/src/types/mod.rs b/crates/agentic-core/src/types/mod.rs index c3a9ee8..8cdde1a 100644 --- a/crates/agentic-core/src/types/mod.rs +++ b/crates/agentic-core/src/types/mod.rs @@ -1,11 +1,16 @@ pub mod event; pub mod io; pub mod request_response; +pub mod tools; pub use io::{ FunctionTool, FunctionToolCall, FunctionToolResultMessage, InputContent, InputImageContent, InputItem, InputMessage, InputMessageContent, InputTextContent, InputTokenDetails, OutputItem, OutputMessage, OutputTextContent, OutputTokenDetails, ReasoningOutput, ReasoningTextContent, ResponseUsage, ResponsesInput, - ResponsesTool, ToolChoice, + ToolChoice, }; pub use request_response::{IncompleteDetails, RequestPayload, ResponsePayload, UpstreamRequest}; +pub use tools::{ + CodeInterpreterToolParam, EmptyToolNameError, FileSearchToolParam, FunctionToolParam, McpToolParam, + NonEmptyToolName, ResponsesTool, WebSearchToolParam, +}; diff --git a/crates/agentic-core/src/types/request_response.rs b/crates/agentic-core/src/types/request_response.rs index de89a08..444a2ee 100644 --- a/crates/agentic-core/src/types/request_response.rs +++ b/crates/agentic-core/src/types/request_response.rs @@ -2,8 +2,9 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use super::io::{ - InputItem, InputMessage, InputMessageContent, OutputItem, ResponseUsage, ResponsesInput, ResponsesTool, ToolChoice, + FunctionTool, InputItem, InputMessage, InputMessageContent, OutputItem, ResponseUsage, ResponsesInput, ToolChoice, }; +use super::tools::ResponsesTool; use crate::utils::common::serialize_to_string; #[derive(Debug, Clone, Serialize, Deserialize)] @@ -39,8 +40,12 @@ pub struct UpstreamRequest<'a> { pub stream: bool, #[serde(skip_serializing_if = "Option::is_none")] pub instructions: Option<&'a str>, + /// Normalised tools forwarded to vLLM — always `Vec` regardless of + /// what tool types the client declared. Gateway-managed types (`MCP`, `web_search`, …) + /// are normalized to function stubs; function tools pass through unchanged. + /// Skipped when empty so vLLM does not receive an empty array. #[serde(skip_serializing_if = "Option::is_none")] - pub tools: Option<&'a Vec>, + pub tools: Option>, #[serde(skip_serializing_if = "is_default_tool_choice")] pub tool_choice: &'a ToolChoice, #[serde(skip_serializing_if = "Option::is_none")] @@ -62,15 +67,27 @@ fn is_default_tool_choice(choice: &ToolChoice) -> bool { } impl RequestPayload { - /// Construct an `UpstreamRequest` borrowing from this request, suitable for forwarding to vLLM. + /// Construct an `UpstreamRequest` suitable for forwarding to vLLM. + /// + /// All tool types are normalised to `Vec` via + /// [`ResponsesTool::to_function_tool`]. Gateway-managed tool types whose handlers + /// have not yet landed (`MCP`, `web_search`, `file_search`, `code_interpreter`) are skipped + /// with a warning — vLLM only understands `type: "function"`. #[must_use] pub fn to_upstream_request(&self, stream: bool) -> UpstreamRequest<'_> { + let tools: Option> = self + .tools + .as_ref() + .map(|tools| tools.iter().filter_map(ResponsesTool::to_function_tool).collect()); + // Treat an empty normalised list the same as no tools (skip the field entirely). + let tools = tools.filter(|v| !v.is_empty()); + UpstreamRequest { model: &self.model, input: &self.input, stream, instructions: self.instructions.as_deref(), - tools: self.tools.as_ref(), + tools, tool_choice: &self.tool_choice, include: self.include.as_ref(), temperature: self.temperature, diff --git a/crates/agentic-core/src/types/tools/mod.rs b/crates/agentic-core/src/types/tools/mod.rs new file mode 100644 index 0000000..dd63b8d --- /dev/null +++ b/crates/agentic-core/src/types/tools/mod.rs @@ -0,0 +1,11 @@ +//! Wire format types for the tool framework. +//! +//! This module contains only serde shapes (serialization/deserialization types). +//! Behavioral logic (registry, handler trait, normalization) lives in [`crate::tool`]. + +pub mod params; + +pub use params::{ + CodeInterpreterToolParam, EmptyToolNameError, FileSearchToolParam, FunctionToolParam, McpToolParam, + NonEmptyToolName, ResponsesTool, WebSearchToolParam, +}; diff --git a/crates/agentic-core/src/types/tools/params.rs b/crates/agentic-core/src/types/tools/params.rs new file mode 100644 index 0000000..ab0757c --- /dev/null +++ b/crates/agentic-core/src/types/tools/params.rs @@ -0,0 +1,238 @@ +use std::collections::HashMap; + +use serde::{Deserialize, Serialize}; +use serde_json::Value; + +/// Error returned when a tool name is empty. +/// +/// Kept in `types/` so the wire-shape module stays self-contained and does +/// not import from the behavioral layer (`tool/`). +#[derive(Debug, thiserror::Error)] +#[error("tool name must not be empty")] +pub struct EmptyToolNameError; + +/// A non-empty tool name, validated at construction. +/// +/// Eliminates scattered empty-name checks by making the invalid state +/// (`name = ""`) unrepresentable. Use [`TryFrom`] or +/// [`TryFrom<&str>`] to construct; serde rejects empty strings automatically. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[serde(try_from = "String", into = "String")] +pub struct NonEmptyToolName(String); + +impl NonEmptyToolName { + /// Returns the name as a `&str`. + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + +impl TryFrom for NonEmptyToolName { + type Error = EmptyToolNameError; + + fn try_from(s: String) -> Result { + if s.is_empty() { + Err(EmptyToolNameError) + } else { + Ok(Self(s)) + } + } +} + +impl TryFrom<&str> for NonEmptyToolName { + type Error = EmptyToolNameError; + + fn try_from(s: &str) -> Result { + Self::try_from(s.to_owned()) + } +} + +impl From for String { + fn from(n: NonEmptyToolName) -> String { + n.0 + } +} + +impl AsRef for NonEmptyToolName { + fn as_ref(&self) -> &str { + &self.0 + } +} + +impl std::fmt::Display for NonEmptyToolName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) + } +} + +// Request-side tool params (serde-enum-representation, api-non-exhaustive) + +/// Wire-compatible with the existing `{"type":"function",...}` format. +/// Replaces the `pub type ResponsesTool = FunctionTool` alias in `io/tools.rs`. +/// +/// Marked `#[non_exhaustive]` because the Responses API adds new tool types +/// (e.g. `computer_use_preview`). Downstream match arms must include a catch-all. +#[non_exhaustive] +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(tag = "type")] +pub enum ResponsesTool { + #[serde(rename = "function")] + Function(FunctionToolParam), + + #[serde(rename = "mcp")] + Mcp(McpToolParam), + + #[serde(rename = "web_search_preview")] + WebSearch(WebSearchToolParam), + + #[serde(rename = "file_search")] + FileSearch(FileSearchToolParam), + + #[serde(rename = "code_interpreter")] + CodeInterpreter(CodeInterpreterToolParam), +} + +/// Parameters for a user-defined function tool. +/// +/// Does NOT carry a `type` field — serde consumes the tag during +/// deserialization and the payload struct must not also carry it. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct FunctionToolParam { + pub name: String, + pub description: Option, + pub parameters: Option, + pub strict: Option, +} + +/// Parameters for an MCP (Model Context Protocol) tool server. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct McpToolParam { + pub server_label: String, + pub server_url: String, + pub allowed_tools: Option>, + /// Per-server auth headers forwarded by the gateway (e.g. `Authorization: Bearer `). + pub headers: Option>, +} + +/// Parameters for a web search tool (no required fields). +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct WebSearchToolParam {} + +/// Parameters for a file search tool. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct FileSearchToolParam { + pub vector_store_ids: Option>, +} + +/// Parameters for a code interpreter tool (no required fields). +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct CodeInterpreterToolParam {} + +// Tests + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn non_empty_name_accepts_valid() { + let n = NonEmptyToolName::try_from("get_weather").unwrap(); + assert_eq!(n.as_str(), "get_weather"); + } + + #[test] + fn non_empty_name_rejects_empty() { + assert!(NonEmptyToolName::try_from(String::new()).is_err()); + assert!(NonEmptyToolName::try_from("").is_err()); + } + + #[test] + fn non_empty_name_serde_round_trips() { + let json = serde_json::json!("get_weather"); + let n: NonEmptyToolName = serde_json::from_value(json).unwrap(); + assert_eq!(n.as_str(), "get_weather"); + assert_eq!(serde_json::to_value(&n).unwrap(), serde_json::json!("get_weather")); + } + + #[test] + fn non_empty_name_serde_rejects_empty() { + assert!(serde_json::from_value::(serde_json::json!("")).is_err()); + } + + #[test] + fn responses_tool_function_round_trips() { + let json = serde_json::json!({ + "type": "function", + "name": "get_weather", + "description": "Get weather for a city", + "parameters": {"type": "object", "properties": {"city": {"type": "string"}}} + }); + let tool: ResponsesTool = serde_json::from_value(json).unwrap(); + assert!(matches!(tool, ResponsesTool::Function(_))); + if let ResponsesTool::Function(ref p) = tool { + assert_eq!(p.name, "get_weather"); + } + let back = serde_json::to_value(&tool).unwrap(); + assert_eq!(back["type"], "function"); + assert_eq!(back["name"], "get_weather"); + } + + #[test] + fn responses_tool_mcp_round_trips_with_field_values() { + let json = serde_json::json!({ + "type": "mcp", + "server_label": "my_server", + "server_url": "http://localhost:9000", + "allowed_tools": ["search", "fetch"] + }); + let tool: ResponsesTool = serde_json::from_value(json).unwrap(); + let back = serde_json::to_value(&tool).unwrap(); + assert_eq!(back["type"], "mcp"); + assert_eq!(back["server_label"], "my_server"); + if let ResponsesTool::Mcp(ref p) = tool { + assert_eq!(p.allowed_tools, Some(vec!["search".to_owned(), "fetch".to_owned()])); + } + } + + #[test] + fn responses_tool_web_search_round_trips() { + let json = serde_json::json!({"type": "web_search_preview"}); + let tool: ResponsesTool = serde_json::from_value(json).unwrap(); + assert!(matches!(tool, ResponsesTool::WebSearch(_))); + assert_eq!(serde_json::to_value(&tool).unwrap()["type"], "web_search_preview"); + } + + #[test] + fn responses_tool_file_search_round_trips() { + let json = serde_json::json!({"type": "file_search", "vector_store_ids": ["vs_abc"]}); + let tool: ResponsesTool = serde_json::from_value(json).unwrap(); + assert!(matches!(tool, ResponsesTool::FileSearch(_))); + let back = serde_json::to_value(&tool).unwrap(); + assert_eq!(back["type"], "file_search"); + assert_eq!(back["vector_store_ids"][0], "vs_abc"); + } + + #[test] + fn responses_tool_code_interpreter_round_trips() { + let json = serde_json::json!({"type": "code_interpreter"}); + let tool: ResponsesTool = serde_json::from_value(json).unwrap(); + assert!(matches!(tool, ResponsesTool::CodeInterpreter(_))); + assert_eq!(serde_json::to_value(&tool).unwrap()["type"], "code_interpreter"); + } + + #[test] + fn mcp_tool_param_round_trips_with_headers() { + let json = serde_json::json!({ + "type": "mcp", + "server_label": "my_server", + "server_url": "http://localhost:9000", + "headers": {"Authorization": "Bearer tok"} + }); + let tool: ResponsesTool = serde_json::from_value(json).unwrap(); + assert_eq!( + serde_json::to_value(&tool).unwrap()["headers"]["Authorization"], + "Bearer tok" + ); + } +} diff --git a/crates/agentic-core/tests/tool_normalization_test.rs b/crates/agentic-core/tests/tool_normalization_test.rs new file mode 100644 index 0000000..429aa69 --- /dev/null +++ b/crates/agentic-core/tests/tool_normalization_test.rs @@ -0,0 +1,194 @@ +//! Cassette-driven validation of the tool framework wire types and normalization pipeline. +//! +//! Validates that real cassette request bodies — the exact JSON the gateway receives — +//! parse correctly into `Vec` and normalize through the full pipeline. + +use serde::Deserialize; + +use agentic_core::tool::{ToolRegistry, ToolType}; +use agentic_core::types::request_response::RequestPayload; +use agentic_core::types::tools::ResponsesTool; + +const MULTI_TURN_DIR: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/tests/cassettes/tool_calls/multi_turn"); + +#[derive(Deserialize)] +struct TurnCassette { + turns: Vec, +} + +#[derive(Deserialize)] +struct Turn { + request: serde_yml::Value, + #[allow(dead_code)] + response: serde_yml::Value, +} + +fn load_cassette(filename: &str) -> TurnCassette { + let path = format!("{MULTI_TURN_DIR}/{filename}"); + let text = std::fs::read_to_string(&path).unwrap_or_else(|e| panic!("read {path}: {e}")); + serde_yml::from_str(&text).unwrap_or_else(|e| panic!("parse {path}: {e}")) +} + +fn tools_from_turn(turn: &Turn) -> Option { + let body = turn.request.get("body")?; + let json: serde_json::Value = serde_json::to_value(body).ok()?; + json.get("tools").cloned() +} + +/// Parse `request.body.tools` from every turn of a cassette into `Vec`. +fn assert_tools_parse(cassette_file: &str) { + let cassette = load_cassette(cassette_file); + for (i, turn) in cassette.turns.iter().enumerate() { + let Some(tools_val) = tools_from_turn(turn) else { + continue; + }; + let tools: Vec = serde_json::from_value(tools_val.clone()) + .unwrap_or_else(|e| panic!("{cassette_file} turn {i}: tools parse failed: {e}\nJSON: {tools_val}")); + assert!( + !tools.is_empty(), + "{cassette_file} turn {i}: expected non-empty tools array" + ); + } +} + +/// For every turn that has tools, verify normalization produces only `FunctionTool` entries. +fn assert_tools_normalize(cassette_file: &str) { + let cassette = load_cassette(cassette_file); + for (i, turn) in cassette.turns.iter().enumerate() { + let Some(tools_val) = tools_from_turn(turn) else { + continue; + }; + let tools: Vec = serde_json::from_value(tools_val).expect("tools parse"); + let normalized: Vec<_> = tools.iter().filter_map(ResponsesTool::to_function_tool).collect(); + for ft in &normalized { + assert_eq!( + ft.type_, "function", + "{cassette_file} turn {i}: normalized type must be 'function'" + ); + assert!( + !ft.name.is_empty(), + "{cassette_file} turn {i}: normalized name must not be empty" + ); + } + // Every Function variant must normalize — count only Function entries + // so the assertion holds even if future cassettes include gateway-only types. + let function_count = tools.iter().filter(|t| matches!(t, ResponsesTool::Function(_))).count(); + assert_eq!( + normalized.len(), + function_count, + "{cassette_file} turn {i}: each Function tool must produce exactly one FunctionTool (got {} of {})", + normalized.len(), + function_count + ); + } +} + +/// For every turn that has tools, verify `ToolRegistry::build` produces correct entries. +fn assert_registry_lookup(cassette_file: &str) { + let cassette = load_cassette(cassette_file); + for (i, turn) in cassette.turns.iter().enumerate() { + let Some(tools_val) = tools_from_turn(turn) else { + continue; + }; + let tools: Vec = serde_json::from_value(tools_val).expect("tools parse"); + let registry = ToolRegistry::build(&tools); + for tool in &tools { + if let ResponsesTool::Function(p) = tool { + let entry = registry + .lookup(&p.name) + .unwrap_or_else(|| panic!("{cassette_file} turn {i}: tool '{}' not found in registry", p.name)); + assert_eq!( + entry.tool_type, + ToolType::Function, + "{cassette_file} turn {i}: tool '{}' must be Function type", + p.name + ); + } + } + } +} + +/// Full round-trip: deserialize `request.body` → `RequestPayload` → `to_upstream_request()` +/// → assert upstream tools only contains `FunctionTool` entries. +fn assert_full_roundtrip(cassette_file: &str) { + let cassette = load_cassette(cassette_file); + for (i, turn) in cassette.turns.iter().enumerate() { + let body = turn.request.get("body").expect("turn has body"); + let json: serde_json::Value = serde_json::to_value(body).expect("body to json"); + let payload: RequestPayload = serde_json::from_value(json.clone()) + .unwrap_or_else(|e| panic!("{cassette_file} turn {i}: RequestPayload parse failed: {e}")); + let upstream = payload.to_upstream_request(false); + if let Some(tools) = &upstream.tools { + for ft in tools { + assert_eq!( + ft.type_, "function", + "{cassette_file} turn {i}: upstream tools must only contain FunctionTool" + ); + assert!( + !ft.name.is_empty(), + "{cassette_file} turn {i}: upstream tool name must not be empty" + ); + } + } + } +} + +#[test] +fn tools_parse_3turn() { + assert_tools_parse("openai_responses_tool_calls_3turn.yaml"); +} + +#[test] +fn tools_parse_5turn() { + assert_tools_parse("openai_responses_tool_calls_5turn.yaml"); +} + +#[test] +fn tools_parse_parallel() { + assert_tools_parse("openai_responses_tool_calls_parallel.yaml"); +} + +#[test] +fn tools_normalize_3turn() { + assert_tools_normalize("openai_responses_tool_calls_3turn.yaml"); +} + +#[test] +fn tools_normalize_5turn() { + assert_tools_normalize("openai_responses_tool_calls_5turn.yaml"); +} + +#[test] +fn tools_normalize_parallel() { + assert_tools_normalize("openai_responses_tool_calls_parallel.yaml"); +} + +#[test] +fn registry_lookup_3turn() { + assert_registry_lookup("openai_responses_tool_calls_3turn.yaml"); +} + +#[test] +fn registry_lookup_5turn() { + assert_registry_lookup("openai_responses_tool_calls_5turn.yaml"); +} + +#[test] +fn registry_lookup_parallel() { + assert_registry_lookup("openai_responses_tool_calls_parallel.yaml"); +} + +#[test] +fn roundtrip_3turn() { + assert_full_roundtrip("openai_responses_tool_calls_3turn.yaml"); +} + +#[test] +fn roundtrip_5turn() { + assert_full_roundtrip("openai_responses_tool_calls_5turn.yaml"); +} + +#[test] +fn roundtrip_parallel() { + assert_full_roundtrip("openai_responses_tool_calls_parallel.yaml"); +} From 8d78d0dbde88f404fbf2fbf51341389dfacfd6c3 Mon Sep 17 00:00:00 2001 From: Ashwin Giridharan Date: Tue, 30 Jun 2026 13:02:33 -0700 Subject: [PATCH 2/2] refactor: split ToolHandler + GatewayExecutor, use NonEmptyToolName in FunctionToolParam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Maral's review comments: 1. Split ToolHandler into two traits (design: every method must apply to all implementors): - ToolHandler: tool_type/validate/normalize — all tool types implement this - GatewayExecutor: ToolHandler + 'static + execute() — gateway-only tools FunctionHandler now implements only ToolHandler, making it a compile-time error to call execute() on a client-owned tool. Add _assert_gateway_executor_dyn_compatible test to lock in Arc object safety. 2. Remove #[allow(unreachable_patterns)] + _ catch-all from normalize.rs and registry.rs — compiler now enforces exhaustiveness when new ResponsesTool variants are added (Maral's inline comment). 3. FunctionToolParam.name: String -> NonEmptyToolName — serde rejects empty names at deserialization time, eliminating three divergent empty-name checks (registry build, to_function_tool, FunctionHandler validate). FunctionHandler::normalize now returns vec![] + warn on invalid param instead of panicking with .expect(). Signed-off-by: Ashwin Giridharan --- crates/agentic-core/src/lib.rs | 4 +- crates/agentic-core/src/tool/function.rs | 37 +++++++---------- crates/agentic-core/src/tool/handler.rs | 41 ++++++++++++------- crates/agentic-core/src/tool/mod.rs | 2 +- crates/agentic-core/src/tool/normalize.rs | 15 ++----- crates/agentic-core/src/tool/registry.rs | 12 ++---- crates/agentic-core/src/types/tools/params.rs | 7 +++- .../tests/tool_normalization_test.rs | 2 +- 8 files changed, 57 insertions(+), 63 deletions(-) diff --git a/crates/agentic-core/src/lib.rs b/crates/agentic-core/src/lib.rs index 2ef5a3e..93bb05a 100644 --- a/crates/agentic-core/src/lib.rs +++ b/crates/agentic-core/src/lib.rs @@ -14,7 +14,9 @@ pub use storage::{ SchemaManager, StorageError, StoreResult, create_pool, create_pool_with_schema, models::{Conversation as DbConversation, Item as DbItem, Response as DbResponse}, }; -pub use tool::{FunctionHandler, ToolEntry, ToolError, ToolHandler, ToolOutput, ToolRegistry, ToolType}; +pub use tool::{ + FunctionHandler, GatewayExecutor, ToolEntry, ToolError, ToolHandler, ToolOutput, ToolRegistry, ToolType, +}; pub use types::{ CodeInterpreterToolParam, EmptyToolNameError, FileSearchToolParam, FunctionTool, FunctionToolCall, FunctionToolParam, FunctionToolResultMessage, IncompleteDetails, InputContent, InputImageContent, InputItem, diff --git a/crates/agentic-core/src/tool/function.rs b/crates/agentic-core/src/tool/function.rs index 671ed52..b8ec324 100644 --- a/crates/agentic-core/src/tool/function.rs +++ b/crates/agentic-core/src/tool/function.rs @@ -1,19 +1,16 @@ -use std::future::Future; -use std::pin::Pin; - use serde_json::Value; use crate::types::io::FunctionTool; use crate::types::tools::FunctionToolParam; -use super::handler::{ToolError, ToolHandler, ToolOutput}; +use super::handler::{ToolError, ToolHandler}; use super::registry::ToolType; impl From<&FunctionToolParam> for FunctionTool { fn from(p: &FunctionToolParam) -> Self { Self { type_: "function".to_owned(), - name: p.name.clone(), + name: p.name.as_str().to_owned(), description: p.description.clone(), parameters: p.parameters.clone(), strict: p.strict, @@ -24,7 +21,9 @@ impl From<&FunctionToolParam> for FunctionTool { /// Handler for `type: "function"` tools. /// /// Function tools are client-owned: the gateway normalises them for vLLM but -/// never executes them. `execute()` is a no-op that should never be called. +/// never executes them. `FunctionHandler` intentionally implements only +/// [`ToolHandler`], not [`super::handler::GatewayExecutor`] — the type system +/// makes it impossible to call `execute()` on a client-owned tool. #[derive(Debug)] pub struct FunctionHandler; @@ -42,22 +41,14 @@ impl ToolHandler for FunctionHandler { fn normalize(&self, param: &Value) -> Vec { // Deserialize into the typed struct so From<&FunctionToolParam> is the single - // conversion path — no risk of the manual-extraction path diverging from it. - let p: FunctionToolParam = serde_json::from_value(param.clone()) - .expect("normalize() called with invalid param — validate() must be called first"); - vec![FunctionTool::from(&p)] - } - - fn execute( - &self, - _tool_name: &str, - _arguments: &str, - _config: &Value, - ) -> Pin> + Send + '_>> { - Box::pin(async { - Err(ToolError::Execution( - "function tools are client-owned and are not executed by the gateway".into(), - )) - }) + // conversion path. name is NonEmptyToolName so serde rejects empty names; + // any remaining deserialize error means validate() was not called first. + match serde_json::from_value::(param.clone()) { + Ok(p) => vec![FunctionTool::from(&p)], + Err(e) => { + tracing::warn!("normalize() called with invalid param: {e} — validate() must be called first"); + vec![] + } + } } } diff --git a/crates/agentic-core/src/tool/handler.rs b/crates/agentic-core/src/tool/handler.rs index 9dad8d7..7e9ba79 100644 --- a/crates/agentic-core/src/tool/handler.rs +++ b/crates/agentic-core/src/tool/handler.rs @@ -19,21 +19,13 @@ pub enum ToolError { Config(String), } -/// Trait implemented by each tool type (function, MCP, `web_search`, …). +/// Trait implemented by every tool type — client-owned and gateway-owned alike. /// -/// Every tool type normalises itself to vLLM-compatible `FunctionTool` definitions -/// and, when gateway-owned, executes via `execute()`. Function tools skip -/// execution and return `requires_action` to the client. +/// Covers validation and normalization: the steps that apply to all tools +/// regardless of who executes them. /// /// Implementations must be `Send + Sync` so they can be stored behind `Arc` and used across async task boundaries. -/// -/// ## Note on `async fn` in traits -/// -/// Native `async fn` in traits (Rust 1.75+) is not yet `dyn`-compatible. Since -/// PR B will store handlers as `Arc`, we use explicit -/// `Pin>` return types. If `dyn` dispatch is not needed in your -/// context, consider `#[trait_variant::make]` or `#[async_trait]`. pub trait ToolHandler: Send + Sync { #[must_use] fn tool_type(&self) -> super::registry::ToolType; @@ -48,12 +40,22 @@ pub trait ToolHandler: Send + Sync { /// Normalise this tool declaration into vLLM-compatible `FunctionTool` entries. #[must_use] fn normalize(&self, param: &Value) -> Vec; +} +/// Extension of [`ToolHandler`] for tool types that are executed by the gateway. +/// +/// Only gateway-owned tools (`Mcp`, `WebSearch`, `FileSearch`, `CodeInterpreter`) +/// implement this trait. Client-owned tools (`Function`) do not — the type system +/// makes it impossible to call `execute()` on them. +/// +/// ## Note on `async fn` in traits +/// +/// Native `async fn` in traits (Rust 1.75+) is not yet `dyn`-compatible. Since +/// PR B will store handlers as `Arc`, we use explicit +/// `Pin>` return types. +pub trait GatewayExecutor: ToolHandler + 'static { /// Execute a tool call and return the result. /// - /// This method is **never called** for `ToolType::Function` — function tools are - /// client-owned and the gateway returns `requires_action` to the caller instead. - /// /// ## `config` parameter /// /// `config` is the serialised **server-level** tool param (i.e. the `*ToolParam` @@ -70,3 +72,14 @@ pub trait ToolHandler: Send + Sync { config: &Value, ) -> Pin> + Send + '_>>; } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use super::*; + + // Compile-time check: Arc must be constructable. + // This fails to compile if GatewayExecutor ever becomes dyn-incompatible. + fn _assert_gateway_executor_dyn_compatible(_: Arc) {} +} diff --git a/crates/agentic-core/src/tool/mod.rs b/crates/agentic-core/src/tool/mod.rs index 7b3b1ae..6b9b49d 100644 --- a/crates/agentic-core/src/tool/mod.rs +++ b/crates/agentic-core/src/tool/mod.rs @@ -9,5 +9,5 @@ pub mod normalize; pub mod registry; pub use function::FunctionHandler; -pub use handler::{ToolError, ToolHandler, ToolOutput}; +pub use handler::{GatewayExecutor, ToolError, ToolHandler, ToolOutput}; pub use registry::{ToolEntry, ToolRegistry, ToolType}; diff --git a/crates/agentic-core/src/tool/normalize.rs b/crates/agentic-core/src/tool/normalize.rs index faaf499..de88a4b 100644 --- a/crates/agentic-core/src/tool/normalize.rs +++ b/crates/agentic-core/src/tool/normalize.rs @@ -17,13 +17,9 @@ impl ResponsesTool { #[must_use] pub fn to_function_tool(&self) -> Option { match self { - ResponsesTool::Function(p) => { - if p.name.is_empty() { - tracing::debug!("function tool with empty name skipped in normalize"); - return None; - } - Some(FunctionTool::from(p)) - } + // name is NonEmptyToolName — empty names are rejected by serde at + // deserialization time, so no runtime check is needed here. + ResponsesTool::Function(p) => Some(FunctionTool::from(p)), ResponsesTool::Mcp(p) => { tracing::debug!( server_label = %p.server_label, @@ -43,11 +39,6 @@ impl ResponsesTool { tracing::debug!("code_interpreter tool skipped in normalize — handler not yet registered"); None } - #[allow(unreachable_patterns)] - _ => { - tracing::debug!("unknown tool type skipped in normalize"); - None - } } } } diff --git a/crates/agentic-core/src/tool/registry.rs b/crates/agentic-core/src/tool/registry.rs index 206ec2a..fd8d438 100644 --- a/crates/agentic-core/src/tool/registry.rs +++ b/crates/agentic-core/src/tool/registry.rs @@ -53,13 +53,11 @@ impl ToolRegistry { for tool in tools { match tool { ResponsesTool::Function(p) => { - if p.name.is_empty() { - tracing::warn!("skipping function tool with empty name during registry build"); - continue; - } + // p.name is NonEmptyToolName — empty names are impossible here + // (serde rejects them at deserialization time). if entries .insert( - p.name.clone(), + p.name.as_str().to_owned(), ToolEntry { tool_type: ToolType::Function, config: serde_json::to_value(p).expect("serialization of known struct is infallible"), @@ -113,10 +111,6 @@ impl ToolRegistry { }, ); } - #[allow(unreachable_patterns)] - _ => { - tracing::debug!("unknown ResponsesTool variant skipped during registry build"); - } } } diff --git a/crates/agentic-core/src/types/tools/params.rs b/crates/agentic-core/src/types/tools/params.rs index ab0757c..e25a0a8 100644 --- a/crates/agentic-core/src/types/tools/params.rs +++ b/crates/agentic-core/src/types/tools/params.rs @@ -97,9 +97,12 @@ pub enum ResponsesTool { /// /// Does NOT carry a `type` field — serde consumes the tag during /// deserialization and the payload struct must not also carry it. +/// +/// `name` is a [`NonEmptyToolName`]: serde rejects empty strings at +/// deserialization time, making the invalid state unrepresentable. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct FunctionToolParam { - pub name: String, + pub name: NonEmptyToolName, pub description: Option, pub parameters: Option, pub strict: Option, @@ -171,7 +174,7 @@ mod tests { let tool: ResponsesTool = serde_json::from_value(json).unwrap(); assert!(matches!(tool, ResponsesTool::Function(_))); if let ResponsesTool::Function(ref p) = tool { - assert_eq!(p.name, "get_weather"); + assert_eq!(p.name.as_str(), "get_weather"); } let back = serde_json::to_value(&tool).unwrap(); assert_eq!(back["type"], "function"); diff --git a/crates/agentic-core/tests/tool_normalization_test.rs b/crates/agentic-core/tests/tool_normalization_test.rs index 429aa69..6065118 100644 --- a/crates/agentic-core/tests/tool_normalization_test.rs +++ b/crates/agentic-core/tests/tool_normalization_test.rs @@ -95,7 +95,7 @@ fn assert_registry_lookup(cassette_file: &str) { for tool in &tools { if let ResponsesTool::Function(p) = tool { let entry = registry - .lookup(&p.name) + .lookup(p.name.as_str()) .unwrap_or_else(|| panic!("{cassette_file} turn {i}: tool '{}' not found in registry", p.name)); assert_eq!( entry.tool_type,