c0re: also reject apply_commit when flake.lock is stale (#317)
This commit is contained in:
parent
45833d0793
commit
008aad0bc5
2 changed files with 161 additions and 5 deletions
|
|
@ -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<Vec<DuplicateGroup>> {
|
|||
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/<n>/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/<n>/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
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
Loading…
Reference in a new issue