From 3321b6f29c10ef1485bd2efe3e584e2c35905636 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 30 Oct 2026 15:59:39 +0100 Subject: [PATCH 01/10] Declare CLI value names on configuration fields The release-help generator falls back to machine value types such as `UTF8PATHBUF`, `USIZE`, and `LIST` whenever a configuration-derived option declares no placeholder. Declare the handwritten parser's human-readable names through `cli(value_name = "...")` so the manual page and PowerShell help describe the same value surface users type. Only derived documentation metadata changes: defaults, merge strategies, field types, and runtime parsing are untouched. Switches and `skip_cli` fields keep their existing treatment. Co-Authored-By: Claude Code --- src/cli/config/mod.rs | 48 ++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/src/cli/config/mod.rs b/src/cli/config/mod.rs index 9783bebd4..991c4cc1a 100644 --- a/src/cli/config/mod.rs +++ b/src/cli/config/mod.rs @@ -198,10 +198,11 @@ pub struct CommandConfigs { #[ortho_config(prefix = "NETSUKE", post_merge_hook)] pub struct CliConfig { /// Path to the Netsuke manifest file to use. - #[ortho_config(default = default_manifest_path())] + #[ortho_config(default = default_manifest_path(), cli(value_name = "FILE"))] pub file: Utf8PathBuf, /// Set the number of parallel build jobs. + #[ortho_config(cli(value_name = "N"))] pub jobs: Option, /// Enable verbose diagnostic logging and completion timing summaries. @@ -209,30 +210,31 @@ pub struct CliConfig { pub verbose: bool, /// Locale tag for CLI copy (for example: en-US, es-ES). + #[ortho_config(cli(value_name = "LOCALE"))] pub locale: Option, /// Additional URL schemes allowed for the `fetch` helper. - #[ortho_config(merge_strategy = "append")] + #[ortho_config(merge_strategy = "append", cli(value_name = "SCHEME"))] #[serde(default)] pub fetch_allow_scheme: Vec, /// Environment variables permitted for the manifest `env()` helper. - #[ortho_config(merge_strategy = "append")] + #[ortho_config(merge_strategy = "append", cli(value_name = "VAR"))] #[serde(default)] pub env_allow_var: Vec, /// Environment variables always blocked for the manifest `env()` helper. - #[ortho_config(merge_strategy = "append")] + #[ortho_config(merge_strategy = "append", cli(value_name = "VAR"))] #[serde(default)] pub env_block_var: Vec, /// Hostnames permitted when default deny is enabled. - #[ortho_config(merge_strategy = "append")] + #[ortho_config(merge_strategy = "append", cli(value_name = "HOST"))] #[serde(default)] pub fetch_allow_host: Vec, /// Hostnames that are always blocked. - #[ortho_config(merge_strategy = "append")] + #[ortho_config(merge_strategy = "append", cli(value_name = "HOST"))] #[serde(default)] pub fetch_block_host: Vec, @@ -245,31 +247,49 @@ pub struct CliConfig { pub trust_project_fetch_policy: bool, /// Maximum `MiniJinja` instructions allocated to one manifest evaluation. - #[ortho_config(default = DEFAULT_MANIFEST_EVALUATION_FUEL)] + #[ortho_config( + default = DEFAULT_MANIFEST_EVALUATION_FUEL, + cli(value_name = "FUEL") + )] pub manifest_evaluation_fuel: u64, /// Maximum `MiniJinja` instructions allocated across one manifest. - #[ortho_config(default = DEFAULT_MANIFEST_FUEL)] + #[ortho_config(default = DEFAULT_MANIFEST_FUEL, cli(value_name = "FUEL"))] pub manifest_fuel: u64, /// Maximum bytes emitted by one rendered manifest value. - #[ortho_config(default = DEFAULT_MANIFEST_RENDERED_VALUE_BYTES)] + #[ortho_config( + default = DEFAULT_MANIFEST_RENDERED_VALUE_BYTES, + cli(value_name = "BYTES") + )] pub manifest_rendered_value_bytes: usize, /// Maximum aggregate bytes emitted across all rendered manifest values. - #[ortho_config(default = DEFAULT_MANIFEST_RENDERED_BYTES)] + #[ortho_config( + default = DEFAULT_MANIFEST_RENDERED_BYTES, + cli(value_name = "BYTES") + )] pub manifest_rendered_manifest_bytes: usize, /// Maximum template and macro-import source bytes consumed per manifest. - #[ortho_config(default = DEFAULT_MANIFEST_SOURCE_BYTES)] + #[ortho_config( + default = DEFAULT_MANIFEST_SOURCE_BYTES, + cli(value_name = "BYTES") + )] pub manifest_source_bytes: usize, /// Maximum values consumed from each manifest `foreach` iterator. - #[ortho_config(default = DEFAULT_MANIFEST_FOREACH_CARDINALITY)] + #[ortho_config( + default = DEFAULT_MANIFEST_FOREACH_CARDINALITY, + cli(value_name = "COUNT") + )] pub manifest_foreach_cardinality: usize, /// Maximum expanded target and action entries per manifest. - #[ortho_config(default = DEFAULT_MANIFEST_EXPANDED_ENTRIES)] + #[ortho_config( + default = DEFAULT_MANIFEST_EXPANDED_ENTRIES, + cli(value_name = "COUNT") + )] pub manifest_expanded_entries: usize, /// Emit machine-readable JSON output. @@ -297,7 +317,7 @@ pub struct CliConfig { pub accessibility: AccessibilityPolicy, /// Compatibility alias for default build targets at the config root. - #[ortho_config(merge_strategy = "append")] + #[ortho_config(merge_strategy = "append", cli(value_name = "TARGET"))] #[serde(default)] pub default_targets: Vec, From 3697947a41e3f80ea5e2e168e9b24bba22a6014a Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 30 Oct 2026 16:05:54 +0100 Subject: [PATCH 02/10] Compare CLI value names against the parser Release help and the parser must agree on every value placeholder, not just on whether an option takes a value. Extend the parity test to require an explicitly declared Clap value name for each value-taking option and to match it against `cli.value_name`, then confirm the representative configuration fields are covered. Document the ownership split: `CliConfig` declares configuration-derived placeholders through `cli(value_name = "...")`, while `parser_fields.rs` stays responsible for parser-only selectors. A missing or disagreeing name now fails the test instead of reaching a release artefact as `UTF8PATHBUF` or `USIZE`. Co-Authored-By: Claude Code --- docs/developers-guide.md | 19 +++++++++++--- src/cli/release_help/metadata_tests.rs | 36 +++++++++++++++++++++++--- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index b520386f4..af10e5449 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -4069,9 +4069,10 @@ check. Release builds generate their manual and PowerShell help explicitly with `cargo-orthohelp`, rather than consuming the ordinary-build help artefacts from `build.rs`. The metadata root is `netsuke::cli::ReleaseHelpCli`, the sole -permitted composition site for release help. `CliConfig` supplies only layered -configuration fields; `Cli::command()` supplies parser-only flags such as -`-C/--directory` and `--config`, plus documented subcommands, including +permitted composition site for release help. `CliConfig` supplies the layered +configuration fields and owns each configuration-derived value placeholder +through `cli(value_name = "...")`; `Cli::command()` supplies parser-only flags +such as `-C/--directory` and `--config`, plus documented subcommands, including `help targets`. The adapter projects existing CLI Fluent keys onto published configuration fields and adds parser-only help metadata without adding an environment or file source. It omits the structural `cmds` container. Keep @@ -4111,6 +4112,18 @@ catalogue. The identifier guard collects `synopsis_id` alongside `help_id` and `about_id`, and the release-help metadata snapshot pins the published value, so a future derive default cannot reintroduce the placeholder silently. +Value placeholders follow the same ownership split. A configuration-derived +option declares its placeholder in `CliConfig` with `cli(value_name = "...")`; +without that declaration the generator falls back to a machine type such as +`UTF8PATHBUF`, `USIZE`, or `LIST`, and publishes it in the manual page and +`PowerShell` help. `src/cli/release_help/parser_fields.rs` remains responsible +only for parser-only selectors, which have no configuration source from which +to derive a placeholder. The parity test compares every value-taking option's +`cli.value_name` against the first value name `Cli::command()` declares, so +adding a value-taking option requires matching declarations in both +`src/cli/command.rs` and `src/cli/config/mod.rs`; a placeholder that disagrees +with the parser fails the test rather than reaching a release artefact. + Release automation delegates installation to [`install_orthohelp.py`](../scripts/install_orthohelp.py), with the pinned version supplied as `INPUT_VERSION`. The helper probes the cached binary and, diff --git a/src/cli/release_help/metadata_tests.rs b/src/cli/release_help/metadata_tests.rs index 875bf5e91..33b164f29 100644 --- a/src/cli/release_help/metadata_tests.rs +++ b/src/cli/release_help/metadata_tests.rs @@ -141,10 +141,11 @@ fn parser_only_metadata_requires_the_parser_argument(#[case] argument_id: &str) /// makes the parity a standing contract: a future metadata drift fails here /// instead of shipping a manual page the binary contradicts. /// -/// Value *names* are deliberately not compared. Netsuke sets no -/// `cli_value_name`, so the generated metadata has always reported `None` while -/// the handwritten parser supplies `FILE`, `N`, and friends; that gap predates -/// the v0.10.0 migration and is not this contract's subject. +/// Value names are compared too. `CliConfig` declares the placeholder for every +/// configuration-derived option through `cli(value_name = "...")`; without that +/// declaration the generator falls back to a machine type such as `UTF8PATHBUF` +/// or `USIZE`, which no reader of the manual page should see. A name missing +/// here, or one that disagrees with the parser, fails the test. #[test] fn release_help_cli_value_shapes_match_the_parser() -> Result<()> { let metadata = ReleaseHelpCli::try_get_doc_metadata()?; @@ -182,6 +183,23 @@ fn release_help_cli_value_shapes_match_the_parser() -> Result<()> { field.name ); if takes_value { + let parser_value_name = argument + .get_value_names() + .and_then(|names| names.first()) + .map(ToString::to_string); + ensure!( + parser_value_name.is_some(), + "parser argument {} should declare a value name to document", + field.name + ); + ensure!( + documentation.value_name == parser_value_name, + "release help should document the value placeholder {name} \ + parses: documented {documented:?}, parser {parsed:?}", + name = field.name, + documented = documentation.value_name, + parsed = parser_value_name, + ); ensure!( documentation.possible_values == argument @@ -218,6 +236,16 @@ fn release_help_cli_value_shapes_match_the_parser() -> Result<()> { "release help should compare {expected} against the parser: {compared:?}" ); } + + // Value-taking fields are where the generator's `UTF8PATHBUF` and `USIZE` + // fallbacks appeared, so their absence from the comparison would hide the + // very regression this contract now guards against. + for expected in ["file", "jobs", "locale", "manifest_fuel", "default_targets"] { + ensure!( + compared.contains(&expected), + "release help should compare the value placeholder for {expected}: {compared:?}" + ); + } Ok(()) } From 07c03dfd7b0f420698424517edc5e816a1f18b0e Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 30 Oct 2026 17:36:33 +0100 Subject: [PATCH 03/10] Split the CLI value-shape parity test into helpers Adding value-name comparison pushed the parity test past Clippy's function-length limit. Extract argument lookup, shape comparison, and value-taking comparison into documented helpers so each has one reason to change and the test reads as its contract statement. Behaviour is unchanged: the focused release-help suite still passes. Co-Authored-By: Claude Code --- src/cli/release_help/metadata_tests.rs | 168 +++++++++++++++---------- 1 file changed, 103 insertions(+), 65 deletions(-) diff --git a/src/cli/release_help/metadata_tests.rs b/src/cli/release_help/metadata_tests.rs index 33b164f29..f7049c2e5 100644 --- a/src/cli/release_help/metadata_tests.rs +++ b/src/cli/release_help/metadata_tests.rs @@ -6,7 +6,8 @@ use super::*; use anyhow::{Context, Result, ensure}; -use clap::Command; +use clap::{Arg, Command}; +use ortho_config::docs::CliMetadata; use rstest::rstest; use super::parser_fields::documented_clap_parser_field; @@ -132,6 +133,105 @@ fn parser_only_metadata_requires_the_parser_argument(#[case] argument_id: &str) Ok(()) } +/// Find the parser argument a documented field claims to describe. +/// +/// # Errors +/// +/// Returns an error when the parser accepts no argument with that identifier, +/// which would mean the manual page advertises a flag the binary rejects. +fn find_parser_argument<'a>(command: &'a Command, field_name: &str) -> Result<&'a Arg> { + command + .get_arguments() + .find(|argument| argument.get_id().as_str() == field_name) + .with_context(|| { + format!( + "release help documents {field_name}, but the parser accepts no such \ + argument; the manual page would advertise a flag this binary rejects" + ) + }) +} + +/// Verify one documented field against the parser argument it describes. +/// +/// # Errors +/// +/// Returns an error when the value shape, placeholder, or accepted values +/// diverge from what the parser accepts. +fn compare_argument_value_shape( + field: &FieldMetadata, + documentation: &CliMetadata, + argument: &Arg, +) -> Result<()> { + let takes_value = argument.get_action().takes_values(); + ensure!( + documentation.takes_value == takes_value, + "release help should document the value shape {name} parses: \ + documented {documented}, parser {parsed}", + name = field.name, + documented = documentation.takes_value, + parsed = takes_value, + ); + ensure!( + !documentation.value_optional, + "release help must not offer the optional-value spelling for {}", + field.name + ); + if takes_value { + compare_value_taking_argument(field, documentation, argument)?; + } else { + // A presence-only switch parses no value at all. Clap still records + // `possible_values` for `SetTrue` actions, so copying them would + // describe a value surface no invocation can reach. + ensure!( + documentation.value_name.is_none() && documentation.possible_values.is_empty(), + "release help should describe {} as a switch with no value", + field.name + ); + } + Ok(()) +} + +/// Verify the placeholder and accepted values of a value-taking argument. +/// +/// # Errors +/// +/// Returns an error when the parser declares no value name, when the documented +/// placeholder disagrees with it, or when the accepted values differ. +fn compare_value_taking_argument( + field: &FieldMetadata, + documentation: &CliMetadata, + argument: &Arg, +) -> Result<()> { + let parser_value_name = argument + .get_value_names() + .and_then(|names| names.first()) + .map(ToString::to_string); + ensure!( + parser_value_name.is_some(), + "parser argument {} should declare a value name to document", + field.name + ); + ensure!( + documentation.value_name == parser_value_name, + "release help should document the value placeholder {name} \ + parses: documented {documented:?}, parser {parsed:?}", + name = field.name, + documented = documentation.value_name, + parsed = parser_value_name, + ); + ensure!( + documentation.possible_values + == argument + .get_possible_values() + .iter() + .map(|value| value.get_name().to_owned()) + .collect::>(), + "release help should document the accepted values for {}", + field.name + ); + Ok(()) +} + /// Verify that documented CLI value shapes match the parser users actually run. /// /// `ortho_config` 0.10.0 documents every derived boolean as `--flag[=BOOL]`, @@ -156,70 +256,8 @@ fn release_help_cli_value_shapes_match_the_parser() -> Result<()> { let Some(documentation) = field.cli.as_ref() else { continue; }; - let argument = command - .get_arguments() - .find(|argument| argument.get_id().as_str() == field.name) - .with_context(|| { - format!( - "release help documents {}, but the parser accepts no such \ - argument; the manual page would advertise a flag this \ - binary rejects", - field.name, - ) - })?; - - let takes_value = argument.get_action().takes_values(); - ensure!( - documentation.takes_value == takes_value, - "release help should document the value shape {name} parses: \ - documented {documented}, parser {parsed}", - name = field.name, - documented = documentation.takes_value, - parsed = takes_value, - ); - ensure!( - !documentation.value_optional, - "release help must not offer the optional-value spelling for {}", - field.name - ); - if takes_value { - let parser_value_name = argument - .get_value_names() - .and_then(|names| names.first()) - .map(ToString::to_string); - ensure!( - parser_value_name.is_some(), - "parser argument {} should declare a value name to document", - field.name - ); - ensure!( - documentation.value_name == parser_value_name, - "release help should document the value placeholder {name} \ - parses: documented {documented:?}, parser {parsed:?}", - name = field.name, - documented = documentation.value_name, - parsed = parser_value_name, - ); - ensure!( - documentation.possible_values - == argument - .get_possible_values() - .iter() - .map(|value| value.get_name().to_owned()) - .collect::>(), - "release help should document the accepted values for {}", - field.name - ); - } else { - // A presence-only switch parses no value at all. Clap still records - // `possible_values` for `SetTrue` actions, so copying them would - // describe a value surface no invocation can reach. - ensure!( - documentation.value_name.is_none() && documentation.possible_values.is_empty(), - "release help should describe {} as a switch with no value", - field.name - ); - } + let argument = find_parser_argument(&command, &field.name)?; + compare_argument_value_shape(field, documentation, argument)?; compared.push(field.name.as_str()); } From 3a69c88d198995825ff08913601c2c47de0fe6f8 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 30 Oct 2026 18:00:48 +0100 Subject: [PATCH 04/10] Split release-help metadata tests into submodules Whitaker's module line cap rejects `metadata_tests.rs` at 419 lines. Move the file to a directory module and place the parser-value-shape parity checks in `value_shapes`, leaving parser-only projection, localization, and identifier provenance in the parent module. The split follows the repository's directory-module layout, and the focused release-help suite is unchanged. Co-Authored-By: Claude Code --- .../mod.rs} | 166 +----------------- .../metadata_tests/value_shapes.rs | 165 +++++++++++++++++ 2 files changed, 172 insertions(+), 159 deletions(-) rename src/cli/release_help/{metadata_tests.rs => metadata_tests/mod.rs} (60%) create mode 100644 src/cli/release_help/metadata_tests/value_shapes.rs diff --git a/src/cli/release_help/metadata_tests.rs b/src/cli/release_help/metadata_tests/mod.rs similarity index 60% rename from src/cli/release_help/metadata_tests.rs rename to src/cli/release_help/metadata_tests/mod.rs index f7049c2e5..b65f8811e 100644 --- a/src/cli/release_help/metadata_tests.rs +++ b/src/cli/release_help/metadata_tests/mod.rs @@ -1,15 +1,17 @@ //! Unit tests for the complete release-help metadata contract. //! -//! The tests cover the configuration-help projection, the parser-only field -//! and value-shape derivation, and the identifiers carried by the composed -//! root and immediate subcommand documentation metadata. +//! The tests cover the configuration-help projection, parser-only field and +//! value-shape derivation, localization, and the identifiers carried by the +//! composed root and immediate subcommand documentation metadata. Value-shape +//! parity with the parser lives in [`value_shapes`]. use super::*; use anyhow::{Context, Result, ensure}; -use clap::{Arg, Command}; -use ortho_config::docs::CliMetadata; +use clap::Command; use rstest::rstest; +mod value_shapes; + use super::parser_fields::documented_clap_parser_field; struct ParserOnlyFieldExpectation { @@ -133,160 +135,6 @@ fn parser_only_metadata_requires_the_parser_argument(#[case] argument_id: &str) Ok(()) } -/// Find the parser argument a documented field claims to describe. -/// -/// # Errors -/// -/// Returns an error when the parser accepts no argument with that identifier, -/// which would mean the manual page advertises a flag the binary rejects. -fn find_parser_argument<'a>(command: &'a Command, field_name: &str) -> Result<&'a Arg> { - command - .get_arguments() - .find(|argument| argument.get_id().as_str() == field_name) - .with_context(|| { - format!( - "release help documents {field_name}, but the parser accepts no such \ - argument; the manual page would advertise a flag this binary rejects" - ) - }) -} - -/// Verify one documented field against the parser argument it describes. -/// -/// # Errors -/// -/// Returns an error when the value shape, placeholder, or accepted values -/// diverge from what the parser accepts. -fn compare_argument_value_shape( - field: &FieldMetadata, - documentation: &CliMetadata, - argument: &Arg, -) -> Result<()> { - let takes_value = argument.get_action().takes_values(); - ensure!( - documentation.takes_value == takes_value, - "release help should document the value shape {name} parses: \ - documented {documented}, parser {parsed}", - name = field.name, - documented = documentation.takes_value, - parsed = takes_value, - ); - ensure!( - !documentation.value_optional, - "release help must not offer the optional-value spelling for {}", - field.name - ); - if takes_value { - compare_value_taking_argument(field, documentation, argument)?; - } else { - // A presence-only switch parses no value at all. Clap still records - // `possible_values` for `SetTrue` actions, so copying them would - // describe a value surface no invocation can reach. - ensure!( - documentation.value_name.is_none() && documentation.possible_values.is_empty(), - "release help should describe {} as a switch with no value", - field.name - ); - } - Ok(()) -} - -/// Verify the placeholder and accepted values of a value-taking argument. -/// -/// # Errors -/// -/// Returns an error when the parser declares no value name, when the documented -/// placeholder disagrees with it, or when the accepted values differ. -fn compare_value_taking_argument( - field: &FieldMetadata, - documentation: &CliMetadata, - argument: &Arg, -) -> Result<()> { - let parser_value_name = argument - .get_value_names() - .and_then(|names| names.first()) - .map(ToString::to_string); - ensure!( - parser_value_name.is_some(), - "parser argument {} should declare a value name to document", - field.name - ); - ensure!( - documentation.value_name == parser_value_name, - "release help should document the value placeholder {name} \ - parses: documented {documented:?}, parser {parsed:?}", - name = field.name, - documented = documentation.value_name, - parsed = parser_value_name, - ); - ensure!( - documentation.possible_values - == argument - .get_possible_values() - .iter() - .map(|value| value.get_name().to_owned()) - .collect::>(), - "release help should document the accepted values for {}", - field.name - ); - Ok(()) -} - -/// Verify that documented CLI value shapes match the parser users actually run. -/// -/// `ortho_config` 0.10.0 documents every derived boolean as `--flag[=BOOL]`, -/// because its generated parser accepts an optional value. Netsuke declares its -/// flags by hand and rejects that spelling, so release help normalizes boolean -/// fields back to presence-only switches. Comparing against `Cli::command()` -/// makes the parity a standing contract: a future metadata drift fails here -/// instead of shipping a manual page the binary contradicts. -/// -/// Value names are compared too. `CliConfig` declares the placeholder for every -/// configuration-derived option through `cli(value_name = "...")`; without that -/// declaration the generator falls back to a machine type such as `UTF8PATHBUF` -/// or `USIZE`, which no reader of the manual page should see. A name missing -/// here, or one that disagrees with the parser, fails the test. -#[test] -fn release_help_cli_value_shapes_match_the_parser() -> Result<()> { - let metadata = ReleaseHelpCli::try_get_doc_metadata()?; - let command = Cli::command(); - let mut compared = Vec::new(); - - for field in &metadata.fields { - let Some(documentation) = field.cli.as_ref() else { - continue; - }; - let argument = find_parser_argument(&command, &field.name)?; - compare_argument_value_shape(field, documentation, argument)?; - compared.push(field.name.as_str()); - } - - // The four boolean flags that 0.10.0 first documented as `--flag[=BOOL]` - // are the reason this contract exists, so their absence proves nothing. - for expected in [ - "verbose", - "fetch_default_deny", - "trust_project_fetch_policy", - "json", - ] { - ensure!( - compared.contains(&expected), - "release help should compare {expected} against the parser: {compared:?}" - ); - } - - // Value-taking fields are where the generator's `UTF8PATHBUF` and `USIZE` - // fallbacks appeared, so their absence from the comparison would hide the - // very regression this contract now guards against. - for expected in ["file", "jobs", "locale", "manifest_fuel", "default_targets"] { - ensure!( - compared.contains(&expected), - "release help should compare the value placeholder for {expected}: {compared:?}" - ); - } - Ok(()) -} - /// Verify that release help includes every parser-only selector. #[test] fn release_help_metadata_composes_every_parser_only_selector() -> Result<()> { diff --git a/src/cli/release_help/metadata_tests/value_shapes.rs b/src/cli/release_help/metadata_tests/value_shapes.rs new file mode 100644 index 000000000..52fec5eb5 --- /dev/null +++ b/src/cli/release_help/metadata_tests/value_shapes.rs @@ -0,0 +1,165 @@ +//! Value-shape parity between release-help metadata and the parser. +//! +//! These checks read the composed metadata and `Cli::command()` together, so +//! they live apart from the parser-only and localization tests that build the +//! metadata in the first place. + +use anyhow::{Context, Result, ensure}; +use clap::{Arg, Command, CommandFactory}; +use ortho_config::docs::{CliMetadata, FieldMetadata}; + +use crate::cli::{Cli, ReleaseHelpCli}; + +/// Find the parser argument a documented field claims to describe. +/// +/// # Errors +/// +/// Returns an error when the parser accepts no argument with that identifier, +/// which would mean the manual page advertises a flag the binary rejects. +fn find_parser_argument<'a>(command: &'a Command, field_name: &str) -> Result<&'a Arg> { + command + .get_arguments() + .find(|argument| argument.get_id().as_str() == field_name) + .with_context(|| { + format!( + "release help documents {field_name}, but the parser accepts no such \ + argument; the manual page would advertise a flag this binary rejects" + ) + }) +} + +/// Verify one documented field against the parser argument it describes. +/// +/// # Errors +/// +/// Returns an error when the value shape, placeholder, or accepted values +/// diverge from what the parser accepts. +fn compare_argument_value_shape( + field: &FieldMetadata, + documentation: &CliMetadata, + argument: &Arg, +) -> Result<()> { + let takes_value = argument.get_action().takes_values(); + ensure!( + documentation.takes_value == takes_value, + "release help should document the value shape {name} parses: \ + documented {documented}, parser {parsed}", + name = field.name, + documented = documentation.takes_value, + parsed = takes_value, + ); + ensure!( + !documentation.value_optional, + "release help must not offer the optional-value spelling for {}", + field.name + ); + if takes_value { + compare_value_taking_argument(field, documentation, argument)?; + } else { + // A presence-only switch parses no value at all. Clap still records + // `possible_values` for `SetTrue` actions, so copying them would + // describe a value surface no invocation can reach. + ensure!( + documentation.value_name.is_none() && documentation.possible_values.is_empty(), + "release help should describe {} as a switch with no value", + field.name + ); + } + Ok(()) +} + +/// Verify the placeholder and accepted values of a value-taking argument. +/// +/// # Errors +/// +/// Returns an error when the parser declares no value name, when the documented +/// placeholder disagrees with it, or when the accepted values differ. +fn compare_value_taking_argument( + field: &FieldMetadata, + documentation: &CliMetadata, + argument: &Arg, +) -> Result<()> { + let parser_value_name = argument + .get_value_names() + .and_then(|names| names.first()) + .map(ToString::to_string); + ensure!( + parser_value_name.is_some(), + "parser argument {} should declare a value name to document", + field.name + ); + ensure!( + documentation.value_name == parser_value_name, + "release help should document the value placeholder {name} \ + parses: documented {documented:?}, parser {parsed:?}", + name = field.name, + documented = documentation.value_name, + parsed = parser_value_name, + ); + ensure!( + documentation.possible_values + == argument + .get_possible_values() + .iter() + .map(|value| value.get_name().to_owned()) + .collect::>(), + "release help should document the accepted values for {}", + field.name + ); + Ok(()) +} + +/// Verify that documented CLI value shapes match the parser users actually run. +/// +/// `ortho_config` 0.10.0 documents every derived boolean as `--flag[=BOOL]`, +/// because its generated parser accepts an optional value. Netsuke declares its +/// flags by hand and rejects that spelling, so release help normalizes boolean +/// fields back to presence-only switches. Comparing against `Cli::command()` +/// makes the parity a standing contract: a future metadata drift fails here +/// instead of shipping a manual page the binary contradicts. +/// +/// Value names are compared too. `CliConfig` declares the placeholder for every +/// configuration-derived option through `cli(value_name = "...")`; without that +/// declaration the generator falls back to a machine type such as `UTF8PATHBUF` +/// or `USIZE`, which no reader of the manual page should see. A name missing +/// here, or one that disagrees with the parser, fails the test. +#[test] +fn release_help_cli_value_shapes_match_the_parser() -> Result<()> { + let metadata = ReleaseHelpCli::try_get_doc_metadata()?; + let command = Cli::command(); + let mut compared = Vec::new(); + + for field in &metadata.fields { + let Some(documentation) = field.cli.as_ref() else { + continue; + }; + let argument = find_parser_argument(&command, &field.name)?; + compare_argument_value_shape(field, documentation, argument)?; + compared.push(field.name.as_str()); + } + + // The four boolean flags that 0.10.0 first documented as `--flag[=BOOL]` + // are the reason this contract exists, so their absence proves nothing. + for expected in [ + "verbose", + "fetch_default_deny", + "trust_project_fetch_policy", + "json", + ] { + ensure!( + compared.contains(&expected), + "release help should compare {expected} against the parser: {compared:?}" + ); + } + + // Value-taking fields are where the generator's `UTF8PATHBUF` and `USIZE` + // fallbacks appeared, so their absence from the comparison would hide the + // very regression this contract now guards against. + for expected in ["file", "jobs", "locale", "manifest_fuel", "default_targets"] { + ensure!( + compared.contains(&expected), + "release help should compare the value placeholder for {expected}: {compared:?}" + ); + } + Ok(()) +} From febfc0e83ee18d960e03624bc3cb3740042070fe Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 31 Oct 2026 11:50:34 +0100 Subject: [PATCH 05/10] Fix release-help test paths in developers' guide The value-shape split moved the identifier-provenance test to metadata_tests/mod.rs and the parity test to metadata_tests/value_shapes.rs, but the guide still cited the retired metadata_tests.rs. Point each named test at the module that now owns it, so readers looking for the identifier or placeholder contract land on the right source. Co-Authored-By: Claude Code --- docs/developers-guide.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index af10e5449..69932518e 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -4093,10 +4093,11 @@ every derived boolean as `--flag[=BOOL]`, because its *generated* clap parser accepts an optional value; Netsuke declares its flags by hand in `src/cli/command.rs` and rejects that spelling, so the adapter reduces boolean fields back to presence-only switches. Two tests hold both projections: -`release_help_identifiers_are_netsuke_declared_not_generated` and -`release_help_cli_value_shapes_match_the_parser` in -`src/cli/release_help/metadata_tests.rs`. A new derived field, or a new -upstream metadata change, is expected to reach one of them. +identifier provenance in `src/cli/release_help/metadata_tests/mod.rs` +(`release_help_identifiers_are_netsuke_declared_not_generated`), and +value-shape parity in `src/cli/release_help/metadata_tests/value_shapes.rs` +(`release_help_cli_value_shapes_match_the_parser`). A new derived field, or a +new upstream metadata change, is expected to reach one of them. The root synopsis is cleared for the same reason. `ortho_config` 0.10.0's derive defaults `synopsis_id` to a generated `-usage` identifier that no From b8d1aacca0d5bfa4101b87660b5543a713ec2bac Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 31 Oct 2026 16:21:16 +0100 Subject: [PATCH 06/10] Refresh generated typos.toml The pinned typos-config-builder generator now emits a tokio attribute ignore pattern and drops the obsolete 'flavor = ' ignore. Make spelling regenerates this file on every run, so leaving it uncommitted keeps the worktree permanently dirty. The same regeneration is isolated in PR #1062; committing the identical generated content here stops the drift from masking real worktree state. Co-Authored-By: Claude Code --- typos.toml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/typos.toml b/typos.toml index d32137d06..1b4d8caa2 100644 --- a/typos.toml +++ b/typos.toml @@ -55,6 +55,8 @@ extend-ignore-re = [ "(?m)^ should_upload_workflow_artifacts: \\$\\{\\{ steps\\.release_modes\\.outputs\\['should-upload-workflow-artifacts'\\] \\}\\}$", "(?m)^ should_upload_workflow_artifacts:$", "(?s)```.*?```", + "(?x)\n\\#\\[\\s*tokio::(?:main|test)\\s*\\(\\s*(?:\n(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*flavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\n|\n(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*flavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\n|\n(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*flavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\n|\nflavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\n|\n(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*flavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\n|\n(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*flavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\n|\nflavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\n|\n(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\\s*,\\s*flavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\n|\nflavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\\s*,\\s*(?:worker_threads\\s*=\\s*[0-9][0-9]*|start_paused\\s*=\\s*(?:true|false)|unhandled_panic\\s*=\\s*(?:\\\\\"(?:shutdown_runtime|ignore)\\\\\"|\"(?:shutdown_runtime|ignore)\"))\n|\nflavor\\s*=\\s*(?:\\\\\"(?:current_thread|multi_thread)\\\\\"|\"(?:current_thread|multi_thread)\")\n)(?:\\s*,\\s*|\\s*)\\)\\s*\\]", + "(?x)\n\\#\\[\\s*tokio::main\\s*\\(\\s*\nflavor\\s*=\\s*(?:\\\\\"local\\\\\"|\"local\")\n(?:\\s*,\\s*|\\s*)\\)\\s*\\]", "--[A-Za-z0-9_-]*color(?:ed|s|ing|ize[ds]?|izing)?(?:\\b|_)", "Azure\\s+Architecture\\s+Center\\s*\\|\\s*Microsoft\\s+Learn\\b", "GitHub\\s+Flavored\\s+Markdown\\b(?:\\s\\(GFM\\))?", @@ -70,7 +72,6 @@ extend-ignore-re = [ "\\bartifact-name\\b", "\\bartifact-suffix\\b", "\\bcolor-(?:mix|contrast)\\b", - "\\bflavor = ", "\\blocalisation\\b", "\\brust-analyzer\\b", "\\{COLOR\\}", From 999c38c0d6f5572a9ce80248ab2833ee86460b30 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 31 Oct 2026 16:45:01 +0100 Subject: [PATCH 07/10] Scope the value-name rule to configuration-derived options The developers guide said every value-taking option needs matching declarations in CliConfig and Cli::command(). That is only true for configuration-derived options; parser-only selectors have no CliConfig field because parser_fields.rs derives their placeholder from the parser declaration. Addresses CodeRabbit review comment 4432256413. Co-Authored-By: Claude Code --- docs/developers-guide.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 69932518e..54d4bdca8 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -4121,9 +4121,11 @@ without that declaration the generator falls back to a machine type such as only for parser-only selectors, which have no configuration source from which to derive a placeholder. The parity test compares every value-taking option's `cli.value_name` against the first value name `Cli::command()` declares, so -adding a value-taking option requires matching declarations in both -`src/cli/command.rs` and `src/cli/config/mod.rs`; a placeholder that disagrees -with the parser fails the test rather than reaching a release artefact. +adding a configuration-derived value-taking option requires matching +declarations in `src/cli/config/mod.rs` and `src/cli/command.rs`. A parser-only +selector needs only the parser declaration, because `parser_fields.rs` derives +its placeholder from that declaration. A placeholder that disagrees with the +parser fails the test rather than reaching a release artefact. Release automation delegates installation to [`install_orthohelp.py`](../scripts/install_orthohelp.py), with the pinned From 35abbbf450d41b5fcf722aa6b4037474d950c510 Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 31 Oct 2026 19:06:06 +0100 Subject: [PATCH 08/10] Render declared value names in release-help tests Make the fake `cargo-orthohelp` used by the release-help script tests consume `cli.value_name` instead of inventing its own placeholder, and assert that every declared placeholder reaches both generated formats. The metadata snapshot now records each field's `value_name`, so a lost declaration fails the suite before it can reach a release artefact rather than silently falling back to a configuration type name. Document the resulting guarantees in the developers' guide and describe the readable placeholders users now see in generated help. Co-Authored-By: Claude Code --- docs/developers-guide.md | 6 +- docs/users-guide.md | 8 ++ tests/ortho_config_metadata_snapshot_tests.rs | 2 + tests/release_help/mod.rs | 44 ++++++-- tests/release_help_script_tests.rs | 105 ++++++++++++++++++ ...help_documentation_metadata_is_stable.snap | 22 ++++ ...help_script_tests__generated_man_page.snap | 36 ++++++ ...ript_tests__generated_powershell_help.snap | 18 +++ 8 files changed, 228 insertions(+), 13 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 54d4bdca8..921f1aede 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -4125,7 +4125,11 @@ adding a configuration-derived value-taking option requires matching declarations in `src/cli/config/mod.rs` and `src/cli/command.rs`. A parser-only selector needs only the parser declaration, because `parser_fields.rs` derives its placeholder from that declaration. A placeholder that disagrees with the -parser fails the test rather than reaching a release artefact. +parser fails the test rather than reaching a release artefact. The release-help +script tests render every declared value name through the fake generator and +assert the placeholder appears in both the manual page and the PowerShell MAML +output, and the metadata snapshot records each field's `value_name`, so a lost +declaration fails the suite before it can reach a release. Release automation delegates installation to [`install_orthohelp.py`](../scripts/install_orthohelp.py), with the pinned diff --git a/docs/users-guide.md b/docs/users-guide.md index 97e704a7d..3f085d886 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -1147,6 +1147,14 @@ only. Translated copy reaches users through the running binary, which embeds every catalogue. The generated help describes the complete public CLI, including `-C/--directory`, `--config`, and the documented command tree. +Generated help uses the same readable value placeholders as `--help`. +Configuration-derived options show `-f, --file `, `-j, --jobs `, +`--locale `, the list options' ``, ``, ``, and +`` names, and the manifest budget options' ``, ``, and +`` names, rather than internal configuration type names. Parser-only +selectors such as `-C/--directory` and `--config` show the placeholders the +command parser declares. + ### Anchor a project with `--directory` `--directory` changes manifest lookup, project configuration discovery and diff --git a/tests/ortho_config_metadata_snapshot_tests.rs b/tests/ortho_config_metadata_snapshot_tests.rs index 3d5f64e0c..094ce03f0 100644 --- a/tests/ortho_config_metadata_snapshot_tests.rs +++ b/tests/ortho_config_metadata_snapshot_tests.rs @@ -54,6 +54,7 @@ struct CliSourceSnapshot { short: Option, multiple: bool, possible_values: Vec, + value_name: Option, } #[derive(Serialize)] @@ -116,6 +117,7 @@ fn metadata_snapshot() -> MetadataSnapshot { short: cli.short, multiple: cli.multiple, possible_values: cli.possible_values.clone(), + value_name: cli.value_name.clone(), }), environment: field.env.as_ref().map(|env| env.var_name.clone()), file: field.file.as_ref().map(|file| file.key_path.clone()), diff --git a/tests/release_help/mod.rs b/tests/release_help/mod.rs index ea21a7711..8dcd37f8d 100644 --- a/tests/release_help/mod.rs +++ b/tests/release_help/mod.rs @@ -137,12 +137,25 @@ for field in parser_only_fields: short = cli["short"] parser_only_flags.append(f"-{short}/--{long}" if short else f"--{long}") +value_placeholders = [] +for field in metadata["fields"]: + cli = field.get("cli") + if not cli or not cli.get("value_name"): + continue + if not cli["long"]: + raise ValueError(f"value-taking {field['name']} metadata must have a long flag") + value_placeholders.append(f"{cli['long']}={cli['value_name']}") +if not value_placeholders: + raise ValueError("release-help metadata must declare value placeholders") + print(" ".join(parser_only_flags)) print(" ".join(command["app_name"] for command in metadata["subcommands"])) +print(" ".join(value_placeholders)) PY ) parser_only_flags="${public_cli_surface[0]}" subcommands="${public_cli_surface[1]}" +value_placeholders="${public_cli_surface[2]}" "#; @@ -161,6 +174,13 @@ MAN cat >>"$out_dir/man/man1/netsuke.1" <>"$out_dir/man/man1/netsuke.1" + done + cat >>"$out_dir/man/man1/netsuke.1" <"$out_dir/powershell/$module_name/$module_name.psm1" printf '@{\n RootModule = "%s.psm1"\n ModuleVersion = "0.1.0"\n}\n' "$module_name" >"$out_dir/powershell/$module_name/$module_name.psd1" - cat >"$help_dir/$module_name-help.xml" < - - - $module_name - - - $parser_only_flags $subcommands - - - -MAML + { + printf '\n' + printf ' \n' + printf ' \n %s\n \n' "$module_name" + printf ' \n %s %s\n \n' "$parser_only_flags" "$subcommands" + for pair in $value_placeholders; do + long="${pair%%=*}" + value="${pair#*=}" + printf ' --%s%s\n' "$long" "$value" + done + printf ' \n\n' + } >"$help_dir/$module_name-help.xml" printf 'TOPIC\n about_%s\n' "$module_name" >"$help_dir/about_$module_name.help.txt" ;; esac diff --git a/tests/release_help_script_tests.rs b/tests/release_help_script_tests.rs index 28572be91..9a05eec37 100644 --- a/tests/release_help_script_tests.rs +++ b/tests/release_help_script_tests.rs @@ -11,6 +11,8 @@ mod release_help; use anyhow::{Context, Result, ensure}; use insta::assert_snapshot; +use netsuke::cli::ReleaseHelpCli; +use ortho_config::docs::OrthoConfigDocs; use release_help::{ ReleaseHelpRun, ScriptFixture, logged_args, run_release_help, script_fixture, script_path, snapshot_settings, @@ -21,6 +23,17 @@ use std::{fs, process::Command}; const DOCUMENTED_SUBCOMMANDS: [&str; 5] = ["build", "clean", "graph", "generate", "help"]; const PARSER_ONLY_SELECTORS: [&str; 2] = ["-C/--directory", "--config"]; +/// Distinct value placeholders the release help must render. +/// +/// Every configuration-derived value-taking option collapses onto one of these +/// names, and `DIR` comes from the parser-only `--directory` selector. A +/// declaration that loses its `value_name` removes its placeholder from both +/// generated formats, so this list fails the test rather than letting a type +/// fallback reach a release artefact. +const EXPECTED_VALUE_NAMES: [&str; 11] = [ + "BYTES", "COUNT", "DIR", "FILE", "FUEL", "HOST", "LOCALE", "N", "SCHEME", "TARGET", "VAR", +]; + /// Generate and verify a manual page for a non-Windows target. /// /// # Errors @@ -183,6 +196,7 @@ fn assert_man_page_structure(man_page: &str) -> Result<()> { "man page should include SYNOPSIS for the binary: {man_page}" ); assert_public_cli_surface(man_page, "man page")?; + assert_man_value_placeholders(man_page)?; Ok(()) } @@ -214,6 +228,7 @@ fn assert_powershell_help_structure( "about help should name the requested module: {about_help}" ); assert_public_cli_surface(maml, "MAML help")?; + assert_maml_value_placeholders(maml)?; Ok(()) } @@ -234,6 +249,96 @@ fn assert_public_cli_surface(document: &str, format_name: &str) -> Result<()> { Ok(()) } +/// Collect each value-taking option and the placeholder it declares. +fn declared_value_placeholders() -> Vec<(String, String)> { + ReleaseHelpCli::get_doc_metadata() + .fields + .iter() + .filter_map(|field| { + let cli = field.cli.as_ref()?; + let value_name = cli.value_name.as_ref()?; + let long = cli.long.clone()?; + Some((long, value_name.clone())) + }) + .collect() +} + +/// Assert that the manual page renders every declared value placeholder. +/// +/// # Errors +/// +/// Returns an error when the metadata declares no placeholders, or when the +/// manual page omits a declared placeholder or an expected distinct value name. +fn assert_man_value_placeholders(man_page: &str) -> Result<()> { + let placeholders = declared_value_placeholders(); + ensure!( + !placeholders.is_empty(), + "release-help metadata should declare value placeholders" + ); + for (long, value_name) in &placeholders { + let rendered = format!("\\fB--{long}\\fR \\fI{value_name}\\fP"); + ensure!( + man_page.contains(&rendered), + "man page should render the {value_name} placeholder for --{long}: {man_page}" + ); + } + for value_name in EXPECTED_VALUE_NAMES { + ensure!( + placeholders + .iter() + .any(|(_, name)| name.as_str() == value_name), + "release-help metadata should declare the {value_name} placeholder" + ); + let rendered = format!("\\fI{value_name}\\fP"); + ensure!( + man_page.contains(&rendered), + "man page should render the {value_name} placeholder: {man_page}" + ); + } + Ok(()) +} + +/// Assert that the PowerShell MAML help renders every declared value placeholder. +/// +/// # Errors +/// +/// Returns an error when the metadata declares no placeholders, or when the +/// MAML help omits a declared placeholder or an expected distinct value name. +fn assert_maml_value_placeholders(maml: &str) -> Result<()> { + let placeholders = declared_value_placeholders(); + ensure!( + !placeholders.is_empty(), + "release-help metadata should declare value placeholders" + ); + for (long, value_name) in &placeholders { + let rendered = format!( + "--{long}\ + {value_name}\ + " + ); + ensure!( + maml.contains(&rendered), + "MAML help should render the {value_name} placeholder for --{long}: {maml}" + ); + } + for value_name in EXPECTED_VALUE_NAMES { + ensure!( + placeholders + .iter() + .any(|(_, name)| name.as_str() == value_name), + "release-help metadata should declare the {value_name} placeholder" + ); + let rendered = format!( + "{value_name}" + ); + ensure!( + maml.contains(&rendered), + "MAML help should render the {value_name} placeholder: {maml}" + ); + } + Ok(()) +} + /// Verify the manual date selected from `SOURCE_DATE_EPOCH`. /// /// # Errors diff --git a/tests/snapshots/ortho_config_metadata_snapshot_tests__release_help_documentation_metadata_is_stable.snap b/tests/snapshots/ortho_config_metadata_snapshot_tests__release_help_documentation_metadata_is_stable.snap index 03f051ce8..82b921d1e 100644 --- a/tests/snapshots/ortho_config_metadata_snapshot_tests__release_help_documentation_metadata_is_stable.snap +++ b/tests/snapshots/ortho_config_metadata_snapshot_tests__release_help_documentation_metadata_is_stable.snap @@ -18,6 +18,7 @@ fields: short: f multiple: false possible_values: [] + value_name: FILE environment: NETSUKE_FILE file: file merge_strategy: replace @@ -28,6 +29,7 @@ fields: short: j multiple: false possible_values: [] + value_name: N environment: NETSUKE_JOBS file: jobs merge_strategy: replace @@ -38,6 +40,7 @@ fields: short: v multiple: false possible_values: [] + value_name: ~ environment: NETSUKE_VERBOSE file: verbose merge_strategy: replace @@ -48,6 +51,7 @@ fields: short: l multiple: false possible_values: [] + value_name: LOCALE environment: NETSUKE_LOCALE file: locale merge_strategy: replace @@ -58,6 +62,7 @@ fields: short: F multiple: true possible_values: [] + value_name: SCHEME environment: NETSUKE_FETCH_ALLOW_SCHEME file: fetch_allow_scheme merge_strategy: append @@ -68,6 +73,7 @@ fields: short: e multiple: true possible_values: [] + value_name: VAR environment: NETSUKE_ENV_ALLOW_VAR file: env_allow_var merge_strategy: append @@ -78,6 +84,7 @@ fields: short: E multiple: true possible_values: [] + value_name: VAR environment: NETSUKE_ENV_BLOCK_VAR file: env_block_var merge_strategy: append @@ -88,6 +95,7 @@ fields: short: t multiple: true possible_values: [] + value_name: HOST environment: NETSUKE_FETCH_ALLOW_HOST file: fetch_allow_host merge_strategy: append @@ -98,6 +106,7 @@ fields: short: T multiple: true possible_values: [] + value_name: HOST environment: NETSUKE_FETCH_BLOCK_HOST file: fetch_block_host merge_strategy: append @@ -108,6 +117,7 @@ fields: short: c multiple: false possible_values: [] + value_name: ~ environment: NETSUKE_FETCH_DEFAULT_DENY file: fetch_default_deny merge_strategy: replace @@ -118,6 +128,7 @@ fields: short: r multiple: false possible_values: [] + value_name: ~ environment: NETSUKE_TRUST_PROJECT_FETCH_POLICY file: trust_project_fetch_policy merge_strategy: replace @@ -128,6 +139,7 @@ fields: short: m multiple: false possible_values: [] + value_name: FUEL environment: NETSUKE_MANIFEST_EVALUATION_FUEL file: manifest_evaluation_fuel merge_strategy: replace @@ -138,6 +150,7 @@ fields: short: M multiple: false possible_values: [] + value_name: FUEL environment: NETSUKE_MANIFEST_FUEL file: manifest_fuel merge_strategy: replace @@ -148,6 +161,7 @@ fields: short: a multiple: false possible_values: [] + value_name: BYTES environment: NETSUKE_MANIFEST_RENDERED_VALUE_BYTES file: manifest_rendered_value_bytes merge_strategy: replace @@ -158,6 +172,7 @@ fields: short: A multiple: false possible_values: [] + value_name: BYTES environment: NETSUKE_MANIFEST_RENDERED_MANIFEST_BYTES file: manifest_rendered_manifest_bytes merge_strategy: replace @@ -168,6 +183,7 @@ fields: short: n multiple: false possible_values: [] + value_name: BYTES environment: NETSUKE_MANIFEST_SOURCE_BYTES file: manifest_source_bytes merge_strategy: replace @@ -178,6 +194,7 @@ fields: short: N multiple: false possible_values: [] + value_name: COUNT environment: NETSUKE_MANIFEST_FOREACH_CARDINALITY file: manifest_foreach_cardinality merge_strategy: replace @@ -188,6 +205,7 @@ fields: short: i multiple: false possible_values: [] + value_name: COUNT environment: NETSUKE_MANIFEST_EXPANDED_ENTRIES file: manifest_expanded_entries merge_strategy: replace @@ -198,6 +216,7 @@ fields: short: J multiple: false possible_values: [] + value_name: ~ environment: NETSUKE_JSON file: json merge_strategy: replace @@ -238,6 +257,7 @@ fields: short: d multiple: true possible_values: [] + value_name: TARGET environment: NETSUKE_DEFAULT_TARGETS file: default_targets merge_strategy: append @@ -248,6 +268,7 @@ fields: short: C multiple: false possible_values: [] + value_name: DIR environment: ~ file: ~ merge_strategy: replace @@ -258,6 +279,7 @@ fields: short: ~ multiple: false possible_values: [] + value_name: FILE environment: ~ file: ~ merge_strategy: replace diff --git a/tests/snapshots/release_help/release_help_script_tests__generated_man_page.snap b/tests/snapshots/release_help/release_help_script_tests__generated_man_page.snap index d99f5d864..aedbaafa9 100644 --- a/tests/snapshots/release_help/release_help_script_tests__generated_man_page.snap +++ b/tests/snapshots/release_help/release_help_script_tests__generated_man_page.snap @@ -10,5 +10,41 @@ netsuke \- dependency-aware build orchestration [OPTIONS] .SH OPTIONS .B -C/--directory --config +.TP +\fB--file\fR \fIFILE\fP +.TP +\fB--jobs\fR \fIN\fP +.TP +\fB--locale\fR \fILOCALE\fP +.TP +\fB--fetch-allow-scheme\fR \fISCHEME\fP +.TP +\fB--env-allow-var\fR \fIVAR\fP +.TP +\fB--env-block-var\fR \fIVAR\fP +.TP +\fB--fetch-allow-host\fR \fIHOST\fP +.TP +\fB--fetch-block-host\fR \fIHOST\fP +.TP +\fB--manifest-evaluation-fuel\fR \fIFUEL\fP +.TP +\fB--manifest-fuel\fR \fIFUEL\fP +.TP +\fB--manifest-rendered-value-bytes\fR \fIBYTES\fP +.TP +\fB--manifest-rendered-manifest-bytes\fR \fIBYTES\fP +.TP +\fB--manifest-source-bytes\fR \fIBYTES\fP +.TP +\fB--manifest-foreach-cardinality\fR \fICOUNT\fP +.TP +\fB--manifest-expanded-entries\fR \fICOUNT\fP +.TP +\fB--default-targets\fR \fITARGET\fP +.TP +\fB--directory\fR \fIDIR\fP +.TP +\fB--config\fR \fIFILE\fP .SH COMMANDS build clean graph generate help diff --git a/tests/snapshots/release_help/release_help_script_tests__generated_powershell_help.snap b/tests/snapshots/release_help/release_help_script_tests__generated_powershell_help.snap index bf7a019c8..a3ac6f5e0 100644 --- a/tests/snapshots/release_help/release_help_script_tests__generated_powershell_help.snap +++ b/tests/snapshots/release_help/release_help_script_tests__generated_powershell_help.snap @@ -19,6 +19,24 @@ function Invoke-CustomNetsuke { param() } -C/--directory --config build clean graph generate help + --fileFILE + --jobsN + --localeLOCALE + --fetch-allow-schemeSCHEME + --env-allow-varVAR + --env-block-varVAR + --fetch-allow-hostHOST + --fetch-block-hostHOST + --manifest-evaluation-fuelFUEL + --manifest-fuelFUEL + --manifest-rendered-value-bytesBYTES + --manifest-rendered-manifest-bytesBYTES + --manifest-source-bytesBYTES + --manifest-foreach-cardinalityCOUNT + --manifest-expanded-entriesCOUNT + --default-targetsTARGET + --directoryDIR + --configFILE From 83ac97db608b455f3ef1800f5b01cf310750784e Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 31 Oct 2026 19:30:54 +0100 Subject: [PATCH 09/10] Share value-placeholder assertions across formats Hoist the format-independent checks into one helper that takes the rendering templates, and keep the man-page and MAML wrappers as the only format-specific code. CodeScene flagged the two near-identical assertion functions as code duplication (9.68 -> 9.09). The shared helper restores the file to its base score while keeping each format's markup explicit. Co-Authored-By: Claude Code --- tests/release_help_script_tests.rs | 90 ++++++++++++++++-------------- 1 file changed, 49 insertions(+), 41 deletions(-) diff --git a/tests/release_help_script_tests.rs b/tests/release_help_script_tests.rs index 9a05eec37..ed7b89d4b 100644 --- a/tests/release_help_script_tests.rs +++ b/tests/release_help_script_tests.rs @@ -263,23 +263,31 @@ fn declared_value_placeholders() -> Vec<(String, String)> { .collect() } -/// Assert that the manual page renders every declared value placeholder. +/// Assert that a generated document renders every declared value placeholder. +/// +/// The man-page and MAML surfaces differ only in markup, so the shared checks +/// live here and each format supplies its own rendering templates. /// /// # Errors /// /// Returns an error when the metadata declares no placeholders, or when the -/// manual page omits a declared placeholder or an expected distinct value name. -fn assert_man_value_placeholders(man_page: &str) -> Result<()> { +/// document omits a declared placeholder or an expected distinct value name. +fn assert_value_placeholders( + document: &str, + format_name: &str, + render_option: impl Fn(&str, &str) -> String, + render_value: impl Fn(&str) -> String, +) -> Result<()> { let placeholders = declared_value_placeholders(); ensure!( !placeholders.is_empty(), "release-help metadata should declare value placeholders" ); for (long, value_name) in &placeholders { - let rendered = format!("\\fB--{long}\\fR \\fI{value_name}\\fP"); + let rendered = render_option(long, value_name); ensure!( - man_page.contains(&rendered), - "man page should render the {value_name} placeholder for --{long}: {man_page}" + document.contains(&rendered), + "{format_name} should render the {value_name} placeholder for --{long}: {document}" ); } for value_name in EXPECTED_VALUE_NAMES { @@ -289,15 +297,30 @@ fn assert_man_value_placeholders(man_page: &str) -> Result<()> { .any(|(_, name)| name.as_str() == value_name), "release-help metadata should declare the {value_name} placeholder" ); - let rendered = format!("\\fI{value_name}\\fP"); + let rendered = render_value(value_name); ensure!( - man_page.contains(&rendered), - "man page should render the {value_name} placeholder: {man_page}" + document.contains(&rendered), + "{format_name} should render the {value_name} placeholder: {document}" ); } Ok(()) } +/// Assert that the manual page renders every declared value placeholder. +/// +/// # Errors +/// +/// Returns an error when the metadata declares no placeholders, or when the +/// manual page omits a declared placeholder or an expected distinct value name. +fn assert_man_value_placeholders(man_page: &str) -> Result<()> { + assert_value_placeholders( + man_page, + "man page", + |long, value_name| format!("\\fB--{long}\\fR \\fI{value_name}\\fP"), + |value_name| format!("\\fI{value_name}\\fP"), + ) +} + /// Assert that the PowerShell MAML help renders every declared value placeholder. /// /// # Errors @@ -305,38 +328,23 @@ fn assert_man_value_placeholders(man_page: &str) -> Result<()> { /// Returns an error when the metadata declares no placeholders, or when the /// MAML help omits a declared placeholder or an expected distinct value name. fn assert_maml_value_placeholders(maml: &str) -> Result<()> { - let placeholders = declared_value_placeholders(); - ensure!( - !placeholders.is_empty(), - "release-help metadata should declare value placeholders" - ); - for (long, value_name) in &placeholders { - let rendered = format!( - "--{long}\ - {value_name}\ - " - ); - ensure!( - maml.contains(&rendered), - "MAML help should render the {value_name} placeholder for --{long}: {maml}" - ); - } - for value_name in EXPECTED_VALUE_NAMES { - ensure!( - placeholders - .iter() - .any(|(_, name)| name.as_str() == value_name), - "release-help metadata should declare the {value_name} placeholder" - ); - let rendered = format!( - "{value_name}" - ); - ensure!( - maml.contains(&rendered), - "MAML help should render the {value_name} placeholder: {maml}" - ); - } - Ok(()) + assert_value_placeholders( + maml, + "MAML help", + |long, value_name| { + format!( + "--{long}\ + {value_name}\ + " + ) + }, + |value_name| { + format!( + "{value_name}\ + " + ) + }, + ) } /// Verify the manual date selected from `SOURCE_DATE_EPOCH`. From 8c69f5cecdd58e46d63255e0cc1f30e0b71e7789 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 1 Nov 2026 00:30:17 +0100 Subject: [PATCH 10/10] Harden the release-help output checks CodeRabbit's review of the release-help tests found two gaps. The MAML help declared `xmlns:maml` on ``, so every sibling `maml:name` was unbound and the fake generator emitted a document no XML parser accepts while the substring assertions passed. Declare the prefix on ``, where both children inherit it, and parse the document with `roxmltree` before the string checks. Every check also read the fake generator's output, so a real renderer that dropped or replaced a placeholder would still pass. The release lane now rejects a generated manual page or PowerShell help that publishes one of the machine type names the renderer substitutes when the metadata declares no value name. That runs against the real pinned `cargo-orthohelp`, which the test suite cannot depend on; the failure path is covered through a new fake-generator switch. Co-Authored-By: Claude Code --- docs/developers-guide.md | 7 +++ scripts/generate-release-help.sh | 44 +++++++++++++++ tests/release_help/mod.rs | 34 ++++++++++- tests/release_help/script_functions.rs | 45 +++++++++++++++ tests/release_help_script_tests.rs | 56 ++++++++++++++++++- ...ript_tests__generated_powershell_help.snap | 4 +- 6 files changed, 183 insertions(+), 7 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 921f1aede..91832eba5 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -4131,6 +4131,13 @@ assert the placeholder appears in both the manual page and the PowerShell MAML output, and the metadata snapshot records each field's `value_name`, so a lost declaration fails the suite before it can reach a release. +The release lane adds a guard on the artefacts themselves: +`scripts/generate-release-help.sh` rejects a generated manual page or +PowerShell help that publishes one of the generator's machine type names +(`UTF8PATHBUF`, `USIZE`, `LIST`, `INT`, `STRING`) as a placeholder, so the real +renderer is checked on what ships, not only under the fake generator the tests +drive. + Release automation delegates installation to [`install_orthohelp.py`](../scripts/install_orthohelp.py), with the pinned version supplied as `INPUT_VERSION`. The helper probes the cached binary and, diff --git a/scripts/generate-release-help.sh b/scripts/generate-release-help.sh index ba874031f..74696188f 100755 --- a/scripts/generate-release-help.sh +++ b/scripts/generate-release-help.sh @@ -116,6 +116,46 @@ file_size_bytes() { wc -c <"$path" | tr -d '[:space:]' } +# Machine type names `cargo-orthohelp` substitutes into a placeholder when the +# metadata declares no value name. Publishing one is the regression this lane +# exists to catch, and only the real renderer can prove the metadata reached it. +machine_value_names=(STRING INT LIST USIZE UTF8PATHBUF) + +# Fail when a generated artefact publishes a machine type name as a placeholder. +# +# The checks match the markup that marks a placeholder, not the bare word, so +# ordinary prose mentioning a string or an integer does not trip them. +reject_machine_value_placeholders() { + local path="$1" + local format="$2" + local name + local published=() + + for name in "${machine_value_names[@]}"; do + case "$format" in + man) + if grep -qF "\\fI${name}\\fP" "$path"; then + published+=("$name") + fi + ;; + ps) + if grep -qF "${name}" "$path"; then + published+=("$name") + fi + ;; + *) + echo "::error title=Release help placeholder::$(annotation_context "$format") unknown format ${format}" >&2 + exit 1 + ;; + esac + done + + if (( ${#published[@]} > 0 )); then + echo "::error title=Release help placeholder::$(annotation_context "$format") machine value names published (${published[*]}) in ${path}" >&2 + exit 1 + fi +} + require_cargo_orthohelp() { if ! command -v cargo-orthohelp >/dev/null 2>&1; then echo "::error title=cargo-orthohelp missing::$(annotation_context setup) cargo-orthohelp was not found on PATH" >&2 @@ -171,6 +211,7 @@ run_cargo_orthohelp "man" \ --man-date "$man_date" require_file "$out_dir/man/man1/${bin_name}.1" "manual page was not generated" +reject_machine_value_placeholders "$out_dir/man/man1/${bin_name}.1" "man" if target_is_windows "$target"; then run_cargo_orthohelp "ps" \ @@ -189,4 +230,7 @@ if target_is_windows "$target"; then "PowerShell MAML help was not generated" require_file "$out_dir/powershell/$module_name/en-US/about_$module_name.help.txt" \ "PowerShell about help was not generated" + reject_machine_value_placeholders \ + "$out_dir/powershell/$module_name/en-US/$module_name-help.xml" \ + "ps" fi diff --git a/tests/release_help/mod.rs b/tests/release_help/mod.rs index 8dcd37f8d..80c26cb21 100644 --- a/tests/release_help/mod.rs +++ b/tests/release_help/mod.rs @@ -180,6 +180,9 @@ MAN value="${pair#*=}" printf '.TP\n\\fB--%s\\fR \\fI%s\\fP\n' "$long" "$value" >>"$out_dir/man/man1/netsuke.1" done + if [[ "${ORTHOHELP_FAKE_FALLBACK_FORMAT:-}" == "man" && -n "${ORTHOHELP_FAKE_FALLBACK:-}" ]]; then + printf '.TP\n\\fB--fake-fallback\\fR \\fI%s\\fP\n' "$ORTHOHELP_FAKE_FALLBACK" >>"$out_dir/man/man1/netsuke.1" + fi cat >>"$out_dir/man/man1/netsuke.1" <"$out_dir/powershell/$module_name/$module_name.psd1" { printf '\n' - printf ' \n' + # Both prefixes are declared on the command element so the sibling + # `maml:para` and `maml:name` elements inherit a binding. Declaring + # `xmlns:maml` on the paragraph instead would leave every `maml:name` + # unbound and the document unparseable, which the test now catches. + printf ' \n' printf ' \n %s\n \n' "$module_name" - printf ' \n %s %s\n \n' "$parser_only_flags" "$subcommands" + printf ' \n %s %s\n \n' "$parser_only_flags" "$subcommands" for pair in $value_placeholders; do long="${pair%%=*}" value="${pair#*=}" printf ' --%s%s\n' "$long" "$value" done + if [[ "${ORTHOHELP_FAKE_FALLBACK_FORMAT:-}" == "ps" && -n "${ORTHOHELP_FAKE_FALLBACK:-}" ]]; then + printf ' --fake-fallback%s\n' "$ORTHOHELP_FAKE_FALLBACK" + fi printf ' \n\n' } >"$help_dir/$module_name-help.xml" printf 'TOPIC\n about_%s\n' "$module_name" >"$help_dir/about_$module_name.help.txt" @@ -241,6 +251,7 @@ pub struct ReleaseHelpRun<'a> { source_date_epoch: Option<&'a str>, fail_cargo: bool, skip_output: Option<&'a str>, + fallback_value: Option<(&'a str, &'a str)>, } impl<'a> ReleaseHelpRun<'a> { @@ -252,6 +263,7 @@ impl<'a> ReleaseHelpRun<'a> { source_date_epoch: None, fail_cargo: false, skip_output: None, + fallback_value: None, } } @@ -278,6 +290,15 @@ impl<'a> ReleaseHelpRun<'a> { self.skip_output = Some(format); self } + + /// Configure the fake generator to publish a machine value name. + /// + /// The generator writes the name into `format`'s placeholder markup, so the + /// script's real-output guard is exercised as it would be by a regression. + pub const fn fallback_value(mut self, format: &'a str, value: &'a str) -> Self { + self.fallback_value = Some((format, value)); + self + } } /// Run the release-help script with the supplied fixture and settings. @@ -316,7 +337,9 @@ pub fn run_release_help(fixture: &ScriptFixture, run: ReleaseHelpRun<'_>) -> Res .env_remove("RELEASE_HELP_CORRELATION_ID") .env_remove("SOURCE_DATE_EPOCH") .env_remove("ORTHOHELP_FAKE_FAIL") - .env_remove("ORTHOHELP_FAKE_SKIP_OUTPUT"); + .env_remove("ORTHOHELP_FAKE_SKIP_OUTPUT") + .env_remove("ORTHOHELP_FAKE_FALLBACK") + .env_remove("ORTHOHELP_FAKE_FALLBACK_FORMAT"); if let Some(value) = run.source_date_epoch { command.env("SOURCE_DATE_EPOCH", value); @@ -327,6 +350,11 @@ pub fn run_release_help(fixture: &ScriptFixture, run: ReleaseHelpRun<'_>) -> Res if let Some(format) = run.skip_output { command.env("ORTHOHELP_FAKE_SKIP_OUTPUT", format); } + if let Some((format, value)) = run.fallback_value { + command + .env("ORTHOHELP_FAKE_FALLBACK_FORMAT", format) + .env("ORTHOHELP_FAKE_FALLBACK", value); + } command.output().context("run release help script") } diff --git a/tests/release_help/script_functions.rs b/tests/release_help/script_functions.rs index 0ec70ef48..3ccf97128 100644 --- a/tests/release_help/script_functions.rs +++ b/tests/release_help/script_functions.rs @@ -167,3 +167,48 @@ fn require_file_logs_non_empty_output_size() -> Result<()> { ); Ok(()) } + +#[test] +fn reject_machine_value_placeholders_ignores_ordinary_prose() -> Result<()> { + let output = run_script_function( + "file=$(mktemp); printf '%s\\n' '.TP' '\\fB--file\\fR \\fIFILE\\fP' \ + 'Text mentions a string and an integer.' >\"$file\"; \ + reject_machine_value_placeholders \"$file\" man", + )?; + + ensure!( + output.status.success(), + "declared placeholders and prose should pass the guard, got {output:?}" + ); + Ok(()) +} + +#[rstest] +#[case("man", "\\fB--jobs\\fR \\fIUTF8PATHBUF\\fP", "UTF8PATHBUF")] +#[case( + "ps", + r#"USIZE"#, + "USIZE" +)] +fn reject_machine_value_placeholders_rejects_published_machine_type_names( + #[case] help_format: &str, + #[case] line: &str, + #[case] expected: &str, +) -> Result<()> { + let command = format!( + "file=$(mktemp); printf '%s\\n' '{line}' >\"$file\"; \ + reject_machine_value_placeholders \"$file\" {help_format}" + ); + let output = run_script_function(&command)?; + + ensure!( + output.status.code() == Some(1), + "a machine value name should fail the guard, got {output:?}" + ); + let stderr = String::from_utf8(output.stderr).context("stderr should be UTF-8")?; + ensure!( + stderr.contains("machine value names published") && stderr.contains(expected), + "expected a diagnostic naming {expected}, got {stderr}" + ); + Ok(()) +} diff --git a/tests/release_help_script_tests.rs b/tests/release_help_script_tests.rs index ed7b89d4b..88364bbd2 100644 --- a/tests/release_help_script_tests.rs +++ b/tests/release_help_script_tests.rs @@ -3,7 +3,9 @@ //! The script shells out to `cargo-orthohelp`, so these tests place a fake //! `cargo-orthohelp` executable first on `PATH`. This validates command //! construction and output verification without requiring network access or a -//! real `cargo-orthohelp` installation. +//! real `cargo-orthohelp` installation. The generated PowerShell help is parsed +//! as XML, so a placeholder emitted under an unbound namespace prefix fails +//! here rather than passing a substring check. #![cfg(unix)] @@ -17,6 +19,7 @@ use release_help::{ ReleaseHelpRun, ScriptFixture, logged_args, run_release_help, script_fixture, script_path, snapshot_settings, }; +use roxmltree::Document; use rstest::rstest; use std::{fs, process::Command}; @@ -204,13 +207,22 @@ fn assert_man_page_structure(man_page: &str) -> Result<()> { /// /// # Errors /// -/// Returns an error when a required PowerShell artefact or CLI item is absent. +/// Returns an error when a required PowerShell artefact or CLI item is absent, +/// or when the MAML help is not well-formed XML. fn assert_powershell_help_structure( ps_module: &str, ps_manifest: &str, maml: &str, about_help: &str, ) -> Result<()> { + // A substring check cannot tell a bound `maml:` prefix from an unbound one, + // so parse the document: an undeclared prefix is a hard XML error. + let maml_tree = Document::parse(maml) + .context("generated MAML help should be well-formed XML with bound prefixes")?; + ensure!( + maml_tree.root_element().tag_name().name() == "helpItems", + "MAML help should be rooted at helpItems: {maml}" + ); ensure!( ps_module.contains("function Invoke-CustomNetsuke"), "PowerShell module should expose the requested module command: {ps_module}" @@ -468,6 +480,46 @@ fn fails_when_expected_help_output_is_missing( Ok(()) } +/// Verify that a machine value name in generated help fails the release lane. +/// +/// The real `cargo-orthohelp` substitutes a machine type name when the metadata +/// declares no value name. This guard is the only check that reads the +/// renderer's actual output, so it is the last line between a lost declaration +/// and a published artefact. +/// +/// # Errors +/// +/// Returns an error when the fixture cannot be built, the script unexpectedly +/// succeeds, or the failure diagnostics cannot be read or validated. +#[rstest] +#[case("x86_64-unknown-linux-gnu", "man", "UTF8PATHBUF")] +#[case("x86_64-pc-windows-msvc", "ps", "USIZE")] +fn fails_when_generated_help_publishes_a_machine_value_name( + script_fixture: Result, + #[case] target: &str, + #[case] format: &str, + #[case] fallback: &str, +) -> Result<()> { + let fixture = script_fixture?; + let output = run_release_help( + &fixture, + ReleaseHelpRun::for_target(target) + .source_date_epoch("1") + .fallback_value(format, fallback), + )?; + + ensure!( + !output.status.success(), + "script should reject a {fallback} placeholder in {format} output: {output:?}" + ); + let stderr = String::from_utf8(output.stderr).context("stderr should be UTF-8")?; + ensure!( + stderr.contains("machine value names published") && stderr.contains(fallback), + "expected machine value name diagnostic naming {fallback}, got {stderr}" + ); + Ok(()) +} + /// Verify that generation stops when cargo-orthohelp is unavailable. /// /// # Errors diff --git a/tests/snapshots/release_help/release_help_script_tests__generated_powershell_help.snap b/tests/snapshots/release_help/release_help_script_tests__generated_powershell_help.snap index a3ac6f5e0..90a9c9f6e 100644 --- a/tests/snapshots/release_help/release_help_script_tests__generated_powershell_help.snap +++ b/tests/snapshots/release_help/release_help_script_tests__generated_powershell_help.snap @@ -12,12 +12,12 @@ function Invoke-CustomNetsuke { param() } --- - + CustomNetsuke - -C/--directory --config build clean graph generate help + -C/--directory --config build clean graph generate help --fileFILE --jobsN