Show section subcommand help when a global flag precedes the section#15
Conversation
The interspersed-flag resolver validated its value-consumption guesses by requiring every flag placed before a command to exist in that command's flagset. That rejected legitimate matches when a global flag (e.g. -C/--cluster) preceded a command whose own flagset doesn't declare it — most visibly section/namespace commands with empty flagsets. The resolver returned nil and the dispatcher fell back to top-level help. Only reject a match when a *known* flag was wrongly assumed to take a value (bool flags don't). Genuinely-unknown flags are still reported by fs.Parse as "unknown flag: X" after resolution, which is clearer than the previous misleading "unknown command: ...". Fixes miren help for `<global-flag> <value> <section>` invocations.
📝 WalkthroughWalkthroughThe dispatcher’s interspersed-flag validation now allows unknown pre-command flags to proceed to later parsing, while rejecting only guessed values for known boolean flags. The obsolete help-flag helper was removed. A regression test covers value-taking global flags before an Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
Review
This PR fixes a real regression in findCommandWithInterspersedFlags: when a global value-taking flag (e.g. -C prod) appeared before a section-style command that has an empty flagset, the old validation loop would see that -C is not in the command's flagset and set valid = false, causing the resolver to fall through and show top-level help instead of the section's sub-commands.
The fix
The old validation code had two independent "invalidity" conditions:
- Known bool flag wrongly assumed to take a value — kept.
- Pre-command flag not found in the command's flagset — removed.
Removing condition 2 is the right call. The comment in the new code explains the reasoning well: unknown pre-command flags are exactly the global-flags-before-subcommand idiom, and if a flag is genuinely bogus fs.Parse will report it correctly as "unknown flag: X" rather than silently swallowing it or emitting a misleading "unknown command" error.
The cleanup of isHelpFlag is a nice side-effect — that helper became dead code once the only caller was removed.
What I checked
- The diff is focused and purposeful: only the validation guard changes; no unrelated movement.
- I traced the execution path for the regression case
["-C", "prod", "auth"]:-Candprodare collected asskippedItemswithhasValue: true.authis the command part;lastCommandIndexlands on theauthtoken.- The old code looked up
-Cinauth's empty flagset, found nothing, markedvalid = false. - With this PR, the unknown-flag check is gone; only the bool-takes-value check remains, and since
auth's flagset has no flags at all,VisitAllsimply never fires —validstaystrue. - The match succeeds and
auth's handler fires, returningErrShowHelp, which the dispatcher converts to the correct section help.
- The
AllowUnknownFlags(true)onauth's flagset in the test is also load-bearing: without it,fs.Parsewould reject-Cas an unknown flag when the pre-command args are included infullArgs. This is consistent with how section commands are constructed in practice. - The four test cases cover the interesting variants:
authalone (triggersErrShowHelp),auth --help,auth -h, andauth help. That's solid coverage of the help-dispatch paths. - One small observation:
auth help(["-C", "prod", "auth", "help"]) works because"help"in the remaining args is caught by thehasHelpscan loop (line 255) — thei == 0guard doesn't apply since"help"isn't the first arg. That path is correct. - Existing tests aren't broken by the relaxation: commands with known bool flags wrongly assigned a value would still be rejected (condition 1 is intact), and the full suite of prior tests covers many valid/invalid flag combinations.
One mild concern (not a blocker)
The broadened acceptance means that if a user genuinely misplaces a flag meant for a different command before an unrelated subcommand, the dispatcher will now match that subcommand and pass the spurious flag into fullArgs. fs.Parse will then report "unknown flag: X". That's actually better UX than the old "unknown command" error, and the existing allowUnknownFlags path already handles global-flag consumers gracefully. So this is an improvement, not a regression — but worth being aware of if you ever want to add flag-provenance diagnostics.
The change is correct, the test is specific and well-structured, and the comment in the code does a good job explaining the reasoning. Ready to merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dispatcher_test.go`:
- Around line 1248-1252: The test fixture for newDispatcher is missing a real
registration for the value-taking -C flag, so it never exercises the intended
known-global-flag-before-section path. Update the fixture to declare -C on the
appropriate global or sibling command while keeping the auth section’s flagset
without it, so Dispatcher candidate validation and Run behavior are tested
against a flag that is known somewhere in the command tree but unknown to auth.
- Around line 1272-1285: The stdout capture in the test helper inside run is
brittle because io.Copy happens only after newDispatcher().Execute returns,
which can deadlock if help output fills the pipe and can also leave os.Stdout
altered on panic. Update this helper to use the project’s existing
output-capture test utility if available, or otherwise read from the pipe
concurrently while Execute runs and restore os.Stdout with defer so the original
stream is always reset even on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a67d341-ad75-45cd-8749-6aa5b60335b4
📒 Files selected for processing (2)
dispatcher.godispatcher_test.go
| // newDispatcher builds a dispatcher with a section-style "auth" command whose | ||
| // flagset declares no flags and whose Run returns ErrShowHelp, plus two | ||
| // sub-commands. A real value-taking flag "-C" is only declared on a sibling | ||
| // command, so it is unknown to the "auth" section's flagset. | ||
| newDispatcher := func() *Dispatcher { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Declare -C as a known value-taking flag in the fixture.
The fixture never registers -C; it is unknown to every flagset. Consequently, these cases do not verify the regression described by the test name and PR objective—handling a known value-taking global flag before a section. Add the real global/sibling flag registration and retain auth without that flag so the candidate-validation path is exercised.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dispatcher_test.go` around lines 1248 - 1252, The test fixture for
newDispatcher is missing a real registration for the value-taking -C flag, so it
never exercises the intended known-global-flag-before-section path. Update the
fixture to declare -C on the appropriate global or sibling command while keeping
the auth section’s flagset without it, so Dispatcher candidate validation and
Run behavior are tested against a flag that is known somewhere in the command
tree but unknown to auth.
| run := func(t *testing.T, args []string) string { | ||
| t.Helper() | ||
| old := os.Stdout | ||
| r, w, _ := os.Pipe() | ||
| os.Stdout = w | ||
|
|
||
| err := newDispatcher().Execute(args) | ||
|
|
||
| w.Close() | ||
| os.Stdout = old | ||
|
|
||
| var buf bytes.Buffer | ||
| io.Copy(&buf, r) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid synchronous pipe capture and restore stdout with defer.
io.Copy runs only after Execute returns, so the test can deadlock if help output fills the pipe buffer. Also, a panic during execution leaves the process-wide os.Stdout modified. Capture output through the project’s existing test helper, or read concurrently and restore stdout with defer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dispatcher_test.go` around lines 1272 - 1285, The stdout capture in the test
helper inside run is brittle because io.Copy happens only after
newDispatcher().Execute returns, which can deadlock if help output fills the
pipe and can also leave os.Stdout altered on panic. Update this helper to use
the project’s existing output-capture test utility if available, or otherwise
read from the pipe concurrently while Execute runs and restore os.Stdout with
defer so the original stream is always reset even on failure.
2 participants
Problem
A value-taking global flag placed before a section/namespace command broke
help rendering. In the runtime (
miren):findCommandWithInterspersedFlagscorrectly guessed that-Cconsumedprod, leavingauth help→ matched theauthsection. But it thenvalidated the guess by requiring every pre-command flag to exist in that
command's flagset. Section commands have empty flagsets, so
-Cwas unknown,the match was rejected, the resolver returned
nil, and the dispatcher fellback to
showHelp()(top-level help).This affected every section for every global flag; regular commands were
immune only because they happen to embed those flags.
Fix
Only reject a match when a known flag was wrongly assumed to take a value
(the
hasValue && IsBool()case — a genuine bad guess). Drop the"unknown flag ⇒ reject the whole command path" rule. Truly-bogus flags are
still reported by
fs.Parseafterward asunknown flag: X, which is clearerthan the old misleading
unknown command: ....Removed the now-unused
isHelpFlaghelper.Tests
TestDispatcherInterspersedValueFlagBeforeSectioncovering-C prod auth [help | --help | -h]and the bare-C prod authform.Consumed by mirendev/runtime for MIR-1309 (paired with a runtime change that
sets
AllowUnknownFlags(true)on section flagsets for the no-help form).