Report a mistyped subcommand under a group command instead of exiting 0 - #41
Merged
Conversation
cobra answers an argument that is not one of a group command's subcommands by printing that group's help to stdout and returning nil, so a caller branching on the exit code reads a failed invocation as a success and finds help text on the stream it parses for results. No Args on a group can catch it: execute returns flag.ErrHelp for a command that is not runnable before ValidateArgs is ever reached. Override the root's help function, which is what cobra calls on that path, and record the failure for Execute to print and map. One override covers the whole tree, including the completion command cobra generates during Execute, because HelpFunc walks to the parent.
The Args/RunE pair on the query group was a local stand-in for the hole the root's help override now closes for every group. Removing it also gains query the suggestion list cobra.NoArgs never produced.
The two tables spelled out a row per group for a branch they share, and started an httptest.Server per row that nothing ever dialled. A loop over groupCommands covers the same cases, and the three assertions every rejected invocation shares move into assertReportedOnce. Also assign SuggestionsMinimumDistance rather than defaulting it: nothing else in rdsh sets the field, so the guard could never take its false branch.
Merged
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.
Why
A mistyped subcommand under a group command reported success:
rdsh data-source lsitwrote thedata-sourcehelp to stdout, nothing to stderr,and exited
0. A caller — a script, or an agent branching on the exit code — read thatas "the command worked", and found help text on the stream it parses for query results.
CLAUDE.mdmakes stdout content and the exit code a machine-consumed contract; a failedinvocation satisfied neither half.
auth,data-source,profile,queryand thecompletioncommand cobra generates were all affected; onlyrdsh bogusat the rootalready reported.
cobra only reports this for the root.
FindcallslegacyArgsonly when the resolvedcommand has no
Args, andlegacyArgsreturnsunknown command …only for a root withsubcommands — under a group the command has a parent, so it returns
nil. Executionthen reaches
if !c.Runnable() { return flag.ErrHelp }inCommand.execute, which sitsbefore
ValidateArgs, andExecuteCtreatsflag.ErrHelpas "print help, returnnil". Setting
Argson the groups does not fix it — theRunnablecheck returnsfirst. cobra's own
completionis the proof: it already hasArgs: NoArgsand stillexited
0.What
Override the help function on the root — the one hook on that path — so a stray argument
to a non-runnable command becomes a recorded failure instead of a help render. One
override covers the whole tree, including the generated
completion, becauseHelpFuncwalks to the parent when a command has none of its own. No field set in rdsh's own
constructors can reach that far.
The message is rebuilt to match
legacyArgsandfindSuggestions, both unexported, soa typo reads the same at either depth. Two details that are easy to lose:
helpis answered with--help. cobra registers ahelpcommand on the root alone,so under a group it is as stray as anything else, and
SuggestionsForonly ever looksat registered subcommands — nothing would be offered without the branch. This changes
rdsh auth helpand friends from today's0to1.SuggestionsMinimumDistanceis set to2before callingSuggestionsFor.findSuggestionssets that default itself; the exportedSuggestionsForreads thedistance as it finds it. Left at zero, every Levenshtein candidate is lost —
lsitstops suggesting
list— while prefix matches keep working, so it fails quietly.No usage listing follows the message, unlike gh's equivalent: the root sets
SilenceUsageprecisely because usage on every failure is noise for the agent consumer,and a root-level typo prints none either.
The second commit drops the
Args/RunEguardrdsh querycarried for this, whosecomment said it could go once this landed.
querynow also gains the suggestion listcobra.NoArgsnever produced.Behaviour that does not change:
rdsh, and each group alone or with--help, stillprint help to stdout and exit
0;rdsh bogusstill reports the same message; everycommand still resolves and runs as before.
Both open questions, and a departure from the suggested approach
The issue left two things to the implementer and suggested a third. All three resolve
together, so they are one decision rather than three.
The suggested approach — ported from
gh— has the help function write the message tocmd.ErrOrStderr()itself and handExecutea bare boolean. This takes the otherhalf of that split: the help function builds the error and records it, and
Execute'sexisting
Error:line prints it. All seven numbered requirements hold either way, sothis departs from the prose, not from the requirements. What it buys:
travel through the same
fmt.Fprintln, so the prefix and the shape cannot drift apart.Worth naming because the sibling CLIs show both outcomes of the alternative:
slioadded a shared
errorPrefixconstant specifically to stop the two writers drifting,and
cfliohas two independent"Error:"literals with nothing tying them together.exitCodestill produces the1through itsdefault arm, so the sentence in
CLAUDE.mdnaming it as the producer of the contract'scodes stays true and needs no amendment — with no sentinel, and no bare
return 1bypassing it. Under the suggested shape this question has to be answered, which is why
the issue asks it;
sliothreads a silent sentinel through its classifier andcflioreturns
1directly, two different answers to the same prompt.runRdshIntowithif err == nil { err = report.err }leaves every outer helper's signature untouched, so the ripple into
auth_test.gotheissue anticipated does not happen, and
TestQueryGroupArgumentskeeps observing thisfailure the way it observes every other one — as a returned error.
The recorded failure lives in
helpReport, a small struct returned alongside the commandfrom
newRootCmd, rather than a package-level var — the reasonglobalFlagsalreadygives.
cflioandslioboth landed the suggested shape. 178inaba/cflio#52 and178inaba/slio#29 track bringing them to this one, so the three CLIs converge rather than
carrying three variants of one workaround.
Where the tests live, and why it is not where the issue expected
The issue predicted that a process exit code could not be observed and that the seam was
in
query_test.go. Both have moved since it was filed:internal/cmd/execute_test.gowithstartRdsh/assertExited, which read a real exit status and keep stdout and stderr apart. So thefirst five acceptance criteria go there directly — exit
1, exactly oneError:lineon stderr, empty stdout — rather than being approximated in-process.
runRdshIntoinrun_test.go, notquery_test.go, andrunRdshSplitalready separated the streams.
TestQueryGroupArgumentsstays in-process and is whatpins the helper's merge.
Every acceptance criterion was also checked against a built binary, including the cases
the tests cover, since the wiring from a recorded failure to a process exit is the part a
test seam cannot reach on its own.
Note on the exit code contract
<group> <typo>and<group> helpgo from0to1.CLAUDE.mdholds exit-codechanges to a higher bar than a human-facing CLI would, so: the meanings of
0/124/1are unchanged, and this only stops a failed invocation from claiming the successcode. No documentation change goes with it —
skills/rdsh/SKILL.mdalready says anyother failure exits
1, andREADME.mdstates only the124and its one1exception.Closes #27