From c87525d81fe5e7833a0bba19ca811399d69d21ca Mon Sep 17 00:00:00 2001 From: Henry Park Date: Thu, 5 Mar 2026 18:20:56 -0800 Subject: [PATCH] fix: sort tool_definitions() for deterministic LLM tool ordering (#582) * fix: sort tool_definitions() for deterministic LLM tool ordering HashMap iteration order is non-deterministic, causing the LLM to receive tools in different orders across calls. Sort alphabetically by name to eliminate position bias in tool selection. Closes #566 Co-Authored-By: Claude Opus 4.6 (1M context) * refactor: use sort_unstable_by for tool definitions ordering Stable sort is unnecessary since tool names are unique. Unstable sort avoids the overhead of preserving equal-element order. Co-Authored-By: Claude Opus 4.6 (1M context) * fix: repair bad merge in registry.rs (missing closing brace and test attribute) The merge of main into fix/sort-tool-definitions dropped the closing `}` of test_tool_definitions_sorted_alphabetically and the `#[tokio::test]` attribute on test_retain_only_filters_tools, causing an unclosed delimiter parse error that failed all CI jobs. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/tools/registry.rs | 53 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/src/tools/registry.rs b/src/tools/registry.rs index 3775c480..a7b09b3f 100644 --- a/src/tools/registry.rs +++ b/src/tools/registry.rs @@ -195,7 +195,8 @@ impl ToolRegistry { /// Get tool definitions for LLM function calling. pub async fn tool_definitions(&self) -> Vec { - self.tools + let mut defs: Vec = self + .tools .read() .await .values() @@ -204,7 +205,9 @@ impl ToolRegistry { description: tool.description().to_string(), parameters: tool.parameters_schema(), }) - .collect() + .collect(); + defs.sort_unstable_by(|a, b| a.name.cmp(&b.name)); + defs } /// Get tool definitions for specific tools. @@ -760,6 +763,52 @@ mod tests { assert_ne!(desc, "EVIL SHADOW"); } + #[tokio::test] + async fn test_tool_definitions_sorted_alphabetically() { + // Create tools with names that would NOT be alphabetical if inserted in this order. + struct ToolZ; + struct ToolA; + struct ToolM; + + macro_rules! impl_tool { + ($ty:ident, $name:expr) => { + #[async_trait::async_trait] + impl Tool for $ty { + fn name(&self) -> &str { + $name + } + fn description(&self) -> &str { + $name + } + fn parameters_schema(&self) -> serde_json::Value { + serde_json::json!({}) + } + async fn execute( + &self, + _: serde_json::Value, + _: &crate::context::JobContext, + ) -> Result { + unreachable!() + } + } + }; + } + + impl_tool!(ToolZ, "zebra"); + impl_tool!(ToolA, "alpha"); + impl_tool!(ToolM, "middle"); + + let registry = ToolRegistry::new(); + // Register in non-alphabetical order + registry.register(Arc::new(ToolZ)).await; + registry.register(Arc::new(ToolA)).await; + registry.register(Arc::new(ToolM)).await; + + let defs = registry.tool_definitions().await; + let names: Vec<&str> = defs.iter().map(|d| d.name.as_str()).collect(); + assert_eq!(names, vec!["alpha", "middle", "zebra"]); + } + #[tokio::test] async fn test_retain_only_filters_tools() { let registry = ToolRegistry::new();