Lower target and action deps (3.14.3)#315
Conversation
Draft an execution plan for lowering target and action `deps` into a new implicit-dependency class on the IR `BuildEdge` and emitting them as Ninja `|` implicit dependencies between explicit inputs and `||` order-only deps. The plan records the cycle-participation decision (explicit inputs and implicit deps participate; order-only deps do not), defers rule-level `Rule.deps` rejection to roadmap item 3.14.6, and keeps action hashing unchanged because `implicit_deps` lives on `BuildEdge` rather than `Action`. Validation runs through `make check-fmt`, `make lint`, `make test`, `make markdownlint`, `make nixie`, and `coderabbit review --agent`. The plan is DRAFT and must be approved before implementation begins. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
OverviewImplements roadmap task 3.14.3: Lower target and action Key ChangesCore IR Implementation
Ninja Output
Recipe Handling
Testing & RefactoringCycle Detection Tests
New Test Coverage
Documentation
ValidationAll existing workspace tests, targeted test suites, linting, and formatting gates passed. No new external crates were added. WalkthroughThis PR implements the complete lowering of manifest ChangesImplicit dependencies implementation and lowering
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 15 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Introduce the `BuildEdge.implicit_deps` field and initialise it as empty at every existing construction site. Keep behaviour unchanged for this step so the later manifest lowering, cycle detection, and Ninja rendering changes can be reviewed separately.
Populate `BuildEdge.implicit_deps` from manifest `deps` while keeping
`sources` as the only explicit recipe input class.
Preserve documented `{{ ins }}` and `{{ outs }}` recipe placeholders
through manifest rendering so IR interpolation can substitute them from
explicit inputs and outputs before action hashing.
Traverse `BuildEdge.implicit_deps` alongside explicit inputs during IR cycle analysis while continuing to ignore order-only dependencies. Add regression coverage for implicit-only cycles, mixed cycles, missing implicit dependencies, and bounded small cycles without adding a new test dependency.
Emit `BuildEdge.implicit_deps` with Ninja's single-pipe separator between explicit inputs and order-only dependencies. Cover explicit, implicit-only, order-only, and phony action edge shapes so the separator ordering remains stable.
Add an end-to-end manifest fixture that lowers target and action `deps`
into IR implicit deps and renders them as Ninja single-pipe dependencies.
Assert that recipe interpolation still receives only explicit `sources`,
including the documented `{{ ins }}` placeholder path.
Record that manifest `sources` become explicit recipe inputs, `deps` become implicit dependencies, and order-only dependencies remain outside cycle detection. Align the user, developer, design, and formal-verification notes with the implemented IR and Ninja dependency classes.
Record the final deterministic validation state before CodeRabbit review and note the existing Markdown formatting exception that prevents using `make fmt` as an all-or-nothing gate for this branch.
Mark the implicit dependency lowering roadmap item complete after the implementation, deterministic gates, and CodeRabbit review all passed. Update the ExecPlan with the final review and validation state.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Mark the ExecPlan complete after pushing the branch and refreshing the draft pull request with the final validation and reviewer context.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Code Duplicationsrc/ir/cycle.rs: What lead to degradation?The module contains 2 functions with similar structure: tests.cycle_detector_records_missing_dependencies,tests.cycle_detector_records_missing_implicit_dependencies Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationsrc/ir/cycle.rs: What lead to degradation?The module contains 2 functions with similar structure: tests.cycle_detector_records_missing_dependencies,tests.cycle_detector_records_missing_implicit_dependencies Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
Remove duplicated `CycleDetector` setup from the missing dependency tests by sharing the assertion skeleton inside the cycle test module.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationsrc/ir/cycle.rs: What lead to degradation?The module contains 2 functions with similar structure: tests.cycle_detector_records_missing_dependencies,tests.cycle_detector_records_missing_implicit_dependencies Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please investigate the cause of the following issue using codegraph exploration and research, identify a fix and provide an AI coding agent prompt for the fix: |
This comment was marked as resolved.
This comment was marked as resolved.
Pass the test target map by reference so Clippy does not flag the helper for taking ownership of data it only inspects.
Reviewer's GuideImplements roadmap task 3.14.3 by introducing an explicit implicit-dependency class ( File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/ir/cmd_interpolate.rs" line_range="142-146" />
<code_context>
let ins_joined = ins.join(" ");
let outs_joined = outs.join(" ");
- let mut out = String::with_capacity(template.len());
+ let placeholder_template = template
+ .replace("{{ ins }}", &ins_joined)
+ .replace("{{ outs }}", &outs_joined);
+ let chars: Vec<char> = placeholder_template.chars().collect();
+ let mut out = String::with_capacity(placeholder_template.len());
let mut in_backticks = false;
let mut i = 0;
</code_context>
<issue_to_address>
**issue (bug_risk):** Template-level replacement of `{{ ins }}`/`{{ outs }}` ignores backtick-protected regions, unlike the rest of the interpolation logic.
Because the `.replace("{{ ins }}", …).replace("{{ outs }}", …)` runs before the backtick-aware scan, these placeholders now get expanded even inside backticks, unlike other substitutions that respect backtick-delimited “no interpolation” regions.
To preserve the existing escape semantics, consider handling `{{ ins }}`/`{{ outs }}` inside the `find_substitution`/scanning loop (or otherwise applying the same backtick rules to them) rather than doing a global replace first.
</issue_to_address>
### Comment 2
<location path="src/manifest/render.rs" line_range="101-110" />
<code_context>
env.render_str(tpl, ctx).with_context(what)
}
+fn render_recipe_str_with(
+ env: &Environment,
+ tpl: &str,
+ ctx: &Vars,
+ what: impl FnOnce() -> String,
+) -> Result<String> {
+ let mut recipe_ctx = ctx.clone();
+ recipe_ctx
+ .entry("ins".into())
+ .or_insert_with(|| ManifestValue::String("{{ ins }}".into()));
+ recipe_ctx
+ .entry("outs".into())
+ .or_insert_with(|| ManifestValue::String("{{ outs }}".into()));
+ render_str_with(env, tpl, &recipe_ctx, what)
+}
+
</code_context>
<issue_to_address>
**question:** Defaulting `ins`/`outs` to `"{{ ins }}"`/`"{{ outs }}"` makes it hard to represent those literals in recipes.
Because `ins`/`outs` are injected as the literal strings `"{{ ins }}"`/`"{{ outs }}"` and then substituted, recipes have no way to emit those exact literals in their output, even when using constructs like `{% raw %}`. If supporting literal `{{ ins }}`/`{{ outs }}` is needed, consider a different internal marker or a way to opt out/override this behavior so there is an escape hatch.
</issue_to_address>
### Comment 3
<location path="docs/execplans/3-14-3-lower-target-and-action-deps.md" line_range="13" />
<code_context>
+## Purpose / big picture
+
+Roadmap item `3.14.3` closes a long-standing gap in Netsuke's manifest contract.
+Today the AST `Target` struct accepts a `deps` field, documentation already
+describes it as Ninja's implicit-dependency class, and the user guide shows
+authors writing `deps: my_app` and `deps: [build/utils.h]`. None of that flows
</code_context>
<issue_to_address>
**issue (review_instructions):** AST is introduced without being expanded; expand it on first use (for example, “Abstract Syntax Tree (AST)”).
The execplan introduces “AST” without defining it. To follow the acronym guideline, expand it at first mention, for example: “Today the Abstract Syntax Tree (AST) `Target` struct accepts a `deps` field…”.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 4
<location path="docs/execplans/3-14-3-lower-target-and-action-deps.md" line_range="284" />
<code_context>
+ including the IR/Ninja module boundary.
+- `docs/ortho-config-users-guide.md`: configuration layering and localized
+ help support if any config or CLI surface changes are required.
+- `docs/rstest-bdd-users-guide.md`: BDD feature and step guidance.
+- `docs/rust-testing-with-rstest-fixtures.md`: fixture and parameterization
+ patterns for unit tests.
</code_context>
<issue_to_address>
**issue (review_instructions):** BDD is used without definition; please expand it on first use (for example, “behaviour-driven development (BDD)”).
The acronym “BDD” is introduced here without being defined. To comply with the acronym rule, expand it on first use, e.g. “behaviour-driven development (BDD) feature and step guidance.”
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| let placeholder_template = template | ||
| .replace("{{ ins }}", &ins_joined) | ||
| .replace("{{ outs }}", &outs_joined); | ||
| let chars: Vec<char> = placeholder_template.chars().collect(); | ||
| let mut out = String::with_capacity(placeholder_template.len()); |
There was a problem hiding this comment.
issue (bug_risk): Template-level replacement of {{ ins }}/{{ outs }} ignores backtick-protected regions, unlike the rest of the interpolation logic.
Because the .replace("{{ ins }}", …).replace("{{ outs }}", …) runs before the backtick-aware scan, these placeholders now get expanded even inside backticks, unlike other substitutions that respect backtick-delimited “no interpolation” regions.
To preserve the existing escape semantics, consider handling {{ ins }}/{{ outs }} inside the find_substitution/scanning loop (or otherwise applying the same backtick rules to them) rather than doing a global replace first.
| fn render_recipe_str_with( | ||
| env: &Environment, | ||
| tpl: &str, | ||
| ctx: &Vars, | ||
| what: impl FnOnce() -> String, | ||
| ) -> Result<String> { | ||
| let mut recipe_ctx = ctx.clone(); | ||
| recipe_ctx | ||
| .entry("ins".into()) | ||
| .or_insert_with(|| ManifestValue::String("{{ ins }}".into())); |
There was a problem hiding this comment.
question: Defaulting ins/outs to "{{ ins }}"/"{{ outs }}" makes it hard to represent those literals in recipes.
Because ins/outs are injected as the literal strings "{{ ins }}"/"{{ outs }}" and then substituted, recipes have no way to emit those exact literals in their output, even when using constructs like {% raw %}. If supporting literal {{ ins }}/{{ outs }} is needed, consider a different internal marker or a way to opt out/override this behavior so there is an escape hatch.
| ## Purpose / big picture | ||
|
|
||
| Roadmap item `3.14.3` closes a long-standing gap in Netsuke's manifest contract. | ||
| Today the AST `Target` struct accepts a `deps` field, documentation already |
There was a problem hiding this comment.
issue (review_instructions): AST is introduced without being expanded; expand it on first use (for example, “Abstract Syntax Tree (AST)”).
The execplan introduces “AST” without defining it. To follow the acronym guideline, expand it at first mention, for example: “Today the Abstract Syntax Tree (AST) Target struct accepts a deps field…”.
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
| including the IR/Ninja module boundary. | ||
| - `docs/ortho-config-users-guide.md`: configuration layering and localized | ||
| help support if any config or CLI surface changes are required. | ||
| - `docs/rstest-bdd-users-guide.md`: BDD feature and step guidance. |
There was a problem hiding this comment.
issue (review_instructions): BDD is used without definition; please expand it on first use (for example, “behaviour-driven development (BDD)”).
The acronym “BDD” is introduced here without being defined. To comply with the acronym rule, expand it on first use, e.g. “behaviour-driven development (BDD) feature and step guidance.”
Review instructions:
Path patterns: **/*.md
Instructions:
Define uncommon acronyms on first use.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ir/cmd_interpolate.rs`:
- Around line 142-146: Add an inline comment in the substitute function
clarifying the two-stage preprocessing: before building chars/out, note that
Stage 1 unconditionally replaces the literal "{{ ins }}" and "{{ outs }}" with
joined values (placeholder_template), and Stage 2 (the following logic) performs
backtick-aware $in/$out substitution; place this comment immediately above the
placeholder_template assignment (near the ins_joined/outs_joined code) so
readers understand the pipeline and ordering.
In `@src/manifest/render.rs`:
- Around line 101-116: Add a doc comment to render_recipe_str_with describing
that it clones the supplied context (Vars) and guarantees template variables
"ins" and "outs" by injecting default placeholder strings "{{ ins }}" and "{{
outs }}" when they are absent, so MiniJinja rendering can reference them before
later Ninja substitution; place the comment immediately above the
render_recipe_str_with signature and mention Environment, Vars, and that
render_str_with is called to perform the rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: df0d8079-1ab0-4937-b753-b959a33b61c3
📒 Files selected for processing (19)
docs/developers-guide.mddocs/execplans/3-14-3-lower-target-and-action-deps.mddocs/formal-verification-methods-in-netsuke.mddocs/netsuke-design.mddocs/roadmap.mddocs/users-guide.mdsrc/ir/cmd_interpolate.rssrc/ir/cycle.rssrc/ir/from_manifest.rssrc/ir/graph.rssrc/manifest/render.rssrc/ninja_gen.rstests/bdd/steps/ir.rstests/bdd_tests.rstests/data/implicit_deps.ymltests/features/ninja.featuretests/ir_from_manifest_tests.rstests/ir_tests.rstests/ninja_gen_tests.rs
| let placeholder_template = template | ||
| .replace("{{ ins }}", &ins_joined) | ||
| .replace("{{ outs }}", &outs_joined); | ||
| let chars: Vec<char> = placeholder_template.chars().collect(); | ||
| let mut out = String::with_capacity(placeholder_template.len()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding a comment explaining the preprocessing stage.
The substitution now operates in two stages: first replacing {{ ins }} and {{ outs }} placeholders (lines 142-144), then performing the existing backtick-aware $in/$out substitution. Add an inline comment clarifying this two-stage pipeline and noting that the {{ }} placeholders are replaced unconditionally before backtick detection.
📝 Suggested inline comment
fn substitute(template: &str, ins: &[String], outs: &[String]) -> String {
let ins_joined = ins.join(" ");
let outs_joined = outs.join(" ");
+ // Stage 1: Replace {{ ins }} and {{ outs }} placeholders unconditionally.
+ // Stage 2 (below) performs backtick-aware $in/$out substitution.
let placeholder_template = template
.replace("{{ ins }}", &ins_joined)
.replace("{{ outs }}", &outs_joined);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ir/cmd_interpolate.rs` around lines 142 - 146, Add an inline comment in
the substitute function clarifying the two-stage preprocessing: before building
chars/out, note that Stage 1 unconditionally replaces the literal "{{ ins }}"
and "{{ outs }}" with joined values (placeholder_template), and Stage 2 (the
following logic) performs backtick-aware $in/$out substitution; place this
comment immediately above the placeholder_template assignment (near the
ins_joined/outs_joined code) so readers understand the pipeline and ordering.
| fn render_recipe_str_with( | ||
| env: &Environment, | ||
| tpl: &str, | ||
| ctx: &Vars, | ||
| what: impl FnOnce() -> String, | ||
| ) -> Result<String> { | ||
| let mut recipe_ctx = ctx.clone(); | ||
| recipe_ctx | ||
| .entry("ins".into()) | ||
| .or_insert_with(|| ManifestValue::String("{{ ins }}".into())); | ||
| recipe_ctx | ||
| .entry("outs".into()) | ||
| .or_insert_with(|| ManifestValue::String("{{ outs }}".into())); | ||
| render_str_with(env, tpl, &recipe_ctx, what) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Document the helper's ins/outs injection behaviour.
Add a doc comment explaining that render_recipe_str_with ensures ins and outs template variables are always available by injecting default placeholder strings ("{{ ins }}" and "{{ outs }}") when absent, allowing recipe commands to reference these variables during MiniJinja rendering before subsequent Ninja substitution.
📝 Suggested doc comment
+/// Render a recipe command template with guaranteed `ins` and `outs` variables.
+///
+/// Clones the provided context and injects default `"{{ ins }}"` and `"{{ outs }}"`
+/// placeholder strings if not already present, ensuring recipe commands can always
+/// reference these variables during MiniJinja rendering. The placeholders are later
+/// substituted with actual paths during command interpolation.
fn render_recipe_str_with(
env: &Environment,
tpl: &str,
ctx: &Vars,
what: impl FnOnce() -> String,
) -> Result<String> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/manifest/render.rs` around lines 101 - 116, Add a doc comment to
render_recipe_str_with describing that it clones the supplied context (Vars) and
guarantees template variables "ins" and "outs" by injecting default placeholder
strings "{{ ins }}" and "{{ outs }}" when they are absent, so MiniJinja
rendering can reference them before later Ninja substitution; place the comment
immediately above the render_recipe_str_with signature and mention Environment,
Vars, and that render_str_with is called to perform the rendering.
1 participant
Summary
This branch implements roadmap task (3.14.3) by lowering target and action
depsinto a dedicated implicit-dependency class in the IR and generated Ninja output. It keepssourcesas the explicit recipe-input class used by$inand{{ ins }}, whiledepsnow affect dependency ordering, rebuild decisions, and cycle detection without appearing in recipe arguments.Roadmap task: (3.14.3)
Execplan: docs/execplans/3-14-3-lower-target-and-action-deps.md
Review walkthrough
BuildEdge.implicit_depsfield and manifest lowering fromtarget.deps.| <implicit_deps>rendering.{{ ins }}/{{ outs }}placeholder preservation needed by the documented recipe contract.Validation
cargo test --workspace: passed after the IR field addition.cargo test --test ir_from_manifest_tests: passed with 15 tests after manifest lowering.cargo test -p netsuke ir::cycle: passed after implicit-dependency cycle traversal.cargo test --test ninja_gen_tests: passed after Ninja emission updates.cargo test --test bdd_tests implicit: passed after behavioural coverage was added.make check-fmt: passed in the final gate.make lint: passed in the final gate.make test: passed in the final gate.make markdownlint: passed in the final gate.make nixie: passed in the final gate.coderabbit review --agent: completed with zero findings.Notes
make fmtstill triggers a pre-existing repository-wide Markdown formatting backlog and rewrites unrelated documentation before failing. This branch restored that unrelated churn and used the non-mutating gates above as the applicable completion checks.No new external crate dependencies were added. The planned
proptestcoverage was replaced with deterministic bounded cycle coverage becauseproptestis not currently in the dependency graph and the execplan requires explicit approval before adding dependencies.Summary by Sourcery
Lower manifest target and action
depsinto a dedicated implicit-dependency class in the IR and generated Ninja output, aligning cycle detection, recipe interpolation, tests, and documentation with the intended dependency semantics.New Features:
implicit_depson IR build edges to represent manifestdepsseparately from explicitsourcesinputs.depsfor both targets and actions as implicit dependencies that appear in generated Ninja files using Ninja's|syntax.Enhancements:
{{ ins }}and{{ outs }}placeholders through manifest rendering so recipe interpolation can expand only explicit sources and outputs.Tests:
depspopulate implicit dependencies and do not affect explicit recipe inputs.