feat(command): add context-aware typed constructors and ArgGroup support - #73
Open
jpage-godaddy wants to merge 3 commits into
Open
feat(command): add context-aware typed constructors and ArgGroup support#73jpage-godaddy wants to merge 3 commits into
jpage-godaddy wants to merge 3 commits into
Conversation
…port Adds RuntimeCommandSpec::new_typed_with_context and new_typed_streaming so commands needing CommandContext (command path, middleware, --dry-run) or NDJSON streaming can still get eager, compiler-checked typed args instead of falling back to context.typed_args::<T>(). Also adds CommandSpec::arg_groups/ with_arg_group, with CommandSpec::from_args capturing clap_derive's struct-level #[group(...)] attribute, replacing required_unless_present_any/ conflicts_with chains with a single declarative relation. Closes the two gaps found while surveying gddy's 67 commands for a typed-args migration (DEVEX-954): new_typed only covered 7 of them, and the engine had no ArgGroup support at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds missing typed-constructor and argument-relation primitives to cli-engine to support migrating downstream CLIs (e.g., gddy) to strongly-typed command handlers without losing access to CommandContext, streaming, or clap ArgGroup constraints.
Changes:
- Introduces
RuntimeCommandSpec::new_typed_with_contextandRuntimeCommandSpec::new_typed_streamingto support typed args for context-aware and streaming handler shapes. - Adds
CommandSpec::arg_groupspluswith_arg_group, and extendsCommandSpec::from_args::<T>()to capture derive-registeredArgGroups (with a debug-assert for the flatten+group footgun). - Expands documentation and integration/unit tests to cover new constructors and ArgGroup behavior (builder and derive paths).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/derive_bridge.rs | Adds integration coverage for the new typed context/streaming constructors and derive-captured ArgGroup constraints. |
| src/command.rs | Implements new runtime constructors and adds ArgGroup support to CommandSpec (including clap command construction). |
| docs/design.md | Documents when to use the new typed constructors and how to express argument relations via ArgGroup (builder and derive). |
| AGENTS.md | Updates the command-authoring checklist to recommend the new APIs and warn about the flatten+group caveat. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Both structs are builder-only in practice — surveyed every consumer (gdx: 140+ command/group sites, gddy: 67+) and found zero struct-literal construction or destructuring of either type. Adding #[non_exhaustive] now, bundled into this release's existing breaking bump, means future field additions (like arg_groups in this same PR) won't need one of their own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…pec, Module, and NextAction non_exhaustive Same rationale as the earlier CommandSpec/GroupSpec change in this PR: audited every public struct with pub fields across gdx, gddy, and this crate's own tests/examples (which compile as separate crates and are subject to the same restriction as any external consumer) for literal construction or exhaustive destructuring. These five had zero hits anywhere, so marking them non_exhaustive now closes off future field additions needing a breaking release of their own, at zero real cost. Excluded from this batch: CliConfig, BuildInfo, GuideEntry, and ActivityEvent, which do have real literal-construction sites in tests/foundation.rs (CliConfig's own doc comment explicitly documents this as intentional). Also excluded: Credential, OutputField, and NextActionParam, which have real literal construction in gdx/gddy with no non-literal alternative available yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/design.md:232
- The
with_arg_groupbullet breaks Markdown list formatting because the continuation line isn’t indented, sorelationships...renders as a separate paragraph instead of part of the bullet.
- `with_arg_group` adds a `clap::ArgGroup` expressing "at least one of" or mutually-exclusive
relationships between args, without a `required_unless_present_any`/`conflicts_with` chain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of DEVEX-954 (migrate
gddyto strongly-typed commands). Closes two engine gaps found while surveyinggddy's 67 command definitions:RuntimeCommandSpec::new_typed_with_contextandnew_typed_streaming: the existingnew_typedonly covers the(CredentialResolver, T)handler shape, which matched just 7 of gddy's 67 commands. 59 usenew_with_context(needCommandContextfor command path, middleware,--dry-run) and 1 usesnew_streaming— neither had a typed equivalent, forcing a fallback tonew_with_context+ manualcontext.typed_args::<T>(). These two new constructors eagerly parseTbefore calling the handler, same guaranteenew_typedgives the credential-only case, for the two majority handler shapes.ArgGroupsupport:CommandSpechad no way to express "at least one of"/mutually-exclusive argument relationships. Confirmed by gddy's own workaround (required_unless_present_anytriplicated three ways, with a comment acknowledging the gap). AddsCommandSpec::arg_groups/with_arg_groupfor the builder path, andCommandSpec::from_args::<T>()now captures a derive struct's#[group(...)]attribute automatically. Includes a debug_assert against a real clap_derive footgun (verified against vendored source): a struct combining#[command(flatten)]with its own#[group(...)]silently gets an empty, unenforced group.Not a breaking change. Seven structs —
CommandSpec,GroupSpec,CommandResult,RuntimeCommandSpec,RuntimeGroupSpec,Module,NextAction— are now#[non_exhaustive], andCommandSpecgains a new publicarg_groupsfield. Both would normally force every literal-construction/exhaustive-destructuring site to update. Audited every public struct withpubfields across both real consumers (gdx: 140+ command/group sites;gddy: 67+) and this crate's owntests//examples/(which compile as separate crates and are subject to the identical restriction as any external consumer) — all seven had zero literal construction or exhaustive destructuring anywhere; every site uses the builder pattern (::new()/::from_args()+with_*chaining) as the docs prescribe. So neither the new field nor any of the#[non_exhaustive]markers requires a single line of downstream code to change, today or for either real consumer — and future field additions to any of the seven won't force a breaking release either.Explicitly not touched, because the audit found real literal-construction sites that would break:
CliConfig,BuildInfo,GuideEntry,ActivityEvent(all have literal construction in this crate's owntests/foundation.rs—CliConfig's doc comment explicitly documents this as intentional), andCredential,OutputField,NextActionParam(real literal construction ingdx/gddywith no non-literal alternative shipped yet).Validated end-to-end against a path-dependency build of
gddy(in a sibling worktree): convertedapplication update(context + ArgGroup) andapplication deploy(streaming) to the new constructors. All existing gddy tests (458) pass unmodified, including the tests that pin the "at least one of" error behavior — confirming theArgGroupswap is behavior-preserving.Test plan
cargo fmt --all --checkcargo clippy --all-targets -- -D warningsRUSTDOCFLAGS='-D warnings' cargo doc --no-depscargo rustdoc --lib -- -W missing-docs(zero missing docs)cargo test --all-targets(new tests intests/derive_bridge.rsandsrc/command.rs's unit test module cover both new constructors and bothArgGroupvariants — required/multiple and required/mutually-exclusive — plus the builder-path and derive-capture cases)cargo test --docgddy's full test suite (458 tests) and manualcargo runsmoke tests of the two converted commands, against a path-dependency build of this branch — re-verified after each#[non_exhaustive]additiongdx/gddy/this crate's owntests+examplesfor literal construction of every struct marked#[non_exhaustive]— zero hits anywhere, confirming no breaking impact🤖 Generated with Claude Code