clean & tools

Cargo.lock

@@ -50,6 +50,17 @@ dependencies = [
  "syn",
 ]
 
+[[package]]
+name = "async-trait"
+version = "0.1.89"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9035ad2d096bed7955a320ee7e2230574d28fd3c3a0f186cbea1ff3c7eed5dbb"
+dependencies = [
+ "proc-macro2",
+ "quote",
+ "syn",
+]
+
 [[package]]
 name = "autocfg"
 version = "1.5.1"
@@ -296,6 +307,12 @@ dependencies = [
  "windows-sys 0.61.2",
 ]
 
+[[package]]
+name = "fastrand"
+version = "2.4.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9f1f227452a390804cdb637b74a86990f2a7d7ba4b7d5693aac9b4dd6defd8d6"
+
 [[package]]
 name = "find-msvc-tools"
 version = "0.1.9"
@@ -565,6 +582,12 @@ version = "0.2.186"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "68ab91017fe16c622486840e4c83c9a37afeff978bd239b5293d61ece587de66"
 
+[[package]]
+name = "linux-raw-sys"
+version = "0.12.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "32a66949e030da00e8c7d4434b251670a91556f4144941d37452769c25d58a53"
+
 [[package]]
 name = "litemap"
 version = "0.8.2"
@@ -991,6 +1014,19 @@ dependencies = [
  "nom",
 ]
 
+[[package]]
+name = "rustix"
+version = "1.1.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "b6fe4565b9518b83ef4f91bb47ce29620ca828bd32cb7e408f0062e9930ba190"
+dependencies = [
+ "bitflags",
+ "errno",
+ "libc",
+ "linux-raw-sys",
+ "windows-sys 0.61.2",
+]
+
 [[package]]
 name = "rustls"
 version = "0.23.41"
@@ -1168,10 +1204,12 @@ checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba"
 name = "shoal-types"
 version = "0.1.0"
 dependencies = [
+ "async-trait",
  "chrono",
  "mtp",
  "serde",
  "serde_json",
+ "tempfile",
  "thiserror",
  "tokio",
  "uuid",
@@ -1243,6 +1281,19 @@ dependencies = [
  "syn",
 ]
 
+[[package]]
+name = "tempfile"
+version = "3.27.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd"
+dependencies = [
+ "fastrand",
+ "getrandom 0.4.3",
+ "once_cell",
+ "rustix",
+ "windows-sys 0.61.2",
+]
+
 [[package]]
 name = "thiserror"
 version = "2.0.18"

Cargo.toml

@@ -12,6 +12,8 @@ serde = { version = "1", features = ["derive"] }
 serde_json = "1"
 mtp = { git = "https://git@git.methanium.net/methanium/mtp.git", features = [] }
 tokio = "1"
+async-trait = "0.1"
 
 [dev-dependencies]
 tokio = { version = "1", features = ["full"] }
+tempfile = "3"

src/lib.rs

@@ -12,6 +12,7 @@ pub mod errors;
 pub mod krill;
 pub mod mutations;
 pub mod project;
+pub mod sandbox;
 pub mod sync;
 pub mod task;
 pub mod todo;
@@ -34,7 +35,8 @@ pub use tools::{
     format_tool_error, format_tools_json, format_tool_result, parse_tool_call_blocks,
     parse_tool_call_stream, tool_definitions, tool_ids_for_agent_type, ApprovalRequirement,
     CompositionStep, ContextVisibility, ExecutionContext, MarketplaceSearchResults, ParsedToolCall,
+    REEF_PROXY_TOOLS, GLOBAL_TOOLS, is_reef_proxy_tool, is_global_tool,
     TestAction, ToolComposition, ToolDefinition, ToolDependency, ToolDocumentation, ToolExample,
-    ToolListing, ToolParser, ToolPlugin, ToolTest, ToolTestResult, ToolTestSuite,
+    ToolListing, ToolParser, ToolPlugin, ToolExecutor, ToolRegistry, ToolTest, ToolTestResult, ToolTestSuite,
     ToolTestSuiteResult, ToolVersion,
 };

src/sandbox.rs

@@ -0,0 +1,210 @@
+use std::path::{Path, PathBuf, Component};
+
+/* Maximum allowed path depth as a safety measure */
+const MAX_PATH_DEPTH: usize = 64;
+
+/* Validate a file path to prevent directory traversal, null bytes, shell
+metacharacters, and other path-based attacks. Returns the normalized path
+if valid, or an error message.
+
+This is the canonical path validation shared by all runtimes (connector-krill,
+Reef API, etc.). */
+pub fn validate_path(path: &str) -> Result<PathBuf, String> {
+    if path.is_empty() {
+        return Err("Path cannot be empty".to_string());
+    }
+
+    if path.len() > 4096 {
+        return Err("Path exceeds maximum length (4096 characters)".to_string());
+    }
+
+    let path = Path::new(path);
+
+    if has_directory_traversal(path) {
+        return Err("Path contains directory traversal ('..') which is not allowed".to_string());
+    }
+
+    if contains_null_bytes(path) {
+        return Err("Path contains null bytes which is not allowed".to_string());
+    }
+
+    if contains_shell_metacharacters(path) {
+        return Err("Path contains shell metacharacters which is not allowed".to_string());
+    }
+
+    let depth = path.components().count();
+    if depth > MAX_PATH_DEPTH {
+        return Err(format!(
+            "Path depth ({}) exceeds maximum allowed ({})",
+            depth, MAX_PATH_DEPTH
+        ));
+    }
+
+    Ok(path.to_path_buf())
+}
+
+/* Given a validated path and a sandbox root, resolve the full path and
+verify it does not escape the sandbox. Handles both existing and new paths.
+The `validated` path should already have passed `validate_path`. */
+pub fn resolve_sandboxed_path(validated: &Path, sandbox_root: &Path) -> Result<PathBuf, String> {
+    let full_path = if validated.is_absolute() {
+        validated.to_path_buf()
+    } else {
+        sandbox_root.join(validated)
+    };
+
+    let canonical = if full_path.exists() {
+        full_path.canonicalize().map_err(|e| {
+            format!("Failed to canonicalize path: {}", e)
+        })?
+    } else {
+        if let Some(parent) = full_path.parent() {
+            if parent.exists() {
+                let canonical_parent = parent.canonicalize().map_err(|e| {
+                    format!("Failed to canonicalize parent path: {}", e)
+                })?;
+                if !canonical_parent.starts_with(sandbox_root) {
+                    return Err("Path escapes sandbox root boundaries".to_string());
+                }
+            } else {
+                return Err("Parent directory does not exist".to_string());
+            }
+        }
+        full_path
+    };
+
+    if !canonical.starts_with(sandbox_root) {
+        return Err("Path escapes sandbox root boundaries".to_string());
+    }
+
+    Ok(canonical)
+}
+
+fn has_directory_traversal(path: &Path) -> bool {
+    path.components().any(|comp| matches!(comp, Component::ParentDir))
+}
+
+fn contains_null_bytes(path: &Path) -> bool {
+    path.to_str().map_or(true, |s| s.contains('\0'))
+}
+
+fn contains_shell_metacharacters(path: &Path) -> bool {
+    const SHELL_METACHARACTERS: &[char] = &[
+        '|', ';', '&', '$', '`', '>', '<', '(', ')', '{', '}',
+        '!', '#', '*', '?', '[', ']', '~', '\n', '\r',
+    ];
+    path.to_str().map_or(true, |s| s.contains(SHELL_METACHARACTERS))
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::fs;
+    use tempfile::tempdir;
+
+    #[test]
+    fn test_rejects_empty_path() {
+        let result = validate_path("");
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn test_rejects_directory_traversal() {
+        let attacks = vec![
+            "../etc/passwd",
+            "../../etc/passwd",
+            "file/../../../etc/passwd",
+            "a/../../b/../../../c",
+            "..",
+        ];
+        for path in attacks {
+            let result = validate_path(path);
+            assert!(result.is_err(), "Expected '{}' to be rejected", path);
+        }
+    }
+
+    #[test]
+    fn test_accepts_valid_paths() {
+        let valid = vec![
+            "file.txt",
+            "subdir/file.txt",
+            "a/b/c/d/file.rs",
+            "src/main.rs",
+            "docs/README.md",
+            "project_name/src/lib.rs",
+        ];
+        for path in valid {
+            let result = validate_path(path);
+            assert!(
+                result.is_ok(),
+                "Expected '{}' to be accepted: {:?}",
+                path,
+                result.err()
+            );
+        }
+    }
+
+    #[test]
+    fn test_rejects_path_with_null_bytes() {
+        assert!(validate_path("file\0.txt").is_err());
+    }
+
+    #[test]
+    fn test_rejects_path_with_shell_metacharacters() {
+        let attacks = vec![
+            "file|echo", "file;rm", "file$(id)", "file`id`", "file>out", "file<in",
+        ];
+        for path in attacks {
+            let result = validate_path(path);
+            assert!(result.is_err(), "Expected '{}' to be rejected", path);
+        }
+    }
+
+    #[test]
+    fn test_rejects_overly_deep_path() {
+        let deep = (0..65).map(|_| "a").collect::<Vec<_>>().join("/");
+        assert!(validate_path(&deep).is_err());
+    }
+
+    #[test]
+    fn test_sandboxed_path_within_root() {
+        let dir = tempdir().unwrap();
+        let file_path = dir.path().join("test.txt");
+        fs::write(&file_path, "content").unwrap();
+
+        let validated = validate_path("test.txt").unwrap();
+        let result = resolve_sandboxed_path(&validated, dir.path());
+        assert!(result.is_ok());
+        assert_eq!(result.unwrap(), file_path);
+    }
+
+    #[test]
+    fn test_sandboxed_path_escape_via_absolute() {
+        let dir = tempdir().unwrap();
+        let outside_file = std::env::temp_dir().join("escape_test.txt");
+        fs::write(&outside_file, "content").unwrap();
+
+        let validated = validate_path(outside_file.to_str().unwrap()).unwrap();
+        let result = resolve_sandboxed_path(&validated, dir.path());
+        assert!(result.is_err());
+        let _ = fs::remove_file(&outside_file);
+    }
+
+    #[test]
+    fn test_sandboxed_path_new_file() {
+        let dir = tempdir().unwrap();
+
+        let validated = validate_path("new_file.txt").unwrap();
+        let result = resolve_sandboxed_path(&validated, dir.path());
+        assert!(result.is_ok());
+    }
+
+    #[test]
+    fn test_sandboxed_path_nonexistent_parent() {
+        let dir = tempdir().unwrap();
+
+        let validated = validate_path("nonexistent_dir/file.txt").unwrap();
+        let result = resolve_sandboxed_path(&validated, dir.path());
+        assert!(result.is_err());
+    }
+}

src/tools.rs

@@ -1,3 +1,4 @@
+use async_trait::async_trait;
 use serde::{Deserialize, Serialize};
 
 /* Context in which a tool should be available */
@@ -227,6 +228,20 @@ impl ToolDefinition {
     pub fn is_compatible_with(&self, other: &Self) -> bool {
         self.name == other.name && !self.deprecated && !other.deprecated
     }
+
+    /* Execution tier classification based on the tool's properties.
+    Tier 1 (Transient):  global tools with no project state — always local
+    Tier 2 (Project Read): read-only project-scoped tools — local or Reef proxy
+    Tier 3 (Persistent):   mutation tools + Reef-only tools — Reef-backed */
+    pub fn tier(&self) -> u8 {
+        if is_global_tool(&self.name) {
+            1
+        } else if self.mutates() || self.execution_context == ExecutionContext::Reef {
+            3
+        } else {
+            2
+        }
+    }
 }
 
 /* Simple semver comparison (major.minor.patch) */
@@ -495,22 +510,6 @@ impl Default for ToolVersion {
     }
 }
 
-/* Execution context for tool filtering. Determines which tools are
-available in which execution environment (Reef vs. Krill vs. both). */
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
-#[serde(rename_all = "snake_case")]
-pub enum ToolContext {
-    Reef,
-    Krill,
-    Both,
-}
-
-impl Default for ToolContext {
-    fn default() -> Self {
-        Self::Both
-    }
-}
-
 /* Tool category classification for UI grouping and filtering. */
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
 #[serde(rename_all = "snake_case")]
@@ -668,6 +667,29 @@ pub trait ToolPlugin: Send + Sync {
     }
 }
 
+/// Shared trait for executing a tool.
+///
+/// Every Krill binary (connector-krill, example-krill, custom) implements
+/// this trait for each tool it supports. The trait is deliberately simple —
+/// it takes parsed JSON arguments and returns either a string result or an
+/// error. Wrapping in a sandbox/project context is handled by the caller.
+#[async_trait]
+pub trait ToolExecutor: Send + Sync {
+    fn definition(&self) -> ToolDefinition;
+
+    async fn execute(&self, arguments: &serde_json::Value) -> Result<String, String>;
+}
+
+/// A registry of tools that can be looked up by name and executed.
+#[async_trait]
+pub trait ToolRegistry: Send + Sync {
+    async fn register(&self, tool: ToolDefinition, executor: Box<dyn ToolExecutor>);
+
+    async fn list_tools(&self) -> Vec<ToolDefinition>;
+
+    async fn execute(&self, name: &str, arguments: &serde_json::Value) -> Result<String, String>;
+}
+
 /* A marketplace listing for a published tool. */
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct ToolListing {
@@ -820,6 +842,41 @@ NOT here: they stay Everywhere so direct chat can call them under approval routi
 (Phase 4). Only genuinely agent-internal collaboration tools are agent-only. */
 pub const AGENT_ONLY_TOOLS: &[&str] = &["submit_batch_plan", "report_completion"];
 
+/* Tools that proxy to the Reef API when a `REEF_URL` / `REEF_API_URL` is
+configured. These tools have both a local implementation and a Reef-backed
+implementation (via Reservoir). When Reef is available the Reef path is
+preferred so that file operations go through the Reservoir-backed sandbox;
+when Reef is unreachable the local fallback executes instead.
+
+This is the Krill → Reef proxy set, not the full set of Reef-only tools.
+Tools like kanban_*, document_*, and strategic items are always executed on
+Reef via MTP and are NOT in this list — they have no local implementation. */
+pub const REEF_PROXY_TOOLS: &[&str] = &[
+    "read_file", "file_read",
+    "write_file", "file_write",
+    "edit_file",
+    "list_directory",
+    "search_files",
+    "grep",
+    "bash", "execute",
+    "delete_workspace",
+    "list_files",
+];
+
+/* Tools that operate without a project scope (global). These are always
+executed locally on the Krill and never need project_id injection. */
+pub const GLOBAL_TOOLS: &[&str] = &["web_search", "web_fetch", "web_api"];
+
+/* Returns true if the tool name is in the global (non-project-scoped) set. */
+pub fn is_global_tool(name: &str) -> bool {
+    GLOBAL_TOOLS.contains(&name)
+}
+
+/* Returns true if the tool name should proxy to Reef when Reef is available. */
+pub fn is_reef_proxy_tool(name: &str) -> bool {
+    REEF_PROXY_TOOLS.contains(&name)
+}
+
 /* Returns all built-in tool definitions across all categories. */
 pub fn tool_definitions() -> Vec<ToolDefinition> {
     let mut tools = vec![