From c8277f43e6b8b5ab836447ae7eb8d2ed683a7545 Mon Sep 17 00:00:00 2001 From: Victor Garcia Date: Sat, 8 Aug 2026 02:02:52 -0700 Subject: [PATCH 1/2] `bullpen run --bg --worktree`: give a background session its own tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two background runs dispatched from the same directory edited the same files. spawn_detached sets no current_dir, so a detached child inherits the dispatcher's cwd; nothing in the store recorded where a run had been placed, and run -r derived its directory from std::env::current_dir() rather than from the session, so resuming from elsewhere silently ran somewhere else. With --worktree the session gets a git worktree at $BULLPEN_HOME/worktrees/ on a run-unique `bullpen/` branch, and both the path and the branch are persisted (schema v6). Resume goes back there without the flag. Concurrent dispatches no longer collide. Without the flag nothing about --bg changes: same cwd inheritance, same store writes, same output. The issue named four questions and left them open. Answers, as implemented: - Cleanup is fail-closed by omission. No code path here removes a worktree or deletes a branch — not on completion, failure, resume, or a timer. Nothing in this change can prove an agent's work was published, and losing the only copy of that work is far worse than leaving a directory behind. Reclaiming one stays a deliberate `git worktree remove`. ARCHITECTURE.md states the posture so a later prune command has to earn its proof-of-publish rule rather than inherit an optimistic one. - run -r on a session whose worktree is gone recreates it at the recorded path from the recorded branch, and says so on stderr. If the branch is gone, the repo is gone, or git disagrees, it fails naming both path and branch. It never falls back to the caller's cwd — that is the bug being closed. The decision is a pure `decide` over two booleans, so all four cases are tested without git. - --worktree outside a git repository is a hard error, as are a missing git binary and a failing `git worktree add`. Degrading to the shared tree would silently reintroduce exactly what the flag exists to prevent. The dispatch aborts before a session row is written, so nothing is left half-created pointing at a directory that does not exist. - The location surfaces in all three views: the `sessions` table, `sessions --json` as additive `worktree_path` / `worktree_branch` keys (null when absent — the wire contract in crates/cli/src/json.rs is a compatibility surface), and the `agents` peek panel. Dispatch also prints the path once on stderr, since otherwise the dispatching user needs a second command to learn where their run went. Notes: - The sandbox had to learn about linked worktrees. A linked worktree's admin dir and the shared object store both live outside the worktree, so under the generated Seatbelt profile an agent could edit files but never commit — `git add` died on index.lock with EPERM. Sandbox::allowing_writes takes the extra roots and worktree::git_write_roots derives them, keeping the sandbox crate ignorant of git. Under the retention rule this was load-bearing, not cosmetic: work that cannot be committed can never become the evidence that would justify reclaiming the directory. - Worktrees live under BULLPEN_HOME rather than beside the repo, so an isolated run never dirties the tree it was dispatched from. - session.cwd keeps recording the dispatch directory; the worktree goes in new columns because recreating one needs both the origin repo and the branch. - --worktree without --bg, and --worktree with --resume, are both rejected rather than ignored. A session's location is fixed at creation. Verified: 125 tests (up from 108), fmt and clippy clean. Beyond the suite, the built binary was exercised against a temp BULLPEN_HOME and a temp repo — both guards bail, dispatch outside a repo leaves no session row and no process, two concurrent dispatches land on distinct branches, `lsof -d cwd` confirms the detached child's real directory is its worktree, resume from / uses the recorded worktree and recreates it from the branch when deleted, and plain --bg produces byte-identical output with NULL columns and no worktrees/ directory. The sandbox test is mutation-checked: removing the allowing_writes call reproduces the index.lock denial. Refs #13 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3 --- ARCHITECTURE.md | 45 ++++- Cargo.lock | 1 + README.md | 16 +- crates/cli/Cargo.toml | 3 + crates/cli/src/agents.rs | 81 +++++--- crates/cli/src/main.rs | 137 +++++++++++-- crates/cli/src/worktree.rs | 390 +++++++++++++++++++++++++++++++++++++ crates/sandbox/src/lib.rs | 10 + crates/store/src/lib.rs | 105 +++++++++- 9 files changed, 743 insertions(+), 45 deletions(-) create mode 100644 crates/cli/src/worktree.rs diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index a4ccd84..5c77c5b 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -105,8 +105,12 @@ never stops the loop. ## Persistence and durable execution One database: `~/.bullpen/bullpen.db`, WAL mode, `busy_timeout` set, schema -versioned by `pragma user_version`. Session ids resolve by unique prefix. -`BULLPEN_HOME` overrides the directory (see README, "Where state lives"). +versioned by `pragma user_version` (v6). Session ids resolve by unique +prefix. `BULLPEN_HOME` overrides the directory (see README, "Where state +lives"). v6 added `sessions.worktree_path` / `worktree_branch`, both NULL +for a session that shares the caller's checkout; a session's `cwd` stays the +directory it was dispatched from, which is what still points at the +repository when the worktree itself is gone. The durability rule, the reduction idea, and the recovery discipline below are adapted from pi's `harness-v2.md` design spec — see @@ -265,6 +269,10 @@ Milestones, in order. Each lands as its own crate or a bounded extension: derived state (Working = running + live pid, Failed = running + dead pid i.e. crashed, Completed, Idle), dispatches from an input line, and peeks a session's latest output. `bullpen logs ` tails captured output. + `--bg --worktree` (opt-in) additionally gives a session its own git + worktree on a run-unique branch, so concurrent background sessions stop + editing each other's files; the location is recorded on the session row + and wins over the caller's cwd on resume (see "Worktree retention" below). Stage 2 (committed): interactive attach to and detach from a *live* process, leaving it running (needs a per-session control socket), needs-input state (needs the approvals feature), notifications. Stage 2+ @@ -336,3 +344,36 @@ that is still the default. Linux has no out-of-process confinement yet (Landlock is the intended mechanism; the in-process write check already works there). Do not point v0 at anything you wouldn't hand to a contractor's laptop. + +### Worktree retention (fail-closed) + +`--worktree` isolates a background session in its own git worktree. Nothing +in bullpen removes that worktree or its branch — not when the run completes, +not when it fails, not on any later invocation. The asymmetry is the whole +argument: a leftover directory costs disk, while an eager cleanup can +destroy the only copy of what an agent did — an uncommitted diff, an +interrupted rebase, a file it wrote but never mentioned. Uncertainty +retains. Even the resume path that restores a deleted worktree uses +`git worktree add --force` rather than pruning the stale entry, because +pruning is a removal. + +That constrains any future `bullpen prune`: it has to earn deletion from +proof that the work was published — the branch is merged or pushed, the +worktree is clean — rather than inherit an optimistic rule like age or +session status. A `completed` session is not evidence its diff was kept. + +The resume rule follows the same posture. The recorded location beats the +directory the command was typed in; a missing directory whose branch +survives is recreated from that branch with one stderr notice; a missing +directory *and* missing branch is an error naming both, never a silent run +somewhere else. `--worktree` outside a git repository is likewise an error, +because falling back to the shared checkout would silently reintroduce +exactly the interference the flag exists to remove. + +`--sandbox` has to be widened to compose with this. A linked worktree's +`.git` is a file; its index, refs and objects all live under the *main* +repository, outside the worktree, so a sandbox confined to the worktree +alone leaves an agent able to edit files and unable to stage or commit +them — which under the rule above is fatal, since a commit is the only +evidence that would ever justify reclaiming its directory. The run therefore +adds the worktree's git dirs to the write roots. diff --git a/Cargo.lock b/Cargo.lock index 97d8719..28624a1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -151,6 +151,7 @@ dependencies = [ "ratatui", "reqwest", "serde_json", + "tempfile", "tokio", "tracing-subscriber", ] diff --git a/README.md b/README.md index fba7287..e477485 100644 --- a/README.md +++ b/README.md @@ -83,6 +83,7 @@ bullpen run -r 6ee4acc9 "now write the fix" ```bash bullpen run --bg "audit the auth module" # detached, returns immediately +bullpen run --bg --worktree "refactor it" # …in its own git worktree bullpen agents # the dashboard in the GIF above bullpen logs 6ee4acc9 # tail a background session ``` @@ -92,6 +93,16 @@ bullpen logs 6ee4acc9 # tail a background session lets you dispatch from the input line, `Space` to peek at output, `Esc` to quit. Quitting stops nothing. +Plain `--bg` sessions share your checkout, so two of them edit the same +files. `--worktree` gives a session a git worktree of its own on a +run-unique `bullpen/` branch, under `$BULLPEN_HOME/worktrees/`; +the path shows up in `bullpen sessions`, in `sessions --json`, and in the +peek panel, and `bullpen run -r ` returns to it from anywhere. Outside a +git repository the flag fails rather than quietly sharing the checkout. +**Nothing removes a worktree or its branch** — not on success, not on +failure, not later. A worktree can hold the only copy of what an agent did, +so cleaning up is yours to decide. + ## The pen The model can delegate bounded work to child agents through the `agent` @@ -130,8 +141,9 @@ can't be opened read-only without their shared-memory file: sqlite3 "file:${BULLPEN_HOME:-$HOME/.bullpen}/bullpen.db?immutable=1" "select id, status from sessions" ``` -Set `BULLPEN_HOME` to move the whole directory — database, `auth.json`, and -background logs land directly in it, with no `.bullpen` segment appended. +Set `BULLPEN_HOME` to move the whole directory — database, `auth.json`, +background logs, and `--worktree` checkouts (`worktrees/`) land +directly in it, with no `.bullpen` segment appended. ## Status diff --git a/crates/cli/Cargo.toml b/crates/cli/Cargo.toml index 2d73399..8b02cec 100644 --- a/crates/cli/Cargo.toml +++ b/crates/cli/Cargo.toml @@ -25,3 +25,6 @@ crossterm.workspace = true libc.workspace = true tokio.workspace = true tracing-subscriber.workspace = true + +[dev-dependencies] +tempfile = "3" diff --git a/crates/cli/src/agents.rs b/crates/cli/src/agents.rs index 87dc196..1f79749 100644 --- a/crates/cli/src/agents.rs +++ b/crates/cli/src/agents.rs @@ -302,34 +302,19 @@ fn draw_peek(f: &mut Frame, app: &App) { let area = centered(70, 60, f.area()); f.render_widget(Clear, area); - let mut lines = vec![ - Line::from(Span::styled( - format!("{} ({})", &session.id[..8], session.provider), - Style::default().add_modifier(Modifier::BOLD), - )), - Line::from(""), - ]; // Latest assistant text from the durable transcript. - match Store::open(&Store::default_path()).and_then(|s| s.path_messages(&session.id)) { - Ok(messages) => { - let latest = messages + let latest = + match Store::open(&Store::default_path()).and_then(|s| s.path_messages(&session.id)) { + Ok(messages) => messages .iter() .rev() .find(|m| m.role == bullpen_llm::Role::Assistant) .map(|m| m.text()) .filter(|t| !t.is_empty()) - .unwrap_or_else(|| "(no output yet)".into()); - for line in latest.lines() { - lines.push(Line::from(line.to_string())); - } - } - Err(e) => lines.push(Line::from(format!("(could not read transcript: {e})"))), - } - lines.push(Line::from("")); - lines.push(Line::from(Span::styled( - format!("continue with: bullpen run -r {} \"…\"", &session.id[..8]), - Style::default().fg(Color::DarkGray), - ))); + .unwrap_or_else(|| "(no output yet)".into()), + Err(e) => format!("(could not read transcript: {e})"), + }; + let lines = peek_lines(session, &latest); let block = Block::default() .borders(Borders::ALL) @@ -342,6 +327,34 @@ fn draw_peek(f: &mut Frame, app: &App) { ); } +/// The peek panel's contents. Pure — the transcript read happens in the +/// caller — so what the panel says about a session can be tested without a +/// terminal or a store. +fn peek_lines(session: &Session, latest: &str) -> Vec> { + let mut lines = vec![Line::from(Span::styled( + format!("{} ({})", &session.id[..8], session.provider), + Style::default().add_modifier(Modifier::BOLD), + ))]; + // An isolated session's output is not in the directory the dashboard was + // started from, so the panel has to say where it is. + if let Some(path) = &session.worktree_path { + lines.push(Line::from(Span::styled( + format!("worktree {path}"), + Style::default().fg(Color::DarkGray), + ))); + } + lines.push(Line::from("")); + for line in latest.lines() { + lines.push(Line::from(line.to_string())); + } + lines.push(Line::from("")); + lines.push(Line::from(Span::styled( + format!("continue with: bullpen run -r {} \"…\"", &session.id[..8]), + Style::default().fg(Color::DarkGray), + ))); + lines +} + fn status_color(status: AgentStatus) -> Color { match status { AgentStatus::Working => Color::Cyan, @@ -389,11 +402,20 @@ mod tests { parent_session_id: None, status: "idle".into(), pid: None, + worktree_path: None, + worktree_branch: None, }, status, } } + fn rendered(lines: &[Line<'static>]) -> Vec { + lines + .iter() + .map(|l| l.spans.iter().map(|s| s.content.as_ref()).collect()) + .collect() + } + #[test] fn arrange_groups_working_first_then_newest() { let rows = vec![ @@ -414,4 +436,19 @@ mod tests { ] ); } + + #[test] + fn peek_lines_shows_the_worktree_for_an_isolated_session() { + let mut r = row("0123456789ab", AgentStatus::Working, "2026-08-07 10:00"); + r.session.worktree_path = Some("/h/.bullpen/worktrees/0123456789ab".into()); + let lines = rendered(&peek_lines(&r.session, "output")); + assert_eq!(lines[1], "worktree /h/.bullpen/worktrees/0123456789ab"); + } + + #[test] + fn peek_lines_says_nothing_about_worktrees_for_a_shared_cwd_session() { + let r = row("0123456789ab", AgentStatus::Working, "2026-08-07 10:00"); + let lines = rendered(&peek_lines(&r.session, "output")); + assert!(!lines.iter().any(|l| l.contains("worktree")), "{lines:?}"); + } } diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 3b1e97f..2fd0d71 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -7,6 +7,7 @@ mod agents; mod bg; mod json; +mod worktree; use std::sync::Arc; @@ -66,6 +67,12 @@ enum Command { /// Watch it with `bullpen agents`. #[arg(long)] bg: bool, + /// Give the background session its own git worktree on its own + /// branch, so concurrent runs cannot edit each other's files. + /// Requires --bg; not compatible with --resume. The worktree is + /// never removed automatically. + #[arg(long)] + worktree: bool, }, /// Dispatch and monitor background sessions from one screen. Agents, @@ -183,6 +190,7 @@ async fn main() -> anyhow::Result<()> { sandbox, sandbox_strict, bg, + worktree, } => { run( prompt, @@ -194,6 +202,7 @@ async fn main() -> anyhow::Result<()> { sandbox, sandbox_strict, bg, + worktree, ) .await } @@ -339,12 +348,31 @@ async fn run( sandbox: bool, sandbox_strict: bool, bg: bool, + worktree: bool, ) -> anyhow::Result<()> { + if worktree && !bg { + bail!("--worktree applies to background dispatch only; add --bg"); + } + if worktree && resume.is_some() { + bail!( + "--worktree creates a worktree for a new session; a resumed one \ + already has its location recorded" + ); + } let cwd = std::env::current_dir()?; // Background dispatch: create/resolve the session here so we have an id to // hand back and to spawn a detached child against, then return. if bg { + // Resolve the repository before any state exists. A directory that + // is not a repository must leave behind no session row, no log file + // and no detached process — not a session pointing at a worktree + // that was never created. + let repo_root = if worktree { + Some(worktree::repo_root(&cwd)?) + } else { + None + }; let store = Store::open(&Store::default_path())?; let session = match &resume { Some(prefix) => store.resolve_session(prefix)?, @@ -359,6 +387,16 @@ async fn run( } else if sandbox { extra.push("--sandbox".to_string()); } + let worktree_at = match &repo_root { + Some(root) => { + let path = worktree::worktree_path(&session.id); + let branch = worktree::branch_for(&session.id); + worktree::create(root, &path, &branch)?; + store.set_worktree(&session.id, &path.display().to_string(), &branch)?; + Some(path) + } + None => None, + }; let pid = bg::spawn_detached(&session.id, &prompt, &extra)?; store.set_run_status(&session.id, "running", Some(pid as i64))?; if json { @@ -366,6 +404,12 @@ async fn run( } else { println!("dispatched {} (pid {pid})", &session.id[..8]); } + if let Some(path) = &worktree_at { + eprintln!( + "[worktree {} — never removed automatically]", + path.display() + ); + } eprintln!( " bullpen agents watch it\n bullpen logs {} tail its output\n bullpen run -r {} \"...\" continue it", &session.id[..8], @@ -374,20 +418,10 @@ async fn run( return Ok(()); } - // Build the write-confinement sandbox, if requested. - let sandbox = if sandbox_strict { - Some(Arc::new(bullpen_sandbox::Sandbox::strict(&cwd))) - } else if sandbox { - Some(Arc::new(bullpen_sandbox::Sandbox::workspace(&cwd))) - } else { - None - }; - if sandbox.is_some() && !bullpen_sandbox::Sandbox::os_enforced() { - eprintln!( - "[sandbox: OS-level shell confinement is macOS-only; on this platform \ - only the file-editing tools are confined, not arbitrary shell commands]" - ); - } + // The session is resolved before the sandbox is built: an isolated + // session's working directory is recorded on its row, and it is that + // directory the sandbox must confine — not the one the caller happened + // to type the command in. let mut store = Store::open(&Store::default_path())?; let (session, provider_kind, model) = match &resume { @@ -406,6 +440,48 @@ async fn run( } }; + // A recorded worktree wins over the caller's cwd. This is the only + // mechanism that places a run: the detached background child is a + // `run --resume` and arrives here too, as does a resume typed by hand + // from anywhere. + let cwd = match worktree::locate( + session.worktree_path.as_deref(), + session.worktree_branch.as_deref(), + std::path::Path::new(&session.cwd), + ) { + worktree::Location::Shared => cwd, + worktree::Location::Use(path) => path, + worktree::Location::Recreate { path, branch } => { + let root = worktree::repo_root(std::path::Path::new(&session.cwd))?; + worktree::recreate(&root, &path, &branch)?; + eprintln!("[recreated worktree {} from {branch}]", path.display()); + path + } + worktree::Location::Fail { path, branch } => bail!( + "session {} ran in worktree {} on branch {branch}, and neither \ + survives — nothing was removed by bullpen, so restore them (or \ + start a new session) rather than running somewhere else", + &session.id[..8], + path.display() + ), + }; + + // Build the write-confinement sandbox, if requested. + let sandbox = (sandbox || sandbox_strict).then(|| { + let base = if sandbox_strict { + bullpen_sandbox::Sandbox::strict(&cwd) + } else { + bullpen_sandbox::Sandbox::workspace(&cwd) + }; + Arc::new(base.allowing_writes(worktree::git_write_roots(&cwd))) + }); + if sandbox.is_some() && !bullpen_sandbox::Sandbox::os_enforced() { + eprintln!( + "[sandbox: OS-level shell confinement is macOS-only; on this platform \ + only the file-editing tools are confined, not arbitrary shell commands]" + ); + } + // Recover any run a previous process left open, then rebuild the // transcript from the durable entry tree. let (transcript, recovery) = bullpen_harness::prepare_session(&mut store, &session.id)?; @@ -598,6 +674,8 @@ fn sessions_json(sessions: &[Session]) -> serde_json::Value { "parent_session_id": s.parent_session_id, "status": s.status, "pid": s.pid, + "worktree_path": s.worktree_path, + "worktree_branch": s.worktree_branch, }) }) .collect() @@ -622,8 +700,12 @@ fn sessions(json: bool) -> anyhow::Result<()> { Some(parent) => format!(" └ child of {}", &parent[..8]), None => String::new(), }; + let worktree_marker = match &s.worktree_path { + Some(path) => format!(" [worktree {path}]"), + None => String::new(), + }; println!( - "{} {} {:<10} {:>6}/{:<6} {}{}", + "{} {} {:<10} {:>6}/{:<6} {}{}{}", &s.id[..8], s.updated_at, s.provider, @@ -635,6 +717,7 @@ fn sessions(json: bool) -> anyhow::Result<()> { &s.title }, child_marker, + worktree_marker, ); } Ok(()) @@ -671,6 +754,8 @@ mod tests { parent_session_id: parent.map(|p| p.to_string()), status: "idle".into(), pid, + worktree_path: None, + worktree_branch: None, } } @@ -696,10 +781,32 @@ mod tests { "parent_session_id": null, "status": "idle", "pid": 4242, + "worktree_path": null, + "worktree_branch": null, }]) ); } + #[test] + fn worktree_keys_are_null_for_shared_cwd_sessions() { + let rows = sessions_json(&[session("shared", None, None)]); + assert_eq!(rows[0]["worktree_path"], json!(null)); + assert_eq!(rows[0]["worktree_branch"], json!(null)); + } + + #[test] + fn worktree_keys_carry_the_location_for_an_isolated_session() { + let mut s = session("isolated", None, None); + s.worktree_path = Some("/h/.bullpen/worktrees/isolated".into()); + s.worktree_branch = Some("bullpen/isolated".into()); + let rows = sessions_json(&[s]); + assert_eq!( + rows[0]["worktree_path"], + json!("/h/.bullpen/worktrees/isolated") + ); + assert_eq!(rows[0]["worktree_branch"], json!("bullpen/isolated")); + } + #[test] fn parent_session_id_is_null_for_top_level_and_a_string_for_children() { let rows = sessions_json(&[ diff --git a/crates/cli/src/worktree.rs b/crates/cli/src/worktree.rs new file mode 100644 index 0000000..fc0618f --- /dev/null +++ b/crates/cli/src/worktree.rs @@ -0,0 +1,390 @@ +//! Per-session git worktrees for `--bg --worktree`. +//! +//! Two background sessions sharing one checkout edit each other's files. +//! Giving each its own worktree on its own branch removes that entirely, at +//! the cost of a directory that has to be found again later — which is why +//! the path and branch are recorded in the store rather than recomputed. +//! +//! **This module creates worktrees and never removes one.** No exit path +//! here deletes a directory or a branch, because a worktree may hold the +//! only copy of an agent's work: an uncommitted diff, a half-finished +//! rebase, a file the agent wrote but never mentioned. Uncertainty retains. +//! A leftover directory costs disk; an eager cleanup costs the work itself. +//! +//! Split like [`bullpen_store::home_dir`]: pure derivation (path, branch +//! name, and the resume decision table) separated from a thin `git` adapter, +//! so the decisions are testable without a repository. git is shelled out to +//! rather than linked, matching how `bullpen-tools` runs bash and grep. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use anyhow::bail; + +/// Where a background session's isolated checkout lives. Under +/// `$BULLPEN_HOME` rather than inside the repository: the worktree is +/// bullpen's state, keyed to a session id, and moving the database without +/// it would split the two apart. +pub fn worktree_path(session_id: &str) -> PathBuf { + worktree_dir(&bullpen_store::home_dir(), session_id) +} + +fn worktree_dir(home: &Path, session_id: &str) -> PathBuf { + home.join("worktrees").join(session_id) +} + +/// The branch a session's worktree checks out. Run-unique because session +/// ids are, so two concurrent dispatches can never collide on it. +pub fn branch_for(session_id: &str) -> String { + format!("bullpen/{}", &session_id[..8]) +} + +/// The repository containing `cwd`. +/// +/// Fails when there isn't one. `--worktree` never degrades to the shared +/// working directory: silently doing that is exactly the behaviour the flag +/// exists to avoid. +pub fn repo_root(cwd: &Path) -> anyhow::Result { + let out = Command::new("git") + .arg("-C") + .arg(cwd) + .args(["rev-parse", "--show-toplevel"]) + .output(); + let out = match out { + Ok(out) => out, + Err(e) => bail!("--worktree needs the git binary, but running it failed: {e}"), + }; + if !out.status.success() { + bail!( + "{} is not inside a git repository, so --worktree has nothing to \ + branch from — it will not fall back to running in the shared \ + working directory", + cwd.display() + ); + } + Ok(PathBuf::from( + String::from_utf8_lossy(&out.stdout).trim().to_string(), + )) +} + +/// Check out a new worktree at `path` on a new `branch` off the current HEAD. +pub fn create(root: &Path, path: &Path, branch: &str) -> anyhow::Result<()> { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + git_worktree_add(root, &["-b", branch], path, "HEAD") +} + +/// Restore a recorded worktree whose directory is gone but whose branch +/// survives. `--force` is required, not optional: git's administrative entry +/// for the deleted directory still claims both the path and the branch. +/// Pruning that entry would be a removal path, which this module does not have. +pub fn recreate(root: &Path, path: &Path, branch: &str) -> anyhow::Result<()> { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent)?; + } + git_worktree_add(root, &["--force"], path, branch) +} + +fn git_worktree_add( + root: &Path, + flags: &[&str], + path: &Path, + commit_ish: &str, +) -> anyhow::Result<()> { + let out = Command::new("git") + .arg("-C") + .arg(root) + .args(["worktree", "add"]) + .args(flags) + .arg(path) + .arg(commit_ish) + .output(); + let out = match out { + Ok(out) => out, + Err(e) => bail!("could not run git worktree add: {e}"), + }; + if !out.status.success() { + bail!( + "git worktree add failed for {}:\n{}", + path.display(), + String::from_utf8_lossy(&out.stderr).trim() + ); + } + Ok(()) +} + +/// The git directories a run in `cwd` must be able to write to, beyond `cwd` +/// itself. Empty for an ordinary checkout, whose `.git` is already inside the +/// workspace; for a linked worktree the index, refs and objects all live under +/// the main repository, so a sandbox confined to the worktree alone would let +/// an agent edit files but never stage or commit them — and a commit is the +/// only evidence that would ever justify reclaiming its directory. +pub fn git_write_roots(cwd: &Path) -> Vec { + let (Some(git_dir), Some(common_dir)) = ( + rev_parse_dir(cwd, "--absolute-git-dir"), + rev_parse_dir(cwd, "--git-common-dir"), + ) else { + return Vec::new(); + }; + if git_dir == common_dir { + return Vec::new(); + } + vec![git_dir, common_dir] +} + +/// `git rev-parse `, resolved to an absolute path. `--git-common-dir` +/// answers relative to `cwd` in an ordinary checkout and absolutely in a +/// linked worktree, so both forms have to be accepted before the two can be +/// compared. +fn rev_parse_dir(cwd: &Path, flag: &str) -> Option { + let out = Command::new("git") + .arg("-C") + .arg(cwd) + .args(["rev-parse", flag]) + .output() + .ok()?; + if !out.status.success() { + return None; + } + let dir = PathBuf::from(String::from_utf8_lossy(&out.stdout).trim().to_string()); + let dir = if dir.is_absolute() { + dir + } else { + cwd.join(dir) + }; + Some(dir.canonicalize().unwrap_or(dir)) +} + +pub fn branch_exists(root: &Path, branch: &str) -> bool { + Command::new("git") + .arg("-C") + .arg(root) + .args(["rev-parse", "--verify", &format!("refs/heads/{branch}")]) + .output() + .is_ok_and(|out| out.status.success()) +} + +/// Where a run should happen, given what the session recorded. +#[derive(Debug, PartialEq, Eq)] +pub enum Location { + /// No worktree was ever recorded: the caller's working directory. + Shared, + Use(PathBuf), + Recreate { + path: PathBuf, + branch: String, + }, + Fail { + path: PathBuf, + branch: String, + }, +} + +/// The resume decision, as a table. git and the filesystem supply only the +/// two booleans, so every row is testable without either. +/// +/// A recorded worktree always wins over the caller's working directory: a +/// `bullpen run -r ` typed from somewhere else must not quietly append +/// to the session in a different tree than the one it has been editing. +pub fn decide(recorded: Option<(&str, &str)>, dir_exists: bool, branch_exists: bool) -> Location { + let Some((path, branch)) = recorded else { + return Location::Shared; + }; + let path = PathBuf::from(path); + match (dir_exists, branch_exists) { + (true, _) => Location::Use(path), + (false, true) => Location::Recreate { + path, + branch: branch.to_string(), + }, + (false, false) => Location::Fail { + path, + branch: branch.to_string(), + }, + } +} + +/// The impure half of [`decide`]: answers its two booleans. `anchor` is the +/// session's recorded cwd — the directory it was dispatched from, which is +/// what still points at the repository once the worktree itself is gone. +pub fn locate( + recorded_path: Option<&str>, + recorded_branch: Option<&str>, + anchor: &Path, +) -> Location { + let Some((path, branch)) = recorded_path.zip(recorded_branch) else { + return Location::Shared; + }; + let dir_exists = Path::new(path).is_dir(); + let branch_lives = + !dir_exists && repo_root(anchor).is_ok_and(|root| branch_exists(&root, branch)); + decide(Some((path, branch)), dir_exists, branch_lives) +} + +#[cfg(test)] +mod tests { + use super::*; + + const ID: &str = "0123456789abcdef0123456789abcdef"; + + /// A repository at `root` with one committed file. + fn init_repo(root: &Path) { + std::fs::create_dir_all(root).unwrap(); + let git = |args: &[&str]| { + let out = Command::new("git") + .arg("-C") + .arg(root) + // CI runners have no global identity, and the commit below + // needs one. + .args(["-c", "user.email=t@example.com", "-c", "user.name=t"]) + .args(args) + .output() + .unwrap(); + assert!(out.status.success(), "git {args:?}: {out:?}"); + }; + git(&["init"]); + std::fs::write(root.join("f.txt"), "x").unwrap(); + git(&["add", "f.txt"]); + git(&["commit", "-m", "init"]); + } + + #[test] + fn branch_name_is_derived_from_the_session_id_prefix() { + assert_eq!(branch_for(ID), "bullpen/01234567"); + } + + #[test] + fn worktree_dir_lives_under_bullpen_home_not_the_repo() { + assert_eq!( + worktree_dir(Path::new("/h/.bullpen"), ID), + PathBuf::from("/h/.bullpen/worktrees/0123456789abcdef0123456789abcdef") + ); + } + + #[test] + fn decide_falls_back_to_the_shared_cwd_only_when_nothing_was_recorded() { + assert_eq!(decide(None, false, false), Location::Shared); + } + + #[test] + fn decide_uses_the_recorded_directory_when_it_still_exists() { + assert_eq!( + decide(Some(("/w/a", "bullpen/a")), true, false), + Location::Use(PathBuf::from("/w/a")) + ); + } + + #[test] + fn decide_recreates_from_the_branch_when_only_the_directory_is_gone() { + assert_eq!( + decide(Some(("/w/a", "bullpen/a")), false, true), + Location::Recreate { + path: PathBuf::from("/w/a"), + branch: "bullpen/a".into(), + } + ); + } + + #[test] + fn missing_branch_fails_rather_than_falling_back_to_the_callers_cwd() { + assert_eq!( + decide(Some(("/w/a", "bullpen/a")), false, false), + Location::Fail { + path: PathBuf::from("/w/a"), + branch: "bullpen/a".into(), + } + ); + } + + #[test] + fn non_repo_error_names_the_directory_and_refuses_to_degrade() { + let dir = tempfile::tempdir().unwrap(); + let err = repo_root(dir.path()).unwrap_err().to_string(); + assert!(err.contains(&dir.path().display().to_string()), "{err}"); + assert!(err.contains("not fall back"), "{err}"); + } + + /// The tests below really shell out: the `--force` recreate path and the + /// layout of a linked worktree's git dirs are claims about git's + /// behaviour, not about ours, and asserting them against a stub would + /// prove nothing. + #[test] + fn create_then_recreate_restores_a_deleted_worktree_on_its_branch() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("repo"); + init_repo(&root); + + let path = dir.path().join("worktrees").join("s1"); + create(&root, &path, "bullpen/s1").unwrap(); + assert!(path.join("f.txt").is_file()); + assert!(branch_exists(&root, "bullpen/s1")); + // macOS resolves TempDir under /private/var, so compare canonical forms. + assert_eq!( + repo_root(&path).unwrap().canonicalize().unwrap(), + path.canonicalize().unwrap() + ); + + std::fs::remove_dir_all(&path).unwrap(); + recreate(&root, &path, "bullpen/s1").unwrap(); + assert!(path.join("f.txt").is_file()); + } + + #[test] + fn an_ordinary_checkout_needs_no_write_roots_beyond_itself() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("repo"); + init_repo(&root); + assert!(git_write_roots(&root).is_empty()); + // Nor does a directory that is not a repository at all. + assert!(git_write_roots(dir.path()).is_empty()); + } + + #[test] + fn a_linked_worktree_needs_the_main_repositorys_git_dirs() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("repo"); + init_repo(&root); + let path = dir.path().join("worktrees").join("s1"); + create(&root, &path, "bullpen/s1").unwrap(); + + let common = root.join(".git").canonicalize().unwrap(); + assert_eq!( + git_write_roots(&path), + vec![common.join("worktrees").join("s1"), common] + ); + } + + /// Write roots alone do not prove an agent can publish its work; only + /// running git under the real generated profile does. macOS-only because + /// that is the platform where the profile is enforced. + #[test] + #[cfg(target_os = "macos")] + fn a_sandboxed_worktree_can_still_commit() { + // Under $HOME rather than a tempdir: the sandbox deliberately allows + // writes anywhere under the system temp roots, so a tempdir repo + // would pass whatever the write roots said. + let base = std::env::home_dir() + .unwrap() + .join(".bullpen-worktree-sandbox-test"); + let _ = std::fs::remove_dir_all(&base); + let root = base.join("repo"); + init_repo(&root); + let path = base.join("worktrees").join("s1"); + create(&root, &path, "bullpen/s1").unwrap(); + + let sandbox = + bullpen_sandbox::Sandbox::workspace(&path).allowing_writes(git_write_roots(&path)); + let (prog, args) = sandbox + .wrap_bash("git -c user.email=t@example.com -c user.name=t commit -qam sandboxed"); + std::fs::write(path.join("f.txt"), "edited").unwrap(); + let out = Command::new(prog) + .args(args) + .current_dir(&path) + .output() + .unwrap(); + assert!(out.status.success(), "{out:?}"); + + std::fs::remove_dir_all(&base).unwrap(); + } +} diff --git a/crates/sandbox/src/lib.rs b/crates/sandbox/src/lib.rs index 32ffdf6..7930d41 100644 --- a/crates/sandbox/src/lib.rs +++ b/crates/sandbox/src/lib.rs @@ -59,6 +59,16 @@ impl Sandbox { Self { caps } } + /// Widen the write confinement to further subtrees. Some workspaces have + /// machinery outside themselves that the agent legitimately writes + /// through — a linked git worktree's index and object store being the + /// case that forced this; the caller names those, since the sandbox has + /// no business knowing what a worktree is. + pub fn allowing_writes(mut self, roots: impl IntoIterator) -> Self { + self.caps.write_roots.extend(roots); + self + } + pub fn capabilities(&self) -> &Capabilities { &self.caps } diff --git a/crates/store/src/lib.rs b/crates/store/src/lib.rs index 60bd844..c81365a 100644 --- a/crates/store/src/lib.rs +++ b/crates/store/src/lib.rs @@ -1,6 +1,6 @@ //! Durable local state. //! -//! One SQLite database in WAL mode. Schema v3 implements the durable +//! One SQLite database in WAL mode. Schema v6 implements the durable //! execution model from ARCHITECTURE.md ("Persistence and durable //! execution"): //! @@ -62,6 +62,12 @@ pub struct Session { pub status: String, /// OS process id of the last run, for liveness checks. pub pid: Option, + /// Set when the session runs in its own git worktree; the directory the + /// run happens in, which outlives the process that created it. + pub worktree_path: Option, + /// Set alongside `worktree_path`; the branch that worktree checks out. + /// It is what a lost directory can be restored from. + pub worktree_branch: Option, } /// One conversation entry. `payload` holds a [`Message`] for kind @@ -227,6 +233,15 @@ impl Store { )?; tx.commit()?; } + if version < 6 { + let tx = self.conn.transaction()?; + tx.execute_batch( + "ALTER TABLE sessions ADD COLUMN worktree_path TEXT; + ALTER TABLE sessions ADD COLUMN worktree_branch TEXT; + PRAGMA user_version = 6;", + )?; + tx.commit()?; + } Ok(()) } @@ -321,11 +336,28 @@ impl Store { Ok(()) } + /// Record where an isolated session runs. Written after the worktree + /// exists, never as part of `create_session`: a row that names a + /// directory git failed to create is worse than a row that names none. + pub fn set_worktree( + &self, + session_id: &str, + path: &str, + branch: &str, + ) -> Result<(), StoreError> { + self.conn.execute( + "UPDATE sessions SET worktree_path = ?2, worktree_branch = ?3 WHERE id = ?1", + params![session_id, path, branch], + )?; + Ok(()) + } + pub fn get_session(&self, id: &str) -> Result { self.conn .query_row( "SELECT id, title, cwd, provider, model, input_tokens, output_tokens, - created_at, updated_at, parent_session_id, status, pid + created_at, updated_at, parent_session_id, status, pid, + worktree_path, worktree_branch FROM sessions WHERE id = ?1", params![id], row_to_session, @@ -339,7 +371,8 @@ impl Store { pub fn resolve_session(&self, prefix: &str) -> Result { let mut stmt = self.conn.prepare( "SELECT id, title, cwd, provider, model, input_tokens, output_tokens, - created_at, updated_at, parent_session_id, status, pid + created_at, updated_at, parent_session_id, status, pid, + worktree_path, worktree_branch FROM sessions WHERE id LIKE ?1 || '%' LIMIT 2", )?; let mut matches: Vec = stmt @@ -355,7 +388,8 @@ impl Store { pub fn list_sessions(&self) -> Result, StoreError> { let mut stmt = self.conn.prepare( "SELECT id, title, cwd, provider, model, input_tokens, output_tokens, - created_at, updated_at, parent_session_id, status, pid + created_at, updated_at, parent_session_id, status, pid, + worktree_path, worktree_branch FROM sessions ORDER BY updated_at DESC", )?; Ok(stmt @@ -712,6 +746,8 @@ fn row_to_session(row: &rusqlite::Row<'_>) -> rusqlite::Result { parent_session_id: row.get(9)?, status: row.get(10)?, pid: row.get(11)?, + worktree_path: row.get(12)?, + worktree_branch: row.get(13)?, }) } @@ -914,4 +950,65 @@ mod tests { assert_eq!(messages[1].role, Role::Assistant); assert!(store.open_run("s1").unwrap().is_none()); } + + #[test] + fn migrates_v5_sessions_adding_worktree_columns() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("v5.db"); + // Build a v5 database by hand. Only `sessions` matters here: the v6 + // migration touches nothing else. + { + let conn = Connection::open(&path).unwrap(); + conn.execute_batch( + "CREATE TABLE sessions ( + id TEXT PRIMARY KEY, title TEXT NOT NULL DEFAULT '', + cwd TEXT NOT NULL, model TEXT NOT NULL, + input_tokens INTEGER NOT NULL DEFAULT 0, + output_tokens INTEGER NOT NULL DEFAULT 0, + created_at TEXT NOT NULL DEFAULT (datetime('now')), + updated_at TEXT NOT NULL DEFAULT (datetime('now')), + provider TEXT NOT NULL DEFAULT 'anthropic', + next_seq INTEGER NOT NULL DEFAULT 1, + parent_session_id TEXT, + status TEXT NOT NULL DEFAULT 'idle', + pid INTEGER + ); + INSERT INTO sessions (id, cwd, model) VALUES ('s1', '/tmp', 'm'); + PRAGMA user_version = 5;", + ) + .unwrap(); + } + + let store = Store::open(&path).unwrap(); + let session = store.get_session("s1").unwrap(); + // Sessions that predate the feature are shared-cwd sessions. + assert_eq!(session.worktree_path, None); + assert_eq!(session.worktree_branch, None); + } + + #[test] + fn set_worktree_round_trips_through_list_and_resolve() { + let (_dir, store) = store(); + let s = store.create_session("/repo", "anthropic", "m").unwrap(); + assert_eq!(s.worktree_path, None); + + store + .set_worktree(&s.id, "/h/.bullpen/worktrees/s1", "bullpen/abcd1234") + .unwrap(); + + for found in [ + store.get_session(&s.id).unwrap(), + store.resolve_session(&s.id[..8]).unwrap(), + store.list_sessions().unwrap().remove(0), + ] { + assert_eq!( + found.worktree_path.as_deref(), + Some("/h/.bullpen/worktrees/s1") + ); + assert_eq!(found.worktree_branch.as_deref(), Some("bullpen/abcd1234")); + // The dispatch anchor is untouched: it is what still points at + // the repository once the worktree directory is gone. + assert_eq!(found.cwd, "/repo"); + } + } } From 069aa13faa9f005256a71ac0d3ef90d0d4f0f590 Mon Sep 17 00:00:00 2001 From: Victor Garcia Date: Sat, 8 Aug 2026 02:13:20 -0700 Subject: [PATCH 2/2] fix(worktree): close the resume and branch-name holes review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three fail-closed gaps in the --worktree path, all of which could place a run somewhere the session had never been: - branch_for truncated the session id to eight hex characters. Two sessions colliding on 32 bits would name one branch, which git refuses to check out in two worktrees, failing the second dispatch. Use the whole id. - locate accepted any directory at the recorded path. An ordinary directory restored there, or a worktree of a different repository, became Location::Use. The candidate is now checked against the session's own repository (shared git dir and top level both), and a foreign one is a third error, Location::Occupied, rather than a fourth place to run. - the session row was written before the worktree but the worktree fields after it, so a failed `git worktree add` left a row naming no worktree at all — which resume reads as a plain shared-cwd session. Record path and branch first: a row naming a directory that is not there is refused, which is the answer this flag exists to give. git_write_roots keeps the whole common directory, and now says why: `git gc` prunes refs/heads/bullpen/ and packs the refs into a packed-refs file at the top of the common dir, so no path allowlist can grant a worktree its own branch and nothing else. The residual risk — a sandboxed agent writing the main repository's config and hooks — is documented rather than papered over. Refs #13 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PfAfAujueuZ3rDTiL9apx3 --- ARCHITECTURE.md | 20 ++++- README.md | 4 +- crates/cli/src/main.rs | 21 ++++- crates/cli/src/worktree.rs | 170 +++++++++++++++++++++++++++++++------ crates/store/src/lib.rs | 7 +- 5 files changed, 181 insertions(+), 41 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 5c77c5b..326dff2 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -366,9 +366,16 @@ The resume rule follows the same posture. The recorded location beats the directory the command was typed in; a missing directory whose branch survives is recreated from that branch with one stderr notice; a missing directory *and* missing branch is an error naming both, never a silent run -somewhere else. `--worktree` outside a git repository is likewise an error, -because falling back to the shared checkout would silently reintroduce -exactly the interference the flag exists to remove. +somewhere else. A directory that exists but is not that worktree — an +ordinary one restored at the path, a worktree of another repository — is a +third error rather than a fourth place to run, so the recorded path is +checked against the session's repository, not merely stat'ed. +`--worktree` outside a git repository is likewise an error, because falling +back to the shared checkout would silently reintroduce exactly the +interference the flag exists to remove. For the same reason the path and +branch are written to the session row *before* `git worktree add` runs: a +row pointing at a directory that failed to appear is refused on resume, +while a row pointing at nothing would read as a plain shared-cwd session. `--sandbox` has to be widened to compose with this. A linked worktree's `.git` is a file; its index, refs and objects all live under the *main* @@ -376,4 +383,9 @@ repository, outside the worktree, so a sandbox confined to the worktree alone leaves an agent able to edit files and unable to stage or commit them — which under the rule above is fatal, since a commit is the only evidence that would ever justify reclaiming its directory. The run therefore -adds the worktree's git dirs to the write roots. +adds the worktree's git dirs to the write roots. That is the whole shared +common directory, because git's ref store cannot be sliced by path — `git +gc` prunes `refs/heads/bullpen/` and moves the refs into a `packed-refs` +file at the top of it — so a sandboxed agent in a worktree can also write +the main repository's config and hooks. Confining that needs a mechanism +other than a path allowlist. diff --git a/README.md b/README.md index e477485..661e766 100644 --- a/README.md +++ b/README.md @@ -96,8 +96,8 @@ quit. Quitting stops nothing. Plain `--bg` sessions share your checkout, so two of them edit the same files. `--worktree` gives a session a git worktree of its own on a run-unique `bullpen/` branch, under `$BULLPEN_HOME/worktrees/`; -the path shows up in `bullpen sessions`, in `sessions --json`, and in the -peek panel, and `bullpen run -r ` returns to it from anywhere. Outside a +the path shows up in `bullpen sessions`, in `bullpen sessions --json`, and +in the peek panel, and `bullpen run -r ` returns to it from anywhere. Outside a git repository the flag fails rather than quietly sharing the checkout. **Nothing removes a worktree or its branch** — not on success, not on failure, not later. A worktree can hold the only copy of what an agent did, diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 2fd0d71..e7e5b3a 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -391,8 +391,13 @@ async fn run( Some(root) => { let path = worktree::worktree_path(&session.id); let branch = worktree::branch_for(&session.id); - worktree::create(root, &path, &branch)?; + // Recorded before it is created, not after: if `git worktree + // add` fails the row then names a directory that does not + // exist, which resume refuses outright. A row naming nothing + // would instead resume into the shared checkout — the bug + // this flag exists to close. store.set_worktree(&session.id, &path.display().to_string(), &branch)?; + worktree::create(root, &path, &branch)?; Some(path) } None => None, @@ -458,9 +463,17 @@ async fn run( path } worktree::Location::Fail { path, branch } => bail!( - "session {} ran in worktree {} on branch {branch}, and neither \ - survives — nothing was removed by bullpen, so restore them (or \ - start a new session) rather than running somewhere else", + "session {} is recorded in worktree {} on branch {branch}, and \ + neither survives — nothing was removed by bullpen, so restore \ + them (or start a new session) rather than running somewhere else", + &session.id[..8], + path.display() + ), + worktree::Location::Occupied { path, branch } => bail!( + "session {} is recorded in worktree {}, but what stands there is \ + not that worktree — move it aside and restore the session's own \ + from branch {branch} (or start a new session) rather than \ + running somewhere else", &session.id[..8], path.display() ), diff --git a/crates/cli/src/worktree.rs b/crates/cli/src/worktree.rs index fc0618f..6fe6b83 100644 --- a/crates/cli/src/worktree.rs +++ b/crates/cli/src/worktree.rs @@ -33,10 +33,12 @@ fn worktree_dir(home: &Path, session_id: &str) -> PathBuf { home.join("worktrees").join(session_id) } -/// The branch a session's worktree checks out. Run-unique because session -/// ids are, so two concurrent dispatches can never collide on it. +/// The branch a session's worktree checks out. The whole session id, not the +/// short prefix the CLI prints: a truncated id is 32 bits, and two sessions +/// that collided would name one branch, which git refuses to check out in two +/// worktrees at once — the second dispatch would simply fail. pub fn branch_for(session_id: &str) -> String { - format!("bullpen/{}", &session_id[..8]) + format!("bullpen/{session_id}") } /// The repository containing `cwd`. @@ -120,6 +122,14 @@ fn git_worktree_add( /// the main repository, so a sandbox confined to the worktree alone would let /// an agent edit files but never stage or commit them — and a commit is the /// only evidence that would ever justify reclaiming its directory. +/// +/// The whole common directory, not the session's own ref and objects: git's +/// ref store is shared mutable state that a path allowlist cannot slice. Once +/// `git gc` packs refs it prunes `refs/heads/bullpen/`, so the next commit has +/// to recreate that directory under `refs/heads` — and `packed-refs` itself +/// sits at the top of the common directory. Granting either grants every +/// branch. The residual risk is real and unavoidable here: a sandboxed agent +/// in a worktree can write the main repository's config and hooks. pub fn git_write_roots(cwd: &Path) -> Vec { let (Some(git_dir), Some(common_dir)) = ( rev_parse_dir(cwd, "--absolute-git-dir"), @@ -179,33 +189,53 @@ pub enum Location { path: PathBuf, branch: String, }, + /// Something that is not this session's worktree sits at the recorded + /// path. + Occupied { + path: PathBuf, + branch: String, + }, +} + +/// What stands at a session's recorded worktree path. +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub enum Candidate { + /// A live worktree of the session's own repository. + Live, + /// Nothing — the directory is gone. + Gone, + /// A directory, but not that worktree: an ordinary one restored at the + /// path, or a worktree of a different repository. + Foreign, } /// The resume decision, as a table. git and the filesystem supply only the -/// two booleans, so every row is testable without either. +/// [`Candidate`] and the branch bit, so every row is testable without either. /// /// A recorded worktree always wins over the caller's working directory: a /// `bullpen run -r ` typed from somewhere else must not quietly append -/// to the session in a different tree than the one it has been editing. -pub fn decide(recorded: Option<(&str, &str)>, dir_exists: bool, branch_exists: bool) -> Location { +/// to the session in a different tree than the one it has been editing. By +/// the same rule a foreign directory is refused outright rather than run in +/// or overwritten — either would be the silent misplacement this avoids. +pub fn decide( + recorded: Option<(&str, &str)>, + candidate: Candidate, + branch_exists: bool, +) -> Location { let Some((path, branch)) = recorded else { return Location::Shared; }; let path = PathBuf::from(path); - match (dir_exists, branch_exists) { - (true, _) => Location::Use(path), - (false, true) => Location::Recreate { - path, - branch: branch.to_string(), - }, - (false, false) => Location::Fail { - path, - branch: branch.to_string(), - }, + let branch = branch.to_string(); + match (candidate, branch_exists) { + (Candidate::Live, _) => Location::Use(path), + (Candidate::Foreign, _) => Location::Occupied { path, branch }, + (Candidate::Gone, true) => Location::Recreate { path, branch }, + (Candidate::Gone, false) => Location::Fail { path, branch }, } } -/// The impure half of [`decide`]: answers its two booleans. `anchor` is the +/// The impure half of [`decide`]: answers its two inputs. `anchor` is the /// session's recorded cwd — the directory it was dispatched from, which is /// what still points at the repository once the worktree itself is gone. pub fn locate( @@ -216,10 +246,34 @@ pub fn locate( let Some((path, branch)) = recorded_path.zip(recorded_branch) else { return Location::Shared; }; - let dir_exists = Path::new(path).is_dir(); - let branch_lives = - !dir_exists && repo_root(anchor).is_ok_and(|root| branch_exists(&root, branch)); - decide(Some((path, branch)), dir_exists, branch_lives) + let candidate = inspect(anchor, Path::new(path)); + let branch_lives = candidate == Candidate::Gone + && repo_root(anchor).is_ok_and(|root| branch_exists(&root, branch)); + decide(Some((path, branch)), candidate, branch_lives) +} + +/// Whether `path` really is a worktree of the repository `anchor` sits in. +/// `is_dir` alone would accept an ordinary directory left at the recorded +/// path — running there is the silent misplacement the recording exists to +/// prevent — so both the shared git directory and the worktree's own top +/// level have to agree. +fn inspect(anchor: &Path, path: &Path) -> Candidate { + if !path.is_dir() { + return Candidate::Gone; + } + let same_repo = rev_parse_dir(path, "--git-common-dir") + .zip(rev_parse_dir(anchor, "--git-common-dir")) + .is_some_and(|(candidate, anchor)| candidate == anchor); + let is_top_level = repo_root(path).is_ok_and(|top| canonical(&top) == canonical(path)); + if same_repo && is_top_level { + Candidate::Live + } else { + Candidate::Foreign + } +} + +fn canonical(path: &Path) -> PathBuf { + path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) } #[cfg(test)] @@ -250,8 +304,12 @@ mod tests { } #[test] - fn branch_name_is_derived_from_the_session_id_prefix() { - assert_eq!(branch_for(ID), "bullpen/01234567"); + fn branch_name_carries_the_whole_session_id_not_the_short_prefix() { + assert_eq!(branch_for(ID), format!("bullpen/{ID}")); + assert_ne!( + branch_for("0123456789abcdef00000000deadbeef"), + branch_for("0123456789abcdef11111111deadbeef") + ); } #[test] @@ -264,13 +322,13 @@ mod tests { #[test] fn decide_falls_back_to_the_shared_cwd_only_when_nothing_was_recorded() { - assert_eq!(decide(None, false, false), Location::Shared); + assert_eq!(decide(None, Candidate::Gone, false), Location::Shared); } #[test] - fn decide_uses_the_recorded_directory_when_it_still_exists() { + fn decide_uses_the_recorded_directory_when_it_is_still_that_worktree() { assert_eq!( - decide(Some(("/w/a", "bullpen/a")), true, false), + decide(Some(("/w/a", "bullpen/a")), Candidate::Live, false), Location::Use(PathBuf::from("/w/a")) ); } @@ -278,7 +336,7 @@ mod tests { #[test] fn decide_recreates_from_the_branch_when_only_the_directory_is_gone() { assert_eq!( - decide(Some(("/w/a", "bullpen/a")), false, true), + decide(Some(("/w/a", "bullpen/a")), Candidate::Gone, true), Location::Recreate { path: PathBuf::from("/w/a"), branch: "bullpen/a".into(), @@ -289,7 +347,7 @@ mod tests { #[test] fn missing_branch_fails_rather_than_falling_back_to_the_callers_cwd() { assert_eq!( - decide(Some(("/w/a", "bullpen/a")), false, false), + decide(Some(("/w/a", "bullpen/a")), Candidate::Gone, false), Location::Fail { path: PathBuf::from("/w/a"), branch: "bullpen/a".into(), @@ -297,6 +355,17 @@ mod tests { ); } + #[test] + fn a_foreign_directory_at_the_recorded_path_is_refused_even_with_the_branch() { + assert_eq!( + decide(Some(("/w/a", "bullpen/a")), Candidate::Foreign, true), + Location::Occupied { + path: PathBuf::from("/w/a"), + branch: "bullpen/a".into(), + } + ); + } + #[test] fn non_repo_error_names_the_directory_and_refuses_to_degrade() { let dir = tempfile::tempdir().unwrap(); @@ -330,6 +399,51 @@ mod tests { assert!(path.join("f.txt").is_file()); } + #[test] + fn locate_accepts_only_a_real_worktree_of_the_sessions_own_repository() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("repo"); + init_repo(&root); + let path = dir.path().join("worktrees").join("s1"); + create(&root, &path, "bullpen/s1").unwrap(); + let recorded = path.display().to_string(); + let at = |anchor: &Path| locate(Some(&recorded), Some("bullpen/s1"), anchor); + + assert_eq!(at(&root), Location::Use(path.clone())); + + // A worktree of some other repository, at the same recorded path. + let other = dir.path().join("other"); + init_repo(&other); + assert_eq!( + at(&other), + Location::Occupied { + path: path.clone(), + branch: "bullpen/s1".into(), + } + ); + + // An ordinary directory restored where the worktree used to be: the + // branch still exists, and it is still refused rather than run in. + std::fs::remove_dir_all(&path).unwrap(); + std::fs::create_dir_all(path.join("f.txt")).unwrap(); + assert_eq!( + at(&root), + Location::Occupied { + path: path.clone(), + branch: "bullpen/s1".into(), + } + ); + + std::fs::remove_dir_all(&path).unwrap(); + assert_eq!( + at(&root), + Location::Recreate { + path, + branch: "bullpen/s1".into(), + } + ); + } + #[test] fn an_ordinary_checkout_needs_no_write_roots_beyond_itself() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/store/src/lib.rs b/crates/store/src/lib.rs index c81365a..1f57893 100644 --- a/crates/store/src/lib.rs +++ b/crates/store/src/lib.rs @@ -336,9 +336,10 @@ impl Store { Ok(()) } - /// Record where an isolated session runs. Written after the worktree - /// exists, never as part of `create_session`: a row that names a - /// directory git failed to create is worse than a row that names none. + /// Record where an isolated session runs. Written before the worktree is + /// created, so a failed creation leaves a row naming a directory that is + /// not there — which resume refuses — rather than a row naming none, + /// which resume would read as an ordinary shared-cwd session. pub fn set_worktree( &self, session_id: &str,