feat(admin-cli): generate RPC commands from protobuf#4124
Conversation
Add custom protobuf options for CLI command and argument metadata, then compile annotated unary RPCs into clap argument types and dispatch implementations. Migrate TPM CA deletion and primary DPU selection as initial examples. Signed-off-by: Dmitry Porokh <dporokh@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
crates/admin-cli/src/managed_host/mod.rs (1)
63-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSecond UUID placeholder deviates from the canonical value.
The DPU machine id example uses
abcdef01-2345-6789-abcd-ef0123456789instead of the documented canonical UUID placeholder12345678-1234-5678-90ab-cdef01234567. Since this command takes two machine-id arguments, consider reusing the canonical UUID for both (as other multi-argument examples in this codebase do) for consistency with the documented placeholder convention.As per path instructions,
crates/admin-cli/**: "Use realistic, consistent placeholder values instead of angle brackets: UUIDs as '12345678-1234-5678-90ab-cdef01234567'."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/admin-cli/src/managed_host/mod.rs` around lines 63 - 76, Update the examples in SetPrimaryDpu so both machine-ID arguments use the canonical UUID placeholder 12345678-1234-5678-90ab-cdef01234567, preserving the existing command syntax and --reboot example.Source: Path instructions
crates/macros/src/lib.rs (1)
77-99: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
mut ctxbinding is unconditional even when norunarm needs mutability.The generated
dispatch()always declaresmut ctx: RuntimeContext(line 122), but onlyrun_armsborrow it mutably.dispatch_armsconsumectxby value and the newrpc_armsonly need&ctx.api_client.0. An enum whose variants are entirely#[dispatch]/#[rpc](no plainrunvariants) will triggerunused_mut, which fails the build under this repo's "warnings as errors" policy. Neither of the two migrated enums in this PR hits this today, but it's worth guarding against now that#[rpc]adds a third non-mutating variant kind.As per coding guidelines,
**/*.rs: "Enable strict linting: treat warnings and Clippy lints as errors, avoid#[allow(...)]."🔧 Proposed conditional `mut`
- let output = quote! { - impl crate::cfg::dispatch::Dispatch for `#name` { - async fn dispatch( - self, - mut ctx: crate::cfg::runtime::RuntimeContext, - ) -> crate::errors::CarbideCliResult<()> { + let ctx_binding = if run_arms.is_empty() { + quote! { ctx } + } else { + quote! { mut ctx } + }; + + let output = quote! { + impl crate::cfg::dispatch::Dispatch for `#name` { + async fn dispatch( + self, + `#ctx_binding`: crate::cfg::runtime::RuntimeContext, + ) -> crate::errors::CarbideCliResult<()> {Also applies to: 118-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/macros/src/lib.rs` around lines 77 - 99, Update the generated dispatch method to make the RuntimeContext binding mutable only when at least one plain run arm requires mutable access. Keep dispatch_arms consuming ctx and rpc_arms borrowing it immutably, while preserving mutable binding for enums that generate run_arms; avoid suppressing the unused_mut lint.Source: Coding guidelines
crates/tonic-client-wrapper/src/codegen.rs (2)
1353-1388: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven negative-path coverage for
make_admin_cli_command.Only the happy path is tested. None of the validation branches (duplicate
--long, positional with long/short set, missing help text, unknown kind, repeated field, oneof field, and — critically — a proto3optionalfield) are exercised. A case with anoptionalfield would have caught the bug flagged above at Line 459.As per coding guidelines,
**/*.{rs,toml}should "Use table-driven tests for functions mapping inputs to outputs or errors...scenarios!withOutcomefor fallible operations."make_admin_cli_commandreturnsResult<TokenStream>, making it a good candidate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tonic-client-wrapper/src/codegen.rs` around lines 1353 - 1388, Expand test_make_admin_cli_command into a table-driven scenarios! test using Outcome for successful and failing make_admin_cli_command calls. Cover duplicate long options, positional arguments with long or short names, missing help, unknown argument kind, repeated fields, oneof fields, and proto3 optional fields, asserting each expected result or validation error while retaining the existing happy-path assertions.Source: Coding guidelines
472-481: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftEnum fields always surface as raw
i32CLI args.
resolve_field_primitive_typemapsType::Enumto"i32"and is checked before thefield.type_namebranch, so any admin_cli-annotated enum field will always become a barei32clap argument (e.g.--mode 2) rather than the generated Rust enum, even for enums this codebase already derivesclap::ValueEnumfor (DpuMode,DeletedFilter, etc. inbuild.rs). This degrades operator UX (no validation, no symbolic names) for any future RPC with an enum request field.No currently-migrated command (TPM CA delete / set-primary-dpu) appears to hit this path, so it's not blocking, but worth tracking before more RPCs are migrated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tonic-client-wrapper/src/codegen.rs` around lines 472 - 481, Update the base-type selection around resolve_field_primitive_type so enum fields use field.type_name and convert_protobuf_type_to_rust_type, allowing generated clap::ValueEnum types instead of raw i32 arguments. Keep primitive scalar fields on the existing resolve_field_primitive_type path and preserve the unsupported-type error for fields lacking both representations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tonic-client-wrapper/src/codegen.rs`:
- Around line 453-470: Update the oneof rejection in admin_cli_field_type to
exempt fields where proto3_optional is true, matching the distinction already
used by make_convenience_converter; continue rejecting genuine oneof fields
while allowing supported proto3 optional fields to reach the existing Option<T>
handling.
---
Nitpick comments:
In `@crates/admin-cli/src/managed_host/mod.rs`:
- Around line 63-76: Update the examples in SetPrimaryDpu so both machine-ID
arguments use the canonical UUID placeholder
12345678-1234-5678-90ab-cdef01234567, preserving the existing command syntax and
--reboot example.
In `@crates/macros/src/lib.rs`:
- Around line 77-99: Update the generated dispatch method to make the
RuntimeContext binding mutable only when at least one plain run arm requires
mutable access. Keep dispatch_arms consuming ctx and rpc_arms borrowing it
immutably, while preserving mutable binding for enums that generate run_arms;
avoid suppressing the unused_mut lint.
In `@crates/tonic-client-wrapper/src/codegen.rs`:
- Around line 1353-1388: Expand test_make_admin_cli_command into a table-driven
scenarios! test using Outcome for successful and failing make_admin_cli_command
calls. Cover duplicate long options, positional arguments with long or short
names, missing help, unknown argument kind, repeated fields, oneof fields, and
proto3 optional fields, asserting each expected result or validation error while
retaining the existing happy-path assertions.
- Around line 472-481: Update the base-type selection around
resolve_field_primitive_type so enum fields use field.type_name and
convert_protobuf_type_to_rust_type, allowing generated clap::ValueEnum types
instead of raw i32 arguments. Keep primitive scalar fields on the existing
resolve_field_primitive_type path and preserve the unsupported-type error for
fields lacking both representations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 256d31ab-bf3e-4875-b750-163c858aef60
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlcrates/admin-cli/src/managed_host/mod.rscrates/admin-cli/src/managed_host/set_primary_dpu/args.rscrates/admin-cli/src/managed_host/set_primary_dpu/cmd.rscrates/admin-cli/src/managed_host/set_primary_dpu/mod.rscrates/admin-cli/src/managed_host/tests.rscrates/admin-cli/src/tpm_ca/delete/args.rscrates/admin-cli/src/tpm_ca/delete/cmd.rscrates/admin-cli/src/tpm_ca/delete/mod.rscrates/admin-cli/src/tpm_ca/mod.rscrates/admin-cli/src/tpm_ca/tests.rscrates/macros/src/lib.rscrates/rpc/Cargo.tomlcrates/rpc/build.rscrates/rpc/proto/admin_cli.protocrates/rpc/proto/forge.protocrates/rpc/src/admin_cli.rscrates/rpc/src/protos/mod.rscrates/tonic-client-wrapper/Cargo.tomlcrates/tonic-client-wrapper/src/codegen.rs
💤 Files with no reviewable changes (6)
- crates/admin-cli/src/managed_host/set_primary_dpu/cmd.rs
- crates/admin-cli/src/tpm_ca/delete/args.rs
- crates/admin-cli/src/tpm_ca/delete/cmd.rs
- crates/admin-cli/src/managed_host/set_primary_dpu/mod.rs
- crates/admin-cli/src/managed_host/set_primary_dpu/args.rs
- crates/admin-cli/src/tpm_ca/delete/mod.rs
| fn admin_cli_field_type( | ||
| &self, | ||
| method_name: &str, | ||
| field: &FieldDescriptorProto, | ||
| required: bool, | ||
| ) -> Result<TokenStream> { | ||
| if field.oneof_index.is_some() { | ||
| return Err(invalid_cli_annotation(format!( | ||
| "{method_name}.{}: oneof fields are not supported", | ||
| field.name() | ||
| ))); | ||
| } | ||
| if field.label == Some(Label::Repeated as i32) { | ||
| return Err(invalid_cli_annotation(format!( | ||
| "{method_name}.{}: repeated fields are not supported", | ||
| field.name() | ||
| ))); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Proto3 optional fields will be incorrectly rejected as "oneof" fields.
field.oneof_index.is_some() fires for any field belonging to a oneof — including the synthetic one-field oneof protoc generates for proto3 optional fields (proto3_optional = true). This function has no exemption for that case, so any CLI-annotated field marked optional in the .proto will fail codegen with "oneof fields are not supported", even though the very code below it (field_is_optional(field) && !required → Option<T>) is written to support exactly this scenario. This same file already gets this distinction right in make_convenience_converter (field.oneof_index.is_some() && field.proto3_optional.is_none_or(|o| !o)).
🐛 Proposed fix
- if field.oneof_index.is_some() {
+ if field.oneof_index.is_some() && !field.proto3_optional.unwrap_or(false) {
return Err(invalid_cli_annotation(format!(
"{method_name}.{}: oneof fields are not supported",
field.name()
)));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn admin_cli_field_type( | |
| &self, | |
| method_name: &str, | |
| field: &FieldDescriptorProto, | |
| required: bool, | |
| ) -> Result<TokenStream> { | |
| if field.oneof_index.is_some() { | |
| return Err(invalid_cli_annotation(format!( | |
| "{method_name}.{}: oneof fields are not supported", | |
| field.name() | |
| ))); | |
| } | |
| if field.label == Some(Label::Repeated as i32) { | |
| return Err(invalid_cli_annotation(format!( | |
| "{method_name}.{}: repeated fields are not supported", | |
| field.name() | |
| ))); | |
| } | |
| fn admin_cli_field_type( | |
| &self, | |
| method_name: &str, | |
| field: &FieldDescriptorProto, | |
| required: bool, | |
| ) -> Result<TokenStream> { | |
| if field.oneof_index.is_some() && !field.proto3_optional.unwrap_or(false) { | |
| return Err(invalid_cli_annotation(format!( | |
| "{method_name}.{}: oneof fields are not supported", | |
| field.name() | |
| ))); | |
| } | |
| if field.label == Some(Label::Repeated as i32) { | |
| return Err(invalid_cli_annotation(format!( | |
| "{method_name}.{}: repeated fields are not supported", | |
| field.name() | |
| ))); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tonic-client-wrapper/src/codegen.rs` around lines 453 - 470, Update
the oneof rejection in admin_cli_field_type to exempt fields where
proto3_optional is true, matching the distinction already used by
make_convenience_converter; continue rejecting genuine oneof fields while
allowing supported proto3 optional fields to reach the existing Option<T>
handling.
1 participant
Many admin CLI commands only parse arguments, construct a protobuf request, and invoke one unary RPC. Maintaining separate argument, command, and dispatch modules for them creates repetitive boilerplate.
This PR explores generating that plumbing from protobuf custom annotations. It migrates tpm-ca delete and managed-host set-primary-dpu as initial examples. Commands requiring custom validation, output formatting, or multiple RPC calls remain handwritten.
Related issues
N/A
Type of Change
Breaking Changes
Testing
Additional Notes