Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions docs/vrs/.experiments/2026-08-31-root-count-retirement-fold.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# 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).


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
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.
17 changes: 14 additions & 3 deletions docs/vrs/spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -768,10 +768,21 @@ 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. 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.
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
Expand Down
14 changes: 12 additions & 2 deletions src/catalog_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"),
Expand All @@ -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)
Expand Down
9 changes: 9 additions & 0 deletions src/supervisor_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Comment thread
schickling-assistant marked this conversation as resolved.
}

/// Every spec from `start` to the root inclusive, `start` first.
pub fn chain<'a>(
specs: &'a [AgentSpec],
Expand Down
68 changes: 44 additions & 24 deletions src/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand All @@ -451,7 +471,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;
Expand Down
107 changes: 107 additions & 0 deletions tests/catalog_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,113 @@ 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_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() {
Expand Down
Loading
Loading