mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-08 01:48:20 -10:00
fix(workspace): keep non-home link confinement, exempt only home state
Review follow-up on the previous commit. The exemption was right; the way it was applied was not. `reject_linked_path` never examines the root itself, so lowering the root from the workspace to `<workspace>/.codewhale` made a repository-supplied `.codewhale` link transparent: fleet profiles were read with their trust and permission fields, the workflow journal wrote, and the subagent lock created directories through the link, all outside the repository. Every one of those three call sites now keeps the workspace as the root with `.codewhale` still inside the walked path. workspace_state_path decides that root once: a home workspace resolves its own `~/.codewhale` junction and uses the target, every other workspace keeps itself and inspects `.codewhale` exactly as before. A state directory that exists but cannot be resolved falls back to the strict root, so an unreachable junction fails closed instead of starting a second state tree. The skills exemption is now per directory rather than per workspace: only the home state directory skips confinement, so links under any other home directory are still followed the old way and still warn. Also reuses `codewhale_paths::CODEWHALE_APP_DIR` instead of a third literal. Verified on Windows with junctions on both sides: `reject_linked_path` still refuses `<repo>/.codewhale` for a non-home workspace and passes `<home>/.codewhale` for a home workspace, where the resolved root is the real target. - cargo check -p codewhale-util -p codewhale-skills: passed - cargo check -p codewhale-engine: passed - cargo test -p codewhale-util: 41 passed; 0 failed - cargo test -p codewhale-skills: 174 passed; 0 failed - cargo test -p codewhale-engine --lib -- fleet::profile: 33 passed; 0 failed - cargo test -p codewhale-engine --lib -- tools::workflow::journal: 8 passed; 0 failed (includes journal_never_writes_through_a_linked_codewhale_dir) - cargo test -p codewhale-engine --lib -- subagent::tests::: 590 passed; 6 failed; the same six fail on the parent commit with this change stashed. Four are real-home skill leakage into a tempdir workspace, and agent_claim_expands_the_callers_write_scope overflows the default Windows test stack before RUST_MIN_STACK is raised. - cargo fmt --all -- --check: passed
This commit is contained in:
1 parent
5897663976
commit
0ce030840b
5 files changed
+87
-31
No files matched your search
@@ -218,10 +218,10 @@ pub fn load_workspace_agent_profiles(workspace: impl AsRef<Path>) -> Result<Vec<
|
||||
///
|
||||
/// A home workspace keeps its `.codewhale` state wherever the user put it, so
|
||||
/// the resolved state directory is the root; links below it are still refused.
|
||||
/// Any other workspace keeps the workspace as the root with `.codewhale`
|
||||
/// inside the walked path, so a repository-supplied link is still refused.
|
||||
fn workspace_agent_profile_dir(workspace: &Path) -> (PathBuf, PathBuf) {
|
||||
let root = codewhale_util::files::workspace_state_dir(workspace);
|
||||
let dir = root.join("agents");
|
||||
(root, dir)
|
||||
codewhale_util::files::workspace_state_path(workspace, Path::new("agents"))
|
||||
}
|
||||
|
||||
/// Read one profile file. Under a workspace `root` a linked file or linked
|
||||
|
||||
@@ -3961,17 +3961,24 @@ pub const COORDINATION_LOCK_TIMEOUT_MARKER: &str =
|
||||
impl CoordinationProcessLock {
|
||||
fn acquire(state_root: &Path, exclusive: bool) -> Result<Self> {
|
||||
let requested_root = normalize_subagent_workspace(state_root);
|
||||
// A home workspace may keep `.codewhale` on another volume behind a
|
||||
// junction the user made; `workspace_state_dir` resolves that one root
|
||||
// and the walk below still refuses links beneath it.
|
||||
let state_dir = codewhale_util::files::workspace_state_dir(&requested_root);
|
||||
let lock_dir = state_dir.join("state");
|
||||
// Refuse a linked `state` before creating anything: `create_dir_all`
|
||||
// follows links and would populate the target.
|
||||
reject_root_relative_symlinks(&state_dir, &lock_dir.join(SUBAGENT_STATE_LOCK_FILE))?;
|
||||
// Root and path for the lock. A non-home workspace keeps itself as the
|
||||
// root, so a repository-supplied `.codewhale` link is walked and
|
||||
// refused before `create_dir_all` could follow it into the target. A
|
||||
// home workspace resolves its own junction once and still walks
|
||||
// everything below it.
|
||||
let (walk_root, lock_path) = codewhale_util::files::workspace_state_path(
|
||||
&requested_root,
|
||||
&Path::new("state").join(SUBAGENT_STATE_LOCK_FILE),
|
||||
);
|
||||
let lock_dir = lock_path
|
||||
.parent()
|
||||
.map_or_else(|| walk_root.clone(), Path::to_path_buf);
|
||||
// Refuse a linked `.codewhale` or `state` before creating anything:
|
||||
// `create_dir_all` follows links and would populate the target.
|
||||
reject_root_relative_symlinks(&walk_root, &lock_path)?;
|
||||
fs::create_dir_all(&lock_dir)?;
|
||||
let ignore = lock_dir.join(".gitignore");
|
||||
let ignored = reject_root_relative_symlinks(&state_dir, &ignore).and_then(|()| {
|
||||
let ignored = reject_root_relative_symlinks(&walk_root, &ignore).and_then(|()| {
|
||||
match fs::OpenOptions::new()
|
||||
.write(true)
|
||||
.create_new(true)
|
||||
@@ -3989,9 +3996,11 @@ impl CoordinationProcessLock {
|
||||
// Windows (for example by adding a `\\?\` prefix). Re-resolve both
|
||||
// sides before checking containment.
|
||||
let state_root = normalize_subagent_workspace(state_root);
|
||||
let state_dir = codewhale_util::files::workspace_state_dir(&state_root);
|
||||
let lock_path = state_dir.join("state").join(SUBAGENT_STATE_LOCK_FILE);
|
||||
reject_root_relative_symlinks(&state_dir, &lock_path)?;
|
||||
let (walk_root, lock_path) = codewhale_util::files::workspace_state_path(
|
||||
&state_root,
|
||||
&Path::new("state").join(SUBAGENT_STATE_LOCK_FILE),
|
||||
);
|
||||
reject_root_relative_symlinks(&walk_root, &lock_path)?;
|
||||
let lock_file = std::fs::OpenOptions::new()
|
||||
.create(true)
|
||||
.truncate(false)
|
||||
|
||||
@@ -178,8 +178,7 @@ enum WorkflowJournalRecord {
|
||||
#[derive(Debug)]
|
||||
struct WorkflowRunJournal {
|
||||
/// State directory the ledger must stay inside; links below it are refused.
|
||||
/// A home workspace resolves `~/.codewhale` to its target here, because that
|
||||
/// junction is where the user chose to keep application state.
|
||||
/// See [`WorkflowRunJournal::open`] for which root a workspace gets.
|
||||
root: PathBuf,
|
||||
ledger_path: PathBuf,
|
||||
}
|
||||
@@ -188,12 +187,15 @@ impl WorkflowRunJournal {
|
||||
/// Opening has no side effects: `.codewhale/` and the ledger appear with
|
||||
/// the first appended record, and only through the confined helpers, so a
|
||||
/// linked `.codewhale` never receives an empty `state/` or ledger.
|
||||
///
|
||||
/// A home workspace resolves `~/.codewhale` to its target, because that
|
||||
/// junction is where the user chose to keep application state. Any other
|
||||
/// workspace keeps itself as the root with `.codewhale` inside the walked
|
||||
/// path, so a repository-supplied link is refused as before.
|
||||
fn open(workspace: &Path) -> Self {
|
||||
let root = codewhale_util::files::workspace_state_dir(workspace);
|
||||
Self {
|
||||
ledger_path: root.join(WORKFLOW_RUNS_FILE),
|
||||
root,
|
||||
}
|
||||
let (root, ledger_path) =
|
||||
codewhale_util::files::workspace_state_path(workspace, Path::new(WORKFLOW_RUNS_FILE));
|
||||
Self { root, ledger_path }
|
||||
}
|
||||
|
||||
fn hydrate_runs(&self, recover_orphans: bool) -> HashMap<String, WorkflowRunRecord> {
|
||||
|
||||
@@ -1246,11 +1246,20 @@ fn merge_watched_directories(
|
||||
let mut merged = SkillRegistry::default();
|
||||
let mut watched = WatchedPaths::default();
|
||||
let mut seen_files = HashSet::new();
|
||||
let confined_workspace =
|
||||
workspace.filter(|workspace| !codewhale_util::files::workspace_is_user_home(workspace));
|
||||
// A home workspace is not a repository, but only its own Codewhale state
|
||||
// directory is exempt: the user may keep `~/.codewhale` behind a junction,
|
||||
// while every other directory under home stays workspace content and goes
|
||||
// on refusing links that escape it.
|
||||
let home_state = workspace
|
||||
.filter(|workspace| codewhale_util::files::workspace_is_user_home(workspace))
|
||||
.map(codewhale_util::files::workspace_state_dir);
|
||||
for dir in dirs {
|
||||
watched.push((dir.clone(), watched_path_stamp(&dir)));
|
||||
let confine_to = confined_workspace.filter(|workspace| dir.starts_with(workspace));
|
||||
let owned_state_dir = home_state.as_ref().is_some_and(|state| {
|
||||
fs::canonicalize(&dir).is_ok_and(|resolved| resolved.starts_with(state))
|
||||
});
|
||||
let confine_to =
|
||||
workspace.filter(|workspace| dir.starts_with(workspace) && !owned_state_dir);
|
||||
let (registry, dir_watched) = SkillRegistry::discover_watched_confined(&dir, confine_to);
|
||||
watched.extend(dir_watched);
|
||||
for mut skill in registry.skills {
|
||||
|
||||
@@ -19,7 +19,6 @@ pub fn path_is_confined(path: &Path) -> bool {
|
||||
/// example a junction onto another volume). Workspace confinement exists to
|
||||
/// keep repository content from reaching outside itself, so a home workspace is
|
||||
/// exempt from it.
|
||||
#[must_use]
|
||||
pub fn workspace_is_user_home(workspace: &Path) -> bool {
|
||||
codewhale_paths::user_home().is_some_and(|home| same_directory(&home, workspace))
|
||||
}
|
||||
@@ -37,13 +36,14 @@ fn same_directory(left: &Path, right: &Path) -> bool {
|
||||
/// The `.codewhale` state directory that owns `workspace`'s application state.
|
||||
///
|
||||
/// A home workspace is resolved: `~/.codewhale` may be a junction the user made
|
||||
/// to keep state on another volume, and a confinement root must be the real
|
||||
/// directory or every path below it is refused. Any other workspace returns its
|
||||
/// literal `.codewhale`, which the callers' link walk still refuses when it is
|
||||
/// linked — repository content must not reach outside the repository.
|
||||
/// to keep state on another volume. Any other workspace returns its literal
|
||||
/// `.codewhale`, which is still repository content. When a home state directory
|
||||
/// exists but cannot be resolved, the literal path comes back and the caller
|
||||
/// treats it as workspace content again — that fails closed instead of creating
|
||||
/// a second state tree beside the one that cannot be reached.
|
||||
#[must_use]
|
||||
pub fn workspace_state_dir(workspace: &Path) -> std::path::PathBuf {
|
||||
let state = workspace.join(".codewhale");
|
||||
let state = workspace.join(codewhale_paths::CODEWHALE_APP_DIR);
|
||||
if workspace_is_user_home(workspace) {
|
||||
state.canonicalize().unwrap_or(state)
|
||||
} else {
|
||||
@@ -51,6 +51,42 @@ pub fn workspace_state_dir(workspace: &Path) -> std::path::PathBuf {
|
||||
}
|
||||
}
|
||||
|
||||
/// Confinement root and state path for `relative` beneath the workspace's
|
||||
/// Codewhale state directory.
|
||||
///
|
||||
/// The root is the boundary a link walk starts from, and
|
||||
/// [`reject_linked_path`] never examines the root itself — only what is below
|
||||
/// it. So the root decides whether `.codewhale` is inspected:
|
||||
///
|
||||
/// - A home workspace resolves `~/.codewhale` once and uses its target as the
|
||||
/// root: the user's own junction is the place they chose for state, and a
|
||||
/// path below it is still walked.
|
||||
/// - Every other workspace keeps itself as the root with `.codewhale` still
|
||||
/// inside the path, so a repository-supplied link is refused exactly as it
|
||||
/// was before this helper existed.
|
||||
#[must_use]
|
||||
pub fn workspace_state_path(
|
||||
workspace: &Path,
|
||||
relative: &Path,
|
||||
) -> (std::path::PathBuf, std::path::PathBuf) {
|
||||
let state = workspace.join(codewhale_paths::CODEWHALE_APP_DIR);
|
||||
let within_workspace = || (workspace.to_path_buf(), state.join(relative));
|
||||
if !workspace_is_user_home(workspace) {
|
||||
return within_workspace();
|
||||
}
|
||||
match state.canonicalize() {
|
||||
Ok(resolved) => {
|
||||
let path = resolved.join(relative);
|
||||
(resolved, path)
|
||||
}
|
||||
// A missing directory is the first run, created inside the home
|
||||
// workspace itself. A directory that exists but cannot be resolved fails
|
||||
// closed against the strict root rather than writing a second state
|
||||
// tree beside the one that cannot be reached.
|
||||
Err(_) => within_workspace(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Refuse `path` when any existing component below `root` is a link. The
|
||||
/// walk stops at the first component that does not exist yet, so a path that
|
||||
/// is about to be created through a real directory chain passes. This is a
|
||||
|
||||
Reference in new issue
Block a user