Conversation
Reviewer's GuideThis PR refines the documentation for command escaping by enforcing that File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughUpdate the Netsuke design documentation to clarify command and script template handling, focusing on shell escaping, variable interpolation, and command security. Revise the roadmap to require that interpolated command strings are parsable by Changes
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/netsuke-design.md(5 hunks)docs/roadmap.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
**/*.md
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- AGENTS.md
⚙️ CodeRabbit Configuration File
🪛 LanguageTool
docs/netsuke-design.md
[style] ~175-~175: Consider using a more formal/concise alternative here.
Context: ...latest/shlex/) crate. Any interpolation other than ins or outs is automatically shell ...
(OTHER_THAN)
[uncategorized] ~178-~178: Loose punctuation mark.
Context: ...automatically shell escaped. - script: A multi-line script declared with the Y...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~581-~581: Loose punctuation mark.
Context: ...ta within templates. - | shell_escape: A filter that operates on both scalar s...
(UNLIKELY_OPENING_PUNCTUATION)
[misspelling] ~582-~582: This expression is normally spelled as one or with a hyphen.
Context: ...lusion as a single shell argument. This non- negotiable security feature uses the shell-quote...
(EN_COMPOUNDS_NON_NEGOTIABLE)
🔇 Additional comments (1)
docs/netsuke-design.md (1)
283-290: Table changes look good.No issues detected; table rows remain unwrapped as required.
| - [ ] Ensure the interpolated `command` value parses successfully with | ||
| `shlex`. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Wrap the new checklist line to 80 columns.
The added bullet slightly exceeds the mandated 80-column limit for Markdown list items. Re-wrap and align the continuation to keep markdownlint (MD013) happy.
- - [ ] Ensure the interpolated `command` value parses successfully with `shlex`
+ - [ ] Ensure the interpolated `command` value parses successfully
+ with `shlex`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - [ ] Ensure the interpolated `command` value parses successfully with | |
| `shlex`. | |
| - [ ] Ensure the interpolated `command` value parses successfully | |
| with `shlex` |
🤖 Prompt for AI Agents
In docs/roadmap.md around lines 118 to 120, the new checklist line exceeds the
80-column limit for Markdown list items. Reformat the line by wrapping the text
so that no line goes beyond 80 characters, and align the continuation lines
properly to maintain Markdown list formatting and satisfy markdownlint (MD013).
| Instead, the implementation must parse the Netsuke command template (e.g., | ||
| `{cc} -c {ins} -o {outs}`) and build the final command string piece by piece. | ||
| The placeholders `{{ ins }}` and `{{ outs }}` remain as Ninja's $in and $out | ||
| variables. After substitution, Netsuke verifies that the resulting command | ||
| string can be parsed by the `shlex` crate. For each segment of the command, if | ||
| it is a variable substitution (like `{ins}`), the value of that variable must be | ||
| passed through the `shell-quote` API before being appended to the output string. | ||
| This ensures that every dynamic part of the command is correctly and safely | ||
| quoted for the target shell. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Wrap the implementation-strategy paragraph to 80 columns.
Multiple lines in this block exceed the limit. Run mdformat-all then markdownlint to auto-wrap and verify.
🤖 Prompt for AI Agents
In docs/netsuke-design.md around lines 852 to 860, the paragraph describing the
implementation strategy exceeds the 80-column line length limit. Reformat the
paragraph by wrapping the text so that no line exceeds 80 characters, using a
markdown formatter like mdformat-all and then verify with markdownlint to ensure
compliance with the style guide.
| - `script`: A multi-line script declared with the YAML `|` block style. The | ||
| entire block is passed to an interpreter (currently `/bin/sh`). | ||
| entire block is passed to an interpreter (currently `/bin/sh`). For `/bin/sh` | ||
| scripts, each interpolation is automatically passed through the `shell_escape` | ||
| filter unless a `| raw` filter is applied. Future versions will allow | ||
| configurable script languages with their own escaping rules. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Re-wrap the script bullet to ≤ 80 columns.
The two lines run well past the 80-column limit. Break after “scripts,” and align the continuation to six spaces to satisfy the documentation style rules.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~178-~178: Loose punctuation mark.
Context: ...automatically shell escaped. - script: A multi-line script declared with the Y...
(UNLIKELY_OPENING_PUNCTUATION)
🤖 Prompt for AI Agents
In docs/netsuke-design.md around lines 178 to 183, the bullet point describing
the `script` field exceeds the 80-column width limit. Re-wrap the text so that
no line is longer than 80 characters, breaking after the word "scripts," and
indent continuation lines with six spaces to align with the documentation style
guidelines.
| Netsuke's IR-to-Ninja generator will translate these into Ninja's native $in | ||
| and $out variables. After interpolation, the value must be parsable by the | ||
| [`shlex`](https://docs.rs/shlex/latest/shlex/) crate. Any interpolation other | ||
| than `ins` or `outs` is automatically shell escaped. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Hyphenate “shell-escaped”.
Use the compound adjective “shell-escaped” for precision and consistency.
-Any interpolation other than `ins` or `outs` is automatically shell escaped.
+Any interpolation other than `ins` or `outs` is automatically shell-escaped.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Netsuke's IR-to-Ninja generator will translate these into Ninja's native $in | |
| and $out variables. After interpolation, the value must be parsable by the | |
| [`shlex`](https://docs.rs/shlex/latest/shlex/) crate. Any interpolation other | |
| than `ins` or `outs` is automatically shell escaped. | |
| Netsuke's IR-to-Ninja generator will translate these into Ninja's native $in | |
| and $out variables. After interpolation, the value must be parsable by the | |
| [`shlex`](https://docs.rs/shlex/latest/shlex/) crate. Any interpolation other | |
| than `ins` or `outs` is automatically shell-escaped. |
🧰 Tools
🪛 LanguageTool
[style] ~175-~175: Consider using a more formal/concise alternative here.
Context: ...latest/shlex/) crate. Any interpolation other than ins or outs is automatically shell ...
(OTHER_THAN)
🤖 Prompt for AI Agents
In docs/netsuke-design.md around lines 173 to 177, the phrase "shell escaped"
should be hyphenated to "shell-escaped" to correctly use the compound adjective
form. Update the text to replace "shell escaped" with "shell-escaped" for
clarity and consistency.
1 participant
Summary
commandvalues must beshlexparsableshell_escapebehaviour for lists and scalarsshlexparsingTesting
make fmtmake lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_6873ac416c248322bb3cd2d8832b65f1
Summary by Sourcery
Refine documentation to clarify command escaping requirements and behaviors, add details on shell escaping filters, note future enhancements for script languages, and update the roadmap to enforce shlex parsing.
Documentation:
commandstrings are parsable by theshlexcrate after interpolation/bin/shscripts and describeshell_escapebehavior for both scalars and listscommandvalues parse successfully withshlex