diff --git a/docs/vrs/spec.md b/docs/vrs/spec.md index 4309fe28..1a1e098c 100644 --- a/docs/vrs/spec.md +++ b/docs/vrs/spec.md @@ -74,6 +74,12 @@ validate ──► materialize ──► host-local st2 scheduler/reconciler ordinary declaration space; only `.git` and `.st2` directories at any depth, the catalog root's `pty` child, and a declaration parent's `resources`, `archive`, and `inbox` children are excluded. + A resolved workspace-relative render destination has one coherent desired + state across the active local fleet: byte-equivalent idempotent claims may + share it, while incompatible + claims fail every conflicting owner before the first workspace write. + Targeted reconciliation checks the selected owner against the full fleet, so + selection cannot bypass this ownership boundary. - **R04:** Each machine schedules and reconciles only its pinned work. The st2 loop is deterministic; exactly one declared root agent provides intelligent host-local supervision, bounded recovery, and escalation. Filesystem reads diff --git a/src/main.rs b/src/main.rs index 35d887f0..1b270808 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1785,6 +1785,7 @@ fn up( if materialize_only { let mut found = discover(&catalog_root); + let ownership_specs = found.specs.clone(); if let Some(selector) = task.as_deref() { let (owner, _, _) = st2::reconcile::resolve_task(&found.specs, selector, &this_host)?; let owner_identity = owner.identity.clone(); @@ -1809,7 +1810,12 @@ fn up( "verifying explicitly installed lifecycle hooks before Codex materialization", )?; } - let report = st2::materialize::materialize_catalog(&catalog_root, &found.specs, &this_host); + let report = st2::materialize::materialize_catalog_against( + &catalog_root, + &found.specs, + &ownership_specs, + &this_host, + ); for item in &report.materialized { println!("{item}"); } diff --git a/src/materialize.rs b/src/materialize.rs index 89f021dd..4001aa1a 100644 --- a/src/materialize.rs +++ b/src/materialize.rs @@ -3,7 +3,7 @@ //! The runner stays harness-agnostic: these are generic, ordered file operations. Content operations //! gate an agent's boot; `git-exclude` is advisory and can never prevent a launch. -use std::collections::{BTreeMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::fs; use std::path::{Component, Path, PathBuf}; use std::process::Command; @@ -452,6 +452,143 @@ enum PreparedOp { }, } +#[derive(Debug, Clone, PartialEq)] +enum RenderClaim { + Replace(Vec), + JsonUpsert(serde_json::Value), + EnsureLine(String), +} + +#[derive(Debug)] +pub struct RenderOwnershipConflict { + pub destination: PathBuf, + pub owners: BTreeSet, +} + +impl RenderOwnershipConflict { + fn error(&self) -> String { + format!( + "conflicting render ownership for '{}': active agents {} declare incompatible content for one shared workspace target", + self.destination.display(), + self.owners.iter().cloned().collect::>().join(", ") + ) + } +} + +fn claims_for_agent( + root: &Path, + spec: &AgentSpec, + this_host: &str, +) -> Result>> { + let plan = parse_plan(spec)?; + if plan.ops.is_empty() { + return Ok(BTreeMap::new()); + } + let workspace_raw = spec + .workspace + .as_deref() + .with_context(|| format!("agent '{}' has render{{}} but no workspace", spec.identity))?; + let env = render_env(root, spec, this_host); + let workspace = PathBuf::from(expand(workspace_raw, &env)); + let workspace = workspace + .canonicalize() + .with_context(|| format!("canonicalizing workspace {}", workspace.display()))?; + let spec_dir = spec.path.parent().unwrap_or(root); + let mut claims = BTreeMap::>::new(); + for op in plan.ops { + let (destination, claim) = match op { + RenderOp::Copy { + source: raw_source, + destination: raw_destination, + } => { + let source = source(root, spec_dir, &raw_source, &env)?; + let bytes = fs::read(&source) + .with_context(|| format!("reading copy source {}", source.display()))?; + ( + destination(&workspace, &raw_destination, &env)?, + RenderClaim::Replace(bytes), + ) + } + RenderOp::File { + destination: raw_destination, + content, + } => ( + destination(&workspace, &raw_destination, &env)?, + RenderClaim::Replace(expand(&content, &env).into_bytes()), + ), + RenderOp::JsonUpsert { + destination: raw_destination, + content, + } => { + let patch = serde_json::from_str(&expand(&content, &env)).with_context(|| { + format!( + "expanded json-upsert for '{}' is not valid JSON", + spec.identity + ) + })?; + ( + destination(&workspace, &raw_destination, &env)?, + RenderClaim::JsonUpsert(patch), + ) + } + RenderOp::EnsureLine { + destination: raw_destination, + line, + } => ( + destination(&workspace, &raw_destination, &env)?, + RenderClaim::EnsureLine(expand(&line, &env)), + ), + // Every git-exclude is additive by contract and resolves through Git's own shared + // metadata path rather than a declared workspace-relative render destination. + RenderOp::GitExclude { .. } => continue, + }; + claims.entry(destination).or_default().push(claim); + } + Ok(claims) +} + +/// Find active local agents that declare incompatible desired content for one resolved workspace +/// destination. Equivalent idempotent plans may share a target; differing plans have no implicit +/// last-writer-wins semantics. +pub fn render_ownership_conflicts( + root: &Path, + specs: &[AgentSpec], + this_host: &str, +) -> Vec { + let mut by_destination = BTreeMap::>>::new(); + for spec in specs { + if spec.retired || spec.resolved_host(this_host) != this_host { + continue; + } + let Ok(claims) = claims_for_agent(root, spec, this_host) else { + // The normal per-agent materialization path reports malformed plans and unavailable + // inputs. Ownership analysis only compares claims it can resolve without writing. + continue; + }; + let owner = spec.bus_id(this_host); + for (destination, plan) in claims { + by_destination + .entry(destination) + .or_default() + .insert(owner.clone(), plan); + } + } + + by_destination + .into_iter() + .filter_map(|(destination, claims)| { + let mut plans = claims.values(); + let first = plans.next()?; + plans + .any(|plan| plan != first) + .then(|| RenderOwnershipConflict { + destination, + owners: claims.into_keys().collect(), + }) + }) + .collect() +} + /// Execute one agent's render plan in declaration order. pub fn materialize_agent(root: &Path, spec: &AgentSpec, this_host: &str) -> Result> { let plan = parse_plan(spec)?; @@ -699,12 +836,45 @@ pub fn validate_agent(root: &Path, spec: &AgentSpec, this_host: &str) -> Result< /// Materialize every active agent assigned to `this_host`. pub fn materialize_catalog(root: &Path, specs: &[AgentSpec], this_host: &str) -> MaterializeReport { + materialize_catalog_against(root, specs, specs, this_host) +} + +/// Materialize `selected_specs` after checking their workspace target ownership against the complete +/// active fleet. This preserves shortest-path selection while preventing a selected owner from +/// bypassing a collision declared by an unselected sibling. +pub fn materialize_catalog_against( + root: &Path, + selected_specs: &[AgentSpec], + ownership_specs: &[AgentSpec], + this_host: &str, +) -> MaterializeReport { let mut report = MaterializeReport::default(); - for spec in specs { + let selected_ids = selected_specs + .iter() + .map(|spec| spec.bus_id(this_host)) + .collect::>(); + for conflict in render_ownership_conflicts(root, ownership_specs, this_host) { + let affected = conflict + .owners + .iter() + .filter(|owner| selected_ids.contains(*owner)) + .cloned() + .collect::>(); + if affected.is_empty() { + continue; + } + report.failed_agents.extend(affected); + report.errors.push(conflict.error()); + } + + for spec in selected_specs { if spec.retired || spec.resolved_host(this_host) != this_host { continue; } let bus_id = spec.bus_id(this_host); + if report.failed_agents.contains(&bus_id) { + continue; + } match materialize_agent(root, spec, this_host) { Ok(notes) => { for note in notes { diff --git a/src/run.rs b/src/run.rs index 10f9a3b2..c06a2993 100644 --- a/src/run.rs +++ b/src/run.rs @@ -803,10 +803,16 @@ fn reconcile_pass( .cloned() .collect::>(); - // Ordered, idempotent pre-boot materialization. A gating render failure removes only that agent - // from this pass; advisory git-exclude failures remain warnings and never block a launch. - let materialized = - crate::materialize::materialize_catalog(root, &materializable_specs, this_host); + // Ordered, idempotent pre-boot materialization, with ownership checked against the complete + // active fleet even when another gate defers one owner's writes. A gating render failure removes + // only that agent from this pass; advisory git-exclude failures remain warnings and never block + // a launch. + let materialized = crate::materialize::materialize_catalog_against( + root, + &materializable_specs, + &found.specs, + this_host, + ); report.warnings.extend(materialized.warnings); report.errors.extend(materialized.errors); let eligible_specs: Vec<_> = found @@ -998,9 +1004,10 @@ pub fn up_once_selected( .push(format!("verify lifecycle hooks: {error}")); return Ok(report); } - let materialized = crate::materialize::materialize_catalog( + let materialized = crate::materialize::materialize_catalog_against( catalog_root, std::slice::from_ref(&owner), + &found.specs, this_host, ); report.warnings.extend(materialized.warnings); diff --git a/src/validate.rs b/src/validate.rs index 070f5b0b..7a543258 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -298,6 +298,21 @@ fn validate_scoped(root: &Path, this_host: Option<&str>) -> Report { } } + if let Some(host) = this_host { + for conflict in crate::materialize::render_ownership_conflicts(root, &d.specs, host) { + issues.push(Issue::error( + "render-owner-conflict", + ".".to_string(), + None, + format!( + "conflicting render ownership for '{}': active agents {} declare incompatible content for one shared workspace target", + conflict.destination.display(), + conflict.owners.iter().cloned().collect::>().join(", ") + ), + )); + } + } + Report { issues, agents: d.specs.len(), diff --git a/tests/hooks.rs b/tests/hooks.rs index e0384f40..06f27381 100644 --- a/tests/hooks.rs +++ b/tests/hooks.rs @@ -547,3 +547,69 @@ fn missing_hooks_do_not_rewrite_or_stop_an_already_live_codex_agent() { ); assert!(!hooks_root.exists()); } + +#[test] +fn missing_hooks_cannot_hide_a_shared_workspace_render_conflict() { + let tmp = tempfile::tempdir().unwrap(); + let catalog = tmp.path().join("catalog"); + let workspace = tmp.path().join("shared-workspace"); + let hooks_root = tmp.path().join("hooks"); + let bin = tmp.path().join("bin"); + let pty_log = tmp.path().join("pty.log"); + fs::create_dir_all(&workspace).unwrap(); + fs::create_dir_all(&bin).unwrap(); + let declaration = |identity: &str, command: &str, content: &str| { + format!( + "agent \"{identity}\" {{\n host \"h\"\n workspace \"{}\"\n \ + env {{ ST_AGENT \"h.{identity}\" }}\n command \"{command}\"\n \ + render {{ file \"shared\" \"{content}\" }}\n}}\n", + workspace.display() + ) + }; + let codex = catalog.join("agents/h/codex/agent.kdl"); + let sibling = catalog.join("agents/h/sibling/agent.kdl"); + fs::create_dir_all(codex.parent().unwrap()).unwrap(); + fs::create_dir_all(sibling.parent().unwrap()).unwrap(); + fs::write(&codex, declaration("codex", "exec codex", "codex")).unwrap(); + fs::write(&sibling, declaration("sibling", "exec sibling", "sibling")).unwrap(); + write_executable( + &bin.join("pty"), + &format!( + "#!/bin/sh\nprintf '%s\\n' \"$*\" >> '{}'\nif [ \"$1\" = list ]; then printf '[]\\n'; fi\n", + pty_log.display() + ), + ); + + let output = command(&hooks_root) + .arg("up") + .arg(&catalog) + .args(["--host", "h", "--once"]) + .env("PATH", &bin) + .output() + .unwrap(); + let report = format!( + "{}{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + assert!(output.status.success(), "{report}"); + assert!(report.contains("materialization deferred"), "{report}"); + assert!(report.contains("conflicting render ownership"), "{report}"); + assert!( + !workspace.join("shared").exists(), + "a hook-gated owner must remain visible to fleet-wide conflict preflight" + ); + assert_eq!( + fs::read_to_string(&pty_log) + .unwrap_or_default() + .lines() + .collect::>(), + ["list --json"], + "neither conflicting owner may launch" + ); + assert!( + !hooks_root.exists(), + "ordinary up must not create or rewrite the hook root" + ); +} diff --git a/tests/materialize.rs b/tests/materialize.rs index d80e6475..c47cb453 100644 --- a/tests/materialize.rs +++ b/tests/materialize.rs @@ -2,7 +2,7 @@ use std::fs; use std::path::Path; use std::process::Command; -use st2::materialize::{materialize_catalog, parse_plan}; +use st2::materialize::{materialize_catalog, materialize_catalog_against, parse_plan}; use st2::{AgentSpec, discover}; fn write(path: &Path, contents: impl AsRef<[u8]>) { @@ -75,6 +75,127 @@ fn task_selector_materializes_only_owning_agent() { assert!(String::from_utf8_lossy(&out.stdout).contains("materialized 1 operation")); } +#[test] +fn shared_workspace_conflicting_copy_targets_fail_before_any_write() { + let tmp = tempfile::tempdir().unwrap(); + let catalog = tmp.path().join("catalog"); + let workspace = tmp.path().join("shared-workspace"); + fs::create_dir_all(&workspace).unwrap(); + write( + &catalog.join("agents/Silber/worker/agent.kdl"), + agent_kdl( + &workspace, + r#" copy "_templates/worker.md" ".st2/PERSONA.md""#, + ), + ); + write( + &catalog.join("agents/Silber/orchestrator/agent.kdl"), + agent_kdl( + &workspace, + r#" copy "_templates/orchestrator.md" ".st2/PERSONA.md""#, + ) + .replace("agent \"cos\"", "agent \"orchestrator\"") + .replace("Silber.cos", "Silber.orchestrator"), + ); + write(&catalog.join("_templates/worker.md"), "worker\n"); + write( + &catalog.join("_templates/orchestrator.md"), + "orchestrator\n", + ); + + let found = discover(&catalog); + assert!(found.errors.is_empty(), "{:?}", found.errors); + let report = materialize_catalog(&catalog, &found.specs, "Silber"); + + assert_eq!(report.errors.len(), 1, "{:?}", report.errors); + assert!( + report.errors[0].contains("conflicting render ownership"), + "{:?}", + report.errors + ); + assert_eq!(report.failed_agents.len(), 2); + assert!( + !workspace.join(".st2/PERSONA.md").exists(), + "a conflicting fleet must be rejected before either owner writes" + ); +} + +#[test] +fn selected_owner_cannot_bypass_a_shared_workspace_conflict() { + let tmp = tempfile::tempdir().unwrap(); + let catalog = tmp.path().join("catalog"); + let workspace = tmp.path().join("shared-workspace"); + fs::create_dir_all(&workspace).unwrap(); + write( + &catalog.join("agents/Silber/worker/agent.kdl"), + agent_kdl( + &workspace, + r#" copy "_templates/worker.md" ".st2/PERSONA.md""#, + ), + ); + write( + &catalog.join("agents/Silber/orchestrator/agent.kdl"), + agent_kdl( + &workspace, + r#" copy "_templates/orchestrator.md" ".st2/PERSONA.md""#, + ) + .replace("agent \"cos\"", "agent \"orchestrator\"") + .replace("Silber.cos", "Silber.orchestrator"), + ); + write(&catalog.join("_templates/worker.md"), "worker\n"); + write( + &catalog.join("_templates/orchestrator.md"), + "orchestrator\n", + ); + let found = discover(&catalog); + let worker = found + .specs + .iter() + .find(|spec| spec.identity == "cos") + .unwrap(); + + let report = materialize_catalog_against( + &catalog, + std::slice::from_ref(worker), + &found.specs, + "Silber", + ); + + assert_eq!(report.errors.len(), 1, "{:?}", report.errors); + assert!(report.failed_agents.contains("Silber.cos")); + assert!(!workspace.join(".st2/PERSONA.md").exists()); +} + +#[test] +fn shared_workspace_byte_identical_claims_are_idempotent() { + let tmp = tempfile::tempdir().unwrap(); + let catalog = tmp.path().join("catalog"); + let workspace = tmp.path().join("shared-workspace"); + fs::create_dir_all(&workspace).unwrap(); + let render = r#" copy "_templates/shared.md" ".st2/bus.md""#; + write( + &catalog.join("agents/Silber/a/agent.kdl"), + agent_kdl(&workspace, render), + ); + write( + &catalog.join("agents/Silber/b/agent.kdl"), + agent_kdl(&workspace, render) + .replace("agent \"cos\"", "agent \"b\"") + .replace("Silber.cos", "Silber.b"), + ); + write(&catalog.join("_templates/shared.md"), "shared\n"); + let found = discover(&catalog); + + let report = materialize_catalog(&catalog, &found.specs, "Silber"); + + assert!(report.errors.is_empty(), "{:?}", report.errors); + assert!(report.failed_agents.is_empty()); + assert_eq!( + fs::read_to_string(workspace.join(".st2/bus.md")).unwrap(), + "shared\n" + ); +} + #[test] fn task_selector_ambiguous_refuses_without_mutation() { let tmp = tempfile::tempdir().unwrap(); diff --git a/tests/validate.rs b/tests/validate.rs index 6fcc4f0e..c518aad6 100644 --- a/tests/validate.rs +++ b/tests/validate.rs @@ -77,6 +77,36 @@ fn an_invalid_resource_binding_is_a_parse_error() { assert!(has(&validate(c.path()), "parse-error", Severity::Error)); } +#[test] +fn shared_workspace_render_conflict_is_an_error() { + let c = tempfile::tempdir().unwrap(); + let workspace = c.path().join("workspace"); + std::fs::create_dir_all(&workspace).unwrap(); + std::fs::create_dir_all(c.path().join("_templates")).unwrap(); + std::fs::write(c.path().join("_templates/a"), "a\n").unwrap(); + std::fs::write(c.path().join("_templates/b"), "b\n").unwrap(); + for (identity, template) in [("a", "a"), ("b", "b")] { + let path = c.path().join(format!("h/{identity}/agent.kdl")); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write( + path, + format!( + "agent \"{identity}\" {{ host \"h\"; workspace \"{}\"; command \"true\"; render {{ copy \"_templates/{template}\" \".st2/PERSONA.md\" }} }}", + workspace.display() + ), + ) + .unwrap(); + } + + let r = validate_for_host(c.path(), "h"); + + assert!( + has(&r, "render-owner-conflict", Severity::Error), + "{:?}", + r.issues + ); +} + // ---- errors ---------------------------------------------------------------------------------- #[test]