Skip to content

Harden workflow codegen module-name derivation#186

Open
sohumt123 wants to merge 1 commit into
warpdotdev:mainfrom
sohumt123:fix/harden-codegen-module-names
Open

Harden workflow codegen module-name derivation#186
sohumt123 wants to merge 1 commit into
warpdotdev:mainfrom
sohumt123:fix/harden-codegen-module-names

Conversation

@sohumt123

Copy link
Copy Markdown

Discord username (optional)

(happy to share if useful for the Contributor role)

Description of changes

This is a build-tooling fix rather than a new/updated workflow. While rebuilding the crate I found two ways that innocently named spec files can silently drop a workflow or brick cargo build for everyone, so this PR hardens the module-name derivation in workflows/build.rs.

Problem

build.rs turns each spec's file name into a Rust module name and emits pub mod <name>;. Two edge cases produce broken generated code:

  1. Module-name collisions (silent overwrite + build break). The name is derived from the basename via to_case(Case::Snake) with no uniqueness check. Two specs that differ only in separators/case — e.g. collide-test.yaml and collide_test.yaml — both normalize to collide_test. The generated .rs file for the first one is silently overwritten (one workflow is lost), and the module list emits a duplicate pub mod collide_test;:

    error[E0428]: the name `collide_test` is defined multiple times
       --> workflows/src/generated_workflows/mod.rs:185:1
    

    WalkDir recurses all of ../specs but only the basename is used, so this can even collide across different tool subdirectories.

  2. Interior extension / stray dot. The extension is stripped with String::replace(".yaml", ""), which removes every occurrence, and the snake-case pass passes . through unchanged. So my.yaml.config.yaml becomes the invalid module my.config:

    error: expected one of `;` or `{`, found `.`
       --> workflows/src/generated_workflows/mod.rs:271:11
       |
    271| pub mod my.config;
       |           ^ expected one of `;` or `{`
    

Fix

  • Factor the derivation into module_name(&str) -> String (new workflows/src/module_name.rs). It strips only the trailing .yaml/.yml, snake-cases the stem, replaces any remaining non-[A-Za-z0-9_] char with _, and prefixes _ if the result is empty or starts with a digit, so the output is always a valid identifier.

  • build.rs now keeps a HashMap<module name, spec path> and, on collision, fails fast with a clear message naming both specs — which also prevents the silent overwrite:

    Error: workflow module name collision: ../specs/collide-test.yaml and ../specs/collide_test.yaml both normalize to module name `collide_test`; rename one of the spec files
    
  • The helper is shared into build.rs via #[path] and compiled into the library under #[cfg(test)], so its unit tests actually run under cargo test (a build script has no test harness of its own). While touching that expression I also replaced the pre-existing .expect("OsStr should convert to str") with proper anyhow context.

Zero behavior change for the existing corpus: verified the new algorithm against all 332 current spec basenames — the generated module names are byte-for-byte identical, so downstream output is untouched.

Testing

New unit tests in module_name.rs (the crate previously had no Rust tests) pin the exact mappings, including the regressions above, plus a property check that every output is a valid identifier:

$ cargo test -p warp-workflows
running 5 tests
test module_name::tests::digit_leading_stem_is_prefixed ... ok
test module_name::tests::colliding_basenames_map_to_the_same_name ... ok
test module_name::tests::strips_only_the_trailing_extension ... ok
test module_name::tests::preserves_existing_snake_case_outputs ... ok
test module_name::tests::output_is_always_a_valid_identifier ... ok
test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Integration check with local-only fixtures (not committed):

# before this PR
$ printf 'name: Dot\ncommand: echo dot\n' > specs/my.yaml.config.yaml
$ rm -rf workflows/src/generated_workflows target && cargo build -p warp-workflows
error: expected one of `;` or `{`, found `.`   ->   pub mod my.config;

# after this PR
$ rm -rf workflows/src/generated_workflows target && cargo build -p warp-workflows
   Finished `dev` profile ...
$ grep my_yaml_config workflows/src/generated_workflows/mod.rs
pub mod my_yaml_config;

# colliding pair now fails fast in the build script (no E0428, nothing overwritten)
$ printf 'name: A\ncommand: echo a\n' > specs/collide-test.yaml
$ printf 'name: B\ncommand: echo b\n' > specs/collide_test.yaml
$ rm -rf workflows/src/generated_workflows target && cargo build -p warp-workflows
Error: workflow module name collision: ../specs/collide-test.yaml and ../specs/collide_test.yaml both normalize to module name `collide_test`; rename one of the spec files

After removing the fixtures, a full rebuild of the pristine 332-spec corpus passes and ls generated_workflows | sort diffs empty against a pre-change snapshot (zero module-name drift). cargo fmt --check, cargo clippy -- -A warnings, and cargo clippy --tests -- -A warnings all pass.

The build script derives a Rust module name for each spec by snake-casing
its file name, but two edge cases produce broken generated code:

- Collisions: the derived name comes from the basename only and there is no
  uniqueness check, so two specs that differ solely in separators or case
  (e.g. `collide-test.yaml` and `collide_test.yaml`) both normalize to
  `collide_test`. One workflow's generated file is silently overwritten and
  the module list emits a duplicate `pub mod collide_test;`, which fails to
  compile with `error[E0428]`. Because WalkDir recurses `../specs` but only
  the basename is used, this can happen across different tool subdirectories.

- Interior extensions: stripping the extension with
  `String::replace(".yaml", "")` removes every occurrence, and the snake-case
  pass leaves stray dots in place, so `my.yaml.config.yaml` becomes the
  invalid module `my.config` and breaks the build.

Extract the derivation into a `module_name` helper that strips only the
trailing extension, snake-cases the stem, replaces any remaining
non-identifier character with `_`, and prefixes `_` when the result would be
empty or start with a digit, so the output is always a valid identifier. The
build script now tracks the module name -> spec path mapping and fails fast
with an error naming both colliding specs instead of silently dropping a
workflow or emitting uncompilable code.

The helper lives in `src/module_name.rs`, pulled into the build script via
`#[path]` and into the library under `cfg(test)` so the new unit tests run
under `cargo test` (build scripts have no test harness). Verified against all
332 existing specs: the generated module names are unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant