Skip to content

Commit 33d159b

Browse files
feat(catalog): let a catalog declare the pty registry its tasks live in (#66)
A catalog's session registry was only expressible as ambient environment: `effective_pty_root` took an exported `PTY_ROOT`, else `<catalog>/pty`. A host that wants one shared registry across catalogs therefore has to export the root into every process that will ever read the catalog, and a reader that misses it resolves a different registry — `doctor` then reports a live agent's task as dead, which is the wrong diagnosis from the one command whose job is diagnosis. `<catalog>/catalog.kdl` lets the folder say it itself: catalog { pty-root "/run/agents/pty" } Resolution becomes exported `PTY_ROOT` → declaration → `<catalog>/pty`. Ambient still wins so an eval run keeps its short decoupled partition, and a catalog that declares nothing is byte-for-byte unchanged. `st2 env`/`pty`/`shell` follow the declaration but not the ambient value: they describe the catalog to bus-aware tools rather than echo the caller's registry. The file is not a spec — `catalog` is not an `agent` node and `parse_spec` rejects it as a top-level node — so a declaring catalog is still dispatched as a catalog, pinned by a test. A mistyped field would resolve silently back to `<catalog>/pty` and reproduce the exact symptom this fixes, so the field set is closed and `st2 validate` fails it; the runtime path stays on the native root rather than inventing one mid-teardown. Co-authored-by: schickling-assistant <schickling.j@gmail.com>
1 parent 55e149e commit 33d159b

8 files changed

Lines changed: 373 additions & 9 deletions

File tree

README.md

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,28 @@ ${XDG_STATE_HOME:-$HOME/.local/state}/st2/default/catalog
4343
Every catalog-aware command accepts `--catalog`; otherwise st2 uses `$CATALOG`, then that standard
4444
location.
4545

46+
### A catalog may declare its session registry
47+
48+
A catalog's tasks live in `<catalog>/pty` unless the catalog says otherwise. To put several catalogs
49+
in one host-wide `pty` registry — so any viewer enumerates every session without knowing which
50+
catalog produced it — declare it in `<catalog>/catalog.kdl`:
51+
52+
```kdl
53+
catalog {
54+
pty-root "/run/agents/pty"
55+
}
56+
```
57+
58+
`pty-root` accepts `$VAR`/`$CATALOG`; a relative value anchors at the catalog root. The resolution
59+
order is an exported `PTY_ROOT` (a deliberate override, used by `st2 eval` for a short socket path),
60+
then this declaration, then `<catalog>/pty`. A catalog that declares nothing is unaffected.
61+
62+
Prefer the declaration over exporting `PTY_ROOT` into readers: a reader that misses the export
63+
resolves a different registry and reports live agents as dead. When adopting it on a host whose
64+
systemd unit was installed with an ambient `PTY_ROOT`, reinstall the unit without one
65+
(`st2 service install`) — the export still wins, and leaving it pins the supervisor to the old
66+
registry while everything else follows the catalog.
67+
4668
Lifecycle hooks are installed only by the explicit `st2 hooks install` command. The installer
4769
publishes an immutable content-addressed set, then atomically selects it with a receipt. `st2 up`
4870
verifies its own immutable set for Codex launches; any local workspace render that actually

docs/vrs/spec.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,15 @@ validate ──► materialize ──► host-local st2 scheduler/reconciler
7474
binary, starts the control plane again, and proves adoption with the same
7575
agent PID/creation identity and no duplicate process.
7676

77+
- **Session registry:** A catalog owns the `pty` registry holding its tasks.
78+
`<catalog>/pty` is the default; a catalog may declare another so that one host
79+
can share a single registry across catalogs. Resolution is an exported
80+
`PTY_ROOT`, then the catalog's declaration, then the default, applied
81+
uniformly to spawn, list, kill, and the bus environment st2 hands to native
82+
tools, so every reader that can resolve the catalog agrees about where its
83+
sessions are. A declaration whose field set does not match fails `st2
84+
validate` rather than resolving silently back to the default.
85+
7786
## Message lifecycle
7887

7988
```text

src/catalog.rs

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
//! The catalog's own declaration — `<catalog>/catalog.kdl`.
2+
//!
3+
//! Every other file in a catalog describes an agent; this one describes the folder. Only the session
4+
//! registry is declarable today:
5+
//!
6+
//! ```kdl
7+
//! catalog {
8+
//! pty-root "/run/agents/pty"
9+
//! }
10+
//! ```
11+
//!
12+
//! It is deliberately not a spec. `catalog` is not an `agent` node, so discovery lowers nothing from
13+
//! it, and `eval_spec::parse_spec` rejects `catalog` as a top-level node, so a catalog that declares
14+
//! a root is still dispatched as a catalog and never mistaken for a single-file team spec.
15+
16+
use std::path::{Path, PathBuf};
17+
18+
use kdl::KdlDocument;
19+
20+
/// The catalog-level declaration, read from the catalog root.
21+
pub const CONFIG_FILE: &str = "catalog.kdl";
22+
23+
/// What `<catalog>/catalog.kdl` declares. An absent file leaves every field `None`.
24+
#[derive(Debug, Default, Clone, PartialEq, Eq)]
25+
pub struct CatalogConfig {
26+
/// The `pty` session registry holding this catalog's tasks. Relative values anchor at the
27+
/// catalog root; `$VAR`/`$CATALOG` are expanded at use.
28+
pub pty_root: Option<String>,
29+
}
30+
31+
/// `<catalog>/catalog.kdl`.
32+
pub fn config_path(catalog_root: &Path) -> PathBuf {
33+
catalog_root.join(CONFIG_FILE)
34+
}
35+
36+
/// Parse a catalog declaration.
37+
///
38+
/// An unknown child of `catalog{}` is an error rather than ignored: a typo'd `pty_root` would
39+
/// silently resolve back to `<catalog>/pty` and reappear as a live agent whose task reads dead —
40+
/// exactly the split registry this declaration exists to prevent. Its value set is closed, so the
41+
/// lint cannot fire on a render-only field st2 ignores by design.
42+
///
43+
/// Top-level nodes other than `catalog` are left alone: the same file may legitimately hold `agent`
44+
/// nodes, which discovery owns.
45+
pub fn parse(text: &str) -> anyhow::Result<CatalogConfig> {
46+
let doc = KdlDocument::parse(text).map_err(|e| anyhow::anyhow!("KDL parse error: {e}"))?;
47+
let mut config = CatalogConfig::default();
48+
let mut seen = false;
49+
50+
for node in doc.nodes().iter().filter(|n| n.name().value() == "catalog") {
51+
if seen {
52+
anyhow::bail!("catalog block declared more than once");
53+
}
54+
seen = true;
55+
let Some(children) = node.children() else {
56+
continue;
57+
};
58+
for child in children.nodes() {
59+
match child.name().value() {
60+
"pty-root" => {
61+
let value = child
62+
.get(0)
63+
.and_then(|v| v.as_string())
64+
.filter(|v| !v.is_empty())
65+
.ok_or_else(|| {
66+
anyhow::anyhow!("pty-root needs a non-empty path, e.g. pty-root \"/run/agents/pty\"")
67+
})?;
68+
config.pty_root = Some(value.to_string());
69+
}
70+
other => anyhow::bail!("unknown catalog field '{other}' (expected pty-root)"),
71+
}
72+
}
73+
}
74+
Ok(config)
75+
}
76+
77+
/// Read `<catalog>/catalog.kdl`. A missing file is the default declaration, not an error.
78+
pub fn load(catalog_root: &Path) -> anyhow::Result<CatalogConfig> {
79+
match std::fs::read_to_string(config_path(catalog_root)) {
80+
Ok(text) => parse(&text),
81+
Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(CatalogConfig::default()),
82+
Err(e) => Err(e.into()),
83+
}
84+
}
85+
86+
/// The session registry the CATALOG itself declares: `pty-root` if it declares one, else the native
87+
/// `<catalog>/pty`. This is what `st2 env`/`st2 pty`/`st2 shell` hand to bus-aware tools, so those
88+
/// describe the catalog rather than whatever registry the caller happens to be standing in.
89+
///
90+
/// A malformed declaration falls back to the default instead of failing: this runs on every spawn,
91+
/// list, and kill, including teardown, and inventing a root there is worse than using the native
92+
/// one. `st2 validate` is where a bad declaration is reported.
93+
pub fn pty_root(catalog_root: &Path) -> PathBuf {
94+
match load(catalog_root).ok().and_then(|c| c.pty_root) {
95+
// Join, so a relative declaration anchors at the catalog instead of the caller's cwd.
96+
Some(declared) => catalog_root.join(crate::expand::expand_catalog(&declared, catalog_root)),
97+
None => catalog_root.join("pty"),
98+
}
99+
}
100+
101+
#[cfg(test)]
102+
mod tests {
103+
use super::*;
104+
105+
#[test]
106+
fn an_undeclared_catalog_keeps_the_native_root() {
107+
let tmp = tempfile::tempdir().unwrap();
108+
assert_eq!(load(tmp.path()).unwrap(), CatalogConfig::default());
109+
assert_eq!(pty_root(tmp.path()), tmp.path().join("pty"));
110+
111+
// A file that declares other things, but no pty root.
112+
std::fs::write(config_path(tmp.path()), "agent \"a\" { command \"true\" }\n").unwrap();
113+
assert_eq!(pty_root(tmp.path()), tmp.path().join("pty"));
114+
}
115+
116+
#[test]
117+
fn a_declared_root_is_expanded_and_anchored_at_the_catalog() {
118+
let tmp = tempfile::tempdir().unwrap();
119+
std::fs::write(
120+
config_path(tmp.path()),
121+
"catalog {\n pty-root \"/run/agents/pty\"\n}\n",
122+
)
123+
.unwrap();
124+
assert_eq!(pty_root(tmp.path()), PathBuf::from("/run/agents/pty"));
125+
126+
std::fs::write(config_path(tmp.path()), "catalog { pty-root \"$CATALOG/../shared\" }\n")
127+
.unwrap();
128+
assert_eq!(pty_root(tmp.path()), tmp.path().join("../shared"));
129+
130+
// A relative value belongs to the catalog, never to the caller's cwd.
131+
std::fs::write(config_path(tmp.path()), "catalog { pty-root \"registry\" }\n").unwrap();
132+
assert_eq!(pty_root(tmp.path()), tmp.path().join("registry"));
133+
}
134+
135+
#[test]
136+
fn a_mistyped_declaration_is_an_error_not_a_silent_default() {
137+
assert!(parse("catalog { pty_root \"/run/agents/pty\" }").is_err());
138+
assert!(parse("catalog { pty-root }").is_err());
139+
assert!(parse("catalog { pty-root \"\" }").is_err());
140+
assert!(parse("catalog { pty-root \"/a\" }\ncatalog { pty-root \"/b\" }").is_err());
141+
assert!(parse("this is (not kdl").is_err());
142+
143+
// Reported by `st2 validate`; the runtime path stays on the native root.
144+
let tmp = tempfile::tempdir().unwrap();
145+
std::fs::write(config_path(tmp.path()), "catalog { pty_root \"/run/agents/pty\" }\n")
146+
.unwrap();
147+
assert_eq!(pty_root(tmp.path()), tmp.path().join("pty"));
148+
}
149+
}

src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
//! each declaration's command, environment, hooks, and workspace materialization block.
66
77
pub mod agents;
8+
pub mod catalog;
89
pub mod compile_agent;
910
pub mod context;
1011
pub mod ding;

src/main.rs

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -884,11 +884,12 @@ fn catalog_root_for_env() -> Result<PathBuf> {
884884
absolute_catalog_path(&root)
885885
}
886886

887-
/// Set the same native catalog environment that `st2 env` prints.
887+
/// Set the same native catalog environment that `st2 env` prints. The catalog's own declared session
888+
/// registry is used, not the caller's ambient one: these hand `pty` the roots of the *catalog*.
888889
fn with_bus_env(cmd: &mut std::process::Command, root: &Path) {
889890
cmd.env("CATALOG", root)
890891
.env("ST_ROOT", root)
891-
.env("PTY_ROOT", root.join("pty"));
892+
.env("PTY_ROOT", st2::catalog::pty_root(root));
892893
}
893894

894895
/// `st2 pty [<pty-args>…]` — a thin pass-through to `pty` with the catalog's bus env pre-set, so
@@ -921,12 +922,15 @@ fn shell_cmd(args: &[String]) -> Result<()> {
921922
}
922923

923924
fn env_cmd(root: &Path) -> Result<()> {
924-
let c = root.canonicalize().unwrap_or_else(|_| root.to_path_buf());
925-
let c = c.display();
925+
let canonical = root.canonicalize().unwrap_or_else(|_| root.to_path_buf());
926+
let c = canonical.display();
926927
// The same roots st2 sets on every task it spawns.
927928
println!("export CATALOG={c}");
928929
println!("export ST_ROOT={c}");
929-
println!("export PTY_ROOT={c}/pty");
930+
println!(
931+
"export PTY_ROOT={}",
932+
st2::catalog::pty_root(&canonical).display()
933+
);
930934
Ok(())
931935
}
932936

src/run.rs

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,8 @@ struct PtyListEntry {
149149

150150
/// The `PTY_ROOT` st2 uses for a pty op. An EXPORTED ambient `PTY_ROOT` WINS — a decoupled partition,
151151
/// e.g. an eval run's short `/tmp/stev-<runid>` that dodges the 104-byte unix-socket-path limit that a
152-
/// deep `<catalog>/pty` would blow — else the native default `<catalog>/pty`. Applied uniformly to
152+
/// deep `<catalog>/pty` would blow — else what the catalog itself declares
153+
/// ([`crate::catalog::pty_root`]), else the native default `<catalog>/pty`. Applied uniformly to
153154
/// spawn and list/kill so st2 always manages sessions where it put them.
154155
pub fn effective_pty_root(catalog_root: &Path) -> PathBuf {
155156
effective_pty_root_from(catalog_root, std::env::var_os("PTY_ROOT"))
@@ -160,7 +161,7 @@ pub fn effective_pty_root(catalog_root: &Path) -> PathBuf {
160161
fn effective_pty_root_from(catalog_root: &Path, ambient: Option<std::ffi::OsString>) -> PathBuf {
161162
match ambient {
162163
Some(v) if !v.is_empty() => PathBuf::from(v),
163-
_ => catalog_root.join("pty"),
164+
_ => crate::catalog::pty_root(catalog_root),
164165
}
165166
}
166167

@@ -1362,6 +1363,30 @@ mod tests {
13621363
);
13631364
}
13641365

1366+
#[test]
1367+
fn a_catalog_declared_root_outranks_the_default_but_never_an_ambient_one() {
1368+
let tmp = tempfile::tempdir().unwrap();
1369+
let cat = tmp.path();
1370+
std::fs::write(
1371+
cat.join(crate::catalog::CONFIG_FILE),
1372+
"catalog { pty-root \"/run/agents/pty\" }\n",
1373+
)
1374+
.unwrap();
1375+
1376+
// The declaration replaces the `<catalog>/pty` default for every st2 pty op — so a reader
1377+
// that resolves the catalog finds the sessions without being handed an env var.
1378+
assert_eq!(
1379+
effective_pty_root_from(cat, None),
1380+
std::path::PathBuf::from("/run/agents/pty")
1381+
);
1382+
// An explicit ambient root still wins: an eval run's short decoupled partition must be able
1383+
// to override a catalog it copied from.
1384+
assert_eq!(
1385+
effective_pty_root_from(cat, Some("/tmp/stev-abc123".into())),
1386+
std::path::PathBuf::from("/tmp/stev-abc123")
1387+
);
1388+
}
1389+
13651390
#[test]
13661391
fn debounce_never_defers_a_never_seen_task() {
13671392
let t0 = Instant::now();

src/validate.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,19 @@ fn validate_scoped(root: &Path, this_host: Option<&str>) -> Report {
133133
issues.push(Issue::error(code, rel(root, &e.path), None, message));
134134
}
135135

136-
// 2. Raw pass (once per file): a typo'd `type` is normalized to `service` by the parser, so it can
136+
// 2. The catalog's own declaration. Its field set is closed (like `type`), so a typo is checkable
137+
// here without touching render-agnosticism — and it must be, because a mistyped `pty-root`
138+
// silently resolves back to `<catalog>/pty` and reads as an agent whose task is dead.
139+
if let Err(e) = crate::catalog::load(root) {
140+
issues.push(Issue::error(
141+
"catalog-config",
142+
crate::catalog::CONFIG_FILE.to_string(),
143+
None,
144+
e.to_string(),
145+
));
146+
}
147+
148+
// 3. Raw pass (once per file): a typo'd `type` is normalized to `service` by the parser, so it can
137149
// only be seen before lowering. A KDL `pty`/`exec` block with no name is silently dropped — the
138150
// task just vanishes, the classic "silently does the wrong thing".
139151
let mut files: Vec<&PathBuf> = d.specs.iter().map(|s| &s.path).collect();
@@ -161,7 +173,7 @@ fn validate_scoped(root: &Path, this_host: Option<&str>) -> Report {
161173
issues.extend(kdl_shape_check(root, f));
162174
}
163175

164-
// 3. Resolved pass: cross-spec + field checks over each agent.
176+
// 4. Resolved pass: cross-spec + field checks over each agent.
165177
let identities: HashSet<&str> = d.specs.iter().map(|s| s.identity.as_str()).collect();
166178
let mut seen: HashMap<String, PathBuf> = HashMap::new();
167179
// Placeholder host for bus-id collision: catalogs carry explicit host, and an empty host still

0 commit comments

Comments
 (0)