From ec355e32fadeccaa78d6aa7e237532d8bf0027ba Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:55:59 +0200 Subject: [PATCH 1/2] fix(validate): retired declarations leave the root slot (#402) Root counting in validate's root-count invariant and the graph's admitted topology counted every supervisor-less spec, so legacy `retired #true` tombstones held a host's root slot and the live dev3 catalog failed with "found 8". The folded desired-state model already maps both retirement spellings to AgentDesiredState::Retired; both consumers now share one predicate (supervisor_chain::is_counted_root) so a retired declaration never counts, a suspended root still does, and zero counted roots remains an error. The graph declarations view folds legacy `retired #true` to desiredState "retired"; an absent lifecycle stays null (lowers to running). Also repairs six tests/validate.rs expectations to the #399 semantics (that target is not CI-gated, so the drift shipped unnoticed): a dangling supervisor is now an error, driver/deliver conflicts refuse at parse time, native drivers require delivery-readiness, and fixtures satisfy the one-root invariant. agent-identity: dev3.direct.omp.wk3aavmh agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.0.11 agent-runtime: OMP 18.0.11 tooling-profile: dotfiles@1defc60 --- .../2026-08-31-root-count-retirement-fold.md | 50 +++++++ docs/vrs/spec.md | 14 +- src/catalog_graph.rs | 14 +- src/supervisor_chain.rs | 9 ++ src/validate.rs | 2 +- tests/catalog_graph.rs | 63 ++++++++ tests/validate.rs | 140 +++++++++++++++--- 7 files changed, 268 insertions(+), 24 deletions(-) create mode 100644 docs/vrs/.experiments/2026-08-31-root-count-retirement-fold.md diff --git a/docs/vrs/.experiments/2026-08-31-root-count-retirement-fold.md b/docs/vrs/.experiments/2026-08-31-root-count-retirement-fold.md new file mode 100644 index 00000000..32738c2d --- /dev/null +++ b/docs/vrs/.experiments/2026-08-31-root-count-retirement-fold.md @@ -0,0 +1,50 @@ +# Root counting must fold retirement before graph construction (#402) + +Date: 2026-08-31 +Worktree: schickling/2026-08-31-issue-402 @ 67b18b7 + fix + +## Question + +Does the one-root-per-host invariant (root-count, admitted topology) reject the live dev3 +catalog because legacy `retired #true` declarations hold the root slot, and does folding +retirement before counting fix it without weakening the invariant for genuine faults? + +## Method + +Built the worktree binary and ran `st2 validate` and `st2 catalog graph --json` against +five temp catalogs (dev3 shape: 1 running root + 2 legacy-retired + 1 new-style-retired +root-shaped declarations; suspended-only root; tombstone-only host; headless host — active +worker under a retired root; two running roots) and once against the live dev3 catalog +under a read-only shared lock. Baseline comparison: same commands with the deployed +pre-#399 `st2` and with the unmodified-main binary (keyed-stash run). + +## Result + +- Unfixed binary, dev3 shape: `root-count: host 'dev3' must declare exactly one root + agent; found 4`; graph `complete: false`; `declarations[].agents[].desiredState` null + for legacy-retired declarations. Live dev3: same error with `found 8`, later confirmed + as `cos` + 7 root-shaped legacy-retired declarations. +- Fixed binary, dev3 shape and live dev3: validate carries no root-count error; live + graph reports exactly one counted root (`dev3.cos`), `cos` gets `rootId: dev3.cos, + depth: 0`, and 622 declaration entries publish the folded `desiredState: "retired"`. +- Suspended-only root: green — a suspended root still counts. +- Tombstone-only host and headless host: `root-count … found 0` — genuine faults stay + errors. Two running roots: `found 2` — regression intact. +- Full `cargo test --test validate --test catalog_graph` green after the fix and the + six stale-test repairs; every CI-gated flake target green. + +## Conclusion + +The defect was #399's root counting (validate.rs root_counts, catalog_graph.rs +admitted_topology) ignoring the folded desired state, not the spec model — the fold +existed end-to-end and only the invariant's predicate and the declarations view missed +it. Excluding retired declarations (either spelling) via one shared predicate +(`supervisor_chain::is_counted_root`) fixes the live catalog while every genuine +topology fault still errors. Zero-count hosts remain faults by design (headless org). + +## VRS Impact + +`docs/vrs/spec.md` (catalog graph / R04–R05 area) now states the counting fold: retired +declarations never hold the root slot, suspended roots still count, and the declarations +view folds legacy `retired #true` to `desiredState: "retired"`. Requirements R04/R35 are +unchanged — "exactly one root" is interpreted over the non-retired org chart. diff --git a/docs/vrs/spec.md b/docs/vrs/spec.md index d1380090..c57847b4 100644 --- a/docs/vrs/spec.md +++ b/docs/vrs/spec.md @@ -768,10 +768,18 @@ also publishes admitted topology: ``` A root has null `parentId`, its own `rootId`, depth zero, and an empty ancestor -array. Duplicate identity, missing or ambiguous parent, cycle, depth beyond 64, -or a host with other than one root is an error. Every affected topology field +array. Root counting folds retirement before the graph is built: a retired +declaration — legacy `retired #true` or `desired-state "retired"` — is outside +the org chart and never holds the root slot, while a suspended root still +counts, so a host suspending its only root stays valid and a host whose every +root is retired reports zero (#402). Duplicate identity, missing or ambiguous +parent, cycle, depth beyond 64, or a host with other than one counted root is +an error. Every affected topology field is null and the graph envelope has `complete: false`; downstream consumers use -these admitted facts rather than walking supervisor edges themselves. +these admitted facts rather than walking supervisor edges themselves. The +`declarations` view applies the same fold to legacy `retired #true`, publishing +`desiredState: "retired"`; an absent lifecycle stays null, which lowers to +running. Retired reconciliation first attempts every live task teardown for the agent. Only when all of those attempts succeed does it settle the declaration's whole diff --git a/src/catalog_graph.rs b/src/catalog_graph.rs index 350b9a05..bfa34b92 100644 --- a/src/catalog_graph.rs +++ b/src/catalog_graph.rs @@ -299,7 +299,8 @@ fn admitted_topology( if specs .iter() .filter(|candidate| { - candidate.resolved_host(this_host) == host && candidate.supervisor.is_none() + candidate.resolved_host(this_host) == host + && crate::supervisor_chain::is_counted_root(candidate) }) .count() != 1 @@ -352,6 +353,14 @@ fn graph_declaration<'a>( .iter() .map(|agent| { let desired = agent.field("desired-state"); + // Legacy `retired #true` carries no `desired-state` node; fold it so the + // declaration view matches the folded spec view (#402). `null` keeps one + // meaning: no lifecycle declared, which lowers to running. + let legacy_retired = agent + .field("retired") + .and_then(|node| node.argument(0)) + .and_then(DeclaredValue::as_bool) + .unwrap_or(false); PartialAgent { identity: agent.identity().and_then(DeclaredValue::as_str).map(str::to_owned), host: declared_field(agent, "host"), @@ -362,7 +371,8 @@ fn graph_declaration<'a>( desired_state: desired .and_then(|node| node.argument(0)) .and_then(DeclaredValue::as_str) - .map(str::to_owned), + .map(str::to_owned) + .or_else(|| legacy_retired.then(|| "retired".to_owned())), desired_state_reason: desired .and_then(|node| node.property("reason")) .and_then(DeclaredValue::as_str) diff --git a/src/supervisor_chain.rs b/src/supervisor_chain.rs index 952da483..c237d7ea 100644 --- a/src/supervisor_chain.rs +++ b/src/supervisor_chain.rs @@ -35,6 +35,15 @@ pub fn resolve_spec<'a>( matches.next().is_none().then_some(first) } +/// Whether a declaration occupies its host's root slot in the org chart: no supervisor, and not +/// retired. Retirement — either spelling; the folded `AgentDesiredState` normalizes legacy +/// `retired #true` and `desired-state "retired"` — removes a declaration from the org chart, so a +/// retired root does not hold the slot. Suspension keeps the declaration in the chart, so a +/// suspended root still counts (#402). +pub fn is_counted_root(spec: &AgentSpec) -> bool { + spec.supervisor.is_none() && !spec.desired_state.is_retired() +} + /// Every spec from `start` to the root inclusive, `start` first. pub fn chain<'a>( specs: &'a [AgentSpec], diff --git a/src/validate.rs b/src/validate.rs index 9f7d5b1a..beed7698 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -451,7 +451,7 @@ pub(crate) fn validate_discovered( for spec in &d.specs { let host = spec.resolved_host(this_host.unwrap_or_default()).to_owned(); root_counts.entry(host).or_default(); - if spec.supervisor.is_none() { + if crate::supervisor_chain::is_counted_root(spec) { *root_counts .get_mut(spec.resolved_host(this_host.unwrap_or_default())) .expect("root count entry was just inserted") += 1; diff --git a/tests/catalog_graph.rs b/tests/catalog_graph.rs index bb1db7bc..818c773c 100644 --- a/tests/catalog_graph.rs +++ b/tests/catalog_graph.rs @@ -162,6 +162,69 @@ fn graph_exposes_admitted_topology_and_delivery_readiness_facts() { ); } +#[test] +fn graph_ignores_retired_roots_and_folds_legacy_retirement_into_declarations() { + // #402 regression fixture: one active root plus root-shaped retired declarations — legacy + // `retired #true` and new-style — must leave the host with exactly one counted root, admit + // the active topology, and expose the fold in the declaration view. + let catalog = tempfile::tempdir().unwrap(); + let root = catalog.path(); + write( + root, + "agents/h/cos/agent.kdl", + r#"agent "cos" { host "h"; command "true" }"#, + ); + write( + root, + "agents/h/old-legacy/agent.kdl", + r#"agent "old-legacy" { host "h"; retired #true; command "true" }"#, + ); + write( + root, + "agents/h/old-explicit/agent.kdl", + r#"agent "old-explicit" { host "h"; desired-state "retired" reason="Replaced by cos"; command "true" }"#, + ); + write( + root, + "agents/h/worker/agent.kdl", + r#"agent "worker" { host "h"; supervisor "h.cos"; command "true" }"#, + ); + + let output = st2(root, &["catalog", "graph", "--host", "h", "--json"], None); + assert_eq!(output.status.code(), Some(0)); + let graph = json(&output); + assert_eq!(graph["complete"], true, "{graph:#}"); + assert!( + !graph["issues"] + .as_array() + .unwrap() + .iter() + .any(|issue| issue["code"] == "root-count"), + "retired roots must not hold the root slot: {graph:#}" + ); + + let rows = graph["agents"].as_array().unwrap(); + let cos = rows.iter().find(|row| row["id"] == "h.cos").unwrap(); + assert_eq!(cos["rootId"], "h.cos"); + assert_eq!(cos["depth"], 0); + let worker = rows.iter().find(|row| row["id"] == "h.worker").unwrap(); + assert_eq!(worker["rootId"], "h.cos"); + assert_eq!(worker["parentId"], "h.cos"); + + let declarations = graph["declarations"].as_array().unwrap(); + let legacy = declarations + .iter() + .find(|row| row["path"] == "agents/h/old-legacy/agent.kdl") + .unwrap(); + assert_eq!(legacy["agents"][0]["desiredState"], "retired"); + // A declaration that states no lifecycle still folds to null (→ running), not "retired". + let active = declarations + .iter() + .find(|row| row["path"] == "agents/h/cos/agent.kdl") + .unwrap(); + assert!(active["agents"][0]["desiredState"].is_null()); +} + #[test] fn graph_rejects_missing_cycle_depth_and_per_host_root_count() { diff --git a/tests/validate.rs b/tests/validate.rs index e2eb8797..44f997eb 100644 --- a/tests/validate.rs +++ b/tests/validate.rs @@ -159,6 +159,7 @@ fn active_agents_may_share_an_opaque_resource_uri() { "h/integrator/agent.kdl", r#"agent "integrator" { host "h" + supervisor "h.reviewer" resource "subject" uri="git-commit://github.com/example/project/0123456789abcdef" reason="reviewed example commit" command "true" }"#, @@ -242,6 +243,8 @@ fn shared_workspace_render_conflict_is_an_error() { #[test] fn a_driver_block_and_deliver_are_two_conflicting_launch_sources() { + // #399 moved the conflict into the parser: a delivery transport pins its session driver, + // so `deliver "app-server"` refuses a `claude` driver block at discovery time. let c = catalog(&[( "h/worker/agent.kdl", r#"agent "worker" { @@ -256,13 +259,14 @@ fn a_driver_block_and_deliver_are_two_conflicting_launch_sources() { let issue = report .issues .iter() - .find(|issue| issue.code == "driver-deliver-conflict") + .find(|issue| issue.code == "parse-error") .unwrap(); assert_eq!(issue.severity, Severity::Error); assert_eq!( issue.message, - "agent 'worker' declares both a driver block and `deliver`; choose one launch source" + "agent 'worker' delivery transport 'app-server' requires session-driver 'codex', not 'claude'" ); + assert_eq!(report.agents, 0, "the declaration never lowers to a spec"); } #[test] @@ -487,6 +491,7 @@ fn fleet_validation_compiles_remote_driver_launches() { host "Silber" workspace "/tmp" claude { prompt "boot" } + delivery-readiness "credential" account-id="tokengate/shared" }"#, )]); let found = st2::discover(c.path()); @@ -502,7 +507,6 @@ fn fleet_validation_compiles_remote_driver_launches() { ); } } - #[test] fn validation_reports_shared_task_compiler_errors() { let c = catalog(&[( @@ -647,19 +651,22 @@ fn the_future_schedule_preview_is_explicitly_rejected() { // ---- warnings -------------------------------------------------------------------------------- #[test] -fn a_dangling_supervisor_is_a_warning() { - let c = catalog(&[( - "hetz/w/agent.kdl", - r#"agent "w" { host "hetz"; type "service"; supervisor "ghost"; pty "agent" { command "x" } }"#, - )]); +fn a_dangling_supervisor_is_an_error() { + // #399 made the supervisor chain authoritative: a reference to a missing parent is a + // structural error, not a warning. + let c = catalog(&[ + ( + "hetz/base/agent.kdl", + r#"agent "base" { host "hetz"; command "x" }"#, + ), + ( + "hetz/w/agent.kdl", + r#"agent "w" { host "hetz"; type "service"; supervisor "ghost"; pty "agent" { command "x" } }"#, + ), + ]); let r = validate(c.path()); - assert!(has(&r, "dangling-supervisor", Severity::Warn)); - assert_eq!( - r.errors(), - 0, - "a dangling supervisor must not be an error: {:?}", - r.issues - ); + assert!(has(&r, "supervisor-missing", Severity::Error)); + assert_eq!(r.errors(), 1, "unexpected issues: {:?}", r.issues); } #[test] @@ -669,6 +676,10 @@ fn a_fully_qualified_supervisor_in_the_catalog_is_clean() { "Silber/cos/agent.kdl", r#"agent "cos" { host "Silber"; command "x" }"#, ), + ( + "hetz/base/agent.kdl", + r#"agent "base" { host "hetz"; command "x" }"#, + ), ( "hetz/w/agent.kdl", r#"agent "w" { host "hetz"; supervisor "Silber.cos"; command "x" }"#, @@ -684,6 +695,97 @@ fn a_fully_qualified_supervisor_in_the_catalog_is_clean() { ); } +#[test] +fn retired_roots_do_not_hold_the_root_slot() { + // #402: legacy `retired #true` (and new-style retirement) removes a declaration from the org + // chart, so root-shaped tombstones must not break the one-root-per-host invariant. + let c = catalog(&[ + ( + "h/cos/agent.kdl", + r#"agent "cos" { host "h"; command "x" }"#, + ), + ( + "h/old-legacy/agent.kdl", + r#"agent "old-legacy" { host "h"; retired #true; command "x" }"#, + ), + ( + "h/old-explicit/agent.kdl", + r#"agent "old-explicit" { host "h"; desired-state "retired" reason="Replaced by cos"; command "x" }"#, + ), + ]); + let r = validate(c.path()); + assert_eq!(r.errors(), 0, "unexpected errors: {:?}", r.issues); +} + +#[test] +fn a_suspended_root_still_holds_the_root_slot() { + // Suspension is a pause inside the org, not an exit: a suspended root keeps the host at + // exactly one root, and a second root-shaped declaration is still an error. + let suspended_only = catalog(&[( + "h/root/agent.kdl", + r#"agent "root" { host "h"; desired-state "suspended" reason="maintenance window"; command "x" }"#, + )]); + assert_eq!( + validate(suspended_only.path()).errors(), + 0, + "a suspended root alone must satisfy the invariant" + ); + + let with_rival = catalog(&[ + ( + "h/root/agent.kdl", + r#"agent "root" { host "h"; desired-state "suspended" reason="maintenance window"; command "x" }"#, + ), + ( + "h/rival/agent.kdl", + r#"agent "rival" { host "h"; retired #true; command "x" }"#, + ), + ]); + assert_eq!( + validate(with_rival.path()).errors(), + 0, + "a retired tombstone must not rival a suspended root" + ); +} + +#[test] +fn a_host_without_one_counted_root_still_fails() { + // Both zero shapes remain faults: a tombstone-only host, and a headless host whose only + // root is retired while workers stay active. + for (name, files) in [ + ( + "tombstone-only", + vec![( + "h/ghost/agent.kdl", + r#"agent "ghost" { host "h"; retired #true; command "x" }"#, + )], + ), + ( + "headless", + vec![ + ( + "h/root/agent.kdl", + r#"agent "root" { host "h"; retired #true; command "x" }"#, + ), + ( + "h/worker/agent.kdl", + r#"agent "worker" { host "h"; supervisor "h.root"; command "x" }"#, + ), + ], + ), + ] { + let c = catalog(&files); + let r = validate(c.path()); + assert!( + r.issues.iter().any(|i| i.code == "root-count" + && i.severity == Severity::Error + && i.message.ends_with("found 0")), + "{name}: expected root-count found 0, got {:?}", + r.issues + ); + } +} + #[test] fn an_identity_folder_mismatch_is_a_warning() { let c = catalog(&[( @@ -775,10 +877,12 @@ fn cli_exits_nonzero_on_an_error_and_strict_promotes_warnings() { )]); assert!(!run_validate(&[err.path().as_os_str()]).status.success()); - // A warning-only catalog exits 0 normally, 1 under --strict. + // A warning-only catalog exits 0 normally, 1 under --strict. Under #399's topology rules a + // lone agent with a mismatched folder name is still exactly one root, so the folder/identity + // mismatch is the only finding — and it is a warning. let warn = catalog(&[( - "hetz/w/agent.kdl", - r#"agent "w" { host "hetz"; type "service"; supervisor "ghost"; pty "agent" { command "x" } }"#, + "hetz/folder-name/agent.kdl", + r#"agent "content-name" { type "service"; pty "agent" { command "x" } }"#, )]); assert!(run_validate(&[warn.path().as_os_str()]).status.success()); let strict = std::ffi::OsStr::new("--strict"); From d041fa9589f5055b6f3d317a0e227bacb91f16d0 Mon Sep 17 00:00:00 2001 From: schickling-assistant <261620128+schickling-assistant@users.noreply.github.com> Date: Mon, 31 Aug 2026 18:11:00 +0200 Subject: [PATCH 2/2] fix(validate): reject active chains rooted at a retired declaration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding on #405 (Codex P1): with retired declarations excluded from root counting, one active root plus a retired root still supervising an active worker validated clean — root-count saw exactly one counted root while the worker's chain terminated at the tombstone, and the graph published complete: true with two disconnected trees. An active agent's chain must now terminate at a counted root (retired-root error); retired chains under a retired root stay legal. Live dev3 catalog verified: zero new errors beyond the pre-existing delivery-readiness ones (#406). agent-identity: dev3.direct.omp.wk3aavmh agent-persona: generalist agent-supervisor: unavailable agent-tool: OMP agent-tool-version: 18.0.11 agent-runtime: OMP 18.0.11 tooling-profile: dotfiles@1defc60 --- .../2026-08-31-root-count-retirement-fold.md | 10 +++ docs/vrs/spec.md | 5 +- src/validate.rs | 66 ++++++++++++------- tests/catalog_graph.rs | 44 +++++++++++++ tests/validate.rs | 53 +++++++++++++++ 5 files changed, 154 insertions(+), 24 deletions(-) diff --git a/docs/vrs/.experiments/2026-08-31-root-count-retirement-fold.md b/docs/vrs/.experiments/2026-08-31-root-count-retirement-fold.md index 32738c2d..c49704f5 100644 --- a/docs/vrs/.experiments/2026-08-31-root-count-retirement-fold.md +++ b/docs/vrs/.experiments/2026-08-31-root-count-retirement-fold.md @@ -42,6 +42,16 @@ it. Excluding retired declarations (either spelling) via one shared predicate (`supervisor_chain::is_counted_root`) fixes the live catalog while every genuine topology fault still errors. Zero-count hosts remain faults by design (headless org). + +Post-review addendum (#405, Codex P1): the first cut of the predicate opened a +hole the pre-fix code had closed only by accident — one active root plus a +retired root still supervising an active worker validated clean (`found 1`, +`complete: true`) while publishing the tombstone as the worker's `rootId`. +Reproduced on the PR head, then closed with a `retired-root` validation error: +an active agent's chain must terminate at a counted root. Retired chains under +a retired root stay legal; the fixture is +`an_active_chain_may_not_terminate_at_a_retired_root`. + ## VRS Impact `docs/vrs/spec.md` (catalog graph / R04–R05 area) now states the counting fold: retired diff --git a/docs/vrs/spec.md b/docs/vrs/spec.md index c57847b4..db303584 100644 --- a/docs/vrs/spec.md +++ b/docs/vrs/spec.md @@ -774,7 +774,10 @@ the org chart and never holds the root slot, while a suspended root still counts, so a host suspending its only root stays valid and a host whose every root is retired reports zero (#402). Duplicate identity, missing or ambiguous parent, cycle, depth beyond 64, or a host with other than one counted root is -an error. Every affected topology field +an error. An active agent whose chain terminates at a retired declaration is +an error too (`retired-root`): the active org chart descends from the counted +root, so one active root plus a retired root still supervising an active +worker does not validate. Every affected topology field is null and the graph envelope has `complete: false`; downstream consumers use these admitted facts rather than walking supervisor edges themselves. The `declarations` view applies the same fold to legacy `retired #true`, publishing diff --git a/src/validate.rs b/src/validate.rs index beed7698..55956b7c 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -403,34 +403,54 @@ pub(crate) fn validate_discovered( } } - if let Err(error) = - crate::supervisor_chain::chain(&d.specs, s, this_host.unwrap_or_default()) - { - let (code, message) = match error { - crate::supervisor_chain::SupervisorChainError::MissingSupervisor => ( - "supervisor-missing", - format!( - "supervisor chain from '{}' references a missing or ambiguous parent", - s.bus_id(this_host.unwrap_or_default()) + match crate::supervisor_chain::chain(&d.specs, s, this_host.unwrap_or_default()) { + Err(error) => { + let (code, message) = match error { + crate::supervisor_chain::SupervisorChainError::MissingSupervisor => ( + "supervisor-missing", + format!( + "supervisor chain from '{}' references a missing or ambiguous parent", + s.bus_id(this_host.unwrap_or_default()) + ), ), - ), - crate::supervisor_chain::SupervisorChainError::Cycle => ( - "supervisor-cycle", - format!( - "supervisor chain from '{}' contains a cycle", - s.bus_id(this_host.unwrap_or_default()) + crate::supervisor_chain::SupervisorChainError::Cycle => ( + "supervisor-cycle", + format!( + "supervisor chain from '{}' contains a cycle", + s.bus_id(this_host.unwrap_or_default()) + ), ), - ), - crate::supervisor_chain::SupervisorChainError::DepthLimit => ( - "supervisor-depth", + crate::supervisor_chain::SupervisorChainError::DepthLimit => ( + "supervisor-depth", + format!( + "supervisor chain from '{}' exceeds the maximum depth of {}", + s.bus_id(this_host.unwrap_or_default()), + crate::supervisor_chain::SUPERVISOR_CHAIN_LIMIT + ), + ), + }; + issues.push(Issue::error(code, rp.clone(), ag.clone(), message)); + } + // Retirement removes a declaration from the org chart, so an active agent's chain + // must terminate at a counted root: one active root plus a retired root still + // supervising an active worker would otherwise validate while the worker's tree is + // headed by a tombstone (#402). + Ok(chain) + if !s.desired_state.is_retired() + && chain.last().is_some_and(|root| root.desired_state.is_retired()) => + { + issues.push(Issue::error( + "retired-root", + rp.clone(), + ag.clone(), format!( - "supervisor chain from '{}' exceeds the maximum depth of {}", + "supervisor chain from '{}' terminates at retired root '{}'; active agents must descend from a counted root", s.bus_id(this_host.unwrap_or_default()), - crate::supervisor_chain::SUPERVISOR_CHAIN_LIMIT + chain.last().expect("chain contains at least its start").bus_id(this_host.unwrap_or_default()), ), - ), - }; - issues.push(Issue::error(code, rp.clone(), ag.clone(), message)); + )); + } + Ok(_) => {} } // Overlay lint: render's persona overlay `@import`s must resolve (WARN — render concern). diff --git a/tests/catalog_graph.rs b/tests/catalog_graph.rs index 818c773c..cc52eca4 100644 --- a/tests/catalog_graph.rs +++ b/tests/catalog_graph.rs @@ -226,6 +226,50 @@ fn graph_ignores_retired_roots_and_folds_legacy_retirement_into_declarations() { } +#[test] +fn graph_is_incomplete_when_an_active_worker_descends_from_a_retired_root() { + // #405 review: one counted root satisfies root-count, but a worker supervised by a retired + // tombstone forms a second, dead-headed tree — the envelope must say so. The worker's own + // row still reports its declared chain fact while the graph is incomplete. + let catalog = tempfile::tempdir().unwrap(); + let root = catalog.path(); + write( + root, + "agents/h/live/agent.kdl", + r#"agent "live" { host "h"; command "true" }"#, + ); + write( + root, + "agents/h/dead/agent.kdl", + r#"agent "dead" { host "h"; retired #true; command "true" }"#, + ); + write( + root, + "agents/h/worker/agent.kdl", + r#"agent "worker" { host "h"; supervisor "h.dead"; command "true" }"#, + ); + + let output = st2(root, &["catalog", "graph", "--host", "h", "--json"], None); + assert_eq!(output.status.code(), Some(1)); + let graph = json(&output); + assert_eq!(graph["complete"], false, "{graph:#}"); + assert!( + graph["issues"] + .as_array() + .unwrap() + .iter() + .any(|issue| issue["code"] == "retired-root"), + "expected a retired-root issue: {graph:#}" + ); + let worker = graph["agents"] + .as_array() + .unwrap() + .iter() + .find(|row| row["id"] == "h.worker") + .unwrap(); + assert_eq!(worker["rootId"], "h.dead"); +} + #[test] fn graph_rejects_missing_cycle_depth_and_per_host_root_count() { let issue_codes = |root: &Path| { diff --git a/tests/validate.rs b/tests/validate.rs index 44f997eb..99f442da 100644 --- a/tests/validate.rs +++ b/tests/validate.rs @@ -743,10 +743,63 @@ fn a_suspended_root_still_holds_the_root_slot() { ]); assert_eq!( validate(with_rival.path()).errors(), + 0, "a retired tombstone must not rival a suspended root" ); } +#[test] +fn an_active_chain_may_not_terminate_at_a_retired_root() { + // One counted root satisfies root-count, but the worker's tree is headed by a retired + // tombstone: the active org chart must descend from the counted root (#405 review). + let c = catalog(&[ + ( + "h/live/agent.kdl", + r#"agent "live" { host "h"; command "x" }"#, + ), + ( + "h/dead/agent.kdl", + r#"agent "dead" { host "h"; retired #true; command "x" }"#, + ), + ( + "h/worker/agent.kdl", + r#"agent "worker" { host "h"; supervisor "h.dead"; command "x" }"#, + ), + ]); + let r = validate(c.path()); + assert!( + r.issues.iter().any(|i| i.code == "retired-root" + && i.severity == Severity::Error + && i.message.contains("h.worker") + && i.message.contains("h.dead")), + "expected retired-root on h.worker, got {:?}", + r.issues + ); + assert_eq!(r.errors(), 1, "unexpected issues: {:?}", r.issues); + + // Retired descendants of a retired root stay legal: tombstone trees are outside the + // org chart and the check only binds active agents. + let tombstones = catalog(&[ + ( + "h/live/agent.kdl", + r#"agent "live" { host "h"; command "x" }"#, + ), + ( + "h/dead/agent.kdl", + r#"agent "dead" { host "h"; retired #true; command "x" }"#, + ), + ( + "h/ghost-worker/agent.kdl", + r#"agent "ghost-worker" { host "h"; supervisor "h.dead"; retired #true; command "x" }"#, + ), + ]); + assert_eq!( + validate(tombstones.path()).errors(), + 0, + "retired chains must not error: {:?}", + validate(tombstones.path()).issues + ); +} #[test] fn a_host_without_one_counted_root_still_fails() {