From 7580da384982a960ef98d903459f5cc975186d54 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 8 Mar 2026 00:03:10 +0000 Subject: [PATCH 01/12] docs(execplans): add detailed ExecPlan for CliConfig schema introduction 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] --- docs/execplans/3-11-1-cli-config-struct.md | 461 +++++++++++++++++++++ 1 file changed, 461 insertions(+) create mode 100644 docs/execplans/3-11-1-cli-config-struct.md diff --git a/docs/execplans/3-11-1-cli-config-struct.md b/docs/execplans/3-11-1-cli-config-struct.md new file mode 100644 index 00000000..6ec7bfd4 --- /dev/null +++ b/docs/execplans/3-11-1-cli-config-struct.md @@ -0,0 +1,461 @@ +# Introduce `CliConfig` as the layered CLI configuration schema + +This ExecPlan (execution plan) is a living document. The sections +`Constraints`, `Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, +`Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work +proceeds. + +Status: DRAFT + +No `PLANS.md` file exists in this repository. + +## Purpose / big picture + +Roadmap item `3.11.1` asks for a dedicated `CliConfig` struct derived with +`OrthoConfig` so Netsuke has one explicit, typed schema for layered CLI +configuration. Today the repository already has partial layered configuration, +but it is centered on [`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs), +where `Cli` currently serves three roles at once: + +1. Clap parser. +2. OrthoConfig merge target. +3. Runtime command model passed into the runner. + +That coupling makes the code difficult to extend. It also leaves roadmap fields +such as colour policy, spinner mode, output format, default targets, and theme +either implicit or missing. + +After this change, a novice should be able to point at one config schema and +see how Netsuke resolves: + +- verbosity +- locale +- colour policy +- spinner mode +- progress and accessible output behaviour +- output format +- default build targets +- theme selection + +Observable success means: + +1. `CliConfig` exists and is the authoritative OrthoConfig-derived schema. +2. Clap parsing still works, but parsing and layered configuration are no + longer the same type. +3. Configuration files, environment variables, and CLI flags resolve through + the same typed fields with documented precedence. +4. Unit tests (`rstest`) and behavioural tests (`rstest-bdd` v0.5.0) cover + happy paths, unhappy paths, and precedence edge cases. +5. [`docs/users-guide.md`](/home/user/project/docs/users-guide.md), + [`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md), and + [`docs/roadmap.md`](/home/user/project/docs/roadmap.md) reflect the final + behaviour. + +## Constraints + +- Keep existing top-level commands stable: `build`, `clean`, `graph`, and + `manifest` must continue to parse and dispatch. +- Do not regress localized Clap help or localized runtime diagnostics. +- Use `ortho_config` as the primary merge mechanism rather than adding another + bespoke loader. +- Preserve the current configuration discovery behaviour for this milestone. + Do not expose the visible `--config` / `NETSUKE_CONFIG` interface here; that + belongs to roadmap item `3.11.3`. +- Preserve existing user-facing flags where feasible. If a new canonical field + supersedes an old one, keep a compatibility path unless the user explicitly + approves a breaking change. +- No source file may exceed 400 lines. This is a hard constraint, not a style + preference. +- All new or changed public types and modules require Rustdoc and module-level + documentation. +- Behaviour must be validated with both `rstest` unit tests and + `rstest-bdd` v0.5.0 behavioural tests. +- The implementation must end with successful `make check-fmt`, `make lint`, + and `make test` runs captured via `tee` and `set -o pipefail`. +- Documentation updates are part of the feature, not follow-up work. + +## Tolerances (exception triggers) + +- Scope: if implementation requires more than 18 files or more than 900 net + new lines, stop and escalate. +- Interfaces: if the runner or subcommand API must change in a way that breaks + existing tests or user-facing CLI syntax, stop and escalate. +- Dependencies: if implementation requires a new runtime dependency beyond + `ortho_config` facilities already in use, stop and escalate. +- Compatibility: if preserving `--no-emoji` compatibility while introducing a + canonical theme field becomes impossible without ambiguous precedence, stop + and escalate. +- Investigation: if configuration merging still behaves unexpectedly after + three red/green cycles, stop and document the blocking case before proceeding. + +## Risks + +- Risk: [`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs) is already 398 + lines, so any additive work there will violate the file-size limit. Severity: + high. Likelihood: high. Mitigation: split parser definitions, config schema, + and merge helpers into separate modules before adding new fields. + +- Risk: the repository already has partial OrthoConfig support, manual config + discovery, and merge tests. A careless refactor could duplicate logic rather + than simplifying it. Severity: medium. Likelihood: high. Mitigation: treat + this as a consolidation task and delete superseded merge code as part of the + same change. + +- Risk: roadmap item `3.11.1` reaches into future roadmap items by naming + output format and theme before `3.10.3` and `3.12.*` are complete. Severity: + medium. Likelihood: high. Mitigation: introduce typed config fields now, but + limit behaviour to what the current product can honour and fail clearly on + unsupported values. + +- Risk: build default targets are subcommand-specific, while most other config + is global. Severity: medium. Likelihood: medium. Mitigation: use + OrthoConfig's subcommand configuration support for `build` instead of forcing + targets into the global namespace. + +## Progress + +- [x] (2026-03-07) Reviewed roadmap `3.11.1`, the OrthoConfig guide, current + CLI/config code, and nearby execplans. +- [x] (2026-03-07) Drafted this ExecPlan. +- [ ] Approval received to implement. +- [ ] Stage A: split parser, config schema, and merge responsibilities. +- [ ] Stage B: introduce typed config groups and compatibility mapping. +- [ ] Stage C: wire global and subcommand merges through `CliConfig`. +- [ ] Stage D: add unit and behavioural coverage. +- [ ] Stage E: update user/design docs, mark roadmap item done, and run all + quality gates. + +## Surprises & Discoveries + +- The codebase already derives `OrthoConfig` on `Cli`; roadmap `3.11.1` is + therefore a refactor-and-expansion task, not a greenfield introduction. +- The current repository already documents configuration discovery in + [`docs/users-guide.md`](/home/user/project/docs/users-guide.md), and already + has merge tests in + [`tests/cli_tests/merge.rs`](/home/user/project/tests/cli_tests/merge.rs). + The implementation must preserve these guarantees while changing the type + layout. +- `rstest-bdd` feature-file edits may require touching + [`tests/bdd_tests.rs`](/home/user/project/tests/bdd_tests.rs) to force Cargo + to rebuild generated scenarios. + +## Decision Log + +- Decision: introduce a dedicated `CliConfig` type and treat the existing CLI + parser as a separate concern. Rationale: parsing tokens and merging layered + configuration are related but not identical jobs, and combining them has + already pushed the current module to the file-size limit. Date/Author: + 2026-03-07 (Codex, plan draft) + +- Decision: use OrthoConfig subcommand configuration for build defaults rather + than a global `default_targets` field. Rationale: target lists only make + sense for `build`; placing them under `cmds.build` matches the OrthoConfig + user's guide and avoids leaking command-specific semantics into global + config. Date/Author: 2026-03-07 (Codex, plan draft) + +- Decision: keep current hidden config-path override behaviour in this + milestone and defer the visible `--config` / `NETSUKE_CONFIG` surface to + roadmap item `3.11.3`. Rationale: the roadmap splits schema introduction from + config-path UX, and this plan should not silently complete later work. + Date/Author: 2026-03-07 (Codex, plan draft) + +- Decision: make `theme` the canonical presentation setting and treat + `no_emoji` as a compatibility alias that resolves to the ASCII theme. + Rationale: roadmap `3.12.*` already talks about themes, while the current + implementation only exposes `no_emoji`; a compatibility bridge avoids + breaking existing users. Date/Author: 2026-03-07 (Codex, plan draft) + +- Decision: add typed enums for colour policy, spinner mode, output format, + and theme even where runtime behaviour is still limited. Rationale: the + schema should become explicit now so future milestones extend behaviour + without reworking config names a second time. Unsupported combinations must + fail with actionable diagnostics. Date/Author: 2026-03-07 (Codex, plan draft) + +These decisions must be recorded in +[`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md) during +implementation if they remain unchanged after coding begins. + +## Outcomes & Retrospective + +Pending. This section must be replaced with the actual result, quality-gate +evidence, and lessons learned once implementation completes. + +## Context and orientation + +The current implementation is spread across the following files: + +- [`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs): current `Cli` type, + Clap parser, OrthoConfig derive, validation parsers, and merge logic. +- [`src/main.rs`](/home/user/project/src/main.rs): startup parse/merge flow and + runtime localization bootstrap. +- [`src/output_mode.rs`](/home/user/project/src/output_mode.rs): accessible + versus standard output mode resolution. +- [`src/output_prefs.rs`](/home/user/project/src/output_prefs.rs): emoji-aware + semantic prefixes and current `no_emoji` handling. +- [`src/runner/mod.rs`](/home/user/project/src/runner/mod.rs): uses merged CLI + state to choose output mode, progress behaviour, and build targets. +- [`tests/cli_tests/merge.rs`](/home/user/project/tests/cli_tests/merge.rs): + current merge precedence coverage. +- [`tests/cli_tests/parsing.rs`](/home/user/project/tests/cli_tests/parsing.rs) + and + [`tests/features/cli.feature`](/home/user/project/tests/features/cli.feature): + current parse-only coverage. + +Two important facts shape this plan: + +1. The repo already has a layered configuration story. +2. The missing piece is a stable, explicit schema that separates merged config + from raw CLI parsing and adds the roadmap fields that do not yet exist. + +## Target architecture + +The implementation should converge on three layers of types. + +1. A Clap-facing parser model, still responsible for token parsing and + user-facing command syntax. +2. A layered configuration model rooted at `CliConfig`, derived with + `OrthoConfig`, `Serialize`, and `Deserialize`. +3. A runtime model passed into the runner after configuration and subcommand + selection have been resolved. + +The preferred shape is: + +- `src/cli/mod.rs`: parser entry points and minimal top-level glue. +- `src/cli/config.rs`: `CliConfig` plus nested typed config groups. +- `src/cli/merge.rs` or similar: conversion from parsed CLI overrides plus + OrthoConfig layer composition into the merged runtime shape. +- `BuildArgs` (or a renamed build-config type) derived with `OrthoConfig` so + `cmds.build` configuration can supply default targets and optional emit-path + defaults. + +The config schema should cover at least these concepts: + +- `verbose` +- `locale` +- `colour_policy` +- `spinner_mode` +- `output_format` +- `theme` +- `progress` +- `accessible` +- current fetch-policy settings +- build default targets through subcommand configuration + +For this milestone, a valid example config should look like this: + +```toml +verbose = true +locale = "es-ES" +colour_policy = "auto" +spinner_mode = "auto" +output_format = "human" +theme = "ascii" +progress = true +accessible = false + +[cmds.build] +targets = ["all"] +``` + +The final user guide must explain the actual accepted values and any +compatibility aliases such as `no_emoji = true`. + +## Plan of work + +### Stage A: split responsibilities before adding new fields + +Start by reducing the blast radius in +[`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs). Move the merge logic and +the future `CliConfig` definition out of that file first. The goal is to make +later edits mechanical instead of risky. + +Concrete work in this stage: + +1. Extract the current OrthoConfig-driven merge helpers into a dedicated module. +2. Introduce a parser-only root type if needed (`Cli`, `CliArgs`, or similar), + while keeping command syntax unchanged. +3. Keep existing parsing tests green before introducing new schema fields. + +Acceptance for Stage A: + +- CLI parsing tests still pass unchanged. +- No file exceeds 400 lines. +- No behaviour changes are visible yet. + +### Stage B: introduce `CliConfig` and typed config groups + +Create `CliConfig` as the authoritative layered configuration schema. Use typed +enums or newtypes where values are semantic rather than free-form text. + +Concrete work in this stage: + +1. Define `CliConfig` and any nested groups needed to keep modules readable. +2. Add typed enums for: + - `ColourPolicy` + - `SpinnerMode` + - `OutputFormat` + - `Theme` +3. Decide and implement compatibility handling for the current `no_emoji` + surface. +4. Use OrthoConfig discovery attributes on `CliConfig` to preserve the current + `NETSUKE_CONFIG_PATH` and hidden config-path behaviour. +5. Keep global fields optional where layered precedence requires absence to be + meaningful. + +Acceptance for Stage B: + +- `CliConfig` can be merged from defaults, file, env, and CLI layers in unit + tests without invoking the full parser. +- Invalid enum values fail with actionable errors. +- Existing config-discovery paths still work. + +### Stage C: merge global config plus selected subcommand defaults + +This stage makes `CliConfig` drive runtime behaviour instead of the current +all-in-one `Cli` type. + +Concrete work in this stage: + +1. Replace `Cli::merge_with_config` with a merge path rooted in `CliConfig`. +2. Merge `build` subcommand defaults using OrthoConfig's subcommand support so + `[cmds.build] targets = [...]` becomes the default target list when the user + does not pass targets explicitly. +3. Convert the merged config plus parsed command into the runtime shape + consumed by [`src/runner/mod.rs`](/home/user/project/src/runner/mod.rs). +4. Update output-mode and output-preference resolution to consume the new typed + fields rather than a loose collection of booleans. +5. Keep startup locale resolution intact so localized Clap help still works + before full merge. + +Acceptance for Stage C: + +- Running `netsuke` with no explicit targets can pick up configured build + targets from `cmds.build`. +- CLI overrides still beat environment and file values. +- Existing commands continue to dispatch correctly. + +### Stage D: add test coverage for happy and unhappy paths + +Unit tests should prove the schema and merge logic. Behavioural tests should +prove the user-visible outcomes. + +Unit coverage to add with `rstest`: + +- defaults < file < env < CLI precedence for representative global fields +- `theme` / `no_emoji` compatibility mapping +- enum parsing failures for `colour_policy`, `spinner_mode`, and + `output_format` +- build-target defaulting through `cmds.build` +- unsupported but syntactically valid combinations fail clearly when required + +Behavioural coverage to add with `rstest-bdd` v0.5.0: + +- config file supplies default build targets and `netsuke` uses them +- CLI `--locale` or `--verbose` overrides file and env values +- invalid config values produce user-facing diagnostics +- compatibility alias (`no_emoji`) still produces ASCII-themed output + +Prefer a dedicated feature file such as +[`tests/features/configuration_preferences.feature`](/home/user/project/tests/features/configuration_preferences.feature) + plus matching step definitions, rather than overloading the existing +CLI-parsing feature with merge semantics. + +### Stage E: document the final contract and close the roadmap item + +After behaviour is working and tested, update the docs as part of the same +change. + +Documentation work: + +1. Update [`docs/users-guide.md`](/home/user/project/docs/users-guide.md) with + the new config schema, precedence rules, accepted values, and example TOML. +2. Update [`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md) + to record: + - separation of parser and config schema + - subcommand config for default build targets + - canonical theme handling and `no_emoji` compatibility + - which output-format values are supported in this milestone +3. Mark roadmap item `3.11.1` as done in + [`docs/roadmap.md`](/home/user/project/docs/roadmap.md). + +Acceptance for Stage E: + +- User docs match the shipped behaviour. +- Design docs capture the decisions from this plan that survived + implementation. +- Only roadmap item `3.11.1` is marked done unless later work is intentionally + completed and validated too. + +## Concrete steps + +Run all commands from `/home/user/project`. + +Before editing feature files, remember the existing `rstest-bdd` gotcha: + +```sh +touch tests/bdd_tests.rs +``` + +Use `tee` and `set -o pipefail` for every long-running gate: + +```sh +set -o pipefail +make check-fmt 2>&1 | tee /tmp/netsuke-check-fmt.log +``` + +```sh +set -o pipefail +make lint 2>&1 | tee /tmp/netsuke-lint.log +``` + +```sh +set -o pipefail +make test 2>&1 | tee /tmp/netsuke-test.log +``` + +Because docs will change, also run: + +```sh +set -o pipefail +PATH="/root/.bun/bin:$PATH" make markdownlint 2>&1 | tee /tmp/netsuke-markdownlint.log +``` + +```sh +set -o pipefail +make nixie 2>&1 | tee /tmp/netsuke-nixie.log +``` + +```sh +set -o pipefail +make fmt 2>&1 | tee /tmp/netsuke-fmt.log +``` + +After `make fmt`, inspect `git status --short` and remove incidental edits in +unrelated files before finalizing the change. + +## Validation and acceptance + +The feature is complete only when all of the following are true: + +1. `CliConfig` is the authoritative OrthoConfig-derived schema. +2. Parser-only code and merge-only code are separated enough to keep files + under the 400-line limit. +3. `build` default targets can be supplied through configuration without + breaking explicit CLI targets. +4. Typed config values are documented and validated with clear unhappy-path + errors. +5. Unit tests and behavioural tests cover both precedence and failure cases. +6. `make check-fmt`, `make lint`, `make test`, `make markdownlint`, and + `make nixie` succeed. +7. [`docs/users-guide.md`](/home/user/project/docs/users-guide.md), + [`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md), and + [`docs/roadmap.md`](/home/user/project/docs/roadmap.md) are updated. + +## Idempotence and recovery + +This work should be implemented in small, reversible steps. If a stage fails, +return the tree to the last green checkpoint, update this ExecPlan's +`Decision Log` and `Progress`, and retry with a narrower diff rather than +stacking speculative fixes. If formatting tools touch unrelated Markdown files, +restore only the incidental edits created during the current turn before +proceeding. From 98e2278d8c41067152a4cb7d4051495b5bf3b29b Mon Sep 17 00:00:00 2001 From: Leynos Date: Sun, 8 Mar 2026 17:45:01 +0000 Subject: [PATCH 02/12] feat(docs): upgrade ortho_config docs and embed v0.8.0 migration guidance - 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] --- docs/execplans/3-11-1-cli-config-struct.md | 63 +++++- docs/ortho-config-users-guide.md | 229 +++++++++++++++++++-- 2 files changed, 269 insertions(+), 23 deletions(-) diff --git a/docs/execplans/3-11-1-cli-config-struct.md b/docs/execplans/3-11-1-cli-config-struct.md index 6ec7bfd4..9bf0c456 100644 --- a/docs/execplans/3-11-1-cli-config-struct.md +++ b/docs/execplans/3-11-1-cli-config-struct.md @@ -13,9 +13,13 @@ No `PLANS.md` file exists in this repository. Roadmap item `3.11.1` asks for a dedicated `CliConfig` struct derived with `OrthoConfig` so Netsuke has one explicit, typed schema for layered CLI -configuration. Today the repository already has partial layered configuration, -but it is centered on [`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs), -where `Cli` currently serves three roles at once: +configuration. This plan now targets `ortho_config` `0.8.0` and uses the +repository copy of +[`docs/ortho-config-users-guide.md`](/home/user/project/docs/ortho-config-users-guide.md), + which has been replaced with the upstream `v0.8.0` guide. Today the repository +already has partial layered configuration, but it is centered on +[`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs), where `Cli` currently +serves three roles at once: 1. Clap parser. 2. OrthoConfig merge target. @@ -55,15 +59,28 @@ Observable success means: - Keep existing top-level commands stable: `build`, `clean`, `graph`, and `manifest` must continue to parse and dispatch. +- Upgrade every in-repo `ortho_config` dependency to `0.8.0`, and add or + upgrade `ortho_config_macros` to `0.8.0` if the final implementation uses + derive-generated selected-subcommand merging. +- Keep the toolchain at Rust `1.88` or newer. The current repository already + uses Rust `1.89.0`, so this is a compatibility floor rather than a required + toolchain migration. - Do not regress localized Clap help or localized runtime diagnostics. - Use `ortho_config` as the primary merge mechanism rather than adding another bespoke loader. +- When derive-generated code touches `figment`, `uncased`, or `xdg`, prefer the + `ortho_config::...` re-exports described by the `0.8.0` guide unless the + application source genuinely needs those crates directly. - Preserve the current configuration discovery behaviour for this milestone. Do not expose the visible `--config` / `NETSUKE_CONFIG` interface here; that belongs to roadmap item `3.11.3`. - Preserve existing user-facing flags where feasible. If a new canonical field supersedes an old one, keep a compatibility path unless the user explicitly approves a breaking change. +- If `cli_default_as_absent` is used on any new or refactored field, use typed + Clap defaults (`default_value_t` / `default_values_t`) rather than + string-based `default_value`, and pass `ArgMatches` into merge flows so + explicit CLI overrides remain distinguishable from inferred defaults. - No source file may exceed 400 lines. This is a hard constraint, not a style preference. - All new or changed public types and modules require Rustdoc and module-level @@ -101,6 +118,12 @@ Observable success means: this as a consolidation task and delete superseded merge code as part of the same change. +- Risk: the repository currently depends on `ortho_config 0.7.0`, while this + plan now targets `0.8.0`. Severity: medium. Likelihood: high. Mitigation: + treat the crate upgrade as part of the same branch and apply the documented + migration rules up front instead of retrofitting them after the schema + refactor lands. + - Risk: roadmap item `3.11.1` reaches into future roadmap items by naming output format and theme before `3.10.3` and `3.12.*` are complete. Severity: medium. Likelihood: high. Mitigation: introduce typed config fields now, but @@ -129,6 +152,9 @@ Observable success means: - The codebase already derives `OrthoConfig` on `Cli`; roadmap `3.11.1` is therefore a refactor-and-expansion task, not a greenfield introduction. +- The repository is already on Rust `1.89.0`, which satisfies the + `ortho_config 0.8.0` minimum of Rust `1.88`. The crate version is the real + migration step; the compiler floor is not. - The current repository already documents configuration discovery in [`docs/users-guide.md`](/home/user/project/docs/users-guide.md), and already has merge tests in @@ -171,6 +197,13 @@ Observable success means: without reworking config names a second time. Unsupported combinations must fail with actionable diagnostics. Date/Author: 2026-03-07 (Codex, plan draft) +- Decision: treat `ortho_config 0.8.0` as the baseline for this work and align + the implementation with its migration rules. Rationale: the local guide now + reflects `v0.8.0`, and the implementation plan should not be written against + `0.7.x` semantics. The repository does not currently alias the dependency + name, so `#[ortho_config(crate = "...")]` is not required unless that changes + during implementation. Date/Author: 2026-03-08 (Codex, plan revision) + These decisions must be recorded in [`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md) during implementation if they remain unchanged after coding begins. @@ -186,6 +219,10 @@ The current implementation is spread across the following files: - [`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs): current `Cli` type, Clap parser, OrthoConfig derive, validation parsers, and merge logic. +- [`Cargo.toml`](/home/user/project/Cargo.toml): currently pins + `ortho_config = "0.7.0"` and `rust-version = "1.89.0"`. +- [`rust-toolchain.toml`](/home/user/project/rust-toolchain.toml): currently + pins toolchain `1.89.0`, which already satisfies the `0.8.0` minimum. - [`src/main.rs`](/home/user/project/src/main.rs): startup parse/merge flow and runtime localization bootstrap. - [`src/output_mode.rs`](/home/user/project/src/output_mode.rs): accessible @@ -214,7 +251,7 @@ The implementation should converge on three layers of types. 1. A Clap-facing parser model, still responsible for token parsing and user-facing command syntax. 2. A layered configuration model rooted at `CliConfig`, derived with - `OrthoConfig`, `Serialize`, and `Deserialize`. + `OrthoConfig`, `Serialize`, and `Deserialize`, using `0.8.0` semantics. 3. A runtime model passed into the runner after configuration and subcommand selection have been resolved. @@ -267,14 +304,17 @@ compatibility aliases such as `no_emoji = true`. Start by reducing the blast radius in [`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs). Move the merge logic and the future `CliConfig` definition out of that file first. The goal is to make -later edits mechanical instead of risky. +later edits mechanical instead of risky. This stage also establishes the +`ortho_config 0.8.0` baseline before higher-level schema refactors pile on. Concrete work in this stage: 1. Extract the current OrthoConfig-driven merge helpers into a dedicated module. 2. Introduce a parser-only root type if needed (`Cli`, `CliArgs`, or similar), while keeping command syntax unchanged. -3. Keep existing parsing tests green before introducing new schema fields. +3. Upgrade `ortho_config` to `0.8.0` and confirm the repository still builds + against Rust `1.89.0` without toolchain changes. +4. Keep existing parsing tests green before introducing new schema fields. Acceptance for Stage A: @@ -301,6 +341,8 @@ Concrete work in this stage: `NETSUKE_CONFIG_PATH` and hidden config-path behaviour. 5. Keep global fields optional where layered precedence requires absence to be meaningful. +6. Where Clap defaults are required, use typed defaults so + `cli_default_as_absent` remains valid under `0.8.0`. Acceptance for Stage B: @@ -326,6 +368,9 @@ Concrete work in this stage: fields rather than a loose collection of booleans. 5. Keep startup locale resolution intact so localized Clap help still works before full merge. +6. If selected-subcommand merge derives are introduced, add + `ortho_config_macros 0.8.0` and pass `ArgMatches` where required by + `cli_default_as_absent`. Acceptance for Stage C: @@ -347,6 +392,8 @@ Unit coverage to add with `rstest`: `output_format` - build-target defaulting through `cmds.build` - unsupported but syntactically valid combinations fail clearly when required +- any `cli_default_as_absent` fields continue to prefer file/env values when + the user did not explicitly supply the CLI flag Behavioural coverage to add with `rstest-bdd` v0.5.0: @@ -433,6 +480,10 @@ make fmt 2>&1 | tee /tmp/netsuke-fmt.log After `make fmt`, inspect `git status --short` and remove incidental edits in unrelated files before finalizing the change. +The local OrthoConfig reference for this task is now the `v0.8.0` guide at +[`docs/ortho-config-users-guide.md`](/home/user/project/docs/ortho-config-users-guide.md). + Do not rely on older `0.7.x` examples when the two guides disagree. + ## Validation and acceptance The feature is complete only when all of the following are true: diff --git a/docs/ortho-config-users-guide.md b/docs/ortho-config-users-guide.md index 2553cdd2..462435c4 100644 --- a/docs/ortho-config-users-guide.md +++ b/docs/ortho-config-users-guide.md @@ -141,7 +141,7 @@ CLI layers. This validates every precedence permutation without copy-pasting setup. Every derived configuration also exposes `compose_layers()` and -`compose_layers_from_iter(..)`. These helpers discover configuration files, +`compose_layers_from_iter(...)`. These helpers discover configuration files, serialize environment variables, and capture CLI input as a `LayerComposition`, keeping discovery separate from merging. The returned composition includes both the ordered layers and any collected errors, letting callers push additional @@ -273,7 +273,7 @@ Add `ortho_config` as a dependency in `Cargo.toml` along with `serde`: ```toml [dependencies] -ortho_config = "0.7.0" # replace with the latest version +ortho_config = "0.8.0" # replace with the latest version serde = { version = "1.0", features = ["derive"] } clap = { version = "4", features = ["derive"] } # required for CLI support ``` @@ -284,7 +284,7 @@ corresponding cargo features: ```toml [dependencies] -ortho_config = { version = "0.7.0", features = ["json5", "yaml"] } +ortho_config = { version = "0.8.0", features = ["json5", "yaml"] } # Enabling these features expands file formats; precedence stays: defaults < file < env < CLI. ``` @@ -301,6 +301,62 @@ Redox targets), and the optional parsers (`figment_json5`, `json5`, is enabled by default because the crate relies on it internally; disable default features only when explicitly opting back into `serde_json`. +### Dependency architecture for derive macro users + +The `#[derive(OrthoConfig)]` macro emits fully qualified paths rooted at +`ortho_config`. For example, generated code references +`ortho_config::figment::Figment` and `ortho_config::uncased::Uncased` rather +than `figment::...` or `uncased::...`. Those paths resolve because +`ortho_config` re-exports these crates. + +For screen readers: The following diagram shows that generated code references +re-exported crates through `ortho_config`, so consumer crates can rely on the +runtime crate dependency. + +```mermaid +flowchart TD + A[Consumer crate] -->|depends on| B[ortho_config] + C[derive OrthoConfig] -->|generates| D[ortho_config::figment::...] + C -->|generates| E[ortho_config::uncased::...] + B -->|re-exports| F[figment] + B -->|re-exports| G[uncased] + B -->|re-exports on Unix/Redox| H[xdg] +``` + +_Figure 1: Derive output resolves parser crates through `ortho_config`._ + +In the common case, `Cargo.toml` does not need direct `figment`, `uncased`, or +`xdg` dependencies: + +```toml +[dependencies] +ortho_config = "0.8.0" +serde = { version = "1.0", features = ["derive"] } +clap = { version = "4", features = ["derive"] } +``` + +### Troubleshooting dependency errors + +- If source code imports `figment`, `uncased`, or `xdg` directly, either switch + imports to `ortho_config::figment` / `ortho_config::uncased` / + `ortho_config::xdg`, or keep explicit dependencies for that direct usage. +- If derive output fails with unresolved `ortho_config::...` paths, ensure the + dependency key is named `ortho_config` in `Cargo.toml` or use the + `#[ortho_config(crate = "...")]` attribute to specify the alias. +- **Dependency aliasing** is supported via the `crate` attribute. When + renaming the dependency in `Cargo.toml` (for example, + `my_cfg = { package = "ortho_config", ... }`), add + `#[ortho_config(crate = "my_cfg")]` to the struct so generated code + references the correct crate path. +- If dependency resolution reports conflicts, inspect duplicates with + `cargo tree -d` and prefer the versions selected through `ortho_config` + unless direct usage requires something else. + +### FAQ: should `figment`, `uncased`, or `xdg` be direct dependencies? + +No for derive-generated code. Yes, only when application code directly imports +those crates without going through the `ortho_config::` re-exports. + YAML parsing is handled by the pure-Rust `serde-saphyr` crate. It adheres to the YAML 1.2 specification, so unquoted scalars such as `yes`, `on`, and `off` remain strings. The provider enables `Options::strict_booleans`, ensuring only @@ -378,13 +434,13 @@ environment variables like `APP_PORT` and file names such as `.app.toml`. Field attributes modify how a field is sourced or merged: -| Attribute | Behaviour | -| --------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `default = expr` | Supplies a default value when no source provides one. The expression can be a literal or a function path. | -| `cli_long = "name"` | Overrides the automatically generated long CLI flag (kebab-case). | -| `cli_short = 'c'` | Adds a single-letter short flag for the field. | -| `merge_strategy = "append"` | For `Vec` fields, specifies that values from different sources should be concatenated. This is currently the only supported strategy and is the default for vector fields. | -| `cli_default_as_absent` | Treats clap's `default_value_t` as absent during subcommand merging. File and environment values take precedence over clap defaults, while explicit CLI overrides still win. | +| Attribute | Behaviour | +| --------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `default = expr` | Supplies a default value when no source provides one. The expression can be a literal or a function path. | +| `cli_long = "name"` | Overrides the automatically generated long CLI flag (kebab-case). | +| `cli_short = 'c'` | Adds a single-letter short flag for the field. | +| `merge_strategy = "append"` | For `Vec` fields, specifies that values from different sources should be concatenated. This is currently the only supported strategy and is the default for vector fields. | +| `cli_default_as_absent` | Treats typed clap defaults (`default_value_t`, `default_values_t`) as absent during configuration merging. File and environment values take precedence, while explicit CLI overrides still win. | Unrecognized keys are ignored by the derive macro for forwards compatibility. Unknown keys will therefore silently do nothing. Developers who require @@ -872,21 +928,32 @@ when the user does not explicitly provide a value on the command line, the field is excluded from the CLI layer so that file and environment values take precedence. -Add the attribute alongside the matching `default` attribute: +Add `cli_default_as_absent` and define the default in clap. The derive macro +now infers the struct default from clap's default metadata, so the default only +needs to be declared once: ```rust #[derive(Parser, Deserialize, Serialize, OrthoConfig)] #[ortho_config(prefix = "APP_")] struct GreetArgs { #[arg(long, default_value_t = String::from("!"))] - #[ortho_config(default = String::from("!"), cli_default_as_absent)] + #[ortho_config(cli_default_as_absent)] punctuation: String, } ``` +`default_value_t` and `default_values_t` are supported for inferred defaults. +`default_value` inference is intentionally unsupported for now; use +`default_value_t` or add an explicit `#[ortho_config(default = ...)]` to avoid +string-parser mismatches. Parser-faithful `default_value` inference is planned +as a day-2 follow-up. + +If `#[ortho_config(default = ...)]` is still provided, that explicit value +remains available for generated defaults/documentation metadata. + **Precedence with the attribute (lowest to highest):** -1. Struct default (`#[ortho_config(default = ...)]`) +1. Struct default (`#[ortho_config(default = ...)]` or inferred from clap) 2. Configuration file 3. Environment variable 4. Explicit CLI override (e.g. `--punctuation "?"`) @@ -1048,6 +1115,134 @@ fn interop(r: ortho_config::OrthoResult) -> Result } ``` +## Documentation metadata (OrthoConfigDocs) + +The derive macro now emits an `OrthoConfigDocs` implementation alongside the +runtime loader. This lets tooling such as `cargo-orthohelp` serialize a stable, +clap-agnostic intermediate representation (IR) for man pages and PowerShell +help. + +```rust +use ortho_config::docs::OrthoConfigDocs; + +#[derive(serde::Deserialize, serde::Serialize, ortho_config::OrthoConfig)] +#[ortho_config(prefix = "APP")] +struct AppConfig { + #[ortho_config(default = 8080)] + port: u16, +} + +let ir = AppConfig::get_doc_metadata(); +let json = ortho_config::serde_json::to_string_pretty(&ir)?; +println!("{json}"); +``` + +When IDs are not supplied, the macro generates deterministic defaults such as +`{app}.about` for the CLI overview and `{app}.fields.{field}.help` for field +descriptions. Field-level metadata can be refined with `help_id`, +`long_help_id`, `value(type = "...")`, `deprecated(note_id = "...")`, +`env(name = "...")`, and `file(key_path = "...")`. These documentation +attributes affect only the emitted IR; they do not change runtime naming or +loading behaviour. + +### Generating IR with cargo-orthohelp + +`cargo-orthohelp` compiles a tiny bridge binary that calls +`OrthoConfigDocs::get_doc_metadata()`, resolves Fluent messages per locale, and +writes localized IR JSON into the chosen output directory. Add metadata to the +package `Cargo.toml` so the tool knows which config type to load: + +```toml +[package.metadata.ortho_config] +root_type = "hello_world::cli::HelloWorldCli" +locales = ["en-US", "ja"] +``` + +Run the tool from the project root: + +```bash +cargo orthohelp --out-dir target/orthohelp --locale en-US +``` + +`--cache` reuses any previously generated IR cached under +`target/orthohelp//ir.json`, while `--no-build` skips the bridge build +and fails if the cache is missing. The generated per-locale JSON lives under +`/ir/.json` and is ready for downstream generators. + +### Generating man pages + +`cargo-orthohelp` can generate roff-formatted man pages from the localized IR. +Use `--format man` to produce `man/man/.` files suitable for +installation via `make install` or packaging: + +```bash +cargo orthohelp --format man --out-dir target/man --locale en-US +``` + +The generator produces standard man page sections in the canonical order: + +1. **NAME** – binary name and one-line description +2. **SYNOPSIS** – usage pattern with flags +3. **DESCRIPTION** – expanded about text +4. **OPTIONS** – CLI flags with types, defaults, and possible values +5. **ENVIRONMENT** – environment variables mapped to fields +6. **FILES** – configuration file paths and discovery locations +7. **PRECEDENCE** – source priority order (defaults → file → env → CLI) +8. **EXAMPLES** – usage examples from the IR +9. **SEE ALSO** – related commands and documentation links +10. **EXIT STATUS** – standard exit codes + +Additional options: + +- `--man-section ` – man page section number (default: 1) +- `--man-date ` – override the date shown in the footer +- `--man-split-subcommands` – generate separate man pages for each subcommand + +Text is automatically escaped for roff: backslashes are doubled, and leading +dashes, periods, and single quotes are escaped to prevent macro interpretation. +Enum fields list their possible values in the OPTIONS description. + +### Generating PowerShell help + +`cargo-orthohelp` can generate PowerShell external help in Microsoft Assistance +Markup Language (MAML) alongside a wrapper module so `Get-Help {BinName} -Full` +surfaces the same configuration metadata as the man page generator. Use the +`ps` format to emit the module layout under `powershell/`: + +```bash +cargo orthohelp --format ps --out-dir target/orthohelp --locale en-US +``` + +The generator produces: + +- `powershell//.psm1` – wrapper module. +- `powershell//.psd1` – module manifest. +- `powershell///-help.xml` – MAML help. +- `powershell///about_.help.txt` – about topic. + +`en-US` help is always generated. If only other locales are rendered, the +generator copies the first locale into `en-US` unless fallback generation is +disabled with `--ensure-en-us false`. + +PowerShell options: + +- `--ps-module-name ` – override the module name (defaults to the binary + name). +- `--ps-split-subcommands ` – emit wrapper functions for subcommands. +- `--ps-include-common-parameters ` – include CommonParameters in MAML. +- `--ps-help-info-uri ` – set `HelpInfoUri` for Update-Help payloads. +- `--ensure-en-us ` – control the `en-US` fallback behaviour. + +To set defaults in `Cargo.toml`, use the Windows metadata table: + +```toml +[package.metadata.ortho_config.windows] +module_name = "MyModule" +include_common_parameters = true +split_subcommands_into_functions = false +help_info_uri = "https://example.com/help/MyModule" +``` + ## Additional notes - **Vector merging** – For `Vec` fields the default merge strategy is @@ -1069,10 +1264,10 @@ fn interop(r: ortho_config::OrthoResult) -> Result while still requiring the CLI to provide a value when defaults are absent; see the `vk` example above. -- **Changing naming conventions** – Currently, only the default - snake/hyphenated (underscores → hyphens)/upper snake mappings are supported. - Future versions may introduce attributes such as `file_key` or `env` to - customize names further. +- **Changing naming conventions** – Runtime naming continues to use the + default snake/hyphenated (underscores → hyphens)/upper snake mappings. For + documentation output, use `env(name = "...")` and `file(key_path = "...")` to + override IR metadata without altering runtime behaviour. - **Testing** – Because the CLI and environment variables are merged at runtime, integration tests should set environment variables and construct CLI From e73f96dede9c857fca2a441cec1b411a9afe3e1a Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 9 Mar 2026 11:03:28 +0000 Subject: [PATCH 03/12] feat(cli): introduce typed CliConfig schema for layered config merging - 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] --- Cargo.lock | 11 +- Cargo.toml | 4 +- build.rs | 8 + docs/execplans/3-11-1-cli-config-struct.md | 73 ++- docs/netsuke-design.md | 74 +-- docs/roadmap.md | 6 +- docs/users-guide.md | 56 +++ src/cli/config.rs | 234 +++++++++ src/cli/merge.rs | 256 ++++++++++ src/cli/mod.rs | 469 +----------------- src/cli/parser.rs | 299 +++++++++++ src/cli/parsing.rs | 20 +- src/main.rs | 2 +- src/runner/mod.rs | 2 +- tests/bdd/fixtures/mod.rs | 4 + tests/bdd/steps/configuration_preferences.rs | 168 +++++++ tests/bdd/steps/locale_resolution.rs | 10 +- tests/bdd/steps/mod.rs | 1 + tests/cli_tests/merge.rs | 155 +++++- .../configuration_preferences.feature | 33 ++ 20 files changed, 1364 insertions(+), 521 deletions(-) create mode 100644 src/cli/config.rs create mode 100644 src/cli/merge.rs create mode 100644 src/cli/parser.rs create mode 100644 tests/bdd/steps/configuration_preferences.rs create mode 100644 tests/features/configuration_preferences.feature diff --git a/Cargo.lock b/Cargo.lock index 727c9106..9dc49fd6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -490,7 +490,7 @@ dependencies = [ "libc", "option-ext", "redox_users 0.5.2", - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -1511,11 +1511,12 @@ checksum = "04744f49eae99ab78e0d5c0b603ab218f515ea8cfe5a456d7629ad883a3b6e7d" [[package]] name = "ortho_config" -version = "0.7.0" +version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2ee9d60c6be312d7c6ce47b7146e3c9b8464c54fc304ef8f7656e29c268faeb1" +checksum = "9314c7a4be184287f3f9a66fcbcec054ac215d4975152df86b7aadeae8d5e019" dependencies = [ "camino", + "cap-std", "clap", "clap-dispatch", "directories", @@ -1537,9 +1538,9 @@ dependencies = [ [[package]] name = "ortho_config_macros" -version = "0.7.0" +version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "992d8fc0b33823adb993fe8968614ac0df3964d9e3564f7a147fd253e27a3acd" +checksum = "a3941ab7c592a0b93aadf12ce30e0ef3d01af87b46c8f363d8157dc33273ba83" dependencies = [ "heck 0.5.0", "proc-macro2", diff --git a/Cargo.toml b/Cargo.toml index 080d7015..dd06398f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,13 +52,13 @@ time = { version = "0.3.44", features = ["formatting", "macros", "parsing", "ser ureq = { version = "2.10.5" } wait-timeout = "0.2" url = "^2.5.0" -ortho_config = { version = "0.7.0", features = ["serde_json"] } +ortho_config = { version = "0.8.0", features = ["serde_json"] } sys-locale = "0.3.2" [build-dependencies] clap = { version = "4.5.0", features = ["derive"] } clap_mangen = "0.2.29" -ortho_config = { version = "0.7.0", features = ["serde_json"] } +ortho_config = { version = "0.8.0", features = ["serde_json"] } serde = { version = "1", features = ["derive"] } serde_json = { version = "1", features = ["preserve_order"] } thiserror = "1" diff --git a/build.rs b/build.rs index 30e5b77d..696f7eed 100644 --- a/build.rs +++ b/build.rs @@ -92,6 +92,9 @@ fn main() -> Result<(), Box> { // Exercise CLI localization, config merge, and host pattern symbols so the // shared modules remain linked when the build script is compiled without // tests. + const _: usize = std::mem::size_of::(); + const _: usize = std::mem::size_of::(); + const _: usize = std::mem::size_of::(); const _: usize = std::mem::size_of::(); const _: fn(&[OsString]) -> Option = cli::locale_hint_from_args; const _: fn(&[OsString]) -> Option = cli::diag_json_hint_from_args; @@ -100,11 +103,16 @@ fn main() -> Result<(), Box> { const _: fn(&cli::Cli, &ArgMatches) -> ortho_config::OrthoResult = cli::merge_with_config; const _: LocalizedParseFn = cli::parse_with_localizer_from; + const _: fn(&cli::Cli) -> Option = cli::Cli::no_emoji_override; + const _: fn(&cli::Cli) -> bool = cli::Cli::progress_enabled; const _: fn(&str) -> Result = HostPattern::parse; const _: fn(&HostPattern, host_pattern::HostCandidate<'_>) -> bool = HostPattern::matches; // Regenerate the manual page when the CLI or metadata changes. println!("cargo:rerun-if-changed=src/cli/mod.rs"); + println!("cargo:rerun-if-changed=src/cli/config.rs"); + println!("cargo:rerun-if-changed=src/cli/merge.rs"); + println!("cargo:rerun-if-changed=src/cli/parser.rs"); println!("cargo:rerun-if-changed=src/cli/parsing.rs"); println!("cargo:rerun-if-env-changed=CARGO_PKG_VERSION"); println!("cargo:rerun-if-env-changed=CARGO_PKG_NAME"); diff --git a/docs/execplans/3-11-1-cli-config-struct.md b/docs/execplans/3-11-1-cli-config-struct.md index 9bf0c456..547750fa 100644 --- a/docs/execplans/3-11-1-cli-config-struct.md +++ b/docs/execplans/3-11-1-cli-config-struct.md @@ -5,7 +5,7 @@ This ExecPlan (execution plan) is a living document. The sections `Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work proceeds. -Status: DRAFT +Status: COMPLETED No `PLANS.md` file exists in this repository. @@ -140,13 +140,16 @@ Observable success means: - [x] (2026-03-07) Reviewed roadmap `3.11.1`, the OrthoConfig guide, current CLI/config code, and nearby execplans. - [x] (2026-03-07) Drafted this ExecPlan. -- [ ] Approval received to implement. -- [ ] Stage A: split parser, config schema, and merge responsibilities. -- [ ] Stage B: introduce typed config groups and compatibility mapping. -- [ ] Stage C: wire global and subcommand merges through `CliConfig`. -- [ ] Stage D: add unit and behavioural coverage. -- [ ] Stage E: update user/design docs, mark roadmap item done, and run all - quality gates. +- [x] (2026-03-09) Continued implementation from the existing branch state. +- [x] (2026-03-09) Stage A: split parser, config schema, and merge + responsibilities. +- [x] (2026-03-09) Stage B: introduce typed config groups and compatibility + mapping. +- [x] (2026-03-09) Stage C: wire global and subcommand merges through + `CliConfig`. +- [x] (2026-03-09) Stage D: add unit and behavioural coverage. +- [x] (2026-03-09) Stage E: update user/design docs, mark roadmap item done, + and run all quality gates. ## Surprises & Discoveries @@ -164,6 +167,10 @@ Observable success means: - `rstest-bdd` feature-file edits may require touching [`tests/bdd_tests.rs`](/home/user/project/tests/bdd_tests.rs) to force Cargo to rebuild generated scenarios. +- The new configuration-preferences BDD coverage initially flaked only in the + full suite because `NETSUKE_CONFIG_PATH` is process-global. Holding + [`EnvLock`](test_support/src/env_lock.rs) for the whole scenario fixed the + race without weakening the coverage. ## Decision Log @@ -204,14 +211,60 @@ Observable success means: name, so `#[ortho_config(crate = "...")]` is not required unless that changes during implementation. Date/Author: 2026-03-08 (Codex, plan revision) +- Decision: keep `Cli` as the parser/runtime command carrier while moving all + layered schema responsibilities into `CliConfig`. Rationale: this achieved + the roadmap separation with a smaller, safer surface-area change while + preserving runner tests and command-dispatch call sites. Date/Author: + 2026-03-09 (Codex, implementation) + +- Decision: validate `output_format = "json"` as unsupported for now instead of + silently accepting it. Rationale: roadmap item `3.10.3` is still open, so + accepting JSON output configuration without delivering the behaviour would be + misleading. Date/Author: 2026-03-09 (Codex, implementation) + These decisions must be recorded in [`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md) during implementation if they remain unchanged after coding begins. ## Outcomes & Retrospective -Pending. This section must be replaced with the actual result, quality-gate -evidence, and lessons learned once implementation completes. +Completed on 2026-03-09. + +Implemented results: + +- Added [`CliConfig`](/home/user/project/src/cli/config.rs) as the + authoritative OrthoConfig-derived schema and split the CLI module into + parser, config, and merge submodules. +- Upgraded `ortho_config` to `0.8.0`. +- Kept `Cli` as the parser/runtime command carrier while rooting configuration + merge in `CliConfig`. +- Added typed config fields for `colour_policy`, `spinner_mode`, + `output_format`, and `theme`. +- Canonicalized `no_emoji = true` to the ASCII theme while rejecting + contradictory combinations. +- Wired `[cmds.build] targets` and `emit` defaults into the runtime build + command when the user does not supply explicit CLI values. +- Added unit coverage in + [`tests/cli_tests/merge.rs`](/home/user/project/tests/cli_tests/merge.rs) + plus behavioural coverage in + [`tests/features/configuration_preferences.feature`](/home/user/project/tests/features/configuration_preferences.feature). +- Updated the user guide, design document, and roadmap entry for `3.11.1`. + +Quality-gate evidence: + +- `make check-fmt` +- `make lint` +- `make test` +- `PATH="/root/.bun/bin:$PATH" make markdownlint` +- `make nixie` + +Lessons learned: + +- 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. ## Context and orientation diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index d5c9ff23..c8bb25cf 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2029,34 +2029,52 @@ the targets listed in the `defaults` section of the manifest are built. ### 8.4 Design Decisions -The CLI is implemented using clap's derive API in `src/cli/mod.rs`. Netsuke -applies `Cli::with_default_command` after parsing so invoking `netsuke` with no -explicit command still triggers a build. Configuration is layered with -OrthoConfig (defaults, configuration files, environment variables, then CLI -overrides) while treating clap defaults as absent so file or environment values -are not masked. Configuration discovery honours `NETSUKE_CONFIG_PATH` and the -standard OrthoConfig search order; environment variables use the `NETSUKE_` -prefix with `__` as a nesting separator. CLI help and clap errors are localized -via Fluent resources; locale resolution is handled in -`src/locale_resolution.rs` with the precedence `--locale` -> `NETSUKE_LOCALE` --> configuration `locale` -> system default. System locale strings are -normalized by stripping encoding suffixes (such as `.UTF-8`), removing variant -suffixes (such as `@latin`), and replacing underscores with hyphens before -validation. English plus Spanish catalogues ship in `locales/`; unsupported -locales fall back to `en-US`. Runtime diagnostics (for example manifest -parsing, stdlib template errors, and runner failures) use the same Fluent -localizer so the locale selection is consistent across user-facing output. A -build-time audit in `build.rs` validates that all referenced Fluent message -keys exist in the bundled catalogues, ensuring missing strings fail CI before -release. CLI execution and dispatch live in `src/runner.rs`, keeping `main.rs` -focused on parsing. Process management, Ninja invocation, argument redaction, -and the temporary file helpers reside in `src/runner/process.rs`, allowing the -runner entry point to delegate low-level concerns. The working directory flag -mirrors Ninja's `-C` option but is resolved internally: Netsuke runs Ninja with -a configured working directory and resolves relative output paths (for example -`build --emit` and `manifest`) under the same directory so behaviour matches a -real directory change. Error scenarios are validated using clap's `ErrorKind` -enumeration in unit tests and via Cucumber steps for behavioural coverage. +The CLI is implemented using clap's derive API in `src/cli/mod.rs`, but the +parser-facing `Cli` type is no longer the merge schema. Layered configuration +now lives in a dedicated `CliConfig` struct derived with OrthoConfig. This +separation keeps parsing, configuration discovery, and runtime command +selection as distinct concerns while preserving the existing command syntax. +Invoking `netsuke` with no explicit subcommand still resolves to `build`, and +the `build` command can now take default `emit` and `targets` values from +`[cmds.build]` in configuration files or `NETSUKE_CMDS__BUILD__*` environment +variables. Explicit CLI targets or `--emit` values still override those +defaults. + +Configuration is layered in the order defaults -> configuration files -> +environment variables -> CLI overrides. Discovery honours `NETSUKE_CONFIG_PATH` +and the standard OrthoConfig search order; environment variables use the +`NETSUKE_` prefix with `__` as a nesting separator. The schema now explicitly +covers verbosity, locale, accessible mode, progress, colour policy, spinner +mode, output format, theme selection, fetch policy, and build defaults. `theme` +is the canonical presentation setting; the older `no_emoji` field remains as a +compatibility alias that canonicalizes to the ASCII theme. Conflicting +combinations such as `theme = "unicode"` together with `no_emoji = true` fail +during merge. `spinner_mode` likewise validates against the legacy `progress` +boolean so contradictory inputs are rejected early. `output_format` is typed +now, but only `human` is accepted until the future JSON diagnostics milestone +lands. + +CLI help and clap errors are localized via Fluent resources; locale resolution +is handled in `src/locale_resolution.rs` with the precedence `--locale` -> +`NETSUKE_LOCALE` -> configuration `locale` -> system default. System locale +strings are normalized by stripping encoding suffixes (such as `.UTF-8`), +removing variant suffixes (such as `@latin`), and replacing underscores with +hyphens before validation. English plus Spanish catalogues ship in `locales/`; +unsupported locales fall back to `en-US`. Runtime diagnostics (for example +manifest parsing, stdlib template errors, and runner failures) use the same +Fluent localizer so the locale selection is consistent across user-facing +output. A build-time audit in `build.rs` validates that all referenced Fluent +message keys exist in the bundled catalogues, ensuring missing strings fail CI +before release. CLI execution and dispatch live in `src/runner.rs`, keeping +`main.rs` focused on parsing. Process management, Ninja invocation, argument +redaction, and the temporary file helpers reside in `src/runner/process.rs`, +allowing the runner entry point to delegate low-level concerns. The working +directory flag mirrors Ninja's `-C` option but is resolved internally: Netsuke +runs Ninja with a configured working directory and resolves relative output +paths (for example `build --emit` and `manifest`) under the same directory so +behaviour matches a real directory change. Error scenarios are validated using +clap's `ErrorKind` enumeration in unit tests and via Cucumber steps for +behavioural coverage. Real-time stage reporting now uses a six-stage model in `src/status.rs` backed by `indicatif::MultiProgress` for standard terminals. The reporter keeps one diff --git a/docs/roadmap.md b/docs/roadmap.md index d76d6e04..5a9c2ee1 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -282,11 +282,11 @@ library, and CLI ergonomics. ### 3.11. Configuration and preferences -- [ ] 3.11.1. Introduce `CliConfig` struct derived with `OrthoConfig`. See +- [x] 3.11.1. Introduce `CliConfig` struct derived with `OrthoConfig`. See [ortho-config-users-guide.md](ortho-config-users-guide.md). - - [ ] Share schema across Clap integration, configuration files, and + - [x] Share schema across Clap integration, configuration files, and environment variables. - - [ ] Cover verbosity, colour policy, locale, spinner mode, output format, + - [x] Cover verbosity, colour policy, locale, spinner mode, output format, default targets, and theme. - [ ] 3.11.2. Discover configuration files in project and user scopes. - [ ] Honour env overrides and CLI precedence. diff --git a/docs/users-guide.md b/docs/users-guide.md index caa36718..03e92c55 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -557,6 +557,62 @@ Environment variables use the `NETSUKE_` prefix (for example, `NETSUKE_JOBS=8`). Use `__` to separate nested keys when matching structured configuration. +The layered schema is rooted in `CliConfig`. Netsuke currently accepts these +top-level configuration keys: + +- `file = "Netsukefile"` +- `jobs = 8` +- `verbose = true|false` +- `locale = "en-US"` +- `accessible = true|false` +- `progress = true|false` +- `theme = "auto"|"unicode"|"ascii"` +- `no_emoji = true|false` +- `spinner_mode = "auto"|"enabled"|"disabled"` +- `colour_policy = "auto"|"always"|"never"` +- `output_format = "human"` + +Build-only defaults live under `[cmds.build]`: + +- `emit = "out.ninja"` +- `targets = ["hello"]` + +Example: + +```toml +verbose = true +locale = "es-ES" +colour_policy = "auto" +spinner_mode = "auto" +output_format = "human" +theme = "ascii" +progress = true +accessible = false + +[cmds.build] +targets = ["hello"] +``` + +`[cmds.build].targets` is used only when the user does not pass explicit build +targets on the command line. Explicit CLI targets always win. + +`theme` is the canonical presentation setting. `no_emoji = true` remains as a +compatibility alias and resolves to the ASCII theme. Conflicting settings such +as `theme = "unicode"` with `no_emoji = true` are rejected during configuration +merge. + +`spinner_mode = "disabled"` is equivalent to disabling progress output unless +the user explicitly sets `progress = true`, which is treated as a conflict. +Likewise, `spinner_mode = "enabled"` conflicts with `progress = false`. + +`output_format = "json"` is intentionally rejected for now. Roadmap item +`3.10.3` will add JSON diagnostics; until then the only supported value is +`"human"`. + +`colour_policy` is accepted and layered today so users can standardize their +preferred setting, but Netsuke does not yet emit coloured terminal output, so +this value currently has no visible effect. + Use `--locale `, `NETSUKE_LOCALE`, or a `locale = "..."` entry in a configuration file to select localized CLI copy and error messages. Locale precedence is: command-line flag, environment variable, configuration file, diff --git a/src/cli/config.rs b/src/cli/config.rs new file mode 100644 index 00000000..78eb6553 --- /dev/null +++ b/src/cli/config.rs @@ -0,0 +1,234 @@ +//! Layered CLI configuration schema. +//! +//! [`CliConfig`] is the single typed schema used for configuration discovery +//! and merging. It captures global CLI settings plus per-subcommand defaults +//! under the `cmds` namespace. + +use ortho_config::{OrthoConfig, OrthoError, OrthoResult, PostMergeContext, PostMergeHook}; +use serde::{Deserialize, Serialize}; +use std::path::PathBuf; +use std::sync::Arc; + +use crate::host_pattern::HostPattern; + +/// Colour-output policy accepted by layered configuration. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "kebab-case")] +pub enum ColourPolicy { + /// Follow the host environment. + #[default] + Auto, + /// Force colour output on when available. + Always, + /// Force colour output off. + Never, +} + +/// Spinner and progress rendering policy. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "kebab-case")] +pub enum SpinnerMode { + /// Follow Netsuke's default progress behaviour. + #[default] + Auto, + /// Force progress summaries on. + Enabled, + /// Disable progress summaries. + Disabled, +} + +/// Top-level diagnostics and output format. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "kebab-case")] +pub enum OutputFormat { + /// Human-readable terminal output. + #[default] + Human, + /// Machine-readable JSON diagnostics. + Json, +} + +/// Presentation theme for semantic prefixes and glyph choices. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[serde(rename_all = "kebab-case")] +pub enum Theme { + /// Follow the host environment. + #[default] + Auto, + /// Prefer the Unicode/emoji presentation. + Unicode, + /// Prefer ASCII-only output. + Ascii, +} + +/// Layered defaults for the `build` subcommand. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] +pub struct BuildConfig { + /// Optional default path for the emitted Ninja manifest. + pub emit: Option, + /// Default targets used when the user does not pass any targets. + #[serde(default)] + pub targets: Vec, +} + +/// Subcommand-specific layered defaults. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] +pub struct CommandConfigs { + /// Configuration that applies only to the `build` subcommand. + #[serde(default)] + pub build: BuildConfig, +} + +/// Authoritative schema for layered CLI configuration. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, OrthoConfig)] +#[ortho_config(prefix = "NETSUKE", post_merge_hook)] +pub struct CliConfig { + /// Path to the Netsuke manifest file to use. + #[ortho_config(default = default_manifest_path())] + pub file: PathBuf, + + /// Set the number of parallel build jobs. + pub jobs: Option, + + /// Enable verbose diagnostic logging and completion timing summaries. + #[ortho_config(default = false)] + pub verbose: bool, + + /// Locale tag for CLI copy (for example: en-US, es-ES). + pub locale: Option, + + /// Additional URL schemes allowed for the `fetch` helper. + #[ortho_config(merge_strategy = "append")] + #[serde(default)] + pub fetch_allow_scheme: Vec, + + /// Hostnames permitted when default deny is enabled. + #[ortho_config(merge_strategy = "append")] + #[serde(default)] + pub fetch_allow_host: Vec, + + /// Hostnames that are always blocked. + #[ortho_config(merge_strategy = "append")] + #[serde(default)] + pub fetch_block_host: Vec, + + /// Deny all hosts by default; only allow the declared allowlist. + #[ortho_config(default = false)] + pub fetch_default_deny: bool, + + /// Force accessible output mode on or off. + pub accessible: Option, + + /// Compatibility alias for requesting the ASCII theme. + pub no_emoji: Option, + + /// Emit machine-readable diagnostics in JSON on stderr. + #[ortho_config(default = false)] + pub diag_json: bool, + + /// Force progress summaries on or off. + pub progress: Option, + + /// Preferred colour policy. + #[ortho_config(skip_cli)] + pub colour_policy: Option, + + /// Preferred spinner or progress mode. + #[ortho_config(skip_cli)] + pub spinner_mode: Option, + + /// Preferred diagnostics/output format. + #[ortho_config(skip_cli)] + pub output_format: Option, + + /// Preferred terminal theme. + #[ortho_config(skip_cli)] + pub theme: Option, + + /// Per-subcommand defaults. + #[ortho_config(skip_cli)] + #[serde(default)] + pub cmds: CommandConfigs, +} + +impl Default for CliConfig { + fn default() -> Self { + Self { + file: default_manifest_path(), + jobs: None, + verbose: false, + locale: None, + fetch_allow_scheme: Vec::new(), + fetch_allow_host: Vec::new(), + fetch_block_host: Vec::new(), + fetch_default_deny: false, + accessible: None, + no_emoji: None, + diag_json: false, + progress: None, + colour_policy: None, + spinner_mode: None, + output_format: None, + theme: None, + cmds: CommandConfigs::default(), + } + } +} + +impl PostMergeHook for CliConfig { + fn post_merge(&mut self, _ctx: &PostMergeContext) -> OrthoResult<()> { + validate_theme_compatibility(self)?; + validate_spinner_mode_compatibility(self)?; + validate_output_format_support(self)?; + Ok(()) + } +} + +fn default_manifest_path() -> PathBuf { + PathBuf::from("Netsukefile") +} + +fn validate_theme_compatibility(config: &CliConfig) -> OrthoResult<()> { + match (config.theme, config.no_emoji) { + (Some(Theme::Unicode), Some(true)) => Err(validation_error( + "theme", + "theme = \"unicode\" conflicts with no_emoji = true; use theme = \"ascii\" instead", + )), + (Some(Theme::Ascii), Some(false)) => Err(validation_error( + "no_emoji", + "no_emoji = false conflicts with theme = \"ascii\"; remove the alias or choose theme = \"unicode\"", + )), + _ => Ok(()), + } +} + +fn validate_spinner_mode_compatibility(config: &CliConfig) -> OrthoResult<()> { + match (config.spinner_mode, config.progress) { + (Some(SpinnerMode::Disabled), Some(true)) => Err(validation_error( + "spinner_mode", + "spinner_mode = \"disabled\" conflicts with progress = true", + )), + (Some(SpinnerMode::Enabled), Some(false)) => Err(validation_error( + "progress", + "progress = false conflicts with spinner_mode = \"enabled\"", + )), + _ => Ok(()), + } +} + +fn validate_output_format_support(config: &CliConfig) -> OrthoResult<()> { + if matches!(config.output_format, Some(OutputFormat::Json)) { + return Err(validation_error( + "output_format", + "output_format = \"json\" is not supported yet; use \"human\" for this milestone", + )); + } + Ok(()) +} + +fn validation_error(key: &str, message: &str) -> Arc { + Arc::new(OrthoError::Validation { + key: key.to_owned(), + message: message.to_owned(), + }) +} diff --git a/src/cli/merge.rs b/src/cli/merge.rs new file mode 100644 index 00000000..f5e1d23d --- /dev/null +++ b/src/cli/merge.rs @@ -0,0 +1,256 @@ +//! Layer-composition and conversion helpers for CLI configuration. + +use clap::ArgMatches; +use clap::parser::ValueSource; +use ortho_config::declarative::LayerComposition; +use ortho_config::figment::{Figment, providers::Env}; +use ortho_config::uncased::Uncased; +use ortho_config::{ + ConfigDiscovery, MergeComposer, OrthoError, OrthoMergeExt, OrthoResult, sanitize_value, +}; +use serde::Serialize; +use serde_json::{Map, Value, json}; +use std::path::PathBuf; +use std::sync::Arc; + +use super::config::{BuildConfig, CliConfig, Theme}; +use super::parser::{BuildArgs, Cli, Commands}; +const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG_PATH"; +const ENV_PREFIX: &str = "NETSUKE_"; + +/// Merge discovered configuration layers over parsed CLI input. +/// +/// # Errors +/// +/// Returns an [`ortho_config::OrthoError`] if layer composition or merging +/// fails. +pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult { + let mut errors = Vec::new(); + let mut composer = MergeComposer::with_capacity(4); + + match sanitize_value(&CliConfig::default()) { + Ok(value) => composer.push_defaults(value), + Err(err) => errors.push(err), + } + + let discovery = config_discovery(cli.directory.as_ref()); + let mut file_layers = discovery.compose_layers(); + errors.append(&mut file_layers.required_errors); + if file_layers.value.is_empty() { + errors.append(&mut file_layers.optional_errors); + } + for layer in file_layers.value { + composer.push_layer(layer); + } + + let env_provider = env_provider() + .map(|key| Uncased::new(key.as_str().to_ascii_uppercase())) + .split("__"); + match Figment::from(env_provider) + .extract::() + .into_ortho_merge() + { + Ok(value) => composer.push_environment(value), + Err(err) => errors.push(err), + } + + match cli_overrides_from_matches(cli, matches) { + Ok(value) if !is_empty_value(&value) => composer.push_cli(value), + Ok(_) => {} + Err(err) => errors.push(err), + } + + let composition = LayerComposition::new(composer.layers(), errors); + let merged = composition.into_merge_result(CliConfig::merge_from_layers)?; + Ok(apply_config(cli, merged)) +} + +fn env_provider() -> Env { + Env::prefixed(ENV_PREFIX) +} + +fn config_discovery(directory: Option<&PathBuf>) -> ConfigDiscovery { + let mut builder = ConfigDiscovery::builder("netsuke").env_var(CONFIG_ENV_VAR); + if let Some(dir) = directory { + builder = builder.clear_project_roots().add_project_root(dir); + } + builder.build() +} + +fn is_empty_value(value: &Value) -> bool { + matches!(value, Value::Object(map) if map.is_empty()) +} + +fn diag_json_from_layer(value: &Value) -> Option { + value + .as_object() + .and_then(|map| map.get("diag_json")) + .and_then(Value::as_bool) +} + +/// Resolve the effective diagnostic JSON preference from the raw config layers. +/// +/// This is used before full config merging so startup and merge-time failures +/// can still honour `diag_json` values sourced from config files or the +/// environment. +#[must_use] +pub fn resolve_merged_diag_json(cli: &Cli, matches: &ArgMatches) -> bool { + let mut diag_json = CliConfig::default().diag_json; + + let discovery = config_discovery(cli.directory.as_ref()); + let file_layers = discovery.compose_layers(); + for layer in file_layers.value { + let layer_value = layer.into_value(); + if let Some(layer_diag_json) = diag_json_from_layer(&layer_value) { + diag_json = layer_diag_json; + } + } + + let env_provider = env_provider() + .map(|key| Uncased::new(key.as_str().to_ascii_uppercase())) + .split("__"); + if let Ok(value) = Figment::from(env_provider).extract::() + && let Some(env_diag_json) = diag_json_from_layer(&value) + { + diag_json = env_diag_json; + } + + if matches.value_source("diag_json") == Some(ValueSource::CommandLine) { + cli.diag_json + } else { + diag_json + } +} + +fn cli_overrides_from_matches(cli: &Cli, matches: &ArgMatches) -> OrthoResult { + 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)?; + maybe_insert_explicit(matches, "diag_json", &cli.diag_json, &mut root)?; + + if let Some(Commands::Build(args)) = cli.command.as_ref() + && let Some(build_matches) = matches.subcommand_matches("build") + { + let build = build_cli_overrides(args, build_matches)?; + if !build.is_empty() { + root.insert("cmds".to_owned(), json!({ "build": Value::Object(build) })); + } + } + + Ok(Value::Object(root)) +} + +fn build_cli_overrides(args: &BuildArgs, matches: &ArgMatches) -> OrthoResult> { + let mut build = Map::new(); + maybe_insert_explicit(matches, "emit", &args.emit, &mut build)?; + maybe_insert_explicit(matches, "targets", &args.targets, &mut build)?; + Ok(build) +} + +fn maybe_insert_explicit( + matches: &ArgMatches, + field: &str, + value: &T, + target: &mut Map, +) -> OrthoResult<()> +where + T: Serialize, +{ + if matches.value_source(field) == Some(ValueSource::CommandLine) { + target.insert(field.to_owned(), serialize_value(field, value)?); + } + Ok(()) +} + +fn serialize_value(field: &str, value: &T) -> OrthoResult +where + T: Serialize, +{ + serde_json::to_value(value).map_err(|err| validation_error(field, &err.to_string())) +} + +fn apply_config(parsed: &Cli, config: CliConfig) -> Cli { + Cli { + file: config.file, + directory: parsed.directory.clone(), + jobs: config.jobs, + verbose: config.verbose, + locale: config.locale, + fetch_allow_scheme: config.fetch_allow_scheme, + fetch_allow_host: config.fetch_allow_host, + fetch_block_host: config.fetch_block_host, + fetch_default_deny: config.fetch_default_deny, + accessible: config.accessible, + no_emoji: config.no_emoji, + diag_json: config.diag_json, + progress: config.progress, + colour_policy: config.colour_policy, + spinner_mode: config.spinner_mode, + output_format: config.output_format, + theme: canonical_theme(config.theme, config.no_emoji), + command: Some(resolve_command(parsed.command.as_ref(), &config.cmds.build)), + } +} + +fn resolve_command(parsed: Option<&Commands>, build_defaults: &BuildConfig) -> Commands { + match parsed { + Some(Commands::Build(args)) => Commands::Build(BuildArgs { + emit: args.emit.clone().or_else(|| build_defaults.emit.clone()), + targets: if args.targets.is_empty() { + build_defaults.targets.clone() + } else { + args.targets.clone() + }, + }), + Some(other) => other.clone(), + None => Commands::Build(BuildArgs { + emit: build_defaults.emit.clone(), + targets: build_defaults.targets.clone(), + }), + } +} + +const fn canonical_theme(theme: Option, no_emoji: Option) -> Option { + match (theme, no_emoji) { + (Some(value), _) => Some(value), + (None, Some(true)) => Some(Theme::Ascii), + _ => None, + } +} + +fn validation_error(key: &str, message: &str) -> Arc { + Arc::new(OrthoError::Validation { + key: key.to_owned(), + message: message.to_owned(), + }) +} diff --git a/src/cli/mod.rs b/src/cli/mod.rs index d9ab7a26..142168ee 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -1,457 +1,18 @@ -//! Command line interface definition using clap. +//! Command-line parsing plus layered CLI configuration support. //! -//! This module defines the [`Cli`] structure and its subcommands. -//! It mirrors the design described in `docs/netsuke-design.md`. - -use clap::builder::{TypedValueParser, ValueParser}; -use clap::error::ErrorKind; -use clap::parser::ValueSource; -use clap::{ArgMatches, Args, CommandFactory, FromArgMatches, Parser, Subcommand}; -use ortho_config::declarative::LayerComposition; -use ortho_config::figment::{Figment, providers::Env}; -use ortho_config::localize_clap_error_with_command; -use ortho_config::uncased::Uncased; -use ortho_config::{ - ConfigDiscovery, LocalizationArgs, Localizer, MergeComposer, OrthoConfig, OrthoMergeExt, - OrthoResult, sanitize_value, -}; -use serde::{Deserialize, Serialize}; -use std::ffi::OsString; -use std::path::PathBuf; -use std::sync::Arc; - -use crate::cli_l10n::localize_command; -use crate::host_pattern::HostPattern; -use crate::localization::{self, keys}; +//! The parser-facing [`Cli`] type remains responsible for user-facing command +//! syntax, while [`CliConfig`] is the authoritative OrthoConfig-derived schema +//! used to merge defaults, configuration files, environment variables, and CLI +//! overrides into the runtime shape consumed by the runner. + +mod config; +mod merge; +mod parser; mod parsing; -use parsing::{parse_host_pattern, parse_jobs, parse_locale, parse_scheme}; - -/// Maximum number of jobs accepted by the CLI. -const MAX_JOBS: usize = 64; -const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG_PATH"; -const ENV_PREFIX: &str = "NETSUKE_"; - -#[derive(Clone)] -struct LocalizedValueParser { - localizer: Arc, - parser: F, -} - -impl LocalizedValueParser { - fn new(localizer: Arc, parser: F) -> Self { - Self { localizer, parser } - } -} - -impl TypedValueParser for LocalizedValueParser -where - F: Fn(&dyn Localizer, &str) -> Result + Clone + Send + Sync + 'static, - T: Send + Sync + Clone + 'static, -{ - type Value = T; - - fn parse_ref( - &self, - cmd: &clap::Command, - _arg: Option<&clap::Arg>, - value: &std::ffi::OsStr, - ) -> Result { - let mut command = cmd.clone(); - let Some(raw_value) = value.to_str() else { - return Err(command.error(ErrorKind::InvalidUtf8, "invalid UTF-8")); - }; - (self.parser)(self.localizer.as_ref(), raw_value) - .map_err(|err| command.error(ErrorKind::ValueValidation, err)) - } -} - -fn validation_message( - localizer: &dyn Localizer, - key: &'static str, - args: Option<&LocalizationArgs<'_>>, - fallback: &str, -) -> String { - localizer.message(key, args, fallback) -} - -/// A modern, friendly build system that uses YAML and Jinja, powered by Ninja. -#[derive(Debug, Parser, Serialize, Deserialize, OrthoConfig)] -#[command(author, version, about, long_about = None)] -#[ortho_config(prefix = "NETSUKE")] -pub struct Cli { - /// Path to the Netsuke manifest file to use. - #[arg(short, long, value_name = "FILE", default_value = "Netsukefile")] - #[ortho_config(default = default_manifest_path())] - pub file: PathBuf, - - /// Run as if started in this directory. - /// - /// This affects manifest lookup, output paths, and config discovery. - #[arg(short = 'C', long, value_name = "DIR")] - pub directory: Option, - - /// Set the number of parallel build jobs. - /// - /// Values must be between 1 and 64. - #[arg(short, long, value_name = "N")] - pub jobs: Option, - - /// Enable verbose diagnostic logging and completion timing summaries. - #[arg(short, long)] - #[ortho_config(default = false)] - pub verbose: bool, - - /// Locale tag for CLI copy (for example: en-US, es-ES). - #[arg(long, value_name = "LOCALE")] - pub locale: Option, - - /// Additional URL schemes allowed for the `fetch` helper. - #[arg(long = "fetch-allow-scheme", value_name = "SCHEME")] - #[ortho_config(merge_strategy = "append")] - pub fetch_allow_scheme: Vec, - - /// Hostnames that are permitted when default deny is enabled. - /// - /// Supports wildcards such as `*.example.com`. - #[arg(long = "fetch-allow-host", value_name = "HOST")] - #[ortho_config(merge_strategy = "append")] - pub fetch_allow_host: Vec, - - /// Hostnames that are always blocked, even when allowed elsewhere. - /// - /// Supports wildcards such as `*.example.com`. - #[arg(long = "fetch-block-host", value_name = "HOST")] - #[ortho_config(merge_strategy = "append")] - pub fetch_block_host: Vec, - - /// Deny all hosts by default; only allow the declared allowlist. - #[arg(long = "fetch-default-deny")] - #[ortho_config(default = false)] - pub fetch_default_deny: bool, - - /// Force accessible output mode on or off (overrides auto-detection). - #[arg(long)] - pub accessible: Option, - - /// Suppress emoji glyphs in output (overrides auto-detection). - #[arg(long)] - pub no_emoji: Option, - - /// Emit machine-readable diagnostics in JSON on stderr. - #[arg(long)] - #[ortho_config(default = false)] - pub diag_json: bool, - - /// Force standard progress summaries on or off. - /// - /// When omitted, Netsuke enables progress summaries in standard mode. - #[arg(long)] - pub progress: Option, - - /// Optional subcommand to execute; defaults to `build` when omitted. - /// - /// `OrthoConfig` merging ignores this field; CLI parsing supplies it. - #[serde(skip)] - #[command(subcommand)] - #[ortho_config(skip_cli)] - pub command: Option, -} - -impl Cli { - /// Apply the default command if none was specified. - #[must_use] - pub fn with_default_command(mut self) -> Self { - if self.command.is_none() { - self.command = Some(Commands::Build(BuildArgs { - emit: None, - targets: Vec::new(), - })); - } - self - } -} - -impl Default for Cli { - fn default() -> Self { - Self { - file: default_manifest_path(), - directory: None, - jobs: None, - verbose: false, - locale: None, - fetch_allow_scheme: Vec::new(), - fetch_allow_host: Vec::new(), - fetch_block_host: Vec::new(), - fetch_default_deny: false, - accessible: None, - progress: None, - no_emoji: None, - diag_json: false, - command: None, - } - .with_default_command() - } -} - -/// Arguments accepted by the `build` command. -#[derive(Debug, Args, PartialEq, Eq, Clone, Serialize, Deserialize)] -pub struct BuildArgs { - /// Write the generated Ninja manifest to this path and retain it. - #[arg(long, value_name = "FILE")] - pub emit: Option, - - /// A list of specific targets to build. - #[serde(default)] - pub targets: Vec, -} - -/// Available top-level commands for Netsuke. -#[derive(Debug, Subcommand, PartialEq, Eq, Clone, Serialize, Deserialize)] -#[serde(rename_all = "kebab-case")] -pub enum Commands { - /// Build specified targets (or default targets if none are given). - Build(BuildArgs), - - /// Remove build artefacts and intermediate files. - Clean, - - /// Display the build dependency graph in DOT format for visualisation. - Graph, - - /// Write the Ninja manifest to the specified file without invoking Ninja. - Manifest { - /// Output path for the generated Ninja file. - /// - /// Use `-` to write to stdout. - #[arg(value_name = "FILE")] - file: PathBuf, - }, -} - -/// Return the default manifest filename when none is provided. -fn default_manifest_path() -> PathBuf { - PathBuf::from("Netsukefile") -} - -/// Inspect raw arguments and extract the requested locale before full parsing. -#[must_use] -pub fn locale_hint_from_args(args: &[OsString]) -> Option { - crate::cli_l10n::locale_hint_from_args(args) -} - -/// Inspect raw arguments and extract the requested `--diag-json` state. -#[must_use] -pub fn diag_json_hint_from_args(args: &[OsString]) -> Option { - crate::cli_l10n::diag_json_hint_from_args(args) -} - -/// Parse CLI arguments with localized clap output. -/// -/// Returns both the parsed CLI struct and the `ArgMatches` required for -/// configuration merging. -/// -/// # Errors -/// -/// Returns a `clap::Error` with localization applied when parsing fails. -pub fn parse_with_localizer_from( - iter: I, - localizer: &Arc, -) -> Result<(Cli, ArgMatches), clap::Error> -where - I: IntoIterator, - T: Into + Clone, -{ - let mut command = localize_command(Cli::command(), localizer.as_ref()); - command = configure_validation_parsers(command, localizer); - let matches = command - .try_get_matches_from_mut(iter) - .map_err(|err| localize_clap_error_with_command(err, localizer.as_ref(), Some(&command)))?; - // Clone matches before from_arg_matches_mut consumes the values. - let matches_for_merge = matches.clone(); - let mut matches_for_parse = matches; - let cli = Cli::from_arg_matches_mut(&mut matches_for_parse).map_err(|clap_err| { - let with_cmd = clap_err.with_cmd(&command); - localize_clap_error_with_command(with_cmd, localizer.as_ref(), Some(&command)) - })?; - Ok((cli, matches_for_merge)) -} - -/// Return the prefixed environment provider for CLI configuration. -fn env_provider() -> Env { - Env::prefixed(ENV_PREFIX) -} - -/// Build configuration discovery rooted in the optional working directory. -fn config_discovery(directory: Option<&PathBuf>) -> ConfigDiscovery { - let mut builder = ConfigDiscovery::builder("netsuke").env_var(CONFIG_ENV_VAR); - if let Some(dir) = directory { - builder = builder.clear_project_roots().add_project_root(dir); - } - builder.build() -} - -/// Return `true` when no CLI overrides were supplied. -/// -/// The merge pipeline treats an empty JSON object as "no overrides". -fn is_empty_value(value: &serde_json::Value) -> bool { - matches!(value, serde_json::Value::Object(map) if map.is_empty()) -} - -fn diag_json_from_layer(value: &serde_json::Value) -> Option { - value - .as_object() - .and_then(|map| map.get("diag_json")) - .and_then(serde_json::Value::as_bool) -} - -/// Resolve the effective diagnostic JSON preference from the raw config layers. -/// -/// This is used before full config merging so startup and merge-time failures -/// can still honour `diag_json` values sourced from config files or the -/// environment. -#[must_use] -pub fn resolve_merged_diag_json(cli: &Cli, matches: &ArgMatches) -> bool { - let mut diag_json = Cli::default().diag_json; - - let discovery = config_discovery(cli.directory.as_ref()); - let file_layers = discovery.compose_layers(); - for layer in file_layers.value { - let layer_value = layer.into_value(); - if let Some(layer_diag_json) = diag_json_from_layer(&layer_value) { - diag_json = layer_diag_json; - } - } - - let env_provider = env_provider() - .map(|key| Uncased::new(key.as_str().to_ascii_uppercase())) - .split("__"); - if let Ok(value) = Figment::from(env_provider).extract::() - && let Some(env_diag_json) = diag_json_from_layer(&value) - { - diag_json = env_diag_json; - } - - if matches.value_source("diag_json") == Some(ValueSource::CommandLine) { - cli.diag_json - } else { - diag_json - } -} - -fn cli_overrides_from_matches(cli: &Cli, matches: &ArgMatches) -> OrthoResult { - let value = sanitize_value(cli)?; - let mut map = match value { - serde_json::Value::Object(map) => map, - other => { - let mut args = LocalizationArgs::default(); - args.insert("value", format!("{other:?}").into()); - let localizer = localization::localizer(); - return Err(Arc::new(ortho_config::OrthoError::Validation { - key: String::from("cli"), - message: validation_message( - localizer.as_ref(), - keys::CLI_CONFIG_EXPECTED_OBJECT, - Some(&args), - &format!("expected parsed CLI values to serialize to an object, got {other:?}"), - ), - })); - } - }; - - map.remove("command"); - for field in [ - "file", - "verbose", - "fetch_default_deny", - "fetch_allow_scheme", - "fetch_allow_host", - "fetch_block_host", - "accessible", - "progress", - "no_emoji", - "diag_json", - ] { - if matches.value_source(field) != Some(ValueSource::CommandLine) { - map.remove(field); - } - } - - Ok(serde_json::Value::Object(map)) -} - -fn configure_validation_parsers( - mut command: clap::Command, - localizer: &Arc, -) -> clap::Command { - let jobs_parser = LocalizedValueParser::new(Arc::clone(localizer), parse_jobs); - let locale_parser = LocalizedValueParser::new(Arc::clone(localizer), parse_locale); - let scheme_parser = LocalizedValueParser::new(Arc::clone(localizer), parse_scheme); - let host_parser = LocalizedValueParser::new(Arc::clone(localizer), parse_host_pattern); - - command = command.mut_arg("jobs", |arg| { - arg.value_parser(ValueParser::new(jobs_parser)) - }); - command = command.mut_arg("locale", |arg| { - arg.value_parser(ValueParser::new(locale_parser)) - }); - command = command.mut_arg("fetch_allow_scheme", |arg| { - arg.value_parser(ValueParser::new(scheme_parser.clone())) - }); - command = command.mut_arg("fetch_allow_host", |arg| { - arg.value_parser(ValueParser::new(host_parser.clone())) - }); - command = command.mut_arg("fetch_block_host", |arg| { - arg.value_parser(ValueParser::new(host_parser)) - }); - command -} - -/// Merge configuration layers over the parsed CLI values. -/// -/// # Errors -/// -/// Returns an [`ortho_config::OrthoError`] if layer composition or merging -/// fails. -pub fn merge_with_config(cli: &Cli, matches: &ArgMatches) -> OrthoResult { - let command = cli.command.clone(); - let mut errors = Vec::new(); - let mut composer = MergeComposer::with_capacity(4); - - match sanitize_value(&Cli::default()) { - Ok(value) => composer.push_defaults(value), - Err(err) => errors.push(err), - } - - let discovery = config_discovery(cli.directory.as_ref()); - let mut file_layers = discovery.compose_layers(); - errors.append(&mut file_layers.required_errors); - if file_layers.value.is_empty() { - errors.append(&mut file_layers.optional_errors); - } - for layer in file_layers.value { - composer.push_layer(layer); - } - - let env_provider = env_provider() - .map(|key| Uncased::new(key.as_str().to_ascii_uppercase())) - .split("__"); - match Figment::from(env_provider) - .extract::() - .into_ortho_merge() - { - Ok(value) => composer.push_environment(value), - Err(err) => errors.push(err), - } - - match cli_overrides_from_matches(cli, matches) { - Ok(value) if !is_empty_value(&value) => composer.push_cli(value), - Ok(_) => {} - Err(err) => errors.push(err), - } - - let composition = LayerComposition::new(composer.layers(), errors); - let mut merged = composition.into_merge_result(Cli::merge_from_layers)?; - merged.command = command; - Ok(merged) -} +pub use config::{CliConfig, ColourPolicy, OutputFormat, SpinnerMode, Theme}; +pub use merge::{merge_with_config, resolve_merged_diag_json}; +pub use parser::{ + BuildArgs, Cli, Commands, diag_json_hint_from_args, locale_hint_from_args, + parse_with_localizer_from, +}; diff --git a/src/cli/parser.rs b/src/cli/parser.rs new file mode 100644 index 00000000..d2faaaaf --- /dev/null +++ b/src/cli/parser.rs @@ -0,0 +1,299 @@ +//! Clap-facing parser types and localized parsing helpers. + +use clap::builder::{TypedValueParser, ValueParser}; +use clap::error::ErrorKind; +use clap::{ArgMatches, Args, CommandFactory, FromArgMatches, Parser, Subcommand}; +use ortho_config::localize_clap_error_with_command; +use ortho_config::{LocalizationArgs, Localizer}; +use serde::{Deserialize, Serialize}; +use std::ffi::OsString; +use std::path::PathBuf; +use std::sync::Arc; + +use super::parsing::{parse_host_pattern, parse_jobs, parse_locale, parse_scheme}; +use super::{ColourPolicy, OutputFormat, SpinnerMode, Theme}; +pub use crate::cli_l10n::{diag_json_hint_from_args, locale_hint_from_args}; +use crate::cli_l10n::localize_command; +use crate::host_pattern::HostPattern; + +#[derive(Clone)] +struct LocalizedValueParser { + localizer: Arc, + parser: F, +} + +impl LocalizedValueParser { + fn new(localizer: Arc, parser: F) -> Self { + Self { localizer, parser } + } +} + +impl TypedValueParser for LocalizedValueParser +where + F: Fn(&dyn Localizer, &str) -> Result + Clone + Send + Sync + 'static, + T: Send + Sync + Clone + 'static, +{ + type Value = T; + + fn parse_ref( + &self, + cmd: &clap::Command, + _arg: Option<&clap::Arg>, + value: &std::ffi::OsStr, + ) -> Result { + let mut command = cmd.clone(); + let Some(raw_value) = value.to_str() else { + return Err(command.error(ErrorKind::InvalidUtf8, "invalid UTF-8")); + }; + (self.parser)(self.localizer.as_ref(), raw_value) + .map_err(|err| command.error(ErrorKind::ValueValidation, err)) + } +} + +pub(super) fn validation_message( + localizer: &dyn Localizer, + key: &'static str, + args: Option<&LocalizationArgs<'_>>, + fallback: &str, +) -> String { + localizer.message(key, args, fallback) +} + +/// A modern, friendly build system that uses YAML and Jinja, powered by Ninja. +#[derive(Debug, Parser, Serialize, Deserialize)] +#[command(author, version, about, long_about = None)] +pub struct Cli { + /// Path to the Netsuke manifest file to use. + #[arg(short, long, value_name = "FILE", default_value = "Netsukefile")] + pub file: PathBuf, + + /// Run as if started in this directory. + /// + /// This affects manifest lookup, output paths, and config discovery. + #[arg(short = 'C', long, value_name = "DIR")] + pub directory: Option, + + /// Set the number of parallel build jobs. + /// + /// Values must be between 1 and 64. + #[arg(short, long, value_name = "N")] + pub jobs: Option, + + /// Enable verbose diagnostic logging and completion timing summaries. + #[arg(short, long)] + pub verbose: bool, + + /// Locale tag for CLI copy (for example: en-US, es-ES). + #[arg(long, value_name = "LOCALE")] + pub locale: Option, + + /// Additional URL schemes allowed for the `fetch` helper. + #[arg(long = "fetch-allow-scheme", value_name = "SCHEME")] + pub fetch_allow_scheme: Vec, + + /// Hostnames that are permitted when default deny is enabled. + /// + /// Supports wildcards such as `*.example.com`. + #[arg(long = "fetch-allow-host", value_name = "HOST")] + pub fetch_allow_host: Vec, + + /// Hostnames that are always blocked, even when allowed elsewhere. + /// + /// Supports wildcards such as `*.example.com`. + #[arg(long = "fetch-block-host", value_name = "HOST")] + pub fetch_block_host: Vec, + + /// Deny all hosts by default; only allow the declared allowlist. + #[arg(long = "fetch-default-deny")] + pub fetch_default_deny: bool, + + /// Force accessible output mode on or off (overrides auto-detection). + #[arg(long)] + pub accessible: Option, + + /// Suppress emoji glyphs in output (overrides auto-detection). + #[arg(long)] + pub no_emoji: Option, + + /// Emit machine-readable diagnostics in JSON on stderr. + #[arg(long)] + pub diag_json: bool, + + /// Force standard progress summaries on or off. + /// + /// When omitted, Netsuke enables progress summaries in standard mode. + #[arg(long)] + pub progress: Option, + + /// Resolved colour policy from layered configuration. + #[arg(skip)] + pub colour_policy: Option, + + /// Resolved spinner mode from layered configuration. + #[arg(skip)] + pub spinner_mode: Option, + + /// Resolved output format from layered configuration. + #[arg(skip)] + pub output_format: Option, + + /// Resolved presentation theme from layered configuration. + #[arg(skip)] + pub theme: Option, + + /// Optional subcommand to execute; defaults to `build` when omitted. + #[serde(skip)] + #[command(subcommand)] + pub command: Option, +} + +impl Cli { + /// Apply the default command if none was specified. + #[must_use] + pub fn with_default_command(mut self) -> Self { + if self.command.is_none() { + self.command = Some(Commands::Build(BuildArgs::default())); + } + self + } + + /// Return the effective emoji override for output preference resolution. + #[must_use] + pub const fn no_emoji_override(&self) -> Option { + if matches!(self.theme, Some(Theme::Ascii)) || matches!(self.no_emoji, Some(true)) { + Some(true) + } else { + self.no_emoji + } + } + + /// Return whether progress summaries should be enabled. + #[must_use] + pub const fn progress_enabled(&self) -> bool { + match (self.progress, self.spinner_mode) { + (Some(value), _) => value, + (None, Some(SpinnerMode::Disabled)) => false, + _ => true, + } + } +} + +impl Default for Cli { + fn default() -> Self { + Self { + file: PathBuf::from("Netsukefile"), + directory: None, + jobs: None, + verbose: false, + locale: None, + fetch_allow_scheme: Vec::new(), + fetch_allow_host: Vec::new(), + fetch_block_host: Vec::new(), + fetch_default_deny: false, + accessible: None, + progress: None, + no_emoji: None, + diag_json: false, + colour_policy: None, + spinner_mode: None, + output_format: None, + theme: None, + command: None, + } + .with_default_command() + } +} + +/// Arguments accepted by the `build` command. +#[derive(Debug, Args, PartialEq, Eq, Clone, Serialize, Deserialize, Default)] +pub struct BuildArgs { + /// Write the generated Ninja manifest to this path and retain it. + #[arg(long, value_name = "FILE")] + pub emit: Option, + + /// A list of specific targets to build. + #[serde(default)] + pub targets: Vec, +} + +/// Available top-level commands for Netsuke. +#[derive(Debug, Subcommand, PartialEq, Eq, Clone, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum Commands { + /// Build specified targets (or default targets if none are given). + Build(BuildArgs), + + /// Remove build artefacts and intermediate files. + Clean, + + /// Display the build dependency graph in DOT format for visualisation. + Graph, + + /// Write the Ninja manifest to the specified file without invoking Ninja. + Manifest { + /// Output path for the generated Ninja file. + /// + /// Use `-` to write to stdout. + #[arg(value_name = "FILE")] + file: PathBuf, + }, +} + +/// Parse CLI arguments with localized clap output. +/// +/// Returns both the parsed CLI struct and the `ArgMatches` required for +/// configuration merging. +/// +/// # Errors +/// +/// Returns a `clap::Error` with localization applied when parsing fails. +pub fn parse_with_localizer_from( + iter: I, + localizer: &Arc, +) -> Result<(Cli, ArgMatches), clap::Error> +where + I: IntoIterator, + T: Into + Clone, +{ + let mut command = localize_command(Cli::command(), localizer.as_ref()); + command = configure_validation_parsers(command, localizer); + let matches = command + .try_get_matches_from_mut(iter) + .map_err(|err| localize_clap_error_with_command(err, localizer.as_ref(), Some(&command)))?; + let matches_for_merge = matches.clone(); + let mut matches_for_parse = matches; + let cli = Cli::from_arg_matches_mut(&mut matches_for_parse).map_err(|clap_err| { + let with_cmd = clap_err.with_cmd(&command); + localize_clap_error_with_command(with_cmd, localizer.as_ref(), Some(&command)) + })?; + Ok((cli, matches_for_merge)) +} + +fn configure_validation_parsers( + mut command: clap::Command, + localizer: &Arc, +) -> clap::Command { + let jobs_parser = LocalizedValueParser::new(Arc::clone(localizer), parse_jobs); + let locale_parser = LocalizedValueParser::new(Arc::clone(localizer), parse_locale); + let scheme_parser = LocalizedValueParser::new(Arc::clone(localizer), parse_scheme); + let host_parser = LocalizedValueParser::new(Arc::clone(localizer), parse_host_pattern); + + command = command.mut_arg("jobs", |arg| { + arg.value_parser(ValueParser::new(jobs_parser)) + }); + command = command.mut_arg("locale", |arg| { + arg.value_parser(ValueParser::new(locale_parser)) + }); + command = command.mut_arg("fetch_allow_scheme", |arg| { + arg.value_parser(ValueParser::new(scheme_parser.clone())) + }); + command = command.mut_arg("fetch_allow_host", |arg| { + arg.value_parser(ValueParser::new(host_parser.clone())) + }); + command = command.mut_arg("fetch_block_host", |arg| { + arg.value_parser(ValueParser::new(host_parser)) + }); + command +} +/// Maximum number of jobs accepted by the CLI. +pub(super) const MAX_JOBS: usize = 64; diff --git a/src/cli/parsing.rs b/src/cli/parsing.rs index 147d4243..fca65bce 100644 --- a/src/cli/parsing.rs +++ b/src/cli/parsing.rs @@ -10,24 +10,24 @@ pub(super) fn parse_jobs(localizer: &dyn Localizer, s: &str) -> Result Result Result { let trimmed = s.trim(); if trimmed.is_empty() { - return Err(super::validation_message( + return Err(super::parser::validation_message( localizer, keys::CLI_SCHEME_EMPTY, None, @@ -50,7 +50,7 @@ pub(super) fn parse_scheme(localizer: &dyn Localizer, s: &str) -> Result Result Result Result { let trimmed = s.trim(); if trimmed.is_empty() { - return Err(super::validation_message( + return Err(super::parser::validation_message( localizer, keys::CLI_LOCALE_EMPTY, None, @@ -85,7 +85,7 @@ pub(super) fn parse_locale(localizer: &dyn Localizer, s: &str) -> Result ExitCode::SUCCESS, Err(err) => handle_runner_error(err, prefs, runtime_mode), diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 0cc6d8c6..17011dab 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -136,7 +136,7 @@ const fn should_force_text_task_updates(mode: OutputMode, stdout_is_tty: bool) - /// Returns an error if manifest generation or the Ninja process fails. pub fn run(cli: &Cli, prefs: OutputPrefs) -> Result<()> { let mode = output_mode::resolve(cli.accessible); - let progress_enabled = cli.progress.unwrap_or(true) && !cli.diag_json; + let progress_enabled = cli.progress_enabled() && !cli.diag_json; let stdout_is_tty = std::io::stdout().is_terminal(); let reporter = make_reporter(ReporterOptions { mode, diff --git a/tests/bdd/fixtures/mod.rs b/tests/bdd/fixtures/mod.rs index 97a405c0..61a252f2 100644 --- a/tests/bdd/fixtures/mod.rs +++ b/tests/bdd/fixtures/mod.rs @@ -27,6 +27,7 @@ use std::path::PathBuf; use std::sync::MutexGuard; use test_support::PathGuard; use test_support::env::{NinjaEnvGuard, restore_many}; +use test_support::env_lock::EnvLock; use test_support::http::HttpServer; /// Combined test world for all BDD scenarios. @@ -148,6 +149,8 @@ pub struct TestWorld { // Environment state /// Snapshot of pre-scenario values for environment variables that were overridden. pub env_vars: RefCell>>, + /// Scenario-scoped guard that serialises environment mutations when needed. + pub env_lock: RefCell>, } impl TestWorld { @@ -190,6 +193,7 @@ impl Drop for TestWorld { self.ninja_env_guard.borrow_mut().take(); self.localization_guard.borrow_mut().take(); self.localization_lock.borrow_mut().take(); + self.env_lock.borrow_mut().take(); self.restore_environment(); self.stdlib_text.clear(); } diff --git a/tests/bdd/steps/configuration_preferences.rs b/tests/bdd/steps/configuration_preferences.rs new file mode 100644 index 00000000..095ed675 --- /dev/null +++ b/tests/bdd/steps/configuration_preferences.rs @@ -0,0 +1,168 @@ +//! Step definitions for layered CLI configuration preferences. + +use crate::bdd::fixtures::{RefCellOptionExt, TestWorld}; +use crate::bdd::helpers::parse_store::store_parse_outcome; +use crate::bdd::helpers::tokens::build_tokens; +use anyhow::{Context, Result, anyhow, bail, ensure}; +use netsuke::cli::{Cli, Commands, Theme}; +use netsuke::cli_localization; +use netsuke::output_prefs; +use rstest_bdd_macros::{given, then, when}; +use std::ffi::OsStr; +use std::fs; +use std::path::PathBuf; +use std::sync::Arc; +use test_support::display_error_chain; +use test_support::env_lock::EnvLock; + +const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG_PATH"; + +fn workspace_path(world: &TestWorld) -> Result { + let temp = world.temp_dir.borrow(); + let dir = temp + .as_ref() + .context("temp dir has not been initialised for configuration steps")?; + Ok(dir.path().to_path_buf()) +} + +fn ensure_env_lock(world: &TestWorld) { + if world.env_lock.borrow().is_none() { + *world.env_lock.borrow_mut() = Some(EnvLock::acquire()); + } +} + +fn write_config(world: &TestWorld, contents: &str) -> Result<()> { + ensure_env_lock(world); + let workspace = workspace_path(world)?; + let path = workspace.join("netsuke.toml"); + fs::write(&path, contents).with_context(|| format!("write {}", path.display()))?; + 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); + Ok(()) +} + +fn merge_cli(world: &TestWorld, args: &str) { + let tokens = build_tokens(args); + let localizer = Arc::from(cli_localization::build_localizer(None)); + let outcome = netsuke::cli::parse_with_localizer_from(tokens, &localizer) + .and_then(|(cli, matches)| { + netsuke::cli::merge_with_config(&cli, &matches).map_err(|err| { + clap::Error::raw( + clap::error::ErrorKind::InvalidValue, + display_error_chain(err.as_ref()), + ) + }) + }) + .map(Cli::with_default_command) + .map_err(|err| err.to_string()); + store_parse_outcome(&world.cli, &world.cli_error, outcome); +} + +#[given("the Netsuke config file sets build targets to {target:string}")] +fn config_sets_build_targets(world: &TestWorld, target: &str) -> Result<()> { + write_config(world, &format!("[cmds.build]\ntargets = [\"{target}\"]\n")) +} + +#[given("the Netsuke config file sets locale to {locale:string}")] +fn config_sets_locale(world: &TestWorld, locale: &str) -> Result<()> { + write_config(world, &format!("locale = \"{locale}\"\n")) +} + +#[given("the Netsuke config file sets output format to {format:string}")] +fn config_sets_output_format(world: &TestWorld, format: &str) -> Result<()> { + write_config(world, &format!("output_format = \"{format}\"\n")) +} + +#[given("the Netsuke config file sets no_emoji to true")] +fn config_sets_no_emoji(world: &TestWorld) -> Result<()> { + write_config(world, "no_emoji = true\n") +} + +#[expect( + clippy::unnecessary_wraps, + reason = "rstest-bdd macro generates Result wrapper; FIXME: https://github.com/leynos/rstest-bdd/issues/381" +)] +#[when("the CLI is parsed and merged with {args:string}")] +fn parse_and_merge_cli(world: &TestWorld, args: &str) -> Result<()> { + merge_cli(world, args); + Ok(()) +} + +#[when("merged output preferences are resolved")] +fn resolve_merged_output_prefs(world: &TestWorld) -> Result<()> { + let prefs = world + .cli + .with_ref(|cli| output_prefs::resolve(cli.no_emoji_override())) + .ok_or_else(|| anyhow!("expected merged CLI before resolving output prefs"))?; + world.output_prefs.set(prefs); + Ok(()) +} + +#[then("the merged CLI uses build target {target:string}")] +fn merged_cli_uses_build_target(world: &TestWorld, target: &str) -> Result<()> { + let command = world + .cli + .with_ref(|cli| cli.command.clone()) + .context("expected merged CLI to be available")? + .context("expected merged CLI command to be set")?; + match command { + Commands::Build(args) => ensure!( + args.targets.first().map(String::as_str) == Some(target), + "expected first merged build target '{target}', got {:?}", + args.targets, + ), + other => bail!("expected merged build command, got {other:?}"), + } + Ok(()) +} + +#[then("the merged locale is {locale:string}")] +fn merged_locale_is(world: &TestWorld, locale: &str) -> Result<()> { + let merged_locale = world + .cli + .with_ref(|cli| cli.locale.clone()) + .context("expected merged CLI to be available")?; + ensure!( + merged_locale.as_deref() == Some(locale), + "expected merged locale '{locale}', got {merged_locale:?}", + ); + Ok(()) +} + +#[then("verbose mode is enabled in the merged CLI")] +fn merged_verbose_enabled(world: &TestWorld) -> Result<()> { + let verbose = world + .cli + .with_ref(|cli| cli.verbose) + .context("expected merged CLI to be available")?; + ensure!(verbose, "expected merged verbose mode to be enabled"); + Ok(()) +} + +#[then("the merged theme is ascii")] +fn merged_theme_is_ascii(world: &TestWorld) -> Result<()> { + let theme = world + .cli + .with_ref(|cli| cli.theme) + .context("expected merged CLI to be available")?; + ensure!( + theme == Some(Theme::Ascii), + "expected merged theme to be ASCII, got {theme:?}", + ); + Ok(()) +} + +#[then("the merge error should contain {fragment:string}")] +fn merge_error_contains(world: &TestWorld, fragment: &str) -> Result<()> { + let error = world + .cli_error + .get() + .context("expected a merge error to be captured")?; + ensure!( + error.contains(fragment), + "expected merge error to contain '{fragment}', got '{error}'", + ); + Ok(()) +} diff --git a/tests/bdd/steps/locale_resolution.rs b/tests/bdd/steps/locale_resolution.rs index 934180bd..d8ddee5c 100644 --- a/tests/bdd/steps/locale_resolution.rs +++ b/tests/bdd/steps/locale_resolution.rs @@ -6,7 +6,7 @@ use crate::bdd::fixtures::TestWorld; use crate::bdd::helpers::tokens::build_tokens; use anyhow::{Context, Result, ensure}; -use netsuke::cli::Cli; +use netsuke::cli::{Cli, CliConfig}; use netsuke::cli_localization; use netsuke::locale_resolution; use netsuke::localization::keys; @@ -17,7 +17,7 @@ use test_support::locale_stubs::{StubEnv, StubSystemLocale}; fn merge_locale_layers(world: &TestWorld) -> Result { let mut composer = MergeComposer::new(); - let defaults = sanitize_value(&Cli::default())?; + let defaults = sanitize_value(&CliConfig::default())?; composer.push_defaults(defaults); if let Some(locale) = world.locale_config.get() { @@ -32,7 +32,11 @@ fn merge_locale_layers(world: &TestWorld) -> Result { composer.push_cli(json!({ "locale": locale })); } - Cli::merge_from_layers(composer.layers()).context("merge locale layers") + let merged = CliConfig::merge_from_layers(composer.layers()).context("merge locale layers")?; + Ok(Cli { + locale: merged.locale, + ..Cli::default() + }) } fn record_resolved_locale(world: &TestWorld, resolved: Option<&str>) { diff --git a/tests/bdd/steps/mod.rs b/tests/bdd/steps/mod.rs index e145f266..a8b084a2 100644 --- a/tests/bdd/steps/mod.rs +++ b/tests/bdd/steps/mod.rs @@ -16,6 +16,7 @@ mod accessibility_preferences; mod accessible_output; mod cli; +mod configuration_preferences; #[cfg(unix)] mod fs; mod ir; diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index 32e65c5f..a2caa702 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -4,7 +4,7 @@ //! CLI) and list-value appending. use anyhow::{Context, Result, ensure}; -use netsuke::cli::Cli; +use netsuke::cli::{Cli, CliConfig, OutputFormat, SpinnerMode, Theme}; use netsuke::cli_localization; use ortho_config::{MergeComposer, sanitize_value}; use rstest::{fixture, rstest}; @@ -18,7 +18,7 @@ use test_support::{EnvVarGuard, env_lock::EnvLock}; #[fixture] fn default_cli_json() -> Result { - sanitize_value(&Cli::default()) + sanitize_value(&CliConfig::default()) } #[rstest] @@ -59,7 +59,7 @@ fn cli_merge_layers_respects_precedence_and_appends_lists( "diag_json": true, "verbose": true })); - let merged = Cli::merge_from_layers(composer.layers())?; + let merged = CliConfig::merge_from_layers(composer.layers())?; ensure!( merged.file.as_path() == Path::new("Configfile"), "file layer should override defaults", @@ -164,10 +164,157 @@ fn cli_merge_layers_prefers_cli_then_env_then_file_for_locale( composer.push_environment(json!({ "locale": "es-ES" })); composer.push_cli(json!({ "locale": "en-US" })); - let merged = Cli::merge_from_layers(composer.layers())?; + let merged = CliConfig::merge_from_layers(composer.layers())?; ensure!( merged.locale.as_deref() == Some("en-US"), "CLI locale should override env and file layers", ); Ok(()) } + +#[rstest] +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(()) +} + +#[rstest] +fn cli_config_explicit_targets_override_configured_build_defaults() -> 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"] +"#, + ) + .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", "build", "lint"], &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("lint")], + "explicit CLI targets should override configured defaults", + ); + Ok(()) +} + +#[rstest] +fn cli_config_validates_theme_alias_conflicts( + default_cli_json: Result, +) -> Result<()> { + let mut composer = MergeComposer::new(); + composer.push_defaults(default_cli_json?); + composer.push_file(json!({ + "theme": "unicode", + "no_emoji": true + }), None); + + let err = CliConfig::merge_from_layers(composer.layers()) + .expect_err("conflicting theme and alias should fail"); + ensure!( + err.to_string().contains("theme = \"unicode\" conflicts with no_emoji = true"), + "unexpected error text: {err}", + ); + Ok(()) +} + +#[rstest] +fn cli_config_validates_spinner_and_progress_conflicts( + default_cli_json: Result, +) -> Result<()> { + let mut composer = MergeComposer::new(); + composer.push_defaults(default_cli_json?); + composer.push_file(json!({ + "spinner_mode": "disabled", + "progress": true + }), None); + + let err = CliConfig::merge_from_layers(composer.layers()) + .expect_err("conflicting spinner and progress settings should fail"); + ensure!( + err.to_string().contains("spinner_mode = \"disabled\" conflicts with progress = true"), + "unexpected error text: {err}", + ); + Ok(()) +} + +#[rstest] +fn cli_config_rejects_unsupported_json_output_format( + default_cli_json: Result, +) -> Result<()> { + let mut composer = MergeComposer::new(); + composer.push_defaults(default_cli_json?); + composer.push_file(json!({ + "output_format": "json" + }), None); + + let err = CliConfig::merge_from_layers(composer.layers()) + .expect_err("unsupported output format should fail"); + ensure!( + err.to_string().contains("output_format = \"json\" is not supported yet"), + "unexpected error text: {err}", + ); + Ok(()) +} + +#[rstest] +fn cli_runtime_canonicalizes_ascii_theme_from_no_emoji_alias() -> 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, "no_emoji = true\n").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")?; + + ensure!( + merged.theme == Some(Theme::Ascii), + "no_emoji compatibility alias should canonicalize to the ASCII theme", + ); + ensure!( + merged.no_emoji == Some(true), + "no_emoji alias should remain available in the runtime CLI", + ); + Ok(()) +} diff --git a/tests/features/configuration_preferences.feature b/tests/features/configuration_preferences.feature new file mode 100644 index 00000000..b798cb61 --- /dev/null +++ b/tests/features/configuration_preferences.feature @@ -0,0 +1,33 @@ +Feature: Configuration preferences + + Scenario: Configured build targets become the default build command targets + Given a minimal Netsuke workspace + And the Netsuke config file sets build targets to "hello" + When the CLI is parsed and merged with "" + Then parsing succeeds + And the merged CLI uses build target "hello" + + Scenario: CLI locale and verbose flags override configuration and environment + Given an empty workspace + And the Netsuke config file sets locale to "es-ES" + When the CLI is parsed and merged with "--locale en-US --verbose" + Then parsing succeeds + And the merged locale is "en-US" + And verbose mode is enabled in the merged CLI + + Scenario: Unsupported output format fails during configuration merge + Given an empty workspace + And the Netsuke config file sets output format to "json" + When the CLI is parsed and merged with "" + Then an error should be returned + And the merge error should contain "output_format = " + + 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 From 507222429f42d1f0f6763c18b6d464b3dcf253b7 Mon Sep 17 00:00:00 2001 From: Leynos Date: Tue, 10 Mar 2026 19:02:15 +0000 Subject: [PATCH 04/12] test(cli): refactor CLI merge layer tests for reusability and clarity 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] --- tests/cli_tests/merge.rs | 185 +++++++++++++++++++-------------------- 1 file changed, 88 insertions(+), 97 deletions(-) diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index a2caa702..63cd8b0b 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -4,23 +4,48 @@ //! CLI) and list-value appending. use anyhow::{Context, Result, ensure}; -use netsuke::cli::{Cli, CliConfig, OutputFormat, SpinnerMode, Theme}; -use netsuke::cli_localization; +use netsuke::cli::{CliConfig, Theme}; use ortho_config::{MergeComposer, sanitize_value}; use rstest::{fixture, rstest}; use serde_json::json; use std::ffi::OsStr; use std::fs; use std::path::Path; -use std::sync::Arc; -use tempfile::tempdir; -use test_support::{EnvVarGuard, env_lock::EnvLock}; +use test_support::EnvVarGuard; #[fixture] fn default_cli_json() -> Result { sanitize_value(&CliConfig::default()) } +fn with_config_file(toml_content: &str, cli_args: &[&str], f: F) -> anyhow::Result +where + F: FnOnce(netsuke::cli::Cli) -> anyhow::Result, +{ + let _env_lock = test_support::env_lock::EnvLock::acquire(); + let temp_dir = tempfile::tempdir().context("create temporary config directory")?; + let config_path = temp_dir.path().join("netsuke.toml"); + std::fs::write(&config_path, toml_content).context("write netsuke.toml")?; + let _config_guard = + test_support::EnvVarGuard::set("NETSUKE_CONFIG_PATH", config_path.as_os_str()); + let localizer = std::sync::Arc::from(netsuke::cli_localization::build_localizer(None)); + let (cli, matches) = netsuke::cli::parse_with_localizer_from(cli_args, &localizer) + .context("parse CLI args for merge")?; + let merged = netsuke::cli::merge_with_config(&cli, &matches) + .context("merge CLI and configuration layers")?; + f(merged) +} + +fn merge_defaults_with_file_layer( + defaults: serde_json::Value, + file_layer: serde_json::Value, +) -> anyhow::Result { + let mut composer = ortho_config::MergeComposer::new(); + composer.push_defaults(defaults); + composer.push_file(file_layer, None); + netsuke::cli::CliConfig::merge_from_layers(composer.layers()).map_err(anyhow::Error::from) +} + #[rstest] fn cli_merge_layers_respects_precedence_and_appends_lists( default_cli_json: Result, @@ -174,80 +199,58 @@ fn cli_merge_layers_prefers_cli_then_env_then_file_for_locale( #[rstest] 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, + 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(()) + }, ) - .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(()) } #[rstest] fn cli_config_explicit_targets_override_configured_build_defaults() -> 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, + with_config_file( r#" [cmds.build] targets = ["all"] "#, + &["netsuke", "build", "lint"], + |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("lint")], + "explicit CLI targets should override configured defaults", + ); + Ok(()) + }, ) - .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", "build", "lint"], &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("lint")], - "explicit CLI targets should override configured defaults", - ); - Ok(()) } #[rstest] fn cli_config_validates_theme_alias_conflicts( default_cli_json: Result, ) -> Result<()> { - let mut composer = MergeComposer::new(); - composer.push_defaults(default_cli_json?); - composer.push_file(json!({ - "theme": "unicode", - "no_emoji": true - }), None); - - let err = CliConfig::merge_from_layers(composer.layers()) - .expect_err("conflicting theme and alias should fail"); + let err = merge_defaults_with_file_layer( + default_cli_json?, + json!({ + "theme": "unicode", + "no_emoji": true + }), + ) + .expect_err("conflicting theme and alias should fail"); ensure!( err.to_string().contains("theme = \"unicode\" conflicts with no_emoji = true"), "unexpected error text: {err}", @@ -259,15 +262,14 @@ fn cli_config_validates_theme_alias_conflicts( fn cli_config_validates_spinner_and_progress_conflicts( default_cli_json: Result, ) -> Result<()> { - let mut composer = MergeComposer::new(); - composer.push_defaults(default_cli_json?); - composer.push_file(json!({ - "spinner_mode": "disabled", - "progress": true - }), None); - - let err = CliConfig::merge_from_layers(composer.layers()) - .expect_err("conflicting spinner and progress settings should fail"); + let err = merge_defaults_with_file_layer( + default_cli_json?, + json!({ + "spinner_mode": "disabled", + "progress": true + }), + ) + .expect_err("conflicting spinner and progress settings should fail"); ensure!( err.to_string().contains("spinner_mode = \"disabled\" conflicts with progress = true"), "unexpected error text: {err}", @@ -279,14 +281,13 @@ fn cli_config_validates_spinner_and_progress_conflicts( fn cli_config_rejects_unsupported_json_output_format( default_cli_json: Result, ) -> Result<()> { - let mut composer = MergeComposer::new(); - composer.push_defaults(default_cli_json?); - composer.push_file(json!({ - "output_format": "json" - }), None); - - let err = CliConfig::merge_from_layers(composer.layers()) - .expect_err("unsupported output format should fail"); + let err = merge_defaults_with_file_layer( + default_cli_json?, + json!({ + "output_format": "json" + }), + ) + .expect_err("unsupported output format should fail"); ensure!( err.to_string().contains("output_format = \"json\" is not supported yet"), "unexpected error text: {err}", @@ -296,25 +297,15 @@ fn cli_config_rejects_unsupported_json_output_format( #[rstest] fn cli_runtime_canonicalizes_ascii_theme_from_no_emoji_alias() -> 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, "no_emoji = true\n").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")?; - - ensure!( - merged.theme == Some(Theme::Ascii), - "no_emoji compatibility alias should canonicalize to the ASCII theme", - ); - ensure!( - merged.no_emoji == Some(true), - "no_emoji alias should remain available in the runtime CLI", - ); - Ok(()) + with_config_file("no_emoji = true\n", &["netsuke"], |merged| { + ensure!( + merged.theme == Some(Theme::Ascii), + "no_emoji compatibility alias should canonicalize to the ASCII theme", + ); + ensure!( + merged.no_emoji == Some(true), + "no_emoji alias should remain available in the runtime CLI", + ); + Ok(()) + }) } From b9399eb56294742691a70e5f7b9acd6ba803aea1 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 11 Mar 2026 13:01:43 +0000 Subject: [PATCH 05/12] test(cli): refactor merge.rs tests with helper assertion functions - 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] --- tests/cli_tests/merge.rs | 82 ++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 37 deletions(-) diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index 63cd8b0b..2cc5a273 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -5,13 +5,16 @@ use anyhow::{Context, Result, ensure}; use netsuke::cli::{CliConfig, Theme}; +use netsuke::cli_localization; use ortho_config::{MergeComposer, sanitize_value}; use rstest::{fixture, rstest}; use serde_json::json; use std::ffi::OsStr; use std::fs; use std::path::Path; -use test_support::EnvVarGuard; +use std::sync::Arc; +use tempfile::tempdir; +use test_support::{EnvVarGuard, env_lock::EnvLock}; #[fixture] fn default_cli_json() -> Result { @@ -36,6 +39,21 @@ where f(merged) } +fn assert_build_targets( + toml_content: &str, + cli_args: &[&str], + expected_targets: &[String], + assertion_msg: &str, +) -> anyhow::Result<()> { + with_config_file(toml_content, cli_args, |merged| { + let Some(netsuke::cli::Commands::Build(args)) = merged.command else { + anyhow::bail!("expected merged command to be build"); + }; + ensure!(args.targets == expected_targets, "{}", assertion_msg); + Ok(()) + }) +} + fn merge_defaults_with_file_layer( defaults: serde_json::Value, file_layer: serde_json::Value, @@ -46,6 +64,20 @@ fn merge_defaults_with_file_layer( netsuke::cli::CliConfig::merge_from_layers(composer.layers()).map_err(anyhow::Error::from) } +fn assert_merge_rejects( + defaults: serde_json::Value, + file_layer: serde_json::Value, + expect_err_hint: &str, + expected_msg: &str, +) -> anyhow::Result<()> { + let err = merge_defaults_with_file_layer(defaults, file_layer).expect_err(expect_err_hint); + ensure!( + err.to_string().contains(expected_msg), + "unexpected error text: {err}", + ); + Ok(()) +} + #[rstest] fn cli_merge_layers_respects_precedence_and_appends_lists( default_cli_json: Result, @@ -199,43 +231,27 @@ fn cli_merge_layers_prefers_cli_then_env_then_file_for_locale( #[rstest] fn cli_config_build_defaults_apply_when_cli_targets_are_absent() -> Result<()> { - with_config_file( + assert_build_targets( 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(()) - }, + &[String::from("all"), String::from("docs")], + "configured build targets should be used when CLI targets are absent", ) } #[rstest] fn cli_config_explicit_targets_override_configured_build_defaults() -> Result<()> { - with_config_file( + assert_build_targets( r#" [cmds.build] targets = ["all"] "#, &["netsuke", "build", "lint"], - |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("lint")], - "explicit CLI targets should override configured defaults", - ); - Ok(()) - }, + &[String::from("lint")], + "explicit CLI targets should override configured defaults", ) } @@ -243,38 +259,30 @@ targets = ["all"] fn cli_config_validates_theme_alias_conflicts( default_cli_json: Result, ) -> Result<()> { - let err = merge_defaults_with_file_layer( + 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", ) - .expect_err("conflicting theme and alias should fail"); - ensure!( - err.to_string().contains("theme = \"unicode\" conflicts with no_emoji = true"), - "unexpected error text: {err}", - ); - Ok(()) } #[rstest] fn cli_config_validates_spinner_and_progress_conflicts( default_cli_json: Result, ) -> Result<()> { - let err = merge_defaults_with_file_layer( + assert_merge_rejects( default_cli_json?, json!({ "spinner_mode": "disabled", "progress": true }), + "conflicting spinner and progress settings should fail", + "spinner_mode = \"disabled\" conflicts with progress = true", ) - .expect_err("conflicting spinner and progress settings should fail"); - ensure!( - err.to_string().contains("spinner_mode = \"disabled\" conflicts with progress = true"), - "unexpected error text: {err}", - ); - Ok(()) } #[rstest] From 91ddd88ed71b128e4dc1e015904536fb33e719e1 Mon Sep 17 00:00:00 2001 From: Leynos Date: Wed, 11 Mar 2026 16:17:52 +0000 Subject: [PATCH 06/12] test(cli): use parameterized tests for conflict validation 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] --- tests/cli_tests/merge.rs | 63 ++++++++++++---------------------------- 1 file changed, 18 insertions(+), 45 deletions(-) diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index 2cc5a273..694cf9d7 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -67,10 +67,10 @@ fn merge_defaults_with_file_layer( fn assert_merge_rejects( defaults: serde_json::Value, file_layer: serde_json::Value, - expect_err_hint: &str, expected_msg: &str, ) -> anyhow::Result<()> { - let err = merge_defaults_with_file_layer(defaults, file_layer).expect_err(expect_err_hint); + let err = merge_defaults_with_file_layer(defaults, file_layer) + .expect_err("merge should have returned an error"); ensure!( err.to_string().contains(expected_msg), "unexpected error text: {err}", @@ -256,51 +256,24 @@ targets = ["all"] } #[rstest] -fn cli_config_validates_theme_alias_conflicts( +#[case( + json!({ "theme": "unicode", "no_emoji": true }), + "theme = \"unicode\" conflicts with no_emoji = true", +)] +#[case( + json!({ "spinner_mode": "disabled", "progress": true }), + "spinner_mode = \"disabled\" conflicts with progress = true", +)] +#[case( + json!({ "output_format": "json" }), + "output_format = \"json\" is not supported yet", +)] +fn cli_config_rejects_conflicting_or_unsupported_settings( default_cli_json: Result, + #[case] file_layer: serde_json::Value, + #[case] expected_msg: &str, ) -> 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", - ) -} - -#[rstest] -fn cli_config_validates_spinner_and_progress_conflicts( - default_cli_json: Result, -) -> Result<()> { - assert_merge_rejects( - default_cli_json?, - json!({ - "spinner_mode": "disabled", - "progress": true - }), - "conflicting spinner and progress settings should fail", - "spinner_mode = \"disabled\" conflicts with progress = true", - ) -} - -#[rstest] -fn cli_config_rejects_unsupported_json_output_format( - default_cli_json: Result, -) -> Result<()> { - let err = merge_defaults_with_file_layer( - default_cli_json?, - json!({ - "output_format": "json" - }), - ) - .expect_err("unsupported output format should fail"); - ensure!( - err.to_string().contains("output_format = \"json\" is not supported yet"), - "unexpected error text: {err}", - ); - Ok(()) + assert_merge_rejects(default_cli_json?, file_layer, expected_msg) } #[rstest] From bef47da477df5f713b7e5f40508f31b515ec61e0 Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 12 Mar 2026 17:49:58 +0000 Subject: [PATCH 07/12] test(cli): improve merge tests with structured error expectations - 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] --- tests/cli_tests/merge.rs | 47 ++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index 694cf9d7..74b47df5 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -43,17 +43,44 @@ fn assert_build_targets( toml_content: &str, cli_args: &[&str], expected_targets: &[String], - assertion_msg: &str, ) -> anyhow::Result<()> { with_config_file(toml_content, cli_args, |merged| { let Some(netsuke::cli::Commands::Build(args)) = merged.command else { anyhow::bail!("expected merged command to be build"); }; - ensure!(args.targets == expected_targets, "{}", assertion_msg); + ensure!( + args.targets == expected_targets, + "build targets mismatch: got {:?}, expected {:?}", + args.targets, + expected_targets, + ); Ok(()) }) } +#[derive(Debug)] +enum ExpectedValidationError { + ThemeAliasConflict, + SpinnerProgressConflict, + UnsupportedOutputFormat, +} + +impl ExpectedValidationError { + fn expected_fragment(&self) -> &'static str { + match self { + Self::ThemeAliasConflict => { + "theme = \"unicode\" conflicts with no_emoji = true" + } + Self::SpinnerProgressConflict => { + "spinner_mode = \"disabled\" conflicts with progress = true" + } + Self::UnsupportedOutputFormat => { + "output_format = \"json\" is not supported yet" + } + } + } +} + fn merge_defaults_with_file_layer( defaults: serde_json::Value, file_layer: serde_json::Value, @@ -67,12 +94,12 @@ fn merge_defaults_with_file_layer( fn assert_merge_rejects( defaults: serde_json::Value, file_layer: serde_json::Value, - expected_msg: &str, + expected_error: ExpectedValidationError, ) -> anyhow::Result<()> { let err = merge_defaults_with_file_layer(defaults, file_layer) .expect_err("merge should have returned an error"); ensure!( - err.to_string().contains(expected_msg), + err.to_string().contains(expected_error.expected_fragment()), "unexpected error text: {err}", ); Ok(()) @@ -238,7 +265,6 @@ targets = ["all", "docs"] "#, &["netsuke"], &[String::from("all"), String::from("docs")], - "configured build targets should be used when CLI targets are absent", ) } @@ -251,29 +277,28 @@ targets = ["all"] "#, &["netsuke", "build", "lint"], &[String::from("lint")], - "explicit CLI targets should override configured defaults", ) } #[rstest] #[case( json!({ "theme": "unicode", "no_emoji": true }), - "theme = \"unicode\" conflicts with no_emoji = true", + ExpectedValidationError::ThemeAliasConflict, )] #[case( json!({ "spinner_mode": "disabled", "progress": true }), - "spinner_mode = \"disabled\" conflicts with progress = true", + ExpectedValidationError::SpinnerProgressConflict, )] #[case( json!({ "output_format": "json" }), - "output_format = \"json\" is not supported yet", + ExpectedValidationError::UnsupportedOutputFormat, )] fn cli_config_rejects_conflicting_or_unsupported_settings( default_cli_json: Result, #[case] file_layer: serde_json::Value, - #[case] expected_msg: &str, + #[case] expected_error: ExpectedValidationError, ) -> Result<()> { - assert_merge_rejects(default_cli_json?, file_layer, expected_msg) + assert_merge_rejects(default_cli_json?, file_layer, expected_error) } #[rstest] From a0b86bab8747e3a3fdc04a12ba03a3c0acf6d07f Mon Sep 17 00:00:00 2001 From: Leynos Date: Thu, 12 Mar 2026 22:21:27 +0000 Subject: [PATCH 08/12] feat(cli): introduce CliConfig as central OrthoConfig-derived schema - 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] --- docs/execplans/3-11-1-cli-config-struct.md | 146 ++++++++---------- docs/ortho-config-users-guide.md | 7 +- docs/users-guide.md | 4 + src/cli/config.rs | 17 +- src/cli/merge.rs | 13 +- src/cli/mod.rs | 10 ++ src/cli/parser.rs | 17 +- tests/bdd/steps/configuration_preferences.rs | 15 ++ tests/cli_tests/merge.rs | 28 +++- tests/cli_tests/parsing.rs | 41 ++++- .../configuration_preferences.feature | 1 + 11 files changed, 183 insertions(+), 116 deletions(-) diff --git a/docs/execplans/3-11-1-cli-config-struct.md b/docs/execplans/3-11-1-cli-config-struct.md index 547750fa..b47b9d88 100644 --- a/docs/execplans/3-11-1-cli-config-struct.md +++ b/docs/execplans/3-11-1-cli-config-struct.md @@ -14,12 +14,10 @@ No `PLANS.md` file exists in this repository. Roadmap item `3.11.1` asks for a dedicated `CliConfig` struct derived with `OrthoConfig` so Netsuke has one explicit, typed schema for layered CLI configuration. This plan now targets `ortho_config` `0.8.0` and uses the -repository copy of -[`docs/ortho-config-users-guide.md`](/home/user/project/docs/ortho-config-users-guide.md), - which has been replaced with the upstream `v0.8.0` guide. Today the repository -already has partial layered configuration, but it is centered on -[`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs), where `Cli` currently -serves three roles at once: +repository copy of `docs/ortho-config-users-guide.md`, which has been replaced +with the upstream `v0.8.0` guide. At planning time the repository already had +partial layered configuration, but it was centered on `src/cli/mod.rs`, where +`Cli` served three roles at once: 1. Clap parser. 2. OrthoConfig merge target. @@ -50,10 +48,8 @@ Observable success means: the same typed fields with documented precedence. 4. Unit tests (`rstest`) and behavioural tests (`rstest-bdd` v0.5.0) cover happy paths, unhappy paths, and precedence edge cases. -5. [`docs/users-guide.md`](/home/user/project/docs/users-guide.md), - [`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md), and - [`docs/roadmap.md`](/home/user/project/docs/roadmap.md) reflect the final - behaviour. +5. `docs/users-guide.md`, `docs/netsuke-design.md`, and `docs/roadmap.md` + reflect the final behaviour. ## Constraints @@ -107,10 +103,10 @@ Observable success means: ## Risks -- Risk: [`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs) is already 398 - lines, so any additive work there will violate the file-size limit. Severity: - high. Likelihood: high. Mitigation: split parser definitions, config schema, - and merge helpers into separate modules before adding new fields. +- Risk: `src/cli/mod.rs` was already 398 lines at planning time, so any + additive work there would violate the file-size limit. Severity: high. + Likelihood: high. Mitigation: split parser definitions, config schema, and + merge helpers into separate modules before adding new fields. - Risk: the repository already has partial OrthoConfig support, manual config discovery, and merge tests. A careless refactor could duplicate logic rather @@ -158,18 +154,16 @@ Observable success means: - The repository is already on Rust `1.89.0`, which satisfies the `ortho_config 0.8.0` minimum of Rust `1.88`. The crate version is the real migration step; the compiler floor is not. -- The current repository already documents configuration discovery in - [`docs/users-guide.md`](/home/user/project/docs/users-guide.md), and already - has merge tests in - [`tests/cli_tests/merge.rs`](/home/user/project/tests/cli_tests/merge.rs). - The implementation must preserve these guarantees while changing the type - layout. -- `rstest-bdd` feature-file edits may require touching - [`tests/bdd_tests.rs`](/home/user/project/tests/bdd_tests.rs) to force Cargo - to rebuild generated scenarios. -- The new configuration-preferences BDD coverage initially flaked only in the - full suite because `NETSUKE_CONFIG_PATH` is process-global. Holding - [`EnvLock`](test_support/src/env_lock.rs) for the whole scenario fixed the +- The repository already documented configuration discovery in + `docs/users-guide.md` and already had merge tests in + `tests/cli_tests/merge.rs`. The implementation needed to preserve these + guarantees while changing the type layout. +- `rstest-bdd` feature-file edits may require touching `tests/bdd_tests.rs` to + force Cargo to rebuild generated scenarios. +- The new configuration-preferences behaviour-driven development (BDD) + coverage initially flaked only in the full suite because + `NETSUKE_CONFIG_PATH` is process-global. Holding + `test_support/src/env_lock.rs`'s `EnvLock` for the whole scenario fixed the race without weakening the coverage. ## Decision Log @@ -222,8 +216,7 @@ Observable success means: accepting JSON output configuration without delivering the behaviour would be misleading. Date/Author: 2026-03-09 (Codex, implementation) -These decisions must be recorded in -[`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md) during +These decisions must be recorded in `docs/netsuke-design.md` during implementation if they remain unchanged after coding begins. ## Outcomes & Retrospective @@ -232,9 +225,9 @@ Completed on 2026-03-09. Implemented results: -- Added [`CliConfig`](/home/user/project/src/cli/config.rs) as the - authoritative OrthoConfig-derived schema and split the CLI module into - parser, config, and merge submodules. +- Added `src/cli/config.rs`'s `CliConfig` as the authoritative + OrthoConfig-derived schema and split the CLI module into parser, config, and + merge submodules. - Upgraded `ortho_config` to `0.8.0`. - Kept `Cli` as the parser/runtime command carrier while rooting configuration merge in `CliConfig`. @@ -244,10 +237,8 @@ Implemented results: contradictory combinations. - Wired `[cmds.build] targets` and `emit` defaults into the runtime build command when the user does not supply explicit CLI values. -- Added unit coverage in - [`tests/cli_tests/merge.rs`](/home/user/project/tests/cli_tests/merge.rs) - plus behavioural coverage in - [`tests/features/configuration_preferences.feature`](/home/user/project/tests/features/configuration_preferences.feature). +- Added unit coverage in `tests/cli_tests/merge.rs` plus behavioural coverage + in `tests/features/configuration_preferences.feature`. - Updated the user guide, design document, and roadmap entry for `3.11.1`. Quality-gate evidence: @@ -255,7 +246,7 @@ Quality-gate evidence: - `make check-fmt` - `make lint` - `make test` -- `PATH="/root/.bun/bin:$PATH" make markdownlint` +- `make markdownlint` (with `markdownlint-cli2` available on `PATH`) - `make nixie` Lessons learned: @@ -263,33 +254,30 @@ Lessons learned: - 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. +- Behaviour-driven development (BDD) coverage that touches process-wide + environment variables must hold `EnvLock` for the full scenario, not only for + individual mutations. ## Context and orientation -The current implementation is spread across the following files: - -- [`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs): current `Cli` type, - Clap parser, OrthoConfig derive, validation parsers, and merge logic. -- [`Cargo.toml`](/home/user/project/Cargo.toml): currently pins - `ortho_config = "0.7.0"` and `rust-version = "1.89.0"`. -- [`rust-toolchain.toml`](/home/user/project/rust-toolchain.toml): currently - pins toolchain `1.89.0`, which already satisfies the `0.8.0` minimum. -- [`src/main.rs`](/home/user/project/src/main.rs): startup parse/merge flow and - runtime localization bootstrap. -- [`src/output_mode.rs`](/home/user/project/src/output_mode.rs): accessible - versus standard output mode resolution. -- [`src/output_prefs.rs`](/home/user/project/src/output_prefs.rs): emoji-aware - semantic prefixes and current `no_emoji` handling. -- [`src/runner/mod.rs`](/home/user/project/src/runner/mod.rs): uses merged CLI - state to choose output mode, progress behaviour, and build targets. -- [`tests/cli_tests/merge.rs`](/home/user/project/tests/cli_tests/merge.rs): - current merge precedence coverage. -- [`tests/cli_tests/parsing.rs`](/home/user/project/tests/cli_tests/parsing.rs) - and - [`tests/features/cli.feature`](/home/user/project/tests/features/cli.feature): - current parse-only coverage. +Historical planning context for this change was spread across the following +files: + +- `src/cli/mod.rs`: then contained the `Cli` type, Clap parser, OrthoConfig + derive, validation parsers, and merge logic. +- `Cargo.toml`: then pinned `ortho_config = "0.7.0"` and + `rust-version = "1.89.0"`. +- `rust-toolchain.toml`: then pinned toolchain `1.89.0`, which already + satisfied the `0.8.0` minimum. +- `src/main.rs`: startup parse/merge flow and runtime localization bootstrap. +- `src/output_mode.rs`: accessible versus standard output mode resolution. +- `src/output_prefs.rs`: emoji-aware semantic prefixes and then-current + `no_emoji` handling. +- `src/runner/mod.rs`: uses merged CLI state to choose output mode, progress + behaviour, and build targets. +- `tests/cli_tests/merge.rs`: merge precedence coverage. +- `tests/cli_tests/parsing.rs` and `tests/features/cli.feature`: parse-only + coverage. Two important facts shape this plan: @@ -354,10 +342,9 @@ compatibility aliases such as `no_emoji = true`. ### Stage A: split responsibilities before adding new fields -Start by reducing the blast radius in -[`src/cli/mod.rs`](/home/user/project/src/cli/mod.rs). Move the merge logic and -the future `CliConfig` definition out of that file first. The goal is to make -later edits mechanical instead of risky. This stage also establishes the +Start by reducing the blast radius in `src/cli/mod.rs`. Move the merge logic +and the future `CliConfig` definition out of that file first. The goal is to +make later edits mechanical instead of risky. This stage also establishes the `ortho_config 0.8.0` baseline before higher-level schema refactors pile on. Concrete work in this stage: @@ -416,7 +403,7 @@ Concrete work in this stage: `[cmds.build] targets = [...]` becomes the default target list when the user does not pass targets explicitly. 3. Convert the merged config plus parsed command into the runtime shape - consumed by [`src/runner/mod.rs`](/home/user/project/src/runner/mod.rs). + consumed by `src/runner/mod.rs`. 4. Update output-mode and output-preference resolution to consume the new typed fields rather than a loose collection of booleans. 5. Keep startup locale resolution intact so localized Clap help still works @@ -456,9 +443,9 @@ Behavioural coverage to add with `rstest-bdd` v0.5.0: - compatibility alias (`no_emoji`) still produces ASCII-themed output Prefer a dedicated feature file such as -[`tests/features/configuration_preferences.feature`](/home/user/project/tests/features/configuration_preferences.feature) - plus matching step definitions, rather than overloading the existing -CLI-parsing feature with merge semantics. +`tests/features/configuration_preferences.feature` plus matching step +definitions, rather than overloading the existing CLI-parsing feature with +merge semantics. ### Stage E: document the final contract and close the roadmap item @@ -467,16 +454,14 @@ change. Documentation work: -1. Update [`docs/users-guide.md`](/home/user/project/docs/users-guide.md) with - the new config schema, precedence rules, accepted values, and example TOML. -2. Update [`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md) - to record: +1. Update `docs/users-guide.md` with the new config schema, precedence rules, + accepted values, and example TOML. +2. Update `docs/netsuke-design.md` to record: - separation of parser and config schema - subcommand config for default build targets - canonical theme handling and `no_emoji` compatibility - which output-format values are supported in this milestone -3. Mark roadmap item `3.11.1` as done in - [`docs/roadmap.md`](/home/user/project/docs/roadmap.md). +3. Mark roadmap item `3.11.1` as done in `docs/roadmap.md`. Acceptance for Stage E: @@ -488,7 +473,7 @@ Acceptance for Stage E: ## Concrete steps -Run all commands from `/home/user/project`. +Run all commands from the repository root. Before editing feature files, remember the existing `rstest-bdd` gotcha: @@ -517,7 +502,7 @@ Because docs will change, also run: ```sh set -o pipefail -PATH="/root/.bun/bin:$PATH" make markdownlint 2>&1 | tee /tmp/netsuke-markdownlint.log +make markdownlint 2>&1 | tee /tmp/netsuke-markdownlint.log ``` ```sh @@ -534,8 +519,8 @@ After `make fmt`, inspect `git status --short` and remove incidental edits in unrelated files before finalizing the change. The local OrthoConfig reference for this task is now the `v0.8.0` guide at -[`docs/ortho-config-users-guide.md`](/home/user/project/docs/ortho-config-users-guide.md). - Do not rely on older `0.7.x` examples when the two guides disagree. +`docs/ortho-config-users-guide.md`. Do not rely on older `0.7.x` examples when +the two guides disagree. ## Validation and acceptance @@ -551,9 +536,8 @@ The feature is complete only when all of the following are true: 5. Unit tests and behavioural tests cover both precedence and failure cases. 6. `make check-fmt`, `make lint`, `make test`, `make markdownlint`, and `make nixie` succeed. -7. [`docs/users-guide.md`](/home/user/project/docs/users-guide.md), - [`docs/netsuke-design.md`](/home/user/project/docs/netsuke-design.md), and - [`docs/roadmap.md`](/home/user/project/docs/roadmap.md) are updated. +7. `docs/users-guide.md`, `docs/netsuke-design.md`, and `docs/roadmap.md` are + updated. ## Idempotence and recovery diff --git a/docs/ortho-config-users-guide.md b/docs/ortho-config-users-guide.md index 462435c4..801d8805 100644 --- a/docs/ortho-config-users-guide.md +++ b/docs/ortho-config-users-guide.md @@ -1205,9 +1205,10 @@ Enum fields list their possible values in the OPTIONS description. ### Generating PowerShell help `cargo-orthohelp` can generate PowerShell external help in Microsoft Assistance -Markup Language (MAML) alongside a wrapper module so `Get-Help {BinName} -Full` -surfaces the same configuration metadata as the man page generator. Use the -`ps` format to emit the module layout under `powershell/`: +Markup Language (MAML) alongside a wrapper module, so +`Get-Help {BinName} -Full` surfaces the same configuration metadata as the man +page generator. Use the `ps` format to emit the module layout under +`powershell/`: ```bash cargo orthohelp --format ps --out-dir target/orthohelp --locale en-US diff --git a/docs/users-guide.md b/docs/users-guide.md index 03e92c55..635f3d8f 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -564,6 +564,10 @@ top-level configuration keys: - `jobs = 8` - `verbose = true|false` - `locale = "en-US"` +- `fetch_allow_scheme = ["https"]` +- `fetch_allow_host = ["example.com"]` +- `fetch_block_host = ["blocked.example.com"]` +- `fetch_default_deny = true|false` - `accessible = true|false` - `progress = true|false` - `theme = "auto"|"unicode"|"ascii"` diff --git a/src/cli/config.rs b/src/cli/config.rs index 78eb6553..1373fb73 100644 --- a/src/cli/config.rs +++ b/src/cli/config.rs @@ -4,11 +4,11 @@ //! and merging. It captures global CLI settings plus per-subcommand defaults //! under the `cmds` namespace. -use ortho_config::{OrthoConfig, OrthoError, OrthoResult, PostMergeContext, PostMergeHook}; +use ortho_config::{OrthoConfig, OrthoResult, PostMergeContext, PostMergeHook}; use serde::{Deserialize, Serialize}; use std::path::PathBuf; -use std::sync::Arc; +use super::validation_error; use crate::host_pattern::HostPattern; /// Colour-output policy accepted by layered configuration. @@ -175,6 +175,12 @@ impl Default for CliConfig { } } +impl CliConfig { + pub(super) fn default_manifest_path() -> PathBuf { + default_manifest_path() + } +} + impl PostMergeHook for CliConfig { fn post_merge(&mut self, _ctx: &PostMergeContext) -> OrthoResult<()> { validate_theme_compatibility(self)?; @@ -225,10 +231,3 @@ fn validate_output_format_support(config: &CliConfig) -> OrthoResult<()> { } Ok(()) } - -fn validation_error(key: &str, message: &str) -> Arc { - Arc::new(OrthoError::Validation { - key: key.to_owned(), - message: message.to_owned(), - }) -} diff --git a/src/cli/merge.rs b/src/cli/merge.rs index f5e1d23d..99b84b9c 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -5,16 +5,14 @@ use clap::parser::ValueSource; use ortho_config::declarative::LayerComposition; use ortho_config::figment::{Figment, providers::Env}; use ortho_config::uncased::Uncased; -use ortho_config::{ - ConfigDiscovery, MergeComposer, OrthoError, OrthoMergeExt, OrthoResult, sanitize_value, -}; +use ortho_config::{ConfigDiscovery, MergeComposer, OrthoMergeExt, OrthoResult, sanitize_value}; use serde::Serialize; use serde_json::{Map, Value, json}; use std::path::PathBuf; -use std::sync::Arc; use super::config::{BuildConfig, CliConfig, Theme}; use super::parser::{BuildArgs, Cli, Commands}; +use super::validation_error; const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG_PATH"; const ENV_PREFIX: &str = "NETSUKE_"; @@ -247,10 +245,3 @@ const fn canonical_theme(theme: Option, no_emoji: Option) -> Option _ => None, } } - -fn validation_error(key: &str, message: &str) -> Arc { - Arc::new(OrthoError::Validation { - key: key.to_owned(), - message: message.to_owned(), - }) -} diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 142168ee..4b37d7d8 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -5,6 +5,9 @@ //! used to merge defaults, configuration files, environment variables, and CLI //! overrides into the runtime shape consumed by the runner. +use ortho_config::OrthoError; +use std::sync::Arc; + mod config; mod merge; mod parser; @@ -16,3 +19,10 @@ pub use parser::{ BuildArgs, Cli, Commands, diag_json_hint_from_args, locale_hint_from_args, parse_with_localizer_from, }; + +pub(super) fn validation_error(key: &str, message: &str) -> Arc { + Arc::new(OrthoError::Validation { + key: key.to_owned(), + message: message.to_owned(), + }) +} diff --git a/src/cli/parser.rs b/src/cli/parser.rs index d2faaaaf..2d4469f6 100644 --- a/src/cli/parser.rs +++ b/src/cli/parser.rs @@ -10,6 +10,7 @@ use std::ffi::OsString; use std::path::PathBuf; use std::sync::Arc; +use super::config::CliConfig; use super::parsing::{parse_host_pattern, parse_jobs, parse_locale, parse_scheme}; use super::{ColourPolicy, OutputFormat, SpinnerMode, Theme}; pub use crate::cli_l10n::{diag_json_hint_from_args, locale_hint_from_args}; @@ -160,10 +161,16 @@ impl Cli { /// Return the effective emoji override for output preference resolution. #[must_use] pub const fn no_emoji_override(&self) -> Option { - if matches!(self.theme, Some(Theme::Ascii)) || matches!(self.no_emoji, Some(true)) { - Some(true) - } else { - self.no_emoji + match self.theme { + Some(Theme::Ascii) => Some(true), + Some(Theme::Unicode) => Some(false), + _ => { + if matches!(self.no_emoji, Some(true)) { + Some(true) + } else { + None + } + } } } @@ -181,7 +188,7 @@ impl Cli { impl Default for Cli { fn default() -> Self { Self { - file: PathBuf::from("Netsukefile"), + file: CliConfig::default_manifest_path(), directory: None, jobs: None, verbose: false, diff --git a/tests/bdd/steps/configuration_preferences.rs b/tests/bdd/steps/configuration_preferences.rs index 095ed675..11d14fad 100644 --- a/tests/bdd/steps/configuration_preferences.rs +++ b/tests/bdd/steps/configuration_preferences.rs @@ -16,6 +16,7 @@ use test_support::display_error_chain; use test_support::env_lock::EnvLock; const CONFIG_ENV_VAR: &str = "NETSUKE_CONFIG_PATH"; +const LOCALE_ENV_VAR: &str = "NETSUKE_LOCALE"; fn workspace_path(world: &TestWorld) -> Result { let temp = world.temp_dir.borrow(); @@ -70,6 +71,20 @@ fn config_sets_locale(world: &TestWorld, locale: &str) -> Result<()> { write_config(world, &format!("locale = \"{locale}\"\n")) } +#[given("the NETSUKE_LOCALE environment variable is {locale:string}")] +#[expect( + clippy::unnecessary_wraps, + reason = "rstest-bdd macro generates Result wrapper; FIXME: https://github.com/leynos/rstest-bdd/issues/381" +)] +fn set_environment_locale_override(world: &TestWorld, locale: &str) -> Result<()> { + ensure_env_lock(world); + 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)) }; + world.track_env_var(LOCALE_ENV_VAR.to_owned(), previous); + Ok(()) +} + #[given("the Netsuke config file sets output format to {format:string}")] fn config_sets_output_format(world: &TestWorld, format: &str) -> Result<()> { write_config(world, &format!("output_format = \"{format}\"\n")) diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index 74b47df5..c35402a9 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -60,20 +60,28 @@ fn assert_build_targets( #[derive(Debug)] enum ExpectedValidationError { - ThemeAliasConflict, - SpinnerProgressConflict, + ThemeUnicodeAliasConflict, + ThemeAsciiAliasConflict, + SpinnerDisabledConflict, + SpinnerEnabledConflict, UnsupportedOutputFormat, } impl ExpectedValidationError { fn expected_fragment(&self) -> &'static str { match self { - Self::ThemeAliasConflict => { + Self::ThemeUnicodeAliasConflict => { "theme = \"unicode\" conflicts with no_emoji = true" } - Self::SpinnerProgressConflict => { + Self::ThemeAsciiAliasConflict => { + "no_emoji = false conflicts with theme = \"ascii\"" + } + Self::SpinnerDisabledConflict => { "spinner_mode = \"disabled\" conflicts with progress = true" } + Self::SpinnerEnabledConflict => { + "progress = false conflicts with spinner_mode = \"enabled\"" + } Self::UnsupportedOutputFormat => { "output_format = \"json\" is not supported yet" } @@ -283,11 +291,19 @@ targets = ["all"] #[rstest] #[case( json!({ "theme": "unicode", "no_emoji": true }), - ExpectedValidationError::ThemeAliasConflict, + ExpectedValidationError::ThemeUnicodeAliasConflict, +)] +#[case( + json!({ "theme": "ascii", "no_emoji": false }), + ExpectedValidationError::ThemeAsciiAliasConflict, )] #[case( json!({ "spinner_mode": "disabled", "progress": true }), - ExpectedValidationError::SpinnerProgressConflict, + ExpectedValidationError::SpinnerDisabledConflict, +)] +#[case( + json!({ "spinner_mode": "enabled", "progress": false }), + ExpectedValidationError::SpinnerEnabledConflict, )] #[case( json!({ "output_format": "json" }), diff --git a/tests/cli_tests/parsing.rs b/tests/cli_tests/parsing.rs index b77980ac..2f880575 100644 --- a/tests/cli_tests/parsing.rs +++ b/tests/cli_tests/parsing.rs @@ -3,8 +3,9 @@ use anyhow::{Context, Result, ensure}; use clap::Parser; use clap::error::ErrorKind; -use netsuke::cli::{BuildArgs, Cli, Commands}; +use netsuke::cli::{BuildArgs, Cli, Commands, Theme}; use netsuke::host_pattern::HostPattern; +use netsuke::output_prefs; use rstest::rstest; use std::path::PathBuf; @@ -209,3 +210,41 @@ fn parse_cli_errors(#[case] argv: Vec<&str>, #[case] expected_error: ErrorKind) ); Ok(()) } + +#[test] +fn no_emoji_override_false_defers_to_environment_suppression() -> Result<()> { + let cli = Cli { + no_emoji: Some(false), + ..Cli::default() + }; + + let prefs = output_prefs::resolve_with(cli.no_emoji_override(), |key| match key { + "NETSUKE_NO_EMOJI" => Some(String::from("1")), + _ => None, + }); + + ensure!( + !prefs.emoji_allowed(), + "no_emoji = false should defer to environment suppression", + ); + Ok(()) +} + +#[test] +fn no_emoji_override_honours_unicode_theme_over_environment_suppression() -> Result<()> { + let cli = Cli { + theme: Some(Theme::Unicode), + ..Cli::default() + }; + + let prefs = output_prefs::resolve_with(cli.no_emoji_override(), |key| match key { + "NETSUKE_NO_EMOJI" => Some(String::from("1")), + _ => None, + }); + + ensure!( + prefs.emoji_allowed(), + "theme = unicode should remain authoritative over environment suppression", + ); + Ok(()) +} diff --git a/tests/features/configuration_preferences.feature b/tests/features/configuration_preferences.feature index b798cb61..7bc656ad 100644 --- a/tests/features/configuration_preferences.feature +++ b/tests/features/configuration_preferences.feature @@ -10,6 +10,7 @@ Feature: Configuration preferences Scenario: CLI locale and verbose flags override configuration and environment Given an empty workspace And the Netsuke config file sets locale to "es-ES" + And the NETSUKE_LOCALE environment variable is "fr-FR" When the CLI is parsed and merged with "--locale en-US --verbose" Then parsing succeeds And the merged locale is "en-US" From 25bc68ee8137ef4b21db579b5dbf9b1b79d75518 Mon Sep 17 00:00:00 2001 From: Leynos Date: Fri, 13 Mar 2026 18:10:15 +0000 Subject: [PATCH 09/12] docs(users-guide): fix comma usage in users guide for clarity Co-authored-by: devboxerhub[bot] --- docs/users-guide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/users-guide.md b/docs/users-guide.md index 635f3d8f..24190165 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -610,7 +610,7 @@ the user explicitly sets `progress = true`, which is treated as a conflict. Likewise, `spinner_mode = "enabled"` conflicts with `progress = false`. `output_format = "json"` is intentionally rejected for now. Roadmap item -`3.10.3` will add JSON diagnostics; until then the only supported value is +`3.10.3` will add JSON diagnostics; until then, the only supported value is `"human"`. `colour_policy` is accepted and layered today so users can standardize their From 82a1f69658529ba2fdd0087737068d8e91721da7 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 14 Mar 2026 00:37:43 +0000 Subject: [PATCH 10/12] style: format rebased cli changes 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. --- src/cli/parser.rs | 2 +- src/runner/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cli/parser.rs b/src/cli/parser.rs index 2d4469f6..f230a851 100644 --- a/src/cli/parser.rs +++ b/src/cli/parser.rs @@ -13,8 +13,8 @@ use std::sync::Arc; use super::config::CliConfig; use super::parsing::{parse_host_pattern, parse_jobs, parse_locale, parse_scheme}; use super::{ColourPolicy, OutputFormat, SpinnerMode, Theme}; -pub use crate::cli_l10n::{diag_json_hint_from_args, locale_hint_from_args}; use crate::cli_l10n::localize_command; +pub use crate::cli_l10n::{diag_json_hint_from_args, locale_hint_from_args}; use crate::host_pattern::HostPattern; #[derive(Clone)] diff --git a/src/runner/mod.rs b/src/runner/mod.rs index 17011dab..416c4821 100644 --- a/src/runner/mod.rs +++ b/src/runner/mod.rs @@ -136,7 +136,7 @@ const fn should_force_text_task_updates(mode: OutputMode, stdout_is_tty: bool) - /// Returns an error if manifest generation or the Ninja process fails. pub fn run(cli: &Cli, prefs: OutputPrefs) -> Result<()> { let mode = output_mode::resolve(cli.accessible); - let progress_enabled = cli.progress_enabled() && !cli.diag_json; + let progress_enabled = cli.progress_enabled() && !cli.diag_json; let stdout_is_tty = std::io::stdout().is_terminal(); let reporter = make_reporter(ReporterOptions { mode, From fe94660c9a3a039854b987f1b34aabf8f988c650 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 14 Mar 2026 12:09:59 +0000 Subject: [PATCH 11/12] refactor(cli): split CLI parsing and config to separate modules 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] --- docs/netsuke-design.md | 11 ++++++----- docs/users-guide.md | 2 +- src/cli/parser.rs | 8 +++++++- test_support/src/netsuke.rs | 5 +++++ tests/cli_tests/merge.rs | 6 ++++-- 5 files changed, 23 insertions(+), 9 deletions(-) diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index c8bb25cf..ef64855a 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2029,11 +2029,12 @@ the targets listed in the `defaults` section of the manifest are built. ### 8.4 Design Decisions -The CLI is implemented using clap's derive API in `src/cli/mod.rs`, but the -parser-facing `Cli` type is no longer the merge schema. Layered configuration -now lives in a dedicated `CliConfig` struct derived with OrthoConfig. This -separation keeps parsing, configuration discovery, and runtime command -selection as distinct concerns while preserving the existing command syntax. +The parser-facing `Cli` type is now defined in `src/cli/parser.rs`, while +layered configuration lives in a dedicated `CliConfig` struct derived with +OrthoConfig in `src/cli/config.rs`. The top-level `src/cli/mod.rs` module +re-exports that public CLI surface. This separation keeps parsing, +configuration discovery, and runtime command selection as distinct concerns +while preserving the existing command syntax. Invoking `netsuke` with no explicit subcommand still resolves to `build`, and the `build` command can now take default `emit` and `targets` values from `[cmds.build]` in configuration files or `NETSUKE_CMDS__BUILD__*` environment diff --git a/docs/users-guide.md b/docs/users-guide.md index 24190165..65e3c316 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -613,7 +613,7 @@ Likewise, `spinner_mode = "enabled"` conflicts with `progress = false`. `3.10.3` will add JSON diagnostics; until then, the only supported value is `"human"`. -`colour_policy` is accepted and layered today so users can standardize their +`colour_policy` is accepted and layered today, so users can standardize their preferred setting, but Netsuke does not yet emit coloured terminal output, so this value currently has no visible effect. diff --git a/src/cli/parser.rs b/src/cli/parser.rs index f230a851..5d8ca121 100644 --- a/src/cli/parser.rs +++ b/src/cli/parser.rs @@ -65,7 +65,12 @@ pub(super) fn validation_message( #[command(author, version, about, long_about = None)] pub struct Cli { /// Path to the Netsuke manifest file to use. - #[arg(short, long, value_name = "FILE", default_value = "Netsukefile")] + #[arg( + short, + long, + value_name = "FILE", + default_value_os_t = CliConfig::default_manifest_path() + )] pub file: PathBuf, /// Run as if started in this directory. @@ -302,5 +307,6 @@ fn configure_validation_parsers( }); command } + /// Maximum number of jobs accepted by the CLI. pub(super) const MAX_JOBS: usize = 64; diff --git a/test_support/src/netsuke.rs b/test_support/src/netsuke.rs index c6f3b3f8..bd4a4299 100644 --- a/test_support/src/netsuke.rs +++ b/test_support/src/netsuke.rs @@ -53,10 +53,15 @@ pub struct NetsukeRun { /// Returns an error when `netsuke` cannot be located or the process cannot be /// spawned. pub fn run_netsuke_in(current_dir: &Path, args: &[&str]) -> Result { + let isolated_config_home = current_dir.join(".config"); let mut cmd = assert_cmd::Command::new(netsuke_executable()?); let output = cmd .current_dir(current_dir) .env("PATH", "") + .env_remove("NETSUKE_CONFIG_PATH") + .env_remove("NETSUKE_OUTPUT_FORMAT") + .env("HOME", current_dir) + .env("XDG_CONFIG_HOME", &isolated_config_home) .args(args) .output() .context("run netsuke command")?; diff --git a/tests/cli_tests/merge.rs b/tests/cli_tests/merge.rs index c35402a9..a0628250 100644 --- a/tests/cli_tests/merge.rs +++ b/tests/cli_tests/merge.rs @@ -104,8 +104,10 @@ fn assert_merge_rejects( file_layer: serde_json::Value, expected_error: ExpectedValidationError, ) -> anyhow::Result<()> { - let err = merge_defaults_with_file_layer(defaults, file_layer) - .expect_err("merge should have returned an error"); + let err = match merge_defaults_with_file_layer(defaults, file_layer) { + Ok(value) => anyhow::bail!("merge should have failed, got {value:#?}"), + Err(err) => err, + }; ensure!( err.to_string().contains(expected_error.expected_fragment()), "unexpected error text: {err}", From 0262cde684f5e22e5b95059653ba223e9968193a Mon Sep 17 00:00:00 2001 From: Leynos Date: Mon, 23 Mar 2026 00:01:59 +0000 Subject: [PATCH 12/12] Add explicit CLI overrides with typed enums; update tests/docs/build (#276) * refactor(tests): use EnvVarGuard for safer env var handling Refactored environment variable setting in BDD configuration preference tests by introducing EnvVarGuard to manage environment state more safely. This replaces unsafe direct std::env::set_var calls and improves test reliability. Additionally, minor spacing cleanup was done in documentation files. Co-authored-by: devboxerhub[bot] * feat(cli): add CLI options to override output appearance settings Add support for overriding colour policy, spinner mode, output format, and theme via CLI options. Derive ValueEnum for these enums to enable command line parsing integration. Update config merging logic to respect these new CLI flags. Refactor build script by splitting main function into smaller functions for clarity and maintainability. Co-authored-by: devboxerhub[bot] * refactor(tests): remove EnvVarGuard and directly set env vars with unsafe Replaced usage of the EnvVarGuard helper with direct calls to std::env::set_var inside unsafe blocks in BDD test steps. This simplifies environment variable management by manually tracking previous values without using the guard abstraction. Co-authored-by: devboxerhub[bot] * test(configuration): add BDD tests for theme, colour policy, and spinner mode precedence - Added various BDD step definitions for setting and verifying configuration preferences - Covered config file, environment variable, and CLI flag precedence for theme, colour policy, and spinner mode - Extended feature file with scenarios validating correct merge and override behavior among sources Co-authored-by: devboxerhub[bot] * refactor(tests/bdd): use typed enums for config and env var steps Replaced string-based configuration for theme, colour policy, spinner mode, and output format in BDD test steps with the corresponding typed enums from clap's ValueEnum trait. Added helper functions to write config values and set environment variables using enum canonical names, and unified assertion logic for merged CLI fields. This improves type safety, validation, and reduces code duplication in the BDD tests for configuration preference handling. Co-authored-by: devboxerhub[bot] --------- Co-authored-by: devboxerhub[bot] --- build.rs | 25 +-- ...-10-1-guarantee-status-message-ordering.md | 9 +- docs/netsuke-design.md | 11 +- src/cli/config.rs | 9 +- src/cli/merge.rs | 4 + src/cli/parser.rs | 16 +- tests/bdd/steps/configuration_preferences.rs | 176 ++++++++++++++++-- .../configuration_preferences.feature | 66 +++++++ 8 files changed, 269 insertions(+), 47 deletions(-) diff --git a/build.rs b/build.rs index 696f7eed..5e9307b2 100644 --- a/build.rs +++ b/build.rs @@ -88,7 +88,7 @@ fn write_man_page(data: &[u8], dir: &Path, page_name: &str) -> std::io::Result

Result<(), Box> { +const fn verify_public_api_symbols() { // Exercise CLI localization, config merge, and host pattern symbols so the // shared modules remain linked when the build script is compiled without // tests. @@ -107,8 +107,9 @@ fn main() -> Result<(), Box> { const _: fn(&cli::Cli) -> bool = cli::Cli::progress_enabled; const _: fn(&str) -> Result = HostPattern::parse; const _: fn(&HostPattern, host_pattern::HostCandidate<'_>) -> bool = HostPattern::matches; +} - // Regenerate the manual page when the CLI or metadata changes. +fn emit_rerun_directives() { println!("cargo:rerun-if-changed=src/cli/mod.rs"); println!("cargo:rerun-if-changed=src/cli/config.rs"); println!("cargo:rerun-if-changed=src/cli/merge.rs"); @@ -125,13 +126,9 @@ fn main() -> Result<(), Box> { println!("cargo:rerun-if-changed=src/localization/keys.rs"); println!("cargo:rerun-if-changed=locales/en-US/messages.ftl"); println!("cargo:rerun-if-changed=locales/es-ES/messages.ftl"); +} - build_l10n_audit::audit_localization_keys()?; - - // Packagers expect man pages under target/generated-man//. - let out_dir = out_dir_for_target_profile(); - - // The top-level page documents the entire command interface. +fn generate_man_page(out_dir: &Path) -> Result<(), Box> { let cmd = cli::Cli::command(); let name = cmd .get_bin_name() @@ -149,7 +146,6 @@ fn main() -> Result<(), Box> { let version = env::var("CARGO_PKG_VERSION").map_err( |_| "CARGO_PKG_VERSION must be set by Cargo; cannot render manual page without it.", )?; - let man = Man::new(cmd) .section("1") .source(format!("{cargo_bin} {version}")) @@ -157,7 +153,7 @@ fn main() -> Result<(), Box> { let mut buf = Vec::new(); man.render(&mut buf)?; let page_name = format!("{cargo_bin}.1"); - write_man_page(&buf, &out_dir, &page_name)?; + write_man_page(&buf, out_dir, &page_name)?; if let Some(extra_dir) = env::var_os("OUT_DIR") { let extra_dir_path = PathBuf::from(extra_dir); if let Err(err) = write_man_page(&buf, &extra_dir_path, &page_name) { @@ -167,6 +163,13 @@ fn main() -> Result<(), Box> { ); } } - Ok(()) } + +fn main() -> Result<(), Box> { + verify_public_api_symbols(); + emit_rerun_directives(); + build_l10n_audit::audit_localization_keys()?; + let out_dir = out_dir_for_target_profile(); + generate_man_page(&out_dir) +} diff --git a/docs/execplans/3-10-1-guarantee-status-message-ordering.md b/docs/execplans/3-10-1-guarantee-status-message-ordering.md index 33674739..29fcb3c6 100644 --- a/docs/execplans/3-10-1-guarantee-status-message-ordering.md +++ b/docs/execplans/3-10-1-guarantee-status-message-ordering.md @@ -276,8 +276,7 @@ model provides the necessary ordering guarantees. `NINJA_STDERR_MARKER`) to avoid coupling to localized UI strings. 2. **Status messages do not contaminate stdout in standard mode**: Verifies - stream - routing in non-accessible mode using the same stable markers. + stream routing in non-accessible mode using the same stable markers. 3. **Build artifacts can be captured via stdout redirection**: Verifies that `netsuke manifest -` output goes to stdout without status contamination. @@ -285,12 +284,10 @@ model provides the necessary ordering guarantees. Supporting infrastructure added: - `FakeNinjaConfig` struct in `tests/bdd/steps/progress_output.rs` for - configurable - fixture generation with optional stderr markers. + configurable fixture generation with optional stderr markers. - `install_fake_ninja_with_config()` function for flexible fixture setup. - Updated `fake_ninja_emits_stdout_output` fixture to emit both stdout and - stderr - markers for comprehensive stream routing verification. + stderr markers for comprehensive stream routing verification. ### Stage E: Documentation (completed) diff --git a/docs/netsuke-design.md b/docs/netsuke-design.md index ef64855a..4990c38d 100644 --- a/docs/netsuke-design.md +++ b/docs/netsuke-design.md @@ -2034,12 +2034,11 @@ layered configuration lives in a dedicated `CliConfig` struct derived with OrthoConfig in `src/cli/config.rs`. The top-level `src/cli/mod.rs` module re-exports that public CLI surface. This separation keeps parsing, configuration discovery, and runtime command selection as distinct concerns -while preserving the existing command syntax. -Invoking `netsuke` with no explicit subcommand still resolves to `build`, and -the `build` command can now take default `emit` and `targets` values from -`[cmds.build]` in configuration files or `NETSUKE_CMDS__BUILD__*` environment -variables. Explicit CLI targets or `--emit` values still override those -defaults. +while preserving the existing command syntax. Invoking `netsuke` with no +explicit subcommand still resolves to `build`, and the `build` command can now +take default `emit` and `targets` values from `[cmds.build]` in configuration +files or `NETSUKE_CMDS__BUILD__*` environment variables. Explicit CLI targets +or `--emit` values still override those defaults. Configuration is layered in the order defaults -> configuration files -> environment variables -> CLI overrides. Discovery honours `NETSUKE_CONFIG_PATH` diff --git a/src/cli/config.rs b/src/cli/config.rs index 1373fb73..587fc55f 100644 --- a/src/cli/config.rs +++ b/src/cli/config.rs @@ -4,6 +4,7 @@ //! and merging. It captures global CLI settings plus per-subcommand defaults //! under the `cmds` namespace. +use clap::ValueEnum; use ortho_config::{OrthoConfig, OrthoResult, PostMergeContext, PostMergeHook}; use serde::{Deserialize, Serialize}; use std::path::PathBuf; @@ -12,7 +13,7 @@ use super::validation_error; use crate::host_pattern::HostPattern; /// Colour-output policy accepted by layered configuration. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, ValueEnum, Default)] #[serde(rename_all = "kebab-case")] pub enum ColourPolicy { /// Follow the host environment. @@ -25,7 +26,7 @@ pub enum ColourPolicy { } /// Spinner and progress rendering policy. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, ValueEnum, Default)] #[serde(rename_all = "kebab-case")] pub enum SpinnerMode { /// Follow Netsuke's default progress behaviour. @@ -38,7 +39,7 @@ pub enum SpinnerMode { } /// Top-level diagnostics and output format. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, ValueEnum, Default)] #[serde(rename_all = "kebab-case")] pub enum OutputFormat { /// Human-readable terminal output. @@ -49,7 +50,7 @@ pub enum OutputFormat { } /// Presentation theme for semantic prefixes and glyph choices. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, ValueEnum, Default)] #[serde(rename_all = "kebab-case")] pub enum Theme { /// Follow the host environment. diff --git a/src/cli/merge.rs b/src/cli/merge.rs index 99b84b9c..03105b80 100644 --- a/src/cli/merge.rs +++ b/src/cli/merge.rs @@ -155,6 +155,10 @@ fn cli_overrides_from_matches(cli: &Cli, matches: &ArgMatches) -> OrthoResult, - /// Resolved colour policy from layered configuration. - #[arg(skip)] + /// Override colour policy for terminal output. + #[arg(long, value_name = "POLICY")] pub colour_policy: Option, - /// Resolved spinner mode from layered configuration. - #[arg(skip)] + /// Override spinner animation mode. + #[arg(long, value_name = "MODE")] pub spinner_mode: Option, - /// Resolved output format from layered configuration. - #[arg(skip)] + /// Override output format style. + #[arg(long, value_name = "FORMAT")] pub output_format: Option, - /// Resolved presentation theme from layered configuration. - #[arg(skip)] + /// Override presentation theme. + #[arg(long, value_name = "THEME")] pub theme: Option, /// Optional subcommand to execute; defaults to `build` when omitted. diff --git a/tests/bdd/steps/configuration_preferences.rs b/tests/bdd/steps/configuration_preferences.rs index 11d14fad..a245c7c7 100644 --- a/tests/bdd/steps/configuration_preferences.rs +++ b/tests/bdd/steps/configuration_preferences.rs @@ -4,7 +4,8 @@ use crate::bdd::fixtures::{RefCellOptionExt, TestWorld}; use crate::bdd::helpers::parse_store::store_parse_outcome; use crate::bdd::helpers::tokens::build_tokens; use anyhow::{Context, Result, anyhow, bail, ensure}; -use netsuke::cli::{Cli, Commands, Theme}; +use clap::ValueEnum as _; +use netsuke::cli::{Cli, ColourPolicy, Commands, OutputFormat, SpinnerMode, Theme}; use netsuke::cli_localization; use netsuke::output_prefs; use rstest_bdd_macros::{given, then, when}; @@ -39,7 +40,7 @@ fn write_config(world: &TestWorld, contents: &str) -> Result<()> { fs::write(&path, contents).with_context(|| format!("write {}", path.display()))?; 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())) }; + unsafe { std::env::set_var(CONFIG_ENV_VAR, path.as_os_str()) }; world.track_env_var(CONFIG_ENV_VAR.to_owned(), previous); Ok(()) } @@ -61,6 +62,93 @@ fn merge_cli(world: &TestWorld, args: &str) { store_parse_outcome(&world.cli, &world.cli_error, outcome); } +/// Convert a clap `ValueEnum` to its canonical name string. +/// +/// # Panics +/// +/// Panics if the enum variant does not have a possible value (which should +/// never happen for well-formed `ValueEnum` implementations). +fn enum_name(value: &T) -> String { + value + .to_possible_value() + .unwrap_or_else(|| panic!("all ValueEnum variants must have a possible value")) + .get_name() + .to_owned() +} + +/// Write a theme value to the config file. +fn write_theme_config(world: &TestWorld, theme: Theme) -> Result<()> { + write_config(world, &format!("theme = \"{}\"\n", enum_name(&theme))) +} + +/// Write a colour policy value to the config file. +fn write_colour_policy_config(world: &TestWorld, policy: ColourPolicy) -> Result<()> { + write_config( + world, + &format!("colour_policy = \"{}\"\n", enum_name(&policy)), + ) +} + +/// Write a spinner mode value to the config file. +fn write_spinner_mode_config(world: &TestWorld, mode: SpinnerMode) -> Result<()> { + write_config(world, &format!("spinner_mode = \"{}\"\n", enum_name(&mode))) +} + +/// Write an output format value to the config file. +fn write_output_format_config(world: &TestWorld, format: OutputFormat) -> Result<()> { + write_config( + world, + &format!("output_format = \"{}\"\n", enum_name(&format)), + ) +} + +/// Set the `NETSUKE_THEME` environment variable. +fn set_env_theme(world: &TestWorld, theme: Theme) { + ensure_env_lock(world); + let previous = std::env::var_os("NETSUKE_THEME"); + let value = enum_name(&theme); + // SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario. + unsafe { std::env::set_var("NETSUKE_THEME", OsStr::new(&value)) }; + world.track_env_var("NETSUKE_THEME".to_owned(), previous); +} + +/// Set the `NETSUKE_COLOUR_POLICY` environment variable. +fn set_env_colour_policy(world: &TestWorld, policy: ColourPolicy) { + ensure_env_lock(world); + let previous = std::env::var_os("NETSUKE_COLOUR_POLICY"); + let value = enum_name(&policy); + // SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario. + unsafe { std::env::set_var("NETSUKE_COLOUR_POLICY", OsStr::new(&value)) }; + world.track_env_var("NETSUKE_COLOUR_POLICY".to_owned(), previous); +} + +/// Set the `NETSUKE_SPINNER_MODE` environment variable. +fn set_env_spinner_mode(world: &TestWorld, mode: SpinnerMode) { + ensure_env_lock(world); + let previous = std::env::var_os("NETSUKE_SPINNER_MODE"); + let value = enum_name(&mode); + // SAFETY: `EnvLock` is held in `world.env_lock` for the lifetime of the scenario. + unsafe { std::env::set_var("NETSUKE_SPINNER_MODE", OsStr::new(&value)) }; + world.track_env_var("NETSUKE_SPINNER_MODE".to_owned(), previous); +} + +/// Assert a merged CLI field value matches the expected value. +fn assert_merged_field(world: &TestWorld, extract: F, expected: T, label: &str) -> Result<()> +where + T: Copy + PartialEq + std::fmt::Debug, + F: FnOnce(&Cli) -> Option, +{ + let value = world + .cli + .with_ref(|cli| extract(cli)) + .context("expected merged CLI to be available")?; + ensure!( + value == Some(expected), + "expected merged {label} to be {expected:?}, got {value:?}", + ); + Ok(()) +} + #[given("the Netsuke config file sets build targets to {target:string}")] fn config_sets_build_targets(world: &TestWorld, target: &str) -> Result<()> { write_config(world, &format!("[cmds.build]\ntargets = [\"{target}\"]\n")) @@ -87,7 +175,9 @@ fn set_environment_locale_override(world: &TestWorld, locale: &str) -> Result<() #[given("the Netsuke config file sets output format to {format:string}")] fn config_sets_output_format(world: &TestWorld, format: &str) -> Result<()> { - write_config(world, &format!("output_format = \"{format}\"\n")) + let typed = OutputFormat::from_str(format, true) + .map_err(|err| anyhow!("invalid output format '{format}': {err}"))?; + write_output_format_config(world, typed) } #[given("the Netsuke config file sets no_emoji to true")] @@ -95,6 +185,51 @@ fn config_sets_no_emoji(world: &TestWorld) -> Result<()> { write_config(world, "no_emoji = true\n") } +#[given("the Netsuke config file sets theme to {theme:string}")] +fn config_sets_theme(world: &TestWorld, theme: &str) -> Result<()> { + let typed = + Theme::from_str(theme, true).map_err(|err| anyhow!("invalid theme '{theme}': {err}"))?; + write_theme_config(world, typed) +} + +#[given("the Netsuke config file sets colour policy to {policy:string}")] +fn config_sets_colour_policy(world: &TestWorld, policy: &str) -> Result<()> { + let typed = ColourPolicy::from_str(policy, true) + .map_err(|err| anyhow!("invalid colour policy '{policy}': {err}"))?; + write_colour_policy_config(world, typed) +} + +#[given("the Netsuke config file sets spinner mode to {mode:string}")] +fn config_sets_spinner_mode(world: &TestWorld, mode: &str) -> Result<()> { + let typed = SpinnerMode::from_str(mode, true) + .map_err(|err| anyhow!("invalid spinner mode '{mode}': {err}"))?; + write_spinner_mode_config(world, typed) +} + +#[given("the NETSUKE_THEME environment variable is {theme:string}")] +fn set_environment_theme_override(world: &TestWorld, theme: &str) -> Result<()> { + let typed = + Theme::from_str(theme, true).map_err(|err| anyhow!("invalid theme '{theme}': {err}"))?; + set_env_theme(world, typed); + Ok(()) +} + +#[given("the NETSUKE_COLOUR_POLICY environment variable is {policy:string}")] +fn set_environment_colour_policy_override(world: &TestWorld, policy: &str) -> Result<()> { + let typed = ColourPolicy::from_str(policy, true) + .map_err(|err| anyhow!("invalid colour policy '{policy}': {err}"))?; + set_env_colour_policy(world, typed); + Ok(()) +} + +#[given("the NETSUKE_SPINNER_MODE environment variable is {mode:string}")] +fn set_environment_spinner_mode_override(world: &TestWorld, mode: &str) -> Result<()> { + let typed = SpinnerMode::from_str(mode, true) + .map_err(|err| anyhow!("invalid spinner mode '{mode}': {err}"))?; + set_env_spinner_mode(world, typed); + Ok(()) +} + #[expect( clippy::unnecessary_wraps, reason = "rstest-bdd macro generates Result wrapper; FIXME: https://github.com/leynos/rstest-bdd/issues/381" @@ -158,15 +293,7 @@ fn merged_verbose_enabled(world: &TestWorld) -> Result<()> { #[then("the merged theme is ascii")] fn merged_theme_is_ascii(world: &TestWorld) -> Result<()> { - let theme = world - .cli - .with_ref(|cli| cli.theme) - .context("expected merged CLI to be available")?; - ensure!( - theme == Some(Theme::Ascii), - "expected merged theme to be ASCII, got {theme:?}", - ); - Ok(()) + assert_merged_field(world, |cli| cli.theme, Theme::Ascii, "theme") } #[then("the merge error should contain {fragment:string}")] @@ -181,3 +308,28 @@ fn merge_error_contains(world: &TestWorld, fragment: &str) -> Result<()> { ); Ok(()) } + +#[then("the merged theme is unicode")] +fn merged_theme_is_unicode(world: &TestWorld) -> Result<()> { + assert_merged_field(world, |cli| cli.theme, Theme::Unicode, "theme") +} + +#[then("the merged colour policy is always")] +fn merged_colour_policy_is_always(world: &TestWorld) -> Result<()> { + assert_merged_field( + world, + |cli| cli.colour_policy, + ColourPolicy::Always, + "colour policy", + ) +} + +#[then("the merged spinner mode is enabled")] +fn merged_spinner_mode_is_enabled(world: &TestWorld) -> Result<()> { + assert_merged_field( + world, + |cli| cli.spinner_mode, + SpinnerMode::Enabled, + "spinner mode", + ) +} diff --git a/tests/features/configuration_preferences.feature b/tests/features/configuration_preferences.feature index 7bc656ad..187a7c37 100644 --- a/tests/features/configuration_preferences.feature +++ b/tests/features/configuration_preferences.feature @@ -32,3 +32,69 @@ Feature: Configuration preferences Then parsing succeeds And the merged theme is ascii And the prefix contains no non-ASCII characters + + Scenario: CLI theme flag overrides configuration file + Given an empty workspace + And the Netsuke config file sets theme to "unicode" + When the CLI is parsed and merged with "--theme ascii" + Then parsing succeeds + And the merged theme is ascii + + Scenario: CLI theme flag overrides environment variable + Given an empty workspace + And the NETSUKE_THEME environment variable is "unicode" + When the CLI is parsed and merged with "--theme ascii" + Then parsing succeeds + And the merged theme is ascii + + Scenario: CLI theme flag has highest precedence over env and config + Given an empty workspace + And the Netsuke config file sets theme to "unicode" + And the NETSUKE_THEME environment variable is "auto" + When the CLI is parsed and merged with "--theme ascii" + Then parsing succeeds + And the merged theme is ascii + + Scenario: CLI colour policy flag overrides configuration file + Given an empty workspace + And the Netsuke config file sets colour policy to "never" + When the CLI is parsed and merged with "--colour-policy always" + Then parsing succeeds + And the merged colour policy is always + + Scenario: CLI colour policy flag overrides environment variable + Given an empty workspace + And the NETSUKE_COLOUR_POLICY environment variable is "never" + When the CLI is parsed and merged with "--colour-policy always" + Then parsing succeeds + And the merged colour policy is always + + Scenario: CLI spinner mode flag overrides configuration file + Given an empty workspace + And the Netsuke config file sets spinner mode to "disabled" + When the CLI is parsed and merged with "--spinner-mode enabled" + Then parsing succeeds + And the merged spinner mode is enabled + + Scenario: CLI spinner mode flag overrides environment variable + Given an empty workspace + And the NETSUKE_SPINNER_MODE environment variable is "disabled" + When the CLI is parsed and merged with "--spinner-mode enabled" + Then parsing succeeds + And the merged spinner mode is enabled + + Scenario: Environment variable overrides configuration for theme + Given an empty workspace + And the Netsuke config file sets theme to "ascii" + And the NETSUKE_THEME environment variable is "unicode" + When the CLI is parsed and merged with "" + Then parsing succeeds + And the merged theme is unicode + + Scenario: Environment variable overrides configuration for colour policy + Given an empty workspace + And the Netsuke config file sets colour policy to "auto" + And the NETSUKE_COLOUR_POLICY environment variable is "always" + When the CLI is parsed and merged with "" + Then parsing succeeds + And the merged colour policy is always