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 old validation code had two independent "invalidity" conditions:
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.
["-C", "prod", "auth"]:
-C and prod are collected as skippedItems with hasValue: true.auth is the command part; lastCommandIndex lands on the auth token.-C in auth's empty flagset, found nothing, marked valid = false.auth's flagset has no flags at all, VisitAll simply never fires — valid stays true.auth's handler fires, returning ErrShowHelp, which the dispatcher converts to the correct section help.AllowUnknownFlags(true) on auth's flagset in the test is also load-bearing: without it, fs.Parse would reject -C as an unknown flag when the pre-command args are included in fullArgs. This is consistent with how section commands are constructed in practice.auth alone (triggers ErrShowHelp), auth --help, auth -h, and auth help. That's solid coverage of the help-dispatch paths.auth help (["-C", "prod", "auth", "help"]) works because "help" in the remaining args is caught by the hasHelp scan loop (line 255) — the i == 0 guard doesn't apply since "help" isn't the first arg. That path is correct.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.
Verdict: ready
{
"owner": "mirendev",
"repo": "mflags",
"number": 15,
"verdict": "ready",
"event": "approve",
"summary": "## Review\n\nThis 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.\n\n### The fix\n\nThe old validation code had two independent \"invalidity\" conditions:\n1. **Known bool flag wrongly assumed to take a value** — kept. \n2. **Pre-command flag not found in the command's flagset** — **removed**.\n\nRemoving 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.\n\nThe cleanup of `isHelpFlag` is a nice side-effect — that helper became dead code once the only caller was removed.\n\n### What I checked\n\n- The diff is focused and purposeful: only the validation guard changes; no unrelated movement.\n- I traced the execution path for the regression case `[\"-C\", \"prod\", \"auth\"]`:\n 1. `-C` and `prod` are collected as `skippedItems` with `hasValue: true`.\n 2. `auth` is the command part; `lastCommandIndex` lands on the `auth` token.\n 3. The old code looked up `-C` in `auth`'s empty flagset, found nothing, marked `valid = false`.\n 4. 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, `VisitAll` simply never fires — `valid` stays `true`.\n 5. The match succeeds and `auth`'s handler fires, returning `ErrShowHelp`, which the dispatcher converts to the correct section help.\n- The `AllowUnknownFlags(true)` on `auth`'s flagset in the test is also load-bearing: without it, `fs.Parse` would reject `-C` as an unknown flag when the pre-command args are included in `fullArgs`. This is consistent with how section commands are constructed in practice.\n- The four test cases cover the interesting variants: `auth` alone (triggers `ErrShowHelp`), `auth --help`, `auth -h`, and `auth help`. That's solid coverage of the help-dispatch paths.\n- One small observation: `auth help` (`[\"-C\", \"prod\", \"auth\", \"help\"]`) works because `\"help\"` in the remaining args is caught by the `hasHelp` scan loop (line 255) — the `i == 0` guard doesn't apply since `\"help\"` isn't the first arg. That path is correct.\n- 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.\n\n### One mild concern (not a blocker)\n\nThe 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.\n\nThe 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."
}