cli: allow global flags before command names

[abraithwaite]

Summary

• Global flags (WithGlobals) can now appear anywhere in the argument list — before, between, or after command/subcommand names (e.g. myapp --profile staging auth login)
• If a handler defines a flag with the same name as a global flag, the framework panics with a clear error instead of silently resolving the collision
• Two commands on separate branches can independently use the same flag name without conflict

Test plan

• Global flag before command: --profile staging auth login
• Global flag between command and subcommand: auth --profile staging login
• Global flag after subcommand: auth login --profile staging
• = syntax: --profile=staging auth login
• Short flag: -p staging auth login
• Boolean global flag: --verbose auth login
• Multiple global flags mixed with command flags
• -- separator stops global stripping
• Collision panics with clear message
• Same flag name on separate branches works independently
• No regression on commands without globals
• Full existing test suite passes

[abraithwaite]

Team Review

Product Manager

Verdict: Approve with suggestions

• The feature solves a real UX problem — users expect myapp --profile staging auth login to work, and it's surprising when it doesn't.
• Collision-as-panic is the right call for a framework. Silent shadowing creates debugging nightmares for framework users.
• Docs and examples are clear. The README section with bash examples is good.
• Naming nit: "global flags" is accurate but might confuse users into thinking these are environment-wide. Most CLI frameworks call these "persistent flags" (cobra) or "parent flags." Not blocking, just worth considering for future docs.

Principal Engineer

Verdict: Needs changes — the two-pass approach can be simplified significantly

The current implementation uses three new functions in sequence:

1. scanGlobalFlags — reflect on globals to build a name→isBool map
2. stripGlobalFlags — remove global flags from args (for routing)
3. removeRoutedSegments — remove command segments from original args (for execution)

This works, but it's a Rube Goldberg: strip flags, route the stripped args, then reverse-engineer which segments of the original args were commands. A single-pass approach would be simpler and more robust.

Suggested approach: teach routeArgsWithPath to skip global flags instead of bailing out at the first - prefix. This eliminates stripGlobalFlags and removeRoutedSegments entirely:

func routeArgsWithPath(children []Node, args []string, prefix string, gflags map[string]globalFlagInfo) (Node, []string, string) {
    // Skip leading global flags to find the command name
    i := 0
    for i < len(args) {
        if args[i] == "--" { break }
        if eqIdx := strings.IndexByte(args[i], '='); eqIdx > 0 && strings.HasPrefix(args[i], "-") {
            if _, ok := gflags[args[i][:eqIdx]]; ok { i++; continue }
            break
        }
        if info, ok := gflags[args[i]]; ok {
            i++
            if !info.isBool && i < len(args) { i++ }
            continue
        }
        break
    }

    if i >= len(args) { return nil, args, prefix }
    name := args[i]
    if strings.HasPrefix(name, "-") { return nil, args, prefix }

    for _, child := range children {
        if child.nodeName() == name {
            fullPath := name
            if prefix != "" { fullPath = prefix + " " + name }
            // Remove only the matched command segment, preserve everything else
            rest := make([]string, 0, len(args)-1)
            rest = append(rest, args[:i]...)
            rest = append(rest, args[i+1:]...)

            // ... recurse for subchildren as before, passing gflags ...
            return child, rest, fullPath
        }
    }
    return nil, args, prefix
}

This cuts ~45 lines of production code (stripGlobalFlags + removeRoutedSegments), produces the correct rest in one pass, and avoids the fragility noted below by the devil's advocate.

Other notes:

• scanGlobalFlags is called on every run() invocation but the globals struct never changes. Cache it on App (e.g. compute in WithGlobals or lazily in run).
• scanGlobalFlags re-derives "is this bool?" via reflect.Kind — makeFlagDef determines the same thing via type-switch. If either changes, they could diverge. The single-pass approach above still needs scanGlobalFlags, but caching makes it less concerning.

Devil's Advocate

Verdict: Suggestions

• removeRoutedSegments is fragile. It matches command names by string equality scanning left-to-right. If a positional arg or flag value happens to equal a command name, it gets eaten. Example: myapp --label auth auth login where --label is a non-global flag with value "auth". After stripping globals (which doesn't touch --label), routing finds auth login. removeRoutedSegments scans the original args for "auth" — it eats the first one (the --label value), not the command. The remaining args become ["--label", "login"], which is wrong. This is an unlikely edge case, but the single-pass approach above avoids it entirely.
• Combined short globals aren't handled. -vp staging where -v is bool and -p takes a value — stripGlobalFlags doesn't recognize the combined form. Routing would see -vp as an unknown flag and fail. The existing FlagSet.parseShort handles this correctly at parse time, so it only affects the pre-route stripping. Not blocking, but worth a test/comment.
• The collision panic happens inside App.Run's recover(), so it silently becomes exit code 1 with a log line. Framework users might not realize their flag name is the problem. Consider returning an error from addGlobalsToFlagSet instead of panicking — the error path already exists since the function returns error.

Sysadmin Greybeard

Verdict: Approve

• CI passes, tests are thorough, coverage hits all the documented edge cases.
• The panic-on-collision surfaces clearly in logs via slog.Error("panic in command", ...) — I can grep for it. Good enough.
• No operational concerns — this is CLI startup behavior, not runtime.

Summary

Overall: Needs changes (one round)

Top priorities:

1. Collapse the two-pass strip-then-reconstruct into a single-pass routeArgsWithPath that skips global flags. Cuts ~45 lines, eliminates the removeRoutedSegments fragility, and is conceptually simpler. This addresses the "300 lines seems like a lot" concern directly.
2. Cache scanGlobalFlags on the App instead of re-reflecting on every run() call.
3. Minor: Consider returning an error instead of panicking on collision, since the panic gets swallowed by recover().