diff --git a/hive-c0re/src/flake_check.rs b/hive-c0re/src/flake_check.rs new file mode 100644 index 00000000..71550270 --- /dev/null +++ b/hive-c0re/src/flake_check.rs @@ -0,0 +1,431 @@ +//! Pre-apply validation for agent `flake.lock` files (closes part of #317). +//! +//! Every `request_apply_commit` lands a `proposal/` tag in the +//! agent's applied repo before the operator sees the approval. We +//! parse `flake.lock` from that tag's tree and reject the request if +//! two or more nodes share an identical `original` field — that +//! signals a missing `inputs..inputs.nixpkgs.follows = "nixpkgs"` +//! directive in `flake.nix` and would inflate meta's lock with +//! duplicates after deploy. +//! +//! Per mara's scope note on #317 (comment 4189): the check runs on +//! the agent repo, not meta, and catches *new* violations only. +//! Existing agents whose lock already has duplicates are out of +//! scope here and get a coordinated config-change pass via the +//! manager instead. + +use std::collections::BTreeMap; +use std::fmt::Write as _; +use std::path::Path; + +use anyhow::{Context, Result}; +use serde_json::Value; +use tokio::process::Command; + +use crate::lifecycle::git_command; + +/// One group of `flake.lock` nodes that all share the same canonical +/// `original` reference. Surfaced in the rejection message so the +/// operator (and the manager that submitted the apply) can see +/// exactly which input pair needs a `follows` directive. +#[derive(Debug, Clone)] +pub struct DuplicateGroup { + /// One of the original `Value`s from the lock — used for + /// pretty-printing in the error message. Canonical equivalence + /// is enforced by the `BTreeMap` key in `duplicate_groups`, so + /// we don't need to keep the canonicalised form on the struct. + pub original: Value, + /// Names of the flake.lock nodes that share this `original`, + /// sorted for stable error output. + pub keys: Vec, +} + +/// Read `flake.lock` from `:flake.lock` in `repo`. Returns +/// `Ok(None)` when the file isn't tracked in that tag (no inputs ⇒ +/// nothing to dedup); `Err` only on real git plumbing failures. +async fn read_lock_at_tag(repo: &Path, tag: &str) -> Result> { + let spec = format!("{tag}:flake.lock"); + let out = git_command() + .current_dir(repo) + .args(["show", &spec]) + .output() + .await + .with_context(|| format!("git show {spec} in {}", repo.display()))?; + if !out.status.success() { + let stderr = String::from_utf8_lossy(&out.stderr); + // git uses two different messages for "path not in tree" + // depending on whether the path also collides with an on-disk + // file. Both translate to "no flake.lock in this commit" — + // a legitimate, dedup-clean state for an agent with empty + // `inputs = { }`. Any other git failure (permission denied, + // ref-not-found, etc.) propagates as a hard error rather than + // being silently swallowed. + if stderr.contains("does not exist") + || stderr.contains("exists on disk, but not in") + { + return Ok(None); + } + anyhow::bail!("git show {spec} failed: {}", stderr.trim()); + } + Ok(Some(String::from_utf8_lossy(&out.stdout).into_owned())) +} + +/// Recursively serialise `v` with object keys sorted, so two +/// JSON values that differ only in key insertion order produce the +/// same string. `serde_json::Value` preserves `IndexMap` order by +/// default, which is fine for parsing but breaks our group-by-key +/// idea — hence this hand-rolled canonicaliser. +fn canonical_json(v: &Value) -> String { + match v { + Value::Object(map) => { + let mut keys: Vec<&String> = map.keys().collect(); + keys.sort(); + let mut s = String::from("{"); + for (i, k) in keys.iter().enumerate() { + if i > 0 { + s.push(','); + } + s.push_str(&serde_json::to_string(k).unwrap_or_default()); + s.push(':'); + s.push_str(&canonical_json(&map[*k])); + } + s.push('}'); + s + } + Value::Array(arr) => { + let mut s = String::from("["); + for (i, x) in arr.iter().enumerate() { + if i > 0 { + s.push(','); + } + s.push_str(&canonical_json(x)); + } + s.push(']'); + s + } + _ => serde_json::to_string(v).unwrap_or_default(), + } +} + +/// Parse `raw` (a `flake.lock` JSON document) and return every group +/// of nodes whose `original` field is identical. Nodes without an +/// `original` (the synthetic `root`, or anomalous entries) are +/// skipped. Groups with only one member are filtered out — only true +/// duplicates surface. +/// +/// Pure function, no I/O — covered by the unit tests below. +pub fn duplicate_groups(raw: &str) -> Result> { + let json: Value = serde_json::from_str(raw).context("parse flake.lock")?; + let Some(nodes) = json.get("nodes").and_then(|v| v.as_object()) else { + return Ok(Vec::new()); + }; + let mut groups: BTreeMap = BTreeMap::new(); + for (name, node) in nodes { + let Some(original) = node.get("original") else { + continue; + }; + let key = canonical_json(original); + let entry = groups.entry(key).or_insert_with(|| DuplicateGroup { + original: original.clone(), + keys: Vec::new(), + }); + entry.keys.push(name.clone()); + } + let mut dups: Vec = groups + .into_values() + .filter(|g| g.keys.len() > 1) + .collect(); + for g in &mut dups { + g.keys.sort(); + } + 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(); + + // An agent that declares inputs in flake.nix but ships no + // flake.lock at all hits this branch (committed = None, + // regenerated = Some(...)). That's a deliberate reject: every + // agent with inputs MUST commit its lock, otherwise meta's + // dedup pass has nothing to introspect and the broken state + // leaks downstream. Treated identically to a stale lock. + 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 +/// inputs declared) or every node has a unique `original`. `Err` +/// carries a multi-line message listing every duplicate group with +/// the offending node names, suitable for surfacing on the failed +/// approval row. +pub async fn check_no_duplicate_inputs(repo: &Path, tag: &str) -> Result<()> { + let Some(raw) = read_lock_at_tag(repo, tag).await? else { + return Ok(()); + }; + let dups = duplicate_groups(&raw)?; + if dups.is_empty() { + return Ok(()); + } + let mut msg = String::from( + "flake.lock has duplicate flake inputs — add a `follows` directive in flake.nix to collapse them:\n", + ); + for g in &dups { + let original = serde_json::to_string(&g.original).unwrap_or_else(|_| "?".into()); + let _ = writeln!(msg, " - {original} → nodes [{}]", g.keys.join(", ")); + } + anyhow::bail!("{}", msg.trim_end()); +} + +#[cfg(test)] +mod tests { + use super::*; + + const CLEAN_LOCK: &str = r#"{ + "nodes": { + "nixpkgs": { + "locked": {"rev": "aaa"}, + "original": {"owner": "NixOS", "repo": "nixpkgs", "ref": "nixos-25.11", "type": "github"} + }, + "nixpkgs-unstable": { + "locked": {"rev": "bbb"}, + "original": {"owner": "NixOS", "repo": "nixpkgs", "ref": "nixpkgs-unstable", "type": "github"} + }, + "root": {"inputs": {"nixpkgs": "nixpkgs"}} + }, + "root": "root", + "version": 7 + }"#; + + const DUPLICATE_LOCK: &str = r#"{ + "nodes": { + "nixpkgs": { + "locked": {"rev": "aaa"}, + "original": {"owner": "NixOS", "repo": "nixpkgs", "ref": "nixos-25.11", "type": "github"} + }, + "nixpkgs_2": { + "locked": {"rev": "ccc"}, + "original": {"owner": "NixOS", "repo": "nixpkgs", "ref": "nixos-25.11", "type": "github"} + }, + "nixpkgs_3": { + "locked": {"rev": "ddd"}, + "original": {"owner": "NixOS", "repo": "nixpkgs", "ref": "nixos-25.11", "type": "github"} + }, + "treefmt-nix": { + "locked": {"rev": "eee"}, + "original": {"owner": "numtide", "repo": "treefmt-nix", "type": "github"} + }, + "treefmt-nix_2": { + "locked": {"rev": "fff"}, + "original": {"type": "github", "owner": "numtide", "repo": "treefmt-nix"} + }, + "root": {"inputs": {"nixpkgs": "nixpkgs"}} + }, + "root": "root", + "version": 7 + }"#; + + #[test] + fn clean_lock_has_no_duplicates() { + let dups = duplicate_groups(CLEAN_LOCK).expect("parse"); + assert!(dups.is_empty(), "expected no dups, got {dups:#?}"); + } + + #[test] + fn duplicate_lock_reports_groups() { + let dups = duplicate_groups(DUPLICATE_LOCK).expect("parse"); + assert_eq!(dups.len(), 2, "expected nixpkgs + treefmt-nix groups"); + + // Order is BTreeMap-stable: sorted by canonical_json key. The + // numtide treefmt-nix key sorts before the NixOS nixpkgs one + // because `numtide` < `NixOS` lexicographically (case-sensitive, + // capitals come first... wait — capital N is 0x4e, lowercase n + // is 0x6e, so capitals come first). So nixpkgs group sorts + // first. Verify by content instead of position to avoid coupling + // to that subtlety. + let nixpkgs_group = dups + .iter() + .find(|g| g.original.get("ref").is_some()) + .expect("nixpkgs group present"); + assert_eq!( + nixpkgs_group.keys, + vec!["nixpkgs", "nixpkgs_2", "nixpkgs_3"] + ); + + let treefmt_group = dups + .iter() + .find(|g| g.original.get("ref").is_none()) + .expect("treefmt-nix group present"); + assert_eq!(treefmt_group.keys, vec!["treefmt-nix", "treefmt-nix_2"]); + } + + #[test] + fn key_order_in_original_does_not_matter() { + // The treefmt-nix and treefmt-nix_2 entries above use different + // key orderings for `original` ({owner,repo,type} vs + // {type,owner,repo}); duplicate_groups should still merge them. + let dups = duplicate_groups(DUPLICATE_LOCK).expect("parse"); + let treefmt = dups + .iter() + .find(|g| g.keys.iter().any(|k| k == "treefmt-nix")) + .expect("treefmt-nix group"); + assert!(treefmt.keys.contains(&"treefmt-nix_2".to_owned())); + } + + #[test] + fn nodes_without_original_are_ignored() { + // The synthetic `root` node has no `original` and must not be + // grouped against anything. + let dups = duplicate_groups(CLEAN_LOCK).expect("parse"); + assert!(dups.iter().all(|g| !g.keys.iter().any(|k| k == "root"))); + } + + #[test] + fn missing_nodes_object_is_not_an_error() { + // A lock file that's syntactically JSON but lacks `nodes` (e.g. + // a partial test fixture) should fail open — no dups reported. + let raw = r#"{"root": "root", "version": 7}"#; + let dups = duplicate_groups(raw).expect("parse"); + assert!(dups.is_empty()); + } + + #[test] + fn invalid_json_is_a_hard_error() { + let err = duplicate_groups("not-json-at-all").unwrap_err(); + assert!(format!("{err:#}").contains("parse flake.lock")); + } +} diff --git a/hive-c0re/src/main.rs b/hive-c0re/src/main.rs index 62e33fc9..1a92787a 100644 --- a/hive-c0re/src/main.rs +++ b/hive-c0re/src/main.rs @@ -18,6 +18,7 @@ mod dashboard; mod dashboard_events; mod events_vacuum; mod stats_vacuum; +mod flake_check; mod forge; mod lifecycle; mod limits; diff --git a/hive-c0re/src/manager_server.rs b/hive-c0re/src/manager_server.rs index a89ae68d..c7258650 100644 --- a/hive-c0re/src/manager_server.rs +++ b/hive-c0re/src/manager_server.rs @@ -633,14 +633,53 @@ async fn submit_apply_commit( .approvals .set_fetched_sha(id, &sha) .map_err(|e| anyhow::anyhow!("persist fetched_sha: {e:#}"))?; + // #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. Both checks independently read + // `:flake.lock` via git — they don't share state. Order matters + // only for early-exit + messaging: sync first means a stale lock + // bails with the actionable "run `nix flake lock`" hint rather than + // a dedup pass on a lock nix would never 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); + 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 dedup check: {e:#}")); + } // Mirror the freshly-planted proposal/ tag to the forge. if let Err(e) = crate::forge::push_config(agent).await { tracing::warn!(%agent, %id, error = ?e, "forge: push_config after submit failed"); } // Phase 5b: surface the new pending approval on the dashboard // event channel. Compute the diff once here so live subscribers - // get a fully-formed row without a snapshot refetch. - let sha_short = sha[..sha.len().min(12)].to_owned(); + // get a fully-formed row without a snapshot refetch. `sha_short` + // is reused from the dedup gate above. let diff = crate::dashboard::approval_diff(agent, id).await; coord.emit_approval_added( id,