fix(missions): a repo-less mission threw away everything its agents wrote
`capture_finished_coding_phases` selects `AND m.repo_id IS NOT NULL`. Every
`research_only` mission is repo-less by design (`requires_repo = false`), so the
whole capture path — including the `sync_out` that copies the agent's work OUT
of the container — never ran, and the container was reaped unread.
Measured on the real mission `019fdc35` ("ClawHDF5 Research"): four agents, 9.5
minutes, EIGHT research documents — an HDF5 parser design, a Rust ecosystem
survey, a seven-crate dependency map, tracing and fuzzing strategy. Result:
`mission_artifacts` = 0, mission `completed`. Not recoverable: no container, no
volume, nothing under the missions root.
The platform did not merely fail to save the work — it INSTRUCTED it. The task
preamble tells every agent "/mission/repo ... is the mission's git checkout",
whether or not one exists, and the research directive says to save findings
there. One agent recorded the contradiction verbatim: "No git repo — file is
written." It looked, saw no repo, complied anyway.
Three changes, one per link in that chain:
1. `mission_outputs::capture_repo_less_phases` — copies `/mission/repo` out of
the container and registers each file as an artifact under `_outputs/`,
which is a SIBLING of the mission dir and survives `teardown_container`.
This is also the code that finally reads `produces`, until now an inert key:
`produces = ["md","pdf"]` now drives `render_pdf` into the existing
pdf_renderer worker.
2. The preamble is conditional. A repo-less mission is told its workspace is
scratch, that git_operations has nothing to act on, and — the part that
matters — that files left there ARE collected and published. An agent told
only "there is no repo" has no reason to write anything to disk.
3. A repo-less phase that produced no files is FAILED, unless it declares
`allow_empty`. The same rule `empty_delivery_is_a_failure` applies to coding,
for the only channel these phases have. Note this is NOT that guard widened:
it keys on `files_changed`, which is meaningless with no checkout, and would
not have saved the ClawHDF5 documents.
Negative controls, each ablated and confirmed failing: ignore `has_repo` and the
preamble test fails; empty the skip-list and the capture test keeps `.git` and
`node_modules`; write artifacts inside the mission dir and the survives-the-reap
test fails.
236 lib tests pass.
This commit is contained in:
Omar Sobh
@@ -0,0 +1,330 @@
|
||||
//! Capture for missions that have no repository.
|
||||
//!
|
||||
//! `mission_delivery` captures a phase's work by diffing a git checkout. A
|
||||
//! mission with `repo_id IS NULL` — every `research_only` mission, because that
|
||||
//! recipe sets `requires_repo = false` — has no checkout, so
|
||||
//! `capture_finished_coding_phases` filters it out at the SQL level
|
||||
//! (`AND m.repo_id IS NOT NULL`) and never reads the container at all.
|
||||
//!
|
||||
//! The agents still write files. The research directive tells them to save
|
||||
//! findings under `/mission/repo/research/`, and it says so whether or not a
|
||||
//! repo exists. So the work lands in the container's own filesystem, is never
|
||||
//! collected, and is destroyed when the sweeper reaps the container.
|
||||
//!
|
||||
//! # What this cost, measured
|
||||
//!
|
||||
//! Mission `019fdc35` ("ClawHDF5 Research"): four agents, 9.5 minutes, **eight
|
||||
//! research documents** — an HDF5 parser design, a Rust ecosystem survey, a
|
||||
//! seven-crate dependency map, tracing and fuzzing strategy. `mission_artifacts`
|
||||
//! held zero rows and the mission reported `completed`. One agent's own summary
|
||||
//! recorded the situation exactly: *"No git repo — file is written."* It noticed,
|
||||
//! wrote anyway, and the platform threw the result away without a word.
|
||||
//!
|
||||
//! Nothing survived but the summarizer's account of it — which is the agents'
|
||||
//! description of the work, not the work.
|
||||
//!
|
||||
//! # Why a separate path rather than widening the diff capture
|
||||
//!
|
||||
//! There is no base commit to diff against and no branch to push, so every
|
||||
//! concept `capture_phase_diff` is built on is absent. What a repo-less mission
|
||||
//! produces is simply *files*, and the honest capture is to copy them out and
|
||||
//! register each as an artifact. `_outputs/` is deliberately a SIBLING of the
|
||||
//! mission directory and survives `teardown_container`, so artifacts registered
|
||||
//! here outlive the reap that destroyed the originals.
|
||||
|
||||
use std::path::{Path, PathBuf};
|
||||
|
||||
use sqlx::{PgPool, Row};
|
||||
use uuid::Uuid;
|
||||
|
||||
/// Directories never worth capturing, whatever an agent leaves behind.
|
||||
///
|
||||
/// Same intent as `mission_fs`'s exclusion list: a captured `.git` or
|
||||
/// `node_modules` is noise that would bury the four documents that matter.
|
||||
const SKIP_DIRS: &[&str] = &[
|
||||
".git",
|
||||
"node_modules",
|
||||
"target",
|
||||
".venv",
|
||||
"venv",
|
||||
"__pycache__",
|
||||
".cache",
|
||||
"dist",
|
||||
"build",
|
||||
];
|
||||
|
||||
/// How many phases to capture per tick, matching `CAPTURE_BATCH`.
|
||||
const BATCH: i64 = 5;
|
||||
|
||||
/// The artifact kind this path registers. Also the idempotency key: a phase with
|
||||
/// one of these has already been captured.
|
||||
pub const OUTPUT_KIND: &str = "document";
|
||||
|
||||
/// Capture the outputs of finished phases on missions that have no repo.
|
||||
pub async fn capture_repo_less_phases(pool: &PgPool) -> Result<(), String> {
|
||||
let rows = sqlx::query(
|
||||
"SELECT mp.id, mp.mission_id, mp.kind, mp.config
|
||||
FROM mission_phases mp
|
||||
JOIN missions m ON m.id = mp.mission_id
|
||||
WHERE mp.status IN ('completed', 'failed')
|
||||
AND m.repo_id IS NULL
|
||||
-- A microVM mission always has a checkout (`run_phase_in_vm` refuses
|
||||
-- to boot without one), so this path is container-only.
|
||||
AND m.runtime_kind <> 'microvm'
|
||||
AND NOT EXISTS (
|
||||
SELECT 1 FROM mission_artifacts a
|
||||
WHERE a.mission_id = mp.mission_id
|
||||
AND a.phase_id = mp.id
|
||||
AND a.kind = $2
|
||||
)
|
||||
ORDER BY mp.completed_at DESC NULLS LAST
|
||||
LIMIT $1",
|
||||
)
|
||||
.bind(BATCH)
|
||||
.bind(OUTPUT_KIND)
|
||||
.fetch_all(pool)
|
||||
.await
|
||||
.map_err(|e| format!("select repo-less phases to capture: {e}"))?;
|
||||
|
||||
for row in rows {
|
||||
let phase_id: Uuid = row.get("id");
|
||||
let mission_id: Uuid = row.get("mission_id");
|
||||
let kind: String = row.get("kind");
|
||||
let config: serde_json::Value = row.get("config");
|
||||
|
||||
let dest = outputs_dir(mission_id, phase_id);
|
||||
let captured = match collect_into(mission_id, &dest).await {
|
||||
Ok(files) => files,
|
||||
Err(e) => {
|
||||
// Loud and retryable, never silently "captured nothing": the
|
||||
// whole defect this module exists for is work disappearing
|
||||
// without a word. The next tick tries again; if the container is
|
||||
// already gone the phase is failed below on the next pass.
|
||||
eprintln!(
|
||||
"mission_outputs: could NOT collect outputs for phase {phase_id} \
|
||||
of mission {mission_id}: {e}"
|
||||
);
|
||||
continue;
|
||||
}
|
||||
};
|
||||
|
||||
let wants_pdf = produces_pdf(&config);
|
||||
for file in &captured {
|
||||
let rel = match file.strip_prefix(missions_root()) {
|
||||
Ok(r) => r.to_string_lossy().to_string(),
|
||||
Err(_) => file.to_string_lossy().to_string(),
|
||||
};
|
||||
let title = file
|
||||
.file_name()
|
||||
.map(|n| n.to_string_lossy().to_string())
|
||||
.unwrap_or_else(|| rel.clone());
|
||||
if let Err(e) = cm_db::repo::missions::register_artifact(
|
||||
pool,
|
||||
cm_db::repo::missions::RegisterArtifact {
|
||||
mission_id,
|
||||
phase_id: Some(phase_id),
|
||||
path: &rel,
|
||||
kind: OUTPUT_KIND,
|
||||
mime: Some(mime_for(file)),
|
||||
title: Some(&title),
|
||||
generated_by_run: None,
|
||||
// `produces` used to be inert — declared by the recipe and
|
||||
// read by nothing. This is the code that reads it.
|
||||
render_pdf: wants_pdf && is_markdown(file),
|
||||
metadata: Some(serde_json::json!({
|
||||
"bytes": std::fs::metadata(file).map(|m| m.len()).unwrap_or(0),
|
||||
"captured_from": "/mission/repo",
|
||||
})),
|
||||
},
|
||||
)
|
||||
.await
|
||||
{
|
||||
eprintln!("mission_outputs: registering {rel}: {e}");
|
||||
}
|
||||
}
|
||||
|
||||
if captured.is_empty() && !allow_empty(&config) {
|
||||
// The same rule `empty_delivery_is_a_failure` applies to a coding
|
||||
// phase, for the only channel a repo-less phase has. Without it a
|
||||
// research mission that produced nothing is indistinguishable from
|
||||
// one that produced eight documents — both `completed`.
|
||||
eprintln!(
|
||||
"mission_outputs: phase {phase_id} ({kind}) of mission {mission_id} produced \
|
||||
NO output files — failing it. Set config.allow_empty = true if this phase is \
|
||||
meant to think rather than produce."
|
||||
);
|
||||
if let Err(e) = sqlx::query("UPDATE mission_phases SET status = 'failed' WHERE id = $1")
|
||||
.bind(phase_id)
|
||||
.execute(pool)
|
||||
.await
|
||||
{
|
||||
eprintln!("mission_outputs: failing empty phase {phase_id}: {e}");
|
||||
}
|
||||
} else {
|
||||
eprintln!(
|
||||
"mission_outputs: captured {} file(s) from phase {phase_id} ({kind}) of \
|
||||
mission {mission_id}",
|
||||
captured.len()
|
||||
);
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Copy `/mission/repo` out of the mission's container and return the files kept.
|
||||
async fn collect_into(mission_id: Uuid, dest: &Path) -> Result<Vec<PathBuf>, String> {
|
||||
let container = crate::mission_runtime::container_name(mission_id);
|
||||
let docker = crate::container_exec::connect()?;
|
||||
// A stale copy from an earlier attempt would be registered as this pass's
|
||||
// output — the same "captured a tree nobody wrote" shape capture avoids.
|
||||
let _ = std::fs::remove_dir_all(dest);
|
||||
std::fs::create_dir_all(dest).map_err(|e| format!("create {}: {e}", dest.display()))?;
|
||||
crate::mission_fs::copy_out(&docker, &container, "/mission/repo", dest).await?;
|
||||
Ok(keep_files(&dest.join("repo")))
|
||||
}
|
||||
|
||||
/// Every regular file worth keeping, recursively.
|
||||
fn keep_files(root: &Path) -> Vec<PathBuf> {
|
||||
let mut out = Vec::new();
|
||||
let mut stack = vec![root.to_path_buf()];
|
||||
while let Some(dir) = stack.pop() {
|
||||
let Ok(entries) = std::fs::read_dir(&dir) else {
|
||||
continue;
|
||||
};
|
||||
for entry in entries.flatten() {
|
||||
let path = entry.path();
|
||||
let name = entry.file_name().to_string_lossy().to_string();
|
||||
if path.is_dir() {
|
||||
if !SKIP_DIRS.contains(&name.as_str()) {
|
||||
stack.push(path);
|
||||
}
|
||||
} else if path.is_file() && !name.starts_with('.') {
|
||||
out.push(path);
|
||||
}
|
||||
}
|
||||
}
|
||||
out.sort();
|
||||
out
|
||||
}
|
||||
|
||||
/// `<missions_root>/_outputs/<mission>/<phase>` — a sibling of the mission
|
||||
/// directory, so `teardown_container` reaping the mission does not take the
|
||||
/// captured artifacts with it.
|
||||
fn outputs_dir(mission_id: Uuid, phase_id: Uuid) -> PathBuf {
|
||||
missions_root()
|
||||
.join("_outputs")
|
||||
.join(mission_id.to_string())
|
||||
.join(phase_id.to_string())
|
||||
}
|
||||
|
||||
fn missions_root() -> PathBuf {
|
||||
std::env::var("CLAWMATES_MISSIONS_ROOT")
|
||||
.map(PathBuf::from)
|
||||
.unwrap_or_else(|_| PathBuf::from("/var/lib/clawmates-missions"))
|
||||
}
|
||||
|
||||
fn is_markdown(p: &Path) -> bool {
|
||||
matches!(
|
||||
p.extension().and_then(|e| e.to_str()),
|
||||
Some("md") | Some("markdown")
|
||||
)
|
||||
}
|
||||
|
||||
fn mime_for(p: &Path) -> &'static str {
|
||||
match p.extension().and_then(|e| e.to_str()) {
|
||||
Some("md") | Some("markdown") => "text/markdown",
|
||||
Some("json") => "application/json",
|
||||
Some("csv") => "text/csv",
|
||||
Some("html") => "text/html",
|
||||
_ => "text/plain",
|
||||
}
|
||||
}
|
||||
|
||||
/// Did the recipe ask for a PDF? `research_only.toml` declares
|
||||
/// `produces = ["md","pdf"]`.
|
||||
fn produces_pdf(config: &serde_json::Value) -> bool {
|
||||
config
|
||||
.get("produces")
|
||||
.and_then(|v| v.as_array())
|
||||
.map(|a| a.iter().any(|v| v.as_str() == Some("pdf")))
|
||||
.unwrap_or(false)
|
||||
}
|
||||
|
||||
fn allow_empty(config: &serde_json::Value) -> bool {
|
||||
config.get("allow_empty").and_then(|v| v.as_bool()) == Some(true)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
fn touch(p: &Path) {
|
||||
std::fs::create_dir_all(p.parent().unwrap()).unwrap();
|
||||
std::fs::write(p, "x").unwrap();
|
||||
}
|
||||
|
||||
/// The documents a research phase writes are what must come back — and the
|
||||
/// machinery around them must not.
|
||||
#[test]
|
||||
fn research_documents_are_kept_and_scaffolding_is_not() {
|
||||
let tmp = tempfile::tempdir().unwrap();
|
||||
let repo = tmp.path().join("repo");
|
||||
touch(&repo.join("research/01_repo_archaeology.md"));
|
||||
touch(&repo.join("research/02_ecosystem.md"));
|
||||
touch(&repo.join("notes.txt"));
|
||||
touch(&repo.join(".git/HEAD"));
|
||||
touch(&repo.join("node_modules/left-pad/index.js"));
|
||||
touch(&repo.join("target/debug/thing"));
|
||||
touch(&repo.join(".hidden"));
|
||||
|
||||
let kept: Vec<String> = keep_files(&repo)
|
||||
.iter()
|
||||
.map(|p| p.strip_prefix(&repo).unwrap().to_string_lossy().to_string())
|
||||
.collect();
|
||||
assert_eq!(
|
||||
kept,
|
||||
vec![
|
||||
"notes.txt".to_string(),
|
||||
"research/01_repo_archaeology.md".to_string(),
|
||||
"research/02_ecosystem.md".to_string(),
|
||||
],
|
||||
"kept: {kept:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// `produces` was registered as an INERT key — declared by the recipe and
|
||||
/// read by nothing, which is why `research_only`'s promised PDF never
|
||||
/// appeared. This is the code that makes it mean something.
|
||||
#[test]
|
||||
fn the_recipes_produces_key_decides_pdf_rendering() {
|
||||
let with = serde_json::json!({ "produces": ["md", "pdf"] });
|
||||
let without = serde_json::json!({ "produces": ["md"] });
|
||||
let absent = serde_json::json!({});
|
||||
assert!(produces_pdf(&with));
|
||||
assert!(!produces_pdf(&without));
|
||||
assert!(!produces_pdf(&absent));
|
||||
|
||||
// Only markdown is rendered; a captured JSON side-file is not a document
|
||||
// to typeset.
|
||||
assert!(is_markdown(Path::new("/x/01_notes.md")));
|
||||
assert!(!is_markdown(Path::new("/x/data.json")));
|
||||
}
|
||||
|
||||
/// Artifacts must land OUTSIDE the mission directory. `teardown_container`
|
||||
/// removes `<missions_root>/<mission_id>` wholesale, so a capture written
|
||||
/// inside it would be destroyed by the very reap it exists to survive.
|
||||
#[test]
|
||||
fn captures_survive_the_mission_directory_being_reaped() {
|
||||
let mission = Uuid::now_v7();
|
||||
let phase = Uuid::now_v7();
|
||||
let out = outputs_dir(mission, phase);
|
||||
let mission_dir = missions_root().join(mission.to_string());
|
||||
assert!(
|
||||
!out.starts_with(&mission_dir),
|
||||
"{} must not be inside {}",
|
||||
out.display(),
|
||||
mission_dir.display()
|
||||
);
|
||||
assert!(out.starts_with(missions_root().join("_outputs")), "{out:?}");
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user