fix(evaluator): the judge verifies a COPY, never the mission's own tree
The harness's uid probe caught this the moment cross-provider validation came back: `checkout has multiple writers (uids=0,65532)`. All 63 root-owned files were under `repo/target/`. The mechanism, confirmed rather than guessed: the judge's verification sandbox execs into `clawmates-runtime`, which runs as ROOT with the missions root bind-mounted, and its workdir was the mission's LIVE checkout. So when the judge ran `cargo test` to check a condition — which is the entire point of the verifying evaluator — cargo wrote `target/` into the checkout as uid 0, in a tree otherwise owned by the server. The next phase's `cargo` would then hit permission-denied on a directory it cannot write, which is the uid-split failure class copy mode exists to eliminate. IT WAS LATENT ALL DAY. While the z.ai credential was dead the judge never ran a single check, so the uid probe kept passing; restoring the credential surfaced it on the first gated mission. A guard that only holds while a dependency is broken is not a guard, and this one was only visible because the harness measures the invariant rather than the feature. Running the checks as the checkout's uid was the obvious fix and is the wrong one: `CARGO_HOME` is root-owned 0755 in that image, so a non-root uid fails, and the evaluator treats "could not run" as unverified — trading a polluted tree for phases that fail closed for a reason unrelated to their work. So the sandbox verifies a copy, made through `mission_fs::pack_dir` so it carries exactly what the delivered diff carries (no `target/`, no `node_modules/`) — one exclusion list, three consumers. The copy lives at `_verify/<mission>`, a sibling of the swept per-mission directories, and is removed on drop. This is the rule the codebase already applies to the `verifier` subagent, which has no Edit and no Write, stated for the judge: verification must not mutate what it verifies. A judge that can change the tree it is judging can make its own verdict true. NEGATIVE CONTROL, run: pointing the sandbox back at the live checkout fails `the_judge_verifies_a_copy_and_never_the_mission_tree`. The test seam (`Sandbox::at`) is never `owned` and never deletes, so a destructive constructor cannot masquerade as a plain one. 553 tests pass, clippy clean. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
1b556c5849
commit
e2f576ec02
@@ -195,11 +195,39 @@ pub fn clamp_output(s: &str) -> String {
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Where a verification copy lives: a sibling of the per-mission directories,
|
||||||
|
/// so the sweeper that deletes `<root>/<mission_id>` never races it and nothing
|
||||||
|
/// under it is ever collected or delivered.
|
||||||
|
fn verify_path(mission_id: Uuid) -> PathBuf {
|
||||||
|
crate::mission_workspace::missions_root()
|
||||||
|
.join("_verify")
|
||||||
|
.join(mission_id.to_string())
|
||||||
|
}
|
||||||
|
|
||||||
/// A checkout the judge may run verification commands against.
|
/// A checkout the judge may run verification commands against.
|
||||||
#[derive(Debug, Clone)]
|
///
|
||||||
|
/// A COPY of the mission's checkout, never the checkout itself. The judge runs
|
||||||
|
/// real commands — `cargo test` is the whole point — and the container it execs
|
||||||
|
/// into runs as ROOT with the missions root bind-mounted, so running them in the
|
||||||
|
/// live tree left `repo/target/` owned by uid 0 in a checkout otherwise owned by
|
||||||
|
/// the server. That breaks the single-writer invariant copy mode exists to
|
||||||
|
/// guarantee, and the next phase's `cargo` would hit permission-denied on a
|
||||||
|
/// directory it cannot write.
|
||||||
|
///
|
||||||
|
/// It stayed invisible all day because a dead validator credential meant the
|
||||||
|
/// judge never ran a single check; restoring the credential surfaced it on the
|
||||||
|
/// first gated mission, via the harness's uid probe.
|
||||||
|
///
|
||||||
|
/// The deeper rule is the one this codebase already applies to the `verifier`
|
||||||
|
/// subagent, which has no Edit and no Write: **verification must not mutate what
|
||||||
|
/// it verifies.** A judge that can change the tree it is judging can make its own
|
||||||
|
/// verdict true.
|
||||||
|
#[derive(Debug)]
|
||||||
pub struct Sandbox {
|
pub struct Sandbox {
|
||||||
container: String,
|
container: String,
|
||||||
workdir: PathBuf,
|
workdir: PathBuf,
|
||||||
|
/// Whether this sandbox created `workdir` and must remove it.
|
||||||
|
owned: bool,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl Sandbox {
|
impl Sandbox {
|
||||||
@@ -208,22 +236,57 @@ impl Sandbox {
|
|||||||
///
|
///
|
||||||
/// Returning `None` rather than an empty sandbox matters: the evaluator
|
/// Returning `None` rather than an empty sandbox matters: the evaluator
|
||||||
/// prompt changes shape depending on whether verification is possible, and
|
/// prompt changes shape depending on whether verification is possible, and
|
||||||
/// a judge must never be told it can check something it cannot.
|
/// a judge must never be told it can check something it cannot. A copy that
|
||||||
|
/// fails to materialise is also `None` for the same reason — an unverifiable
|
||||||
|
/// phase must not be told it can verify.
|
||||||
pub fn for_mission(mission_id: Uuid) -> Option<Sandbox> {
|
pub fn for_mission(mission_id: Uuid) -> Option<Sandbox> {
|
||||||
let workdir = crate::mission_workspace::checkout_path(mission_id);
|
Sandbox::for_checkout(
|
||||||
|
&crate::mission_workspace::checkout_path(mission_id),
|
||||||
|
&verify_path(mission_id),
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The testable half of [`Sandbox::for_mission`]. The paths are parameters
|
||||||
|
/// because `missions_root()` reads process environment, and this workspace
|
||||||
|
/// does not mutate that in tests — the same split as
|
||||||
|
/// `mission_runtime::provider_env_from` and
|
||||||
|
/// `mission_workspace::auth_with_token`.
|
||||||
|
pub fn for_checkout(source: &Path, root: &Path) -> Option<Sandbox> {
|
||||||
|
if !source.is_dir() {
|
||||||
|
return None;
|
||||||
|
}
|
||||||
|
// A stale copy from a previous pass would be verified instead of this
|
||||||
|
// one's work — the same "judged a tree nobody wrote" shape the evaluator
|
||||||
|
// exists to prevent.
|
||||||
|
let _ = std::fs::remove_dir_all(root);
|
||||||
|
// Through the transport packer, so the copy carries exactly what the
|
||||||
|
// delivered diff carries: no `target/`, no `node_modules/`. One
|
||||||
|
// exclusion list, three consumers.
|
||||||
|
let archive = crate::mission_fs::pack_dir(source, "repo").ok()?;
|
||||||
|
crate::mission_fs::unpack_into(&archive, root).ok()?;
|
||||||
|
let workdir = root.join("repo");
|
||||||
if !workdir.is_dir() {
|
if !workdir.is_dir() {
|
||||||
return None;
|
return None;
|
||||||
}
|
}
|
||||||
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());
|
||||||
Some(Sandbox { container, workdir })
|
Some(Sandbox {
|
||||||
|
container,
|
||||||
|
workdir,
|
||||||
|
owned: true,
|
||||||
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Construct against an explicit path. Test seam.
|
/// Construct against an explicit path. Test seam.
|
||||||
|
///
|
||||||
|
/// Never `owned`: a caller-supplied directory is the caller's, and deleting
|
||||||
|
/// it on drop would make this seam destructive in a way its users could not
|
||||||
|
/// see.
|
||||||
pub fn at(container: impl Into<String>, workdir: impl AsRef<Path>) -> Sandbox {
|
pub fn at(container: impl Into<String>, workdir: impl AsRef<Path>) -> Sandbox {
|
||||||
Sandbox {
|
Sandbox {
|
||||||
container: container.into(),
|
container: container.into(),
|
||||||
workdir: workdir.as_ref().to_path_buf(),
|
workdir: workdir.as_ref().to_path_buf(),
|
||||||
|
owned: false,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -329,6 +392,27 @@ fn git_ownership_env(workdir: &str) -> Vec<String> {
|
|||||||
]
|
]
|
||||||
}
|
}
|
||||||
|
|
||||||
|
impl Drop for Sandbox {
|
||||||
|
/// Remove the copy when the evaluation is done.
|
||||||
|
///
|
||||||
|
/// Best-effort: a leaked copy costs disk, and failing an evaluation over
|
||||||
|
/// cleanup would trade a real verdict for a housekeeping error. The copy
|
||||||
|
/// lives under `_verify/<mission>`, which the next pass clears anyway.
|
||||||
|
fn drop(&mut self) {
|
||||||
|
if !self.owned {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
if let Some(root) = self.workdir.parent() {
|
||||||
|
if let Err(e) = std::fs::remove_dir_all(root) {
|
||||||
|
eprintln!(
|
||||||
|
"evaluator_tools: could not remove the verification copy at {} ({e})",
|
||||||
|
root.display()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// One verification command and what became of it.
|
/// One verification command and what became of it.
|
||||||
///
|
///
|
||||||
/// This exists because the first version recorded *attempted* commands. The
|
/// This exists because the first version recorded *attempted* commands. The
|
||||||
@@ -427,6 +511,58 @@ mod tests {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// THE regression. The judge runs real commands in a container that runs as
|
||||||
|
/// ROOT with the missions root bind-mounted, so verifying the live checkout
|
||||||
|
/// left `repo/target/` owned by uid 0 in a tree owned by the server — the
|
||||||
|
/// single-writer invariant broken by the thing that was supposed to be
|
||||||
|
/// checking the work. Verifying a COPY makes it unrepresentable.
|
||||||
|
#[test]
|
||||||
|
fn the_judge_verifies_a_copy_and_never_the_mission_tree() {
|
||||||
|
let tmp = tempfile::tempdir().unwrap();
|
||||||
|
let root = tmp.path().join("missions-root");
|
||||||
|
let mission = Uuid::now_v7();
|
||||||
|
let checkout = root.join(mission.to_string()).join("repo");
|
||||||
|
std::fs::create_dir_all(checkout.join("src")).unwrap();
|
||||||
|
std::fs::write(checkout.join("Cargo.toml"), "[package]\nname='x'\n").unwrap();
|
||||||
|
std::fs::write(checkout.join("src/lib.rs"), "pub fn a() {}").unwrap();
|
||||||
|
// Build output the transport already excludes; the copy must not carry
|
||||||
|
// it either, or the judge measures a stale artifact.
|
||||||
|
std::fs::create_dir_all(checkout.join("target/debug")).unwrap();
|
||||||
|
std::fs::write(checkout.join("target/debug/junk"), "x").unwrap();
|
||||||
|
|
||||||
|
let sandbox = Sandbox::for_checkout(&checkout, &root.join("_verify").join(mission.to_string()))
|
||||||
|
.expect("a checkout on disk yields a sandbox");
|
||||||
|
|
||||||
|
assert_ne!(
|
||||||
|
sandbox.workdir(),
|
||||||
|
checkout,
|
||||||
|
"the judge must not be pointed at the mission's own checkout"
|
||||||
|
);
|
||||||
|
assert!(sandbox.workdir().join("src/lib.rs").is_file(), "the copy has the source");
|
||||||
|
assert!(
|
||||||
|
!sandbox.workdir().join("target").exists(),
|
||||||
|
"the copy must not carry build output: {}",
|
||||||
|
sandbox.workdir().display()
|
||||||
|
);
|
||||||
|
|
||||||
|
// And dropping it takes the copy with it, leaving the mission untouched.
|
||||||
|
let copy_root = sandbox.workdir().parent().unwrap().to_path_buf();
|
||||||
|
drop(sandbox);
|
||||||
|
assert!(!copy_root.exists(), "the copy outlived its sandbox");
|
||||||
|
assert!(checkout.join("src/lib.rs").is_file(), "the mission tree is intact");
|
||||||
|
assert!(checkout.join("target/debug/junk").is_file());
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The test seam must not delete a directory it was handed. A destructive
|
||||||
|
/// constructor that looks like a plain one is how a test wipes a real tree.
|
||||||
|
#[test]
|
||||||
|
fn an_explicit_workdir_is_never_deleted() {
|
||||||
|
let tmp = tempfile::tempdir().unwrap();
|
||||||
|
std::fs::write(tmp.path().join("keep.txt"), "x").unwrap();
|
||||||
|
drop(Sandbox::at("c", tmp.path()));
|
||||||
|
assert!(tmp.path().join("keep.txt").is_file());
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn refuses_programs_off_the_list() {
|
fn refuses_programs_off_the_list() {
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
|
|||||||
Reference in New Issue
Block a user