diff --git a/client/src/project.rs b/client/src/project.rs index 5d84d2da..3e977ae5 100644 --- a/client/src/project.rs +++ b/client/src/project.rs @@ -402,13 +402,41 @@ pub async fn create( if let Some(effort) = effort { params["effort"] = json!(effort); } + // The client's settings, such as a sandbox, with the model's. + let mut settings = policy["settings"].as_object().cloned().unwrap_or_default(); if let Some(ttl) = crate::policy::cache_ttl(model) { - params["settings"] = json!({"cache_ttl": ttl}); + settings.insert("cache_ttl".into(), json!(ttl)); + } + if !settings.is_empty() { + params["settings"] = Value::Object(settings); } // The tools its client declares on the lead, from its first turn. if !policy["declare"].is_null() { params["declare"] = policy["declare"].clone(); } + // The client's gate, `create`'s approval fields, for the tools the lead + // has: the client may not know which its model is given. + if let (Value::Object(params), Some(gate)) = (&mut params, policy["gate"].as_object()) { + let has = |name: &str| { + params["tools"] + .as_array() + .into_iter() + .flatten() + .any(|tool| tool == name) + || (params.get("declare").and_then(Value::as_array)) + .into_iter() + .flatten() + .any(|tool| tool["name"] == name) + }; + let mut gate = gate.clone(); + if let Some(Value::Array(approve)) = gate.get_mut("approve") { + approve.retain(|tool| tool.as_str().is_some_and(has)); + if approve.is_empty() { + gate.remove("approver"); + } + } + params.extend(gate); + } let record = match daemon.request("create", params).await { Ok(record) => record, // Made since the look above: as when it was there before. @@ -688,6 +716,29 @@ mod tests { ); assert!(create["instructions"].as_str().unwrap().ends_with("rules\n\nWhen this folder is a git repository, every task works in its own worktree, so tasks do not collide: only you work in this folder.")); std::fs::remove_dir_all(dir.parent().unwrap()).unwrap(); + // A client's gate and settings go with the lead it makes. + let dir = root("gated"); + let daemon = Fake::default(); + make( + &daemon, + &dir, + &json!({"instructions": "rules", "model": "alpha/role", + "gate": {"approve": ["shell"], "approver": "auto"}, + "settings": {"sandbox": "workspace-write", "network": true}}), + &Picks::default(), + ) + .unwrap(); + let calls = daemon.calls.lock().unwrap().clone(); + let create = &calls.iter().find(|(op, _)| op == "create").unwrap().1; + assert_eq!( + (&create["approve"], &create["approver"]), + (&json!(["shell"]), &json!("auto")) + ); + assert_eq!( + create["settings"], + json!({"sandbox": "workspace-write", "network": true}) + ); + std::fs::remove_dir_all(dir.parent().unwrap()).unwrap(); } #[test] diff --git a/docs/RUST_PROTOTYPE.md b/docs/RUST_PROTOTYPE.md index a9c8c59d..43bd5156 100644 --- a/docs/RUST_PROTOTYPE.md +++ b/docs/RUST_PROTOTYPE.md @@ -1184,8 +1184,12 @@ since its history is kept in that family's encoding `approve: []` alone removes them, including the gates a bot took from the bot that made it: whoever configures a bot decides, as a person overriding one does, and bots it already made keep theirs. The gate is checked before -anything is written. It answers `{bot, bot_id, changed}`, `changed` naming the fields -(`model`, `instructions`, `gates`, `declared`) that differed; the same request again +anything is written. `sandbox`, with `writable` and `network` (as `create`'s +settings name them), replaces the bot's sandbox, and the two left out go back +to none and off; a bot another bot made keeps no weaker a sandbox than its +creator's, as when it was made, and a bot's own socket may not configure +at all. It answers `{bot, bot_id, changed}`, `changed` naming the fields +(`model`, `instructions`, `gates`, `declared`, `sandbox`) that differed; the same request again changes nothing, so a resend is harmless. A change is told to the bot's followers as a live `configured` event with the same fields, which is not kept, so a client that attaches later reads the bot as it is. It is refused with `bot_busy` @@ -2106,7 +2110,19 @@ read-only, and a missing `.git`, `.agents` or `.codex` may not be made: git runs hooks and config from there outside the sandbox, and agents trust what is there. The daemon's store and the files beside it stay read-only too. A protected folder that is a symlink refuses the call with -`sandbox_unavailable`, since the link could be swapped from inside. Network is +`sandbox_unavailable`, since the link could be swapped from inside. A git +folder given as a `writable` folder, so a bot may commit, keeps what git +runs read-only as `.git` does: its `hooks`, `config`, `config.worktree`, +submodules' `modules`, and the `commondir` that would point git at another +folder's hooks and config, its own and each linked worktree's under +`worktrees`. Missing `hooks` or `modules` may not be made, nor the git +folder's own missing `commondir` on macOS; on Linux, where a folder held in +its place would stop git, one the command makes is removed when it ends. So a sandboxed `git +worktree remove` of an existing worktree is refused. It is +known by its name, `.git` or `NAME.git`, which the bot can't change, not by +what is inside it. One whose config is missing, turns on per-worktree config +or includes other files stays read-only as a whole, since git reads those +files wherever they are made. Network is off unless `network` is set. `danger-full-access`, or no `sandbox`, runs unconfined. diff --git a/sonny/README.md b/sonny/README.md index 7874a40a..018d612e 100644 --- a/sonny/README.md +++ b/sonny/README.md @@ -237,6 +237,14 @@ off again never shows. now once it is idle. While the judge reviews a call the status row says `Reviewing approval request`, and a denied call is drawn as `✗ Request denied for …` with the judge's reason, in either view. + The mode sets the lead's sandbox too, as Codex's modes do: in `auto` + and `ask` its commands, writes and edits stay in its folder (and + `/tmp`), reach the network, and may commit to the folder's repository, + whose hooks and config stay read-only; a call that leaves the sandbox + waits for the verdict. In `full` the lead runs unconfined. Every task + is sandboxed the same way in its own folder, whatever the lead's mode. + A lead made before Sonny sandboxed leads is sandboxed when Sonny opens + it. On Linux sandboxes need bubblewrap. - `/approve` lists the calls the judge denied while Sonny watched, with the reason for each. Enter writes the call out whole, and a second Enter sends your approval of that exact call as your own message, so diff --git a/sonny/src/approvals.rs b/sonny/src/approvals.rs index 88492c80..19e3ae00 100644 --- a/sonny/src/approvals.rs +++ b/sonny/src/approvals.rs @@ -67,6 +67,56 @@ pub fn gate(mode: &str, tools: &[String]) -> Map { } } +/// The sandbox fields, as `configure` takes them and `create`'s settings +/// hold them, for an agent working in `workspace`: Codex's +/// `workspace-write`, reaching the network, with the folder's repository +/// writable too so a commit runs without asking (its hooks and config stay +/// read-only). A lead in `full` mode runs unconfined, as Codex's full +/// access does; a task is always confined. +pub fn sandbox(mode: &str, lead: bool, workspace: &std::path::Path) -> Map { + let fields = match mode { + "full" if lead => json!({"sandbox": "danger-full-access"}), + _ => { + let writable: Vec = repository(workspace).into_iter().collect(); + json!({"sandbox": "workspace-write", "writable": writable, "network": true}) + } + }; + match fields { + Value::Object(fields) => fields, + _ => Map::new(), + } +} + +/// The git folder of the repository `dir` is in, which its linked +/// worktrees share, by its real path as the daemon keeps it. Only one +/// named as the daemon knows a git folder, `.git` or `NAME.git`: in one +/// named otherwise, such as a separate git folder, its hooks and config +/// would not be kept, so a commit there asks instead. +pub fn repository(dir: &std::path::Path) -> Option { + let found = crate::task::git( + dir, + &["rev-parse", "--path-format=absolute", "--git-common-dir"], + ) + .ok() + .filter(|path| !path.is_empty())?; + let real = std::fs::canonicalize(found).ok()?; + real.to_str() + .filter(|path| path.ends_with(".git")) + .map(str::to_owned) +} + +/// Whether `record`'s sandbox is the one `fields` give it: its mode, its +/// writable folders, which a moved folder leaves stale, and its network. +pub fn sandboxed_as(record: &Value, fields: &Map) -> bool { + let fields = Value::Object(fields.clone()); + let settings = &record["settings"]; + let writable = |value: &Value| value["writable"].as_array().cloned().unwrap_or_default(); + settings["sandbox"] == fields["sandbox"] + && writable(settings) == writable(&fields) + && settings["network"].as_bool().unwrap_or(false) + == fields["network"].as_bool().unwrap_or(false) +} + /// What a bot's gate may hold back: its own `tools`, and of the tools its /// client declared, those Sonny gates (`served::GATED`). pub fn gateable(tools: &[String], declared: &Value) -> Vec { @@ -150,6 +200,51 @@ pub fn regate(record: &Value, mode: &str) -> Option> { mod tests { use super::*; + #[test] + fn a_lead_is_confined_but_in_full_mode_and_a_task_always() { + let dir = std::env::temp_dir().join(format!("sonny-sandbox-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let dir = std::fs::canonicalize(&dir).unwrap(); + crate::task::git(&dir, &["init", "-q"]).unwrap(); + let git = dir.join(".git").display().to_string(); + let confined = json!({"sandbox": "workspace-write", "writable": [git], "network": true}); + for (mode, _) in MODES { + let fields = sandbox(mode, false, &dir); + assert_eq!(Value::Object(fields.clone()), confined); + assert!(sandboxed_as(&json!({"settings": fields}), &fields)); + } + assert_eq!(Value::Object(sandbox("auto", true, &dir)), confined); + let full = sandbox("full", true, &dir); + assert_eq!( + Value::Object(full.clone()), + json!({"sandbox": "danger-full-access"}) + ); + let unconfined = json!({"settings": {"sandbox": "danger-full-access", "writable": [], "network": false}}); + assert!(sandboxed_as(&unconfined, &full)); + // A lead made before leads were confined is confined again, and + // one whose repository moved is given it where it is now. + let auto = sandbox("auto", true, &dir); + assert!(!sandboxed_as(&json!({"settings": {}}), &auto)); + let moved = json!({"settings": {"sandbox": "workspace-write", "writable": ["/gone/.git"], + "network": true}}); + assert!(!sandboxed_as(&moved, &auto)); + // Outside a repository, only its own folder; with a git folder + // named otherwise, whose hooks the daemon wouldn't know to keep, + // commits ask. + let plain = std::env::temp_dir(); + assert_eq!(sandbox("ask", true, &plain)["writable"], json!([])); + let apart = dir.join("apart"); + let tree = dir.join("tree"); + std::fs::create_dir_all(&tree).unwrap(); + crate::task::git( + &tree, + &["init", "-q", "--separate-git-dir", apart.to_str().unwrap()], + ) + .unwrap(); + assert_eq!(sandbox("ask", true, &tree)["writable"], json!([])); + std::fs::remove_dir_all(&dir).unwrap(); + } + #[test] fn modes_are_read_back_from_the_gates_they_make() { let tools: Vec = ["shell", "read", "wait", "history"] diff --git a/sonny/src/main.rs b/sonny/src/main.rs index 8e639c38..0e4d2b80 100644 --- a/sonny/src/main.rs +++ b/sonny/src/main.rs @@ -870,6 +870,19 @@ async fn run() -> Result<(), String> { app.workspace = dir.display().to_string(); app.move_to = Some(app.workspace.clone()); } + // A lead made before Sonny confined leads, or whose sandbox is not its + // mode's in the folder it now works in, takes its mode's again. Only + // ever confined this way: a lead is unconfined by the person's pick. + if project.is_some() + && app.gating.is_none() + && let Some(mode) = app.mode.filter(|mode| *mode != "full") + && !approvals::sandboxed_as( + &bot, + &approvals::sandbox(mode, true, std::path::Path::new(&app.workspace)), + ) + { + app.gating = Some(mode); + } app.project = project; // A new project's lead has no history yet: Sonny says hello over it. if fresh && options.side.is_none() { @@ -921,9 +934,12 @@ async fn create(client: &Client, options: &Options, bot: &str) -> Result<(), Str "instructions": instructions, "tools": tools, "declare": declare, "compaction_instructions": agent_client::policy::DEFAULT_COMPACTION_INSTRUCTIONS, "request_id": unique("tui-create")}); + // Confined from the start, as the mode it is made with confines it. + let mut settings = approvals::sandbox(approvals::MODES[0].0, true, &workspace); if let Some(ttl) = agent_client::policy::cache_ttl(&model) { - asked["settings"] = json!({"cache_ttl": ttl}); + settings.insert("cache_ttl".into(), json!(ttl)); } + asked["settings"] = Value::Object(settings); let tools: Vec = tools.into_iter().map(String::from).collect(); let tools = approvals::gateable(&tools, &declare); if let Value::Object(fields) = &mut asked { @@ -2502,6 +2518,11 @@ impl App { mode, &approvals::gateable(&self.tools, declared), )); + // A lead's sandbox goes with its mode. + if self.project.is_some() { + let workspace = std::path::Path::new(&self.workspace); + fields.extend(approvals::sandbox(mode, true, workspace)); + } } match self.client.request("configure", asked).await { Ok(_) => { diff --git a/sonny/src/project.rs b/sonny/src/project.rs index b5504470..51125018 100644 --- a/sonny/src/project.rs +++ b/sonny/src/project.rs @@ -495,12 +495,31 @@ async fn make( picks: &agent_client::project::Picks, ) -> Result { let (instructions, _) = folder(dir, role)?; + // Gated and confined from the start, as a new lead's mode makes it, + // whoever opens it first. + let mode = crate::approvals::MODES[0].0; + let tools: Vec = match &role.tools { + Some(tools) => tools.clone(), + // The tools the client will give it; one the gate names that the + // lead doesn't get is left out there. + None => { + let model = picks.model.as_deref().or(role.model.as_deref()); + agent_client::default_tools(client.providers(), model.unwrap_or(""), picks.web_search) + .into_iter() + .map(String::from) + .collect() + } + }; + let declare = json!(crate::declared::tools(true)); + let gate = crate::approvals::gate(mode, &crate::approvals::gateable(&tools, &declare)); let policy = json!({ "instructions": instructions, "compaction_instructions": agent_client::policy::DEFAULT_COMPACTION_INSTRUCTIONS, "model": role.model, "tools": role.tools, - "declare": crate::declared::tools(true), + "declare": declare, + "gate": gate, + "settings": crate::approvals::sandbox(mode, true, dir), }); let copies = crate::settings::Settings::load().copies; agent_client::project::create( diff --git a/sonny/src/task.rs b/sonny/src/task.rs index e4329a55..5e720ef2 100644 --- a/sonny/src/task.rs +++ b/sonny/src/task.rs @@ -171,10 +171,16 @@ pub fn start(here: &Path, lead: &str, start: &Start) -> Result { let flags: Vec<&String> = flags.iter().collect(); let flags = with_defaults(&info, &flags, lead, config.sonny.task_effort); let flags: Vec<&String> = flags.iter().collect(); - let flags = match told(&place.dir, &flags) { + let mut flags = match told(&place.dir, &flags) { Ok(flags) => flags, Err(error) => return failed(&place, false, error), }; + // Every task works in a sandbox, whatever its lead's mode, and may + // commit to its repository. + flags.extend(["--sandbox", "workspace-write", "--network"].map(String::from)); + if let Some(git) = crate::approvals::repository(&place.dir) { + flags.extend(["--writable".to_owned(), git]); + } // A path in the lead's folder would have the task write there instead // of in its copy, where nothing keeps it apart from the lead's work. let brief = vec![start.brief.clone()]; diff --git a/src/sandbox.rs b/src/sandbox.rs index b00ebe93..e98cf51b 100644 --- a/src/sandbox.rs +++ b/src/sandbox.rs @@ -35,6 +35,15 @@ const PROTECTED: [&str; 4] = [".git", ".agents", ".codex", ".aws"]; /// would be run by git outside the sandbox, and a new `.agents` read as /// the project's own settings. const PROTECTED_MISSING: [&str; 3] = [".git", ".agents", ".codex"]; +/// In a git folder given as a root, so a bot may commit, what git runs +/// stays read-only as it does under `.git`: its hooks, the config that can +/// name other hooks or programs, its submodules' git folders, which hold +/// their own, and `commondir`, which would have git take another folder's +/// hooks and config in its place. Missing hooks, submodules and `commondir` +/// are kept from being made. A git folder is known by its name, `.git` or +/// `NAME.git`, which the bot can't change, never by what is inside it, +/// which it can. +const GIT_PROTECTED: [&str; 5] = ["hooks", "config", "config.worktree", "modules", "commondir"]; /// A folder the bot may write, and what inside it stays read-only. #[derive(Debug, Clone, PartialEq, Eq)] @@ -44,6 +53,11 @@ pub struct Root { pub read_only: Vec, /// Paths inside the root that don't exist and may not be made. pub missing: Vec, + /// Paths right under the root that don't exist and may not be left made: + /// files git reads whenever they are there, so an empty folder held in + /// their place would stop git. On Linux, removed when the command ends; + /// elsewhere they are `missing`. + pub left: Vec, } /// What a sandboxed command may do: write its roots, and reach the network @@ -125,6 +139,7 @@ impl Policy { .read_only .iter() .chain(&root.missing) + .chain(&root.left) .any(|kept| path.starts_with(kept)) }) } @@ -372,6 +387,7 @@ fn root(path: &Path, daemon: &Daemon) -> Result { } let mut read_only = Vec::new(); let mut missing = Vec::new(); + let mut left = Vec::new(); for name in PROTECTED { let kept = path.join(name); if held(&kept) { @@ -379,20 +395,7 @@ fn root(path: &Path, daemon: &Daemon) -> Result { continue; } match std::fs::symlink_metadata(&kept) { - // A symlink the sandbox may replace can't be held read-only: - // whatever is mounted over its target, the link itself can be - // swapped for a folder of the command's own. Codex refuses it - // too. - Ok(found) if found.file_type().is_symlink() => { - return fail_with( - "sandbox_unavailable", - format!( - "{} is a symlink in a folder the bot may write, so it can't be kept \ - read-only", - kept.display() - ), - ); - } + Ok(found) if found.file_type().is_symlink() => return linked(&kept), Ok(found) => { // A linked worktree's .git names its repository's folder for // it, which stays read-only too when the root holds it. @@ -409,6 +412,51 @@ fn root(path: &Path, daemon: &Daemon) -> Result { Err(_) => {} } } + let git = path + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.ends_with(".git")); + if git && !git_folder_writable(path) { + read_only.push(path.to_owned()); + } else if git { + for name in GIT_PROTECTED { + let kept = path.join(name); + if held(&kept) { + missing.push(kept); + continue; + } + match std::fs::symlink_metadata(&kept) { + Ok(found) if found.file_type().is_symlink() => return linked(&kept), + Ok(_) => read_only.push(kept), + Err(_) if matches!(name, "hooks" | "modules") => missing.push(kept), + Err(_) if name == "commondir" && cfg!(target_os = "linux") => left.push(kept), + Err(_) if name == "commondir" => missing.push(kept), + Err(_) => {} + } + } + // Each linked worktree's folder names the repository's folder for + // it the same way. One without it was made since, and serves only a + // checkout the bot made to name it. Nothing this deep is removed + // after a command, as the root's own `commondir` is: its folder + // could be swapped for a link elsewhere before the removal follows. + for entry in std::fs::read_dir(path.join("worktrees")) + .into_iter() + .flatten() + .flatten() + { + match entry.file_type() { + Ok(found) if found.is_symlink() => return linked(&entry.path()), + Ok(found) if found.is_dir() => {} + _ => continue, + } + let kept = entry.path().join("commondir"); + match std::fs::symlink_metadata(&kept) { + Ok(found) if found.file_type().is_symlink() => return linked(&kept), + Ok(_) => read_only.push(kept), + Err(_) => {} + } + } + } if let Some(store) = &daemon.store && store.starts_with(path) { @@ -430,9 +478,34 @@ fn root(path: &Path, daemon: &Daemon) -> Result { path: path.to_owned(), read_only, missing, + left, + }) +} + +/// Whether a git folder may be written with its hooks and config kept: not +/// when its config is missing, which git would read once made, nor when it +/// turns on per-worktree config or includes other files, which git reads +/// wherever they are, made or not. Any mention of either counts. +fn git_folder_writable(path: &Path) -> bool { + std::fs::read_to_string(path.join("config")).is_ok_and(|config| { + let config = config.to_ascii_lowercase(); + !config.contains("worktreeconfig") && !config.contains("include") }) } +/// A symlink the sandbox may replace can't be held read-only: whatever is +/// mounted over its target, the link itself can be swapped for a folder of +/// the command's own. Codex refuses it too. +fn linked(kept: &Path) -> Result { + fail_with( + "sandbox_unavailable", + format!( + "{} is a symlink in a folder the bot may write, so it can't be kept read-only", + kept.display() + ), + ) +} + /// The store and the files SQLite and the daemon keep beside it. fn store_files(store: &Path) -> impl Iterator + '_ { ["", "-wal", "-shm", "-journal", ".owner-lock", ".sock"] @@ -724,8 +797,9 @@ fn held(path: &Path) -> bool { } /// What a running sandbox holds in place; dropping it lets the -/// placeholders go once no other sandbox holds them. -pub struct Hold(Vec); +/// placeholders go once no other sandbox holds them, and removes what the +/// command may not leave made. +pub struct Hold(Vec, Vec); /// Hold `policy`'s placeholders while its command runs, once any of them /// being removed is gone. @@ -758,7 +832,12 @@ pub async fn hold(policy: &Policy) -> Hold { }), } } - return Hold(paths); + let left = policy + .roots + .iter() + .flat_map(|root| root.left.iter().cloned()) + .collect(); + return Hold(paths, left); } } removed.await; @@ -788,12 +867,20 @@ impl Drop for Hold { } } } - if released.is_empty() { + let left = std::mem::take(&mut self.1); + if released.is_empty() && left.is_empty() { return; } // Off the runtime thread, and with no lock held: no sandbox takes - // one up while it is marked as being removed. + // one up while it is marked as being removed. On a thread of its + // own, since a runtime shutting down drops blocking work not yet + // started, which would leave them in place. let remove = move || { + // Git never makes these in a repository's own folder: one there + // now is the command's. + for path in &left { + let _ = std::fs::remove_file(path); + } for path in &released { // Only an empty one: a placeholder, never what a command // outside the sandbox made there since. @@ -804,9 +891,21 @@ impl Drop for Hold { .retain(|held| !(held.removing && released.contains(&held.path))); REMOVED.notify_waiters(); }; - match tokio::runtime::Handle::try_current() { - Ok(runtime) => drop(runtime.spawn_blocking(remove)), - Err(_) => remove(), + type Job = std::sync::Mutex>>; + let run = |job: &Job| { + if let Some(remove) = job.lock().unwrap().take() { + remove(); + } + }; + let job: std::sync::Arc = + std::sync::Arc::new(std::sync::Mutex::new(Some(Box::new(remove)))); + let theirs = job.clone(); + if std::thread::Builder::new() + .name("sandbox-placeholders".into()) + .spawn(move || run(&theirs)) + .is_err() + { + run(&job); } } } @@ -1030,6 +1129,70 @@ mod tests { )); } + #[test] + fn a_git_folder_given_as_a_root_keeps_what_git_runs() { + let base = std::env::temp_dir().join(format!("agent-sandbox-git-{}", std::process::id())); + let git = base.join(".git"); + std::fs::create_dir_all(git.join("objects")).unwrap(); + std::fs::write(git.join("HEAD"), "ref: refs/heads/main\n").unwrap(); + std::fs::write(git.join("config"), "").unwrap(); + std::fs::create_dir_all(git.join("worktrees/wt")).unwrap(); + std::fs::write(git.join("worktrees/wt/commondir"), "../..\n").unwrap(); + let base = std::fs::canonicalize(&base).unwrap(); + let git = base.join(".git"); + let policy = Policy::new( + Some(Mode::WorkspaceWrite), + Some(&base), + &[git.to_str().unwrap().to_owned()], + false, + &Daemon::default(), + ) + .unwrap() + .unwrap(); + assert!(policy.can_write(&git.join("objects/ab/cdef"))); + assert!(policy.can_write(&git.join("index.lock"))); + assert!(!policy.can_write(&git.join("config"))); + // Missing hooks may not be made, nor a submodule's git folder. + assert!(!policy.can_write(&git.join("hooks/pre-commit"))); + assert!(!policy.can_write(&git.join("modules/sub/hooks/pre-commit"))); + // Nor a `commondir`, which would have git take another folder's hooks + // and config, here or in a linked worktree's folder. + assert!(!policy.can_write(&git.join("commondir"))); + assert!(!policy.can_write(&git.join("worktrees/wt/commondir"))); + assert!(policy.can_write(&git.join("worktrees/wt/index.lock"))); + // With per-worktree config on, whose files git reads wherever they + // are made, none of it is written. + std::fs::write( + git.join("config"), + "[extensions]\n\tworktreeConfig = true\n", + ) + .unwrap(); + let policy = Policy::new( + Some(Mode::WorkspaceWrite), + Some(&base), + &[git.to_str().unwrap().to_owned()], + false, + &Daemon::default(), + ) + .unwrap() + .unwrap(); + assert!(!policy.can_write(&git.join("objects/ab/cdef"))); + assert!(!policy.can_write(&git.join("worktrees/x/config.worktree"))); + // As with config it includes, which may be anywhere. + std::fs::write(git.join("config"), "[include]\n\tpath = extra.conf\n").unwrap(); + let policy = Policy::new( + Some(Mode::WorkspaceWrite), + Some(&base), + &[git.to_str().unwrap().to_owned()], + false, + &Daemon::default(), + ) + .unwrap() + .unwrap(); + assert!(!policy.can_write(&git.join("extra.conf"))); + std::fs::remove_dir_all(&base).unwrap(); + } + #[test] fn the_deepest_root_decides_and_seatbelt_keeps_what_it_keeps() { let base = std::env::temp_dir().join(format!("agent-sandbox-unit-{}", std::process::id())); @@ -1198,6 +1361,7 @@ mod tests { Root { read_only: PROTECTED.iter().map(|name| path.join(name)).collect(), missing: Vec::new(), + left: Vec::new(), path: path.clone(), } }) diff --git a/src/server/mod.rs b/src/server/mod.rs index c3e2bb41..a043399d 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -265,6 +265,12 @@ enum Command { /// The tools its client declares, replacing those it has; `[]` /// removes them. declare: Option>, + /// Its command sandbox, replacing the one it has, as `create`'s + /// settings name it: with `writable` and `network`, which + /// otherwise go back to none and off. + sandbox: Option, + writable: Option>, + network: Option, }, /// Summarize a bot's context with its compaction instructions before /// its next model call: a running turn's next round boundary, or an @@ -2679,10 +2685,39 @@ impl Service { approver, approve_expire_ms, declare, + sandbox, + writable, + network, } => { if effort.is_some() && model.is_none() { return fail_with("invalid_effort", "effort goes with model"); } + let sandbox = match sandbox { + Some(mode) => { + let mut settings = Settings { + sandbox: Some(mode), + writable: writable.unwrap_or_default(), + network: network.unwrap_or(false), + ..Settings::default() + }; + settings.validate()?; + // Checking for a sandbox and resolving folders touch + // the filesystem: off the runtime thread. + let settings = tokio::task::spawn_blocking(move || { + confine(&mut settings).map(|()| settings) + }) + .await + .map_err(|_| Error::new("sandbox_unavailable"))??; + Some((settings.sandbox, settings.writable, settings.network)) + } + None if writable.is_some() || network.is_some() => { + return fail_with( + "invalid_setting", + "writable and network go with sandbox", + ); + } + None => None, + }; let gate = match (&approve, &approver, approve_expire_ms) { (None, None, None) => None, (Some(tools), None, None) if tools.is_empty() => Some(None), @@ -2769,6 +2804,11 @@ impl Service { { changed.push("declared"); } + if let Some(sandbox) = sandbox + && db.set_sandbox(&bot, Some(id), sandbox)? + { + changed.push("sandbox"); + } Ok(json!({"bot":bot,"bot_id":id,"changed":changed})) }) diff --git a/src/store/db.rs b/src/store/db.rs index 43bfa904..7ebcd683 100644 --- a/src/store/db.rs +++ b/src/store/db.rs @@ -2247,6 +2247,45 @@ impl Database { )?; Ok(changed == 1) } + /// Replace a bot's sandbox: its mode, the folders it may write besides + /// its own and whether it reaches the network. A bot another bot made + /// keeps no weaker a sandbox than its creator's, as when it was made. + /// Whether anything changed. + pub fn set_sandbox( + &mut self, + name: &str, + bot_id: Option, + sandbox: (Option, Vec, bool), + ) -> Result { + let id = self.identity(name, bot_id)?; + let bot = self.inspect(name)?; + let mut settings = bot.settings; + (settings.sandbox, settings.writable, settings.network) = sandbox; + let creator = match bot.created_by_id { + Some(creator) => self + .conn + .query_row("SELECT settings FROM bots WHERE id=?", [creator], |r| { + r.get::<_, Option>(0) + }) + .optional()? + .map(|text| match text { + Some(text) => serde_json::from_str(&text), + None => Ok(Settings::default()), + }) + .transpose()?, + None => None, + }; + if let Some(creator) = &creator { + settings = settings.under(creator); + } + settings.validate()?; + let stored = settings.stored()?; + let changed = self.conn.execute( + "UPDATE bots SET settings=? WHERE id=? AND settings IS NOT ?", + params![stored, id, stored], + )?; + Ok(changed == 1) + } /// Bounded keyset pages. Full instructions remain available through inspect. /// A page of bots in name order after `after`. `name` keeps those whose /// name matches a GLOB pattern, which reads only its literal prefix's diff --git a/tests/test_approvals.py b/tests/test_approvals.py index 37f168fb..09d695c2 100644 --- a/tests/test_approvals.py +++ b/tests/test_approvals.py @@ -132,6 +132,57 @@ def test_a_sandboxed_bot_asks_only_to_leave_its_sandbox(self): self.assertEqual(sorted(p.name for p in outside.iterdir()), ['escaped', 'written']) self.assertTrue((self.path / 'notes.md').exists()) + @unittest.skipUnless(shutil.which('bwrap') or sys.platform == 'darwin', 'sandboxes need bubblewrap') + def test_a_bot_given_its_repository_commits_in_its_worktree_and_leaves_what_git_runs(self): + git = ['git', '-c', 'user.name=t', '-c', 'user.email=t@example.com'] + repo, tree = self.path / 'repo', self.path / 'tree' + repo.mkdir() + for args in (['init', '-q', '-b', 'main'], ['commit', '-q', '--allow-empty', '-m', 'one'], + ['worktree', 'add', '-q', '-b', 'task', str(tree)]): + subprocess.run(git + args, cwd=repo, check=True, capture_output=True) + config = (repo / '.git/config').read_text() + # An unsandboxed bot is confined once configured; one it makes keeps + # its sandbox whatever it is configured to. + client = self.client('shell') + bob = client.request('create', bot='Bob', workspace=str(tree))['result']['bot_id'] + configured = client.request('configure', bot='Bob', sandbox='workspace-write', + writable=[str(repo / '.git')], network=True)['result'] + self.assertEqual(configured['changed'], ['sandbox']) + client.request('create', bot='Kid', workspace=str(tree), created_by='Bob', created_by_id=bob) + client.request('configure', bot='Kid', sandbox='danger-full-access') + self.assertEqual(client.request('inspect', bot='Kid')['result']['settings']['sandbox'], 'workspace-write') + self.assertEqual(client.request('configure', bot='Bob', network=True)['error'], 'invalid_setting') + calls = [['shell', {'command': ' '.join(git) + ' commit -q --allow-empty -m two'}], + ['shell', {'command': f'echo exit 1 > {repo}/.git/hooks/pre-commit'}], + ['shell', {'command': f'echo "[core] hooksPath = /tmp" >> {repo}/.git/config'}], + # What is inside the folder doesn't make it git's: one call + # moving HEAD leaves the next still kept from the hooks. + ['shell', {'command': f'mv {repo}/.git/HEAD {repo}/.git/HEAD.x'}], + ['shell', {'command': f'echo exit 1 > {repo}/.git/hooks/post-checkout; ' + f'mv {repo}/.git/HEAD.x {repo}/.git/HEAD'}], + # Nor may it point git at another folder's hooks and config. + ['shell', {'command': f'echo /tmp > {repo}/.git/commondir'}], + ['shell', {'command': f'echo /tmp > {repo}/.git/worktrees/tree/commondir'}]] + turn = client.request('submit', bot='Bob', request_id='t', + prompt='calls:' + json.dumps(calls))['result']['turn'] + self.assertEqual(client.finished(turn)['data']['status'], 'completed') + self.assertEqual(self.tool_output(client, 'shell-1')[1]['exit_code'], 0) + log = subprocess.run(['git', 'log', '--format=%s', 'task'], cwd=repo, capture_output=True, text=True) + self.assertEqual(log.stdout.split(), ['two', 'one']) + self.assertNotEqual(self.tool_output(client, 'shell-2')[1]['exit_code'], 0) + self.assertNotEqual(self.tool_output(client, 'shell-3')[1]['exit_code'], 0) + self.assertEqual(self.tool_output(client, 'shell-4')[1]['exit_code'], 0) + self.assertFalse((repo / '.git/hooks/pre-commit').exists()) + self.assertFalse((repo / '.git/hooks/post-checkout').exists()) + self.assertEqual((repo / '.git/config').read_text(), config) + self.assertNotEqual(self.tool_output(client, 'shell-7')[1]['exit_code'], 0) + self.assertEqual((repo / '.git/worktrees/tree/commondir').read_text(), '../..\n') + # Linux can't keep it from being made, only removes it after. + deadline = time.monotonic() + 5 + while (repo / '.git/commondir').exists() and time.monotonic() < deadline: + time.sleep(0.05) + self.assertFalse((repo / '.git/commondir').exists()) + @unittest.skipUnless(shutil.which('bwrap') or sys.platform == 'darwin', 'sandboxes need bubblewrap') def test_a_sandboxed_bot_without_a_gate_cannot_leave_its_sandbox(self): (ROOT / '.local').mkdir(exist_ok=True) diff --git a/tests/test_served.py b/tests/test_served.py index addfcf00..3c4ff686 100644 --- a/tests/test_served.py +++ b/tests/test_served.py @@ -87,6 +87,11 @@ def test_a_lead_starts_a_task_in_a_worktree_of_its_own(self): # It has the tools every agent has, not its lead's own. record = self.daemon.request('inspect', bot='calc.fix')['result'] self.assertEqual([tool['name'] for tool in record['declared']], ['show_agent']) + # It works in a sandbox, reaching the network, and may commit to the + # repository its worktree shares. + self.assertEqual(record['settings']['sandbox'], 'workspace-write') + self.assertEqual(record['settings']['writable'], [str((repo / '.git').resolve())]) + self.assertTrue(record['settings']['network']) # The brief names the task's copy, not the lead's folder. turns = self.daemon.request('turns', bot='calc.fix')['result']['turns'] prompts = self.daemon.request('prompts', bot='calc.fix', turn=turns[0]['turn'])['result']['prompts']