diff --git a/hive-c0re/src/actions.rs b/hive-c0re/src/actions.rs index 7b442178..a84fdde6 100644 --- a/hive-c0re/src/actions.rs +++ b/hive-c0re/src/actions.rs @@ -198,7 +198,14 @@ fn deploy_ctx(coord: &Coordinator, approval_id: i64) -> Result { /// (the operator must re-review the new head); /// 2. fetch the reviewed head into the applied repo so later git ops resolve /// it locally; -/// 3. eval-verify the reviewed commit against the meta flake. +/// 3. ancestry gate — the reviewed head must descend from `applied/main`; +/// 4. eval-verify the reviewed commit against the meta flake. +/// +/// Steps 1 and 3 ask different questions and both are load-bearing. The drift +/// gate asks whether the *head* is still what was reviewed; the ancestry gate +/// asks whether the *base* is still underneath it. A PR opened from a stale base +/// passes the drift gate untouched and then rewinds `main` when it lands, +/// silently dropping every commit made in between. /// /// Nothing here needs undoing on failure: the fetch only adds objects, and /// `main` doesn't move. That's the whole reason this is its own node — a @@ -208,9 +215,9 @@ fn deploy_ctx(coord: &Coordinator, approval_id: i64) -> Result { /// /// Returns an error if the approval can't be loaded, if the live PR head has /// drifted from the reviewed sha, if fetching that head into the applied repo -/// fails, or if the eval-verify of the reviewed commit fails. Every one of -/// these leaves the forge and `main` untouched, so the node is safely -/// retryable. +/// fails, if the reviewed head does not descend from `applied/main`, or if the +/// eval-verify of the reviewed commit fails. Every one of these leaves the forge +/// and `main` untouched, so the node is safely retryable. pub async fn run_deploy_merge_verify(coord: &Arc, approval_id: i64) -> Result<()> { let ctx = deploy_ctx(coord, approval_id)?; let pr = ctx.pr; @@ -231,7 +238,23 @@ pub async fn run_deploy_merge_verify(coord: &Arc, approval_id: i64) .await .map_err(|e| anyhow::anyhow!("fetch PR #{pr} head into applied: {e}"))?; - // 3. Eval-verify BEFORE the irreversible merge (bad nix fails fast here). + // 3. Ancestry gate: `main` must be reachable from the reviewed head, or the + // "fast-forward" in prepare_applied_target is really a rewind that drops + // every commit between the PR's base and where `main` actually is now. + let current_main = lifecycle::git_rev_parse(&ctx.applied_dir, "refs/heads/main") + .await + .map_err(|e| anyhow::anyhow!("read applied/main: {e:#}"))?; + if !lifecycle::git_is_ancestor(&ctx.applied_dir, ¤t_main, reviewed) + .await + .map_err(|e| anyhow::anyhow!("ancestry check {current_main}..{reviewed}: {e:#}"))? + { + bail!( + "PR #{pr} does not descend from applied/main (main {current_main}, reviewed {reviewed}); \ + merging it would discard commits — rebase the PR onto main and re-review" + ); + } + + // 4. Eval-verify BEFORE the irreversible merge (bad nix fails fast here). crate::meta::verify_commit(ctx.approval.agent.as_str(), &ctx.applied_dir, reviewed) .await .map_err(|e| anyhow::anyhow!("verify merge head {reviewed}: {e:#}"))?; @@ -282,7 +305,7 @@ pub async fn run_deploy_apply(coord: &Arc, approval_id: i64) -> Res Err(e) => bail!("ff-merge PR #{pr}: {e}"), } - prepare_applied_target(agent, &ctx.applied_dir, &ctx.reviewed).await + prepare_applied_target(agent, &ctx.applied_dir, &ctx.reviewed, &prev_main).await } /// `DeployTail` node body — compensation + bookkeeping, `AfterAny` the apply @@ -771,13 +794,23 @@ async fn prepare_applied_target( agent: &str, applied_dir: &std::path::Path, target: &str, + expected_main: &str, ) -> Result<()> { // Fast-forward applied/main to target + sync the working tree. Meta input // pins `?ref=main`, so this is what makes nix re-lock to the target commit // on the prepare_deploy step below. - lifecycle::git_update_ref(applied_dir, "refs/heads/main", target) + // + // Compare-and-swap, not a bare set: `main` must still be the sha the caller + // read before the merge. A plain `update-ref` here moves the branch to + // `target` whatever it currently points at, which turns "fast-forward" into + // "discard anything that landed in the meantime" — the ancestry gate in + // run_deploy_merge_verify only proves the target is safe against the `main` + // observed *then*, so this is what makes that proof still true *now*. + lifecycle::git_update_ref_cas(applied_dir, "refs/heads/main", target, expected_main) .await - .map_err(|e| anyhow::anyhow!("ff main to {target}: {e:#}"))?; + .map_err(|e| { + anyhow::anyhow!("ff main {expected_main} -> {target} (concurrent move?): {e:#}") + })?; lifecycle::git_read_tree_reset(applied_dir, "refs/heads/main") .await .map_err(|e| anyhow::anyhow!("read-tree to main: {e:#}"))?; diff --git a/hive-c0re/src/lifecycle/git.rs b/hive-c0re/src/lifecycle/git.rs index 3461c956..9a7b190d 100644 --- a/hive-c0re/src/lifecycle/git.rs +++ b/hive-c0re/src/lifecycle/git.rs @@ -168,6 +168,59 @@ pub async fn git_update_ref(dir: &Path, refname: &str, target: &str) -> Result<( git(dir, &["update-ref", refname, target]).await } +/// Compare-and-swap a ref: move `refname` to `new` only if it currently points +/// at `old`. `git update-ref ` refuses — leaving the ref +/// untouched — when the current value differs. That refusal is the whole +/// difference between "advance this branch" and "overwrite whatever is there". +/// +/// Prefer this over [`git_update_ref`] whenever the caller already knows the +/// value it believes the ref holds: a plain `update-ref` that raced another +/// writer discards the other writer's commits without a word. +/// +/// # Errors +/// +/// Returns an error if the ref does not currently point at `old` (the CAS lost) +/// or if the `git` invocation itself fails. +pub async fn git_update_ref_cas(dir: &Path, refname: &str, new: &str, old: &str) -> Result<()> { + git(dir, &["update-ref", refname, new, old]).await +} + +/// True when `ancestor` is reachable from `descendant` — i.e. moving a branch +/// from `ancestor` to `descendant` is a genuine fast-forward that discards +/// nothing. +/// +/// Not the same question as "did this branch land upstream": a squash-merge +/// rewrites the commit, so `--is-ancestor` correctly answers `false` for a +/// branch whose *contents* were merged. This helper compares two commits with +/// real shared ancestry inside one repo, which is precisely what it decides. +/// +/// # Errors +/// +/// Returns an error if `git` fails to run or either revision can't be resolved. +/// A clean "no, not an ancestor" is `Ok(false)`, not an error. +pub async fn git_is_ancestor(dir: &Path, ancestor: &str, descendant: &str) -> Result { + let out = git_command() + .current_dir(dir) + .args(["merge-base", "--is-ancestor", ancestor, descendant]) + .output() + .await + .with_context(|| { + format!( + "git merge-base --is-ancestor {ancestor} {descendant} in {}", + dir.display() + ) + })?; + match out.status.code() { + Some(0) => Ok(true), + Some(1) => Ok(false), + _ => bail!( + "git merge-base --is-ancestor {ancestor} {descendant} failed ({}): {}", + out.status, + String::from_utf8_lossy(&out.stderr).trim() + ), + } +} + /// Delete a ref. The counterpart to [`git_update_ref`] for the bookkeeping /// refs a deploy parks in the applied repo (`refs/hyperhive/rollback/`, /// which records the pre-merge `main` so the deploy tail can compensate a diff --git a/hive-c0re/src/lifecycle/mod.rs b/hive-c0re/src/lifecycle/mod.rs index c06b0985..52d5aa41 100644 --- a/hive-c0re/src/lifecycle/mod.rs +++ b/hive-c0re/src/lifecycle/mod.rs @@ -7,8 +7,8 @@ mod setup; mod tests; pub use git::{ - git, git_command, git_delete_ref, git_read_tree_reset, git_rev_parse, git_tag, - git_tag_annotated, git_update_ref, + git, git_command, git_delete_ref, git_is_ancestor, git_read_tree_reset, git_rev_parse, git_tag, + git_tag_annotated, git_update_ref, git_update_ref_cas, }; pub use host_config::write_dropins; pub use setup::{ diff --git a/hive-c0re/src/lifecycle/tests.rs b/hive-c0re/src/lifecycle/tests.rs index 761ae999..0e056994 100644 --- a/hive-c0re/src/lifecycle/tests.rs +++ b/hive-c0re/src/lifecycle/tests.rs @@ -91,3 +91,92 @@ async fn setup_proposed_idempotent() { "expected exactly one commit after idempotent call" ); } + +/// Build a two-commit repo and return `(dir, repo_path, first, second)`. +async fn two_commit_repo() -> (tempfile::TempDir, std::path::PathBuf, String, String) { + let dir = tempfile::tempdir().expect("tempdir"); + let repo = dir.path().join("proposed"); + setup_proposed(&repo, "test-agent") + .await + .expect("setup_proposed"); + let first = git_rev_parse(&repo, "HEAD").await.expect("rev-parse first"); + + std::fs::write(repo.join("second.txt"), "second").expect("write second.txt"); + git(&repo, &["add", "second.txt"]).await.expect("git add"); + git( + &repo, + &[ + "-c", + "user.name=test", + "-c", + "user.email=test@example.com", + "commit", + "-m", + "second", + ], + ) + .await + .expect("git commit"); + let second = git_rev_parse(&repo, "HEAD") + .await + .expect("rev-parse second"); + + assert_ne!(first, second, "second commit did not advance HEAD"); + (dir, repo, first, second) +} + +/// `git_is_ancestor` answers reachability in both directions. This is the +/// check that decides whether moving a branch is a real fast-forward or a +/// rewind that discards commits. +#[tokio::test] +async fn is_ancestor_distinguishes_direction() { + let (_dir, repo, first, second) = two_commit_repo().await; + + assert!( + git_is_ancestor(&repo, &first, &second) + .await + .expect("ancestor check forward"), + "first must be an ancestor of second" + ); + assert!( + !git_is_ancestor(&repo, &second, &first) + .await + .expect("ancestor check reverse"), + "second must NOT be an ancestor of first" + ); +} + +/// Regression test for the deploy path that ate a committed agent config: +/// `git_update_ref_cas` must refuse to move a ref whose current value is not +/// the expected one, and must leave the ref untouched when it refuses. A bare +/// `update-ref` accepts that move and silently drops everything in between. +#[tokio::test] +async fn update_ref_cas_refuses_stale_expectation() { + let (_dir, repo, first, second) = two_commit_repo().await; + + // Park a ref at `first`, then CAS it forward with the correct old value. + git_update_ref(&repo, "refs/heads/cas", &first) + .await + .expect("plant ref"); + git_update_ref_cas(&repo, "refs/heads/cas", &second, &first) + .await + .expect("CAS with the correct old value must succeed"); + assert_eq!( + git_rev_parse(&repo, "refs/heads/cas").await.unwrap(), + second + ); + + // Now retry with the SAME (now stale) expectation, as a racing deploy + // holding a pre-merge sha would: it must fail rather than rewind. + assert!( + git_update_ref_cas(&repo, "refs/heads/cas", &first, &first) + .await + .is_err(), + "CAS accepted a stale old value" + ); + assert_eq!( + git_rev_parse(&repo, "refs/heads/cas").await.unwrap(), + second, + "ref moved even though the CAS failed" + ); +}