Skip to content

feat(cli): convert command args to cli-engine's typed-args API - #181

Merged
jpage-godaddy merged 1 commit into
mainfrom
cli-typed-args
Aug 4, 2026
Merged

feat(cli): convert command args to cli-engine's typed-args API#181
jpage-godaddy merged 1 commit into
mainfrom
cli-typed-args

Conversation

@jpage-godaddy

@jpage-godaddy jpage-godaddy commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Converted every stringly-typed command — 63 commands across 26 files — to #[derive(clap::Args)] structs via CommandSpec::from_args / RuntimeCommandSpec::new_typed*, now that cli-engine 0.6.1 ships those constructors on crates.io.
  • Shared arg shapes flatten a common struct (dns::records::RecordWriteArgs used by add/set; application's UiExtensionArgs used by embed/checkout).
  • Mutual-exclusion groups (auth scopes) now use #[group(...)] instead of manual conflicts_with chains.
  • Fixes a real panic in hosting nodejs deployment list --limit: its local --limit was typed u32 while the engine's global --limit override requires i64, causing a downcast panic when reading the root ArgMatches.
  • Removed now-dead stringly-typed helpers (arg_str, arg_bool, string_list, with_record_write_args, etc.) as their last caller converted.

Test plan

  • cargo check
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test --workspace
  • cargo fmt --check
  • Manually verified --help output for representative commands (platform app update, dns set, domain suggest, api call) matches pre-conversion output
  • Reproduced the hosting nodejs deployment list --limit panic before the fix and confirmed it's gone after
  • Addressed Copilot review feedback on empty-string handling in hosting nodejs commands

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings August 3, 2026 23:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 --limit type mismatch by typing the local override as i64 and 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

  • --repo and --repo-url are optional but can still be passed as empty/whitespace strings (e.g. --repo ""). Previously optional_str filtered empty strings out, but now an empty value will be forwarded into repoFullName/repoUrl. Filtering whitespace-only values to None keeps 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.

Comment thread rust/src/hosting/nodejs/mod.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated no new comments.

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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::limit shares the same arg id as cli-engine’s global --limit, which likely supplies a default (commonly 0). In that case args.limit can be Some(0) even when the user did not pass --limit, and the current conversion will forward limit=0 to list_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 to u32, similar to domain/suggest’s nonzero handling.
        |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?;

@jpage-godaddy
jpage-godaddy merged commit b6bf793 into main Aug 4, 2026
6 checks passed
@jpage-godaddy
jpage-godaddy deleted the cli-typed-args branch August 4, 2026 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants