mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
feat(agent): filesystem-offload convention for worker artifacts on long-horizon tasks (#3883) (#5217)
This commit is contained in:
Mega Mind
@@ -178,6 +178,43 @@ Some tool calls return enormous payloads - a Composio action dumping 200 KB of J
|
||||
|
||||
When a tool result exceeds the summarizer's threshold, it gets routed through a dedicated `summarizer` sub-agent before entering the parent's history. The summarizer compresses the payload per an extraction contract that preserves identifiers and key facts, and the parent agent only sees the compressed summary. Hard truncation remains the backstop downstream when summarization fails or the payload is so absurdly large that paying for an LLM call on it makes no economic sense.
|
||||
|
||||
### Filesystem offload - `outputs/` and `workspace/`
|
||||
|
||||
Compression alone does not survive a long-horizon run. Summaries still accumulate step after step, and no amount of compressing restores the fidelity that was thrown away. So for minutes-to-hours tasks the harness moves large results **out of context and onto disk**, and hands the next step a **path**.
|
||||
|
||||
Two directories under the agent's existing `action_dir` at runtime (the implementation lives in `src/openhuman/agent/harness/artifact_offload/`):
|
||||
|
||||
| Directory | Holds |
|
||||
| ------------------------ | ----------------------------------------------------------------------------- |
|
||||
| `action_dir/outputs/` | Deliverables. Meant to outlive the step that made them, handed on by path. |
|
||||
| `action_dir/workspace/` | Scratch. Intermediate files a worker needs but does not hand back. |
|
||||
|
||||
Note `action_dir/workspace/` is a scratch folder inside the agent's action root. It is **not** the core's internal `workspace_dir`, which agent writes may never reach.
|
||||
|
||||
Two halves enforce the convention:
|
||||
|
||||
* **Prompt.** A sub-agent that actually holds `file_write` gets a Long-horizon Artifact Offload contract in its system prompt: results past roughly 2 000 tokens go to a file under `outputs/`, and the reply is that relative path plus a short abstract. The gate is deliberate — a prompt may only name tools the agent can really call, or the model emits calls that fail. `researcher` (search + fetch only) and skill-filtered specialists get no contract text, and dedicated guards assert their prompts never mention a filesystem tool. They stay covered by the harness half below, which needs no cooperation from the model. The relevant archetype prompts (`researcher`, `planner`) spell out what the convention means for their own work; the planner is told to reference artifact paths across DAG nodes rather than pasting payloads forward.
|
||||
* **Harness.** `offload_oversized_result` runs on every sub-agent outcome, so an oversized result is offloaded even when the worker inlined it anyway. It fires **before** the definition's `max_result_chars` cap, so the full body lands on disk instead of being cut.
|
||||
|
||||
What the parent receives is a pointer, not a payload:
|
||||
|
||||
```text
|
||||
[artifact] kind=output path=outputs/researcher/sub-1234-result.md bytes=52318
|
||||
read_with: file_read {"path":"outputs/researcher/sub-1234-result.md"}
|
||||
note: The full result was written to the action workspace instead of being inlined. …
|
||||
|
||||
[abstract]
|
||||
HEADLINE FINDING …
|
||||
```
|
||||
|
||||
`SubagentRunOutcome.artifact_paths` carries the same paths structurally, parsed out of the `[artifact]` pointers in the output, so the handoff carries paths whether the harness offloaded the result or the worker wrote the file itself. Any path the parent takes delivery of is recoverable with an ordinary `file_read` long after the child's context is gone.
|
||||
|
||||
**The summarizer is now the fallback, not the first resort.** Offload catches the common case; the summarizer detour and the `tool_result_budget_bytes` truncation above still handle everything it does not, including every failure mode here. A refused or failed offload is deliberately soft: the caller keeps its inline payload and falls through to those backstops.
|
||||
|
||||
**Path hardening is fail-closed.** `resolve_artifact_path` rejects absolute paths, `..` traversal, and anything that escapes its convention root after lexical normalization. When a `SecurityPolicy` is available it also refuses anything under `workspace_dir`, both by blanket containment and via `is_workspace_internal_path`. Offload targets resolve under `action_dir`, never `workspace_dir` - including the case where someone configures `action_dir` inside the workspace root, where every offload is refused rather than quietly writing to internal state.
|
||||
|
||||
Writes log `[artifact] wrote worker artifact under action_dir`; each path a handoff carries logs `[artifact] handoff carried an artifact path to the parent`, on both the producing and the consuming side, so a run journal shows both ends of every pointer.
|
||||
|
||||
### TokenJuice - content-aware tool-output compaction (Stage 1a)
|
||||
|
||||
Before a fresh tool result enters history (and ahead of the byte-budget backstop), it passes through the **TokenJuice content router** in the vendored TinyJuice crate (`vendor/tinyjuice`), with OpenHuman adapters in `src/openhuman/tokenjuice/`. Inspired by Headroom, the router *detects the content kind* (JSON, code, log, search, diff, HTML, plain text) from the bytes and/or a hint derived from the tool name and arguments, then dispatches to a specialised compressor:
|
||||
|
||||
@@ -0,0 +1,70 @@
|
||||
//! The prompt-side half of the artifact-offload convention (#3883).
|
||||
//!
|
||||
//! Single source of truth for the directory names the model is told about, so
|
||||
//! the prompt and [`super::paths::resolve_artifact_path`] can never drift.
|
||||
|
||||
use std::collections::HashSet;
|
||||
|
||||
use super::types::{OUTPUTS_DIR, SCRATCH_DIR};
|
||||
|
||||
/// Approximate token budget quoted to the model as the offload trigger.
|
||||
/// Mirrors [`super::types::DEFAULT_OFFLOAD_THRESHOLD_BYTES`] at the harness-wide
|
||||
/// 4-chars-per-token estimate.
|
||||
const CONTRACT_THRESHOLD_TOKENS: usize = 2_000;
|
||||
|
||||
/// Heading the contract is rendered under. Used by the idempotence check in
|
||||
/// `subagent_runner::ops::prompt` so a re-rendered prompt never stacks two
|
||||
/// copies.
|
||||
pub const ARTIFACT_OFFLOAD_HEADING: &str = "## Long-horizon Artifact Offload";
|
||||
|
||||
/// Tool a sub-agent must actually hold before the contract is worth rendering.
|
||||
///
|
||||
/// A prompt may only name tools the agent can really call: advertising one it
|
||||
/// lacks produces hallucinated calls that fail. `researcher` (search + fetch
|
||||
/// only) and every skill-filtered specialist have no filesystem tools, and
|
||||
/// dedicated guards assert their prompts never mention one — see
|
||||
/// `agent_registry::agents::researcher::prompt::tests::build_returns_nonempty_body`
|
||||
/// and `subagent_runner::ops_tests::typed_mode_filters_tools_by_skill_filter`.
|
||||
pub const OFFLOAD_WRITE_TOOL: &str = "file_write";
|
||||
|
||||
/// Whether the offload contract should be rendered for an agent whose visible
|
||||
/// tools are `visible_tool_names`.
|
||||
///
|
||||
/// Only agents that can actually write a file are told to. Everyone else stays
|
||||
/// covered by the harness-side automatic offload in
|
||||
/// [`super::offload_oversized_result`], which needs no cooperation from the
|
||||
/// model — which is exactly why that half exists.
|
||||
pub fn should_render_offload_contract(visible_tool_names: &HashSet<String>) -> bool {
|
||||
visible_tool_names.contains(OFFLOAD_WRITE_TOOL)
|
||||
}
|
||||
|
||||
/// Render the offload contract for a sub-agent that holds a file-write tool.
|
||||
///
|
||||
/// Deliberately parameter-free and byte-stable: the sub-agent system prompt is
|
||||
/// prefix-cached, so this must render identically on every run. Absolute paths
|
||||
/// are never interpolated for the same reason (and because a worktree-isolated
|
||||
/// worker resolves its own action root).
|
||||
///
|
||||
/// Names no tool other than [`OFFLOAD_WRITE_TOOL`]. In particular it does not
|
||||
/// name the parent's *reading* tool: how the parent opens the file is not this
|
||||
/// agent's business, and mentioning it would leak a tool name into prompts of
|
||||
/// agents that do not hold it.
|
||||
pub fn render_artifact_offload_contract() -> String {
|
||||
format!(
|
||||
"{ARTIFACT_OFFLOAD_HEADING}\n\n\
|
||||
Large results belong on disk, not in your reply. Two directories sit under your action directory:\n\
|
||||
- `{OUTPUTS_DIR}/` for deliverables, anything the parent or a later step needs to read.\n\
|
||||
- `{SCRATCH_DIR}/` for scratch, intermediate files you do not intend to hand back.\n\
|
||||
\n\
|
||||
When a result would run past roughly {CONTRACT_THRESHOLD_TOKENS} tokens:\n\
|
||||
1. Write the full content to a file under `{OUTPUTS_DIR}/` with `{OFFLOAD_WRITE_TOOL}`.\n\
|
||||
2. Reply with that relative path plus a short abstract, not the full payload.\n\
|
||||
3. Quote the path exactly, so the parent can open it verbatim.\n\
|
||||
\n\
|
||||
Rules:\n\
|
||||
- Offload paths are always relative to your action directory. Never write outside it, and never target the core's internal workspace state.\n\
|
||||
- Keep the abstract honest: say what the file holds and what is still open. Never present it as the complete result.\n\
|
||||
- Small results stay inline. A pointer to a two-line file costs the parent more than the two lines.\n\
|
||||
- If you inline an oversized result anyway, the harness persists it under `{OUTPUTS_DIR}/` and hands the parent the path instead.\n"
|
||||
)
|
||||
}
|
||||
@@ -0,0 +1,74 @@
|
||||
//! Filesystem-offload convention for worker artifacts on long-horizon tasks
|
||||
//! (#3883).
|
||||
//!
|
||||
//! ## 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. The summarizer detour
|
||||
//! ([`crate::openhuman::tinyagents::payload_summarizer`]) shrinks one oversized
|
||||
//! payload at a time; it does not stop the aggregate from growing.
|
||||
//!
|
||||
//! ## The convention
|
||||
//!
|
||||
//! Two directories under the agent's existing `action_dir`:
|
||||
//!
|
||||
//! | Directory | Holds |
|
||||
//! | ---------------------- | -------------------------------------------------------- |
|
||||
//! | `action_dir/outputs/` | Deliverables. Handed between steps **by path**. |
|
||||
//! | `action_dir/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:
|
||||
//!
|
||||
//! * **Prompt** — [`render_artifact_offload_contract`] is appended to every
|
||||
//! typed sub-agent's system prompt, so workers offload on purpose.
|
||||
//! * **Harness** — [`offload_oversized_result`] runs on every sub-agent outcome,
|
||||
//! so an oversized result is offloaded even when the worker inlined it anyway.
|
||||
//!
|
||||
//! The summarizer detour and the `tool_result_budget_bytes` truncation stay
|
||||
//! exactly as they are: they are 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 them.
|
||||
//!
|
||||
//! ## Hardening
|
||||
//!
|
||||
//! [`resolve_artifact_path`] is fail-closed. Absolute paths, `..` traversal, and
|
||||
//! anything that escapes its convention root are refused, and when a
|
||||
//! `SecurityPolicy` is available so is anything reaching the core's internal
|
||||
//! `workspace_dir` — both the blanket containment check and
|
||||
//! `is_workspace_internal_path`. Offload targets resolve under `action_dir`,
|
||||
//! never `workspace_dir`.
|
||||
//!
|
||||
//! ## Logging
|
||||
//!
|
||||
//! Every write emits `[artifact] wrote worker artifact under action_dir`, and
|
||||
//! every path a handoff carries to a parent emits `[artifact] handoff carried an
|
||||
//! artifact path to the parent`, so a run journal shows both ends of a pointer.
|
||||
|
||||
mod contract;
|
||||
mod ops;
|
||||
mod paths;
|
||||
mod types;
|
||||
|
||||
pub use contract::{
|
||||
render_artifact_offload_contract, should_render_offload_contract, ARTIFACT_OFFLOAD_HEADING,
|
||||
OFFLOAD_WRITE_TOOL,
|
||||
};
|
||||
pub use ops::{
|
||||
build_abstract, effective_offload_threshold, extract_artifact_paths, note_artifact_handoff,
|
||||
offload_oversized_result, render_artifact_pointer, should_offload, ArtifactOffload,
|
||||
HANDOFF_STAGE_CONSUMED, HANDOFF_STAGE_RECORDED,
|
||||
};
|
||||
pub use paths::{relative_to_action_dir, resolve_artifact_path, sanitize_component};
|
||||
pub use types::{
|
||||
ArtifactKind, OffloadError, OffloadedArtifact, ABSTRACT_BUDGET_CHARS, ARTIFACT_POINTER_PREFIX,
|
||||
DEFAULT_OFFLOAD_THRESHOLD_BYTES, OUTPUTS_DIR, SCRATCH_DIR,
|
||||
};
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,369 @@
|
||||
//! Write, pointer-render, and handoff plumbing for the artifact-offload
|
||||
//! convention (#3883).
|
||||
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::sync::Arc;
|
||||
|
||||
use crate::openhuman::memory_store::safety::sanitize_text;
|
||||
use crate::openhuman::security::SecurityPolicy;
|
||||
|
||||
use super::paths::{relative_to_action_dir, resolve_artifact_path, sanitize_component};
|
||||
use super::types::{
|
||||
ArtifactKind, OffloadError, OffloadedArtifact, ABSTRACT_BUDGET_CHARS, ARTIFACT_POINTER_PREFIX,
|
||||
};
|
||||
|
||||
/// Whether a payload of `bytes` should be offloaded at `threshold_bytes`.
|
||||
///
|
||||
/// A zero threshold disables offload entirely (used to opt a run 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` down to at most `budget_chars` characters for the
|
||||
/// pointer's abstract.
|
||||
///
|
||||
/// Prefers cutting at a line break, then at a word break, so the parent reads a
|
||||
/// whole thought rather than a word sliced in half. Only falls back to a hard
|
||||
/// character cut 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 the machine-readable pointer ([`extract_artifact_paths`]
|
||||
/// parses it); the rest is for the model reading the handoff.
|
||||
pub fn render_artifact_pointer(artifact: &OffloadedArtifact, abstract_text: &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: file_read {{\"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<String> {
|
||||
let mut found: Vec<String> = 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 — `flow_memory_agent` caps at 4 000 chars, `context_scout` at
|
||||
/// 5 000, several built-ins at 8 000, all under the 8 KiB default. 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>,
|
||||
) -> 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 `action_dir`, the security policy used for the
|
||||
/// fail-closed workspace checks, and the identifiers that name generated
|
||||
/// artifacts and tag the `[artifact]` logs.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct ArtifactOffload {
|
||||
action_dir: PathBuf,
|
||||
/// Root the handed-back path is rendered against. Normally identical to
|
||||
/// `action_dir`; see [`Self::with_render_root`].
|
||||
render_root: PathBuf,
|
||||
policy: Option<Arc<SecurityPolicy>>,
|
||||
agent_id: String,
|
||||
task_id: String,
|
||||
}
|
||||
|
||||
impl ArtifactOffload {
|
||||
pub fn new(
|
||||
action_dir: PathBuf,
|
||||
policy: Option<Arc<SecurityPolicy>>,
|
||||
agent_id: impl Into<String>,
|
||||
task_id: impl Into<String>,
|
||||
) -> Self {
|
||||
Self {
|
||||
render_root: action_dir.clone(),
|
||||
action_dir,
|
||||
policy,
|
||||
agent_id: agent_id.into(),
|
||||
task_id: task_id.into(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Render handed-back paths relative to `render_root` instead of the write
|
||||
/// root.
|
||||
///
|
||||
/// A worktree-isolated worker writes inside its own checkout, but the parent
|
||||
/// that receives the pointer no longer holds that worker's
|
||||
/// `WorkspaceDescriptor` — a bare `outputs/…` would resolve against the
|
||||
/// *parent's* action root and miss the file entirely. Rendering against the
|
||||
/// parent's root yields a relative path when the worktree is nested inside
|
||||
/// it, and falls back to 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:
|
||||
/// `<agent_id>/<task_id>-result.md` with 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 `action_dir` without writing.
|
||||
/// Exposed so callers can validate a model-supplied path before acting on
|
||||
/// it.
|
||||
pub fn resolve(&self, kind: ArtifactKind, relative: &str) -> Result<PathBuf, OffloadError> {
|
||||
resolve_artifact_path(&self.action_dir, self.policy.as_deref(), kind, relative)
|
||||
}
|
||||
|
||||
/// Write `content` to `relative` under the convention directory for `kind`.
|
||||
///
|
||||
/// The body is passed through credential/PII redaction before it touches
|
||||
/// disk, matching how oversized tool results are persisted.
|
||||
pub async fn write(
|
||||
&self,
|
||||
kind: ArtifactKind,
|
||||
relative: &str,
|
||||
content: &str,
|
||||
) -> Result<OffloadedArtifact, OffloadError> {
|
||||
self.write_returning_stored(kind, relative, content)
|
||||
.await
|
||||
.map(|(artifact, _stored)| artifact)
|
||||
}
|
||||
|
||||
/// Same as [`Self::write`], but also hands back the **stored** (redacted)
|
||||
/// body.
|
||||
///
|
||||
/// Callers that surface any part of the artifact back into the 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 `sanitize_text` 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 -> <workspace_dir>`) 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 sanitized = sanitize_text(content);
|
||||
tokio::fs::write(&absolute, sanitized.value.as_bytes()).await?;
|
||||
|
||||
let relative_path = relative_to_action_dir(&self.render_root, &absolute);
|
||||
let artifact = OffloadedArtifact {
|
||||
kind,
|
||||
relative_path,
|
||||
absolute_path: absolute,
|
||||
stored_bytes: sanitized.value.len(),
|
||||
original_bytes: content.len(),
|
||||
redacted: sanitized.report.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 action_dir"
|
||||
);
|
||||
|
||||
Ok((artifact, sanitized.value))
|
||||
}
|
||||
|
||||
/// Resolve `parent` through symlinks and confirm it still sits inside this
|
||||
/// kind's convention root (and outside `workspace_dir`).
|
||||
///
|
||||
/// `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 root = self.action_dir.join(kind.subdir());
|
||||
let real_parent = tokio::fs::canonicalize(parent).await?;
|
||||
// The root itself may be reached through a symlinked `action_dir` (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(&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() {
|
||||
if policy.is_workspace_internal_path(&real_parent)
|
||||
|| tokio::fs::canonicalize(&policy.workspace_dir)
|
||||
.await
|
||||
.map(|ws| real_parent.starts_with(&ws))
|
||||
.unwrap_or(false)
|
||||
{
|
||||
return Err(OffloadError::SymlinkEscape {
|
||||
path: parent.display().to_string(),
|
||||
resolved: real_parent.display().to_string(),
|
||||
});
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Action directory this writer is rooted at.
|
||||
pub fn action_dir(&self) -> &Path {
|
||||
&self.action_dir
|
||||
}
|
||||
}
|
||||
|
||||
/// 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 the original payload back and the existing summarizer detour and
|
||||
/// tool-result budget stay in charge as the fallback.
|
||||
pub async fn offload_oversized_result(
|
||||
output: String,
|
||||
offload: &ArtifactOffload,
|
||||
threshold_bytes: usize,
|
||||
) -> (String, Option<OffloadedArtifact>) {
|
||||
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);
|
||||
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 (summarizer/truncation backstop applies)"
|
||||
);
|
||||
(output, None)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,150 @@
|
||||
//! Path hardening for the artifact-offload convention (#3883).
|
||||
//!
|
||||
//! Everything an agent offloads resolves under `action_dir/<outputs|workspace>`.
|
||||
//! The resolver is deliberately fail-closed: it rejects absolute paths, `..`
|
||||
//! traversal, anything that lands outside its convention root after lexical
|
||||
//! normalization, and anything that reaches the core's internal `workspace_dir`
|
||||
//! (whether or not the specific subdirectory is one of the
|
||||
//! `is_workspace_internal_path` state locations).
|
||||
|
||||
use std::path::{Component, Path, PathBuf};
|
||||
|
||||
use crate::openhuman::security::SecurityPolicy;
|
||||
|
||||
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 an `action_dir`-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
|
||||
/// `action_dir` (unreachable via [`resolve_artifact_path`], which validates
|
||||
/// containment first).
|
||||
pub fn relative_to_action_dir(action_dir: &Path, absolute: &Path) -> String {
|
||||
match absolute.strip_prefix(action_dir) {
|
||||
Ok(rel) => rel
|
||||
.components()
|
||||
.map(|c| c.as_os_str().to_string_lossy().to_string())
|
||||
.collect::<Vec<_>>()
|
||||
.join("/"),
|
||||
Err(_) => absolute.to_string_lossy().to_string(),
|
||||
}
|
||||
}
|
||||
|
||||
/// Resolve `relative` into an absolute offload target under
|
||||
/// `action_dir/<kind.subdir()>`, or explain why it is refused.
|
||||
///
|
||||
/// When `policy` is supplied the resolved path is additionally checked against
|
||||
/// the core's internal state root:
|
||||
///
|
||||
/// * anything under `policy.workspace_dir` is refused outright, because the
|
||||
/// convention is that offload targets live under `action_dir` and never
|
||||
/// `workspace_dir`;
|
||||
/// * anything [`SecurityPolicy::is_workspace_internal_path`] flags is refused
|
||||
/// with the more specific error, which keeps the message useful when
|
||||
/// `action_dir` has been configured inside the workspace root.
|
||||
///
|
||||
/// Passing `policy: None` skips only the workspace checks; the traversal and
|
||||
/// containment checks always run.
|
||||
pub fn resolve_artifact_path(
|
||||
action_dir: &Path,
|
||||
policy: Option<&SecurityPolicy>,
|
||||
kind: ArtifactKind,
|
||||
relative: &str,
|
||||
) -> Result<PathBuf, OffloadError> {
|
||||
let trimmed = relative.trim();
|
||||
if trimmed.is_empty() {
|
||||
return Err(OffloadError::EmptyName);
|
||||
}
|
||||
|
||||
let requested = Path::new(trimmed);
|
||||
let root = action_dir.join(kind.subdir());
|
||||
for component in requested.components() {
|
||||
match component {
|
||||
Component::Normal(_) | Component::CurDir => {}
|
||||
Component::ParentDir => {
|
||||
return Err(OffloadError::PathEscape {
|
||||
root: root.display().to_string(),
|
||||
path: trimmed.to_string(),
|
||||
});
|
||||
}
|
||||
Component::RootDir | Component::Prefix(_) => {
|
||||
return Err(OffloadError::AbsolutePath {
|
||||
path: trimmed.to_string(),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let candidate = normalize_lexically(&root.join(requested));
|
||||
// `normalize_lexically` cannot climb above `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(&root) {
|
||||
return Err(OffloadError::PathEscape {
|
||||
root: root.display().to_string(),
|
||||
path: candidate.display().to_string(),
|
||||
});
|
||||
}
|
||||
|
||||
if let Some(policy) = policy {
|
||||
if policy.is_workspace_internal_path(&candidate) {
|
||||
return Err(OffloadError::WorkspaceInternal {
|
||||
path: candidate.display().to_string(),
|
||||
});
|
||||
}
|
||||
if candidate.starts_with(&policy.workspace_dir) {
|
||||
return Err(OffloadError::WorkspaceTarget {
|
||||
path: candidate.display().to_string(),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
Ok(candidate)
|
||||
}
|
||||
@@ -0,0 +1,735 @@
|
||||
//! Tests for the artifact-offload convention (#3883).
|
||||
//!
|
||||
//! Covers the happy path (oversized result lands in `outputs/`, parent gets a
|
||||
//! path + abstract), the fallback path (offload refused, inline payload
|
||||
//! survives for the summarizer/truncation backstop), and the fail-closed path
|
||||
//! hardening that keeps offload inside `action_dir` and out of `workspace_dir`.
|
||||
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
|
||||
use super::*;
|
||||
use crate::openhuman::security::{AutonomyLevel, SecurityPolicy, TrustedAccess, TrustedRoot};
|
||||
use crate::openhuman::tools::traits::Tool;
|
||||
use crate::openhuman::tools::FileReadTool;
|
||||
use serde_json::json;
|
||||
|
||||
/// Policy with disjoint action/workspace roots, the shipped default layout
|
||||
/// (`~/OpenHuman/projects` vs `~/.openhuman/users/<id>/workspace`).
|
||||
///
|
||||
/// `action_dir` is granted as a read-write trusted root, because that is what
|
||||
/// production does: `SecurityPolicy::from_config` grants the projects dir
|
||||
/// exactly this way (`from_config_grants_default_projects_dir_as_readwrite_root`).
|
||||
/// Without the grant, `is_resolved_path_allowed_for` refuses everything outside
|
||||
/// `workspace_dir` — that check is unconditional and is NOT relaxed by
|
||||
/// `workspace_only`, so a hand-built policy that omits the grant cannot read a
|
||||
/// file the shipped app reads fine.
|
||||
fn policy_with(action_dir: PathBuf, workspace_dir: PathBuf) -> Arc<SecurityPolicy> {
|
||||
// Canonicalize the granted root: `validate_path` compares the CANONICAL
|
||||
// resolved path against it, and on macOS a temp dir under `/tmp` resolves
|
||||
// to `/private/tmp`, so an uncanonicalized grant would silently never match.
|
||||
let granted = action_dir
|
||||
.canonicalize()
|
||||
.unwrap_or_else(|_| action_dir.clone());
|
||||
Arc::new(SecurityPolicy {
|
||||
autonomy: AutonomyLevel::Supervised,
|
||||
trusted_roots: vec![TrustedRoot {
|
||||
path: granted.to_string_lossy().to_string(),
|
||||
access: TrustedAccess::ReadWrite,
|
||||
}],
|
||||
action_dir,
|
||||
workspace_dir,
|
||||
..SecurityPolicy::default()
|
||||
})
|
||||
}
|
||||
|
||||
fn offload_for(action_dir: &std::path::Path, workspace_dir: &std::path::Path) -> ArtifactOffload {
|
||||
ArtifactOffload::new(
|
||||
action_dir.to_path_buf(),
|
||||
Some(policy_with(
|
||||
action_dir.to_path_buf(),
|
||||
workspace_dir.to_path_buf(),
|
||||
)),
|
||||
"researcher",
|
||||
"sub-1234",
|
||||
)
|
||||
}
|
||||
|
||||
// ── Convention directories ──────────────────────────────────────────────────
|
||||
|
||||
#[test]
|
||||
fn kinds_map_to_the_documented_directories() {
|
||||
assert_eq!(ArtifactKind::Output.subdir(), OUTPUTS_DIR);
|
||||
assert_eq!(ArtifactKind::Output.subdir(), "outputs");
|
||||
assert_eq!(ArtifactKind::Scratch.subdir(), SCRATCH_DIR);
|
||||
assert_eq!(ArtifactKind::Scratch.subdir(), "workspace");
|
||||
assert_eq!(ArtifactKind::Output.as_str(), "output");
|
||||
assert_eq!(ArtifactKind::Scratch.as_str(), "scratch");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prompt_contract_names_both_directories_and_the_write_step() {
|
||||
let rendered = render_artifact_offload_contract();
|
||||
assert!(rendered.starts_with(ARTIFACT_OFFLOAD_HEADING));
|
||||
assert!(rendered.contains("`outputs/`"));
|
||||
assert!(rendered.contains("`workspace/`"));
|
||||
assert!(rendered.contains(OFFLOAD_WRITE_TOOL));
|
||||
// Byte-stable: the sub-agent system prompt is prefix-cached.
|
||||
assert_eq!(rendered, render_artifact_offload_contract());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prompt_contract_names_no_tool_the_agent_may_not_hold() {
|
||||
// A sub-agent prompt may only advertise tools it can actually call, or the
|
||||
// model emits calls that fail. `file_write` is gated on the agent holding
|
||||
// it; the parent's *reading* tool must never appear at all, since this
|
||||
// prompt also reaches agents that have no filesystem tools.
|
||||
let rendered = render_artifact_offload_contract();
|
||||
assert!(
|
||||
!rendered.contains("file_read"),
|
||||
"the parent's read tool must not leak into a child prompt: {rendered}"
|
||||
);
|
||||
// Guarded upstream by researcher::prompt::tests::build_returns_nonempty_body
|
||||
// and ops_tests::typed_mode_filters_tools_by_skill_filter.
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn offload_contract_is_rendered_only_for_agents_holding_a_write_tool() {
|
||||
let mut writer = std::collections::HashSet::new();
|
||||
writer.insert(OFFLOAD_WRITE_TOOL.to_string());
|
||||
assert!(should_render_offload_contract(&writer));
|
||||
|
||||
// `researcher` (search + fetch) and skill-filtered specialists land here.
|
||||
let mut reader_only = std::collections::HashSet::new();
|
||||
reader_only.insert("web_search_tool".to_string());
|
||||
reader_only.insert("notion__search".to_string());
|
||||
assert!(!should_render_offload_contract(&reader_only));
|
||||
assert!(!should_render_offload_contract(
|
||||
&std::collections::HashSet::new()
|
||||
));
|
||||
}
|
||||
|
||||
// ── Path hardening ──────────────────────────────────────────────────────────
|
||||
|
||||
#[test]
|
||||
fn resolves_under_the_convention_directory() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let policy = policy_with(action.path().to_path_buf(), workspace.path().to_path_buf());
|
||||
|
||||
let resolved = resolve_artifact_path(
|
||||
action.path(),
|
||||
Some(&*policy),
|
||||
ArtifactKind::Output,
|
||||
"researcher/report.md",
|
||||
)
|
||||
.expect("a plain relative name resolves");
|
||||
|
||||
assert_eq!(
|
||||
resolved,
|
||||
action
|
||||
.path()
|
||||
.join("outputs")
|
||||
.join("researcher")
|
||||
.join("report.md")
|
||||
);
|
||||
assert_eq!(
|
||||
relative_to_action_dir(action.path(), &resolved),
|
||||
"outputs/researcher/report.md"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn scratch_kind_resolves_under_action_dir_workspace_not_the_core_workspace() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let policy = policy_with(action.path().to_path_buf(), workspace.path().to_path_buf());
|
||||
|
||||
let resolved = resolve_artifact_path(
|
||||
action.path(),
|
||||
Some(&*policy),
|
||||
ArtifactKind::Scratch,
|
||||
"notes.txt",
|
||||
)
|
||||
.expect("scratch resolves");
|
||||
|
||||
assert!(resolved.starts_with(action.path().join("workspace")));
|
||||
assert!(
|
||||
!resolved.starts_with(workspace.path()),
|
||||
"action_dir/workspace must never be the core workspace_dir"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_parent_traversal() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let err = resolve_artifact_path(
|
||||
action.path(),
|
||||
None,
|
||||
ArtifactKind::Output,
|
||||
"../../etc/passwd",
|
||||
)
|
||||
.expect_err("`..` traversal must be refused");
|
||||
assert!(matches!(err, OffloadError::PathEscape { .. }), "{err}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_absolute_paths() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let absolute = if cfg!(windows) {
|
||||
"C:\\Windows\\System32\\drivers\\etc\\hosts"
|
||||
} else {
|
||||
"/etc/passwd"
|
||||
};
|
||||
let err = resolve_artifact_path(action.path(), None, ArtifactKind::Output, absolute)
|
||||
.expect_err("absolute paths must be refused");
|
||||
assert!(matches!(err, OffloadError::AbsolutePath { .. }), "{err}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_empty_and_whitespace_names() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
for name in ["", " ", "\n\t"] {
|
||||
let err = resolve_artifact_path(action.path(), None, ArtifactKind::Output, name)
|
||||
.expect_err("empty names must be refused");
|
||||
assert!(matches!(err, OffloadError::EmptyName), "{err}");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn accepts_leading_current_dir_segments() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let resolved =
|
||||
resolve_artifact_path(action.path(), None, ArtifactKind::Output, "./report.md").unwrap();
|
||||
assert_eq!(resolved, action.path().join("outputs").join("report.md"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_targets_inside_workspace_dir_fail_closed() {
|
||||
// action_dir configured INSIDE workspace_dir: every offload target lands in
|
||||
// the core's internal state root, so every offload must be refused rather
|
||||
// than quietly writing there.
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let action = workspace.path().join("projects");
|
||||
let policy = policy_with(action.clone(), workspace.path().to_path_buf());
|
||||
|
||||
let err = resolve_artifact_path(&action, Some(&*policy), ArtifactKind::Output, "report.md")
|
||||
.expect_err("a target under workspace_dir must be refused");
|
||||
assert!(matches!(err, OffloadError::WorkspaceTarget { .. }), "{err}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rejects_workspace_internal_state_paths() {
|
||||
// `memory/` is one of the internal state dirs `is_workspace_internal_path`
|
||||
// fences off. Pointing action_dir at workspace_dir itself makes
|
||||
// `outputs/..`-free names still land on internal state once the caller
|
||||
// names one, so the more specific error wins.
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let action = workspace.path().to_path_buf();
|
||||
let policy = policy_with(action.clone(), workspace.path().to_path_buf());
|
||||
|
||||
let err = resolve_artifact_path(&action, Some(&*policy), ArtifactKind::Output, "report.md")
|
||||
.expect_err("workspace-rooted action_dir must be refused");
|
||||
// Either fail-closed variant is acceptable; both refuse the write.
|
||||
assert!(
|
||||
matches!(
|
||||
err,
|
||||
OffloadError::WorkspaceTarget { .. } | OffloadError::WorkspaceInternal { .. }
|
||||
),
|
||||
"{err}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn workspace_internal_dir_is_refused_by_the_policy_check() {
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let policy = policy_with(
|
||||
workspace.path().to_path_buf(),
|
||||
workspace.path().to_path_buf(),
|
||||
);
|
||||
// `<workspace>/memory` is workspace-internal; resolve with a kind whose
|
||||
// subdir IS `memory` is impossible, so assert the policy predicate the
|
||||
// resolver delegates to directly on the path it would build.
|
||||
assert!(policy.is_workspace_internal_path(&workspace.path().join("memory")));
|
||||
assert!(!policy.is_workspace_internal_path(&workspace.path().join("outputs")));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sanitize_component_strips_separators_and_never_returns_empty() {
|
||||
assert_eq!(sanitize_component("researcher"), "researcher");
|
||||
assert_eq!(sanitize_component("sub-12/34"), "sub-12_34");
|
||||
assert_eq!(sanitize_component("../../etc"), "______etc");
|
||||
assert_eq!(sanitize_component(""), "unknown");
|
||||
assert_eq!(sanitize_component("///"), "___");
|
||||
assert!(sanitize_component(&"x".repeat(500)).chars().count() <= 80);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn relative_to_action_dir_falls_back_to_display_for_outside_paths() {
|
||||
let action = PathBuf::from("/tmp/action");
|
||||
let outside = PathBuf::from("/var/other/file.md");
|
||||
assert_eq!(
|
||||
relative_to_action_dir(&action, &outside),
|
||||
outside.to_string_lossy()
|
||||
);
|
||||
}
|
||||
|
||||
// ── Threshold + abstract ────────────────────────────────────────────────────
|
||||
|
||||
#[test]
|
||||
fn should_offload_respects_threshold_and_the_zero_disable() {
|
||||
assert!(should_offload(100, 50));
|
||||
assert!(!should_offload(50, 50), "exactly at threshold stays inline");
|
||||
assert!(!should_offload(10, 50));
|
||||
assert!(
|
||||
!should_offload(usize::MAX, 0),
|
||||
"zero threshold disables offload"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_abstract_returns_short_content_unchanged() {
|
||||
assert_eq!(build_abstract(" short answer ", 100), "short answer");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_abstract_cuts_at_a_line_boundary_when_one_is_available() {
|
||||
let content = format!("{}\n{}", "a".repeat(60), "b".repeat(200));
|
||||
let out = build_abstract(&content, 100);
|
||||
assert!(out.ends_with("..."));
|
||||
assert!(!out.contains('b'), "should stop at the line break: {out}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_abstract_cuts_at_a_word_boundary_when_there_is_no_line_break() {
|
||||
let content = format!("{} {}", "word ".repeat(20), "tail".repeat(50));
|
||||
let out = build_abstract(&content, 60);
|
||||
assert!(out.ends_with("..."));
|
||||
assert!(out.chars().count() <= 64);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_abstract_handles_a_zero_budget_and_boundary_free_text() {
|
||||
assert_eq!(build_abstract("anything", 0), "");
|
||||
let out = build_abstract(&"x".repeat(500), 40);
|
||||
assert!(out.ends_with("..."));
|
||||
assert!(out.chars().count() <= 44);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn build_abstract_never_splits_a_multibyte_character() {
|
||||
let content = "é".repeat(400);
|
||||
let out = build_abstract(&content, 50);
|
||||
assert!(out.ends_with("..."));
|
||||
assert!(out.chars().all(|c| c == 'é' || c == '.'));
|
||||
}
|
||||
|
||||
// ── Pointer render + parse ──────────────────────────────────────────────────
|
||||
|
||||
fn sample_artifact(redacted: bool) -> OffloadedArtifact {
|
||||
OffloadedArtifact {
|
||||
kind: ArtifactKind::Output,
|
||||
relative_path: "outputs/researcher/sub-1234-result.md".to_string(),
|
||||
absolute_path: PathBuf::from("/tmp/action/outputs/researcher/sub-1234-result.md"),
|
||||
stored_bytes: 4096,
|
||||
original_bytes: 4096,
|
||||
redacted,
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pointer_carries_path_size_and_a_file_read_call() {
|
||||
let rendered = render_artifact_pointer(&sample_artifact(false), "two-line abstract");
|
||||
assert!(rendered.starts_with(ARTIFACT_POINTER_PREFIX));
|
||||
assert!(rendered.contains("kind=output"));
|
||||
assert!(rendered.contains("path=outputs/researcher/sub-1234-result.md"));
|
||||
assert!(rendered.contains("bytes=4096"));
|
||||
assert!(rendered
|
||||
.contains(r#"read_with: file_read {"path":"outputs/researcher/sub-1234-result.md"}"#));
|
||||
assert!(rendered.contains("[abstract]\ntwo-line abstract"));
|
||||
assert!(!rendered.contains("redaction was applied"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pointer_discloses_redaction_when_it_happened() {
|
||||
let rendered = render_artifact_pointer(&sample_artifact(true), "abstract");
|
||||
assert!(rendered.contains("Credential/PII redaction was applied"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_artifact_paths_reads_pointers_out_of_a_handoff() {
|
||||
let rendered = render_artifact_pointer(&sample_artifact(false), "abstract");
|
||||
assert_eq!(
|
||||
extract_artifact_paths(&rendered),
|
||||
vec!["outputs/researcher/sub-1234-result.md".to_string()]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_artifact_paths_dedupes_and_keeps_encounter_order() {
|
||||
let text = "[artifact] kind=output path=outputs/a.md bytes=1\n\
|
||||
prose in between\n\
|
||||
[artifact] kind=scratch path=workspace/b.md bytes=2\n\
|
||||
[artifact] kind=output path=outputs/a.md bytes=1\n";
|
||||
assert_eq!(
|
||||
extract_artifact_paths(text),
|
||||
vec!["outputs/a.md".to_string(), "workspace/b.md".to_string()]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_artifact_paths_ignores_non_pointer_and_malformed_lines() {
|
||||
let text = "ordinary answer text\n\
|
||||
[artifact] kind=output bytes=1\n\
|
||||
[artifact] kind=output path= bytes=1\n\
|
||||
the word [artifact] appearing mid-sentence path=nope\n";
|
||||
assert!(extract_artifact_paths(text).is_empty());
|
||||
assert!(extract_artifact_paths("").is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn note_artifact_handoff_reports_how_many_paths_crossed() {
|
||||
let paths = vec!["outputs/a.md".to_string(), "outputs/b.md".to_string()];
|
||||
assert_eq!(
|
||||
note_artifact_handoff(HANDOFF_STAGE_RECORDED, "researcher", "sub-1", &paths),
|
||||
2
|
||||
);
|
||||
assert_eq!(
|
||||
note_artifact_handoff(HANDOFF_STAGE_CONSUMED, "researcher", "sub-1", &[]),
|
||||
0
|
||||
);
|
||||
// The two ends of one pointer must be distinguishable in a run journal.
|
||||
assert_ne!(HANDOFF_STAGE_RECORDED, HANDOFF_STAGE_CONSUMED);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn offload_threshold_tightens_to_an_agents_own_result_cap() {
|
||||
// A cap below the default would truncate the result before offload ever
|
||||
// fired (flow_memory_agent 4 000, context_scout 5 000).
|
||||
assert_eq!(
|
||||
effective_offload_threshold(DEFAULT_OFFLOAD_THRESHOLD_BYTES, Some(4_000)),
|
||||
4_000
|
||||
);
|
||||
// A cap above the default leaves the default in charge.
|
||||
assert_eq!(
|
||||
effective_offload_threshold(DEFAULT_OFFLOAD_THRESHOLD_BYTES, Some(50_000)),
|
||||
DEFAULT_OFFLOAD_THRESHOLD_BYTES
|
||||
);
|
||||
// Uncapped agents (agent_memory) and a nonsense zero cap keep the default.
|
||||
assert_eq!(
|
||||
effective_offload_threshold(DEFAULT_OFFLOAD_THRESHOLD_BYTES, None),
|
||||
DEFAULT_OFFLOAD_THRESHOLD_BYTES
|
||||
);
|
||||
assert_eq!(
|
||||
effective_offload_threshold(DEFAULT_OFFLOAD_THRESHOLD_BYTES, Some(0)),
|
||||
DEFAULT_OFFLOAD_THRESHOLD_BYTES
|
||||
);
|
||||
}
|
||||
|
||||
// ── Write path ──────────────────────────────────────────────────────────────
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_persists_under_outputs_and_reports_the_relative_path() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let offload = offload_for(action.path(), workspace.path());
|
||||
|
||||
let artifact = offload
|
||||
.write(ArtifactKind::Output, "researcher/report.md", "full body")
|
||||
.await
|
||||
.expect("write succeeds");
|
||||
|
||||
assert_eq!(artifact.relative_path, "outputs/researcher/report.md");
|
||||
assert_eq!(artifact.kind, ArtifactKind::Output);
|
||||
assert!(!artifact.redacted);
|
||||
assert_eq!(
|
||||
tokio::fs::read_to_string(&artifact.absolute_path)
|
||||
.await
|
||||
.unwrap(),
|
||||
"full body"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_redacts_credentials_before_they_reach_disk() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let offload = offload_for(action.path(), workspace.path());
|
||||
|
||||
let body = "findings ghp_abcdefghijklmnopqrstuvwxyz123456";
|
||||
let artifact = offload
|
||||
.write(ArtifactKind::Output, "leaky.md", body)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(artifact.redacted, "the token must be scrubbed");
|
||||
let stored = tokio::fs::read_to_string(&artifact.absolute_path)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(!stored.contains("ghp_abcdefghijklmnopqrstuvwxyz123456"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn write_refuses_a_traversal_target_without_touching_disk() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let offload = offload_for(action.path(), workspace.path());
|
||||
|
||||
let err = offload
|
||||
.write(ArtifactKind::Output, "../escaped.md", "body")
|
||||
.await
|
||||
.expect_err("traversal must be refused");
|
||||
|
||||
assert!(matches!(err, OffloadError::PathEscape { .. }), "{err}");
|
||||
assert!(!action.path().join("escaped.md").exists());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn default_result_name_sanitizes_both_identifiers() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let offload = ArtifactOffload::new(
|
||||
action.path().to_path_buf(),
|
||||
Some(policy_with(
|
||||
action.path().to_path_buf(),
|
||||
workspace.path().to_path_buf(),
|
||||
)),
|
||||
"team/researcher",
|
||||
"sub/../1",
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
offload.default_result_name(),
|
||||
"team_researcher/sub____1-result.md"
|
||||
);
|
||||
assert_eq!(offload.action_dir(), action.path());
|
||||
assert!(offload
|
||||
.resolve(ArtifactKind::Output, &offload.default_result_name())
|
||||
.is_ok());
|
||||
}
|
||||
|
||||
// ── End-to-end offload ──────────────────────────────────────────────────────
|
||||
|
||||
#[tokio::test]
|
||||
async fn oversized_result_is_offloaded_and_the_parent_gets_a_path_plus_abstract() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let offload = offload_for(action.path(), workspace.path());
|
||||
|
||||
let body = format!("HEADLINE FINDING\n{}", "detail line\n".repeat(2_000));
|
||||
let (handed_to_parent, artifact) =
|
||||
offload_oversized_result(body.clone(), &offload, DEFAULT_OFFLOAD_THRESHOLD_BYTES).await;
|
||||
|
||||
let artifact = artifact.expect("an oversized result must be offloaded");
|
||||
assert_eq!(
|
||||
artifact.relative_path,
|
||||
"outputs/researcher/sub-1234-result.md"
|
||||
);
|
||||
assert!(
|
||||
handed_to_parent.len() < body.len(),
|
||||
"the pointer must be smaller than the payload it replaces"
|
||||
);
|
||||
assert!(handed_to_parent.starts_with(ARTIFACT_POINTER_PREFIX));
|
||||
assert!(
|
||||
handed_to_parent.contains("HEADLINE FINDING"),
|
||||
"abstract keeps the lede"
|
||||
);
|
||||
assert_eq!(
|
||||
extract_artifact_paths(&handed_to_parent),
|
||||
vec![artifact.relative_path.clone()]
|
||||
);
|
||||
|
||||
// Full fidelity survives on disk — the whole point of the convention. The
|
||||
// body is byte-identical to what the worker produced, not an abstract.
|
||||
assert_eq!(
|
||||
tokio::fs::read_to_string(&artifact.absolute_path)
|
||||
.await
|
||||
.unwrap(),
|
||||
body
|
||||
);
|
||||
|
||||
// And the parent recovers it with an ordinary relative `file_read`, which
|
||||
// resolves against action_dir under the same trusted-root grant production
|
||||
// gives the projects dir.
|
||||
let reader_policy = policy_with(action.path().to_path_buf(), workspace.path().to_path_buf());
|
||||
let read = FileReadTool::new(reader_policy)
|
||||
.execute(json!({ "path": artifact.relative_path }))
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(!read.is_error, "{}", read.output());
|
||||
assert!(read.output().contains("HEADLINE FINDING"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn small_result_stays_inline() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let offload = offload_for(action.path(), workspace.path());
|
||||
|
||||
let (out, artifact) = offload_oversized_result(
|
||||
"short answer".to_string(),
|
||||
&offload,
|
||||
DEFAULT_OFFLOAD_THRESHOLD_BYTES,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(out, "short answer");
|
||||
assert!(artifact.is_none());
|
||||
assert!(!action.path().join("outputs").exists(), "no file written");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn offload_failure_keeps_the_inline_payload_for_the_summarizer_fallback() {
|
||||
// action_dir inside workspace_dir: every target is refused fail-closed, so
|
||||
// the caller must get its payload back untouched rather than losing it.
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let action = workspace.path().join("projects");
|
||||
let offload = ArtifactOffload::new(
|
||||
action.clone(),
|
||||
Some(policy_with(action, workspace.path().to_path_buf())),
|
||||
"researcher",
|
||||
"sub-1234",
|
||||
);
|
||||
|
||||
let body = "y".repeat(DEFAULT_OFFLOAD_THRESHOLD_BYTES + 1);
|
||||
let (out, artifact) =
|
||||
offload_oversized_result(body.clone(), &offload, DEFAULT_OFFLOAD_THRESHOLD_BYTES).await;
|
||||
|
||||
assert_eq!(
|
||||
out, body,
|
||||
"the inline payload must survive a refused 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 its
|
||||
// abstract from the raw output would re-expose exactly the credential
|
||||
// `write` scrubbed out of the file on disk.
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let offload = offload_for(action.path(), workspace.path());
|
||||
|
||||
let secret = "ghp_abcdefghijklmnopqrstuvwxyz123456";
|
||||
let body = format!("{secret}\n{}", "filler line\n".repeat(2_000));
|
||||
let (handed_to_parent, artifact) =
|
||||
offload_oversized_result(body, &offload, DEFAULT_OFFLOAD_THRESHOLD_BYTES).await;
|
||||
|
||||
let artifact = artifact.expect("offloaded");
|
||||
assert!(artifact.redacted);
|
||||
assert!(
|
||||
!handed_to_parent.contains(secret),
|
||||
"the abstract leaked a credential that was redacted on disk: {handed_to_parent}"
|
||||
);
|
||||
let stored = tokio::fs::read_to_string(&artifact.absolute_path)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(!stored.contains(secret));
|
||||
}
|
||||
|
||||
// Unix-only: creating a directory symlink on Windows needs a privilege the CI
|
||||
// runner does not have. The guard itself is platform-independent.
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn write_refuses_a_parent_that_symlinks_out_of_the_convention_root() {
|
||||
// `resolve_artifact_path` is lexical by necessity (the target does not
|
||||
// exist yet), so a pre-existing symlink is only catchable once the parent
|
||||
// materialises. Without this check the write would follow the link.
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let outside = tempfile::tempdir().unwrap();
|
||||
|
||||
let outputs_root = action.path().join("outputs");
|
||||
tokio::fs::create_dir_all(&outputs_root).await.unwrap();
|
||||
std::os::unix::fs::symlink(outside.path(), outputs_root.join("linked")).unwrap();
|
||||
|
||||
let offload = offload_for(action.path(), workspace.path());
|
||||
let err = offload
|
||||
.write(ArtifactKind::Output, "linked/report.md", "body")
|
||||
.await
|
||||
.expect_err("a symlinked parent must be refused");
|
||||
|
||||
assert!(matches!(err, OffloadError::SymlinkEscape { .. }), "{err}");
|
||||
assert!(
|
||||
!outside.path().join("report.md").exists(),
|
||||
"nothing may be written through the link"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn worktree_artifact_renders_a_path_the_parent_can_resolve() {
|
||||
// The worker writes inside its isolated checkout; the parent resolves
|
||||
// relative paths against its own action root. Rendering against the
|
||||
// parent's root keeps the pointer meaningful instead of handing back a bare
|
||||
// `outputs/…` that would miss the file.
|
||||
let parent_action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let worktree = parent_action.path().join("worktrees").join("w1");
|
||||
|
||||
let nested = ArtifactOffload::new(
|
||||
worktree.clone(),
|
||||
Some(policy_with(
|
||||
parent_action.path().to_path_buf(),
|
||||
workspace.path().to_path_buf(),
|
||||
)),
|
||||
"code_executor",
|
||||
"sub-7",
|
||||
)
|
||||
.with_render_root(parent_action.path().to_path_buf());
|
||||
|
||||
let artifact = nested
|
||||
.write(ArtifactKind::Output, "report.md", "body")
|
||||
.await
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
artifact.relative_path, "worktrees/w1/outputs/report.md",
|
||||
"a nested worktree stays relative to the parent's action root"
|
||||
);
|
||||
|
||||
// A worktree OUTSIDE the parent's root cannot be expressed relatively, so
|
||||
// the pointer carries the absolute path rather than a wrong relative one.
|
||||
let outside = tempfile::tempdir().unwrap();
|
||||
let detached = ArtifactOffload::new(outside.path().to_path_buf(), None, "code_executor", "s8")
|
||||
.with_render_root(parent_action.path().to_path_buf());
|
||||
let artifact = detached
|
||||
.write(ArtifactKind::Output, "report.md", "body")
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(
|
||||
std::path::Path::new(&artifact.relative_path).is_absolute(),
|
||||
"expected an absolute fallback, got {}",
|
||||
artifact.relative_path
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn offload_is_disabled_by_a_zero_threshold() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let offload = offload_for(action.path(), workspace.path());
|
||||
|
||||
let body = "z".repeat(100_000);
|
||||
let (out, artifact) = offload_oversized_result(body.clone(), &offload, 0).await;
|
||||
|
||||
assert_eq!(out, body);
|
||||
assert!(artifact.is_none());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn offload_without_a_policy_still_enforces_containment() {
|
||||
let action = tempfile::tempdir().unwrap();
|
||||
let offload = ArtifactOffload::new(action.path().to_path_buf(), None, "planner", "sub-9");
|
||||
|
||||
let artifact = offload
|
||||
.write(ArtifactKind::Scratch, "plan.md", "scratch body")
|
||||
.await
|
||||
.expect("no policy means no workspace checks, containment still applies");
|
||||
assert_eq!(artifact.relative_path, "workspace/plan.md");
|
||||
|
||||
let err = offload
|
||||
.write(ArtifactKind::Scratch, "../../outside.md", "body")
|
||||
.await
|
||||
.expect_err("traversal is refused with or without a policy");
|
||||
assert!(matches!(err, OffloadError::PathEscape { .. }), "{err}");
|
||||
}
|
||||
@@ -0,0 +1,120 @@
|
||||
//! Domain types for the worker artifact-offload convention (#3883).
|
||||
|
||||
use std::path::PathBuf;
|
||||
|
||||
/// Deliverables directory under `action_dir`. 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 `action_dir`. 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/` and replaced by a pointer + abstract.
|
||||
///
|
||||
/// ~2 000 tokens at the harness-wide 4-chars-per-token estimate (the same
|
||||
/// heuristic used by `tinyagents::payload_summarizer` and
|
||||
/// `subagent_runner::handoff`). 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 `file_read` the full artifact.
|
||||
pub const ABSTRACT_BUDGET_CHARS: usize = 600;
|
||||
|
||||
/// Line prefix every pointer line carries. Grep-friendly and the anchor
|
||||
/// [`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 `action_dir/outputs/`.
|
||||
Output,
|
||||
/// Scratch, written under `action_dir/workspace/`.
|
||||
///
|
||||
/// Note this is `action_dir/workspace`, which is NOT the core's internal
|
||||
/// `workspace_dir`. The offload resolver refuses to place an artifact under
|
||||
/// `workspace_dir` regardless of kind.
|
||||
Scratch,
|
||||
}
|
||||
|
||||
impl ArtifactKind {
|
||||
/// Directory name under `action_dir` 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 `action_dir`, always `/`-separated so it can be pasted
|
||||
/// straight into a `file_read` call.
|
||||
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 credential/PII redaction 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 the inline payload and the summarizer / truncation
|
||||
/// backstops still apply.
|
||||
#[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 action_dir, got {path}")]
|
||||
AbsolutePath { path: String },
|
||||
|
||||
/// The relative path escaped its convention directory (`..` traversal).
|
||||
#[error("artifact path escapes {root}: {path}")]
|
||||
PathEscape { root: String, path: String },
|
||||
|
||||
/// The resolved path landed inside the core's internal `workspace_dir`.
|
||||
/// Fail-closed: offload targets resolve under `action_dir`, never
|
||||
/// `workspace_dir`.
|
||||
#[error(
|
||||
"artifact path resolves inside workspace_dir, which agent writes may never reach: {path}"
|
||||
)]
|
||||
WorkspaceTarget { path: String },
|
||||
|
||||
/// The resolved path is an internal workspace state location per
|
||||
/// `SecurityPolicy::is_workspace_internal_path`.
|
||||
#[error("artifact path is workspace-internal state: {path}")]
|
||||
WorkspaceInternal { 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` 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 { path: String, resolved: String },
|
||||
|
||||
/// Creating the parent directory or writing the file failed.
|
||||
#[error("artifact write failed: {0}")]
|
||||
Io(#[from] std::io::Error),
|
||||
}
|
||||
@@ -23,6 +23,7 @@
|
||||
|
||||
pub mod agent_graph;
|
||||
pub mod archivist;
|
||||
pub mod artifact_offload;
|
||||
pub(crate) mod builtin_definitions;
|
||||
pub(crate) mod credentials;
|
||||
pub mod definition;
|
||||
@@ -47,6 +48,9 @@ pub mod turn_attachments_context;
|
||||
pub mod turn_subagent_usage;
|
||||
|
||||
pub use agent_graph::{AgentGraph, AgentTurnRequest, AgentTurnResult, AgentTurnUsage};
|
||||
// NOTE: deliberately no flat re-export here. `artifact_offload::ArtifactKind`
|
||||
// would shadow the unrelated `openhuman::artifacts::ArtifactKind` for anyone
|
||||
// glob-importing this module; callers use the `artifact_offload::` path.
|
||||
pub use definition::{
|
||||
AgentDefinition, AgentDefinitionRegistry, DefinitionSource, ModelSpec, PromptSource,
|
||||
SandboxMode, ToolScope, TriggerMemoryAgent,
|
||||
|
||||
@@ -3,6 +3,9 @@
|
||||
//! Includes the role-contract suffix, its injector, and the tool-spec
|
||||
//! deduplication helper used before sending specs to the provider.
|
||||
|
||||
use crate::openhuman::agent::harness::artifact_offload::{
|
||||
render_artifact_offload_contract, should_render_offload_contract, ARTIFACT_OFFLOAD_HEADING,
|
||||
};
|
||||
use crate::openhuman::tools::ToolSpec;
|
||||
use std::collections::HashSet;
|
||||
|
||||
@@ -77,6 +80,56 @@ pub(crate) fn append_subagent_role_contract(base_prompt: String, agent_id: &str)
|
||||
prompt
|
||||
}
|
||||
|
||||
/// Append the filesystem-offload contract (#3883), but only for a sub-agent
|
||||
/// that actually holds a file-write tool.
|
||||
///
|
||||
/// Kept separate from [`append_subagent_role_contract`] because it is **not**
|
||||
/// universal. A sub-agent's system prompt may only name tools it can really
|
||||
/// call — `researcher` is search + fetch only, and a skill-filtered specialist
|
||||
/// sees just its own toolkit. Telling either to write a file yields
|
||||
/// hallucinated calls that fail, and two guards enforce it
|
||||
/// (`researcher::prompt::tests::build_returns_nonempty_body`,
|
||||
/// `ops_tests::typed_mode_filters_tools_by_skill_filter`).
|
||||
///
|
||||
/// Agents skipped here are still covered: `offload_oversized_result` runs on
|
||||
/// every sub-agent outcome and needs no cooperation from the model.
|
||||
///
|
||||
/// The contract text is rendered from the artifact module so the directory
|
||||
/// names the model is told about can never drift from the ones
|
||||
/// `resolve_artifact_path` will actually accept. The heading check makes it
|
||||
/// idempotent, and lets an archetype prompt spell the convention out itself.
|
||||
pub(crate) fn append_artifact_offload_contract(
|
||||
base_prompt: String,
|
||||
agent_id: &str,
|
||||
visible_tool_names: &HashSet<String>,
|
||||
) -> String {
|
||||
if !should_render_offload_contract(visible_tool_names) {
|
||||
tracing::debug!(
|
||||
agent_id = %agent_id,
|
||||
"[artifact] sub-agent holds no file-write tool — skipping offload contract (harness-side offload still applies)"
|
||||
);
|
||||
return base_prompt;
|
||||
}
|
||||
if base_prompt.contains(ARTIFACT_OFFLOAD_HEADING) {
|
||||
return base_prompt;
|
||||
}
|
||||
|
||||
let mut prompt = base_prompt;
|
||||
if !prompt.ends_with('\n') {
|
||||
prompt.push('\n');
|
||||
}
|
||||
prompt.push('\n');
|
||||
prompt.push_str(&render_artifact_offload_contract());
|
||||
|
||||
tracing::debug!(
|
||||
agent_id = %agent_id,
|
||||
final_chars = prompt.chars().count(),
|
||||
"[artifact] appended offload contract to sub-agent system prompt"
|
||||
);
|
||||
|
||||
prompt
|
||||
}
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Tool-spec deduplication
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -11,6 +11,11 @@ use std::sync::Arc;
|
||||
use std::time::Instant;
|
||||
|
||||
use crate::openhuman::agent::harness::agent_graph::{AgentTurnRequest, AgentTurnUsage};
|
||||
use crate::openhuman::agent::harness::artifact_offload::{
|
||||
effective_offload_threshold, extract_artifact_paths, note_artifact_handoff,
|
||||
offload_oversized_result, ArtifactOffload, DEFAULT_OFFLOAD_THRESHOLD_BYTES,
|
||||
HANDOFF_STAGE_RECORDED,
|
||||
};
|
||||
use crate::openhuman::agent::harness::definition::{
|
||||
validate_tier_transition, AgentDefinition, AgentDefinitionRegistry, AgentTier, IterationPolicy,
|
||||
PromptSource, SandboxMode as AgentSandboxMode,
|
||||
@@ -42,7 +47,9 @@ use crate::openhuman::tools::{Tool, ToolCategory, ToolSpec};
|
||||
use tinyagents::harness::tool::SandboxMode as TinyagentsSandboxMode;
|
||||
use tinyagents::harness::workspace::WorkspaceDescriptor;
|
||||
|
||||
use super::prompt::{append_subagent_role_contract, dedup_tool_specs_by_name};
|
||||
use super::prompt::{
|
||||
append_artifact_offload_contract, append_subagent_role_contract, dedup_tool_specs_by_name,
|
||||
};
|
||||
use super::provider::{
|
||||
resolve_subagent_source, user_is_signed_in_to_composio, LazyToolkitResolver,
|
||||
};
|
||||
@@ -287,6 +294,9 @@ async fn try_deterministic_memory_retrieval(
|
||||
status: SubagentRunStatus::Completed,
|
||||
final_history: Vec::new(),
|
||||
usage: SubagentUsage::default(),
|
||||
// Deterministic memory hits are already bounded; nothing is offloaded
|
||||
// on this path.
|
||||
artifact_paths: Vec::new(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -432,6 +442,13 @@ pub async fn run_subagent(
|
||||
})
|
||||
.await?;
|
||||
|
||||
// #3883: offload an oversized worker result to `action_dir/outputs/`
|
||||
// BEFORE the cap below truncates it, so the parent receives a path plus
|
||||
// an abstract and the full-fidelity body survives on disk instead of
|
||||
// being cut. A refused or failed offload is soft: the inline payload
|
||||
// continues on to the cap and the summarizer detour exactly as before.
|
||||
offload_outcome_artifacts(&mut outcome, definition, &options, &task_id).await;
|
||||
|
||||
// Truncate result to the definition's cap if set (shared with the
|
||||
// deterministic memory fast path via `apply_max_result_chars`).
|
||||
apply_max_result_chars(&mut outcome.output, definition.max_result_chars, &definition.id);
|
||||
@@ -452,6 +469,116 @@ pub async fn run_subagent(
|
||||
.await
|
||||
}
|
||||
|
||||
/// Apply the filesystem-offload convention to a finished sub-agent run (#3883).
|
||||
///
|
||||
/// Writes an oversized result to `action_dir/outputs/` and swaps `output` for a
|
||||
/// path + abstract, then records every `[artifact]` pointer the outgoing payload
|
||||
/// carries — the harness-written one and any the worker authored itself by
|
||||
/// following the prompt contract — onto `SubagentRunOutcome::artifact_paths`, so
|
||||
/// the parent receives the paths structurally, not only as prose.
|
||||
///
|
||||
/// Every failure is soft. With no resolvable action root (or a refused target)
|
||||
/// the outcome is left untouched and the summarizer detour plus
|
||||
/// `tool_result_budget_bytes` truncation stay in charge as the fallback.
|
||||
async fn offload_outcome_artifacts(
|
||||
outcome: &mut SubagentRunOutcome,
|
||||
definition: &AgentDefinition,
|
||||
options: &SubagentRunOptions,
|
||||
task_id: &str,
|
||||
) {
|
||||
// Pointers the WORKER authored itself (prompt contract) are read first: the
|
||||
// harness may be about to replace `output` wholesale with its own pointer,
|
||||
// which would otherwise drop them. They also have to survive the early
|
||||
// returns below, so a run with no resolvable action root still reports the
|
||||
// paths its child wrote by hand.
|
||||
let mut paths = extract_artifact_paths(&outcome.output);
|
||||
|
||||
// A worktree-isolated worker offloads into its own checkout; everyone else
|
||||
// uses the live policy's action root, which is the same root a parent's
|
||||
// relative read resolves the returned path against.
|
||||
let policy = crate::openhuman::security::live_policy::current();
|
||||
let Some(action_dir) = options
|
||||
.worktree_action_dir
|
||||
.clone()
|
||||
.or_else(|| policy.as_ref().map(|p| p.action_dir.clone()))
|
||||
else {
|
||||
tracing::debug!(
|
||||
task_id = %task_id,
|
||||
agent_id = %outcome.agent_id,
|
||||
worker_authored_paths = paths.len(),
|
||||
"[artifact] no resolvable action_dir — skipping offload (summarizer/truncation backstop applies)"
|
||||
);
|
||||
outcome.artifact_paths = paths;
|
||||
note_artifact_handoff(
|
||||
HANDOFF_STAGE_RECORDED,
|
||||
&outcome.agent_id,
|
||||
task_id,
|
||||
&outcome.artifact_paths,
|
||||
);
|
||||
return;
|
||||
};
|
||||
|
||||
// A read-only tier means this run may not mutate the disk at all, so the
|
||||
// harness does not persist on its behalf either. The result stays inline and
|
||||
// the summarizer / truncation backstops handle it, exactly as before #3883.
|
||||
if policy
|
||||
.as_ref()
|
||||
.is_some_and(|p| p.autonomy == crate::openhuman::security::AutonomyLevel::ReadOnly)
|
||||
{
|
||||
tracing::debug!(
|
||||
task_id = %task_id,
|
||||
agent_id = %outcome.agent_id,
|
||||
"[artifact] readonly autonomy tier — skipping offload (summarizer/truncation backstop applies)"
|
||||
);
|
||||
outcome.artifact_paths = paths;
|
||||
note_artifact_handoff(
|
||||
HANDOFF_STAGE_RECORDED,
|
||||
&outcome.agent_id,
|
||||
task_id,
|
||||
&outcome.artifact_paths,
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
// Offload at the tighter of the global default and this agent's own result
|
||||
// cap, so a definition capped below the default (flow_memory_agent at 4 000
|
||||
// chars, context_scout at 5 000) gets its full body on disk instead of
|
||||
// truncated by `apply_max_result_chars` immediately after.
|
||||
let threshold =
|
||||
effective_offload_threshold(DEFAULT_OFFLOAD_THRESHOLD_BYTES, definition.max_result_chars);
|
||||
|
||||
// Worktree-isolated workers write inside their own checkout, but the parent
|
||||
// that receives the pointer resolves relative paths against ITS action root.
|
||||
// Render against that root so the handed-back path is one the parent can
|
||||
// actually open (relative when the worktree nests inside it, absolute when
|
||||
// it does not) rather than a bare `outputs/…` that silently misses.
|
||||
let render_root = policy
|
||||
.as_ref()
|
||||
.map(|p| p.action_dir.clone())
|
||||
.unwrap_or_else(|| action_dir.clone());
|
||||
let offload = ArtifactOffload::new(action_dir, policy, outcome.agent_id.clone(), task_id)
|
||||
.with_render_root(render_root);
|
||||
let (output, _artifact) =
|
||||
offload_oversized_result(std::mem::take(&mut outcome.output), &offload, threshold).await;
|
||||
outcome.output = output;
|
||||
|
||||
// Merge: the harness pointer (if it fired) plus any worker-authored pointer
|
||||
// read before the swap. `extract_artifact_paths` already de-duplicates
|
||||
// within a payload; dedupe across the two sources here.
|
||||
for path in extract_artifact_paths(&outcome.output) {
|
||||
if !paths.contains(&path) {
|
||||
paths.push(path);
|
||||
}
|
||||
}
|
||||
outcome.artifact_paths = paths;
|
||||
note_artifact_handoff(
|
||||
HANDOFF_STAGE_RECORDED,
|
||||
&outcome.agent_id,
|
||||
task_id,
|
||||
&outcome.artifact_paths,
|
||||
);
|
||||
}
|
||||
|
||||
fn workspace_descriptor_for_subagent(
|
||||
definition: &AgentDefinition,
|
||||
options: &SubagentRunOptions,
|
||||
@@ -1068,6 +1195,11 @@ async fn run_typed_mode(
|
||||
};
|
||||
|
||||
let system_prompt = append_subagent_role_contract(system_prompt, &definition.id);
|
||||
// #3883: only agents that actually hold a file-write tool are told to
|
||||
// offload. `visible_tool_names` is this sub-agent's real, post-filter tool
|
||||
// surface, so the contract can never advertise a tool the child cannot call.
|
||||
let system_prompt =
|
||||
append_artifact_offload_contract(system_prompt, &definition.id, &visible_tool_names);
|
||||
|
||||
// ── Build the user message (with optional context prefix) ──────────
|
||||
// Shared one-line stamp (#3602) so sub-agents report time in the same
|
||||
@@ -1425,6 +1557,9 @@ async fn run_typed_mode(
|
||||
status,
|
||||
final_history: history,
|
||||
usage,
|
||||
// Filled in by `run_subagent` once the offload step has had its say, so
|
||||
// both the harness-offloaded and worker-authored pointers are counted.
|
||||
artifact_paths: Vec::new(),
|
||||
})
|
||||
}
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -137,6 +137,16 @@ pub struct SubagentRunOutcome {
|
||||
/// the session totals (tokens + USD) and the global cost tracker. See
|
||||
/// [`SubagentUsage`].
|
||||
pub usage: SubagentUsage,
|
||||
/// `action_dir`-relative paths of artifacts this run handed to its parent
|
||||
/// (#3883). Populated from the `[artifact]` pointers present in `output`,
|
||||
/// whether the harness offloaded an oversized result itself or the worker
|
||||
/// followed the prompt contract and wrote the file on its own.
|
||||
///
|
||||
/// This is what makes the handoff carry **paths** rather than payloads: the
|
||||
/// parent can read any of these with `file_read` to recover full fidelity
|
||||
/// long after the child's context is gone. Empty for the common case of a
|
||||
/// small, fully inline result.
|
||||
pub artifact_paths: Vec<String>,
|
||||
}
|
||||
|
||||
/// Token + cost totals for a single sub-agent run.
|
||||
|
||||
@@ -491,6 +491,7 @@ mod tests {
|
||||
},
|
||||
final_history: Vec::new(),
|
||||
usage: SubagentUsage::default(),
|
||||
artifact_paths: Vec::new(),
|
||||
};
|
||||
|
||||
let res = awaiting_outcome_to_tool_result(&outcome, &question);
|
||||
|
||||
@@ -593,6 +593,17 @@ impl Tool for SpawnSubagentTool {
|
||||
Ok(ToolResult::success(envelope))
|
||||
}
|
||||
SubagentRunStatus::Completed => {
|
||||
// #3883: log the orchestrator taking delivery of each
|
||||
// artifact path the child handed back, so a run journal
|
||||
// shows both ends of every `[artifact]` pointer. The
|
||||
// `consumed_by_parent` stage distinguishes this from the
|
||||
// child's `recorded_by_child` line for the same path.
|
||||
crate::openhuman::agent::harness::artifact_offload::note_artifact_handoff(
|
||||
crate::openhuman::agent::harness::artifact_offload::HANDOFF_STAGE_CONSUMED,
|
||||
&outcome.agent_id,
|
||||
&outcome.task_id,
|
||||
&outcome.artifact_paths,
|
||||
);
|
||||
crate::openhuman::agent_orchestration::subagent_events::publish_subagent_completed(
|
||||
parent_session,
|
||||
outcome.task_id.clone(),
|
||||
@@ -958,6 +969,7 @@ mod tests {
|
||||
status: SubagentRunStatus::Completed,
|
||||
final_history: Vec::new(),
|
||||
usage: Default::default(),
|
||||
artifact_paths: Vec::new(),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -50,3 +50,12 @@ Return **only** valid JSON matching this schema:
|
||||
7. **No cycles** — The graph must be a DAG (directed acyclic graph).
|
||||
8. **Max 8 nodes** — Keep plans manageable. Split larger projects into multiple plans.
|
||||
9. **Read-only** — You have no write tools. If a plan depends on saving an insight, facts, or artefacts, capture that as an explicit node (e.g. "archivist: store X") in the DAG so a downstream agent performs the write.
|
||||
|
||||
## Long-horizon Artifacts
|
||||
|
||||
Plans for long-horizon work should hand data between nodes by **path**, not by pasting it forward.
|
||||
|
||||
- When a node produces a large deliverable (a report, a dataset, a generated document), say so in its description and have it written under `outputs/`.
|
||||
- Reference that path in the dependent node's description, so the downstream worker reads the file instead of receiving the payload through context.
|
||||
- `workspace/` is for per-node scratch that no later node needs.
|
||||
- Both directories are relative to the action directory. Plans must never target the core's internal workspace state.
|
||||
|
||||
@@ -29,3 +29,11 @@ You are the **Researcher** agent. You find accurate, up-to-date information.
|
||||
- If you could answer, put the answer first, then list the URLs you used.
|
||||
- If you could not answer, say exactly what is missing and what you tried.
|
||||
- Never finish with only tool calls or internal notes; the orchestrator needs a compact synthesis it can pass on or evaluate.
|
||||
|
||||
## Long-horizon Artifacts
|
||||
|
||||
You are search-and-fetch only, so you never write files yourself. What you can do is keep the handoff small.
|
||||
|
||||
- Lead with the answer and the sources that support it. A long dossier pasted into your reply costs the orchestrator context on every later step of a long-horizon task.
|
||||
- If your synthesis is genuinely large, the harness persists it under the action directory's `outputs/` folder and hands the orchestrator a path plus your abstract instead of the whole body. Write the reply so its opening lines stand alone as that abstract.
|
||||
- If a delegated task hands you an artifact path to work from, treat that path as the source of record and quote it back in your answer rather than re-pasting its contents.
|
||||
|
||||
@@ -4048,6 +4048,7 @@ async fn agent_subagent_public_types_cover_task_local_and_error_display_paths()
|
||||
status: SubagentRunStatus::Completed,
|
||||
final_history: Vec::new(),
|
||||
usage: SubagentUsage::default(),
|
||||
artifact_paths: Vec::new(),
|
||||
};
|
||||
assert_eq!(outcome.mode.as_str(), "typed");
|
||||
assert_eq!(outcome.elapsed.as_millis(), 12);
|
||||
|
||||
Reference in New Issue
Block a user