diff --git a/crates/agent-spec/src/discovery.rs b/crates/agent-spec/src/discovery.rs index c897db3f..c0e03897 100644 --- a/crates/agent-spec/src/discovery.rs +++ b/crates/agent-spec/src/discovery.rs @@ -3,9 +3,9 @@ //! The folder is *both* the catalog and the inboxes: one directory per agent holding its spec plus //! its `inbox/`/`archive/`. Discovery walks `` recursively, parses every `*.{toml,json,kdl}` //! that looks like a spec, and resolves each spec's `identity`/`host` with the gist's precedence: -//! **content wins, the path supplies defaults, a mismatch is a warning** (a convention, not a -//! constraint). Malformed files are collected as errors rather than halting the walk — one bad edit -//! must not wedge the whole reconcile. +//! an explicit identity+host pair is path-independent; otherwise **content wins, the path supplies +//! defaults, and a mismatch is a warning**. Malformed files are collected as errors rather than +//! halting the walk — one bad edit must not wedge the whole reconcile. use std::fs; use std::path::{Component, Path, PathBuf}; @@ -54,10 +54,76 @@ pub fn discover(root: &Path) -> Discovered { out } -/// Recursively gather candidate spec files, skipping dotfiles/dotdirs (`.git`, hidden config), the -/// top-level `pty/` runtime registry, and anything that isn't one of [`SPEC_EXTS`]. `pty` session -/// metadata includes JSON that can resemble an agent spec; it is runner state, never catalog input. -/// Unreadable directories are skipped, not fatal. +/// Whether `path` is in catalog declaration space rather than a known control/runtime namespace. +/// +/// A leading dot has no generic meaning: organizational directories such as `.managed` and +/// `.retired` remain visible. `.git` and `.st2` control directories at any depth, the catalog root's +/// `pty` child, and an actual declaration parent's `resources`, `archive`, and `inbox` children have +/// explicit non-catalog meaning. +pub fn is_catalog_path(root: &Path, path: &Path) -> bool { + let Ok(rel) = path.strip_prefix(root) else { + return false; + }; + let components: Vec<_> = rel + .components() + .filter_map(|component| match component { + Component::Normal(name) => Some(name), + _ => None, + }) + .collect(); + + if components + .iter() + .any(|name| matches!(name.to_str(), Some(".git" | ".st2"))) + || components.first().and_then(|name| name.to_str()) == Some("pty") + { + return false; + } + + let mut parent = root.to_path_buf(); + for name in components { + if matches!(name.to_str(), Some("resources" | "archive" | "inbox")) + && is_declaration_parent(&parent) + { + return false; + } + parent.push(name); + } + true +} + +/// Whether `dir` anchors at least one declaration whose adjacent state directories are not +/// recursively discoverable catalog input. +/// +/// Generic `agent.*` filenames reserve the namespace even while malformed so a broken declaration +/// cannot suddenly expose its inbox as candidate specs. Named declaration files are recognized +/// only when they parse as an agent spec, which keeps ordinary project JSON/TOML/KDL from claiming +/// an unrelated `resources` directory. +fn is_declaration_parent(dir: &Path) -> bool { + let Ok(entries) = fs::read_dir(dir) else { + return false; + }; + entries.flatten().any(|entry| { + let path = entry.path(); + if !entry.file_type().is_ok_and(|kind| kind.is_file()) + || !path + .extension() + .and_then(|extension| extension.to_str()) + .is_some_and(|extension| SPEC_EXTS.contains(&extension)) + { + return false; + } + if path.file_stem().and_then(|stem| stem.to_str()) == Some("agent") { + return true; + } + parse_raw_file(&path).is_ok_and(|raws| raws.iter().any(RawSpec::looks_like_spec)) + }) +} + +/// Recursively gather candidate spec files, skipping only explicit control/runtime namespaces and +/// anything that isn't one of [`SPEC_EXTS`]. `pty` session metadata includes JSON that can resemble +/// an agent spec; it is runner state, never catalog input. Unreadable directories are skipped, not +/// fatal. fn collect_spec_files(root: &Path, dir: &Path, acc: &mut Vec) { let entries = match fs::read_dir(dir) { Ok(e) => e, @@ -65,23 +131,14 @@ fn collect_spec_files(root: &Path, dir: &Path, acc: &mut Vec) { }; for entry in entries.flatten() { let path = entry.path(); - let name = entry.file_name(); - let name = name.to_string_lossy(); - if name.starts_with('.') { - continue; // skip hidden files and directories + if !is_catalog_path(root, &path) { + continue; } let ft = match entry.file_type() { Ok(ft) => ft, Err(_) => continue, }; if ft.is_dir() { - if path == root.join("pty") - || name == "resources" - || name == "archive" - || name == "inbox" - { - continue; - } collect_spec_files(root, &path, acc); } else if ft.is_file() && let Some(ext) = path.extension().and_then(|e| e.to_str()) @@ -103,6 +160,8 @@ fn collect_spec_files(root: &Path, dir: &Path, acc: &mut Vec) { pub struct Declared { /// `identity` as written in the file. `None` when the file relies on [`path_defaults`]. pub identity: Option, + /// `host` as written in the file. `None` when the file relies on [`path_defaults`]. + pub host: Option, /// `type` as written, before it is normalized to `JobType::Service`. `None` when unset. pub job_type: Option, } @@ -117,6 +176,7 @@ pub fn parse_declared(path: &Path) -> anyhow::Result> { .into_iter() .map(|raw| Declared { identity: raw.identity, + host: raw.host, job_type: raw.job_type, }) .collect()) @@ -156,7 +216,8 @@ fn load_specs(root: &Path, path: &Path) -> anyhow::Result<(Vec, Vec { + (Some(c), Some(p)) if c != p && !explicit_placement => { warnings.push(format!( "{}: identity mismatch — content '{c}' vs path '{p}'; using content", path.display() @@ -190,7 +252,7 @@ fn resolve_spec( }; let host = match (raw.host.clone(), path_host) { - (Some(c), Some(p)) if c != p => { + (Some(c), Some(p)) if c != p && !explicit_placement => { warnings.push(format!( "{}: host mismatch — content '{c}' vs path '{p}'; using content", path.display() diff --git a/crates/agent-spec/src/lib.rs b/crates/agent-spec/src/lib.rs index ccacb15e..0377dcc7 100644 --- a/crates/agent-spec/src/lib.rs +++ b/crates/agent-spec/src/lib.rs @@ -24,7 +24,9 @@ pub mod discovery; mod kdl_format; pub mod spec; -pub use discovery::{Declared, Discovered, SpecError, discover, parse_declared, path_defaults}; +pub use discovery::{ + Declared, Discovered, SpecError, discover, is_catalog_path, parse_declared, path_defaults, +}; pub use spec::{ AgentSpec, JobType, Resource, Restart, RestartMode, Task, TaskKind, parse_duration, }; diff --git a/crates/agent-spec/tests/discovery.rs b/crates/agent-spec/tests/discovery.rs index 874eabe9..c340b69f 100644 --- a/crates/agent-spec/tests/discovery.rs +++ b/crates/agent-spec/tests/discovery.rs @@ -496,10 +496,7 @@ fn empty_or_ambiguous_argv_is_rejected_in_every_task_shape() { argv "true""#, ), ("empty-explicit", r#"pty "agent" { argv }"#), - ( - "empty-program-explicit", - r#"pty "agent" { argv "" }"#, - ), + ("empty-program-explicit", r#"pty "agent" { argv "" }"#), ( "both-explicit", r#"pty "agent" { command "true"; argv "true" }"#, @@ -657,11 +654,10 @@ fn path_supplies_identity_and_host_when_content_omits_them() { } #[test] -fn content_wins_over_path_and_mismatch_warns() { +fn an_explicit_identity_with_an_implicit_host_keeps_the_path_warning() { let tmp = tempfile::tempdir().unwrap(); let spec = r#" identity = "real-name" -host = "real-host" [pty.agent] command = "exec claude 'boot'" "#; @@ -671,17 +667,63 @@ command = "exec claude 'boot'" assert!(found.errors.is_empty()); let s = &found.specs[0]; assert_eq!(s.identity, "real-name"); - assert_eq!(s.host.as_deref(), Some("real-host")); - assert_eq!(found.warnings.len(), 2); + assert_eq!(s.host.as_deref(), Some("wrong-host")); + assert_eq!(found.warnings.len(), 1); assert!( found .warnings .iter() .any(|w| w.contains("identity mismatch")) ); +} + +#[test] +fn an_explicit_host_with_an_implicit_identity_keeps_the_path_warning() { + let tmp = tempfile::tempdir().unwrap(); + let spec = r#" +host = "real-host" +[pty.agent] +command = "exec claude 'boot'" +"#; + write(tmp.path(), "agents/wrong-host/path-name/agent.toml", spec); + + let found = discover(tmp.path()); + assert!(found.errors.is_empty()); + let s = &found.specs[0]; + assert_eq!(s.identity, "path-name"); + assert_eq!(s.host.as_deref(), Some("real-host")); + assert_eq!(found.warnings.len(), 1); assert!(found.warnings.iter().any(|w| w.contains("host mismatch"))); } +#[test] +fn an_explicit_identity_and_host_are_path_independent() { + let tmp = tempfile::tempdir().unwrap(); + let rel = "teams/.managed/groups/archive/project/declaration/agent.kdl"; + write( + tmp.path(), + rel, + r#"agent "stable-agent" { host "stable-host"; command "exec codex" }"#, + ); + + let found = discover(tmp.path()); + assert!(found.errors.is_empty(), "errors: {:?}", found.errors); + assert!(found.warnings.is_empty(), "warnings: {:?}", found.warnings); + assert_eq!(found.specs.len(), 1); + let spec = &found.specs[0]; + assert_eq!(spec.identity, "stable-agent"); + assert_eq!(spec.host.as_deref(), Some("stable-host")); + assert_eq!( + spec.path.parent(), + Some( + tmp.path() + .join("teams/.managed/groups/archive/project/declaration") + .as_path() + ), + "the declaration parent remains the state/resource anchor" + ); +} + #[test] fn malformed_file_is_collected_as_error_and_does_not_halt_the_walk() { let tmp = tempfile::tempdir().unwrap(); @@ -822,29 +864,66 @@ fn nonexistent_root_yields_empty_not_error() { } #[test] -fn hidden_runner_state_and_resources_are_ignored() { +fn only_contextually_reserved_namespaces_are_ignored() { let tmp = tempfile::tempdir().unwrap(); write( tmp.path(), - "agents/hetz/a/agent.toml", - "identity=\"a\"\n[pty.agent]\ncommand=\"x\"\n", - ); - // dot-prefixed runner state (R03) + a resource message — neither is a spec. - write(tmp.path(), ".st2.hetz.lock", "12345"); - write( - tmp.path(), - "agents/hetz/a/resources/inbox/1784-abc.md", - "a message", - ); - // The canonical PTY_ROOT is `/pty`. Its session JSON contains command/cwd fields and - // must never be mistaken for a catalog agent declaration. - write( - tmp.path(), - "pty/hetz.a.json", - r#"{"name":"hetz.a","status":"running","command":"sh -c x","cwd":"/tmp"}"#, - ); + "agents/hetz/live/agent.kdl", + r#"agent "live" { host "hetz"; command "x" }"#, + ); + for (path, identity) in [ + (".managed/team/agent.kdl", "managed"), + (".retired/team/agent.kdl", "dot-retired"), + ("agents/archive/project/agent.kdl", "archive-project"), + ("agents/resources/project/agent.kdl", "resources-project"), + ("agents/inbox/project/agent.kdl", "inbox-project"), + ] { + write( + tmp.path(), + path, + &format!(r#"agent "{identity}" {{ host "h"; command "x" }}"#), + ); + } + + for path in [ + ".git/project/agent.kdl", + ".st2/project/agent.kdl", + "organizations/project/.git/nested/agent.kdl", + "organizations/project/.st2/nested/agent.kdl", + "pty/project/agent.kdl", + "agents/hetz/live/resources/project/agent.kdl", + "agents/hetz/live/archive/project/agent.kdl", + "agents/hetz/live/inbox/project/agent.kdl", + ] { + write( + tmp.path(), + path, + r#"agent "excluded" { host "h"; command "x" }"#, + ); + } let found = discover(tmp.path()); - assert_eq!(found.specs.len(), 1); - assert_eq!(found.specs[0].identity, "a"); + assert!(found.errors.is_empty(), "errors: {:?}", found.errors); + assert_eq!(found.specs.len(), 6, "specs: {:?}", found.specs); + for identity in [ + "live", + "managed", + "dot-retired", + "archive-project", + "resources-project", + "inbox-project", + ] { + assert!( + found.specs.iter().any(|spec| spec.identity == identity), + "{identity} must remain discoverable" + ); + } + assert!( + !find(&found.specs, "dot-retired").retired, + "a .retired folder has no lifecycle meaning" + ); + assert!( + found.specs.iter().all(|spec| spec.identity != "excluded"), + "reserved control/state namespaces must not become declarations" + ); } diff --git a/docs/vrs/requirements.md b/docs/vrs/requirements.md index 2470e616..c9ab5bae 100644 --- a/docs/vrs/requirements.md +++ b/docs/vrs/requirements.md @@ -33,8 +33,16 @@ accepted. - **R01 Agent-spec compliance:** st2 validates and implements every agent-spec capability it claims to support, and identifies unsupported capabilities. -- **R02 Canonical KDL:** Hand-authored KDL is the canonical declaration; any - generator is optional and its output is inspectable before reconciliation. +- **R02 Canonical KDL and declaration identity:** Hand-authored KDL is the + canonical declaration; any generator is optional and its output is + inspectable before reconciliation. Declarations are discovered recursively. + An explicit `identity` and `host` pair is authoritative independent of its + folder path, while either omitted field retains path-derived defaults and + mismatch diagnostics. The declaration's parent remains its state/resource + anchor. Dot-prefixed and other organizational folders have no implicit + lifecycle meaning; discovery excludes `.git` and `.st2` control directories + at any depth, the catalog-root `pty` runtime directory, and state namespaces + directly owned by a declaration. - **R03 Host-pinned placement:** Every runnable agent or task resolves to its declared host; host-local roots own reconciliation. diff --git a/docs/vrs/spec.md b/docs/vrs/spec.md index c44cd108..4309fe28 100644 --- a/docs/vrs/spec.md +++ b/docs/vrs/spec.md @@ -66,7 +66,14 @@ validate ──► materialize ──► host-local st2 scheduler/reconciler - **R01–R03:** Fleet validation separates structural errors from selected-host runtime facts. Materialization is inspectable and host reconciliation starts - only declarations pinned to the local host. + only declarations pinned to the local host. Discovery is recursive: an + explicit `identity` and `host` pair is authoritative independent of the + declaration's path, whose parent remains the state/resource anchor. When + either field is omitted, the path supplies defaults and mismatches remain + diagnostic. Dot-prefixed folders, including `.managed` and `.retired`, are + 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. - **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 @@ -160,7 +167,7 @@ Watchers are deny-by-default. The classifier/action contract is: | Event | Minimal action | | --- | --- | -| `agents/**/agent.kdl` create/modify/remove | validate, materialize, and converge that agent and derived tasks | +| declaration-space `**/agent.kdl` create/modify/remove | validate, materialize, and converge that agent and derived tasks | | referenced `_templates/**` mutation | converge dependent agents only | | inbox create/archive/remove | DING consumer only; supervisor no-op | | plan/resource/status mutation | specialized consumer only; supervisor no-op | diff --git a/src/validate.rs b/src/validate.rs index 90797191..070f5b0b 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -5,11 +5,12 @@ //! - **ERROR** — the agent will fail to run, or run and silently do the wrong thing (parse failure, //! no identity, unknown `type`, a task silently dropped, an unrendered service, a duplicate id, a //! relative path, or a missing **catalog-rooted** path — the renderer's own output). Exits non-zero. -//! - **WARN** — advisory; the run still works (identity/host path↔content mismatch — the spec says a -//! mismatch is a warning; a dangling supervisor — crash-dings just route nowhere; a missing -//! **external** path for an agent assigned to the selected validation host; an overlay `@import` -//! that does not resolve — a *render* concern, not st2 law, since a valid spec may carry no -//! persona). `--strict` promotes every WARN to a failure so a renderer's CI can demand spotless. +//! - **WARN** — advisory; the run still works (a partially explicit identity/host placement that +//! mismatches its path-derived default; a dangling supervisor — crash-dings just route nowhere; a +//! missing **external** path for an agent assigned to the selected validation host; an overlay +//! `@import` that does not resolve — a *render* concern, not st2 law, since a valid spec may carry +//! no persona). `--strict` promotes every WARN to a failure so a renderer's CI can demand +//! spotless. //! //! st2 stays render-agnostic: render-only fields (`harness`, `model`, `persona`, //! `permissions`, …) are never required — their absence is never an issue. @@ -151,9 +152,13 @@ fn validate_scoped(root: &Path, this_host: Option<&str>) -> Report { let mut files: Vec<&PathBuf> = d.specs.iter().map(|s| &s.path).collect(); files.sort(); files.dedup(); + let mut explicit_placements: HashSet<(PathBuf, String, String)> = HashSet::new(); for f in files { if let Ok(raws) = parse_declared(f) { for raw in raws { + if let (Some(identity), Some(host)) = (&raw.identity, &raw.host) { + explicit_placements.insert((f.clone(), identity.clone(), host.clone())); + } if let Some(t) = &raw.job_type && t != "service" { @@ -214,10 +219,15 @@ fn validate_scoped(root: &Path, this_host: Option<&str>) -> Report { )); } - // identity / host path↔content mismatch (content wins; advisory) — re-derived structurally. + // An explicit identity+host pair is authoritative regardless of folder names. When either + // field is omitted, path defaults remain part of placement and mismatches stay advisory. + let explicit_placement = s.host.as_ref().is_some_and(|host| { + explicit_placements.contains(&(s.path.clone(), s.identity.clone(), host.clone())) + }); let (path_id, path_host) = path_defaults(root, &s.path); if let Some(pid) = &path_id && pid != &s.identity + && !explicit_placement { issues.push(Issue::warn( "id-path-mismatch", @@ -231,6 +241,7 @@ fn validate_scoped(root: &Path, this_host: Option<&str>) -> Report { } if let (Some(h), Some(ph)) = (&s.host, &path_host) && h != ph + && !explicit_placement { issues.push(Issue::warn( "host-path-mismatch", diff --git a/src/watch.rs b/src/watch.rs index b07f8711..6f4a133d 100644 --- a/src/watch.rs +++ b/src/watch.rs @@ -50,6 +50,9 @@ pub(crate) fn watch_catalog_declarations( } fn is_declaration_path(root: &Path, path: &Path) -> bool { + if !agent_spec::is_catalog_path(root, path) { + return false; + } let rel = path.strip_prefix(root).unwrap_or(path); let mut components = rel.components(); if matches!( @@ -97,28 +100,60 @@ mod tests { } #[test] - fn declaration_filter_ignores_runtime_state() { - let root = Path::new("/catalog"); + fn declaration_filter_uses_catalog_scoped_namespace_semantics() { + let catalog = tempfile::tempdir().unwrap(); + let root = catalog.path(); + std::fs::create_dir_all(root.join("agents/h/live")).unwrap(); + std::fs::write( + root.join("agents/h/live/agent.kdl"), + r#"agent "live" { host "h"; command "x" }"#, + ) + .unwrap(); + + assert!(is_declaration_path( + root, + &root.join("teams/.managed/project/agent.kdl") + )); + assert!(is_declaration_path( + root, + &root.join("teams/.retired/project/agent.kdl") + )); assert!(is_declaration_path( root, - Path::new("/catalog/team/agent.kdl") + &root.join("agents/archive/project/agent.kdl") )); assert!(is_declaration_path( root, - Path::new("/catalog/_templates/base.kdl") + &root.join("agents/resources/project/agent.kdl") )); + assert!(is_declaration_path(root, &root.join("_templates/base.kdl"))); assert!(!is_declaration_path( root, - Path::new("/catalog/pty/session.json") + &root.join(".git/project/agent.kdl") )); assert!(!is_declaration_path( root, - Path::new("/catalog/bus/inbox/msg") + &root.join(".st2/project/agent.kdl") )); assert!(!is_declaration_path( root, - Path::new("/catalog/team/rendered.kdl") + &root.join("organizations/project/.git/nested/agent.kdl") )); + assert!(!is_declaration_path( + root, + &root.join("organizations/project/.st2/nested/agent.kdl") + )); + assert!(!is_declaration_path( + root, + &root.join("pty/project/agent.kdl") + )); + for reserved in ["resources", "archive", "inbox"] { + assert!(!is_declaration_path( + root, + &root.join(format!("agents/h/live/{reserved}/project/agent.kdl")) + )); + } + assert!(!is_declaration_path(root, &root.join("team/rendered.kdl"))); } #[cfg(target_os = "linux")] diff --git a/tests/validate.rs b/tests/validate.rs index 33bd4c55..6fcc4f0e 100644 --- a/tests/validate.rs +++ b/tests/validate.rs @@ -202,6 +202,18 @@ fn a_duplicate_bus_id_is_an_error() { assert!(has(&validate(c.path()), "dup-id", Severity::Error)); } +#[test] +fn explicit_identity_and_host_are_path_independent_but_still_unique() { + let c = catalog(&[( + "organization/.managed/archive/arbitrary/declaration/agent.kdl", + r#"agent "stable" { host "pinned"; command "x" }"#, + )]); + let r = validate(c.path()); + assert_eq!(r.errors(), 0, "unexpected errors: {:?}", r.issues); + assert_eq!(r.warnings(), 0, "unexpected warnings: {:?}", r.issues); + assert_eq!(r.agents, 1); +} + #[test] fn an_unrendered_service_is_not_runnable() { let c = catalog(&[( @@ -328,7 +340,7 @@ fn a_fully_qualified_supervisor_in_the_catalog_is_clean() { fn an_identity_folder_mismatch_is_a_warning() { let c = catalog(&[( "hetz/folder-name/agent.kdl", - r#"agent "content-name" { host "hetz"; type "service"; pty "agent" { command "x" } }"#, + r#"agent "content-name" { type "service"; pty "agent" { command "x" } }"#, )]); assert!(has(&validate(c.path()), "id-path-mismatch", Severity::Warn)); } @@ -337,7 +349,7 @@ fn an_identity_folder_mismatch_is_a_warning() { fn a_host_folder_mismatch_is_a_warning() { let c = catalog(&[( "folderhost/w/agent.kdl", - r#"agent "w" { host "confighost"; type "service"; pty "agent" { command "x" } }"#, + r#"agent { host "confighost"; type "service"; pty "agent" { command "x" } }"#, )]); assert!(has( &validate(c.path()),