feat(cli): convert command args to cli-engine's typed-args API - #181
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the CLI’s remaining “stringly-typed” commands to cli-engine’s typed-args API (#[derive(clap::Args)] + CommandSpec::from_args / RuntimeCommandSpec::new_typed*), improving argument correctness, reducing manual JSON extraction, and eliminating several now-dead helper functions. It also aligns local --limit overrides with the engine’s global --limit type to prevent a downcast panic.
Changes:
- Converted many commands to typed args structs, including shared/flattened arg shapes and clap groups for mutual exclusion / “at least one of” constraints.
- Removed legacy arg-extraction helpers (
arg_str,string_list, DNS record arg helpers, etc.) made obsolete by typed parsing. - Fixed the
hosting nodejs deployment list --limittype mismatch by typing the local override asi64and converting safely.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rust/src/update/mod.rs | Converts update apply to typed args (ApplyArgs). |
| rust/src/scopes_cmd.rs | Converts auth scopes to typed args with a clap group for mutual exclusion. |
| rust/src/pat/mod.rs | Converts PAT add/remove commands to typed args; simplifies token resolution API. |
| rust/src/hosting/nodejs/mod.rs | Converts Node.js hosting subcommands to typed args; fixes --limit override type; removes manual arg helpers. |
| rust/src/env/mod.rs | Converts env set to typed args while avoiding collision with global --env. |
| rust/src/domain/suggest.rs | Converts domain suggest to typed args and keeps the --limit override rationale/type alignment. |
| rust/src/domain/quote.rs | Converts domain quote to typed args. |
| rust/src/domain/purchase.rs | Converts domain purchase to typed args. |
| rust/src/domain/operation.rs | Converts domain operation status to typed args. |
| rust/src/domain/nameservers.rs | Converts domain nameservers set to typed args. |
| rust/src/domain/mod.rs | Removes string_list re-export now that callers are typed. |
| rust/src/domain/list.rs | Converts domain list to typed args. |
| rust/src/domain/get.rs | Converts domain get to typed args. |
| rust/src/domain/contacts.rs | Converts domain contacts init to typed args. |
| rust/src/domain/common.rs | Removes string_list helper (no longer needed with typed Vec args). |
| rust/src/domain/available.rs | Converts domain available to typed args. |
| rust/src/domain/agreements.rs | Converts domain agreements to typed args. |
| rust/src/dns/set.rs | Converts dns set to typed args using flattened RecordWriteArgs. |
| rust/src/dns/records.rs | Introduces RecordWriteArgs and adapts RecordOptions construction; removes old arg helpers. |
| rust/src/dns/list.rs | Converts dns list to typed args with requires relationships. |
| rust/src/dns/delete.rs | Converts dns delete to typed args. |
| rust/src/dns/add.rs | Converts dns add to typed args using shared RecordWriteArgs. |
| rust/src/application/commands/mod.rs | Converts multiple platform app commands/groups to typed args, including structured groups/flattening. |
| rust/src/api_explorer/mod.rs | Converts API explorer commands to typed args; removes string_list helper/tests made obsolete. |
| rust/src/actions_catalog/mod.rs | Converts actions catalog describe to typed args. |
| rust/Cargo.lock | Updates dependencies including cli-engine to 0.6.1 and related transitive bumps. |
Suppressed comments (1)
rust/src/hosting/nodejs/mod.rs:628
--repoand--repo-urlare optional but can still be passed as empty/whitespace strings (e.g.--repo ""). Previouslyoptional_strfiltered empty strings out, but now an empty value will be forwarded intorepoFullName/repoUrl. Filtering whitespace-only values toNonekeeps the previous behavior and avoids sending invalid payload fields.
if let Some(repo) = args.repo {
body["repoFullName"] = json!(repo);
}
if let Some(repo_url) = args.repo_url {
body["repoUrl"] = json!(repo_url);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replaces stringly-typed clap::Arg + ctx.args.get("key") access with typed
derive(clap::Args) structs and CommandSpec::from_args across every gddy
command, now that cli-engine 0.6.1 ships the typed-args constructors.
Shared arg shapes (dns add/set, embed/checkout extensions) flatten a common
struct; mutual-exclusion groups (auth scopes) use a group(...) attribute
instead of manual conflicts_with chains.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
144f591 to
9575bf8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.
Suppressed comments (1)
rust/src/hosting/nodejs/mod.rs:404
DeploymentListArgs::limitshares the same arg id as cli-engine’s global--limit, which likely supplies a default (commonly0). In that caseargs.limitcan beSome(0)even when the user did not pass--limit, and the current conversion will forwardlimit=0tolist_deployments. That regresses the prior behavior of omitting missing/non-positive limits (so the API can apply its own default). Consider filtering out non-positive values before converting tou32, similar todomain/suggest’snonzerohandling.
|ctx, args: DeploymentListArgs| async move {
let app_id = args.app_id;
let limit = args.limit.and_then(|n| u32::try_from(n).ok());
let client = make_client(&ctx, &[APPS_READ]).await?;
Summary
#[derive(clap::Args)]structs viaCommandSpec::from_args/RuntimeCommandSpec::new_typed*, now thatcli-engine0.6.1 ships those constructors on crates.io.dns::records::RecordWriteArgsused byadd/set;application'sUiExtensionArgsused byembed/checkout).auth scopes) now use#[group(...)]instead of manualconflicts_withchains.hosting nodejs deployment list --limit: its local--limitwas typedu32while the engine's global--limitoverride requiresi64, causing a downcast panic when reading the rootArgMatches.arg_str,arg_bool,string_list,with_record_write_args, etc.) as their last caller converted.Test plan
cargo checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspacecargo fmt --check--helpoutput for representative commands (platform app update,dns set,domain suggest,api call) matches pre-conversion outputhosting nodejs deployment list --limitpanic before the fix and confirmed it's gone afterhosting nodejscommands🤖 Generated with Claude Code