diff --git a/Cargo.toml b/Cargo.toml index a780e9340..5f7c4bc01 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,7 +31,10 @@ tracing = "0.1" # for `DurabilityMode::Async` background checkpoint writes, `spawn_blocking` # file I/O, and `block_in_place` JSONL task-store writes (which requires the # multi-threaded runtime feature). -tokio = { version = "1", default-features = false, features = ["sync", "time", "macros", "rt", "rt-multi-thread"] } +# `fs` is required by `harness::artifacts`, which writes offloaded worker +# artifacts to disk. It enables more of tokio, not another package, so the +# kernel-floor package count is unaffected. +tokio = { version = "1", default-features = false, features = ["sync", "time", "macros", "rt", "rt-multi-thread", "fs"] } # HTTP client used by hosted providers and the embedded Langfuse exporter. The # `stream` feature enables `Response::bytes_stream` for Server-Sent-Events diff --git a/src/harness/artifacts/mod.rs b/src/harness/artifacts/mod.rs new file mode 100644 index 000000000..ea0a868db --- /dev/null +++ b/src/harness/artifacts/mod.rs @@ -0,0 +1,80 @@ +//! Filesystem offload for oversized worker artifacts on long-horizon runs. +//! +//! ## The problem +//! +//! For minutes-to-hours runs, keeping compressed results *in context* still +//! accumulates summary text step after step, and it can never restore full +//! fidelity. Summarising one oversized payload at a time does not stop the +//! aggregate from growing. +//! +//! ## The convention +//! +//! Two directories under the agent's artifact root: +//! +//! | Directory | Holds | +//! | ------------- | -------------------------------------------------------- | +//! | `outputs/` | Deliverables. Handed between steps **by path**. | +//! | `workspace/` | Scratch. Intermediate files not meant to be handed back. | +//! +//! A worker that produces a large result writes it to `outputs/` and returns the +//! path plus a short abstract. Context stays lean and the full artifact is +//! recoverable with an ordinary file read. +//! +//! Two halves enforce it, and the split is the point: +//! +//! * **Prompt** — a host renders an offload contract into its worker prompts, so +//! workers offload on purpose. That half is **host-owned**: it names host tools +//! and is host prompt text, so it is deliberately not in this crate. +//! * **Harness** — [`offload_oversized_result`] runs on every worker outcome, so +//! an oversized result is offloaded even when the worker inlined it anyway. +//! That half needs no cooperation from the model, which is exactly why it +//! exists. +//! +//! Whatever summarisation or truncation backstop the host already has stays +//! exactly as it is: it is the fallback for anything this convention does not +//! catch, and for every failure mode here — a refused path, a full disk — the +//! caller keeps its inline payload and falls through to it. +//! +//! ## Hardening +//! +//! [`resolve_artifact_path`] is fail-closed. Absolute paths, `..` traversal, and +//! anything escaping the convention root are refused; when an +//! [`ArtifactPathPolicy`] is supplied, so is anything reaching the host's +//! internal state. +//! +//! The lexical checks cannot see symlinks, because the target does not exist +//! when they run. So the real parent directory is re-validated after +//! `create_dir_all` and before the write — that is the first moment the +//! link-resolved location can be checked at all. +//! +//! ## What the host supplies +//! +//! Two policies, both of which a redistributed crate cannot decide for itself: +//! [`ArtifactPathPolicy`] (which paths are off limits) and [`ArtifactRedactor`] +//! (what is scrubbed before bytes hit disk). See [`policy`]. +//! +//! ## Logging +//! +//! Every write emits `[artifact] wrote worker artifact under the artifact root`, +//! and every path a handoff carries emits `[artifact] handoff carried an +//! artifact path`, so a run journal shows both ends of a pointer. + +mod ops; +mod paths; +pub mod policy; +mod types; + +pub use ops::{ + ArtifactOffload, HANDOFF_STAGE_CONSUMED, HANDOFF_STAGE_RECORDED, build_abstract, + effective_offload_threshold, extract_artifact_paths, note_artifact_handoff, + offload_oversized_result, render_artifact_pointer, should_offload, +}; +pub use paths::{relative_to_root, resolve_artifact_path, sanitize_component}; +pub use policy::{ArtifactPathPolicy, ArtifactRedactor, NoRedaction, OpenPathPolicy, Redacted}; +pub use types::{ + ABSTRACT_BUDGET_CHARS, ARTIFACT_POINTER_PREFIX, ArtifactKind, DEFAULT_OFFLOAD_THRESHOLD_BYTES, + OUTPUTS_DIR, OffloadError, OffloadedArtifact, SCRATCH_DIR, +}; + +#[cfg(test)] +mod test; diff --git a/src/harness/artifacts/ops.rs b/src/harness/artifacts/ops.rs new file mode 100644 index 000000000..2f58feb68 --- /dev/null +++ b/src/harness/artifacts/ops.rs @@ -0,0 +1,407 @@ +//! Write, pointer-render, and handoff plumbing for the artifact-offload +//! convention. + +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use super::paths::{relative_to_root, resolve_artifact_path, sanitize_component}; +use super::policy::{ArtifactPathPolicy, ArtifactRedactor}; +use super::types::{ + ABSTRACT_BUDGET_CHARS, ARTIFACT_POINTER_PREFIX, ArtifactKind, OffloadError, OffloadedArtifact, +}; + +/// Whether a payload of `bytes` should be offloaded at `threshold_bytes`. +/// +/// A zero threshold disables offload entirely, so a run can opt out without +/// threading an `Option` through every call site. +pub fn should_offload(bytes: usize, threshold_bytes: usize) -> bool { + threshold_bytes > 0 && bytes > threshold_bytes +} + +/// Condense `content` to at most `budget_chars` characters for a pointer's +/// abstract. +/// +/// Prefers cutting at a line break, then at a word break, so the reader gets a +/// whole thought rather than a word sliced in half. Falls back to a hard +/// character cut only when neither boundary sits in the back half of the budget. +pub fn build_abstract(content: &str, budget_chars: usize) -> String { + let trimmed = content.trim(); + if budget_chars == 0 { + return String::new(); + } + if trimmed.chars().count() <= budget_chars { + return trimmed.to_string(); + } + + let mut head: String = trimmed.chars().take(budget_chars).collect(); + let floor = head.len() / 2; + if let Some(idx) = head.rfind('\n').filter(|idx| *idx >= floor) { + head.truncate(idx); + } else if let Some(idx) = head.rfind(' ').filter(|idx| *idx >= floor) { + head.truncate(idx); + } + format!("{}...", head.trim_end()) +} + +/// Render the text a worker hands back in place of an offloaded payload. +/// +/// The first line is machine-readable ([`extract_artifact_paths`] parses it); +/// the rest is for the model reading the handoff. +/// +/// `read_tool` names the tool the reader should call. It is a parameter rather +/// than a constant because tool names are host vocabulary — a crate that +/// hard-coded one would put a tool the host may not have into its prompts. +pub fn render_artifact_pointer( + artifact: &OffloadedArtifact, + abstract_text: &str, + read_tool: &str, +) -> String { + let redaction_note = if artifact.redacted { + " Credential/PII redaction was applied before storage." + } else { + "" + }; + format!( + "{ARTIFACT_POINTER_PREFIX} kind={kind} path={path} bytes={bytes}\n\ + read_with: {read_tool} {{\"path\":\"{path}\"}}\n\ + note: The full result was written to the action workspace instead of being inlined. \ + Read the file for complete fidelity; the abstract below is not exhaustive.{redaction_note}\n\n\ + [abstract]\n{abstract_text}", + kind = artifact.kind.as_str(), + path = artifact.relative_path, + bytes = artifact.stored_bytes, + ) +} + +/// Pull every artifact path out of a handoff payload. +/// +/// Scans for [`ARTIFACT_POINTER_PREFIX`] lines and returns their `path=` values +/// in encounter order, de-duplicated. A worker that inlined a pointer by hand — +/// following the prompt contract rather than being offloaded by the harness — is +/// picked up by exactly the same parse. +pub fn extract_artifact_paths(text: &str) -> Vec { + let mut found: Vec = Vec::new(); + for line in text.lines() { + let line = line.trim_start(); + if !line.starts_with(ARTIFACT_POINTER_PREFIX) { + continue; + } + let Some(rest) = line.split(" path=").nth(1) else { + continue; + }; + // Split on the FIRST whitespace rather than `split_whitespace`, which + // skips leading separators: an empty `path=` followed by another field + // would otherwise yield that next field (`bytes=1`) as the "path". + let path = rest.split(char::is_whitespace).next().unwrap_or_default(); + if path.is_empty() || found.iter().any(|existing| existing == path) { + continue; + } + found.push(path.to_string()); + } + found +} + +/// Emit an `[artifact]` reference log for every path crossing a handoff, and +/// return how many were surfaced. +/// +/// `stage` distinguishes the two ends of the same pointer so a run journal can +/// tell them apart instead of showing the identical line twice: the child +/// *recording* paths onto its outcome, and the parent *consuming* them. Use +/// [`HANDOFF_STAGE_RECORDED`] / [`HANDOFF_STAGE_CONSUMED`]. +pub fn note_artifact_handoff( + stage: &str, + agent_id: &str, + task_id: &str, + paths: &[String], +) -> usize { + for path in paths { + tracing::info!( + stage = %stage, + agent_id = %agent_id, + task_id = %task_id, + path = %path, + "[artifact] handoff carried an artifact path" + ); + } + paths.len() +} + +/// Producing side: the child recorded these paths onto its outcome. +pub const HANDOFF_STAGE_RECORDED: &str = "recorded_by_child"; + +/// Consuming side: the parent took delivery of these paths. +pub const HANDOFF_STAGE_CONSUMED: &str = "consumed_by_parent"; + +/// Effective offload threshold for an agent whose definition caps its result at +/// `max_result_chars`. +/// +/// A cap below the default would otherwise truncate the result before offload +/// ever fired, so the artifact would never reach disk. Offloading at the tighter +/// of the two means anything the cap would have cut is on disk first. Chars are +/// treated as a byte budget, which is conservative for multibyte text: it +/// offloads slightly earlier, never later. +pub fn effective_offload_threshold( + default_threshold_bytes: usize, + max_result_chars: Option, +) -> usize { + match max_result_chars { + Some(cap) if cap > 0 => default_threshold_bytes.min(cap), + _ => default_threshold_bytes, + } +} + +/// Per-run writer for the offload convention. +/// +/// Holds the resolved artifact root, the host policies used for the fail-closed +/// path checks and redaction, and the identifiers that name generated artifacts +/// and tag the `[artifact]` logs. +#[derive(Clone)] +pub struct ArtifactOffload { + root: PathBuf, + /// Root the handed-back path is rendered against. Normally identical to + /// `root`; see [`Self::with_render_root`]. + render_root: PathBuf, + policy: Option>, + redactor: Option>, + agent_id: String, + task_id: String, +} + +impl std::fmt::Debug for ArtifactOffload { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ArtifactOffload") + .field("root", &self.root) + .field("render_root", &self.render_root) + .field("policy", &self.policy.as_ref().map(|_| "set")) + .field("redactor", &self.redactor.as_ref().map(|_| "set")) + .field("agent_id", &self.agent_id) + .field("task_id", &self.task_id) + .finish() + } +} + +impl ArtifactOffload { + /// A writer rooted at `root` for the given agent and task. + /// + /// Starts with no path policy and no redactor: add them with + /// [`with_path_policy`](Self::with_path_policy) and + /// [`with_redactor`](Self::with_redactor). A writer with no redactor stores + /// bytes verbatim — see [`ArtifactRedactor`]. + pub fn new(root: PathBuf, agent_id: impl Into, task_id: impl Into) -> Self { + Self { + render_root: root.clone(), + root, + policy: None, + redactor: None, + agent_id: agent_id.into(), + task_id: task_id.into(), + } + } + + /// Consults `policy` for host-internal path refusals. + pub fn with_path_policy(mut self, policy: Arc) -> Self { + self.policy = Some(policy); + self + } + + /// Scrubs bodies through `redactor` before they touch disk. + pub fn with_redactor(mut self, redactor: Arc) -> Self { + self.redactor = Some(redactor); + self + } + + /// Render handed-back paths relative to `render_root` instead of the write + /// root. + /// + /// An isolated worker writes inside its own checkout, but the parent that + /// receives the pointer does not hold that worker's root — a bare + /// `outputs/…` would resolve against the *parent's* root and miss the file + /// entirely. Rendering against the parent's root yields a relative path when + /// the worker's root is nested inside it, and an absolute path when it is + /// not, so the pointer is never silently wrong. + pub fn with_render_root(mut self, render_root: PathBuf) -> Self { + self.render_root = render_root; + self + } + + /// Convention-stable name for a worker's offloaded final result: + /// `/-result.md`, both components sanitized. + pub fn default_result_name(&self) -> String { + format!( + "{}/{}-result.md", + sanitize_component(&self.agent_id), + sanitize_component(&self.task_id) + ) + } + + /// Resolve `relative` under this run's root without writing. + /// + /// Exposed so callers can validate a model-supplied path before acting on it. + pub fn resolve(&self, kind: ArtifactKind, relative: &str) -> Result { + resolve_artifact_path(&self.root, self.policy.as_deref(), kind, relative) + } + + /// Write `content` to `relative` under the convention directory for `kind`. + /// + /// The body passes through the host redactor before it touches disk. + pub async fn write( + &self, + kind: ArtifactKind, + relative: &str, + content: &str, + ) -> Result { + self.write_returning_stored(kind, relative, content) + .await + .map(|(artifact, _stored)| artifact) + } + + /// Same as [`write`](Self::write), but also hands back the **stored** + /// (redacted) body. + /// + /// Callers surfacing any part of the artifact back into a model's context + /// must render it from this value, never from their own input: building a + /// preview from the raw text would re-expose exactly the credentials the + /// redactor just scrubbed out of the file. + pub async fn write_returning_stored( + &self, + kind: ArtifactKind, + relative: &str, + content: &str, + ) -> Result<(OffloadedArtifact, String), OffloadError> { + let absolute = self.resolve(kind, relative)?; + if let Some(parent) = absolute.parent() { + tokio::fs::create_dir_all(parent).await?; + // The checks in `resolve` are lexical, so a pre-existing symlink + // (`outputs -> /elsewhere`, or `outputs/x -> `) would + // still be followed by the write below. Re-validate the parent that + // actually materialised on disk before touching it. + self.assert_real_parent_inside_root(kind, parent).await?; + } + + let stored = match self.redactor.as_deref() { + Some(redactor) => redactor.redact(content), + None => super::policy::Redacted::unchanged(content), + }; + tokio::fs::write(&absolute, stored.text.as_bytes()).await?; + + let relative_path = relative_to_root(&self.render_root, &absolute); + let artifact = OffloadedArtifact { + kind, + relative_path, + absolute_path: absolute, + stored_bytes: stored.text.len(), + original_bytes: content.len(), + redacted: stored.changed, + }; + + tracing::info!( + agent_id = %self.agent_id, + task_id = %self.task_id, + kind = artifact.kind.as_str(), + path = %artifact.relative_path, + original_bytes = artifact.original_bytes, + stored_bytes = artifact.stored_bytes, + redacted = artifact.redacted, + "[artifact] wrote worker artifact under the artifact root" + ); + + Ok((artifact, stored.text)) + } + + /// Resolve `parent` through symlinks and confirm it still sits inside this + /// kind's convention root, and outside the host's internal state. + /// + /// [`resolve_artifact_path`] cannot do this: its target usually does not + /// exist yet, so it is lexical by necessity. Once `create_dir_all` has run + /// the parent *does* exist, which is the first moment the real, + /// link-resolved location can be checked. + async fn assert_real_parent_inside_root( + &self, + kind: ArtifactKind, + parent: &Path, + ) -> Result<(), OffloadError> { + let convention_root = self.root.join(kind.subdir()); + let real_parent = tokio::fs::canonicalize(parent).await?; + // The root itself may be reached through a symlink (macOS `/tmp` -> + // `/private/tmp` is the everyday case), so compare like with like + // rather than against the lexical root. + let real_root = tokio::fs::canonicalize(&convention_root).await?; + if !real_parent.starts_with(&real_root) { + return Err(OffloadError::SymlinkEscape { + path: parent.display().to_string(), + resolved: real_parent.display().to_string(), + }); + } + if let Some(policy) = self.policy.as_deref() { + let inside_internal_root = match policy.internal_root() { + Some(root) => tokio::fs::canonicalize(root) + .await + .map(|resolved| real_parent.starts_with(&resolved)) + .unwrap_or(false), + None => false, + }; + if policy.is_internal_state(&real_parent) || inside_internal_root { + return Err(OffloadError::SymlinkEscape { + path: parent.display().to_string(), + resolved: real_parent.display().to_string(), + }); + } + } + Ok(()) + } + + /// Artifact root this writer is anchored at. + pub fn root(&self) -> &Path { + &self.root + } +} + +/// Offload `output` when it exceeds `threshold_bytes`, returning the text the +/// parent should receive plus the artifact when one was written. +/// +/// This is the deterministic half of the convention: it fires whether or not the +/// worker followed the prompt contract. **Every failure mode is soft** — the +/// caller gets its original payload back, and whatever summarisation or +/// truncation backstop it already had stays in charge. +pub async fn offload_oversized_result( + output: String, + offload: &ArtifactOffload, + threshold_bytes: usize, + read_tool: &str, +) -> (String, Option) { + if !should_offload(output.len(), threshold_bytes) { + return (output, None); + } + + let name = offload.default_result_name(); + match offload + .write_returning_stored(ArtifactKind::Output, &name, &output) + .await + { + Ok((artifact, stored)) => { + // Build the abstract from the STORED (redacted) body, never from + // `output`. The pointer goes straight into the parent's context, so + // rendering it from the raw text would re-expose the very + // credentials `write` just scrubbed out of the file. + let abstract_text = build_abstract(&stored, ABSTRACT_BUDGET_CHARS); + let pointer = render_artifact_pointer(&artifact, &abstract_text, read_tool); + tracing::info!( + path = %artifact.relative_path, + inline_bytes = output.len(), + pointer_bytes = pointer.len(), + threshold_bytes, + "[artifact] replaced oversized worker result with a path + abstract" + ); + (pointer, Some(artifact)) + } + Err(err) => { + tracing::warn!( + error = %err, + inline_bytes = output.len(), + threshold_bytes, + "[artifact] offload refused; keeping the inline result (host backstop applies)" + ); + (output, None) + } + } +} diff --git a/src/harness/artifacts/paths.rs b/src/harness/artifacts/paths.rs new file mode 100644 index 000000000..9c55d8815 --- /dev/null +++ b/src/harness/artifacts/paths.rs @@ -0,0 +1,157 @@ +//! Path hardening for the artifact-offload convention. +//! +//! Everything an agent offloads resolves under +//! `/`. The resolver is deliberately +//! fail-closed: it rejects absolute paths, `..` traversal, anything landing +//! outside its convention root after lexical normalization, and — when the host +//! supplies an [`ArtifactPathPolicy`] — anything reaching the host's internal +//! state. + +use std::path::{Component, Path, PathBuf}; + +use super::policy::ArtifactPathPolicy; +use super::types::{ArtifactKind, OffloadError}; + +/// Maximum characters kept from a single path component when deriving a name +/// from an agent id / task id. Keeps generated names well inside filesystem +/// limits on every supported platform. +const MAX_COMPONENT_CHARS: usize = 80; + +/// Reduce an arbitrary string to a safe single path component. +/// +/// Anything that is not ASCII alphanumeric, `-`, or `_` becomes `_`, so a task +/// id like `sub-1a/2b` — or an agent id carrying a path separator — can never +/// introduce a directory level of its own. +pub fn sanitize_component(value: &str) -> String { + let mut out = String::with_capacity(value.len().min(MAX_COMPONENT_CHARS)); + for ch in value.chars().take(MAX_COMPONENT_CHARS) { + if ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' { + out.push(ch); + } else { + out.push('_'); + } + } + if out.is_empty() { + "unknown".to_string() + } else { + out + } +} + +/// Lexically normalize `path` by dropping `.` components and popping a +/// directory for each `..`. +/// +/// Purely lexical on purpose: the target usually does not exist yet, so +/// `canonicalize` is unavailable. [`resolve_artifact_path`] rejects `..` +/// outright before calling this; the popping here is defence in depth for any +/// future caller. +fn normalize_lexically(path: &Path) -> PathBuf { + let mut out = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => { + out.pop(); + } + other => out.push(other.as_os_str()), + } + } + out +} + +/// Render `absolute` as a `root`-relative, `/`-separated path, so the string can +/// be pasted straight into a file-read call on any platform. +/// +/// Falls back to the lossy display form when `absolute` is somehow not under +/// `root` — unreachable via [`resolve_artifact_path`], which validates +/// containment first, but reachable when a caller renders against a *different* +/// root than it wrote to (see `ArtifactOffload::with_render_root`). Falling back +/// to an absolute path is correct there: a wrong relative path would silently +/// resolve against the reader's own root and miss the file. +pub fn relative_to_root(root: &Path, absolute: &Path) -> String { + match absolute.strip_prefix(root) { + Ok(rel) => rel + .components() + .map(|c| c.as_os_str().to_string_lossy().to_string()) + .collect::>() + .join("/"), + Err(_) => absolute.to_string_lossy().to_string(), + } +} + +/// Resolve `relative` into an absolute offload target under +/// `root/`, or explain why it is refused. +/// +/// When `policy` is supplied the resolved path is additionally checked against +/// the host's internal state: +/// +/// * anything [`ArtifactPathPolicy::is_internal_state`] flags is refused with +/// [`OffloadError::InternalState`]; +/// * anything under [`ArtifactPathPolicy::internal_root`] is refused with +/// [`OffloadError::InternalRoot`]. +/// +/// The specific check runs **first** so its more precise error survives when a +/// host has configured the artifact root inside its internal root — reporting +/// the blanket containment failure there would lose the useful detail. +/// +/// Passing `policy: None` skips only those two checks; traversal and +/// containment always run. +pub fn resolve_artifact_path( + root: &Path, + policy: Option<&dyn ArtifactPathPolicy>, + kind: ArtifactKind, + relative: &str, +) -> Result { + let trimmed = relative.trim(); + if trimmed.is_empty() { + return Err(OffloadError::EmptyName); + } + + let requested = Path::new(trimmed); + let convention_root = root.join(kind.subdir()); + for component in requested.components() { + match component { + Component::Normal(_) | Component::CurDir => {} + Component::ParentDir => { + return Err(OffloadError::PathEscape { + root: convention_root.display().to_string(), + path: trimmed.to_string(), + }); + } + Component::RootDir | Component::Prefix(_) => { + return Err(OffloadError::AbsolutePath { + path: trimmed.to_string(), + }); + } + } + } + + let candidate = normalize_lexically(&convention_root.join(requested)); + // `normalize_lexically` cannot climb above the root given the `..` + // rejection above, but assert containment anyway so a future relaxation of + // that rejection cannot silently widen the write surface. + if !candidate.starts_with(&convention_root) { + return Err(OffloadError::PathEscape { + root: convention_root.display().to_string(), + path: candidate.display().to_string(), + }); + } + + if let Some(policy) = policy { + if policy.is_internal_state(&candidate) { + return Err(OffloadError::InternalState { + path: candidate.display().to_string(), + }); + } + if policy + .internal_root() + .is_some_and(|internal_root| candidate.starts_with(internal_root)) + { + return Err(OffloadError::InternalRoot { + path: candidate.display().to_string(), + }); + } + } + + Ok(candidate) +} diff --git a/src/harness/artifacts/policy.rs b/src/harness/artifacts/policy.rs new file mode 100644 index 000000000..2b0287084 --- /dev/null +++ b/src/harness/artifacts/policy.rs @@ -0,0 +1,160 @@ +//! The two host policies an artifact write consults. +//! +//! Offloading a worker's result to disk is generic mechanics — thresholds, +//! path resolution, pointer rendering. Two decisions inside it are emphatically +//! not, and neither can be made correctly by a redistributed crate: +//! +//! * **Which paths are off limits.** A host keeps internal state somewhere and +//! an agent write must never land in it. Only the host knows where that is. +//! * **What must be scrubbed before bytes touch disk.** Credential and PII +//! patterns are a host's compliance surface, not a library's. +//! +//! Both are therefore *gates the runtime calls*, never behaviour the runtime is +//! trusted to have performed — RFC §2 rule 5. + +use std::fmt; +use std::path::Path; + +// ── ArtifactPathPolicy ──────────────────────────────────────────────────────── + +/// Host policy: which resolved paths an artifact write must refuse. +/// +/// The crate already refuses absolute paths, `..` traversal and anything +/// escaping the artifact root — those are containment rules it can evaluate on +/// its own. This trait covers the rule it cannot: a host's *internal state* +/// location, which has no meaning here. +/// +/// Both methods are consulted, and they are separate because they fail for +/// different reasons and a host wants to tell them apart in a log: a path may +/// sit under the internal root wholesale, or be a specific state location that +/// happens to live elsewhere. +pub trait ArtifactPathPolicy: Send + Sync + fmt::Debug { + /// Whether `path` is a host-internal state location that an agent write may + /// never reach. + /// + /// Evaluated on a lexically-resolved path before the write, and again on the + /// real, symlink-resolved parent directory afterwards. An implementation + /// must therefore be **pure and cheap** — it is called on a hot path and its + /// answer must not depend on when it was asked. + fn is_internal_state(&self, path: &Path) -> bool; + + /// Root of the host's internal state, when it has a single one. + /// + /// Anything under this is refused outright, independently of + /// [`is_internal_state`](Self::is_internal_state). Returning `None` skips + /// only that containment check; the crate's own traversal and root checks + /// always run. + fn internal_root(&self) -> Option<&Path>; +} + +// ── ArtifactRedactor ────────────────────────────────────────────────────────── + +/// The result of running a host's redactor over an artifact body. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Redacted { + /// The body as it should be stored. This — never the caller's input — is + /// what gets written and what any preview must be rendered from. + pub text: String, + /// Whether the redactor rewrote anything. + pub changed: bool, +} + +impl Redacted { + /// A body the redactor left alone. + pub fn unchanged(text: impl Into) -> Self { + Self { + text: text.into(), + changed: false, + } + } + + /// A body the redactor rewrote. + pub fn rewritten(text: impl Into) -> Self { + Self { + text: text.into(), + changed: true, + } + } +} + +/// Host policy: credential/PII scrubbing applied before an artifact is stored. +/// +/// # This is not optional in the way the sinks are +/// +/// A `None` redactor means **bytes are written exactly as the agent produced +/// them**. That is a legitimate configuration for a host with no secrets in +/// play, and it is the honest default for a crate that cannot know a host's +/// patterns — but it is a security decision, not an absence of one. Hosts +/// handling credentials must supply an implementation. +/// +/// # The stored body is the only safe source for a preview +/// +/// [`redact`](Self::redact) returns the text to store, and callers rendering any +/// part of an artifact back into a model's context must render it from that +/// value. Building a preview from the original input would re-expose precisely +/// the credentials this trait just removed from the file — the failure is +/// silent, because the file on disk looks correctly scrubbed. +pub trait ArtifactRedactor: Send + Sync + fmt::Debug { + /// Scrubs `content` for storage. + fn redact(&self, content: &str) -> Redacted; +} + +// ── NoRedaction ─────────────────────────────────────────────────────────────── + +/// A redactor that stores content verbatim. +/// +/// For tests, and for hosts that genuinely have nothing to scrub. Prefer +/// passing `None` in production code so the choice is visible at the call site +/// rather than hidden behind a type name that reads like a policy. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct NoRedaction; + +impl ArtifactRedactor for NoRedaction { + fn redact(&self, content: &str) -> Redacted { + Redacted::unchanged(content) + } +} + +// ── OpenPathPolicy ──────────────────────────────────────────────────────────── + +/// A path policy that forbids nothing beyond the crate's own containment rules. +/// +/// For tests and for hosts with no internal state under the artifact root. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct OpenPathPolicy; + +impl ArtifactPathPolicy for OpenPathPolicy { + fn is_internal_state(&self, _path: &Path) -> bool { + false + } + + fn internal_root(&self) -> Option<&Path> { + None + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn no_redaction_reports_the_body_unchanged() { + let out = NoRedaction.redact("hunter2"); + assert_eq!(out.text, "hunter2"); + // If this ever reported `changed`, every pointer would carry a + // redaction note for a body nothing touched. + assert!(!out.changed); + } + + #[test] + fn open_policy_forbids_nothing() { + assert!(!OpenPathPolicy.is_internal_state(Path::new("/anywhere"))); + assert_eq!(OpenPathPolicy.internal_root(), None); + } + + #[test] + fn redacted_constructors_set_the_changed_flag() { + assert!(!Redacted::unchanged("a").changed); + assert!(Redacted::rewritten("b").changed); + } +} diff --git a/src/harness/artifacts/test.rs b/src/harness/artifacts/test.rs new file mode 100644 index 000000000..651ffbfbd --- /dev/null +++ b/src/harness/artifacts/test.rs @@ -0,0 +1,648 @@ +//! Tests for the artifact-offload convention. +//! +//! Covers the happy path (oversized result lands in `outputs/`, the parent gets +//! a path + abstract), the fallback path (offload refused, inline payload +//! survives for the host's backstop), and the fail-closed path hardening that +//! keeps offload inside the artifact root and out of host-internal state. + +use std::path::{Path, PathBuf}; +use std::sync::Arc; + +use super::*; + +// ── Test policies ───────────────────────────────────────────────────────────── + +/// Stands in for a host security policy: an internal state root plus a rule +/// naming specific internal state locations. +/// +/// Mirrors the real shape — a host has both a root it owns wholesale and +/// individual state paths that may sit elsewhere — because the two are checked +/// separately and produce different errors. +#[derive(Debug)] +struct TestPolicy { + internal_root: PathBuf, + /// Path suffixes treated as internal state wherever they appear. + internal_suffixes: Vec, +} + +impl TestPolicy { + fn rooted_at(internal_root: PathBuf) -> Self { + Self { + internal_root, + internal_suffixes: Vec::new(), + } + } + + fn with_internal_suffix(mut self, suffix: &str) -> Self { + self.internal_suffixes.push(suffix.to_string()); + self + } +} + +impl ArtifactPathPolicy for TestPolicy { + fn is_internal_state(&self, path: &Path) -> bool { + let shown = path.to_string_lossy().replace('\\', "/"); + self.internal_suffixes + .iter() + .any(|suffix| shown.contains(suffix)) + } + + fn internal_root(&self) -> Option<&Path> { + Some(&self.internal_root) + } +} + +/// Stands in for a host credential scrubber. +#[derive(Debug)] +struct TestRedactor; + +const SECRET: &str = "sk-live-0123456789abcdef"; +const REDACTION: &str = "[redacted]"; + +impl ArtifactRedactor for TestRedactor { + fn redact(&self, content: &str) -> Redacted { + if content.contains(SECRET) { + Redacted::rewritten(content.replace(SECRET, REDACTION)) + } else { + Redacted::unchanged(content) + } + } +} + +const READ_TOOL: &str = "file_read"; + +fn offload_for(root: &Path, internal_root: &Path) -> ArtifactOffload { + ArtifactOffload::new(root.to_path_buf(), "worker-agent", "task-1") + .with_path_policy(Arc::new(TestPolicy::rooted_at(internal_root.to_path_buf()))) + .with_redactor(Arc::new(TestRedactor)) +} + +// ── Kinds and directories ───────────────────────────────────────────────────── + +#[test] +fn kinds_map_to_the_documented_directories() { + assert_eq!(ArtifactKind::Output.subdir(), OUTPUTS_DIR); + assert_eq!(ArtifactKind::Scratch.subdir(), SCRATCH_DIR); + assert_eq!(ArtifactKind::Output.as_str(), "output"); + assert_eq!(ArtifactKind::Scratch.as_str(), "scratch"); +} + +// ── Path resolution ─────────────────────────────────────────────────────────── + +#[test] +fn resolves_under_the_convention_directory() { + let root = PathBuf::from("/action"); + let resolved = + resolve_artifact_path(&root, None, ArtifactKind::Output, "report.md").expect("resolve"); + assert_eq!(resolved, root.join("outputs").join("report.md")); +} + +#[test] +fn scratch_resolves_under_the_artifact_root_not_the_internal_root() { + // The `workspace` subdir of the artifact root and the host's internal root + // are different places that happen to share a word. Conflating them would + // route every scratch write into host state. + let root = PathBuf::from("/action"); + let internal = PathBuf::from("/internal/workspace"); + let policy = TestPolicy::rooted_at(internal.clone()); + let resolved = resolve_artifact_path(&root, Some(&policy), ArtifactKind::Scratch, "notes.md") + .expect("resolve"); + assert_eq!(resolved, root.join("workspace").join("notes.md")); + assert!(!resolved.starts_with(&internal)); +} + +#[test] +fn rejects_parent_traversal() { + let err = resolve_artifact_path( + Path::new("/action"), + None, + ArtifactKind::Output, + "../../etc/passwd", + ) + .expect_err("traversal must be refused"); + assert!( + matches!(err, OffloadError::PathEscape { .. }), + "got {err:?}" + ); +} + +#[test] +fn rejects_absolute_paths() { + let err = resolve_artifact_path( + Path::new("/action"), + None, + ArtifactKind::Output, + "/etc/passwd", + ) + .expect_err("absolute path must be refused"); + assert!( + matches!(err, OffloadError::AbsolutePath { .. }), + "got {err:?}" + ); +} + +#[test] +fn rejects_empty_and_whitespace_names() { + for name in ["", " ", "\t\n"] { + let err = resolve_artifact_path(Path::new("/action"), None, ArtifactKind::Output, name) + .expect_err("blank name must be refused"); + assert!(matches!(err, OffloadError::EmptyName), "got {err:?}"); + } +} + +#[test] +fn accepts_leading_current_dir_segments() { + // `./report.md` is how a model commonly writes a relative path; refusing it + // would reject a correct request on a formatting technicality. + let resolved = resolve_artifact_path( + Path::new("/action"), + None, + ArtifactKind::Output, + "./report.md", + ) + .expect("resolve"); + assert_eq!(resolved, Path::new("/action/outputs/report.md")); +} + +#[test] +fn rejects_targets_inside_the_internal_root_fail_closed() { + // An artifact root configured inside the host's internal root: containment + // passes, so only the policy check stands between an agent and host state. + let root = PathBuf::from("/internal/action"); + let policy = TestPolicy::rooted_at(PathBuf::from("/internal")); + let err = resolve_artifact_path(&root, Some(&policy), ArtifactKind::Output, "leak.md") + .expect_err("internal root must be refused"); + assert!( + matches!(err, OffloadError::InternalRoot { .. }), + "got {err:?}" + ); +} + +#[test] +fn rejects_host_internal_state_paths_with_the_specific_error() { + // The specific check runs first so its more useful message survives when a + // path trips both rules. + let root = PathBuf::from("/internal/action"); + let policy = TestPolicy::rooted_at(PathBuf::from("/internal")).with_internal_suffix("outputs"); + let err = resolve_artifact_path(&root, Some(&policy), ArtifactKind::Output, "leak.md") + .expect_err("internal state must be refused"); + assert!( + matches!(err, OffloadError::InternalState { .. }), + "the specific rule must win over blanket containment, got {err:?}" + ); +} + +#[test] +fn a_policy_free_resolve_still_enforces_containment() { + // `policy: None` relaxes only the host checks. If it ever relaxed traversal + // too, every host without a policy would gain an escape. + let err = resolve_artifact_path( + Path::new("/action"), + None, + ArtifactKind::Output, + "../out.md", + ) + .expect_err("traversal must still be refused without a policy"); + assert!( + matches!(err, OffloadError::PathEscape { .. }), + "got {err:?}" + ); +} + +#[test] +fn sanitize_component_strips_separators_and_never_returns_empty() { + assert_eq!(sanitize_component("sub-1a/2b"), "sub-1a_2b"); + assert_eq!(sanitize_component("../etc"), "___etc"); + assert_eq!(sanitize_component("ok_name-1"), "ok_name-1"); + // A component that sanitizes to nothing must still be a usable directory + // name, or the resulting path is malformed rather than merely odd. + assert_eq!(sanitize_component(""), "unknown"); + assert_eq!(sanitize_component("///"), "___"); + assert!(sanitize_component(&"x".repeat(500)).len() <= 80); +} + +#[test] +fn relative_to_root_falls_back_to_display_for_outside_paths() { + // An absolute fallback is correct here: a bogus relative path would resolve + // against the reader's own root and silently miss the file. + let rendered = relative_to_root(Path::new("/action"), Path::new("/elsewhere/outputs/x.md")); + assert_eq!(rendered, "/elsewhere/outputs/x.md"); +} + +#[test] +fn relative_to_root_renders_slash_separated() { + let rendered = relative_to_root( + Path::new("/action"), + &PathBuf::from("/action").join("outputs").join("x.md"), + ); + assert_eq!(rendered, "outputs/x.md"); +} + +// ── Thresholds ──────────────────────────────────────────────────────────────── + +#[test] +fn should_offload_respects_threshold_and_the_zero_disable() { + assert!(should_offload(100, 50)); + assert!(!should_offload(50, 50), "the threshold is exclusive"); + assert!(!should_offload(10, 50)); + // Zero is the documented opt-out, not "offload everything". + assert!(!should_offload(usize::MAX, 0)); +} + +#[test] +fn offload_threshold_tightens_to_an_agents_own_result_cap() { + // A cap below the default would truncate the result before offload fired, + // so the artifact would never reach disk at all. + assert_eq!(effective_offload_threshold(8_192, Some(4_000)), 4_000); + assert_eq!(effective_offload_threshold(8_192, Some(16_000)), 8_192); + assert_eq!(effective_offload_threshold(8_192, None), 8_192); + // A zero cap means "no cap", not "offload nothing". + assert_eq!(effective_offload_threshold(8_192, Some(0)), 8_192); +} + +// ── Abstracts ───────────────────────────────────────────────────────────────── + +#[test] +fn build_abstract_returns_short_content_unchanged() { + assert_eq!(build_abstract(" short ", 100), "short"); +} + +#[test] +fn build_abstract_cuts_at_a_line_boundary_when_one_is_available() { + let content = format!("{}\n{}", "a".repeat(60), "b".repeat(60)); + let out = build_abstract(&content, 100); + assert!(out.ends_with("...")); + assert!(!out.contains('b'), "should have cut at the newline: {out}"); +} + +#[test] +fn build_abstract_cuts_at_a_word_boundary_when_there_is_no_line_break() { + let content = format!("{} {}", "a".repeat(60), "b".repeat(60)); + let out = build_abstract(&content, 100); + assert!(out.ends_with("...")); + assert!(!out.contains('b'), "should have cut at the space: {out}"); +} + +#[test] +fn build_abstract_handles_a_zero_budget_and_boundary_free_text() { + assert_eq!(build_abstract("anything", 0), ""); + // No line or word break in the back half — a hard cut is the only option. + let out = build_abstract(&"a".repeat(200), 100); + assert!(out.ends_with("...")); +} + +#[test] +fn build_abstract_never_splits_a_multibyte_character() { + // Budget counted in chars, truncation done on a String: a byte-indexed cut + // here would panic rather than merely misformat. + let content = "é".repeat(200); + let out = build_abstract(&content, 50); + assert!(out.ends_with("...")); + assert!(out.is_char_boundary(out.len())); +} + +// ── Pointers ────────────────────────────────────────────────────────────────── + +fn sample_artifact(redacted: bool) -> OffloadedArtifact { + OffloadedArtifact { + kind: ArtifactKind::Output, + relative_path: "outputs/agent/task-result.md".to_string(), + absolute_path: PathBuf::from("/action/outputs/agent/task-result.md"), + stored_bytes: 1234, + original_bytes: 1300, + redacted, + } +} + +#[test] +fn pointer_carries_path_size_and_a_read_call() { + let rendered = render_artifact_pointer(&sample_artifact(false), "the abstract", READ_TOOL); + assert!(rendered.starts_with(ARTIFACT_POINTER_PREFIX)); + assert!(rendered.contains("path=outputs/agent/task-result.md")); + assert!(rendered.contains("bytes=1234")); + assert!(rendered.contains("kind=output")); + assert!(rendered.contains(READ_TOOL)); + assert!(rendered.contains("the abstract")); +} + +#[test] +fn pointer_names_the_read_tool_it_was_given() { + // The tool name is host vocabulary. Hard-coding one here would put a tool + // the host may not have into its prompts. + let rendered = render_artifact_pointer(&sample_artifact(false), "x", "read_file"); + assert!(rendered.contains("read_with: read_file")); + assert!(!rendered.contains("file_read")); +} + +#[test] +fn pointer_discloses_redaction_when_it_happened() { + let clean = render_artifact_pointer(&sample_artifact(false), "x", READ_TOOL); + assert!(!clean.contains("redaction")); + let redacted = render_artifact_pointer(&sample_artifact(true), "x", READ_TOOL); + assert!(redacted.contains("redaction was applied")); +} + +#[test] +fn extract_artifact_paths_reads_pointers_out_of_a_handoff() { + let handoff = + format!("{ARTIFACT_POINTER_PREFIX} kind=output path=outputs/a.md bytes=10\nprose"); + assert_eq!(extract_artifact_paths(&handoff), vec!["outputs/a.md"]); +} + +#[test] +fn extract_artifact_paths_dedupes_and_keeps_encounter_order() { + let handoff = format!( + "{ARTIFACT_POINTER_PREFIX} path=b.md bytes=1\n\ + {ARTIFACT_POINTER_PREFIX} path=a.md bytes=1\n\ + {ARTIFACT_POINTER_PREFIX} path=b.md bytes=1" + ); + assert_eq!(extract_artifact_paths(&handoff), vec!["b.md", "a.md"]); +} + +#[test] +fn extract_artifact_paths_ignores_non_pointer_and_malformed_lines() { + let handoff = format!( + "ordinary prose\n\ + {ARTIFACT_POINTER_PREFIX} no path field here\n\ + {ARTIFACT_POINTER_PREFIX} path= bytes=1\n\ + {ARTIFACT_POINTER_PREFIX} path=good.md bytes=1" + ); + // The empty `path=` case is the subtle one: splitting on whitespace rather + // than the FIRST whitespace would yield `bytes=1` as the path. + assert_eq!(extract_artifact_paths(&handoff), vec!["good.md"]); +} + +#[test] +fn note_artifact_handoff_reports_how_many_paths_crossed() { + let paths = vec!["a.md".to_string(), "b.md".to_string()]; + assert_eq!( + note_artifact_handoff(HANDOFF_STAGE_RECORDED, "agent", "task", &paths), + 2 + ); + assert_eq!( + note_artifact_handoff(HANDOFF_STAGE_CONSUMED, "agent", "task", &[]), + 0 + ); +} + +#[test] +fn handoff_stages_are_distinct() { + // They exist to tell the two ends of one pointer apart in a journal; equal + // values would render the same line twice. + assert_ne!(HANDOFF_STAGE_RECORDED, HANDOFF_STAGE_CONSUMED); +} + +// ── Writing ─────────────────────────────────────────────────────────────────── + +fn temp_roots() -> (tempfile::TempDir, PathBuf, PathBuf) { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path().join("action"); + let internal = dir.path().join("internal"); + std::fs::create_dir_all(&root).expect("action dir"); + std::fs::create_dir_all(&internal).expect("internal dir"); + (dir, root, internal) +} + +#[tokio::test] +async fn write_persists_under_outputs_and_reports_the_relative_path() { + let (_dir, root, internal) = temp_roots(); + let offload = offload_for(&root, &internal); + + let artifact = offload + .write(ArtifactKind::Output, "report.md", "hello") + .await + .expect("write"); + + assert_eq!(artifact.relative_path, "outputs/report.md"); + assert_eq!(artifact.stored_bytes, 5); + assert_eq!(artifact.original_bytes, 5); + assert!(!artifact.redacted); + let on_disk = tokio::fs::read_to_string(&artifact.absolute_path) + .await + .expect("read back"); + assert_eq!(on_disk, "hello"); +} + +#[tokio::test] +async fn write_redacts_credentials_before_they_reach_disk() { + let (_dir, root, internal) = temp_roots(); + let offload = offload_for(&root, &internal); + let body = format!("token: {SECRET}\n"); + + let artifact = offload + .write(ArtifactKind::Output, "creds.md", &body) + .await + .expect("write"); + + assert!(artifact.redacted, "the redactor rewrote the body"); + let on_disk = tokio::fs::read_to_string(&artifact.absolute_path) + .await + .expect("read back"); + assert!( + !on_disk.contains(SECRET), + "the secret reached disk: {on_disk}" + ); + assert!(on_disk.contains(REDACTION)); + // Original bytes describe the caller's payload, stored bytes the file. + assert_eq!(artifact.original_bytes, body.len()); + assert_eq!(artifact.stored_bytes, on_disk.len()); +} + +#[tokio::test] +async fn write_without_a_redactor_stores_bytes_verbatim() { + // The documented consequence of omitting a redactor. Pinned so the default + // cannot quietly become "scrub something" and give false assurance. + let (_dir, root, _internal) = temp_roots(); + let offload = ArtifactOffload::new(root.clone(), "agent", "task"); + let artifact = offload + .write(ArtifactKind::Output, "raw.md", SECRET) + .await + .expect("write"); + assert!(!artifact.redacted); + let on_disk = tokio::fs::read_to_string(&artifact.absolute_path) + .await + .expect("read back"); + assert_eq!(on_disk, SECRET); +} + +#[tokio::test] +async fn write_refuses_a_traversal_target_without_touching_disk() { + let (_dir, root, internal) = temp_roots(); + let offload = offload_for(&root, &internal); + + let err = offload + .write(ArtifactKind::Output, "../escape.md", "x") + .await + .expect_err("traversal must be refused"); + + assert!( + matches!(err, OffloadError::PathEscape { .. }), + "got {err:?}" + ); + assert!( + !root.join("escape.md").exists(), + "nothing may be written on a refused path" + ); +} + +#[tokio::test] +async fn default_result_name_sanitizes_both_identifiers() { + let (_dir, root, _internal) = temp_roots(); + let offload = ArtifactOffload::new(root, "agent/../x", "task id/1"); + // Each of `/`, `.`, `.`, `/` becomes its own underscore — the separators are + // replaced, never collapsed, so no identifier can smuggle in a path level. + assert_eq!( + offload.default_result_name(), + "agent____x/task_id_1-result.md" + ); +} + +#[tokio::test] +async fn write_refuses_a_parent_that_symlinks_out_of_the_convention_root() { + let (dir, root, internal) = temp_roots(); + let outside = dir.path().join("outside"); + std::fs::create_dir_all(&outside).expect("outside dir"); + std::fs::create_dir_all(root.join("outputs")).expect("outputs dir"); + + // `outputs/escape -> ../../outside`. The lexical checks cannot see this: + // the target does not exist when they run. + #[cfg(unix)] + std::os::unix::fs::symlink(&outside, root.join("outputs").join("escape")).expect("symlink"); + #[cfg(not(unix))] + return; + + let offload = offload_for(&root, &internal); + let err = offload + .write(ArtifactKind::Output, "escape/leak.md", "x") + .await + .expect_err("symlink escape must be refused"); + + assert!( + matches!(err, OffloadError::SymlinkEscape { .. }), + "got {err:?}" + ); + assert!( + !outside.join("leak.md").exists(), + "the write must not have followed the link" + ); +} + +#[tokio::test] +async fn worktree_artifact_renders_a_path_the_parent_can_resolve() { + // An isolated worker writes in its own checkout; the parent that receives + // the pointer holds a different root. Rendering against the parent's root is + // what keeps the path resolvable on the receiving side. + let (dir, root, internal) = temp_roots(); + let parent_root = dir.path().to_path_buf(); + let offload = offload_for(&root, &internal).with_render_root(parent_root); + + let artifact = offload + .write(ArtifactKind::Output, "report.md", "x") + .await + .expect("write"); + + assert_eq!(artifact.relative_path, "action/outputs/report.md"); +} + +#[tokio::test] +async fn a_render_root_outside_the_write_root_falls_back_to_absolute() { + // Better an absolute path than a relative one that resolves against the + // wrong root and silently misses the file. + let (_dir, root, internal) = temp_roots(); + let offload = offload_for(&root, &internal).with_render_root(PathBuf::from("/unrelated")); + + let artifact = offload + .write(ArtifactKind::Output, "report.md", "x") + .await + .expect("write"); + + assert!( + Path::new(&artifact.relative_path).is_absolute(), + "expected an absolute fallback, got {}", + artifact.relative_path + ); +} + +// ── The offload entry point ─────────────────────────────────────────────────── + +#[tokio::test] +async fn oversized_result_is_offloaded_and_the_parent_gets_a_path_plus_abstract() { + let (_dir, root, internal) = temp_roots(); + let offload = offload_for(&root, &internal); + let big = "x".repeat(10_000); + + let (text, artifact) = offload_oversized_result(big.clone(), &offload, 8_192, READ_TOOL).await; + let artifact = artifact.expect("an artifact was written"); + + assert!(text.starts_with(ARTIFACT_POINTER_PREFIX)); + assert!( + text.len() < big.len(), + "the pointer must be smaller than the payload it replaced" + ); + let on_disk = tokio::fs::read_to_string(&artifact.absolute_path) + .await + .expect("read back"); + assert_eq!(on_disk, big, "full fidelity is preserved on disk"); +} + +#[tokio::test] +async fn small_result_stays_inline() { + let (_dir, root, internal) = temp_roots(); + let offload = offload_for(&root, &internal); + + let (text, artifact) = + offload_oversized_result("small".to_string(), &offload, 8_192, READ_TOOL).await; + + assert_eq!(text, "small"); + assert!(artifact.is_none()); +} + +#[tokio::test] +async fn offload_is_disabled_by_a_zero_threshold() { + let (_dir, root, internal) = temp_roots(); + let offload = offload_for(&root, &internal); + let big = "x".repeat(10_000); + + let (text, artifact) = offload_oversized_result(big.clone(), &offload, 0, READ_TOOL).await; + + assert_eq!(text, big); + assert!(artifact.is_none()); +} + +#[tokio::test] +async fn offload_failure_keeps_the_inline_payload_for_the_host_backstop() { + // The load-bearing soft-failure contract: a refused offload must never cost + // the caller its content, or a disk problem turns into data loss. + let (_dir, _root, internal) = temp_roots(); + let unwritable = PathBuf::from("/proc/nonexistent-artifact-root"); + let offload = offload_for(&unwritable, &internal); + let big = "x".repeat(10_000); + + let (text, artifact) = offload_oversized_result(big.clone(), &offload, 8_192, READ_TOOL).await; + + assert_eq!( + text, big, + "the inline payload must survive a failed offload" + ); + assert!(artifact.is_none()); +} + +#[tokio::test] +async fn abstract_is_built_from_the_redacted_body_not_the_raw_output() { + // The pointer goes straight into the parent's context. Building the + // abstract from the raw text would re-expose the credential that was just + // scrubbed out of the file — and the file would still look correct. + let (_dir, root, internal) = temp_roots(); + let offload = offload_for(&root, &internal); + let body = format!("{SECRET} {}", "x".repeat(10_000)); + + let (text, artifact) = offload_oversized_result(body, &offload, 8_192, READ_TOOL).await; + let artifact = artifact.expect("an artifact was written"); + + assert!(artifact.redacted); + assert!( + !text.contains(SECRET), + "the secret leaked into the pointer: {text}" + ); + assert!(text.contains(REDACTION)); +} diff --git a/src/harness/artifacts/types.rs b/src/harness/artifacts/types.rs new file mode 100644 index 000000000..a13c2debb --- /dev/null +++ b/src/harness/artifacts/types.rs @@ -0,0 +1,146 @@ +//! Inert value types for the artifact-offload convention. +//! +//! Dependency-free by design (std + `thiserror` only) so a host can build +//! against these without pulling the engine — RFC §2 rule 2. + +use std::path::PathBuf; + +/// Deliverables directory under the artifact root. Artifacts here are meant to +/// outlive the step that produced them and to be handed to a parent agent or a +/// later step **by path**, not by value. +pub const OUTPUTS_DIR: &str = "outputs"; + +/// Scratch directory under the artifact root. Intermediate files a worker needs +/// while it works but does not intend to hand back. +pub const SCRATCH_DIR: &str = "workspace"; + +/// Byte threshold above which a worker's final result is written to +/// [`OUTPUTS_DIR`] and replaced by a pointer + abstract. +/// +/// ~2,000 tokens at the harness-wide 4-chars-per-token estimate. Below this, +/// inlining is cheaper than a file round-trip plus the pointer envelope. +pub const DEFAULT_OFFLOAD_THRESHOLD_BYTES: usize = 8_192; + +/// Characters of the offloaded body reproduced as the abstract in the pointer. +/// Enough for a parent to decide whether to read the full artifact. +pub const ABSTRACT_BUDGET_CHARS: usize = 600; + +/// Line prefix every pointer line carries. Grep-friendly, and the anchor +/// [`extract_artifact_paths`](super::extract_artifact_paths) keys off when +/// reading a handoff. +pub const ARTIFACT_POINTER_PREFIX: &str = "[artifact]"; + +/// Which of the two convention directories an artifact belongs in. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ArtifactKind { + /// A deliverable, written under [`OUTPUTS_DIR`]. + Output, + /// Scratch, written under [`SCRATCH_DIR`]. + /// + /// Note this is the artifact root's own `workspace` subdirectory, which is + /// **not** a host's internal state root. The resolver refuses to place an + /// artifact in the latter regardless of kind. + Scratch, +} + +impl ArtifactKind { + /// Directory name under the artifact root for this kind. + pub fn subdir(self) -> &'static str { + match self { + Self::Output => OUTPUTS_DIR, + Self::Scratch => SCRATCH_DIR, + } + } + + /// Stable, log-friendly label. + pub fn as_str(self) -> &'static str { + match self { + Self::Output => "output", + Self::Scratch => "scratch", + } + } +} + +/// A worker artifact that was successfully written to disk. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OffloadedArtifact { + /// Which convention directory it landed in. + pub kind: ArtifactKind, + /// Path relative to the render root, always `/`-separated so it can be + /// pasted straight into a file-read call on any platform. + pub relative_path: String, + /// Absolute path on disk. + pub absolute_path: PathBuf, + /// Bytes actually stored (post-redaction). + pub stored_bytes: usize, + /// Bytes of the caller's original payload (pre-redaction). + pub original_bytes: usize, + /// Whether the host redactor rewrote the body before storage. + pub redacted: bool, +} + +/// Why an offload was refused. +/// +/// Every variant is non-fatal at the call site: the caller keeps its inline +/// payload and whatever summarisation or truncation backstop it already had +/// still applies. An offload failure must never fail a turn. +#[derive(Debug, thiserror::Error)] +pub enum OffloadError { + /// The requested relative path was empty or whitespace-only. + #[error("artifact name is empty")] + EmptyName, + + /// The relative path was absolute (or carried a Windows drive/UNC prefix). + #[error("artifact path must be relative to the artifact root, got {path}")] + AbsolutePath { + /// The offending path, as supplied. + path: String, + }, + + /// The relative path escaped its convention directory (`..` traversal). + #[error("artifact path escapes {root}: {path}")] + PathEscape { + /// The convention root the path was resolved against. + root: String, + /// The offending path. + path: String, + }, + + /// The resolved path landed inside the host's internal state root. + /// + /// Fail-closed: offload targets resolve under the artifact root, never a + /// host's internal state. + #[error( + "artifact path resolves inside the host internal root, which agent writes may never reach: {path}" + )] + InternalRoot { + /// The offending path. + path: String, + }, + + /// The resolved path is a host-internal state location per + /// [`ArtifactPathPolicy::is_internal_state`](super::ArtifactPathPolicy::is_internal_state). + #[error("artifact path is host-internal state: {path}")] + InternalState { + /// The offending path. + path: String, + }, + + /// The parent directory that actually materialised on disk resolves, + /// through symlinks, to somewhere outside the convention root. + /// + /// The lexical checks in + /// [`resolve_artifact_path`](super::resolve_artifact_path) cannot see this, + /// because the target does not exist yet when they run. + #[error("artifact parent escapes its root through a symlink: {path} resolves to {resolved}")] + SymlinkEscape { + /// The parent directory as requested. + path: String, + /// Where it actually resolves to. + resolved: String, + }, + + /// Creating the parent directory or writing the file failed. + #[error("artifact write failed: {0}")] + Io(#[from] std::io::Error), +} diff --git a/src/harness/mod.rs b/src/harness/mod.rs index 449f45f40..6b03ce0e8 100644 --- a/src/harness/mod.rs +++ b/src/harness/mod.rs @@ -12,6 +12,7 @@ //! without creating one large runtime file. pub mod agent_loop; +pub mod artifacts; pub mod cache; pub mod cancel; pub mod config;