Tighten error handling across fennel and ci/lua
Each Fennel error now carries context at the right layer (Internal no
longer strips the underlying mlua message; TypeMismatch no longer
duplicates it; setup errors stop being mislabeled as Eval). Runtime
invariants that masqueraded as user-facing Lua errors now panic at the
bug site, and Cmd::Argv enforces non-empty argv at the type level
instead of indexing into an empty Vec.

Assisted-by: Claude Opus 4.7 (1M context) via Claude Code
change llvlqsztrzovymtxymnulvoulsuurrky
commit aafe613ddc98e718120a4ea8bca84815f9e16ae7
author Alpha Chen <alpha@kejadlen.dev>
date
parent yutsqqzu
diff --git a/src/ci/lua.rs b/src/ci/lua.rs
index 8a14ffc..20302ad 100644
--- a/src/ci/lua.rs
+++ b/src/ci/lua.rs
@@ -128,6 +128,13 @@ impl Runtime {
     ///
     /// Takes ownership of the pipeline (including its Lua VM). `meta`
     /// provides the push data for `:quire/push` source outputs.
+    ///
+    /// Panics if any of the Lua table operations below fail. They run
+    /// against a freshly initialized VM with `String`/`&str` keys and
+    /// values, so the only realistic failure mode is allocation
+    /// failure — abort is the right answer there. The matching
+    /// `RuntimeHandle::into_lua` call at the executor's call site uses
+    /// the same boundary.
     pub(super) fn new(
         pipeline: super::pipeline::Pipeline,
         secrets: HashMap<String, SecretString>,
@@ -186,7 +193,15 @@ impl Runtime {
     /// Mark `id` as the currently executing job. `(sh …)` invocations
     /// from this job's `run_fn` will record output under `id`, and
     /// `(jobs …)` lookups will resolve against `id`'s view.
+    ///
+    /// Panics if `id` has no inputs view — every job built by
+    /// `Runtime::new` gets one, so a missing view means the executor
+    /// is calling `enter_job` with an id that wasn't in the pipeline.
     pub(super) fn enter_job(&self, id: &str) {
+        assert!(
+            self.inputs.contains_key(id),
+            "enter_job called with unknown job id '{id}'"
+        );
         *self.current_job.borrow_mut() = Some(id.to_string());
     }
 
@@ -248,10 +263,13 @@ fn lookup_input(lua: &Lua, name: String) -> mlua::Result<mlua::Value> {
     let calling = calling
         .as_ref()
         .ok_or_else(|| mlua::Error::external("(jobs ...) called outside a job's run-fn"))?;
-    let view = rt.inputs.get(calling).ok_or_else(|| {
-        // cov-excl-line
-        mlua::Error::external(format!("no inputs view for calling job '{calling}'")) // cov-excl-line
-    })?; // cov-excl-line
+    // Runtime::new builds a view for every job and enter_job is the only
+    // setter for current_job, so a missing view is a programming error,
+    // not a user-reachable condition.
+    let view = rt
+        .inputs
+        .get(calling)
+        .unwrap_or_else(|| unreachable!("no inputs view for calling job '{calling}'"));
     match view.get(&name) {
         Some(Some(value)) => Ok(value.clone()),
         Some(None) => Ok(mlua::Value::Nil),
@@ -267,6 +285,16 @@ fn lookup_input(lua: &Lua, name: String) -> mlua::Result<mlua::Value> {
 /// Body of `(secret name)`. Errors as a Lua error if the runtime
 /// isn't installed, the name is undeclared, or the file form fails to
 /// read.
+//
+// Errors here cross the mlua boundary via `Error::external`, which
+// erases them to `Box<dyn Error + Send + Sync>`. The `std::error::Error`
+// source chain is preserved, but miette `Diagnostic` metadata
+// (codes, labels, source spans) does not survive the round trip —
+// the resulting `mlua::Error` becomes the `#[source]` of
+// `Error::JobFailed` at the executor, which only renders the chain
+// as plain `Display`. Don't reach for richer error types here
+// expecting them to render: rephrase the Display string to carry
+// what the user needs to see.
 fn lookup_secret(lua: &Lua, name: String) -> mlua::Result<String> {
     let rt = lua
         .app_data_ref::<Rc<Runtime>>()
@@ -283,11 +311,14 @@ fn lookup_secret(lua: &Lua, name: String) -> mlua::Result<String> {
 
 /// The two valid shapes of `cmd` for `(sh cmd …)`. A bare string
 /// runs under `sh -c`; a sequence runs as argv with no shell.
-#[derive(serde::Deserialize)]
-#[serde(untagged)]
+///
+/// `Argv` splits the program from its arguments at construction so
+/// `From<Cmd> for Command` can't be handed an empty argv. The
+/// non-empty invariant is enforced in [`mlua::FromLua`] before this
+/// type is ever built.
 enum Cmd {
     Shell(String),
-    Argv(Vec<String>),
+    Argv { program: String, args: Vec<String> },
 }
 
 impl From<Cmd> for std::process::Command {
@@ -298,9 +329,9 @@ impl From<Cmd> for std::process::Command {
                 c.arg("-c").arg(s);
                 c
             }
-            Cmd::Argv(argv) => {
-                let mut c = std::process::Command::new(&argv[0]);
-                c.args(&argv[1..]);
+            Cmd::Argv { program, args } => {
+                let mut c = std::process::Command::new(program);
+                c.args(args);
                 c
             }
         }
@@ -315,6 +346,10 @@ impl Cmd {
     // TODO: stream stdout/stderr live instead of buffering. `output()`
     // captures the full child output in memory and only returns at exit,
     // so long-running or chatty jobs show nothing until they finish.
+    // The streaming rewrite should also revisit the `from_utf8_lossy`
+    // calls below — non-UTF-8 bytes are silently replaced with U+FFFD
+    // and `:stdout` / `:stderr` end up as mojibake with no signal that
+    // anything was lost.
     fn run(self, opts: ShOpts) -> std::io::Result<ShOutput> {
         let mut command: std::process::Command = self.into();
         for (k, v) in opts.env {
@@ -336,11 +371,8 @@ impl Cmd {
 
 impl mlua::FromLua for Cmd {
     fn from_lua(value: mlua::Value, lua: &Lua) -> mlua::Result<Self> {
-        // Pre-check the Lua type so wrong-shape inputs get specific
-        // FromLuaConversionError messages — serde's untagged dispatch
-        // would otherwise just say "data did not match any variant".
         match &value {
-            mlua::Value::String(_) => lua.from_value(value),
+            mlua::Value::String(_) => Ok(Cmd::Shell(lua.from_value(value)?)),
             mlua::Value::Table(t) if t.raw_len() == 0 => {
                 // `raw_len() == 0` covers both an empty sequence (`[]`)
                 // and a string-keyed table (`{:env {...}}`) passed in
@@ -353,7 +385,15 @@ impl mlua::FromLua for Cmd {
                     ),
                 })
             }
-            mlua::Value::Table(_) => lua.from_value(value),
+            mlua::Value::Table(_) => {
+                let argv: Vec<String> = lua.from_value(value)?;
+                let mut iter = argv.into_iter();
+                let program = iter.next().expect("raw_len > 0 ensures argv is non-empty");
+                Ok(Cmd::Argv {
+                    program,
+                    args: iter.collect(),
+                })
+            }
             other => Err(mlua::Error::FromLuaConversionError {
                 from: other.type_name(),
                 to: "Cmd".into(),
diff --git a/src/error.rs b/src/error.rs
index d06cab6..3ace9e1 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -68,7 +68,9 @@ mod tests {
 
     #[test]
     fn from_fennel_error() {
-        let fennel_err = FennelError::FileNotFound("test.fnl".to_string());
+        let fennel_err = FennelError::Empty {
+            name: "test.fnl".to_string(),
+        };
         let err: Error = fennel_err.into();
         assert!(err.to_string().contains("test.fnl"));
     }
diff --git a/src/fennel.rs b/src/fennel.rs
index a1f63bf..e16f9d2 100644
--- a/src/fennel.rs
+++ b/src/fennel.rs
@@ -1,6 +1,6 @@
 use std::path::Path;
 
-use miette::{Diagnostic, Result, SourceOffset};
+use miette::{Diagnostic, SourceOffset};
 use mlua::{Lua, LuaSerdeExt};
 
 const FENNEL_LUA: &str = include_str!("../vendor/fennel.lua");
@@ -8,14 +8,11 @@ const FENNEL_LUA: &str = include_str!("../vendor/fennel.lua");
 /// Error kinds from the Fennel loader.
 #[derive(Debug, thiserror::Error, Diagnostic)]
 pub enum FennelError {
-    #[error("file not found: {0}")]
-    FileNotFound(String),
-
     #[error(transparent)]
     #[diagnostic(code(fennel::io))]
     Io(#[from] std::io::Error),
 
-    #[error("internal fennel error")]
+    #[error("internal fennel error: {0}")]
     #[diagnostic(code(fennel::internal))]
     Internal(#[from] mlua::Error),
 
@@ -26,7 +23,7 @@ pub enum FennelError {
         #[source_code]
         source_code: String,
         #[label("here")]
-        label: SourceOffset,
+        label: Option<SourceOffset>,
         #[source]
         source: Box<mlua::Error>,
     },
@@ -93,7 +90,7 @@ impl Fennel {
         name: &str,
         setup: impl Fn(&Lua) -> mlua::Result<()>,
     ) -> Result<mlua::Value, FennelError> {
-        setup(&self.lua).map_err(|e| FennelError::from_lua(source, name, e))?;
+        setup(&self.lua)?;
 
         let fennel: mlua::Table = self.lua.globals().get("fennel")?;
 
@@ -141,7 +138,7 @@ impl Fennel {
         self.lua
             .from_value(result)
             .map_err(|e| FennelError::TypeMismatch {
-                message: format!("{name}: {e}"),
+                message: name.to_string(),
                 source: Box::new(e),
             })
     }
@@ -152,10 +149,6 @@ impl Fennel {
     where
         T: serde::de::DeserializeOwned,
     {
-        if !path.exists() {
-            return Err(FennelError::FileNotFound(path.display().to_string()));
-        }
-
         let source = fs_err::read_to_string(path)?;
         self.load_string(&source, &path.display().to_string())
     }
@@ -171,14 +164,14 @@ impl FennelError {
         let message = name.to_string();
 
         // Try to extract a line number from the Lua error for a label.
-        let offset = extract_line_offset(&err)
-            .and_then(|line| line_offset(source, line))
-            .unwrap_or(SourceOffset::from(0));
+        // None when the error message doesn't carry a line — miette renders
+        // the source block without an inline pointer in that case.
+        let label = extract_line_offset(&err).and_then(|line| line_offset(source, line));
 
         FennelError::Eval {
             message,
             source_code: source.to_string(),
-            label: offset,
+            label,
             source: Box::new(err),
         }
     }
@@ -357,8 +350,15 @@ mod tests {
     fn load_file_rejects_missing_file() {
         let f = fennel();
         let result: Result<MirrorConfig, _> = f.load_file(Path::new("/no/such/file.fnl"));
-        assert!(result.is_err());
-        assert!(matches!(result.unwrap_err(), FennelError::FileNotFound(..)));
+        let err = result.unwrap_err();
+        assert!(
+            matches!(&err, FennelError::Io(e) if e.kind() == std::io::ErrorKind::NotFound),
+            "expected NotFound io error, got: {err}"
+        );
+        assert!(
+            err.to_string().contains("/no/such/file.fnl"),
+            "io error should mention path: {err}"
+        );
     }
 
     #[test]
@@ -371,7 +371,7 @@ mod tests {
             unreachable!()
         };
         assert_eq!(
-            label.offset(),
+            label.expect("label should be set when line is extractable").offset(),
             1,
             "label should point at line 2 despite colons in name"
         );
diff --git a/src/quire.rs b/src/quire.rs
index f52f65a..3a10dc5 100644
--- a/src/quire.rs
+++ b/src/quire.rs
@@ -888,10 +888,13 @@ mod tests {
         };
 
         let err = repo.config().unwrap_err();
-        let msg = err.to_string();
+        let chain: Vec<String> =
+            std::iter::successors(Some(&err as &dyn std::error::Error), |e| e.source())
+                .map(|e| e.to_string())
+                .collect();
         assert!(
-            msg.contains("credentials"),
-            "expected credential error, got: {msg}"
+            chain.iter().any(|m| m.contains("credentials")),
+            "expected credential error in chain, got: {chain:?}"
         );
     }
 }