fix(missions): make an unrunnable test suite legible, and check the runtime at boot
ci / gates (push) Failing after 6s
ci / rust (push) Skipped
ci / frontend (push) Skipped
ci / e2e (push) Skipped
ci / publish (push) Skipped

Two changes against the same defect: the platform could not tell a missing
capability from a legitimate negative result.

verify_tests returned Option<bool>, collapsing four outcomes into None:
no suite found, docker unreachable, exec failed, and no exit status. When
clawmates-runtime shipped without cargo, every on_green_tests phase
returned None and landed on -wip — identical to the reading for "this
repo has no tests", which is the conclusion I drew and reported. The gate
was correct throughout; it simply could not say why it was unproven.

TestOutcome now names the four cases. Gating is unchanged (only Passed
clears, unproven is never a pass), and tests_verified keeps its tri-state
meaning for existing readers. tests_status and tests_detail are new, so an
artifact distinguishes no_suite from could_not_run, and a CouldNotRun is
logged as the infrastructure fault it is rather than passing quietly.

runtime_preflight probes the runtime container at boot for every tool the
platform invokes inside it and names what each absence disables. This is
the check that was missing: the Dockerfile gained a toolchain, the image
was never built, gw-04 ran the old one for days, and the only symptoms
were an ungated suite and a security scan that scanned nothing. A report,
not a gate — a missing scanner should stop us believing a scan, not stop
the server. Its test guards the probes themselves, since a typo would
produce a permanent false "missing" and train operators to ignore it.

Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
Omar Sobh
2026-08-02 17:22:47 -07:00
co-authored by Claude Opus 5
parent 9bdc3cd89b
commit f7e336ff5f
5 changed files with 350 additions and 16 deletions
+5
View File
@@ -402,6 +402,11 @@ async fn run() -> Result<(), String> {
.await .await
.map_err(|e| format!("bind {} failed: {e}", config.listen_addr))?; .map_err(|e| format!("bind {} failed: {e}", config.listen_addr))?;
println!("clawmates-server listening on {}", config.listen_addr); println!("clawmates-server listening on {}", config.listen_addr);
// Say plainly whether the mission runtime carries the tools we invoke in
// it. The image on the host silently fell behind its Dockerfile once, and
// every consequence — an ungated test suite, a scan that scanned nothing —
// looked like a normal result rather than a broken deployment.
cm_api::runtime_preflight::report_at_boot();
// Graceful shutdown: on SIGTERM/Ctrl-C, stop accepting, finish in-flight // Graceful shutdown: on SIGTERM/Ctrl-C, stop accepting, finish in-flight
// requests, then DRAIN the sandbox managers so no container is left running. // requests, then DRAIN the sandbox managers so no container is left running.
let shutdown = async move { let shutdown = async move {
+1
View File
@@ -17,6 +17,7 @@ mod mcp_skills;
pub mod mission_orchestrator; pub mod mission_orchestrator;
pub mod mission_refiner; pub mod mission_refiner;
pub mod mission_delivery; pub mod mission_delivery;
pub mod runtime_preflight;
pub mod mission_runtime; pub mod mission_runtime;
pub mod mission_workspace; pub mod mission_workspace;
pub mod node_rules; pub mod node_rules;
+106 -16
View File
@@ -293,17 +293,28 @@ pub async fn capture_phase_diff_at(
// Gate, then publish. Both are best-effort on top of an artifact that has // 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, // 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. // still has its patch on disk and its work on a local branch.
let mut verified: Option<bool> = None; let mut outcome: Option<TestOutcome> = None;
let mut published: Option<Publish> = None; let mut published: Option<Publish> = None;
if let Some(c) = committed.as_ref() { if let Some(c) = committed.as_ref() {
if !empty { if !empty {
if gate == Gate::OnGreenTests { if gate == Gate::OnGreenTests {
let container = std::env::var("CLAWMATES_RUNTIME_CONTAINER") let container = std::env::var("CLAWMATES_RUNTIME_CONTAINER")
.unwrap_or_else(|_| "clawmates-runtime".to_string()); .unwrap_or_else(|_| "clawmates-runtime".to_string());
verified = verify_tests(&repo, &container).await; let o = verify_tests(&repo, &container).await;
// 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 { match push_url_for(pool, mission_id).await {
Some(url) => { Some(url) => {
let verified = outcome.as_ref().and_then(TestOutcome::verified);
published = publish_phase_branch(&repo, &url, &c.branch, gate, verified) published = publish_phase_branch(&repo, &url, &c.branch, gate, verified)
.await .await
.ok(); .ok();
@@ -330,7 +341,12 @@ pub async fn capture_phase_diff_at(
Gate::OnGreenTests => "on_green_tests", Gate::OnGreenTests => "on_green_tests",
Gate::OnReviewerApproval => "on_reviewer_approval", Gate::OnReviewerApproval => "on_reviewer_approval",
}, },
"tests_verified": verified, // `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), "pushed": published.as_ref().map(|p| p.pushed),
"push_error": published.as_ref().and_then(|p| p.error.clone()), "push_error": published.as_ref().and_then(|p| p.error.clone()),
"commit_error": commit_error, "commit_error": commit_error,
@@ -744,20 +760,94 @@ pub struct Publish {
/// reason: this codebase has repeatedly found things reporting success while /// 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 /// doing nothing, and a test suite that never ran must not license a push to a
/// mission branch. /// mission branch.
pub async fn verify_tests(repo: &Path, container: &str) -> Option<bool> { pub async fn verify_tests(repo: &Path, container: &str) -> TestOutcome {
let argv = discover_test_command(repo)?; let Some(argv) = discover_test_command(repo) else {
return TestOutcome::NoSuite;
};
let workdir = repo.display().to_string(); let workdir = repo.display().to_string();
let docker = crate::container_exec::connect().ok()?; let docker = match crate::container_exec::connect() {
let out = crate::container_exec::exec(&docker, container, Some(&workdir), &argv, TEST_TIMEOUT) Ok(d) => d,
.await Err(e) => return TestOutcome::CouldNotRun(format!("docker unreachable: {e}")),
.ok()?; };
eprintln!( match crate::container_exec::exec(&docker, container, Some(&workdir), &argv, TEST_TIMEOUT).await
"mission_delivery: {} → exit {:?}", {
argv.join(" "), Ok(out) => {
out.exit_code eprintln!(
); "mission_delivery: {} → exit {:?}",
// An unreadable status is not a pass. argv.join(" "),
Some(out.success()) 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()),
}
}
} }
/// Mark a phase as impossible to capture, so it stops being selected. /// Mark a phase as impossible to capture, so it stops being selected.
+190
View File
@@ -0,0 +1,190 @@
//! Does the mission runtime actually carry the tools we depend on?
//!
//! Every capability in this codebase is written twice: once as code that
//! invokes a binary, and once as a Dockerfile line that installs it. The two
//! are only connected by someone having built and shipped the image, and
//! nothing checked that they agreed.
//!
//! They did not. `deploy/clawmates-runtime/Dockerfile` gained a Rust
//! toolchain, `gitleaks`, `trivy`, `semgrep` and `cargo-audit`; the image was
//! never built, and gw-04 kept running the previous one for days. The
//! consequences were all silent:
//!
//! - `verify_tests` could not launch `cargo test`, so every `on_green_tests`
//! phase landed on `-wip` — indistinguishable from "no test suite here"
//! - `security_scan` emitted `tool_error` rows and reported completion
//! - the evaluator's allow-listed checks could not run the scanners
//!
//! No error, no log line, no failing test. The code was right and the machine
//! was not. This module makes that specific disagreement observable: it asks
//! the running container what it has and says so plainly at boot.
//!
//! It is a report, not a gate. A missing scanner should not stop the server
//! from serving — it should stop us believing a scan that scanned nothing.
use crate::container_exec;
use std::time::Duration;
const PROBE_TIMEOUT: Duration = Duration::from_secs(20);
/// A tool the platform invokes inside the runtime container, and what breaks
/// without it. The consequence text is the point: a bare list of missing
/// binaries does not tell an operator what is now quietly not happening.
struct Dependency {
argv: &'static [&'static str],
needed_for: &'static str,
}
const DEPENDENCIES: &[Dependency] = &[
Dependency {
argv: &["cargo", "--version"],
needed_for: "the on_green_tests gate for Rust repos; without it every \
phase is unverified and lands on -wip",
},
Dependency {
argv: &["git", "--version"],
needed_for: "agent-side git operations in the mission checkout",
},
Dependency {
argv: &["gitleaks", "version"],
needed_for: "secret scanning in security_scan phases and evaluator checks",
},
Dependency {
argv: &["trivy", "--version"],
needed_for: "vulnerability scanning in security_scan phases",
},
Dependency {
argv: &["semgrep", "--version"],
needed_for: "static analysis in security_scan phases",
},
Dependency {
argv: &["cargo-audit", "--version"],
needed_for: "dependency advisories in security_scan phases",
},
];
/// One tool's availability, as reported by the container itself.
pub struct ToolStatus {
pub program: String,
pub present: bool,
/// Version string when present, error when not.
pub detail: String,
pub needed_for: &'static str,
}
/// Probe the runtime container for everything we invoke inside it.
///
/// Returns an empty vec if Docker itself is unreachable — that is a different
/// and louder failure which the caller reports separately, and emitting six
/// "missing" lines for it would be misleading.
pub async fn probe(container: &str) -> Result<Vec<ToolStatus>, String> {
let docker = container_exec::connect().map_err(|e| format!("docker unreachable: {e}"))?;
let mut out = Vec::with_capacity(DEPENDENCIES.len());
for dep in DEPENDENCIES {
let argv: Vec<String> = dep.argv.iter().map(|s| s.to_string()).collect();
let status =
match container_exec::exec(&docker, container, None, &argv, PROBE_TIMEOUT).await {
Ok(r) if r.success() => ToolStatus {
program: dep.argv[0].to_string(),
present: true,
detail: r
.combined()
.lines()
.next()
.unwrap_or("")
.trim()
.chars()
.take(80)
.collect(),
needed_for: dep.needed_for,
},
Ok(r) => ToolStatus {
program: dep.argv[0].to_string(),
present: false,
detail: r.combined().trim().chars().take(160).collect(),
needed_for: dep.needed_for,
},
Err(e) => ToolStatus {
program: dep.argv[0].to_string(),
present: false,
detail: e.chars().take(160).collect(),
needed_for: dep.needed_for,
},
};
out.push(status);
}
Ok(out)
}
/// Probe at startup and write the result to stderr.
///
/// Spawned rather than awaited so a slow or absent Docker socket cannot delay
/// the server coming up — the report is diagnostic, and the platform has to
/// keep working without it.
pub fn report_at_boot() {
tokio::spawn(async {
let container = std::env::var("CLAWMATES_RUNTIME_CONTAINER")
.unwrap_or_else(|_| "clawmates-runtime".to_string());
match probe(&container).await {
Err(e) => eprintln!(
"runtime_preflight: could not probe `{container}` ({e}) — mission \
test gating and security scans may silently do nothing"
),
Ok(tools) => {
let missing: Vec<&ToolStatus> = tools.iter().filter(|t| !t.present).collect();
if missing.is_empty() {
let names: Vec<&str> = tools.iter().map(|t| t.program.as_str()).collect();
eprintln!(
"runtime_preflight: `{container}` has all {} expected tools ({})",
tools.len(),
names.join(", ")
);
return;
}
eprintln!(
"runtime_preflight: `{container}` is MISSING {} of {} tools the \
platform invokes. The image on this host is behind \
deploy/clawmates-runtime/Dockerfile — rebuild and redeploy it.",
missing.len(),
tools.len()
);
for t in missing {
eprintln!(
"runtime_preflight: {} — absent. Disables: {}. ({})",
t.program, t.needed_for, t.detail
);
}
}
}
});
}
#[cfg(test)]
mod tests {
use super::*;
/// Every dependency must be probed with a flag that exits zero and prints
/// a version. A typo here produces a permanent false "missing" that would
/// train an operator to ignore the report — worse than no report at all.
#[test]
fn every_dependency_probe_is_a_version_query() {
for dep in DEPENDENCIES {
assert!(
dep.argv.len() >= 2,
"{} needs an argument that exits 0",
dep.argv[0]
);
let flag = dep.argv[1];
assert!(
flag == "--version" || flag == "version",
"{} probes with `{flag}`, which may not exit 0",
dep.argv[0]
);
assert!(
!dep.needed_for.is_empty(),
"{} must say what breaks without it",
dep.argv[0]
);
}
}
}
+48
View File
@@ -828,3 +828,51 @@ async fn commit_subjects_read_correctly_on_first_pass_and_rerun() {
); );
} }
} }
/// "No test suite" and "could not run the test suite" must not look alike.
///
/// verify_tests returned Option<bool>, so both produced `None`. That is how a
/// runtime image shipped without `cargo` stayed invisible: every on_green_tests
/// phase landed on -wip, which reads exactly like a repository that has no
/// tests — the conclusion I drew at the time and reported.
///
/// Both still gate identically, and that part is deliberate: unproven is not a
/// pass, whatever the reason. What changes is that the artifact now says which
/// of the two happened, so an infrastructure fault is legible as one.
#[tokio::test]
async fn an_unrunnable_suite_is_distinguishable_from_no_suite() {
use cm_api::mission_delivery::{verify_tests, TestOutcome};
let tmp = tempfile::tempdir().unwrap();
let mission = Uuid::now_v7();
let repo = seed_repo(tmp.path(), mission);
// No Cargo.toml / package.json / pytest markers: nothing to run.
let none = verify_tests(&repo, "clawmates-runtime-does-not-exist").await;
assert_eq!(none, TestOutcome::NoSuite);
assert_eq!(none.status(), "no_suite");
assert_eq!(none.verified(), None, "no suite must not clear the gate");
// A suite exists, but the container named here does not, so it cannot run.
std::fs::write(
repo.join("Cargo.toml"),
"[package]\nname = \"p\"\nversion = \"0.1.0\"\nedition = \"2021\"\n",
)
.unwrap();
let unrunnable = verify_tests(&repo, "clawmates-runtime-does-not-exist").await;
assert_eq!(unrunnable.status(), "could_not_run");
assert_eq!(
unrunnable.verified(),
None,
"an unrunnable suite must not clear the gate either"
);
assert!(
unrunnable.detail().is_some_and(|d| !d.is_empty()),
"an infrastructure fault must carry its reason into the artifact"
);
assert_ne!(
unrunnable.status(),
none.status(),
"the two must be distinguishable — this is the whole point"
);
}