diff --git a/README.md b/README.md index 6c3dbd3..6d9ef20 100644 --- a/README.md +++ b/README.md @@ -167,6 +167,7 @@ duplicate comments. | Check Run creation and completion | Implemented | Check Runs attach to the resolved target head SHA; PR reviews re-fetch the head pre-publish and complete as neutral/Stale instead of publishing findings against a moved ref. | | Headless execution contract | Locked (v1) | Brief, result envelope, exit codes, and git-auth channel are pinned in [`docs/headless-contract.md`](docs/headless-contract.md) with JSON Schemas, golden fixtures, and a conformance test. | | `coven-code --headless` execution | Partial | Worker spawns headless sessions with a tokenless session brief and enforces task timeouts; result quality depends on the runtime. | +| Worker container isolation | Implemented | `worker.backend = "container"` runs each attempt in a fresh hardened container (read-only rootfs, cap-drop ALL, cpu/memory/pids/tmpfs/network limits, env-only token injection, kill-by-name on timeout); hosted posture refuses host execution without explicit opt-in — see [docs/container-isolation.md](docs/container-isolation.md). | | Pull request creation | Partial | Opens draft PRs from session results against the repository's resolved default/base branch. | | CovenCave task polling | Implemented | Task API served from the durable store, survives restarts, and is gated by the tenant boundary — `token` mode fails closed, tenant tokens are installation-scoped, and every read is audited (see [docs/security.md](docs/security.md)). | | Durable queue / task store | Implemented | Deliveries deduplicated by `X-GitHub-Delivery` before GitHub hears success; the SQLite `tasks` table is the queue (atomic claims, no drop path) and interrupted work is requeued at startup ([design](docs/durable-task-store.md)). | diff --git a/config/example.toml b/config/example.toml index a78eb62..b4152b2 100644 --- a/config/example.toml +++ b/config/example.toml @@ -18,6 +18,22 @@ coven_code_bin = "/usr/local/bin/coven-code" # Path to coven-code binary workspace_root = "/tmp/coven-github-tasks" # Ephemeral workspace root timeout_secs = 600 # 10 minutes per task max_retries = 2 # Retries for infra errors (exit code 2) +# ── Execution backend (issue #5) ──────────────────────────────────────────── +# "host" — run coven-code directly (development / trusted self-hosting) +# "container" — one fresh hardened container per task attempt (hosted posture) +# With [[installations]] configured, the host backend is refused unless +# allow_host_backend = true. +# backend = "container" +# allow_host_backend = false +# [worker.container] +# image = "ghcr.io/opencoven/coven-code:latest" +# docker_bin = "docker" # docker / podman / nerdctl +# coven_code_bin = "coven-code" # invocation inside the image +# cpus = 1.0 # docker --cpus +# memory = "2g" # docker --memory (SIGKILL at the limit) +# pids = 512 # docker --pids-limit +# tmpfs_size = "256m" # writable /tmp inside the read-only rootfs +# network = "bridge" # or "none", or a custom egress-limited network [storage] # Durable adapter state: webhook deliveries (idempotency) and task records. diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index e55274d..d6b98d4 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -373,6 +373,98 @@ pub struct WorkerConfig { /// Max retry attempts for infra errors (exit code 2) #[serde(default = "default_retries")] pub max_retries: u32, + /// Execution backend (issue #5): `host` runs coven-code directly (dev / + /// self-hosted); `container` runs each task attempt in a fresh container + /// with resource limits — the hosted posture. + #[serde(default)] + pub backend: WorkerBackendKind, + /// Container backend settings; ignored for the host backend. + #[serde(default)] + pub container: ContainerConfig, + /// Hosted mode (any [[installations]] configured) refuses the host + /// backend unless the operator explicitly opts in here. + #[serde(default)] + pub allow_host_backend: bool, +} + +/// Which sandbox executes coven-code sessions (issue #5). +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Deserialize, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum WorkerBackendKind { + #[default] + Host, + Container, +} + +/// Container backend settings (issue #5). Defaults are a conservative +/// hardened profile; see `docs/container-isolation.md`. +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct ContainerConfig { + /// Image containing the coven-code runtime. + #[serde(default = "default_container_image")] + pub image: String, + /// Docker-compatible CLI (docker, podman, nerdctl). + #[serde(default = "default_docker_bin")] + pub docker_bin: PathBuf, + /// coven-code invocation inside the image. + #[serde(default = "default_container_coven_code")] + pub coven_code_bin: String, + /// CPU limit (docker `--cpus`). + #[serde(default = "default_container_cpus")] + pub cpus: f64, + /// Memory limit (docker `--memory`), e.g. "2g". + #[serde(default = "default_container_memory")] + pub memory: String, + /// Process count limit (docker `--pids-limit`). + #[serde(default = "default_container_pids")] + pub pids: u32, + /// Size of the writable /tmp tmpfs, e.g. "256m". + #[serde(default = "default_container_tmpfs")] + pub tmpfs_size: String, + /// Network mode (docker `--network`): "bridge", "none", or a custom + /// egress-restricted network. + #[serde(default = "default_container_network")] + pub network: String, +} + +impl Default for ContainerConfig { + fn default() -> Self { + Self { + image: default_container_image(), + docker_bin: default_docker_bin(), + coven_code_bin: default_container_coven_code(), + cpus: default_container_cpus(), + memory: default_container_memory(), + pids: default_container_pids(), + tmpfs_size: default_container_tmpfs(), + network: default_container_network(), + } + } +} + +fn default_container_image() -> String { + "ghcr.io/opencoven/coven-code:latest".to_string() +} +fn default_docker_bin() -> PathBuf { + PathBuf::from("docker") +} +fn default_container_coven_code() -> String { + "coven-code".to_string() +} +fn default_container_cpus() -> f64 { + 1.0 +} +fn default_container_memory() -> String { + "2g".to_string() +} +fn default_container_pids() -> u32 { + 512 +} +fn default_container_tmpfs() -> String { + "256m".to_string() +} +fn default_container_network() -> String { + "bridge".to_string() } fn default_timeout() -> u64 { @@ -535,14 +627,44 @@ impl Config { )); } - if !binary_resolvable(&self.worker.coven_code_bin) { - out.push(Diagnostic::error( - "worker.coven_code_bin", - format!( - "coven-code binary not found at '{}' (and not on PATH) — build/install coven-code or fix the path.", - self.worker.coven_code_bin.display() - ), - )); + match self.worker.backend { + WorkerBackendKind::Host => { + if !binary_resolvable(&self.worker.coven_code_bin) { + out.push(Diagnostic::error( + "worker.coven_code_bin", + format!( + "coven-code binary not found at '{}' (and not on PATH) — build/install coven-code or fix the path.", + self.worker.coven_code_bin.display() + ), + )); + } + // Hosted posture gate (issue #5): multi-tenant installations + // must not run arbitrary repository workloads on the host + // unless the operator explicitly accepts that risk. + if !self.installations.is_empty() && !self.worker.allow_host_backend { + out.push(Diagnostic::error( + "worker.backend", + "installations are configured (hosted posture) but worker.backend is 'host' — set worker.backend = \"container\", or set worker.allow_host_backend = true to accept host execution explicitly.", + )); + } + } + WorkerBackendKind::Container => { + if !binary_resolvable(&self.worker.container.docker_bin) { + out.push(Diagnostic::error( + "worker.container.docker_bin", + format!( + "container CLI not found at '{}' (and not on PATH) — install docker/podman or fix worker.container.docker_bin.", + self.worker.container.docker_bin.display() + ), + )); + } + if self.worker.container.image.trim().is_empty() { + out.push(Diagnostic::error( + "worker.container.image", + "container image is empty — set worker.container.image to the coven-code runtime image.", + )); + } + } } // ── Familiars ─────────────────────────────────────────────────── @@ -862,6 +984,15 @@ fn next_step_for(field: &str, _message: &str) -> &'static str { "worker.coven_code_bin" => { "Install coven-code with headless support or set worker.coven_code_bin to the binary path." } + "worker.backend" => { + "Set worker.backend = \"container\" for hosted isolation, or worker.allow_host_backend = true to explicitly accept host execution." + } + "worker.container.docker_bin" => { + "Install a docker-compatible CLI or point worker.container.docker_bin at it." + } + "worker.container.image" => { + "Set worker.container.image to the image that carries the coven-code runtime." + } "familiars" => "Add at least one [[familiars]] block for the bot account that should receive work.", "familiars[].id" => "Give each familiar a stable, unique id.", "familiars[].bot_username" => { @@ -1128,6 +1259,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![good_familiar()], ); @@ -1169,6 +1303,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![good_familiar()], ); @@ -1208,6 +1345,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![good_familiar()], ); @@ -1232,6 +1372,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![good_familiar()], ); @@ -1260,6 +1403,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![good_familiar()], ); @@ -1285,6 +1431,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![], ); @@ -1313,6 +1462,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![good_familiar()], ); @@ -1362,6 +1514,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![good_familiar()], ); @@ -1389,6 +1544,62 @@ mod tests { assert!(errs.contains(&"installations[].id"), "{errs:?}"); } + #[test] + fn hosted_posture_refuses_the_host_backend_without_explicit_opt_in() { + let dir = tmpdir(); + let pem = write_pem(&dir); + let bin = write_bin(&dir); + let mut cfg = config_with( + GitHubAppConfig { + app_id: 123, + private_key_path: pem, + webhook_secret: "a-long-random-webhook-secret".into(), + api_base_url: None, + }, + WorkerConfig { + concurrency: 4, + coven_code_bin: bin.clone(), + workspace_root: dir.clone(), + timeout_secs: 600, + max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, + }, + vec![good_familiar()], + ); + cfg.installations = vec![InstallationConfig { + id: 7, + account: None, + familiars: vec![], + triggers: TriggerPolicy::default(), + limits: InstallationLimits::default(), + repos: std::collections::HashMap::new(), + }]; + + // Hosted posture + host backend: refused. + let diags = cfg.check(); + assert!(errors(&diags).contains(&"worker.backend"), "{diags:?}"); + + // Explicit operator opt-in clears it. + cfg.worker.allow_host_backend = true; + let diags = cfg.check(); + assert!(!errors(&diags).contains(&"worker.backend"), "{diags:?}"); + + // Container backend also clears it — but validates the runtime CLI. + cfg.worker.allow_host_backend = false; + cfg.worker.backend = WorkerBackendKind::Container; + cfg.worker.container.docker_bin = bin; // any executable file + let diags = cfg.check(); + assert!(!errors(&diags).contains(&"worker.backend"), "{diags:?}"); + cfg.worker.container.docker_bin = dir.join("no-such-docker"); + let diags = cfg.check(); + assert!( + errors(&diags).contains(&"worker.container.docker_bin"), + "{diags:?}" + ); + } + #[test] fn first_run_errors_include_operator_next_steps() { let dir = tmpdir(); @@ -1405,6 +1616,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![], ); @@ -1452,6 +1666,9 @@ mod tests { workspace_root: dir.clone(), timeout_secs: 600, max_retries: 2, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, }, vec![good_familiar(), good_familiar()], ); diff --git a/crates/webhook/src/routes.rs b/crates/webhook/src/routes.rs index da2c8c7..9b725a1 100644 --- a/crates/webhook/src/routes.rs +++ b/crates/webhook/src/routes.rs @@ -1001,6 +1001,9 @@ mod tests { workspace_root: PathBuf::from("/tmp/coven-github-test"), timeout_secs: 60, max_retries: 0, + backend: coven_github_config::WorkerBackendKind::Host, + container: coven_github_config::ContainerConfig::default(), + allow_host_backend: false, }, familiars: vec![FamiliarConfig { id: "cody".to_string(), diff --git a/crates/worker/src/backend.rs b/crates/worker/src/backend.rs new file mode 100644 index 0000000..839bc20 --- /dev/null +++ b/crates/worker/src/backend.rs @@ -0,0 +1,283 @@ +//! Worker execution backends (issue #5). +//! +//! Self-hosted development runs `coven-code` directly on the host; hosted +//! OpenCoven must not — tasks clone private repositories and execute project +//! commands. The container backend runs each task attempt in a fresh +//! container with resource limits and a hardened profile, torn down +//! unconditionally by `--rm` plus an explicit kill on timeout. +//! +//! Git authority is injected by environment variable *name* (`-e NAME` +//! inherits the value from the docker CLI's own environment), so the token +//! never appears in any argv, container inspect output, or shell history. + +use std::path::{Path, PathBuf}; +use std::time::Duration; + +use coven_github_config::{ContainerConfig, WorkerBackendKind, WorkerConfig}; +use tokio::process::Command; +use tracing::warn; + +/// Filename of the session brief inside the task workspace. +pub const BRIEF_FILE: &str = "session-brief.json"; +/// Filename of the result envelope inside the task workspace. +pub const RESULT_FILE: &str = "result.json"; +/// Workspace mount point inside the container. +const CONTAINER_WORKSPACE: &str = "/workspace"; + +/// How one `coven-code` launch ended, before contract classification. +#[derive(Debug)] +pub enum LaunchOutcome { + /// The process (or container) exited; `None` = killed by signal. + Exited(Option), + /// The wall-clock limit expired; the session was killed. + TimedOut, + /// The backend could not launch or await the session. + Failed(String), +} + +/// Execution backend, chosen by `worker.backend`. +pub enum Backend { + Host, + Container(ContainerConfig), +} + +impl Backend { + pub fn from_config(worker: &WorkerConfig) -> Self { + match worker.backend { + WorkerBackendKind::Host => Backend::Host, + WorkerBackendKind::Container => Backend::Container(worker.container.clone()), + } + } + + /// The workspace path as the runtime will see it: the host path for host + /// execution, the container mount point for container execution. The + /// session brief must reference this view (the brief travels into the + /// sandbox; host paths would be meaningless there). + pub fn workspace_view(&self, host_workspace: &Path) -> PathBuf { + match self { + Backend::Host => host_workspace.to_path_buf(), + Backend::Container(_) => PathBuf::from(CONTAINER_WORKSPACE), + } + } + + /// True when a nonstandard exit code plausibly means the sandbox limit + /// fired (docker reports SIGKILL terminations — OOM kill, `docker kill` + /// — as exit 137). + pub fn explains_kill(&self, code: i32) -> Option<&'static str> { + match self { + Backend::Container(_) if code == 137 => { + Some("container killed (exit 137) — memory limit or forced stop") + } + _ => None, + } + } + + /// Runs one `coven-code --headless` session to completion or timeout. + /// The brief must already be at `/session-brief.json`; + /// the result is expected at `/result.json` (both paths + /// as seen from the host). + pub async fn run( + &self, + worker: &WorkerConfig, + host_workspace: &Path, + task_id: &str, + git_token: &str, + ) -> LaunchOutcome { + let timeout = Duration::from_secs(worker.timeout_secs); + match self { + Backend::Host => { + let mut command = Command::new(&worker.coven_code_bin); + command + .arg("--headless") + .arg("--context") + .arg(host_workspace.join(BRIEF_FILE)) + .arg("--output") + .arg(host_workspace.join(RESULT_FILE)) + // Git auth is injected via the environment, never written + // to the session brief or any durable artifact (issue #4). + .env("COVEN_GIT_TOKEN", git_token); + await_child(command, timeout, None).await + } + Backend::Container(container) => { + let name = container_name(task_id); + let mut command = Command::new(&container.docker_bin); + command.args(docker_run_args(container, host_workspace, &name)); + // `-e COVEN_GIT_TOKEN` (name-only) inherits the value from + // the docker CLI's environment — set here, absent from argv. + command.env("COVEN_GIT_TOKEN", git_token); + let kill = KillSpec { + docker_bin: container.docker_bin.clone(), + name, + }; + await_child(command, timeout, Some(kill)).await + } + } + } +} + +/// How to stop a containerized session whose docker CLI we killed. +struct KillSpec { + docker_bin: PathBuf, + name: String, +} + +/// Container name for one task attempt. Unique per attempt: docker rejects +/// duplicate names, so a stale name must never collide with a retry. +fn container_name(task_id: &str) -> String { + format!("coven-task-{task_id}-{}", uuid::Uuid::new_v4().simple()) +} + +/// The full `docker run` argv (everything after the docker binary itself). +/// Pure so tests can pin the isolation profile. +pub fn docker_run_args( + container: &ContainerConfig, + host_workspace: &Path, + name: &str, +) -> Vec { + let mut args: Vec = vec![ + "run".into(), + "--rm".into(), + "--name".into(), + name.into(), + // Resource limits. + "--cpus".into(), + container.cpus.to_string(), + "--memory".into(), + container.memory.clone(), + "--pids-limit".into(), + container.pids.to_string(), + // Egress policy. + "--network".into(), + container.network.clone(), + // Hardened profile: read-only root, no capabilities, no privilege + // escalation; writable space is the workspace mount plus a bounded + // tmpfs. + "--read-only".into(), + "--cap-drop".into(), + "ALL".into(), + "--security-opt".into(), + "no-new-privileges".into(), + "--tmpfs".into(), + format!("/tmp:size={}", container.tmpfs_size), + // Only the task workspace is mounted; no other host state exists + // inside the sandbox. + "-v".into(), + format!("{}:{CONTAINER_WORKSPACE}", host_workspace.display()), + "-w".into(), + CONTAINER_WORKSPACE.into(), + // Name-only env forwarding: the value never enters argv. + "-e".into(), + "COVEN_GIT_TOKEN".into(), + container.image.clone(), + ]; + args.extend([ + container.coven_code_bin.clone(), + "--headless".into(), + "--context".into(), + format!("{CONTAINER_WORKSPACE}/{BRIEF_FILE}"), + "--output".into(), + format!("{CONTAINER_WORKSPACE}/{RESULT_FILE}"), + ]); + args +} + +async fn await_child( + mut command: Command, + timeout: Duration, + kill: Option, +) -> LaunchOutcome { + let mut child = match command.spawn() { + Ok(child) => child, + Err(e) => return LaunchOutcome::Failed(format!("failed to spawn session: {e}")), + }; + match tokio::time::timeout(timeout, child.wait()).await { + Ok(Ok(status)) => LaunchOutcome::Exited(status.code()), + Ok(Err(e)) => LaunchOutcome::Failed(format!("failed to await session: {e}")), + Err(_) => { + // Kill the CLI process first… + let _ = child.kill().await; + let _ = child.wait().await; + // …then the container itself: killing the docker CLI does not + // reliably stop the container it launched. + if let Some(kill) = kill { + match Command::new(&kill.docker_bin) + .args(["kill", &kill.name]) + .output() + .await + { + Ok(_) => {} + Err(e) => warn!(container = %kill.name, "docker kill failed: {e}"), + } + } + LaunchOutcome::TimedOut + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn container() -> ContainerConfig { + ContainerConfig::default() + } + + #[test] + fn docker_argv_pins_the_isolation_profile() { + let args = docker_run_args(&container(), Path::new("/srv/tasks/t1"), "coven-task-x"); + let joined = args.join(" "); + for expected in [ + "run --rm", + "--cpus 1", + "--memory 2g", + "--pids-limit 512", + "--network bridge", + "--read-only", + "--cap-drop ALL", + "--security-opt no-new-privileges", + "--tmpfs /tmp:size=256m", + "-v /srv/tasks/t1:/workspace", + "-w /workspace", + "-e COVEN_GIT_TOKEN", + "--headless --context /workspace/session-brief.json --output /workspace/result.json", + ] { + assert!(joined.contains(expected), "missing `{expected}` in: {joined}"); + } + } + + #[test] + fn token_values_never_enter_argv() { + // The env var is forwarded by NAME; no argv element may ever carry a + // value for it. + let args = docker_run_args(&container(), Path::new("/srv/tasks/t1"), "n"); + let position = args.iter().position(|a| a == "-e").expect("-e present"); + assert_eq!(args[position + 1], "COVEN_GIT_TOKEN"); + assert!( + !args.iter().any(|a| a.contains('=') && a.contains("COVEN_GIT_TOKEN")), + "no NAME=value form allowed: {args:?}" + ); + } + + #[test] + fn container_names_are_unique_per_attempt() { + assert_ne!(container_name("t1"), container_name("t1")); + } + + #[test] + fn exit_137_is_explained_only_for_containers() { + let containerised = Backend::Container(container()); + assert!(containerised.explains_kill(137).is_some()); + assert!(containerised.explains_kill(1).is_none()); + assert!(Backend::Host.explains_kill(137).is_none()); + } + + #[test] + fn workspace_view_maps_into_the_container() { + let host = Path::new("/srv/tasks/t1"); + assert_eq!(Backend::Host.workspace_view(host), host); + assert_eq!( + Backend::Container(container()).workspace_view(host), + Path::new("/workspace") + ); + } +} diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 88ff611..3e7f0d9 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -3,7 +3,6 @@ use anyhow::Result; use std::path::Path; use std::time::Duration; -use tokio::process::Command; use tracing::{error, info, warn}; use coven_github_api::{ @@ -13,6 +12,7 @@ use coven_github_api::{ use coven_github_config::{Config, FamiliarConfig}; use coven_github_store::{Store, Terminal, TerminalState}; +pub mod backend; pub mod brief; pub mod findings; pub mod memory; @@ -783,15 +783,16 @@ async fn run_and_publish( let brief = brief::build( task, familiar, - workspace, + // The brief travels into the sandbox: reference the workspace as the + // runtime will see it (issue #5). + &backend::Backend::from_config(&config.worker).workspace_view(workspace), &targets.default_branch, review.as_ref(), memory_policy .as_ref() .map(|p| serde_json::to_value(p).expect("memory policy serializes")), ); - let brief_path = workspace.join("session-brief.json"); - let result_path = workspace.join("result.json"); + let brief_path = workspace.join(backend::BRIEF_FILE); let brief_json = serde_json::to_string_pretty(&brief)?; // Belt-and-braces on top of the serialization guard test: refuse to hand // the agent a brief that somehow embeds a live credential. @@ -847,8 +848,8 @@ async fn run_and_publish( // retried; exit 1 (gave up) and exit 3 (needs input) are terminal. let mut result = run_session( config, - &brief_path, - &result_path, + workspace, + &task.id, &agent_git, config.worker.max_retries, ) @@ -1138,15 +1139,15 @@ enum Attempt { async fn run_session( config: &Config, - brief_path: &Path, - result_path: &Path, + workspace: &Path, + task_id: &str, git_token: &str, max_retries: u32, ) -> Result { run_session_with_backoff( config, - brief_path, - result_path, + workspace, + task_id, git_token, max_retries, RETRY_BACKOFF_BASE, @@ -1160,15 +1161,15 @@ async fn run_session( /// retry boundary is exit 2 / timeout / signal, never exit 1 or 3. async fn run_session_with_backoff( config: &Config, - brief_path: &Path, - result_path: &Path, + workspace: &Path, + task_id: &str, git_token: &str, max_retries: u32, backoff_base: Duration, ) -> Result { let mut attempts = 0u32; loop { - match run_coven_code(config, brief_path, result_path, git_token).await { + match run_coven_code(config, workspace, task_id, git_token).await { Attempt::Completed(result) => return Ok(*result), Attempt::RetrySafe(e) if attempts < max_retries => { attempts += 1; @@ -1180,63 +1181,57 @@ async fn run_session_with_backoff( } } +/// Runs one session attempt through the configured backend (issue #5) and +/// classifies the outcome per the exit-code contract +/// (`docs/headless-contract.md` §4). async fn run_coven_code( config: &Config, - brief_path: &Path, - result_path: &Path, + workspace: &Path, + task_id: &str, git_token: &str, ) -> Attempt { - let child = Command::new(&config.worker.coven_code_bin) - .arg("--headless") - .arg("--context") - .arg(brief_path) - .arg("--output") - .arg(result_path) - // Git auth is injected via the environment, never written to the - // session brief or any durable artifact (issue #4). - .env("COVEN_GIT_TOKEN", git_token) - .spawn(); - - let mut child = match child { - Ok(child) => child, - Err(e) => return Attempt::RetrySafe(anyhow::anyhow!("failed to spawn coven-code: {e}")), - }; - - let status = match tokio::time::timeout( - Duration::from_secs(config.worker.timeout_secs), - child.wait(), - ) - .await + let backend = backend::Backend::from_config(&config.worker); + let result_path = workspace.join(backend::RESULT_FILE); + match backend + .run(&config.worker, workspace, task_id, git_token) + .await { - Ok(Ok(status)) => status, - Ok(Err(e)) => { - return Attempt::RetrySafe(anyhow::anyhow!("failed to await coven-code: {e}")) - } - Err(_) => { - let _ = child.kill().await; - let _ = child.wait().await; - return Attempt::RetrySafe(anyhow::anyhow!( - "coven-code timed out after {} seconds", - config.worker.timeout_secs - )); - } - }; - - match status.code() { // Terminal outcomes. result.json MUST be present and parseable for these // exit codes; if it isn't, the runtime misbehaved — fall back to a // retry-safe failure rather than silently losing the task. - Some(code @ (0 | 1 | 3)) => match read_result(result_path).await { - Ok(result) => Attempt::Completed(Box::new(result)), - Err(e) => Attempt::RetrySafe(anyhow::anyhow!( - "coven-code exited {code} but result.json was unusable: {e}" - )), - }, - Some(2) => Attempt::RetrySafe(anyhow::anyhow!("coven-code infra error (exit 2)")), - Some(code) => Attempt::RetrySafe(anyhow::anyhow!( - "coven-code exited with unexpected code {code}" + backend::LaunchOutcome::Exited(Some(code @ (0 | 1 | 3))) => { + match read_result(&result_path).await { + Ok(result) => Attempt::Completed(Box::new(result)), + Err(e) => Attempt::RetrySafe(anyhow::anyhow!( + "coven-code exited {code} but result.json was unusable: {e}" + )), + } + } + backend::LaunchOutcome::Exited(Some(2)) => { + Attempt::RetrySafe(anyhow::anyhow!("coven-code infra error (exit 2)")) + } + backend::LaunchOutcome::Exited(Some(code)) => { + // Resource-limit terminations must be visible in failure states + // (issue #5): name the sandbox explanation when there is one. + match backend.explains_kill(code) { + Some(reason) => Attempt::RetrySafe(anyhow::anyhow!( + "coven-code exited with unexpected code {code}: {reason}" + )), + None => Attempt::RetrySafe(anyhow::anyhow!( + "coven-code exited with unexpected code {code}" + )), + } + } + backend::LaunchOutcome::Exited(None) => { + Attempt::RetrySafe(anyhow::anyhow!("coven-code killed by signal")) + } + backend::LaunchOutcome::TimedOut => Attempt::RetrySafe(anyhow::anyhow!( + "coven-code timed out after {} seconds", + config.worker.timeout_secs )), - None => Attempt::RetrySafe(anyhow::anyhow!("coven-code killed by signal")), + backend::LaunchOutcome::Failed(message) => { + Attempt::RetrySafe(anyhow::anyhow!("{message}")) + } } } @@ -1892,6 +1887,9 @@ mod disposition_tests { workspace_root: PathBuf::from("/tmp/coven-github-test"), timeout_secs: 30, max_retries: 1, + backend: coven_github_config::WorkerBackendKind::Host, + container: coven_github_config::ContainerConfig::default(), + allow_host_backend: false, }, familiars: vec![], review: coven_github_config::ReviewConfig::default(), @@ -2011,6 +2009,9 @@ mod process_tests { // The timeout test overrides this to a short value on purpose. timeout_secs: 30, max_retries, + backend: coven_github_config::WorkerBackendKind::Host, + container: coven_github_config::ContainerConfig::default(), + allow_host_backend: false, }, familiars: vec![FamiliarConfig { id: "cody".to_string(), @@ -2055,11 +2056,10 @@ mod process_tests { // This test specifically exercises the kill-on-timeout path. config.worker.timeout_secs = 1; let brief_path = root.join("session-brief.json"); - let result_path = root.join("result.json"); fs::write(&brief_path, "{}").expect("brief should be written"); let started = Instant::now(); - let attempt = run_coven_code(&config, &brief_path, &result_path, "test-token").await; + let attempt = run_coven_code(&config, &root, "task-timeout", "test-token").await; assert!(matches!(attempt, Attempt::RetrySafe(_))); assert!( @@ -2077,10 +2077,9 @@ mod process_tests { let (root, path) = scratch("exit0", &script); let config = test_config(path, root.clone(), 0); let brief = root.join("session-brief.json"); - let result = root.join("result.json"); fs::write(&brief, "{}").unwrap(); - let attempt = run_coven_code(&config, &brief, &result, "tok").await; + let attempt = run_coven_code(&config, &root, "task-x", "tok").await; match attempt { Attempt::Completed(r) => assert_eq!(r.status, SessionStatus::Success), Attempt::RetrySafe(e) => panic!("expected Completed, got RetrySafe: {e:#}"), @@ -2095,10 +2094,9 @@ mod process_tests { let (root, path) = scratch("exit3", &script); let config = test_config(path, root.clone(), 0); let brief = root.join("session-brief.json"); - let result = root.join("result.json"); fs::write(&brief, "{}").unwrap(); - let attempt = run_coven_code(&config, &brief, &result, "tok").await; + let attempt = run_coven_code(&config, &root, "task-x", "tok").await; match attempt { Attempt::Completed(r) => assert_eq!(r.status, SessionStatus::NeedsInput), Attempt::RetrySafe(e) => panic!("exit 3 must be terminal, got RetrySafe: {e:#}"), @@ -2111,10 +2109,9 @@ mod process_tests { let (root, path) = scratch("exit2", "#!/usr/bin/env bash\nexit 2\n"); let config = test_config(path, root.clone(), 0); let brief = root.join("session-brief.json"); - let result = root.join("result.json"); fs::write(&brief, "{}").unwrap(); - let attempt = run_coven_code(&config, &brief, &result, "tok").await; + let attempt = run_coven_code(&config, &root, "task-x", "tok").await; assert!(matches!(attempt, Attempt::RetrySafe(_))); let _ = fs::remove_dir_all(root); } @@ -2130,13 +2127,12 @@ mod process_tests { let (root, path) = scratch("exit1", &script); let config = test_config(path, root.clone(), 2); // budget of 2 retries let brief = root.join("session-brief.json"); - let result = root.join("result.json"); fs::write(&brief, "{}").unwrap(); let session = run_session_with_backoff( &config, - &brief, - &result, + &root, + "task-x", "tok", config.worker.max_retries, Duration::from_millis(1), @@ -2163,13 +2159,12 @@ mod process_tests { let (root, path) = scratch("exit2-retries", &script); let config = test_config(path, root.clone(), 2); let brief = root.join("session-brief.json"); - let result = root.join("result.json"); fs::write(&brief, "{}").unwrap(); let err = run_session_with_backoff( &config, - &brief, - &result, + &root, + "task-x", "tok", config.worker.max_retries, Duration::from_millis(1), @@ -2193,10 +2188,9 @@ mod process_tests { let (root, path) = scratch("exit0-noresult", "#!/usr/bin/env bash\nexit 0\n"); let config = test_config(path, root.clone(), 0); let brief = root.join("session-brief.json"); - let result = root.join("result.json"); fs::write(&brief, "{}").unwrap(); - let attempt = run_coven_code(&config, &brief, &result, "tok").await; + let attempt = run_coven_code(&config, &root, "task-x", "tok").await; assert!(matches!(attempt, Attempt::RetrySafe(_))); let _ = fs::remove_dir_all(root); } @@ -2294,6 +2288,9 @@ exit 0 workspace_root: root.clone(), timeout_secs: 30, max_retries: 0, + backend: coven_github_config::WorkerBackendKind::Host, + container: coven_github_config::ContainerConfig::default(), + allow_host_backend: false, }, familiars: vec![familiar.clone()], review: coven_github_config::ReviewConfig::default(), @@ -2518,6 +2515,9 @@ exit 0 workspace_root: root.clone(), timeout_secs: 30, max_retries: 0, + backend: coven_github_config::WorkerBackendKind::Host, + container: coven_github_config::ContainerConfig::default(), + allow_host_backend: false, }, familiars: vec![FamiliarConfig { id: "cody".to_string(), @@ -2654,6 +2654,9 @@ mod command_and_marker_tests { workspace_root: PathBuf::from("/nonexistent/workspaces"), timeout_secs: 1, max_retries: 0, + backend: coven_github_config::WorkerBackendKind::Host, + container: coven_github_config::ContainerConfig::default(), + allow_host_backend: false, }, familiars: vec![FamiliarConfig { id: "cody".to_string(), @@ -3119,6 +3122,9 @@ mod publication_gate_tests { workspace_root: root.clone(), timeout_secs: 30, max_retries: 0, + backend: coven_github_config::WorkerBackendKind::Host, + container: coven_github_config::ContainerConfig::default(), + allow_host_backend: false, }, familiars: vec![FamiliarConfig { id: "cody".to_string(), @@ -3157,6 +3163,12 @@ mod publication_gate_tests { .await .expect("review must publish cleanly"); + // Success path: the per-task workspace is destroyed (issue #5). + assert!( + !root.join("task-gates").exists(), + "workspace must be removed after a successful run" + ); + let requests = server.received_requests().await.expect("requests"); let _ = fs::remove_dir_all(root); requests @@ -3251,3 +3263,281 @@ mod publication_gate_tests { ); } } + +#[cfg(test)] +mod container_backend_tests { + //! Container backend behavior through a fake docker CLI (issue #5): the + //! isolation argv is used, the token travels by environment only, the + //! result copies out of the bind-mounted workspace, and timeout kills + //! the container by name. + use super::*; + use coven_github_config::{ContainerConfig, WorkerBackendKind}; + use std::fs; + use std::os::unix::fs::PermissionsExt; + use std::path::PathBuf; + + /// Fake docker: records argv + the forwarded token env, then emulates a + /// session by writing result.json into the `-v host:/workspace` mount. + const FAKE_DOCKER: &str = r#"#!/usr/bin/env bash +dir="$(dirname "$0")" +printf '%s\n' "$*" >> "$dir/docker-invocations.log" +printf '%s\n' "${COVEN_GIT_TOKEN:-unset}" >> "$dir/docker-env.log" +if [ "$1" = "kill" ]; then exit 0; fi +host="" +prev="" +for arg in "$@"; do + if [ "$prev" = "-v" ]; then host="${arg%%:*}"; fi + prev="$arg" +done +cat > "$host/result.json" <<'RESULT' +{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"containerized run","pr_body":"","review":{"mode":"none","evidence_status":"not_applicable","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[]},"exit_reason":null,"memory_used":null} +RESULT +exit 0 +"#; + + fn scratch(name: &str, script: &str) -> (PathBuf, PathBuf) { + let root = std::env::temp_dir().join(format!("coven-container-{name}-{}", uuid::Uuid::new_v4())); + fs::create_dir_all(&root).expect("dir"); + let bin = root.join("fake-docker.sh"); + fs::write(&bin, script).expect("script"); + fs::set_permissions(&bin, fs::Permissions::from_mode(0o755)).expect("chmod"); + (root, bin) + } + + fn container_config(docker_bin: PathBuf, workspace_root: PathBuf) -> Config { + let mut config = test_container_base(workspace_root); + config.worker.backend = WorkerBackendKind::Container; + config.worker.container = ContainerConfig { + docker_bin, + ..ContainerConfig::default() + }; + config + } + + fn test_container_base(workspace_root: PathBuf) -> Config { + use coven_github_config::{GitHubAppConfig, ServerConfig, WorkerConfig}; + Config { + server: ServerConfig { + bind: "127.0.0.1:0".to_string(), + cave_base_url: None, + }, + github: GitHubAppConfig { + app_id: 1, + private_key_path: PathBuf::from("private.pem"), + webhook_secret: "secret".to_string(), + api_base_url: None, + }, + worker: WorkerConfig { + concurrency: 1, + coven_code_bin: PathBuf::from("/nonexistent/never-used-in-container-mode"), + workspace_root, + timeout_secs: 30, + max_retries: 0, + backend: WorkerBackendKind::Host, + container: ContainerConfig::default(), + allow_host_backend: false, + }, + familiars: vec![], + review: coven_github_config::ReviewConfig::default(), + storage: coven_github_config::StorageConfig::default(), + memory: coven_github_config::MemoryConfig::default(), + api: coven_github_config::ApiConfig::default(), + installations: vec![], + } + } + + #[tokio::test] + async fn container_run_uses_the_isolation_profile_and_env_only_token() { + let (root, docker) = scratch("run", FAKE_DOCKER); + let workspace = root.join("ws"); + fs::create_dir_all(&workspace).expect("ws"); + let config = container_config(docker, root.clone()); + + let attempt = run_coven_code(&config, &workspace, "task-c1", "secret-git-token").await; + match attempt { + Attempt::Completed(result) => assert_eq!(result.summary, "containerized run"), + Attempt::RetrySafe(e) => panic!("expected completion, got {e:#}"), + } + + let argv = fs::read_to_string(root.join("docker-invocations.log")).expect("argv log"); + assert!(argv.contains("--read-only"), "hardened profile used: {argv}"); + assert!(argv.contains("--cap-drop ALL"), "{argv}"); + assert!(argv.contains("--memory 2g"), "{argv}"); + assert!( + !argv.contains("secret-git-token"), + "the git token must never appear in docker argv: {argv}" + ); + let env = fs::read_to_string(root.join("docker-env.log")).expect("env log"); + assert!( + env.contains("secret-git-token"), + "the token must reach the CLI environment for -e forwarding: {env}" + ); + let _ = fs::remove_dir_all(root); + } + + #[tokio::test] + async fn container_timeout_kills_the_container_by_name() { + // This fake docker never writes a result and sleeps past the timeout. + let sleeper = r#"#!/usr/bin/env bash +dir="$(dirname "$0")" +printf '%s\n' "$*" >> "$dir/docker-invocations.log" +if [ "$1" = "kill" ]; then exit 0; fi +sleep 5 +"#; + let (root, docker) = scratch("timeout", sleeper); + let workspace = root.join("ws"); + fs::create_dir_all(&workspace).expect("ws"); + let mut config = container_config(docker, root.clone()); + config.worker.timeout_secs = 1; + + let started = std::time::Instant::now(); + let attempt = run_coven_code(&config, &workspace, "task-c2", "tok").await; + assert!(matches!(attempt, Attempt::RetrySafe(_))); + assert!(started.elapsed().as_secs() < 4, "must stop near the timeout"); + + let argv = fs::read_to_string(root.join("docker-invocations.log")).expect("argv log"); + assert!( + argv.lines().any(|l| l.starts_with("kill coven-task-task-c2-")), + "the container must be killed by name after timeout: {argv}" + ); + let _ = fs::remove_dir_all(root); + } +} + +#[cfg(test)] +mod cleanup_tests { + //! Workspace teardown on the failure path (issue #5). The success path is + //! asserted inside `publication_gate_tests::run_review`. + use super::*; + use coven_github_api::installation::TokenRole; + use coven_github_config::{FamiliarConfig, GitHubAppConfig, ServerConfig, WorkerConfig}; + use std::collections::HashMap; + use std::fs; + use std::os::unix::fs::PermissionsExt; + use std::path::PathBuf; + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + #[tokio::test] + async fn workspace_is_destroyed_when_the_session_fails() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo")) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(serde_json::json!({"default_branch": "main"})), + ) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/branches/main")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "commit": { "sha": "abc123" } + }))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/repos/OpenCoven/demo/check-runs")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 7}))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path("/repos/OpenCoven/demo/check-runs/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({}))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/issues/42/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/repos/OpenCoven/demo/issues/42/comments")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 1}))) + .mount(&server) + .await; + + // Infra failure (exit 2) with a zero retry budget: the session errors. + let root = + std::env::temp_dir().join(format!("coven-cleanup-{}", uuid::Uuid::new_v4())); + fs::create_dir_all(&root).expect("dir"); + let script = root.join("fake-coven-code.sh"); + fs::write(&script, "#!/usr/bin/env bash\nexit 2\n").expect("script"); + fs::set_permissions(&script, fs::Permissions::from_mode(0o755)).expect("chmod"); + + let config = Config { + server: ServerConfig { + bind: "127.0.0.1:0".to_string(), + cave_base_url: None, + }, + github: GitHubAppConfig { + app_id: 1, + private_key_path: PathBuf::from("/nonexistent/never-read.pem"), + webhook_secret: "secret".to_string(), + api_base_url: Some(server.uri()), + }, + worker: WorkerConfig { + concurrency: 1, + coven_code_bin: script, + workspace_root: root.clone(), + timeout_secs: 30, + max_retries: 0, + backend: coven_github_config::WorkerBackendKind::Host, + container: coven_github_config::ContainerConfig::default(), + allow_host_backend: false, + }, + familiars: vec![FamiliarConfig { + id: "cody".to_string(), + display_name: "Cody".to_string(), + bot_username: "coven-cody[bot]".to_string(), + model: None, + skills: vec![], + trigger_labels: vec![], + }], + review: coven_github_config::ReviewConfig::default(), + storage: coven_github_config::StorageConfig::default(), + memory: coven_github_config::MemoryConfig::default(), + api: coven_github_config::ApiConfig::default(), + installations: vec![], + }; + let task = Task { + id: "task-cleanup".to_string(), + installation_id: 1, + repo_owner: "OpenCoven".to_string(), + repo_name: "demo".to_string(), + familiar_id: "cody".to_string(), + commander: None, + kind: TaskKind::FixIssue { + issue_number: 42, + issue_title: "t".to_string(), + issue_body: "b".to_string(), + }, + }; + let minter = Minter::Fixed(HashMap::from([ + ( + TokenRole::Orchestration, + "ghs_orchestration0000000000000000000000".to_string(), + ), + ( + TokenRole::AgentGit, + "ghs_agentgit000000000000000000000000000".to_string(), + ), + ])); + + let outcome = execute_task_with_minter( + &config, + Store::open_in_memory().expect("store"), + task, + &minter, + ) + .await; + assert!(outcome.is_ok(), "failure is handled, not propagated: {outcome:?}"); + + // The workspace — which held the brief and any partial clone — is gone. + assert!( + !root.join("task-cleanup").exists(), + "workspace must be removed after a failed run" + ); + let _ = fs::remove_dir_all(root); + } +} diff --git a/docs/container-isolation.md b/docs/container-isolation.md index 20975d4..f08353c 100644 --- a/docs/container-isolation.md +++ b/docs/container-isolation.md @@ -56,8 +56,43 @@ Recommended baseline: Recommended order: -1. Enforce process timeouts for all local workers. -2. Persist task state before and after each worker phase. -3. Add Docker-based worker backend behind the current worker interface. -4. Add resource limits and workspace cleanup tests. +1. Enforce process timeouts for all local workers. ✅ +2. Persist task state before and after each worker phase. ✅ (#2) +3. Add Docker-based worker backend behind the current worker interface. ✅ (#5) +4. Add resource limits and workspace cleanup tests. ✅ (#5) 5. Add a dedicated worker pool tier for paid hosted customers. + +## Operator Configuration (issue #5) + +The worker backend is configured in `[worker]`: + +```toml +[worker] +backend = "container" # "host" (dev) or "container" (hosted) +# allow_host_backend = true # explicit opt-in to host execution when + # [[installations]] are configured + +[worker.container] +image = "ghcr.io/opencoven/coven-code:latest" +docker_bin = "docker" # any docker-compatible CLI: podman, nerdctl +cpus = 1.0 +memory = "2g" +pids = 512 +tmpfs_size = "256m" +network = "bridge" # "none" for full egress denial +``` + +Each task attempt runs `docker run --rm` with a fresh, uniquely named +container: read-only rootfs, `--cap-drop ALL`, `no-new-privileges`, a +bounded `/tmp` tmpfs, and only the task workspace bind-mounted. Git +authority is forwarded by environment variable *name* (`-e COVEN_GIT_TOKEN`) +so the token never appears in argv or `docker inspect`. On timeout the +adapter kills both the CLI process and the container by name; `--rm` tears +the container down on every path, and the host workspace is deleted after +success and failure alike. + +**Minimum supported backends:** any docker-CLI-compatible runtime (Docker +Engine, Podman, nerdctl/containerd). The `host` backend remains supported +for development and trusted self-hosting; once `[[installations]]` blocks +exist (the hosted posture), `doctor` refuses `host` unless +`worker.allow_host_backend = true` is set deliberately.