Escape backend dollar syntax after Netsuke placeholder lowering (3.14.7)

[leynos]

Summary

Drafts the execution plan for roadmap task 3.14.7, which makes the Ninja backend escape residual literal dollars as $$ after Netsuke's own placeholder lowering, so shell variables such as $PATH, ${CARGO:-cargo}, and $RUSTFLAGS survive to the shell while the intermediate representation stays free of Ninja-specific escaping.

Plan: docs/execplans/3-14-7-escape-backend-dollar-syntax-after-netsuke-placeholder-lowering.md

This is a plan only. No production code changes. The plan requires approval before implementation begins.

What reconnaissance found

Reconnaissance and an adversarial design review established several facts that reshape the task beyond its roadmap wording. All were reproduced against ninja 1.11.1.

• Two distinct failures exist today, not one. command = echo PATH is $PATH is silently erased by Ninja's lexer and prints PATH is . ${CARGO:-cargo} is a hard parse error — bad $-escape (literal $ must be written as $$). A test matrix built only on "the variable came back empty" would miss the case the roadmap names explicitly.
• $in and $out already work inside script: recipes by accident, via Ninja's own built-in variables, because register_action (src/ir/from_manifest_support.rs:54) lowers only command recipes and passes scripts through unchanged. Escaping alone would regress every such script, so script lowering must land before escaping.
• The script wrapper is corrupted today, not merely the variable. printf %b 'echo HOME is \$HOME' executes as printf %b 'echo HOME is \' — Ninja eats $HOME and leaves a trailing backslash inside the quoted argument.
• A scalar command: containing a newline injects raw Ninja syntax into the generated file, creating targets the manifest never declared. shlex::split treats \n as whitespace, so neither the IR validation nor the assert_shell_command debug guard rejects it. Command lists are guarded; scalar commands are not.
• Escaping commands while leaving path emission raw would make the dependency edge and the command disagree for a path such as input$1 — which tests/command_escaping_tests.rs:55 already uses as a fixture.
• The existing snapshot corpus is blind here. Only one Ninja snapshot contains a $ at all, and it comes from the command-list wrapper rather than user text. The proptest generator at src/ninja_gen_property_tests.rs:177 is echo [a-z]{1,12}, which cannot produce a $.

Proposed approach

A ShellText → NinjaValue seam in a new src/ninja_gen_escape.rs, with private fields and a single fallible constructor, so "escaped exactly once" becomes a compile-time property rather than a review finding. escape_ninja_value also rejects control characters, closing the injection hole at the same seam.

Four milestones, sequenced so each is a coherent plateau:

1. EP-M0 — real-ninja differential oracle and the red regression matrix.
2. EP-M1 — lower $in/$out for script: recipes (must precede escaping).
3. EP-M2 — the escaping seam; command_list_entry drops its hand-baked $$.
4. EP-M3 — fallible path emission, converting corruption into a diagnostic.
5. EP-M4 — users' guide migration, design-doc update, ADR-011, roadmap tick.

Verification

The oracle is the real ninja binary (ninja -t commands), not a hand-written lexer model — a model would be written from the same mental model as the escaper, so a shared misconception would pass green, and it structurally cannot express "Ninja accepts this file". Seven obligations (I1–I7) are stated with non-vacuity controls and seeded faults for each.

Kani and Verus are explicitly rejected with reasons: the introduced function is a pure total string map, and a Verus proof would require axiomatising str::replace then proving the axiom implies the specification.

Decisions needing approval before implementation

1. D-BACKTICK — substitute preserves backtick regions, so cat `basename $in` leaves $in unlowered; after escaping the shell receives a literal $in and silently produces nothing. Recommendation: reject with a typed diagnostic rather than silently diverge.
2. D-METADATA — whether to escape description, depfile, deps, and pool. Recommendation: no. Descriptions are never $in/$out-lowered, so escaping them alone removes the working description = CC $out idiom and gives nothing back; depfile = $out.d is the canonical Ninja idiom that roadmap 3.14.6 will depend on. Scope to command and script text exactly as the approved design states, and record the gap as a follow-up.

Note on branch naming

The task brief asked for branch 3-14-5-regression-coverage-for-conditional-action-dependency-manifests and a (3.14.5) PR title, but the task body, the roadmap entry, and the requested plan filename are all 3.14.7. That 3.14.5 branch already exists on origin at a857fde carrying the separate 3.14.5 plan in #387, so pushing here would have collided with it. I treated the 3.14.5 naming as a stale carry-over and used a 3.14.7 branch. Happy to move it if that was wrong.

Validation

make markdownlint passes (82 files, 0 errors). Docs-only change; no Rust source touched, so the cargo gates are unaffected.

References

• Roadmap: docs/roadmap.md:212-219 (3.14.7)
• Design: docs/netsuke-design.md §§2.6 and 5.4
• Ninja manual, lexical syntax: https://ninja-build.org/manual.html
• Lody session: https://lody.ai/leynos/sessions/e6fb5970-3785-4215-b5b0-8d20c0ec4f14

🤖 Generated with Claude Code

Summary by Sourcery

Preserve shell dollar syntax through Ninja generation by lowering Netsuke placeholders first, escaping residual dollars at the backend boundary, and adding comprehensive validation and real-Ninja coverage.

New Features:

• Preserve ordinary shell dollar expressions in generated Ninja recipes while keeping placeholder lowering in the backend-neutral IR.
• Lower $in and $out placeholders consistently in script recipes and reject unsupported backtick usage with diagnostics.
• Add real-Ninja differential and execution coverage for scalar, script, command-list, and shell-variable behaviour.

Bug Fixes:

• Prevent Ninja from erasing or rejecting shell variable expressions in commands and scripts.
• Reject unsafe command control characters and unsupported path syntax before generating ambiguous or injectable Ninja files.
• Ensure command-list fixtures and runtime shell arithmetic use ordinary shell dollar syntax under the new backend escaping contract.

Enhancements:

• Introduce a typed shell-text to Ninja-value boundary so backend escaping is applied once after placeholder lowering.
• Add validation for metadata and path emission, plus clearer diagnostics for unsafe Ninja values and paths.

CI:

• Require the Ninja executable in CI integration coverage via NETSUKE_REQUIRE_NINJA=1.

Documentation:

• Document the backend escaping boundary, migration from historical $$ recipe syntax, script placeholder behaviour, and path restrictions.
• Add ADR-014 and mark roadmap item 3.14.7 complete.

Tests:

• Add real-Ninja regression, property, integration, and BDD coverage for dollar escaping, placeholder lowering, control characters, and unsafe paths.

Chores:

• Update documentation examples and fixtures to use ordinary shell syntax and newline-safe scalar commands.

[sourcery-ai]

Reviewer's Guide

Adds a detailed execution plan document for roadmap task 3.14.7 describing how to implement backend dollar escaping in the Ninja generator while preserving IR purity, including milestones, risks, verification strategy, and design decisions.

Sequence diagram for planned command generation with Ninja escaping

sequenceDiagram
    participant Manifest as Manifest
    participant IR as IrGraph
    participant CmdInterp as cmd_interpolate
    participant NinjaGen as ninja_gen
    participant Escape as ninja_gen_escape
    participant Ninja as ninja_binary

    Manifest->>CmdInterp: interpolate_command_with_bindings(template, bindings)
    CmdInterp-->>IR: Action.command(shell_text)

    IR->>NinjaGen: generate_into(graph, writer)
    NinjaGen->>Escape: escape_ninja_value(ShellText)
    Escape-->>NinjaGen: NinjaValue or NinjaGenError
    NinjaGen->>writer: write "command = " + NinjaValue

    NinjaGen->>Ninja: build.ninja
    Ninja-->>NinjaGen: parses & expands dollars correctly

Loading

File-Level Changes

Change	Details	Files
Introduce an ExecPlan markdown document for roadmap item 3.14.7 that specifies how to escape Ninja backend dollar syntax after Netsuke placeholder lowering, including sequencing of milestones, design constraints, risks, verification obligations, and decisions requiring approval.	Create docs/execplans/3-14-7-escape-backend-dollar-syntax-after-netsuke-placeholder-lowering.md with a full execution plan for implementing backend dollar escaping. Document current dollar-handling failures in Ninja, including silent variable erasure and bad $-escape parse errors. Define the ShellText→NinjaValue escaping seam concept, planned new module, and fallible constructor semantics to ensure dollars are escaped exactly once and control characters are rejected. Lay out milestones EP-M0–EP-M4 covering test matrix creation, script placeholder lowering, escaping implementation, fallible path emission, and documentation/ADR updates. Record risks (e.g., script regression, build-file injection via newlines, path/command disagreement), and a verification plan with specific test obligations (I1–I7) using the real ninja binary as oracle. Capture open design decisions needing approval (backtick handling and whether to escape metadata fields like description/depfile) and branch-naming rationale.	docs/execplans/3-14-7-escape-backend-dollar-syntax-after-netsuke-placeholder-lowering.md

---

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.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

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

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: cc613e31-461b-492c-bcf6-cb6838760574

📥 Commits

Reviewing files that changed from the base of the PR and between 50e8711 and e728f67.

📒 Files selected for processing (14)

• README.md
• docs/debugging/debugging-plan-2026-08-27-serial-runtime-test.md
• docs/execplans/3-14-7-escape-backend-dollar-syntax-after-netsuke-placeholder-lowering.md
• docs/users-guide.md
• src/ir/cmd_interpolate.rs
• src/ir/cmd_interpolate_property_tests.rs
• src/ninja_gen/mod.rs
• src/ninja_gen_error.rs
• src/ninja_gen_escape.rs
• src/ninja_gen_property_tests.rs
• src/ninja_gen_property_tests/ninja_oracle.rs
• src/ninja_gen_tests.rs
• src/runner/dyndep_generation_telemetry.rs
• tests/ninja_dollar_escaping_tests.rs

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• leynos/monotony (auto-detected)
• leynos/rstest-bdd (auto-detected)
• leynos/whitaker (auto-detected)
• leynos/ortho-config (auto-detected)
• leynos/lading (auto-detected)
• leynos/shared-actions (auto-detected)

💤 Files with no reviewable changes (2)

• src/runner/dyndep_generation_telemetry.rs
• src/ninja_gen_error.rs

Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

---

Summary

• Add a typed ShellText to NinjaValue escaping seam.
• Escape residual dollar signs for Ninja while preserving shell variables and $in/$out placeholders.
• Lower script placeholders before backend escaping.
• Reject unsafe control characters, metadata values, and unsupported Ninja path syntax.
• Add real-Ninja, child-shell, BDD, property, and cross-platform coverage.
• Require Ninja integration coverage in Linux CI through NETSUKE_REQUIRE_NINJA=1.
• Document the design in ADR 014.
• Record implementation details in the new 3.14.7 execplan.
• Mark roadmap task 3.14.7 as complete.

Walkthrough

The change moves Ninja dollar escaping to the backend boundary. It adds typed validation for shell text, metadata, and paths. Recipe lowering, real-Ninja tests, integration fixtures, documentation, and CI coverage now follow this contract.

Changes

Ninja escaping pipeline

Layer / File(s)	Summary
Recipe and script placeholder lowering src/ir/..., src/manifest/render.rs	Commands and scripts resolve placeholders before Ninja generation. Backtick placeholders are rejected.
Ninja value and path boundary src/ninja_gen/...	ShellText converts to escaped NinjaValue. Unsafe control characters, metadata, and Ninja-special path characters are rejected with typed errors.
Shell wrapper expansion syntax src/ninja_gen_command_list.rs	Generated wrappers use single-dollar shell expansions before backend escaping.
Real-Ninja and regression verification tests/ninja_dollar_escaping_tests.rs, src/ninja_gen_property_tests*, tests/bdd/..., tests/*	Tests execute generated Ninja files and verify dollar expansion, placeholder lowering, script handling, and rejection cases.
Escaping contract and CI enforcement docs/..., .github/workflows/ci.yml, test_support/src/ninja.rs	Documentation records the escaping seam and migration rules. CI can require Ninja integration coverage. NETSUKE_REQUIRE_NINJA=1 makes missing Ninja fail the Linux job.

Sequence Diagram(s)

sequenceDiagram
  participant Manifest
  participant IR
  participant NamedAction
  participant Ninja
  participant Shell
  Manifest->>IR: resolve command or script placeholders
  IR->>NamedAction: provide completed shell text
  NamedAction->>Ninja: escape dollars and write build rule
  Ninja->>Shell: execute generated command
  Shell-->>Ninja: return expanded command output

Loading

Poem

Raw dollars cross the recipe bright
Ninja doubles them in flight
Paths reject unsafe signs
Shells receive their proper lines
Real tests watch the output glow

Merge Risk: 🔵 Low · up to e728f

The PR changes Ninja dollar escaping and script placeholder handling, while its debugging plan still contains conflicting remediation guidance and its migration documentation overstates which script placeholders remain compatible. The bounded risk is user or maintainer confusion, so the change is mergeable with explicit owner follow-up.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error, 4 warnings)

Check name	Status	Explanation	Resolution
Testing (Overall)	❌ Error	The new tests cover the main Ninja escaping paths well, but they do not cover all changed behaviour. src/manifest/render.rs:104-132 now applies the reserved ins/outs recipe context to scripts. T…	Add a manifest rendering test that defines ins and outs in target variables, renders a Recipe::Script containing {{ ins }} and {{ outs }}, and asserts that both values become INS_TOKEN and OUTS_TOKEN while ordinary variables s…
User-Facing Documentation	⚠️ Warning	The users' guide documents ordinary shell-dollar syntax, placeholder lowering, backtick rejection, path restrictions, metadata control characters, and the $$PATH migration. It does not document all …	Update docs/users-guide.md in the safety-boundary section. State that command and script text containing newline, carriage-return, or NUL characters is rejected during Ninja generation. State how literal $ characters in description, `…
Developer Documentation	⚠️ Warning	The pull request documents the main architecture and updates the roadmap, but the documentation is not fully current. The new ADR-014 was introduced as accepted, then its decision was changed in a lat…	Add a dated addendum to ADR-014 that records the metadata-decision change and preserves the decision history. Change the ADR implementation reference to src/ninja_gen/mod.rs. Update the ExecPlan's current-state and conformance references …
Testing (Compile-Time / Ui)	⚠️ Warning	Fail the compile-time testing requirement. The pull request introduces a Rust compile-time seam: ShellText has no Display, NinjaValue has a private field and no public constructor, and `escape_n…	Add a Rust compile-fail UI test, using the repository's direct-rustc harness where trybuild is unsuitable, and compile it against the actual src/ninja_gen_escape.rs module. Assert that a second escape_ninja_value call is rejected beca…
Performance And Resource Use	⚠️ Warning	The PR adds avoidable per-entry allocation and scanning in the manifest-to-IR path. interpolate_command_with_bindings now calls has_placeholder_in_backticks, which allocates a Vec<char> and scan…	Combine backtick detection and placeholder substitution into one pass, or reuse one character buffer, so ordinary commands do not allocate and scan the same text twice. Prepare escaped metadata once before emitting an action, then reuse tho…

✅ Passed checks (15 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the backend dollar-escaping implementation and references roadmap task 3.14.7. It does not incorrectly describe the work as planning.
Description check	✅ Passed	The description clearly relates to the dollar-escaping work and roadmap task 3.14.7. However, it incorrectly states that the pull request is plan-only and contains no production changes, while the cha…
Docstring Coverage	✅ Passed	Docstring coverage is 89.58% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 96 functions across 23 files. (4 skipped: 4…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Module-Level Documentation	✅ Passed	Pass the module-level documentation check. The PR's new Rust modules, including src/ninja_gen_escape.rs and src/ninja_gen_property_tests/ninja_oracle.rs, begin with //! documentation that states…
Testing (Unit And Behavioural)	✅ Passed	Pass the testing check. The change adds local tests for dollar escaping, metadata escaping and rejection, unsafe control characters, path validation, empty recipes, command-list errors, and output inv…
Testing (Property / Proof)	✅ Passed	Pass the check. The change introduces range-based invariants for Ninja escaping and placeholder lowering, and it adds Rust proptest coverage. The 128-case scalar property generates dollar-bearing an…
Unit Architecture	✅ Passed	PASS — the changed production code keeps the boundaries explicit. generate and generate_bundle transform an in-memory BuildGraph into owned text and do not access the environment, filesystem, ne…
Domain Architecture	✅ Passed	Keep the boundary. The changed IR code performs Netsuke placeholder lowering for $in/$out and returns domain IrGenError values; it does not import ninja_gen, access environment variables, spaw…
Observability	✅ Passed	Pass the observability check. The new Ninja failure path is covered at the runner generation boundary. instrument_bundle_generation records bounded action, target, and dependency counts, outcome, du…
Security And Privacy	✅ Passed	PASS — The pull request introduces no secrets, credentials, permission changes, or authentication or authorization changes. The new Ninja boundary validates and escapes residual shell dollars, rejects…
Concurrency And State	✅ Passed	Pass the concurrency and state check. Keep the new state isolated: NinjaCommandOracle owns one TempDir and capability-scoped Dir, each BDD scenario owns its TestWorld, and child Ninja processe…
Architectural Complexity And Maintainability	✅ Passed	Accept the architectural changes. The new ShellText → NinjaValue seam solves a real layering and invariant problem: private fields, no Display for ShellText, and the consuming `escape_ninja_va…
Rust Compiler Lint Integrity	✅ Passed	Accept the Rust changes. The PR diff adds no #[allow(dead_code)], #[allow(unused_imports)], #[allow(unused)], or equivalent broad suppression. The only #[expect] attributes in the changed Ninj…

Full details: Description check

Explanation

The description clearly relates to the dollar-escaping work and roadmap task 3.14.7. However, it incorrectly states that the pull request is plan-only and contains no production changes, while the changeset implements the feature and adds tests, CI updates, and documentation.

Full details: Docstring Coverage

Explanation

Docstring coverage is 89.58% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 96 functions across 23 files. (4 skipped: 4 unsupported.)

Full details: Testing (Overall)

Explanation

The new tests cover the main Ninja escaping paths well, but they do not cover all changed behaviour. src/manifest/render.rs:104-132 now applies the reserved ins/outs recipe context to scripts. The script test in src/manifest/render_tests.rs:241-280 only uses {{ subject }}, so reverting scripts to the old full-variable context would still pass. The command-list test covers reserved-binding precedence, but no equivalent script test exists. The new NETSUKE_REQUIRE_NINJA behaviour in test_support/src/ninja.rs:74-90 also has no test for the unavailable-Ninja failure path; the integration tests only exercise it when Ninja is available. The scalar control-character test covers newline and carriage return, but not NUL through the recipe emission path.

Resolution

Add a manifest rendering test that defines ins and outs in target variables, renders a Recipe::Script containing {{ ins }} and {{ outs }}, and asserts that both values become INS_TOKEN and OUTS_TOKEN while ordinary variables still render. Add deterministic unit coverage for NETSUKE_REQUIRE_NINJA=1, including the unavailable-probe path, by injecting the probe or using a controlled fake executable; assert that required mode fails rather than permits a skip, and that non-required mode retains the skip result. Add scalar and script NUL cases to the Ninja-value rejection matrix. Run the expanded tests with Ninja available and unavailable.

Full details: User-Facing Documentation

Explanation

The users' guide documents ordinary shell-dollar syntax, placeholder lowering, backtick rejection, path restrictions, metadata control characters, and the $$PATH migration. It does not document all changed user-facing behaviour. The new escape_ninja_value path rejects newline, carriage-return, and NUL characters in assembled scalar commands and scripts, but the guide limits this statement to emitted metadata. The new metadata emission also doubles literal dollars, but the guide documents only metadata control-character rejection. These behaviours are introduced by the pull request and are covered only in developer-facing documentation and tests.

Resolution

Update docs/users-guide.md in the safety-boundary section. State that command and script text containing newline, carriage-return, or NUL characters is rejected during Ninja generation. State how literal $ characters in description, depfile, deps, and pool metadata are handled, and distinguish this from shell command and script placeholder lowering. Update the migration guide entry if needed so the release migration summary matches the expanded user-facing behaviour.

Full details: Developer Documentation

Explanation

The pull request documents the main architecture and updates the roadmap, but the documentation is not fully current. The new ADR-014 was introduced as accepted, then its decision was changed in a later commit without a logged addendum. Its implementation reference also links to the non-existent src/ninja_gen.rs; the implementation is in src/ninja_gen/mod.rs. The new ExecPlan remains marked complete but its current-state section still describes src/ninja_gen.rs, NamedAction's Display implementation, and pre-change script lowering, which contradict the current code. These are introduced documentation defects.

Resolution

Add a dated addendum to ADR-014 that records the metadata-decision change and preserves the decision history. Change the ADR implementation reference to src/ninja_gen/mod.rs. Update the ExecPlan's current-state and conformance references to the current module layout, method names, and script-lowering behaviour, or clearly mark the obsolete passages as historical.

Full details: Module-Level Documentation

Explanation

Pass the module-level documentation check. The PR's new Rust modules, including src/ninja_gen_escape.rs and src/ninja_gen_property_tests/ninja_oracle.rs, begin with //! documentation that states their purpose and utility. A repository-wide scan found leading module documentation in all 520 tracked Rust files and documentation in all 66 inline module bodies.

Full details: Testing (Unit And Behavioural)

Explanation

Pass the testing check. The change adds local tests for dollar escaping, metadata escaping and rejection, unsafe control characters, path validation, empty recipes, command-list errors, and output invariants. Property tests cover scalar shell syntax, braced expansions, command-list ordering, and serial graph invariants. Real-Ninja tests exercise generated-file parsing, shell execution for scalar, script, and command-list recipes, placeholder lowering, heredocs, environment defaults, backtick rejection, unsafe input, and dollar-free output. The BDD scenario also runs a generated target and checks its output, so the behavioural coverage uses the external Ninja boundary rather than only private helpers.

Full details: Testing (Property / Proof)

Explanation

Pass the check. The change introduces range-based invariants for Ninja escaping and placeholder lowering, and it adds Rust proptest coverage. The 128-case scalar property generates dollar-bearing and ${...} commands, builds graphs, and compares ninja -t commands output with the pre-escaped command text. Additional property tests cover backtick placeholder rejection, command-list boundaries, and serial dependency cases. The private ShellText to NinjaValue boundary provides the structural exactly-once guarantee. No new lemma or formal proof assumption requires exhaustive proof; the documented Ninja lexical rules are external behaviour tested with the real Ninja binary.

Full details: Testing (Compile-Time / Ui)

Explanation

Fail the compile-time testing requirement. The pull request introduces a Rust compile-time seam: ShellText has no Display, NinjaValue has a private field and no public constructor, and escape_ninja_value(ShellText) consumes its input. The execution plan explicitly defines double escaping as a compile-time property and states that escape_ninja_value(escape_ninja_value(x)) must fail to compile. The changed test inventory contains runtime, property, real-Ninja, and focused text assertions, but no trybuild or equivalent compile-fail fixture for this seam. Existing UI tests cover unrelated contracts.

Resolution

Add a Rust compile-fail UI test, using the repository's direct-rustc harness where trybuild is unsuitable, and compile it against the actual src/ninja_gen_escape.rs module. Assert that a second escape_ninja_value call is rejected because it does not accept NinjaValue/the first call's result, and assert that ShellText cannot be formatted directly. Add a compiling control fixture so the harness cannot pass because of broken wiring. Keep the existing focused Ninja output assertions and retained snapshot coverage for stable generated text.

Full details: Unit Architecture

Explanation

PASS — the changed production code keeps the boundaries explicit. generate and generate_bundle transform an in-memory BuildGraph into owned text and do not access the environment, filesystem, network, clock, or processes. generate_into and NamedAction::write_into take an explicit writer and return Result. Recipe, metadata, path validation, and ShellText to NinjaValue conversion also return explicit errors. Manifest and IR mutation use explicit mutable inputs. Real-Ninja work is confined to clearly named test helpers, uses isolated temporary workspaces, and exposes setup and process failures through Result; the CI-only requirement is documented and deliberate. The added tests exercise the real Ninja boundary and verify validation before output.

Full details: Domain Architecture

Explanation

Keep the boundary. The changed IR code performs Netsuke placeholder lowering for $in/$out and returns domain IrGenError values; it does not import ninja_gen, access environment variables, spawn processes, or perform filesystem I/O. The Ninja-specific ShellText to NinjaValue conversion, path validation, metadata validation, and NinjaGenError remain inside ninja_gen. The dependency direction is one-way: the Ninja adapter consumes IR types. Process, environment, and temporary-workspace code remains in runner, test-support, or test modules. No stated domain-architecture failure condition is introduced.

Full details: Observability

Explanation

Pass the observability check. The new Ninja failure path is covered at the runner generation boundary. instrument_bundle_generation records bounded action, target, and dependency counts, outcome, duration, and a stable error_category; the new UnsafeNinjaValue variant maps to unsafe_ninja_value. The failure is also returned through the existing human and JSON diagnostics. Telemetry excludes command text, paths, identifiers, and generated content, so it avoids secrets and unbounded labels. No new service, network, queue, or asynchronous boundary requires tracing or alerts.

Full details: Security And Privacy

Explanation

PASS — The pull request introduces no secrets, credentials, permission changes, or authentication or authorization changes. The new Ninja boundary validates and escapes residual shell dollars, rejects newline, carriage-return, and NUL values before binding emission, and rejects unsafe graph-path characters. Command-list failure context hashes action identifiers, and UnsafeNinjaValue telemetry records only a category. New integration tests use env_clear() and the clearly fake NETSUKE_TEST_SENTINEL=sentinel-value; no host secret values enter test commands or logs. The CI change adds only NETSUKE_REQUIRE_NINJA=1 and retains contents: read. The added documentation contains test placeholders and build-debug details, but no personal, customer, tenant, credential, or operationally sensitive data that matches the check's failure conditions.

Full details: Performance And Resource Use

Explanation

The PR adds avoidable per-entry allocation and scanning in the manifest-to-IR path. interpolate_command_with_bindings now calls has_placeholder_in_backticks, which allocates a Vec<char> and scans the full template, then substitute allocates a second Vec<char> and scans it again. This runs for every scalar command and every command-list entry, whereas the base revision performed only the latter conversion. The new metadata path also validates each value in validate_action_metadata, then validates it again in escape_metadata_value, while allocating owned strings for each present field. These are changed-code regressions against the explicit unnecessary-allocation and repeated-work conditions. The real-Ninja property test is bounded to 128 cases and its process use is intentional and documented, so it is not the failure basis.

Resolution

Combine backtick detection and placeholder substitution into one pass, or reuse one character buffer, so ordinary commands do not allocate and scan the same text twice. Prepare escaped metadata once before emitting an action, then reuse those values and remove the separate validation pass; keep the fallible pre-write behaviour so invalid metadata cannot produce partial output. Add a representative large command-list and metadata workload check, or a benchmark, to verify linear work and bounded temporary memory.

Full details: Concurrency And State

Explanation

Pass the concurrency and state check. Keep the new state isolated: NinjaCommandOracle owns one TempDir and capability-scoped Dir, each BDD scenario owns its TestWorld, and child Ninja processes use per-command environments with env_clear(). No PR-added global mutable state, cache, singleton, shared registry, async task, lock, or unowned background worker exists. The command-list change preserves the existing current-shell and fail-fast model, documents background-PID handling, and retains real-Ninja tests for background waiting, failure short-circuiting, shared shell state, and serial ordering. The repository diff provides no changed concurrent interleaving or shared-state path that meets an explicit failure condition.

Full details: Architectural Complexity And Maintainability

Explanation

Accept the architectural changes. The new ShellText → NinjaValue seam solves a real layering and invariant problem: private fields, no Display for ShellText, and the consuming escape_ninja_value call enforce one Ninja escaping boundary. The seam has immediate consumers in command and metadata emission, with its contract documented in the ADR and developer guide. Keep path values separate, as the implementation does. The recipe helpers and the focused NinjaCommandOracle reduce duplication for their concrete use cases. The shared Ninja probe extends the existing test-support helper. No new third-party dependencies, generic traits, registries, global state, lifecycle hooks, or circular module edges were introduced. The new modules remain private to the Ninja adapter or its tests, and the IR remains independent of Ninja-specific escaping.

Full details: Rust Compiler Lint Integrity

Explanation

Accept the Rust changes. The PR diff adds no #[allow(dead_code)], #[allow(unused_imports)], #[allow(unused)], or equivalent broad suppression. The only #[expect] attributes in the changed Ninja writer pre-date the PR and target narrow Clippy diagnostics. New helpers, imports, types, fields, and re-exports have concrete call sites in the production path or test harness. The added clones express ownership boundaries: ShellText and NinjaValue own emitted text, metadata and paths return owned strings, invalid-command errors own their payload, and oracle work keeps independent command inputs. The temporary-directory fields use underscore names to keep resources alive, which is a real lifetime requirement. No artificial lint anchors or stale helper surfaces appear in the changed Rust code.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 3-14-7-escape-backend-dollar-syntax-after-netsuke-placeholder-lowering

Warning

Your free Security trial is over. An organization admin can activate Security or dismiss this notice.

---

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

[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. 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 in a code block that I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

tests/ninja_dollar_escaping_tests.rs

Comment on lines +56 to +80

fn ninja_commands(ninja_file: &str, target: &str) -> Result<String> {
    let workspace = required_ninja_workspace()?;
    let path = Utf8PathBuf::from_path_buf(workspace.path().to_path_buf())
        .map_err(|non_utf8| anyhow::anyhow!("non-UTF-8 temporary path: {non_utf8:?}"))?;
    let directory = Dir::open_ambient_dir(&path, ambient_authority())
        .with_context(|| format!("open Ninja workspace {path}"))?;
    directory
        .write("build.ninja", ninja_file)
        .context("write generated Ninja file")?;

    let output = Command::new("ninja")
        .args(["-f", "build.ninja", "-t", "commands", target])
        .current_dir(path.as_std_path())
        .env_clear()
        .env(SENTINEL, SENTINEL_VALUE)
        .output()
        .context("run Ninja command oracle")?;
    if !output.status.success() {
        bail!(
            "Ninja rejected generated file: {}",
            String::from_utf8_lossy(&output.stderr)
        );
    }
    String::from_utf8(output.stdout).context("Ninja command output was not UTF-8")
}

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: ninja_commands,ninja_output

[leynos]

@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:

        FAIL [   0.344s] (1181/2418) netsuke-build::bdd_tests features_scenarios::ninja_shell_variables_stay_visible_to_the_child_shell
  stdout ───

    running 1 test
    test features_scenarios::ninja_shell_variables_stay_visible_to_the_child_shell ... FAILED

    failures:

    failures:
        features_scenarios::ninja_shell_variables_stay_visible_to_the_child_shell

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 255 filtered out; finished in 0.34s
    
  stderr ───

    thread 'features_scenarios::ninja_shell_variables_stay_visible_to_the_child_shell' (33550) panicked at tests/bdd_tests.rs:20:1:
    Step failed at index ⁨2⁩: ⁨When⁩ ⁨the generated Ninja target "dollar-output" is run with sentinel "sentinel-value"⁩ - ⁨Error executing step '⁨the generated Ninja target {target:string} is run with sentinel {value:string}⁩' via function '⁨run_generated_ninja_target⁩': ⁨run generated Ninja target⁩⁩ (feature: ⁨/home/runner/work/netsuke/netsuke/tests/features/ninja.feature⁩, scenario: ⁨Shell variables stay visible to the child shell⁩)
    stack backtrace:
       0: __rustc::rust_begin_unwind
       1: core::panicking::panic_fmt
       2: core::panicking::panic_display::<alloc::string::String>
       3: bdd_tests::features_scenarios::ninja_shell_variables_stay_visible_to_the_child_shell::ninja_shell_variables_stay_visible_to_the_child_shell
       4: bdd_tests::features_scenarios::ninja_shell_variables_stay_visible_to_the_child_shell
       5: bdd_tests::features_scenarios::ninja_shell_variables_stay_visible_to_the_child_shell::{closure#0}
       6: <bdd_tests::features_scenarios::ninja_shell_variables_stay_visible_to_the_child_shell::{closure#0} as core::ops::function::FnOnce<()>>::call_once
    note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

  Cancelling due to test failure: 3 tests still running
        PASS [   0.323s] (1182/2418) netsuke-build::bdd_tests features_scenarios::novice_flows_help_flag_output_matches_the_documented_journey
        PASS [   0.228s] (1183/2418) netsuke-build::bdd_tests features_scenarios::novice_flows_help_subcommand_output_matches_the_flag_form
        PASS [   0.219s] (1184/2418) netsuke-build::bdd_tests features_scenarios::novice_flows_missing_manifest_shows_guided_failure
────────────
     Summary [  19.910s] 1184/2418 tests run: 1183 passed, 1 failed, 3 skipped
        FAIL [   0.344s] (1181/2418) netsuke-build::bdd_tests features_scenarios::ninja_shell_variables_stay_visible_to_the_child_shell
warning: 1234/2418 tests were not run due to test failure (run with --no-fail-fast to run all tests, or run with --max-fail)
error: test run failed

[leynos]

@coderabbitai please assess the implementation in this PR for completeness and correctness against the execplan:

docs/execplans/3-14-7-escape-backend-dollar-syntax-after-netsuke-placeholder-lowering.md

[leynos]

@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:

        FAIL [   0.851s] ( 549/2184) netsuke-build ninja_gen::property_tests::scalar_command_output_matches_ninja_oracle
  stdout ───

    running 1 test
    test ninja_gen::property_tests::scalar_command_output_matches_ninja_oracle ... FAILED

    failures:

    failures:
        ninja_gen::property_tests::scalar_command_output_matches_ninja_oracle

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 900 filtered out; finished in 0.82s
    
  stderr ───
    proptest: Saving this and future failures in D:\a\netsuke\netsuke\src\ninja_gen\proptest-regressions\ninja_gen_property_tests.txt
    proptest: If this test was run on a CI system, you may wish to add the following line to your copy of the file. (You may need to create it.)
    cc 2de21c2a9555677253ace8d0285ed8d57488de5d7d2c34608decbe269bea7ce3

    thread 'ninja_gen::property_tests::scalar_command_output_matches_ninja_oracle' (3964) panicked at src\ninja_gen\..\ninja_gen_property_tests.rs:197:1:
    Test failed: assertion failed: `(left == right)` 
      left: `Some("echo plain\r")`,
     right: `Some("echo plain")` at src\ninja_gen\..\ninja_gen_property_tests.rs:276.
    minimal failing input: (command, braced_command) = (
        "echo plain",
        "echo plain ${value:-fallback}",
    )
    	successes: 0
    	local rejects: 0
    	global rejects: 0

    stack backtrace:
       0: std::panicking::panic_handler
                 at /rustc/f28ac764c36004fa6a6e098d15b4016a838c13c6/library\std\src\panicking.rs:678
       1: core::panicking::panic_fmt
                 at /rustc/f28ac764c36004fa6a6e098d15b4016a838c13c6/library\core\src\panicking.rs:80
       2: netsuke::runner::tests::query_loader_rejects_effectful_template_helpers
       3: netsuke::ninja_gen::property_tests::scalar_command_output_matches_ninja_oracle::{closure#0}
       4: <netsuke::ninja_gen::property_tests::scalar_command_output_matches_ninja_oracle::{closure#0} as core::ops::function::FnOnce<()>>::call_once
       5: core::ops::function::FnOnce::call_once
                 at /rustc/f28ac764c36004fa6a6e098d15b4016a838c13c6/library\core\src\ops\function.rs:250
    note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

  Cancelling due to test failure: 3 tests still running
        PASS [   0.017s] ( 550/2184) netsuke-build runner::process::command_list_telemetry::tests::attributed_failure_records_bounded_outcome_and_duration
        PASS [   0.019s] ( 551/2184) netsuke-build runner::process::command_env::tests::differently_cased_keys_denote_one_variable
        PASS [   4.124s] ( 552/2184) netsuke-build manifest::tests::workspace::from_path_uses_manifest_directory_for_caches
────────────
     Summary [   9.995s] 552/2184 tests run: 551 passed, 1 failed, 2 skipped
        FAIL [   0.851s] ( 549/2184) netsuke-build ninja_gen::property_tests::scalar_command_output_matches_ninja_oracle
warning: 1632/2184 tests were not run due to test failure (run with --no-fail-fast to run all tests, or run with --max-fail)

https://github.com/leynos/netsuke/actions/runs/33030842021/job/98382749611?pr=565

[codescene-access]

No quality gates enabled for this code.