From c5721393cc8d1ab5ad853db8e60706354a87227d Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Tue, 21 Apr 2026 09:58:15 -0500 Subject: [PATCH 1/6] fix(cli): surface usage errors on stderr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main() skipped stderr emission for any error wrapping ErrUsage on the theory the error had already been reported. Leaf flag errors bubble up through ParseFlags wrapped in ErrUsage, but every leaf's FlagSet uses io.Discard output — so nothing was ever printed. Result: misplaced or unknown leaf flags exited 1 with no output. Print every non-ErrHelp error to stderr. ErrHelp stays skipped since its text is already written to stdout by the help-rendering code path. --- cmd/ana/main.go | 9 +++++---- cmd/ana/main_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/cmd/ana/main.go b/cmd/ana/main.go index a467555..72a21e7 100644 --- a/cmd/ana/main.go +++ b/cmd/ana/main.go @@ -32,10 +32,11 @@ import ( func main() { stdio := cli.DefaultIO() err := run(os.Args[1:], stdio, os.Getenv) - if err != nil && !errors.Is(err, cli.ErrUsage) && !errors.Is(err, cli.ErrHelp) { - // ErrUsage and ErrHelp mean help/usage text has already been emitted - // to the appropriate stream; any other error is a runtime failure - // that hasn't been reported yet, so surface it on stderr. + if err != nil && !errors.Is(err, cli.ErrHelp) { + // ErrHelp means help text was already written to stdout; skip. Every + // other error (including ErrUsage from leaves whose FlagSet output is + // io.Discard) still needs to surface — otherwise misplaced flags + // exit 1 with silent output. fmt.Fprintln(stdio.Stderr, err) } os.Exit(cli.ExitCode(err)) diff --git a/cmd/ana/main_test.go b/cmd/ana/main_test.go index 3049594..252c0bc 100644 --- a/cmd/ana/main_test.go +++ b/cmd/ana/main_test.go @@ -510,6 +510,31 @@ func TestRun_EndToEnd_ConnectorList(t *testing.T) { } } +// TestRun_LeafUsageErrorReturned asserts that an unknown flag on a leaf verb +// surfaces as a non-nil ErrUsage-wrapped error whose message names the leaf +// and the stdlib "flag provided but not defined" cause. main() then prints +// the error text on stderr — previously it was swallowed because main skipped +// any ErrUsage-wrapping error as "already reported", but leaf FlagSets use +// io.Discard output so nothing had actually been reported. +func TestRun_LeafUsageErrorReturned(t *testing.T) { + t.Parallel() + var out, errb bytes.Buffer + stdio := cli.IO{Stdin: strings.NewReader(""), Stdout: &out, Stderr: &errb, Env: func(string) string { return "" }, Now: time.Now} + err := run([]string{"org", "show", "--no-such-flag"}, stdio, func(string) string { return "" }) + if err == nil { + t.Fatalf("want non-nil error") + } + if cli.ExitCode(err) != 1 { + t.Fatalf("exit code = %d, want 1 (err=%v)", cli.ExitCode(err), err) + } + if !strings.Contains(err.Error(), "flag provided but not defined") { + t.Errorf("err missing stdlib message: %v", err) + } + if !strings.Contains(err.Error(), "show") { + t.Errorf("err missing leaf name: %v", err) + } +} + // TestRun_UnknownProfile drives the ErrUnknownProfile branch in run: a // --profile pointing at a slot that doesn't exist (and no env fallback) // must print the canonical error to stderr and exit 1 via ErrUsage. From 86cbd766207b9a5cb1108bdb501fac30dd56a0f1 Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Tue, 21 Apr 2026 10:02:30 -0500 Subject: [PATCH 2/6] fix(cli): parse global flags regardless of position Stdlib flag.FlagSet.Parse stops at the first positional, so ParseGlobal only saw flags placed before the verb. `ana --json org show` worked; `ana org show --json` fell through to the leaf FlagSet, which rejected the unknown flag and (since 1a) now surfaces the error properly but is still the wrong outcome. Add StripGlobals: a single linear walk over argv that consumes every known global wherever it appears (before, after, or interleaved with the verb path and its positionals), with `--` as a hard terminator. Everything unknown passes through in original order so the leaf's FlagSet still reports precise unknown-flag errors. Dispatch uses StripGlobals instead of ParseGlobal; cmd/ana/main's config-resolution pre-pass uses it too so a trailing --profile/--endpoint /--token-file still influences config load. ParseGlobal stays because it's still the reference FlagSet shape for the registry-sync test and a few existing callers. TestGlobalFlagsRegistrySync prevents drift: any new global added to ParseGlobal must also land in globalFlagRegistry. --- cmd/ana/main.go | 7 +- internal/cli/CLAUDE.md | 2 +- internal/cli/cli_test.go | 203 +++++++++++++++++++++++++++++++++++++++ internal/cli/dispatch.go | 5 +- internal/cli/root.go | 161 +++++++++++++++++++++++++++++++ 5 files changed, 374 insertions(+), 4 deletions(-) diff --git a/cmd/ana/main.go b/cmd/ana/main.go index 72a21e7..bcf88f9 100644 --- a/cmd/ana/main.go +++ b/cmd/ana/main.go @@ -57,8 +57,11 @@ func run(args []string, stdio cli.IO, env func(string) string) error { // Parse global flags up front so the token-file/endpoint/profile // overrides are available before we touch config on disk. cli.Dispatch // re-parses globals, but doing it here lets us wire deps correctly - // before dispatch. - global, _, err := cli.ParseGlobal(args) + // before dispatch. StripGlobals (rather than ParseGlobal) so operators + // can place `--profile`/`--endpoint`/`--token-file` after the verb and + // still have config resolution honour them — ParseGlobal stops at the + // first positional, which would leave those flags invisible here. + global, _, err := cli.StripGlobals(args) if err != nil { // Don't return here — let Dispatch produce the canonical usage error // (it prints root help + err to stderr). Fall through with zero diff --git a/internal/cli/CLAUDE.md b/internal/cli/CLAUDE.md index 5a0b032..2c4c7e1 100644 --- a/internal/cli/CLAUDE.md +++ b/internal/cli/CLAUDE.md @@ -6,7 +6,7 @@ Argument-dispatch core shared by every verb. Defines the `Command` interface, th - `cli.go` — `Command`, `IO`, `DefaultIO`, `Group` (nested-verb dispatcher with auto-generated help listing; optional `Flags` closure declares group-level flags that descend to every leaf via `WithAncestorFlags`), `dispatchChild` (the Group/Dispatch handoff that scans a resolved leaf's args for `--help`/`-h` and renders its `Help()` before `Run` is called — and if the leaf implements `Flagger`, appends a `Flags:` block enumerating own + ancestor flags via `renderFlagsAsText`; Groups are skipped so the flag reaches the deepest leaf, and bare positional `help` is left alone so leaves can receive it as an argument), `Flagger` (opt-in interface: leaves that implement `Flags(fs)` get the ancestor-aware `Flags:` block in `--help`), and `renderFlagsAsText` (sorted `--name usage (default: X)` enumeration). Precedence when ancestor and leaf declare the same name: **leaf wins**, because leaves call `ApplyAncestorFlags` AFTER declaring their own flags, and ancestor registrars use `DeclareString` / `DeclareBool` Lookup-guards that skip already-declared names (stdlib `flag.FlagSet.StringVar` panics on duplicate names, verified empirically). - `dispatch.go` — `Dispatch` (root entry: short-circuits help, parses globals, routes to the matching verb via `dispatchChild`) and `RootHelp`. -- `root.go` — `Global` shape, `WithGlobal`/`GlobalFrom` context helpers (both require a non-nil ctx per stdlib `context.WithValue` convention — nil panics), `ParseGlobal` (strips known root flags from argv before verb dispatch), and the Phase 2 flag-registrar stack: `WithAncestorFlags(ctx, reg)` / `ApplyAncestorFlags(ctx, fs)` (context-carried slice of `func(*flag.FlagSet)` closures that `Group.Run` appends to and leaves replay on their own `FlagSet`), plus `DeclareString` / `DeclareBool` (Lookup-guarded wrappers ancestor closures use instead of raw `StringVar` / `BoolVar` to avoid the stdlib redeclaration panic when a leaf already declared the same name). +- `root.go` — `Global` shape, `WithGlobal`/`GlobalFrom` context helpers (both require a non-nil ctx per stdlib `context.WithValue` convention — nil panics), `ParseGlobal` (stdlib-style front-anchored parse: stops at the first positional), and `StripGlobals` (position-tolerant: walks argv once, consumes known global flags wherever they appear, passes everything else through in order so the leaf's FlagSet reports unknown-flag errors). The two share the authoritative `globalFlagRegistry` list — `TestGlobalFlagsRegistrySync` enforces that the registry matches `ParseGlobal`'s FlagSet shape. `Dispatch` uses `StripGlobals`; `cmd/ana/main.go`'s early config-resolution pre-pass uses it too so `ana org show --profile prod` honours `--profile` even when it's placed after the verb. `globalFlagsHelp` renders the canonical `Global Flags:` block that both `RootHelp` and the leaf `--help` path append so `--json`/`--endpoint`/`--token-file`/`--profile` are discoverable from every help surface. Phase 2 flag-registrar stack: `WithAncestorFlags(ctx, reg)` / `ApplyAncestorFlags(ctx, fs)` (context-carried slice of `func(*flag.FlagSet)` closures that `Group.Run` appends to and leaves replay on their own `FlagSet`), plus `DeclareString` / `DeclareBool` (Lookup-guarded wrappers ancestor closures use instead of raw `StringVar` / `BoolVar` to avoid the stdlib redeclaration panic when a leaf already declared the same name). - `flags.go` — `ParseFlags`, which tolerates positional args interleaved with flags (stdlib `FlagSet.Parse` stops at the first non-flag, silently dropping later flags); `FlagWasSet`, the `fs.Visit` wrapper partial-update verbs use to tell "user left this alone" from "user explicitly passed the zero value"; `RequireFlags`, which emits a single sorted `missing required flags: --a, --b` usage error for any name not explicitly set on fs; and three typed `flag.Value` constructors — `EnumFlag` (allow-list validation at parse time), `IntListFlag` (CSV → `[]int` with whitespace tolerance), `SinceFlag` (accepts non-negative `time.ParseDuration` or RFC3339, stored UTC via an injected clock). The stdlib `flag.Parse` re-wraps `Set` errors with `%v`, so the `ErrUsage` chain survives only through the outer `ParseFlags` wrap — tests that exercise these helpers must go through `ParseFlags`, not bare `fs.Parse`. - `token.go` — `Token` (named `string` type whose `String`/`Format` always render the `RedactToken` mask, so any accidental `%s`/`%v`/`%+v`/`%#v`/`%q` on a logger or error can't leak a bearer token) and its `Value()` escape hatch. `config.Profile.Token` and `auth.Config.Token` are declared as `cli.Token`; the only authorized raw-emit call sites are `cmd/ana/main.go`'s `tokenFn` (Authorization header) and `auth/keys.go:emitPlaintextToken` (one-shot API key print). Since the underlying kind is `string`, `encoding/json` marshals/unmarshals transparently and untyped string literals still work in tests (`cli.Token("t")` — or just `"t"` in a `Token`-typed field). - `helpers.go` — shared verb-package helpers extracted from per-verb duplicates: `NewFlagSet`, `UsageErrf`, `WriteJSON`, `Remarshal`, `RenderOutput` (generic JSON-vs-typed-render dispatch: `--json` → `WriteJSON`; else `Remarshal` into `*T` then call the render closure), `RequireStringID`, `RequireIntID`, `RenderTwoCol`, `ReadToken` (shared login/profile-add stdin reader with JWT-sized buffer boost; trims surrounding whitespace), `ReadPassword` (same JWT-sized buffer but strips ONLY the trailing line terminator — passwords can legitimately begin or end with whitespace, so surrounding bytes must not be mutated), `NewTableWriter` (canonical tabwriter config), `FirstLine`, `DashIfEmpty` (table-cell rendering primitives; fall-through chains use stdlib `cmp.Or`), `RedactToken` (masks bearer tokens for human-readable echoes — `(unset)` or `********** (last 4: xxxx)`). Phase 0 of the shared-cli-kit refactor; Phases 1–10 migrate each verb package over and delete its local copies. diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 3b63fa2..601ab71 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -261,6 +261,209 @@ func TestParseGlobalUnknownFlag(t *testing.T) { } } +func TestStripGlobalsBeforeVerb(t *testing.T) { + t.Parallel() + g, rest, err := StripGlobals([]string{"--json", "org", "show"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if !g.JSON { + t.Errorf("JSON not set: %+v", g) + } + if len(rest) != 2 || rest[0] != "org" || rest[1] != "show" { + t.Errorf("rest=%v", rest) + } +} + +func TestStripGlobalsAfterVerb(t *testing.T) { + t.Parallel() + g, rest, err := StripGlobals([]string{"org", "show", "--json"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if !g.JSON { + t.Errorf("JSON not set: %+v", g) + } + if len(rest) != 2 || rest[0] != "org" || rest[1] != "show" { + t.Errorf("rest=%v", rest) + } +} + +func TestStripGlobalsInterleaved(t *testing.T) { + t.Parallel() + // Global interleaved with a leaf positional; the positional must reach + // the leaf unchanged. + g, rest, err := StripGlobals([]string{"chat", "send", "--json", "id-123", "hello"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if !g.JSON { + t.Errorf("JSON not set: %+v", g) + } + want := []string{"chat", "send", "id-123", "hello"} + if len(rest) != len(want) { + t.Fatalf("rest=%v want %v", rest, want) + } + for i := range want { + if rest[i] != want[i] { + t.Errorf("rest[%d]=%q want %q", i, rest[i], want[i]) + } + } +} + +func TestStripGlobalsEqualsForm(t *testing.T) { + t.Parallel() + g, rest, err := StripGlobals([]string{"org", "show", "--endpoint=https://x", "--json"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if g.Endpoint != "https://x" { + t.Errorf("Endpoint=%q", g.Endpoint) + } + if !g.JSON { + t.Errorf("JSON not set") + } + if len(rest) != 2 || rest[0] != "org" || rest[1] != "show" { + t.Errorf("rest=%v", rest) + } +} + +func TestStripGlobalsSpaceForm(t *testing.T) { + t.Parallel() + g, rest, err := StripGlobals([]string{"--endpoint", "https://x", "org", "show"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if g.Endpoint != "https://x" { + t.Errorf("Endpoint=%q", g.Endpoint) + } + if len(rest) != 2 || rest[0] != "org" || rest[1] != "show" { + t.Errorf("rest=%v", rest) + } +} + +func TestStripGlobalsDoubleDashTerminator(t *testing.T) { + t.Parallel() + // After `--`, `--json` must be passed through as a positional so the leaf + // receives it and reports an unknown-flag error. StripGlobals must not + // consume it as the global. + g, rest, err := StripGlobals([]string{"org", "show", "--", "--json"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if g.JSON { + t.Errorf("JSON should NOT be set after --") + } + want := []string{"org", "show", "--", "--json"} + if len(rest) != len(want) { + t.Fatalf("rest=%v want %v", rest, want) + } + for i := range want { + if rest[i] != want[i] { + t.Errorf("rest[%d]=%q want %q", i, rest[i], want[i]) + } + } +} + +func TestStripGlobalsDuplicateLastWins(t *testing.T) { + t.Parallel() + g, _, err := StripGlobals([]string{"--endpoint", "https://a", "org", "--endpoint", "https://b", "show"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if g.Endpoint != "https://b" { + t.Errorf("Endpoint=%q want https://b", g.Endpoint) + } +} + +func TestStripGlobalsUnknownFlagPassesThrough(t *testing.T) { + t.Parallel() + // Unknown flags stay in rest unchanged so the leaf's FlagSet emits the + // canonical `flag provided but not defined: --xyz` error. + g, rest, err := StripGlobals([]string{"org", "show", "--xyz"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if g.JSON || g.Endpoint != "" { + t.Errorf("nothing should be consumed: %+v", g) + } + want := []string{"org", "show", "--xyz"} + if len(rest) != len(want) { + t.Fatalf("rest=%v want %v", rest, want) + } +} + +func TestStripGlobalsMissingValue(t *testing.T) { + t.Parallel() + _, _, err := StripGlobals([]string{"org", "show", "--endpoint"}) + if err == nil { + t.Fatalf("want error for trailing value-less flag") + } + if !strings.Contains(err.Error(), "flag needs an argument") { + t.Errorf("err=%v", err) + } +} + +func TestStripGlobalsAllFourFlags(t *testing.T) { + t.Parallel() + args := []string{ + "connector", "list", + "--json", + "--endpoint=https://api", + "--token-file", "/tmp/t", + "--profile=prod", + } + g, rest, err := StripGlobals(args) + if err != nil { + t.Fatalf("err=%v", err) + } + want := Global{JSON: true, Endpoint: "https://api", TokenFile: "/tmp/t", Profile: "prod"} + if g != want { + t.Errorf("global=%+v want %+v", g, want) + } + if len(rest) != 2 || rest[0] != "connector" || rest[1] != "list" { + t.Errorf("rest=%v", rest) + } +} + +// TestGlobalFlagsRegistrySync catches drift between ParseGlobal's FlagSet and +// globalFlagRegistry. Any new global flag must appear in both; if the registry +// lags, StripGlobals would silently pass the flag through to the leaf (which +// would fail with `flag provided but not defined`). +func TestGlobalFlagsRegistrySync(t *testing.T) { + t.Parallel() + fs := flag.NewFlagSet("sync", flag.ContinueOnError) + fs.SetOutput(io.Discard) + var g Global + fs.BoolVar(&g.JSON, "json", false, "") + fs.StringVar(&g.Endpoint, "endpoint", "", "") + fs.StringVar(&g.TokenFile, "token-file", "", "") + fs.StringVar(&g.Profile, "profile", "", "") + // Reflect ParseGlobal: every flag it declares must have an entry in the + // registry with the correct takesValue classification. + fs.VisitAll(func(f *flag.Flag) { + spec, ok := lookupGlobal(f.Name) + if !ok { + t.Errorf("global flag %q missing from globalFlagRegistry", f.Name) + return + } + // Stdlib flag.Flag.Value is a flag.Value; bool-typed values satisfy + // flag.boolFlag (unexported) with IsBoolFlag() bool. Use the adapter + // getter below to detect bool-ness without reaching into stdlib. + boolish, isBool := f.Value.(interface{ IsBoolFlag() bool }) + takesValue := !(isBool && boolish.IsBoolFlag()) + if spec.takesValue != takesValue { + t.Errorf("flag %q takesValue=%v want %v", f.Name, spec.takesValue, takesValue) + } + }) + // And no extras in the registry that ParseGlobal doesn't declare. + for _, spec := range globalFlagRegistry { + if fs.Lookup(spec.name) == nil { + t.Errorf("registry has %q but ParseGlobal does not declare it", spec.name) + } + } +} + func TestDispatchHappyPath(t *testing.T) { t.Parallel() child := &fakeCmd{help: "child"} diff --git a/internal/cli/dispatch.go b/internal/cli/dispatch.go index 4584a2b..376ff30 100644 --- a/internal/cli/dispatch.go +++ b/internal/cli/dispatch.go @@ -17,7 +17,10 @@ func Dispatch(ctx context.Context, verbs map[string]Command, args []string, stdi RootHelp(stdio.Stdout, verbs) return ErrHelp } - global, rest, err := ParseGlobal(args) + // StripGlobals instead of ParseGlobal: globals may appear before, after, + // or interleaved with the verb path. Anything unrecognised is passed + // through to rest so the leaf's own FlagSet reports the usage error. + global, rest, err := StripGlobals(args) if err != nil { fmt.Fprintln(stdio.Stderr, err) RootHelp(stdio.Stderr, verbs) diff --git a/internal/cli/root.go b/internal/cli/root.go index d23e2e5..f12bd10 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -5,6 +5,8 @@ import ( "flag" "fmt" "io" + "slices" + "strings" ) // flagRegistrar declares flags on a target FlagSet. Used for Group.Flags @@ -79,6 +81,56 @@ type Flagger interface { Flags(fs *flag.FlagSet) } +// globalFlagSpec records a root-level flag name and whether it consumes the +// next token as its value. Keep in sync with ParseGlobal's FlagSet — the +// TestGlobalFlagsRegistrySync regression test asserts they do not drift. +type globalFlagSpec struct { + name string + takesValue bool +} + +// globalFlagRegistry is the canonical list of root-level flags StripGlobals +// knows about. ParseGlobal is the source of truth for behavior; this slice +// mirrors that shape so StripGlobals can consume globals at any position in +// argv instead of only at the front (stdlib flag.FlagSet.Parse stops at the +// first positional). +var globalFlagRegistry = []globalFlagSpec{ + {name: "json", takesValue: false}, + {name: "endpoint", takesValue: true}, + {name: "token-file", takesValue: true}, + {name: "profile", takesValue: true}, +} + +// globalFlagsHelp renders the root-level flags as a `Global Flags:` block for +// both RootHelp and the leaf --help path. One source of truth so every +// surface lists the same flags with the same usage strings. +func globalFlagsHelp() string { + var b strings.Builder + fs := flag.NewFlagSet("global", flag.ContinueOnError) + fs.SetOutput(io.Discard) + var g Global + fs.BoolVar(&g.JSON, "json", false, "emit JSON output") + fs.StringVar(&g.Endpoint, "endpoint", "", "override API endpoint URL") + fs.StringVar(&g.TokenFile, "token-file", "", "path to bearer-token file") + fs.StringVar(&g.Profile, "profile", "", "config profile to use") + b.WriteString("Global Flags:\n") + // Gather, sort, then measure for a stable two-column layout. + names := make([]string, 0, 4) + fs.VisitAll(func(f *flag.Flag) { names = append(names, f.Name) }) + slices.Sort(names) + width := 0 + for _, n := range names { + if len(n) > width { + width = len(n) + } + } + for _, n := range names { + f := fs.Lookup(n) + fmt.Fprintf(&b, " --%-*s %s\n", width, f.Name, f.Usage) + } + return strings.TrimRight(b.String(), "\n") +} + // Global holds the root-level flags that apply to every verb. Command // implementations read it from context via GlobalFrom; ParseGlobal produces // it from raw argv. @@ -124,3 +176,112 @@ func ParseGlobal(args []string) (Global, []string, error) { } return g, fs.Args(), nil } + +// StripGlobals walks args once and splits it into (Global, rest, err). Unlike +// ParseGlobal, which relies on stdlib flag.FlagSet.Parse and stops at the +// first positional, StripGlobals consumes known global flags wherever they +// appear — before, after, or interleaved with the verb path and leaf flags. +// Everything it does not recognise is passed through to rest in original +// order so the leaf's FlagSet still handles unknown-flag errors. +// +// Supported forms per known flag in globalFlagRegistry: +// +// - `--name` (bool) or `--name=value` / `--name value` (takesValue) +// +// A bare `--` terminator stops global consumption: every remaining token is +// copied verbatim to rest (including the `--` itself), so leaves can still +// use `--` to force positional interpretation of an arg that looks like a +// flag. +// +// Duplicate globals follow stdlib semantics (last wins). Unknown flags are +// left in rest unchanged so the leaf's own FlagSet reports a precise +// `flag provided but not defined: --xyz` at its verb name. +func StripGlobals(args []string) (Global, []string, error) { + var g Global + fs := flag.NewFlagSet("ana", flag.ContinueOnError) + fs.SetOutput(io.Discard) + fs.BoolVar(&g.JSON, "json", false, "emit JSON output") + fs.StringVar(&g.Endpoint, "endpoint", "", "override API endpoint URL") + fs.StringVar(&g.TokenFile, "token-file", "", "path to bearer-token file") + fs.StringVar(&g.Profile, "profile", "", "config profile to use") + + rest := make([]string, 0, len(args)) + i := 0 + for i < len(args) { + tok := args[i] + if tok == "--" { + // Terminator: preserve it and pass every remaining token through. + rest = append(rest, args[i:]...) + break + } + name, value, hasEquals, isLong := parseFlagToken(tok) + if !isLong { + rest = append(rest, tok) + i++ + continue + } + spec, known := lookupGlobal(name) + if !known { + rest = append(rest, tok) + i++ + continue + } + if spec.takesValue { + if hasEquals { + if err := fs.Set(name, value); err != nil { + return Global{}, nil, fmt.Errorf("parse global flags: invalid value %q for -%s: %w", value, name, err) + } + i++ + continue + } + // Consumes next token as value. Missing next token is a usage error + // — mirrors stdlib behavior for `flag needs an argument`. + if i+1 >= len(args) { + return Global{}, nil, fmt.Errorf("parse global flags: flag needs an argument: -%s", name) + } + if err := fs.Set(name, args[i+1]); err != nil { + return Global{}, nil, fmt.Errorf("parse global flags: invalid value %q for -%s: %w", args[i+1], name, err) + } + i += 2 + continue + } + // Bool-valued global: `--name` sets true; `--name=false` respects the + // literal value. A bare `--name` with no `=` is the common path and + // always sets true. + raw := "true" + if hasEquals { + raw = value + } + if err := fs.Set(name, raw); err != nil { + return Global{}, nil, fmt.Errorf("parse global flags: invalid value %q for -%s: %w", raw, name, err) + } + i++ + } + return g, rest, nil +} + +// parseFlagToken classifies a token as a long-form flag (`--name` or +// `--name=value`) and returns its components. isLong is false for anything +// that doesn't start with `--` or for the bare `--` terminator — both are +// passed through to rest untouched. +func parseFlagToken(tok string) (name, value string, hasEquals, isLong bool) { + if len(tok) < 3 || !strings.HasPrefix(tok, "--") { + return "", "", false, false + } + body := tok[2:] + if eq := strings.IndexByte(body, '='); eq >= 0 { + return body[:eq], body[eq+1:], true, true + } + return body, "", false, true +} + +// lookupGlobal reports whether name is one of the recognised global flags +// and, if so, whether it consumes a value token. +func lookupGlobal(name string) (globalFlagSpec, bool) { + for _, spec := range globalFlagRegistry { + if spec.name == name { + return spec, true + } + } + return globalFlagSpec{}, false +} From 96094c00c0da32740ad6327fb07c8b6eafb54129 Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Tue, 21 Apr 2026 10:04:15 -0500 Subject: [PATCH 3/6] feat(cli): list global flags in root and leaf help MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Globals (--json/--endpoint/--token-file/--profile) were invisible in help output — only discoverable by reading the source. Add a canonical Global Flags section rendered by globalFlagsHelp (one source of truth for naming/usage) and append it to: - RootHelp (ana --help / ana with no args) - Leaf --help (via dispatchChild's --help short-circuit) Group.Help is left alone: children are already listed, and adding a global block at every Group level would duplicate noise without discovery value. The Global Flags block renders once at the root and once on each leaf's help — the surfaces users actually land on. --- internal/cli/cli.go | 2 ++ internal/cli/cli_test.go | 33 +++++++++++++++++++++++++++++++++ internal/cli/dispatch.go | 4 +++- 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index ea35228..949d170 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -116,6 +116,8 @@ func dispatchChild(ctx context.Context, cmd Command, args []string, stdio IO) er fmt.Fprint(stdio.Stdout, block) } } + fmt.Fprintln(stdio.Stdout) + fmt.Fprintln(stdio.Stdout, globalFlagsHelp()) return ErrHelp } } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 601ab71..480de51 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -609,6 +609,39 @@ func TestRootHelpWritesSortedList(t *testing.T) { if strings.Contains(s, "more") { t.Errorf("only first line of child help should appear: %q", s) } + // Global flags block must appear with every known flag, each exactly once. + if !strings.Contains(s, "Global Flags:") { + t.Errorf("missing Global Flags section: %q", s) + } + for _, name := range []string{"--json", "--endpoint", "--token-file", "--profile"} { + if n := strings.Count(s, name); n != 1 { + t.Errorf("flag %q appeared %d times, want 1: %q", name, n, s) + } + } +} + +func TestDispatchLeafHelpShowsGlobalFlags(t *testing.T) { + t.Parallel() + child := &fakeCmd{help: "run leaf help"} + verbs := map[string]Command{"run": child} + stdio, out, _ := testIO() + err := Dispatch(context.Background(), verbs, []string{"run", "--help"}, stdio) + if !errors.Is(err, ErrHelp) { + t.Fatalf("err=%v want ErrHelp", err) + } + s := out.String() + if !strings.Contains(s, "run leaf help") { + t.Errorf("leaf help missing: %q", s) + } + if !strings.Contains(s, "Global Flags:") { + t.Errorf("leaf --help should append Global Flags block: %q", s) + } + // No double-emit: each flag appears exactly once. + for _, name := range []string{"--json", "--endpoint", "--token-file", "--profile"} { + if n := strings.Count(s, name); n != 1 { + t.Errorf("flag %q appeared %d times, want 1: %q", name, n, s) + } + } } func TestWithGlobalAndFrom(t *testing.T) { diff --git a/internal/cli/dispatch.go b/internal/cli/dispatch.go index 376ff30..48cf51f 100644 --- a/internal/cli/dispatch.go +++ b/internal/cli/dispatch.go @@ -48,7 +48,7 @@ func Dispatch(ctx context.Context, verbs map[string]Command, args []string, stdi } // RootHelp writes a sorted listing of the top-level verbs to w, each followed -// by the first line of its own Help(). +// by the first line of its own Help(), then the canonical Global Flags block. func RootHelp(w io.Writer, verbs map[string]Command) { names := make([]string, 0, len(verbs)) for name := range verbs { @@ -67,4 +67,6 @@ func RootHelp(w io.Writer, verbs map[string]Command) { for _, n := range names { fmt.Fprintf(w, " %-*s %s\n", width, n, FirstLine(verbs[n].Help())) } + fmt.Fprintln(w) + fmt.Fprintln(w, globalFlagsHelp()) } From 4ac73ce425726b05abe4df7153e87a786aa93ac8 Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Tue, 21 Apr 2026 10:58:30 -0500 Subject: [PATCH 4/6] fix(cli): gofmt + drop unreachable StripGlobals error branches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gofmt complained about a stray blank line between globalFlagsHelp and the Global type doc. Same pass drops the two string-flag fs.Set error paths in StripGlobals — stdlib stringValue.Set cannot fail, so those branches were dead code that kept internal/cli at 97.9% and broke the 100% gate. Keep the bool-flag fs.Set error path (reachable via --json=notbool) and add tests for it plus the Dispatch StripGlobals-error surface path. --- internal/cli/cli_test.go | 42 ++++++++++++++++++++++++++++++++++++++++ internal/cli/root.go | 9 +++------ 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 480de51..aad7766 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -404,6 +404,32 @@ func TestStripGlobalsMissingValue(t *testing.T) { } } +// Bool-valued globals accept an explicit `--name=value`. A non-bool-parseable +// value should surface as a usage error rather than being silently coerced. +func TestStripGlobalsBoolEqualsInvalid(t *testing.T) { + t.Parallel() + _, _, err := StripGlobals([]string{"--json=notbool", "org", "show"}) + if err == nil { + t.Fatalf("want error for invalid bool value") + } + if !strings.Contains(err.Error(), "invalid value") { + t.Errorf("err=%v", err) + } +} + +// Bool-valued globals accept `--name=false` to explicitly disable (stdlib +// semantics). Confirms the hasEquals branch propagates the literal value. +func TestStripGlobalsBoolEqualsFalse(t *testing.T) { + t.Parallel() + g, _, err := StripGlobals([]string{"--json=false", "org", "show"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if g.JSON { + t.Errorf("JSON=true, want false for --json=false") + } +} + func TestStripGlobalsAllFourFlags(t *testing.T) { t.Parallel() args := []string{ @@ -560,6 +586,22 @@ func TestDispatchBadGlobalFlag(t *testing.T) { } } +// StripGlobals-level failures (missing value, invalid bool) must surface to +// stderr and map to ErrUsage — distinct from leaf-level flag errors which +// TestDispatchBadGlobalFlag covers via the pass-through path. +func TestDispatchStripGlobalsError(t *testing.T) { + t.Parallel() + verbs := map[string]Command{"x": &fakeCmd{help: "x"}} + stdio, _, errb := testIO() + err := Dispatch(context.Background(), verbs, []string{"--endpoint"}, stdio) + if !errors.Is(err, ErrUsage) { + t.Fatalf("err=%v want ErrUsage", err) + } + if !strings.Contains(errb.String(), "flag needs an argument") { + t.Errorf("stderr missing global-parse msg: %q", errb.String()) + } +} + func TestDispatchPropagatesChildError(t *testing.T) { t.Parallel() want := errors.New("inner") diff --git a/internal/cli/root.go b/internal/cli/root.go index f12bd10..95aa4ef 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -228,9 +228,8 @@ func StripGlobals(args []string) (Global, []string, error) { } if spec.takesValue { if hasEquals { - if err := fs.Set(name, value); err != nil { - return Global{}, nil, fmt.Errorf("parse global flags: invalid value %q for -%s: %w", value, name, err) - } + // String-valued flags: stdlib stringValue.Set never errors. + _ = fs.Set(name, value) i++ continue } @@ -239,9 +238,7 @@ func StripGlobals(args []string) (Global, []string, error) { if i+1 >= len(args) { return Global{}, nil, fmt.Errorf("parse global flags: flag needs an argument: -%s", name) } - if err := fs.Set(name, args[i+1]); err != nil { - return Global{}, nil, fmt.Errorf("parse global flags: invalid value %q for -%s: %w", args[i+1], name, err) - } + _ = fs.Set(name, args[i+1]) i += 2 continue } From 1dd59f4d1a3b0bcd26dcdd6503e7cc7ef9f5d776 Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Tue, 21 Apr 2026 11:29:13 -0500 Subject: [PATCH 5/6] fix(cli): single-dash flags + ErrReported sentinel to prevent double-print MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit found two issues post-rebase: 1. Dispatch and run() both write diagnostics to stderr, and main now also prints on any non-ErrHelp error (introduced in 1a so leaf FlagSets whose output is io.Discard get their errors surfaced). Result: bad globals, unknown commands, and unknown profiles produce two stderr lines — the callee's clean message plus main's wrapped repeat. Fix: add cli.ErrReported sentinel. Dispatch + run wrap their errors with errors.Join(err, ErrReported) after emitting the diagnostic themselves. main's fallback stderr print skips any error carrying ErrReported. Leaf flag errors still flow through main's print path unchanged. 2. StripGlobals only recognized --name; stdlib flag.FlagSet.Parse treats -name and --name as equivalent (per its docs: "One or two dashes may be used; they are equivalent"). A user running `ana -json org show` would have -json silently passed through to the org-show FlagSet, which then fails with "flag provided but not defined". Fix: parseFlagToken now accepts both single- and double-dash forms. Bare -, --, ---, -=val noise tokens are left in rest untouched so the leaf receives them as-is. --- cmd/ana/main.go | 14 +++++++----- cmd/ana/main_test.go | 8 +++++++ internal/cli/CLAUDE.md | 2 +- internal/cli/cli_test.go | 47 +++++++++++++++++++++++++++++++++++++++- internal/cli/dispatch.go | 5 +++-- internal/cli/errors.go | 6 +++++ internal/cli/root.go | 28 +++++++++++++++++------- 7 files changed, 92 insertions(+), 18 deletions(-) diff --git a/cmd/ana/main.go b/cmd/ana/main.go index bcf88f9..f43a60b 100644 --- a/cmd/ana/main.go +++ b/cmd/ana/main.go @@ -32,11 +32,13 @@ import ( func main() { stdio := cli.DefaultIO() err := run(os.Args[1:], stdio, os.Getenv) - if err != nil && !errors.Is(err, cli.ErrHelp) { - // ErrHelp means help text was already written to stdout; skip. Every - // other error (including ErrUsage from leaves whose FlagSet output is - // io.Discard) still needs to surface — otherwise misplaced flags - // exit 1 with silent output. + if err != nil && !errors.Is(err, cli.ErrHelp) && !errors.Is(err, cli.ErrReported) { + // ErrHelp means help text was already written to stdout; skip. + // ErrReported means the callee already wrote its diagnostic to + // stderr (Dispatch for bad globals / unknown commands, run() for + // unknown profiles). Every other error — including ErrUsage from + // leaves whose FlagSet output is io.Discard — still needs to + // surface, otherwise misplaced flags exit 1 with silent output. fmt.Fprintln(stdio.Stderr, err) } os.Exit(cli.ExitCode(err)) @@ -99,7 +101,7 @@ func run(args []string, stdio cli.IO, env func(string) string) error { // by wrapping cli.ErrUsage. if errors.Is(rerr, config.ErrUnknownProfile) { fmt.Fprintf(stdio.Stderr, "ana: unknown profile %q\n", global.Profile) - return fmt.Errorf("%w: %s", cli.ErrUsage, rerr) + return errors.Join(fmt.Errorf("%w: %s", cli.ErrUsage, rerr), cli.ErrReported) } return rerr } diff --git a/cmd/ana/main_test.go b/cmd/ana/main_test.go index 252c0bc..49dd226 100644 --- a/cmd/ana/main_test.go +++ b/cmd/ana/main_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "os" @@ -564,4 +565,11 @@ func TestRun_UnknownProfile(t *testing.T) { if !strings.Contains(errb.String(), `unknown profile "ghost"`) { t.Errorf("stderr missing message: %q", errb.String()) } + // run() writes the diagnostic itself and returns an ErrReported-wrapped + // err so main's fallback print is suppressed. Regression guard — if + // ErrReported leaks off the error, users would see the same message + // twice. Confirms the sentinel is actually in the chain. + if !errors.Is(err, cli.ErrReported) { + t.Errorf("err should carry cli.ErrReported to suppress main's duplicate print: %v", err) + } } diff --git a/internal/cli/CLAUDE.md b/internal/cli/CLAUDE.md index 2c4c7e1..ef1b1c9 100644 --- a/internal/cli/CLAUDE.md +++ b/internal/cli/CLAUDE.md @@ -10,5 +10,5 @@ Argument-dispatch core shared by every verb. Defines the `Command` interface, th - `flags.go` — `ParseFlags`, which tolerates positional args interleaved with flags (stdlib `FlagSet.Parse` stops at the first non-flag, silently dropping later flags); `FlagWasSet`, the `fs.Visit` wrapper partial-update verbs use to tell "user left this alone" from "user explicitly passed the zero value"; `RequireFlags`, which emits a single sorted `missing required flags: --a, --b` usage error for any name not explicitly set on fs; and three typed `flag.Value` constructors — `EnumFlag` (allow-list validation at parse time), `IntListFlag` (CSV → `[]int` with whitespace tolerance), `SinceFlag` (accepts non-negative `time.ParseDuration` or RFC3339, stored UTC via an injected clock). The stdlib `flag.Parse` re-wraps `Set` errors with `%v`, so the `ErrUsage` chain survives only through the outer `ParseFlags` wrap — tests that exercise these helpers must go through `ParseFlags`, not bare `fs.Parse`. - `token.go` — `Token` (named `string` type whose `String`/`Format` always render the `RedactToken` mask, so any accidental `%s`/`%v`/`%+v`/`%#v`/`%q` on a logger or error can't leak a bearer token) and its `Value()` escape hatch. `config.Profile.Token` and `auth.Config.Token` are declared as `cli.Token`; the only authorized raw-emit call sites are `cmd/ana/main.go`'s `tokenFn` (Authorization header) and `auth/keys.go:emitPlaintextToken` (one-shot API key print). Since the underlying kind is `string`, `encoding/json` marshals/unmarshals transparently and untyped string literals still work in tests (`cli.Token("t")` — or just `"t"` in a `Token`-typed field). - `helpers.go` — shared verb-package helpers extracted from per-verb duplicates: `NewFlagSet`, `UsageErrf`, `WriteJSON`, `Remarshal`, `RenderOutput` (generic JSON-vs-typed-render dispatch: `--json` → `WriteJSON`; else `Remarshal` into `*T` then call the render closure), `RequireStringID`, `RequireIntID`, `RenderTwoCol`, `ReadToken` (shared login/profile-add stdin reader with JWT-sized buffer boost; trims surrounding whitespace), `ReadPassword` (same JWT-sized buffer but strips ONLY the trailing line terminator — passwords can legitimately begin or end with whitespace, so surrounding bytes must not be mutated), `NewTableWriter` (canonical tabwriter config), `FirstLine`, `DashIfEmpty` (table-cell rendering primitives; fall-through chains use stdlib `cmp.Or`), `RedactToken` (masks bearer tokens for human-readable echoes — `(unset)` or `********** (last 4: xxxx)`). Phase 0 of the shared-cli-kit refactor; Phases 1–10 migrate each verb package over and delete its local copies. -- `errors.go` — `ErrUsage`, `ErrHelp`, and `ExitCode` (maps these plus `auth.ErrNotLoggedIn` to the process exit code). +- `errors.go` — `ErrUsage`, `ErrHelp`, `ErrReported` (sentinel marking errors whose diagnostic text was already written to stderr by the callee — `Dispatch` wraps with it for bad globals / unknown commands, `cmd/ana/main.go`'s `run` wraps with it for unknown-profile; `main()` skips its fallback stderr print when the error chain contains this sentinel, so a leaf that needs main to print simply returns a non-wrapped `ErrUsage` like the `io.Discard` FlagSet path does), and `ExitCode` (maps these plus `auth.ErrNotLoggedIn` to the process exit code). - `cli_test.go`, `flags_test.go`, `helpers_test.go` — cover dispatch, help rendering, flag parsing, exit-code mapping, and every branch of the shared verb helpers. `//lint:ignore SA1012` directives mark the intentional nil-context panic assertions for `WithGlobal`/`GlobalFrom`. diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index aad7766..b81839f 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -404,6 +404,44 @@ func TestStripGlobalsMissingValue(t *testing.T) { } } +// Stdlib `flag.FlagSet.Parse` treats `-name` and `--name` as equivalent. +// StripGlobals must do the same so `ana -json org show` works the same as +// `ana --json org show` — otherwise a user used to the single-dash form +// would have their flag silently passed through to the leaf's FlagSet. +func TestStripGlobalsSingleDashForm(t *testing.T) { + t.Parallel() + g, rest, err := StripGlobals([]string{"-json", "org", "-endpoint=https://x", "show"}) + if err != nil { + t.Fatalf("err=%v", err) + } + if !g.JSON || g.Endpoint != "https://x" { + t.Errorf("global=%+v", g) + } + if len(rest) != 2 || rest[0] != "org" || rest[1] != "show" { + t.Errorf("rest=%v", rest) + } +} + +// Pathological dash-only tokens (`-`, `---`, `-=val`) are not flags; they +// must be passed through to rest unchanged so the leaf can reject or accept +// them on its own terms. +func TestStripGlobalsDashNoise(t *testing.T) { + t.Parallel() + _, rest, err := StripGlobals([]string{"-", "org", "---", "-=val", "show"}) + if err != nil { + t.Fatalf("err=%v", err) + } + want := []string{"-", "org", "---", "-=val", "show"} + if len(rest) != len(want) { + t.Fatalf("rest=%v want %v", rest, want) + } + for i := range want { + if rest[i] != want[i] { + t.Errorf("rest[%d]=%q want %q", i, rest[i], want[i]) + } + } +} + // Bool-valued globals accept an explicit `--name=value`. A non-bool-parseable // value should surface as a usage error rather than being silently coerced. func TestStripGlobalsBoolEqualsInvalid(t *testing.T) { @@ -568,6 +606,9 @@ func TestDispatchUnknownVerb(t *testing.T) { if !errors.Is(err, ErrUsage) { t.Fatalf("err=%v", err) } + if !errors.Is(err, ErrReported) { + t.Errorf("err should carry ErrReported: %v", err) + } if !strings.Contains(errb.String(), "unknown command: zzz") { t.Errorf("stderr missing unknown msg: %q", errb.String()) } @@ -588,7 +629,8 @@ func TestDispatchBadGlobalFlag(t *testing.T) { // StripGlobals-level failures (missing value, invalid bool) must surface to // stderr and map to ErrUsage — distinct from leaf-level flag errors which -// TestDispatchBadGlobalFlag covers via the pass-through path. +// TestDispatchBadGlobalFlag covers via the pass-through path. The returned +// err must also carry ErrReported so main() knows not to double-print. func TestDispatchStripGlobalsError(t *testing.T) { t.Parallel() verbs := map[string]Command{"x": &fakeCmd{help: "x"}} @@ -597,6 +639,9 @@ func TestDispatchStripGlobalsError(t *testing.T) { if !errors.Is(err, ErrUsage) { t.Fatalf("err=%v want ErrUsage", err) } + if !errors.Is(err, ErrReported) { + t.Errorf("err should carry ErrReported: %v", err) + } if !strings.Contains(errb.String(), "flag needs an argument") { t.Errorf("stderr missing global-parse msg: %q", errb.String()) } diff --git a/internal/cli/dispatch.go b/internal/cli/dispatch.go index 48cf51f..ff023f9 100644 --- a/internal/cli/dispatch.go +++ b/internal/cli/dispatch.go @@ -2,6 +2,7 @@ package cli import ( "context" + "errors" "fmt" "io" "slices" @@ -24,7 +25,7 @@ func Dispatch(ctx context.Context, verbs map[string]Command, args []string, stdi if err != nil { fmt.Fprintln(stdio.Stderr, err) RootHelp(stdio.Stderr, verbs) - return fmt.Errorf("%w: %s", ErrUsage, err.Error()) + return errors.Join(fmt.Errorf("%w: %s", ErrUsage, err.Error()), ErrReported) } ctx = WithGlobal(ctx, global) @@ -42,7 +43,7 @@ func Dispatch(ctx context.Context, verbs map[string]Command, args []string, stdi if !ok { fmt.Fprintf(stdio.Stderr, "unknown command: %s\n", name) RootHelp(stdio.Stderr, verbs) - return fmt.Errorf("unknown command %q: %w", name, ErrUsage) + return errors.Join(fmt.Errorf("unknown command %q: %w", name, ErrUsage), ErrReported) } return dispatchChild(ctx, verb, rest[1:], stdio) } diff --git a/internal/cli/errors.go b/internal/cli/errors.go index f5a5327..6bc7f25 100644 --- a/internal/cli/errors.go +++ b/internal/cli/errors.go @@ -12,6 +12,12 @@ var ErrUsage = errors.New("usage") // user asked for help and got it — that is success, not a usage error. var ErrHelp = errors.New("help") +// ErrReported marks errors whose diagnostic text has already been written +// to stderr by the callee. main()'s fallback stderr print skips these to +// avoid double-reporting. Wrap with errors.Join(err, ErrReported) after +// emitting the diagnostic yourself. +var ErrReported = errors.New("reported") + // authError is an optional interface a transport/auth error can implement so // the root dispatcher can map it to exit code 3 without importing that package. type authError interface { diff --git a/internal/cli/root.go b/internal/cli/root.go index 95aa4ef..8461fff 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -184,9 +184,11 @@ func ParseGlobal(args []string) (Global, []string, error) { // Everything it does not recognise is passed through to rest in original // order so the leaf's FlagSet still handles unknown-flag errors. // -// Supported forms per known flag in globalFlagRegistry: +// Supported forms per known flag in globalFlagRegistry (single- and double- +// dash spellings are equivalent, matching stdlib `flag.FlagSet.Parse`): // -// - `--name` (bool) or `--name=value` / `--name value` (takesValue) +// - `-name` / `--name` (bool) or `-name=value` / `--name=value` / +// `-name value` / `--name value` (takesValue) // // A bare `--` terminator stops global consumption: every remaining token is // copied verbatim to rest (including the `--` itself), so leaves can still @@ -257,15 +259,25 @@ func StripGlobals(args []string) (Global, []string, error) { return g, rest, nil } -// parseFlagToken classifies a token as a long-form flag (`--name` or -// `--name=value`) and returns its components. isLong is false for anything -// that doesn't start with `--` or for the bare `--` terminator — both are -// passed through to rest untouched. +// parseFlagToken classifies a token as a long-form flag (`--name`, +// `-name`, `--name=value`, or `-name=value`) and returns its components. +// Both single- and double-dash spellings are accepted per stdlib +// `flag.FlagSet.Parse` docs ("One or two dashes may be used; they are +// equivalent"). isLong is false for non-flag tokens, the bare `--` +// terminator, or a bare `-` — all are passed through to rest untouched. func parseFlagToken(tok string) (name, value string, hasEquals, isLong bool) { - if len(tok) < 3 || !strings.HasPrefix(tok, "--") { + if len(tok) < 2 || tok[0] != '-' { + return "", "", false, false + } + body := tok[1:] + if body[0] == '-' { + // `--` terminator or `--name` — strip the second dash. A bare `--` + // has body=="-" here which we reject below (len check). + body = body[1:] + } + if body == "" || body == "-" { return "", "", false, false } - body := tok[2:] if eq := strings.IndexByte(body, '='); eq >= 0 { return body[:eq], body[eq+1:], true, true } From 9112c906a6933dc08e23e7cf9fd01bc380bef256 Mon Sep 17 00:00:00 2001 From: Brad Fair Date: Tue, 21 Apr 2026 11:38:06 -0500 Subject: [PATCH 6/6] fix(cli): preserve inner error chain with multi-%w wrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit nit + golangci-lint errorlint: formatting the inner error with %s in fmt.Errorf loses its chain, so downstream errors.Is against the inner sentinel fails. ErrUnknownProfile is checked upstream so behavior was fine today, but the pattern is brittle. Go 1.20+ fmt.Errorf accepts multiple %w verbs; switching "%w: %s" → "%w: %w" preserves both sentinels (ErrUsage for exit-code mapping plus the inner error for future errors.Is callers) while keeping the rendered string identical. --- cmd/ana/main.go | 2 +- internal/cli/dispatch.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/ana/main.go b/cmd/ana/main.go index f43a60b..b91a0de 100644 --- a/cmd/ana/main.go +++ b/cmd/ana/main.go @@ -101,7 +101,7 @@ func run(args []string, stdio cli.IO, env func(string) string) error { // by wrapping cli.ErrUsage. if errors.Is(rerr, config.ErrUnknownProfile) { fmt.Fprintf(stdio.Stderr, "ana: unknown profile %q\n", global.Profile) - return errors.Join(fmt.Errorf("%w: %s", cli.ErrUsage, rerr), cli.ErrReported) + return errors.Join(fmt.Errorf("%w: %w", cli.ErrUsage, rerr), cli.ErrReported) } return rerr } diff --git a/internal/cli/dispatch.go b/internal/cli/dispatch.go index ff023f9..3dcb2af 100644 --- a/internal/cli/dispatch.go +++ b/internal/cli/dispatch.go @@ -25,7 +25,7 @@ func Dispatch(ctx context.Context, verbs map[string]Command, args []string, stdi if err != nil { fmt.Fprintln(stdio.Stderr, err) RootHelp(stdio.Stderr, verbs) - return errors.Join(fmt.Errorf("%w: %s", ErrUsage, err.Error()), ErrReported) + return errors.Join(fmt.Errorf("%w: %w", ErrUsage, err), ErrReported) } ctx = WithGlobal(ctx, global)