From 5296ce6cd23f0368853471d9fe6767ded8d1a7c5 Mon Sep 17 00:00:00 2001 From: LargeModGames <84450916+LargeModGames@users.noreply.github.com> Date: Sat, 22 Aug 2026 19:55:27 +0200 Subject: [PATCH 1/4] fix(dj): scratch dir fallback for unwritable home The agent CLI ran from /spotatui/dj-scratch. When that directory cannot be created (a build sandbox sets HOME to a missing path), spawn fails with an ENOENT that the message blamed on the binary. agent_scratch_dir() now resolves once per process: the config scratch dir when it can be created and read, otherwise /dj-scratch, never the shared temp root. The spawn error names the cwd and the OS error. Fixes #478 --- src/infra/dj/brain/agent_cli.rs | 10 ++++- src/infra/dj/session.rs | 77 +++++++++++++++++++++++++++++++-- 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/src/infra/dj/brain/agent_cli.rs b/src/infra/dj/brain/agent_cli.rs index ce3ef051..642a7888 100644 --- a/src/infra/dj/brain/agent_cli.rs +++ b/src/infra/dj/brain/agent_cli.rs @@ -230,8 +230,14 @@ impl AgentCliBrain { } } - let mut child = command.spawn().with_context(|| { - format!("could not run `{program}`. Is it installed and on PATH? (behavior.dj_agent_command)") + // The OS error and the cwd go in the message: `spawn` fails with the same + // ENOENT for a missing binary and for a cwd it cannot enter (#478), and the + // transcript shows only the top of the error chain. + let mut child = command.spawn().map_err(|e| { + anyhow!( + "could not run `{program}` from `{}` ({e}). Is it installed and on PATH? (behavior.dj_agent_command)", + self.cwd.display() + ) })?; let stdin = match self.prompt_via { diff --git a/src/infra/dj/session.rs b/src/infra/dj/session.rs index e5b56020..a19655b3 100644 --- a/src/infra/dj/session.rs +++ b/src/infra/dj/session.rs @@ -22,6 +22,7 @@ use crate::core::app::App; use crate::core::user_config::BehaviorConfig; use crate::infra::history::RecapPeriod; use anyhow::{anyhow, Result}; +use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::Duration; use tokio::sync::Mutex; @@ -38,10 +39,57 @@ pub const API_KEY_ENV: &str = "SPOTATUI_DJ_API_KEY"; /// **Not** the user's current directory. Coding agents read `CLAUDE.md` / /// `AGENTS.md` and project files from their working directory, so an agent /// launched inside a repository answers with that repository on its mind. -fn agent_scratch_dir() -> std::path::PathBuf { - crate::core::user_config::default_app_config_dir() - .unwrap_or_else(std::env::temp_dir) - .join("dj-scratch") +/// +/// Resolved once per process, so every step of every turn runs from the same +/// directory and the fallback is reported once. The config dir is preferred; a +/// private subdirectory of the OS temp dir is the fallback (#478: a build +/// sandbox sets `HOME` to a path that cannot be created, and `spawn` with a +/// missing cwd fails with an ENOENT that looks like a missing binary). +fn agent_scratch_dir() -> PathBuf { + static DIR: std::sync::OnceLock = std::sync::OnceLock::new(); + DIR + .get_or_init(|| { + scratch_dir_from( + crate::core::user_config::default_app_config_dir().map(|dir| dir.join("dj-scratch")), + &std::env::temp_dir(), + ) + }) + .clone() +} + +/// [`agent_scratch_dir`] with the candidates passed in rather than read. +/// +/// Candidates, in order: the config scratch dir, then `/dj-scratch`. A +/// relative temp dir is skipped, as `paths.rs` skips a relative XDG value: it +/// would resolve against the process cwd, the one place an agent must not run. +/// A candidate counts only when it can be created *and* read, so a leftover +/// directory the process cannot enter is skipped too. When nothing qualifies, +/// the first candidate is returned and `spawn` reports the cwd it could not +/// enter. +fn scratch_dir_from(preferred: Option, temp: &Path) -> PathBuf { + let fallback = temp.is_absolute().then(|| temp.join("dj-scratch")); + let candidates: Vec = preferred.into_iter().chain(fallback).collect(); + for (index, dir) in candidates.iter().enumerate() { + if usable_dir(dir) { + if index > 0 { + log::warn!( + "DJ: could not use the scratch dir {}; running agent CLIs from {}", + candidates[0].display(), + dir.display() + ); + } + return dir.clone(); + } + } + log::warn!("DJ: no usable scratch dir among {candidates:?}; agent CLIs will not start"); + candidates + .into_iter() + .next() + .unwrap_or_else(|| temp.join("dj-scratch")) +} + +fn usable_dir(dir: &Path) -> bool { + std::fs::create_dir_all(dir).is_ok() && std::fs::read_dir(dir).is_ok() } /// The API-key precedence rule: the env var wins, the config field is the @@ -610,4 +658,25 @@ mod tests { assert_ne!(scratch, cwd); assert!(scratch.ends_with("dj-scratch")); } + + #[test] + fn the_config_scratch_dir_is_used_when_it_can_be_created() { + let temp = tempfile::tempdir().unwrap(); + let preferred = temp.path().join("config").join("dj-scratch"); + let chosen = scratch_dir_from(Some(preferred.clone()), temp.path()); + assert_eq!(chosen, preferred); + assert!(chosen.is_dir(), "it is created, not only named"); + } + + #[test] + fn an_uncreatable_config_dir_falls_back_to_a_private_temp_subdir() { + // #478: a build sandbox sets HOME to a path that cannot be created. The + // agent must still get a directory of its own, never the shared temp root. + let temp = tempfile::tempdir().unwrap(); + let blocker = temp.path().join("not-a-dir"); + std::fs::write(&blocker, b"").unwrap(); + let chosen = scratch_dir_from(Some(blocker.join("dj-scratch")), temp.path()); + assert_eq!(chosen, temp.path().join("dj-scratch")); + assert!(chosen.is_dir()); + } } From 259a37f29e57569b8c97fe557a1eb8f5bacfb2be Mon Sep 17 00:00:00 2001 From: LargeModGames <84450916+LargeModGames@users.noreply.github.com> Date: Sat, 22 Aug 2026 20:07:11 +0200 Subject: [PATCH 2/4] fix(dj): fresh private temp fallback and a cwd probe Review fixes. The temp fallback is a uniquely named directory made with create_dir, never a fixed name another local user can plant. A scratch dir counts only when a file can be created in it, which needs the search and write permissions chdir and the agent need. The missing-binary test pins the cwd and the OS error in the message. test_attribute_total moves to 1518 for the three new tests. --- src/infra/dj/brain/agent_cli.rs | 7 ++ src/infra/dj/session.rs | 111 +++++++++++++++++++++++--------- tools/gates.count | 2 +- 3 files changed, 90 insertions(+), 30 deletions(-) diff --git a/src/infra/dj/brain/agent_cli.rs b/src/infra/dj/brain/agent_cli.rs index 642a7888..b63097e6 100644 --- a/src/infra/dj/brain/agent_cli.rs +++ b/src/infra/dj/brain/agent_cli.rs @@ -592,6 +592,13 @@ esac"#, .unwrap_err() .to_string(); assert!(err.contains("Is it installed"), "{err}"); + // The cwd and the OS error are part of the diagnostic (#478): a cwd that + // cannot be entered fails `spawn` with the same ENOENT as a missing binary. + assert!(err.contains(&scratch().display().to_string()), "{err}"); + assert!( + err.contains("No such file") || err.contains("not found"), + "{err}" + ); } #[test] diff --git a/src/infra/dj/session.rs b/src/infra/dj/session.rs index a19655b3..7aa3fbab 100644 --- a/src/infra/dj/session.rs +++ b/src/infra/dj/session.rs @@ -57,39 +57,70 @@ fn agent_scratch_dir() -> PathBuf { .clone() } -/// [`agent_scratch_dir`] with the candidates passed in rather than read. +/// [`agent_scratch_dir`] with the inputs passed in rather than read. /// -/// Candidates, in order: the config scratch dir, then `/dj-scratch`. A -/// relative temp dir is skipped, as `paths.rs` skips a relative XDG value: it -/// would resolve against the process cwd, the one place an agent must not run. -/// A candidate counts only when it can be created *and* read, so a leftover -/// directory the process cannot enter is skipped too. When nothing qualifies, -/// the first candidate is returned and `spawn` reports the cwd it could not -/// enter. +/// The config scratch dir is used when it can be created, entered, and written. +/// Otherwise a fresh, uniquely named directory is made under `temp`. Never a +/// fixed name there: a shared temp root lets another local user plant that name +/// (or a symlink to a directory of theirs) and so choose the agent's working +/// directory, instruction files included. A relative temp dir is skipped, as +/// `paths.rs` skips a relative XDG value: it would resolve against the process +/// cwd, the one place an agent must not run. When nothing qualifies, the +/// preferred path is returned and `spawn` reports the cwd it could not enter. fn scratch_dir_from(preferred: Option, temp: &Path) -> PathBuf { - let fallback = temp.is_absolute().then(|| temp.join("dj-scratch")); - let candidates: Vec = preferred.into_iter().chain(fallback).collect(); - for (index, dir) in candidates.iter().enumerate() { - if usable_dir(dir) { - if index > 0 { - log::warn!( - "DJ: could not use the scratch dir {}; running agent CLIs from {}", - candidates[0].display(), - dir.display() - ); - } - return dir.clone(); + if let Some(dir) = preferred.as_ref().filter(|dir| usable_dir(dir)) { + return dir.clone(); + } + match temp.is_absolute().then(|| fresh_temp_dir(temp)).flatten() { + Some(dir) => { + log::warn!( + "DJ: could not use the scratch dir {preferred:?}; running agent CLIs from {}", + dir.display() + ); + dir + } + None => { + log::warn!( + "DJ: no usable scratch dir (config {preferred:?}, temp {}); agent CLIs will not start", + temp.display() + ); + preferred.unwrap_or_else(|| temp.join("dj-scratch")) } } - log::warn!("DJ: no usable scratch dir among {candidates:?}; agent CLIs will not start"); - candidates - .into_iter() - .next() - .unwrap_or_else(|| temp.join("dj-scratch")) } +/// Whether `Command::current_dir` can enter `dir` and the agent can write in it. +/// +/// `create_dir_all` alone is not enough: it succeeds on a directory that already +/// exists without search permission, and `spawn` then fails on the `chdir` with +/// the same misleading error as a missing cwd. Creating and removing a file +/// inside needs search and write on the directory, which is what the agent +/// needs too. fn usable_dir(dir: &Path) -> bool { - std::fs::create_dir_all(dir).is_ok() && std::fs::read_dir(dir).is_ok() + if std::fs::create_dir_all(dir).is_err() { + return false; + } + let probe = dir.join(format!(".probe-{}", std::process::id())); + let written = std::fs::write(&probe, b"").is_ok(); + let _ = std::fs::remove_file(&probe); + written +} + +/// A new, uniquely named directory under `temp`, or `None` when none could be +/// made. `create_dir` (not `create_dir_all`) refuses an existing entry, a +/// symlink included, so the result is always one this process made. It is not +/// removed on exit: temp is the one location the platform clears on its own, +/// and the per-process log file already relies on that. +fn fresh_temp_dir(temp: &Path) -> Option { + let pid = std::process::id(); + (0..8).find_map(|attempt| { + let nanos = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|since| since.subsec_nanos()) + .unwrap_or(0); + let dir = temp.join(format!("spotatui-dj-scratch-{pid}-{nanos}-{attempt}")); + std::fs::create_dir(&dir).ok().map(|()| dir) + }) } /// The API-key precedence rule: the env var wins, the config field is the @@ -669,14 +700,36 @@ mod tests { } #[test] - fn an_uncreatable_config_dir_falls_back_to_a_private_temp_subdir() { + fn an_uncreatable_config_dir_falls_back_to_a_fresh_private_temp_dir() { // #478: a build sandbox sets HOME to a path that cannot be created. The // agent must still get a directory of its own, never the shared temp root. let temp = tempfile::tempdir().unwrap(); let blocker = temp.path().join("not-a-dir"); std::fs::write(&blocker, b"").unwrap(); let chosen = scratch_dir_from(Some(blocker.join("dj-scratch")), temp.path()); - assert_eq!(chosen, temp.path().join("dj-scratch")); - assert!(chosen.is_dir()); + assert!(chosen.is_dir(), "{}", chosen.display()); + assert_eq!(chosen.parent(), Some(temp.path()), "{}", chosen.display()); + // Fresh every time: never a fixed name another local user can plant. + let again = scratch_dir_from(Some(blocker.join("dj-scratch")), temp.path()); + assert_ne!(again, chosen); + } + + #[cfg(unix)] + #[test] + fn a_config_dir_without_search_permission_is_not_used() { + use std::os::unix::fs::PermissionsExt; + let temp = tempfile::tempdir().unwrap(); + let preferred = temp.path().join("dj-scratch"); + std::fs::create_dir(&preferred).unwrap(); + // Read and write, no search: `create_dir_all` is content, `chdir` is not. + std::fs::set_permissions(&preferred, std::fs::Permissions::from_mode(0o600)).unwrap(); + if std::fs::write(preferred.join("probe"), b"").is_ok() { + // A privileged user ignores mode bits; there is nothing to test here. + return; + } + let chosen = scratch_dir_from(Some(preferred.clone()), temp.path()); + std::fs::set_permissions(&preferred, std::fs::Permissions::from_mode(0o700)).unwrap(); + assert_ne!(chosen, preferred); + assert!(chosen.is_dir(), "{}", chosen.display()); } } diff --git a/tools/gates.count b/tools/gates.count index 2529cd13..458c69a2 100644 --- a/tools/gates.count +++ b/tools/gates.count @@ -13,4 +13,4 @@ synthetic_keys_in_mouse_handler = 15 # target 0 (13 production + 2 test sites) wildcard_arms_in_action_tree = 0 # target 0, must stay 0 view_writes_outside_tui = 12 # target 0 (producers outside tui/ and core/app/ writing App::view) action_refs_in_tui_handlers = 54 # adoption: may only rise -test_attribute_total = 1515 # adoption: may only rise +test_attribute_total = 1518 # adoption: may only rise From c390577a5878ad82701aff04d06e344c6e7bfc50 Mon Sep 17 00:00:00 2001 From: LargeModGames <84450916+LargeModGames@users.noreply.github.com> Date: Sat, 22 Aug 2026 20:20:27 +0200 Subject: [PATCH 3/4] fix(dj): owner-only temp fallback, portable error assert The fresh temp fallback is created with mode 0700 on Unix, whatever the umask, and the fallback test checks that group and others get nothing. The missing-binary test pins the message structure on every platform and the OS error wording only on Unix. --- src/infra/dj/brain/agent_cli.rs | 7 +++++-- src/infra/dj/session.rs | 21 ++++++++++++++++++--- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/infra/dj/brain/agent_cli.rs b/src/infra/dj/brain/agent_cli.rs index b63097e6..cd595039 100644 --- a/src/infra/dj/brain/agent_cli.rs +++ b/src/infra/dj/brain/agent_cli.rs @@ -594,11 +594,14 @@ esac"#, assert!(err.contains("Is it installed"), "{err}"); // The cwd and the OS error are part of the diagnostic (#478): a cwd that // cannot be entered fails `spawn` with the same ENOENT as a missing binary. - assert!(err.contains(&scratch().display().to_string()), "{err}"); + // The OS error sits in the parentheses after the cwd; its wording is the + // platform's, so only the structure is pinned everywhere. assert!( - err.contains("No such file") || err.contains("not found"), + err.contains(&format!("from `{}` (", scratch().display())), "{err}" ); + #[cfg(unix)] + assert!(err.contains("No such file or directory"), "{err}"); } #[test] diff --git a/src/infra/dj/session.rs b/src/infra/dj/session.rs index 7aa3fbab..b123e962 100644 --- a/src/infra/dj/session.rs +++ b/src/infra/dj/session.rs @@ -106,12 +106,20 @@ fn usable_dir(dir: &Path) -> bool { written } -/// A new, uniquely named directory under `temp`, or `None` when none could be -/// made. `create_dir` (not `create_dir_all`) refuses an existing entry, a +/// A new, uniquely named, owner-only directory under `temp`, or `None` when +/// none could be made. A non-recursive create refuses an existing entry, a /// symlink included, so the result is always one this process made. It is not /// removed on exit: temp is the one location the platform clears on its own, /// and the per-process log file already relies on that. fn fresh_temp_dir(temp: &Path) -> Option { + let mut builder = std::fs::DirBuilder::new(); + #[cfg(unix)] + { + // Owner-only from the start, whatever the umask: the agent's working + // directory must not be readable or writable by other local users. + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + } let pid = std::process::id(); (0..8).find_map(|attempt| { let nanos = std::time::SystemTime::now() @@ -119,7 +127,7 @@ fn fresh_temp_dir(temp: &Path) -> Option { .map(|since| since.subsec_nanos()) .unwrap_or(0); let dir = temp.join(format!("spotatui-dj-scratch-{pid}-{nanos}-{attempt}")); - std::fs::create_dir(&dir).ok().map(|()| dir) + builder.create(&dir).ok().map(|()| dir) }) } @@ -712,6 +720,13 @@ mod tests { // Fresh every time: never a fixed name another local user can plant. let again = scratch_dir_from(Some(blocker.join("dj-scratch")), temp.path()); assert_ne!(again, chosen); + // And private: group and others get nothing, whatever the umask. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = std::fs::metadata(&chosen).unwrap().permissions().mode(); + assert_eq!(mode & 0o077, 0, "mode {mode:o}"); + } } #[cfg(unix)] From 710c074ef2fea30408a0b497b637f46c92aabb08 Mon Sep 17 00:00:00 2001 From: LargeModGames <84450916+LargeModGames@users.noreply.github.com> Date: Sat, 22 Aug 2026 20:21:04 +0200 Subject: [PATCH 4/4] fix(dj): build the temp dir builder without an unused mut --- src/infra/dj/session.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/infra/dj/session.rs b/src/infra/dj/session.rs index b123e962..8af800d1 100644 --- a/src/infra/dj/session.rs +++ b/src/infra/dj/session.rs @@ -112,14 +112,17 @@ fn usable_dir(dir: &Path) -> bool { /// removed on exit: temp is the one location the platform clears on its own, /// and the per-process log file already relies on that. fn fresh_temp_dir(temp: &Path) -> Option { - let mut builder = std::fs::DirBuilder::new(); + // Owner-only from the start, whatever the umask: the agent's working + // directory must not be readable or writable by other local users. #[cfg(unix)] - { - // Owner-only from the start, whatever the umask: the agent's working - // directory must not be readable or writable by other local users. + let builder = { use std::os::unix::fs::DirBuilderExt; + let mut builder = std::fs::DirBuilder::new(); builder.mode(0o700); - } + builder + }; + #[cfg(not(unix))] + let builder = std::fs::DirBuilder::new(); let pid = std::process::id(); (0..8).find_map(|attempt| { let nanos = std::time::SystemTime::now()