fix(missions): every phase of a mission shared one branch
`branch_name` took `[..8]` of both the mission and the phase id. Both are
UUIDv7, which leads with a 48-bit timestamp, so ids minted in the same
millisecond — which is exactly what happens when a mission inserts its phases
in one transaction — share their leading hex. Production produced:
clawmates/mission-019fc40e-019fc40e
for both the research and the coding phase. Each phase's commit moved the ref
the previous one had just set, so a two-phase mission ended with one branch
and the earlier phase's work reachable only by sha.
The segments now come from opposite ends: the mission keeps its time-ordered
prefix so branches group and sort usefully, and the phase contributes its
random tail so siblings cannot collide.
The existing test missed this because it compared iteration 0 against
iteration 1 of the *same* phase, where the `-i2` suffix guaranteed a
difference. The new test asserts the precondition explicitly — two v7 ids
minted together do share leading hex — and then that their branches differ
anyway.
Also adds the `commit_policy` gate, which three workflow recipes have declared
since they were written with nothing reading it. Two properties it must have:
a failed gate redirects work to `<branch>-wip` rather than discarding it, and
an unrunnable or undiscoverable test suite counts as unproven, never as green.
`discover_test_command` returns None for a `package.json` with no test script,
because `npm test` exits non-zero for a missing script and would read as a red
suite rather than an absent one. Not yet wired to publishing.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
ca1fd46e08
commit
3ea288dbb5
@@ -433,14 +433,21 @@ pub async fn commit_phase_work(
|
||||
|
||||
/// The branch a phase's work lands on.
|
||||
///
|
||||
/// Short ids keep it readable; the pair is unique per phase, and the iteration
|
||||
/// suffix keeps a re-run from colliding with the pass before it. Deliberately
|
||||
/// namespaced under `clawmates/` so it is obvious in a branch list who created
|
||||
/// 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[..8]);
|
||||
let base = format!("clawmates/mission-{}-{}", &m[..8], &p[p.len() - 8..]);
|
||||
if iteration > 0 {
|
||||
format!("{base}-i{}", iteration + 1)
|
||||
} else {
|
||||
@@ -448,6 +455,80 @@ pub fn branch_name(mission_id: Uuid, phase_id: Uuid, iteration: i32) -> String {
|
||||
}
|
||||
}
|
||||
|
||||
/// 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
|
||||
}
|
||||
|
||||
/// 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
|
||||
@@ -503,6 +584,85 @@ pub async fn record_uncapturable(
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
// ── 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!(
|
||||
|
||||
Reference in New Issue
Block a user