From 4147ca8689a170735402b3a401f0704ad2de301a Mon Sep 17 00:00:00 2001 From: Max Haarhaus Date: Sat, 15 Aug 2026 15:28:02 -0400 Subject: [PATCH] fix(run): resolve a run's cwd to one path spelling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POSIX gives this away for free: `getcwd` resolves symlinks, so a Unix process and everything it spawns already agree on how the working directory is spelled. Windows makes no such promise — it hands back whatever spelling the cwd was set with — and one directory there has several valid names: an 8.3 short name, a junction, a `subst` drive, a redirected profile. Nothing forced the two sides of the write guard's comparison onto one of them. Measured, spawning each tool the way a harness is spawned: node's `process.cwd()`, node's `realpathSync`, and `cmd`'s `cd` all echo the alias back, while git prints the resolved name for every path it emits. Both spellings are therefore reachable from inside a single task env, and `is_under` compares strings. Driving the hook with a short-form root, a write relative to a long-form cwd is denied, as is an absolute long-form target under the env — legitimate writes, refused. A genuine escape is still denied, so the guard never got weaker, only falsely strict. Every task env is a git repo, which makes `rev-parse --show-toplevel` a one-step route to the refused spelling. Resolve once, where the run's roots are derived, rather than at either end of the comparison: canonicalising only the marker inverts the failure instead of removing it, denying the same write when the cwd is the alias. Every path in a `RunContext` now shares one spelling, which makes it a property of the struct rather than of the one field someone remembered. Resolution walks up to the deepest ancestor that exists and re-attaches the rest, because a run names directories before it creates them and the alias always lives in an ancestor, never in the leaf. The cli fixtures that built roots with a bare `fs::canonicalize` now use the helper that strips the verbatim prefix. They had been comparing against `\?\` paths, a spelling no agent ever produces and one the CLI no longer emits. Co-Authored-By: Claude Opus 5 --- src/core/context.rs | 99 +++++++++++++++++++---- src/core/fs.rs | 157 +++++++++++++++++++++++++++++++++++-- tests/cli/guard_denials.rs | 4 +- tests/cli/helpers.rs | 29 ++++--- tests/cli/stray_writes.rs | 12 +-- 5 files changed, 262 insertions(+), 39 deletions(-) diff --git a/src/core/context.rs b/src/core/context.rs index f22fab4..760437f 100644 --- a/src/core/context.rs +++ b/src/core/context.rs @@ -94,9 +94,10 @@ pub enum ContextError { Io(#[from] std::io::Error), } -/// Lexically absolutize a path (join onto cwd if relative; normalize `.`/`..`). -/// Mirrors node's `resolve()` — it does NOT resolve symlinks or require -/// existence, unlike `std::fs::canonicalize`. +/// Absolutize a path (join onto `cwd` if relative) and resolve it to the one +/// spelling every path in a [`RunContext`] shares — see +/// [`crate::core::fs::real_path`]. Routing every path through here is what makes +/// that a property of the struct rather than of the one field someone remembered. fn absolutize(cwd: &Path, p: &str) -> Result { let path = Path::new(p); let joined = if path.is_absolute() { @@ -104,7 +105,7 @@ fn absolutize(cwd: &Path, p: &str) -> Result { } else { cwd.join(path) }; - Ok(std::path::absolute(joined)?) + Ok(crate::core::fs::real_path(&joined)?) } fn skill_name_from_dir(skill_subdir: &Path) -> Result { @@ -162,8 +163,10 @@ fn infer_only_skill_name(skill_dir: &Path) -> Result { /// validates `SKILL.md`, an optional existing `--bootstrap`, and defaults the /// workspace/stage roots from the current directory. pub fn detect_run_context(input: DetectInput) -> Result { - let cwd = input.cwd.unwrap_or(std::env::current_dir()?); - let cwd = std::path::absolute(cwd)?; + let cwd = input.cwd.map_or_else(std::env::current_dir, Ok)?; + // Every root below derives from this one path, so resolving the alias here + // is what keeps a run's paths and an agent's paths comparable at all. + let cwd = crate::core::fs::real_path(&cwd)?; let (skill_dir, skill_name, skill_subdir, sibling_skill_names, stage_siblings) = match input.skill_dir { Some(skill_dir_raw) => { @@ -292,7 +295,7 @@ mod tests { assert_eq!(ctx.skill_name, "mr-review"); assert_eq!( ctx.skill_subdir, - std::path::absolute(&skill_subdir).unwrap() + crate::core::fs::real_path(&skill_subdir).unwrap() ); assert!(ctx.sibling_skill_names.is_empty()); assert!(!ctx.stage_siblings); @@ -313,7 +316,7 @@ mod tests { assert_eq!(ctx.skill_name, "beta"); assert_eq!( ctx.skill_subdir, - std::path::absolute(skill_dir.join("beta")).unwrap() + crate::core::fs::real_path(&skill_dir.join("beta")).unwrap() ); assert!(ctx.sibling_skill_names.is_empty()); assert!(!ctx.stage_siblings); @@ -414,11 +417,14 @@ mod tests { let tmp = TempDir::new().unwrap(); let skill_dir = make_skill_dir(tmp.path(), &["mr-review"]); let ctx = detect_run_context(input(&skill_dir, "mr-review")).unwrap(); - assert_eq!(ctx.skill_dir, std::path::absolute(&skill_dir).unwrap()); + assert_eq!( + ctx.skill_dir, + crate::core::fs::real_path(&skill_dir).unwrap() + ); assert_eq!(ctx.skill_name, "mr-review"); assert_eq!( ctx.skill_subdir, - std::path::absolute(skill_dir.join("mr-review")).unwrap() + crate::core::fs::real_path(&skill_dir.join("mr-review")).unwrap() ); assert!(ctx.sibling_skill_names.is_empty()); assert!(ctx.bootstrap_path.is_none()); @@ -452,8 +458,8 @@ mod tests { let tmp = TempDir::new().unwrap(); let skill_dir = make_skill_dir(tmp.path(), &["foo"]); let ctx = detect_run_context(input(&skill_dir, "foo")).unwrap(); - let expected = std::env::current_dir().unwrap().join(".eval-magic"); - assert_eq!(ctx.workspace_root, expected); + let cwd = crate::core::fs::real_path(&std::env::current_dir().unwrap()).unwrap(); + assert_eq!(ctx.workspace_root, cwd.join(".eval-magic")); } #[test] @@ -467,7 +473,10 @@ mod tests { ..input(&skill_dir, "foo") }) .unwrap(); - assert_eq!(ctx.workspace_root, std::path::absolute(&custom).unwrap()); + assert_eq!( + ctx.workspace_root, + crate::core::fs::real_path(&custom).unwrap() + ); } #[test] @@ -475,7 +484,67 @@ mod tests { let tmp = TempDir::new().unwrap(); let skill_dir = make_skill_dir(tmp.path(), &["foo"]); let ctx = detect_run_context(input(&skill_dir, "foo")).unwrap(); - assert_eq!(ctx.stage_root, std::env::current_dir().unwrap()); + assert_eq!( + ctx.stage_root, + crate::core::fs::real_path(&std::env::current_dir().unwrap()).unwrap() + ); + } + + /// Every root derives from the cwd, and the guard later compares those roots + /// against paths the agent's own tools report — so an alias of the cwd has to + /// collapse here, once, or the two sides disagree forever after. + /// + /// Windows spells one directory several ways (8.3 short names, junctions, + /// `subst` drives, redirected profiles); each is one `canonicalize` apart + /// from the real path, so exercising one exercises the mechanism. + #[test] + fn a_cwd_alias_collapses_so_every_derived_root_shares_one_spelling() { + let tmp = TempDir::new().unwrap(); + let real = tmp.path().join("real-workspace"); + fs::create_dir_all(&real).unwrap(); + let alias = tmp.path().join("alias-workspace"); + crate::core::fs::create_directory_alias(&real, &alias).unwrap(); + make_skill_dir(&real, &["foo"]); + + // Enter through the alias, exactly as a user whose workspace sits under a + // junction or a redirected profile directory does. + let ctx = detect_run_context(DetectInput { + skill: Some("foo".to_string()), + ..input_from(&alias.join("skill-dir")) + }) + .unwrap(); + + let expected = crate::core::fs::real_path(&real).unwrap(); + assert_eq!(ctx.stage_root, expected.join("skill-dir")); + assert_eq!( + ctx.workspace_root, + expected.join("skill-dir").join(".eval-magic") + ); + assert_eq!(ctx.skill_dir, expected.join("skill-dir")); + } + + /// `--workspace-dir` is the second way into the same tree: the guard's roots + /// descend from it, so an alias passed here would reintroduce the split the + /// cwd resolution just closed. + #[test] + fn an_aliased_workspace_dir_flag_resolves_to_the_same_spelling() { + let tmp = TempDir::new().unwrap(); + let real = tmp.path().join("real-workspace"); + fs::create_dir_all(&real).unwrap(); + let alias = tmp.path().join("alias-workspace"); + crate::core::fs::create_directory_alias(&real, &alias).unwrap(); + let skill_dir = make_skill_dir(tmp.path(), &["foo"]); + + let ctx = detect_run_context(DetectInput { + workspace_dir: Some(alias.join("nested-ws").to_string_lossy().into_owned()), + ..input(&skill_dir, "foo") + }) + .unwrap(); + + assert_eq!( + ctx.workspace_root, + crate::core::fs::real_path(&real).unwrap().join("nested-ws") + ); } #[test] @@ -491,7 +560,7 @@ mod tests { .unwrap(); assert_eq!( ctx.bootstrap_path, - Some(std::path::absolute(&bootstrap).unwrap()) + Some(crate::core::fs::real_path(&bootstrap).unwrap()) ); } diff --git a/src/core/fs.rs b/src/core/fs.rs index 24c0fda..ec2f010 100644 --- a/src/core/fs.rs +++ b/src/core/fs.rs @@ -23,7 +23,7 @@ use std::fs; use std::io; -use std::path::Path; +use std::path::{Path, PathBuf}; use serde::Serialize; @@ -49,16 +49,64 @@ pub fn artifact_path(path: &Path) -> String { if !cfg!(windows) { return rendered.into_owned(); } - let unprefixed = match rendered.strip_prefix(r"\\?\UNC\") { + normalize_separators(&strip_verbatim_prefix(&rendered)) +} + +/// Drop Windows' verbatim (`\\?\`) prefix, keeping the host's own separators. +fn strip_verbatim_prefix(rendered: &str) -> String { + match rendered.strip_prefix(r"\\?\UNC\") { // Verbatim UNC collapses back to the `\\server\share` form; dropping // the whole prefix would leave a bare `UNC\` component. Some(rest) => format!(r"\\{rest}"), None => rendered .strip_prefix(r"\\?\") - .unwrap_or(&rendered) + .unwrap_or(rendered) .to_string(), - }; - normalize_separators(&unprefixed) + } +} + +/// The one spelling of `path` that every participant in a run agrees on. +/// +/// POSIX hands this out for free: `getcwd` resolves symlinks, so a Unix process +/// and everything it spawns already share one spelling of the working directory. +/// Windows makes no such promise — it hands back whatever spelling the cwd was +/// set with — and one directory there has several valid names: an 8.3 short +/// name (`RUNNER~1`), a junction, a `subst` drive, a redirected profile +/// directory. Tools then disagree about which to report: `git` prints the +/// resolved name, while node's `process.cwd()` and `cmd`'s `cd` echo the alias +/// back. Anything comparing those strings — the write guard's allowed roots +/// against the paths an agent's own tools hand it — silently stops matching. +/// +/// Resolving once, at the point a run's roots are derived, gives Windows the +/// guarantee POSIX already provides. The verbatim (`\\?\`) prefix comes off +/// because a spawned child reports the plain form, so plain is the spelling the +/// comparisons actually see. +/// +/// A run names directories before it creates them, so resolution walks up to the +/// deepest ancestor that exists and re-attaches the rest: the alias always lives +/// in an ancestor — a temp dir, a junction, an 8.3 profile name — never in the +/// leaf about to be created. With no ancestor on disk at all, the lexical form +/// is all there is. +pub fn real_path(path: &Path) -> io::Result { + let absolute = std::path::absolute(path)?; + let mut unresolved = Vec::new(); + let mut anchor = absolute.as_path(); + loop { + if let Ok(canonical) = fs::canonicalize(anchor) { + let mut resolved = if cfg!(windows) { + PathBuf::from(strip_verbatim_prefix(&canonical.to_string_lossy())) + } else { + canonical + }; + resolved.extend(unresolved.iter().rev()); + return Ok(resolved); + } + let (Some(parent), Some(name)) = (anchor.parent(), anchor.file_name()) else { + return Ok(absolute); + }; + unresolved.push(name.to_os_string()); + anchor = parent; + } } /// Rewrite Windows separators to forward slashes for *comparison*. @@ -163,6 +211,37 @@ fn create_parent(path: &Path) -> io::Result<()> { } } +/// Make `link` a second name for the directory `target`, for tests that need one +/// directory reachable by two spellings. +/// +/// Deliberately *not* gated on the symlink capability. The paths that resolve +/// aliases differently are a Windows problem, so a fixture that skips on a +/// stock Windows box would leave that platform uncovered exactly where it +/// matters. A junction is the Windows alias that needs no Developer Mode and no +/// elevation, and `canonicalize` collapses it the same way it collapses a +/// symlink, an 8.3 short name, or a `subst` drive. +#[cfg(test)] +pub(crate) fn create_directory_alias(target: &Path, link: &Path) -> io::Result<()> { + if !cfg!(windows) { + return create_symlink(target, link, true); + } + let status = std::process::Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(link) + .arg(target) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status()?; + if status.success() { + return Ok(()); + } + Err(io::Error::other(format!( + "mklink /J could not alias {} to {}", + link.display(), + target.display() + ))) +} + #[cfg(test)] mod tests { use super::*; @@ -194,6 +273,74 @@ mod tests { ) } + /// Two names for one directory have to come back as one path — that is the + /// entire point of the function. + #[test] + fn real_path_collapses_an_alias_onto_the_resolved_spelling() { + let tmp = TempDir::new().unwrap(); + let real = tmp.path().join("real-dir"); + fs::create_dir_all(&real).unwrap(); + fs::create_dir_all(real.join("nested")).unwrap(); + let alias = tmp.path().join("alias-dir"); + create_directory_alias(&real, &alias).unwrap(); + + assert_eq!( + real_path(&alias.join("nested")).unwrap(), + real_path(&real.join("nested")).unwrap() + ); + } + + /// The verbatim prefix is an OS escape hatch: a spawned child reports the + /// plain form, so a root carrying `\\?\` would fail to match every path the + /// comparisons actually see. + #[test] + fn real_path_never_returns_a_verbatim_prefix() { + let tmp = TempDir::new().unwrap(); + let resolved = real_path(tmp.path()).unwrap(); + assert!( + !resolved.to_string_lossy().starts_with(r"\\?\"), + "{resolved:?} still carries the verbatim prefix" + ); + } + + /// A run names directories before it creates them — a workspace root, an + /// iteration dir. Resolving only whole existing paths would leave exactly + /// those unresolved, and the alias lives in the *ancestor* anyway (a temp + /// dir, a junction, an 8.3 profile name), never in the leaf about to be + /// created. So resolve as far down as the disk goes and re-attach the rest. + #[test] + fn real_path_resolves_the_existing_ancestor_of_a_path_not_yet_created() { + let tmp = TempDir::new().unwrap(); + let real = tmp.path().join("real-dir"); + fs::create_dir_all(&real).unwrap(); + let alias = tmp.path().join("alias-dir"); + create_directory_alias(&real, &alias).unwrap(); + + let unborn = alias.join("workspace").join("iteration-1"); + assert_eq!( + real_path(&unborn).unwrap(), + real_path(&real) + .unwrap() + .join("workspace") + .join("iteration-1") + ); + } + + /// With nothing on disk to anchor to, the lexical form is all there is — + /// no worse than the spelling the caller passed in. + #[test] + fn real_path_falls_back_to_the_lexical_form_when_no_ancestor_exists() { + let absent = Path::new(if cfg!(windows) { + r"C:\no-such-root-here\child" + } else { + "/no-such-root-here/child" + }); + assert_eq!( + real_path(absent).unwrap(), + std::path::absolute(absent).unwrap() + ); + } + /// A path already in wire form passes through unchanged on every host — /// the fixtures and goldens that spell paths POSIX-style stay byte-stable. #[test] diff --git a/tests/cli/guard_denials.rs b/tests/cli/guard_denials.rs index 78d6d14..a0fd147 100644 --- a/tests/cli/guard_denials.rs +++ b/tests/cli/guard_denials.rs @@ -1,6 +1,6 @@ //! Guard-denial collection and benchmark validity warnings. -use crate::helpers::{canonical_root, skill_eval}; +use crate::helpers::{canonical_root, resolved, skill_eval}; use assert_cmd::Command; use predicates::str::contains; use std::fs; @@ -79,7 +79,7 @@ fn write_conditions(iteration_dir: &Path, skill_md: &str) { fn setup_guard_denial_iteration(tmp: &TempDir) -> (PathBuf, PathBuf, PathBuf) { use serde_json::json; - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); diff --git a/tests/cli/helpers.rs b/tests/cli/helpers.rs index f3cdbd2..93f0511 100644 --- a/tests/cli/helpers.rs +++ b/tests/cli/helpers.rs @@ -2,7 +2,7 @@ use assert_cmd::Command; use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use tempfile::TempDir; /// Build a `Command` for the built `eval-magic` binary. @@ -14,17 +14,24 @@ pub fn skill_eval() -> Command { cmd } -/// A canonicalized temp root (resolves macOS /var → /private/var so the binary's -/// cwd-derived workspace path matches the fixtures it reads). +/// `fs::canonicalize` with Windows' verbatim (`\\?\`) prefix removed — the +/// spelling the CLI itself resolves paths to, and the one a child process +/// reports as its cwd. Fixtures built on any other spelling of the same +/// directory will not match the paths the CLI emits. +/// +/// Both halves matter, and each is a different host's problem: the resolution +/// covers macOS (/var → /private/var), the stripping covers Windows. +pub fn resolved(path: &Path) -> PathBuf { + let canonical = fs::canonicalize(path).unwrap(); + match canonical.to_string_lossy().strip_prefix(r"\\?\") { + Some(plain) => PathBuf::from(plain), + None => canonical, + } +} + +/// A temp root already in the spelling [`resolved`] describes. pub fn canonical_root() -> (TempDir, PathBuf) { let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); - // `canonicalize` returns a verbatim (`\\?\`) path on Windows, but a child - // process launched with it reports the plain form as its cwd — so paths the - // CLI emits would never match ones a test joins onto this root. - let root = match root.to_string_lossy().strip_prefix(r"\\?\") { - Some(plain) => PathBuf::from(plain), - None => root, - }; + let root = resolved(tmp.path()); (tmp, root) } diff --git a/tests/cli/stray_writes.rs b/tests/cli/stray_writes.rs index e1373b9..5486556 100644 --- a/tests/cli/stray_writes.rs +++ b/tests/cli/stray_writes.rs @@ -1,6 +1,6 @@ //! The `detect-stray-writes` subcommand. -use crate::helpers::skill_eval; +use crate::helpers::{resolved, skill_eval}; use predicates::prelude::PredicateBooleanExt; use predicates::str::contains; use std::fs; @@ -14,7 +14,7 @@ fn detect_stray_writes_reports_live_source_reads() { let tmp = TempDir::new().unwrap(); // realpath: the binary reads its cwd resolved (macOS /var → /private/var), so // fixture paths must match that form for prefix checks to line up. - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); @@ -106,7 +106,7 @@ fn detect_stray_writes_flags_unverifiable_when_nothing_was_inspected() { use serde_json::json; let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); @@ -181,7 +181,7 @@ fn detect_stray_writes_skips_write_classification_without_dispatch_eval_root() { use serde_json::json; let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); @@ -275,7 +275,7 @@ fn detect_stray_writes_uses_eval_root_boundary_from_dispatch() { use serde_json::json; let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap(); @@ -386,7 +386,7 @@ fn detect_stray_writes_scans_nested_run_dirs_and_reports_run_index() { use serde_json::json; let tmp = TempDir::new().unwrap(); - let root = fs::canonicalize(tmp.path()).unwrap(); + let root = resolved(tmp.path()); let skill_dir = root.join("skill-dir"); let skill_sub = skill_dir.join("mr-review"); fs::create_dir_all(&skill_sub).unwrap();