From ce4951589285e7e5d2dd4ec9e2522368e72bf4ef Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 14 Aug 2026 20:22:53 -0500 Subject: [PATCH 1/3] fix(store): reject unreadable stored TLS modes on account read tls_mode_from_db coerced any unrecognized imap_tls_mode/smtp_tls_mode token to ImplicitTls on the path every ordinary account read takes, so a corrupted or tampered row silently handed callers a transport the operator never configured -- a STARTTLS account read back as implicit TLS, with no error and no log. Parse now fails closed with DatabaseError::Corrupted naming the account, the column, and the offending value. list_accounts already returned a Result, so no caller signature changes and no caller panics; get_account inherits the behaviour through it. Refs #271 --- crates/nuncio-store/src/db.rs | 122 ++++++++++++++++++++++++++++++---- 1 file changed, 108 insertions(+), 14 deletions(-) diff --git a/crates/nuncio-store/src/db.rs b/crates/nuncio-store/src/db.rs index 12ef57e..ca96d34 100644 --- a/crates/nuncio-store/src/db.rs +++ b/crates/nuncio-store/src/db.rs @@ -413,13 +413,36 @@ fn tls_mode_to_db(mode: nuncio_core::TlsMode) -> &'static str { } /// Parse a persisted `accounts` TLS-mode column back into -/// [`nuncio_core::TlsMode`]. An unrecognized value falls back to the safe -/// default (implicit TLS) rather than failing the whole account load. -fn tls_mode_from_db(value: &str) -> nuncio_core::TlsMode { +/// [`nuncio_core::TlsMode`]. +/// +/// Fails closed. TLS modes are validated before they are written, so the only +/// way an unrecognized token reaches this function is corruption or tampering +/// of the row. Coercing such a value to a default would silently hand the +/// caller a transport the operator never configured -- reading `"start_tls"` +/// as implicit TLS, or an unreadable value as *any* concrete mode, changes the +/// account's security posture with no signal at all. The account's real mode is +/// unknown at that point, and an unknown security posture is not something this +/// layer may guess at, so the read is refused and the offending account, column, +/// and raw value are named in the error. +/// +/// This is deliberately stricter than the best-effort recovery path in +/// [`crate::recovery`]: salvage exists to rescue what it can from an already +/// broken file and has no caller to return an error to, whereas this is the +/// path every ordinary account read takes. +fn tls_mode_from_db( + account_id: &str, + column: &str, + value: &str, +) -> Result { match value { - "start_tls" => nuncio_core::TlsMode::StartTls, - "plain" => nuncio_core::TlsMode::Plain, - _ => nuncio_core::TlsMode::ImplicitTls, + "implicit_tls" => Ok(nuncio_core::TlsMode::ImplicitTls), + "start_tls" => Ok(nuncio_core::TlsMode::StartTls), + "plain" => Ok(nuncio_core::TlsMode::Plain), + other => Err(DatabaseError::Corrupted(format!( + "account {account_id} has an unrecognized {column} value {other:?}; refusing to \ + load it under a guessed TLS mode -- re-save the account with a valid transport \ + security setting" + ))), } } @@ -1369,8 +1392,7 @@ impl DatabaseEngine { .await .map_err(DatabaseError::Query)?; - Ok(rows - .into_iter() + rows.into_iter() .map( |( id, @@ -1387,7 +1409,8 @@ impl DatabaseEngine { smtp_tls_mode, collection_url, filters_enabled, - )| { + )| + -> Result { let protocol = serde_json::from_str(&protocol_str) .unwrap_or(nuncio_core::AccountProtocol::ImapSmtp); // Reconstruct the exact transport variant from the @@ -1410,10 +1433,18 @@ impl DatabaseEngine { nuncio_core::Transport::ImapSmtp(nuncio_core::ImapSmtpTransport { imap_host: server_host, imap_port: server_port as u16, - imap_tls_mode: tls_mode_from_db(&imap_tls_mode), + imap_tls_mode: tls_mode_from_db( + &id, + "imap_tls_mode", + &imap_tls_mode, + )?, smtp_host: resolved_smtp_host, smtp_port: resolved_smtp_port, - smtp_tls_mode: tls_mode_from_db(&smtp_tls_mode), + smtp_tls_mode: tls_mode_from_db( + &id, + "smtp_tls_mode", + &smtp_tls_mode, + )?, }) } nuncio_core::AccountProtocol::Jmap => { @@ -1427,7 +1458,7 @@ impl DatabaseEngine { }) } }; - nuncio_core::AccountConfig { + Ok(nuncio_core::AccountConfig { id, name, email_address, @@ -1435,10 +1466,10 @@ impl DatabaseEngine { sync_interval_secs: sync_interval_secs as u64, filters_enabled: filters_enabled != 0, transport, - } + }) }, ) - .collect()) + .collect() } /// Fetch the persisted inbound-sync checkpoint for a folder, or `None` if @@ -4289,6 +4320,69 @@ mod tests { assert_eq!(fetched, acct); } + /// An unreadable stored TLS mode must fail the read rather than resolve to + /// a guessed one. The account below is configured for STARTTLS; its stored + /// mode is then overwritten out-of-band with a token this build does not + /// know. Previously that token was coerced to implicit TLS, so every + /// ordinary account read handed the caller a transport the operator never + /// chose, with nothing anywhere to say so. Both the IMAP and the SMTP + /// column are checked, and `get_account` is checked alongside + /// `list_accounts` because it reads through the same path. + #[tokio::test] + async fn unrecognized_stored_tls_mode_fails_the_read_instead_of_defaulting() { + for column in ["imap_tls_mode", "smtp_tls_mode"] { + let (engine, _dir) = DatabaseEngine::connect_ephemeral().await.unwrap(); + + let acct = nuncio_core::AccountConfig { + id: "acct-tls-corrupt".to_string(), + name: "Tampered TLS Account".to_string(), + email_address: "tampered@nuncio.mx".to_string(), + keyring_secret_key: "nuncio/acct-tls-corrupt".to_string(), + sync_interval_secs: 60, + filters_enabled: false, + transport: nuncio_core::Transport::ImapSmtp(nuncio_core::ImapSmtpTransport { + imap_host: "imap.nuncio.mx".to_string(), + imap_port: 143, + imap_tls_mode: nuncio_core::TlsMode::StartTls, + smtp_host: "smtp.nuncio.mx".to_string(), + smtp_port: 587, + smtp_tls_mode: nuncio_core::TlsMode::StartTls, + }), + }; + engine.save_account(&acct).await.expect("save succeeds"); + + // Simulates corruption or tampering of the row: writes are + // validated, so this value could never arrive through the API. + sqlx::query(&format!("UPDATE accounts SET {column} = ? WHERE id = ?")) + .bind("tls") + .bind("acct-tls-corrupt") + .execute(&engine.pool) + .await + .expect("corrupting update succeeds"); + + let err = engine + .list_accounts() + .await + .expect_err("an unreadable TLS mode must not resolve to a default"); + let message = err.to_string(); + assert!( + matches!(err, DatabaseError::Corrupted(_)), + "expected a corruption error, got: {message}" + ); + for expected in ["acct-tls-corrupt", column, "tls"] { + assert!( + message.contains(expected), + "error must name {expected}, got: {message}" + ); + } + + engine + .get_account("acct-tls-corrupt") + .await + .expect_err("get_account must fail closed on the same row"); + } + } + /// A CalDAV account's `collection_url` must survive save/reload, and a /// mail account's empty `collection_url` must round-trip as empty (not /// NULL-mangled), proving the backfill-safe column persists both. -- 2.54.0 From 98b59b8a981e6cad744f342acc82b49cf520ffc2 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 14 Aug 2026 20:22:58 -0500 Subject: [PATCH 2/3] fix(filter): size counts all body parts and attachments The NSQL size field was computed from body_plain alone, so an HTML-only message measured zero bytes and a short note carrying a large attachment measured only the note. Every size comparison then answered confidently and wrongly on exactly the mail such rules are written to catch. size is now the total byte size of the content the store holds: both body parts plus every attachment payload. Both parts are summed because a multipart/alternative message genuinely carries both on the wire. Headers and MIME framing are excluded and documented as excluded -- the raw RFC822 octets are not retained and no column carries their length, so the figure is a consistent lower bound rather than a guess. Refs #256 --- crates/nuncio-filter/src/ast.rs | 3 +- crates/nuncio-filter/src/engine.rs | 128 +++++++++++++++++++++++++++-- 2 files changed, 125 insertions(+), 6 deletions(-) diff --git a/crates/nuncio-filter/src/ast.rs b/crates/nuncio-filter/src/ast.rs index 2719e59..a184982 100644 --- a/crates/nuncio-filter/src/ast.rs +++ b/crates/nuncio-filter/src/ast.rs @@ -17,7 +17,8 @@ pub enum FilterField { Header(String), /// Attachment presence indicator (`true`/`false`). HasAttachment, - /// Message size in bytes. + /// Message size in bytes: all stored body parts plus all attachment + /// payloads. Excludes headers and MIME framing, which are not retained. Size, /// Current folder location (e.g. `INBOX`). Folder, diff --git a/crates/nuncio-filter/src/engine.rs b/crates/nuncio-filter/src/engine.rs index 9139b6c..34d861b 100644 --- a/crates/nuncio-filter/src/engine.rs +++ b/crates/nuncio-filter/src/engine.rs @@ -97,6 +97,42 @@ impl CompiledFilter { } } + /// Total byte size of the message content Nuncio holds: every body part + /// plus every attachment payload. + /// + /// This is what `WHERE size > N` compares against. Counting only the + /// plaintext body would report `0` for the very common HTML-only message + /// and would ignore attachments entirely, so a size rule would quietly + /// return the wrong verdict on exactly the mail it is usually written to + /// catch (the big ones). Both body parts are summed rather than one being + /// preferred: a `multipart/alternative` message genuinely carries the + /// plaintext *and* the HTML on the wire, so summing them tracks the real + /// message rather than one arbitrary view of it. + /// + /// The figure is the size of the decoded content as stored, not the exact + /// RFC822 octet count of the original transmission -- headers, MIME + /// boundaries, and transfer encoding are not counted, because the raw + /// octets are not retained and no stored column carries their length. It is + /// therefore a consistent lower bound on the wire size, never a guess: for + /// base64-encoded attachments the transmitted form is roughly a third + /// larger again. Rules are written against what the store actually has. + fn message_size_bytes(email: &Email) -> i64 { + let mut total: u64 = 0; + for part in [email.body_plain.as_deref(), email.body_html.as_deref()] + .into_iter() + .flatten() + { + total = total.saturating_add(part.len() as u64); + } + for attachment in &email.attachments { + total = total.saturating_add(attachment.content.len() as u64); + } + // Saturate rather than wrap: a size past `i64::MAX` is not a real + // message, and a wrapped negative would make `size > N` answer the + // opposite of the truth. + i64::try_from(total).unwrap_or(i64::MAX) + } + fn eval_leaf( leaf: &ConditionLeaf, placed: PlacedEmail<'_>, @@ -146,11 +182,7 @@ impl CompiledFilter { } } FilterField::Size => { - let size = email - .body_plain - .as_ref() - .map(|b| b.len() as i64) - .unwrap_or(0); + let size = Self::message_size_bytes(email); if let FilterValue::Number(target) = leaf.value { match leaf.operator { FilterOperator::Equals => size == target, @@ -526,6 +558,92 @@ mod tests { assert_eq!(preview.matched_rule_name, Some("Urgent".to_string())); } + /// Evaluate one NSQL rule against one message, returning whether it fired. + fn size_rule_matches(nsql: &str, email: &Email) -> bool { + let rule = crate::parser::NsqlParser::parse_rule("Size Rule", 1, nsql).unwrap(); + let engine = FilterEngine::new(vec![rule]).unwrap(); + let (_, placement) = test_placed_email(&email.account_id, "", "inbox"); + !engine + .evaluate(PlacedEmail { + email, + placement: &placement, + }) + .is_empty() + } + + /// `size` must reflect every body part and every attachment, not the + /// plaintext body alone. An HTML-only message has no plaintext body at + /// all, so it used to measure zero bytes and silently fell on the wrong + /// side of every size comparison -- `size > 100` said no for a 400-byte + /// message, and `size < 100` said yes. + #[test] + fn size_counts_html_body_of_html_only_message() { + let (mut email, _) = test_placed_email("acct-1", "HTML only", "inbox"); + email.body_plain = None; + email.body_html = Some(format!("

{}

", "x".repeat(400))); + + assert!( + size_rule_matches("WHERE size > 100 ACTION MARK READ", &email), + "a 407-byte HTML-only message must satisfy size > 100" + ); + assert!( + !size_rule_matches("WHERE size < 100 ACTION MARK READ", &email), + "a 407-byte HTML-only message must not satisfy size < 100" + ); + } + + /// Attachment payloads are part of the message's size. A short note + /// carrying a large attachment is exactly the mail a size rule is written + /// to catch, and counting only the note's text missed it entirely. + #[test] + fn size_counts_attachment_payloads() { + let (mut email, _) = test_placed_email("acct-1", "Invoice", "inbox"); + email.body_plain = Some("See attached.".to_string()); + email.attachments = vec![nuncio_core::model::Attachment { + filename: "invoice.pdf".to_string(), + mime_type: "application/pdf".to_string(), + content: vec![0u8; 5_000].into(), + }]; + + assert!( + size_rule_matches("WHERE size > 4096 ACTION MARK READ", &email), + "a 13-byte note with a 5000-byte attachment must satisfy size > 4096" + ); + } + + /// A `multipart/alternative` message carries both renderings on the wire, + /// so both are counted rather than one being preferred -- this pins the + /// definition `size` is documented to have. + #[test] + fn size_sums_both_body_parts_and_attachments() { + let (mut email, _) = test_placed_email("acct-1", "Both parts", "inbox"); + email.body_plain = Some("a".repeat(100)); + email.body_html = Some("b".repeat(250)); + email.attachments = vec![nuncio_core::model::Attachment { + filename: "logo.png".to_string(), + mime_type: "image/png".to_string(), + content: vec![0u8; 650].into(), + }]; + + assert_eq!(CompiledFilter::message_size_bytes(&email), 1_000); + assert!(size_rule_matches( + "WHERE size = 1000 ACTION MARK READ", + &email + )); + } + + /// A message with neither body part nor attachments genuinely measures + /// zero, and must still compare as a number rather than being skipped. + #[test] + fn size_of_empty_message_is_zero() { + let (mut email, _) = test_placed_email("acct-1", "Empty", "inbox"); + email.body_plain = None; + email.body_html = None; + + assert_eq!(CompiledFilter::message_size_bytes(&email), 0); + assert!(size_rule_matches("WHERE size < 1 ACTION MARK READ", &email)); + } + #[test] fn test_webhook_signature() { let sig = -- 2.54.0 From 3508b5e10adf704c4a5efad1d47bba6b9d4cd408 Mon Sep 17 00:00:00 2001 From: James Maes Date: Fri, 14 Aug 2026 20:28:27 -0500 Subject: [PATCH 3/3] refactor(cli): drop the runner's dead ephemeral store The headless runner still built an ephemeral DatabaseEngine, an EventBus, and a TempDir that no command read -- a remnant of the CLI-holds-its-own-store design that predates the gRPC API. Nothing but two dead_code accessors touched them, so remove the fields, the accessors, and the now-unreachable RunnerError. With no store to open, construction is neither fallible nor async, so the constructors lose their Result and async, and 'ephemeral' no longer describes them: they are renamed connect/connect_with/connect_with_token_override. Doc comments that contrasted the daemon's persistent store against 'this runner's own ephemeral local db' now described a field that does not exist; they are rewritten to say the CLI keeps no store at all. --- crates/nuncio-cli/src/main.rs | 2 +- crates/nuncio-cli/src/runner.rs | 245 +++++------------- crates/nuncio-cli/tests/cli_system_test.rs | 14 +- crates/nunciod/tests/daemon_e2e_test.rs | 4 +- .../nunciod/tests/headless_cli_token_test.rs | 19 +- 5 files changed, 82 insertions(+), 202 deletions(-) diff --git a/crates/nuncio-cli/src/main.rs b/crates/nuncio-cli/src/main.rs index 15d87e7..6e6425d 100644 --- a/crates/nuncio-cli/src/main.rs +++ b/crates/nuncio-cli/src/main.rs @@ -26,7 +26,7 @@ async fn main() -> Result<(), Box> { *password = PasswordArg(entered); } - let runner = HeadlessRunner::ephemeral().await?; + let runner = HeadlessRunner::connect(); let output_str = runner.execute_command(&cli.command, cli.json).await; println!("{}", output_str); diff --git a/crates/nuncio-cli/src/runner.rs b/crates/nuncio-cli/src/runner.rs index edaabf7..cb21ad3 100644 --- a/crates/nuncio-cli/src/runner.rs +++ b/crates/nuncio-cli/src/runner.rs @@ -1,14 +1,11 @@ //! Headless engine runner executing CLI commands against core services. -use nuncio_core::EventBus; use nuncio_store::vault::{ get_or_create_key_bytes_bounded, BoundedVaultError, SecretManager, GRPC_TOKEN_ACCOUNT, KEYRING_READ_TIMEOUT, }; -use nuncio_store::{DatabaseEngine, DatabaseError}; use serde_json::json; use std::sync::Arc; -use thiserror::Error; use crate::args::{ AccountSubcommand, AuditSubcommand, CalSubcommand, Commands, ContactSubcommand, @@ -336,107 +333,78 @@ fn filter_execution_log_proto_to_json( }) } -/// Errors emitted by the CLI headless runner. -#[derive(Error, Debug)] -pub enum RunnerError { - /// Engine initialization failure. - #[error("failed to initialize engine: {0}")] - InitFailed(String), - /// Database operation error. - #[error("database failure: {0}")] - Database(#[from] DatabaseError), -} - /// Headless core runner executing CLI commands non-interactively. /// -/// Most commands operate against an ephemeral local engine (database + -/// event bus) for now. `system status` (see [`Self::handle_system_status`]) -/// and `account add` / `account list` (see [`Self::handle_add_account`] / -/// [`Self::handle_accounts_list`]) are the exceptions: they are thin gRPC -/// clients of the real `nunciod` daemon's `nuncio.v1.System` and -/// `nuncio.v1.Accounts` APIs, authenticated by a bearer token read from an -/// injected [`SecretManager`] — production code uses -/// [`SecretManager::production`] (the real OS keyring), while tests inject -/// [`SecretManager::mock`] so no test ever touches the real vault. Setting -/// [`GRPC_TOKEN_ENV`] overrides that lookup entirely, which is how the CLI is -/// driven from a non-interactive session. +/// Every command is a thin gRPC client of the real `nunciod` daemon: the CLI +/// holds no database, no event bus, and no business logic of its own, because +/// the daemon owns all state. The single exception is `update check` / `update +/// apply` (see [`Self::handle_update_check`]), which is deliberately +/// client-local — the daemon it would have to ask may be the very binary being +/// replaced. /// -/// `account add` persists through the daemon so the account (and its -/// password, stored ONLY in the daemon's OS keyring vault) survives past -/// this CLI process exiting -- unlike this runner's own ephemeral local -/// `db`, which is thrown away when the process exits. +/// Calls are authenticated by a bearer token read from an injected +/// [`SecretManager`] — production code uses [`SecretManager::production`] (the +/// real OS keyring), while tests inject [`SecretManager::mock`] so no test ever +/// touches the real vault. Setting [`GRPC_TOKEN_ENV`] overrides that lookup +/// entirely, which is how the CLI is driven from a non-interactive session. +/// +/// Because nothing is persisted locally, state a command creates (an account +/// and its password, stored ONLY in the daemon's OS keyring vault) survives +/// this CLI process exiting. pub struct HeadlessRunner { - event_bus: EventBus, - db: DatabaseEngine, secrets: Arc, grpc_addr: String, // When present, the bearer token to send instead of consulting `secrets`. // Resolved once at construction so a single CLI invocation cannot change // which credential it authenticates with halfway through. token_override: Option, - // Kept only to hold the ephemeral database's backing directory open for - // the runner's lifetime: dropping it would unlink the directory out from - // under `db` while the pool may still need to open new connections. - _db_dir: tempfile::TempDir, } impl HeadlessRunner { - /// Initialize a new `HeadlessRunner` with an ephemeral database, the - /// real OS keyring vault ([`SecretManager::production`]), and the gRPC - /// daemon address resolved from [`nuncio_proto::grpc_addr_from_env`]. + /// Initialize a new `HeadlessRunner` against the real OS keyring vault + /// ([`SecretManager::production`]) and the gRPC daemon address resolved + /// from [`nuncio_proto::grpc_addr_from_env`]. + /// /// [`GRPC_TOKEN_ENV`], when set, supplies the bearer token instead of the /// keyring. - pub async fn ephemeral() -> Result { - Self::ephemeral_with_token_override( + pub fn connect() -> Self { + Self::connect_with_token_override( Arc::new(SecretManager::production()), nuncio_proto::grpc_addr_from_env(), grpc_token_from_env(), ) - .await } - /// Initialize a new `HeadlessRunner` with an ephemeral database and an - /// explicit secret vault + gRPC daemon address. + /// Initialize a new `HeadlessRunner` with an explicit secret vault + gRPC + /// daemon address. /// - /// This is the constructor tests MUST use whenever they exercise - /// `system status`: pass a [`SecretManager::mock`]-backed instance - /// (never the real OS keyring) and the address of a test-local gRPC - /// server. + /// This is the constructor tests MUST use: pass a + /// [`SecretManager::mock`]-backed instance (never the real OS keyring) and + /// the address of a test-local gRPC server. /// The ambient [`GRPC_TOKEN_ENV`] override is deliberately NOT consulted /// here, so a variable that happens to be exported in a developer's shell /// can never change what an injected-vault test authenticates with; tests /// that want the override pass it explicitly to - /// [`Self::ephemeral_with_token_override`]. - pub async fn ephemeral_with( - secrets: Arc, - grpc_addr: String, - ) -> Result { - Self::ephemeral_with_token_override(secrets, grpc_addr, None).await + /// [`Self::connect_with_token_override`]. + pub fn connect_with(secrets: Arc, grpc_addr: String) -> Self { + Self::connect_with_token_override(secrets, grpc_addr, None) } - /// Initialize a new `HeadlessRunner` with an ephemeral database, an - /// explicit secret vault + gRPC daemon address, and an explicit bearer - /// token override. + /// Initialize a new `HeadlessRunner` with an explicit secret vault + gRPC + /// daemon address and an explicit bearer token override. /// /// `token_override`, when `Some`, is used verbatim as the `authorization: /// Bearer` credential and the vault is never consulted for it. - pub async fn ephemeral_with_token_override( + pub fn connect_with_token_override( secrets: Arc, grpc_addr: String, token_override: Option, - ) -> Result { - let (db, db_dir) = DatabaseEngine::connect_ephemeral() - .await - .map_err(|e| RunnerError::InitFailed(e.to_string()))?; - let event_bus = EventBus::new(); - Ok(Self { - event_bus, - db, + ) -> Self { + Self { secrets, - _db_dir: db_dir, grpc_addr, token_override: normalize_token_override(token_override), - }) + } } /// Resolves the gRPC bearer token, preferring an explicit override over the @@ -476,18 +444,6 @@ impl HeadlessRunner { } } - /// Access the underlying `EventBus`. - #[allow(dead_code)] - pub fn event_bus(&self) -> &EventBus { - &self.event_bus - } - - /// Access the underlying `DatabaseEngine`. - #[allow(dead_code)] - pub fn db(&self) -> &DatabaseEngine { - &self.db - } - /// Execute a CLI subcommand, returning a formatted string output. pub async fn execute_command(&self, cmd: &Commands, json_mode: bool) -> String { match cmd { @@ -877,10 +833,8 @@ impl HeadlessRunner { /// `mail sync`: a real thin gRPC client of the running `nunciod` /// daemon's `nuncio.v1.Mail/Sync` API. - /// Replaces this command's previous local-ephemeral behavior (flipping - /// this runner's own throwaway `EventBus` status flag, which never - /// fetched a single real message) with a real inbound sync against the - /// daemon's persistent store -- the RPC awaits full completion before + /// Runs a real inbound sync against the daemon's persistent store -- + /// the RPC awaits full completion before /// returning, so a successful response's `synced_count` reflects /// messages that are already visible via `mail list`/`mail read`. async fn handle_sync(&self, json_mode: bool) -> String { @@ -911,8 +865,7 @@ impl HeadlessRunner { /// `mail list`: a real thin gRPC client of the running `nunciod` /// daemon's `nuncio.v1.Mail` API. Lists /// messages in `folder`, newest first, from the daemon's real, - /// persistent store -- NOT this runner's own ephemeral local `db`, which - /// is thrown away when this CLI process exits. + /// persistent store -- the CLI keeps no store of its own. async fn handle_list_folder(&self, folder: &str, json_mode: bool) -> String { let mut client = match self.connect_mail_client().await { Ok(client) => client, @@ -1633,8 +1586,7 @@ impl HeadlessRunner { /// `filter list`: a real thin gRPC client of the running `nunciod` /// daemon's `nuncio.v1.Filters` API. Lists /// every persisted filter rule from the daemon's real, persistent - /// store -- NOT this runner's own ephemeral local `db`, which is thrown - /// away when this CLI process exits. + /// store -- the CLI keeps no store of its own. async fn handle_filter_list(&self, json_mode: bool) -> String { let mut client = match self.connect_filters_client().await { Ok(client) => client, @@ -1687,9 +1639,8 @@ impl HeadlessRunner { /// `filter create`: a real thin gRPC client of the running `nunciod` /// daemon's `nuncio.v1.Filters` API. The daemon parses, validates /// (6-pass `NsqlValidator`), and persists the - /// rule, then reloads its own live `FilterEngine` -- this runner's own - /// ephemeral local `db` is never touched, so the rule survives this CLI - /// process exiting. + /// rule, then reloads its own live `FilterEngine`. The rule lands in the + /// daemon's persistent store, so it survives this CLI process exiting. async fn handle_filter_create( &self, name: &str, @@ -1880,8 +1831,7 @@ impl HeadlessRunner { /// `filter export`: a real thin gRPC client of the running `nunciod` /// daemon's `nuncio.v1.Filters` API, rendering every rule persisted in - /// the daemon's real, persistent store -- not this runner's own - /// ephemeral local `db`. + /// the daemon's real, persistent store -- the CLI keeps none of its own. async fn handle_filter_export(&self, format: &str, json_mode: bool) -> String { let format = match parse_rule_export_format(format) { Ok(f) => f, @@ -2102,9 +2052,9 @@ impl HeadlessRunner { /// The account configuration AND `password` are sent to the daemon in a /// single `AddAccount` RPC; the daemon is solely responsible for /// writing the password to the OS keyring vault and the config to its - /// persistent database -- this runner's own ephemeral local `db` is - /// never touched for this command, so the account survives this CLI - /// process exiting. `password` is never logged here, only forwarded. + /// persistent database. The CLI writes nothing locally, so the account + /// survives this CLI process exiting. `password` is never logged here, + /// only forwarded. #[allow(clippy::too_many_arguments)] async fn handle_add_account( &self, @@ -2319,8 +2269,7 @@ impl HeadlessRunner { /// Fetches every account from the daemon and returns the one matching /// `id`, or an error string. There is deliberately no single-account /// `GetAccount` RPC in the contract, so `account show`/`edit` filter the - /// `ListAccounts` result client-side -- always over the real daemon API, - /// never this runner's own ephemeral local `db`. + /// `ListAccounts` result client-side -- always over the real daemon API. async fn fetch_account_by_id( &self, id: &str, @@ -2340,8 +2289,7 @@ impl HeadlessRunner { /// `account show`: a real thin gRPC client of the daemon's /// `nuncio.v1.Accounts/ListAccounts` API, filtered client-side to a single - /// account. Reads ONLY the daemon's persistent store -- never this - /// runner's own ephemeral local `db`. + /// account. Reads ONLY the daemon's persistent store. async fn handle_account_show(&self, id: &str, json_mode: bool) -> String { let account = match self.fetch_account_by_id(id).await { Ok(account) => account, @@ -3143,13 +3091,11 @@ mod tests { .expect("mock vault mints a token"), ); - let runner = HeadlessRunner::ephemeral_with_token_override( + let runner = HeadlessRunner::connect_with_token_override( Arc::clone(&secrets), "127.0.0.1:1".to_string(), Some(" override-token ".to_string()), - ) - .await - .expect("runner init"); + ); let resolved = runner .resolve_grpc_token() @@ -3169,10 +3115,7 @@ mod tests { .expect("mock vault mints a token"), ); - let runner = - HeadlessRunner::ephemeral_with(Arc::clone(&secrets), "127.0.0.1:1".to_string()) - .await - .expect("runner init"); + let runner = HeadlessRunner::connect_with(Arc::clone(&secrets), "127.0.0.1:1".to_string()); assert_eq!( runner @@ -3189,10 +3132,7 @@ mod tests { #[tokio::test] async fn ephemeral_with_ignores_an_ambient_env_override() { let secrets = Arc::new(SecretManager::mock()); - let runner = - HeadlessRunner::ephemeral_with(Arc::clone(&secrets), "127.0.0.1:1".to_string()) - .await - .expect("runner init"); + let runner = HeadlessRunner::connect_with(Arc::clone(&secrets), "127.0.0.1:1".to_string()); assert_eq!(runner.token_override, None); } @@ -3260,9 +3200,7 @@ mod tests { #[tokio::test] async fn account_add_caldav_requires_collection_url_before_dialing() { let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), "127.0.0.1:1".into()) - .await - .expect("runner init"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), "127.0.0.1:1".into()); let out = runner .execute_command( @@ -3294,9 +3232,7 @@ mod tests { #[tokio::test] async fn account_add_rejects_invalid_tls_mode_before_dialing_the_daemon() { let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), "127.0.0.1:1".into()) - .await - .expect("runner init"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), "127.0.0.1:1".into()); let bad_imap_mode = runner .execute_command( @@ -3361,9 +3297,7 @@ mod tests { #[tokio::test] async fn system_status_reports_honest_error_when_daemon_unreachable() { let addr = reserve_unreachable_addr().await; - let runner = HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr) - .await - .expect("ephemeral runner initializes"); + let runner = HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr); let json_out = runner .execute_command( @@ -3459,9 +3393,7 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); let out = runner .execute_command( @@ -3607,9 +3539,7 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); let add_out = runner .execute_command( @@ -3677,8 +3607,8 @@ mod tests { assert!(list_out_text.contains("stub@nuncio.mx")); assert!(list_out_text.contains("imap.nuncio.mx")); - // `account show` reads the daemon's ListAccounts result (filtered - // client-side), not the runner's own ephemeral local db. + // `account show` reads the daemon's ListAccounts result, filtered + // client-side. let show_out = runner .execute_command( &Commands::Account { @@ -4004,15 +3934,12 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); // `mail sync`: proves the CLI is a real gRPC client of `Mail/Sync` // -- it sends a `SyncRequest` with no `account_id` (sync every // account) and reports the daemon's real - // `synced_count` in its output, rather than fabricating a status by - // only flipping this runner's own throwaway local `EventBus`. + // `synced_count` in its output, rather than fabricating a status locally. let mail_sync = runner .execute_command( &Commands::Mail { @@ -4323,9 +4250,7 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); let out = runner .execute_command( @@ -4476,9 +4401,7 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); // Text mode: human message, gRPC code, and the typed reason -- not a // raw status dump, and not silently dropping the code/reason. @@ -4617,9 +4540,7 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); // `cal list`: proves the CLI is a real gRPC client of // `Calendar/ListEvents` -- the exact account/calendar/window @@ -4659,8 +4580,7 @@ mod tests { // `cal sync`: proves the CLI is a real gRPC client of // `Calendar/Sync` and reports the daemon's real `synced_count`, - // rather than fabricating a "calendar_sync_started" status by only - // flipping this runner's own throwaway local `EventBus`. + // rather than fabricating a "calendar_sync_started" status locally. let cal_sync = runner .execute_command( &Commands::Cal { @@ -4822,9 +4742,7 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); // `contact list`: proves the CLI is a real gRPC client of // `Contacts/ListContacts` -- the exact account supplied reaches the @@ -5172,9 +5090,7 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); // `filter create` happy path. let create_out = runner @@ -5417,9 +5333,7 @@ mod tests { #[tokio::test] async fn filter_commands_report_honest_error_when_daemon_unreachable() { let addr = reserve_unreachable_addr().await; - let runner = HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr) - .await - .expect("ephemeral runner initializes"); + let runner = HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr); let out = runner .execute_command( @@ -5487,9 +5401,7 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); let out = runner .execute_command( @@ -5556,9 +5468,7 @@ mod tests { // first, the error would be a transport failure instead of the // expected format-parsing error. let addr = reserve_unreachable_addr().await; - let runner = HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr) - .await - .expect("ephemeral runner initializes"); + let runner = HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr); let out = runner .execute_command( @@ -5640,9 +5550,7 @@ mod tests { }); let runner = - HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("ephemeral runner initializes"); + HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); let list_out = runner .execute_command( @@ -5687,9 +5595,7 @@ mod tests { #[tokio::test] async fn mail_export_and_system_audit_report_honest_error_when_daemon_unreachable() { let addr = reserve_unreachable_addr().await; - let runner = HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr) - .await - .expect("ephemeral runner initializes"); + let runner = HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr); let export_out = runner .execute_command( @@ -5733,13 +5639,4 @@ mod tests { assert!(audit_verify_out.contains(r#""status":"error""#)); assert!(audit_verify_out.contains("unreachable")); } - - #[test] - fn runner_error_display() { - let err = RunnerError::InitFailed("failed to open database".to_string()); - assert_eq!( - err.to_string(), - "failed to initialize engine: failed to open database" - ); - } } diff --git a/crates/nuncio-cli/tests/cli_system_test.rs b/crates/nuncio-cli/tests/cli_system_test.rs index 3994ee3..1962c1f 100644 --- a/crates/nuncio-cli/tests/cli_system_test.rs +++ b/crates/nuncio-cli/tests/cli_system_test.rs @@ -12,7 +12,7 @@ use std::sync::Arc; #[tokio::test] async fn system_test_cli_noun_verb_execution_matrix() { - let runner = HeadlessRunner::ephemeral().await.expect("runner init"); + let runner = HeadlessRunner::connect(); // 1. System status is a real gRPC client of the `nunciod` daemon, so it // is exercised separately below via `ephemeral_with` + `SecretManager::mock()` @@ -60,9 +60,7 @@ async fn system_status_reports_honest_error_when_daemon_unreachable() { let addr = listener.local_addr().expect("listener has local addr"); drop(listener); // free the port; nothing is listening on it now - let runner = HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("runner init"); + let runner = HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); let out: String = runner .execute_command( @@ -92,9 +90,7 @@ async fn account_add_and_list_report_honest_errors_when_daemon_unreachable() { let addr = listener.local_addr().expect("listener has local addr"); drop(listener); // free the port; nothing is listening on it now - let runner = HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("runner init"); + let runner = HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); let list_out: String = runner .execute_command( @@ -152,9 +148,7 @@ async fn mail_and_folder_report_honest_errors_when_daemon_unreachable() { let addr = listener.local_addr().expect("listener has local addr"); drop(listener); // free the port; nothing is listening on it now - let runner = HeadlessRunner::ephemeral_with(Arc::new(SecretManager::mock()), addr.to_string()) - .await - .expect("runner init"); + let runner = HeadlessRunner::connect_with(Arc::new(SecretManager::mock()), addr.to_string()); let assert_honest_error = |out: String| { let json: Value = serde_json::from_str(&out).expect("valid json"); diff --git a/crates/nunciod/tests/daemon_e2e_test.rs b/crates/nunciod/tests/daemon_e2e_test.rs index c038877..cdb7602 100644 --- a/crates/nunciod/tests/daemon_e2e_test.rs +++ b/crates/nunciod/tests/daemon_e2e_test.rs @@ -79,9 +79,7 @@ async fn cli_system_status_round_trips_over_grpc_to_live_daemon() { // client path against the live daemon, reading the token back from the // shared mock `SecretManager` (proving the CLI's injected-vault design // works end-to-end, not just against a hand-rolled token string). - let cli_runner = HeadlessRunner::ephemeral_with(secrets, addr.to_string()) - .await - .expect("cli headless runner initializes"); + let cli_runner = HeadlessRunner::connect_with(secrets, addr.to_string()); let cli_status = cli_runner .execute_command( diff --git a/crates/nunciod/tests/headless_cli_token_test.rs b/crates/nunciod/tests/headless_cli_token_test.rs index d1b3e6e..55e82da 100644 --- a/crates/nunciod/tests/headless_cli_token_test.rs +++ b/crates/nunciod/tests/headless_cli_token_test.rs @@ -100,13 +100,8 @@ async fn env_override_authenticates_without_consulting_the_vault() { ); assert_ne!(unrelated_token, token); - let runner = HeadlessRunner::ephemeral_with_token_override( - unrelated_vault, - addr.to_string(), - Some(token), - ) - .await - .expect("cli headless runner initializes"); + let runner = + HeadlessRunner::connect_with_token_override(unrelated_vault, addr.to_string(), Some(token)); let out = run_status(&runner).await; assert!( @@ -127,9 +122,7 @@ async fn keyring_path_is_unchanged_when_no_override_is_set() { ); let addr = boot_daemon(Arc::clone(&secrets), token).await; - let runner = HeadlessRunner::ephemeral_with(secrets, addr.to_string()) - .await - .expect("cli headless runner initializes"); + let runner = HeadlessRunner::connect_with(secrets, addr.to_string()); let out = run_status(&runner).await; assert!( @@ -211,13 +204,11 @@ async fn rejected_override_never_echoes_the_token_to_output_or_logs() { ); let addr = boot_daemon(Arc::clone(&daemon_secrets), token).await; - let runner = HeadlessRunner::ephemeral_with_token_override( + let runner = HeadlessRunner::connect_with_token_override( Arc::new(SecretManager::mock()), addr.to_string(), Some(SENTINEL_TOKEN.to_string()), - ) - .await - .expect("cli headless runner initializes"); + ); let out = run_status(&runner).await; assert!( -- 2.54.0