Replace stringly-typed CI errors with structured types
ValidationError was a struct with a single message field, forcing
callers to substring-match for specific rule violations. Now an enum
with Cycle, EmptyInputs, Unreachable, and ReservedSlash variants that
carry the relevant data. eval_ci similarly stopped flattening mlua
errors into Error::Lua(String) and now produces FennelError::Eval with
source snippets and line labels, sharing the error infrastructure from
fennel.rs. Error::Lua is gone.

Assisted-by: GLM-5.1 via pi
change krmnnzpxnryssrmkrxwqymstmlwqltzk
commit a7a41b8dd42637fa28454ca5b0fd37e4d71b2001
author Alpha Chen <alpha@kejadlen.dev>
date
parent wyypmzsr
diff --git a/src/ci.rs b/src/ci.rs
index 9323870..e0bb90a 100644
--- a/src/ci.rs
+++ b/src/ci.rs
@@ -314,7 +314,19 @@ pub fn eval_ci(
     source: &str,
     name: &str,
 ) -> crate::Result<EvalResult> {
-    eval_ci_inner(source, name).map_err(|e| crate::Error::Lua(e.to_string()))
+    eval_ci_inner(source, name).map_err(|e| {
+        // Convert mlua errors into rich FennelError with source snippets.
+        match e {
+            mlua::Error::RuntimeError(_) | mlua::Error::SyntaxError { .. } => {
+                crate::fennel::eval_error(source, name, &e).into()
+            }
+            _ => crate::Error::Fennel(crate::fennel::FennelError::Eval {
+                message: format!("{name}: {e}"),
+                source_code: source.to_string(),
+                label: miette::SourceOffset::from(0),
+            }),
+        }
+    })
 }
 
 fn eval_ci_inner(source: &str, name: &str) -> mlua::Result<EvalResult> {
@@ -368,9 +380,20 @@ fn eval_ci_inner(source: &str, name: &str) -> mlua::Result<EvalResult> {
 
 /// A validation error found in the job graph.
 #[derive(Debug, thiserror::Error, miette::Diagnostic)]
-#[error("{message}")]
-pub struct ValidationError {
-    pub message: String,
+pub enum ValidationError {
+    #[error("Cycle detected among jobs: {}", cycle_jobs.join(", "))]
+    Cycle { cycle_jobs: Vec<String> },
+
+    #[error(
+        "Job '{job_id}' has empty inputs. Pass [:quire/push] (or another input) so it has something to fire it."
+    )]
+    EmptyInputs { job_id: String },
+
+    #[error("Job '{job_id}' is not reachable from any source ref (e.g. :quire/push).")]
+    Unreachable { job_id: String },
+
+    #[error("Job id '{job_id}' contains '/', which is reserved for the 'quire/' source namespace.")]
+    ReservedSlash { job_id: String },
 }
 
 /// Validate the structural rules of a job graph.
@@ -385,11 +408,8 @@ pub fn validate(jobs: &[JobDef]) -> std::result::Result<(), Vec<ValidationError>
     // Rule 4: no '/' in user job ids.
     for job in jobs {
         if job.id.contains('/') {
-            errors.push(ValidationError {
-                message: format!(
-                    "Job id '{}' contains '/', which is reserved for the 'quire/' source namespace.",
-                    job.id
-                ),
+            errors.push(ValidationError::ReservedSlash {
+                job_id: job.id.clone(),
             });
         }
     }
@@ -397,11 +417,8 @@ pub fn validate(jobs: &[JobDef]) -> std::result::Result<(), Vec<ValidationError>
     // Rule 2: non-empty inputs.
     for job in jobs {
         if job.inputs.is_empty() {
-            errors.push(ValidationError {
-                message: format!(
-                    "Job '{}' has empty inputs. Pass [:quire/push] (or another input) so it has something to fire it.",
-                    job.id
-                ),
+            errors.push(ValidationError::EmptyInputs {
+                job_id: job.id.clone(),
             });
         }
     }
@@ -446,14 +463,13 @@ pub fn validate(jobs: &[JobDef]) -> std::result::Result<(), Vec<ValidationError>
     }
 
     if sorted.len() != jobs.len() {
-        let cycle_jobs: Vec<&str> = jobs
+        let cycle_jobs: Vec<String> = jobs
             .iter()
             .map(|j| j.id.as_str())
             .filter(|id| !sorted.contains(id))
+            .map(|s| s.to_string())
             .collect();
-        errors.push(ValidationError {
-            message: format!("Cycle detected among jobs: {}", cycle_jobs.join(", ")),
-        });
+        errors.push(ValidationError::Cycle { cycle_jobs });
     }
 
     // Rule 3: reachability — every job's transitive inputs must include a source ref.
@@ -480,11 +496,8 @@ pub fn validate(jobs: &[JobDef]) -> std::result::Result<(), Vec<ValidationError>
         }
 
         if !found_source {
-            errors.push(ValidationError {
-                message: format!(
-                    "Job '{}' is not reachable from any source ref (e.g. :quire/push).",
-                    job.id
-                ),
+            errors.push(ValidationError::Unreachable {
+                job_id: job.id.clone(),
             });
         }
     }
@@ -896,9 +909,8 @@ mod tests {
         ];
         let errs = validate(&jobs).unwrap_err();
         assert!(
-            errs.iter()
-                .any(|e| e.message.to_lowercase().contains("cycle")),
-            "should report a cycle: {errs:?}"
+            errs.iter().any(|e| matches!(e, ValidationError::Cycle { cycle_jobs } if cycle_jobs.contains(&"a".to_string()) && cycle_jobs.contains(&"b".to_string()))),
+            "should report a cycle involving a and b: {errs:?}"
         );
     }
 
@@ -911,7 +923,7 @@ mod tests {
         let errs = validate(&jobs).unwrap_err();
         assert!(
             errs.iter()
-                .any(|e| e.message.contains("setup") && e.message.contains("empty inputs")),
+                .any(|e| matches!(e, ValidationError::EmptyInputs { job_id } if job_id == "setup")),
             "should report empty inputs for 'setup': {errs:?}"
         );
     }
@@ -924,8 +936,9 @@ mod tests {
         }];
         let errs = validate(&jobs).unwrap_err();
         assert!(
-            errs.iter()
-                .any(|e| e.message.contains("orphan") && e.message.contains("source")),
+            errs.iter().any(
+                |e| matches!(e, ValidationError::Unreachable { job_id } if job_id == "orphan")
+            ),
             "should report unreachable job 'orphan': {errs:?}"
         );
     }
@@ -938,8 +951,9 @@ mod tests {
         }];
         let errs = validate(&jobs).unwrap_err();
         assert!(
-            errs.iter()
-                .any(|e| e.message.contains("foo/bar") && e.message.contains("'/'")),
+            errs.iter().any(
+                |e| matches!(e, ValidationError::ReservedSlash { job_id } if job_id == "foo/bar")
+            ),
             "should report slash in job id: {errs:?}"
         );
     }
diff --git a/src/error.rs b/src/error.rs
index 7956c64..0e16cde 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -25,9 +25,6 @@ pub enum Error {
     #[related]
     Validation(Vec<ValidationError>),
 
-    #[error("lua error: {0}")]
-    Lua(String),
-
     #[error("git error: {0}")]
     Git(String),
 
diff --git a/src/fennel.rs b/src/fennel.rs
index ecc6153..e6459e4 100644
--- a/src/fennel.rs
+++ b/src/fennel.rs
@@ -151,7 +151,7 @@ impl Fennel {
     }
 }
 
-fn eval_error(source: &str, name: &str, err: &mlua::Error) -> FennelError {
+pub(crate) fn eval_error(source: &str, name: &str, err: &mlua::Error) -> FennelError {
     let message = format!("{name}: {err}");
 
     // Try to extract a line number from the Lua error for a label.
@@ -172,7 +172,7 @@ fn eval_error(source: &str, name: &str, err: &mlua::Error) -> FennelError {
 /// The name may contain colons (e.g. `HEAD:.quire/config.fnl`), so splitting
 /// from the left breaks. Match the first `:LINE:COLUMN: ` run, which is
 /// unambiguous — filenames don't end with `:digits:digits:`.
-fn extract_line_offset(err: &mlua::Error) -> Option<usize> {
+pub(crate) fn extract_line_offset(err: &mlua::Error) -> Option<usize> {
     let msg = err.to_string();
     let re = regex::Regex::new(r":(\d+):\d+: ").ok()?;
     let caps = re.captures(&msg)?;
@@ -184,7 +184,7 @@ fn extract_line_offset(err: &mlua::Error) -> Option<usize> {
 }
 
 /// Convert a 1-based line number to a byte offset in the source.
-fn line_offset(source: &str, line: usize) -> Option<SourceOffset> {
+pub(crate) fn line_offset(source: &str, line: usize) -> Option<SourceOffset> {
     if line == 0 {
         return None;
     }