Conversation
Reviewer's GuideThis PR updates the design documentation to reflect new inline Entity relationship diagram for Rule, Target, and Action with Recipe and TargetRecipeerDiagram
RULE ||--o| RECIPE : uses
TARGET ||--o| TARGET_RECIPE : uses
ACTION ||--o| RECIPE : uses
RECIPE {
string command
string script
}
TARGET_RECIPE {
string rule
string command
string script
}
Class diagram for updated Rule, Target, and Action types with Recipe and TargetRecipe enumsclassDiagram
class Rule {
+String name
+Recipe recipe
+Option<String> description
+Option<String> deps
}
class Recipe {
}
Recipe <|-- Command
Recipe <|-- Script
class Command {
+String command
}
class Script {
+String script
}
class Target {
+StringOrList name
+TargetRecipe recipe
+StringOrList sources
+Option<StringOrList> order_only
+Option<StringOrList> implicit
+Option<StringOrList> outputs
+Option<StringOrList> byproducts
+Option<String> description
+bool always
}
class TargetRecipe {
}
TargetRecipe <|-- RuleRef
TargetRecipe <|-- CommandRef
TargetRecipe <|-- ScriptRef
class RuleRef {
+String rule
}
class CommandRef {
+String command
}
class ScriptRef {
+String script
}
class Action {
+Recipe recipe
+Option<String> description
+Option<String> depfile
+Option<String> deps_format
}
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 9 minutes and 28 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 (2)
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughUpdate the Netsuke build manifest schema and its Rust data structures to support specifying either a single-line 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}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Add validation in manifest parsing to enforce exactly one of command or script on rules and exactly one of rule, command, or script on targets.
- Consider refactoring the mutually exclusive command/script/ rule fields into an enum rather than multiple Option to improve schema clarity and prevent invalid states.
- Ensure the IR-to-Ninja backend is updated to detect and correctly emit script blocks (invoking /bin/sh) alongside regular commands.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add validation in manifest parsing to enforce exactly one of command or script on rules and exactly one of rule, command, or script on targets.
- Consider refactoring the mutually exclusive command/script/ rule fields into an enum rather than multiple Option<String> to improve schema clarity and prevent invalid states.
- Ensure the IR-to-Ninja backend is updated to detect and correctly emit script blocks (invoking /bin/sh) alongside regular commands.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
docs/netsuke-design.md (1)
339-346: DeriveDeserializeonRuleExample code shows
#[serde(deny_unknown_fields)]without#[derive(Deserialize)], so it would not compile. Add the derive to keep the sample correct.-#[serde(deny_unknown_fields)] -pub struct Rule { +#[derive(Deserialize)] +#[serde(deny_unknown_fields)] +pub struct Rule {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/netsuke-design.md(5 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
[uncategorized] ~167-~167: Loose punctuation mark.
Context: ...hat defines a reusable action. - name: A unique string identifier for the rule...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~169-~169: Loose punctuation mark.
Context: ...ng identifier for the rule. - command: A single command string to be executed....
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~174-~174: Loose punctuation mark.
Context: ...$inand $outvariables. -script`: A multi-line script declared with the Y...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~198-~198: Loose punctuation mark.
Context: ...for building this target. - command: A single command string to run directly...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~200-~200: Loose punctuation mark.
Context: ...un directly for this target. - script: A multi-line script passed to the inter...
(UNLIKELY_OPENING_PUNCTUATION)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
docs/netsuke-design.md (2)
171-183: Wrap bullet-point lines to 80 columns
These bullet entries are still over-long, repeating an issue flagged in the
earlier review. Break them after logical clause boundaries so every line sits
within the 80-column limit.
205-214: Wrap target-level bullets to 80 columns
Thecommand/scriptexclusivity bullets again exceed the style-guide width.
Re-flow them as requested previously.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/netsuke-design.md(7 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
[uncategorized] ~169-~169: Loose punctuation mark.
Context: ...hat defines a reusable action. - name: A unique string identifier for the rule...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~171-~171: Loose punctuation mark.
Context: ...ng identifier for the rule. - command: A single command string to be executed....
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~176-~176: Loose punctuation mark.
Context: ...$inand $outvariables. -script`: A multi-line script declared with the Y...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~182-~182: Would you like to use the Oxford spelling “deserialize”? The spelling ‘deserialise’ is also correct.
Context: ...id states. Internally, these options deserialise into an enum Recipe to ensure the e...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~205-~205: Loose punctuation mark.
Context: ...for building this target. - command: A single command string to run directly...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~207-~207: Loose punctuation mark.
Context: ...un directly for this target. - script: A multi-line script passed to the inter...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~211-~211: Would you like to use the Oxford spelling “deserialization”? The spelling ‘deserialisation’ is also correct.
Context: ...ser validates this exclusivity during deserialisation. This union deserialises into an enu...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~213-~213: Would you like to use the Oxford spelling “deserializes”? The spelling ‘deserialises’ is also correct.
Context: ...y during deserialisation. This union deserialises into an enum TargetRecipe so that the...
(OXFORD_SPELLING_Z_NOT_S)
| When an action's `recipe` is a script, the generated rule wraps the script in | ||
| an invocation of `/bin/sh -e -c` so that multi-line scripts execute | ||
| consistently across platforms. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Wrap explanatory lines to 80 columns
The newly added explanation of the /bin/sh -e -c wrapper breaches the 80-column
limit. Re-wrap for consistency with documentation guidelines.
🤖 Prompt for AI Agents
In docs/netsuke-design.md around lines 746 to 749, the explanation about the
`/bin/sh -e -c` wrapper exceeds the 80-column line length limit. Reformat the
text by inserting line breaks to ensure no line goes beyond 80 characters,
maintaining readability and consistency with the documentation style.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Consider consolidating the
RecipeandTargetRecipeenums into a single generic enum (or a shared type) since they share identical structure, to reduce duplication and ensure consistent behaviour across rules and targets. - Add a serde alias or default variant for manifests that don’t include the new
kindtag so existing configs without explicitkindfields continue to deserialize correctly and avoid breaking backward compatibility. - Make the shell invocation for multi-line scripts configurable (or respect shebangs) instead of hardcoding
/bin/sh -e -c, so users can opt into other interpreters or platform-specific shells.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider consolidating the `Recipe` and `TargetRecipe` enums into a single generic enum (or a shared type) since they share identical structure, to reduce duplication and ensure consistent behaviour across rules and targets.
- Add a serde alias or default variant for manifests that don’t include the new `kind` tag so existing configs without explicit `kind` fields continue to deserialize correctly and avoid breaking backward compatibility.
- Make the shell invocation for multi-line scripts configurable (or respect shebangs) instead of hardcoding `/bin/sh -e -c`, so users can opt into other interpreters or platform-specific shells.
## Individual Comments
### Comment 1
<location> `AGENTS.md:139` </location>
<code_context>
- files and fix table markup.
+- Run `make fmt` after any documentation changes to format all Markdown files
+ and fix table markup.
- Validate Markdown Mermaid diagrams using by running `make nixie`.
- Markdown paragraphs and bullet points must be wrapped at 80 columns.
- Code blocks must be wrapped at 120 columns.
</code_context>
<issue_to_address>
Typo: 'using by running' should be 'by running'.
It should read: 'Validate Markdown Mermaid diagrams by running `make nixie`.'
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
- Validate Markdown Mermaid diagrams using by running `make nixie`.
=======
- Validate Markdown Mermaid diagrams by running `make nixie`.
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai resolve |
1 participant
Summary
commandandscriptoptions for rules and targetsTesting
make fmtmake markdownlintmake nixiemake lintmake testhttps://chatgpt.com/codex/tasks/task_e_6872b6c0e33883228d643fad30bbe296
Summary by Sourcery
Introduce first-class
commandandscriptrecipes for rules and targets, refactoring the manifest schema and Ninja generator to support and enforce these options, and update documentation accordingly.New Features:
commanddirectly on a target without a rulescriptblocks for both rules and targetsEnhancements:
commandfield in Rule and Target with tagged enums (RecipeandTargetRecipe) to enforce one-of exclusivityscriptrecipes in/bin/sh -e -cfor consistent executionDocumentation:
commandandscriptoptions in the design spec and update all related code examples