feat(auto-merge): merge additive branches, refuse everything else
Closes the branch pile-up: a catalogue branch that only adds notes now merges into main by itself, so the work is actually in the vault rather than waiting in a branch nobody opened. Additive-only is measured from the diff, not assumed from the mission type. Three conditions, all required: the type declares additive_only, the run verified, and `git diff --name-status base...branch` contains only A entries. A research harvest that somehow rewrote a hand-written note is refused by the same check that lets its new notes through — which is the case the test pins down, asserting README.md on main is byte-identical afterwards. Renames and deletes count as non-additive. A rename is a delete plus an add and the delete half can destroy hand-written work. Unknown merge_policy values fail closed to Never. A typo must not grant auto-merge. The diff is taken against FETCH_HEAD, freshly fetched, using `...` so an unrelated commit landing on main meanwhile is not misread as ours. A conflicted merge aborts and leaves the branch for a human rather than wedging the checkout for the next run. merge_reason is always populated and surfaced in the API: a branch that quietly did not merge is indistinguishable from one never delivered. 399 tests, clippy clean. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
Omar Sobh
co-authored by
Claude Opus 5
Claude Opus 5
parent
3124fd3c8f
commit
9de2cf34e4
@@ -0,0 +1,222 @@
|
||||
//! Merging a delivered branch into the base, when that is provably safe.
|
||||
//!
|
||||
//! Every mission type delivers to a branch and never to `main`. For most that
|
||||
//! is where it should stop — a human reads the code and merges. But some
|
||||
//! missions only ever *add* files in a folder they own: a paper catalogue, a
|
||||
//! benchmark record. Those branches carry no judgement call, and leaving them
|
||||
//! to pile up unmerged means the work is done but not actually in the vault.
|
||||
//!
|
||||
//! # Additive-only is a property, not a preference
|
||||
//!
|
||||
//! The gate is not "is this mission type trusted". It is measured from the
|
||||
//! diff: if the branch modifies or deletes anything that already existed, it
|
||||
//! does not qualify, whatever its template says. A research harvest that
|
||||
//! somehow rewrote a hand-written note would be refused by the same check
|
||||
//! that lets its new notes through.
|
||||
//!
|
||||
//! Three conditions, all required:
|
||||
//!
|
||||
//! 1. the mission type declares [`MergePolicy::AdditiveOnly`]
|
||||
//! 2. verification passed — a run that did not prove its work does not merge
|
||||
//! 3. the diff against the base contains only additions
|
||||
//!
|
||||
//! Anything else lands as a branch for a human, which is the existing
|
||||
//! behaviour and the safe default.
|
||||
|
||||
use std::path::Path;
|
||||
|
||||
/// What a mission type is allowed to do with its own branch.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum MergePolicy {
|
||||
/// Always leave the branch for a human. Correct for anything that touches
|
||||
/// code: `refactor`, `research_and_code`, security patches.
|
||||
Never,
|
||||
/// Merge automatically when the diff is provably additive and the run
|
||||
/// verified. Correct for catalogues and recorded measurements.
|
||||
AdditiveOnly,
|
||||
}
|
||||
|
||||
impl MergePolicy {
|
||||
/// Parse a template's `merge_policy`. Unknown values fall back to `Never`
|
||||
/// and say so: a typo must not silently grant auto-merge.
|
||||
pub fn parse(raw: Option<&str>) -> MergePolicy {
|
||||
match raw.map(str::trim) {
|
||||
Some("additive_only") => MergePolicy::AdditiveOnly,
|
||||
Some("never") | None => MergePolicy::Never,
|
||||
Some(other) => {
|
||||
eprintln!(
|
||||
"auto_merge: unknown merge_policy {other:?} — refusing to auto-merge"
|
||||
);
|
||||
MergePolicy::Never
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Why a branch was or was not merged. The reason is always recorded: a
|
||||
/// branch that silently did not merge is indistinguishable from one that was
|
||||
/// never delivered.
|
||||
#[derive(Debug, Clone)]
|
||||
pub struct MergeOutcome {
|
||||
pub merged: bool,
|
||||
pub reason: String,
|
||||
}
|
||||
|
||||
impl MergeOutcome {
|
||||
fn refused(reason: impl Into<String>) -> MergeOutcome {
|
||||
MergeOutcome {
|
||||
merged: false,
|
||||
reason: reason.into(),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Classify a `git diff --name-status` body.
|
||||
///
|
||||
/// Returns the offending entries, empty when every change is an addition.
|
||||
/// Split out so the rule is testable without a repository.
|
||||
pub fn non_additive_changes(name_status: &str) -> Vec<String> {
|
||||
name_status
|
||||
.lines()
|
||||
.filter(|l| !l.trim().is_empty())
|
||||
.filter(|l| {
|
||||
// Status is the first field: A/M/D/R###/C###.
|
||||
!matches!(l.chars().next(), Some('A'))
|
||||
})
|
||||
.map(|l| l.trim().to_string())
|
||||
.collect()
|
||||
}
|
||||
|
||||
async fn git(repo: &Path, args: &[&str]) -> Result<String, String> {
|
||||
let out = tokio::process::Command::new("git")
|
||||
.arg("-C")
|
||||
.arg(repo)
|
||||
.args(["-c", &format!("safe.directory={}", repo.display())])
|
||||
.args(args)
|
||||
.env("GIT_AUTHOR_NAME", crate::mission_delivery::commit_identity().0)
|
||||
.env("GIT_AUTHOR_EMAIL", crate::mission_delivery::commit_identity().1)
|
||||
.env(
|
||||
"GIT_COMMITTER_NAME",
|
||||
crate::mission_delivery::commit_identity().0,
|
||||
)
|
||||
.env(
|
||||
"GIT_COMMITTER_EMAIL",
|
||||
crate::mission_delivery::commit_identity().1,
|
||||
)
|
||||
.output()
|
||||
.await
|
||||
.map_err(|e| format!("spawn git: {e}"))?;
|
||||
if !out.status.success() {
|
||||
return Err(format!(
|
||||
"git {} → {}: {}",
|
||||
args.first().copied().unwrap_or("?"),
|
||||
out.status,
|
||||
crate::mission_workspace::redact_token(&String::from_utf8_lossy(&out.stderr))
|
||||
.chars()
|
||||
.take(300)
|
||||
.collect::<String>()
|
||||
));
|
||||
}
|
||||
Ok(String::from_utf8_lossy(&out.stdout).into_owned())
|
||||
}
|
||||
|
||||
/// Merge `branch` into `base` and push, if all three conditions hold.
|
||||
///
|
||||
/// Never returns `Err` for a refusal — a refusal is a normal outcome with a
|
||||
/// reason. `Err` is reserved for the merge itself going wrong after we decided
|
||||
/// to attempt it.
|
||||
pub async fn try_merge(
|
||||
repo: &Path,
|
||||
push_url: &str,
|
||||
branch: &str,
|
||||
base: &str,
|
||||
policy: MergePolicy,
|
||||
verified: bool,
|
||||
) -> Result<MergeOutcome, String> {
|
||||
if policy != MergePolicy::AdditiveOnly {
|
||||
return Ok(MergeOutcome::refused(
|
||||
"merge_policy is not additive_only; left for a human",
|
||||
));
|
||||
}
|
||||
if !verified {
|
||||
return Ok(MergeOutcome::refused(
|
||||
"run did not verify; refusing to merge unproven work",
|
||||
));
|
||||
}
|
||||
|
||||
// Compare against the base as the REMOTE has it, not a local ref that may
|
||||
// be stale. `...` gives changes on the branch since it diverged, so an
|
||||
// unrelated commit landing on main meanwhile is not misread as ours.
|
||||
git(repo, &["fetch", push_url, base]).await?;
|
||||
let diff = git(
|
||||
repo,
|
||||
&["diff", "--name-status", &format!("FETCH_HEAD...{branch}")],
|
||||
)
|
||||
.await?;
|
||||
|
||||
let offending = non_additive_changes(&diff);
|
||||
if !offending.is_empty() {
|
||||
return Ok(MergeOutcome::refused(format!(
|
||||
"diff is not additive ({} non-add change(s), first: {}); left for a human",
|
||||
offending.len(),
|
||||
offending.first().map(String::as_str).unwrap_or("?")
|
||||
)));
|
||||
}
|
||||
if diff.trim().is_empty() {
|
||||
return Ok(MergeOutcome::refused("branch adds nothing"));
|
||||
}
|
||||
|
||||
// Merge onto the freshly fetched base rather than a local branch.
|
||||
git(repo, &["checkout", "-B", base, "FETCH_HEAD"]).await?;
|
||||
if let Err(e) = git(
|
||||
repo,
|
||||
&["merge", "--no-ff", "-m", &format!("auto-merge {branch}"), branch],
|
||||
)
|
||||
.await
|
||||
{
|
||||
// Leave the repo clean so the next run is not fighting a wedged merge.
|
||||
let _ = git(repo, &["merge", "--abort"]).await;
|
||||
return Ok(MergeOutcome::refused(format!(
|
||||
"merge conflicted ({e}); left for a human"
|
||||
)));
|
||||
}
|
||||
|
||||
git(repo, &["push", push_url, &format!("HEAD:refs/heads/{base}")]).await?;
|
||||
Ok(MergeOutcome {
|
||||
merged: true,
|
||||
reason: format!("additive-only and verified; merged into {base}"),
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn only_pure_additions_qualify() {
|
||||
assert!(non_additive_changes("A\t60 Papers/a.md\nA\t60 Papers/b.md\n").is_empty());
|
||||
|
||||
// A modification disqualifies the whole branch.
|
||||
let m = non_additive_changes("A\t60 Papers/a.md\nM\tREADME.md\n");
|
||||
assert_eq!(m.len(), 1);
|
||||
assert!(m[0].contains("README.md"));
|
||||
|
||||
// So do deletes and renames — a rename is a delete plus an add, and
|
||||
// the delete half can destroy hand-written work.
|
||||
assert_eq!(non_additive_changes("D\tnotes/old.md\n").len(), 1);
|
||||
assert_eq!(non_additive_changes("R100\ta.md\tb.md\n").len(), 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_unknown_policy_never_grants_auto_merge() {
|
||||
assert_eq!(MergePolicy::parse(None), MergePolicy::Never);
|
||||
assert_eq!(MergePolicy::parse(Some("never")), MergePolicy::Never);
|
||||
assert_eq!(
|
||||
MergePolicy::parse(Some("additive_only")),
|
||||
MergePolicy::AdditiveOnly
|
||||
);
|
||||
// A typo must fail closed, not open.
|
||||
assert_eq!(MergePolicy::parse(Some("aditive_only")), MergePolicy::Never);
|
||||
assert_eq!(MergePolicy::parse(Some("always")), MergePolicy::Never);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user