huskies: merge 1142 story Force coder agents through MCP-validated Edit/Write/Bash to prevent writes to master worktree

This commit is contained in:
dave
2026-05-18 16:52:49 +00:00
parent 34e78bdbd5
commit f8ff63af0e
6 changed files with 565 additions and 2 deletions
+60
View File
@@ -116,6 +116,23 @@ pub(super) fn maybe_inject_gate_failure(args: &mut Vec<String>, 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<String>) {
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<String> = 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");
}
}
+3
View File
@@ -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,
@@ -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<PathBuf, String> {
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<String, String> {
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<String, String> {
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"));
}
}
+3 -1
View File
@@ -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,
};
+3 -1
View File
@@ -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]
@@ -173,6 +173,50 @@ pub(super) fn system_tools() -> Vec<Value> {
"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.",