Escape backend dollar syntax after Netsuke placeholder lowering (3.14.7) - #565
Draft
leynos wants to merge 3 commits into
Draft
Escape backend dollar syntax after Netsuke placeholder lowering (3.14.7)#565leynos wants to merge 3 commits into
leynos wants to merge 3 commits into
Conversation
Contributor
Reviewer's GuideAdds 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 escapingsequenceDiagram
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
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueWarning Your free Security trial is over. An organization admin can activate billing to continue. Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Draft 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 survive to the shell while the
IR stays free of Ninja-specific escaping.
Reconnaissance and an adversarial design review established several facts
that reshape the task beyond its roadmap wording:
- Two distinct failures exist today, not one. `$PATH` is silently erased
by Ninja's lexer; `${CARGO:-cargo}` is a hard parse error. Assertions
must distinguish them.
- `$in` and `$out` already work inside `script:` recipes by accident,
via Ninja's own built-ins, because `register_action` lowers only
command recipes. Escaping alone would regress every such script, so
script lowering must land first.
- A scalar command containing a newline injects raw Ninja syntax into
the generated file. The new escaping constructor is therefore fallible
and rejects control characters.
- Escaping commands while leaving path emission raw would make the
dependency edge and the command disagree for a path such as `input$1`,
which an existing fixture already uses.
The plan proposes a `ShellText` to `NinjaValue` seam that makes "escaped
exactly once" a compile-time property, sequences the work as four
milestones, and grounds verification in differential testing against the
real Ninja binary rather than a hand-written lexer model. Kani and Verus
are explicitly rejected with reasons.
Two decisions are marked as needing approval before implementation:
handling of `$in`/`$out` inside backtick regions, and whether the escape
extends to `description` and `depfile`.
Refs: docs/roadmap.md 3.14.7; netsuke-design.md 2.6, 5.4.
The rebase onto `origin/main` was clean, but "Add target descriptions and netsuke help targets" (#551) restructured `src/manifest/render.rs` and shifted every documentation section the plan cites, so the plan's file:line references no longer resolved. Description and recipe rendering are now the shared helpers `render_description` and `render_recipe`, each taking a `subject` for diagnostics. That makes the EP-M1 render change a single-arm edit to `render_recipe` rather than the two call sites the plan described. Targets also gained descriptions, consumed by the new `netsuke help targets` catalogue, while `src/ir/from_manifest.rs` deliberately keeps target descriptions out of the generated Ninja file. This strengthens decision `D-METADATA`: `description` now feeds a non-backend consumer, so applying a Ninja-specific transform to it would repeat the layering mistake the task exists to correct. The recommendation to scope escaping to command and script text stands. The core findings are untouched. `src/ir/from_manifest_support.rs:54` still passes script recipes through unlowered, so script `$in`/`$out` lowering must still precede escaping. Milestones, verification obligations, and the two open decisions are unchanged. The plan remains DRAFT pending approval.
leynos
force-pushed
the
3-14-7-escape-backend-dollar-syntax-after-netsuke-placeholder-lowering
branch
from
August 17, 2026 00:15
39a2090 to
546d45b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Lower Netsuke placeholders before converting completed shell text to a Ninja binding, so shell variables survive generation without coupling the IR to Ninja syntax. Reject ambiguous paths and control characters, require real Ninja coverage in CI, and document the migration from historical `$$` recipe spellings.
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| ninja_dollar_escaping_tests.rs | 1 advisory rule | 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
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") | ||
| } |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 2 functions with similar structure: ninja_commands,ninja_output
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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$RUSTFLAGSsurvive 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.mdThis 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
ninja1.11.1.command = echo PATH is $PATHis silently erased by Ninja's lexer and printsPATH 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.$inand$outalready work insidescript:recipes by accident, via Ninja's own built-in variables, becauseregister_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.printf %b 'echo HOME is \$HOME'executes asprintf %b 'echo HOME is \'— Ninja eats$HOMEand leaves a trailing backslash inside the quoted argument.command:containing a newline injects raw Ninja syntax into the generated file, creating targets the manifest never declared.shlex::splittreats\nas whitespace, so neither the IR validation nor theassert_shell_commanddebug guard rejects it. Command lists are guarded; scalar commands are not.input$1— whichtests/command_escaping_tests.rs:55already uses as a fixture.$at all, and it comes from the command-list wrapper rather than user text. The proptest generator atsrc/ninja_gen_property_tests.rs:177isecho [a-z]{1,12}, which cannot produce a$.Proposed approach
A
ShellText→NinjaValueseam in a newsrc/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_valuealso rejects control characters, closing the injection hole at the same seam.Four milestones, sequenced so each is a coherent plateau:
ninjadifferential oracle and the red regression matrix.$in/$outforscript:recipes (must precede escaping).command_list_entrydrops its hand-baked$$.Verification
The oracle is the real
ninjabinary (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::replacethen proving the axiom implies the specification.Decisions needing approval before implementation
D-BACKTICK—substitutepreserves backtick regions, socat `basename $in`leaves$inunlowered; after escaping the shell receives a literal$inand silently produces nothing. Recommendation: reject with a typed diagnostic rather than silently diverge.D-METADATA— whether to escapedescription,depfile,deps, andpool. Recommendation: no. Descriptions are never$in/$out-lowered, so escaping them alone removes the workingdescription = CC $outidiom and gives nothing back;depfile = $out.dis 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-manifestsand 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 onoriginata857fdecarrying 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 markdownlintpasses (82 files, 0 errors). Docs-only change; no Rust source touched, so the cargo gates are unaffected.References
docs/roadmap.md:212-219(3.14.7)docs/netsuke-design.md§§2.6 and 5.4🤖 Generated with Claude Code
Summary by Sourcery
Make Ninja backend emission preserve shell dollar syntax safely after Netsuke placeholder lowering.
New Features:
$inand$outplaceholders in script recipes before Ninja emission and reject unsupported backtick usage with diagnostics.Bug Fixes:
Enhancements:
CI:
Documentation:
$$workarounds, placeholder-lowering behaviour, and the new backend escaping boundary.Tests:
Chores: