From 008aad0bc52363a082b3b0342643ac8388cd89e6 Mon Sep 17 00:00:00 2001 From: damocles Date: Mon, 25 May 2026 23:26:58 +0200 Subject: [PATCH] c0re: also reject apply_commit when flake.lock is stale (#317) --- hive-c0re/src/flake_check.rs | 140 ++++++++++++++++++++++++++++++++ hive-c0re/src/manager_server.rs | 26 ++++-- 2 files changed, 161 insertions(+), 5 deletions(-) diff --git a/hive-c0re/src/flake_check.rs b/hive-c0re/src/flake_check.rs index 3364e32a..658b24ee 100644 --- a/hive-c0re/src/flake_check.rs +++ b/hive-c0re/src/flake_check.rs @@ -20,6 +20,7 @@ use std::path::Path; use anyhow::{Context, Result}; use serde_json::Value; +use tokio::process::Command; use crate::lifecycle::git_command; @@ -139,6 +140,145 @@ pub fn duplicate_groups(raw: &str) -> Result> { Ok(dups) } +/// Re-derive the agent's `flake.lock` from its `flake.nix` (in a +/// throw-away worktree at the proposal tag) and reject the apply when +/// the result differs from what's committed — that means the manager +/// edited `flake.nix` but didn't commit the regenerated lock, so the +/// shipped state lies about what nix will actually fetch. +/// +/// Plain `nix flake lock` (no `--update-input` flags) only fills in +/// MISSING entries; it never refreshes existing ones. So a lock that +/// matches its `flake.nix` round-trips to a no-op, and any diff is a +/// real "stale lock" signal. +/// +/// Materialises the proposal tag into a temp worktree under +/// `std::env::temp_dir()` to avoid touching `applied//main` while +/// the check runs. Cleanup is unconditional via the inner-fn pattern. +/// +/// Returns `Ok(())` when in sync (or there's no `flake.nix` at all); +/// `Err` with a human-readable message on stale lock or nix tooling +/// failure. +pub async fn check_lock_in_sync(repo: &Path, tag: &str, approval_id: i64) -> Result<()> { + let suffix = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_nanos()) + .unwrap_or(0); + let tmp_dir = std::env::temp_dir().join(format!("hive-flake-check-{approval_id}-{suffix}")); + + // Detached worktree at the proposal tag — gives us a clean, mutable + // copy of the agent's tree without disturbing whatever's currently + // checked out on `applied//main`. + let out = git_command() + .current_dir(repo) + .args([ + "worktree", + "add", + "--detach", + &tmp_dir.to_string_lossy(), + tag, + ]) + .output() + .await + .with_context(|| format!("git worktree add {} {tag}", tmp_dir.display()))?; + if !out.status.success() { + anyhow::bail!( + "git worktree add failed: {}", + String::from_utf8_lossy(&out.stderr).trim() + ); + } + + let result = lock_in_sync_inner(&tmp_dir).await; + + // Best-effort cleanup. `git worktree remove --force` handles the + // common case; `remove_dir_all` mops up if git decided the worktree + // is half-gone (or if the inner work bailed before nix touched the + // tree). Failures here are logged, not propagated — the check's + // result is what matters. + if let Err(e) = remove_worktree(repo, &tmp_dir).await { + tracing::warn!( + worktree = %tmp_dir.display(), + error = %format!("{e:#}"), + "flake_check: temp worktree cleanup failed" + ); + } + + result +} + +async fn lock_in_sync_inner(worktree: &Path) -> Result<()> { + // No `flake.nix` means there's nothing for nix to lock — skip the + // check cleanly (the dedup check will likewise no-op). + if !worktree.join("flake.nix").exists() { + return Ok(()); + } + + let committed = tokio::fs::read_to_string(worktree.join("flake.lock")) + .await + .ok(); + + // `--extra-experimental-features` mirrors `meta::nix` for hosts + // that haven't already enabled flakes in `nix.conf`. Plain + // `nix flake lock` (no `--update-input`) fills missing entries but + // never refreshes existing ones — exactly the semantics we want. + let out = Command::new("nix") + .current_dir(worktree) + .args([ + "--extra-experimental-features", + "nix-command flakes", + "flake", + "lock", + ]) + .output() + .await + .with_context(|| format!("nix flake lock in {}", worktree.display()))?; + if !out.status.success() { + anyhow::bail!( + "nix flake lock failed: {}", + String::from_utf8_lossy(&out.stderr).trim() + ); + } + + let regenerated = tokio::fs::read_to_string(worktree.join("flake.lock")) + .await + .ok(); + + if committed.as_deref() != regenerated.as_deref() { + anyhow::bail!( + "flake.lock is out of sync with flake.nix — `nix flake lock` produces a different lock. \ + Run `nix flake lock` in your agent config, commit the result, and re-submit request_apply_commit." + ); + } + Ok(()) +} + +async fn remove_worktree(repo: &Path, worktree: &Path) -> Result<()> { + let out = git_command() + .current_dir(repo) + .args([ + "worktree", + "remove", + "--force", + &worktree.to_string_lossy(), + ]) + .output() + .await + .with_context(|| format!("git worktree remove {}", worktree.display()))?; + if !out.status.success() { + // `git worktree remove` already errored — still try the raw + // rmdir so we don't leak the dir on disk. Surface the original + // git stderr for context. + let _ = tokio::fs::remove_dir_all(worktree).await; + anyhow::bail!( + "git worktree remove failed: {}", + String::from_utf8_lossy(&out.stderr).trim() + ); + } + // git removed the worktree's metadata but the dir itself may + // linger on stripped-down git versions — best-effort clean. + let _ = tokio::fs::remove_dir_all(worktree).await; + Ok(()) +} + /// Run the dedup check against the agent's freshly-applied tree. /// /// `Ok(())` means either the commit doesn't carry a `flake.lock` (no diff --git a/hive-c0re/src/manager_server.rs b/hive-c0re/src/manager_server.rs index a7edd1d5..ee682ca6 100644 --- a/hive-c0re/src/manager_server.rs +++ b/hive-c0re/src/manager_server.rs @@ -633,12 +633,28 @@ async fn submit_apply_commit( .approvals .set_fetched_sha(id, &sha) .map_err(|e| anyhow::anyhow!("persist fetched_sha: {e:#}"))?; - // #317 dedup gate: parse the just-fetched flake.lock and reject if - // the agent declared two inputs that resolve to the same upstream - // (a missing `follows` directive). Runs after `set_fetched_sha` so - // the failed approval row carries the sha that broke — handy when - // the manager needs to inspect the bad commit. + // #317 pre-flight gates: both reject the apply before approval if + // the agent's flake state would inflate meta's lock with duplicates + // or lie about what nix will fetch. Order matters — sync first so + // the dedup pass acts on the lock nix would actually produce. + // + // Runs after `set_fetched_sha` so the failed row carries the sha + // that broke. Both failure paths mark + emit, then bail. let sha_short = sha[..sha.len().min(12)].to_owned(); + if let Err(e) = crate::flake_check::check_lock_in_sync(&applied_dir, &tag, id).await { + let note = format!("{e:#}"); + let _ = coord.approvals.mark_failed(id, ¬e); + coord.emit_approval_resolved( + id, + agent, + "apply_commit", + Some(sha_short.clone()), + "failed", + Some(note), + description.map(str::to_owned), + ); + return Err(anyhow::anyhow!("flake lock-sync check: {e:#}")); + } if let Err(e) = crate::flake_check::check_no_duplicate_inputs(&applied_dir, &tag).await { let note = format!("{e:#}"); let _ = coord.approvals.mark_failed(id, ¬e);