feat(cli): accept a node sub-command for running the node - #591
Conversation
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.
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.
🤖 Kimi Code ReviewThe diff is clean, well-tested, and correctly handles the sub-command dispatch logic. No critical issues found. Minor suggestions: bin/ethlambda/src/main.rs:83 let Invocation::Node(options) = command::parse();While irrefutable today (only one enum variant), consider using bin/ethlambda/src/command.rs:28-30 Style nit: Acknowledgments:
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewNo findings. This PR is CLI-only and does not touch fork choice, attestation handling, state transition, XMSS, or SSZ paths. The Residual risk is low and mostly around CLI UX, not consensus correctness. I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewReview SummaryThe core mechanism in
One confirmed discrepancy (reported via ReportFindings): the PR description explicitly claims No security, memory-safety, or consensus-layer concerns — this PR is pure CLI plumbing and doesn't touch fork choice, attestations, state transition, or signature code. Automated review by Claude (Anthropic) · sonnet · custom prompt |
| author = "LambdaClass", | ||
| version = version::CLIENT_VERSION, | ||
| about = "ethlambda consensus client", | ||
| after_help = crate::command::HELP_NOTE |
There was a problem hiding this comment.
I don't like how we're essentially hacking our way around clap here. We should find a way to use clap's subcommands for this: https://docs.rs/clap/latest/clap/trait.Subcommand.html
There was a problem hiding this comment.
You're right, and the argv surgery is gone. node is a real Subcommand now.
CliOptions became a plain clap::Args group and keeps every field exactly as it was — no Option<T>, no required = true, no unwrap on the node path — with the binary's name/version/about moved to a top-level parser holding #[command(subcommand)]. So clap owns the parts we were hand-rolling: --help lists the sub-commands itself (the hand-written after_help note is deleted), usage lines name the sub-command, and an unknown sub-command gets clap's own error instead of a stray-positional one.
The one thing I could not get from clap is a default sub-command. There is no default_subcommand, and the flat ethlambda --genesis … form has to keep working — the Dockerfile, lean-quickstart, the hive shim and the devnet skills all pass exactly that. So a single check remains in front of the parser: if the first token names no sub-command and is not -h/--help/-V/--version or clap's generated help, node is inserted. That is the whole of it, four lines in default_subcommand.
Two behaviour changes fell out of moving the parser identity up a level, both pinned by tests:
--versionused to sit onCliOptions, so it was accepted after node flags.propagate_versionkeeps that working, anddisplay_name = "ethlambda"keeps the printed string byte-identical instead ofethlambda-node; a test asserts all three forms produce the same output.- A bare
ethlambdanow prints clap's top-level help listing the sub-commands, rather than a missing-argument list for the node. Still exits non-zero.
Worth saying plainly: this is longer than the token stripping it replaces — a sub-command enum plus a top-level parser costs more lines than a Vec::remove — but the parts a reader has to trust are clap's now instead of ours.
If you would rather have no argv handling whatsoever, the alternative is dropping the flat form and requiring ethlambda node … everywhere. That is genuinely clean, but it breaks every running devnet and needs a lean-quickstart change, so I did not take it on unilaterally — say the word and I will.
Unrelated, so it does not mislead anyone reading down the thread: the Claude review bot above reports after_help/HELP_NOTE as missing from the diff. That was true of the commit it ran on, not of the branch head — its run predates a force-push. Moot either way now, since clap lists the sub-commands on its own and HELP_NOTE is deleted.
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<T>`, 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.
| /// 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). |
There was a problem hiding this comment.
| /// Run the consensus node (assumed when no sub-command is given). | |
| /// Run the consensus node (default when no sub-command is given). |
| #[derive(Debug, clap::Parser)] | ||
| #[command(name = "ethlambda", author = "LambdaClass", version = version::CLIENT_VERSION, about = "ethlambda consensus client")] | ||
| #[derive(Debug, clap::Args)] | ||
| pub(crate) struct CliOptions { |
There was a problem hiding this comment.
We should rename this to something else, like NodeOptions
There was a problem hiding this comment.
Renamed to NodeOptions in 8784780 — it is the node sub-command's option group now, not the whole CLI. Rename only, no field or attribute changes. The PR description is updated to match.
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.
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".
`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.
2 participants
🗒️ Description / Motivation
The binary has only ever run the node, so an invocation is a bare list of node flags.
The offline block-building benchmark adds a second entry point, which means the node
first needs a name of its own.
nodeis an ordinary clap sub-command on a top-level parser that owns the binary's name,version and about.
NodeOptions(renamed fromCliOptions) becomes a plainclap::Argsgroup and keeps every field
exactly as it is — no
Option<T>, norequired = true, no unwrap helper on the nodepath, which is what the review of #497 objected to.
The flat
ethlambda --genesis ...form keeps working, because that is what theDockerfile, lean-quickstart, the hive shim and the devnet skills all pass. clap has no
default_subcommand, so exactly one thing sits in front of the parser: a command linethat names no sub-command gets
nodeinserted.What Changed
bin/ethlambda/src/command.rsCliparser +Commandsub-command enum, anddefault_subcommand, which insertsnodeunless the first token is a sub-command,-h/--help/-V/--version, or clap's generatedhelpbin/ethlambda/src/cli.rsclap::Parser→clap::Args, andCliOptionsrenamed toNodeOptions; the#[command(...)]attribute moves to the top-level parser. No field changesbin/ethlambda/src/main.rscommand::parse()and matches onCommandcommand.rsalso carries a test-onlyparse_node_optionshelper. Mergingmainbrought#579's
cli.rstests, which calledCliOptions::parse_from— aclap::Parsermethod thegroup lost when it became
clap::Args. Git merged both sides cleanly, so nothing flaggedit; the test build was broken until
a3d7e52, and both test modules now parse a nodecommand line through the real dispatch.
Correctness / Behavior Guarantees
not have to trust us for:
--helplists the sub-commands itself, usage lines name thesub-command, and an unknown sub-command produces clap's error rather than a
stray-positional one.
NodeOptionsdeclares no positional arguments, so the first token after the programname 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.
--versionafter node flags still works and still prints the same string. It usedto live on the node options, so it was accepted anywhere;
propagate_versionkeeps that,and
display_name = "ethlambda"keeps the output byte-identical rather thanethlambda-node. All three forms are asserted equal.ethlambdanow prints clap's top-level help, listingthe sub-commands, instead of a missing-argument list. It still exits non-zero, and the
test asserts both.
Tests Added / Run
Unit tests in
command.rspin: the flat parse; the two forms agreeing field for field; a--node-idvalue that is literallynode; a trailingnodetoken still rejected; asecond
nodetoken rejected; missing required flags in both forms; the bare invocation'serror kind and non-zero exit;
--help/--versionstaying top-level;--versionprintingone identical string across all three forms; and
--helplisting the sub-commands.make fmt,make lint,make test(574 tests, 30 suites) — all clean.Related Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test(cargo test --workspace --profile release-fast) — all passing