Complete CLI config struct implementation with OrthoConfig (0.8.0)#273
Open
Complete CLI config struct implementation with OrthoConfig (0.8.0)#273
Conversation
Contributor
Reviewer's GuideRefactors the CLI into separate parser, config, and merge layers driven by a new OrthoConfig-derived CliConfig schema, wires layered configuration (including build subcommand defaults and typed presentation enums) through this path, upgrades ortho_config to 0.8.0, and updates tests and documentation to match the new precedence and configuration model. Sequence diagram for CLI parse and layered configuration mergesequenceDiagram
actor User
participant Main as main_rs
participant Parser as parser_Cli
participant Merge as merge_module
participant Ortho as OrthoConfig_runtime
participant Runner as runner_run
User->>Main: invoke_netsuke(args)
Main->>Parser: parse_with_localizer_from(args, localizer)
Parser->>Parser: configure_validation_parsers()
Parser->>Parser: Cli::from_arg_matches_mut()
Parser-->>Main: (parsed_cli Cli, matches ArgMatches)
Main->>Merge: merge_with_config(parsed_cli, matches)
Merge->>Merge: config_discovery(directory)
Merge->>Ortho: compose_layers(defaults, files, env, cli_overrides)
Ortho-->>Merge: LayerComposition
Merge->>Ortho: CliConfig::merge_from_layers(LayerComposition)
Ortho-->>Merge: merged_config CliConfig
Merge->>Merge: apply_config(parsed_cli, merged_config)
Merge-->>Main: merged_cli Cli
Main->>Main: init_tracing(merged_cli.verbose)
Main->>Main: prefs = output_prefs::resolve(merged_cli.no_emoji_override())
Main->>Runner: run(&merged_cli, prefs)
Runner->>Runner: progress_enabled = merged_cli.progress_enabled()
Runner-->>Main: Result
Main-->>User: ExitCode
Class diagram for layered CLI configuration (CliConfig, parser Cli, and merge)classDiagram
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_bool_ accessible
+Option_bool_ no_emoji
+Option_bool_ progress
+Option_ColourPolicy_ colour_policy
+Option_SpinnerMode_ spinner_mode
+Option_OutputFormat_ output_format
+Option_Theme_ theme
+Option_Commands_ command
+with_default_command() Cli
+no_emoji_override() Option_bool_
+progress_enabled() bool
}
class Commands {
<<enumeration>>
Build
Clean
Graph
Manifest
}
class BuildArgs {
+Option_PathBuf_ emit
+Vec_String_ targets
}
class CliConfig {
+PathBuf file
+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_bool_ accessible
+Option_bool_ no_emoji
+Option_bool_ progress
+Option_ColourPolicy_ colour_policy
+Option_SpinnerMode_ spinner_mode
+Option_OutputFormat_ output_format
+Option_Theme_ theme
+CommandConfigs cmds
+post_merge(ctx PostMergeContext) OrthoResult~()~
}
class CommandConfigs {
+BuildConfig build
}
class BuildConfig {
+Option_PathBuf_ emit
+Vec_String_ targets
}
class ColourPolicy {
<<enumeration>>
Auto
Always
Never
}
class SpinnerMode {
<<enumeration>>
Auto
Enabled
Disabled
}
class OutputFormat {
<<enumeration>>
Human
Json
}
class Theme {
<<enumeration>>
Auto
Unicode
Ascii
}
class MergeModule {
+merge_with_config(cli Cli, matches ArgMatches) OrthoResult~Cli~
+apply_config(parsed Cli, config CliConfig) Cli
+resolve_command(parsed Option_Commands_, build_defaults BuildConfig) Commands
+canonical_theme(theme Option_Theme_, no_emoji Option_bool_) Option_Theme_
}
class ParserModule {
+parse_with_localizer_from(iter I, localizer Arc_Localizer_) Result~(Cli,ArgMatches),clap::Error~
+validation_message(localizer Localizer, key str, args Option_LocalizationArgs_, fallback str) String
}
Cli --> Commands : uses
Commands --> BuildArgs : Build(BuildArgs)
CliConfig --> CommandConfigs : has
CommandConfigs --> BuildConfig : has
Cli --> ColourPolicy : uses
Cli --> SpinnerMode : uses
Cli --> OutputFormat : uses
Cli --> Theme : uses
CliConfig --> ColourPolicy : uses
CliConfig --> SpinnerMode : uses
CliConfig --> OutputFormat : uses
CliConfig --> Theme : uses
MergeModule --> CliConfig : merges
MergeModule --> Cli : produces
ParserModule --> Cli : parses
ParserModule ..> MergeModule : passes Cli, ArgMatches
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
WalkthroughIntroduce a dedicated OrthoConfig-derived Changes
Sequence DiagramsequenceDiagram
participant U as User/CLI
participant P as parser.rs (Clap)
participant M as merge.rs (Composer)
participant CD as ConfigDiscovery (Files)
participant EP as Env Provider (ENV)
participant V as Validation (Post-Merge)
participant C as CliConfig (Schema)
U->>P: supply CLI args
P->>P: parse with localizer
P->>M: send (Cli, ArgMatches)
M->>M: init CliConfig::default()
M->>CD: discover config files
CD-->>M: return file layers
M->>EP: collect NETSUKE_* env vars
EP-->>M: return env layer
M->>M: build CLI overrides from ArgMatches
M->>M: compose layers (defaults, files, env, CLI)
M->>C: merge_from_layers()
C-->>M: merged CliConfig
M->>V: run post-merge validations
V-->>M: validation result
M->>M: apply_config onto parsed Cli
M-->>U: finalised Cli
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@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 fn cli_config_build_defaults_apply_when_cli_targets_are_absent() -> Result<()> {
let _env_lock = EnvLock::acquire();
let temp_dir = tempdir().context("create temporary config directory")?;
let config_path = temp_dir.path().join("netsuke.toml");
fs::write(
&config_path,
r#"
[cmds.build]
targets = ["all", "docs"]
"#,
)
.context("write netsuke.toml")?;
let _config_guard = EnvVarGuard::set("NETSUKE_CONFIG_PATH", config_path.as_os_str());
let localizer = Arc::from(cli_localization::build_localizer(None));
let (cli, matches) = netsuke::cli::parse_with_localizer_from(["netsuke"], &localizer)
.context("parse CLI args for merge")?;
let merged = netsuke::cli::merge_with_config(&cli, &matches)
.context("merge CLI and configuration layers")?;
let Some(netsuke::cli::Commands::Build(args)) = merged.command else {
anyhow::bail!("expected merged command to be build");
};
ensure!(
args.targets == vec![String::from("all"), String::from("docs")],
"configured build targets should be used when CLI targets are absent",
);
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@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 fn cli_config_build_defaults_apply_when_cli_targets_are_absent() -> Result<()> {
with_config_file(
r#"
[cmds.build]
targets = ["all", "docs"]
"#,
&["netsuke"],
|merged| {
let Some(netsuke::cli::Commands::Build(args)) = merged.command else {
anyhow::bail!("expected merged command to be build");
};
ensure!(
args.targets == vec![String::from("all"), String::from("docs")],
"configured build targets should be used when CLI targets are absent",
);
Ok(())
},
)
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@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 fn cli_config_validates_theme_alias_conflicts(
default_cli_json: Result<serde_json::Value>,
) -> Result<()> {
assert_merge_rejects(
default_cli_json?,
json!({
"theme": "unicode",
"no_emoji": true
}),
"conflicting theme and alias should fail",
"theme = \"unicode\" conflicts with no_emoji = true",
)
}❌ New issue: Code Duplication |
Owner
Author
|
@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 use anyhow::{Context, Result, ensure};
use netsuke::cli::Cli;
use netsuke::cli::{CliConfig, Theme};❌ New issue: String Heavy Function Arguments |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9733
Root causeAll three of these functions follow the exact same pattern: The Fix1. Drop
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@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 use anyhow::{Context, Result, ensure};
use netsuke::cli::Cli;
use netsuke::cli::{CliConfig, Theme};❌ New issue: String Heavy Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The
validation_errorhelper is implemented in bothsrc/cli/config.rsandsrc/cli/merge.rs; consider centralizing this in a shared module/function to avoid drift in error shaping and messaging. - Building the CLI override JSON in
cli_overrides_from_matchesandbuild_cli_overridesis now field-by-field and will require updates in multiple spots when new fields are added; it may be worth factoring this into a more data-driven mapping or a helper to reduce the risk of missing fields during future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `validation_error` helper is implemented in both `src/cli/config.rs` and `src/cli/merge.rs`; consider centralizing this in a shared module/function to avoid drift in error shaping and messaging.
- Building the CLI override JSON in `cli_overrides_from_matches` and `build_cli_overrides` is now field-by-field and will require updates in multiple spots when new fields are added; it may be worth factoring this into a more data-driven mapping or a helper to reduce the risk of missing fields during future changes.
## Individual Comments
### Comment 1
<location path="src/cli/parser.rs" line_range="180" />
<code_context>
+impl Default for Cli {
+ fn default() -> Self {
+ Self {
+ file: PathBuf::from("Netsukefile"),
+ directory: None,
+ jobs: None,
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating the default manifest path literal in multiple places.
`CliConfig` already centralizes this value via `default_manifest_path()`, but this `Default` impl hard-codes "Netsukefile" again. Please pull the default from a shared constant or helper so parser and config defaults can’t drift if the path changes in the future.
Suggested implementation:
```rust
Self {
file: CliConfig::default_manifest_path(),
directory: None,
```
To compile, ensure `CliConfig` is in scope in this file. At the top of `src/cli/parser.rs`, add or adjust an import such as:
- `use crate::config::CliConfig;`
If `default_manifest_path()` is an associated function with a different path or name, update the call accordingly (e.g. `config::CliConfig::default_manifest_path()` or whatever matches your existing `CliConfig` definition).
</issue_to_address>
### Comment 2
<location path="tests/cli_tests/merge.rs" line_range="108" />
<code_context>
+ Ok(())
}
#[rstest]
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for the complementary validation error cases in CliConfig
The `CliConfig` post-merge hook enforces four conflict combinations:
- `theme = "unicode"` with `no_emoji = true`
- `theme = "ascii"` with `no_emoji = false`
- `spinner_mode = "disabled"` with `progress = true`
- `spinner_mode = "enabled"` with `progress = false`
This rstest only covers the first and third cases plus the unsupported `output_format` case. Please add `#[case]` rows for:
- `json!({ "theme": "ascii", "no_emoji": false })` -> `ExpectedValidationError::ThemeAliasConflict`
- `json!({ "spinner_mode": "enabled", "progress": false })` -> `ExpectedValidationError::SpinnerProgressConflict`
so that all branches in `validate_theme_compatibility` and `validate_spinner_mode_compatibility` are exercised.
Suggested implementation:
```rust
#[rstest]
#[case(json!({ "theme": "unicode", "no_emoji": true }), ExpectedValidationError::ThemeAliasConflict)]
#[case(json!({ "theme": "ascii", "no_emoji": false }), ExpectedValidationError::ThemeAliasConflict)]
#[case(json!({ "spinner_mode": "disabled", "progress": true }), ExpectedValidationError::SpinnerProgressConflict)]
#[case(json!({ "spinner_mode": "enabled", "progress": false }), ExpectedValidationError::SpinnerProgressConflict)]
#[case(json!({ "output_format": "foo" }), ExpectedValidationError::UnsupportedOutputFormat)]
```
I inferred the existing `#[case]` lines and their formatting. If the actual test uses a different layout (e.g. multi-line `#[case(...)]` or different argument order), mirror that style when adding:
1. A case for `json!({ "theme": "ascii", "no_emoji": false })` with `ExpectedValidationError::ThemeAliasConflict`.
2. A case for `json!({ "spinner_mode": "enabled", "progress": false })` with `ExpectedValidationError::SpinnerProgressConflict`.
Ensure these are added to the same rstest that currently covers the unicode/no_emoji, disabled/progress, and unsupported `output_format` cases.
</issue_to_address>
### Comment 3
<location path="src/cli/merge.rs" line_range="208" />
<code_context>
+ Ok(())
+}
+
+fn validation_error(key: &str, message: &str) -> Arc<OrthoError> {
+ Arc::new(OrthoError::Validation {
+ key: key.to_owned(),
</code_context>
<issue_to_address>
**issue (review_instructions):** Deduplicate the validation_error helper to avoid repeating identical logic defined in multiple modules.
Extract validation_error into a shared helper (for example in a small internal module or a common cli/utils module) and reuse it from both config.rs and merge.rs instead of redefining it twice. This keeps error construction consistent and satisfies the DRY requirement without harming readability.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
Keep code DRY, but readable. Use refactoring approaches best suited for the language in question.
</details>
</issue_to_address>
### Comment 4
<location path="docs/execplans/3-11-1-cli-config-struct.md" line_range="266" />
<code_context>
+- Keeping the parser/runtime type stable while introducing a separate merge
+ schema is a pragmatic migration path when downstream code already consumes
+ the parser type pervasively.
+- BDD coverage that touches process-wide environment variables must hold
+ `EnvLock` for the full scenario, not only for individual mutations.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym “BDD” is used here without having been defined earlier in the document.
Please expand this at first use to something like “behaviour-driven development (BDD) coverage …”, and then use “BDD” on its own afterwards.
<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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf2ccff65d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/3-11-1-cli-config-struct.md`:
- Around line 17-22: Update the documentation to remove hard-coded absolute
paths and commands: replace occurrences of "/home/user/project/..." and
"/root/.bun/bin" (and any other machine-local paths) with repo-relative links
and repository-root command examples (e.g., use ./docs/..., <repo-root>/path or
repository-relative commands like ./scripts/... or yarn/npm run from the repo
root); adjust the examples in this section and the same pattern in lines 491-537
so all links and shell commands work when the repo is checked out in any
location and do not assume a specific user/home directory.
- Around line 271-288: Update the docs/execplans/3-11-1-cli-config-struct.md
section so it is either marked explicitly as a "pre-change baseline" or
rewritten to reflect the final shipped state: replace present-tense phrases like
"The current implementation" and the literal mention of Cargo.toml pinning
`ortho_config = "0.7.0"` with historical/contextual wording or the actual
shipped values (e.g., `ortho_config = "0.8.0"` and the current toolchain), and
ensure references to `src/cli/mod.rs`, `src/main.rs`, `src/output_mode.rs`,
`src/output_prefs.rs`, `src/runner/mod.rs`, and `tests/cli_tests/merge.rs` are
accurate to the final layout.
In `@docs/netsuke-design.md`:
- Around line 2013-2037: Replace the US spelling "canonicalizes" with the
en-GB-oxendict spelling "canonicalises" in the documentation sentence that
discusses the `theme` field and the compatibility alias `no_emoji` (the
occurrence of the word "canonicalizes" in that paragraph should be changed to
"canonicalises").
In `@docs/ortho-config-users-guide.md`:
- Around line 1197-1234: In the "Generating PowerShell help" section update the
sentence that reads "alongside a wrapper module so `Get-Help {BinName} -Full`
surfaces the same configuration metadata as the man page generator" by inserting
a comma after "wrapper module" (i.e., "alongside a wrapper module, so `Get-Help
{BinName} -Full`...") to improve readability; locate this text near the
cargo-orthohelp PowerShell example and adjust the copy accordingly.
In `@docs/users-guide.md`:
- Around line 560-574: The users-guide top-level schema list omits the
fetch-policy fields from CliConfig; update the schema paragraph under CliConfig
to include the four fetch-related keys: fetch_allow_scheme, fetch_allow_host,
fetch_block_host, and fetch_default_deny, showing their expected types/values
(e.g. arrays for allow/block and boolean for default deny) so the docs match the
shipped CliConfig schema; ensure the new entries appear in the same style as the
other keys and keep ordering consistent around locale and accessible.
In `@src/cli/merge.rs`:
- Around line 208-213: The helper function validation_error (returning
Arc<OrthoError::Validation>) is duplicated between the CLI modules; remove the
duplicate in src/cli/merge.rs and centralize it by extracting it to a shared CLI
error helper (e.g., a new cli::error module) or by exporting the existing
validation_error from src/cli/config.rs and importing it in merge.rs; update
references in merge.rs to call the single shared validation_error and ensure the
function signature (validation_error(key: &str, message: &str) ->
Arc<OrthoError>) and the OrthoError variant names remain unchanged.
In `@src/cli/parser.rs`:
- Around line 156-164: The no_emoji_override function currently returns
self.no_emoji verbatim so --no-emoji false yields Some(false) and acts as a hard
override; change no_emoji_override (pub const fn no_emoji_override(&self) ->
Option<bool>) so that only Some(true) or Theme::Ascii produce Some(true) and any
Some(false) is treated as None (i.e., defer to env/auto-detection); additionally
add a regression test (e.g., test_no_emoji_false_defers_to_env) that sets
self.no_emoji = Some(false) and verifies the function defers to
environment-based suppression rather than returning Some(false).
In `@tests/features/configuration_preferences.feature`:
- Around line 10-16: The scenario "CLI locale and verbose flags override
configuration and environment" currently only sets a config file and CLI flags,
so either add a step that explicitly sets an environment value (e.g., Given the
environment variable NETSUKE_LOCALE is "es-ES" or similar) before parsing to
exercise env precedence, or rename the scenario to remove "and environment"
(e.g., "CLI locale and verbose flags override configuration") to accurately
reflect what it tests; update the scenario title and/or add the Given
environment step and use the existing CLI parsing/merging steps (the scenario
name is the unique identifier to change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51194ab1-c2b6-4616-8579-de5d0567360d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlbuild.rsdocs/execplans/3-11-1-cli-config-struct.mddocs/netsuke-design.mddocs/ortho-config-users-guide.mddocs/roadmap.mddocs/users-guide.mdsrc/cli/config.rssrc/cli/merge.rssrc/cli/mod.rssrc/cli/parser.rssrc/cli/parsing.rssrc/main.rssrc/runner/mod.rstests/bdd/fixtures/mod.rstests/bdd/steps/configuration_preferences.rstests/bdd/steps/locale_resolution.rstests/bdd/steps/mod.rstests/cli_tests/merge.rstests/features/configuration_preferences.feature
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/users-guide.md`:
- Around line 612-614: Edit the sentence containing `output_format = "json"` and
roadmap item `3.10.3` to insert a comma after the introductory phrase "until
then" so it reads "...until then, the only supported value is `"human"`"; ensure
the change follows en-GB-oxendict spelling and punctuation conventions.
In `@src/cli/merge.rs`:
- Around line 82-116: The CLI override builder cli_overrides_from_matches is
missing explicit display-format fields so flags like colour_policy,
spinner_mode, output_format, and theme are never injected into the overrides
map; add maybe_insert_explicit(...) calls for each missing field (using the same
pattern and &mut root) referencing the corresponding Cli struct members
(cli.colour_policy, cli.spinner_mode, cli.output_format, cli.theme) so these
values are present in the overrides used later when merging configs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50379dd5-d37a-419c-852c-6a0ee3aff9fc
📒 Files selected for processing (11)
docs/execplans/3-11-1-cli-config-struct.mddocs/ortho-config-users-guide.mddocs/users-guide.mdsrc/cli/config.rssrc/cli/merge.rssrc/cli/mod.rssrc/cli/parser.rstests/bdd/steps/configuration_preferences.rstests/cli_tests/merge.rstests/cli_tests/parsing.rstests/features/configuration_preferences.feature
Comment on lines
+82
to
+116
| fn cli_overrides_from_matches(cli: &Cli, matches: &ArgMatches) -> OrthoResult<Value> { | ||
| let mut root = Map::new(); | ||
|
|
||
| maybe_insert_explicit(matches, "file", &cli.file, &mut root)?; | ||
| maybe_insert_explicit(matches, "jobs", &cli.jobs, &mut root)?; | ||
| maybe_insert_explicit(matches, "verbose", &cli.verbose, &mut root)?; | ||
| maybe_insert_explicit(matches, "locale", &cli.locale, &mut root)?; | ||
| maybe_insert_explicit( | ||
| matches, | ||
| "fetch_allow_scheme", | ||
| &cli.fetch_allow_scheme, | ||
| &mut root, | ||
| )?; | ||
| maybe_insert_explicit( | ||
| matches, | ||
| "fetch_allow_host", | ||
| &cli.fetch_allow_host, | ||
| &mut root, | ||
| )?; | ||
| maybe_insert_explicit( | ||
| matches, | ||
| "fetch_block_host", | ||
| &cli.fetch_block_host, | ||
| &mut root, | ||
| )?; | ||
| maybe_insert_explicit( | ||
| matches, | ||
| "fetch_default_deny", | ||
| &cli.fetch_default_deny, | ||
| &mut root, | ||
| )?; | ||
| maybe_insert_explicit(matches, "accessible", &cli.accessible, &mut root)?; | ||
| maybe_insert_explicit(matches, "progress", &cli.progress, &mut root)?; | ||
| maybe_insert_explicit(matches, "no_emoji", &cli.no_emoji, &mut root)?; | ||
|
|
Contributor
There was a problem hiding this comment.
Insert all explicit display-format CLI fields into overrides.
Fix this now. Lines 82-116 omit colour_policy, spinner_mode,
output_format, and theme. Line 172-Line 176 then read those values only
from merged config, so explicit CLI flags are lost or overridden by lower
precedence layers.
🐛 Patch
maybe_insert_explicit(matches, "accessible", &cli.accessible, &mut root)?;
maybe_insert_explicit(matches, "progress", &cli.progress, &mut root)?;
maybe_insert_explicit(matches, "no_emoji", &cli.no_emoji, &mut root)?;
+ maybe_insert_explicit(matches, "colour_policy", &cli.colour_policy, &mut root)?;
+ maybe_insert_explicit(matches, "spinner_mode", &cli.spinner_mode, &mut root)?;
+ maybe_insert_explicit(matches, "output_format", &cli.output_format, &mut root)?;
+ maybe_insert_explicit(matches, "theme", &cli.theme, &mut root)?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/merge.rs` around lines 82 - 116, The CLI override builder
cli_overrides_from_matches is missing explicit display-format fields so flags
like colour_policy, spinner_mode, output_format, and theme are never injected
into the overrides map; add maybe_insert_explicit(...) calls for each missing
field (using the same pattern and &mut root) referencing the corresponding Cli
struct members (cli.colour_policy, cli.spinner_mode, cli.output_format,
cli.theme) so these values are present in the overrides used later when merging
configs.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat src/cli/merge.rs
Length of output: 6713
@leynos, the issue has not been resolved in the latest commit. The cli_overrides_from_matches function still terminates at no_emoji without inserting colour_policy, spinner_mode, output_format, or theme:
maybe_insert_explicit(matches, "accessible", &cli.accessible, &mut root)?;
maybe_insert_explicit(matches, "progress", &cli.progress, &mut root)?;
maybe_insert_explicit(matches, "no_emoji", &cli.no_emoji, &mut root)?;
// colour_policy, spinner_mode, output_format, theme still missingapply_config confirms Cli holds all four fields (they are assigned there from CliConfig), so explicit CLI flags for those options will currently be lost, overridden by file or environment layers.
Here is the AI agent prompt to address this:
In `src/cli/merge.rs`, inside `cli_overrides_from_matches`, add four
`maybe_insert_explicit` calls immediately after the existing `no_emoji` call
and before the `if let Some(Commands::Build(args))` block:
```rust
maybe_insert_explicit(matches, "colour_policy", &cli.colour_policy, &mut root)?;
maybe_insert_explicit(matches, "spinner_mode", &cli.spinner_mode, &mut root)?;
maybe_insert_explicit(matches, "output_format", &cli.output_format, &mut root)?;
maybe_insert_explicit(matches, "theme", &cli.theme, &mut root)?;
```
Constraints:
- Do not change any other function, test, or file.
- Follow the same pattern as the existing `maybe_insert_explicit` calls: pass
`matches`, the kebab-case field name matching the Clap argument ID, a shared
reference to the corresponding `cli` field, and `&mut root`.
- Verify that `Cli` exposes `colour_policy`, `spinner_mode`, `output_format`,
and `theme` fields (confirmed visible in `apply_config`).
Postconditions:
- `src/cli/merge.rs` compiles without errors or new warnings.
- An explicit `--colour-policy`, `--spinner-mode`, `--output-format`, or
`--theme` flag supplied on the command line is no longer silently discarded
when file or environment layers are also present.
🧠 Learnings used
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.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: In Rust projects, when a type is #[must_use] and you would otherwise discard its value, prefer using drop(...) instead of lines like 'let _ = ...' to satisfy must_use checks. This aligns with an enforcement in Cargo.toml: let_underscore_must_use = "deny". Apply this guideline across Rust source files (e.g., in src/status.rs and other Rust files) to ensure lint compliance and avoid accidental ignored results.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: Enforce the deny(lint) setting for must_use results in Cargo.toml. When discarding must_use values (e.g., Result) after operations like writeln!(io::stderr(), ...), prefer the drop(...) idiom over using let _ = to satisfy the lint. Apply this pattern across all Rust source files (src/ and beyond) to ensure consistent lint compliance.
Owner
Author
There was a problem hiding this comment.
@coderabbitai the engineer writes:
In
src/cli/merge.rs, insidecli_overrides_from_matches, add fourmaybe_insert_explicitcalls immediately after the existingno_emojicall and before theif let Some(Commands::Build(args))block:maybe_insert_explicit(matches, "colour_policy", &cli.colour_policy, &mut root)?; maybe_insert_explicit(matches, "spinner_mode", &cli.spinner_mode, &mut root)?; maybe_insert_explicit(matches, "output_format", &cli.output_format, &mut root)?; maybe_insert_explicit(matches, "theme", &cli.theme, &mut root)?;Constraints:
- Do not change any other function, test, or file.
- Follow the same pattern as the existing
maybe_insert_explicitcalls: passmatches, the kebab-case field name matching the Clap argument ID, a shared reference to the correspondingclifield, and&mut root.- Verify that
Cliexposescolour_policy,spinner_mode,output_format, andthemefields (confirmed visible inapply_config).Postconditions:
src/cli/merge.rscompiles without errors or new warnings.- An explicit
--colour-policy,--spinner-mode,--output-format, or--themeflag supplied on the command line is no longer silently discarded when file or environment layers are also present.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Task: Add missing maybe_insert_explicit calls for display-format fields
- Verify current state of
cli_overrides_from_matchesinsrc/cli/merge.rs - Verify
Clistruct exposescolour_policy,spinner_mode,output_format, andthemefields - Insert the four missing calls after
no_emojiand before theCommands::Buildblock
🏁 Script executed:
#!/bin/bash
# Confirm current state of cli_overrides_from_matches
rg -n "maybe_insert_explicit|Commands::Build|no_emoji" src/cli/merge.rs
# Confirm Cli struct fields for the four display-format options
rg -n "colour_policy|spinner_mode|output_format|theme" src/cli/parser.rs
Length of output: 1728
@leynos, the field names are confirmed on Cli in src/cli/parser.rs and the insertion point is clear (after Line 115, before the if let Some(Commands::Build(args)) block at Line 117). Apply this diff to src/cli/merge.rs:
maybe_insert_explicit(matches, "accessible", &cli.accessible, &mut root)?;
maybe_insert_explicit(matches, "progress", &cli.progress, &mut root)?;
maybe_insert_explicit(matches, "no_emoji", &cli.no_emoji, &mut root)?;
+ maybe_insert_explicit(matches, "colour_policy", &cli.colour_policy, &mut root)?;
+ maybe_insert_explicit(matches, "spinner_mode", &cli.spinner_mode, &mut root)?;
+ maybe_insert_explicit(matches, "output_format", &cli.output_format, &mut root)?;
+ maybe_insert_explicit(matches, "theme", &cli.theme, &mut root)?;
if let Some(Commands::Build(args)) = cli.command.as_ref()No other files need to change. Once applied:
- Explicit
--colour-policy,--spinner-mode,--output-format, and--themeflags will be injected into the overrides map at the highest-precedence layer, so they can no longer be silently overridden by file or environment layers. - The existing
canonical_themelogic inapply_configis unaffected — it still runs over the final mergedCliConfigafter all layers are resolved.
🧠 Learnings used
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.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: In Rust projects, when a type is #[must_use] and you would otherwise discard its value, prefer using drop(...) instead of lines like 'let _ = ...' to satisfy must_use checks. This aligns with an enforcement in Cargo.toml: let_underscore_must_use = "deny". Apply this guideline across Rust source files (e.g., in src/status.rs and other Rust files) to ensure lint compliance and avoid accidental ignored results.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: Enforce the deny(lint) setting for must_use results in Cargo.toml. When discarding must_use values (e.g., Result) after operations like writeln!(io::stderr(), ...), prefer the drop(...) idiom over using let _ = to satisfy the lint. Apply this pattern across all Rust source files (src/ and beyond) to ensure consistent lint compliance.
Failed to handle agent chat message. Please try again.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "maybe_insert_explicit|Commands::Build|colour_policy|spinner_mode|output_format|theme" src/cli/merge.rs
Length of output: 1449
@leynos, the fix has not been applied in the latest commit. The cli_overrides_from_matches function still terminates at diag_json (Line 157) without inserting colour_policy, spinner_mode, output_format, or theme:
154: maybe_insert_explicit(matches, "accessible", &cli.accessible, &mut root)?;
155: maybe_insert_explicit(matches, "progress", &cli.progress, &mut root)?;
156: maybe_insert_explicit(matches, "no_emoji", &cli.no_emoji, &mut root)?;
157: maybe_insert_explicit(matches, "diag_json", &cli.diag_json, &mut root)?;
159: if let Some(Commands::Build(args)) = cli.command.as_ref() // ← all four still missing
Here is the updated AI agent prompt:
In `src/cli/merge.rs`, inside `cli_overrides_from_matches`, add four
`maybe_insert_explicit` calls immediately after the existing `diag_json` call
(currently Line 157) and before the `if let Some(Commands::Build(args))` block
(currently Line 159):
```rust
maybe_insert_explicit(matches, "colour_policy", &cli.colour_policy, &mut root)?;
maybe_insert_explicit(matches, "spinner_mode", &cli.spinner_mode, &mut root)?;
maybe_insert_explicit(matches, "output_format", &cli.output_format, &mut root)?;
maybe_insert_explicit(matches, "theme", &cli.theme, &mut root)?;
```
Constraints:
- Modify only `src/cli/merge.rs`; do not change any other file.
- The four new calls must appear after `maybe_insert_explicit(matches, "diag_json", ...)`
and before `if let Some(Commands::Build(args))`.
- Use the underscore-separated field names matching the Clap argument IDs as
already used for the other fields: `"colour_policy"`, `"spinner_mode"`,
`"output_format"`, `"theme"`.
- Follow the same single-line pattern as the `accessible`, `progress`,
`no_emoji`, and `diag_json` calls.
Postconditions:
- `src/cli/merge.rs` compiles without errors or new warnings.
- Explicit `--colour-policy`, `--spinner-mode`, `--output-format`, and `--theme`
flags supplied on the command line are no longer silently discarded when file
or environment layers are also present.
🧠 Learnings used
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.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: In Rust projects, when a type is #[must_use] and you would otherwise discard its value, prefer using drop(...) instead of lines like 'let _ = ...' to satisfy must_use checks. This aligns with an enforcement in Cargo.toml: let_underscore_must_use = "deny". Apply this guideline across Rust source files (e.g., in src/status.rs and other Rust files) to ensure lint compliance and avoid accidental ignored results.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: Enforce the deny(lint) setting for must_use results in Cargo.toml. When discarding must_use values (e.g., Result) after operations like writeln!(io::stderr(), ...), prefer the drop(...) idiom over using let _ = to satisfy the lint. Apply this pattern across all Rust source files (src/ and beyond) to ensure consistent lint compliance.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "maybe_insert_explicit|Commands::Build|colour_policy|spinner_mode|output_format|theme" src/cli/merge.rs
Length of output: 1449
@leynos, the fix has not been applied in the latest commit. After Line 157 (diag_json), the four fields are still absent before the Commands::Build block at Line 159:
156: maybe_insert_explicit(matches, "no_emoji", &cli.no_emoji, &mut root)?;
157: maybe_insert_explicit(matches, "diag_json", &cli.diag_json, &mut root)?;
← colour_policy, spinner_mode, output_format, theme still missing
159: if let Some(Commands::Build(args)) = cli.command.as_ref()
Here is the AI agent prompt:
In `src/cli/merge.rs`, inside `cli_overrides_from_matches`, add four
`maybe_insert_explicit` calls immediately after the existing `diag_json` call
(Line 157) and before the `if let Some(Commands::Build(args))` block (Line 159):
```rust
maybe_insert_explicit(matches, "colour_policy", &cli.colour_policy, &mut root)?;
maybe_insert_explicit(matches, "spinner_mode", &cli.spinner_mode, &mut root)?;
maybe_insert_explicit(matches, "output_format", &cli.output_format, &mut root)?;
maybe_insert_explicit(matches, "theme", &cli.theme, &mut root)?;
```
Constraints:
- Modify only `src/cli/merge.rs`; do not change any other file.
- The four new calls must appear after `maybe_insert_explicit(matches, "diag_json", ...)`
and before `if let Some(Commands::Build(args))`.
- Use the underscore-separated field names as already used for the other fields:
`"colour_policy"`, `"spinner_mode"`, `"output_format"`, `"theme"`.
- Follow the same single-line pattern as the `accessible`, `progress`,
`no_emoji`, and `diag_json` calls immediately above.
- `cli.colour_policy`, `cli.spinner_mode`, `cli.output_format`, and `cli.theme`
are confirmed present on `Cli` (visible in `apply_config` at Lines 215–218).
Postconditions:
- `src/cli/merge.rs` compiles without errors or new warnings.
- Explicit `--colour-policy`, `--spinner-mode`, `--output-format`, and `--theme`
flags supplied on the command line are no longer silently discarded when file
or environment layers are also present.
🧠 Learnings used
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.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: In Rust projects, when a type is #[must_use] and you would otherwise discard its value, prefer using drop(...) instead of lines like 'let _ = ...' to satisfy must_use checks. This aligns with an enforcement in Cargo.toml: let_underscore_must_use = "deny". Apply this guideline across Rust source files (e.g., in src/status.rs and other Rust files) to ensure lint compliance and avoid accidental ignored results.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: Enforce the deny(lint) setting for must_use results in Cargo.toml. When discarding must_use values (e.g., Result) after operations like writeln!(io::stderr(), ...), prefer the drop(...) idiom over using let _ = to satisfy the lint. Apply this pattern across all Rust source files (src/ and beyond) to ensure consistent lint compliance.
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1287
No requirements were annulled — none of the four violate en-GB-oxendict conventions, and none introduce unnecessary cyclomatic complexity. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Add a comprehensive execution plan document outlining the introduction of the `CliConfig` struct as the layered CLI configuration schema. This living document covers the purpose, constraints, tolerances, risks, progress stages, design decisions, and future steps for implementing a typed `CliConfig` configuration with OrthoConfig integration, separating CLI parsing from configuration merging, and improving schema explicitness for Netsuke project roadmap item 3.11.1. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…ance - Updated docs/execplans/3-11-1-cli-config-struct.md to align with ortho_config 0.8.0 semantics and migration rules. - Replaced ortho-config-users-guide.md with upstream 0.8.0 version, adding detailed guidance on derive macro usage, dependency architecture, new attributes, typed Clap defaults with `cli_default_as_absent`, and documentation metadata generation. - Clarified dependency and macro usage with ortho_config crate aliasing support. - Introduced best practices for maintaining compatibility and upgrading from 0.7.x to 0.8.0. - Enhanced examples for default handling and integration with cargo-orthohelp documentation tooling. - Focused on improving reliability and usability of config schema derivation and CLI integration. These documentation enhancements provide a comprehensive reference for implementing the roadmap item 3.11.1 and safely upgrading ortho_config dependency while preserving existing CLI and config behavior. Refs #3.11.1 roadmap, ortho_config 0.8.0 migration Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
- Added `CliConfig` as the authoritative OrthoConfig-derived schema for CLI configuration. - Split CLI module into parser, config, and merge submodules to separate parsing from merging. - Implement layered merging of defaults, configuration files, environment variables, and CLI overrides. - Added typed config fields for verbosity, locale, colour_policy, spinner_mode, output_format, theme, and build defaults. - Added validation for incompatible theme and spinner/progress settings. - Introduced `no_emoji` compatibility alias resolving to ASCII theme. - Provided default build command targets from config when CLI targets are absent. - Updated CLI parsing to preserve older `Cli` type for syntax and localized parsing. - Added extensive unit and BDD tests to cover merging behaviour and validation. - Updated documentation and roadmap for configuration preferences milestone 3.11.1. This refactor improves configuration schema clarity and layering, enabling unified merging from multiple sources while preserving CLI parsing ergonomics. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Refactor CLI merge layer tests by introducing utility functions `with_config_file` and `merge_defaults_with_file_layer` to handle configuration file setup and merging. Replace repetitive tempdir and environment variable management with these helpers to improve test clarity and reduce boilerplate. Updated various tests to use these helpers, keeping test behavior unchanged but improving maintainability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
- Introduced assert_build_targets helper to simplify build target checks. - Introduced assert_merge_rejects helper to validate expected merge errors. - Replaced repetitive inline assertions with helper functions in merge.rs tests. - Improved test readability and reduced duplication in cli merge tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Refactor CLI config validation tests to use parameterized test cases with rstest's #[case] attributes. This reduces test code duplication and improves clarity when testing conflicting or unsupported CLI config settings. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
- Introduce ExpectedValidationError enum to represent specific merge error cases. - Refactor assert_merge_rejects to use ExpectedValidationError instead of raw strings. - Update test cases to use the enum, improving clarity and maintainability. - Enhance error assertion messages for better diagnostics in build target tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
- Added `CliConfig` in `src/cli/config.rs` to encapsulate global CLI settings and subcommand defaults with OrthoConfig derive. - Split `src/cli/mod.rs` responsibilities into parser, config, and merge modules. - Upgraded ortho_config to v0.8.0 and migrated configuration layering accordingly. - Updated merge and parsing logic to use `CliConfig` as authoritative merge target. - Extended unit tests (`tests/cli_tests/merge.rs`), behavioural tests (`tests/features/configuration_preferences.feature`), and BDD steps. - Updated documentation (`docs/users-guide.md`, `docs/netsuke-design.md`, `docs/roadmap.md`) to reflect new config schema, precedence rules, and CLI defaults. - Improved handling for output preferences, locale overrides, and theme compatibility validation. This change modularizes CLI configuration management, clarifies schema separation, and sets foundation for safer future extensions while preserving legacy behavior. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Apply formatter-required whitespace and import ordering changes\nafter rebasing the CLI config work onto origin/main.\n\nThis keeps the rebased branch passing the repository quality\ngates without introducing unrelated documentation churn.
devboxerhub
bot
force-pushed
the
add-cli-config-struct-vcf3xy
branch
from
c7a0211 to
82a1f69
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/netsuke-design.md`:
- Around line 2032-2036: Update the paragraph to point readers at the parser
module rather than the old location and clarify the re-export: mention the
parser-facing Cli type (symbol: Cli) is defined in the parser module and that
layered configuration now lives in CliConfig (derived with OrthoConfig), while
the top-level module re-exports the public CLI surface; replace the sentence
that references the old location with this clarified wording so docs match the
code split between the parser, CliConfig, and the module that re-exports the
public CLI surface.
In `@docs/users-guide.md`:
- Around line 616-618: In the sentence containing `colour_policy` and "Netsuke"
(the line that reads "`colour_policy` is accepted and layered today so users can
standardize..."), insert a comma after the word "today" so the clause reads
"...layered today, so users can standardize..." to correctly join the
independent clauses.
In `@src/cli/parser.rs`:
- Around line 304-306: Insert a single blank line between the preceding function
closing brace and the module-level constant declaration so the constant
pub(super) const MAX_JOBS: usize = 64; is separated from the function above;
locate the closing '}' just before MAX_JOBS in parser.rs and add one empty line
before the constant to improve readability.
- Around line 68-69: Replace the hardcoded default_value on the `file` field in
parser.rs with the centralized default from `CliConfig` to avoid drift: use
`default_value_t` (or `default_value_os_t` if
`CliConfig::default_manifest_path()` cannot be const) pointing to
`CliConfig::default_manifest_path()` (or a new `const DEFAULT_MANIFEST_PATH`) so
the `pub file: PathBuf` argument's default is sourced from the single canonical
method/constant; if necessary make `CliConfig::default_manifest_path()` a `const
fn` or introduce a `const DEFAULT_MANIFEST_PATH` and reference that in the
attribute.
In `@tests/bdd/steps/configuration_preferences.rs`:
- Around line 40-43: Replace the unsafe direct call to std::env::set_var with
the shared test helper: import test_support::env::set_var and call that helper
instead of unsafe { std::env::set_var(CONFIG_ENV_VAR,
OsStr::new(path.as_os_str())) }; keep the surrounding logic (capturing previous
via std::env::var_os(CONFIG_ENV_VAR), the SAFETY note about EnvLock, and
world.track_env_var(CONFIG_ENV_VAR.to_owned(), previous)) unchanged so the
EnvLock and world tracking continue to protect the environment mutation.
In `@tests/cli_tests/merge.rs`:
- Around line 102-113: The helper function assert_merge_rejects should stop
using expect_err (which is banned outside tests); update the call to
merge_defaults_with_file_layer to a match: on Ok(value) call anyhow::bail! with
a message like "merge should have failed, got {value:#?}", and on Err(e) bind e
to err for the subsequent assertions; reference assert_merge_rejects and
merge_defaults_with_file_layer when changing the code and preserve the existing
ensure! check that verifies err.to_string() contains
expected_error.expected_fragment().
In `@tests/features/configuration_preferences.feature`:
- Around line 26-34: Move the Gherkin step "Then parsing succeeds" so it appears
immediately after the step "And the success prefix is rendered" (i.e., place the
primary Then assertion before its trailing And-steps), ensuring the scenario
reads: Given..., And the Netsuke config..., When..., And merged output
preferences are resolved, And the success prefix is rendered, Then parsing
succeeds, And the merged theme is ascii, And the prefix contains no non-ASCII
characters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b2a7bc7-be70-4ff8-9d22-d0abb6304a56
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlbuild.rsdocs/execplans/3-11-1-cli-config-struct.mddocs/netsuke-design.mddocs/ortho-config-users-guide.mddocs/roadmap.mddocs/users-guide.mdsrc/cli/config.rssrc/cli/merge.rssrc/cli/mod.rssrc/cli/parser.rssrc/cli/parsing.rssrc/main.rssrc/runner/mod.rstests/bdd/fixtures/mod.rstests/bdd/steps/configuration_preferences.rstests/bdd/steps/locale_resolution.rstests/bdd/steps/mod.rstests/cli_tests/merge.rstests/cli_tests/parsing.rstests/features/configuration_preferences.feature
Comment on lines
+40
to
+43
| let previous = std::env::var_os(CONFIG_ENV_VAR); | ||
| // SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario. | ||
| unsafe { std::env::set_var(CONFIG_ENV_VAR, OsStr::new(path.as_os_str())) }; | ||
| world.track_env_var(CONFIG_ENV_VAR.to_owned(), previous); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the file content at the specified lines
echo "=== Content at lines 40-43 and 81-84 in configuration_preferences.rs ==="
if [ -f "tests/bdd/steps/configuration_preferences.rs" ]; then
echo "Lines 35-50:"
sed -n '35,50p' tests/bdd/steps/configuration_preferences.rs
echo
echo "Lines 76-90:"
sed -n '76,90p' tests/bdd/steps/configuration_preferences.rs
else
echo "File not found at expected path"
git ls-files | grep -i configuration_preferences || true
fi
echo
echo "=== Search for direct std::env::(set_var|remove_var) calls in test code ==="
rg -n --type rust 'std::env::(set_var|remove_var)' tests/ --max-count 20 || true
echo
echo "=== Search for existing env helpers (EnvLock, EnvVarGuard, etc.) ==="
rg -n --type rust '\b(EnvLock|EnvVarGuard|track_env_var|set_var|remove_var)\b' tests/ --max-count 30 || true
echo
echo "=== Check for test_utils, test_helpers, or shared test support crates ==="
find tests -type d -name '*test*' -o -name '*helper*' -o -name '*util*' | head -20
echo
echo "=== Check repository structure for shared test infrastructure ==="
ls -la tests/ | head -20Repository: leynos/netsuke
Length of output: 11884
Replace direct std::env::set_var calls with the shared test_support::env::set_var helper.
Direct calls to std::env::set_var in test code violate the repository rule requiring environment mutations to use shared guards from the test-support layer. Other BDD steps already follow this pattern (fs.rs, manifest/mod.rs). Import and use test_support::env::set_var instead.
Affects lines 42 and 83.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/bdd/steps/configuration_preferences.rs` around lines 40 - 43, Replace
the unsafe direct call to std::env::set_var with the shared test helper: import
test_support::env::set_var and call that helper instead of unsafe {
std::env::set_var(CONFIG_ENV_VAR, OsStr::new(path.as_os_str())) }; keep the
surrounding logic (capturing previous via std::env::var_os(CONFIG_ENV_VAR), the
SAFETY note about EnvLock, and world.track_env_var(CONFIG_ENV_VAR.to_owned(),
previous)) unchanged so the EnvLock and world tracking continue to protect the
environment mutation.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Full content of configuration_preferences.rs ==="
if [ -f "tests/bdd/steps/configuration_preferences.rs" ]; then
cat -n tests/bdd/steps/configuration_preferences.rs
else
echo "File not found"
git ls-files | grep -i configuration_preferences || true
fi
echo
echo "=== Search for direct std::env::(set_var|remove_var) in this file ==="
rg -n 'std::env::(set_var|remove_var)|env::set_var|env::remove_var' tests/bdd/steps/configuration_preferences.rs || echo "No matches found"
echo
echo "=== Search for test_support::env usage in this file ==="
rg -n 'test_support::env' tests/bdd/steps/configuration_preferences.rs || echo "No matches found"
echo
echo "=== Check what test_support::env provides ==="
rg -rn 'pub fn set_var\|pub fn remove_var' --type rust tests/ || true
find . -path "*/test_support*" -name "*.rs" | head -20
Length of output: 8925
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Content of test_support/src/env.rs ==="
cat -n test_support/src/env.rs
echo
echo "=== Content of test_support/src/lib.rs (to see public API) ==="
cat -n test_support/src/lib.rs
Length of output: 17955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Content of test_support/src/env_lock.rs ==="
cat -n test_support/src/env_lock.rs
Length of output: 1117
@leynos, this is not yet resolved. Lines 42 and 83 still contain direct unsafe { std::env::set_var(...) } calls in the test file.
There is also a subtlety that prevents a naïve replacement: EnvLock uses a plain std::sync::Mutex, which is not re-entrant. Both write_config and set_environment_locale_override call ensure_env_lock(world) first, storing an EnvLock in world.env_lock. Calling test_support::env::set_var (which internally calls EnvLock::acquire()) while that lock is already held on the same thread would deadlock.
The correct fix is to add a set_var_locked helper to test_support::env that accepts an already-held &EnvLock as proof of serialisation, keeping the unsafe call inside the shared crate:
In `test_support/src/env.rs`, add the following public function after `set_var`:
```rust
/// Set an environment variable while the caller already holds an [`EnvLock`].
///
/// Returns the previous value. Use this when a scenario-wide lock must span
/// several mutations; taking `&EnvLock` proves serialisation without
/// re-acquiring the mutex (which would deadlock, as `Mutex` is not
/// re-entrant).
pub fn set_var_locked(_lock: &EnvLock, key: &str, value: &OsStr) -> Option<OsString> {
let previous = std::env::var_os(key);
// SAFETY: caller holds `EnvLock`, which serialises all mutations.
unsafe { std::env::set_var(key, value) };
previous
}
```
Then in `tests/bdd/steps/configuration_preferences.rs`:
1. Add `use test_support::env::set_var_locked;` to the imports.
2. In `write_config` (around line 40–43), replace:
```rust
let previous = std::env::var_os(CONFIG_ENV_VAR);
// SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario.
unsafe { std::env::set_var(CONFIG_ENV_VAR, OsStr::new(path.as_os_str())) };
```
with:
```rust
let lock = world.env_lock.borrow();
let lock_ref = lock.as_ref().expect("env_lock must be held before write_config");
let previous = set_var_locked(lock_ref, CONFIG_ENV_VAR, OsStr::new(path.as_os_str()));
```
3. In `set_environment_locale_override` (around line 81–83), replace:
```rust
let previous = std::env::var_os(LOCALE_ENV_VAR);
// SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario.
unsafe { std::env::set_var(LOCALE_ENV_VAR, OsStr::new(locale)) };
```
with:
```rust
let lock = world.env_lock.borrow();
let lock_ref = lock.as_ref().expect("env_lock must be held before set_environment_locale_override");
let previous = set_var_locked(lock_ref, LOCALE_ENV_VAR, OsStr::new(locale));
```
4. Remove the `use std::ffi::OsStr;` import only if it is no longer used elsewhere in the file (it is still needed for `OsStr::new` in the calls above, so keep it).
5. Ensure the `SAFETY` comments and the `unsafe` block are now absent from `configuration_preferences.rs`.
DO NOT change test names, step definitions, `#[rstest]` annotations, fixtures, or any logic outside the two env-mutation sites described above.
🧠 Learnings used
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.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: In Rust projects, when a type is #[must_use] and you would otherwise discard its value, prefer using drop(...) instead of lines like 'let _ = ...' to satisfy must_use checks. This aligns with an enforcement in Cargo.toml: let_underscore_must_use = "deny". Apply this guideline across Rust source files (e.g., in src/status.rs and other Rust files) to ensure lint compliance and avoid accidental ignored results.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: Enforce the deny(lint) setting for must_use results in Cargo.toml. When discarding must_use values (e.g., Result) after operations like writeln!(io::stderr(), ...), prefer the drop(...) idiom over using let _ = to satisfy the lint. Apply this pattern across all Rust source files (src/ and beyond) to ensure consistent lint compliance.
Comment on lines
+26
to
+34
| Scenario: no_emoji compatibility alias resolves to the ASCII theme | ||
| Given a minimal Netsuke workspace | ||
| And the Netsuke config file sets no_emoji to true | ||
| When the CLI is parsed and merged with "" | ||
| And merged output preferences are resolved | ||
| And the success prefix is rendered | ||
| Then parsing succeeds | ||
| And the merged theme is ascii | ||
| And the prefix contains no non-ASCII characters |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reorder the "Then parsing succeeds" step before the "And" steps.
The Gherkin step "Then parsing succeeds" at line 32 appears after several "And" continuation steps. Standard BDD practice places the primary assertion ("Then") before its refinements. Move line 32 to follow line 29 immediately.
♻️ Proposed fix
Scenario: no_emoji compatibility alias resolves to the ASCII theme
Given a minimal Netsuke workspace
And the Netsuke config file sets no_emoji to true
When the CLI is parsed and merged with ""
+ Then parsing succeeds
And merged output preferences are resolved
And the success prefix is rendered
- Then parsing succeeds
And the merged theme is ascii
And the prefix contains no non-ASCII characters📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Scenario: no_emoji compatibility alias resolves to the ASCII theme | |
| Given a minimal Netsuke workspace | |
| And the Netsuke config file sets no_emoji to true | |
| When the CLI is parsed and merged with "" | |
| And merged output preferences are resolved | |
| And the success prefix is rendered | |
| Then parsing succeeds | |
| And the merged theme is ascii | |
| And the prefix contains no non-ASCII characters | |
| Scenario: no_emoji compatibility alias resolves to the ASCII theme | |
| Given a minimal Netsuke workspace | |
| And the Netsuke config file sets no_emoji to true | |
| When the CLI is parsed and merged with "" | |
| Then parsing succeeds | |
| And merged output preferences are resolved | |
| And the success prefix is rendered | |
| And the merged theme is ascii | |
| And the prefix contains no non-ASCII characters |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/features/configuration_preferences.feature` around lines 26 - 34, Move
the Gherkin step "Then parsing succeeds" so it appears immediately after the
step "And the success prefix is rendered" (i.e., place the primary Then
assertion before its trailing And-steps), ensuring the scenario reads: Given...,
And the Netsuke config..., When..., And merged output preferences are resolved,
And the success prefix is rendered, Then parsing succeeds, And the merged theme
is ascii, And the prefix contains no non-ASCII characters.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat tests/features/configuration_preferences.feature
Length of output: 1450
@leynos, the fix has not been applied in the latest commit. The no_emoji scenario in tests/features/configuration_preferences.feature still reads:
Scenario: no_emoji compatibility alias resolves to the ASCII theme
Given a minimal Netsuke workspace
And the Netsuke config file sets no_emoji to true
When the CLI is parsed and merged with ""
And merged output preferences are resolved
And the success prefix is rendered
Then parsing succeeds ← still misplaced here
And the merged theme is ascii
And the prefix contains no non-ASCII charactersHere is an agent prompt to address it:
In `tests/features/configuration_preferences.feature`, fix the step ordering in the "no_emoji compatibility alias resolves to the ASCII theme" scenario. Move the `Then parsing succeeds` step so it appears immediately after `When the CLI is parsed and merged with ""`, before the `And merged output preferences are resolved` and `And the success prefix is rendered` steps. The corrected scenario should read:
Scenario: no_emoji compatibility alias resolves to the ASCII theme
Given a minimal Netsuke workspace
And the Netsuke config file sets no_emoji to true
When the CLI is parsed and merged with ""
Then parsing succeeds
And merged output preferences are resolved
And the success prefix is rendered
And the merged theme is ascii
And the prefix contains no non-ASCII characters
Do NOT alter any other scenario or any other file. Ensure the file is valid Gherkin and that the BDD test suite still passes.
Owner
Author
|
@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 const _: usize = std::mem::size_of::<cli::BuildArgs>();
const _: usize = std::mem::size_of::<cli::CliConfig>();
const _: usize = std::mem::size_of::<cli::Commands>();❌ New issue: Large Method |
This comment was marked as resolved.
This comment was marked as resolved.
Moved parser-facing `Cli` type to `src/cli/parser.rs` and layered configuration into a `CliConfig` struct in `src/cli/config.rs`. The top-level `src/cli/mod.rs` now re-exports the public CLI surface. This separation improves modularity by distinguishing parsing, configuration discovery, and runtime command selection while preserving existing command syntax. Also updated default manifest path handling in CLI arguments and enhanced test support environment isolation. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Gates Failed
(1 file with Large Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| build.rs | 1 advisory rule | 10.00 → 9.61 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
leynos
changed the title
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Large Methodbuild.rs: main What lead to degradation?main has 70 lines, threshold = 70 Why does this problem occur?Overly long functions make the code harder to read. The recommended maximum function length for the Rust language is 70 lines of code. Severity: Brain Method - Complex Method - Long Method. How to fix it?We recommend to be careful here -- just splitting long functions don't necessarily make the code easier to read. Instead, look for natural chunks inside the functions that expresses a specific task or concern. Often, such concerns are indicated by a Code Comment followed by an if-statement. Use the EXTRACT FUNCTION refactoring to encapsulate that concern. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7882
Root cause
Extracting all three drops Fix
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
no_emoji.cmds.build.targets) through the layered config path.Changes
New modules and schema:
Refactor: moved merge logic and OrthoConfig-driven flow out of src/cli/mod.rs into dedicated modules for maintainability and testability.
Parser/merge wiring: CliConfig is wired into the OrthoConfig discovery/merge path so global values and subcommand defaults (e.g. cmds.build.targets) resolve through the same typed fields.
Compatibility: preserve hidden config-path behavior and keep --config handling out of scope for this milestone.
Tooling and dependencies:
Documentation and tests:
Example config (TOML) (for reference):
verbose = true
locale = "es-ES"
colour_policy = "auto"
spinner_mode = "auto"
output_format = "human"
theme = "ascii"
progress = true
accessible = false
[cmds.build]
targets = ["all"]
This demonstrates a canonical view where global fields live at the root and subcommand defaults (like build targets) live under their respective sections.
Documentation impact
How to test locally
Plan alignment and next steps
Idempotence and recovery
📎 Task: https://www.devboxer.com/task/c5d87114-1d80-439d-b1a9-21848725bc15