Skip to content

feat(cli)!: tidy --help output and make search a real command - #68

Merged
jpage-godaddy merged 3 commits into
mainfrom
cli-engine-field-defaults
Jul 30, 2026
Merged

feat(cli)!: tidy --help output and make search a real command#68
jpage-godaddy merged 3 commits into
mainfrom
cli-engine-field-defaults

Conversation

@jpage-godaddy

@jpage-godaddy jpage-godaddy commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • --fields now shows its native clap default (like --dry-run's [default: false]) instead of a hardcoded fallback; the per-command Output fields: table moved from the command description into --fields's own help text, with each default field marked (default).
  • --filter/--expr carry contextual usage examples on their own flags instead of a disconnected "Filter examples:"/"Expr examples:" section.
  • --json/--toon/--human shorthand flags are hidden from --help (still fully functional) and documented on --output instead, whose displayed default now reflects the actual TTY-based choice rather than a hardcoded json.
  • --dry-run is hidden from --help on commands that don't mutate, where it's a no-op.
  • Command-specific flags render before global flags in --help, in declaration order, and global flags render in the engine's own declared order — fixes clap's colliding per-Command auto display-order counters, which previously interleaved the two. This also covers the conditionally-registered --env flag (added directly in Cli::new when CliConfig.environments is set), which Copilot's review caught was missing the same fix (refined during review).
  • BREAKING CHANGE: --search is now a real search <query...> [--scope <path>] command instead of a raw-argv pre-parse bypass flag that silently discarded whatever command it was attached to (some-command --search x used to return empty results instead of running some-command or erroring). Removed the now-dead GlobalFlags.search, Middleware.search, and the public extract_search_query function.

Test plan

  • cargo fmt --all --check
  • cargo clippy --all-targets -- -D warnings
  • RUSTDOCFLAGS='-D warnings' cargo doc --no-deps
  • cargo rustdoc --lib -- -W missing-docs (zero)
  • cargo test --all-targets
  • cargo test --doc
  • Verified against a real consumer CLI binary patched to this branch (help output, search/--scope, hidden-flag behavior, TTY-aware --output default)
  • Regression test for the --env display-order fix, confirmed to fail against the pre-fix code

🤖 Generated with Claude Code

- `--fields` now shows its native clap default (like `--dry-run`'s
  `[default: false]`) instead of a hardcoded fallback; the per-command
  `Output fields:` table moved from the command description into
  `--fields`'s own help text, with each default field marked `(default)`.
- `--filter`/`--expr` carry contextual usage examples on their own
  flags instead of a disconnected "Filter examples:"/"Expr examples:"
  section.
- `--json`/`--toon`/`--human` shorthand flags are hidden from `--help`
  (still fully functional) and documented on `--output` instead, whose
  displayed default now reflects the actual TTY-based choice rather
  than a hardcoded `json`.
- `--dry-run` is hidden from `--help` on commands that don't mutate,
  where it's a no-op.
- Command-specific flags render before global flags in `--help`, in
  declaration order, and global flags render in the engine's own
  declared order — fixes clap's colliding per-`Command` auto
  display-order counters, which previously interleaved the two.
- BREAKING CHANGE: `--search` is now a real `search <query...>
  [--scope <path>]` command instead of a raw-argv pre-parse bypass
  flag that silently discarded whatever command it was attached to
  (`some-command --search x` used to return empty results instead of
  running `some-command` or erroring). Removed the now-dead
  `GlobalFlags.search`, `Middleware.search`, and the public
  `extract_search_query` function.

Test plan:
- cargo fmt --all --check
- cargo clippy --all-targets -- -D warnings
- RUSTDOCFLAGS='-D warnings' cargo doc --no-deps
- cargo rustdoc --lib -- -W missing-docs (zero)
- cargo test --all-targets
- cargo test --doc
- Verified against a real consumer CLI binary patched to this branch

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

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 refines cli-engine’s help/UX around global flags and command help text, and turns legacy --search behavior into a first-class search <query...> [--scope <path>] built-in command (removing the pre-parse bypass and related public APIs).

Changes:

  • Reworked help rendering: per-command output-fields table moved onto --fields, contextual examples moved onto --filter/--expr, and output-format shorthands are hidden and documented via --output.
  • Fixed --help flag ordering by introducing explicit display-ordering for engine globals and enforcing declaration-order for command-specific flags.
  • Replaced --search bypass with a real search built-in command, updating docs and tests and removing GlobalFlags.search, Middleware.search, and extract_search_query.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/foundation.rs Updates/expands tests for help text, flag ordering, hidden shorthands, default fields marking, and new search command behavior.
tests/exhaustive_public_api.rs Removes --search/search coverage from global flags parsing expectations.
tests/consumer_cli.rs Updates consumer-style test to use search ... --scope ... instead of legacy --search.
tests/argv0_dispatch.rs Updates argv0 dispatch test to invoke the search command.
src/output/schema.rs Extends help-section formatting to mark default fields and removes standalone filter/expr examples section.
src/middleware.rs Removes the legacy search field from middleware state.
src/lib.rs Stops re-exporting removed extract_search_query API.
src/flags.rs Adds explicit display-ordering for global flags; hides output shorthands; updates output help/default behavior; removes search flag and extraction helper.
src/command.rs Forces command-specific arg ordering via explicit display_order in declaration order.
src/cli/help.rs Updates root help “Find Commands” section to show search <keyword> as a command and improves alignment.
src/cli/builtins.rs Introduces the search built-in command and args parsing.
src/cli.rs Wires search into built-ins, removes search bypass, resolves --scope canonically, hides certain globals per-command, and filters hidden args out of search indexing.
docs/design.md Updates documentation from --search bypass to search built-in command.
docs/concepts.md Updates concepts documentation to the new search command and scope semantics.
docs/argv0-dispatch.md Updates argv0 dispatch docs to reference the search command instead of --search.
Comments suppressed due to low confidence (1)

src/cli.rs:903

  • The new explicit display_order scheme for global flags relies on every global arg having a high, non-colliding order. The conditional --env flag (registered when CliConfig.environments is set) is global(true) but has no display_order, so clap will assign it an implicit low order (likely 0) and it can interleave with command-specific args again, undermining the help-ordering fix for env-enabled CLIs.
            .subcommand(search_command());
        if let Some(register_flags) = &config.register_flags {
            root = register_flags(root);
        }
        // `--reason` is only meaningful when something actually consumes it —

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

Copilot review on #68 caught that the engine-registered `--env` flag
(added directly in `Cli::new`, conditionally, when `CliConfig.environments`
is set — not via `register_global_flags`) had no `display_order`, so it
fell back to clap's implicit low per-`Command` counter and collided with
command-specific flags again, exactly the bug the rest of this PR fixes
for every other global flag.

Add `global_flag_order::ENV` and apply it. Regression test confirmed
against the pre-fix code (fails: --env sorts before --help and the
command's own flags; passes after).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

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 15 out of 15 changed files in this pull request and generated no new comments.

@jpage-godaddy

jpage-godaddy commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

Here's a before and after for reference.

Before:

List the domains registered to your account. Shows domain, status, expiry, and auto-renew by default; use --fields to pick columns. Hides domains that are cancelled or otherwise not visible unless --show-hidden is passed; use --status to filter to specific status values (repeatable, overrides the default filter).

Output fields:
  authCode                string  (optional)
  contactAdmin            object  (optional)
  contactBilling          object  (optional)
  contactRegistrant       object  (optional)
  contactTech             object  (optional)
  createdAt               string  (optional)
  deletedAt               string  (optional)
  domain                  string  (optional)
  domainId                float  (optional)
  expirationProtected     bool  (optional)
  expires                 string  (optional)
  exposeWhois             bool  (optional)
  holdRegistrar           bool  (optional)
  locked                  bool  (optional)
  nameServers             []string  (optional)
  privacy                 bool  (optional)
  registrarCreatedAt      string  (optional)
  renewAuto               bool  (optional)
  renewDeadline           string  (optional)
  renewable               bool  (optional)
  status                  string  (optional)
  transferAwayEligibleAt  string  (optional)
  transferProtected       bool  (optional)

Filter examples:
  --filter "contains(authCode, 'example')"
  --filter 'expirationProtected'

Expr examples:
  --expr 'length(@)'
  --expr '[].authCode'


Usage: gddy domain list [OPTIONS]

Options:
  -h, --help
          Print help

      --status <STATUS>
          Only domains with this status, e.g. ACTIVE (repeatable)

  -o, --output <FORMAT>
          Output format: toon|json|human

          [default: json]

      --show-hidden
          Include domains hidden by default, e.g. cancelled or confiscated

      --verbose [<FIELDS>]
          Include metadata in output (all, or comma-separated: system,duration,args,env,identity,command,effective_args,timestamp)

      --dry-run[=<dry-run>]
          Preview mutations without executing

          [default: false]

      --fields <FIELDS>
          Comma-separated fields to include in output (use 'all' or '*' for everything)

      --filter <EXPR>
          Per-item JMESPath predicate for list data

      --expr <EXPR>
          JMESPath query applied to the whole result

      --limit <limit>
          Max items to return (client-side, 0=all)

          [default: 0]

      --offset <offset>
          Skip N items before applying limit

          [default: 0]

      --schema[=<schema>]
          Dump output field metadata instead of running the command

          [default: false]

      --timeout <DURATION>
          Overall command timeout (e.g. 60s, 5m); default 0s = no timeout

          [default: 0s]

      --debug [<PATTERN>]
          Enable debug logging (comma-separated component patterns, e.g. *, transport, *,-auth)

      --search <KEYWORD>
          Search commands and guides by keyword

      --credential-store <MODE>
          Credential storage: auto|keyring|file (overrides env and config)

      --json
          Shorthand for --output json

      --toon
          Shorthand for --output toon

      --human
          Shorthand for --output human

      --env <ENV>
          Override the active environment (see: env list)

After:

List the domains registered to your account. Shows domain, status, expiry, and auto-renew by default; use --fields to pick columns. Hides domains that are cancelled or otherwise not visible unless --show-hidden is passed; use --status to filter to specific status values (repeatable, overrides the default filter).

Usage: gddy domain list [OPTIONS]

Options:
      --status <STATUS>
          Only domains with this status, e.g. ACTIVE (repeatable)

      --show-hidden
          Include domains hidden by default, e.g. cancelled or confiscated

      --env <ENV>
          Override the active environment (see: env list)

  -h, --help
          Print help

  -o, --output <FORMAT>
          Output format: toon|json|human (shorthand: --json, --toon, --human); defaults to human in an interactive terminal, json otherwise

          [default: human]

      --verbose [<FIELDS>]
          Include metadata in output (all, or comma-separated: system,duration,args,env,identity,command,effective_args,timestamp)

      --fields <FIELDS>
          Comma-separated fields to include in output (use 'all' or '*' for everything)

          Output fields:
            authCode                string  (optional)
            contactAdmin            object  (optional)
            contactBilling          object  (optional)
            contactRegistrant       object  (optional)
            contactTech             object  (optional)
            createdAt               string  (optional)
            deletedAt               string  (optional)
            domain                  string  (optional)  (default)
            domainId                float  (optional)
            expirationProtected     bool  (optional)
            expires                 string  (optional)  (default)
            exposeWhois             bool  (optional)
            holdRegistrar           bool  (optional)
            locked                  bool  (optional)
            nameServers             []string  (optional)
            privacy                 bool  (optional)
            registrarCreatedAt      string  (optional)
            renewAuto               bool  (optional)  (default)
            renewDeadline           string  (optional)
            renewable               bool  (optional)
            status                  string  (optional)  (default)
            transferAwayEligibleAt  string  (optional)
            transferProtected       bool  (optional)

          [default: domain,status,expires,renewAuto]

      --filter <EXPR>
          Per-item JMESPath predicate for list data
          e.g. --filter "contains(authCode, 'example')"
          e.g. --filter 'expirationProtected'

      --expr <EXPR>
          JMESPath query applied to the whole result
          e.g. --expr 'length(@)'
          e.g. --expr '[].authCode'

      --limit <limit>
          Max items to return (client-side, 0=all)

          [default: 0]

      --offset <offset>
          Skip N items before applying limit

          [default: 0]

      --schema[=<schema>]
          Dump output field metadata instead of running the command

          [default: false]

      --timeout <DURATION>
          Overall command timeout (e.g. 60s, 5m); default 0s = no timeout

          [default: 0s]

      --debug [<PATTERN>]
          Enable debug logging (comma-separated component patterns, e.g. *, transport, *,-auth)

      --credential-store <MODE>
          Credential storage: auto|keyring|file (overrides env and config)

Comment thread src/cli.rs
Comment thread src/flags.rs Outdated
…st gap

Addresses Qi's review feedback on #68:

- An unresolvable `search --scope <path>` (typo, or a stale/removed
  command) used to fall back to a full-tree search with no indication
  anything was off. Now prints a best-effort stderr warning first
  ("warning: --scope ... did not match a known command path; searching
  everything instead") and proceeds with the same full-tree search as
  before — no behavior change on stdout, no error, just a heads-up.
  Written directly to a locked stderr handle, matching the transport
  module's `StderrTransportLogger` convention for this kind of
  best-effort side-channel diagnostic (outside `Cli::run`'s captured
  output, so not asserted on directly in tests — the functional
  fallback is).
- Closed the numbering gap in `global_flag_order` left by removing
  `SEARCH` a few commits back (1012 was skipped). Values are compared
  only relatively, so the gap was cosmetic, but normalizing doesn't
  hurt.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@jpage-godaddy
jpage-godaddy merged commit 8353f96 into main Jul 30, 2026
2 checks passed
@jpage-godaddy
jpage-godaddy deleted the cli-engine-field-defaults branch July 30, 2026 22:13
jpage-godaddy pushed a commit that referenced this pull request Jul 30, 2026
🤖 I have created a release *beep* *boop*
---


##
[0.5.0](cli-engine-v0.4.10...cli-engine-v0.5.0)
(2026-07-30)


### ⚠ BREAKING CHANGES

* **cli:** make search a real command
([#68](#68))

### Features

* **cli:** tidy --help output
([#68](#68))
([8353f96](8353f96))


### Miscellaneous

* **deps:** bump quinn-proto from 0.11.14 to 0.11.16
([#67](#67))
([e508548](e508548))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
jpage-godaddy added a commit that referenced this pull request Jul 31, 2026
…nfig (#69)

## Summary

Replaces the stringly-typed environment config system with a general
`#[derive(EnvConfig)]` mechanism: a typed struct declares, per field,
where to find its value (a TOML key, an opt-in app-scoped env-var
suffix, a literal or computed default) and how to convert it, and
`Environments` threads a chain of sources through the struct's generated
`assemble` function to build it.

**Supersedes `more-flexible-envs` (#66) outright rather than building on
top of it** — none of that branch's
`EnvironmentDef`/`with_init`/`field_defaults`/`field_validators` design
survives here. #66 should stay closed unmerged.

- New `cli-engine-macros` crate (required — proc-macro crates compile
for the host, not the target) providing `#[derive(EnvConfig)]`, and
`src/env_config.rs` with the runtime pieces: the `ConfigSource` trait,
`EnvSource`/`EnvVarSource`/`ValueSource`, and `SourceChain`.
- `Environments` (`src/environments.rs`) rewritten around this:
`EnvTable` stores typed `toml::Value` data instead of stringified
fields, `with_environment` handles compiled-in environment
registrations, and `resolve::<T>()` assembles any `EnvConfig` struct
from the config file and environment variables.
- `PkceAuthProvider`'s OAuth config is now an ordinary `EnvConfig`
struct (`OAuthSection`).
- Feature-flags also use this configuration system

Implements DEVEX-947 with a materially different design than #66;
verified end-to-end against a real consumer (`gddy`, patched via a path
dependency) throughout development.

## What this actually looks like for a consumer

The old system described each config field as an entry in a shared,
loosely-typed lookup table, with separate helper functions bolted on by
key name for validation/computed defaults:

```rust
// BEFORE — every field is a plain string in a shared map. A typo in a key
// name, or a field that should really be a number or a list, is invisible
// to the compiler and only shows up as a bug when something runs.
let environments = Environments::new("prod").with_environment(
    "prod",
    EnvironmentDef::new()
        .with_client_id("prod-client-id")
        .with_field("api_url", "https://api.example.com")
        .with_field_validator("auth_url", looks_like_a_url)
        .with_field_default("account_url", |env| derive_account_url(env)),
);
```

Now, a consumer just writes an ordinary Rust struct with real field
types. A couple of small annotations on top say *where to look for a
value* and *what to do if nothing supplied one* — the framework does the
actual lookup and type-conversion:

```rust
#[derive(EnvConfig)]
struct ApiConfig {
    pub api_url: String,
    pub client_id: String,

    // "GDDY_AUTH_URL" (or whatever the app is called) now overrides this
    // field automatically at runtime — no extra code anywhere for that.
    #[env_config(env = "AUTH_URL", default_fn = default_auth_url)]
    pub auth_url: String,

    // A real list, not a comma-joined string someone has to split by hand.
    #[env_config(default = Vec::new())]
    pub scopes: Vec<String>,
}
```

Fetching a fully-resolved config for the active environment is one line,
and you get back a normal, already-correct Rust value — no manual
per-field merging, no "did every layer actually apply" bookkeeping:

```rust
let config: ApiConfig = environments.resolve("prod")?;
```

And because every field's type is real (not a stringified detour), a
required field that's genuinely missing everywhere — not configured in
the file, no default, nothing — is a clear, immediate error instead of a
silently wrong empty value making it into a live request.

## Test plan
- [x] `cargo fmt --all --check`
- [x] `cargo clippy --all-targets --all-features -- -D warnings`
- [x] `RUSTDOCFLAGS='-D warnings' cargo doc --no-deps --all-features`
- [x] `cargo test --all-targets --all-features` (236 lib tests + 282
integration tests, all passing)
- [x] `cargo test --doc`
- [x] Confirmed `debug_transport_logger_for` (#61), the
`ENV_LOCK`-guarded `persist_active_without_app_id_errors_clearly` test
(#61), and the `Fix`/`with_fix`/`error_fix` machinery (#65) all survived
the merge from `origin/main` intact and still pass.
- [x] Merged `main` a second time mid-review to pick up #68's
`--help`/search changes and the 0.5.0 release; resolved the one real
conflict (a `tests/foundation.rs` test still using the old
`EnvironmentDef` API) and re-verified the full suite.
- [x] `gddy` (path-patched consumer) builds and passes its own full
suite against this branch.
- [x] Addressed both Copilot review findings: a docs typo, and a real
one — derive-generated code referenced `toml::Value` in a way that
forced every consumer to add their own direct `toml` dependency even for
plain fields with no custom conversion. Fixed by re-exporting `toml`
from `cli_engine::env_config` and routing generated code through it.
- [x] `cargo publish --dry-run` in CI now tolerates the one expected
gap: `cli-engine-macros` isn't on crates.io until its first release
(wired into `release-please-config.json`/`release.yml` to publish it
before `cli-engine`, in dependency order) — any other dry-run failure
still fails the check.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
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