Reject duplicate job ids in registration
Catches (ci.job :foo …) (ci.job :foo …) — previously the second
silently overwrote the first in the node-index map while both stayed
in the jobs Vec. As a side benefit, mirror's singleton bookkeeping
disappears: a second (ci.mirror …) now collides on the quire/mirror
id like any other duplicate.

Assisted-by: Claude Opus 4.7 via Claude Code
change rtrwqlpxotntslmszwkknkspmnqlosqk
commit e3d35f94996b5dd370e42766e5699ae872f81d69
author Alpha Chen <alpha@kejadlen.dev>
date
parent yvyklkzq
diff --git a/src/ci/mirror.rs b/src/ci/mirror.rs
index dd3088d..5bc8892 100644
--- a/src/ci/mirror.rs
+++ b/src/ci/mirror.rs
@@ -18,14 +18,6 @@ use super::runtime::{Cmd, Runtime, ShOpts, ShOutput};
 use crate::Result;
 use crate::error::Error;
 
-/// Marker that `(ci.mirror …)` was called. Held only so a second
-/// call can produce `DuplicateMirror`; the resulting job is built
-/// inline in `register_mirror` and pushed onto `Registration::jobs`
-/// like any other.
-pub(super) struct MirrorRegistration {
-    _line: u32,
-}
-
 /// Parsed options from `(ci.mirror url opts)`. Captured at
 /// registration time and moved into the run-fn closure.
 struct MirrorOpts {
@@ -97,9 +89,11 @@ fn parse_mirror_opts(lua: &Lua, opts: mlua::Table) -> mlua::Result<MirrorOpts> {
     })
 }
 
-/// Body of `(ci.mirror url opts)`. Validates singleton, parses opts,
-/// and registers a single internal job at `quire/mirror` whose run-fn
-/// performs the tag-and-push at execute time.
+/// Body of `(ci.mirror url opts)`. Parses opts and registers an
+/// internal job at `quire/mirror` whose run-fn performs the
+/// tag-and-push at execute time. Singleton-ness is enforced by
+/// generic id uniqueness in `Registration::add_job` — a second
+/// `(ci.mirror …)` collides on the `quire/mirror` id.
 pub(super) fn register_mirror(lua: &Lua, (url, opts): (String, mlua::Table)) -> mlua::Result<()> {
     let r = lua
         .app_data_ref::<Registration>()
@@ -110,27 +104,16 @@ pub(super) fn register_mirror(lua: &Lua, (url, opts): (String, mlua::Table)) ->
         .map(|l| l as u32)
         .unwrap_or(0);
 
-    // Singleton check.
-    {
-        let mut m = r.mirror.borrow_mut();
-        if m.is_some() {
-            let span = pipeline::span_for_line(&r.source, line);
-            r.errors
-                .borrow_mut()
-                .push(DefinitionError::DuplicateMirror { span });
-            return Ok(());
-        }
-        *m = Some(MirrorRegistration { _line: line });
-    }
-
     let parsed = match parse_mirror_opts(lua, opts) {
         Ok(p) => p,
         Err(e) => {
             let span = pipeline::span_for_line(&r.source, line);
-            r.errors.borrow_mut().push(DefinitionError::InvalidMirrorCall {
-                message: e.to_string(),
-                span,
-            });
+            r.errors
+                .borrow_mut()
+                .push(DefinitionError::InvalidMirrorCall {
+                    message: e.to_string(),
+                    span,
+                });
             return Ok(());
         }
     };
@@ -153,7 +136,7 @@ pub(super) fn register_mirror(lua: &Lua, (url, opts): (String, mlua::Table)) ->
     inputs.extend(parsed.after);
 
     match Job::new("quire/mirror".to_string(), inputs, run_fn, line, &r.source) {
-        Ok(job) => r.jobs.borrow_mut().push(job),
+        Ok(job) => r.add_job(job, line),
         Err(e) => r.errors.borrow_mut().push(e),
     }
     Ok(())
@@ -228,12 +211,11 @@ fn execute_mirror(
     // of the argv (visible in `ps`); piping via $T is the smallest
     // stdin-free alternative.
     let token_pair = format!("x-access-token:{secret}");
-    let encoded_output = Cmd::Shell("printf '%s' \"$T\" | base64 --wrap=0".to_string()).run(
-        ShOpts {
+    let encoded_output =
+        Cmd::Shell("printf '%s' \"$T\" | base64 --wrap=0".to_string()).run(ShOpts {
             env: HashMap::from([("T".to_string(), token_pair)]),
             cwd: None,
-        },
-    )?;
+        })?;
     let auth_header = format!("Authorization: Basic {}", encoded_output.stdout.trim());
 
     // Push the configured refs (or the trigger ref, if none) plus the tag.
@@ -366,17 +348,14 @@ mod tests {
 (ci.mirror "https://github.com/example/repo.git"
   {:secret :github_token :tag (fn [_] "v1") :after [:build]})"#;
         let inputs = mirror_job_inputs(source);
-        assert_eq!(
-            inputs,
-            vec!["quire/push".to_string(), "build".to_string()]
-        );
+        assert_eq!(inputs, vec!["quire/push".to_string(), "build".to_string()]);
     }
 
     #[test]
-    fn mirror_duplicate_call_errors() {
+    fn mirror_duplicate_call_errors_via_id_collision() {
         let source = r#"(local ci (require :quire.ci))
-(ci.mirror "https://github.com/example/repo.git" {:secret :a})
-(ci.mirror "https://github.com/example/other.git" {:secret :b})"#;
+(ci.mirror "https://github.com/example/repo.git" {:secret :a :tag (fn [_] "v1")})
+(ci.mirror "https://github.com/example/other.git" {:secret :b :tag (fn [_] "v1")})"#;
         let Err(err) = compile(source, "ci.fnl") else {
             panic!("expected error");
         };
@@ -386,9 +365,10 @@ mod tests {
         assert!(
             pe.diagnostics.iter().any(|d| matches!(
                 d,
-                Diagnostic::Definition(DefinitionError::DuplicateMirror { .. })
+                Diagnostic::Definition(DefinitionError::DuplicateJob { job_id, .. })
+                    if job_id == "quire/mirror"
             )),
-            "expected DuplicateMirror in: {:?}",
+            "expected DuplicateJob('quire/mirror') in: {:?}",
             pe.diagnostics
         );
     }
diff --git a/src/ci/pipeline.rs b/src/ci/pipeline.rs
index 1a6f6ed..9943f11 100644
--- a/src/ci/pipeline.rs
+++ b/src/ci/pipeline.rs
@@ -41,9 +41,10 @@ pub enum DefinitionError {
         span: SourceSpan,
     },
 
-    #[error("Pipeline mirror declared more than once.")]
-    DuplicateMirror {
-        #[label("duplicate mirror declaration")]
+    #[error("Job '{job_id}' is registered more than once.")]
+    DuplicateJob {
+        job_id: String,
+        #[label("duplicate registration")]
         span: SourceSpan,
     },
 
@@ -512,7 +513,8 @@ mod tests {
     /// definition errors it produced.
     fn registration_errors(source: &str) -> Vec<DefinitionError> {
         let f = Fennel::new().expect("Fennel::new() should succeed");
-        let err = registration::register(&f, source, "ci.fnl").expect_err("expected registration errors");
+        let err =
+            registration::register(&f, source, "ci.fnl").expect_err("expected registration errors");
         let crate::Error::Pipeline(pe) = err else {
             panic!("expected PipelineError, got {err:?}")
         };
@@ -633,6 +635,21 @@ mod tests {
         );
     }
 
+    #[test]
+    fn register_rejects_duplicate_job_id() {
+        let errors = registration_errors(
+            r#"(local ci (require :quire.ci))
+(ci.job :build [:quire/push] (fn [_] nil))
+(ci.job :build [:quire/push] (fn [_] nil))"#,
+        );
+        assert!(
+            errors.iter().any(
+                |e| matches!(e, DefinitionError::DuplicateJob { job_id, .. } if job_id == "build")
+            ),
+            "should report duplicate job id 'build': {errors:?}"
+        );
+    }
+
     #[test]
     fn validate_does_not_double_report_cycle_as_unreachable() {
         // Jobs in a cycle are technically also unreachable from any
diff --git a/src/ci/registration.rs b/src/ci/registration.rs
index 9a0a947..ea6f62f 100644
--- a/src/ci/registration.rs
+++ b/src/ci/registration.rs
@@ -38,7 +38,6 @@ pub(super) struct Registrations {
 pub(super) fn register(fennel: &Fennel, source: &str, name: &str) -> Result<Registrations> {
     let jobs: Rc<RefCell<Vec<Job>>> = Rc::new(RefCell::new(Vec::new()));
     let image = Rc::new(RefCell::new(None));
-    let mirror_cell = Rc::new(RefCell::new(None));
     let src = Rc::new(source.to_string());
 
     let errors = Rc::new(RefCell::new(Vec::new()));
@@ -50,7 +49,6 @@ pub(super) fn register(fennel: &Fennel, source: &str, name: &str) -> Result<Regi
                 jobs: jobs.clone(),
                 errors: errors.clone(),
                 image: image.clone(),
-                mirror: mirror_cell.clone(),
                 source: src.clone(),
             },
         )
@@ -96,7 +94,6 @@ pub(super) struct Registration {
     pub(super) jobs: Rc<RefCell<Vec<Job>>>,
     pub(super) errors: Rc<RefCell<Vec<DefinitionError>>>,
     image: Rc<RefCell<Option<ImageRegistration>>>,
-    pub(super) mirror: Rc<RefCell<Option<mirror::MirrorRegistration>>>,
     pub(super) source: Rc<String>,
 }
 
@@ -111,6 +108,26 @@ impl IntoLua for Registration {
     }
 }
 
+impl Registration {
+    /// Push a registered job after enforcing id uniqueness. On
+    /// collision, records `DuplicateJob` against the caller's source
+    /// line and drops the new job; the first registration wins.
+    pub(super) fn add_job(&self, job: Job, line: u32) {
+        let mut jobs = self.jobs.borrow_mut();
+        if jobs.iter().any(|j| j.id == job.id) {
+            let span = pipeline::span_for_line(&self.source, line);
+            self.errors
+                .borrow_mut()
+                .push(DefinitionError::DuplicateJob {
+                    job_id: job.id,
+                    span,
+                });
+            return;
+        }
+        jobs.push(job);
+    }
+}
+
 /// A pending image registration extracted from the Lua callback.
 struct ImageRegistration {
     name: String,
@@ -170,7 +187,7 @@ fn register_job(
     }
 
     match Job::new(id, inputs, RunFn::Lua(run_fn), line, &r.source) {
-        Ok(job) => r.jobs.borrow_mut().push(job),
+        Ok(job) => r.add_job(job, line),
         Err(e) => r.errors.borrow_mut().push(e),
     }
     Ok(())
diff --git a/src/ci/run.rs b/src/ci/run.rs
index 4222666..2513ced 100644
--- a/src/ci/run.rs
+++ b/src/ci/run.rs
@@ -12,8 +12,8 @@ use std::rc::Rc;
 use jiff::Timestamp;
 use mlua::IntoLua;
 
-use super::runtime::{Runtime, RuntimeHandle, ShOutput};
 use super::pipeline::{Pipeline, RunFn};
+use super::runtime::{Runtime, RuntimeHandle, ShOutput};
 use crate::display_chain;
 use crate::secret::SecretString;
 use crate::{Error, Result};