Default-subcommand manifest build with missing-manifest error#256
Default-subcommand manifest build with missing-manifest error#256
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughSummarise implemented roadmap items 3.6.1–3.6.3: add a ManifestNotFound runner error and early existence check; localise CLI help and per‑argument texts; add a Hello World quickstart example and docs; emit miette diagnostics for runner failures; add BDD tests validating missing‑manifest scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as CLI
participant Localizer as Localizer
participant Runner as Runner
participant FileSystem as FileSystem
participant Miette as Miette
User->>CLI: invoke netsuke [args]
CLI->>Localizer: load and localise command & arguments
CLI->>Runner: call generate_ninja(parsed_args)
Runner->>FileSystem: check manifest path exists
alt manifest exists
Runner->>Runner: load manifest, generate ninja, run build
Runner->>User: success output
else manifest missing
Runner->>Runner: construct RunnerError::ManifestNotFound
Runner->>Miette: create diagnostic Report
Runner-->>CLI: return error
CLI->>User: print miette diagnostic to stderr
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideImplements friendly missing-manifest handling with a new RunnerError surfaced via miette diagnostics, wires localized help text for root and subcommand flags, and adds tests, docs, and a hello-world example to support the onboarding experience and roadmap items 3.6.1–3.6.3. Sequence diagram for missing manifest handling with RunnerErrorsequenceDiagram
actor User
participant Main as main
participant Runner as runner_run
participant Gen as generate_ninja
User->>Main: execute netsuke
Main->>Runner: run(merged_cli)
Runner->>Gen: generate_ninja(&Cli)
Gen->>Gen: resolve_manifest_path(cli)
Gen->>Gen: manifest_path.exists()
alt manifest missing
Gen-->>Runner: Err(RunnerError::ManifestNotFound)
Runner-->>Main: Err(RunnerError)
Main->>Main: downcast_ref::<RunnerError>()
Main->>Main: Report::new_boxed(runner_err.clone())
Main->>User: write diagnostic report to stderr
else manifest exists
Gen-->>Runner: Ok(NinjaContent)
Runner-->>Main: Ok(())
Main-->>User: ExitCode::SUCCESS
end
Sequence diagram for localized CLI argument help textsequenceDiagram
participant Main as main
participant CliParse as parse_with_localization
participant Localizer as Localizer
participant Command as ClapCommand
Main->>CliParse: parse_with_localization(iter, localizer)
CliParse->>Command: Cli::command()
CliParse->>CliParse: localize_command(command, localizer)
CliParse->>CliParse: localize_arguments(command, localizer, None)
loop each root argument
CliParse->>Localizer: message(cli.flag.{arg_id}.help, fallback_help)
Localizer-->>CliParse: localized_help or fallback
CliParse->>Command: set help for argument
end
CliParse->>CliParse: localize_subcommands(command, localizer)
loop each subcommand
CliParse->>CliParse: localize_arguments(subcommand, localizer, Some(name))
loop each subcommand argument
CliParse->>Localizer: message(cli.subcommand.{name}.flag.{arg_id}.help, fallback_help)
Localizer-->>CliParse: localized_help or fallback
CliParse->>Command: set help for subcommand argument
end
end
CliParse->>CliParse: try_get_matches_from_mut(iter)
CliParse->>CliParse: Cli::from_arg_matches_mut(matches_for_parse)
CliParse-->>Main: (Cli, matches_for_merge)
Class diagram for RunnerError and manifest missing diagnosticsclassDiagram
class RunnerError {
<<enum>>
+ManifestNotFound
}
class ManifestNotFoundData {
+String manifest_name
+String directory
+PathBuf path
}
class ErrorTrait {
<<trait>>
}
class DiagnosticTrait {
<<trait>>
}
RunnerError <|-- ManifestNotFoundData
RunnerError ..|> ErrorTrait
RunnerError ..|> DiagnosticTrait
class GenerateNinjaFn {
+generate_ninja(cli : &Cli) Result~NinjaContent~
}
class Cli {
+Option~String~ directory
}
GenerateNinjaFn --> Cli : uses
GenerateNinjaFn --> RunnerError : returns on error
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/manifest_command.rs Comment on lines +252 to +265 fn run_netsuke_with_args(world: &TestWorld, args: &str) -> Result<()> {
let temp_path = get_temp_path(world)?;
let args: Vec<&str> = args.split_whitespace().collect();
let run = run_netsuke_in(&temp_path, &args)?;
store_run_result(
world,
RunResult {
stdout: run.stdout,
stderr: run.stderr,
success: run.success,
},
);
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
![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: f0550983d6
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In @docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md:
- Line 162: Replace British/Oxford spellings used in the doc: change every
occurrence of "localisation" to "localization" and "localised" to "localized"
(including the instances flagged in the diff such as the sentence starting "Add
localisation keys..."), and run a quick grep/Find-in-File to ensure no other
occurrences remain.
- Around line 284-287: Replace British "-isation"/"localisation"/"localise"
spellings with American "-ization"/"localization"/"localize" consistently on the
specified lines: update line 284, line 296, lines 305–306, and line 325 to use
"localize"/"localization" (and any related "-ize"/"-ization" forms) so the
document consistently uses the -ize/-ization variants.
- Around line 328-333: Table entries listing new files use a hyphen ("-")
instead of an em dash ("—"); update each affected table cell for the entries
`tests/features/missing_manifest.feature`, `tests/features/quickstart.feature`,
`docs/quickstart.md`, `examples/hello-world/Netsukefile`,
`examples/hello-world/input.txt`, and `examples/hello-world/README.md` to
replace " - " with " — " so the format reads e.g. **New** — BDD scenarios,
applying the same change to lines 329–333 as suggested.
- Around line 44-45: The docs use en-GB-oxendict but the words "localise" and
"localisation" appear; update these to the required Oxford spelling "localize"
and "localization" across the referenced items: change the checklist entries to
"Extend `cli_l10n.rs` to localize flag help strings." and "Add localization keys
to Fluent files (`en-US`, `es-ES`)." Also search for and replace other
occurrences of "localise"/"localisation" in this document and related docs to
the new "localize"/"localization" forms, and keep `cli_l10n.rs` and Fluent file
names unchanged.
- Around line 270-272: The three consecutive bullets under the help examples in
the document repeat the same "Running" sentence start; edit the three lines (the
bullets that begin with "Running `netsuke --help`...", "Running `netsuke build
--help`...", and "Running `netsuke --locale es-ES --help`...") to vary phrasing
and improve flow—e.g., start one with the command output ("`netsuke --help`
shows..."), another with the subcommand context ("For `netsuke build --help`,
the subcommand flags show..."), and the third with the locale condition ("With
`--locale es-ES`, help text is shown in Spanish...") so each bullet has a
distinct opening while preserving the original meaning.
- Around line 75-78: The paragraph uses mixed spellings "localise" and
"localisation"; update the decision entry to use Oxford spelling consistently by
changing all occurrences to "localize" and "localization" (e.g., in the sentence
mentioning `cli_l10n.rs` and the `localize_arguments()` helper), ensuring the
module and helper names remain accurate and the text reads consistently.
- Around line 102-103: Change the non‑Oxford spelling "localizes" to "localises"
in the documentation and code comments; search for the token "localizes" (e.g.,
in the doc line about src/cli_l10n.rs and any comments or help text emitted by
cli_l10n) and replace each occurrence with "localises" so spelling is consistent
across docs and the cli_l10n module.
- Line 109: The phrase should use Oxford spelling; replace any American
spellings (e.g., "localized") with the Oxford variant "localised" in this
document and ensure consistency for other common words (e.g., "optimise",
"prioritise", "customise", "organisation"); specifically search for and update
occurrences of "localized" to "localised" and scan the file for other US-to-UK
spelling mismatches and correct them.
In @docs/quickstart.md:
- Line 159: The sentence "Try `netsuke graph` to visualise your build dependency
graph" uses British "visualise"; update the text to use the -ize spelling per
project guideline by replacing "visualise" with "visualize" in the
docs/quickstart.md line containing that sentence so it reads "Try `netsuke
graph` to visualize your build dependency graph".
In @src/runner/mod.rs:
- Around line 7-16: Update the module-level suppression comment that precedes
the `#![allow(...)]` block (which currently lists `clippy::allow_attributes`,
`clippy::allow_attributes_without_reason`, and `unused_assignments`) to include
a link to a newly created tracking issue and a short issue ID/URL in the FIXME
line so future reviewers can follow up when upstream `miette`/`thiserror` is
fixed; create the issue in the repo describing the lint/version incompatibility
and reference that issue URL in the FIXME comment so the temporary
`#![allow(...)]` can be revisited and removed later.
- Around line 278-301: The code silently falls back to "Netsukefile" when
manifest_path.file_name() returns None; instead, validate the presence of a file
name in resolve_manifest_path() and surface a ManifestNotFound error for
malformed inputs. Update resolve_manifest_path() to check
manifest_path.file_name().ok_or_else(|| RunnerError::ManifestNotFound {
manifest_name: manifest_path.into_std_path_buf().file_name().and_then(|s|
s.to_str()).map(|s| s.to_string()).unwrap_or_default(), directory: /* build as
before from cli.directory/parent */, path: manifest_path.into_std_path_buf() })
(or construct the error directly) when file_name() is None, and remove the
.unwrap_or("Netsukefile") fallback in mod.rs so the caller no longer masks
malformed paths; ensure the error uses RunnerError::ManifestNotFound with the
same fields used elsewhere.
In @tests/bdd/steps/manifest_command.rs:
- Around line 225-231: In empty_workspace_at_path, add a sanity check before
calling fs::remove_dir_all to avoid deleting sensitive locations: reject
absolute paths (Path::is_absolute) and paths containing parent components (check
path.components() for Component::ParentDir), and if you have a known test root
available on TestWorld (e.g., world.test_root or similar), canonicalize the
target and ensure it starts_with that test root; if any check fails, return an
Err with_context explaining the rejected path instead of removing it. Ensure
these checks occur immediately before the remove_dir_all call that currently
uses dir.
In @tests/features/missing_manifest.feature:
- Around line 16-20: The scenario uses a hardcoded path
"/tmp/netsuke-test-empty" which breaks isolation; change the Given step to use
the existing empty_workspace step without a path (e.g., "Given an empty
workspace") or generate a tempfile::tempdir() and pass its path into the When
step so the temp dir is unique and auto-cleaned; update the steps referenced
("Given an empty workspace at path", "When netsuke is run with arguments") to
use the dynamic/temp workspace name and remove the hardcoded
"/tmp/netsuke-test-empty".
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.mddocs/roadmap.mddocs/users-guide.mdexamples/hello-world/Netsukefileexamples/hello-world/README.mdexamples/hello-world/input.txtlocales/en-US/messages.ftllocales/es-ES/messages.ftlsrc/cli.rssrc/cli_l10n.rssrc/main.rssrc/runner/mod.rstests/bdd/steps/manifest_command.rstests/features/missing_manifest.featuretests/logging_stderr_tests.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
examples/hello-world/README.mddocs/quickstart.mddocs/roadmap.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/users-guide.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
examples/hello-world/README.mddocs/quickstart.mddocs/roadmap.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/users-guide.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization'; suffix ‑lyse in words like 'analyse', 'paralyse', 'catalyse'; suffix -our in 'colour', 'behaviour', 'neighbour'; suffix -re in 'calibre', 'centre', 'fibre'; double 'l' in 'cancelled', 'counsellor', 'cruellest'; maintain 'e' in 'likeable', 'liveable', 'rateable'; suffix -ogue in 'analogue', 'catalogue'. Keep US spelling in external APIs, commands, and libraries (e.g., 'color'). Spell licence file as 'LICENSE'.
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension.
Treat company names as collective nouns in documentation: 'df12 Productions are expanding'.
Avoid first and second person personal pronouns in documentation outside of README.md files.
Write headings in sentence case in documentation.
Use Markdown headings (#, ##, ###, etc.) in order without skipping levels in documentation.
Follow markdownlint recommendations for Markdown formatting and structure.
Provide code blocks and lists using standard Markdown syntax in documentation.
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change in documentation.
Prefer inline links using 'text' or angle brackets around the URL in documentation.
Ensure blank lines before and after bulleted lists and fenced blocks in documentation.
Ensure tables have a delimiter line below the header row in documentation.
Expand any uncommon acronym on first us...
Files:
docs/quickstart.mddocs/roadmap.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/users-guide.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/logging_stderr_tests.rssrc/main.rssrc/cli_l10n.rssrc/runner/mod.rssrc/cli.rstests/bdd/steps/manifest_command.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/logging_stderr_tests.rssrc/main.rssrc/cli_l10n.rssrc/runner/mod.rssrc/cli.rstests/bdd/steps/manifest_command.rs
🧠 Learnings (1)
📚 Learning: 2025-12-29T19:00:20.890Z
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Applied to files:
tests/logging_stderr_tests.rssrc/main.rssrc/cli_l10n.rssrc/runner/mod.rssrc/cli.rstests/bdd/steps/manifest_command.rs
🧬 Code graph analysis (2)
tests/logging_stderr_tests.rs (1)
src/stdlib/which/lookup/workspace/mod.rs (1)
contains(44-46)
tests/bdd/steps/manifest_command.rs (1)
test_support/src/netsuke.rs (1)
run_netsuke_in(55-68)
🪛 LanguageTool
docs/quickstart.md
[uncategorized] ~42-~42: Possible missing comma found.
Context: ...ates a file with a greeting - A default target so running netsuke without arguments ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~159-~159: Would you like to use the Oxford spelling “visualize”? The spelling ‘visualise’ is also correct.
Context: ...ilable options - Try netsuke graph to visualise your build dependency graph ## Trouble...
(OXFORD_SPELLING_Z_NOT_S)
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
[style] ~31-~31: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ... 2. Running netsuke --help and seeing localised descriptions for all flags. 3. Followi...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~44-~44: Would you like to use the Oxford spelling “localize”? The spelling ‘localise’ is also correct.
Context: ... loading. - [ ] Extend cli_l10n.rs to localise flag help strings. - [ ] Add localisati...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~45-~45: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...o localise flag help strings. - [ ] Add localisation keys to Fluent files (en-US, es-ES)...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~75-~75: Would you like to use the Oxford spelling “localize”? The spelling ‘localise’ is also correct.
Context: ...y) - Decision: Extend cli_l10n.rs to localise flag help strings using a `localize_a...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~75-~75: Do not mix variants of the same word (‘localize’ and ‘localise’) within a single text.
Context: ...to localise flag help strings using a localize_arguments()` helper. Rationale: Follows...
(EN_WORD_COHERENCY)
[style] ~77-~77: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...ing pattern for subcommand about text localisation and keeps all clap localisation logic i...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~77-~77: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...ut text localisation and keeps all clap localisation logic in one module. Date/Author: 202...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~102-~102: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...d(). - src/cli_l10n.rs` contains clap localisation logic; currently localises subcommand...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~102-~102: Would you like to use the Oxford spelling “localizes”? The spelling ‘localises’ is also correct.
Context: ...ains clap localisation logic; currently localises subcommand about/long_about but not f...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~103-~103: Possible missing comma found.
Context: ... localises subcommand about/long_about but not flag help strings. - `src/runner/mo...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~109-~109: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ... locales/es-ES/messages.ftl contain localised CLI messages. - tests/features/ conta...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~157-~157: Do not mix variants of the same word (‘localize’ and ‘localise’) within a single text.
Context: ...gaps in flag descriptions. 6. Extend localize_command(). Add a `localize_arguments...
(EN_WORD_COHERENCY)
[uncategorized] ~157-~157: Do not mix variants of the same word (‘localize’ and ‘localise’) within a single text.
Context: ... Extend localize_command(). Add a localize_arguments() helper in `src/cli_l10n...
(EN_WORD_COHERENCY)
[style] ~162-~162: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...flags. 7. Add Fluent messages. Add localisation keys for all flags to `locales/en-US...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~173-~173: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...** Verify help output contains expected localised strings for both English and Spanish...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~231-~231: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...t error type, existence check, and flag localisation. 4. Add Fluent messages and BDD scenar...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~270-~270: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...ename. - Running netsuke --help shows localised descriptions for all flags. - Running `...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~271-~271: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...localised descriptions for all flags. - Running netsuke build --help shows localised ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~271-~271: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ... - Running netsuke build --help shows localised subcommand flag help. - Running `netsuk...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~272-~272: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...shows localised subcommand flag help. - Running netsuke --locale es-ES --help shows S...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~276-~276: Loose punctuation mark.
Context: ...d quickstart example. - make check-fmt, make lint, and make test all pass. ...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~284-~284: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...run; they do not modify state. - Flag localisation changes are additive and safe to re-run...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~285-~285: Consider using a different verb for a more formal wording.
Context: ...afe to re-run. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[style] ~287-~287: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...log file to keep evidence current. - If localisation keys conflict with existing entries, re...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~296-~296: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...mp/netsuke-help.log— help output with localised flag descriptions. -/tmp/netsuke-test...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~305-~305: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...from_path_with_policy()call. - **Flag localisation**:src/cli_l10n.rs— addlocalize_ar...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~305-~305: Do not mix variants of the same word (‘localize’ and ‘localise’) within a single text.
Context: ...localisation**: src/cli_l10n.rs — add localize_arguments() helper. - Localisation...
(EN_WORD_COHERENCY)
[style] ~306-~306: Would you like to use the Oxford spelling “Localization”? The spelling ‘Localisation’ is also correct.
Context: ... add localize_arguments() helper. - Localisation: locales/en-US/messages.ftl and `lo...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~325-~325: Do not mix variants of the same word (‘localize’ and ‘localise’) within a single text.
Context: ...te_ninja()| |src/cli_l10n.rs| Addlocalize_arguments()` to localise flag help stri...
(EN_WORD_COHERENCY)
[style] ~325-~325: Would you like to use the Oxford spelling “localize”? The spelling ‘localise’ is also correct.
Context: ...10n.rs| Addlocalize_arguments()to localise flag help strings | |locales/en-US/me...
(OXFORD_SPELLING_Z_NOT_S)
[typographical] ~328-~328: To join two clauses or introduce examples, consider using an em dash.
Context: ...ures/missing_manifest.feature| **New** - BDD scenarios for missing manifest | |...
(DASH_RULE)
[typographical] ~329-~329: To join two clauses or introduce examples, consider using an em dash.
Context: ...s/features/quickstart.feature` | New - BDD scenario exercising hello-world exam...
(DASH_RULE)
[typographical] ~330-~330: To join two clauses or introduce examples, consider using an em dash.
Context: ...ample | | docs/quickstart.md | New - step-by-step tutorial | | `examples/hell...
(DASH_RULE)
[typographical] ~331-~331: To join two clauses or introduce examples, consider using an em dash.
Context: ...mples/hello-world/Netsukefile| **New** - minimal working example | |examples/he...
(DASH_RULE)
[typographical] ~332-~332: To join two clauses or introduce examples, consider using an em dash.
Context: ...xamples/hello-world/input.txt| **New** - sample input | |examples/hello-world/R...
(DASH_RULE)
[typographical] ~333-~333: To join two clauses or introduce examples, consider using an em dash.
Context: ...xamples/hello-world/README.md| **New** - example documentation | |docs/users-gu...
(DASH_RULE)
[grammar] ~339-~339: Make sure that the singular noun after the number ‘3.6’ is correct.
Context: ...xecPlan created for roadmap section 3.6 covering missing manifest error handling (3.6....
(CD_POINT_CD_NN)
🔍 Remote MCP Deepwiki
Summary of Relevant Context for PR Review
Based on the project documentation and architecture, here are the key facts relevant to reviewing this PR:
CLI Localization Context
The Netsuke CLI was previously built using the clap crate for argument parsing and help message generation but did not have a localization system. This PR (#256) introduces localization infrastructure by:
- Adding
localize_arguments()helper insrc/cli_l10n.rs - Extending localization to cover root-level and subcommand-specific flag help texts
- Adding localization keys in both English (en-US) and Spanish (es-ES) message catalogs
This represents a significant architectural addition to the CLI that extends beyond the prior documentation's scope.
Error Handling Architecture
The codebase employs a two-tier error architecture where anyhow is used to capture internal context as errors propagate, and miette is used for rendering user-facing diagnostics, ensuring that all surface errors implement miette::Diagnostic. The RunnerError::ManifestNotFound type introduced in this PR follows this pattern by implementing miette::Diagnostic for rich diagnostic output in main.rs.
BDD Test Framework
Integration tests use cucumber-rs with Gherkin feature files in tests/features/*.feature, a CliWorld struct for managing scenario state, and Rust step definitions implementing the Given-When-Then idiom. The new missing_manifest.feature and BDD steps in tests/bdd/steps/manifest_command.rs follow this established pattern.
Key Validation Points for Review
- Localization Integration: Verify that
localize_arguments()correctly resolves localization keys for both root flags and subcommand-specific arguments - Error Path: Confirm that
RunnerError::ManifestNotFoundproperly implementsmiette::Diagnosticand integrates with the two-tier error handling strategy - Test Duplication Fix: The flagged code duplication in manifest_command.rs has been addressed with the
run_netsuke_and_store()helper extraction - Related PR Dependencies: PRs #254 (CLI localization scaffolding), #250 (manifest subcommand), and #251 (BDD harness refactor) introduce overlapping infrastructure that this PR builds upon
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: Sourcery review
🔇 Additional comments (22)
docs/users-guide.md (1)
62-76: LGTM: Clear error guidance and Quick Start reference.The expanded error messaging provides actionable guidance for users encountering a missing manifest. The Quick Start link appropriately directs new users to onboarding material.
docs/quickstart.md (1)
1-178: Well-structured quickstart guide.The guide provides a clear, progressive introduction to Netsuke with practical examples, proper troubleshooting guidance, and appropriate next-step references. The content follows Markdown formatting guidelines and maintains proper line wrapping throughout.
docs/roadmap.md (1)
218-227: LGTM: Roadmap completion accurately reflects delivered functionality.The completed items 3.6.1–3.6.3 properly document the missing-manifest error handling, CLI localization, and quickstart guide implementations. The descriptions remain outcome-focused and measurable per the roadmap guidelines.
src/runner/mod.rs (1)
29-46: Well-structured diagnostic error type.The
RunnerErrorenum correctly derives boththiserror::Errorandmiette::Diagnostic, providing a rich user-facing error with a helpful hint. The field documentation is clear and follows Rustdoc conventions.docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md (2)
1-339: Comprehensive and well-structured ExecPlan.The document provides thorough context, clear decision rationale, concrete validation criteria, and idempotence guidance. The plan of work sections are actionable and well-organised. Apart from the spelling consistency issues noted above, this is a solid planning document.
111-111: Therstest-bddversion reference in the documentation is accurate and matches the version specified inCargo.toml.examples/hello-world/input.txt (1)
1-2: Simple and effective example input.The content is clear and demonstrates the text transformation without unnecessary complexity. Suitable for a quickstart example.
examples/hello-world/README.md (1)
1-31: Clear and concise example documentation.The README effectively documents the example's purpose, usage, expected outputs, and file listing. The structure follows good documentation practices with appropriate headings and code blocks.
tests/features/missing_manifest.feature (2)
1-9: Well-designed scenario for default behaviour.This scenario correctly tests the primary use case: running
netsukein a directory without a manifest. The assertions verify both the failure status and the presence of helpful guidance in stderr.
10-14: Good coverage for custom manifest paths.This scenario validates error handling when a user specifies a non-existent manifest file explicitly. The assertion correctly expects the custom filename in the error message.
tests/logging_stderr_tests.rs (1)
14-24: LGTM!The updated test correctly validates that
ManifestNotFounderrors are rendered via miette to stderr, whilst stdout remains free of the error message. The use of.expect()is appropriate in test context.src/main.rs (1)
44-53: LGTM!The downcast-based error handling correctly renders
RunnerErrorvariants via miette diagnostics whilst falling back to tracing for other error types. Thedrop(writeln!(...))pattern is acceptable at the application boundary where propagation is not possible.src/cli.rs (1)
246-256: LGTM!The clone-before-parse pattern correctly preserves
ArgMatchesfor configuration merging whilst allowingfrom_arg_matches_mutto consume values from the parse copy. The comment on line 249 clarifies the rationale.locales/es-ES/messages.ftl (1)
7-41: LGTM!The Spanish localisation strings are well-structured and align with the corresponding English keys. The coverage includes global flags, subcommand descriptions, subcommand-specific flags, and Clap error messages.
tests/bdd/steps/manifest_command.rs (2)
98-111: LGTM!The
run_netsuke_and_storehelper consolidates the duplicated run/store logic, addressing the reviewer feedback. The function correctly propagates errors and stores results in the test world.
244-258: LGTM!The new step definitions for running netsuke with and without arguments correctly delegate to the consolidated helper and follow the established patterns.
examples/hello-world/Netsukefile (2)
1-19: Solid quickstart example.The manifest demonstrates core Netsuke features: variables, shell commands, Jinja templating, and default targets. It serves as an effective "Hello World" for new users.
10-12: No changes required—scalarsourcesformat is correct.Line 12 uses the standard format for single sources throughout the Netsuke codebase. Both test files and examples consistently use scalar format (
sources: value) for single sources and list format only when specifying multiple sources. Converting to list syntax would deviate from the established pattern.src/cli_l10n.rs (2)
43-67: LGTM!The
localize_argumentsfunction correctly localizes argument help text for both top-level commands and subcommands. The key-generation logic properly distinguishes between the two contexts, and the fallback behaviour when no localisation exists is sound.
37-38: LGTM!The integration points correctly wire argument localisation into both the top-level command (line 37) and each subcommand (line 90), ensuring consistent localised help text throughout the CLI.
Also applies to: 89-90
locales/en-US/messages.ftl (2)
7-16: Well-structured root-level flag help entries.The naming convention
cli.flag.<name>.helpis consistent and maps logically to CLI arguments. The descriptions are clear and provide useful context for each flag.One minor point: line 12 uses "CLI copy" which is editorial jargon. "CLI messages" or "CLI text" would be more immediately understood by a technical audience, though this is not blocking.
28-33: Subcommand flag help entries are consistent and clear.The naming pattern
cli.subcommand.<cmd>.flag.<name>.helpaligns with the root-level convention and makes key lookup predictable. The descriptions accurately convey the purpose of each argument.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
locales/en-US/messages.ftl (1)
39-41: Duplicate message text detected.
clap-error-invalid-value(line 39) andclap-error-value-validation(line 41) are identical. Clap distinguishesErrorKind::InvalidValue(e.g., enum variant mismatch) fromErrorKind::ValueValidation(custom validator failure). If both should display the same text intentionally, add a comment clarifying this. Otherwise, differentiate the messages—for example:clap-error-invalid-value = Invalid value for { $argument }: { $value } clap-error-value-validation = Validation failed for { $argument }: { $value }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
locales/en-US/messages.ftllocales/es-ES/messages.ftl
🧰 Additional context used
🔍 Remote MCP Deepwiki
Additional relevant facts for reviewing this PR
-
Manifest load is the pipeline entry (Stage 1); missing manifest should be detected early during "manifest ingestion" before YAML parse—this aligns with generate_ninja() returning ManifestNotFound when file absent.
-
Manifest processing is a two‑phase flow: parse YAML to serde_yml::Value, do structural expansion (foreach/when) with minijinja, then deserialize to typed AST and render strings. Changes to runner/manifest-not-found affect Stage 1 (ingestion) and error surface shown to users.
-
Template engine (minijinja) is configured strict (UndefinedBehavior::Strict) and exposes env() and glob() helpers; manifest loading and expansion rely on those functions—tests exercise env()/glob() behavior. Ensure ManifestNotFound integrates with template/env expectations (no attempted render when manifest absent).
-
Error strategy: library/domain errors use thiserror enums; application boundary uses anyhow with context; diagnostics aim to render via miette (manifest diagnostics already map serde_yml errors to miette Report). main.rs change to render RunnerError via miette fits existing error model.
-
Filesystem access uses cap-std + camino for UTF-8 paths; glob() and manifest file reads use capability-based I/O. The manifest-not-found reporting should include the resolved path/directory consistent with resolve_manifest_path behavior.
-
Command/IR behavior: IR generation validates rules, deduplicates actions (SHA256), expands placeholders and shell-escapes with shell-quote and validates with shlex. Changes to CLI/localization must not alter identifier/key shapes used when rendering help or deciding defaults.
-
Tests: project uses multiple layers—unit (rstest), BDD (cucumber feature files + CliWorld), snapshot (insta). The PR added BDD feature(s) and step helpers; ensure new steps follow CliWorld state patterns and environment isolation used elsewhere. New tests should run in CI job matrix (make test) which treats warnings as errors.
-
Ninja generation & runner expectations: generate_ninja() → NinjaContent → handle_build() writes either temporary NamedTempFile or user --emit path and invokes ninja subprocess (run_ninja). Temporary-file RAII and path resolution (-C behavior) are important for tests that assert behavior when manifest missing or when running in different working dirs.
Sources: Deepwiki (repo docs and implementation pages)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: build-test (ubuntu-latest, nightly, true)
🔇 Additional comments (5)
locales/es-ES/messages.ftl (3)
7-16: LGTM!Root-level flag help texts are well-structured. The snake_case key naming (
fetch_allow_scheme,fetch_allow_host, etc.) correctly aligns with the localisation fix for matchingarg.get_id()lookups. Translations are clear and appropriately technical.
28-33: LGTM!Subcommand flag help texts follow a consistent key hierarchy (
cli.subcommand.<cmd>.flag.<arg>.help) and the Spanish translations accurately convey the English source meanings.
39-41: Verify whether duplicate message text is intentional.
clap-error-invalid-valueandclap-error-value-validationhave identical text. If Clap'sInvalidValueandValueValidationerror kinds warrant distinct user-facing messages, differentiate them. Otherwise, if the duplication is deliberate, add a comment explaining why both keys exist with the same value.locales/en-US/messages.ftl (2)
7-16: LGTM!Root-level flag help texts are clear and concise. The snake_case key naming matches
arg.get_id()output, ensuring localisation lookups succeed for fetch-related flags as per the commit fix.
28-33: LGTM!Subcommand flag help texts are appropriately structured and provide useful guidance for
--emit,--targets, and manifest output path arguments.
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In @docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md:
- Around line 52-55: Update the "Surprises & Discoveries" section (the
"Surprises & Discoveries" header and its current lines "Observation: None yet.
Evidence: N/A.") to either list actual findings from the implementation or
replace the placeholder with a clear explicit statement such as "No surprises
encountered during implementation." Ensure the text is definitive (not "None
yet") and, if leaving it empty of discoveries, include a brief justification or
evidence line like "Evidence: Implementation matched expectations" to avoid
ambiguity.
- Around line 89-92: Update the "## Outcomes & Retrospective" section to replace
"Outcome: Pending. Notes: N/A." with the actual results of the implementation
and review status: state that the PR is ready for review, summarize key outcomes
(e.g., default subcommand now builds manifest defaults, tests added/updated, CI
passing), and add retrospective notes about design decisions, known limitations,
and any follow-up tasks or improvements to track.
- Around line 39-50: The Progress checklist in the document still has all items
unchecked; update the checkboxes to reflect completed work by marking the
appropriate items as done (e.g., change "- [ ]" to "- [x]") for the implemented
entries such as implementing `ManifestNotFound` with `miette::Diagnostic`,
adding file existence check in `generate_ninja()`, extending `cli_l10n.rs`,
adding localization keys, creating `docs/quickstart.md`, adding the
`examples/hello-world/` fixture, adding BDD scenarios, updating
`docs/users-guide.md`, and running formatting/lint/tests; ensure the Progress
list in the same file and section is consistent with the PR description so the
items above are checked.
- Around line 125-206: The ordered list in the "Plan of Work" uses continuous
numbering across sections, causing markdownlint MD029; update the lists under
the headings "3.6.1 Missing Manifest Error Handling", "3.6.2 Curate Help
Output", and "3.6.3 Hello World Quickstart" to restart numbering (e.g., begin
each subsection's list at 1) or convert all items to lazy numbering (use "1."
for each item) so each list is its own context and MD029 is resolved.
In @docs/quickstart.md:
- Around line 39-42: Update the prose in the doc snippet to add the missing
commas for clarity: in the first bullet add a comma after the word "greeting"
and in the second bullet add a comma after the word "target" so the lines read
"...creates a file with a greeting," and "A default target, so running `netsuke`
without arguments builds `hello.txt`". Ensure only punctuation is changed and
surrounding wording and backticks (e.g., `hello.txt`, `netsuke`) remain
unchanged.
In @src/runner/mod.rs:
- Around line 7-17: The module-level #[allow(...)] blanket suppression in
src/runner/mod.rs should be removed and the workaround localized: identify the
specific types/impls using the miette/thiserror derive macros (the structs/enums
whose derives trigger unused_assignments) and move them into a tiny submodule
(e.g., runner::derived_workaround) and apply the
#[allow(clippy::allow_attributes, clippy::allow_attributes_without_reason,
unused_assignments)] only on that submodule or directly on the affected item(s);
alternatively, if feasible, replace the derives with manual trait
implementations for those specific types to avoid the macro-induced lint, or if
neither is practicable, revert to pinning the Rust toolchain in CI and add an
ADR noting the documented deviation and tracking the upstream
rust-lang/rust#130021 issue.
In @tests/bdd/steps/manifest_command.rs:
- Around line 262-271: The current run_netsuke_with_args uses
args.split_whitespace() which breaks quoted arguments; replace it with a
shell-style parser (e.g., shell_words::split or shlex::split) to properly handle
quotes and escapes in run_netsuke_with_args, handle the Result from that parser
(return an Err on parse failure with a helpful message), and adapt the call to
run_netsuke_and_store by either converting the resulting Vec<String> to a
Vec<&str> before calling or by changing run_netsuke_and_store to accept owned
Strings (Vec<String>) so lifetimes remain correct.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.mdlocales/en-US/messages.ftllocales/es-ES/messages.ftlsrc/runner/mod.rstests/bdd/steps/manifest_command.rstests/features/missing_manifest.feature
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization'; suffix ‑lyse in words like 'analyse', 'paralyse', 'catalyse'; suffix -our in 'colour', 'behaviour', 'neighbour'; suffix -re in 'calibre', 'centre', 'fibre'; double 'l' in 'cancelled', 'counsellor', 'cruellest'; maintain 'e' in 'likeable', 'liveable', 'rateable'; suffix -ogue in 'analogue', 'catalogue'. Keep US spelling in external APIs, commands, and libraries (e.g., 'color'). Spell licence file as 'LICENSE'.
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension.
Treat company names as collective nouns in documentation: 'df12 Productions are expanding'.
Avoid first and second person personal pronouns in documentation outside of README.md files.
Write headings in sentence case in documentation.
Use Markdown headings (#, ##, ###, etc.) in order without skipping levels in documentation.
Follow markdownlint recommendations for Markdown formatting and structure.
Provide code blocks and lists using standard Markdown syntax in documentation.
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change in documentation.
Prefer inline links using 'text' or angle brackets around the URL in documentation.
Ensure blank lines before and after bulleted lists and fenced blocks in documentation.
Ensure tables have a delimiter line below the header row in documentation.
Expand any uncommon acronym on first us...
Files:
docs/quickstart.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/quickstart.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/quickstart.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
src/runner/mod.rstests/bdd/steps/manifest_command.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/runner/mod.rstests/bdd/steps/manifest_command.rs
🧠 Learnings (1)
📚 Learning: 2025-12-29T19:00:20.890Z
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Applied to files:
src/runner/mod.rstests/bdd/steps/manifest_command.rs
🧬 Code graph analysis (1)
tests/bdd/steps/manifest_command.rs (2)
tests/bdd/fixtures/mod.rs (1)
world(167-169)test_support/src/netsuke.rs (1)
run_netsuke_in(55-68)
🪛 LanguageTool
docs/quickstart.md
[uncategorized] ~42-~42: Possible missing comma found.
Context: ...ates a file with a greeting - A default target so running netsuke without arguments ...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
[uncategorized] ~276-~276: Loose punctuation mark.
Context: ...d quickstart example. - make check-fmt, make lint, and make test all pass. ...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~285-~285: Consider using a different verb for a more formal wording.
Context: ...afe to re-run. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[grammar] ~339-~339: Make sure that the singular noun after the number ‘3.6’ is correct.
Context: ...xecPlan created for roadmap section 3.6 covering missing manifest error handling (3.6....
(CD_POINT_CD_NN)
🪛 markdownlint-cli2 (0.18.1)
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
154-154: Ordered list item prefix
Expected: 1; Actual: 5; Style: 1/2/3
(MD029, ol-prefix)
157-157: Ordered list item prefix
Expected: 2; Actual: 6; Style: 1/2/3
(MD029, ol-prefix)
162-162: Ordered list item prefix
Expected: 3; Actual: 7; Style: 1/2/3
(MD029, ol-prefix)
170-170: Ordered list item prefix
Expected: 4; Actual: 8; Style: 1/2/3
(MD029, ol-prefix)
173-173: Ordered list item prefix
Expected: 5; Actual: 9; Style: 1/2/3
(MD029, ol-prefix)
178-178: Ordered list item prefix
Expected: 1; Actual: 10; Style: 1/2/3
(MD029, ol-prefix)
187-187: Ordered list item prefix
Expected: 2; Actual: 11; Style: 1/2/3
(MD029, ol-prefix)
192-192: Ordered list item prefix
Expected: 3; Actual: 12; Style: 1/2/3
(MD029, ol-prefix)
199-199: Ordered list item prefix
Expected: 4; Actual: 13; Style: 1/2/3
(MD029, ol-prefix)
204-204: Ordered list item prefix
Expected: 5; Actual: 14; Style: 1/2/3
(MD029, ol-prefix)
🔍 Remote MCP Deepwiki
Summary of additional review-relevant facts (from repository docs):
-
Manifest ingestion is Stage 1 of the six‑stage pipeline (read file → Stage 2 YAML parse → Stage 3 template expansion → Stage 4 deserialization). Missing-manifest detection belongs in Stage 1 (before YAML parse) and should include resolved path/directory context — matches RunnerError::ManifestNotFound semantics in this PR.
-
Template evaluation uses minijinja with UndefinedBehavior::Strict; custom functions env() and glob() are registered during manifest loading and used in both structural expansion (foreach/when) and final string rendering. Ensure ManifestNotFound short‑circuits before any template eval or file glob is attempted.
-
Filesystem access uses cap-std + camino (UTF‑8 paths) and glob() enforces literal‑separator semantics and lexicographic sorting; glob errors map to manifest diagnostics. RunnerError::ManifestNotFound should be consistent with capability-based path resolution behavior.
-
Error model: domain errors are thiserror enums; application layers use anyhow with context; diagnostics are rendered via miette (manifest parsing already maps serde_yml errors to miette::Report). main.rs change to render RunnerError via miette aligns with existing diagnostic strategy — reviewer should confirm RunnerError::ManifestNotFound implements Diagnostic as intended and that main prints miette::Report for it.
-
IR/Ninja generation pipeline: manifest → BuildGraph → ninja_gen; IR generation deduplicates actions by SHA‑256 and performs validations (rule existence, duplicate outputs, cycle detection). Changes to resolve_manifest_path/generate_ninja will affect early validation and the error surface seen by downstream stages/tests.
-
Testing layers: unit (rstest), snapshot (insta), BDD (cucumber features + CliWorld), integration (assert_cmd). The PR adds BDD scenarios and step helpers — verify new/changed steps follow CliWorld state patterns and use test_support utilities. Also note reviewer comment about duplicated helper extraction in manifest_command.rs matches test infrastructure patterns and suggested helper aligns with existing CliWorld usage.
-
CLI localization: CLI help and argument localization is implemented via localize_arguments/localize_command in cli_l10n; locales/en-US and es-ES contain new keys. Confirm keys used by localize_arguments match the keys added in locales files and that fallback logic preserves existing help when no localization key exists. Also confirm parse_with_localizer_from changes preserve ArgMatches semantics for downstream code.
Citations: all facts sourced from repository documentation pages retrieved via Deepwiki_read_wiki_contents.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, nightly, true)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (19)
locales/en-US/messages.ftl (3)
7-16: LGTM — Root-level flag help keys are well-structured.The naming convention
cli.flag.{arg_id}.helpis consistent and aligns with the localization pattern. The help text is concise and actionable.
28-33: LGTM — Subcommand flag help keys follow the established pattern.The
cli.subcommand.{cmd}.flag.{arg_id}.helpnaming is consistent with the root-level convention.
35-44: LGTM — Clap error messages provide clear differentiation.The comment block (lines 41-43) explaining the distinction between
clap-error-invalid-valueandclap-error-value-validationis helpful for maintainers.locales/es-ES/messages.ftl (2)
7-16: LGTM — Spanish translations for global options are present.Keys align with the en-US locale, maintaining parity.
41-44: Developer comment remains in English — acceptable but verify intent.The explanatory comment (lines 41-43) is in English within the Spanish locale file. This is fine for developer-facing documentation, but ensure this is intentional. If Spanish comments are preferred for consistency, translate it.
docs/quickstart.md (3)
151-159: LGTM — Next steps section provides clear onboarding paths.Links to the user guide, examples directory, and CLI help are appropriate for guiding new users.
161-178: LGTM — Troubleshooting section covers common issues.The guidance for missing manifest and missing Ninja errors is practical and actionable.
125-129: Thewith_suffixJinja filter is registered and fully implemented.The filter is defined in
src/stdlib/path/filters.rs(lines 18–30) and accepts a requiredsuffixparameter plus optionalcountandsepparameters. The documentation's usage{{ item | basename | with_suffix('.out') }}is valid, using only the suffix argument with default values for the optional parameters. Comprehensive test coverage exists intests/std_filter_tests/path_filters.rs.docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md (2)
1-7: LGTM — Document structure and purpose are clear.The execution plan clearly covers roadmap items 3.6.1-3.6.3 with appropriate context and living document sections.
57-88: LGTM — Decision log is well-documented.Each decision includes rationale, date, and author, providing good traceability for future maintainers.
tests/features/missing_manifest.feature (1)
1-20: All step definitions are implemented. The three scenarios comprehensively cover missing manifest error handling, with all steps properly defined intests/bdd/steps/manifest_command.rs:
- "an empty workspace"
- "netsuke is run without arguments"
- "netsuke is run with arguments {args:string}"
- "netsuke is run with directory flag pointing to the workspace"
This provides solid coverage for the
ManifestNotFounderror variant across multiple invocation patterns.src/runner/mod.rs (4)
23-28: LGTM!The imports for
miette::Diagnostic,PathBuf, andthiserror::Errorcorrectly support the newRunnerErrorenum implementation.
30-47: Well-structured diagnostic error type.The
RunnerErrorenum correctly derivesErrorandDiagnostic, providing:
- A clear error message with context (manifest name and directory)
- A diagnostic code following the
netsuke::namespace- Actionable help text for users
- Proper documentation for all fields
This aligns with the error model described in the external tools context.
279-304: Sound implementation with justifiedexpectusage.The missing-manifest check correctly:
- Short-circuits before any template evaluation or file globbing (per pipeline requirements)
- Uses a narrowly-scoped
#[expect]with documented reasoning- Provides contextual error information (manifest name, directory description, path)
The
expectat line 287 is safe becauseresolve_manifest_path()guaranteesfile_name()returnsSome(validated at lines 354-359).
344-361: LGTM!The
file_name()validation at lines 354-359 correctly establishes the invariant thatgenerate_ninja()depends upon. The error message clearly identifies the problematic path.tests/bdd/steps/manifest_command.rs (4)
98-111: Good extraction of duplicated logic.This helper addresses the code duplication flagged in the PR review. It correctly consolidates the shared pattern of:
- Obtaining the temp path
- Running netsuke
- Storing results in the test world
207-217: LGTM!The
empty_workspacestep correctly mirrors theminimal_workspacepattern whilst omitting the Netsukefile copy, providing proper test isolation.
256-260: LGTM!Clean delegation to the shared helper with an empty argument slice.
273-287: LGTM!The step correctly extracts the temp directory path and passes it via the
-Cflag. Usingto_string_lossy()is acceptable here sincetempfile::tempdir()paths are reliably UTF-8 on supported platforms.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md:
- Around line 202-208: In the locales/en-US/messages.ftl snippet, add an Oxford
comma to the root flags list by inserting a comma before the final item so the
line reads: `file`, `directory`, `jobs`, `verbose`, `locale`,
`fetch-allow-scheme`, `fetch-allow-host`, `fetch-block-host`, and
`fetch-default-deny`; update that list entry in the file to include the comma
before "and `fetch-default-deny`".
In @docs/quickstart.md:
- Around line 3-4: Change the awkward phrase "a first Netsuke build" to a
clearer alternative like "a Netsuke build" or "an initial Netsuke build" in the
sentence "This guide walks through creating and running a first Netsuke build in
under five minutes." Update the text in docs/quickstart.md to read e.g. "This
guide walks through creating and running a Netsuke build in under five minutes."
to improve grammar and clarity.
- Around line 1-178: Convert all Markdown headings in the document to sentence
case: find each header line beginning with # (e.g., "Quick Start: A First
Netsuke Build", "Prerequisites", "Step 1: Create a Project Directory", "Step 2:
Create the First Manifest", "Step 3: Run Netsuke", "Step 4: Add Variables and
Templates", "Step 5: Use Globbing and Foreach", "Next Steps", "Troubleshooting",
subheading like "No `Netsukefile` found in the current directory" and "ninja:
command not found") and change them to sentence case (capitalize only the first
word and proper nouns like "Netsuke", keep single-word headings with initial
capital), preserving punctuation and code blocks; update all level header
variants (#, ##, ###) consistently.
In @src/runner/mod.rs:
- Around line 254-258: The code uses manifest_path.file_name().expect(...) which
must be replaced with fallible error handling: extract the file name from
manifest_path with manifest_path.file_name().map(|n| n.to_owned()).ok_or_else(||
/* return a descriptive error */ )? and propagate the error from the surrounding
function (or convert to the crate's error type), updating the binding for
manifest_name accordingly; ensure the function that contains this code returns a
Result so you can use ? and preserve the existing comment about
resolve_manifest_path by returning an explicit error if file_name is missing
rather than panicking.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.mdsrc/runner/error.rssrc/runner/mod.rstests/bdd/steps/manifest_command.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
src/runner/error.rstests/bdd/steps/manifest_command.rssrc/runner/mod.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/runner/error.rstests/bdd/steps/manifest_command.rssrc/runner/mod.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization'; suffix ‑lyse in words like 'analyse', 'paralyse', 'catalyse'; suffix -our in 'colour', 'behaviour', 'neighbour'; suffix -re in 'calibre', 'centre', 'fibre'; double 'l' in 'cancelled', 'counsellor', 'cruellest'; maintain 'e' in 'likeable', 'liveable', 'rateable'; suffix -ogue in 'analogue', 'catalogue'. Keep US spelling in external APIs, commands, and libraries (e.g., 'color'). Spell licence file as 'LICENSE'.
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension.
Treat company names as collective nouns in documentation: 'df12 Productions are expanding'.
Avoid first and second person personal pronouns in documentation outside of README.md files.
Write headings in sentence case in documentation.
Use Markdown headings (#, ##, ###, etc.) in order without skipping levels in documentation.
Follow markdownlint recommendations for Markdown formatting and structure.
Provide code blocks and lists using standard Markdown syntax in documentation.
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change in documentation.
Prefer inline links using 'text' or angle brackets around the URL in documentation.
Ensure blank lines before and after bulleted lists and fenced blocks in documentation.
Ensure tables have a delimiter line below the header row in documentation.
Expand any uncommon acronym on first us...
Files:
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.md
🧠 Learnings (1)
📚 Learning: 2025-12-29T19:00:20.890Z
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Applied to files:
src/runner/error.rstests/bdd/steps/manifest_command.rssrc/runner/mod.rs
🧬 Code graph analysis (1)
tests/bdd/steps/manifest_command.rs (1)
test_support/src/netsuke.rs (1)
run_netsuke_in(55-68)
🪛 LanguageTool
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
[style] ~54-~54: Consider using the typographical ellipsis character here instead.
Context: ...rvation: The thiserror derive macro's #[error(...)] attribute captures struct fields f...
(ELLIPSIS)
[uncategorized] ~106-~106: Possible missing article found.
Context: ...lemented and PR ready for review. - Default subcommand now validates manifest exist...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~315-~315: Loose punctuation mark.
Context: ...d quickstart example. - make check-fmt, make lint, and make test all pass. ...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~324-~324: Consider using a different verb for a more formal wording.
Context: ...afe to re-run. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[grammar] ~378-~378: Make sure that the singular noun after the number ‘3.6’ is correct.
Context: ...xecPlan created for roadmap section 3.6 covering missing manifest error handling (3.6....
(CD_POINT_CD_NN)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, nightly, true)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (10)
src/runner/error.rs (2)
1-17: Module-level lint suppression is well-documented.The
#![allow(...)]usage is justified by the documented constraint:#[expect]fails when the lint does not fire on certain Rust versions, andunfulfilled_lint_expectationscannot itself be expected. The FIXME link to rust-lang/rust#130021 provides traceability. The module is small and narrowly focused, limiting the suppression's scope.
23-40: LGTM!The
RunnerErrorenum is well-structured with proper derives forthiserrorandmiette. Thepathfield retains the full path for programmatic access whilst the display uses the more readabledirectorydescription. Field documentation is present and clear.src/runner/mod.rs (3)
7-9: LGTM!The error submodule is correctly declared and
RunnerErroris publicly re-exported for external consumers.
259-275: LGTM!The directory description logic correctly distinguishes between explicit
-Cusage and the default current directory. The error is properly constructed and converted.
318-332: LGTM!The
file_name()validation ensures the resolved path includes a file name, which is then relied upon bygenerate_ninja. The early return with a clear error message handles edge cases gracefully.tests/bdd/steps/manifest_command.rs (5)
98-111: LGTM!The
run_netsuke_and_storehelper correctly extracts the shared logic from step definitions, addressing the code duplication flagged in PR comments. The function uses?for error propagation and has appropriate documentation.
207-217: LGTM!The
empty_workspacestep correctly creates a clean temporary directory without a Netsukefile, enabling missing-manifest scenarios. World state is properly reset.
230-258: LGTM!The safety validation at lines 236-242 correctly prevents accidental deletion of
/tmpor the system temp directory root. Only proper subdirectories are permitted. The documentation clearly explains the constraints and error conditions.
260-282: LGTM!Both steps correctly delegate to
run_netsuke_and_store, eliminating the duplication flagged in PR comments. Thesplit_whitespace()limitation is clearly documented, noting that quoted arguments with embedded spaces are not supported.
284-297: LGTM!The step correctly retrieves the workspace path and passes it via the
-Cflag. Usingto_string_lossy()is acceptable here since test temporary directories are typically ASCII paths.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md:
- Around line 104-114: The bullet that begins "Default subcommand now validates
manifest existence before loading..." should be changed to start with "The" for
grammatical clarity; update that sentence to read "The default subcommand now
validates manifest existence before loading, producing a clear error with
actionable hint when the manifest is missing." Locate the bullet text in the
document (the line starting "Default subcommand now validates manifest...") and
insert "The" at the start, preserving the rest of the sentence and punctuation.
In @docs/quickstart.md:
- Around line 17-22: Reword the imperative step that uses second-person phrasing
in the quickstart block containing the shell commands (the lines with "mkdir
hello-netsuke" and "cd hello-netsuke") to remove second-person pronouns; for
example, change "Open a terminal and create a new directory for the project:" to
a neutral phrasing like "Open a terminal, then create a new project directory:"
or "Create a new project directory and change into it:" so the commands remain
the same but the surrounding instruction avoids addressing the reader as "you."
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.mdsrc/runner/mod.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
src/runner/mod.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/runner/mod.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization'; suffix ‑lyse in words like 'analyse', 'paralyse', 'catalyse'; suffix -our in 'colour', 'behaviour', 'neighbour'; suffix -re in 'calibre', 'centre', 'fibre'; double 'l' in 'cancelled', 'counsellor', 'cruellest'; maintain 'e' in 'likeable', 'liveable', 'rateable'; suffix -ogue in 'analogue', 'catalogue'. Keep US spelling in external APIs, commands, and libraries (e.g., 'color'). Spell licence file as 'LICENSE'.
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension.
Treat company names as collective nouns in documentation: 'df12 Productions are expanding'.
Avoid first and second person personal pronouns in documentation outside of README.md files.
Write headings in sentence case in documentation.
Use Markdown headings (#, ##, ###, etc.) in order without skipping levels in documentation.
Follow markdownlint recommendations for Markdown formatting and structure.
Provide code blocks and lists using standard Markdown syntax in documentation.
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change in documentation.
Prefer inline links using 'text' or angle brackets around the URL in documentation.
Ensure blank lines before and after bulleted lists and fenced blocks in documentation.
Ensure tables have a delimiter line below the header row in documentation.
Expand any uncommon acronym on first us...
Files:
docs/quickstart.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/quickstart.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/quickstart.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
🧠 Learnings (1)
📚 Learning: 2025-12-29T19:00:20.890Z
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Applied to files:
src/runner/mod.rs
🪛 LanguageTool
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
[style] ~54-~54: Consider using the typographical ellipsis character here instead.
Context: ...rvation: The thiserror derive macro's #[error(...)] attribute captures struct fields f...
(ELLIPSIS)
[uncategorized] ~106-~106: Possible missing article found.
Context: ...lemented and PR ready for review. - Default subcommand now validates manifest exist...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~315-~315: Loose punctuation mark.
Context: ...d quickstart example. - make check-fmt, make lint, and make test all pass. ...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~324-~324: Consider using a different verb for a more formal wording.
Context: ...afe to re-run. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[grammar] ~378-~378: Make sure that the singular noun after the number ‘3.6’ is correct.
Context: ...xecPlan created for roadmap section 3.6 covering missing manifest error handling (3.6....
(CD_POINT_CD_NN)
🔍 Remote MCP Deepwiki
Let me search for more specific information about the PR structure and Clap/Fluent localization patterns:Now let me gather information about the code review concern regarding test duplication:
Summary of Relevant Context for PR Review
Error Handling & Diagnostics Implementation
To implement custom error types with miette diagnostics for Rust, the recommended pattern combines #[derive(Error, Diagnostic)] from thiserror and miette. The Diagnostic trait extends std::error::Error and provides rich metadata including diagnostic codes, help messages, and URLs. Miette allows specifying these attributes via #[diagnostic(...)] macros, making it straightforward to create user-friendly error reports.
In this PR, the RunnerError::ManifestNotFound variant follows this pattern, implementing the error with miette's diagnostic attributes to provide clear, formatted error output when manifest files are missing.
CLI Localization with Fluent (FTL)
FTL (Fluent Translation List) is a localization file format designed to be simple to read while allowing representation of complex natural language concepts like gender, plurals, and conjugations.
The PR adds comprehensive localization support:
- Message structure: Keys like
cli.flag.file.helpandcli.subcommand.build.flag.emit.helpfollow a hierarchical naming convention - Locales supported: en-US and es-ES translations are provided
- Message substitution: The FTL format supports variable interpolation (e.g.,
{ $argument }and{ $value }) which is used in the Clap error messages
The fluent-rs workspace provides Rust crates implementing Project Fluent, a localization system designed to support the full expressive power of natural language translations.
Clap CLI Parser Integration
Clap is a full-featured, fast command-line argument parser for Rust that allows creation of command-line parsers with all features declaratively or procedurally. The new localize_arguments() function in src/cli_l10n.rs extends the localization pipeline to handle argument help text for both global flags and subcommand-specific flags, following the naming convention established in the FTL files.
Code Quality Issue: Test Duplication
The reviewer correctly identified code duplication in tests/bdd/steps/manifest_command.rs. The functions run_netsuke_no_args() and run_netsuke_with_args() duplicate the same sequence:
- Get the temporary workspace path via
get_temp_path(world)? - Call
run_netsuke_in(temp_path, args)? - Store the RunResult with stdout, stderr, and success flag
This duplication should be consolidated into a single private helper function run_netsuke_and_store(world: &TestWorld, args: &[&str]) -> Result<()> as the reviewer suggested, allowing both step functions to delegate to it while preserving the existing BDD step definitions.
Key Files Modified
- src/runner/error.rs: New error module with
RunnerError::ManifestNotFound(with miette Diagnostic trait) - src/cli_l10n.rs: New
localize_arguments()helper for argument help text localization - locales/*.ftl: Expanded message keys for CLI flags, subcommands, and Clap error handling
- tests/bdd/steps/manifest_command.rs: BDD step definitions with refactoring opportunity
- tests/features/missing_manifest.feature: Gherkin scenarios validating error handling,,,,
Review Focus Areas:
- Error diagnostics: Verify
RunnerError::ManifestNotFoundrenders correctly via miette in various terminal contexts - Localization coverage: Confirm all CLI flags and subcommands have corresponding FTL keys in both en-US and es-ES
- Test refactoring: Address the identified duplication in manifest_command.rs BDD steps to improve maintainability
- Argument localization: Validate that the
localize_arguments()function correctly resolves keys and applies help text to both global and subcommand arguments
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, nightly, true)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (9)
docs/quickstart.md (2)
92-96: LGTM!The step-by-step instructions are clear and use appropriate imperative mood for tutorial content.
161-178: LGTM!The troubleshooting section provides clear, actionable guidance for common issues. Platform-specific installation commands are helpful.
src/runner/mod.rs (3)
7-9: LGTM!Clean module organisation with public re-export of
RunnerErrorfor downstream use.
250-274: Well-structured error handling.The existence check correctly uses
.ok_or_else()rather than.expect(), aligning with coding guidelines. The directory description logic provides helpful context for users.Note: Lines 254–257 defensively re-check
file_name(), whichresolve_manifest_path()already validates at line 324. This redundancy is acceptable as a safety net but could be simplified if desired.
314-331: LGTM!The added validation ensures the manifest path always includes a filename, providing a clear error message when the path is malformed. Uses fallible error handling correctly.
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md (4)
1-51: LGTM!The ExecPlan is well-structured with clear progress tracking, purpose statement, and acceptance criteria. The living document sections (Progress, Surprises, Decision Log, Outcomes) provide good traceability.
70-101: LGTM!The decision log provides excellent traceability with clear rationale for each architectural choice. The decision to detect missing manifests at the runner level (rather than in the manifest loader) is well-justified — it enables access to CLI context for constructing helpful directory descriptions.
359-375: LGTM!The critical files table provides a clear summary of all modifications with appropriate header/delimiter formatting.
54-59: Correct ExecPlan documentation—lint suppression is insrc/runner/error.rs, notsrc/runner/mod.rs.The ExecPlan (lines 58–59) inaccurately states that
#![allow(unused_assignments)]was required insrc/runner/mod.rs. The actual suppression is insrc/runner/error.rs(lines 13–17) and includes three lints:unused_assignments,clippy::allow_attributes, andclippy::allow_attributes_without_reason.The
#[allow]approach is justified here—not a coding guideline violation—because#[expect]fails when a lint doesn't fire across Rust versions. The comment (lines 9–12) documents this limitation and references the upstream issue (rust-lang/rust#130021). Update the ExecPlan to reflect the correct file location and explain the version-dependent constraint.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/quickstart.md`:
- Line 70: Replace the awkward sentence "This completes a first Netsuke build."
with "This completes the first Netsuke build." in the docs/quickstart.md content
so the phrasing is grammatically correct and reads naturally.
♻️ Duplicate comments (1)
docs/quickstart.md (1)
159-159: Remove second-person pronoun.Line 159 uses "your", which violates the coding guideline requiring avoidance of second-person pronouns in documentation outside README.md files. Change "your build dependency graph" to "the build dependency graph".
Proposed fix
-- Try `netsuke graph` to visualize your build dependency graph +- Try `netsuke graph` to visualize the build dependency graphAs per coding guidelines, avoid first and second person personal pronouns in documentation outside of README.md files.
🤖 Prompt for AI Agents
In `@docs/quickstart.md` at line 159, replace the second-person pronoun "your" with the article "the" so the line reads "Try `netsuke graph` to visualize the build dependency graph" to comply with the documentation guideline prohibiting second-person pronouns outside README.md.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.md
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization'; suffix ‑lyse in words like 'analyse', 'paralyse', 'catalyse'; suffix -our in 'colour', 'behaviour', 'neighbour'; suffix -re in 'calibre', 'centre', 'fibre'; double 'l' in 'cancelled', 'counsellor', 'cruellest'; maintain 'e' in 'likeable', 'liveable', 'rateable'; suffix -ogue in 'analogue', 'catalogue'. Keep US spelling in external APIs, commands, and libraries (e.g., 'color'). Spell licence file as 'LICENSE'.
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension.
Treat company names as collective nouns in documentation: 'df12 Productions are expanding'.
Avoid first and second person personal pronouns in documentation outside of README.md files.
Write headings in sentence case in documentation.
Use Markdown headings (#, ##, ###, etc.) in order without skipping levels in documentation.
Follow markdownlint recommendations for Markdown formatting and structure.
Provide code blocks and lists using standard Markdown syntax in documentation.
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change in documentation.
Prefer inline links using 'text' or angle brackets around the URL in documentation.
Ensure blank lines before and after bulleted lists and fenced blocks in documentation.
Ensure tables have a delimiter line below the header row in documentation.
Expand any uncommon acronym on first us...
Files:
docs/quickstart.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/quickstart.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/quickstart.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
🪛 LanguageTool
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
[style] ~54-~54: Consider using the typographical ellipsis character here instead.
Context: ...rvation: The thiserror derive macro's #[error(...)] attribute captures struct fields f...
(ELLIPSIS)
[uncategorized] ~315-~315: Loose punctuation mark.
Context: ...d quickstart example. - make check-fmt, make lint, and make test all pass. ...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~324-~324: Consider using a different verb for a more formal wording.
Context: ...afe to re-run. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[grammar] ~378-~378: Make sure that the singular noun after the number ‘3.6’ is correct.
Context: ...xecPlan created for roadmap section 3.6 covering missing manifest error handling (3.6....
(CD_POINT_CD_NN)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/quickstart.md`:
- Line 44: Update the heading text "Step 3: Run netsuke" to "Step 3: Run
Netsuke" so the tool name is capitalized consistently; locate the heading string
"Step 3: Run netsuke" in the document and change the lowercase "netsuke" to
"Netsuke".
- Line 159: The phrase "visualize your build dependency graph" uses a
second-person pronoun; update the sentence in docs/quickstart.md (line
containing "Try `netsuke graph` to visualize your build dependency graph") to
remove "your" by changing it to "visualize the build dependency graph" so the
documentation avoids second-person pronouns and follows the project's
documentation guidelines.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/quickstart.md
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization'; suffix ‑lyse in words like 'analyse', 'paralyse', 'catalyse'; suffix -our in 'colour', 'behaviour', 'neighbour'; suffix -re in 'calibre', 'centre', 'fibre'; double 'l' in 'cancelled', 'counsellor', 'cruellest'; maintain 'e' in 'likeable', 'liveable', 'rateable'; suffix -ogue in 'analogue', 'catalogue'. Keep US spelling in external APIs, commands, and libraries (e.g., 'color'). Spell licence file as 'LICENSE'.
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension.
Treat company names as collective nouns in documentation: 'df12 Productions are expanding'.
Avoid first and second person personal pronouns in documentation outside of README.md files.
Write headings in sentence case in documentation.
Use Markdown headings (#, ##, ###, etc.) in order without skipping levels in documentation.
Follow markdownlint recommendations for Markdown formatting and structure.
Provide code blocks and lists using standard Markdown syntax in documentation.
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change in documentation.
Prefer inline links using 'text' or angle brackets around the URL in documentation.
Ensure blank lines before and after bulleted lists and fenced blocks in documentation.
Ensure tables have a delimiter line below the header row in documentation.
Expand any uncommon acronym on first us...
Files:
docs/quickstart.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/quickstart.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/quickstart.md
🔍 Remote MCP Deepwiki
Based on the available context, I can provide a comprehensive summary of the relevant information for this PR review:
Summary of Additional Context for PR #256 Review
Based on analysis of the PR materials and project information, here are the key relevant facts for reviewing this pull request:
Project Overview
The Netsuke project is a Rust-based build system that uses manifest files called "Netsukefile" (similar to Makefile) to define build targets. This PR (PR #256) implements the final three items of roadmap section 3.6 (Onboarding and defaults):
- 3.6.1: Missing manifest error handling with guided error messages
- 3.6.2: Localized help output for CLI flags and subcommands
- 3.6.3: Hello World quickstart guide
Key Architectural Changes in This PR
Error Handling:
- Introduces a new
src/runner/error.rsmodule withRunnerError::ManifestNotFoundenum variant - The error includes fields:
manifest_name: String,directory: String,path: PathBuf - Uses
miette::Diagnosticfor rich error rendering with diagnostic codes and help hints - Manifest existence check added to
generate_ninja()insrc/runner/mod.rsbefore loading
Localization Infrastructure:
- New
localize_arguments()helper function added tosrc/cli_l10n.rsto localize flag help text - Localizes based on context:
cli.flag.{arg_id}.helpfor top-level flags,cli.subcommand.{name}.flag.{arg_id}.helpfor subcommand-specific flags - Applied to both global flags and subcommand arguments in localization flow
Testing Coverage:
- BDD feature file
tests/features/missing_manifest.featuretests three scenarios: empty workspace, custom manifest path, and directory flag behavior - New BDD step helpers in
tests/bdd/steps/manifest_command.rsincludingrun_netsuke_and_store(),empty_workspace(), and directory flag support - Updated
tests/logging_stderr_tests.rsto verify miette diagnostic output for missing manifests
Code Review Observations
Flagged Issue from Reviewer:
CodeRabbitAI identified code duplication in tests/bdd/steps/manifest_command.rs where run_netsuke_no_args() and run_netsuke_with_args() share identical logic. The suggestion is to extract a private helper run_netsuke_and_store() to reduce duplication while preserving step behavior and clippy annotations.
Documentation Changes:
- New
docs/quickstart.mdwith Hello World example covering prerequisites, manifest structure, build commands, and troubleshooting - Updated
docs/users-guide.mdto show structured error output with hints and path examples - New
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mdwith detailed planning and decision logs
Localization Additions:
locales/en-US/messages.ftl: 18 new keys for flag help, subcommand descriptions, and Clap error messageslocales/es-ES/messages.ftl: Spanish translations matching English keys with updated value-validation wording
Testing Plan Specification
The PR objectives specify:
cargo testto verify all unit and integration tests- BDD scenarios for missing manifest: empty directory, custom filename,
-Cdirectory flag - Locale verification for en-US and es-ES message keys
- Running the quickstart example to confirm default targets build
[::web_search::],
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, nightly, true)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (1)
docs/quickstart.md (1)
70-70: Past issue now resolved.Previous review comments flagged the phrasing "This completes a first Netsuke build" as awkward. The current text correctly reads "This completes the first Netsuke build." The grammatical issue has been addressed.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/quickstart.md`:
- Line 169: Replace the example command string "netsuke -f
path/to/your/manifest.yml" in docs/quickstart.md with a neutral placeholder that
removes the second-person pronoun, e.g. "netsuke -f path/to/manifest.yml";
update the literal in the markdown so the example no longer contains "your".
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/quickstart.md
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization'; suffix ‑lyse in words like 'analyse', 'paralyse', 'catalyse'; suffix -our in 'colour', 'behaviour', 'neighbour'; suffix -re in 'calibre', 'centre', 'fibre'; double 'l' in 'cancelled', 'counsellor', 'cruellest'; maintain 'e' in 'likeable', 'liveable', 'rateable'; suffix -ogue in 'analogue', 'catalogue'. Keep US spelling in external APIs, commands, and libraries (e.g., 'color'). Spell licence file as 'LICENSE'.
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension.
Treat company names as collective nouns in documentation: 'df12 Productions are expanding'.
Avoid first and second person personal pronouns in documentation outside of README.md files.
Write headings in sentence case in documentation.
Use Markdown headings (#, ##, ###, etc.) in order without skipping levels in documentation.
Follow markdownlint recommendations for Markdown formatting and structure.
Provide code blocks and lists using standard Markdown syntax in documentation.
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change in documentation.
Prefer inline links using 'text' or angle brackets around the URL in documentation.
Ensure blank lines before and after bulleted lists and fenced blocks in documentation.
Ensure tables have a delimiter line below the header row in documentation.
Expand any uncommon acronym on first us...
Files:
docs/quickstart.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/quickstart.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/quickstart.md
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: build-test (ubuntu-latest, nightly, true)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/quickstart.md`:
- Around line 12-13: Update the prerequisite text in docs/quickstart.md by
removing the second-person pronoun "your": change the phrase "in your PATH
(install via your package manager, e.g., `apt install ninja-build` or `brew
install ninja`)" to a neutral form such as "in the system PATH (install via the
package manager, e.g., `apt install ninja-build` or `brew install ninja`)" so
the line no longer uses "your" and complies with documentation guidelines.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/quickstart.md
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization'; suffix ‑lyse in words like 'analyse', 'paralyse', 'catalyse'; suffix -our in 'colour', 'behaviour', 'neighbour'; suffix -re in 'calibre', 'centre', 'fibre'; double 'l' in 'cancelled', 'counsellor', 'cruellest'; maintain 'e' in 'likeable', 'liveable', 'rateable'; suffix -ogue in 'analogue', 'catalogue'. Keep US spelling in external APIs, commands, and libraries (e.g., 'color'). Spell licence file as 'LICENSE'.
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension.
Treat company names as collective nouns in documentation: 'df12 Productions are expanding'.
Avoid first and second person personal pronouns in documentation outside of README.md files.
Write headings in sentence case in documentation.
Use Markdown headings (#, ##, ###, etc.) in order without skipping levels in documentation.
Follow markdownlint recommendations for Markdown formatting and structure.
Provide code blocks and lists using standard Markdown syntax in documentation.
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change in documentation.
Prefer inline links using 'text' or angle brackets around the URL in documentation.
Ensure blank lines before and after bulleted lists and fenced blocks in documentation.
Ensure tables have a delimiter line below the header row in documentation.
Expand any uncommon acronym on first us...
Files:
docs/quickstart.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/quickstart.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/quickstart.md
🔍 Remote MCP
I'll search for relevant technical information to help review this PR effectively, focusing on the key technologies and patterns being introduced.## Summary of Additional Context for PR Review
Based on my research into the key technologies used in this PR, here is the relevant context:
Error Handling with miette & thiserror
The miette crate integrates well with thiserror, which is a standard approach for defining error types with miette Diagnostics. The PR's implementation of RunnerError::ManifestNotFound follows this pattern correctly:
- The miette Diagnostic trait adds rich metadata to errors that can be used by Report to print human-friendly error messages
- miette includes Report and IntoDiagnostic tools that work together to convert regular std::error::Errors into Diagnostics
- The PR's approach of creating a diagnostic-enabled error in
src/runner/error.rsand rendering it viamiette::Reportinmain.rsis the correct pattern
Fluent Localization Format
FTL (Fluent Translation List) is a localization file format designed to be simple to read while allowing representation of complex natural language concepts like gender, plurals, and conjugations. The PR's .ftl file additions follow standard Fluent conventions:
- Messages in FTL consist of a message identifier and a value, with the simplest form being straightforward key-value pairs
- Variables (prefixed with
$) should never be translated, which the PR respects (e.g.,{ $argument },{ $value })
Clap Integration for Argument Localization
Clap provides functionality to generate parsing logic for arguments and provides a clean CLI with argument explanations. The PR's localize_arguments() function in src/cli_l10n.rs implements argument help localization by:
- Iterating over Clap's arguments and setting localized help text
- Using subcommand context to determine localization keys (
cli.flag.{arg_id}.helpvscli.subcommand.{name}.flag.{arg_id}.help)
This approach is sound for Clap's builder pattern and does not conflict with standard Clap usage patterns.
Code Duplication Issue (Reviewer Comment)
The reviewer identified valid code duplication in tests/bdd/steps/manifest_command.rs. The suggested refactoring to extract a run_netsuke_and_store() helper function is reasonable and follows DRY principles. Both run_netsuke_no_args() and run_netsuke_with_args() perform nearly identical operations:
- Get temp path
- Call
run_netsuke_in() - Construct and store
RunResult
The provided extraction strategy preserves test semantics while reducing duplication.
[::web_search::]
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, nightly, true)
- GitHub Check: build-test (ubuntu-latest, stable)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Address PR review feedback: - Replace all second-person pronouns (you, your, you've) with third-person or impersonal alternatives per documentation style guide - Add comma after "target" in line 42 for clarity - Add periods to list items for consistency 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…odule Address PR review feedback about module-level #[allow]: - Extract RunnerError enum to dedicated src/runner/error.rs submodule - Scope lint suppressions to the error submodule only, not the entire runner module - Re-export RunnerError from mod.rs for external access This isolates the version-dependent thiserror/miette derive macro lint false positive to the smallest possible scope while maintaining the same public API. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address PR review feedback: - Fix safeguard to prevent deleting /tmp itself (require subdirectory) - Change doc comment from "# Panics" to "# Errors" to match actual behaviour (returns Err, not panic) - Document split_whitespace() limitation for arguments with embedded spaces 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address PR review feedback: - Convert all headings to sentence case per documentation style guide - Add "and" before final items in flag lists for proper Oxford comma usage 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address PR review feedback: - Convert all headings to sentence case per documentation style guide - Fix awkward "a first Netsuke build" phrasing to "a Netsuke build" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address PR review feedback: - Replace .expect() with .ok_or_else() to propagate errors instead of panicking, as expect() is forbidden outside tests per coding guidelines - The validation in resolve_manifest_path() guarantees file_name() is Some, but error propagation is preferred over panicking in production 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address PR review feedback: - Add "The" before "default subcommand" in outcomes section for grammatical completeness 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address PR review feedback:
- Replace imperative constructions ("Open a terminal", "Create a file",
"Run netsuke", "Check the result", etc.) with passive or declarative
forms to comply with documentation style guide requirement to avoid
second-person pronouns outside README.md
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address PR review feedback:
- Replace imperative constructions ("Open a terminal", "Create a file",
"Run netsuke", "Check the result", etc.) with passive or declarative
forms to comply with documentation style guide requirement to avoid
second-person pronouns outside README.md
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address PR review feedback:
- Replace imperative constructions ("Open a terminal", "Create a file",
"Run netsuke", "Check the result", etc.) with passive or declarative
forms to comply with documentation style guide requirement to avoid
second-person pronouns outside README.md
- Capitalize "Netsuke" consistently in heading
- Change "a first" to "the first" for grammatical clarity
- Remove "your" from "visualize your build dependency graph"
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Updated the example command in the quickstart guide to use the correct manifest file path `path/to/manifest.yml` instead of `path/to/your/manifest.yml` for clarity and accuracy. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Updated the description of the Ninja build tool requirement to specify it must be in the system PATH, and slightly improved wording for installation guidance. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
force-pushed
the
terragon/ensure-default-subcommand-manifest-wmtqpr
branch
from
80b1469 to
4dd3f77
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@docs/quickstart.md`:
- Around line 74-75: The sentence "Netsuke supports Jinja templating for dynamic
manifests. The `Netsukefile` can be updated as follows:" exceeds the 80-column
doc guideline; reflow it into two lines under 80 characters each (for example,
split after "manifests.") so the text wraps cleanly at 80 columns while
preserving wording and the inline code token `Netsukefile`.
- Around line 112-113: The paragraph "For more complex builds, Netsuke can
process multiple files. Some input files can be created:" exceeds the 80-column
documentation guideline; rewrap this sentence into two lines under 80 characters
(e.g., split after "files.") so each line is <=80 columns in docs/quickstart.md
and ensure punctuation and spacing remain correct.
In `@docs/users-guide.md`:
- Line 70: Change the sentence "You can specify a different manifest path using
the `-f` or `--file` option:" to remove the second-person pronoun; update it to
a passive or imperative form such as "A different manifest path can be specified
using the `-f` or `--file` option:" in docs/users-guide.md so it no longer uses
"You".
- Around line 61-62: Replace the phrase "guiding you:" in the sentence that
references `Netsukefile` with a neutral phrasing that avoids second-person
pronouns (e.g., change "guiding you:" to "with guidance:"), so the line reads:
"If no `Netsukefile` is found, Netsuke will provide a helpful error message with
guidance:"; update only that fragment in the `docs/users-guide.md` content.
In `@examples/hello-world/Netsukefile`:
- Around line 14-15: The command string in the Netsukefile uses single quotes
around the Jinja expansion ("echo '{{ greeting }}!' > greeting.txt"), which will
break if greeting contains a single quote; update the command that generates
greeting.txt (the command value) to produce a shell-safe value by either
switching to a double-quoted Jinja expansion or escaping/quoting the expanded
value (e.g., emit a double-quoted string or use a shell-safe printer like
printf) so that the greeting variable is safely handled when written to
greeting.txt.
In `@locales/es-ES/messages.ftl`:
- Around line 41-44: Translate the English explanatory comment above the message
id clap-error-value-validation into Spanish to match the rest of the file;
replace the three English lines ("# Note: value-validation uses distinct wording
from invalid-value to differentiate", "# custom validator failures
(ErrorKind::ValueValidation) from type mismatches", "#
(ErrorKind::InvalidValue).") with an equivalent Spanish explanation, keeping the
structure and comment markers intact and leaving the clap-error-value-validation
entry unchanged.
In `@tests/bdd/steps/manifest_command.rs`:
- Around line 230-258: Canonicalize the incoming path before performing the
safety checks in empty_workspace_at_path: compute a resolved_path by calling
Path::canonicalize (or if that fails, resolve via
current_dir().join(path).canonicalize or reject the input) and also canonicalize
the system temp_dir; then replace uses of dir, is_safe_tmp and is_safe_temp to
compare resolved_path.starts_with("/tmp") and
resolved_path.starts_with(&canonical_temp_dir) (or equivalent) so traversal like
/tmp/../etc is rejected; ensure you use the canonicalized path for subsequent
fs::remove_dir_all and fs::create_dir_all operations and update variable
references accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.mddocs/roadmap.mddocs/users-guide.mdexamples/hello-world/Netsukefileexamples/hello-world/README.mdexamples/hello-world/input.txtlocales/en-US/messages.ftllocales/es-ES/messages.ftlsrc/cli.rssrc/cli_l10n.rssrc/main.rssrc/runner/error.rssrc/runner/mod.rstests/bdd/steps/manifest_command.rstests/features/missing_manifest.featuretests/logging_stderr_tests.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
In Rust, extract meaningfully named helper functions when a function is too long, adhering to separation of concerns and CQRS.
In Rust, group related parameters in meaningfully named structs when a function has too many parameters.
In Rust, consider usingArcto reduce data returned when a function is returning a large error.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs in Rust using Rustdoc comments (///) so documentation can be generated withcargo doc.
In Rust, prefer immutable data and avoid unnecessarymutbindings.
In Rust, handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly.
In Rust, place function attributes after doc comments.
In Rust, do not usereturnin single-line functions.
In Rust, use predicate functions for conditional criteria with more than two branches.
Lints in Rust must not be silenced except as a last resort.
Lint rule suppressions in Rust must be tightly scoped and include a clear reason.
In Rust, preferexpectoverallow.
In Rust tests, userstestfixtures for shared setup.
In Rust tests, replace duplicated tests with#[rstest(...)]parameterized cases.
In Rust, prefermockallfor mocks and stubs.
In Rust, useconcat!()to combine long string literals rather than escaping newlines with a backslash.
In Rust, prefer single line versions of functions where appropriate (e.g.,pub fn new(id: u64) -> Self { Self(id) }instead of multi-line).
Use NewTypes in Rust to model domain values and eliminate 'integer soup'. Reach fornewt-hypewhen introducing many homogeneous wrappers; add small shims for string-backed wrappers. For path-centric wrappers implementAsRef<Path>along...
Files:
tests/logging_stderr_tests.rssrc/main.rssrc/cli.rssrc/runner/error.rssrc/runner/mod.rstests/bdd/steps/manifest_command.rssrc/cli_l10n.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/logging_stderr_tests.rssrc/main.rssrc/cli.rssrc/runner/error.rssrc/runner/mod.rstests/bdd/steps/manifest_command.rssrc/cli_l10n.rs
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
docs/**/*.md: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state.
Documentation must use en-GB-oxendict spelling and grammar ("-ize" / "-yse" / "-our"), with exception for the LICENSE filename.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization'; suffix ‑lyse in words like 'analyse', 'paralyse', 'catalyse'; suffix -our in 'colour', 'behaviour', 'neighbour'; suffix -re in 'calibre', 'centre', 'fibre'; double 'l' in 'cancelled', 'counsellor', 'cruellest'; maintain 'e' in 'likeable', 'liveable', 'rateable'; suffix -ogue in 'analogue', 'catalogue'. Keep US spelling in external APIs, commands, and libraries (e.g., 'color'). Spell licence file as 'LICENSE'.
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension.
Treat company names as collective nouns in documentation: 'df12 Productions are expanding'.
Avoid first and second person personal pronouns in documentation outside of README.md files.
Write headings in sentence case in documentation.
Use Markdown headings (#, ##, ###, etc.) in order without skipping levels in documentation.
Follow markdownlint recommendations for Markdown formatting and structure.
Provide code blocks and lists using standard Markdown syntax in documentation.
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text.
Use '-' as the first level bullet and renumber lists when items change in documentation.
Prefer inline links using 'text' or angle brackets around the URL in documentation.
Ensure blank lines before and after bulleted lists and fenced blocks in documentation.
Ensure tables have a delimiter line below the header row in documentation.
Expand any uncommon acronym on first us...
Files:
docs/roadmap.mddocs/users-guide.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Tables and headings in Markdown must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/roadmap.mddocs/users-guide.mdexamples/hello-world/README.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/users-guide.mdexamples/hello-world/README.mddocs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.mddocs/quickstart.md
🧠 Learnings (1)
📚 Learning: 2025-12-29T19:00:20.890Z
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Applied to files:
tests/logging_stderr_tests.rssrc/main.rssrc/cli.rssrc/runner/error.rssrc/runner/mod.rstests/bdd/steps/manifest_command.rssrc/cli_l10n.rs
🧬 Code graph analysis (1)
tests/bdd/steps/manifest_command.rs (2)
tests/bdd/fixtures/mod.rs (1)
world(167-169)test_support/src/netsuke.rs (1)
run_netsuke_in(55-68)
🪛 LanguageTool
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md
[style] ~54-~54: Consider using the typographical ellipsis character here instead.
Context: ...rvation: The thiserror derive macro's #[error(...)] attribute captures struct fields f...
(ELLIPSIS)
[uncategorized] ~315-~315: Loose punctuation mark.
Context: ...d quickstart example. - make check-fmt, make lint, and make test all pass. ...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~324-~324: Consider using a different verb for a more formal wording.
Context: ...afe to re-run. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[grammar] ~378-~378: Make sure that the singular noun after the number ‘3.6’ is correct.
Context: ...xecPlan created for roadmap section 3.6 covering missing manifest error handling (3.6....
(CD_POINT_CD_NN)
🔍 Remote MCP
Let me try a more direct approach to find information about miette error formatting and the Clap CLI framework patterns mentioned in the PR.Based on my search results, I've gathered relevant context about the technologies and patterns used in this PR. Let me compile the findings into a concise summary for PR review.
Contextual Findings for PR Review
Based on research of the Rust ecosystem patterns and libraries referenced in this PR, here are the key findings relevant to your review:
Error Handling with miette (src/main.rs, src/runner/error.rs)
The miette crate adds rich metadata to errors that enables fancy graphical diagnostic output with ANSI/Unicode text support. The PR's implementation of RunnerError::ManifestNotFound with the #[derive(Diagnostic)] macro is aligned with recommended patterns. Miette plays nicely with thiserror for defining error types, which the PR correctly combines.
Key points for review:
- The
src/runner/error.rsdiagnostic code derives bothErrorandDiagnostic, following best practices - Miette enables defining error types with additional context and supports reporting those errors, which matches the PR's
ManifestNotFoundvariant containingmanifest_name,directory, andpathfields - The
main.rsdowncast-and-report pattern aligns with standard miette usage for CLI applications
CLI Localization Architecture
The PR's use of Fluent message files (.ftl) is a modern localization standard. Project Fluent is a localization system designed to unleash the entire expressive power of natural language translations, keeping simple things simple and making complex things possible.
Key points for review:
- Multi-language support in Clap can be implemented by dynamically setting help strings based on the user's locale before running the CLI parser, which appears to match the
localize_arguments()helper added insrc/cli_l10n.rs - The addition of structured localization keys (e.g.,
cli.flag.file.help,cli.subcommand.build.flag.targets.help) follows the standard key-value mapping pattern for Fluent
Code Duplication Fix (tests/bdd/steps/manifest_command.rs)
The reviewer's suggestion to extract run_netsuke_and_store() as a helper addresses a valid DRY principle violation. The proposed refactor:
- Centralizes common logic: temp path retrieval →
run_netsuke_in()invocation → result storage - Preserves test semantics and step annotations
- Reduces maintenance burden for future changes to the test harness
Review consideration: This refactor is low-risk and aligns with test best practices for CLI integration tests.
Manifest Path Validation (src/runner/mod.rs)
The runtime check for missing manifest files in generate_ninja() with pre-load validation in resolve_manifest_path() enforces stricter validation before attempting manifest loading, reducing ambiguous error states downstream.
[::web_search::]
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: release / build-macos (x86_64-apple-darwin, macos-15-intel, macos-x86_64, macos-x86_64) / Build macos artefacts
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- GitHub Check: release / build-linux (aarch64-unknown-linux-gnu, linux-arm64, arm64) / Build linux artefacts
- GitHub Check: release / build-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: release / build-macos (aarch64-apple-darwin, macos-15, macos-arm64, macos-aarch64) / Build macos artefacts
- GitHub Check: release / build-windows (aarch64-pc-windows-msvc, arm64, arm64, windows-aarch64) / Build windows artefacts
- GitHub Check: Sourcery review
- GitHub Check: build-test (ubuntu-latest, nightly, true)
- GitHub Check: build-test (ubuntu-latest, stable)
🔇 Additional comments (28)
examples/hello-world/README.md (2)
1-1: Convert the heading to sentence case.The heading "Hello World Example" uses title case. Change it to "Hello world example" to comply with the documentation guideline requiring sentence case for all headings.
📝 Proposed fix
-# Hello World Example +# Hello world example🤖 Prompt for AI agents
In `@examples/hello-world/README.md` at line 1, change the heading from "# Hello World Example" to "# Hello world example" to convert it to sentence case as required by the documentation guidelines.As per coding guidelines, write headings in sentence case.
Likely an incorrect or invalid review comment.
20-20: Convert the heading to sentence case.Change "Expected Output" to "Expected output" to comply with sentence case requirements for documentation headings.
📝 Proposed fix
-## Expected Output +## Expected output🤖 Prompt for AI agents
In `@examples/hello-world/README.md` at line 20, change the heading from "## Expected Output" to "## Expected output" to convert it to sentence case.As per coding guidelines, write headings in sentence case.
Likely an incorrect or invalid review comment.
docs/execplans/3-6-1-ensure-default-subcommand-builds-manifest-defaults.md (2)
1-380: Document structure and content are sound.The ExecPlan is comprehensive, with progress tracking, decision log, concrete steps, and acceptance criteria. All past spelling and formatting issues appear resolved. The document follows the required structure and provides clear guidance for implementation.
203-206: Remove stray "and" from list item.Line 205 has "and
fetch-default-deny" but line 206 continues with more items ("Build subcommand:emitandtargets"). The Oxford comma rule applies to the final item of a list, but here the list continues past the "and". Move the conjunction to the actual final item or restructure:- Root flags: `file`, `directory`, `jobs`, `verbose`, `locale`, - `fetch-allow-scheme`, `fetch-allow-host`, `fetch-block-host`, - and `fetch-default-deny` - - Build subcommand: `emit` and `targets` + `fetch-allow-scheme`, `fetch-allow-host`, `fetch-block-host`, and + `fetch-default-deny` + - Build subcommand: `emit`, `targets` - Manifest subcommand: output file argumentLikely an incorrect or invalid review comment.
examples/hello-world/Netsukefile (2)
1-19: Minimal working example is well-structured.The manifest demonstrates core Netsuke features (variables, targets, sources, defaults) without external dependencies. The text-processing approach is portable and appropriate for a quickstart.
10-12: Scalarsourcesis valid syntax.The Netsuke manifest supports both scalar and list formats for the
sourcesfield, as evidenced by examples/basic_c.yml where scalar sources appear on lines 18, 22 (and list format on lines 26–28). The usagesources: input.txtis correct.tests/logging_stderr_tests.rs (1)
14-24: Test correctly validates error output routing.The test verifies that
ManifestNotFounddiagnostics appear on stderr (not stdout) when running in a directory without a manifest. The assertions are clear and the comment on line 17 provides useful context about miette rendering.src/main.rs (2)
44-53: Diagnostic reporting for runner errors is well-implemented.The downcast-and-report pattern correctly separates
RunnerError(with miette diagnostics) from other error types. Using{report:?}produces the full diagnostic output including help text.One observation:
drop(writeln!(...))silently discards any stderr write failure. This is acceptable for a CLI application where stderr write failures are rare and unrecoverable, but document the intent with a brief comment if desired.
5-8: Imports are correctly structured.The new imports for
miette::Reportandstd::io::Writesupport the diagnostic reporting functionality added below.src/cli.rs (1)
246-256: ArgMatches cloning preserves values for configuration merging.The change correctly addresses the issue where
from_arg_matches_mutconsumes values fromArgMatches, leaving them unavailable for subsequentmerge_with_configoperations. Cloning before parsing ensures the original matches remain intact.The variable naming (
matches_for_merge,matches_for_parse) clearly communicates intent.src/runner/error.rs (1)
1-40: LGTM!The isolation of derive-macro-affected code into a dedicated submodule is the correct approach to scope the lint suppression narrowly. The FIXME with upstream issue link, detailed justification, and comprehensive documentation on the error type and its fields are all exemplary.
tests/features/missing_manifest.feature (1)
1-20: LGTM!The scenarios provide comprehensive coverage of missing manifest error handling. The use of isolated temporary workspaces and dynamic directory references avoids the hardcoded path issues flagged in previous reviews.
src/runner/mod.rs (3)
7-9: LGTM!The submodule declaration and re-export cleanly expose
RunnerErrorfor use across the codebase and tests whilst keeping the derive-macro lint suppression isolated.
250-274: LGTM!The missing manifest check provides a clear diagnostic with contextual information. The fallible
file_name()extraction withok_or_elseis defensive programming that gracefully handles any edge cases where the precondition might not hold.
314-331: LGTM!The explicit validation that the resolved path includes a file name catches malformed inputs early and surfaces a clear error to the user rather than producing confusing behaviour downstream.
src/cli_l10n.rs (2)
43-46: Use en-GB-oxendict spelling in documentation.The doc comment uses "Localise" which is correct for en-GB-oxendict. However, the function name
localize_argumentsuses American spelling. For consistency with the codebase's British English documentation standard, the doc comments are fine, but note this creates a mismatch with the function name (which follows Rust ecosystem conventions where American spellings predominate in APIs).
47-67: LGTM!The implementation cleanly handles both top-level and subcommand argument localization with a consistent three-way pattern that matches the existing
about/long_aboutlocalization approach.tests/bdd/steps/manifest_command.rs (4)
98-111: LGTM!The
run_netsuke_and_storehelper consolidates the previously duplicated logic for executing netsuke and storing results. This addresses the code duplication concern raised in the PR comments and improves maintainability.
207-217: LGTM!The step correctly creates an isolated temporary workspace without a manifest file, following the same state management pattern as
minimal_workspace.
260-282: LGTM!The step functions correctly delegate to the shared
run_netsuke_and_storehelper, and thesplit_whitespace()limitation is clearly documented for future maintainers.
284-298: LGTM!The step correctly retrieves the dynamically created workspace path and passes it via the
-Cflag, enabling isolated testing of directory flag behaviour without hardcoded paths.locales/en-US/messages.ftl (4)
7-16: LGTM!Root-level flag help entries are well-structured with consistent key naming (
cli.flag.{id}.help) and clear, concise descriptions. The snake_case keys align correctly with Clap argument IDs.
18-26: LGTM!Section comment improves file organisation. Subcommand descriptions are clear and follow the established pattern.
28-33: LGTM!Subcommand flag help entries follow the consistent naming pattern and provide clear guidance.
35-44: LGTM!The Clap error messages are well-structured with correct Fluent interpolation syntax. The explanatory comments (lines 41-43) clarifying the distinction between
ValueValidationandInvalidValueerror kinds are helpful for future maintainers.locales/es-ES/messages.ftl (3)
7-16: LGTM!Spanish translations for root-level flags are accurate and natural. The snake_case keys correctly match the Clap argument IDs, resolving the previous localisation issue.
18-26: LGTM!Subcommand descriptions are well-translated with consistent terminology.
28-33: LGTM!Build and manifest subcommand flag help entries are accurately translated.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Added `normalize_path` function to resolve `.` and `..` in paths without filesystem dependence. - Applied path normalization before safety checks for test workspace paths to prevent traversal outside allowed temp directories. - Improved safety by ensuring test directories are proper subdirectories, enhancing test reliability and security. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
…t command Replaced `echo` with `printf` to properly handle the output of the greeting variable in greeting.txt, improving robustness and preventing issues with special characters. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/bdd/steps/manifest_command.rs`:
- Around line 236-280: The safety check uses normalize_path which doesn't
resolve symlinks—change empty_workspace_at_path to canonicalize the requested
path (using fs::canonicalize) before enforcing the temp-root guard and
performing removal/creation, and also canonicalize std::env::temp_dir() for a
proper comparison; if the target path does not yet exist, fall back to using the
normalized path but ensure any existing ancestor symlinks are resolved when
possible, then use the canonicalized path for the
exists/remove_dir_all/create_dir_all calls and for the starts_with/inequality
checks so symlink escapes of /tmp or the system temp dir cannot bypass the
guard.
- Around line 306-320: The step currently passes world.temp_dir to the -C flag
but must pass the workspace path created by empty_workspace_at_path; add a new
field on TestWorld (e.g., workspace_path: Option<PathBuf>), set that field
inside empty_workspace_at_path when creating the workspace, and update
run_netsuke_with_directory_flag to read world.workspace_path (with proper Option
handling and context message) and pass its to_string_lossy() value to
run_netsuke_and_store instead of world.temp_dir; keep world.temp_dir untouched
for execution working dir uses.
- Add explicit workspace_path field to TestWorld to track workspace directory for `-C` flag tests. - Implement `resolve_path_safe` to safely canonicalize workspace paths allowing non-existing targets. - Update `empty_workspace_at_path` step to use safe path resolving and store resolved path. - Modify `run_netsuke_with_directory_flag` to run netsuke using the stored workspace path. - Ensure workspace path restrictions confine directories under /tmp or system temp to prevent traversal/symlink attacks. - Clean up and create workspace dirs at resolved paths for reliable test isolation. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
1 participant
Summary
Changes
Runtime
CLI localization
Localization assets
CLI/runtime wiring
Tests
Docs and examples
Artifacts
Why
Testing Plan
How to Review
Related issue(s)
📎 Task: https://www.terragonlabs.com/task/bde9b9c5-0ead-42e3-b911-a7f6658a099b