jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-10-28 13:01:10 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(buzz-dev-mcp): expand leading ~ in read_file/str_replace paths (#6271)

Browse Source 
## Problem

`resolve_path` (crates/buzz-dev-mcp/src/paths.rs) only ever treats a
path as absolute or joins it onto the workspace root. A user-named tilde
path like `~/.claude/skills/context-health-check/SKILL.md` is not
absolute, so it resolves to `<workdir>/~/.claude/...` (e.g.
`/app/~/.claude/...`), which never exists. The `shell` tool expands `~`
via bash, so the file tools diverged from it.

Surfaced in the PR #6261 named-path benchmark: all five agents initially
passed a literal `~` to `read_file`, which resolved incorrectly under
`/app/~`. They recovered, but the ergonomics are wrong — agents already
expect `~` to work; the resolver violated that expectation.

## Fix

Expand a leading `~` (bare `~` or `~/...`) to the user home directory at
the single chokepoint `resolve_path`, matching shell semantics. Home is
read from `$HOME` (`%USERPROFILE%` on Windows). Both `read_file` and
`str_replace` route through `resolve_path`, so both are fixed.

- `~user` (another user home) is intentionally NOT handled — it needs a
passwd lookup and is out of scope, consistent with the conservative
posture for un-mappable MSYS forms already in this file. `~user...`
falls through untouched and fails with the clear `path not accessible`
error.
- `expand_tilde` takes `home` as a parameter so it stays pure and
unit-testable without mutating process environment; env lookup lives in
a thin `home_dir()` helper.
- Tool/param descriptions are intentionally left unchanged: `~` is
normal shared path behavior, not a per-tool feature. Advertising it in
each `resolve_path` caller's description would spend prompt tokens on an
implementation detail and invite drift across callers. The tests on
`resolve_path` document the behavior.

## Tests

- `expand_tilde_forms`: pure coverage of non-tilde passthrough,
`~user`/`~foo` passthrough, bare `~`, `~/rest`, and unset/empty home.
- `resolve_path_expands_tilde_against_home`: end-to-end through the real
`home_dir()` env read (creates a marker file under `$HOME`, skips
cleanly if no home).
- Full package suite green: `cargo test -p buzz-dev-mcp` — 98 passed, 0
failed. `cargo clippy -p buzz-dev-mcp --all-targets` clean.

Closes #6270

---

## Follow-up (review by @wpfleger): honor `$HOME` on Windows + translate
MSYS home

The first cut read `%USERPROFILE%` on Windows and `$HOME` elsewhere.
That diverges from the shell tool, which is the whole point of this fix:

- `HOME` is passed through to the MCP child on **every** platform
(`buzz-agent` `PASSTHROUGH_ENV`, not cfg-gated) and inherited verbatim
by the `shell` tool (`shell.rs` spawns bash with no `env_clear`). Bash
expands `~` against `HOME`. So on Windows the file tools and shell tool
disagreed whenever `HOME != USERPROFILE` — e.g. a git-bash
`HOME=/c/Users/x`, or an `mcpServers[].env` override setting `HOME`.
- Ordering also mattered: `~` expansion runs **after** `msys_to_windows`
in `resolve_path`, so a spliced-in MSYS home (`/c/Users/x`) never got
translated and `canonicalize` rejected it on Windows.

**Change (`paths.rs` only):**
- Prefer non-empty `$HOME` on every platform; fall back to
`%USERPROFILE%` only on Windows when `HOME` is unset.
- On Windows, run the chosen home through `msys_to_windows` so
`HOME=/c/Users/x` → `C:\Users\x`. An MSYS home with no Windows
equivalent (`/home/x`) is left untranslated and fails with the clear
`path not accessible` error — the intended conservative outcome.
- Selection factored into a pure `select_home(home, userprofile)` so it
is unit-testable without mutating process env.

**Tests:** `select_home_prefers_home` (HOME-first + empty/unset
fallback, all platforms) and
`select_home_translates_msys_home_and_prefers_it` (Windows: MSYS
translation + divergent `HOME`/`USERPROFILE`, which also fixes the prior
test's silent no-op when `HOME` is unset on CI).

`cargo test -p buzz-dev-mcp` — 99 passed, 0 failed; clippy clean.
Pre-push `rust-tests` + `desktop-tauri-checks` green. HEAD `29e0082ff`.

---------

Signed-off-by: Salman Mohammed <smohammed@squareup.com>
Co-authored-by: Leo <5faf251baee50ee6bcde338aef6acdd70bb3e60115664c2cd490d94a55dfc488@buzz.block.builderlab.xyz>
This commit is contained in:
Salman Mohammed
2026-08-19 14:49:00 -04:00
committed by GitHub
parent bbd20fae75
commit 7f69b13b45
1 changed files with 204 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

206
crates/buzz-dev-mcp/src/paths.rs
View File

@@ -1,8 +1,10 @@
//! Path resolution and file I/O shared across dev-mcp tools.
//!
//! `resolve_path` resolves and canonicalizes a user-supplied path against a
//! workspace root. No containment enforcement — the resolved path may land
//! anywhere on the filesystem (consistent with the `shell` tool's posture).
//! workspace root. A leading `~` expands to the user's home directory (bare
//! `~` or `~/...`), matching the shell tool. No containment enforcement — the
//! resolved path may land anywhere on the filesystem (consistent with the
//! `shell` tool's posture).
//!
//! `read_text_file` builds on `resolve_path` to provide the full
//! resolve → stat → size-check → read → UTF-8 decode pipeline shared by
@@ -28,6 +30,17 @@ pub(crate) fn resolve_path(root: &Path, path: &str) -> Result<PathBuf, String> {
#[cfg(windows)]
let path = &msys_to_windows(path);
// Expand a leading `~` (bare or `~/...`) to the user's home directory,
// matching the shell tool's tilde semantics. Without this, a user-named
// path like `~/.claude/skills/x` takes the relative branch and resolves
// under the workspace root (`<root>/~/.claude/...`), which never exists.
// We deliberately do NOT handle `~user` (another user's home): that needs
// a passwd lookup and is out of scope, mirroring the conservative posture
// for un-mappable MSYS forms above. `~user...` falls through untouched and
// fails with the clear `path not accessible` error rather than mis-mapping.
let expanded = expand_tilde(path, home_dir().as_deref());
let path: &str = expanded.as_deref().unwrap_or(path);
let raw = Path::new(path);
let candidate: PathBuf = if raw.is_absolute() {
raw.to_path_buf()
@@ -41,6 +54,88 @@ pub(crate) fn resolve_path(root: &Path, path: &str) -> Result<PathBuf, String> {
Ok(resolved)
}
/// Expand a leading `~` to the user's home directory, returning `Some(expanded)`
/// when a rewrite happened and `None` when the input should be used unchanged.
///
/// Handles the two shell forms that map deterministically to a home directory:
/// - bare `~` -> `home`
/// - `~/rest` (or `~\rest` on Windows) -> `<home>/rest`
///
/// A leading `~` followed by anything else (`~user`, `~+`, `~foo`) is a form we
/// cannot resolve without extra state, so it is left untouched — consistent with
/// how `msys_to_windows` leaves un-mappable inputs alone. Returns `None` when
/// `home` is `None` (unset) so the caller falls back to the raw path. Kept pure
/// (home passed in) so it is testable without mutating process environment.
fn expand_tilde(path: &str, home: Option<&str>) -> Option<String> {
let rest = path.strip_prefix('~')?;
// Only a bare `~` or a `~` immediately followed by a path separator is a
// home-relative reference. Anything else (`~user`) is left to the caller.
let is_sep = |c: char| c == '/' || (cfg!(windows) && c == '\\');
if !rest.is_empty() && !rest.starts_with(is_sep) {
return None;
}
let home = home?;
if home.is_empty() {
return None;
}
if rest.is_empty() {
// Bare `~` -> home directory.
return Some(home.to_string());
}
// `~/rest` -> `<home>/rest`. `rest` begins with a separator, so strip it to
// avoid an absolute-looking join and let `Path` re-add the separator.
let tail = rest.trim_start_matches(is_sep);
let joined = Path::new(home).join(tail);
Some(joined.to_string_lossy().into_owned())
}
/// The user's home directory from the environment. Reads `$HOME` first, falling
/// back to `%USERPROFILE%` on Windows, then hands the raw values to `select_home`
/// (pure, so it is testable without mutating process env). Returns `None` if no
/// usable value is set or the value is not UTF-8.
fn home_dir() -> Option<String> {
let home = std::env::var_os("HOME").and_then(|v| v.into_string().ok());
#[cfg(windows)]
let userprofile = std::env::var_os("USERPROFILE").and_then(|v| v.into_string().ok());
#[cfg(not(windows))]
let userprofile: Option<String> = None;
select_home(home.as_deref(), userprofile.as_deref())
}
/// Choose the home directory from the two env candidates, preferring `$HOME`.
///
/// `$HOME` is preferred because that is exactly what bash — and therefore the
/// `shell` tool — expands `~` against, and `HOME` is passed through to the MCP
/// child on every platform (see `buzz-agent`'s `PASSTHROUGH_ENV`). Picking
/// `USERPROFILE` first on Windows would diverge from the shell tool whenever the
/// two differ (a git-bash `HOME=/c/Users/x`, or an `mcpServers[].env` override),
/// which is precisely the "match the shell tool" contract this fix exists for.
/// `USERPROFILE` is only a Windows fallback for when `HOME` is unset.
///
/// On Windows the chosen value is passed through `msys_to_windows` so an MSYS
/// `HOME` (`/c/Users/x`) becomes a native path (`C:\Users\x`) — `~` expansion
/// happens after `msys_to_windows` in `resolve_path`, so the spliced-in home
/// would otherwise never be translated and `canonicalize` would reject it. An
/// MSYS form with no Windows equivalent (`/home/x`) falls through untranslated
/// and fails with the clear `path not accessible` error, the correct outcome.
/// Empty strings are treated as unset.
fn select_home(home: Option<&str>, userprofile: Option<&str>) -> Option<String> {
fn non_empty(v: Option<&str>) -> Option<&str> {
v.filter(|s| !s.is_empty())
}
let chosen = non_empty(home).or_else(|| non_empty(userprofile))?;
#[cfg(windows)]
{
Some(msys_to_windows(chosen))
}
#[cfg(not(windows))]
{
Some(chosen.to_string())
}
}
/// Translate the MSYS/Cygwin absolute path forms bash would accept into a
/// native Windows path, matching `cygpath -w` semantics so the file tools
/// resolve the same inputs the `shell` tool does. Anything that is not a
@@ -208,6 +303,80 @@ mod tests {
assert!(p.ends_with("file.txt"));
}
// `expand_tilde` is pure (home is passed in), so these cases need no env
// mutation and cannot race parallel tests.
#[test]
fn expand_tilde_forms() {
let home = "/home/agent";
// Non-tilde inputs are never rewritten.
assert_eq!(expand_tilde("file.txt", Some(home)), None);
assert_eq!(expand_tilde("/abs/path", Some(home)), None);
assert_eq!(expand_tilde("sub/~notleading", Some(home)), None);
// `~user` and other non-separator suffixes are left for the caller.
assert_eq!(expand_tilde("~user/x", Some(home)), None);
assert_eq!(expand_tilde("~foo", Some(home)), None);
// Bare `~` and `~/rest` expand against the supplied home.
assert_eq!(expand_tilde("~", Some(home)), Some(home.to_string()));
let expanded = expand_tilde("~/.claude/skills/x", Some(home)).expect("expands");
assert_eq!(
expanded,
Path::new(home).join(".claude/skills/x").to_string_lossy()
);
// Unset or empty home -> no rewrite, caller falls back to the raw path.
assert_eq!(expand_tilde("~/rest", None), None);
assert_eq!(expand_tilde("~", None), None);
assert_eq!(expand_tilde("~/rest", Some("")), None);
}
// `select_home` is pure (both env candidates passed in), so it exercises the
// HOME-first preference and empty/unset handling without mutating process
// env or racing parallel tests. `select_home` itself does not gate the
// fallback by platform — `home_dir` is what only supplies `userprofile` on
// Windows — so these assertions hold identically on every platform.
#[test]
fn select_home_prefers_home() {
// $HOME wins when both are set.
assert_eq!(
select_home(Some("/home/agent"), Some("/other")),
Some("/home/agent".to_string())
);
// Empty $HOME is treated as unset -> fall back to the second candidate.
assert_eq!(
select_home(Some(""), Some("/other")),
Some("/other".to_string())
);
// No usable candidate -> None.
assert_eq!(select_home(None, None), None);
assert_eq!(select_home(Some(""), Some("")), None);
}
// End-to-end through `resolve_path`, exercising the real `home_dir()` env
// read: a `~/...` path resolves against the actual home directory, not the
// workspace root. Uses a temp file created under the real home so it does
// not mutate the environment.
#[test]
fn resolve_path_expands_tilde_against_home() {
let home = match home_dir() {
Some(h) if !h.is_empty() => h,
_ => return, // No home in this environment (e.g. minimal CI) — skip.
};
let marker = format!(".dev-mcp-tilde-test-{}", std::process::id());
let target = Path::new(&home).join(&marker);
fs::write(&target, b"z").expect("write under home");
let workspace = tempdir().expect("tempdir");
let resolved = resolve_path(workspace.path(), &format!("~/{marker}"))
.expect("tilde path resolves against home, not workspace");
let want = std::fs::canonicalize(&target).expect("canon");
assert_eq!(resolved, want);
let _ = fs::remove_file(&target);
}
// Windows MSYS-absolute path translation. These test `msys_to_windows`
// directly (the pure rewrite) rather than `resolve_path`, because the latter
// canonicalizes against the real filesystem and we want deterministic
@@ -239,6 +408,39 @@ mod tests {
assert_eq!(msys_to_windows(r"C:\Users\x"), r"C:\Users\x");
}
// Windows `select_home` behavior: HOME still wins over USERPROFILE, and
// an MSYS-form HOME is translated to a native path so the value spliced
// in during `~` expansion (which runs after `msys_to_windows`) resolves.
// Both candidates are passed in, so this needs no process-env mutation
// and does not silently no-op the way a real-env read would when HOME is
// unset on CI.
#[test]
fn select_home_translates_msys_home_and_prefers_it() {
// Divergent HOME/USERPROFILE: HOME wins, and its MSYS cygdrive form
// is translated to the native path so canonicalize can use it.
assert_eq!(
select_home(Some("/c/Users/agent"), Some(r"C:\Users\other")),
Some(r"C:\Users\agent".to_string())
);
// A native-form HOME is preferred and passes through unchanged.
assert_eq!(
select_home(Some(r"C:\Users\agent"), Some(r"C:\Users\other")),
Some(r"C:\Users\agent".to_string())
);
// HOME unset -> fall back to USERPROFILE (already native).
assert_eq!(
select_home(None, Some(r"C:\Users\other")),
Some(r"C:\Users\other".to_string())
);
// An MSYS HOME with no Windows equivalent (`/home/x`) is left
// untranslated; it fails downstream with a clear error rather than
// being mis-mapped — the intended conservative outcome.
assert_eq!(
select_home(Some("/home/agent"), None),
Some("/home/agent".to_string())
);
}
#[test]
fn relative_path_passes_through_unchanged() {
// No leading slash — left for the caller's `root.join`.