From f8ff63af0e659a2ad24d22e8c07c542b8af5dced Mon Sep 17 00:00:00 2001 From: dave Date: Mon, 18 May 2026 16:52:49 +0000 Subject: [PATCH] huskies: merge 1142 story Force coder agents through MCP-validated Edit/Write/Bash to prevent writes to master worktree --- server/src/agents/pool/start/spawn.rs | 60 +++ server/src/http/mcp/dispatch.rs | 3 + server/src/http/mcp/shell_tools/file_tools.rs | 452 ++++++++++++++++++ server/src/http/mcp/shell_tools/mod.rs | 4 +- server/src/http/mcp/tools_list/mod.rs | 4 +- .../src/http/mcp/tools_list/system_tools.rs | 44 ++ 6 files changed, 565 insertions(+), 2 deletions(-) create mode 100644 server/src/http/mcp/shell_tools/file_tools.rs diff --git a/server/src/agents/pool/start/spawn.rs b/server/src/agents/pool/start/spawn.rs index 313b2db8..9e2cad3e 100644 --- a/server/src/agents/pool/start/spawn.rs +++ b/server/src/agents/pool/start/spawn.rs @@ -116,6 +116,23 @@ pub(super) fn maybe_inject_gate_failure(args: &mut Vec, story_id: &str) } } +/// Append `Edit,Write,Bash` to the `--disallowedTools` flag so worktree agents +/// cannot write to the master tree via Claude's built-in tools. If +/// `--disallowedTools` is already present (from agent config), the three names +/// are appended to the existing value rather than replacing it. +pub(super) fn inject_worktree_disallowed_tools(args: &mut Vec) { + const BLOCKED: &str = "Edit,Write,Bash"; + if let Some(pos) = args.iter().position(|a| a == "--disallowedTools") { + if let Some(val) = args.get_mut(pos + 1) { + val.push(','); + val.push_str(BLOCKED); + } + } else { + args.push("--disallowedTools".to_string()); + args.push(BLOCKED.to_string()); + } +} + /// Run the background worktree-creation + agent-launch flow. /// /// Caller (`AgentPool::start_agent`) wraps this in `tokio::spawn` and stores @@ -264,6 +281,10 @@ pub(super) async fn run_agent_spawn( maybe_inject_gate_failure(&mut args, &sid); // Cap turns and budget for merge-gate fixup sessions (story 981). maybe_cap_for_merge_fixup(&mut args, &sid); + // Every agent that runs inside a worktree must use the validated MCP + // edit/write tools instead of Claude's built-in Edit/Write/Bash. This + // prevents accidental writes to the master worktree (stories 1127, 1136). + inject_worktree_disallowed_tools(&mut args); // Append project-local prompt content (.huskies/AGENT.md) to the // baked-in prompt so every agent role sees project-specific guidance @@ -1297,4 +1318,43 @@ mod tests { item.stage().dir_name() ); } + + // ── inject_worktree_disallowed_tools (AC1, story 1142) ─────────── + + /// AC3(c) proxy: worktree agents get `--disallowedTools Edit,Write,Bash`. + #[test] + fn worktree_disallowed_tools_added_when_absent() { + let mut args: Vec = vec!["--verbose".to_string()]; + inject_worktree_disallowed_tools(&mut args); + let pos = args + .iter() + .position(|a| a == "--disallowedTools") + .expect("--disallowedTools must be present"); + let val = &args[pos + 1]; + assert!(val.contains("Edit"), "must include Edit"); + assert!(val.contains("Write"), "must include Write"); + assert!(val.contains("Bash"), "must include Bash"); + } + + /// Existing `--disallowedTools` value is extended, not replaced. + #[test] + fn worktree_disallowed_tools_appended_to_existing() { + let mut args = vec!["--disallowedTools".to_string(), "SomeOtherTool".to_string()]; + inject_worktree_disallowed_tools(&mut args); + // Only one --disallowedTools flag. + let count = args + .iter() + .filter(|a| a.as_str() == "--disallowedTools") + .count(); + assert_eq!(count, 1, "must not duplicate --disallowedTools"); + let pos = args.iter().position(|a| a == "--disallowedTools").unwrap(); + let val = &args[pos + 1]; + assert!( + val.contains("SomeOtherTool"), + "original tool must be preserved" + ); + assert!(val.contains("Edit"), "Edit must be added"); + assert!(val.contains("Write"), "Write must be added"); + assert!(val.contains("Bash"), "Bash must be added"); + } } diff --git a/server/src/http/mcp/dispatch.rs b/server/src/http/mcp/dispatch.rs index a5458edb..588c4511 100644 --- a/server/src/http/mcp/dispatch.rs +++ b/server/src/http/mcp/dispatch.rs @@ -107,6 +107,9 @@ pub async fn dispatch_tool_call( // Freeze / unfreeze story "freeze_story" => story_tools::tool_freeze_story(&args, ctx), "unfreeze_story" => story_tools::tool_unfreeze_story(&args, ctx), + // Worktree-sandboxed file editing (replaces Claude's built-in Edit/Write for coder agents) + "edit" => shell_tools::tool_edit(&args, ctx), + "write" => shell_tools::tool_write(&args, ctx), // Shell command execution "run_command" => shell_tools::tool_run_command(&args, ctx).await, "run_tests" => shell_tools::tool_run_tests(&args, ctx).await, diff --git a/server/src/http/mcp/shell_tools/file_tools.rs b/server/src/http/mcp/shell_tools/file_tools.rs new file mode 100644 index 00000000..8b7263ea --- /dev/null +++ b/server/src/http/mcp/shell_tools/file_tools.rs @@ -0,0 +1,452 @@ +//! MCP file-editing tools: `edit` and `write`. +//! +//! These are worktree-sandboxed equivalents of Claude's built-in `Edit` and +//! `Write` tools. All paths must canonicalize to inside `.huskies/worktrees/` +//! so agents cannot write to the master working tree. + +use crate::http::context::AppContext; +use serde_json::Value; +use std::path::{Path, PathBuf}; + +/// Validate that `file_path` is an absolute path whose nearest existing +/// ancestor lies inside the project's `.huskies/worktrees/` directory. +/// +/// Unlike [`crate::service::shell::io::validate_working_dir`], the target file +/// itself need not exist (write creates it), so we walk up to the first +/// existing ancestor before canonicalising. +/// +/// Returns the original (non-canonicalized) `PathBuf` on success so the +/// caller can use it directly for I/O. +/// +/// # Errors +/// Returns a `String` error naming both the worktrees root and the offending +/// path, matching the style of the `run_command` guard. +pub(super) fn validate_worktree_file_path( + file_path: &str, + ctx: &AppContext, +) -> Result { + let path = PathBuf::from(file_path); + + if !path.is_absolute() { + return Err(format!( + "file_path must be an absolute path, got: {file_path}" + )); + } + + let project_root = ctx.services.agents.get_project_root(&ctx.state)?; + let worktrees_root = project_root.join(".huskies").join("worktrees"); + + if !worktrees_root.exists() { + return Err(format!( + "No worktrees directory found; file_path must be inside {worktrees_root:?}, got: {file_path}" + )); + } + + let canonical_wt = worktrees_root + .canonicalize() + .map_err(|e| format!("Cannot canonicalize worktrees root: {e}"))?; + + // Walk up to find the deepest existing ancestor so we can canonicalize it. + let canonical_ancestor = find_existing_ancestor(&path) + .ok_or_else(|| format!("file_path has no accessible ancestor on disk: {file_path}"))? + .canonicalize() + .map_err(|e| format!("Cannot canonicalize path: {e}"))?; + + if !canonical_ancestor.starts_with(&canonical_wt) { + return Err(format!( + "file_path must be inside worktrees root {worktrees_root:?}. Got: {file_path}" + )); + } + + Ok(path) +} + +/// Return the deepest ancestor of `p` (inclusive) that exists on disk. +fn find_existing_ancestor(p: &Path) -> Option<&Path> { + let mut current = p; + loop { + if current.exists() { + return Some(current); + } + current = current.parent()?; + } +} + +/// Replace `old_string` with `new_string` in a file inside the agent's worktree. +/// +/// Mirrors Claude's built-in `Edit` tool with worktree path validation. +/// By default replaces only the first occurrence; pass `replace_all: true` +/// to replace every occurrence. +pub(crate) fn tool_edit(args: &Value, ctx: &AppContext) -> Result { + let file_path = args + .get("file_path") + .and_then(|v| v.as_str()) + .ok_or("Missing required argument: file_path")?; + let old_string = args + .get("old_string") + .and_then(|v| v.as_str()) + .ok_or("Missing required argument: old_string")?; + let new_string = args + .get("new_string") + .and_then(|v| v.as_str()) + .ok_or("Missing required argument: new_string")?; + let replace_all = args + .get("replace_all") + .and_then(|v| v.as_bool()) + .unwrap_or(false); + + let path = validate_worktree_file_path(file_path, ctx)?; + + if !path.exists() { + return Err(format!("file_path does not exist: {file_path}")); + } + + let content = + std::fs::read_to_string(&path).map_err(|e| format!("Failed to read {file_path}: {e}"))?; + + if !content.contains(old_string) { + return Err(format!( + "old_string not found in {file_path}: {old_string:?}" + )); + } + + let new_content = if replace_all { + content.replace(old_string, new_string) + } else { + content.replacen(old_string, new_string, 1) + }; + + std::fs::write(&path, &new_content).map_err(|e| format!("Failed to write {file_path}: {e}"))?; + + Ok(format!("Edited {file_path}")) +} + +/// Write `content` to a file inside the agent's worktree, creating the file +/// (and any missing parent directories) if necessary. +/// +/// Mirrors Claude's built-in `Write` tool with worktree path validation. +pub(crate) fn tool_write(args: &Value, ctx: &AppContext) -> Result { + let file_path = args + .get("file_path") + .and_then(|v| v.as_str()) + .ok_or("Missing required argument: file_path")?; + let content = args + .get("content") + .and_then(|v| v.as_str()) + .ok_or("Missing required argument: content")?; + + let path = validate_worktree_file_path(file_path, ctx)?; + + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent) + .map_err(|e| format!("Failed to create parent dirs for {file_path}: {e}"))?; + } + + std::fs::write(&path, content).map_err(|e| format!("Failed to write {file_path}: {e}"))?; + + Ok(format!("Written {file_path}")) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::http::test_helpers::test_ctx; + use serde_json::json; + + fn make_worktree(tmp: &tempfile::TempDir, name: &str) -> PathBuf { + let wt = tmp.path().join(".huskies").join("worktrees").join(name); + std::fs::create_dir_all(&wt).unwrap(); + wt + } + + // ── validate_worktree_file_path ─────────────────────────────────── + + #[test] + fn validate_rejects_relative_path() { + let tmp = tempfile::tempdir().unwrap(); + make_worktree(&tmp, "42_test"); + let ctx = test_ctx(tmp.path()); + let result = validate_worktree_file_path("relative/path.rs", &ctx); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("absolute")); + } + + #[test] + fn validate_rejects_path_outside_worktree() { + let tmp = tempfile::tempdir().unwrap(); + make_worktree(&tmp, "42_test"); + let ctx = test_ctx(tmp.path()); + // /workspace/server/foo.rs is outside .huskies/worktrees/ + let outside = tmp.path().join("server").join("foo.rs"); + let result = validate_worktree_file_path(outside.to_str().unwrap(), &ctx); + assert!(result.is_err(), "expected rejection, got ok"); + let msg = result.unwrap_err(); + assert!( + msg.contains("worktrees"), + "error should name worktrees root: {msg}" + ); + } + + #[test] + fn validate_accepts_existing_file_inside_worktree() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "42_test"); + let file = wt.join("foo.rs"); + std::fs::write(&file, "content").unwrap(); + let ctx = test_ctx(tmp.path()); + let result = validate_worktree_file_path(file.to_str().unwrap(), &ctx); + assert!(result.is_ok(), "expected ok, got: {:?}", result); + } + + #[test] + fn validate_accepts_nonexistent_file_inside_worktree() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "42_test"); + // File doesn't exist yet — parent dir does + let file = wt.join("new_file.rs"); + let ctx = test_ctx(tmp.path()); + let result = validate_worktree_file_path(file.to_str().unwrap(), &ctx); + assert!( + result.is_ok(), + "expected ok for new file, got: {:?}", + result + ); + } + + #[test] + fn validate_rejects_no_worktrees_dir() { + let tmp = tempfile::tempdir().unwrap(); + // Do NOT create worktrees dir + let ctx = test_ctx(tmp.path()); + let path = tmp.path().join("file.rs"); + let result = validate_worktree_file_path(path.to_str().unwrap(), &ctx); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("worktrees")); + } + + // ── tool_edit ───────────────────────────────────────────────────── + + /// AC3(a) — path outside worktree is rejected + #[test] + fn tool_edit_rejects_path_outside_worktree() { + let tmp = tempfile::tempdir().unwrap(); + make_worktree(&tmp, "42_test"); + // Create a file outside worktrees + let outside = tmp.path().join("server"); + std::fs::create_dir_all(&outside).unwrap(); + let outside_file = outside.join("foo.rs"); + std::fs::write(&outside_file, "old content").unwrap(); + let ctx = test_ctx(tmp.path()); + + let result = tool_edit( + &json!({ + "file_path": outside_file.to_str().unwrap(), + "old_string": "old content", + "new_string": "new content" + }), + &ctx, + ); + assert!(result.is_err(), "expected rejection"); + // Master file unchanged + let content = std::fs::read_to_string(&outside_file).unwrap(); + assert_eq!(content, "old content", "master file must be unchanged"); + } + + /// AC3(b) — path inside worktree succeeds + #[test] + fn tool_edit_accepts_path_inside_worktree() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "42_test"); + let file = wt.join("foo.rs"); + std::fs::write(&file, "fn old_fn() {}").unwrap(); + let ctx = test_ctx(tmp.path()); + + let result = tool_edit( + &json!({ + "file_path": file.to_str().unwrap(), + "old_string": "old_fn", + "new_string": "new_fn" + }), + &ctx, + ); + assert!(result.is_ok(), "expected ok, got: {:?}", result); + let content = std::fs::read_to_string(&file).unwrap(); + assert!(content.contains("new_fn")); + assert!(!content.contains("old_fn")); + } + + #[test] + fn tool_edit_replace_all_replaces_every_occurrence() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "43_test"); + let file = wt.join("multi.rs"); + std::fs::write(&file, "foo foo foo").unwrap(); + let ctx = test_ctx(tmp.path()); + + tool_edit( + &json!({ + "file_path": file.to_str().unwrap(), + "old_string": "foo", + "new_string": "bar", + "replace_all": true + }), + &ctx, + ) + .unwrap(); + + let content = std::fs::read_to_string(&file).unwrap(); + assert_eq!(content, "bar bar bar"); + } + + #[test] + fn tool_edit_default_replaces_first_occurrence_only() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "44_test"); + let file = wt.join("single.rs"); + std::fs::write(&file, "foo foo foo").unwrap(); + let ctx = test_ctx(tmp.path()); + + tool_edit( + &json!({ + "file_path": file.to_str().unwrap(), + "old_string": "foo", + "new_string": "bar" + }), + &ctx, + ) + .unwrap(); + + let content = std::fs::read_to_string(&file).unwrap(); + assert_eq!(content, "bar foo foo"); + } + + #[test] + fn tool_edit_fails_when_old_string_not_found() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "45_test"); + let file = wt.join("missing.rs"); + std::fs::write(&file, "hello world").unwrap(); + let ctx = test_ctx(tmp.path()); + + let result = tool_edit( + &json!({ + "file_path": file.to_str().unwrap(), + "old_string": "not present", + "new_string": "x" + }), + &ctx, + ); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("not found")); + } + + #[test] + fn tool_edit_fails_when_file_does_not_exist() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "46_test"); + let ctx = test_ctx(tmp.path()); + + let result = tool_edit( + &json!({ + "file_path": wt.join("ghost.rs").to_str().unwrap(), + "old_string": "x", + "new_string": "y" + }), + &ctx, + ); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("does not exist")); + } + + // ── tool_write ──────────────────────────────────────────────────── + + #[test] + fn tool_write_rejects_path_outside_worktree() { + let tmp = tempfile::tempdir().unwrap(); + make_worktree(&tmp, "42_test"); + let outside = tmp.path().join("master_file.rs"); + let ctx = test_ctx(tmp.path()); + + let result = tool_write( + &json!({ + "file_path": outside.to_str().unwrap(), + "content": "evil" + }), + &ctx, + ); + assert!(result.is_err(), "expected rejection"); + assert!(!outside.exists(), "master file must not be created"); + } + + #[test] + fn tool_write_creates_new_file_inside_worktree() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "47_test"); + let file = wt.join("new.rs"); + let ctx = test_ctx(tmp.path()); + + tool_write( + &json!({ + "file_path": file.to_str().unwrap(), + "content": "pub fn hello() {}" + }), + &ctx, + ) + .unwrap(); + + let content = std::fs::read_to_string(&file).unwrap(); + assert_eq!(content, "pub fn hello() {}"); + } + + #[test] + fn tool_write_overwrites_existing_file() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "48_test"); + let file = wt.join("existing.rs"); + std::fs::write(&file, "old").unwrap(); + let ctx = test_ctx(tmp.path()); + + tool_write( + &json!({ + "file_path": file.to_str().unwrap(), + "content": "new" + }), + &ctx, + ) + .unwrap(); + + let content = std::fs::read_to_string(&file).unwrap(); + assert_eq!(content, "new"); + } + + #[test] + fn tool_write_creates_parent_dirs() { + let tmp = tempfile::tempdir().unwrap(); + let wt = make_worktree(&tmp, "49_test"); + let file = wt.join("deep").join("nested").join("file.rs"); + let ctx = test_ctx(tmp.path()); + + tool_write( + &json!({ + "file_path": file.to_str().unwrap(), + "content": "deep content" + }), + &ctx, + ) + .unwrap(); + + let content = std::fs::read_to_string(&file).unwrap(); + assert_eq!(content, "deep content"); + } + + #[test] + fn tool_write_missing_content_arg_errors() { + let tmp = tempfile::tempdir().unwrap(); + make_worktree(&tmp, "50_test"); + let ctx = test_ctx(tmp.path()); + + let result = tool_write(&json!({"file_path": "/some/path"}), &ctx); + assert!(result.is_err()); + assert!(result.unwrap_err().contains("content")); + } +} diff --git a/server/src/http/mcp/shell_tools/mod.rs b/server/src/http/mcp/shell_tools/mod.rs index 9d93e9da..52a765e9 100644 --- a/server/src/http/mcp/shell_tools/mod.rs +++ b/server/src/http/mcp/shell_tools/mod.rs @@ -1,12 +1,14 @@ -//! MCP shell tools — run commands, execute tests, and stream output via MCP. +//! MCP shell tools — run commands, execute tests, edit and write files. //! //! This file is a thin adapter: it deserialises MCP payloads, delegates to //! `crate::service::shell` for all business logic, and serialises responses. mod exec; +mod file_tools; mod script; pub(crate) use exec::tool_run_command; +pub(crate) use file_tools::{tool_edit, tool_write}; pub(crate) use script::{ tool_get_test_result, tool_run_build, tool_run_check, tool_run_lint, tool_run_tests, }; diff --git a/server/src/http/mcp/tools_list/mod.rs b/server/src/http/mcp/tools_list/mod.rs index 24fd2504..9b424ea7 100644 --- a/server/src/http/mcp/tools_list/mod.rs +++ b/server/src/http/mcp/tools_list/mod.rs @@ -115,7 +115,9 @@ mod tests { assert!(names.contains(&"list_timers")); assert!(names.contains(&"cancel_timer")); assert!(names.contains(&"convert_item_type")); - assert_eq!(tools.len(), 83); + assert!(names.contains(&"edit")); + assert!(names.contains(&"write")); + assert_eq!(tools.len(), 85); } #[test] diff --git a/server/src/http/mcp/tools_list/system_tools.rs b/server/src/http/mcp/tools_list/system_tools.rs index e2638872..ce465476 100644 --- a/server/src/http/mcp/tools_list/system_tools.rs +++ b/server/src/http/mcp/tools_list/system_tools.rs @@ -173,6 +173,50 @@ pub(super) fn system_tools() -> Vec { "required": [] } }), + json!({ + "name": "edit", + "description": "Replace old_string with new_string in a file inside the agent's assigned worktree. Mirrors Claude's built-in Edit tool but validates that file_path is inside .huskies/worktrees/ to prevent writes to the master worktree. By default replaces the first occurrence only; set replace_all to true to replace every occurrence.", + "inputSchema": { + "type": "object", + "properties": { + "file_path": { + "type": "string", + "description": "Absolute path to the file to edit. Must be inside .huskies/worktrees/." + }, + "old_string": { + "type": "string", + "description": "The exact string to replace." + }, + "new_string": { + "type": "string", + "description": "The replacement string." + }, + "replace_all": { + "type": "boolean", + "description": "If true, replace every occurrence of old_string. Default: false (replace first occurrence only)." + } + }, + "required": ["file_path", "old_string", "new_string"] + } + }), + json!({ + "name": "write", + "description": "Write content to a file inside the agent's assigned worktree, creating the file (and any missing parent directories) if necessary. Mirrors Claude's built-in Write tool but validates that file_path is inside .huskies/worktrees/ to prevent writes to the master worktree.", + "inputSchema": { + "type": "object", + "properties": { + "file_path": { + "type": "string", + "description": "Absolute path to the file to write. Must be inside .huskies/worktrees/." + }, + "content": { + "type": "string", + "description": "The content to write to the file." + } + }, + "required": ["file_path", "content"] + } + }), json!({ "name": "git_status", "description": "Return the working tree status of an agent's worktree (staged, unstaged, and untracked files). The worktree_path must be inside .huskies/worktrees/. Push and remote operations are not available.",