Harden workflow codegen module-name derivation

workflows/Cargo.toml

@@ -9,6 +9,10 @@ license.workspace = true
 [dependencies]
 warp-workflows-types = {path = "../workflow-types" }
 
+[dev-dependencies]
+# Needed to compile `src/module_name.rs` (shared with `build.rs`) under `cfg(test)` for its unit tests.
+convert_case = "0.11.0"
+
 [build-dependencies]
 anyhow = "1.0"
 convert_case = "0.11.0"

workflows/build.rs

@@ -1,22 +1,33 @@
-use anyhow::Result;
-use convert_case::{Case, Casing};
+use anyhow::{bail, Context, Result};
+use std::collections::HashMap;
 use std::ffi::OsStr;
 use std::fs::File;
 use std::io::Write;
+use std::path::PathBuf;
 use std::process::Command;
 use walkdir::WalkDir;
 use warp_workflows_types::Workflow;
 
+// The module-name derivation is shared with the library (see `workflows/src/lib.rs`) so it can be
+// unit tested; build scripts are compiled as a separate crate with no test harness.
+#[path = "src/module_name.rs"]
+mod module_name;
+use module_name::module_name;
+
 /// Generates Workflows as rust files from the yaml stored in /specs. Each Workflow is stored within
 /// its own mod within the `generated_workflows` module. Additionally, a function called `workflows`
 /// is generated that returns a vector of all the `Workflow`s that were created.
 fn main() -> Result<()> {
     println!("cargo:rerun-if-changed=../specs");
+    println!("cargo:rerun-if-changed=src/module_name.rs");
 
     std::fs::create_dir_all("src/generated_workflows")?;
     let parent_module = std::fs::File::create("src/generated_workflows/mod.rs")?;
 
     let mut workflows_added = Vec::new();
+    // Maps each derived module name to the first spec path that produced it, so colliding specs are
+    // caught and reported instead of silently overwriting one another's generated file.
+    let mut module_names: HashMap<String, PathBuf> = HashMap::new();
 
     for entry in WalkDir::new("../specs") {
         let entry = entry?;
@@ -36,15 +47,25 @@ fn main() -> Result<()> {
             let workflow: Workflow = serde_yaml::from_str(yaml_content)?;
             println!("generated workflow is {workflow:?}");
 
-            let file_name = entry
-                .file_name()
-                .to_str()
-                .expect("OsStr should convert to str")
-                .replace(".yaml", "")
-                .replace(".yml", "")
-                .to_case(Case::Snake);
+            let raw_name = entry.file_name().to_str().with_context(|| {
+                format!("spec file name is not valid UTF-8: {:?}", entry.path())
+            })?;
+            let file_name = module_name(raw_name);
             println!("file name is {file_name:?}");
 
+            // Two specs whose names normalize to the same module would silently overwrite each
+            // other's generated file and emit a duplicate `pub mod`, so fail fast with both paths.
+            if let Some(existing) =
+                module_names.insert(file_name.clone(), entry.path().to_path_buf())
+            {
+                bail!(
+                    "workflow module name collision: {} and {} both normalize to module name `{}`; rename one of the spec files",
+                    existing.display(),
+                    entry.path().display(),
+                    file_name
+                );
+            }
+
             // Create a module for each Workflow within the parent module.
             workflows_added.push(file_name.clone());
 

workflows/src/lib.rs

@@ -1,4 +1,10 @@
 mod generated_workflows;
 
+// Shared with `build.rs` via `#[path = "src/module_name.rs"]`. Compiled into the library only
+// under `cfg(test)` so its unit tests are exercised by `cargo test` (build scripts have no test
+// harness of their own).
+#[cfg(test)]
+mod module_name;
+
 pub use generated_workflows::workflows;
 pub use warp_workflows_types::*;

workflows/src/module_name.rs

@@ -0,0 +1,112 @@
+use convert_case::{Case, Casing};
+
+/// Derives the Rust module name used for a workflow spec from its file name.
+///
+/// The spec's *trailing* `.yaml`/`.yml` extension is removed, the remaining stem is converted to
+/// `snake_case`, any character outside `[A-Za-z0-9_]` is replaced with `_`, and a leading `_` is
+/// added when the result would otherwise be empty or start with an ASCII digit. This guarantees the
+/// return value is always a valid Rust identifier, which the code generator relies on when it emits
+/// `pub mod <name>;`.
+///
+/// Only the trailing extension is stripped (rather than every `.yaml`/`.yml` substring), so a file
+/// such as `my.yaml.config.yaml` maps to `my_yaml_config` instead of an invalid `my.config`.
+pub(crate) fn module_name(file_name: &str) -> String {
+    let stem = file_name
+        .strip_suffix(".yaml")
+        .or_else(|| file_name.strip_suffix(".yml"))
+        .unwrap_or(file_name);
+
+    let snake = stem.to_case(Case::Snake);
+
+    let mut sanitized: String = snake
+        .chars()
+        .map(|c| {
+            if c.is_ascii_alphanumeric() || c == '_' {
+                c
+            } else {
+                '_'
+            }
+        })
+        .collect();
+
+    if sanitized.chars().next().is_none_or(|c| c.is_ascii_digit()) {
+        sanitized.insert(0, '_');
+    }
+
+    sanitized
+}
+
+#[cfg(test)]
+mod tests {
+    use super::module_name;
+
+    #[test]
+    fn preserves_existing_snake_case_outputs() {
+        // Real spec: specs/cosmwasm/cosmwasm-optimize.yaml
+        assert_eq!(module_name("cosmwasm-optimize.yaml"), "cosmwasm_optimize");
+        // Real spec: specs/chef/run_cookbook_manually.yml (a `.yml` file)
+        assert_eq!(
+            module_name("run_cookbook_manually.yml"),
+            "run_cookbook_manually"
+        );
+        // Plain snake identity.
+        assert_eq!(module_name("list_directories.yaml"), "list_directories");
+    }
+
+    #[test]
+    fn strips_only_the_trailing_extension() {
+        // Regression: `String::replace(".yaml", "")` used to strip every occurrence and leave the
+        // interior dot in place, yielding the invalid module `my.config`.
+        assert_eq!(module_name("my.yaml.config.yaml"), "my_yaml_config");
+    }
+
+    #[test]
+    fn colliding_basenames_map_to_the_same_name() {
+        // Documents the collision class the build script now detects and rejects: two specs that
+        // differ only in separators normalize to one identifier.
+        assert_eq!(
+            module_name("collide-test.yaml"),
+            module_name("collide_test.yaml")
+        );
+    }
+
+    #[test]
+    fn digit_leading_stem_is_prefixed() {
+        // `2fa-setup` snake-cases to `2_fa_setup`; a leading `_` keeps it a valid identifier.
+        assert_eq!(module_name("2fa-setup.yaml"), "_2_fa_setup");
+    }
+
+    #[test]
+    fn output_is_always_a_valid_identifier() {
+        let inputs = [
+            "cosmwasm-optimize.yaml",
+            "run_cookbook_manually.yml",
+            "list_directories.yaml",
+            "my.yaml.config.yaml",
+            "collide-test.yaml",
+            "collide_test.yaml",
+            "2fa-setup.yaml",
+            "weird!!name.yaml",
+            ".yaml",
+            "123.yml",
+        ];
+
+        for input in inputs {
+            let name = module_name(input);
+
+            match name.chars().next() {
+                None => panic!("empty module name for {input:?}"),
+                Some(first) => assert!(
+                    first.is_ascii_lowercase() || first == '_',
+                    "module name {name:?} for {input:?} starts with an invalid char"
+                ),
+            }
+
+            assert!(
+                name.chars()
+                    .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '_'),
+                "module name {name:?} for {input:?} contains an invalid char"
+            );
+        }
+    }
+}