From e838454782ec4137ba3acf3900dd7c2ac779f706 Mon Sep 17 00:00:00 2001 From: Gary Basin Date: Thu, 16 Jul 2026 15:18:32 -0400 Subject: [PATCH 1/2] fix(node-sync): write the git identity through the merged view, not the overlay upper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #541 shipped git author identity and it did not work in production. A live prod session with a correctly resolved identity still committed as `Centaur AI `. The daemon wrote the identity into the overlay's `upper` before `mount_overlay`. That is fine for a cold create, and silently useless for a warm claim — which is the common case. A warm pod's overlay is mounted when the pod is created, minutes before the claim that gives it an identity, and modifying an upperdir behind a live mount is undefined: the file lands on disk (the upper listed it, 10s before the commit) while the merged view keeps serving its cached negative dentry, so the agent never sees it. It also leaves the path incoherent — a later mkdir through the mount failed with ESTALE. The lowerdir comment a few lines up in the same loop describes this exact hazard; the identity write walked into it anyway. Write through `mounted.merged` after the mount instead, which is the supported way to modify an overlay. `materialize_git_identity` now takes the `OverlayMountPlan` rather than a bare `&Path` and derives `merged` itself, so passing the upper is no longer expressible. That is the actual fix: the old signature made the wrong call site type-check, and every tempdir unit test passed while production was broken. Adds tests/git_identity_overlay_visibility.rs — a real overlay, mounted first, then the identity, asserting through the merged view. That shape is the only one that can catch this; a tempdir test passes either way. Ordering caveat, documented at the call site: a warm pod's ready marker predates the claim, so there is no write-before-ready guarantee here — the agent could commit in the seconds before the identity lands. That degrades to the baked identity rather than a wrong one. Closing it for real needs the claim path to carry the identity. --- .../centaur-node-syncd/linux_daemon/mod.rs | 37 ++++-- runtime/node-sync/src/materializer.rs | 38 +++++-- .../tests/git_identity_overlay_visibility.rs | 107 ++++++++++++++++++ 3 files changed, 164 insertions(+), 18 deletions(-) create mode 100644 runtime/node-sync/tests/git_identity_overlay_visibility.rs 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); +} From cec263b04cd73e8fec765d71b2b9a4ef67a36fe6 Mon Sep 17 00:00:00 2001 From: Gary Basin Date: Thu, 16 Jul 2026 15:51:11 -0400 Subject: [PATCH 2/2] ci(node-sync): run the privileged overlay tests as root so they actually gate cargo test runs as the unprivileged runner, so every test needing a real overlay mount self-skips there. That is exactly how the identity bug reached prod green: the write went to an upperdir behind a live mount, and no lane could see it. --- .github/workflows/centaur-ci.yml | 13 +++++++++++++ 1 file changed, 13 insertions(+) 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