fix: only register --reason when authz/auditor/activity is configured - #50
Merged
Conversation
--reason was registered on every app and claimed to be "required for destructive commands," but nothing enforced or read it unless the app had wired an Authorizer, Auditor, or ActivityEmitter. Apps with none of those configured got a flag that was captured and silently discarded. register_reason_flag is now called conditionally in Cli::new, gated on config.authz/auditor/activity, mirroring the existing config.environments.is_some() pattern.
There was a problem hiding this comment.
Pull request overview
This PR updates cli_engine’s global flag registration so --reason is only exposed when it can be meaningfully consumed (authz/audit/activity), and avoids clap panics when the flag is not registered.
Changes:
- Split
--reasonregistration intoregister_reason_flagand register it conditionally inCli::new. - Updated global flag extraction to use
try_get_one("reason")to avoid panics when--reasonis not registered. - Updated tests and docs to reflect that
--reasonis conditional.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/foundation.rs | Adjusts tests to account for --reason no longer being always present; updates one command builder test to explicitly register the reason flag. |
| tests/exhaustive_public_api.rs | Updates the “common args” command builder to include register_reason_flag so API surface tests cover it. |
| src/lib.rs | Re-exports register_reason_flag from the crate root. |
| src/flags.rs | Removes unconditional --reason registration; adds register_reason_flag; makes global_flags_from_matches resilient when the arg isn’t registered. |
| src/cli.rs | Conditionally registers --reason during CLI construction based on configured authz/auditor/activity hooks. |
| docs/design.md | Documents that --reason is only registered when relevant hooks are configured. |
| docs/concepts.md | Documents the conditional presence of --reason and its intended use. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review on #50 raised three points: - --reason help text said it's forwarded to "your authorizer/auditor" but omitted the activity emitter, which also consumes it. - The authz/auditor/activity gate in Cli::new only sees CliConfig's eager fields, not an authorizer/auditor/activity wired later through init_deps (which runs per-request, after flag registration). Made this explicit in doc comments and docs/{design,concepts}.md rather than changing the gating itself, since inspecting init_deps at registration time isn't possible. - Added an integration test asserting --reason is an unknown argument with no authz/auditor/activity configured, and parses successfully once one is.
exit_code != 0 could pass for any failure, not specifically an unknown --reason argument. Cli::run forwards clap parse errors via err.exit_code(), which is clap's USAGE_CODE (2) for an unrecognized argument — assert that specific code instead.
runruh-godaddy
approved these changes
Jul 16, 2026
jpage-godaddy
pushed a commit
that referenced
this pull request
Jul 16, 2026
🤖 I have created a release *beep* *boop* --- ## [0.4.4](cli-engine-v0.4.3...cli-engine-v0.4.4) (2026-07-16) ### Features * **auth:** add scopes tracking, extensible auth commands, standalone module walk ([#49](#49)) ([bebe5bb](bebe5bb)) ### Bug Fixes * only register --reason when authz/auditor/activity is configured ([#50](#50)) ([81f50b1](81f50b1)) --- 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>
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
--reasonwas registered unconditionally byregister_global_flags, with help text claiming it was "required for destructive commands." Nothing enforced or read that value unless an app had wired anAuthorizer,Auditor, orActivityEmitter— apps without any of those (e.g. the basegddyCLI) got a flag that was captured and silently discarded (DEVEX-884).--reasonregistration out into a newregister_reason_flag, called conditionally inCli::newwhenconfig.authz/auditor/activityis set — mirroring the existingconfig.environments.is_some()conditional right below it.global_flags_from_matchesto read"reason"viatry_get_oneinstead ofget_one: clap'sget_onepanics (not just returnsNone) for an arg id that was never registered on theCommandat all, which is now the case for apps that don't configure authz/auditor/activity.docs/design.mdanddocs/concepts.mdto note the flag is conditional, and that it's gated onCliConfig's eagerauthz/auditor/activityfields specifically — an authorizer/auditor/activity emitter wired later viainit_deps(which runs per-request, after flag registration) does not register--reason(refined during review).--reasonnow mentions all three consumers (authorizer, auditor, activity emitter), not just the first two (refined during review).reason_flag_is_registered_only_when_authz_auditor_or_activity_is_configured) asserting--reasonis a clap usage error (exit code 2) with none of the three configured, and parses successfully onceauthzis set (added during review).Test plan
cargo fmt --all --checkcargo clippy --all-targets -- -D warningscargo test --all-targetsManual verification
Build a throwaway
Cliwith noauthz/auditor/activityand confirm--helpno longer lists--reason; build one with.with_authz(...)and confirm it does.