Refine CLI output clarity with OrthoConfig localisation and tests#254
Refine CLI output clarity with OrthoConfig localisation and tests#254
Conversation
…larity Introduce a new ExecPlan in docs/execplans/cli-output-clarity.md that outlines the purpose, progress checklist, decision log, and concrete steps for enhancing the Netsuke CLI output. This includes integrating OrthoConfig for configuration layering, localizing CLI help and error messages, refining user-facing messages, adding tests, and updating documentation to improve usability and consistency across CLI commands. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
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. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntegrate Fluent-based CLI localisation, OrthoConfig-backed configuration discovery and layered merge, add English/Spanish locale resources, localiser builders and clap-localisation helpers, extend CLI parsing/merge flow, enable HostPattern (de)serialisation, update main entrypoint and tests, and add documentation and roadmap updates. Changes
Sequence Diagram(s)sequenceDiagram
%% Styling note: keep palette subtle for accessibility
participant OS as OS / Env
participant Main as main()
participant Localizer as build_localizer()
participant Parser as parse_with_localizer_from()
participant ConfigMerge as merge_with_config()
participant Runner as runner::run()
OS->>Main: argv (OsString[]) + env
Main->>Main: collect args as OsString
Main->>Main: extract locale hint from args
Main->>Main: consult NETSUKE_LOCALE env
Main->>Localizer: build_localizer(preferred_locale)
Localizer-->>Main: Box<dyn Localizer>
Main->>Parser: parse_with_localizer_from(args, localizer)
Parser->>Parser: clap parse using Localizer (help/errors)
Parser-->>Main: (Cli, ArgMatches)
Main->>ConfigMerge: merge_with_config(&cli, &matches)
ConfigMerge->>ConfigMerge: discover layers (defaults, file, env, CLI)
ConfigMerge->>ConfigMerge: apply precedence & merge lists
ConfigMerge-->>Main: merged Cli or error
alt merge succeeds
Main->>Main: init_tracing(merged_cli.verbose)
Main->>Runner: runner::run(&merged_cli)
Runner-->>Main: exit code
else merge fails
Main->>Main: init_tracing(ERROR)
Main->>OS: log merge failure, exit FAILURE
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (6)
🧰 Additional context used📓 Path-based instructions (2)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
Cargo.toml📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-12-29T19:00:20.890ZApplied to files:
🧬 Code graph analysis (4)tests/cli_tests/merge.rs (1)
build.rs (3)
src/cli_l10n.rs (1)
src/cli.rs (2)
⏰ 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)
🔇 Additional comments (22)
Comment |
Reviewer's GuideImplements OrthoConfig-backed, locale-aware CLI configuration and parsing, wires it through main and runner, adds Fluent-based English/Spanish localisation resources, and extends tests, docs, and build configuration to cover configuration precedence and localised CLI output/errors. Sequence diagram for OrthoConfig-backed, localised CLI parsing and executionsequenceDiagram
actor User
participant Binary as main_rs
participant CliMod as cli_rs
participant CliLoc as cli_localization_rs
participant Ortho as ortho_config
participant Runner as runner_mod
User->>Binary: invoke netsuke [ARGS]
Binary->>Binary: collect args_os()
Binary->>CliMod: locale_hint_from_args(args)
Binary->>Binary: read NETSUKE_LOCALE env
Binary->>CliLoc: build_localizer(locale)
CliLoc->>CliLoc: parse_locale / build_en_localizer
CliLoc-->>Binary: Localizer
Binary->>CliMod: parse_with_localizer_from(args, Localizer)
CliMod->>CliMod: localize_command(Cli::command(), Localizer)
CliMod->>CliMod: try_get_matches_from_mut()
alt clap parse error
CliMod->>Ortho: localize_clap_error_with_command(err)
Ortho-->>CliMod: localized clap::Error
CliMod-->>Binary: Err(clap::Error)
Binary->>Binary: is_display_request(err)
Binary-->>User: print localized help/error
else parse ok
CliMod->>CliMod: Cli::from_arg_matches_mut(&mut matches)
CliMod-->>Binary: (Cli, ArgMatches)
end
Binary->>CliMod: merge_with_config(&Cli, &ArgMatches)
CliMod->>CliMod: MergeComposer::with_capacity(4)
CliMod->>CliMod: sanitize_value(&Cli::default())
CliMod->>CliMod: config_discovery(cli.directory)
CliMod->>Ortho: ConfigDiscovery::compose_layers()
Ortho-->>CliMod: file_layers
CliMod->>Ortho: Env::prefixed("NETSUKE_")
CliMod->>Ortho: Figment::from(env_provider).extract()
CliMod->>CliMod: cli.extract_user_provided(matches)
CliMod->>Ortho: LayerComposition::into_merge_result(Cli::merge_from_layers)
Ortho-->>CliMod: merged Cli
alt merge error
CliMod-->>Binary: Err(OrthoError)
Binary->>Binary: init_tracing(Level::ERROR)
Binary-->>User: log configuration load failed
else merge ok
CliMod-->>Binary: Ok(merged Cli)
end
Binary->>Binary: merged_cli.with_default_command()
Binary->>Binary: init_tracing(max_level)
Binary->>Runner: run(&merged_cli)
Runner-->>Binary: Result<()> (build/clean/graph/manifest)
Binary-->>User: exit code + logs
Updated class diagram for CLI configuration, commands, and localisationclassDiagram
class Cli {
+PathBuf file
+Option_PathBuf directory
+Option_usize jobs
+bool verbose
+Option_String locale
+Vec_String fetch_allow_scheme
+Vec_HostPattern fetch_allow_host
+Vec_HostPattern fetch_block_host
+bool fetch_default_deny
+Option_Commands command
+default() Cli
}
class BuildArgs {
+Option_PathBuf emit
+Vec_String targets
}
class Commands {
<<enum>>
+Build(BuildArgs)
+Clean
+Graph
+Manifest
}
class HostPattern {
-String pattern
-bool wildcard
+parse(value: String) Result_HostPattern_HostPatternError
+matches(input: String) bool
+from_str(value: String) Result_HostPattern_HostPatternError
}
class LayeredLocalizer {
-Localizer primary
-Localizer fallback
+new(primary: Localizer, fallback: Localizer) LayeredLocalizer
+lookup(id: String, args: LocalizationArgs) Option_String
}
class Localizer {
<<trait>>
+lookup(id: String, args: LocalizationArgs) Option_String
}
class FluentLocalizerBuilder {
+with_consumer_resources(resources: StringList) FluentLocalizerBuilder
+disable_defaults() FluentLocalizerBuilder
+try_build() Result_FluentLocalizer_Error
}
class FluentLocalizer {
+with_en_us_defaults(resources: StringList) Result_FluentLocalizer_Error
+builder(locale: LanguageIdentifier) FluentLocalizerBuilder
}
class NoOpLocalizer {
+new() NoOpLocalizer
+lookup(id: String, args: LocalizationArgs) Option_String
}
class MergeComposer {
+with_capacity(size: usize) MergeComposer
+push_defaults(value: JsonValue)
+push_layer(value: JsonValue)
+push_environment(value: JsonValue)
+push_cli(value: JsonValue)
+layers() Vec_JsonValue
}
class ConfigDiscovery {
+builder(app: String) ConfigDiscoveryBuilder
+compose_layers() FileLayers
}
class ConfigDiscoveryBuilder {
+env_var(name: String) ConfigDiscoveryBuilder
+clear_project_roots() ConfigDiscoveryBuilder
+add_project_root(dir: PathBuf) ConfigDiscoveryBuilder
+build() ConfigDiscovery
}
class LayerComposition {
+new(layers: Vec_JsonValue, errors: Vec_OrthoError) LayerComposition
+into_merge_result(merge_fn: fn) OrthoResult_Cli
}
class ArgMatches
Cli --> Commands : has
Commands --> BuildArgs : uses
Cli --> HostPattern : uses
LayeredLocalizer ..|> Localizer : implements
FluentLocalizer ..> Localizer : returns
NoOpLocalizer ..|> Localizer : implements
ConfigDiscoveryBuilder --> ConfigDiscovery : builds
ConfigDiscovery --> LayerComposition : produces
MergeComposer --> LayerComposition : layers
class CliModuleHelpers {
+default_manifest_path() PathBuf
+usage_body(usage: String) String
+localize_command(command: Command, localizer: Localizer) Command
+localize_subcommands(command: Command, localizer: Localizer)
+locale_hint_from_args(args: OsStringList) Option_String
+parse_with_localizer_from(iter: OsStringList, localizer: Localizer) Result_Cli_ArgMatches
+merge_with_config(cli: Cli, matches: ArgMatches) OrthoResult_Cli
}
CliModuleHelpers --> Cli : constructs
CliModuleHelpers --> LayeredLocalizer : uses via Localizer
CliModuleHelpers --> MergeComposer : uses
CliModuleHelpers --> ConfigDiscovery : uses
CliModuleHelpers --> ArgMatches : uses
Flow diagram for configuration precedence and merging in merge_with_configflowchart TD
A[Start merge_with_config] --> B[Copy cli.command for later]
B --> C[Create MergeComposer with_capacity_4]
C --> D[Sanitize Cli::default and push_defaults]
D --> E[Build ConfigDiscovery based on cli.directory]
E --> F[Compose file_layers from ConfigDiscovery]
F --> G{file_layers.value is empty?}
G -- Yes --> H[Record required_errors only]
G -- No --> I[Record all errors; push each file layer]
H --> J
I --> J[Prepare Env provider with NETSUKE_ prefix]
J --> K[Use Figment to extract environment JsonValue]
K --> L[Push environment layer or record error]
L --> M[Extract user_provided CLI values from matches]
M --> N{CLI values empty?}
N -- No --> O[Push CLI layer into composer]
N -- Yes --> P[Skip CLI layer]
O --> Q
P --> Q[Build LayerComposition from layers and errors]
Q --> R[Call into_merge_result with Cli::merge_from_layers]
R --> S{Merge succeeded?}
S -- No --> T[Return OrthoError from OrthoConfig]
S -- Yes --> U[Restore original command field on merged Cli]
U --> V[Return merged Cli]
T --> W[End]
V --> W[End]
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}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the ExecPlan section that specifies adding
ortho_config = "0.7.0", consider referencing the existing dependency management conventions (e.g., workspace-wide versions or a tooling file) instead of a hardcoded example version so the document doesn’t drift from the actual Cargo.toml state. - Where you describe new Fluent resources (e.g.,
locales/en-US/messages.ftl), it would help to explicitly tie this to any existing i18n/locale directory layout in the repo or to state that this is a new convention, so future implementers don’t accidentally introduce a parallel structure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the ExecPlan section that specifies adding `ortho_config = "0.7.0"`, consider referencing the existing dependency management conventions (e.g., workspace-wide versions or a tooling file) instead of a hardcoded example version so the document doesn’t drift from the actual Cargo.toml state.
- Where you describe new Fluent resources (e.g., `locales/en-US/messages.ftl`), it would help to explicitly tie this to any existing i18n/locale directory layout in the repo or to state that this is a new convention, so future implementers don’t accidentally introduce a parallel structure.
## Individual Comments
### Comment 1
<location> `docs/execplans/cli-output-clarity.md:69` </location>
<code_context>
+for CLI behaviour are in `docs/netsuke-design.md` and the roadmap entry in
+`docs/roadmap.md` (Phase 3 → “CLI and Feature Completeness”).
+
+Testing guidance for fixtures, DI, and BDD lives in:
+
+- `docs/rust-testing-with-rstest-fixtures.md`
</code_context>
<issue_to_address>
**suggestion (review_instructions):** DI and BDD are acronyms that are not expanded when first mentioned.
Consider expanding these acronyms on first use, for example: "dependency injection (DI)" and "behaviour-driven development (BDD)", so readers are not expected to infer their meaning from context.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docs/execplans/cli-output-clarity.md
🧰 Additional context used
📓 Path-based instructions (4)
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 (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)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 use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/execplans/cli-output-clarity.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/cli-output-clarity.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/cli-output-clarity.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/execplans/cli-output-clarity.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/execplans/cli-output-clarity.md
🪛 LanguageTool
docs/execplans/cli-output-clarity.md
[style] ~1-~1: Would you like to use the Oxford spelling “Localized”? The spelling ‘Localised’ is also correct.
Context: # Refine CLI Output With OrthoConfig Localised Help This ExecPlan is a living documen...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~14-~14: Would you like to use the Oxford spelling “Localized”? The spelling ‘Localised’ is also correct.
Context: ...les, and CLI flags behave consistently. Localised help strings should be used for CLI usa...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~26-~26: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...rthoConfig-backed CLI configuration and localised help plumbing. - [ ] Refine user-faci...
(OXFORD_SPELLING_Z_NOT_S)
[grammar] ~30-~30: The verb after “to” should be in the base form as part of the to-infinitive. A verb can take many forms, but the base form is always used in the to-infinitive.
Context: ...and test gates; update roadmap entry to done. ## Surprises & Discoveries - Observa...
(TO_NON_BASE)
[style] ~64-~64: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ... which explains configuration layering, localised help via Fluent, and error localisation...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~65-~65: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...g, localised help via Fluent, and error localisation helpers. Design expectations for CLI be...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~94-~94: Possible missing comma found.
Context: ... explicit handling for missing required values and make sure clap defaults do not i...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~98-~98: Would you like to use the Oxford spelling “Localize”? The spelling ‘Localise’ is also correct.
Context: ...i_default_as_absent` where needed). 3. Localise CLI help and clap errors. Create Fluent...
(OXFORD_SPELLING_Z_NOT_S)
[grammar] ~98-~98: The word ‘help’ is a conjunction. In formal texts, use “to”.
Context: ...t` where needed). 3. Localise CLI help and clap errors. Create Fluent resources (f...
(HELP_AND_VB)
[uncategorized] ~100-~100: Do not mix variants of the same word (‘localize’ and ‘localise’) within a single text.
Context: ...FluentLocalizer into CLI parsing. Use command().localize(&localizer) before parsing and `loc...
(EN_WORD_COHERENCY)
[uncategorized] ~101-~101: Do not mix variants of the same word (‘localize’ and ‘localise’) within a single text.
Context: ...lize(&localizer) before parsing andlocalize_clap_error_with_command` on errors. Ens...
(EN_WORD_COHERENCY)
[style] ~102-~102: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...k path preserves stock clap output when localisation fails. 4. Refine CLI output messages. ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~109-~109: Would you like to use the Oxford spelling “centralize”? The spelling ‘centralise’ is also correct.
Context: ...troduce a small output helper module to centralise user message formatting. 5. Add tes...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~117-~117: Would you like to use the Oxford spelling “Localized”? The spelling ‘Localised’ is also correct.
Context: ...` to avoid touching process state. - Localised help contains expected copy, and clap e...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~117-~117: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...ains expected copy, and clap errors are localised when possible. - Unhappy paths ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~129-~129: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...lain the new configuration layering, localised help, and any changes to CLI output. ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~147-~147: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: .... Implement OrthoConfig integration and localisation. Update or add the relevant files an...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~168-~168: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...elp(viacargo run -- --help`) prints localised, plain-language descriptions for ever...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~171-~171: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ... Error output for invalid CLI inputs is localised when a translation exists. - Configur...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~177-~177: Loose punctuation mark.
Context: ... and config layering. - make check-fmt, make lint, and make test pass. - `d...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~183-~183: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...potence and Recovery - OrthoConfig and localisation changes are safe to re-run; regenerate ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~185-~185: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...nd help output as often as needed. - If localisation setup fails, fall back to default clap ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~187-~187: Consider using a different verb for a more formal wording.
Context: ... error output. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[style] ~194-~194: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...idence: - netsuke --help output with localised strings (from /tmp/netsuke-help.log...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~196-~196: Possible missing comma found.
Context: ...tmp/netsuke-help.log`). - A failing CLI invocation showing localised error output (record ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~196-~196: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...g`). - A failing CLI invocation showing localised error output (record the command and ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~214-~214: Would you like to use the Oxford spelling “localizer”? The spelling ‘localiser’ is also correct.
Context: ...e CLI parsing in src/main.rs uses the localiser and merges config layers before calli...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~222-~222: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...ted to cover OrthoConfig integration, localised help, CLI output clarity, and test/docu...
(OXFORD_SPELLING_Z_NOT_S)
🔍 Remote MCP
Summary of Findings
Based on comprehensive exploration of the Netsuke repository through its documentation and codebase, here are the key findings relevant for reviewing PR #254:
What This PR Adds
The PR introduces a new ExecPlan document at docs/execplans/cli-output-clarity.md - a "living blueprint" for refining CLI output with OrthoConfig-backed localization. This is a documentation-only PR with no functional code changes.
Important Context About the Repository
-
Project Structure: Netsuke is a build system compiler (not executor) that transforms YAML manifests with Jinja2 templating into Ninja build files. It implements a six-stage processing pipeline from manifest loading through Ninja code generation.
-
ExecPlan Concept: While ExecPlans aren't currently documented in the codebase, they appear to be planning documents for major initiatives. The new
cli-output-clarity.mdfile follows the pattern of outlining purpose, progress, decisions, validation criteria, and dependencies. -
CLI Architecture (source: deepwiki):
- Built with
clapderive API - Currently supports three commands:
build(default),clean, andgraph - Provides options for
--file,--directory(-C),--jobs(-j), and--verbose(-v) - CLI is defined in
src/cli.rsand entry point insrc/main.rs
- Built with
-
OrthoConfig and Localization: While the PR mentions "OrthoConfig-backed configuration" and localization, these components don't exist yet in the codebase. This confirms the ExecPlan is a forward-looking design document for future implementation.
-
Repository Quality Standards (source: deepwiki):
- Strict code quality through clippy with denial of unsafe patterns
- Comprehensive multi-layer testing (unit, BDD, snapshot, integration)
- Strong emphasis on security (shell escaping via shell-quote, capability-based I/O via cap-std)
- Clear error handling strategy using thiserror/anyhow
- File system operations use UTF-8 paths (camino) and sandboxed access (cap-std)
Verification Steps for Review
As mentioned in the PR, reviewers should:
- ✅ Verify the ExecPlan aligns with repository goals and design patterns (evidenced by the comprehensive design documentation available)
- ✅ Ensure the CLI output clarity initiative fits with current architecture (CLI is a clear, bounded subsystem in src/cli.rs)
- ✅ Validate that proposed scope doesn't conflict with existing functionality (clean/graph commands are currently stubs awaiting implementation)
⏰ 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). (5)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux artefacts
- 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-windows (x86_64-pc-windows-msvc, amd64, x64, windows-x86_64) / Build windows artefacts
- GitHub Check: Sourcery review
🔇 Additional comments (1)
docs/execplans/cli-output-clarity.md (1)
1-222: Document structure, content, and alignment with ExecPlan purpose are sound.The ExecPlan file provides clear purpose, context, concrete steps, validation criteria, and dependencies. Heading hierarchy, code formatting, checklist syntax, and reference documentation links are all correctly structured. Once the three preceding issues (line wrapping, grammar, spelling) are resolved, this document is ready for integration.
- Introduce OrthoConfig-backed layering for CLI configuration with priorities: defaults, config files, env vars, CLI flags - Make CLI defaults absent so they do not mask config file or environment values - Support configuration discovery honoring NETSUKE_CONFIG_PATH and standard OS config locations - Add Fluent-based localisation of CLI help and error messages with English and Spanish support - Parse and apply --locale and NETSUKE_LOCALE environment variable for localisation - Enhance CLI parsing to produce localized error messages - Update CLI documentation and user guide with localisation and config layering details - Add unit tests and behavioural tests for configuration merging and localisation - Modify main executable to perform config merging and setup localized CLI parsing This improves user experience by supporting layered, overrideable configurations and localized CLI output, improving clarity and internationalisation. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
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: Comment on file //! The module centralises normalisation and matching logic so CLI parsing and
//! runtime policy evaluation agree on allowable host syntax.
use serde::{Deserialize, Serialize};❌ New issue: String Heavy Function Arguments |
|
@sourcery-ai review |
This comment was marked as resolved.
This comment was marked as resolved.
Changed test assertion to use `merged.file.as_path() == Path::new("Configfile")` instead of `PathBuf::from("Configfile")` for more idiomatic path comparison in tests.
Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Hey - I've found 9 issues, and left some high level feedback:
- In
main.rs, theis_display_request(&err)branch currently callserr.exit()in both cases, making theifcondition redundant; consider either simplifying to a singleerr.exit()or using the predicate to differentiate behavior (e.g., exit code or logging) for display vs error requests. - There are two
parse_localehelpers (cli::parse_localeandcli_localization::parse_locale) with overlapping concerns but different return types; consider consolidating or renaming them to make the separation of responsibilities clearer and avoid future confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `main.rs`, the `is_display_request(&err)` branch currently calls `err.exit()` in both cases, making the `if` condition redundant; consider either simplifying to a single `err.exit()` or using the predicate to differentiate behavior (e.g., exit code or logging) for display vs error requests.
- There are two `parse_locale` helpers (`cli::parse_locale` and `cli_localization::parse_locale`) with overlapping concerns but different return types; consider consolidating or renaming them to make the separation of responsibilities clearer and avoid future confusion.
## Individual Comments
### Comment 1
<location> `src/main.rs:20-22` </location>
<code_context>
+ let locale = locale_hint.as_deref().or(env_locale.as_deref());
+ let localizer = cli_localization::build_localizer(locale);
+
+ let (parsed_cli, matches) = match cli::parse_with_localizer_from(args, localizer.as_ref()) {
+ Ok(parsed) => parsed,
+ Err(err) => {
+ if is_display_request(&err) {
+ err.exit();
</code_context>
<issue_to_address>
**issue:** The `is_display_request` branch is redundant because `err.exit()` is unconditionally called.
Because `err.exit()` terminates the process, the second call after the `if is_display_request(&err)` block is unreachable. Consider removing the `if is_display_request` branch (keeping a single `err.exit()`) or changing it to perform some different behavior (e.g., a different exit code) if that was intended.
</issue_to_address>
### Comment 2
<location> `src/cli.rs:253-262` </location>
<code_context>
+fn localize_subcommands(command: &mut clap::Command, localizer: &dyn Localizer) {
</code_context>
<issue_to_address>
**suggestion (performance):** Subcommands are cloned unnecessarily when applying localized `about`/`long_about` text.
Inside `localize_subcommands`, you can drop the `subcommand.clone()` and reassignment. Since Clap’s `about`/`long_about` return `&mut Command`, call them directly on `subcommand` to avoid extra allocations and simplify the code.
</issue_to_address>
### Comment 3
<location> `src/cli.rs:339-348` </location>
<code_context>
+pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult<Cli> {
</code_context>
<issue_to_address>
**question:** Preserving `cli.command` after merging layers prevents configuration from influencing the default subcommand.
Currently `merge_with_config` clones `cli.command` at the start and restores it after merging, so any `command` value from config is discarded. If config should be able to set a default subcommand (overridden by explicit CLI args), this logic prevents it. If instead config-driven subcommands are disallowed, consider excluding `command` from the merge entirely (e.g., `#[ortho_config(skip)]`) so that intent is enforced and self-documenting.
</issue_to_address>
### Comment 4
<location> `Cargo.toml:18-20` </location>
<code_context>
[features]
-default = []
+default = ["serde_json"]
legacy-digests = ["sha1", "md5"]
+serde_json = ["ortho_config/serde_json"]
[dependencies]
</code_context>
<issue_to_address>
**question (bug_risk):** Using `serde_json` in the public API without gating the code may break `default-features = false` consumers.
Because `serde_json` is now a default feature but `serde_json::Value` is used directly in `cli::merge_with_config` without `cfg` gating, `netsuke` still requires `serde_json` even when built with `default-features = false`. To keep `serde_json` truly optional, gate the relevant code with `#[cfg(feature = "serde_json")]` (and provide an alternative or stub when disabled), or drop the separate feature if it isn’t meant to be optional.
</issue_to_address>
### Comment 5
<location> `tests/cli_tests.rs:315-284` </location>
<code_context>
+fn cli_merge_layers_respects_precedence_and_appends_lists() -> Result<()> {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test that exercises `merge_with_config` end-to-end instead of only `merge_from_layers`
This test covers precedence and list semantics at the `Cli::merge_from_layers` level, but `merge_with_config` also wires in `ConfigDiscovery`, env vars, and `extract_user_provided` (including the `is_empty_value` short-circuit). Please add an integration-style unit test that exercises `merge_with_config` end-to-end (config discovery from a temp dir, `NETSUKE_` env vars, CLI args via `ArgMatches`) and asserts the final `Cli` reflects the correct precedence and ignores empty CLI overrides.
Suggested implementation:
```rust
#[rstest]
fn cli_merge_with_config_respects_precedence_and_ignores_empty_cli_values() -> Result<()> {
use std::fs;
use std::path::PathBuf;
// Arrange: create a temporary config root with a config file
let tmp_dir = tempfile::tempdir().context("failed to create temp dir for config discovery")?;
let config_root: PathBuf = tmp_dir.path().to_path_buf();
// Config file sets a few values that will be overridden by env and CLI
// - jobs: 2 (to be overridden)
// - fetch_allow_scheme: ["https"] (to be appended to / overridden)
// - some other option to ensure config is picked up at all
let config_toml = r#"
jobs = 2
fetch_allow_scheme = ["https"]
"#;
fs::write(config_root.join("netsuke.toml"), config_toml)
.context("failed to write netsuke.toml")?;
// Arrange: set NETSUKE_ env vars that override config
// - NETSUKE_JOBS should win over config value, unless overridden by non-empty CLI
// - NETSUKE_FETCH_ALLOW_SCHEME should be merged/overridden according to semantics
std::env::set_var("NETSUKE_JOBS", "4");
std::env::set_var("NETSUKE_FETCH_ALLOW_SCHEME", "http");
// Arrange: build CLI ArgMatches simulating real invocation
// We pass:
// - --jobs with an *empty* string, which should be treated as "no user override"
// and therefore ignored by `extract_user_provided` / `is_empty_value`.
// - --fetch-allow-scheme multiple times to exercise list semantics (append/override).
//
// The expectations are:
// - `jobs` should *not* be overridden by the empty CLI value, so it should resolve
// to the env var value (4), not the config value (2) and not default.
// - `fetch_allow_scheme` should reflect the precedence rules including
// merging/appending from config/env/CLI (final assertion below tailored to actual semantics).
//
// NOTE: if `Cli::command()` is not available, adjust this to however ArgMatches
// are constructed in the main binary.
let matches = Cli::command().get_matches_from([
"netsuke",
"--jobs",
"", // empty value: should be ignored as a user override
"--fetch-allow-scheme",
"ssh",
"--fetch-allow-scheme",
"git",
]);
// Arrange: wire ConfigDiscovery using the temp config root
// Adjust constructor if your real type differs.
let discovery = ConfigDiscovery::from_root(config_root);
// Act: perform full merge, including:
// - config discovery (using ConfigDiscovery)
// - env vars (NETSUKE_)
// - CLI args (ArgMatches)
// - extraction of user-provided values and empty-value short-circuit
let mut cli = Cli::default();
cli.merge_with_config(&matches, &discovery)
.context("merge_with_config failed")?;
// Assert: precedence and empty-override semantics
// `jobs`:
// - default: Cli::default().jobs
// - config: 2
// - env: NETSUKE_JOBS=4
// - CLI: "--jobs ''" (empty, should be ignored)
//
// So the final value should come from env, not config and not be overridden by
// the empty CLI value.
assert_eq!(
cli.jobs, 4,
"empty CLI override for jobs should be ignored; env var should win over config"
);
// `fetch_allow_scheme`:
// Base config: ["https"] from netsuke.toml
// Env: "http" from NETSUKE_FETCH_ALLOW_SCHEME
// CLI: ["ssh", "git"] via multiple --fetch-allow-scheme flags
//
// The exact expected list depends on semantics of `merge_from_layers` for lists:
// - If it *appends* layers, then lower-precedence values appear first.
// - If it *overrides* with the highest precedence, only the CLI list appears.
//
// Here we assert the "append with increasing precedence" behavior:
// config ("https") + env ("http") + CLI ("ssh", "git")
// Adjust this expectation if your merging semantics differ.
assert_eq!(
cli.fetch_allow_scheme,
vec![
"https".to_string(),
"http".to_string(),
"ssh".to_string(),
"git".to_string()
],
"merge_with_config should merge list fields respecting precedence across \
config file, NETSUKE_ env vars, and CLI args"
);
Ok(())
}
#[rstest]
```
This new test assumes some APIs and types that may need to be adjusted to match your actual code:
1. **ConfigDiscovery construction**
- The test uses `ConfigDiscovery::from_root(config_root)`. If the real API is different (e.g. `ConfigDiscovery::new(Some(config_root))` or a builder pattern), update the construction accordingly, while still pointing it at `config_root` so that `netsuke.toml` is discovered.
2. **CLI ArgMatches construction**
- The test uses `Cli::command().get_matches_from(...)`. If your `Cli` type uses a different method (e.g. `Cli::command_from_clap()` or a global `command()` function), replace this line with however you normally obtain `ArgMatches` for `Cli`.
3. **Field names and types**
- The assertions reference `cli.jobs` (integer) and `cli.fetch_allow_scheme` (`Vec<String>`). If your `Cli` struct uses different field names or types, update the assertions and the TOML/env/CLI inputs to match.
- Ensure that `Cli::default()` and `Cli::merge_with_config(&matches, &discovery)` exist and are in scope in `tests/cli_tests.rs`.
4. **Crate imports**
- The test uses:
- `tempfile::tempdir`
- `anyhow::Context` (via `.context(...)`)
- `serde_json::json` / the `Result` alias already used elsewhere in the file.
- If these imports are not yet present at the top of `tests/cli_tests.rs`, add:
- `use tempfile;`
- `use anyhow::Context;`
- Any others needed to compile.
5. **List merge semantics**
- The expected `fetch_allow_scheme` value assumes the lists from config, env, and CLI are **appended in precedence order**: config < env < CLI.
- If your actual `merge_from_layers` implementation uses a different strategy (e.g. "highest precedence overwrites entire list"), adjust the expected vector in the assertion to match those semantics while still verifying that `merge_with_config` correctly wires config discovery, env vars, CLI args, and empty CLI values.
</issue_to_address>
### Comment 6
<location> `tests/cli_tests.rs:361-284` </location>
<code_context>
+}
+
+#[rstest]
+fn cli_localises_invalid_subcommand_in_spanish() -> Result<()> {
+ let localizer = cli_localization::build_localizer(Some("es-ES"));
+ let err = netsuke::cli::parse_with_localizer_from(
+ ["netsuke", "--locale", "es-ES", "unknown"],
+ localizer.as_ref(),
+ )
+ .err()
+ .context("parser should reject invalid subcommand")?;
+ ensure!(
+ err.to_string().contains("Subcomando desconocido"),
+ "expected Spanish localisation, got: {err}",
+ );
+ Ok(())
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for `locale_hint_from_args` to cover argument parsing edge cases
`locale_hint_from_args` is only covered indirectly via `main`/BDD, so its behavior isn’t locked down. Please add focused unit tests, e.g.:
- `--locale es-ES` and `--locale=en-US` forms
- `--locale` as the last arg (no value) returning `None` without panicking
- Arguments containing `--` so locale parsing ignores positional args after end-of-options (if required)
- Multiple `--locale` flags to document which one takes precedence
This will clarify and stabilize the locale selection contract for future refactors.
Suggested implementation:
```rust
#[rstest]
fn locale_hint_from_args_accepts_space_form() -> Result<()> {
let args = ["netsuke", "--locale", "es-ES"];
let hint = netsuke::cli::locale_hint_from_args(&args);
ensure!(
hint.as_deref() == Some("es-ES"),
"expected Some(\"es-ES\"), got: {hint:?}"
);
Ok(())
}
#[rstest]
fn locale_hint_from_args_accepts_equals_form() -> Result<()> {
let args = ["netsuke", "--locale=es-ES"];
let hint = netsuke::cli::locale_hint_from_args(&args);
ensure!(
hint.as_deref() == Some("es-ES"),
"expected Some(\"es-ES\"), got: {hint:?}"
);
Ok(())
}
#[rstest]
fn locale_hint_from_args_trailing_locale_flag_yields_none() -> Result<()> {
let args = ["netsuke", "--locale"];
let hint = netsuke::cli::locale_hint_from_args(&args);
ensure!(
hint.is_none(),
"expected None for trailing --locale without value, got: {hint:?}"
);
Ok(())
}
#[rstest]
fn locale_hint_from_args_ignores_args_after_double_dash() -> Result<()> {
let args = ["netsuke", "--verbose", "--", "--locale", "es-ES"];
let hint = netsuke::cli::locale_hint_from_args(&args);
ensure!(
hint.is_none(),
"expected None when --locale appears after end-of-options \"--\", got: {hint:?}"
);
Ok(())
}
#[rstest]
fn locale_hint_from_args_uses_last_locale_flag() -> Result<()> {
let args = ["netsuke", "--locale", "es-ES", "--locale", "en-US"];
let hint = netsuke::cli::locale_hint_from_args(&args);
ensure!(
hint.as_deref() == Some("en-US"),
"expected last --locale to win (\"en-US\"), got: {hint:?}"
);
Ok(())
}
#[rstest]
fn cli_extract_user_provided_omits_defaults() -> Result<()> {
```
- If `locale_hint_from_args` is not exposed as `netsuke::cli::locale_hint_from_args`, adjust the path or add a `use` at the top of `tests/cli_tests.rs`, e.g.:
`use crate::cli::locale_hint_from_args;` and call it as `locale_hint_from_args(&args)`.
- If the current implementation prefers the *first* `--locale` instead of the last, update the expectation in `locale_hint_from_args_uses_last_locale_flag` accordingly (and rename the test to reflect the actual contract).
- If your implementation currently *does* consider `--locale` after `--`, either change the implementation to match the test (ignoring post-`--` options) or update the test to assert the current behaviour you intend to keep.
</issue_to_address>
### Comment 7
<location> `docs/users-guide.md:543` </location>
<code_context>
+`$HOME/.netsuke.toml`, and finally the project root.
+
+Environment variables use the `NETSUKE_` prefix (for example,
+`NETSUKE_JOBS=8`). Use `__` to separate nested keys if you need to match
+structured configuration.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The sentence "if you need to match" uses the 2nd person pronoun "you", which the style guide forbids.
Consider rephrasing to avoid the explicit 2nd person, for example: "Use `__` to separate nested keys when matching structured configuration."
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>
### Comment 8
<location> `docs/execplans/cli-output-clarity.md:78` </location>
<code_context>
+for CLI behaviour are in `docs/netsuke-design.md` and the roadmap entry in
+`docs/roadmap.md` (Phase 3 → “CLI and Feature Completeness”).
+
+Testing guidance for fixtures, DI, and BDD lives in:
+
+- `docs/rust-testing-with-rstest-fixtures.md`
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronyms "DI" and "BDD" are introduced without expansion, which conflicts with the requirement to define uncommon acronyms on first use.
Consider expanding both acronyms on first mention, for example: "fixtures, dependency injection (DI), and behaviour-driven development (BDD)".
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 9
<location> `docs/execplans/cli-output-clarity.md:82` </location>
<code_context>
+
+- `docs/rust-testing-with-rstest-fixtures.md`
+- `docs/reliable-testing-in-rust-via-dependency-injection.md`
+- `docs/behavioural-testing-in-rust-with-cucumber.md` (applies to Gherkin
+ structure, even though we use `rstest-bdd`)
+- `docs/rust-doctest-dry-guide.md` (for any new public API docs)
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The parenthetical "even though we use `rstest-bdd`" uses the 1st person plural pronoun "we", which is disallowed by the style guide.
Rephrase the parenthetical to avoid 1st person, for example: "(applies to Gherkin structure, even though `rstest-bdd` is used instead)".
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Added tests for locale_hint_from_args covering various argument forms and conditions. - Added tests for cli_merge_with_config to verify precedence and skipping empty CLI layers. - Introduced EnvGuard for managing environment variables in tests safely. - Improved testing coverage and robustness for CLI parsing and merging behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cli_tests.rs (1)
1-502: File exceeds 400-line limit.At 502 lines, this file exceeds the 400-line maximum specified in the coding guidelines. Split locale-related tests, merge-related tests, and network policy tests into separate test modules.
♻️ Duplicate comments (1)
Cargo.toml (1)
17-20: The cfg-gating concern was raised previously.The past review flagged that
serde_json::Valueusage incli::merge_with_configlacks#[cfg(feature = "serde_json")]guards, which breaksdefault-features = falseconsumers.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlbuild.rsdocs/execplans/cli-output-clarity.mddocs/netsuke-design.mddocs/roadmap.mddocs/users-guide.mdlocales/en-US/messages.ftllocales/es-ES/messages.ftlsrc/cli.rssrc/cli_localization.rssrc/cli_policy.rssrc/host_pattern.rssrc/lib.rssrc/main.rssrc/runner/mod.rssrc/runner/process/file_io.rssrc/runner/process/mod.rstests/bdd/steps/cli.rstests/cli_tests.rstests/features/cli.feature
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/cli_policy.rssrc/cli_localization.rssrc/runner/mod.rsbuild.rssrc/main.rstests/bdd/steps/cli.rssrc/lib.rstests/cli_tests.rssrc/runner/process/mod.rssrc/runner/process/file_io.rssrc/cli.rssrc/host_pattern.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/cli_policy.rssrc/cli_localization.rssrc/runner/mod.rsbuild.rssrc/main.rstests/bdd/steps/cli.rssrc/lib.rstests/cli_tests.rssrc/runner/process/mod.rssrc/runner/process/file_io.rssrc/cli.rssrc/host_pattern.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 (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)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 use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/users-guide.mddocs/roadmap.mddocs/netsuke-design.mddocs/execplans/cli-output-clarity.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/users-guide.mddocs/roadmap.mddocs/netsuke-design.mddocs/execplans/cli-output-clarity.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/users-guide.mddocs/roadmap.mddocs/netsuke-design.mddocs/execplans/cli-output-clarity.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/users-guide.mddocs/roadmap.mddocs/netsuke-design.mddocs/execplans/cli-output-clarity.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/users-guide.mddocs/roadmap.mddocs/netsuke-design.mddocs/execplans/cli-output-clarity.md
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: In RustCargo.toml, use explicit version ranges and keep dependencies up-to-date.
All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3") to allow safe non-breaking updates.
The use of wildcard (*) or open-ended inequality (>=) version requirements inCargo.tomlis strictly forbidden.
Tilde requirements (~) inCargo.tomlshould only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🧠 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/cli_policy.rssrc/cli_localization.rssrc/runner/mod.rsbuild.rssrc/main.rstests/bdd/steps/cli.rssrc/lib.rstests/cli_tests.rssrc/runner/process/mod.rssrc/runner/process/file_io.rssrc/cli.rssrc/host_pattern.rs
🧬 Code graph analysis (4)
build.rs (1)
src/cli.rs (3)
locale_hint_from_args(280-305)merge_with_config(356-397)parse_with_localizer_from(315-332)
src/main.rs (2)
src/cli.rs (3)
locale_hint_from_args(280-305)parse_with_localizer_from(315-332)merge_with_config(356-397)src/cli_localization.rs (1)
build_localizer(65-82)
tests/bdd/steps/cli.rs (2)
src/cli.rs (2)
locale_hint_from_args(280-305)parse_with_localizer_from(315-332)src/cli_localization.rs (1)
build_localizer(65-82)
src/cli.rs (1)
src/host_pattern.rs (1)
matches(165-177)
🪛 LanguageTool
docs/users-guide.md
[uncategorized] ~488-~488: Loose punctuation mark.
Context: ...e logging output. - --locale <LOCALE>: Localise CLI help and error messages (f...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~488-~488: Would you like to use the Oxford spelling “Localize”? The spelling ‘Localise’ is also correct.
Context: ...logging output. - --locale <LOCALE>: Localise CLI help and error messages (for exampl...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~496-~496: Loose punctuation mark.
Context: ...ions: - --fetch-allow-scheme <SCHEME>: Allow additional URL schemes beyond the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~499-~499: Loose punctuation mark.
Context: ...defaults. - --fetch-allow-host <HOST>: Allow the provided hostnames when defau...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~502-~502: Loose punctuation mark.
Context: ...pported). - --fetch-block-host <HOST>: Always block the provided hostnames (wi...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~505-~505: Loose punctuation mark.
Context: ...e allowlisted. - --fetch-default-deny: Deny all hosts by default and only perm...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~530-~530: Would you like to use the Oxford spelling “Localization”? The spelling ‘Localisation’ is also correct.
Context: ...s like --html. ### Configuration and Localisation Netsuke layers configuration in this o...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~546-~546: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...orNETSUKE_LOCALE to select localised CLI copy and error messages. Spanish (...
(OXFORD_SPELLING_Z_NOT_S)
docs/execplans/cli-output-clarity.md
[style] ~1-~1: Would you like to use the Oxford spelling “Localized”? The spelling ‘Localised’ is also correct.
Context: # Refine CLI Output With OrthoConfig Localised Help This ExecPlan is a living documen...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~14-~14: Would you like to use the Oxford spelling “Localized”? The spelling ‘Localised’ is also correct.
Context: ...les, and CLI flags behave consistently. Localised help strings should be used for CLI usa...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~26-~26: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...rthoConfig-backed CLI configuration and localised help plumbing. - [x] Refine user-faci...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~45-~45: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...dex) - Decision: Provide Fluent-backed localisation with English defaults and a Spanish c...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~47-~47: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...nsumer resource. Rationale: Validates localised help/error support while keeping fallba...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~53-~53: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...es: OrthoConfig configuration layering, localisation resources (including Spanish), update...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~73-~73: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ... which explains configuration layering, localised help via Fluent, and error localisation...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~74-~74: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...g, localised help via Fluent, and error localisation helpers. Design expectations for CLI be...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~105-~105: Possible missing comma found.
Context: ... explicit handling for missing required values and make sure clap defaults do not i...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~109-~109: Would you like to use the Oxford spelling “Localize”? The spelling ‘Localise’ is also correct.
Context: ...efault_as_absent` where needed). 3. Localise CLI help and clap errors. Create Fluent...
(OXFORD_SPELLING_Z_NOT_S)
[grammar] ~109-~109: The word ‘help’ is a conjunction. In formal texts, use “to”.
Context: ...where needed). 3. Localise CLI help and clap errors. Create Fluent resources (f...
(HELP_AND_VB)
[uncategorized] ~111-~111: Do not mix variants of the same word (‘localize’ and ‘localise’) within a single text.
Context: ...FluentLocalizer into CLI parsing. Use command().localize(&localizer) before parsing and `loc...
(EN_WORD_COHERENCY)
[uncategorized] ~112-~112: Do not mix variants of the same word (‘localize’ and ‘localise’) within a single text.
Context: ...lize(&localizer) before parsing andlocalize_clap_error_with_command` on errors. Ens...
(EN_WORD_COHERENCY)
[style] ~113-~113: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...k path preserves stock clap output when localisation fails. 4. Refine CLI output messages. ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~128-~128: Would you like to use the Oxford spelling “Localized”? The spelling ‘Localised’ is also correct.
Context: ...` to avoid touching process state. - Localised help contains expected copy, and clap e...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~128-~128: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...ains expected copy, and clap errors are localised when possible. - Unhappy paths ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~140-~140: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...lain the new configuration layering, localised help, and any changes to CLI output. ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~158-~158: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: .... Implement OrthoConfig integration and localisation. Update or add the relevant files an...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~179-~179: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...elp(viacargo run -- --help`) prints localised, plain-language descriptions for ever...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~182-~182: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ... Error output for invalid CLI inputs is localised when a translation exists. - Configur...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~188-~188: Loose punctuation mark.
Context: ... and config layering. - make check-fmt, make lint, and make test pass. - `d...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~194-~194: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...potence and Recovery - OrthoConfig and localisation changes are safe to re-run; regenerate ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~196-~196: Would you like to use the Oxford spelling “localization”? The spelling ‘localisation’ is also correct.
Context: ...nd help output as often as needed. - If localisation setup fails, fall back to default clap ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~198-~198: Consider using a different verb for a more formal wording.
Context: ... error output. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[style] ~205-~205: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...idence: - netsuke --help output with localised strings (from /tmp/netsuke-help.log...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~207-~207: Possible missing comma found.
Context: ...tmp/netsuke-help.log`). - A failing CLI invocation showing localised error output (record ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~207-~207: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...g`). - A failing CLI invocation showing localised error output (record the command and ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~225-~225: Would you like to use the Oxford spelling “localizer”? The spelling ‘localiser’ is also correct.
Context: ...e CLI parsing in src/main.rs uses the localiser and merges config layers before calli...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~233-~233: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...ted to cover OrthoConfig integration, localised help, CLI output clarity, and test/docu...
(OXFORD_SPELLING_Z_NOT_S)
🔍 Remote MCP Deepwiki
Additional relevant context for reviewing PR #254
-
Repository wiki contains a Command Line Interface user guide and developer testing guidance that are directly relevant to this PR's changes to CLI behaviour, localization, and tests:
- Wiki sections: "User Guide → Command Line Interface" and "Developer Guide → Testing Framework" (useful for verifying docs and test updates).
-
The repo documents a "Six-Stage Pipeline" and "Module Organization" under Architecture — useful background when assessing whether CLI/config changes interact with manifest parsing, IR generation, or the runner/subcommands.
-
There is a "Code Quality Standards" and CI/CD guidance under Developer Guide which is relevant given the PR adds a compile-time feature gate (serde_json) and many new tests; reviewers should confirm CI will run with the new feature set and test matrix.
Sources:
- Deepwiki repository structure for leynos/netsuke
⏰ 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). (6)
- GitHub Check: release / build-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux 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-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: build-test (ubuntu-latest, stable)
🔇 Additional comments (45)
src/cli_policy.rs (1)
20-24: Documentation examples correctly demonstrate locale field initialisation.Both code examples in the documentation now properly show the
locale: Nonefield when constructing aClistruct. This aligns with the new locale field added in this PR and ensures the examples remain accurate and compilable.Also applies to: 34-39
src/runner/process/file_io.rs (1)
34-34: Logging messages improved for clarity and idiomaticity.The wording updates from "Generated...at" to "Wrote...to" make user-facing log output clearer and more consistent with standard English conventions. These minor improvements align with the PR's objective to refine CLI output for clarity.
Also applies to: 87-87
docs/roadmap.md (1)
187-188: Roadmap task marked complete as CLI output refinement is delivered.The "Refine all CLI output for clarity" task is correctly marked as completed, reflecting the localisation support, Fluent-based message catalogues, and logging improvements delivered in this PR.
src/runner/process/mod.rs (1)
126-129: LGTM!The wording change from "Running" to "Executing" is consistent with the broader log-message clarity improvements across the runner modules.
src/runner/mod.rs (4)
122-122: LGTM!The
locale: Nonefield addition keeps the doc example in sync with the updatedClistruct definition.
173-177: Improved observability with structured logging.The structured
info!macro with explicittargetandsubcommandfields provides better filtering and searchability compared to the previous unstructured message.
234-234: LGTM!Doc example updated to reflect the new
localefield.
274-274: LGTM!Doc example updated to reflect the new
localefield.locales/en-US/messages.ftl (3)
1-6: LGTM!The CLI about and usage keys are well-structured with appropriate placeholders.
7-14: LGTM!Subcommand descriptions are clear and action-oriented. The use of "artefacts" at line 9 follows en-GB-oxendict spelling conventions.
19-21: Duplicate message templates are justified, but consider whether distinct error scenarios merit distinct messages.
clap-error-invalid-valueandclap-error-value-validationcorrectly map to distinctclaperror kinds: the former applies when a value is not in the set of allowed values, whilst the latter applies when a value fails custom validation or parser logic. Keeping separate message keys is the right choice.However, both currently use identical templates across all locales. If your users benefit from understanding why validation failed (values disallowed versus values invalid via parser), consider varying the templates. Otherwise, the current approach is acceptable—both represent user-supplied values that failed acceptance checks.
Cargo.toml (3)
8-8: LGTM!Including
locales/**ensures locale resources are packaged with the crate.
60-62: LGTM!Build-dependencies are correctly specified with SemVer-compatible caret requirements (implicit). The
preserve_orderfeature onserde_jsonmaintains deterministic key ordering during config serialization.
55-55:ortho_config = "0.7.0"is a published version on crates.io and the version specification complies with the SemVer-compatible caret requirements.docs/execplans/cli-output-clarity.md (2)
22-31: LGTM!Progress tracking is clear and timestamps follow ISO 8601 format.
78-84: LGTM!Acronyms DI and BDD are expanded on first use, and the first-person pronoun issue has been resolved.
build.rs (2)
23-24: LGTM!The
LocalizedParseFntype alias clearly documents the expected signature for the localized CLI parsing entry point.
75-80: Excellent compile-time contract enforcement.These static const assertions validate function signatures at build time, ensuring
climodule exports remain compatible with the documented API. The build fails immediately if signatures drift, catching breaking changes early.docs/users-guide.md (1)
491-506: LGTM!The Network Policy Options section clearly documents the
fetchhelper's security controls with wildcard support noted.docs/netsuke-design.md (1)
2013-2031: LGTM!The documentation changes correctly describe the new CLI design with OrthoConfig layering and Fluent localisation. The text uses en-GB-oxendict spelling ("localised", "behaviour"), maintains 80-column wrapping, and avoids personal pronouns as required by the coding guidelines.
tests/features/cli.feature (1)
53-58: LGTM!The updated error message expectation and new Spanish localisation scenario provide good coverage for the locale-aware CLI parsing. The scenario correctly places
--locale es-ESbefore the invalid subcommand, ensuring the locale hint is extracted before clap encounters the error.src/lib.rs (1)
11-11: LGTM!Exposing
cli_localizationpublicly enables tests and downstream consumers to build localisers. This aligns with the module's use inmain.rsand the BDD test steps.locales/es-ES/messages.ftl (1)
1-21: LGTM!The Spanish translations are well-structured and the error message keys align with the clap error formatting expected by the localisation layer. The "Subcomando desconocido" message at line 20 correctly matches the BDD test expectation in
cli.feature.src/main.rs (4)
24-38: Doubleinit_tracingcall will panic at runtime.When
merge_with_configfails,init_tracing(Level::ERROR)is called at line 27. However, if it succeeds,init_tracing(max_level)is called at line 38. If the error path is taken first on one invocation and success on another, this is fine. But consider the control flow: the error path returns early, so only one call occurs per execution. However,fmt().init()internally callsset_global_default, which panics if called more than once in the same process.This is actually safe because the error path returns
ExitCode::FAILUREbefore reaching line 38. Disregard this concern.
12-22: LGTM!The locale detection flow correctly prioritises CLI
--localeover theNETSUKE_LOCALEenvironment variable. The localiser is built before parsing, ensuring error messages are localised from the outset.
24-31: LGTM!The config merge error handling correctly initialises tracing at ERROR level before logging, ensuring the failure message is visible. The early return prevents the double-init issue.
49-54: LGTM!The
init_tracinghelper centralises subscriber setup. Writing to stderr is appropriate for diagnostic output.src/cli_localization.rs (3)
1-4: LGTM!The module documentation clearly describes the purpose and the fallback strategy.
24-34: LGTM!The
Localizerimplementation forLayeredLocalizercorrectly delegates to the primary localiser first and falls back when the lookup returnsNone.
63-81: LGTM!The
build_localizerfunction correctly prioritises the preferred locale, falling back to English when parsing fails or the locale is unsupported. The#[must_use]attribute ensures callers do not discard the result.tests/bdd/steps/cli.rs (2)
22-31: LGTM!The updated
apply_clifunction mirrors the locale-aware parsing flow inmain.rs, ensuring BDD tests exercise the same code paths as production. The locale hint extraction and localiser construction are correctly sequenced.
85-87: LGTM!Removing the
mutbinding and delegating towith_default_command()is cleaner and aligns with the method's ownership semantics.src/host_pattern.rs (3)
10-42: LGTM! ValidationContext addresses the string-heavy function arguments alert.The refactoring groups the original pattern reference into a context struct, reducing parameter repetition across validation calls. This directly addresses the CodeScene alert about 58.3% string arguments.
196-202: LGTM! FromStr implementation is idiomatic.Delegating to
Self::parseensures consistent behaviour across all string-to-HostPattern conversions.
218-226: LGTM! Deserialize implementation correctly maps parse errors.Using
serde::de::Error::customto convertHostPatternErrorpreserves the structured error message for downstream consumers.tests/cli_tests.rs (3)
327-380: LGTM! locale_hint_from_args tests cover the requested edge cases.These tests address the previous review comment requesting coverage for space form, equals form, trailing flag, double-dash handling, and last-wins precedence.
455-486: LGTM! merge_with_config integration test validates end-to-end precedence.This test exercises config discovery, environment variables, and CLI merging as requested in the previous review comment.
488-501: LGTM! Spanish localisation test validates error message translation.The test confirms that invalid subcommand errors are localised correctly.
src/cli.rs (7)
6-19: LGTM! Import organization is clean.The imports are well-grouped by source crate and support the new localisation and configuration features.
58-66: LGTM! parse_locale validates using LanguageIdentifier.Delegating to
LanguageIdentifier::from_strensures only valid BCP 47 locale tags are accepted.
78-149: LGTM! Cli struct enhancements for OrthoConfig integration.The
#[ortho_config(...)]attributes correctly configure default handling, merge strategies for list fields, and CLI-default-as-absent semantics for proper layering.
254-276: LGTM! Subcommand localisation avoids unnecessary cloning.Using
std::mem::taketo move ownership addresses the previous performance concern about cloning subcommands.
278-305: LGTM! locale_hint_from_args handles edge cases correctly.The implementation properly handles
--separator, equals form, space form, and last-wins semantics as verified by the corresponding tests.
350-397: LGTM! merge_with_config implements correct layer precedence.The function composes defaults → file → environment → CLI layers with proper error accumulation. Preserving
cli.commandafter merge is intentional, as noted in the previous review comment.
222-224: Document theusage_bodyfunction.Public utility functions require Rustdoc comments per coding guidelines. Add a
///comment explaining the purpose.Proposed fix
+/// Extract the body of a usage string, stripping the "Usage: " prefix if present. fn usage_body(usage: &str) -> &str { usage.strip_prefix("Usage: ").unwrap_or(usage) }Likely an incorrect or invalid review comment.
This change integrates OrthoConfig for layered configuration and Fluent for localized CLI help and error messages. English and Spanish locales are included, with support for the --locale flag and NETSUKE_LOCALE environment variable. It updates CLI parsing to use localized clap errors, refines user-facing CLI output for clarity, and implements configuration merging respecting OrthoConfig precedence. Comprehensive unit and behavioral tests verify parsing, localization fallback, and network policy enforcement derived from CLI flags. The extensive test suite replaces the older monolithic CLI tests, introducing modular test files for parsing, locale, merge, and policy coverage. Documentation is updated to reflect localization support, configuration layering, and refined CLI behaviors. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.rs (1)
1-5: File exceeds the 400-line limit (currently 402 lines).Per coding guidelines, files must not exceed 400 lines. Extract the localisation helpers (
localize_command,localize_subcommands,usage_body,locale_hint_from_args) into a dedicated module (e.g.,cli_l10n.rsor merge intocli_localization.rs) to bring this file under the threshold.
♻️ Duplicate comments (4)
docs/users-guide.md (1)
488-489: Past spelling concern already addressed.The documentation now correctly uses "Localize" (en-GB-oxendict -ize form) as requested in previous reviews. This aligns with the coding guidelines.
src/host_pattern.rs (1)
223-235: Serialization no longer clones unnecessarily for non-wildcard patterns.The previous review feedback has been addressed: non-wildcard patterns now call
serializer.serialize_str(&self.pattern)directly without allocation, whilst wildcard patterns allocate only when necessary viaformat!.src/cli.rs (2)
336-348: Documentation added as requested in previous review.The brief doc comments on
env_providerandconfig_discoveryaddress the earlier feedback.
350-353: Documentation added as requested.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
Cargo.tomlbuild.rsdocs/execplans/cli-output-clarity.mddocs/netsuke-design.mddocs/users-guide.mdsrc/cli.rssrc/cli_localization.rssrc/host_pattern.rssrc/lib.rssrc/stdlib/network/policy/mod.rstests/cli_tests/helpers.rstests/cli_tests/locale.rstests/cli_tests/merge.rstests/cli_tests/mod.rstests/cli_tests/parsing.rstests/cli_tests/policy.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.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/cli_tests/mod.rssrc/lib.rstests/cli_tests/parsing.rstests/cli_tests/locale.rssrc/host_pattern.rssrc/stdlib/network/policy/mod.rstests/cli_tests/policy.rstests/cli_tests/merge.rssrc/cli.rsbuild.rstests/cli_tests/helpers.rssrc/cli_localization.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/cli_tests/mod.rssrc/lib.rstests/cli_tests/parsing.rstests/cli_tests/locale.rssrc/host_pattern.rssrc/stdlib/network/policy/mod.rstests/cli_tests/policy.rstests/cli_tests/merge.rssrc/cli.rsbuild.rstests/cli_tests/helpers.rssrc/cli_localization.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: In RustCargo.toml, use explicit version ranges and keep dependencies up-to-date.
All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3") to allow safe non-breaking updates.
The use of wildcard (*) or open-ended inequality (>=) version requirements inCargo.tomlis strictly forbidden.
Tilde requirements (~) inCargo.tomlshould only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
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 (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)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 use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/users-guide.mddocs/execplans/cli-output-clarity.mddocs/netsuke-design.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/users-guide.mddocs/execplans/cli-output-clarity.mddocs/netsuke-design.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/users-guide.mddocs/execplans/cli-output-clarity.mddocs/netsuke-design.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/users-guide.mddocs/execplans/cli-output-clarity.mddocs/netsuke-design.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/users-guide.mddocs/execplans/cli-output-clarity.mddocs/netsuke-design.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/cli_tests/mod.rssrc/lib.rstests/cli_tests/parsing.rstests/cli_tests/locale.rssrc/host_pattern.rssrc/stdlib/network/policy/mod.rstests/cli_tests/policy.rstests/cli_tests/merge.rssrc/cli.rsbuild.rstests/cli_tests/helpers.rssrc/cli_localization.rs
🧬 Code graph analysis (5)
tests/cli_tests/mod.rs (1)
src/stdlib/network/mod.rs (1)
policy(360-360)
tests/cli_tests/locale.rs (3)
tests/cli_tests/helpers.rs (1)
os_args(5-7)src/cli.rs (2)
locale_hint_from_args(282-307)parse_with_localizer_from(317-334)src/cli_localization.rs (1)
build_localizer(68-85)
tests/cli_tests/policy.rs (3)
src/cli.rs (1)
default(168-182)src/stdlib/network/policy/mod.rs (2)
default(323-325)evaluate(286-319)src/host_pattern.rs (1)
parse(176-182)
tests/cli_tests/merge.rs (2)
src/cli.rs (2)
default(168-182)merge_with_config(361-402)test_support/src/env_lock.rs (1)
acquire(24-29)
src/cli.rs (2)
src/host_pattern.rs (2)
matches(184-196)new(33-35)tests/cli_tests/parsing.rs (1)
default(26-43)
🪛 LanguageTool
docs/users-guide.md
[uncategorized] ~488-~488: Loose punctuation mark.
Context: ...e logging output. - --locale <LOCALE>: Localize CLI help and error messages (f...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~496-~496: Loose punctuation mark.
Context: ...ions: - --fetch-allow-scheme <SCHEME>: Allow additional URL schemes beyond the...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~499-~499: Loose punctuation mark.
Context: ...defaults. - --fetch-allow-host <HOST>: Allow the provided hostnames when defau...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~502-~502: Loose punctuation mark.
Context: ...pported). - --fetch-block-host <HOST>: Always block the provided hostnames (wi...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~505-~505: Loose punctuation mark.
Context: ...e allowlisted. - --fetch-default-deny: Deny all hosts by default and only perm...
(UNLIKELY_OPENING_PUNCTUATION)
docs/execplans/cli-output-clarity.md
[uncategorized] ~105-~105: Possible missing comma found.
Context: ... explicit handling for missing required values and make sure clap defaults do not i...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~109-~109: The word ‘help’ is a conjunction. In formal texts, use “to”.
Context: ...where needed). 3. Localize CLI help and clap errors. Create Fluent resources (f...
(HELP_AND_VB)
[uncategorized] ~188-~188: Loose punctuation mark.
Context: ... and config layering. - make check-fmt, make lint, and make test pass. - `d...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~198-~198: Consider using a different verb for a more formal wording.
Context: ... error output. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[uncategorized] ~207-~207: Possible missing comma found.
Context: ...tmp/netsuke-help.log`). - A failing CLI invocation showing localized error output (record ...
(AI_HYDRA_LEO_MISSING_COMMA)
🔍 Remote MCP Deepwiki
Additional context gathered (relevant to reviewing PR #254)
-
Repository docs/wiki confirm CLI/user-guide, testing, and CI expectations that PR touches:
- CLI uses clap; default command is build; man page generated by build.rs — verify build.rs changes keep man generation CI-friendly and SOURCE_DATE_EPOCH handling.
- Testing matrix includes unit, snapshot (insta), BDD (cucumber) and integration tests; CI runs cargo test --all-features and treats warnings as errors — new tests/localization must pass existing CI gates.
- Test infrastructure uses cap-std/camino for filesystem, and strict clippy rules; changes adding ortho_config and serde_json feature should not break lint/format/test policies.
-
Manifest/template/templating behavior and expectations (important since PR adds localization + ortho_config):
- Manifest parsing is two-pass: structural expansion (foreach/when) then deserialization + string rendering via minijinja; custom functions (env, glob) and strict undefined behavior are used — ensure localization hooks and OrthoConfig merging integrate with this pipeline without changing rendering order.
- glob/env functions and cap-std/camino constraints exist; tests rely on deterministic ordering and snapshot stability — new locale files and CLI flow must preserve determinism used by insta snapshots.
-
IR / Ninja generation and security expectations (relevant because CLI/localization affects error messages and man page text):
- Commands are shell-escaped via shell-quote and validated with shlex; multi-line scripts are encoded for printf %b wrapper — localization must not introduce unescaped content into command paths/messages.
- Generated Ninja and snapshots must remain deterministic (sorted actions/edges); adding locale resources or ortho_config features must not change ordering of IR/Ninja outputs used in tests.
-
Build, CI, and release constraints that reviewers should check:
- CI runs formatting, clippy deny-warnings, and full tests on stable + MSRV (1.89.0). New Cargo.toml defaults/features (serde_json, ortho_config) must not break MSRV or clippy.
- build.rs produces the man page and uses SOURCE_DATE_EPOCH for reproducible builds — ensure new type alias and const checks do not alter build script side-effects or cause warnings treated as errors.
Tools used
- Deepwiki_read_wiki_contents — repository wiki and docs content retrieval.
⏰ 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-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 (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: build-test (ubuntu-latest, stable)
🔇 Additional comments (34)
docs/users-guide.md (1)
530-548: Configuration and localization section is clear and complete.The section accurately describes OrthoConfig discovery order, environment variable conventions, and locale selection. The documentation follows en-GB-oxendict spelling conventions throughout.
docs/netsuke-design.md (2)
1052-1109: API changes accurately documented in the class diagram.The updated signatures for
StdlibConfig,WhichModule,WhichResolver,EnvSnapshot,WhichConfig, andWorkspaceSkipListcorrectly reflect the new type aliases (OptionalPath,StringList, etc.) and parameter changes introduced in the PR. The diagram maintains consistency with the codebase.
2013-2031: CLI implementation section accurately describes the integration.The documentation clearly explains OrthoConfig layering, locale selection via
--localeorNETSUKE_LOCALE, configuration discovery order, and environment variable conventions. This aligns with the implementation changes across the PR.src/lib.rs (1)
8-8: New localization module correctly exposed.The
pub mod cli_localizationdeclaration appropriately exports the localization functionality for use inmain.rsand tests. Placement betweencliandcli_policyis logical and follows the module organization pattern.tests/cli_tests/helpers.rs (1)
5-7: Helper function is clear and appropriate.The
os_argsfunction provides a straightforward conversion for test arguments. Thepub(super)visibility correctly restricts access to the test module hierarchy, and the implementation is concise and correct.tests/cli_tests/locale.rs (8)
1-1: Module documentation is clear and appropriate.The module-level doc comment succinctly describes the module's purpose.
3-8: Imports are well-organized.All imports are necessary and correctly scoped for test usage.
10-19: Test logic is correct and well-structured.The test validates space-form locale parsing with clear diagnostics via
ensure!.
21-30: Test validates equals-form parsing correctly.The test complements the space-form test and maintains consistent structure.
32-41: Edge case handling is properly validated.The test correctly verifies that a trailing
--localeflag without a value yieldsNone.
43-52: Double-dash terminator behaviour is correctly validated.The test ensures arguments after
--are properly ignored during locale hint extraction.
54-63: Last-flag precedence is correctly validated.The test ensures that when multiple
--localeflags are provided, the last one takes precedence.
65-79: Spanish localization integration is properly tested.The test validates end-to-end localized error messaging by building a Spanish localizer and confirming the error contains "Subcomando desconocido".
docs/execplans/cli-output-clarity.md (6)
1-8: Documentation opening is correct and well-formatted.The title uses proper en-GB-oxendict spelling with "-ized" form, and the living document note is appropriately wrapped.
9-31: Purpose and progress sections are clear and actionable.The purpose statement clearly defines success criteria, and the progress checklist tracks completion effectively.
38-77: Decision log and context sections provide clear guidance.The decision log captures rationales for OrthoConfig and localization choices, whilst the context section properly references relevant files and documentation.
78-143: Testing guidance and plan of work are comprehensive.Acronyms are properly expanded on first use, the plan is detailed and actionable, and en-GB-oxendict spelling is consistently applied throughout.
144-191: Concrete steps and validation criteria are clear and executable.The shell commands follow best practices with pipefail and tee, and validation criteria are measurable and comprehensive.
192-233: Closing sections complete the ExecPlan effectively.Idempotence guidance, artifact requirements, and interface specifications are all clearly documented, with consistent en-GB-oxendict spelling throughout.
Cargo.toml (3)
8-8: Locale resources correctly included in package.The addition of
locales/**to the include list ensures localization resources are distributed with the package.
56-56: OrthoConfig dependency version is correct.The version specification uses implicit caret semantics as permitted by the coding guidelines.
61-63: Build dependencies are correctly specified.The additions of
ortho_config,serde, andserde_jsonto build-dependencies support the build-time localization and type handling introduced inbuild.rs.src/stdlib/network/policy/mod.rs (3)
11-11: HostCandidate import is correct.The import addition properly brings the new wrapper type into scope for the matching calls below.
298-306: Blocked-hosts matching correctly updated.The change wraps the host string in
HostCandidateto match the updatedHostPattern::matchessignature, improving type safety as per the newtype pattern.
308-316: Allowlist matching consistently updated.The change mirrors the blocked-hosts update, ensuring consistent usage of
HostCandidateacross both policy checks.tests/cli_tests/policy.rs (5)
1-8: Module documentation and imports are appropriate.The module doc clearly describes the test scope, and imports are correctly organized.
10-30: Default HTTPS policy is correctly validated.The test verifies that the default policy permits HTTPS whilst rejecting HTTP with the appropriate
SchemeNotAllowederror.
32-57: Default-deny allowlist behaviour is thoroughly tested.The test validates that default-deny mode permits only allowlisted hosts and rejects others with
HostNotAllowlisted.
59-83: Blocklist precedence is correctly validated.The test confirms that blocklist entries override allowlist entries and validates both the error variant and error message text.
85-96: Invalid scheme rejection is properly tested.The test validates that schemes starting with digits are rejected with an appropriate error message.
src/host_pattern.rs (2)
10-27: Newtypes successfully reduce string-heavy function arguments.The introduction of
HostPatternInput<'a>andHostCandidate<'a>newtypes addresses the CodeScene flag about string-heavy function arguments. By wrapping&strin domain-specific types, you've improved type safety and reduced primitive obsession whilst making function signatures more self-documenting.
28-61:ValidationContextsuccessfully refactors validation logic.Moving validation logic into
ValidationContext::validate_labelreduces string parameters from the originalvalidate_label(label, original)toself.validate_label(label). This captures the original pattern for error messages within the context, improving the design and contributing to the resolution of the string-heavy arguments issue.src/cli.rs (2)
256-278: The cloning concern from the previous review has been addressed.The use of
std::mem::taketo extract, modify, and reassign each subcommand avoids the unnecessary clone that was previously flagged.
309-334: Well-structured public API with proper error localisation.The function correctly localizes both the command and any parsing errors. The documentation is complete with an
# Errorssection as expected for fallible public APIs.
- Moved localization helpers and related functions from `src/cli.rs` to new module `src/cli_l10n.rs`. - Updated `build.rs` and `src/lib.rs` to include the new module. - Simplified `cli.rs` by re-exporting and using functions from `cli_l10n`. This improves code organization by separating localization concerns from CLI definitions. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
These tests validate OrthoConfig layer precedence including defaults, file, env, CLI, list-value appending, and user-provided override extraction. Added additional documentation comments to test modules for clarity and maintenance. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai Have the following now been resolved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10872 @leynos: I've reviewed the current state of ✅ ResolvedComment 3 (DI and BDD acronyms): This has been addressed. Line 69 area now reads:
Both acronyms are properly expanded on first use. ❌ Not ResolvedComment 1 (Hardcoded ortho_config version): The "Interfaces and Dependencies" section still contains:
The hardcoded Comment 2 (Fluent resources directory convention): Step 3 under "Plan of Work" mentions:
This doesn't clarify whether Summary: One of three issues has been resolved; two remain outstanding. |
…cumentation - Improve instructions on setting up Fluent localization, emphasizing following the existing locales/ layout. - Refine instructions for wiring FluentLocalizer before parsing CLI and handling errors. - Update dependency addition instructions to refer to the version in Cargo.toml rather than specifying a caret version explicitly. - Ensure documentation reflects current best practices for CLI output clarity and dependencies. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
build.rsdocs/execplans/cli-output-clarity.mdsrc/cli.rssrc/lib.rstests/cli_tests/merge.rstests/cli_tests/mod.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/lib.rstests/cli_tests/mod.rsbuild.rstests/cli_tests/merge.rssrc/cli.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/lib.rstests/cli_tests/mod.rsbuild.rstests/cli_tests/merge.rssrc/cli.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 (en-GB) spelling with -ize endings (realize, organization), -lyse endings (analyse, paralyse, catalyse), -our endings (colour, behaviour), -re endings (calibre, centre, fibre), double 'l' (cancelled, counsellor), maintained 'e' (likeable, liveable), and -ogue endings (analogue, catalogue)
Use the Oxford comma in documentation: 'ships, planes, and hovercraft'
Treat company names as collective nouns in documentation: 'Concordat Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#,##,###) in order without skipping levels
Follow markdownlint recommendations for Markdown formatting
Provide code blocks and lists using standard Markdown syntax
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first level bullet and renumber lists when items change in documentation
Prefer inline links using[text](url)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 use in documentation (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use GitHub-flavoured numeric footnotes referenced as[^1]in documentation
Number footnotes in order of appearance in the document
Caption every table and every diagram in documentation
Usefor embedding figures with brief alt te...
Files:
docs/execplans/cli-output-clarity.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/cli-output-clarity.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/cli-output-clarity.md
docs/**/{README,readme,GUIDE,guide,*.md}
📄 CodeRabbit inference engine (docs/adr-001-replace-serde-yml-with-serde-saphyr.md)
Document in Netsuke's README and user guide that the project now supports YAML 1.2 (not YAML 1.1) and that duplicate keys in manifests are no longer permitted
Files:
docs/execplans/cli-output-clarity.md
docs/**/!(README).md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
Avoid first and second person personal pronouns outside the
README.mdfile in documentation
Files:
docs/execplans/cli-output-clarity.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/lib.rstests/cli_tests/mod.rsbuild.rstests/cli_tests/merge.rssrc/cli.rs
🧬 Code graph analysis (4)
tests/cli_tests/mod.rs (1)
src/stdlib/network/mod.rs (1)
policy(360-360)
build.rs (2)
src/cli_l10n.rs (1)
locale_hint_from_args(68-93)src/cli.rs (2)
merge_with_config(280-321)parse_with_localizer_from(234-251)
tests/cli_tests/merge.rs (1)
src/cli.rs (3)
default(170-184)parse_with_localizer_from(234-251)merge_with_config(280-321)
src/cli.rs (1)
src/cli_l10n.rs (2)
locale_hint_from_args(68-93)localize_command(14-40)
🪛 LanguageTool
docs/execplans/cli-output-clarity.md
[uncategorized] ~105-~105: Possible missing comma found.
Context: ... explicit handling for missing required values and make sure clap defaults do not i...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~109-~109: The word ‘help’ is a conjunction. In formal texts, use “to”.
Context: ...where needed). 3. Localize CLI help and clap errors. Create Fluent resources (f...
(HELP_AND_VB)
[uncategorized] ~189-~189: Loose punctuation mark.
Context: ... and config layering. - make check-fmt, make lint, and make test pass. - `d...
(UNLIKELY_OPENING_PUNCTUATION)
[style] ~199-~199: Consider using a different verb for a more formal wording.
Context: ... error output. - If tests fail mid-run, fix the underlying issue and re-run the sam...
(FIX_RESOLVE)
[uncategorized] ~208-~208: Possible missing comma found.
Context: ...tmp/netsuke-help.log`). - A failing CLI invocation showing localized error output (record ...
(AI_HYDRA_LEO_MISSING_COMMA)
🔍 Remote MCP
I'll help you gather additional context for reviewing this PR. Let me search for information about the key libraries and patterns being used.Based on my research, I've gathered relevant context about the key technologies used in this PR. Let me provide a concise summary of findings that are directly applicable to reviewing this PR.
Summary of Key Findings for PR Review
Fluent Localization (FTL Format)
FTL is a localization file format designed to be simple to read while allowing complex concepts from natural languages like gender, plurals, and conjugations. The PR's message files (locales/en-US/messages.ftl, locales/es-ES/messages.ftl) follow this standard approach.
Best Practice Validation Points:
- All attributes of messages in the reference language (en-US) must be present in localizations, enforced by tools that warn on missing or obsolete attributes. This is relevant for verifying that the Spanish translations are complete.
- In FTL files, indentation is part of the syntax; incorrect indentation will lead to parsing errors. The .ftl files should be verified for consistent indentation.
Clap Localization Patterns
Standard clap localization approaches include: replacing hard-coded strings in help templates (e.g., "Usage") with localized versions via Command::help_template, and using custom formatters for error messages. The PR's src/cli_l10n.rs implements similar patterns with the localize_command function.
Configuration Merge Pattern:
A common layered config approach involves: getting all matches from clap, removing default values to identify overrides, calling merge with parsed arguments using original matches, adding other layers (TOML, environment variables), and deserializing into the config struct. This aligns with the PR's merge_with_config function design.
Configuration Layering Standard
Layered configuration is a established pattern in Rust: Hierarchical merging supports layering configurations, ensuring CLI arguments override environment variables, which in turn override file-based configurations. The PR implements this correctly with the precedence order: defaults → file → environment → CLI.
⏰ 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-linux (x86_64-unknown-linux-gnu, linux-amd64, amd64) / Build linux 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-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: Sourcery review
- GitHub Check: build-test (ubuntu-latest, stable)
- GitHub Check: build-test (ubuntu-latest, nightly, true)
🔇 Additional comments (10)
tests/cli_tests/mod.rs (1)
1-9: LGTM! Clean test module structure.The module documentation is accurate and the submodule organisation is clear and follows standard conventions.
src/lib.rs (1)
11-12: LGTM! Module declarations follow standard patterns.The private
cli_l10nmodule and publiccli_localizationmodule are declared correctly and align with the localisation infrastructure introduced in this PR.docs/execplans/cli-output-clarity.md (1)
1-234: LGTM! ExecPlan document is comprehensive and well-structured.The document follows the established format, uses correct en-GB-oxendict spelling throughout, and provides clear guidance for CLI output refinement and localisation. Previous review concerns have been addressed.
tests/cli_tests/merge.rs (3)
19-44: LGTM! Clear test validating default value extraction.The test correctly verifies that default values are not treated as user-provided CLI overrides. Error context and assertion messages are clear and helpful.
46-90: LGTM! Comprehensive test of layer precedence and list appending.The test uses distinct values for each layer (jobs: 1→2→3→4; schemes: https→http→ftp→git), making it easy to verify correct precedence and list-appending behaviour. Well-designed test.
92-124: LGTM! Integration test correctly uses environment guards.The test properly uses
EnvLockandEnvVarGuardfromtest_supportfor safe environment variable manipulation, complying with Rust 2024 environment safety guidelines. The test validates that environment variables override file config when CLI values are absent.build.rs (4)
3-10: LGTM! Import additions support localised parsing.The addition of
ArgMatchesand reorganisation of imports are appropriate for the new localisation and configuration merge checks.
18-19: LGTM! Module declaration follows established patterns.The
cli_l10nmodule declaration with path attribute is consistent with the existingcliandhost_patternmodule declarations in this build script.
26-27: LGTM! Type alias correctly captures localised parse signature.The
LocalizedParseFntype alias accurately represents the signature ofparse_with_localizer_from, enabling the const check at line 82 to verify the function remains linked.
78-84: LGTM! Const checks ensure shared symbols remain linked.The const expressions correctly reference the locale hint extraction, config merge, localised parsing, and host pattern functions, ensuring these symbols remain linked when the build script is compiled without tests.
- Remove default values from CLI override extraction to prevent false positives. - Implement `cli_overrides_from_matches` to keep only flags explicitly set via command line. - Adjust tests accordingly to reflect updated override behavior. - Remove serde_json from default Cargo features and enable it explicitly in ortho_config deps. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai Have the following now been resolved? ❌ Not Resolved Comment 1 (Hardcoded ortho_config version): The "Interfaces and Dependencies" section still contains: The hardcoded 0.7.0 example remains. Consider rephrasing to reference the actual dependency management approach, for example: "Add ortho_config as a dependency at the version specified in Cargo.toml" or similar. Comment 2 (Fluent resources directory convention): Step 3 under "Plan of Work" mentions: This doesn't clarify whether locales/ is a new convention being established or ties to an existing i18n directory layout. Consider adding a sentence like "This establishes locales/ as the standard i18n directory structure for the project" or "This follows the existing locale convention in locales/". |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10918 @leynos: Both issues have now been resolved! ✅ ✅ Comment 1 (Hardcoded ortho_config version)The "Interfaces and Dependencies" section now reads:
The hardcoded ✅ Comment 2 (Fluent resources directory convention)Step 3 in the "Plan of Work" section now includes:
The added sentence clarifies that Summary: All previously unresolved documentation issues have been addressed. |
1 participant
Summary
Changes
Rationale
Provides a robust, test-covered approach to localising CLI help, error messages and command output, while layering defaults, config files, environment variables, and CLI overrides via OrthoConfig. This helps ensure predictable behaviour across locales and easier testing.
Verification
Next steps
🌿 Generated by Terry
📎 Task: https://www.terragonlabs.com/task/101b9946-a656-4e1c-8851-42910e061429