Refactor manifest module into dedicated submodules

[leynos]

Summary

• split src/manifest.rs into dedicated diagnostics, glob, expansion, and rendering submodules while preserving the public manifest API surface
• update manifest tests to import helpers from the new module structure and keep glob and YAML diagnostics coverage intact
• closes Refactor src/manifest.rs: Split module to satisfy ≤400-line rule #156

Testing

• make fmt
• make lint
• make test

---

https://chatgpt.com/codex/tasks/task_e_68d96f5606a48322a56e65a1f89eb3de

Summary by Sourcery

Split the monolithic manifest implementation into dedicated submodules for diagnostics, glob pattern handling, template rendering, and foreach expansion, retaining the existing public API and updating tests accordingly.

Enhancements:

• Extract glob normalization and validation logic into a new manifest::glob submodule.
• Move Jinja-based template rendering into a new manifest::render submodule.
• Isolate foreach expansion logic in a new manifest::expand submodule.
• Centralize YAML error mapping and diagnostics in a new manifest::diagnostics submodule.

Tests:

• Update manifest tests to import helpers from the new module structure.
• Add tests for empty glob matches and unmatched brace errors.
• Add unit tests for foreach expansion, template rendering, and YAML diagnostics hints.

[coderabbitai]

Note

Reviews paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Summary by CodeRabbit

• New Features

  • Added cross-platform glob pattern handling with improved brace validation and clearer errors.
  • Introduced foreach expansion for manifest targets, enabling generation of multiple targets from lists.
  • Enhanced manifest rendering for targets, rules, variables, and lists with consistent templating.

• Bug Fixes

  • Normalised path separators across platforms to reduce glob mismatches.
  • More precise and helpful YAML parse errors with locations and indentation hints.

• Refactor

  • Modularised manifest parsing, expansion, globbing, rendering, and diagnostics to improve maintainability without changing public entry points.

Walkthrough

Summarise the refactor: extract diagnostics, foreach expansion, glob handling and rendering from src/manifest.rs into dedicated manifest/* modules; re-export ManifestError; keep public entry points and orchestrate parsing via expand_foreach, render_manifest, and glob_paths. Update tests to import new module items.

Changes

Cohort / File(s)	Summary
Manifest module refactor src/manifest.rs	Remove in-file diagnostics, foreach, glob and render logic; wire new submodules and re-export ManifestError; keep public constructors delegating to new helpers.
Diagnostics subsystem src/manifest/diagnostics.rs	Add ManifestError enum and map_yaml_error; implement YamlDiagnostic with spans, indentation hints and miette integration.
Foreach expansion src/manifest/expand.rs	Add expand_foreach(doc, env) and helpers to expand targets via foreach/when, evaluate expressions and inject iteration variables.
Glob handling src/manifest/glob.rs	Add glob_paths, normalize_separators, brace validation, platform-aware escaping and structured error translation for template helpers.
Rendering src/manifest/render.rs	Add render_manifest(manifest, env) with per-target / per-rule templating, render_vars, and contextual error messages.
Tests update src/manifest/tests.rs	Update imports to use map_yaml_error, glob_paths, normalize_separators, YamlError, YAML_HINTS; keep behaviour unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Manifest as Manifest API
  participant YAML as serde_yaml
  participant Diag as diagnostics::map_yaml_error
  participant Expand as expand::expand_foreach
  participant Render as render::render_manifest
  participant Glob as glob::glob_paths

  Caller->>Manifest: from_str_named(name, src)
  Manifest->>YAML: parse(src)
  YAML-->>Manifest: Ok(doc) or Err(err)
  alt YAML error
    Manifest->>Diag: map_yaml_error(err, src, name)
    Diag-->>Manifest: Diagnostic (span, hints)
    Manifest-->>Caller: Err(ManifestError::Parse)
  else Parsed
    Manifest->>Expand: expand_foreach(doc, env)
    Expand-->>Manifest: Result<doc>
    Manifest->>Render: render_manifest(manifest, env)
    note over Render,Glob: Templates call helper to resolve file patterns
    Render->>Glob: glob_paths(pattern)
    Glob-->>Render: Vec<Path>
    Render-->>Manifest: Result<manifest>
    Manifest-->>Caller: Ok(manifest)
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

• Refactor src/manifest.rs: Extract glob helpers to submodule (exceeds 400-line limit) #136 — Move of glob_paths and normalize_separators to manifest/glob.rs matches this PR’s changes; link this PR to that issue.
• Refactor src/manifest.rs: Split module into focused submodules (≤400-line limit) #157 — Implements the proposed split of src/manifest.rs into diagnostics, glob, expand, render; link this PR to that issue.

Poem

Files once one, now split with care,
Diagnostics point to faults laid bare.
Foreach fans targets, globs resolve the chase,
Rendering binds variables in place.
A tidy manifest strides anew—cheers to the refactor!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 29.17% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title Check	✅ Passed	Recognise that the title succinctly describes the core change of splitting the manifest module into dedicated submodules, directly reflecting the primary refactor in the PR.
Linked Issues Check	✅ Passed	Confirm that the refactoring fulfils issue #156 by decomposing src/manifest.rs into the specified diagnostics, glob, expansion and render modules while retaining the main manifest structure and API surface as required.
Out of Scope Changes Check	✅ Passed	Ensure that all code changes are confined to the manifest modules and related tests, without introducing any modifications outside the scope defined by the linked issue objectives.
Description Check	✅ Passed	The pull request description directly addresses the changeset. It states that src/manifest.rs is being split into dedicated diagnostics, glob, expansion, and rendering submodules whilst preserving the public manifest API surface, which aligns precisely with the file-level summaries showing the creation of diagnostics.rs, glob.rs, expand.rs, and render.rs modules and the refactoring of manifest.rs to use these new modules. The description also mentions updating manifest tests to import helpers from the new module structure, matching the changes shown in tests.rs. The description references issue #156, provides testing commands, and includes additional context from Sourcery, all of which relate to the refactoring work evidenced in the changeset.

---

🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

• Public repositories are always opted into early access features.
• You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sourcery-ai]

Reviewer's Guide

The PR refactors the manifest module by extracting core responsibilities—glob pattern handling, template rendering, YAML expansion, and diagnostics—into dedicated submodules, removes the previous monolithic implementations, and updates tests to align with the new module layout while preserving the public API surface.

File-Level Changes

Change	Details	Files
Modularize manifest implementation into submodules	Extract glob pattern validation, normalization, and matching into manifest/glob.rs Move YAML foreach expansion logic into manifest/expand.rs Relocate Jinja rendering routines into manifest/render.rs Introduce YAML error diagnostics mapping in manifest/diagnostics.rs Clean up src/manifest.rs by removing inline logic and adding module declarations	src/manifest.rs src/manifest/glob.rs src/manifest/expand.rs src/manifest/render.rs src/manifest/diagnostics.rs
Update manifest tests for new module structure	Adjust imports to reference diagnostics::map_yaml_error and glob::{glob_paths, normalize_separators} Add new test cases for empty glob matches and unmatched braces Retain YAML diagnostics coverage via hints	src/manifest/tests.rs

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#156	Decompose src/manifest.rs into smaller submodules: manifest/diagnostics.rs, manifest/glob.rs, manifest/expand.rs, and manifest/render.rs, moving relevant code to each.	✅
#156	Retain main manifest structure definitions, public API surface, and module orchestration in src/manifest.rs.	✅
#156	Update tests and imports to use the new submodule structure, ensuring diagnostics and glob coverage remain intact.	✅

Possibly linked issues

• Refactor src/manifest.rs: Split module to satisfy ≤400-line rule #156: The PR directly implements the issue's proposed refactoring by splitting src/manifest.rs into glob, expand, render, and diagnostics submodules.
• Refactor src/manifest.rs: Split module to satisfy ≤400-line rule #156: The PR refactors src/manifest.rs by splitting it into diagnostics, glob, expand, and render submodules, as outlined in the issue.
• Refactor src/manifest.rs: Split module to satisfy ≤400-line rule #156: The PR extracts glob-specific helper functions from src/manifest.rs into src/manifest/glob.rs, directly addressing the issue's primary request, and further refactors other related functionalities.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

src/manifest/glob.rs

Comment on file

use minijinja::{Error, ErrorKind};

❌ New issue: Primitive Obsession
In this module, 48.8% of all function arguments are primitive types, threshold = 30.0%

[leynos]

@coderabbitai pause

[coderabbitai]

✅ Actions performed

Reviews paused.

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

src/manifest/glob.rs

Comment on lines +247 to +297

    fn process_character(
        &mut self,
        ch: char,
        pos: usize,
        pattern: &GlobPattern,
    ) -> std::result::Result<(), Error> {
        let context = CharContext {
            ch,
            position: pos,
            in_class: self.state.in_class,
            escaped: self.escaped,
        };

        if context.escaped {
            self.escaped = false;
            return Ok(());
        }

        if context.ch == char::from(0x5c) && self.state.escape_active {
            self.escaped = true;
            return Ok(());
        }

        if context.ch == '[' && !context.in_class {
            self.state.in_class = true;
            return Ok(());
        }

        if context.ch == ']' && context.in_class {
            self.state.in_class = false;
            return Ok(());
        }

        if context.ch == '}' && !context.in_class && self.state.depth == 0 {
            return Err(create_unmatched_brace_error(&GlobErrorContext {
                pattern: pattern.raw.clone(),
                error_char: context.ch,
                position: context.position,
                error_type: GlobErrorType::UnmatchedBrace,
            }));
        }

        if context.ch == '{' && !context.in_class {
            self.state.depth += 1;
            self.state.last_open_pos = Some(context.position);
        } else if context.ch == '}' && !context.in_class {
            self.state.depth -= 1;
        }

        Ok(())
    }

❌ New issue: Complex Method
BraceValidator.process_character has a cyclomatic complexity of 15, threshold = 9

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

src/manifest/glob.rs

Comment on file

            return Ok(());
        }

        if context.ch == '}' && !context.in_class && self.state.depth == 0 {

❌ New issue: Complex Conditional
BraceValidator.process_character has 1 complex conditionals with 2 branches, threshold = 2

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

src/manifest/render.rs

Comment on lines +103 to +182

    fn render_manifest_renders_targets_and_rules() -> Result<()> {
        let env = Environment::new();
        let mut target_vars = Vars::new();
        target_vars.insert("greet".into(), YamlValue::String("hello".into()));
        target_vars.insert("subject".into(), YamlValue::String("world".into()));
        target_vars.insert(
            "message".into(),
            YamlValue::String("{{ greet }} {{ subject }}".into()),
        );
        let target = Target {
            name: StringOrList::String("{{ message }}!".into()),
            recipe: Recipe::Command {
                command: "{{ message }}".into(),
            },
            sources: StringOrList::List(vec!["{{ subject }}.txt".into()]),
            deps: StringOrList::Empty,
            order_only_deps: StringOrList::List(vec!["{{ subject }}.meta".into()]),
            vars: target_vars,
            phony: false,
            always: false,
        };
        let rule = Rule {
            name: "example".into(),
            recipe: Recipe::Command {
                command: "{{ 2 + 2 }}".into(),
            },
            description: Some("{{ 1 + 1 }}".into()),
            deps: StringOrList::List(vec!["{{ message }}".into()]),
        };
        let mut manifest_vars = Vars::new();
        manifest_vars.insert("message".into(), YamlValue::String("hello world".into()));

        let manifest = NetsukeManifest {
            netsuke_version: Version::parse("1.0.0")?,
            vars: manifest_vars,
            rules: vec![rule],
            actions: Vec::new(),
            targets: vec![target],
            defaults: Vec::new(),
        };

        let rendered = render_manifest(manifest, &env)?;
        let rendered_target = rendered.targets.first().expect("rendered target");
        let vars = &rendered_target.vars;
        let message = vars
            .get("message")
            .and_then(|v| v.as_str())
            .expect("rendered message");
        assert_eq!(message, "hello world");
        match &rendered_target.name {
            StringOrList::String(s) => assert_eq!(s, "hello world!"),
            other => panic!("expected string name, got {other:?}"),
        }
        let rendered_sources = match &rendered_target.sources {
            StringOrList::List(items) => items.clone(),
            other => panic!("expected list sources, got {other:?}"),
        };
        assert_eq!(rendered_sources, vec!["world.txt".to_string()]);
        match &rendered_target.recipe {
            Recipe::Command { command } => assert_eq!(command, "hello world"),
            other => panic!("expected command recipe, got {other:?}"),
        }
        match &rendered_target.order_only_deps {
            StringOrList::List(items) => assert_eq!(items, &["world.meta".to_string()]),
            other => panic!("expected order-only deps list, got {other:?}"),
        }

        let rendered_rule = rendered.rules.first().expect("rendered rule");
        assert_eq!(rendered_rule.description.as_deref(), Some("2"));
        match &rendered_rule.recipe {
            Recipe::Command { command } => assert_eq!(command, "4"),
            other => panic!("expected command recipe, got {other:?}"),
        }
        match &rendered_rule.deps {
            StringOrList::List(items) => assert_eq!(items, &["hello world".to_string()]),
            other => panic!("expected deps list, got {other:?}"),
        }

        Ok(())
    }

❌ New issue: Large Method
tests.render_manifest_renders_targets_and_rules has 76 lines, threshold = 70

[leynos]

@sourcery-ai review

[leynos]

@coderabbitai please create a new GitHub issue for the following:

• The brace‐validation and escape logic in glob.rs is quite intricate—consider extracting shared helpers (like CharContext/BraceValidationState) into a smaller internal utility or leveraging a dedicated glob/brace-expansion crate to reduce maintenance overhead.

[leynos]

@coderabbitai GitHub

[leynos]

@coderabbitai please create a new GitHub issue for the following:

Review the use of ambient authority for directory access in glob.rs; if tighter sandboxing is possible (e.g. restricting to the manifest’s directory), it would improve security and least-privilege adherence.

[leynos]

@coderabbitai please create a new GitHub issue for the following:

src/manifest/glob.rs:47
<code_context>

+    IoError,
+}
+
+pub type GlobEntryResult = std::result::Result<std::path::PathBuf, glob::GlobError>;
+
+fn create_glob_error(context: &GlobErrorContext, details: Option<String>) -> Error {

</code_context>

<issue_to_address>
suggestion: Consider using type alias visibility more restrictively.

GlobEntryResult is only used within this module, so consider reducing its visibility to pub(crate) or removing pub to minimize the public API.

type GlobEntryResult = std::result::Result<std::path::PathBuf, glob::GlobError>;

</issue_to_address>

[leynos]

@coderabbitai Has the following now been resolved?

Ensure the top-level manifest module re-exports all moved functions (e.g. glob_paths, render_manifest, expand_foreach, map_yaml_error) so the public API remains exactly the same after splitting into submodules.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the main manifest.rs file for re-exports
echo "=== Main manifest.rs re-exports ==="
cat src/manifest.rs | grep -E "^pub use|^pub mod"

echo -e "\n=== Functions mentioned in the question ==="
echo "Looking for: glob_paths, render_manifest, expand_foreach, map_yaml_error"

echo -e "\n=== Checking glob.rs exports ==="
grep -n "pub.*glob_paths" src/manifest/glob.rs || echo "glob_paths not found as public in glob.rs"

echo -e "\n=== Checking render.rs exports ==="
grep -n "pub.*render_manifest" src/manifest/render.rs || echo "render_manifest not found as public in render.rs"

echo -e "\n=== Checking expand.rs exports ==="
grep -n "pub.*expand_foreach" src/manifest/expand.rs || echo "expand_foreach not found as public in expand.rs"

echo -e "\n=== Checking diagnostics.rs exports ==="
grep -n "pub.*map_yaml_error" src/manifest/diagnostics.rs || echo "map_yaml_error not found as public in diagnostics.rs"

echo -e "\n=== All public functions in submodules ==="
echo "--- glob.rs ---"
grep -n "^pub " src/manifest/glob.rs

echo -e "\n--- render.rs ---"
grep -n "^pub " src/manifest/render.rs

echo -e "\n--- expand.rs ---"
grep -n "^pub " src/manifest/expand.rs

echo -e "\n--- diagnostics.rs ---"
grep -n "^pub " src/manifest/diagnostics.rs

Length of output: 1268

---

@leynos Yes, this has been resolved! ✅

The main src/manifest.rs file correctly re-exports all the moved functions you mentioned:

pub use diagnostics::{ManifestError, map_yaml_error};
pub use glob::glob_paths;
pub use expand::expand_foreach;
pub use render::render_manifest;

Status check:

• ✅ glob_paths - re-exported from glob module
• ✅ render_manifest - re-exported from render module
• ✅ expand_foreach - re-exported from expand module
• ✅ map_yaml_error - re-exported from diagnostics module

Each function is properly declared as pub in its respective submodule and then re-exported at the top level, ensuring the public API remains exactly the same after the refactor. Users can continue calling these functions from the manifest module without any changes to their code.

The refactoring successfully maintains API compatibility whilst achieving better code organisation through the submodule structure.