diff --git a/crates/helexa-acp/src/main.rs b/crates/helexa-acp/src/main.rs index 91ae108..1586250 100644 --- a/crates/helexa-acp/src/main.rs +++ b/crates/helexa-acp/src/main.rs @@ -18,6 +18,7 @@ use std::sync::Arc; mod agent; mod compaction; mod config; +mod path_util; mod prompt; mod provider; mod qwen3; diff --git a/crates/helexa-acp/src/path_util.rs b/crates/helexa-acp/src/path_util.rs new file mode 100644 index 0000000..6cdb12c --- /dev/null +++ b/crates/helexa-acp/src/path_util.rs @@ -0,0 +1,184 @@ +//! Path expansion shared across every tool that takes a path. +//! +//! Models often emit shell-style paths like `~/git/repo/file.rs` or +//! `$HOME/notes.md`. ACP's `fs/read_text_file` and friends — and our +//! own local `std::fs` reads — both want a real absolute path; the +//! `~` / `$HOME` forms reach them as literal strings and the open +//! fails. The tool schemas already document "absolute path" but in +//! practice the model slips up often enough that handling it +//! server-side is the difference between "works" and "the agent is +//! brittle". +//! +//! Scope is deliberately small: +//! +//! - `~` and `~/` (current user only — `~user` lookups would require +//! pulling in passwd parsing). +//! - `$HOME` and `$HOME/`. +//! +//! Any other shell variable (`$PWD`, `${HOME}`, …) passes through +//! unchanged. The shell already expands them inside `bash` tool +//! commands; for the file-tool argument fields, we deliberately +//! limit the set so the behaviour is predictable. +//! +//! Falls back to the input path verbatim when `HOME` is unset +//! (stripped-down container env). That preserves the "no surprise +//! mutations" rule — never invent a path the caller didn't ask for. + +use std::path::{Path, PathBuf}; + +/// Expand `~`, `~/`, `$HOME`, and `$HOME/` prefixes against the +/// current user's home directory. All other inputs pass through +/// unchanged. +/// +/// Returns the input verbatim if `HOME` isn't set in the env. +pub fn expand_path(input: &Path) -> PathBuf { + let Some(s) = input.to_str() else { + return input.to_path_buf(); + }; + let Ok(home) = std::env::var("HOME") else { + return input.to_path_buf(); + }; + let home = PathBuf::from(home); + if s == "~" || s == "$HOME" { + return home; + } + if let Some(rest) = s.strip_prefix("~/") { + return home.join(rest); + } + if let Some(rest) = s.strip_prefix("$HOME/") { + return home.join(rest); + } + input.to_path_buf() +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Set HOME for the duration of the test. Tests using this run + /// serially under one mutex because env mutation isn't + /// thread-safe — `cargo test` parallel workers would race + /// without it. + fn with_home(home: &str, body: F) { + use std::sync::Mutex; + static LOCK: Mutex<()> = Mutex::new(()); + let _g = LOCK.lock().unwrap(); + let prior = std::env::var("HOME").ok(); + // SAFETY: tests touch process-global env. The mutex + // serialises access; sub-threads in other test modules + // touching HOME aren't expected (none in this crate). + unsafe { + std::env::set_var("HOME", home); + } + body(); + unsafe { + match prior { + Some(p) => std::env::set_var("HOME", p), + None => std::env::remove_var("HOME"), + } + } + } + + #[test] + fn expands_tilde_slash() { + with_home("/home/me", || { + assert_eq!( + expand_path(Path::new("~/git/repo/file.rs")), + PathBuf::from("/home/me/git/repo/file.rs") + ); + }); + } + + #[test] + fn expands_bare_tilde() { + with_home("/home/me", || { + assert_eq!(expand_path(Path::new("~")), PathBuf::from("/home/me")); + }); + } + + #[test] + fn expands_dollar_home_slash() { + with_home("/home/me", || { + assert_eq!( + expand_path(Path::new("$HOME/notes.md")), + PathBuf::from("/home/me/notes.md") + ); + }); + } + + #[test] + fn expands_bare_dollar_home() { + with_home("/home/me", || { + assert_eq!(expand_path(Path::new("$HOME")), PathBuf::from("/home/me")); + }); + } + + #[test] + fn absolute_path_passes_through() { + with_home("/home/me", || { + assert_eq!( + expand_path(Path::new("/etc/hostname")), + PathBuf::from("/etc/hostname") + ); + }); + } + + #[test] + fn relative_path_passes_through() { + with_home("/home/me", || { + assert_eq!( + expand_path(Path::new("src/main.rs")), + PathBuf::from("src/main.rs") + ); + }); + } + + #[test] + fn tilde_user_form_not_expanded() { + // ~other is shell sugar for /home/other and would require + // passwd parsing to resolve. Out of scope — pass it + // through and let the open fail with a clear error. + with_home("/home/me", || { + assert_eq!( + expand_path(Path::new("~other/x")), + PathBuf::from("~other/x") + ); + }); + } + + #[test] + fn no_home_env_passes_through() { + // Lock + clear HOME for this one. + use std::sync::Mutex; + static LOCK: Mutex<()> = Mutex::new(()); + let _g = LOCK.lock().unwrap(); + let prior = std::env::var("HOME").ok(); + // SAFETY: serialised by LOCK above. + unsafe { + std::env::remove_var("HOME"); + } + assert_eq!( + expand_path(Path::new("~/git/repo")), + PathBuf::from("~/git/repo") + ); + unsafe { + if let Some(p) = prior { + std::env::set_var("HOME", p); + } + } + } + + #[test] + fn dollar_other_var_not_expanded() { + with_home("/home/me", || { + assert_eq!( + expand_path(Path::new("$PWD/file")), + PathBuf::from("$PWD/file") + ); + assert_eq!( + expand_path(Path::new("${HOME}/file")), + PathBuf::from("${HOME}/file") + ); + }); + } +} diff --git a/crates/helexa-acp/src/tool_runner.rs b/crates/helexa-acp/src/tool_runner.rs index 18019c4..eca03e1 100644 --- a/crates/helexa-acp/src/tool_runner.rs +++ b/crates/helexa-acp/src/tool_runner.rs @@ -37,6 +37,7 @@ use serde::Deserialize; use serde_json::json; use tokio_util::sync::CancellationToken; +use crate::path_util::expand_path; use crate::session::{MODE_BYPASS, MODE_DEFAULT, MODE_PLAN}; use crate::store; use crate::tools::{BASH, EDIT_FILE, LIST_DIR, READ_FILE, WRITE_FILE}; @@ -431,7 +432,7 @@ pub async fn dispatch_tool_call( let path = args_value .get("path") .and_then(|v| v.as_str()) - .map(std::path::PathBuf::from); + .map(|s| expand_path(std::path::Path::new(s))); let inside_plan_dir = match (path.as_deref(), plan_dir.as_deref()) { (Some(p), Some(pd)) => p.starts_with(pd), _ => false, @@ -621,10 +622,38 @@ async fn exec_read_file( ) -> Result<(String, Vec), String> { let args: ReadFileArgs = serde_json::from_value(args_value.clone()).map_err(|e| format!("read_file: {e}"))?; - let content = ops - .read_text_file(session_id, args.path, args.line, args.limit) - .await - .map_err(|e| format!("read_file: {e:#}"))?; + let path = expand_path(&args.path); + + // Try the editor's filesystem first. Zed will show the open + // buffer (if any) and respect any per-workspace mount points + // / overlays it has configured. + let acp_result = ops + .read_text_file(session_id, path.clone(), args.line, args.limit) + .await; + let content = match acp_result { + Ok(c) => c, + Err(e) => { + // ACP failures on read are almost always Zed's + // workspace-boundary check (read paths outside the + // session cwd are refused). Fall back to local + // std::fs so the agent can still pull in shared + // material like `~/git/architecture/generic.md` that + // sits outside the active project. The user-process + // file permissions still apply — this is not a + // sandbox escape, just a way around Zed's + // workspace-only default. + tracing::warn!( + path = %path.display(), + error = %format!("{e:#}"), + "fs/read_text_file failed; falling back to local std::fs" + ); + let raw = std::fs::read_to_string(&path).map_err(|fs_err| { + format!("read_file: ACP returned {e:#}; local fallback also failed: {fs_err}") + })?; + apply_line_limit(&raw, args.line, args.limit) + } + }; + let blocks = vec![ToolCallContent::Content( agent_client_protocol::schema::Content::new(ContentBlock::Text(TextContent::new( content.clone(), @@ -633,6 +662,24 @@ async fn exec_read_file( Ok((content, blocks)) } +/// Slice a file's contents the same way ACP's `fs/read_text_file` +/// does (1-based line, optional line count). Used by the local-fs +/// fallback in `exec_read_file` so out-of-workspace reads honour +/// the same `line`/`limit` args the model passed. +fn apply_line_limit(content: &str, line: Option, limit: Option) -> String { + if line.is_none() && limit.is_none() { + return content.to_string(); + } + let start = line.unwrap_or(1).max(1) as usize - 1; + let count = limit.map(|l| l as usize).unwrap_or(usize::MAX); + content + .lines() + .skip(start) + .take(count) + .collect::>() + .join("\n") +} + async fn exec_write_file( ops: &dyn ClientOps, session_id: &SessionId, @@ -640,25 +687,22 @@ async fn exec_write_file( ) -> Result<(String, Vec), String> { let args: WriteFileArgs = serde_json::from_value(args_value.clone()).map_err(|e| format!("write_file: {e}"))?; + let path = expand_path(&args.path); // Best-effort read of the existing file so Zed can render a diff. // Failure here just means we render the write as an additive diff // — not a fatal error, the actual write below still runs. let old_text = ops - .read_text_file(session_id, args.path.clone(), None, None) + .read_text_file(session_id, path.clone(), None, None) .await .ok(); - ops.write_text_file(session_id, args.path.clone(), args.content.clone()) + ops.write_text_file(session_id, path.clone(), args.content.clone()) .await .map_err(|e| format!("write_file: {e:#}"))?; - let mut diff = Diff::new(args.path.clone(), args.content.clone()); + let mut diff = Diff::new(path.clone(), args.content.clone()); if let Some(old) = old_text { diff = diff.old_text(old); } - let summary = format!( - "wrote {} ({} bytes)", - args.path.display(), - args.content.len() - ); + let summary = format!("wrote {} ({} bytes)", path.display(), args.content.len()); Ok((summary, vec![ToolCallContent::Diff(diff)])) } @@ -669,41 +713,39 @@ async fn exec_edit_file( ) -> Result<(String, Vec), String> { let args: EditFileArgs = serde_json::from_value(args_value.clone()).map_err(|e| format!("edit_file: {e}"))?; + let path = expand_path(&args.path); let original = ops - .read_text_file(session_id, args.path.clone(), None, None) + .read_text_file(session_id, path.clone(), None, None) .await - .map_err(|e| format!("edit_file: read {}: {e:#}", args.path.display()))?; + .map_err(|e| format!("edit_file: read {}: {e:#}", path.display()))?; let occurrences = original.matches(args.old_text.as_str()).count(); if occurrences == 0 { return Err(format!( "edit_file: old_text not found in {}", - args.path.display() + path.display() )); } if occurrences > 1 { return Err(format!( "edit_file: old_text appears {occurrences} times in {} — make it unique", - args.path.display() + path.display() )); } let new_content = original.replacen(args.old_text.as_str(), args.new_text.as_str(), 1); - ops.write_text_file(session_id, args.path.clone(), new_content.clone()) + ops.write_text_file(session_id, path.clone(), new_content.clone()) .await - .map_err(|e| format!("edit_file: write {}: {e:#}", args.path.display()))?; - let diff = Diff::new(args.path.clone(), new_content.clone()).old_text(original); - let summary = format!( - "edited {} ({} bytes)", - args.path.display(), - new_content.len() - ); + .map_err(|e| format!("edit_file: write {}: {e:#}", path.display()))?; + let diff = Diff::new(path.clone(), new_content.clone()).old_text(original); + let summary = format!("edited {} ({} bytes)", path.display(), new_content.len()); Ok((summary, vec![ToolCallContent::Diff(diff)])) } fn exec_list_dir(args_value: &serde_json::Value) -> Result<(String, Vec), String> { let args: ListDirArgs = serde_json::from_value(args_value.clone()).map_err(|e| format!("list_dir: {e}"))?; - let entries = std::fs::read_dir(&args.path) - .map_err(|e| format!("list_dir: read {}: {e}", args.path.display()))?; + let path = expand_path(&args.path); + let entries = + std::fs::read_dir(&path).map_err(|e| format!("list_dir: read {}: {e}", path.display()))?; let mut lines: Vec = Vec::new(); for entry in entries.flatten() { let name = entry.file_name().to_string_lossy().into_owned(); @@ -734,7 +776,15 @@ async fn exec_bash( ) -> Result<(String, Vec), String> { let args: BashArgs = serde_json::from_value(args_value.clone()).map_err(|e| format!("bash: {e}"))?; - let cwd = args.cwd.unwrap_or_else(|| session_cwd.to_path_buf()); + // Expand the cwd if the model passed one; otherwise inherit the + // session cwd verbatim. Don't expand the command string — sh + // already handles `~` and `$HOME` inside the command line, and + // pre-expanding would break the more interesting cases + // (`echo ~`, `cd ~`, …). + let cwd = match args.cwd { + Some(c) => expand_path(&c), + None => session_cwd.to_path_buf(), + }; tracing::debug!( command = %args.command, @@ -1261,4 +1311,148 @@ mod tests { "reads in plan mode must not prompt: {events:?}" ); } + + // ── Path expansion + local read fallback ──────────────────────── + + #[tokio::test] + // We must hold the env-mutation lock across the await — releasing + // it would let another test mutate HOME mid-dispatch and lose + // the very thing we're testing for. The clippy lint is the + // correct *default*; this is the documented exception. + #[allow(clippy::await_holding_lock)] + async fn read_file_expands_tilde_before_dispatch() { + // HOME mutation is process-global; serialise tests that + // touch it under a single std::sync::Mutex. + use std::sync::Mutex; + static LOCK: Mutex<()> = Mutex::new(()); + let _g = LOCK.lock().unwrap(); + let prior = std::env::var("HOME").ok(); + unsafe { + std::env::set_var("HOME", "/home/me"); + } + + let fake = FakeClient::default(); + // The fake's canned-read map is keyed on the expanded path, + // not the literal `~/...` — if expansion didn't happen the + // lookup would miss and ACP would error → fallback to + // local-fs (which also misses → final error). So a success + // path here proves expansion ran before dispatch. + fake.set_read(PathBuf::from("/home/me/notes.md"), Ok("body".into())); + let res = dispatch_tool_call( + &fake, + &sid(), + &mode_default(), + Path::new("/tmp"), + make_call(READ_FILE, json!({"path": "~/notes.md"})), + &CancellationToken::new(), + ) + .await; + + unsafe { + match prior { + Some(p) => std::env::set_var("HOME", p), + None => std::env::remove_var("HOME"), + } + } + + assert!(!res.is_error, "result: {}", res.content); + assert_eq!(res.content, "body"); + } + + #[tokio::test] + async fn read_file_falls_back_to_local_fs_when_acp_errors() { + // ACP read errors → local std::fs reads succeed for a file + // we control. Use a temp file under CARGO_TARGET_TMPDIR. + let tmpdir = std::env::var("CARGO_TARGET_TMPDIR") + .ok() + .map(PathBuf::from) + .unwrap_or_else(std::env::temp_dir); + std::fs::create_dir_all(&tmpdir).unwrap(); + let pid = std::process::id(); + let target = tmpdir.join(format!("helexa-acp-fallback-{pid}.txt")); + std::fs::write(&target, "line 1\nline 2\nline 3\n").unwrap(); + + let fake = FakeClient::default(); + // No canned read → ACP returns Err. Fallback path should + // pick up the local file. + let res = dispatch_tool_call( + &fake, + &sid(), + &mode_default(), + Path::new("/tmp"), + make_call(READ_FILE, json!({"path": target.to_str().unwrap()})), + &CancellationToken::new(), + ) + .await; + let _ = std::fs::remove_file(&target); + assert!(!res.is_error, "expected fallback success: {}", res.content); + assert!( + res.content.contains("line 1") && res.content.contains("line 3"), + "unexpected fallback content: {}", + res.content + ); + } + + #[tokio::test] + async fn read_file_fallback_honours_line_and_limit() { + let tmpdir = std::env::var("CARGO_TARGET_TMPDIR") + .ok() + .map(PathBuf::from) + .unwrap_or_else(std::env::temp_dir); + std::fs::create_dir_all(&tmpdir).unwrap(); + let pid = std::process::id(); + let target = tmpdir.join(format!("helexa-acp-fallback-slice-{pid}.txt")); + std::fs::write(&target, "a\nb\nc\nd\ne\n").unwrap(); + + let fake = FakeClient::default(); + let res = dispatch_tool_call( + &fake, + &sid(), + &mode_default(), + Path::new("/tmp"), + make_call( + READ_FILE, + json!({"path": target.to_str().unwrap(), "line": 2, "limit": 2}), + ), + &CancellationToken::new(), + ) + .await; + let _ = std::fs::remove_file(&target); + assert!(!res.is_error, "result: {}", res.content); + assert_eq!(res.content, "b\nc"); + } + + #[tokio::test] + async fn read_file_fallback_failure_surfaces_combined_error() { + let fake = FakeClient::default(); + let res = dispatch_tool_call( + &fake, + &sid(), + &mode_default(), + Path::new("/tmp"), + make_call( + READ_FILE, + json!({"path": "/definitely/not/a/real/path/xyz"}), + ), + &CancellationToken::new(), + ) + .await; + assert!(res.is_error, "expected error: {}", res.content); + // The error message should cite BOTH the ACP failure and + // the local-fs failure so the model knows what happened. + assert!( + res.content.contains("local fallback also failed"), + "expected combined error message, got: {}", + res.content + ); + } + + #[test] + fn apply_line_limit_basic_slice() { + let body = "a\nb\nc\nd\ne"; + assert_eq!(apply_line_limit(body, None, None), body); + assert_eq!(apply_line_limit(body, Some(1), Some(2)), "a\nb"); + assert_eq!(apply_line_limit(body, Some(3), None), "c\nd\ne"); + assert_eq!(apply_line_limit(body, None, Some(2)), "a\nb"); + } }