From 67ee7d61b58d26625d3334bfebc76c9f74e28ae2 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Mon, 24 Aug 2026 15:01:09 -0300 Subject: [PATCH 1/6] feat(cli): accept a `node` sub-command for running the node The binary has only ever run the node, so an invocation is a bare list of node flags. An upcoming offline block-building benchmark adds a second entry point, which means the node first needs a name of its own. `node` is the default sub-command: `ethlambda node --genesis ...` and the existing flat `ethlambda --genesis ...` both run the node. A leading `node` token is stripped before parsing and the same CliOptions parser then sees exactly the arguments it saw before, so for the flat form the help text, error messages, exit codes and --version are unchanged by construction rather than by convention. That form is what the Dockerfile, lean-quickstart, the hive shim and the devnet skills all use, and none of them has to move. Tests pin the flat parse, the two forms agreeing field for field, a --node-id value that is literally "node", a trailing `node` token still being rejected, missing required flags in both forms, the bare invocation, and --help/--version staying top-level flags. --- bin/ethlambda/src/command.rs | 179 +++++++++++++++++++++++++++++++++++ bin/ethlambda/src/main.rs | 6 +- 2 files changed, 182 insertions(+), 3 deletions(-) create mode 100644 bin/ethlambda/src/command.rs diff --git a/bin/ethlambda/src/command.rs b/bin/ethlambda/src/command.rs new file mode 100644 index 00000000..f5909e2f --- /dev/null +++ b/bin/ethlambda/src/command.rs @@ -0,0 +1,179 @@ +//! Sub-command dispatch. +//! +//! `node` is the default sub-command: it can be named explicitly +//! (`ethlambda node --genesis ...`) or left out entirely +//! (`ethlambda --genesis ...`). Leaving it out is what the Dockerfile, +//! lean-quickstart, the hive shim and the devnet skills all do, so that form +//! stays the one this module is careful about: the token is simply removed +//! before parsing, and the very same [`CliOptions`] parser then sees the very +//! same arguments it saw before this module existed. Help text, error +//! messages, exit codes and `--version` are therefore unchanged for it, by +//! construction rather than by test. + +use std::ffi::OsString; + +use clap::Parser; + +use crate::cli::CliOptions; + +/// The sub-command token accepted in first position. +/// +/// `CliOptions` declares no positional arguments, so the first token after the +/// program name is either a flag or this sub-command: a flag *value* never +/// lands there and is never mistaken for it. +const NODE: &str = "node"; + +/// What the command line asked the binary to do. +#[derive(Debug)] +pub(crate) enum Invocation { + /// Run the consensus node. + Node(CliOptions), +} + +/// Parse the process arguments, exiting the way clap does on a parse error, +/// `--help` or `--version`. +pub(crate) fn parse() -> Invocation { + match try_parse_from(std::env::args_os()) { + Ok(invocation) => invocation, + Err(err) => err.exit(), + } +} + +fn try_parse_from(args: I) -> Result +where + I: IntoIterator, + I::Item: Into, +{ + let mut args: Vec = args.into_iter().map(Into::into).collect(); + if args.get(1).is_some_and(|arg| arg == NODE) { + args.remove(1); + } + CliOptions::try_parse_from(args).map(Invocation::Node) +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use clap::error::ErrorKind; + + use super::*; + + /// The flat invocation shape used by the Dockerfile, lean-quickstart, the + /// hive shim and the devnet skills. It must keep parsing unchanged. + const FLAT: &[&str] = &[ + "ethlambda", + "--genesis", + "config.yaml", + "--validators", + "annotated_validators.yaml", + "--bootnodes", + "nodes.yaml", + "--validator-config", + "validator-config.yaml", + "--hash-sig-keys-dir", + "hash-sig-keys/", + "--node-key", + "node.key", + "--node-id", + "ethlambda_0", + "--gossipsub-port", + "9001", + "--is-aggregator", + ]; + + /// `FLAT` with an explicit `node` sub-command token. + fn with_node_token() -> Vec<&'static str> { + let mut args = vec!["ethlambda", NODE]; + args.extend_from_slice(&FLAT[1..]); + args + } + + fn node_options(args: &[&str]) -> CliOptions { + let Invocation::Node(options) = + try_parse_from(args.iter().map(OsString::from)).expect("invocation parses"); + options + } + + #[test] + fn flat_invocation_parses_unchanged() { + let options = node_options(FLAT); + assert_eq!(options.genesis, PathBuf::from("config.yaml")); + assert_eq!(options.hash_sig_keys_dir, PathBuf::from("hash-sig-keys/")); + assert_eq!(options.node_id, "ethlambda_0"); + assert_eq!(options.gossipsub_port, 9001); + assert!(options.is_aggregator); + } + + #[test] + fn node_sub_command_accepts_the_same_flags_as_the_flat_form() { + let flat = node_options(FLAT); + let scoped = node_options(&with_node_token()); + assert_eq!(format!("{flat:?}"), format!("{scoped:?}")); + } + + #[test] + fn a_flag_value_of_node_is_not_taken_for_the_sub_command() { + let mut args: Vec<&str> = FLAT.to_vec(); + let value = args + .iter() + .position(|arg| *arg == "ethlambda_0") + .expect("node id value present"); + args[value] = NODE; + assert_eq!(node_options(&args).node_id, NODE); + } + + #[test] + fn a_node_token_after_the_flags_is_still_rejected() { + // Only a leading token is a sub-command; anywhere else it stays the + // stray positional argument it has always been. + let mut args: Vec<&str> = FLAT.to_vec(); + args.push(NODE); + let err = try_parse_from(args.iter().map(OsString::from)) + .expect_err("a trailing token must not be swallowed"); + assert_eq!(err.kind(), ErrorKind::UnknownArgument); + } + + #[test] + fn missing_required_flag_keeps_the_clap_error_in_both_forms() { + // `--genesis config.yaml` dropped from the front of the flag list. + let flat: Vec<&str> = std::iter::once("ethlambda") + .chain(FLAT[3..].iter().copied()) + .collect(); + let mut scoped = vec!["ethlambda", NODE]; + scoped.extend_from_slice(&flat[1..]); + + for args in [flat, scoped] { + let err = try_parse_from(args.iter().map(OsString::from)) + .expect_err("a missing required flag must error"); + assert_eq!(err.kind(), ErrorKind::MissingRequiredArgument); + } + } + + #[test] + fn bare_invocation_still_errors_on_the_required_flags() { + for args in [vec!["ethlambda"], vec!["ethlambda", NODE]] { + let err = try_parse_from(args.iter().map(OsString::from)) + .expect_err("an argument-less invocation must not start a node"); + assert_eq!(err.kind(), ErrorKind::MissingRequiredArgument); + } + } + + #[test] + fn help_and_version_stay_top_level_flags() { + // `ethereum/hive` builds its ethlambda image by piping + // `ethlambda --version` into a file, with and without flags in front. + let mut version_after_flags: Vec<&str> = FLAT.to_vec(); + version_after_flags.push("--version"); + + for (args, expected) in [ + (vec!["ethlambda", "--help"], ErrorKind::DisplayHelp), + (vec!["ethlambda", "--version"], ErrorKind::DisplayVersion), + (version_after_flags, ErrorKind::DisplayVersion), + ] { + let err = try_parse_from(args.iter().map(OsString::from)) + .expect_err("help and version short-circuit parsing"); + assert_eq!(err.kind(), expected); + } + } +} diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index b1d84a40..b80fa959 100644 --- a/bin/ethlambda/src/main.rs +++ b/bin/ethlambda/src/main.rs @@ -1,5 +1,6 @@ mod checkpoint_sync; mod cli; +mod command; mod fd_limit; mod version; @@ -31,8 +32,7 @@ use std::{ }; use tokio_util::sync::CancellationToken; -use clap::Parser; -use cli::CliOptions; +use command::Invocation; use ethlambda_blockchain::MILLISECONDS_PER_SLOT; use ethlambda_blockchain::block_builder::ProposerConfig; use ethlambda_blockchain::key_manager::ValidatorKeyPair; @@ -80,7 +80,7 @@ async fn main() -> eyre::Result<()> { tracing::subscriber::set_global_default(subscriber) .wrap_err("failed to set global tracing subscriber")?; - let options = CliOptions::parse(); + let Invocation::Node(options) = command::parse(); options.validate_discovery()?; #[cfg(feature = "shadow-integration")] From e518c7be9f845993c1d59f733af4926eaabc0d84 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Wed, 26 Aug 2026 13:12:13 -0300 Subject: [PATCH 2/6] refactor(cli): document `node` in --help and drop the one-variant enum Review follow-up on the sub-command module. The token is stripped before clap ever sees it, so `--help` advertised no sub-command at all and `node` was undiscoverable from the help output. It is now listed there, from a HELP_NOTE const this module owns and cli.rs only points at, and a test pins it. Invocation had a single variant, so it bought nothing that returning CliOptions does not: main.rs destructured it irrefutably, and a second variant would force that line to become a match either way. Gone until there is a second entry point to name. Also pin that a *second* `node` token is left for clap to reject like any other stray positional, and record why the two parses are compared through Debug rather than PartialEq. --- bin/ethlambda/src/cli.rs | 8 +++++- bin/ethlambda/src/command.rs | 54 +++++++++++++++++++++++------------- bin/ethlambda/src/main.rs | 3 +- 3 files changed, 42 insertions(+), 23 deletions(-) diff --git a/bin/ethlambda/src/cli.rs b/bin/ethlambda/src/cli.rs index 30382a0e..fd3a128b 100644 --- a/bin/ethlambda/src/cli.rs +++ b/bin/ethlambda/src/cli.rs @@ -6,7 +6,13 @@ use std::path::PathBuf; use crate::version; #[derive(Debug, clap::Parser)] -#[command(name = "ethlambda", author = "LambdaClass", version = version::CLIENT_VERSION, about = "ethlambda consensus client")] +#[command( + name = "ethlambda", + author = "LambdaClass", + version = version::CLIENT_VERSION, + about = "ethlambda consensus client", + after_help = crate::command::HELP_NOTE +)] pub(crate) struct CliOptions { /// Path to the chain genesis config (e.g., config.yaml). #[arg(long)] diff --git a/bin/ethlambda/src/command.rs b/bin/ethlambda/src/command.rs index f5909e2f..ec806561 100644 --- a/bin/ethlambda/src/command.rs +++ b/bin/ethlambda/src/command.rs @@ -1,4 +1,4 @@ -//! Sub-command dispatch. +//! Sub-command handling. //! //! `node` is the default sub-command: it can be named explicitly //! (`ethlambda node --genesis ...`) or left out entirely @@ -6,9 +6,9 @@ //! lean-quickstart, the hive shim and the devnet skills all do, so that form //! stays the one this module is careful about: the token is simply removed //! before parsing, and the very same [`CliOptions`] parser then sees the very -//! same arguments it saw before this module existed. Help text, error -//! messages, exit codes and `--version` are therefore unchanged for it, by -//! construction rather than by test. +//! same arguments it saw before this module existed. Its error messages, exit +//! codes and `--version` output are therefore unchanged by construction rather +//! than by test; only `--help` differs, by the [`HELP_NOTE`] it appends. use std::ffi::OsString; @@ -23,23 +23,18 @@ use crate::cli::CliOptions; /// lands there and is never mistaken for it. const NODE: &str = "node"; -/// What the command line asked the binary to do. -#[derive(Debug)] -pub(crate) enum Invocation { - /// Run the consensus node. - Node(CliOptions), -} +/// Appended to `--help` by `CliOptions`. The token never reaches clap, so +/// without this the sub-command would be undiscoverable from the help output. +pub(crate) const HELP_NOTE: &str = "Sub-commands:\n node \ + Run the consensus node (assumed when omitted)"; /// Parse the process arguments, exiting the way clap does on a parse error, /// `--help` or `--version`. -pub(crate) fn parse() -> Invocation { - match try_parse_from(std::env::args_os()) { - Ok(invocation) => invocation, - Err(err) => err.exit(), - } +pub(crate) fn parse() -> CliOptions { + try_parse_from(std::env::args_os()).unwrap_or_else(|err| err.exit()) } -fn try_parse_from(args: I) -> Result +fn try_parse_from(args: I) -> Result where I: IntoIterator, I::Item: Into, @@ -48,7 +43,7 @@ where if args.get(1).is_some_and(|arg| arg == NODE) { args.remove(1); } - CliOptions::try_parse_from(args).map(Invocation::Node) + CliOptions::try_parse_from(args) } #[cfg(test)] @@ -90,9 +85,7 @@ mod tests { } fn node_options(args: &[&str]) -> CliOptions { - let Invocation::Node(options) = - try_parse_from(args.iter().map(OsString::from)).expect("invocation parses"); - options + try_parse_from(args.iter().map(OsString::from)).expect("invocation parses") } #[test] @@ -109,6 +102,9 @@ mod tests { fn node_sub_command_accepts_the_same_flags_as_the_flat_form() { let flat = node_options(FLAT); let scoped = node_options(&with_node_token()); + // Compared through `Debug`, which the derive prints field by field, + // because `CliOptions` derives no `PartialEq` — and deriving one for a + // test would touch the parser this module deliberately leaves alone. assert_eq!(format!("{flat:?}"), format!("{scoped:?}")); } @@ -134,6 +130,17 @@ mod tests { assert_eq!(err.kind(), ErrorKind::UnknownArgument); } + #[test] + fn only_the_leading_node_token_is_stripped() { + // `ethlambda node node --genesis ...`: the second token is left for + // clap, which rejects it like any other stray positional. + let mut args = with_node_token(); + args.insert(1, NODE); + let err = try_parse_from(args.iter().map(OsString::from)) + .expect_err("only one leading token is a sub-command"); + assert_eq!(err.kind(), ErrorKind::UnknownArgument); + } + #[test] fn missing_required_flag_keeps_the_clap_error_in_both_forms() { // `--genesis config.yaml` dropped from the front of the flag list. @@ -176,4 +183,11 @@ mod tests { assert_eq!(err.kind(), expected); } } + + #[test] + fn help_documents_the_node_sub_command() { + let err = try_parse_from(["ethlambda", "--help"].iter().map(OsString::from)) + .expect_err("--help short-circuits parsing"); + assert!(err.to_string().contains(NODE), "{err}"); + } } diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index b80fa959..6638a21d 100644 --- a/bin/ethlambda/src/main.rs +++ b/bin/ethlambda/src/main.rs @@ -32,7 +32,6 @@ use std::{ }; use tokio_util::sync::CancellationToken; -use command::Invocation; use ethlambda_blockchain::MILLISECONDS_PER_SLOT; use ethlambda_blockchain::block_builder::ProposerConfig; use ethlambda_blockchain::key_manager::ValidatorKeyPair; @@ -80,7 +79,7 @@ async fn main() -> eyre::Result<()> { tracing::subscriber::set_global_default(subscriber) .wrap_err("failed to set global tracing subscriber")?; - let Invocation::Node(options) = command::parse(); + let options = command::parse(); options.validate_discovery()?; #[cfg(feature = "shadow-integration")] From ce1b33689a6c81a846399079a3919f5e0f8d4331 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Wed, 26 Aug 2026 17:17:17 -0300 Subject: [PATCH 3/6] refactor(cli): make `node` a real clap sub-command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous version removed a leading `node` token from argv and handed the rest to CliOptions, so clap never knew a sub-command existed: `--help` could not list it (a hand-written HELP_NOTE had to), usage lines could not name it, and an unknown sub-command produced a stray-positional error instead of clap's own. `node` is now an ordinary clap sub-command on a top-level parser that owns the binary's name, version and about. CliOptions becomes a plain `clap::Args` group and keeps every field exactly as it is — no `Option`, no `required = true`, no unwrap on the node path. What argv manipulation remains is one thing clap cannot express: a default sub-command. The Dockerfile, lean-quickstart, the hive shim and the devnet skills all invoke the binary as a bare list of node flags, so a command line that names no sub-command gets `node` inserted. A bare invocation is left alone, and so are `-h/--help/-V/--version` and clap's generated `help`, which is the whole of the special-casing. Two behaviours needed care to keep: - `--version` used to live on the node options, so it was accepted after node flags. `propagate_version` keeps that working, and `display_name = "ethlambda"` keeps the printed string identical rather than `ethlambda-node`; a test pins all three forms against each other. - A bare invocation now prints clap's top-level help (listing the sub-commands) instead of a missing-argument list, still exiting non-zero. The test asserts both. This is longer in lines than the token stripping it replaces — a sub-command enum and a top-level parser cost more than a `Vec::remove` — but the parts a reader must trust are now clap's, not ours. --- bin/ethlambda/src/cli.rs | 11 +-- bin/ethlambda/src/command.rs | 143 +++++++++++++++++++++++++---------- bin/ethlambda/src/main.rs | 4 +- 3 files changed, 109 insertions(+), 49 deletions(-) diff --git a/bin/ethlambda/src/cli.rs b/bin/ethlambda/src/cli.rs index fd3a128b..238b7dbb 100644 --- a/bin/ethlambda/src/cli.rs +++ b/bin/ethlambda/src/cli.rs @@ -3,16 +3,7 @@ use std::net::IpAddr; use std::path::PathBuf; -use crate::version; - -#[derive(Debug, clap::Parser)] -#[command( - name = "ethlambda", - author = "LambdaClass", - version = version::CLIENT_VERSION, - about = "ethlambda consensus client", - after_help = crate::command::HELP_NOTE -)] +#[derive(Debug, clap::Args)] pub(crate) struct CliOptions { /// Path to the chain genesis config (e.g., config.yaml). #[arg(long)] diff --git a/bin/ethlambda/src/command.rs b/bin/ethlambda/src/command.rs index ec806561..fe02f8cc 100644 --- a/bin/ethlambda/src/command.rs +++ b/bin/ethlambda/src/command.rs @@ -1,49 +1,87 @@ -//! Sub-command handling. +//! Sub-command definition and dispatch. //! -//! `node` is the default sub-command: it can be named explicitly -//! (`ethlambda node --genesis ...`) or left out entirely -//! (`ethlambda --genesis ...`). Leaving it out is what the Dockerfile, -//! lean-quickstart, the hive shim and the devnet skills all do, so that form -//! stays the one this module is careful about: the token is simply removed -//! before parsing, and the very same [`CliOptions`] parser then sees the very -//! same arguments it saw before this module existed. Its error messages, exit -//! codes and `--version` output are therefore unchanged by construction rather -//! than by test; only `--help` differs, by the [`HELP_NOTE`] it appends. +//! `node` is an ordinary clap sub-command, so clap owns its help, usage lines +//! and error messages. The one thing clap cannot express is a *default* +//! sub-command, and the node needs one: the Dockerfile, +//! lean-quickstart, the hive shim and the devnet skills all invoke the binary as +//! a bare list of node flags, from before there was anything else to run. That +//! form keeps working because a missing sub-command is filled in as `node` +//! before parsing — see [`default_subcommand`]. use std::ffi::OsString; use clap::Parser; use crate::cli::CliOptions; +use crate::version; + +/// Tokens that already say what to run, so no default is inserted ahead of +/// them. `help` is clap's own generated sub-command (`ethlambda help node`). +const EXPLICIT: &[&str] = &[NODE, "help", "-h", "--help", "-V", "--version"]; -/// The sub-command token accepted in first position. -/// -/// `CliOptions` declares no positional arguments, so the first token after the -/// program name is either a flag or this sub-command: a flag *value* never -/// lands there and is never mistaken for it. const NODE: &str = "node"; -/// Appended to `--help` by `CliOptions`. The token never reaches clap, so -/// without this the sub-command would be undiscoverable from the help output. -pub(crate) const HELP_NOTE: &str = "Sub-commands:\n node \ - Run the consensus node (assumed when omitted)"; +#[derive(Debug, clap::Parser)] +#[command( + name = "ethlambda", + author = "LambdaClass", + version = version::CLIENT_VERSION, + about = "ethlambda consensus client", + // `--version` used to sit on the node options, so it was accepted after + // node flags; `ethereum/hive` builds its image that way. Propagating it to + // the sub-commands keeps those invocations working. + propagate_version = true +)] +struct Cli { + #[command(subcommand)] + command: Command, +} + +/// What the command line asked the binary to do. +#[derive(Debug, clap::Subcommand)] +pub(crate) enum Command { + /// Run the consensus node (assumed when no sub-command is given). + /// + /// `ethlambda --genesis ...` and `ethlambda node --genesis ...` are the + /// same invocation. + // + // Deliberately not part of the doc comment above, which clap renders as + // this sub-command's `long_about`: `display_name` keeps `--version` + // printing `ethlambda ` after node flags, as it did when the node + // flags were the whole command line. It is still listed and invoked as + // `node`. + #[command(display_name = "ethlambda")] + Node(CliOptions), +} /// Parse the process arguments, exiting the way clap does on a parse error, /// `--help` or `--version`. -pub(crate) fn parse() -> CliOptions { +pub(crate) fn parse() -> Command { try_parse_from(std::env::args_os()).unwrap_or_else(|err| err.exit()) } -fn try_parse_from(args: I) -> Result +fn try_parse_from(args: I) -> Result where I: IntoIterator, I::Item: Into, { let mut args: Vec = args.into_iter().map(Into::into).collect(); - if args.get(1).is_some_and(|arg| arg == NODE) { - args.remove(1); + if let Some(token) = default_subcommand(&args) { + args.insert(1, token.into()); } - CliOptions::try_parse_from(args) + Cli::try_parse_from(args).map(|cli| cli.command) +} + +/// The sub-command to insert, if the arguments do not name one. +/// +/// `CliOptions` declares no positional arguments, so the first token after the +/// program name is either a flag or a sub-command — a flag *value* never lands +/// there and is never mistaken for one. A leading flag therefore means the flat +/// node form, and gets `node` inserted ahead of it; a bare invocation is left +/// alone so clap prints its own "requires a subcommand" help. +fn default_subcommand(args: &[OsString]) -> Option<&'static str> { + let first = args.get(1)?.to_str()?; + (!EXPLICIT.contains(&first)).then_some(NODE) } #[cfg(test)] @@ -85,7 +123,10 @@ mod tests { } fn node_options(args: &[&str]) -> CliOptions { - try_parse_from(args.iter().map(OsString::from)).expect("invocation parses") + let command = try_parse_from(args.iter().map(OsString::from)).expect("invocation parses"); + match command { + Command::Node(options) => options, + } } #[test] @@ -121,8 +162,8 @@ mod tests { #[test] fn a_node_token_after_the_flags_is_still_rejected() { - // Only a leading token is a sub-command; anywhere else it stays the - // stray positional argument it has always been. + // The default is inserted at the front or not at all, so a later token + // stays the stray positional argument it has always been. let mut args: Vec<&str> = FLAT.to_vec(); args.push(NODE); let err = try_parse_from(args.iter().map(OsString::from)) @@ -131,13 +172,11 @@ mod tests { } #[test] - fn only_the_leading_node_token_is_stripped() { - // `ethlambda node node --genesis ...`: the second token is left for - // clap, which rejects it like any other stray positional. + fn a_second_node_token_is_rejected_by_clap() { let mut args = with_node_token(); args.insert(1, NODE); let err = try_parse_from(args.iter().map(OsString::from)) - .expect_err("only one leading token is a sub-command"); + .expect_err("only one sub-command is accepted"); assert_eq!(err.kind(), ErrorKind::UnknownArgument); } @@ -158,12 +197,16 @@ mod tests { } #[test] - fn bare_invocation_still_errors_on_the_required_flags() { - for args in [vec!["ethlambda"], vec!["ethlambda", NODE]] { - let err = try_parse_from(args.iter().map(OsString::from)) - .expect_err("an argument-less invocation must not start a node"); - assert_eq!(err.kind(), ErrorKind::MissingRequiredArgument); - } + fn bare_invocation_asks_for_a_sub_command() { + // Nothing to default: clap prints the top-level help, which lists the + // sub-commands, rather than a missing-argument list for one of them. + let err = try_parse_from(["ethlambda"].iter().map(OsString::from)) + .expect_err("an argument-less invocation must not start a node"); + assert_eq!( + err.kind(), + ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand + ); + assert_ne!(err.exit_code(), 0, "a bare invocation must not exit 0"); } #[test] @@ -185,7 +228,31 @@ mod tests { } #[test] - fn help_documents_the_node_sub_command() { + fn version_output_is_identical_for_every_form() { + // `--version` moved from the node options to the top-level command, so + // pin that it still prints one string: `ethereum/hive` records this + // output as the client version. + let mut after_flags: Vec<&str> = FLAT.to_vec(); + after_flags.push("--version"); + let printed: Vec = [ + vec!["ethlambda", "--version"], + vec!["ethlambda", NODE, "--version"], + after_flags, + ] + .into_iter() + .map(|args| { + try_parse_from(args.iter().map(OsString::from)) + .expect_err("--version short-circuits parsing") + .to_string() + }) + .collect(); + assert_eq!(printed[0], printed[1]); + assert_eq!(printed[0], printed[2]); + } + + #[test] + fn help_lists_the_sub_commands() { + // Listed by clap itself, because they are real sub-commands. let err = try_parse_from(["ethlambda", "--help"].iter().map(OsString::from)) .expect_err("--help short-circuits parsing"); assert!(err.to_string().contains(NODE), "{err}"); diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index 6638a21d..480382b7 100644 --- a/bin/ethlambda/src/main.rs +++ b/bin/ethlambda/src/main.rs @@ -32,6 +32,8 @@ use std::{ }; use tokio_util::sync::CancellationToken; +use command::Command; + use ethlambda_blockchain::MILLISECONDS_PER_SLOT; use ethlambda_blockchain::block_builder::ProposerConfig; use ethlambda_blockchain::key_manager::ValidatorKeyPair; @@ -79,7 +81,7 @@ async fn main() -> eyre::Result<()> { tracing::subscriber::set_global_default(subscriber) .wrap_err("failed to set global tracing subscriber")?; - let options = command::parse(); + let Command::Node(options) = command::parse(); options.validate_discovery()?; #[cfg(feature = "shadow-integration")] From a3d7e52a7e6460b59fef23060ed33d732ad729d2 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Wed, 26 Aug 2026 18:49:05 -0300 Subject: [PATCH 4/6] fix(cli): parse the discovery tests through the sub-command dispatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging main brought #579's cli.rs tests, which call `CliOptions::parse_from`. `CliOptions` has been a `clap::Args` group since `node` became a sub-command, so that constructor no longer exists and the test build stopped compiling. Git merged both sides cleanly — the conflict is semantic, so nothing flagged it. The tests now go through `command::parse_node_options`, a single test-only helper that runs the real dispatch and hands back the node options. The command.rs tests use it too, so there is one way to parse a node command line in tests rather than two. --- bin/ethlambda/src/cli.rs | 3 +-- bin/ethlambda/src/command.rs | 20 ++++++++++++++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/bin/ethlambda/src/cli.rs b/bin/ethlambda/src/cli.rs index 480724e3..56862824 100644 --- a/bin/ethlambda/src/cli.rs +++ b/bin/ethlambda/src/cli.rs @@ -217,7 +217,6 @@ pub(crate) struct ShadowOptions { #[cfg(test)] mod tests { use super::*; - use clap::Parser as _; /// The required flags, so a test can vary only what it cares about. fn parse(extra: &[&str]) -> CliOptions { @@ -239,7 +238,7 @@ mod tests { "ethlambda_0", ]; argv.extend_from_slice(extra); - CliOptions::parse_from(argv) + crate::command::parse_node_options(argv) } /// `--discovery.enable` on its own has to work: a default that is never diff --git a/bin/ethlambda/src/command.rs b/bin/ethlambda/src/command.rs index fe02f8cc..f5f79fb4 100644 --- a/bin/ethlambda/src/command.rs +++ b/bin/ethlambda/src/command.rs @@ -84,6 +84,21 @@ fn default_subcommand(args: &[OsString]) -> Option<&'static str> { (!EXPLICIT.contains(&first)).then_some(NODE) } +/// Parse a command line that is expected to run the node, returning just its +/// options. +/// +/// `CliOptions` is a `clap::Args` group rather than a parser of its own, so +/// tests that exercise it go through the real dispatch like everything else. +#[cfg(test)] +pub(crate) fn parse_node_options(args: I) -> CliOptions +where + I: IntoIterator, + I::Item: Into, +{ + let Command::Node(options) = try_parse_from(args).expect("node options parse"); + options +} + #[cfg(test)] mod tests { use std::path::PathBuf; @@ -123,10 +138,7 @@ mod tests { } fn node_options(args: &[&str]) -> CliOptions { - let command = try_parse_from(args.iter().map(OsString::from)).expect("invocation parses"); - match command { - Command::Node(options) => options, - } + parse_node_options(args.iter().map(OsString::from)) } #[test] From 8784780fa352d2290255471ca60d0db58921435b Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Wed, 26 Aug 2026 19:13:43 -0300 Subject: [PATCH 5/6] refactor(cli): rename `CliOptions` to `NodeOptions` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The struct is the `node` sub-command's option group now, not the whole CLI — the CLI is the top-level `Cli` parser. Rename only; no field or attribute changes. Also takes the review's wording for the sub-command's help line: "default when no sub-command is given" rather than "assumed". --- bin/ethlambda/src/cli.rs | 6 +++--- bin/ethlambda/src/command.rs | 26 +++++++++++++------------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/bin/ethlambda/src/cli.rs b/bin/ethlambda/src/cli.rs index 56862824..2b774153 100644 --- a/bin/ethlambda/src/cli.rs +++ b/bin/ethlambda/src/cli.rs @@ -5,7 +5,7 @@ use std::net::IpAddr; use std::path::PathBuf; #[derive(Debug, clap::Args)] -pub(crate) struct CliOptions { +pub(crate) struct NodeOptions { /// Path to the chain genesis config (e.g., config.yaml). #[arg(long)] pub(crate) genesis: PathBuf, @@ -159,7 +159,7 @@ pub(crate) struct DiscoveryConfig { pub(crate) target_peers: usize, } -impl CliOptions { +impl NodeOptions { /// Reject a discovery port that collides with the QUIC port. /// /// Both are UDP. Without this the collision surfaces at bind time as an @@ -219,7 +219,7 @@ mod tests { use super::*; /// The required flags, so a test can vary only what it cares about. - fn parse(extra: &[&str]) -> CliOptions { + fn parse(extra: &[&str]) -> NodeOptions { let mut argv = vec![ "ethlambda", "--genesis", diff --git a/bin/ethlambda/src/command.rs b/bin/ethlambda/src/command.rs index f5f79fb4..15a4cbc3 100644 --- a/bin/ethlambda/src/command.rs +++ b/bin/ethlambda/src/command.rs @@ -12,7 +12,7 @@ use std::ffi::OsString; use clap::Parser; -use crate::cli::CliOptions; +use crate::cli::NodeOptions; use crate::version; /// Tokens that already say what to run, so no default is inserted ahead of @@ -40,7 +40,7 @@ struct Cli { /// What the command line asked the binary to do. #[derive(Debug, clap::Subcommand)] pub(crate) enum Command { - /// Run the consensus node (assumed when no sub-command is given). + /// Run the consensus node (default when no sub-command is given). /// /// `ethlambda --genesis ...` and `ethlambda node --genesis ...` are the /// same invocation. @@ -51,7 +51,7 @@ pub(crate) enum Command { // flags were the whole command line. It is still listed and invoked as // `node`. #[command(display_name = "ethlambda")] - Node(CliOptions), + Node(NodeOptions), } /// Parse the process arguments, exiting the way clap does on a parse error, @@ -74,11 +74,11 @@ where /// The sub-command to insert, if the arguments do not name one. /// -/// `CliOptions` declares no positional arguments, so the first token after the -/// program name is either a flag or a sub-command — a flag *value* never lands -/// there and is never mistaken for one. A leading flag therefore means the flat -/// node form, and gets `node` inserted ahead of it; a bare invocation is left -/// alone so clap prints its own "requires a subcommand" help. +/// `NodeOptions` declares no positional arguments, so the first token after +/// the program name is either a flag or a sub-command — a flag *value* never +/// lands there and is never mistaken for one. A leading flag therefore means +/// the flat node form, and gets `node` inserted ahead of it; a bare invocation +/// is left alone so clap prints its own "requires a subcommand" help. fn default_subcommand(args: &[OsString]) -> Option<&'static str> { let first = args.get(1)?.to_str()?; (!EXPLICIT.contains(&first)).then_some(NODE) @@ -87,10 +87,10 @@ fn default_subcommand(args: &[OsString]) -> Option<&'static str> { /// Parse a command line that is expected to run the node, returning just its /// options. /// -/// `CliOptions` is a `clap::Args` group rather than a parser of its own, so +/// `NodeOptions` is a `clap::Args` group rather than a parser of its own, so /// tests that exercise it go through the real dispatch like everything else. #[cfg(test)] -pub(crate) fn parse_node_options(args: I) -> CliOptions +pub(crate) fn parse_node_options(args: I) -> NodeOptions where I: IntoIterator, I::Item: Into, @@ -137,7 +137,7 @@ mod tests { args } - fn node_options(args: &[&str]) -> CliOptions { + fn node_options(args: &[&str]) -> NodeOptions { parse_node_options(args.iter().map(OsString::from)) } @@ -156,8 +156,8 @@ mod tests { let flat = node_options(FLAT); let scoped = node_options(&with_node_token()); // Compared through `Debug`, which the derive prints field by field, - // because `CliOptions` derives no `PartialEq` — and deriving one for a - // test would touch the parser this module deliberately leaves alone. + // because `NodeOptions` derives no `PartialEq` — and deriving one for + // a test would touch the parser this module deliberately leaves alone. assert_eq!(format!("{flat:?}"), format!("{scoped:?}")); } From 9385f6d52a1306e52dfba64841d472d653912369 Mon Sep 17 00:00:00 2001 From: Pablo Deymonnaz Date: Wed, 26 Aug 2026 19:33:23 -0300 Subject: [PATCH 6/6] refactor(cli): inline the node-options test helper into its two callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `parse_node_options` existed only so two test modules could destructure the `node` command, which is one line each. `try_parse_from` becomes `pub(crate)` — it is already the testable core that `parse()` wraps — and both callers destructure inline, so there is no test-only function in the module's surface. --- bin/ethlambda/src/cli.rs | 7 ++++++- bin/ethlambda/src/command.rs | 21 ++++----------------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/bin/ethlambda/src/cli.rs b/bin/ethlambda/src/cli.rs index 2b774153..d8bb6963 100644 --- a/bin/ethlambda/src/cli.rs +++ b/bin/ethlambda/src/cli.rs @@ -217,8 +217,12 @@ pub(crate) struct ShadowOptions { #[cfg(test)] mod tests { use super::*; + use crate::command::{Command, try_parse_from}; /// The required flags, so a test can vary only what it cares about. + /// + /// `NodeOptions` is a `clap::Args` group rather than a parser of its own, + /// so this parses through the real dispatch, as the binary does. fn parse(extra: &[&str]) -> NodeOptions { let mut argv = vec![ "ethlambda", @@ -238,7 +242,8 @@ mod tests { "ethlambda_0", ]; argv.extend_from_slice(extra); - crate::command::parse_node_options(argv) + let Command::Node(options) = try_parse_from(argv).expect("node options parse"); + options } /// `--discovery.enable` on its own has to work: a default that is never diff --git a/bin/ethlambda/src/command.rs b/bin/ethlambda/src/command.rs index 15a4cbc3..8209bc9a 100644 --- a/bin/ethlambda/src/command.rs +++ b/bin/ethlambda/src/command.rs @@ -60,7 +60,7 @@ pub(crate) fn parse() -> Command { try_parse_from(std::env::args_os()).unwrap_or_else(|err| err.exit()) } -fn try_parse_from(args: I) -> Result +pub(crate) fn try_parse_from(args: I) -> Result where I: IntoIterator, I::Item: Into, @@ -84,21 +84,6 @@ fn default_subcommand(args: &[OsString]) -> Option<&'static str> { (!EXPLICIT.contains(&first)).then_some(NODE) } -/// Parse a command line that is expected to run the node, returning just its -/// options. -/// -/// `NodeOptions` is a `clap::Args` group rather than a parser of its own, so -/// tests that exercise it go through the real dispatch like everything else. -#[cfg(test)] -pub(crate) fn parse_node_options(args: I) -> NodeOptions -where - I: IntoIterator, - I::Item: Into, -{ - let Command::Node(options) = try_parse_from(args).expect("node options parse"); - options -} - #[cfg(test)] mod tests { use std::path::PathBuf; @@ -138,7 +123,9 @@ mod tests { } fn node_options(args: &[&str]) -> NodeOptions { - parse_node_options(args.iter().map(OsString::from)) + let Command::Node(options) = + try_parse_from(args.iter().map(OsString::from)).expect("invocation parses"); + options } #[test]