feat(helexa-acp): expand ~ / $HOME and fall back to local fs on ACP read errors

Two related polish fixes for daily use:

- New `path_util` module expands `~`, `~/…`, `$HOME`, and `$HOME/…`
  prefixes in every tool that takes a path (read_file, write_file,
  edit_file, list_dir, bash cwd). The expansion is also applied to
  the plan-mode write gate so `~/.local/share/helexa-acp/plans/…`
  comparisons behave correctly regardless of which form the model
  emits.
- `read_file` now falls back to `std::fs::read_to_string` when ACP's
  `fs/read_text_file` errors out. Zed's workspace-scoped read was
  the source of "model can't see ~/git/architecture/generic.md"
  when the session cwd is a different project; the fallback lets
  the agent pull in shared material that lives outside the active
  workspace, the same way `list_dir` already does via local
  `std::fs::read_dir`. Local fallback honours line/limit args.

The fallback also produces a combined error message when both ACP
and local-fs reads fail, so the model sees what actually broke
rather than just the ACP-side error.

14 new unit tests cover path_util's prefix matrix, fallback
success/failure paths, and the line/limit slicing in fallback.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

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;

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<F: FnOnce()>(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")
+            );
+        });
+    }
+}

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<ToolCallContent>), 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<u32>, limit: Option<u32>) -> 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::<Vec<_>>()
+        .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<ToolCallContent>), 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<ToolCallContent>), 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<ToolCallContent>), 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<String> = 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<ToolCallContent>), 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");
+    }
 }