Conversation
Treat `$in` and `$out` in script recipes as shell variables so the Ninja backend doubles their dollars instead of expanding Ninja markers. Keep Netsuke markers and internal tokens distinct in code and tests.
Define markers, internal tokens, and shell variables consistently and record the reversal of script `$in` and `$out` lowering in ADR-033.
Keep the ADR status within the documented value set while retaining the ADR-033 supersession reference.
Keep ADR-014 accepted because ADR-033 reverses only its script $in and $out decision. The backend-escaping, path, metadata, and marker decisions remain active.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (13)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. SummaryPreserve Keep Update interpolation logic, unit tests, property tests, and Ninja regression tests. Clarify the behaviour in ADR-033, ADR-014, the design documentation, user guide, and structured-command RFC. Relate the implementation to issue ValidationFormatting, lint, documentation, tests, Markdown checks, and Nixie checks passed. WalkthroughThe change separates Netsuke markers from shell variables. Script interpolation now lowers only internal input and output tokens. Ninja generation escapes residual dollars, and documentation and tests record the updated contract. ChangesScript shell-variable preservation
Sequence Diagram(s)sequenceDiagram
participant ScriptRecipe
participant ScriptInterpolator
participant NinjaBackend
participant NinjaBuild
ScriptRecipe->>ScriptInterpolator: Provide script with markers and shell variables
ScriptInterpolator->>ScriptInterpolator: Lower {{ ins }} and {{ outs }}
ScriptInterpolator->>NinjaBackend: Preserve $in and $out
NinjaBackend->>NinjaBuild: Escape residual dollars
Suggested labels: Priority: ⬇️ Low Change: Bug fix Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
Full details: Testing (Overall)Explanation The new tests do not rigorously guard the end-to-end Resolution Add a POSIX Ninja integration test for a Full details: User-Facing DocumentationExplanation
Resolution Update the applicable n+1 migration guide, currently Markers take their token path Comment |
Reviewer's GuideScripts now preserve Sequence diagram for preserving script shell variablessequenceDiagram
participant Manifest
participant Interpolation
participant NinjaBackend
participant Shell
Manifest->>Interpolation: Render {{ ins }} and {{ outs }} as internal tokens
Interpolation->>Interpolation: find_substitution()
Interpolation-->>NinjaBackend: Preserve $in and $out in ShellText
NinjaBackend->>NinjaBackend: Escape residual dollars as $$
NinjaBackend-->>Shell: Execute script with $in and $out unchanged
Flow diagram for script markers and shell variablesflowchart LR
A[Script text] --> B{Token type}
B -->|"{{ ins }} or {{ outs }}"| C[Internal token]
C --> D[Substitute input or output paths]
B -->|$in or $out| E[Shell variable unchanged]
D --> F[ShellText to NinjaValue]
E --> F
F --> G[Ninja emits doubled dollars]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbbef5bfd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `$in` and `$out` are shell variables in every recipe. They are not Netsuke | ||
| markers or internal tokens, so script interpolation leaves them unchanged. The |
There was a problem hiding this comment.
This new contract conflicts with the active proposed specification in docs/rfcs/0001-structured-command-blocks.md: lines 101–105 still state that standalone $in and $out undergo late substitution, while lines 1261–1265 promise that their existing behaviour remains for legacy and shell-mode commands. Since roadmap tasks use that RFC as their implementation specification, leaving it unchanged can reintroduce the superseded script semantics or produce incorrect compatibility guarantees; amend those sections or add an explicit ADR-033 supersession note.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Define only {{ ins }} and {{ outs }} as Netsuke markers, retain dollar-prefixed names as shell variables, and record ADR-033 supersession of script-specific lowering.
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
Issue
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Summary
script:recipes now preserve$inand$outas shell variables. The Ninjabackend writes them as
$$inand$$out, preventing Ninja from expanding itsinput and output markers.
{{ ins }}and{{ outs }}retain their Netsukemarker behaviour.
Closes #737.
Review walkthrough
now uses the same internal-token matcher as commands, so literal shell
variables fall through to backend escaping.
proves scripts emit doubled
$inand$out, while quoted marker tests retainpath-substitution coverage.
and the user guide
distinguish Netsuke markers, internal tokens, and shell variables.
Validation
make check-fmtmake lintmake doc-coverage(98.80%)make test(3,227 passed, 5 skipped; doctests passed)make markdownlintmake nixiecoderabbit review --agent(zero findings)References
Summary by Sourcery
Preserve
$inand$outas shell variables in scripts while retaining explicit Netsuke markers for input and output path substitution.Bug Fixes:
$inand$outinscript:recipes as shell variables so Ninja no longer expands them as its own input and output markers.Enhancements:
{{ ins }}and{{ outs }}are treated as Netsuke path markers while shell variables remain unchanged.Documentation:
Tests:
{{ ins }}and{{ outs }}path substitution.