Omar SobhandClaude Opus 5
cb8184e784
`capture_phase_diff_at` parsed `git diff --stat` down to three integers and
threw the filenames away. Nothing downstream could name a single file a coding
phase changed: the World can draw a coding station but nothing underneath it,
and an operator reading a mission sees "11 files" with no way to learn which.
A second `--name-status` call now records the paths into the code_diff metadata
and into `names.txt` beside `diffstat.txt`, so raw evidence survives
independently of the JSONB.
Three ways this could have been wrong, each guarded:
- Different revision or excludes from the `--stat` call would make
`files_changed` and the path list describe different diffs, with no way to
tell which lied. A source-walk test pins both to the same `base_sha` and
the same `excludes`.
- Running after `git reset --quiet` would drop newly CREATED files, since
`--intent-to-add` is what makes them visible to diff at all — and the stat
would still count them, so the list would look merely incomplete rather
than wrong. A test asserts the ordering.
- A rename is `R100\told\tnew` — three fields. Taking field two records where
the file USED to be, naming a path nobody can open, and the bug is
invisible in any repo where nothing was renamed. `changed_paths` is now
shared with auto_merge (which had the same parse) and takes the NEW path,
with tests for renames and copies.
The list is capped at 500 paths with `files_truncated` beside it: a cap that
silently clips is worse than no cap, because "touched 12 files" and "touched at
least 500" would look identical.
Co-Authored-By: Claude Opus 5 <[email protected]>
1443 lines
61 KiB
Rust
1443 lines
61 KiB
Rust
//! Getting a coding phase's work out of the mission and somewhere durable.
|
|
//!
|
|
//! Until now a coding mission produced nothing. Agents cloned a repo, edited
|
|
//! files, and thirty minutes after the mission finished `teardown_container`
|
|
//! called `remove_dir_all` on the checkout (`mission_runtime`). Nothing
|
|
//! pushed, no artifact was registered — `register_artifact` had no callers at
|
|
//! all — and the only durable output was an LLM-written narrative of what the
|
|
//! agents *said* they had done.
|
|
//!
|
|
//! This module captures the work first and publishes it second, in that
|
|
//! order and never the reverse. The patch is written to disk before any
|
|
//! remote is contacted, so a push that fails — a rotated token, a rejected
|
|
//! ref, an unreachable forge — costs a branch and not the work.
|
|
//!
|
|
//! ## Why the server does this and not the agent
|
|
//!
|
|
//! An agent that reports "I committed and pushed" is making a claim, and this
|
|
//! codebase has spent a lot of effort learning not to bank claims (see
|
|
//! `evaluator`). Server-side git means the exit codes, the shas and the
|
|
//! diffstat are ours: what lands in `mission_artifacts` is what git actually
|
|
//! did. It is also the only version that can be gated — a `commit_policy`
|
|
//! enforced by asking an agent to please check its tests first is a wish.
|
|
//!
|
|
//! ## Retention
|
|
//!
|
|
//! Output goes to `<missions_root>/_outputs/<mission>/<phase>/`, a *sibling*
|
|
//! of the per-mission directories the sweeper deletes, and outside every bind
|
|
//! mount handed to a container (`ensure_container` mounts only
|
|
//! `<root>/<mission_id>`). So agents cannot reach their own delivery record,
|
|
//! and teardown cannot take it with them.
|
|
|
|
use std::path::{Path, PathBuf};
|
|
|
|
use serde_json::json;
|
|
use uuid::Uuid;
|
|
|
|
use crate::mission_workspace;
|
|
|
|
/// Paths never worth capturing: build output and vendored dependencies. A
|
|
/// coding phase that ran `cargo build` leaves a `target/` directory larger
|
|
/// than most repositories, and a patch containing it is unreadable as well as
|
|
/// enormous.
|
|
///
|
|
/// `mission_fs` uses this same list for the TRANSPORT, and that is not a
|
|
/// convenience — it is the fix for a real failure. The diff excluded `target/`
|
|
/// while the tar that carried the tree in and out did not, so a phase that ran
|
|
/// `cargo test` shipped its whole build directory over vsock twice. `vm_collect`
|
|
/// timed out at 300s on mission 019fd43e with the agent's work finished and
|
|
/// stranded inside a VM. Two layers, one list.
|
|
pub(crate) const EXCLUDED_PATHS: &[&str] = &[
|
|
"target",
|
|
"node_modules",
|
|
".venv",
|
|
"venv",
|
|
"dist",
|
|
"build",
|
|
".next",
|
|
"__pycache__",
|
|
".pytest_cache",
|
|
".mypy_cache",
|
|
"vendor",
|
|
// Agent workaround debris. `.gitconfig_temp` appeared on mission 019fc3ba
|
|
// when an agent hit git's ownership check and wrote its own safe.directory
|
|
// config into the repository root. The cause is fixed (mission containers
|
|
// now carry GIT_CONFIG_* env), but excluding the artefact keeps a stray
|
|
// workaround out of a user's repository if an agent invents another one.
|
|
".gitconfig_temp",
|
|
".gitconfig.tmp",
|
|
];
|
|
|
|
/// Cap on the captured patch. Past this the diff is truncated with a marker
|
|
/// rather than dropped: a 40 MB patch is a signal in itself (something
|
|
/// generated or vendored got committed), and the head of it is what an
|
|
/// operator needs to see to work out what happened.
|
|
const MAX_PATCH_BYTES: usize = 4 * 1024 * 1024;
|
|
/// Cap on the recorded path list. A cap that silently truncates is worse than
|
|
/// no cap, so the metadata carries `files_truncated` beside it — a reader must
|
|
/// be able to tell "touched 12 files" from "touched at least 500".
|
|
const MAX_CAPTURED_PATHS: usize = 500;
|
|
|
|
/// Who delivery commits as.
|
|
///
|
|
/// The operator's identity by default, so pushed commits associate with their
|
|
/// forge account the way their own commits do. Overridable per deployment via
|
|
/// `CLAWMATES_COMMIT_NAME` / `CLAWMATES_COMMIT_EMAIL` — a shared instance
|
|
/// wants a bot identity here, not a person's.
|
|
///
|
|
/// What matters for correctness is only that *some* identity is always set:
|
|
/// the server container has none of its own, so `git commit` fails outright
|
|
/// without this. The particular value is attribution, not function.
|
|
///
|
|
/// Attribution alone, to be clear — the push credential is `GITEA_TOKEN` and
|
|
/// is unaffected by any of this.
|
|
const DEFAULT_COMMIT_NAME: &str = "Omar Sobh";
|
|
const DEFAULT_COMMIT_EMAIL: &str = "[email protected]";
|
|
|
|
pub(crate) fn commit_identity() -> (String, String) {
|
|
let name = std::env::var("CLAWMATES_COMMIT_NAME")
|
|
.ok()
|
|
.filter(|v| !v.trim().is_empty())
|
|
.unwrap_or_else(|| DEFAULT_COMMIT_NAME.to_string());
|
|
let email = std::env::var("CLAWMATES_COMMIT_EMAIL")
|
|
.ok()
|
|
.filter(|v| !v.trim().is_empty())
|
|
.unwrap_or_else(|| DEFAULT_COMMIT_EMAIL.to_string());
|
|
(name, email)
|
|
}
|
|
|
|
/// Ceiling on the gate's test run. Long enough for a real suite, short enough
|
|
/// that a hung test does not hold a phase open indefinitely.
|
|
const TEST_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(900);
|
|
|
|
/// What a phase produced.
|
|
#[derive(Debug, Clone)]
|
|
pub struct Capture {
|
|
pub base_sha: String,
|
|
pub files_changed: usize,
|
|
pub insertions: usize,
|
|
pub deletions: usize,
|
|
/// The phase changed nothing. Still recorded — "this coding phase wrote no
|
|
/// code" is currently invisible to an operator, and it is worth saying.
|
|
pub empty: bool,
|
|
/// Why the diff could not be computed, if it could not be. `empty` is only
|
|
/// meaningful when this is `None`: otherwise the tree was never read, and
|
|
/// callers deciding anything on the strength of "no changes" must not.
|
|
pub diff_error: Option<String>,
|
|
pub truncated: bool,
|
|
/// The paths this phase touched, with their `--name-status` letter. The
|
|
/// diffstat gives counts only; this is what lets anything downstream say
|
|
/// WHICH files changed.
|
|
pub files: Vec<(char, String)>,
|
|
/// The path list hit `MAX_CAPTURED_PATHS`. Recorded so a reader can tell a
|
|
/// complete list from a clipped one.
|
|
pub files_truncated: bool,
|
|
pub patch_path: PathBuf,
|
|
/// Set once the work has been committed to a mission branch.
|
|
pub committed: Option<Commit>,
|
|
}
|
|
|
|
/// A commit made on the mission's own branch.
|
|
#[derive(Debug, Clone)]
|
|
pub struct Commit {
|
|
pub branch: String,
|
|
pub sha: String,
|
|
}
|
|
|
|
/// Where a mission's durable output lives. Sibling of the swept per-mission
|
|
/// directories, deliberately.
|
|
pub fn outputs_root(mission_id: Uuid) -> PathBuf {
|
|
mission_workspace::missions_root()
|
|
.join("_outputs")
|
|
.join(mission_id.to_string())
|
|
}
|
|
|
|
/// Capture the working-tree diff for one phase and register it as an artifact.
|
|
///
|
|
/// Runs against the host checkout. Returns `Ok(None)` when the mission has no
|
|
/// repository — a research-only phase has nothing to capture and that is not
|
|
/// an error.
|
|
pub async fn capture_phase_diff(
|
|
pool: &sqlx::PgPool,
|
|
mission_id: Uuid,
|
|
phase_id: Uuid,
|
|
) -> Result<Option<Capture>, String> {
|
|
// The phase's pass number, so a re-run lands on its own branch instead of
|
|
// colliding with the previous attempt.
|
|
let iteration: i32 = sqlx::query_scalar("SELECT iteration FROM mission_phases WHERE id = $1")
|
|
.bind(phase_id)
|
|
.fetch_optional(pool)
|
|
.await
|
|
.ok()
|
|
.flatten()
|
|
.unwrap_or(0);
|
|
// `commit_policy` lives in the phase config, merged there from the
|
|
// workflow recipe by `phases_for_create`.
|
|
let policy: Option<String> =
|
|
sqlx::query_scalar("SELECT config->>'commit_policy' FROM mission_phases WHERE id = $1")
|
|
.bind(phase_id)
|
|
.fetch_optional(pool)
|
|
.await
|
|
.ok()
|
|
.flatten();
|
|
capture_phase_diff_at(
|
|
pool,
|
|
mission_id,
|
|
phase_id,
|
|
&mission_workspace::checkout_path(mission_id),
|
|
&outputs_root(mission_id),
|
|
iteration,
|
|
Gate::parse(policy.as_deref()),
|
|
)
|
|
.await
|
|
}
|
|
|
|
/// [`capture_phase_diff`] with the two paths supplied.
|
|
///
|
|
/// Path resolution reads `CLAWMATES_MISSIONS_ROOT`, which is process-global;
|
|
/// tests that set it race each other and silently capture the wrong tree. The
|
|
/// seam keeps the interesting behaviour — what git sees, what lands on disk,
|
|
/// what is registered — testable in parallel without touching the environment.
|
|
pub async fn capture_phase_diff_at(
|
|
pool: &sqlx::PgPool,
|
|
mission_id: Uuid,
|
|
phase_id: Uuid,
|
|
repo: &Path,
|
|
outputs: &Path,
|
|
iteration: i32,
|
|
gate: Gate,
|
|
) -> Result<Option<Capture>, String> {
|
|
let repo = repo.to_path_buf();
|
|
if !repo.is_dir() {
|
|
return Ok(None);
|
|
}
|
|
|
|
// Diff from where the mission *started*, not from HEAD.
|
|
//
|
|
// `HEAD` is the wrong baseline the moment an agent commits, and committing
|
|
// is the normal path — `rust_sdlc` has a committer role. A mission that did
|
|
// its job properly leaves a clean tree, so a HEAD-relative diff reports
|
|
// nothing changed. That is exactly what happened on mission 019fc372: the
|
|
// agent committed the file it was asked to create and capture recorded
|
|
// `empty: true` beside a commit that plainly contained the work.
|
|
//
|
|
// Diffing against the recorded clone point covers committed, staged and
|
|
// unstaged changes in one pass. Falling back to HEAD keeps checkouts made
|
|
// before the base was recorded working, at the cost of missing committed
|
|
// work — which is why the fallback says so in the metadata.
|
|
let recorded_base = mission_workspace::base_commit(&repo);
|
|
let base_is_recorded = recorded_base.is_some();
|
|
let base_sha = match recorded_base {
|
|
Some(sha) => sha,
|
|
None => git(&repo, &["rev-parse", "HEAD"])
|
|
.await
|
|
.map(|s| s.trim().to_string())
|
|
.unwrap_or_else(|_| "HEAD".to_string()),
|
|
};
|
|
|
|
// `--intent-to-add` registers untracked files with the index without
|
|
// staging their content, which is what makes them appear in `git diff`.
|
|
// Without it a phase that only *created* files would produce an empty
|
|
// patch — the most likely shape for generated code, and the worst one to
|
|
// silently lose.
|
|
let mut add = vec!["add", "--intent-to-add", "--", "."];
|
|
let excludes: Vec<String> = EXCLUDED_PATHS
|
|
.iter()
|
|
.map(|p| format!(":(exclude){p}"))
|
|
.collect();
|
|
add.extend(excludes.iter().map(String::as_str));
|
|
// A repo with nothing to add is fine; keep going and let the diff be empty.
|
|
let _ = git(&repo, &add).await;
|
|
|
|
// A failed `git diff` and a phase that changed nothing both yield an empty
|
|
// string, and `unwrap_or_default` used to erase the difference: a corrupt
|
|
// index or an unreadable base would land `empty: true, files_changed: 0` —
|
|
// byte-identical to an honest no-op, and just as quiet. Whatever went
|
|
// wrong is recorded so the artifact can say which of the two it was.
|
|
let mut diff_error: Option<String> = None;
|
|
let mut note_diff_failure = |what: &str, e: String| {
|
|
eprintln!(
|
|
"mission_delivery: mission {mission_id} phase {phase_id} could not compute \
|
|
{what} against {base_sha}: {e}"
|
|
);
|
|
if diff_error.is_none() {
|
|
diff_error = Some(format!("{what}: {}", e.chars().take(300).collect::<String>()));
|
|
}
|
|
};
|
|
|
|
let mut diff_args = vec!["diff", base_sha.as_str(), "--"];
|
|
diff_args.extend(excludes.iter().map(String::as_str));
|
|
let patch = match git(&repo, &diff_args).await {
|
|
Ok(p) => p,
|
|
Err(e) => {
|
|
note_diff_failure("patch", e);
|
|
String::new()
|
|
}
|
|
};
|
|
|
|
let mut stat_args = vec!["diff", base_sha.as_str(), "--stat", "--"];
|
|
stat_args.extend(excludes.iter().map(String::as_str));
|
|
let diffstat = match git(&repo, &stat_args).await {
|
|
Ok(s) => s,
|
|
Err(e) => {
|
|
note_diff_failure("diffstat", e);
|
|
String::new()
|
|
}
|
|
};
|
|
|
|
// The paths themselves, not just the counts.
|
|
//
|
|
// The diffstat gives three integers and throws the filenames away, so
|
|
// nothing downstream could say WHICH files a phase touched — the World
|
|
// could draw a "coding" station but nothing under it. Same `base_sha` and
|
|
// the same excludes as the `--stat` call above: if the two disagreed,
|
|
// `files_changed` and this list would contradict each other and nobody
|
|
// could tell which one lied.
|
|
//
|
|
// Must run BEFORE the reset below — `--intent-to-add` is what makes newly
|
|
// created files visible to diff at all.
|
|
let mut name_args = vec!["diff", base_sha.as_str(), "--name-status", "--"];
|
|
name_args.extend(excludes.iter().map(String::as_str));
|
|
let name_status = match git(&repo, &name_args).await {
|
|
Ok(s) => s,
|
|
Err(e) => {
|
|
note_diff_failure("name-status", e);
|
|
String::new()
|
|
}
|
|
};
|
|
|
|
// Put the index back. `--intent-to-add` is a mutation of the agent's
|
|
// workspace, and capture must not change what a later commit would see.
|
|
let _ = git(&repo, &["reset", "--quiet"]).await;
|
|
|
|
let (files_changed, insertions, deletions) = parse_diffstat(&diffstat);
|
|
// Shared with auto_merge so the two cannot disagree about what a
|
|
// `--name-status` line means (renames are three fields; the NEW path is the
|
|
// one that changed).
|
|
let all_paths = crate::auto_merge::changed_paths(&name_status);
|
|
let files_truncated = all_paths.len() > MAX_CAPTURED_PATHS;
|
|
let files: Vec<(char, String)> = all_paths.into_iter().take(MAX_CAPTURED_PATHS).collect();
|
|
let empty = patch.trim().is_empty();
|
|
let truncated = patch.len() > MAX_PATCH_BYTES;
|
|
let stored = if truncated {
|
|
let head: String = patch.chars().take(MAX_PATCH_BYTES).collect();
|
|
format!(
|
|
"{head}\n\n… patch truncated at {MAX_PATCH_BYTES} bytes \
|
|
({} bytes total) …\n",
|
|
patch.len()
|
|
)
|
|
} else {
|
|
patch
|
|
};
|
|
|
|
let dir = outputs.join(phase_id.to_string());
|
|
std::fs::create_dir_all(&dir).map_err(|e| format!("create {}: {e}", dir.display()))?;
|
|
let patch_path = dir.join("diff.patch");
|
|
std::fs::write(&patch_path, &stored)
|
|
.map_err(|e| format!("write {}: {e}", patch_path.display()))?;
|
|
// Raw evidence on disk, independent of the JSONB. When the metadata and
|
|
// the picture disagree, this is the tiebreaker.
|
|
let _ = std::fs::write(dir.join("names.txt"), &name_status);
|
|
std::fs::write(dir.join("diffstat.txt"), &diffstat)
|
|
.map_err(|e| format!("write diffstat: {e}"))?;
|
|
|
|
// Commit only after the patch is safely on disk. If this fails, the work
|
|
// is still captured and the artifact still lands — the branch is the
|
|
// convenience, the patch is the guarantee.
|
|
// Why a phase has no branch belongs in the artifact, not only in the log.
|
|
// Mission `019fc437` recorded `branch: null, push_error: null` for both
|
|
// phases — indistinguishable from a phase that was never eligible to
|
|
// commit. The reason was in stderr on the host, where nothing reading the
|
|
// mission would find it.
|
|
let mut commit_error: Option<String> = None;
|
|
let committed = match commit_phase_work(&repo, mission_id, phase_id, iteration).await {
|
|
Ok(c) => c,
|
|
Err(e) => {
|
|
eprintln!(
|
|
"mission_delivery: mission {mission_id} phase {phase_id} captured but not \
|
|
committed: {e}"
|
|
);
|
|
commit_error = Some(e.chars().take(500).collect());
|
|
None
|
|
}
|
|
};
|
|
|
|
// Hand the next phase a base that excludes this one's work. Done here
|
|
// rather than inside `commit_phase_work` so the patch on disk is already
|
|
// written: if the process dies between the two, the worst case is a phase
|
|
// that re-reports work, not a phase whose work is invisible.
|
|
if let Some(c) = committed.as_ref() {
|
|
mission_workspace::advance_base_commit(&repo, &c.sha);
|
|
}
|
|
|
|
// Gate, then publish. Both are best-effort on top of an artifact that has
|
|
// already landed: a phase whose tests fail, or whose push is rejected,
|
|
// still has its patch on disk and its work on a local branch.
|
|
//
|
|
// `empty` suppresses publishing, so a diff we could not COMPUTE would
|
|
// otherwise skip the push and leave `push_error: null` — the phase looking
|
|
// exactly like one that correctly had nothing to publish. See
|
|
// [`untrusted_empty_reason`].
|
|
let mut outcome: Option<TestOutcome> = None;
|
|
let mut published: Option<Publish> = None;
|
|
let mut publish_error: Option<String> = untrusted_empty_reason(empty, diff_error.as_deref());
|
|
if let Some(c) = committed.as_ref() {
|
|
if !empty {
|
|
if gate == Gate::OnGreenTests {
|
|
let container = std::env::var("CLAWMATES_RUNTIME_CONTAINER")
|
|
.unwrap_or_else(|_| "clawmates-runtime".to_string());
|
|
// Against a COPY, never the checkout. `verify_tests` execs
|
|
// `cargo test` in a container running as ROOT, which writes
|
|
// `target/` — in the live tree that leaves root-owned build
|
|
// output in a checkout owned by uid 65532 and breaks the
|
|
// single-writer invariant. Measured the first time this gate
|
|
// ever ran end to end: `uids=0,65532`.
|
|
//
|
|
// The gate had been implemented but never exercised (every
|
|
// harness fixture used `commit_policy: "always"`), which is why
|
|
// a bug this mechanical survived in it.
|
|
let gate_root = crate::root_copy::copy_root("_gate", mission_id);
|
|
crate::root_copy::purge(&container, &gate_root).await;
|
|
let o = match crate::root_copy::RootCopy::of(&repo, &gate_root) {
|
|
Ok(copy) => {
|
|
let r = verify_tests(copy.workdir(), &container).await;
|
|
crate::root_copy::purge(&container, &gate_root).await;
|
|
r
|
|
}
|
|
// Fail-closed: an unverifiable suite must not license a push.
|
|
Err(e) => TestOutcome::CouldNotRun(format!(
|
|
"could not copy the checkout to test it: {e}"
|
|
)),
|
|
};
|
|
// An infrastructure fault must be loud. The gate degrades
|
|
// safely either way, but "we could not run the suite" is a
|
|
// problem with the platform and needs to look like one.
|
|
if let TestOutcome::CouldNotRun(why) = &o {
|
|
eprintln!(
|
|
"mission_delivery: mission {mission_id} phase {phase_id} could NOT \
|
|
run the test suite — gating as unverified: {why}"
|
|
);
|
|
}
|
|
outcome = Some(o);
|
|
}
|
|
match push_url_for(pool, mission_id).await {
|
|
Ok(Some(url)) => {
|
|
let verified = outcome.as_ref().and_then(TestOutcome::verified);
|
|
match publish_phase_branch(&repo, &url, &c.branch, gate, verified).await {
|
|
Ok(p) => published = Some(p),
|
|
// `publish_phase_branch` only returns Err for a local
|
|
// git failure; a rejected push is Ok with an error
|
|
// inside. Both must reach the artifact.
|
|
Err(e) => {
|
|
eprintln!(
|
|
"mission_delivery: mission {mission_id} phase {phase_id} \
|
|
could not publish {}: {e}",
|
|
c.branch
|
|
);
|
|
// Both ends, not the first 500 chars. Git prints its
|
|
// REASON last — "non-fast-forward", "fetch first",
|
|
// "protected branch" — so a head-only clamp keeps the
|
|
// noise and drops the answer. A real push failure was
|
|
// recorded as two auth lines plus a branch name cut
|
|
// off mid-word, with the reject reason gone.
|
|
publish_error =
|
|
Some(crate::evaluator_tools::clamp_output(&e));
|
|
}
|
|
}
|
|
}
|
|
Ok(None) => {
|
|
// Legitimate: a mission with no repo bound has nowhere to
|
|
// push. Still recorded, because "not pushed" with no reason
|
|
// is the ambiguity this whole pass exists to remove.
|
|
publish_error =
|
|
Some("mission has no repo bound; work is committed locally only".into());
|
|
}
|
|
Err(e) => {
|
|
eprintln!(
|
|
"mission_delivery: mission {mission_id} could not resolve a push \
|
|
URL ({e}) — work is committed locally on {} but not published",
|
|
c.branch
|
|
);
|
|
publish_error = Some(format!("could not resolve push URL: {e}"));
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
let meta = json!({
|
|
"base_sha": base_sha,
|
|
"base_recorded": base_is_recorded,
|
|
"branch": published
|
|
.as_ref()
|
|
.map(|p| p.branch.clone())
|
|
.or_else(|| committed.as_ref().map(|c| c.branch.clone())),
|
|
"head_sha": committed.as_ref().map(|c| c.sha.clone()),
|
|
"commit_policy": match gate {
|
|
Gate::Always => "always",
|
|
Gate::OnGreenTests => "on_green_tests",
|
|
Gate::OnReviewerApproval => "on_reviewer_approval",
|
|
},
|
|
// `tests_verified` keeps its original tri-state meaning for existing
|
|
// readers; `tests_status` is what distinguishes the two ways of being
|
|
// null — a repo with no suite from a runtime that could not run one.
|
|
"tests_verified": outcome.as_ref().and_then(TestOutcome::verified),
|
|
"tests_status": outcome.as_ref().map(TestOutcome::status),
|
|
"tests_detail": outcome.as_ref().and_then(TestOutcome::detail),
|
|
"pushed": published.as_ref().map(|p| p.pushed),
|
|
"push_error": published
|
|
.as_ref()
|
|
.and_then(|p| p.error.clone())
|
|
.or(publish_error),
|
|
"commit_error": commit_error,
|
|
"files_changed": files_changed,
|
|
"insertions": insertions,
|
|
"deletions": deletions,
|
|
"empty": empty,
|
|
// Non-null means `empty`/`files_changed` describe a failed read, not
|
|
// an unchanged tree. Readers that treat `empty: true` as "the phase
|
|
// did nothing" must check this first.
|
|
"diff_error": diff_error,
|
|
"truncated": truncated,
|
|
// WHICH files, not just how many. Same base_sha and the same excludes
|
|
// as `files_changed`, so the two describe the same diff.
|
|
"files": files
|
|
.iter()
|
|
.map(|(st, path)| serde_json::json!({ "status": st.to_string(), "path": path }))
|
|
.collect::<Vec<_>>(),
|
|
"files_truncated": files_truncated,
|
|
"excluded_paths": EXCLUDED_PATHS,
|
|
});
|
|
std::fs::write(
|
|
dir.join("delivery.json"),
|
|
serde_json::to_string_pretty(&meta).unwrap_or_default(),
|
|
)
|
|
.map_err(|e| format!("write delivery.json: {e}"))?;
|
|
|
|
// Path is stored relative to the MISSIONS ROOT — the convention every
|
|
// artifact uses, and what `routes::missions::artifact_content` resolves
|
|
// against. (The old `pdf_renderer` claimed to match this and did not: it
|
|
// joined the mission id first, producing a doubled id and ENOENT. It is
|
|
// gone; this comment named it as the authority, which it never was.)
|
|
let rel = format!("_outputs/{mission_id}/{phase_id}/diff.patch");
|
|
cm_db::repo::missions::register_artifact(
|
|
pool,
|
|
cm_db::repo::missions::RegisterArtifact {
|
|
mission_id,
|
|
phase_id: Some(phase_id),
|
|
path: &rel,
|
|
kind: "code_diff",
|
|
mime: Some("text/x-patch"),
|
|
title: Some(if empty {
|
|
"No code changes"
|
|
} else {
|
|
"Code changes"
|
|
}),
|
|
generated_by_run: None,
|
|
render_pdf: false,
|
|
metadata: Some(meta),
|
|
},
|
|
)
|
|
.await
|
|
.map_err(|e| format!("register code_diff artifact: {e}"))?;
|
|
|
|
eprintln!(
|
|
"mission_delivery: mission {mission_id} phase {phase_id} → {} \
|
|
(+{insertions}/-{deletions} across {files_changed} file(s)){}",
|
|
if empty { "no changes" } else { "captured" },
|
|
if truncated { ", truncated" } else { "" },
|
|
);
|
|
|
|
Ok(Some(Capture {
|
|
committed,
|
|
base_sha,
|
|
files_changed,
|
|
insertions,
|
|
deletions,
|
|
empty,
|
|
diff_error,
|
|
truncated,
|
|
files,
|
|
files_truncated,
|
|
patch_path,
|
|
}))
|
|
}
|
|
|
|
/// Run git in `repo`, returning stdout.
|
|
///
|
|
/// Every invocation carries `-c safe.directory`: under `CLAWMATES_MISSION_FS=bind`
|
|
/// the server clones as uid 65532 while agents write into the same tree as
|
|
/// root, so without it git refuses the repository outright — the failure that
|
|
/// had the phase evaluator silently falling back to guesswork. Copy mode makes
|
|
/// the tree single-uid and this redundant, but it stays while the bind path is
|
|
/// still selectable: a workaround may only be deleted once the situation it
|
|
/// works around can no longer be chosen.
|
|
async fn git(repo: &Path, args: &[&str]) -> Result<String, String> {
|
|
let repo_s = repo.display().to_string();
|
|
// Owned, not `Box::leak`. The leak was justified as "the process is
|
|
// short-lived", which is true of a CLI and false of cm-api — it is a
|
|
// long-running server, so that was one permanently leaked allocation per
|
|
// git call, growing with every phase of every mission for the life of the
|
|
// process.
|
|
let mut full: Vec<String> = vec![
|
|
"-C".into(),
|
|
repo_s.clone(),
|
|
"-c".into(),
|
|
format!("safe.directory={repo_s}"),
|
|
];
|
|
full.extend(args.iter().map(|a| (*a).to_string()));
|
|
let (name, email) = commit_identity();
|
|
let mut cmd = tokio::process::Command::new("git");
|
|
cmd.args(&full);
|
|
let out = crate::mission_workspace::no_terminal_prompt(&mut cmd)
|
|
// The server container has no git identity — `git config --global
|
|
// user.email` exits 1 — so `git commit` fails with "Author identity
|
|
// unknown" unless one is supplied. Mission `019fc450` lost its first
|
|
// phase to exactly that.
|
|
//
|
|
// This was the third failure in a row whose trigger was *agent
|
|
// behaviour rather than our code*: earlier runs committed only because
|
|
// an agent had happened to run `git config user.email` in the
|
|
// checkout, leaving a local identity the server then inherited. Config
|
|
// the agent may or may not have written is not a dependency delivery
|
|
// can hold, so the identity is supplied here on every call.
|
|
//
|
|
// Environment rather than `-c`, because these override config without
|
|
// needing a leaked string per invocation, and because they name the
|
|
// committer as the pipeline — which is the truth. The agents' own
|
|
// commits keep whatever identity they set.
|
|
.env("GIT_AUTHOR_NAME", &name)
|
|
.env("GIT_AUTHOR_EMAIL", &email)
|
|
.env("GIT_COMMITTER_NAME", &name)
|
|
.env("GIT_COMMITTER_EMAIL", &email)
|
|
.output()
|
|
.await
|
|
.map_err(|e| format!("spawn git: {e}"))?;
|
|
if !out.status.success() {
|
|
return Err(format!(
|
|
"git {} → {}: {}",
|
|
args.first().copied().unwrap_or("?"),
|
|
out.status,
|
|
// Both ends, never a head-only clamp. THIS is where the reason was
|
|
// being lost: `publish_phase_branch` returns a rejected push as
|
|
// `Ok(Publish { error })`, so the string it carries was already
|
|
// truncated here — 300 chars of auth noise, with "non-fast-forward"
|
|
// cut off — before the caller's own both-ends clamp ever saw it.
|
|
// Clamping the caller fixed the path that was already fine.
|
|
crate::mission_workspace::redact_token(&crate::evaluator_tools::clamp_output(
|
|
&String::from_utf8_lossy(&out.stderr)
|
|
))
|
|
));
|
|
}
|
|
Ok(String::from_utf8_lossy(&out.stdout).into_owned())
|
|
}
|
|
|
|
/// Pull `(files, insertions, deletions)` out of `git diff --stat`'s summary.
|
|
///
|
|
/// The last line looks like
|
|
/// `3 files changed, 12 insertions(+), 4 deletions(-)`, with any of the three
|
|
/// clauses absent when its count is zero.
|
|
pub fn parse_diffstat(stat: &str) -> (usize, usize, usize) {
|
|
let Some(summary) = stat.lines().last() else {
|
|
return (0, 0, 0);
|
|
};
|
|
let mut files = 0;
|
|
let mut ins = 0;
|
|
let mut del = 0;
|
|
for part in summary.split(',') {
|
|
let part = part.trim();
|
|
let Some((count, rest)) = part.split_once(' ') else {
|
|
continue;
|
|
};
|
|
let Ok(n) = count.parse::<usize>() else {
|
|
continue;
|
|
};
|
|
if rest.starts_with("file") {
|
|
files = n;
|
|
} else if rest.starts_with("insertion") {
|
|
ins = n;
|
|
} else if rest.starts_with("deletion") {
|
|
del = n;
|
|
}
|
|
}
|
|
(files, ins, del)
|
|
}
|
|
|
|
/// Commit a phase's work onto a branch of its own.
|
|
///
|
|
/// Runs after capture, never before: the patch is already on disk and
|
|
/// registered, so a commit that goes wrong costs a branch and not the work.
|
|
///
|
|
/// Three rules, none of them negotiable:
|
|
///
|
|
/// - **Never the default branch.** The branch name is derived from the mission
|
|
/// and phase, so a mission can only ever add a ref nobody else owns.
|
|
/// - **Never force.** A rejected update is reported, not overwritten.
|
|
/// - **Same exclusions as capture.** Whatever was too noisy to put in a patch
|
|
/// is too noisy to put in someone's history — build output, vendored trees,
|
|
/// and the workaround files agents write when infrastructure fights them.
|
|
///
|
|
/// Returns `Ok(None)` when there is nothing to commit, which is a normal
|
|
/// outcome and not an error: the phase may have changed nothing, or the agents
|
|
/// may have committed their own work already.
|
|
pub async fn commit_phase_work(
|
|
repo: &Path,
|
|
mission_id: Uuid,
|
|
phase_id: Uuid,
|
|
iteration: i32,
|
|
) -> Result<Option<Commit>, String> {
|
|
let branch = branch_name(mission_id, phase_id, iteration);
|
|
|
|
// Work already committed by the agents still needs a branch pointing at
|
|
// it, or it is unreachable once the checkout is reaped. So the branch is
|
|
// created regardless, and only the staging step is conditional.
|
|
git(repo, &["checkout", "-B", &branch]).await?;
|
|
|
|
let mut add: Vec<&str> = vec!["add", "--", "."];
|
|
let excludes: Vec<String> = EXCLUDED_PATHS
|
|
.iter()
|
|
.map(|p| format!(":(exclude){p}"))
|
|
.collect();
|
|
add.extend(excludes.iter().map(String::as_str));
|
|
git(repo, &add).await?;
|
|
|
|
// `--cached` compares the index against HEAD: empty means the agents left
|
|
// nothing unstaged for us, which is the normal case when they committed
|
|
// themselves.
|
|
let staged = git(repo, &["diff", "--cached", "--stat"])
|
|
.await
|
|
.unwrap_or_default();
|
|
if !staged.trim().is_empty() {
|
|
let message = format!(
|
|
// The trailer is the provenance record. It matters more now that
|
|
// the author line carries a person's name: without it, autonomous
|
|
// work would be indistinguishable from hand-written commits in
|
|
// `git log`. Keep it on any change to this message.
|
|
"clawmates: phase work{}\n\
|
|
\n\
|
|
Mission: {mission_id}\n\
|
|
Phase: {phase_id}\n\
|
|
\n\
|
|
Committed by the ClawMates delivery pipeline from the agents' \
|
|
working tree. Authored by agents, not by the named committer.",
|
|
if iteration > 0 {
|
|
format!(" (pass {})", iteration + 1)
|
|
} else {
|
|
String::new()
|
|
}
|
|
);
|
|
clear_stale_commit_editmsg(repo);
|
|
git(repo, &["commit", "--no-verify", "-m", &message]).await?;
|
|
}
|
|
|
|
let sha = git(repo, &["rev-parse", "HEAD"])
|
|
.await
|
|
.map(|s| s.trim().to_string())
|
|
.unwrap_or_default();
|
|
if sha.is_empty() {
|
|
return Ok(None);
|
|
}
|
|
eprintln!(
|
|
"mission_delivery: mission {mission_id} phase {phase_id} → branch {branch} at {}",
|
|
&sha[..sha.len().min(8)]
|
|
);
|
|
Ok(Some(Commit { branch, sha }))
|
|
}
|
|
|
|
/// The branch a phase's work lands on.
|
|
///
|
|
/// Namespaced under `clawmates/` so it is obvious in a branch list who created
|
|
/// it and safe to delete in bulk.
|
|
///
|
|
/// The two segments are taken from opposite ends of the ids, and that is
|
|
/// load-bearing. Both are UUIDv7, which leads with a 48-bit timestamp, so ids
|
|
/// minted in the same millisecond share their leading hex — taking `[..8]` of
|
|
/// each produced `clawmates/mission-019fc40e-019fc40e` in production, the same
|
|
/// branch for every phase of the mission, each one silently moving the ref the
|
|
/// last phase had just set. The mission keeps its time-ordered prefix so
|
|
/// branches group and sort usefully; the phase contributes its random tail so
|
|
/// sibling phases cannot collide.
|
|
pub fn branch_name(mission_id: Uuid, phase_id: Uuid, iteration: i32) -> String {
|
|
let m = mission_id.simple().to_string();
|
|
let p = phase_id.simple().to_string();
|
|
let base = format!("clawmates/mission-{}-{}", &m[..8], &p[p.len() - 8..]);
|
|
if iteration > 0 {
|
|
format!("{base}-i{}", iteration + 1)
|
|
} else {
|
|
base
|
|
}
|
|
}
|
|
|
|
/// What a phase's `commit_policy` requires before its branch may be published.
|
|
///
|
|
/// Declared in `templates/workflows/*.toml` and merged into `mission_phases.
|
|
/// config`. Until now it had no reader at all — three recipes have been
|
|
/// carrying `commit_policy = "on_green_tests"` that did precisely nothing.
|
|
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
|
pub enum Gate {
|
|
/// Publish unconditionally.
|
|
Always,
|
|
/// Publish to the mission branch only if the project's own tests pass.
|
|
OnGreenTests,
|
|
/// Publish to a review branch and wait for a human.
|
|
OnReviewerApproval,
|
|
}
|
|
|
|
impl Gate {
|
|
pub fn parse(policy: Option<&str>) -> Gate {
|
|
match policy.map(str::trim) {
|
|
Some("on_green_tests") => Gate::OnGreenTests,
|
|
Some("on_reviewer_approval") => Gate::OnReviewerApproval,
|
|
Some("always") | None | Some("") => Gate::Always,
|
|
Some(other) => {
|
|
eprintln!(
|
|
"mission_delivery: unknown commit_policy {other:?} — treating as `always`"
|
|
);
|
|
Gate::Always
|
|
}
|
|
}
|
|
}
|
|
|
|
/// The branch suffix that carries the verdict to a human.
|
|
///
|
|
/// A failed gate never discards work — it changes where the work lands.
|
|
/// Deleting a red-test branch is how you get back to the old behaviour
|
|
/// (work destroyed) with extra steps; a `-wip` branch is a thing someone
|
|
/// can look at, fix, and push properly.
|
|
pub fn branch_suffix(self, verified: Option<bool>) -> &'static str {
|
|
match (self, verified) {
|
|
(Gate::Always, _) => "",
|
|
(Gate::OnGreenTests, Some(true)) => "",
|
|
// Red, unrunnable, or no test command found — all "not proven".
|
|
(Gate::OnGreenTests, _) => "-wip",
|
|
(Gate::OnReviewerApproval, _) => "-review",
|
|
}
|
|
}
|
|
}
|
|
|
|
/// The command that runs a project's own tests, inferred from what is in the
|
|
/// tree.
|
|
///
|
|
/// Returns `None` when nothing recognisable is present, which
|
|
/// [`Gate::branch_suffix`] treats as unproven rather than as passing —
|
|
/// "we could not check" must never read as "it is fine".
|
|
pub fn discover_test_command(repo: &Path) -> Option<Vec<String>> {
|
|
let has = |f: &str| repo.join(f).exists();
|
|
if has("Cargo.toml") {
|
|
return Some(vec!["cargo".into(), "test".into(), "--quiet".into()]);
|
|
}
|
|
if has("package.json") {
|
|
let pkg = std::fs::read_to_string(repo.join("package.json")).unwrap_or_default();
|
|
// Only claim a test command when the project actually declares one;
|
|
// `npm test` on a package without a test script exits non-zero and
|
|
// would read as a red suite rather than as "nothing to run".
|
|
if pkg.contains("\"test\"") {
|
|
return Some(vec!["npm".into(), "test".into(), "--silent".into()]);
|
|
}
|
|
return None;
|
|
}
|
|
if has("pyproject.toml") || has("pytest.ini") || repo.join("tests").is_dir() {
|
|
return Some(vec!["pytest".into(), "-q".into()]);
|
|
}
|
|
None
|
|
}
|
|
|
|
/// The authenticated URL to push this mission's work to.
|
|
///
|
|
/// Built fresh from the repo row and the ambient token rather than read from
|
|
/// `.git/config`, which no longer carries credentials — the token is scrubbed
|
|
/// after clone because agents run as root in a container that mounts the
|
|
/// checkout. Building it here also means a rotated token takes effect
|
|
/// immediately instead of at the next clone.
|
|
async fn push_url_for(pool: &sqlx::PgPool, mission_id: Uuid) -> Result<Option<String>, String> {
|
|
let url: Option<String> = sqlx::query_scalar(
|
|
"SELECT r.clone_url FROM missions m JOIN repos r ON r.id = m.repo_id WHERE m.id = $1",
|
|
)
|
|
.bind(mission_id)
|
|
.fetch_optional(pool)
|
|
.await
|
|
// `.ok().flatten()` used to collapse a failed query into the same `None`
|
|
// as a mission with no repo bound, so a database fault was recorded as
|
|
// "nothing to push to" — the shape that made `commit_error` necessary.
|
|
.map_err(|e| format!("query push URL: {e}"))?
|
|
.flatten();
|
|
let Some(url) = url else { return Ok(None) };
|
|
|
|
let auth = mission_workspace::with_ambient_auth(&url);
|
|
// Fail here, not at the tty. An unauthenticated URL to OUR forge cannot
|
|
// push, and every second it survives past this point is spent producing a
|
|
// symptom that looks like something else: git asking for a username, then
|
|
// `/dev/tty: No such device or address`, then a `push_error` about auth that
|
|
// sent #55's investigation after credentials which were never the problem.
|
|
// A third-party host is left alone — ssh keys and .netrc are legitimate.
|
|
if let Some(why) = &auth.unauthenticated {
|
|
if auth.is_forge() {
|
|
return Err(format!(
|
|
"cannot authenticate the push URL for this mission — {why}. The work \
|
|
is committed locally; fix the credential and re-run delivery."
|
|
));
|
|
}
|
|
eprintln!("mission_delivery: pushing to a non-forge remote unauthenticated — {why}");
|
|
}
|
|
Ok(Some(auth.url))
|
|
}
|
|
|
|
/// Did the forge reject this push because our history diverged from the ref?
|
|
///
|
|
/// Git says this several ways depending on version and refspec, and all of them
|
|
/// mean the same thing here: the branch already exists with commits ours does not
|
|
/// contain.
|
|
fn is_non_fast_forward(err: &str) -> bool {
|
|
let e = err.to_ascii_lowercase();
|
|
e.contains("non-fast-forward")
|
|
|| e.contains("fetch first")
|
|
|| e.contains("updates were rejected")
|
|
|| (e.contains("[rejected]") && !e.contains("stale info"))
|
|
}
|
|
|
|
/// Run the gate, then push the branch if the gate allows it.
|
|
///
|
|
/// Publishing is last on purpose. By the time this runs the patch is on disk,
|
|
/// the artifact is registered and the work is committed to a local branch — so
|
|
/// every failure mode here costs a ref that did not reach the forge, and
|
|
/// nothing that was already captured.
|
|
///
|
|
/// The branch name carries the verdict. A gate that fails redirects to
|
|
/// `<branch>-wip` or `<branch>-review` and pushes it anyway: a human can
|
|
/// inspect, fix and re-push a branch, but cannot recover work that was thrown
|
|
/// away for failing a test. Deleting a red branch reproduces the old
|
|
/// behaviour — work destroyed — deliberately rather than by accident.
|
|
pub async fn publish_phase_branch(
|
|
repo: &Path,
|
|
push_url: &str,
|
|
branch: &str,
|
|
gate: Gate,
|
|
verified: Option<bool>,
|
|
) -> Result<Publish, String> {
|
|
let suffix = gate.branch_suffix(verified);
|
|
let target = format!("{branch}{suffix}");
|
|
if !suffix.is_empty() {
|
|
// Move the local ref too, so the checkout and the forge agree about
|
|
// where this work lives.
|
|
git(repo, &["branch", "-f", &target, "HEAD"]).await?;
|
|
}
|
|
|
|
// Never force. A rejected update is reported and left alone: the remote
|
|
// ref belongs to whoever set it, and overwriting it to make delivery look
|
|
// tidy is how a mission eats someone else's commit.
|
|
let refspec = format!("HEAD:refs/heads/{target}");
|
|
match git(repo, &["push", push_url, &refspec]).await {
|
|
Ok(_) => {
|
|
eprintln!("mission_delivery: pushed {target}");
|
|
Ok(Publish {
|
|
branch: target,
|
|
pushed: true,
|
|
error: None,
|
|
})
|
|
}
|
|
// #55: a mission whose checkout was re-cloned — a retry, a container
|
|
// teardown, disk loss — builds divergent history against its OWN
|
|
// deterministic branch, and every push it ever attempts is rejected.
|
|
// Before this, that was terminal: the work stayed on a local branch in a
|
|
// directory that gets reaped.
|
|
//
|
|
// The escape is a NEW ref, not `--force`. Forcing would overwrite
|
|
// whatever the earlier attempt pushed — which may be the only copy of
|
|
// that work — to make this attempt look tidy. Suffixing with the commit
|
|
// sha is deterministic (the same history always lands on the same ref),
|
|
// self-describing in a branch list, and cannot collide, since divergent
|
|
// history is by definition a different sha.
|
|
Err(e) if is_non_fast_forward(&e) => {
|
|
let sha = git(repo, &["rev-parse", "HEAD"])
|
|
.await
|
|
.map(|s| s.trim().to_string())
|
|
.unwrap_or_default();
|
|
let Some(short) = sha.get(..8) else {
|
|
eprintln!("mission_delivery: push of {target} rejected and HEAD unreadable: {e}");
|
|
return Ok(Publish {
|
|
branch: target,
|
|
pushed: false,
|
|
error: Some(e),
|
|
});
|
|
};
|
|
let alt = format!("{target}-{short}");
|
|
eprintln!(
|
|
"mission_delivery: {target} exists on the forge with history this \
|
|
checkout does not contain — pushing to {alt} instead of forcing. \
|
|
Original: {e}"
|
|
);
|
|
git(repo, &["branch", "-f", &alt, "HEAD"]).await?;
|
|
match git(repo, &["push", push_url, &format!("HEAD:refs/heads/{alt}")]).await {
|
|
Ok(_) => Ok(Publish {
|
|
branch: alt,
|
|
pushed: true,
|
|
// Not an error — the work reached the forge — but the
|
|
// redirect is a fact the operator needs, or two branches for
|
|
// one phase look like a bug rather than a rescue.
|
|
error: None,
|
|
}),
|
|
Err(e2) => Ok(Publish {
|
|
branch: alt,
|
|
pushed: false,
|
|
error: Some(format!("{e}\n\nand the diverged-history retry also failed: {e2}")),
|
|
}),
|
|
}
|
|
}
|
|
Err(e) => {
|
|
// Redacted by `git`'s error path already; the patch and the local
|
|
// branch both survive, so this is a degraded success.
|
|
eprintln!("mission_delivery: push of {target} failed: {e}");
|
|
Ok(Publish {
|
|
branch: target,
|
|
pushed: false,
|
|
error: Some(e),
|
|
})
|
|
}
|
|
}
|
|
}
|
|
|
|
/// Where a phase's work ended up, and whether the forge has it.
|
|
#[derive(Debug, Clone)]
|
|
pub struct Publish {
|
|
pub branch: String,
|
|
pub pushed: bool,
|
|
pub error: Option<String>,
|
|
}
|
|
|
|
/// Run the project's own tests to decide whether a green-tests gate is met.
|
|
///
|
|
/// `None` means "could not establish", which the gate treats as unproven. That
|
|
/// is the same fail-closed stance the phase evaluator takes, and for the same
|
|
/// reason: this codebase has repeatedly found things reporting success while
|
|
/// doing nothing, and a test suite that never ran must not license a push to a
|
|
/// mission branch.
|
|
pub async fn verify_tests(repo: &Path, container: &str) -> TestOutcome {
|
|
let Some(argv) = discover_test_command(repo) else {
|
|
return TestOutcome::NoSuite;
|
|
};
|
|
let workdir = repo.display().to_string();
|
|
let docker = match crate::container_exec::connect() {
|
|
Ok(d) => d,
|
|
Err(e) => return TestOutcome::CouldNotRun(format!("docker unreachable: {e}")),
|
|
};
|
|
match crate::container_exec::exec(&docker, container, Some(&workdir), &argv, TEST_TIMEOUT).await
|
|
{
|
|
Ok(out) => {
|
|
eprintln!(
|
|
"mission_delivery: {} → exit {:?}",
|
|
argv.join(" "),
|
|
out.exit_code
|
|
);
|
|
match out.exit_code {
|
|
Some(0) => TestOutcome::Passed,
|
|
// An unreadable status is not a pass, and it is not a red
|
|
// suite either — the command may never have started.
|
|
None => TestOutcome::CouldNotRun(format!(
|
|
"`{}` produced no exit status: {}",
|
|
argv.join(" "),
|
|
out.combined().chars().take(300).collect::<String>()
|
|
)),
|
|
Some(code) => TestOutcome::Failed(code),
|
|
}
|
|
}
|
|
Err(e) => TestOutcome::CouldNotRun(format!("exec in `{container}` failed: {e}")),
|
|
}
|
|
}
|
|
|
|
/// What happened when the gate tried to verify a phase.
|
|
///
|
|
/// This was `Option<bool>`, and collapsing four outcomes into `None` is what
|
|
/// let a missing toolchain hide for days. `clawmates-runtime` shipped without
|
|
/// `cargo`, so `verify_tests` returned `None` on every mission — identical to
|
|
/// the reading for "this repository has no test suite", which is what I
|
|
/// concluded at the time and stated in a summary. The gate behaved correctly
|
|
/// throughout (unproven is not a pass); it simply could not say *why* it was
|
|
/// unproven, so nobody could tell a repo without tests from a runtime without
|
|
/// a test runner.
|
|
///
|
|
/// Only `Passed` clears the gate. The rest differ in what an operator should
|
|
/// do about them, which is the entire reason they are separate variants.
|
|
#[derive(Debug, Clone, PartialEq, Eq)]
|
|
pub enum TestOutcome {
|
|
/// The suite ran and passed.
|
|
Passed,
|
|
/// The suite ran and failed, with its exit code.
|
|
Failed(i64),
|
|
/// No test command could be discovered for this repository.
|
|
NoSuite,
|
|
/// A suite exists but could not be executed. Always an infrastructure
|
|
/// fault on our side, never a verdict about the code.
|
|
CouldNotRun(String),
|
|
}
|
|
|
|
impl TestOutcome {
|
|
/// The gate's view: `Some(true)` only when the suite actually passed.
|
|
/// Preserved so `Gate::branch_suffix` keeps its existing contract.
|
|
pub fn verified(&self) -> Option<bool> {
|
|
match self {
|
|
TestOutcome::Passed => Some(true),
|
|
TestOutcome::Failed(_) => Some(false),
|
|
TestOutcome::NoSuite | TestOutcome::CouldNotRun(_) => None,
|
|
}
|
|
}
|
|
|
|
/// Stable machine-readable label for artifact metadata.
|
|
pub fn status(&self) -> &'static str {
|
|
match self {
|
|
TestOutcome::Passed => "passed",
|
|
TestOutcome::Failed(_) => "failed",
|
|
TestOutcome::NoSuite => "no_suite",
|
|
TestOutcome::CouldNotRun(_) => "could_not_run",
|
|
}
|
|
}
|
|
|
|
/// Human-readable detail, when there is any beyond the label.
|
|
pub fn detail(&self) -> Option<String> {
|
|
match self {
|
|
TestOutcome::Passed | TestOutcome::NoSuite => None,
|
|
TestOutcome::Failed(code) => Some(format!("test command exited {code}")),
|
|
TestOutcome::CouldNotRun(why) => Some(why.clone()),
|
|
}
|
|
}
|
|
}
|
|
|
|
/// Remove a `COMMIT_EDITMSG` the agent left behind as root.
|
|
///
|
|
/// The checkout is shared between the server (uid 65532) and the agent
|
|
/// container (root). `core.sharedRepository` makes git create *objects and
|
|
/// refs* group-writable — `.git/index` lands as 0666, which is why commits
|
|
/// work at all — but it does not cover `COMMIT_EDITMSG`, which git writes
|
|
/// with the default umask. An agent that runs `git commit` itself leaves that
|
|
/// file owned by root at 0644, and the server's next commit dies with:
|
|
///
|
|
/// ```text
|
|
/// git commit → exit 128: could not open '.git/COMMIT_EDITMSG': Permission denied
|
|
/// ```
|
|
///
|
|
/// Observed on mission `019fcd0c`, which produced correct work — a reviewed,
|
|
/// tested function plus a REVIEW.md quoting a real `cargo test` summary — and
|
|
/// then delivered none of it.
|
|
///
|
|
/// Unlinking works where overwriting does not: removing a file requires write
|
|
/// permission on the *directory*, and `.git/` is owned by the server. Silent
|
|
/// on failure by design — if the file is absent or cannot be removed, the
|
|
/// commit below reports the real error rather than this speculative cleanup.
|
|
fn clear_stale_commit_editmsg(repo: &Path) {
|
|
let msg = repo.join(".git/COMMIT_EDITMSG");
|
|
if msg.exists() {
|
|
let _ = std::fs::remove_file(&msg);
|
|
}
|
|
}
|
|
|
|
/// Mark a phase as impossible to capture, so it stops being selected.
|
|
///
|
|
/// A phase whose checkout has already been reaped can never be captured. It
|
|
/// must still be recorded: the capture batch is bounded, and a row that stays
|
|
/// eligible forever occupies a slot forever. Enough of them and no live
|
|
/// mission is ever captured again — head-of-line blocking with a silent
|
|
/// failure mode, which is how this was found.
|
|
///
|
|
/// The artifact is deliberately honest about *why* it is empty. "No changes"
|
|
/// and "we lost the checkout before looking" are different facts, and an
|
|
/// operator reading the mission needs to be able to tell them apart.
|
|
pub async fn record_uncapturable(
|
|
pool: &sqlx::PgPool,
|
|
mission_id: Uuid,
|
|
phase_id: Uuid,
|
|
) -> Result<(), String> {
|
|
// Write a real file behind the artifact. `_outputs` survives teardown, so
|
|
// it is still writable even though the checkout is gone — and an artifact
|
|
// row pointing at a path with nothing behind it turns every reader into a
|
|
// 404 with no explanation.
|
|
let dir = outputs_root(mission_id).join(phase_id.to_string());
|
|
if std::fs::create_dir_all(&dir).is_ok() {
|
|
let _ = std::fs::write(
|
|
dir.join("diff.patch"),
|
|
"The mission checkout was removed before this phase's changes could be\n captured. Nothing was lost that had already been captured; this phase\n simply finished after its working tree had been reaped.\n",
|
|
);
|
|
}
|
|
let rel = format!("_outputs/{mission_id}/{phase_id}/diff.patch");
|
|
cm_db::repo::missions::register_artifact(
|
|
pool,
|
|
cm_db::repo::missions::RegisterArtifact {
|
|
mission_id,
|
|
phase_id: Some(phase_id),
|
|
path: &rel,
|
|
kind: "code_diff",
|
|
mime: Some("text/x-patch"),
|
|
title: Some("Not captured — checkout unavailable"),
|
|
generated_by_run: None,
|
|
render_pdf: false,
|
|
metadata: Some(json!({
|
|
"empty": true,
|
|
"captured": false,
|
|
"reason": "the mission checkout was removed before the diff could be captured",
|
|
})),
|
|
},
|
|
)
|
|
.await
|
|
.map(|_| ())
|
|
.map_err(|e| format!("register uncapturable marker: {e}"))
|
|
}
|
|
|
|
/// Why an empty patch must not be believed, if it must not be believed.
|
|
///
|
|
/// An empty patch has two causes that produce identical bytes: the tree really
|
|
/// did not change, or `git diff` failed and we have no idea what the tree
|
|
/// looks like. The first is an ordinary outcome; the second is a platform
|
|
/// fault. Returning `Some` for the second is what stops the fault from being
|
|
/// filed under the ordinary outcome — the recurring shape where a failure and
|
|
/// a legitimate negative share one representation.
|
|
fn untrusted_empty_reason(empty: bool, diff_error: Option<&str>) -> Option<String> {
|
|
match (empty, diff_error) {
|
|
(true, Some(why)) => Some(format!(
|
|
"not published: the diff could not be computed, so an empty patch \
|
|
cannot be trusted to mean an unchanged tree ({why})"
|
|
)),
|
|
_ => None,
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod changed_path_capture_tests {
|
|
/// The path list and `files_changed` must describe the SAME diff.
|
|
///
|
|
/// They come from two separate git invocations — `--stat` and
|
|
/// `--name-status`. If those are ever given different revisions or
|
|
/// different exclude pathspecs, the count and the list disagree and there
|
|
/// is no way to tell which is right: both look like plausible output.
|
|
#[test]
|
|
fn both_diff_calls_use_the_same_revision_and_excludes() {
|
|
let src = include_str!("mission_delivery.rs");
|
|
let body = src
|
|
.split("let mut stat_args")
|
|
.nth(1)
|
|
.and_then(|s| s.split("let (files_changed").next())
|
|
.expect("the capture block");
|
|
assert!(
|
|
body.contains("let mut name_args = vec![\"diff\", base_sha.as_str(), \"--name-status\", \"--\"]"),
|
|
"the name-status call must use the same base_sha as --stat"
|
|
);
|
|
assert!(
|
|
body.contains("name_args.extend(excludes.iter().map(String::as_str))"),
|
|
"and the same excludes, or files_changed and the path list describe \
|
|
different diffs"
|
|
);
|
|
}
|
|
|
|
/// `--name-status` must run before the index is put back, or newly created
|
|
/// files — which `--intent-to-add` is what makes visible — vanish from the
|
|
/// list while still being counted by the stat.
|
|
#[test]
|
|
fn paths_are_read_before_the_index_reset() {
|
|
let src = include_str!("mission_delivery.rs");
|
|
let name_at = src.find("--name-status").expect("name-status call");
|
|
let reset_at = src
|
|
.find("git(&repo, &[\"reset\", \"--quiet\"])")
|
|
.expect("index reset");
|
|
assert!(
|
|
name_at < reset_at,
|
|
"the path list must be captured while --intent-to-add is still in \
|
|
effect, or created files are invisible to it"
|
|
);
|
|
}
|
|
}
|
|
|
|
#[cfg(test)]
|
|
mod tests {
|
|
use super::*;
|
|
|
|
/// Git says "your history diverged" several ways, and the one production
|
|
/// actually produced (`! [rejected] ... (fetch first)`) is not the phrase
|
|
/// anyone reaches for first. Missing a phrasing means the rescue does not
|
|
/// fire and the work stays on a local branch in a directory that gets
|
|
/// reaped — silently, since the push failure is a degraded success.
|
|
#[test]
|
|
fn every_way_git_says_diverged_is_recognised() {
|
|
for e in [
|
|
"git push → exit 1: ! [rejected] HEAD -> b (fetch first)\nhint: …",
|
|
" ! [rejected] HEAD -> b (non-fast-forward)",
|
|
"hint: Updates were rejected because the remote contains work that you \
|
|
do not have locally.",
|
|
] {
|
|
assert!(is_non_fast_forward(e), "not recognised: {e}");
|
|
}
|
|
}
|
|
|
|
/// And it must not fire on failures a new branch cannot fix. Retrying a
|
|
/// permissions or network error onto a second ref just produces a second
|
|
/// failure and a confusing branch name.
|
|
#[test]
|
|
fn other_push_failures_are_not_mistaken_for_divergence() {
|
|
for e in [
|
|
"fatal: repository 'https://forge/x.git' not found",
|
|
"remote: error: GH006: Protected branch update failed",
|
|
"fatal: could not read Username for 'https://forge': terminal prompts disabled",
|
|
" ! [rejected] (stale info)",
|
|
] {
|
|
assert!(!is_non_fast_forward(e), "wrongly recognised: {e}");
|
|
}
|
|
}
|
|
|
|
// ── The gate ───────────────────────────────────────────────────────
|
|
|
|
/// Three recipes have declared `commit_policy` since they were written and
|
|
/// nothing has ever read it. The parse must at least be forgiving about an
|
|
/// unknown value rather than refusing to deliver.
|
|
#[test]
|
|
fn commit_policy_parses_the_declared_values() {
|
|
assert_eq!(Gate::parse(Some("on_green_tests")), Gate::OnGreenTests);
|
|
assert_eq!(
|
|
Gate::parse(Some("on_reviewer_approval")),
|
|
Gate::OnReviewerApproval
|
|
);
|
|
assert_eq!(Gate::parse(Some("always")), Gate::Always);
|
|
assert_eq!(Gate::parse(None), Gate::Always);
|
|
assert_eq!(Gate::parse(Some(" on_green_tests ")), Gate::OnGreenTests);
|
|
assert_eq!(
|
|
Gate::parse(Some("nonsense")),
|
|
Gate::Always,
|
|
"unknown policy still delivers"
|
|
);
|
|
}
|
|
|
|
/// A failed gate must move the work, never drop it. Deleting a red-test
|
|
/// branch reproduces the old behaviour — work destroyed — with extra steps.
|
|
#[test]
|
|
fn a_failed_gate_redirects_rather_than_discards() {
|
|
assert_eq!(Gate::OnGreenTests.branch_suffix(Some(true)), "");
|
|
assert_eq!(Gate::OnGreenTests.branch_suffix(Some(false)), "-wip");
|
|
assert_eq!(
|
|
Gate::OnReviewerApproval.branch_suffix(Some(true)),
|
|
"-review"
|
|
);
|
|
assert_eq!(
|
|
Gate::Always.branch_suffix(Some(false)),
|
|
"",
|
|
"always means always"
|
|
);
|
|
}
|
|
|
|
/// "We could not check" must not read as "it passed". An unrunnable or
|
|
/// undiscoverable test suite lands on `-wip` exactly like a red one.
|
|
#[test]
|
|
fn an_unverifiable_suite_is_not_treated_as_green() {
|
|
assert_eq!(Gate::OnGreenTests.branch_suffix(None), "-wip");
|
|
}
|
|
|
|
#[test]
|
|
fn test_command_is_discovered_from_the_tree() {
|
|
let dir = tempfile::tempdir().unwrap();
|
|
assert_eq!(
|
|
discover_test_command(dir.path()),
|
|
None,
|
|
"nothing recognisable"
|
|
);
|
|
|
|
std::fs::write(dir.path().join("Cargo.toml"), "[package]\nname=\"x\"\n").unwrap();
|
|
assert_eq!(
|
|
discover_test_command(dir.path()),
|
|
Some(vec!["cargo".into(), "test".into(), "--quiet".into()])
|
|
);
|
|
}
|
|
|
|
/// A `package.json` with no test script must yield None, not `npm test` —
|
|
/// npm exits non-zero for a missing script, which would look like a red
|
|
/// suite instead of an absent one.
|
|
#[test]
|
|
fn a_package_without_a_test_script_yields_no_command() {
|
|
let dir = tempfile::tempdir().unwrap();
|
|
std::fs::write(dir.path().join("package.json"), r#"{"name":"x"}"#).unwrap();
|
|
assert_eq!(discover_test_command(dir.path()), None);
|
|
|
|
std::fs::write(
|
|
dir.path().join("package.json"),
|
|
r#"{"name":"x","scripts":{"test":"vitest"}}"#,
|
|
)
|
|
.unwrap();
|
|
assert!(discover_test_command(dir.path()).is_some());
|
|
}
|
|
|
|
#[test]
|
|
fn diffstat_summary_is_parsed() {
|
|
assert_eq!(
|
|
parse_diffstat(" src/a.rs | 2 +-\n 3 files changed, 12 insertions(+), 4 deletions(-)"),
|
|
(3, 12, 4)
|
|
);
|
|
// Clauses are omitted when zero.
|
|
assert_eq!(
|
|
parse_diffstat(" a.rs | 1 +\n 1 file changed, 1 insertion(+)"),
|
|
(1, 1, 0)
|
|
);
|
|
assert_eq!(
|
|
parse_diffstat(" a.rs | 1 -\n 1 file changed, 1 deletion(-)"),
|
|
(1, 0, 1)
|
|
);
|
|
}
|
|
|
|
/// An empty stat means an empty phase, not a parse failure. This is the
|
|
/// case that must still produce an artifact.
|
|
/// The whole point: a tree that genuinely did not change stays silent, and
|
|
/// a diff that could not be computed does not get to borrow that silence.
|
|
#[test]
|
|
fn an_uncomputable_diff_is_not_an_unchanged_tree() {
|
|
assert_eq!(
|
|
untrusted_empty_reason(true, None),
|
|
None,
|
|
"a genuinely unchanged tree must not report an error"
|
|
);
|
|
let reason = untrusted_empty_reason(true, Some("patch: fatal: bad object"))
|
|
.expect("an empty patch from a FAILED diff must be reported, not accepted");
|
|
assert!(
|
|
reason.contains("bad object"),
|
|
"the reason must name what went wrong, got: {reason}"
|
|
);
|
|
assert_eq!(
|
|
untrusted_empty_reason(false, Some("diffstat: fatal: bad object")),
|
|
None,
|
|
"a non-empty patch stands on its own even if the diffstat failed"
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn an_empty_diffstat_is_all_zeroes() {
|
|
assert_eq!(parse_diffstat(""), (0, 0, 0));
|
|
assert_eq!(parse_diffstat("\n"), (0, 0, 0));
|
|
}
|
|
|
|
/// Build output and agent workaround debris must never reach a patch. A
|
|
/// phase that ran `cargo build` leaves a `target/` bigger than the
|
|
/// repository, and an agent that fought git's ownership check left a
|
|
/// `.gitconfig_temp` beside the real work.
|
|
#[test]
|
|
fn build_output_is_excluded() {
|
|
for p in [
|
|
"target",
|
|
"node_modules",
|
|
".venv",
|
|
"dist",
|
|
"__pycache__",
|
|
".gitconfig_temp",
|
|
] {
|
|
assert!(
|
|
EXCLUDED_PATHS.contains(&p),
|
|
"{p} must be excluded from capture"
|
|
);
|
|
}
|
|
}
|
|
|
|
/// The outputs directory must be a sibling of the per-mission directories
|
|
/// the sweeper deletes — not inside one, or teardown takes the record with
|
|
/// it, and not inside a bind mount, or agents can edit their own evidence.
|
|
#[test]
|
|
fn outputs_live_outside_the_swept_mission_directory() {
|
|
let mission = Uuid::now_v7();
|
|
let out = outputs_root(mission);
|
|
let swept = mission_workspace::checkout_path(mission);
|
|
assert!(
|
|
!out.starts_with(swept.parent().unwrap()),
|
|
"outputs must not sit under the directory teardown removes"
|
|
);
|
|
assert!(out.to_string_lossy().contains("_outputs"));
|
|
}
|
|
}
|