vigil: fix slice-1 review findings

- dirge vigil list merges config + VigilStore entries with status,
  so store-only and paused/rested vigils are visible (was config-only)
- build_vigil_entry validates interval/reap intervals > 0 and rejects
  keyless or unknown key=value args instead of silently dropping them
- harbinger CLI entries default to Template mode (an empty Commands map
  was an invalid entry)
- Remove/Pause/Resume/Rest propagate store errors as non-zero exit
- add VigilStore::list_all; add tests for all of the above

Author: allen-munsch

src/extras/vigil_db.rs

@@ -151,13 +151,26 @@ impl VigilStore {
     }
 
     pub fn list_non_resting(&self) -> Result<Vec<VigilRow>, String> {
+        self.query_rows(
+            "SELECT name, payload_json, status, created_at, updated_at
+             FROM vigils WHERE status != 'resting' ORDER BY name",
+        )
+    }
+
+    /// All rows, including resting vigils. `dirge vigil list` needs the
+    /// full picture so a vigil laid to rest still shows up with its status.
+    pub fn list_all(&self) -> Result<Vec<VigilRow>, String> {
+        self.query_rows(
+            "SELECT name, payload_json, status, created_at, updated_at
+             FROM vigils ORDER BY name",
+        )
+    }
+
+    fn query_rows(&self, sql: &str) -> Result<Vec<VigilRow>, String> {
         let conn = self.conn.lock().unwrap();
         let mut stmt = conn
-            .prepare(
-                "SELECT name, payload_json, status, created_at, updated_at
-                 FROM vigils WHERE status != 'resting' ORDER BY name",
-            )
-            .map_err(|e| format!("prepare list_non_resting: {e}"))?;
+            .prepare(sql)
+            .map_err(|e| format!("prepare vigil list: {e}"))?;
         let rows = stmt
             .query_map([], |row| {
                 Ok(VigilRow {
@@ -176,7 +189,7 @@ impl VigilStore {
                     updated_at: row.get(4)?,
                 })
             })
-            .map_err(|e| format!("list_non_resting: {e}"))?
+            .map_err(|e| format!("list vigils: {e}"))?
             .filter_map(|r| r.ok())
             .collect();
         Ok(rows)
@@ -236,6 +249,21 @@ mod tests {
         assert_eq!(names, vec!["a", "c"]);
     }
 
+    #[test]
+    fn list_all_includes_resting() {
+        let (store, _dir) = temp_db();
+        store.upsert("a", "1").unwrap();
+        store.upsert("b", "2").unwrap();
+        store.set_status("b", VigilStatus::Resting).unwrap();
+        let names: Vec<String> = store
+            .list_all()
+            .unwrap()
+            .into_iter()
+            .map(|r| r.name)
+            .collect();
+        assert_eq!(names, vec!["a", "b"]);
+    }
+
     #[test]
     fn remove_deletes_row() {
         let (store, _dir) = temp_db();

src/main.rs

@@ -2706,9 +2706,9 @@ async fn handle_vigil_command(action: &crate::cli::VigilAction) -> anyhow::Resul
 
     match action {
         crate::cli::VigilAction::List => {
-            let vigils = config::load().vigils.unwrap_or_default();
-            println!("Vigils (from config):");
-            for v in &vigils {
+            let vigils = collect_vigils_for_list(&paths, config::load().vigils.unwrap_or_default());
+            println!("Vigils:");
+            for (v, status) in &vigils {
                 let trigger = match &v.trigger {
                     crate::config::VigilTrigger::Toll { interval_secs } => {
                         format!("toll every {interval_secs}s")
@@ -2733,8 +2733,10 @@ async fn handle_vigil_command(action: &crate::cli::VigilAction) -> anyhow::Resul
                     v.prompt.clone()
                 };
                 println!(
-                    "  {} - {trigger} - reap every {}s - prompt: {prompt}",
-                    v.name, v.reap_interval_secs
+                    "  {} - {trigger} - reap every {}s - {} - prompt: {prompt}",
+                    v.name,
+                    v.reap_interval_secs,
+                    status.as_str()
                 );
             }
             if vigils.is_empty() {
@@ -2759,31 +2761,29 @@ async fn handle_vigil_command(action: &crate::cli::VigilAction) -> anyhow::Resul
         }
         crate::cli::VigilAction::Remove { name } => {
             let store = VigilStore::open(&paths).map_err(|e| anyhow::anyhow!("{e}"))?;
-            match store.remove(name) {
-                Ok(()) => println!("Removed vigil '{name}'."),
-                Err(e) => eprintln!("{e}"),
-            }
+            store.remove(name).map_err(|e| anyhow::anyhow!("{e}"))?;
+            println!("Removed vigil '{name}'.");
         }
         crate::cli::VigilAction::Pause { name } => {
             let store = VigilStore::open(&paths).map_err(|e| anyhow::anyhow!("{e}"))?;
-            match store.set_status(name, VigilStatus::Paused) {
-                Ok(()) => println!("Paused vigil '{name}'."),
-                Err(e) => eprintln!("{e}"),
-            }
+            store
+                .set_status(name, VigilStatus::Paused)
+                .map_err(|e| anyhow::anyhow!("{e}"))?;
+            println!("Paused vigil '{name}'.");
         }
         crate::cli::VigilAction::Resume { name } => {
             let store = VigilStore::open(&paths).map_err(|e| anyhow::anyhow!("{e}"))?;
-            match store.set_status(name, VigilStatus::Active) {
-                Ok(()) => println!("Resumed vigil '{name}'."),
-                Err(e) => eprintln!("{e}"),
-            }
+            store
+                .set_status(name, VigilStatus::Active)
+                .map_err(|e| anyhow::anyhow!("{e}"))?;
+            println!("Resumed vigil '{name}'.");
         }
         crate::cli::VigilAction::Rest { name } => {
             let store = VigilStore::open(&paths).map_err(|e| anyhow::anyhow!("{e}"))?;
-            match store.set_status(name, VigilStatus::Resting) {
-                Ok(()) => println!("vigil '{name}' resting (will sleep until next trigger)."),
-                Err(e) => eprintln!("{e}"),
-            }
+            store
+                .set_status(name, VigilStatus::Resting)
+                .map_err(|e| anyhow::anyhow!("{e}"))?;
+            println!("vigil '{name}' resting (will sleep until next trigger).");
         }
     }
     Ok(())
@@ -2796,23 +2796,38 @@ fn build_vigil_entry(
     trigger: &crate::cli::VigilAddTrigger,
     args: &[String],
 ) -> anyhow::Result<crate::config::VigilEntry> {
-    use crate::config::{VigilEntry, VigilRite, VigilTrigger};
-
-    let parsed: std::collections::HashMap<String, String> = args
-        .iter()
-        .filter_map(|a| a.split_once('='))
-        .map(|(k, v)| (k.to_string(), v.to_string()))
-        .collect();
+    use crate::config::{SocketMode, VigilEntry, VigilRite, VigilTrigger};
+
+    // Parse key=value args strictly: a keyless arg is a typo, not a value.
+    let mut parsed: std::collections::HashMap<String, String> = std::collections::HashMap::new();
+    for arg in args {
+        let (key, value) = arg
+            .split_once('=')
+            .ok_or_else(|| anyhow::anyhow!("invalid vigil arg '{arg}': expected key=value"))?;
+        parsed.insert(key.to_string(), value.to_string());
+    }
+
+    let known: &[&str] = match trigger {
+        crate::cli::VigilAddTrigger::Toll => &["interval_secs", "reap_interval_secs", "prompt"],
+        crate::cli::VigilAddTrigger::Watcher => &["path", "reap_interval_secs", "prompt"],
+        crate::cli::VigilAddTrigger::Harbinger => &[
+            "address",
+            "protocol",
+            "socket_mode",
+            "reap_interval_secs",
+            "prompt",
+        ],
+    };
+    for key in parsed.keys() {
+        if !known.contains(&key.as_str()) {
+            anyhow::bail!("unknown vigil arg '{key}' for {trigger:?}");
+        }
+    }
 
     let trigger = match trigger {
         crate::cli::VigilAddTrigger::Toll => {
-            let secs = parsed
-                .get("interval_secs")
-                .and_then(|v| v.parse().ok())
-                .unwrap_or(30);
-            VigilTrigger::Toll {
-                interval_secs: secs,
-            }
+            let interval_secs = parse_positive_secs(&parsed, "interval_secs", 30)?;
+            VigilTrigger::Toll { interval_secs }
         }
         crate::cli::VigilAddTrigger::Watcher => {
             let path = parsed
@@ -2827,19 +2842,28 @@ fn build_vigil_entry(
                 .cloned()
                 .unwrap_or_else(|| "127.0.0.1:9000".to_string());
             let protocol = parsed.get("protocol").cloned().unwrap_or_default();
+            // The flat key=value CLI cannot express a commands map, so a
+            // CLI-added harbinger is template mode. Commands-mode harbingers
+            // must come from config or a .dirge/vigils/*.json file.
+            let socket_mode = match parsed.get("socket_mode").map(String::as_str) {
+                None | Some("template") => SocketMode::Template,
+                Some("commands") => anyhow::bail!(
+                    "commands-mode harbingers need a commands map; define '{name}' in config or a vigil JSON file instead"
+                ),
+                Some(other) => {
+                    anyhow::bail!("invalid socket_mode '{other}': expected template or commands")
+                }
+            };
             VigilTrigger::Harbinger {
                 address,
                 protocol,
-                socket_mode: crate::config::SocketMode::Commands,
+                socket_mode,
                 commands: std::collections::HashMap::new(),
             }
         }
     };
 
-    let reap_interval_secs = parsed
-        .get("reap_interval_secs")
-        .and_then(|v| v.parse().ok())
-        .unwrap_or(30);
+    let reap_interval_secs = parse_positive_secs(&parsed, "reap_interval_secs", 30)?;
 
     let prompt = parsed.get("prompt").cloned().unwrap_or_default();
 
@@ -2855,3 +2879,75 @@ fn build_vigil_entry(
         }),
     })
 }
+
+/// Parse a positive-integer seconds arg, rejecting zero and non-numeric
+/// values. Zero trips tokio's non-zero interval assert (the trigger dies
+/// silently) and a zero reap interval tight-loops the reaper.
+#[cfg(feature = "vigil")]
+fn parse_positive_secs(
+    parsed: &std::collections::HashMap<String, String>,
+    key: &str,
+    default: u64,
+) -> anyhow::Result<u64> {
+    match parsed.get(key) {
+        None => Ok(default),
+        Some(raw) => {
+            let secs: u64 = raw
+                .parse()
+                .map_err(|_| anyhow::anyhow!("{key} must be a positive integer, got '{raw}'"))?;
+            if secs == 0 {
+                anyhow::bail!("{key} must be greater than zero");
+            }
+            Ok(secs)
+        }
+    }
+}
+
+/// Enumerate vigils for `dirge vigil list`, merged from config and the SQLite
+/// store by name. Config entries win on name collision; the store is
+/// authoritative for status, so a config vigil paused or rested via the CLI
+/// still shows its state. Entries only in the store (added via
+/// `dirge vigil add`) show up too.
+#[cfg(feature = "vigil")]
+fn collect_vigils_for_list(
+    paths: &crate::extras::dirge_paths::ProjectPaths,
+    config_vigils: Vec<crate::config::VigilEntry>,
+) -> Vec<(
+    crate::config::VigilEntry,
+    crate::extras::vigil_db::VigilStatus,
+)> {
+    use crate::config::VigilEntry;
+    use crate::extras::vigil_db::{VigilStatus, VigilStore};
+    use std::collections::HashMap;
+
+    let mut status_by_name: HashMap<String, VigilStatus> = HashMap::new();
+    let mut entry_by_name: HashMap<String, VigilEntry> = HashMap::new();
+
+    // Store first (lowest entry precedence; authoritative for status).
+    if let Ok(store) = VigilStore::open(paths) {
+        for row in store.list_all().unwrap_or_default() {
+            status_by_name.insert(row.name.clone(), row.status);
+            if let Ok(entry) = serde_json::from_str::<VigilEntry>(&row.payload_json) {
+                entry_by_name.insert(row.name.clone(), entry);
+            }
+        }
+    }
+
+    // Config wins over store on name collision.
+    for entry in config_vigils {
+        entry_by_name.insert(entry.name.clone(), entry);
+    }
+
+    let mut out: Vec<(VigilEntry, VigilStatus)> = entry_by_name
+        .into_iter()
+        .map(|(name, entry)| {
+            let status = status_by_name
+                .get(&name)
+                .copied()
+                .unwrap_or(VigilStatus::Active);
+            (entry, status)
+        })
+        .collect();
+    out.sort_by(|a, b| a.0.name.cmp(&b.0.name));
+    out
+}