diff --git a/.github/workflows/centaur-ci.yml b/.github/workflows/centaur-ci.yml index 87b175ec..2de157a1 100644 --- a/.github/workflows/centaur-ci.yml +++ b/.github/workflows/centaur-ci.yml @@ -304,6 +304,19 @@ jobs: cargo clippy --all-targets -- -D warnings cargo test + # `cargo test` above runs as the unprivileged runner, so every test that needs + # a real overlay mount SELF-SKIPS there — silently. That is how the git identity + # shipped writing to an overlay's upperdir behind a live mount: invisible to the + # agent, and green on every lane. Re-run those under sudo so they actually gate + # (ubuntu-latest supports overlayfs + sudo — same assumption ci/overlay-validation.sh + # makes). Separate CARGO_TARGET_DIR so root-owned artifacts cannot poison the + # rust-cache the runner saves at job end. + - name: Privileged overlay tests (real mounts, root) + working-directory: runtime/node-sync + run: | + sudo -E env "PATH=$PATH" CARGO_TARGET_DIR=/tmp/privileged-target \ + cargo test --test git_identity_overlay_visibility -- --nocapture + # Real overlay + scan-demo: locks in the syscall proofs (whiteout→Delete, # symlink→metadata-only, openat2 NO_SYMLINKS blocks the /etc/shadow escape). - name: Overlay scanner validation diff --git a/runtime/node-sync/src/bin/centaur-node-syncd/linux_daemon/mod.rs b/runtime/node-sync/src/bin/centaur-node-syncd/linux_daemon/mod.rs index b0d2ffe2..1fc4991c 100644 --- a/runtime/node-sync/src/bin/centaur-node-syncd/linux_daemon/mod.rs +++ b/runtime/node-sync/src/bin/centaur-node-syncd/linux_daemon/mod.rs @@ -427,12 +427,28 @@ fn run_multi_session(global: GlobalConfig, overlays_root: PathBuf, once: bool, i }; let wip_remounted = !has_active_mount || mounted_overlays.get(&discovered.session) != Some(&plan); + let mounted = match mount_overlay(plan, Some(discovered.manifest.agent_uid)) { + Ok(plan) => plan, + Err(e) => { + eprintln!("session {}: overlay mount: {e}", discovered.session); + continue; + } + }; // Git author identity is server-derived per CLAIM, and a claim is a // mount — so refresh it only when the mount is being (re)established, // never on every tick. An unconditional per-session GET per tick is // exactly the steady-state cost the atrium delta protocol exists to - // avoid. Writing into the upper before mount_overlay is deliberate: - // the agent must never observe a home without its identity. + // avoid. + // + // This MUST write through `mounted.merged`, never `discovered.upper`. + // A warm pod's overlay is already mounted long before the claim that + // gives it an identity, and modifying an overlay's upperdir behind a + // live mount is undefined: the file lands on disk but the merged view + // keeps serving its cached (negative) dentry, so the agent never sees + // it — and the path is left so incoherent that a later mkdir through + // the mount fails with ESTALE. Shipped exactly that way once: the + // identity file was present in the upper and the agent still committed + // as "Centaur AI". Same hazard the lowerdir comment above describes. // // Failures here must NOT skip the mount. The image bakes a // "Centaur AI" identity and /opt/centaur/gitconfig includes this file @@ -440,6 +456,14 @@ fn run_multi_session(global: GlobalConfig, overlays_root: PathBuf, once: bool, i // pre-existing behavior. Gating the mount on identity would turn a // surface blip into "the agent's home never mounts" — trading a // recoverable misattributed commit for an unrecoverable dead session. + // + // Ordering caveat: mount_overlay writes the ready marker, and a warm + // pod's marker predates this claim entirely, so there is no + // write-before-ready guarantee to be had here — the agent could in + // principle commit in the seconds before this lands. That degrades to + // the baked identity rather than a wrong one, which is why it is + // acceptable; closing it for real needs the claim path itself to carry + // the identity. if wip_remounted { let identity_client = HttpAtriumClient::new( &global.base_url, @@ -449,7 +473,7 @@ fn run_multi_session(global: GlobalConfig, overlays_root: PathBuf, once: bool, i match identity_client.get_git_identity() { Ok(identity) => { if let Err(error) = materialize_git_identity( - &discovered.upper, + &mounted, identity.as_ref(), Some(discovered.manifest.agent_uid), ) { @@ -467,13 +491,6 @@ fn run_multi_session(global: GlobalConfig, overlays_root: PathBuf, once: bool, i } } } - let mounted = match mount_overlay(plan, Some(discovered.manifest.agent_uid)) { - Ok(plan) => plan, - Err(e) => { - eprintln!("session {}: overlay mount: {e}", discovered.session); - continue; - } - }; let session = session_config_from_discovered(&discovered, &mounted); mounted_overlays.insert(discovered.session.clone(), mounted); let first_seen = !states.contains_key(&session.session); diff --git a/runtime/node-sync/src/materializer.rs b/runtime/node-sync/src/materializer.rs index eb4dd23e..618846a6 100644 --- a/runtime/node-sync/src/materializer.rs +++ b/runtime/node-sync/src/materializer.rs @@ -6,6 +6,7 @@ use std::sync::{Mutex, OnceLock}; use std::time::{Duration, Instant}; use crate::http_client::GitIdentity; +use crate::overlay_mount::OverlayMountPlan; use crate::runtime::{AtriumChannel, AtriumClient, ContextDeltaRequest, ContextDocResponse}; use crate::state::{ContextDocState, DaemonState}; @@ -87,14 +88,24 @@ If a listed path is missing, it is still materializing — wait a few seconds an pub const CONTEXT_READY_MARKER: &str = ".atrium-context-ready"; pub const GIT_IDENTITY_RELATIVE_PATH: &str = ".config/git/atrium-identity"; -/// Authoritatively materialize the server-derived identity into an overlay upper. +/// Authoritatively materialize the server-derived identity into a session's home. /// `None` is the 204 fallback and removes a prior identity so the image default wins. +/// +/// Takes the mounted `OverlayMountPlan` rather than a bare path ON PURPOSE: this must +/// be written through `plan.merged`, and an earlier version took a `&Path` and got +/// handed the overlay's `upper` instead. That compiled, passed every tempdir unit +/// test, and was silently useless in production — a warm pod's overlay is already +/// mounted long before the claim that gives it an identity, and writing to an +/// upperdir behind a live mount is undefined: the file lands on disk while the merged +/// view keeps serving its cached negative dentry, so the agent never sees it and +/// commits as the baked "Centaur AI". Deriving the path here instead of accepting one +/// makes that mistake unrepresentable rather than merely documented. pub fn materialize_git_identity( - home: &Path, + plan: &OverlayMountPlan, identity: Option<&GitIdentity>, agent_uid: Option, ) -> Result<(), String> { - let dst = home.join(GIT_IDENTITY_RELATIVE_PATH); + let dst = plan.merged.join(GIT_IDENTITY_RELATIVE_PATH); let Some(identity) = identity else { remove_file_if_present(&dst)?; return Ok(()); @@ -988,6 +999,17 @@ mod tests { } } + // Build via the real constructor so the plan cannot drift from production's shape. + // `merged` is a plain tempdir here: these unit tests cover the FILE contract + // (escaping, idempotency, 204-removal). They CANNOT cover overlay visibility, which + // is what actually broke in production — see tests/git_identity_overlay_visibility.rs. + fn plan_for(home: &Path) -> OverlayMountPlan { + let overlays_root = home.join("overlays-root"); + std::fs::create_dir_all(&overlays_root).unwrap(); + crate::overlay_mount::plan_overlay_mount(&overlays_root, "unit", home, "", &[], None) + .unwrap() + } + // Pinned to `/` for the same reason the writer is: git discovers a repository from // the cwd even for `--file` reads, so running the suite from inside a linked // worktree (whose .git is a gitfile) would fail these on repo discovery, not on @@ -1009,7 +1031,7 @@ mod tests { std::fs::create_dir_all(dst.parent().unwrap()).unwrap(); std::fs::write(&dst, b"stale").unwrap(); - materialize_git_identity(temp.path(), None, None).unwrap(); + materialize_git_identity(&plan_for(temp.path()), None, None).unwrap(); assert!(!dst.exists()); } @@ -1020,7 +1042,7 @@ mod tests { let identity = git_identity("Allan Niemerg"); let dst = temp.path().join(GIT_IDENTITY_RELATIVE_PATH); - materialize_git_identity(temp.path(), Some(&identity), None).unwrap(); + materialize_git_identity(&plan_for(temp.path()), Some(&identity), None).unwrap(); assert_eq!( std::fs::read_to_string(&dst).unwrap(), @@ -1043,7 +1065,7 @@ mod tests { let identity = git_identity(hostile); let dst = temp.path().join(GIT_IDENTITY_RELATIVE_PATH); - materialize_git_identity(temp.path(), Some(&identity), None).unwrap(); + materialize_git_identity(&plan_for(temp.path()), Some(&identity), None).unwrap(); let output = git_config_get(&dst, "user.name"); assert!(output.status.success()); @@ -1064,9 +1086,9 @@ mod tests { let identity = git_identity("Allan Niemerg"); let dst = temp.path().join(GIT_IDENTITY_RELATIVE_PATH); - materialize_git_identity(temp.path(), Some(&identity), None).unwrap(); + materialize_git_identity(&plan_for(temp.path()), Some(&identity), None).unwrap(); let first = std::fs::read(&dst).unwrap(); - materialize_git_identity(temp.path(), Some(&identity), None).unwrap(); + materialize_git_identity(&plan_for(temp.path()), Some(&identity), None).unwrap(); assert_eq!(std::fs::read(&dst).unwrap(), first); } diff --git a/runtime/node-sync/tests/git_identity_overlay_visibility.rs b/runtime/node-sync/tests/git_identity_overlay_visibility.rs new file mode 100644 index 00000000..7091fca9 --- /dev/null +++ b/runtime/node-sync/tests/git_identity_overlay_visibility.rs @@ -0,0 +1,107 @@ +//! Pins the one thing a tempdir test can never see: the git author identity has to +//! be visible to the AGENT, which reads it through the overlay's merged view. +//! +//! We shipped this wrong once. The daemon wrote the identity into the overlay's +//! upperdir before mounting, which is fine for a cold create but silently useless +//! for a warm claim: a warm pod's overlay is already mounted minutes before the +//! claim that gives it an identity, and modifying an upperdir behind a live mount +//! is undefined behaviour. The file landed on disk, `ls` on the upper showed it, +//! every unit test passed — and the agent still committed as "Centaur AI", because +//! the merged view kept serving its cached negative dentry. The upper was left so +//! incoherent that a later mkdir through the mount failed with ESTALE. +//! +//! So: assert against the MERGED path, on a real overlay, with the mount already +//! established — the warm-claim shape. A unit test on a plain tempdir passes either +//! way and proves nothing. +//! +//! Needs Linux + root + a non-overlayfs TMPDIR (overlay upperdirs cannot live on +//! overlayfs; in a container, mount a tmpfs and point TMPDIR at it). Skips silently +//! otherwise, same as the other privileged on-node validations. + +#![cfg(target_os = "linux")] + +use std::fs; +use std::process::Command; + +use centaur_node_sync::http_client::GitIdentity; +use centaur_node_sync::materializer::{GIT_IDENTITY_RELATIVE_PATH, materialize_git_identity}; +use centaur_node_sync::overlay_mount::{mount_overlay, plan_overlay_mount, unmount_overlay}; + +fn identity() -> GitIdentity { + // `source` rides the wire for observability but the daemon is a tolerant reader + // and never needs it, so it is deliberately absent from this struct. + GitIdentity { + author_name: "Gary".to_string(), + author_email: "10901359+gbasin@users.noreply.github.com".to_string(), + session_id: "2f30f3db-e964-4e25-9371-7d323836c1c7".to_string(), + harness: "codex".to_string(), + } +} + +fn git_get(file: &std::path::Path, key: &str) -> String { + let out = Command::new("git") + .current_dir("/") + .args(["config", "--file"]) + .arg(file) + .args(["--get", key]) + .output() + .unwrap(); + String::from_utf8(out.stdout).unwrap().trim().to_string() +} + +/// The warm-claim shape: overlay ALREADY mounted, then the identity arrives. +#[test] +fn identity_written_after_mount_is_visible_through_the_merged_view() { + if unsafe { libc::geteuid() } != 0 { + eprintln!( + "SKIP: identity_written_after_mount_is_visible_through_the_merged_view requires root" + ); + return; + } + + let session = "git-identity-visibility-it"; + let root = std::env::temp_dir().join(format!("gid-it-{}", std::process::id())); + let _ = fs::remove_dir_all(&root); + let overlays_root = root.join("overlays"); + let merged = root.join("merged").join(session); + fs::create_dir_all(&overlays_root).unwrap(); + fs::create_dir_all(&merged).unwrap(); + + let plan = plan_overlay_mount(&overlays_root, session, &merged, "", &[], None).unwrap(); + let mounted = mount_overlay(plan, None).expect("overlay must mount"); + + // The agent is already live against this mount at this point — exactly the warm + // pod that has been sitting in the pool. NOW the claim's identity shows up. + materialize_git_identity(&mounted, Some(&identity()), None).expect("materialize"); + + let seen_by_agent = mounted.merged.join(GIT_IDENTITY_RELATIVE_PATH); + assert!( + seen_by_agent.exists(), + "identity is invisible through the merged view — the agent would commit as the baked \ + image identity. This is the exact bug that shipped: writing via the upperdir of a live \ + overlay leaves the file on disk but out of the agent's view." + ); + assert_eq!( + git_get(&seen_by_agent, "user.email"), + "10901359+gbasin@users.noreply.github.com" + ); + assert_eq!(git_get(&seen_by_agent, "user.name"), "Gary"); + // The [atrium] block feeds the commit-msg hook's provenance trailers; it travels + // in the same file, so emit both or neither. + assert_eq!( + git_get(&seen_by_agent, "atrium.sessionId"), + "2f30f3db-e964-4e25-9371-7d323836c1c7" + ); + assert_eq!(git_get(&seen_by_agent, "atrium.harness"), "codex"); + + // A 204 must clear it through the same view, so a revoked identity cannot linger + // and keep authoring as someone who no longer resolves. + materialize_git_identity(&mounted, None, None).expect("204 removes"); + assert!( + !seen_by_agent.exists(), + "a 204 must remove the identity through the merged view, not just the upper" + ); + + unmount_overlay(&mounted).unwrap(); + let _ = fs::remove_dir_all(&root); +}