From 9dafc4bfaa8a6253bc5a5ee65e488631e46e1d48 Mon Sep 17 00:00:00 2001 From: xavix-yo Date: Sat, 25 Jul 2026 00:52:21 +0100 Subject: [PATCH] fs-tools: show agent path, not host path, in tool messages rewrite_to_host overwrote args["path"] with the resolved absolute host path, which then leaked into every message the on-disk execute returned to the agent (e.g. edit_file's "Text not found in /home/.../SKALD.md"). The agent must only ever see its virtual namespace. rewrite_to_host now stashes the agent-visible path under a private key while keeping the host path in args["path"] for I/O; each execute renders messages from the stashed path. The key is never persisted (tool args are logged from call.arguments before run_with rewrites them) nor sent to the LLM. Fixed across edit_file, write_file, insert_at_line, replace_lines, search_file and grep_files. read_file and list_files were already correct. Added a regression test asserting no host path component appears in the output of a physical-path write/edit/grep. --- crates/skald-core/src/tools/fs/edit_file.rs | 5 +- crates/skald-core/src/tools/fs/grep_files.rs | 9 +-- .../skald-core/src/tools/fs/insert_at_line.rs | 3 +- crates/skald-core/src/tools/fs/mod.rs | 69 +++++++++++++++++++ .../skald-core/src/tools/fs/replace_lines.rs | 3 +- crates/skald-core/src/tools/fs/search_file.rs | 3 +- crates/skald-core/src/tools/fs/write_file.rs | 5 +- 7 files changed, 86 insertions(+), 11 deletions(-) diff --git a/crates/skald-core/src/tools/fs/edit_file.rs b/crates/skald-core/src/tools/fs/edit_file.rs index 192fdeb..b680e6b 100644 --- a/crates/skald-core/src/tools/fs/edit_file.rs +++ b/crates/skald-core/src/tools/fs/edit_file.rs @@ -183,9 +183,10 @@ impl Tool for EditFile { fn execute(&self, args: Value) -> Result { let user_path = args["path"].as_str() .ok_or_else(|| anyhow::anyhow!("Missing required argument: path"))?; + let display = super::display_path_arg(&args); let content = read_to_string(user_path)?; - let updated = apply_edit(&content, &args, user_path)?; + let updated = apply_edit(&content, &args, display)?; write_string(user_path, &updated)?; - Ok(format!("Edited {user_path}.")) + Ok(format!("Edited {display}.")) } } diff --git a/crates/skald-core/src/tools/fs/grep_files.rs b/crates/skald-core/src/tools/fs/grep_files.rs index 8e47041..bce565d 100644 --- a/crates/skald-core/src/tools/fs/grep_files.rs +++ b/crates/skald-core/src/tools/fs/grep_files.rs @@ -108,6 +108,7 @@ impl Tool for GrepFiles { fn execute(&self, args: Value) -> Result { let user_path = args["path"].as_str().ok_or_else(|| anyhow::anyhow!("Missing: path"))?; + let display = super::display_path_arg(&args); let pattern = args["pattern"].as_str().ok_or_else(|| anyhow::anyhow!("Missing: pattern"))?; let case_sensitive = args["case_sensitive"].as_bool().unwrap_or(false); let include_glob = args["include_glob"].as_str(); @@ -123,7 +124,7 @@ impl Tool for GrepFiles { let glob_pattern = include_glob.and_then(|g| glob::Pattern::new(g).ok()); let root = resolve(user_path)?; if !root.exists() { - anyhow::bail!("Path not found: {user_path}"); + anyhow::bail!("Path not found: {display}"); } // Walkers emit absolute paths (the `path` arg is resolved to an absolute working @@ -138,7 +139,7 @@ impl Tool for GrepFiles { collect_matching_files(&root, &re, &glob_pattern, max_results + offset, &mut files)?; let files: Vec = files.into_iter().skip(offset).take(max_results).map(rel).collect(); if files.is_empty() { - return Ok(format!("No files match {:?} in {user_path}.", pattern)); + return Ok(format!("No files match {:?} in {display}.", pattern)); } Ok(format!("{} file(s):\n{}", files.len(), files.join("\n"))) } @@ -147,7 +148,7 @@ impl Tool for GrepFiles { collect_match_counts(&root, &re, &glob_pattern, max_results + offset, &mut counts)?; let counts: Vec<(String, usize)> = counts.into_iter().skip(offset).take(max_results).collect(); if counts.is_empty() { - return Ok(format!("No matches for {:?} in {user_path}.", pattern)); + return Ok(format!("No matches for {:?} in {display}.", pattern)); } let lines: Vec = counts.into_iter().map(|(f, n)| format!("{}: {n}", rel(f))).collect(); Ok(format!("{} file(s):\n{}", lines.len(), lines.join("\n"))) @@ -160,7 +161,7 @@ impl Tool for GrepFiles { let matches: Vec = matches.into_iter().skip(offset).take(max_results).map(rel).collect(); if matches.is_empty() { - return Ok(format!("No matches for {:?} in {user_path}.", pattern)); + return Ok(format!("No matches for {:?} in {display}.", pattern)); } let mut out = format!("{} match(es):\n", matches.len()); out.push_str(&matches.join("\n")); diff --git a/crates/skald-core/src/tools/fs/insert_at_line.rs b/crates/skald-core/src/tools/fs/insert_at_line.rs index ac5d8c4..77d76d7 100644 --- a/crates/skald-core/src/tools/fs/insert_at_line.rs +++ b/crates/skald-core/src/tools/fs/insert_at_line.rs @@ -120,8 +120,9 @@ impl Tool for InsertAtLine { fn execute(&self, args: Value) -> Result { let user_path = args["path"].as_str() .ok_or_else(|| anyhow::anyhow!("Missing required argument: path"))?; + let display = super::display_path_arg(&args); let text = read_to_string(user_path)?; - let (updated, msg) = apply_insert(&text, &args, user_path)?; + let (updated, msg) = apply_insert(&text, &args, display)?; write_string(user_path, &updated)?; Ok(msg) } diff --git a/crates/skald-core/src/tools/fs/mod.rs b/crates/skald-core/src/tools/fs/mod.rs index 375032c..5933d85 100644 --- a/crates/skald-core/src/tools/fs/mod.rs +++ b/crates/skald-core/src/tools/fs/mod.rs @@ -238,12 +238,32 @@ pub fn resolve_view_path(fs: &UserFs, input: &str) -> Result<(PathBuf, String)> /// Rewrites the `path` argument of a physical fs-tool call to the resolved absolute /// host path, so the on-disk `execute` (which takes absolute paths as-is) acts on /// the caller's per-user workspace rather than the process working directory. +/// +/// The caller's agent-visible path is stashed under [`DISPLAY_PATH_KEY`] so `execute` +/// can show it in its messages — the model must never see the host path. This key is +/// never persisted: tool args are logged from `call.arguments` *before* `run_with` +/// rewrites them, and tool results are plain strings. pub(crate) fn rewrite_to_host(fs: &UserFs, agent_path: &str, mut args: Value) -> Result { let host = resolve_host_path(fs, agent_path)?; + args[DISPLAY_PATH_KEY] = Value::String(agent_path.to_string()); args["path"] = Value::String(host.to_string_lossy().into_owned()); Ok(args) } +/// Private stash key for the agent-visible path, set by [`rewrite_to_host`] alongside +/// the host path in `path`. +const DISPLAY_PATH_KEY: &str = "__display_path"; + +/// The path to show in user-facing messages: the agent-visible path stashed by +/// [`rewrite_to_host`] when present, falling back to `path` itself for the +/// context-free legacy path (where `path` was never rewritten and is already the +/// agent path). +pub(crate) fn display_path_arg(args: &Value) -> &str { + args.get(DISPLAY_PATH_KEY).and_then(Value::as_str) + .or_else(|| args.get("path").and_then(Value::as_str)) + .unwrap_or("") +} + /// A tool execution that fails immediately — surfaces a containment / access error /// from `run_with` without attempting a disk op. pub(crate) fn error_exec<'a>(msg: String) -> Box { @@ -597,4 +617,53 @@ mod tests { let _ = std::fs::remove_dir_all(&udir); let _ = std::fs::remove_dir_all(&sdir); } + + /// Physical-path fs tools must report the **agent-visible** path in every + /// message they return — never the resolved host path. The agent's virtual + /// namespace (`~/…`, `shared/…`, `projects/…`) is all it should ever see; + /// the host workspace location is an internal detail. Regression for the + /// host-path leak that `rewrite_to_host` introduced into `execute`'s output. + #[tokio::test] + async fn physical_fs_tools_show_agent_path_not_host() { + let (shared, sdir) = store("phys-shared").await; + let (user, udir) = store("phys-user").await; + + let root = std::env::temp_dir().join(format!("skald-phys-{}", uuid::Uuid::new_v4())); + let home = root.join("homes").join("u1"); + std::fs::create_dir_all(&home).unwrap(); + + let fs = Arc::new(UserFs::new( + "u1", home.clone(), "skald-u1", PathBuf::from("/root"), vec![], vec![], None, + )); + let ctx = ToolContext { session_id: 1, user_id: "u1".into(), pool: Arc::clone(&user), fs }; + let write = WriteFile::new(Arc::clone(&shared)); + let edit = EditFile::new(Arc::clone(&shared)); + let grep = GrepFiles::new(); + + // `/homes/u1` only ever appears in the resolved host path, never in the + // agent namespace — so it is a robust, OS-independent leak detector. + let leak_marker = "/homes/u1"; + + // write_file success → "Created ~/notes.md", never the host home. + let out = drive(&write, &ctx, json!({"path":"~/notes.md","content":"hello\nworld"})) + .await.unwrap(); + assert!(out.contains("~/notes.md"), "agent path missing: {out}"); + assert!(!out.contains(leak_marker), "host path leaked into write_file result: {out}"); + + // edit_file failure → the error names the agent path, never the host path. + let err = drive(&edit, &ctx, json!({"path":"~/notes.md","old":"nope","new":"x"})) + .await.unwrap_err(); + assert!(err.contains("~/notes.md"), "agent path missing from error: {err}"); + assert!(!err.contains(leak_marker), "host path leaked into edit_file error: {err}"); + + // grep_files no-match → "in ~/notes.md", never the host path. + let out = drive(&grep, &ctx, json!({"path":"~/notes.md","pattern":"zzz"})) + .await.unwrap(); + assert!(out.contains("~/notes.md"), "agent path missing from grep: {out}"); + assert!(!out.contains(leak_marker), "host path leaked into grep result: {out}"); + + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&udir); + let _ = std::fs::remove_dir_all(&sdir); + } } diff --git a/crates/skald-core/src/tools/fs/replace_lines.rs b/crates/skald-core/src/tools/fs/replace_lines.rs index 1d8aefb..5377afe 100644 --- a/crates/skald-core/src/tools/fs/replace_lines.rs +++ b/crates/skald-core/src/tools/fs/replace_lines.rs @@ -125,8 +125,9 @@ impl Tool for ReplaceLines { fn execute(&self, args: Value) -> Result { let user_path = args["path"].as_str() .ok_or_else(|| anyhow::anyhow!("Missing required argument: path"))?; + let display = super::display_path_arg(&args); let content = read_to_string(user_path)?; - let (updated, msg) = apply_replace(&content, &args, user_path)?; + let (updated, msg) = apply_replace(&content, &args, display)?; write_string(user_path, &updated)?; Ok(msg) } diff --git a/crates/skald-core/src/tools/fs/search_file.rs b/crates/skald-core/src/tools/fs/search_file.rs index a71e4ab..4a58a93 100644 --- a/crates/skald-core/src/tools/fs/search_file.rs +++ b/crates/skald-core/src/tools/fs/search_file.rs @@ -136,7 +136,8 @@ impl Tool for SearchFile { fn execute(&self, args: Value) -> Result { let user_path = args["path"].as_str() .ok_or_else(|| anyhow::anyhow!("Missing required argument: path"))?; + let display = super::display_path_arg(&args); let text = read_to_string(user_path)?; - render_search(&text, &args, user_path) + render_search(&text, &args, display) } } diff --git a/crates/skald-core/src/tools/fs/write_file.rs b/crates/skald-core/src/tools/fs/write_file.rs index a2f4804..3edf35c 100644 --- a/crates/skald-core/src/tools/fs/write_file.rs +++ b/crates/skald-core/src/tools/fs/write_file.rs @@ -92,6 +92,7 @@ impl Tool for WriteFile { fn execute(&self, args: Value) -> Result { let user_path = args["path"].as_str() .ok_or_else(|| anyhow::anyhow!("Missing required argument: path"))?; + let display = super::display_path_arg(&args); let content = args["content"].as_str() .ok_or_else(|| anyhow::anyhow!("Missing required argument: content"))?; @@ -100,9 +101,9 @@ impl Tool for WriteFile { write_string(user_path, content)?; if existed { - Ok(format!("Overwrote {user_path} ({} bytes).", content.len())) + Ok(format!("Overwrote {display} ({} bytes).", content.len())) } else { - Ok(format!("Created {user_path} ({} bytes).", content.len())) + Ok(format!("Created {display} ({} bytes).", content.len())) } } }