From 2ccf2de1b4045797dc5d4f8f8fc8b72d76d3c28d Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Thu, 30 Jul 2026 17:08:39 +0200 Subject: [PATCH] fix: fail closed when listing unknown catalog agents --- README.md | 4 +- src/main.rs | 9 ++- src/message.rs | 39 ++++++++++ tests/message_cli.rs | 167 ++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 214 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index d3f7abee..2c6758ec 100644 --- a/README.md +++ b/README.md @@ -278,7 +278,9 @@ unchanged and appends `[retired]` to a retired row. For a catalog-backed agent, every native bus operation resolves the same agent directory used by the roster: presence is `/status`, while unread messages, archive receipts, context, and links live under `/resources/`. The flat `/` layout remains only as the -intentional catalog-less fallback used by isolated folder evals. +intentional catalog-less fallback used by isolated folder evals. In a catalog-backed root, +`st2 message ls` rejects an absent identity; recovery inspection of a deliberately orphaned flat +box must be explicit with `st2 message ls --orphan` (and optionally `--archive`). Adopters should cut directly to the native layout. Before launching a migrated identity, install and verify hooks, validate and materialize its hand-authored declaration, stop any predecessor transport, diff --git a/src/main.rs b/src/main.rs index 35d887f0..41266b2e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -458,6 +458,9 @@ enum MessageCmd { /// List the archive instead of the inbox. #[arg(long)] archive: bool, + /// Recovery-only: list the raw flat `/` box without catalog resolution. + #[arg(long)] + orphan: bool, /// Print only the message count. #[arg(long)] count: bool, @@ -1326,6 +1329,7 @@ fn message_cmd(cmd: MessageCmd) -> Result<()> { MessageCmd::Ls { identity, archive, + orphan, count, include_body, from, @@ -1338,10 +1342,11 @@ fn message_cmd(cmd: MessageCmd) -> Result<()> { Some(id) => id, None => acting_id(&ctx)?, }; + let dir = message::resolve_list_box(&root, &id, &host, archive, orphan)?; let mut msgs = if archive { - message::list_dir(&message::resolve_archive(&root, &id, &host))? + message::list_dir(&dir)? } else { - message::list_inbox(&message::resolve_inbox(&root, &id, &host))? + message::list_inbox(&dir)? }; if let Some(sender) = &from { msgs.retain(|m| m.from.as_deref() == Some(sender.as_str())); diff --git a/src/message.rs b/src/message.rs index 892a6109..3cb497f3 100644 --- a/src/message.rs +++ b/src/message.rs @@ -335,6 +335,45 @@ pub fn resolve_archive(root: &Path, id: &str, host: &str) -> PathBuf { } } +/// Resolve one box for `message ls`. +/// +/// The permissive flat layout is automatic only when discovery proves that `root` is catalog-less. +/// Once any valid or malformed declaration makes it a catalog, an absent identity is an error. +/// `orphan` is the explicit recovery path for inspecting a raw flat box inside such a root. +pub fn resolve_list_box( + root: &Path, + id: &str, + host: &str, + archive: bool, + orphan: bool, +) -> anyhow::Result { + let flat = || { + root.join(id) + .join(if archive { "archive" } else { "inbox" }) + }; + if orphan { + return Ok(flat()); + } + let discovered = crate::discover(root); + if let Some(agent_dir) = discovered + .specs + .iter() + .find(|spec| spec.bus_id(host) == id || spec.identity == id) + .and_then(|spec| spec.path.parent()) + { + return Ok(if archive { + archive_dir(agent_dir) + } else { + inbox_dir(agent_dir) + }); + } + + if discovered.specs.is_empty() && discovered.errors.is_empty() { + return Ok(flat()); + } + anyhow::bail!("no agent '{id}' found in catalog {}", root.display()) +} + /// Resolve a recipient (a bus id `.` or a bare identity) to its agent folder in the /// catalog, via content discovery. Returns `None` if no agent matches. pub fn resolve_agent_dir(catalog_root: &Path, recipient: &str, this_host: &str) -> Option { diff --git a/tests/message_cli.rs b/tests/message_cli.rs index 59d4442c..c8a5969e 100644 --- a/tests/message_cli.rs +++ b/tests/message_cli.rs @@ -13,9 +13,21 @@ fn write_message(inbox: &Path, ts_ms: u64, suffix: &str, from: &str) { .unwrap(); } -fn list(root: &Path, extra: &[&str]) -> std::process::Output { +fn write_agent(root: &Path, identity: &str) { + let directory = root.join("h").join(identity); + fs::create_dir_all(&directory).unwrap(); + fs::write( + directory.join("agent.kdl"), + format!( + "agent \"{identity}\" {{\n identity \"{identity}\"\n host \"h\"\n type \"service\"\n pty \"agent\" {{ command \"x\" }}\n}}\n" + ), + ) + .unwrap(); +} + +fn list_identity(root: &Path, identity: &str, extra: &[&str]) -> std::process::Output { Command::new(env!("CARGO_BIN_EXE_st2")) - .args(["message", "ls", "bob", "--root"]) + .args(["message", "ls", identity, "--root"]) .arg(root) .args(["--host", "h"]) .args(extra) @@ -23,6 +35,10 @@ fn list(root: &Path, extra: &[&str]) -> std::process::Output { .unwrap() } +fn list(root: &Path, extra: &[&str]) -> std::process::Output { + list_identity(root, "bob", extra) +} + #[test] fn since_is_strict_and_composes_with_other_list_filters() { let tmp = tempfile::tempdir().unwrap(); @@ -103,3 +119,150 @@ fn archived_filename_wins_over_a_restored_raw_inbox_copy_in_all_list_modes() { ); assert_eq!(String::from_utf8_lossy(&out.stdout).trim(), "1"); } + +#[test] +fn an_unknown_catalog_identity_fails_before_every_output_mode_reads_a_box() { + let tmp = tempfile::tempdir().unwrap(); + write_agent(tmp.path(), "alice"); + + // A populated flat box must not turn an unknown catalog identity into a valid result. + write_message( + &tmp.path().join("missing/inbox"), + 1_700_000_000_000, + "aaaaaa", + "alice", + ); + write_message( + &tmp.path().join("missing/archive"), + 1_700_000_000_001, + "bbbbbb", + "alice", + ); + + for extra in [ + vec![], + vec!["--json"], + vec!["--count"], + vec!["--archive"], + vec!["--archive", "--json"], + vec!["--archive", "--count"], + ] { + let out = list_identity(tmp.path(), "missing", &extra); + assert!(!out.status.success(), "mode {extra:?} must fail"); + assert!( + out.stdout.is_empty(), + "mode {extra:?} must fail before rendering output" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!(stderr.contains("no agent 'missing' found in catalog")); + assert!(stderr.contains(&tmp.path().display().to_string())); + } +} + +#[test] +fn known_empty_native_and_catalog_less_flat_boxes_remain_valid() { + let catalog = tempfile::tempdir().unwrap(); + write_agent(catalog.path(), "alice"); + for extra in [ + vec![], + vec!["--json"], + vec!["--count"], + vec!["--archive", "--count"], + ] { + let out = list_identity(catalog.path(), "alice", &extra); + assert!( + out.status.success(), + "known native mode {extra:?}: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + + let flat = tempfile::tempdir().unwrap(); + write_message( + &flat.path().join("bob/inbox"), + 1_700_000_000_000, + "aaaaaa", + "alice", + ); + let out = list(flat.path(), &["--count"]); + assert!( + out.status.success(), + "{}", + String::from_utf8_lossy(&out.stderr) + ); + assert_eq!(String::from_utf8_lossy(&out.stdout).trim(), "1"); +} + +#[test] +fn orphan_mode_explicitly_reads_raw_flat_inbox_and_archive() { + let tmp = tempfile::tempdir().unwrap(); + write_agent(tmp.path(), "alice"); + write_message( + &tmp.path().join("missing/inbox"), + 1_700_000_000_000, + "aaaaaa", + "alice", + ); + write_message( + &tmp.path().join("missing/archive"), + 1_700_000_000_001, + "bbbbbb", + "alice", + ); + + let inbox = list_identity(tmp.path(), "missing", &["--orphan", "--json"]); + assert!( + inbox.status.success(), + "{}", + String::from_utf8_lossy(&inbox.stderr) + ); + assert_eq!( + serde_json::from_slice::(&inbox.stdout) + .unwrap() + .as_array() + .unwrap() + .len(), + 1 + ); + + let archive = list_identity(tmp.path(), "missing", &["--orphan", "--archive", "--count"]); + assert!( + archive.status.success(), + "{}", + String::from_utf8_lossy(&archive.stderr) + ); + assert_eq!(String::from_utf8_lossy(&archive.stdout).trim(), "1"); +} + +#[test] +fn malformed_catalog_declarations_disable_implicit_flat_fallback() { + let tmp = tempfile::tempdir().unwrap(); + let declaration = tmp.path().join("h/broken/agent.kdl"); + fs::create_dir_all(declaration.parent().unwrap()).unwrap(); + fs::write(&declaration, "agent \"broken\" { this is not valid").unwrap(); + write_message( + &tmp.path().join("missing/inbox"), + 1_700_000_000_000, + "aaaaaa", + "alice", + ); + + for extra in [ + vec![], + vec!["--json"], + vec!["--count"], + vec!["--archive"], + vec!["--archive", "--json"], + vec!["--archive", "--count"], + ] { + let out = list_identity(tmp.path(), "missing", &extra); + assert!(!out.status.success(), "mode {extra:?} must fail"); + assert!( + out.stdout.is_empty(), + "mode {extra:?} must fail before rendering output" + ); + assert!( + String::from_utf8_lossy(&out.stderr).contains("no agent 'missing' found in catalog") + ); + } +}