From bc4fc2f35cd9b3f02e315766a90024b0d46881f1 Mon Sep 17 00:00:00 2001 From: Shizuku <2163018547@qq.com> Date: Sat, 10 Oct 2026 00:11:51 +0800 Subject: [PATCH 1/3] fix(subagent): re-root the state-path check when the state root is relocated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A workspace whose `.codewhale` is a junction — the documented way to keep application state on another volume — failed every sub-agent at step 0 with "sub-agent state path must stay within state root: \\?\D:\...\state\subagent-transcripts\....jsonl". default_state_path and the transcript artifact writer both build `.codewhale/state/...` under the state root, so the junction resolves the child into its target — exactly where that user keeps the state — while the containment check still compares against the unresolved root and refuses. effective_state_root re-roots only that known, user-owned link: when `/.codewhale` is a link and the child resolves inside its target, the target becomes the root for both the containment check and the link walk. Any other resolution keeps the stricter root, so an escape past both roots is still refused. Reproduced end to end before the fix: an installed 0.10.1 in a workspace with .codewhale pointing at an outside target failed the child with steps=0 and that exact message. --- crates/tui/src/tools/subagent/mod.rs | 33 ++++++++++++ crates/tui/src/tools/subagent/tests.rs | 74 ++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index f1dd3da9bb..93d80df2f0 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -10453,6 +10453,10 @@ fn checked_subagent_state_path(state_root: &Path, path: &Path) -> Result Result/.codewhale` a junction (or a +/// symlink), which resolves the child outside the state root while landing +/// exactly where that user put the state. When the link exists and the child +/// resolves inside its target, the target is the root the containment check +/// and the link walk must use; otherwise the state root is returned unchanged. +/// +/// A child that resolves anywhere else keeps the stricter state root, so this +/// re-roots one known, user-owned link and does not widen the boundary. +fn effective_state_root(state_root: &Path, state_path: &Path) -> PathBuf { + let linked = state_root.join(codewhale_config::CODEWHALE_APP_DIR); + let Ok(metadata) = fs::symlink_metadata(&linked) else { + return state_root.to_path_buf(); + }; + // Windows junctions are reparse points without the symlink tag, so the + // shared predicate is required here; `FileType::is_symlink` alone would + // miss exactly the case this exists for. + if !crate::plugins::metadata_is_link_or_reparse(&metadata) { + return state_root.to_path_buf(); + } + match resolve_path_for_containment(&linked) { + Ok(resolved) if path_is_within_state_root(state_path, &resolved) => resolved, + _ => state_root.to_path_buf(), + } +} + /// Canonicalize `path` when it exists; otherwise canonicalize the deepest /// existing ancestor and re-append the missing tail. Keeps containment checks /// on the same identity spelling as [`normalize_subagent_workspace`]. diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index b1227a9998..b2d40fa72a 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -11615,6 +11615,80 @@ fn persist_state_rejects_state_path_outside_state_root() { assert!(format!("{err:#}").contains("must stay within state root")); } +/// A user who keeps application state on another volume makes the workspace's +/// `.codewhale` a junction, so `.codewhale/state/...` resolves inside that +/// target. That target is where the state lives; refusing it failed every +/// child at step 0. +#[cfg(windows)] +#[test] +fn state_path_accepts_a_state_root_relocated_behind_a_junction() { + let tmp = tempdir().expect("tempdir"); + let workspace = tmp.path().join("workspace"); + let relocated = tmp.path().join("relocated-state"); + std::fs::create_dir_all(&workspace).expect("mkdir workspace"); + std::fs::create_dir_all(&relocated).expect("mkdir relocated"); + let link = workspace.join(".codewhale"); + let output = Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(&link) + .arg(&relocated) + .output() + .expect("invoke Windows junction creation"); + assert!( + output.status.success(), + "failed to create junction: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let state_path = checked_subagent_state_path( + &workspace, + &std::path::Path::new(".codewhale") + .join("state") + .join(SUBAGENT_STATE_FILE), + ) + .expect("a relocated state root must pass containment"); + + let resolved_target = relocated.canonicalize().expect("canonical target"); + assert!( + state_path.starts_with(&resolved_target), + "state must land in the junction target {}: {}", + resolved_target.display(), + state_path.display() + ); + + // The re-root covers that one user-owned link, not the boundary: a path + // that resolves outside both roots is still refused. + let escaped = std::path::Path::new("..") + .join("escaped") + .join(SUBAGENT_STATE_FILE); + assert!( + checked_subagent_state_path(&workspace, &escaped).is_err(), + "an escape past both roots must still fail" + ); +} + +/// The same workspace without the link keeps its state inside the workspace. +#[test] +fn state_path_stays_in_the_workspace_without_a_link() { + let tmp = tempdir().expect("tempdir"); + let workspace = tmp.path().join("workspace"); + std::fs::create_dir_all(&workspace).expect("mkdir workspace"); + + let state_path = checked_subagent_state_path( + &workspace, + &std::path::Path::new(".codewhale") + .join("state") + .join(SUBAGENT_STATE_FILE), + ) + .expect("plain workspace state path"); + + assert!( + state_path.starts_with(&workspace), + "state stays in the workspace: {}", + state_path.display() + ); +} + #[test] fn explicit_state_roots_isolate_managers_for_the_same_execution_workspace() { let tmp = tempdir().expect("tempdir"); From a7f85e33829232d23ebfb726334f3f35434a134e Mon Sep 17 00:00:00 2001 From: Shizuku <2163018547@qq.com> Date: Sat, 10 Oct 2026 01:06:18 +0800 Subject: [PATCH 2/3] fix(subagent): make every state guard follow the re-rooted decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of this branch found the re-root applied too narrowly: only checked_subagent_state_path used the effective root, while the guards that run after it still compared the resolved path against the unresolved root. The containment check passed and the write still failed with the same "must stay within state root" message on the child's first step — prepare_subagent_transcript_parent, append_private_subagent_transcript, write_json_atomic and read_subagent_state_file all reach it, so the reported failure survived the fix. reject_state_path_symlinks is now the guard those state-path callers use: it resolves the same root decision checked_subagent_state_path made and then runs the unchanged link walk. CoordinationProcessLock keeps rejecting a linked .codewhale on purpose — its comment says create_dir_all must not populate the link target — so the lock guards are left as they are. The Windows test now drives the paths the child actually takes: manager persist_state and load_state, then the transcript writer's create and append, all under a junction whose target is outside the workspace. --- crates/tui/src/tools/subagent/mod.rs | 22 ++++++++--- crates/tui/src/tools/subagent/tests.rs | 54 ++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index 93d80df2f0..0a2d78da10 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -10394,7 +10394,7 @@ pub fn load_subagent_transcript_artifact( fn remove_subagent_transcript_artifact(state_root: &Path, agent_id: &str) -> Result { let state_root = normalize_subagent_workspace(state_root); let path = checked_subagent_transcript_artifact_path(&state_root, agent_id)?; - reject_root_relative_symlinks(&state_root, &path)?; + reject_state_path_symlinks(&state_root, &path)?; let metadata = match fs::symlink_metadata(&path) { Ok(metadata) => metadata, Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(false), @@ -10496,6 +10496,16 @@ fn effective_state_root(state_root: &Path, state_path: &Path) -> PathBuf { } } +/// [`reject_root_relative_symlinks`] for a path that may sit under a state +/// root the user relocated behind a link. Every guard that runs after +/// `checked_subagent_state_path` must reach the same root decision it did, or +/// a relocated root passes that check and fails the write; roots outside this +/// module's state layout come back unchanged from [`effective_state_root`]. +fn reject_state_path_symlinks(state_root: &Path, path: &Path) -> Result<()> { + let root = effective_state_root(state_root, path); + reject_root_relative_symlinks(&root, path) +} + /// Canonicalize `path` when it exists; otherwise canonicalize the deepest /// existing ancestor and re-append the missing tail. Keeps containment checks /// on the same identity spelling as [`normalize_subagent_workspace`]. @@ -10739,7 +10749,7 @@ const MAX_SUBAGENT_STATE_BYTES: u64 = 16 * 1024 * 1024; fn read_subagent_state_file(state_root: &Path, path: &Path) -> Result { let state_root = normalize_subagent_workspace(state_root); - reject_root_relative_symlinks(&state_root, path)?; + reject_state_path_symlinks(&state_root, path)?; let metadata = fs::symlink_metadata(path)?; let file_type = metadata.file_type(); if file_type.is_symlink() || !file_type.is_file() { @@ -10779,14 +10789,14 @@ fn open_subagent_state_file(path: &Path) -> Result { } fn prepare_subagent_transcript_parent(state_root: &Path, path: &Path) -> Result<()> { - reject_root_relative_symlinks(state_root, path)?; + reject_state_path_symlinks(state_root, path)?; let parent = path .parent() .ok_or_else(|| anyhow!("sub-agent transcript artifact must have a parent directory"))?; fs::create_dir_all(parent)?; // Re-check after creation so a pre-existing component cannot redirect the // private transcript outside the state root. - reject_root_relative_symlinks(state_root, path)?; + reject_state_path_symlinks(state_root, path)?; #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; @@ -10809,7 +10819,7 @@ fn append_private_subagent_transcript( bytes: &[u8], durable: bool, ) -> Result<()> { - reject_root_relative_symlinks(state_root, path)?; + reject_state_path_symlinks(state_root, path)?; let mut file = open_private_subagent_transcript(path, true)?; if !bytes.is_empty() { file.write_all(bytes)?; @@ -10880,7 +10890,7 @@ static STATE_PUBLISH_SEQUENCES: std::sync::OnceLock Result<()> { let state_root = normalize_subagent_workspace(state_root); - reject_root_relative_symlinks(&state_root, path)?; + reject_state_path_symlinks(&state_root, path)?; if let Some(parent) = path.parent() { fs::create_dir_all(parent)?; } diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index b2d40fa72a..11e7411053 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -11667,6 +11667,60 @@ fn state_path_accepts_a_state_root_relocated_behind_a_junction() { ); } +/// The guards that run after `checked_subagent_state_path` must reach the same +/// root decision it did. Before they did, the containment check passed and the +/// write still failed with the same message on the child's first step. +#[cfg(windows)] +#[test] +fn state_writes_follow_a_relocated_state_root() { + let tmp = tempdir().expect("tempdir"); + let workspace = tmp.path().join("workspace"); + let relocated = tmp.path().join("relocated-state"); + std::fs::create_dir_all(&workspace).expect("mkdir workspace"); + std::fs::create_dir_all(&relocated).expect("mkdir relocated"); + let link = workspace.join(".codewhale"); + let output = Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(&link) + .arg(&relocated) + .output() + .expect("invoke Windows junction creation"); + assert!( + output.status.success(), + "failed to create junction: {}", + String::from_utf8_lossy(&output.stderr) + ); + + // The manager state file: write_json_atomic and read_subagent_state_file. + let mut manager = SubAgentManager::new(workspace.clone(), 1); + manager + .persist_state() + .expect("persist under a relocated state root"); + manager + .load_state() + .expect("load under a relocated state root"); + + // The child's transcript: the pre-create and append guards. + let state_root = normalize_subagent_workspace(&workspace); + let writer = SubAgentTranscriptArtifactWriter::create(&state_root, "agent_relocated") + .expect("transcript create under a relocated state root"); + append_private_subagent_transcript( + &state_root, + &writer.path, + b"{\"kind\":\"message\"}\n", + true, + ) + .expect("transcript append under a relocated state root"); + + let resolved_target = relocated.canonicalize().expect("canonical target"); + assert!( + writer.path.starts_with(&resolved_target), + "the transcript must land in the junction target {}: {}", + resolved_target.display(), + writer.path.display() + ); +} + /// The same workspace without the link keeps its state inside the workspace. #[test] fn state_path_stays_in_the_workspace_without_a_link() { From 6021aecc8efd1c9d00d50eb745601418d6331cd6 Mon Sep 17 00:00:00 2001 From: Shizuku <2163018547@qq.com> Date: Sat, 10 Oct 2026 01:19:22 +0800 Subject: [PATCH 3/3] fix(subagent): pin the not-yet-created target and drop a test that proved nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings addressed: - "The re-root only covers the branch where the target resolves" — checked against real Rust std rather than the Python stand-in the review used, and it does not hold. `std::fs::canonicalize` returns NotFound for a dangling junction (Python's os.path.realpath resolves it instead), so both the link and the child fall back to the link's own spelling and containment passes. The case is real — mklink /J accepts a target that does not exist, and creating the junction before the first run is a normal setup order — so it is now pinned as a regression case in the relocated-root test. - state_path_stays_in_the_workspace_without_a_link is removed: it took the no-link branch, which is the behaviour before this branch, so it could not fail without the fix, and its assertion depended on the test process's working directory. --- crates/tui/src/tools/subagent/tests.rs | 52 +++++++++++++++----------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 11e7411053..dc2013f851 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -11665,6 +11665,36 @@ fn state_path_accepts_a_state_root_relocated_behind_a_junction() { checked_subagent_state_path(&workspace, &escaped).is_err(), "an escape past both roots must still fail" ); + + // `mklink /J` accepts a target that does not exist yet, and creating the + // junction before the first run is a normal setup order. Both sides then + // resolve to the link's own spelling, so containment holds until the + // target exists. + let dangling_workspace = tmp.path().join("dangling"); + let dangling_target = tmp.path().join("not-created-yet"); + std::fs::create_dir_all(&dangling_workspace).expect("mkdir dangling workspace"); + let output = Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(dangling_workspace.join(".codewhale")) + .arg(&dangling_target) + .output() + .expect("invoke Windows junction creation"); + assert!( + output.status.success(), + "failed to create dangling junction: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert!( + !dangling_target.exists(), + "the probe needs a target that does not exist yet" + ); + checked_subagent_state_path( + &dangling_workspace, + &std::path::Path::new(".codewhale") + .join("state") + .join(SUBAGENT_STATE_FILE), + ) + .expect("a junction whose target is not created yet must pass containment"); } /// The guards that run after `checked_subagent_state_path` must reach the same @@ -11721,28 +11751,6 @@ fn state_writes_follow_a_relocated_state_root() { ); } -/// The same workspace without the link keeps its state inside the workspace. -#[test] -fn state_path_stays_in_the_workspace_without_a_link() { - let tmp = tempdir().expect("tempdir"); - let workspace = tmp.path().join("workspace"); - std::fs::create_dir_all(&workspace).expect("mkdir workspace"); - - let state_path = checked_subagent_state_path( - &workspace, - &std::path::Path::new(".codewhale") - .join("state") - .join(SUBAGENT_STATE_FILE), - ) - .expect("plain workspace state path"); - - assert!( - state_path.starts_with(&workspace), - "state stays in the workspace: {}", - state_path.display() - ); -} - #[test] fn explicit_state_roots_isolate_managers_for_the_same_execution_workspace() { let tmp = tempdir().expect("tempdir");