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]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Warning

Your free Security trial is over. An organization admin can activate billing to continue.

---

_{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

[codescene-access]

No quality gates enabled for this code.