fix(cli): fail loudly on unrecognized subcommands under any command group - #101
Merged
Conversation
…roup
Cobra's built-in "unknown command" detection (legacyArgs in args.go) only
raises an error for the root command; a non-root group's leftover args
silently pass Find(). Every command group in this CLI (alert, incident,
oncall schedule, incident war-room, the generated groups via genGroup, ...)
was declared as a bare &cobra.Command{Use, Short} with no RunE, so cobra
judged it non-Runnable and returned flag.ErrHelp — which ExecuteC always
turns into "print help, exit 0" regardless of SilenceErrors. A typo'd verb
(e.g. `incident list-alerts`, `incident list-timeline`, `alert detail`) looked
exactly like a successful run, so an agent guessing a subcommand name had no
signal that it guessed wrong.
Add a shared newGroupCmd constructor (command.go) that gives every group a
real Args validator (groupUnknownSubcommand, which reuses cobra's own
SuggestionsFor/SuggestionsMinimumDistance mechanism to append a "Did you mean
this?" block when a close match exists) plus a RunE that prints help. This
makes the group Runnable so cobra actually calls ValidateArgs on the leftover
token instead of discarding it, while running the group with no subcommand at
all is unchanged (help, exit 0).
Route every curated group constructor and the generated-command genGroup
helper through newGroupCmd so the fix applies uniformly across the whole
command tree instead of being copied into each file.
newGroupCmd (#101) gives every command group a real RunE so a mistyped subcommand fails loudly, which made Build's `c.Runnable() && !c.Hidden` predicate start emitting a card entry for all 23 groups (incident, oncall, change, ...). A group only dispatches to its subcommands; it has no behavior of its own, so documenting it as an invocable command is wrong content, not just churn. Key the predicate off HasSubCommands() instead: every node in the tree with children is a pure container (no command mixes its own business logic with child commands), so this cleanly separates groups from leaves regardless of Runnable(). Regenerating all fences with this fix reproduces the base branch's cards byte-for-byte.
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
1 participant
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.
Problem
A cobra command group (
incident,oncall,automation, …) is registered as anon-Runnable
&cobra.Command{Use, Short}. For a non-Runnable command, cobra'sexecute()returnsflag.ErrHelpbeforeValidateArgsand before anyPersistentPreRunhook ever runs, andExecuteCtranslates that into"print help, return nil".
So a typo'd or hallucinated subcommand printed the group's help text and exited 0:
An agent reading exit 0 concludes the command succeeded and moves on with no data.
The leftover argument never reaches an
Argsvalidator, so nothing else catches it.Fix
newGroupCmdgives every group a realRunE(making it Runnable, so cobra proceedsto argument validation) plus
Args: groupUnknownSubcommand, which rejects anunrecognized subcommand with a non-zero exit and a Levenshtein suggestion:
A bare group with no subcommand still prints help and exits 0 — unchanged.
groupUnknownSubcommandmirrors cobra's ownSuggestionsMinimumDistancedefault.Root is deliberately left alone: it has no parent, so cobra's built-in
legacyArgsalready rejects unknown top-level commands.
Scope
grep -rln '\.AddCommand(' internal/cli/*.golists exactly the 17 files touched hereplus
root.go. Coverage is complete, not sampled.Verification
go build ./...,go vet ./...,gofmt -l internal/cli/*.go— all cleango test ./... -count=1— 9/9 packages ok, 246/246 subtests pass, including 5 newtests in
internal/cli/group_test.gobug (help + exit 0); this branch exits 1 with a clear message. Also checked nested
groups, legit subcommands, and a bare group.
Known side effect
Making groups Runnable means root's
PersistentPreRunE/PersistentPostRun(
--output-formatvalidation, auto-update check) now run for a bare groupinvocation, which previously short-circuited earlier. The update check is gated on
isTerminalFn(stderr), false whenever stderr is piped — the normal agent/automationcase. Measured under a non-TTY: 7ms, no network call, byte-identical help output.