feat(dry-run): let dry-run validate input and preview real effects by jpage-godaddy · Pull Request #141 · godaddy/cli

jpage-godaddy · 2026-07-21T21:41:00Z

Summary

--dry-run has always short-circuited mutating commands generically, before the handler ever ran, so it could never validate input or show a real preview. cli-engine 0.4.7 adds an opt-in escape hatch (CommandSpec::handles_dry_run) for exactly this — this PR wires it up across the commands that most needed it, and hardens each one against the "dry-run lies about what a real run would do" class of bug found during review.

domain contacts init — --dry-run now runs the real exists/--force check and reports "would overwrite"/"would create" instead of the generic "would execute" (DEVEX-890).
pat add — --dry-run now validates the env, token format, registry path, and loads the existing registry file (so a malformed pat.toml is rejected the same way a real run's save_pat would reject it) before reporting "would store" (GDDEVPLAT-81, duplicate of DEVEX-889). Rewords the invalid-token error to point at gddy guide auth rather than implying a literal typed format (the rest of DEVEX-889 already landed on rust-port via fix(pat): stop over-specifying PAT shape in error and docs (DEVEX-889) #131 — this only adds the dry-run piece). The real success path now also returns an explicit "action": "stored", matching the dry-run preview's shape; PatAddResult's schema documents it.
pat remove — same treatment: previews "would remove"/"not found" without touching the registry. (has_pat is a plain sync fn — it never awaited anything.)
env set — --dry-run validates the target environment name and previews "would activate" without persisting it. Both paths return an explicit "action", documented via a dedicated EnvSetResult schema (kept separate from EnvActive, which env get also uses and has no action concept).
dns set/dns delete — --dry-run now fetches the real current records and previews the actual reconcile plan / records that would be deleted, matching the module's own doc comment's long-standing (and previously false) promise of "a preview". The preview reuses DnsSetResult/DnsDeleteResult's own field names (replaced/created/deleted, deleted/failed) instead of inventing new ones, and plan/records are documented as optional schema fields — both matter because --output json always projects through with_default_fields absent --fields, so unlisted fields are silently dropped. delete's preview also distinguishes records that would actually delete from ones that would fail (missing recordId), mirroring the real per-record handling. Since building an accurate preview needs a live authenticated call, --dry-run on these two now resolves auth same as a real run (previously no dry-run command touched the network or auth at all).
api call — was statically tagged Tier::Mutate regardless of --method, so --dry-run --method GET was needlessly short-circuited. Now only short-circuits for non-GET/HEAD methods (checked with an allocation-free eq_ignore_ascii_case), validates the method unconditionally before that check (so a malformed method like "POST " is rejected under --dry-run too, not just on the real path), and skips auth resolution entirely on the short-circuited path (auth_optional() + a dry_run() check before requesting a credential) so a --dry-run preview of a mutating call never triggers an unnecessary login.

Also bumps the cli-engine dependency to 0.4.7 (published — no more path override needed) and rebases cleanly onto the dns module refactor (add/set/delete/list/records/conflicts split, --replace-conflicting-types) that landed on rust-port in the meantime.

Test plan

cargo build
cargo test (339 passing, including regression coverage for every fix above — notably tests that run each dry-run preview through cli_engine::output::filter_fields with the command's real default_fields string, since a pure call to the preview function alone can't catch a field being silently stripped from default output)
cargo clippy -- -D warnings
cargo fmt --check

Manual verification

# DEVEX-890
gddy domain contacts init --force --dry-run
# → {"action": "would overwrite", "path": "..."} instead of the generic string

# GDDEVPLAT-81 / DEVEX-889
gddy pat add --env ote "test" --token "literally anything" --dry-run
# → rejected with a real validation error, nothing stored

# dns set/delete preview
gddy dns set <domain> --type A --name www --data 1.2.3.4 --dry-run
gddy dns delete <domain> --type A --name www --dry-run
# → real current-records fetch + reconcile plan / would-delete list, not the generic string

# api call over-gating fix
gddy api call /v1/some/read/endpoint --method GET --dry-run
# → actually executes (previously short-circuited); a mutating method still previews without executing

--dry-run has always short-circuited mutating commands generically, before the handler ever ran, so it could never validate input or show a real preview. cli-engine 0.4.7 adds an opt-in escape hatch (CommandSpec::handles_dry_run) for exactly this; wire it up here: - domain contacts init: --dry-run now runs the real exists/--force check and reports "would overwrite"/"would create" instead of the generic "would execute" (DEVEX-890). - pat add: --dry-run now validates the env and token format for real before reporting "would store", so a garbage or empty token is rejected instead of silently succeeding (GDDEVPLAT-81). pat remove gets the same treatment. - env set: --dry-run validates the target environment name and previews "would activate" without persisting it. - dns set/delete: --dry-run now fetches the real current records and previews the actual reconcile plan / records that would be deleted, matching the module's own doc comment's long-standing (and previously false) promise of "a preview". - api call: was statically tagged Tier::Mutate regardless of --method, so --dry-run needlessly short-circuited reads too; now only short-circuits for non-GET/HEAD methods, and skips auth resolution entirely on the short-circuited path (auth_optional). Bumps the cli-engine dependency to 0.4.7 (published, no more path override needed).

Copilot

Pull request overview

This PR updates multiple mutating commands to opt into handler-driven --dry-run (via cli-engine 0.4.7’s handles_dry_run) so dry-run can still validate inputs and (for DNS) fetch live state to produce a meaningful preview instead of the engine’s generic short-circuit.

Changes:

Opts several commands into handler-driven --dry-run and adds per-command preview/validation behavior (pat add/remove, env set, domain contacts init, dns set/delete, api call).
Adds/updates tests to ensure validation still runs under --dry-run and that auth is required/optional as intended.
Bumps cli-engine dependency to 0.4.7 and updates the lockfile accordingly.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
rust/src/pat/mod.rs	Adds PAT existence check for dry-run remove, makes `pat add/remove` handle dry-run in-handler, and adds tests.
rust/src/env/mod.rs	Makes `env set` handle dry-run in-handler and adds tests.
rust/src/domain/contacts.rs	Makes `domain contacts init` handle dry-run in-handler and adds shared logic + tests for consistent behavior.
rust/src/dns/set.rs	Makes `dns set` handle dry-run in-handler and adds a reconcile-plan-based preview + unit test.
rust/src/dns/delete.rs	Makes `dns delete` handle dry-run in-handler and adds a matched-records preview + unit test.
rust/src/dns/mod.rs	Adds tests asserting auth is still required for DNS dry-run paths.
rust/src/api_explorer/mod.rs	Makes `api call` dry-run gating depend on runtime HTTP method and defers auth resolution where possible + tests.
rust/Cargo.toml	Bumps `cli-engine` to `0.4.7`.
rust/Cargo.lock	Updates resolved dependency graph for the `cli-engine` bump.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- pat::has_pat: make synchronous (never awaited anything) instead of forcing every caller to await it. - pat add --dry-run: resolve the registry path too, so it can't report success in an environment (no config dir) where a real run would immediately fail before getting that far; include it in the preview. - pat add: add an explicit "action" on the real (non-dry-run) success path too, matching the dry-run preview's shape. - dns set --dry-run: include an "action" field, matching DnsSetResult's output schema. - dns delete --dry-run: distinguish records that would actually delete from ones that would fail (missing recordId) instead of claiming every matched record as a delete, mirroring the real handler's per-record outcome; add a matching "action" field. - env set: add an explicit "action" on the real success path too, matching the dry-run preview's shape.

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

rust/src/dns/set.rs:158

dns set --dry-run returns a JSON shape that omits the command’s declared/default fields (replaced, created, deleted). Since the command is registered with with_output_schema::<DnsSetResult>() + with_default_fields("domain,type,name,replaced,created,deleted"), this can lead to missing columns/field filtering issues under dry-run output formats. Include the schema fields in the dry-run preview as well (you can keep the extra would* + plan fields).

    json!({
        "domain": domain,
        "type": record_type,
        "name": name,
        "wouldReplace": count(|a| matches!(a, SetAction::Replace { .. })),

rust/src/dns/delete.rs:95

dns delete --dry-run omits the declared/default output fields (deleted, failed) even though the command is registered with with_output_schema::<DnsDeleteResult>() and with_default_fields("domain,type,name,deleted,failed"). This can produce empty columns or break field selection in non-JSON outputs. Include deleted/failed in the preview (you can still keep wouldDelete/wouldFail and the per-record records list).

    json!({
        "domain": domain,
        "type": record_type,
        "name": name,
        "wouldDelete": would_delete,

A malformed method (e.g. "POST " with trailing whitespace) was only validated on the real execution path (method.parse()), so under --dry-run it fell through the is_mutating_method check and returned a fake "would execute" success instead of being rejected like a real run would. Validate the method unconditionally, before the dry-run check, and reuse the parsed value on the real path instead of re-parsing.

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

rust/src/dns/set.rs:162

dns set --dry-run preview uses wouldReplace/wouldCreate/wouldDelete keys, but the command’s output schema and default fields are replaced/created/deleted. This will make --dry-run table output omit the counts and diverges from the documented schema. Prefer keeping the canonical keys (with action: "would set") and optionally adding extra preview-only fields like plan.

        "wouldReplace": count(|a| matches!(a, SetAction::Replace { .. })),
        "wouldCreate": count(|a| matches!(a, SetAction::Create { .. })),
        "wouldDelete": count(|a| matches!(a, SetAction::Delete { .. })),
        "plan": plan_json,
        "action": "would set",

rust/src/dns/delete.rs:99

dns delete --dry-run preview returns wouldDelete/wouldFail, but the command’s output schema and default fields are deleted/failed. This will make default/table output omit the counts under --dry-run and diverges from the schema. Consider using the canonical keys for the preview counts (with action: "would delete") and keeping records as additional preview detail.

        "wouldDelete": would_delete,
        "wouldFail": would_fail,
        "records": records,
        "action": "would delete",
    })

dry_run_set_preview/dry_run_delete_preview used invented field names (wouldReplace/wouldCreate/wouldDelete/wouldFail) that don't appear in each command's with_default_fields list. Since --output json always projects through default_fields when the user doesn't pass --fields, every one of those fields — the entire useful content of the preview — was silently stripped down to just {domain, type, name}. Rename the preview fields to reuse DnsSetResult/DnsDeleteResult's own field names (replaced/created/deleted, deleted/failed) instead of inventing new ones, and add "action" to both commands' default_fields so it survives too. Add a regression test for each that runs the preview through cli_engine::output::filter_fields with the command's real default_fields string — a pure call to the preview function alone can't catch this, since it never goes through the projection.

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

The counts (replaced/created/deleted, deleted/failed) survived the default_fields projection after the last fix, but the actual per-record detail (plan for set, records for delete) — the part of the preview that answers "which records, specifically" — still didn't, since neither field was in with_default_fields. That undermines the point of a dry-run preview, which exists to show real removed effects, not just a count. Add plan/records to both commands' default_fields.

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

Resolving the registry path wasn't enough: a real run's save_pat also loads and parses the existing pat.toml before writing, so an existing malformed registry file makes a real run fail immediately. --dry-run skipped that load and could report "would store" success in exactly that state. Call load_registry in the dry-run branch too, and add a unit test pinning that it rejects malformed TOML.

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

DnsSetResult/DnsDeleteResult's output_schema! declarations didn't mention plan/records, even though both now appear in default output under --dry-run. --help and --schema are generated from these declarations, so users had no way to discover the fields. Add them using the macro's existing `, optional` marker, since neither field is present on the real (non-dry-run) execution path.

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

- is_mutating_method: use eq_ignore_ascii_case instead of to_ascii_uppercase, avoiding a String allocation on every call. - env set: give it its own output schema (EnvSetResult, with `action`) instead of adding `action` to the shared EnvActive schema — EnvActive is also used by env get, which never has an action concept, so widening it there would have misdocumented get's actual output.

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.

pat add's real and dry-run outputs both include "action", but PatAddResult (used only by this command, unlike EnvActive) never declared it, leaving --schema/help incomplete.

Copilot