From 5e42554482e87afe8062fed212e244d802908158 Mon Sep 17 00:00:00 2001 From: Steven Enamakel Date: Thu, 13 Aug 2026 16:10:02 +0300 Subject: [PATCH] feat(harness): add a generic artifact-offload module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Long-horizon runs accumulate context: summarising one oversized payload at a time shrinks each result but never stops the aggregate from growing, and it can never restore full fidelity. The fix is to put large results on disk and hand the next step a path. This lands the generic half of that convention as `harness::artifacts`, ported from OpenHuman's `agent/harness/artifact_offload/` under `plan-agents.md` Phase 5. It is the first family in that phase's order, chosen because its inbound coupling is the lowest of the set. Two host decisions are the whole reason this could not move as written, and both become injected policy rather than imports: - `ArtifactPathPolicy` — 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. - `ArtifactRedactor` — what gets scrubbed before bytes touch disk. Credential and PII patterns are a host's compliance surface, not a library's. Both are gates the runtime calls, never behaviour the runtime is trusted to have performed (RFC section 2 rule 5). A `None` redactor stores bytes verbatim, which is documented as a security decision rather than an absence of one. Two things deliberately did NOT move: - The prompt contract. It is OpenHuman prompt text naming OpenHuman tools, which `plan-agents.md` section 6 lists as not publishable. It stays host-side, and `render_artifact_pointer` takes the read-tool name as a parameter for the same reason — a hard-coded tool name would put a tool the host may not have into its prompts. - The `is_workspace_internal_path` / `workspace_dir` split. It survives as two separate policy methods producing two distinct errors, because a host wants to tell "under the internal root" apart from "a specific state location" in a log. The specific check runs first so its more precise error wins when a path trips both. The symlink re-validation is preserved as-is and is the subtle part: the resolver's checks are necessarily lexical because the target does not exist when they run, so the real parent is re-checked after `create_dir_all` and before the write. `tokio`'s `fs` feature is now required. It enables more of tokio rather than adding a package, so the kernel-floor package count is unaffected. 44 tests, covering the happy path, the soft-failure path (a refused offload must never cost the caller its content), and the fail-closed hardening. The load-bearing one is that the abstract is built from the redacted body and never from the raw output — the pointer goes straight into a parent's context, so rendering it from raw text would re-expose the credential just scrubbed out of the file, while the file itself still looked correct. Co-authored-by: Medulla --- Cargo.toml | 5 +- src/harness/artifacts/mod.rs | 80 ++++ src/harness/artifacts/ops.rs | 407 ++++++++++++++++++++ src/harness/artifacts/paths.rs | 157 ++++++++ src/harness/artifacts/policy.rs | 160 ++++++++ src/harness/artifacts/test.rs | 648 ++++++++++++++++++++++++++++++++ src/harness/artifacts/types.rs | 146 +++++++ src/harness/mod.rs | 1 + 8 files changed, 1603 insertions(+), 1 deletion(-) create mode 100644 src/harness/artifacts/mod.rs create mode 100644 src/harness/artifacts/ops.rs create mode 100644 src/harness/artifacts/paths.rs create mode 100644 src/harness/artifacts/policy.rs create mode 100644 src/harness/artifacts/test.rs create mode 100644 src/harness/artifacts/types.rs 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;