feat(merge): gate publication on the merged tree's own tests
The other half of the merge button. Merging told you the branch went in; nothing
checked that what came out still worked.
Verified BEFORE publishing, not reverted after. `merge_locally` and
`push_merged` are separate functions so the caller can run the project's tests
between them, which means a merge that breaks the base is simply never pushed —
`main` is not broken for however long it takes someone to notice. A test asserts
`merge_locally` contains no push, because the moment it does, verification
becomes after-the-fact and the guarantee is gone.
Outcomes, all reported to the operator rather than swallowed:
Passed -> published
NoSuite -> published, and SAID so; a repo with no tests is a fact about the
repo, not a pass
Failed -> not published, exit code reported, branch untouched so it can be
fixed and merged again
CouldNotRun -> not published. Fail closed: a suite that could not run has not
passed, and publishing on "we could not check" is how a green
main stops meaning anything.
`verify_tests` runs `cargo test` as ROOT in a container, so the merge workdir
ends up holding a root-owned `target/` the server (uid 65532) cannot delete —
the same leak found three times today. Purged through the container before the
ordinary cleanup.
248 lib tests.
This commit is contained in:
@@ -249,13 +249,75 @@ pub async fn merge_on_operator_approval(
|
|||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
merge_and_push(workdir, push_url, branch, base, "merge mission branch").await
|
merge_locally(workdir, branch, base, "merge mission branch").await
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Merge onto the fetched base WITHOUT publishing it.
|
||||||
|
///
|
||||||
|
/// Split from the push so a caller can run the project's tests against the
|
||||||
|
/// merged tree first. Verifying BEFORE publishing rather than reverting after is
|
||||||
|
/// the difference between "main was never broken" and "main was broken for as
|
||||||
|
/// long as it took us to notice".
|
||||||
|
pub async fn merge_locally(
|
||||||
|
repo: &Path,
|
||||||
|
branch: &str,
|
||||||
|
base: &str,
|
||||||
|
label: &str,
|
||||||
|
) -> Result<MergeOutcome, String> {
|
||||||
|
git(repo, &["checkout", "-B", base, "FETCH_HEAD"]).await?;
|
||||||
|
if let Err(e) = git(
|
||||||
|
repo,
|
||||||
|
&["merge", "--no-ff", "-m", &format!("{label} {branch}"), branch],
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
// Leave the repo clean so the next attempt is not fighting a wedged merge.
|
||||||
|
let _ = git(repo, &["merge", "--abort"]).await;
|
||||||
|
return Ok(MergeOutcome::refused(format!(
|
||||||
|
"merge conflicted ({e}); left for a human"
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
Ok(MergeOutcome {
|
||||||
|
merged: true,
|
||||||
|
reason: format!("merged into {base} locally, not yet published"),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Publish an already-merged base.
|
||||||
|
pub async fn push_merged(repo: &Path, push_url: &str, base: &str) -> Result<(), String> {
|
||||||
|
git(repo, &["push", push_url, &format!("HEAD:refs/heads/{base}")])
|
||||||
|
.await
|
||||||
|
.map(|_| ())
|
||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|
||||||
|
/// Publication must be gated on the merged tree, and refusal must not push.
|
||||||
|
///
|
||||||
|
/// The two halves are separate functions precisely so a caller can run tests
|
||||||
|
/// BETWEEN them. If `merge_locally` ever pushed, verification would be
|
||||||
|
/// after-the-fact and `main` would be broken for as long as it took to
|
||||||
|
/// notice — which is the failure mode this whole thing exists to avoid.
|
||||||
|
#[test]
|
||||||
|
fn merging_locally_never_publishes() {
|
||||||
|
let src = include_str!("auto_merge.rs");
|
||||||
|
let body = src
|
||||||
|
.split("pub async fn merge_locally")
|
||||||
|
.nth(1)
|
||||||
|
.and_then(|s| s.split("\n}").next())
|
||||||
|
.unwrap_or("");
|
||||||
|
assert!(!body.is_empty(), "merge_locally not found");
|
||||||
|
assert!(
|
||||||
|
!body.contains("\"push\""),
|
||||||
|
"merge_locally must not push — publication is the caller's decision \
|
||||||
|
after it has verified the result"
|
||||||
|
);
|
||||||
|
// And the push half must exist separately, or the caller cannot publish.
|
||||||
|
assert!(src.contains("pub async fn push_merged"), "push_merged missing");
|
||||||
|
}
|
||||||
|
|
||||||
/// An operator merge and an automatic one must run the SAME git commands.
|
/// An operator merge and an automatic one must run the SAME git commands.
|
||||||
///
|
///
|
||||||
/// The gates differ — that is the whole point — but if the mechanics
|
/// The gates differ — that is the whole point — but if the mechanics
|
||||||
|
|||||||
@@ -469,8 +469,61 @@ pub async fn merge_branch(
|
|||||||
})));
|
})));
|
||||||
}
|
}
|
||||||
|
|
||||||
let outcome =
|
let container = std::env::var("CLAWMATES_RUNTIME_CONTAINER")
|
||||||
crate::auto_merge::merge_on_operator_approval(&workdir, &auth.url, &branch, base).await;
|
.unwrap_or_else(|_| "clawmates-runtime".to_string());
|
||||||
|
let outcome = async {
|
||||||
|
let merged =
|
||||||
|
crate::auto_merge::merge_on_operator_approval(&workdir, &auth.url, &branch, base)
|
||||||
|
.await?;
|
||||||
|
if !merged.merged {
|
||||||
|
return Ok(merged);
|
||||||
|
}
|
||||||
|
|
||||||
|
// Run the project's own tests against the MERGED tree, before it is
|
||||||
|
// published. Verifying first rather than reverting after is the
|
||||||
|
// difference between "main was never broken" and "main was broken until
|
||||||
|
// someone noticed".
|
||||||
|
//
|
||||||
|
// The merge is already committed locally at this point; refusing here
|
||||||
|
// simply never pushes it, and the branch is still there to retry.
|
||||||
|
match crate::mission_delivery::verify_tests(&workdir, &container).await {
|
||||||
|
crate::mission_delivery::TestOutcome::Passed => {}
|
||||||
|
crate::mission_delivery::TestOutcome::NoSuite => {
|
||||||
|
eprintln!(
|
||||||
|
"missions::merge_branch: {branch} has no discoverable test suite — publishing unverified"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
crate::mission_delivery::TestOutcome::Failed(code) => {
|
||||||
|
return Ok(crate::auto_merge::MergeOutcome {
|
||||||
|
merged: false,
|
||||||
|
reason: format!(
|
||||||
|
"the merged tree FAILS the project's tests (exit {code}) — not published. The branch is unchanged; fix it and merge again."
|
||||||
|
),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
// Fail closed. A suite that could not run has not passed, and
|
||||||
|
// publishing on "we could not check" is how a green main stops
|
||||||
|
// meaning anything.
|
||||||
|
crate::mission_delivery::TestOutcome::CouldNotRun(why) => {
|
||||||
|
return Ok(crate::auto_merge::MergeOutcome {
|
||||||
|
merged: false,
|
||||||
|
reason: format!("could not run the tests on the merged tree ({why}) — not published"),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
crate::auto_merge::push_merged(&workdir, &auth.url, base).await?;
|
||||||
|
Ok::<_, String>(crate::auto_merge::MergeOutcome {
|
||||||
|
merged: true,
|
||||||
|
reason: format!("tests pass on the merged tree; published to {base}"),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
.await;
|
||||||
|
|
||||||
|
// Purge through the container: `verify_tests` runs `cargo test` as ROOT, so
|
||||||
|
// the workdir now holds a root-owned `target/` the server (uid 65532) cannot
|
||||||
|
// delete. Same defect as the bench and judge copies.
|
||||||
|
crate::root_copy::purge(&container, &workdir).await;
|
||||||
let _ = tokio::fs::remove_dir_all(&workdir).await;
|
let _ = tokio::fs::remove_dir_all(&workdir).await;
|
||||||
|
|
||||||
match outcome {
|
match outcome {
|
||||||
|
|||||||
Reference in New Issue
Block a user