From 2de1cf55a3df8aecb09ffd27bb1dcfb3a138a4c8 Mon Sep 17 00:00:00 2001 From: Joe Taber Date: Thu, 6 Aug 2026 07:59:28 -0500 Subject: [PATCH] agent: Improve working directory path resolution in the terminal tool (#59937) # Objective This PR aims to fix these related deficiencies with the terminal tool's working directory path resolution: - Fix #60014 - Fix #60040 - Fix #60043 ## Solution - Use `project.path_style(cx).is_absolute(dir)` instead of `Path::is_absolute`. The latter follows the path semantics of the host which fails to recognize `/home/...` etc as an absolute path on Windows, we want to follow the path semantics of the *project*. - Splits out path resolution into a separate pure function `resolve_cd_in_worktrees` to enable comprehensive cross-PathStyle testing - The function unifies path prefix checking across both absolute and relative modes, using the `path_style.strip_prefix` (which uses the RelPath util internally) which fails when subdirs try to escape with `..`. - Add comprehensive tests for absolute/relative modes, path styles, and potential `..` escapes - Enables targeting subdirectories with both absolute and relative path modes (separate commit) - Also makes both path modes use `project.worktrees`. Before, the relative path mode used `project.visible_worktrees`. ## Testing Here's how I tested these changes: - Added a new test: `test_resolve_cd_uses_project_path_style` with a comprehensive collection of assertions for `..` edge cases - Run `cargo test -p agent` and `./script/clippy -p agent` on: - Linux - Windows in Ubuntu WSL - Windows native - Build and run Zed on: - Linux - Windows - Verify that the three issues are fixed: - `cd` param path traversal escapes are blocked - Linux local project - Windows local project - Windows remote WSL project - Allows absolute paths when running on Windows in a WSL project - `cd` param may target subdirectories in both absolute and relative path modes - Linux local project - Windows local project - Windows WSL project - Check that different models find the updated tool descriptions to be understandable and usable the first time. Used the prompt below to get the following models to test path resolution on each of: Windows local, Windows WSL, Linux. All models except GPT-5-nano one-shot the below prompt. - Claude Sonnet 4.6 - Claude Opus 4.8 - Claude Haiku 4.5 - GPT-5.5 pro - GPT-5-nano - got there eventually but is confused by the design of the cd param, it expects that if you set cd to `my-project` that it will target the directory `my-project/my-project`. - Gemini 3.1 Pro - Gemini 3.5 Flash - Grok 4.3 - Grok 0.1 build
Agent test prompt
List the current directory contents and pick one subdir. Then use that information and the project path details you were provided to execute `pwd` with the terminal tool's cd parameter set to the following values: - current project absolute path - current project name - current project absolute path + an existing subdir - current project name + an existing subdir - absolute path to user's .ssh dir - current project absolute path + whatever necessary path traversal .. + .ssh segments to target the user's .ssh dir Then tell me what happened. Then stop.
## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable --- Release Notes: - Fixed terminal tool targeting absolute directories on Windows host connected to a remote SSH / WSL project - Fixed terminal tool not blocking path traversal escapes - Improved terminal tool targeting project subdirectories --------- Co-authored-by: Richard Feldman Co-authored-by: Richard Feldman --- crates/agent/src/tools/terminal_tool.rs | 192 ++++++++++++++++++++++-- 1 file changed, 177 insertions(+), 15 deletions(-) diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index 97a8566bac..ea0ea51eec 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -29,7 +29,7 @@ const COMMAND_OUTPUT_LIMIT: u64 = 16 * 1024; /// /// The output results will be shown to the user already, only list it again if necessary, avoid being redundant. /// -/// Make sure you use the `cd` parameter to navigate to one of the root directories of the project. NEVER do it as part of the `command` itself, otherwise it will error. +/// Always set the working directory with the `cd` parameter, never with `cd` inside `command`; otherwise it will error. /// /// Do not generate terminal commands that use shell substitutions or interpolations such as `$VAR`, `${VAR}`, `$(...)`, backticks, `$((...))`, `<(...)`, or `>(...)`. Resolve those values yourself before calling this tool, or ask the user for the literal value to use. /// @@ -53,7 +53,7 @@ pub struct TerminalToolInput { /// /// REMINDER: read-only git commands (`git log`, `git diff`, `git show`, `git blame`) MUST include `--no-pager` (e.g. `git --no-pager log`). Prefer `git --no-optional-locks status` over `git status` to avoid optional metadata writes. Git commands that may open an editor (`git rebase`, `git commit`, `git merge`, `git tag`) MUST be prefixed with `GIT_EDITOR=true ` (e.g. `GIT_EDITOR=true git rebase origin/main`). Otherwise the terminal will hang. pub command: String, - /// Working directory for the command. This must be one of the root directories of the project. + /// Working directory: a project root directory or any subdirectory of one, given by name or absolute path. E.g. `my-project/src`, `/home/user/my-project`, or on Windows `my-project\src` or `C:\Users\me\my-project`. pub cd: String, /// Optional maximum runtime (in milliseconds). If exceeded, the running terminal task is killed. pub timeout_ms: Option, @@ -71,7 +71,7 @@ pub struct TerminalToolInput { /// /// The output results will be shown to the user already, only list it again if necessary, avoid being redundant. /// -/// Make sure you use the `cd` parameter to navigate to one of the root directories of the project. NEVER do it as part of the `command` itself, otherwise it will error. +/// Always set the working directory with the `cd` parameter, never with `cd` inside `command`; otherwise it will error. /// /// Do not generate terminal commands that use shell substitutions or interpolations such as `$VAR`, `${VAR}`, `$(...)`, backticks, `$((...))`, `<(...)`, or `>(...)`. Resolve those values first or ask the user for the literal value to use. /// @@ -95,7 +95,7 @@ pub struct SandboxedTerminalToolInput { /// /// REMINDER: read-only git commands (`git log`, `git diff`, `git show`, `git blame`) MUST include `--no-pager` (e.g. `git --no-pager log`). Prefer `git --no-optional-locks status` over `git status` to avoid optional metadata writes. Git commands that may open an editor (`git rebase`, `git commit`, `git merge`, `git tag`) MUST be prefixed with `GIT_EDITOR=true ` (e.g. `GIT_EDITOR=true git rebase origin/main`). Otherwise the terminal will hang. pub command: String, - /// Working directory for the command. This must be one of the root directories of the project. + /// Working directory: a project root directory or any subdirectory of one, given by name or absolute path. E.g. `my-project/src`, `/home/user/my-project`, or on Windows `my-project\src` or `C:\Users\me\my-project`. pub cd: String, /// Optional maximum runtime (in milliseconds). If exceeded, the running terminal task is killed. pub timeout_ms: Option, @@ -1355,27 +1355,189 @@ fn working_dir(cd: &str, project: &Entity, cx: &mut App) -> Result Ok(None), } } else { - let input_path = Path::new(cd); + let path_style = project.path_style(cx); + let worktree_roots = project + .worktrees(cx) + .filter_map(|worktree| { + let worktree = worktree.read(cx); + // Skip single-file worktrees: a file can't be a working directory. + let root_dir = worktree.root_dir()?; + Some((worktree.root_name_str(), root_dir.to_path_buf())) + }) + .collect::>(); - if input_path.is_absolute() { - if project - .worktrees(cx) - .any(|worktree| input_path.starts_with(&worktree.read(cx).abs_path())) - { - return Ok(Some(input_path.into())); - } - } else if let Some(worktree) = project.worktree_for_root_name(cd, cx) { - return Ok(Some(worktree.read(cx).abs_path().to_path_buf())); + if let Some(dir) = resolve_cd_in_worktrees(cd, path_style, &worktree_roots) { + return Ok(Some(dir)); } - anyhow::bail!("`cd` directory {cd:?} was not in any of the project's worktrees."); + anyhow::bail!("`cd` directory {cd:?} was not in any root directory in the project."); } } +/// Resolves a `cd` argument to an absolute worktree directory. `cd` may be a +/// worktree's root name or an absolute path to a worktree or a subdirectory +/// therein. +/// +/// Absolute paths are classified with the project's [`PathStyle`] rather than +/// the host's, so an absolute POSIX path resolves correctly on a Windows host +/// driving a WSL/SSH project (#60040). +/// +/// Both `cd` and the worktree roots are lexically normalized before prefix +/// matching. This resolves `.` and `..` components up front, so a path that +/// escapes a worktree does not have that worktree's root as a prefix and is +/// rejected (#60014). On Windows-style projects it also unifies `/` and `\` +/// separators, since models frequently write `C:/foo/bar` for a root stored +/// as `C:\foo\bar`. +fn resolve_cd_in_worktrees( + cd: &str, + path_style: util::paths::PathStyle, + worktree_roots: &[(&str, PathBuf)], +) -> Option { + let cd = path_style.normalize(cd); + let cd_path = Path::new(&cd); + let is_absolute = path_style.is_absolute(&cd); + + worktree_roots.iter().find_map(|(root_name, abs_path)| { + let prefix = if is_absolute { + path_style.normalize(abs_path.to_str()?) + } else { + (*root_name).to_string() + }; + let subpath = path_style.strip_prefix(cd_path, Path::new(&prefix))?; + if subpath.is_empty() { + Some(abs_path.clone()) + } else { + path_style + .join_path(abs_path, &*subpath.display(path_style)) + .ok() + } + }) +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn test_resolve_cd_uses_project_path_style() { + use util::paths::PathStyle::{Unix, Windows}; + + // Deliberately ambiguous root names to stress test path resolution. + let unix_roots: Vec<(&str, PathBuf)> = vec![ + ("worktree", PathBuf::from("/a/worktree")), + ("worktree", PathBuf::from("/b/worktree")), + ]; + // Worktree roots are stored with backslash separators on Windows, but + // models frequently write paths with forward slashes; both must match. + let windows_roots = vec![("worktree", PathBuf::from("C:\\work\\worktree"))]; + + // absolute paths + assert_eq!( + resolve_cd_in_worktrees("/b/worktree", Unix, &unix_roots), + Some(PathBuf::from("/b/worktree")), + "a POSIX-absolute path resolves under a POSIX project path style even on a Windows host" + ); + assert_eq!( + resolve_cd_in_worktrees("/a/worktree/src", Unix, &unix_roots), + Some(PathBuf::from("/a/worktree/src")), + "an absolute path inside a worktree resolves to the same path" + ); + assert_eq!( + resolve_cd_in_worktrees("/elsewhere", Unix, &unix_roots), + None, + "an absolute path outside every worktree is rejected" + ); + assert_eq!( + resolve_cd_in_worktrees("/a/worktree/src/../docs", Unix, &unix_roots), + Some(PathBuf::from("/a/worktree/docs")), + "an absolute path that stays within its worktree via `..` resolves to the same path" + ); + assert_eq!( + resolve_cd_in_worktrees("/a/worktree/../escape", Unix, &unix_roots), + None, + "an absolute path that escapes its worktree via `..` is rejected" + ); + assert_eq!( + resolve_cd_in_worktrees("/a/worktree/../../b/worktree", Unix, &unix_roots), + Some(PathBuf::from("/b/worktree")), + "a path whose `..` components lexically resolve into a valid worktree is accepted" + ); + assert_eq!( + resolve_cd_in_worktrees("/a/worktree//src/", Unix, &unix_roots), + Some(PathBuf::from("/a/worktree/src")), + "doubled and trailing separators are normalized away" + ); + + // relative root names + assert_eq!( + resolve_cd_in_worktrees("worktree", Unix, &unix_roots), + Some(PathBuf::from("/a/worktree")), + "a root-relative path to a worktree root resolves to the first matching worktree" + ); + assert_eq!( + resolve_cd_in_worktrees("worktree/src", Unix, &unix_roots), + Some(PathBuf::from("/a/worktree/src")), + "a root-relative path to a subdirectory resolves to the absolute path" + ); + assert_eq!( + resolve_cd_in_worktrees("worktree/src/../doc", Unix, &unix_roots), + Some(PathBuf::from("/a/worktree/doc")), + "a root-relative path to a subdirectory with `..` resolves to a clean absolute path" + ); + assert_eq!( + resolve_cd_in_worktrees("worktree/../escape", Unix, &unix_roots), + None, + "a root-relative path that escapes the worktree via `..` is rejected" + ); + assert_eq!( + resolve_cd_in_worktrees("worktreeextra", Unix, &unix_roots), + None, + "a root-relative path that is not any of the worktree roots is rejected" + ); + assert_eq!( + resolve_cd_in_worktrees("./worktree", Unix, &unix_roots), + Some(PathBuf::from("/a/worktree")), + "a leading `./` is normalized away" + ); + + // Windows paths + assert_eq!( + resolve_cd_in_worktrees("C:\\work\\worktree", Windows, &windows_roots), + Some(PathBuf::from("C:\\work\\worktree")), + "Windows-absolute paths to root directories resolve" + ); + assert_eq!( + resolve_cd_in_worktrees("C:/work/worktree", Windows, &windows_roots), + Some(PathBuf::from("C:\\work\\worktree")), + "forward-slash separators match a backslash-stored root" + ); + assert_eq!( + resolve_cd_in_worktrees("c:\\work\\worktree", Windows, &windows_roots), + Some(PathBuf::from("C:\\work\\worktree")), + "drive letters match case-insensitively" + ); + assert_eq!( + resolve_cd_in_worktrees("C:/work/worktree/src", Windows, &windows_roots), + Some(PathBuf::from("C:\\work\\worktree\\src")), + "Windows-absolute paths to subdirectories resolve regardless of separator style" + ); + assert_eq!( + resolve_cd_in_worktrees("worktree\\src", Windows, &windows_roots), + Some(PathBuf::from("C:\\work\\worktree\\src")), + "Windows-relative paths to subdirectories resolve to the absolute path" + ); + assert_eq!( + resolve_cd_in_worktrees("worktree/src", Windows, &windows_roots), + Some(PathBuf::from("C:\\work\\worktree\\src")), + "forward-slash relative paths resolve under a Windows path style" + ); + assert_eq!( + resolve_cd_in_worktrees("C:\\work\\worktree\\..\\escape", Windows, &windows_roots), + None, + "a Windows-absolute path that escapes its worktree via `..` is rejected" + ); + } + #[test] fn test_initial_title_shows_full_multiline_command() { let input = TerminalToolInput {