diff --git a/cmd/ana/main.go b/cmd/ana/main.go index a467555..b91a0de 100644 --- a/cmd/ana/main.go +++ b/cmd/ana/main.go @@ -32,10 +32,13 @@ 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) && !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)) @@ -56,8 +59,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 @@ -95,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: %w", cli.ErrUsage, rerr), cli.ErrReported) } return rerr } diff --git a/cmd/ana/main_test.go b/cmd/ana/main_test.go index 3049594..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" @@ -510,6 +511,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. @@ -539,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 5a0b032..ef1b1c9 100644 --- a/internal/cli/CLAUDE.md +++ b/internal/cli/CLAUDE.md @@ -6,9 +6,9 @@ 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. -- `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.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 3b63fa2..b81839f 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -261,6 +261,273 @@ 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) + } +} + +// 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) { + 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{ + "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"} @@ -339,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()) } @@ -357,6 +627,26 @@ 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. 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"}} + stdio, _, errb := testIO() + err := Dispatch(context.Background(), verbs, []string{"--endpoint"}, stdio) + 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()) + } +} + func TestDispatchPropagatesChildError(t *testing.T) { t.Parallel() want := errors.New("inner") @@ -406,6 +696,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 4584a2b..3dcb2af 100644 --- a/internal/cli/dispatch.go +++ b/internal/cli/dispatch.go @@ -2,6 +2,7 @@ package cli import ( "context" + "errors" "fmt" "io" "slices" @@ -17,11 +18,14 @@ 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) - return fmt.Errorf("%w: %s", ErrUsage, err.Error()) + return errors.Join(fmt.Errorf("%w: %w", ErrUsage, err), ErrReported) } ctx = WithGlobal(ctx, global) @@ -39,13 +43,13 @@ 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) } // 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 { @@ -64,4 +68,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()) } 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 d23e2e5..8461fff 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,121 @@ 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 (single- and double- +// dash spellings are equivalent, matching stdlib `flag.FlagSet.Parse`): +// +// - `-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 +// 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 { + // String-valued flags: stdlib stringValue.Set never errors. + _ = fs.Set(name, value) + 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) + } + _ = fs.Set(name, args[i+1]) + 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`, +// `-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) < 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 + } + 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 +}