From 7e3b1821093464d5bc9670b64abfdf5b043b9566 Mon Sep 17 00:00:00 2001
From: Pi Agent <agent@pi-agent.local>
Date: Wed, 11 Mar 2026 13:30:02 +0100
Subject: [PATCH] refactor: deduplicate make_params test helper in tool-broker
 (issue #224)

Extract shared make_params() into crate-level test_util module and replace
6 identical copies across dispatch, fs_read, fs_write, run_code, run_shell,
and package_install test modules.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
---
 services/tool-broker/src/dispatch.rs                 |  8 +-------
 services/tool-broker/src/executors/fs_read.rs        |  8 +-------
 services/tool-broker/src/executors/fs_write.rs       |  8 +-------
 .../tool-broker/src/executors/package_install.rs     |  8 +-------
 services/tool-broker/src/executors/run_code.rs       |  8 +-------
 services/tool-broker/src/executors/run_shell.rs      |  8 +-------
 services/tool-broker/src/lib.rs                      | 12 ++++++++++++
 7 files changed, 18 insertions(+), 42 deletions(-)

diff --git a/services/tool-broker/src/dispatch.rs b/services/tool-broker/src/dispatch.rs
index 6488b8d..6c01a63 100644
--- a/services/tool-broker/src/dispatch.rs
+++ b/services/tool-broker/src/dispatch.rs
@@ -294,13 +294,7 @@ impl ToolDispatcher {
 #[cfg(test)]
 mod tests {
     use super::*;
-
-    fn make_params(pairs: &[(&str, &str)]) -> HashMap<String, String> {
-        pairs
-            .iter()
-            .map(|(k, v)| (k.to_string(), v.to_string()))
-            .collect()
-    }
+    use crate::test_util::make_params;
 
     #[tokio::test]
     async fn test_internal_executor_success() {
diff --git a/services/tool-broker/src/executors/fs_read.rs b/services/tool-broker/src/executors/fs_read.rs
index 356ba8a..5802dfe 100644
--- a/services/tool-broker/src/executors/fs_read.rs
+++ b/services/tool-broker/src/executors/fs_read.rs
@@ -211,15 +211,9 @@ impl ToolExecutor for FsReadExecutor {
 mod tests {
     use super::*;
     use std::os::unix::fs::PermissionsExt;
+    use crate::test_util::make_params;
     use tempfile::tempdir;
 
-    fn make_params(pairs: &[(&str, &str)]) -> HashMap<String, String> {
-        pairs
-            .iter()
-            .map(|(k, v)| (k.to_string(), v.to_string()))
-            .collect()
-    }
-
     #[tokio::test]
     async fn test_read_existing_file() {
         let dir = tempdir().unwrap();
diff --git a/services/tool-broker/src/executors/fs_write.rs b/services/tool-broker/src/executors/fs_write.rs
index 94be79c..f45f610 100644
--- a/services/tool-broker/src/executors/fs_write.rs
+++ b/services/tool-broker/src/executors/fs_write.rs
@@ -117,15 +117,9 @@ impl ToolExecutor for FsWriteExecutor {
 mod tests {
     use super::*;
     use std::os::unix::fs::PermissionsExt;
+    use crate::test_util::make_params;
     use tempfile::tempdir;
 
-    fn make_params(pairs: &[(&str, &str)]) -> HashMap<String, String> {
-        pairs
-            .iter()
-            .map(|(k, v)| (k.to_string(), v.to_string()))
-            .collect()
-    }
-
     #[tokio::test]
     async fn test_write_new_file() {
         let dir = tempdir().unwrap();
diff --git a/services/tool-broker/src/executors/package_install.rs b/services/tool-broker/src/executors/package_install.rs
index 0e35528..719dc49 100644
--- a/services/tool-broker/src/executors/package_install.rs
+++ b/services/tool-broker/src/executors/package_install.rs
@@ -297,13 +297,7 @@ fn build_output(
 #[cfg(test)]
 mod tests {
     use super::*;
-
-    fn make_params(pairs: &[(&str, &str)]) -> HashMap<String, String> {
-        pairs
-            .iter()
-            .map(|(k, v)| (k.to_string(), v.to_string()))
-            .collect()
-    }
+    use crate::test_util::make_params;
 
     // --- Package name validation ---
 
diff --git a/services/tool-broker/src/executors/run_code.rs b/services/tool-broker/src/executors/run_code.rs
index 9e964a2..327c510 100644
--- a/services/tool-broker/src/executors/run_code.rs
+++ b/services/tool-broker/src/executors/run_code.rs
@@ -394,13 +394,7 @@ fn build_output(
 #[cfg(test)]
 mod tests {
     use super::*;
-
-    fn make_params(pairs: &[(&str, &str)]) -> HashMap<String, String> {
-        pairs
-            .iter()
-            .map(|(k, v)| (k.to_string(), v.to_string()))
-            .collect()
-    }
+    use crate::test_util::make_params;
 
     fn fast_executor() -> RunCodeExecutor {
         RunCodeExecutor::with_limits(
diff --git a/services/tool-broker/src/executors/run_shell.rs b/services/tool-broker/src/executors/run_shell.rs
index 7bda47c..f729cce 100644
--- a/services/tool-broker/src/executors/run_shell.rs
+++ b/services/tool-broker/src/executors/run_shell.rs
@@ -345,13 +345,7 @@ fn build_output(
 #[cfg(test)]
 mod tests {
     use super::*;
-
-    fn make_params(pairs: &[(&str, &str)]) -> HashMap<String, String> {
-        pairs
-            .iter()
-            .map(|(k, v)| (k.to_string(), v.to_string()))
-            .collect()
-    }
+    use crate::test_util::make_params;
 
     fn fast_executor() -> RunShellExecutor {
         RunShellExecutor::with_limits(
diff --git a/services/tool-broker/src/lib.rs b/services/tool-broker/src/lib.rs
index 020f719..4708376 100644
--- a/services/tool-broker/src/lib.rs
+++ b/services/tool-broker/src/lib.rs
@@ -9,3 +9,15 @@ pub mod loop_detector;
 pub mod manifest;
 pub mod result_tagger;
 pub mod service;
+
+#[cfg(test)]
+pub(crate) mod test_util {
+    use std::collections::HashMap;
+
+    pub fn make_params(pairs: &[(&str, &str)]) -> HashMap<String, String> {
+        pairs
+            .iter()
+            .map(|(k, v)| (k.to_string(), v.to_string()))
+            .collect()
+    }
+}
-- 
2.49.1