From e34d113c8006694ed5c598b107d657acc72cc687 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Wed, 28 Oct 2026 20:29:16 +0100 Subject: [PATCH] fix(bonfire): read the =value forms of the global flags and use the parsed globals in the error block (IN-02, quick-261028-rqd) --- modules/bonfire/README.md | 4 +- modules/bonfire/execute.go | 20 ++++++++- modules/bonfire/globals.go | 76 ++++++++++++++++++++++++++++++--- modules/bonfire/globals_test.go | 45 +++++++++++++++++++ modules/bonfire/root.go | 3 ++ scripts/check-phase19.8.sh | 3 +- 6 files changed, 140 insertions(+), 11 deletions(-) diff --git a/modules/bonfire/README.md b/modules/bonfire/README.md index ef89b4ef..4f59628d 100644 --- a/modules/bonfire/README.md +++ b/modules/bonfire/README.md @@ -41,7 +41,7 @@ Use cases: - Command state for helpers: `bonfire.GlobalsOf`, `bonfire.VerbosityOf`, `bonfire.IsQuiet` and `bonfire.Interactive` read the running command's global options from its `bonfire.Output`. - Passthrough commands (`bonfire.Command.Passthrough`) skip flag parsing, so every argument, including `--help`, reaches `bonfire.Input.Args` unchanged; this suits commands that forward to another program. `bonfire.Command.Long` adds extended help text that the help command shows instead of the description. - Artisan-style command list: a bare binary (or `--help`, or `list`) prints a version header (`bonfire.Version.Header`, read from the build information by `bonfire.BuildVersion`), the usage line, the global options and the commands, top-level names first and then one block per namespace, with aligned descriptions. -- Global options on every command: `-v`/`-vv`/`-vvv` (`--verbose`), `-q` (`--quiet`, implies `-n`), `--ansi`/`--no-ansi` (last wins), `-n` (`--no-interaction`) and `--env`. `bonfire.PeekGlobals` reads them from the raw arguments before the root exists, so the generated main can pick the configuration environment and the log verbosity before plugins boot; `bonfire.OnGlobals` hooks receive the parsed `bonfire.Globals` before every command runs (roots built by `bonfire.Call` run no hook). +- Global options on every command: `-v`/`-vv`/`-vvv` (`--verbose`), `-q` (`--quiet`, implies `-n`), `--ansi`/`--no-ansi` (last wins), `-n` (`--no-interaction`) and `--env`. `bonfire.PeekGlobals` reads them from the raw arguments before the root exists, so the generated main can pick the configuration environment and the log verbosity before plugins boot; it reads the `=value` forms the way pflag does (`--verbose=2`, `-v=3`, `--quiet=false`, `--ansi=false`), and a value-taking command flag followed by a global-looking token (`--name -v`) is read as the global before boot, while the command and its error block use cobra's reading; `bonfire.OnGlobals` hooks receive the parsed `bonfire.Globals` before every command runs (roots built by `bonfire.Call` run no hook). - Artisan-style help: `help ` and ` --help` print Description, Usage, Arguments, Options with `[default: ...]` and "(multiple values allowed)", the global options, Help (`bonfire.Command.Long`) and Examples (`bonfire.Command.Examples`, printed after `bin/` or `summer`). - Hidden commands (`bonfire.Command.Hidden`) run but stay out of the list unless `list --all`. - JSON catalog: `list --format=json` prints every command, hidden ones flagged, with arguments, options, defaults and examples as a `bonfire.Listing` (`bonfire.Describe` builds it from a command slice). @@ -125,7 +125,7 @@ if catalog.Has("blog:import") { | `bonfire.WithVersion` | Sets the `bonfire.Version` shown in the list header; without it the root reads `bonfire.BuildVersion`. | | `bonfire.OnGlobals` | Registers a function that receives the parsed `bonfire.Globals` before every command of the root runs. | | `bonfire.Globals` | The global options: `Verbosity` (0 to 3), `Quiet`, `Color` (a `bonfire.ColorMode`), `NoInteraction` and `Env`. | -| `bonfire.PeekGlobals` | Reads `bonfire.Globals` from raw arguments without failing; stops at `--`. | +| `bonfire.PeekGlobals` | Reads `bonfire.Globals` from raw arguments without failing, `=value` forms included; stops at `--`. | | `bonfire.Globals.Args` | Turns `bonfire.Globals` back into flags (`-vv`, `-q`, `-n`, `--ansi` or `--no-ansi`, `--env=`) for forwarding. | | `bonfire.Version` | Application and framework versions of a binary (`App`, `Framework`, `Local`). | | `bonfire.BuildVersion` | Reads the `bonfire.Version` from the binary's build information. | diff --git a/modules/bonfire/execute.go b/modules/bonfire/execute.go index d8a5cbeb..fbec9a53 100644 --- a/modules/bonfire/execute.go +++ b/modules/bonfire/execute.go @@ -56,10 +56,16 @@ func Execute(ctx context.Context, root *cobra.Command, args []string) int { lipgloss.EnableLegacyWindowsANSI(f) } } - err := root.ExecuteContext(ctx) + parsed := &parsedGlobals{} + err := root.ExecuteContext(context.WithValue(ctx, parsedGlobalsKey{}, parsed)) if err == nil { return ExitOK } + if parsed.set { + // cobra parsed the command's flags: use its reading, which knows + // which tokens are the values of command flags. + g = parsed.g + } err = unknownCommand(root, args, err) name := root.Name() RenderError(root.ErrOrStderr(), name, wrongDirectory(name, err, args, ""), g) @@ -79,6 +85,18 @@ func Fail(w io.Writer, name string, err error, g Globals) int { return ExitFailure } +// parsedGlobalsKey is the context key of the parsedGlobals Execute +// installs. +type parsedGlobalsKey struct{} + +// parsedGlobals receives the globals the root's PersistentPreRunE applied, +// so Execute's error block can use cobra's reading of the command line +// instead of PeekGlobals. Call never installs one. +type parsedGlobals struct { + g Globals + set bool +} + func exitCode(err error) int { var carrier *exitCodeError switch { diff --git a/modules/bonfire/globals.go b/modules/bonfire/globals.go index 12bb627e..6c972f2d 100644 --- a/modules/bonfire/globals.go +++ b/modules/bonfire/globals.go @@ -48,11 +48,32 @@ var ( // exists, so the generated main can pick the configuration environment and // the log verbosity before config loads and plugins boot. The scan is // tolerant: it never fails, ignores every other token and stops at "--". -// Quiet also sets NoInteraction. +// It reads the =value forms the way pflag does (--verbose=2, -v=3, +// --quiet=false, -n=1, --ansi=false), ignoring a value pflag would reject. +// Quiet also sets NoInteraction. A value-taking command flag followed by a +// global-looking token (--name -v) is read as the global before boot; the +// command and its error block use cobra's reading. func PeekGlobals(args []string) Globals { var g Globals for i := 0; i < len(args); i++ { a := args[i] + if name, value, ok := strings.Cut(a, "="); ok && strings.HasPrefix(name, "--") { + switch strings.TrimPrefix(name, "--") { + case flagVerbose: + peekCount(&g.Verbosity, value) + case flagQuiet: + peekBool(&g.Quiet, value) + case flagNoInteraction: + peekBool(&g.NoInteraction, value) + case flagANSI: + peekColor(&g.Color, ColorAlways, value) + case flagNoANSI: + peekColor(&g.Color, ColorNever, value) + case flagEnv: + g.Env = value + } + continue + } switch { case a == "--": return g.normalize() @@ -71,8 +92,6 @@ func PeekGlobals(args []string) Globals { g.Env = args[i+1] i++ } - case strings.HasPrefix(a, "--"+flagEnv+"="): - g.Env = strings.TrimPrefix(a, "--"+flagEnv+"=") case isShorthandCluster(a): g.peekShorthands(a[1:]) } @@ -80,17 +99,60 @@ func PeekGlobals(args []string) Globals { return g.normalize() } +// peekCount applies a --verbose=N or -v=N value the way pflag's count flag +// does: the value replaces the count; an unparsable value is ignored. +func peekCount(n *int, value string) { + if v, err := strconv.ParseInt(value, 0, 0); err == nil { + *n = int(v) + } +} + +// peekBool applies a bool flag's =value the way pflag does; an unparsable +// value is ignored. +func peekBool(b *bool, value string) { + if v, err := strconv.ParseBool(value); err == nil { + *b = v + } +} + +// peekColor applies --ansi=B or --no-ansi=B the way colorFlag.Set does: +// true sets the flag's mode, false resets the mode to ColorAuto only when it +// is the flag's own; an unparsable value is ignored. +func peekColor(mode *ColorMode, set ColorMode, value string) { + on, err := strconv.ParseBool(value) + switch { + case err != nil: + case on: + *mode = set + case *mode == set: + *mode = ColorAuto + } +} + // isShorthandCluster reports a single-dash token such as -v, -vvv or -qn. func isShorthandCluster(a string) bool { return len(a) >= 2 && a[0] == '-' && a[1] != '-' } // peekShorthands reads a cluster of shorthands the way pflag does (-qn is -// -q -n, -vvq is -v -v -q). It stops at the first letter that is not a -// global shorthand: that may be a command flag whose value is the rest of -// the token. +// -q -n, -vvq is -v -v -q, -v=3 sets the count, -qn=false turns -n off). It +// stops at the first letter that is not a global shorthand: that may be a +// command flag whose value is the rest of the token. func (g *Globals) peekShorthands(cluster string) { - for _, r := range cluster { + for i := 0; i < len(cluster); i++ { + r := cluster[i] + if i+1 < len(cluster) && cluster[i+1] == '=' { + value := cluster[i+2:] + switch r { + case 'v': + peekCount(&g.Verbosity, value) + case 'q': + peekBool(&g.Quiet, value) + case 'n': + peekBool(&g.NoInteraction, value) + } + return + } switch r { case 'v': g.Verbosity++ diff --git a/modules/bonfire/globals_test.go b/modules/bonfire/globals_test.go index 07b0098b..521f2eb2 100644 --- a/modules/bonfire/globals_test.go +++ b/modules/bonfire/globals_test.go @@ -3,6 +3,8 @@ package bonfire import ( "bytes" "context" + "errors" + "fmt" "slices" "strings" "testing" @@ -41,6 +43,23 @@ var peekCases = []struct { {"combined -vq", []string{"-vq", "migrate"}, Globals{Verbosity: 1, Quiet: true, NoInteraction: true}, true}, {"combined -nvv", []string{"migrate", "-nvv"}, Globals{Verbosity: 2, NoInteraction: true}, true}, {"everything", []string{"-vv", "--env=dev", "migrate", "-n", "--no-ansi"}, Globals{Verbosity: 2, NoInteraction: true, Color: ColorNever, Env: "dev"}, true}, + // IN-02: the =value forms, read with pflag's value rules. + {"--verbose=2", []string{"migrate", "--verbose=2"}, Globals{Verbosity: 2}, true}, + {"-v=3", []string{"migrate", "-v=3"}, Globals{Verbosity: 3}, true}, + {"-vv=1", []string{"migrate", "-vv=1"}, Globals{Verbosity: 1}, true}, + {"-v --verbose=2 -v", []string{"migrate", "-v", "--verbose=2", "-v"}, Globals{Verbosity: 3}, true}, + {"--quiet=true", []string{"migrate", "--quiet=true"}, Globals{Quiet: true, NoInteraction: true}, true}, + {"-q --quiet=false", []string{"migrate", "-q", "--quiet=false"}, Globals{}, true}, + {"-q=false", []string{"migrate", "-q=false"}, Globals{}, true}, + {"--no-interaction=1", []string{"migrate", "--no-interaction=1"}, Globals{NoInteraction: true}, true}, + {"-n=t", []string{"migrate", "-n=t"}, Globals{NoInteraction: true}, true}, + {"-qn=false", []string{"migrate", "-qn=false"}, Globals{Quiet: true, NoInteraction: true}, true}, + {"--ansi=false", []string{"migrate", "--ansi=false"}, Globals{Color: ColorAuto}, true}, + {"--ansi --ansi=false", []string{"migrate", "--ansi", "--ansi=false"}, Globals{Color: ColorAuto}, true}, + {"--ansi=true", []string{"migrate", "--ansi=true"}, Globals{Color: ColorAlways}, true}, + {"--no-ansi=true", []string{"migrate", "--no-ansi=true"}, Globals{Color: ColorNever}, true}, + {"--no-ansi --ansi=false", []string{"migrate", "--no-ansi", "--ansi=false"}, Globals{Color: ColorNever}, true}, + {"unparsable values ignored", []string{"migrate", "-v", "--verbose=x", "--quiet=maybe", "--ansi=sure"}, Globals{Verbosity: 1}, false}, {"unknown flags ignored", []string{"--bogus", "-x", "migrate", "-", "--env"}, Globals{}, false}, {"--env without a value", []string{"migrate", "--env"}, Globals{}, false}, {"empty", nil, Globals{}, false}, @@ -121,3 +140,29 @@ func TestGlobalFlagsAgree(t *testing.T) { }) } } + +// TestErrorBlockUsesParsedGlobals (IN-02): once cobra has parsed the +// command's flags, the error block uses cobra's globals. In +// "migrate --name -v" the -v is the value of --name, so the block stays at +// verbosity 0 although PeekGlobals reads -v. +func TestErrorBlockUsesParsedGlobals(t *testing.T) { + plainEnv(t) + cmd := Command{ + Name: "migrate", + Description: "Run the migrations", + Flags: []Flag{{Name: "name", Description: "A name"}}, + Run: func(_ context.Context, in Input, _ Output) error { + if v, _ := in.Flag("name"); v != "-v" { + t.Errorf("--name = %q, want -v", v) + } + return fmt.Errorf("outer: %w", errors.New("inner")) + }, + } + _, errOut, code := runGolden(t, []Command{cmd}, "migrate", "--name", "-v") + if code != ExitFailure { + t.Fatalf("exit %d, want %d\n%s", code, ExitFailure, errOut) + } + if !strings.Contains(errOut, "Run with -v for details.") || strings.Contains(errOut, "Caused by:") { + t.Errorf("block does not use the parsed verbosity 0:\n%s", errOut) + } +} diff --git a/modules/bonfire/root.go b/modules/bonfire/root.go index 2e9b74de..3ff94141 100644 --- a/modules/bonfire/root.go +++ b/modules/bonfire/root.go @@ -152,6 +152,9 @@ func NewRootIO(name string, commands []Command, in io.Reader, out, errW io.Write g = flags.globals() } output.setGlobals(g) + if parsed, ok := cmd.Context().Value(parsedGlobalsKey{}).(*parsedGlobals); ok && parsed != nil { + parsed.g, parsed.set = g, true + } for _, hook := range cfg.hooks { hook(g) } diff --git a/scripts/check-phase19.8.sh b/scripts/check-phase19.8.sh index dad95f60..7e385bda 100755 --- a/scripts/check-phase19.8.sh +++ b/scripts/check-phase19.8.sh @@ -131,8 +131,9 @@ BONFIRE_TESTS=( TestProcessViewStripsControl TestTableStripsControl # quick 261022-4z8 (re-review fixes): IN-15, WR-09. TestNestedProcessBuildsNoProgram TestPrintAboveAfterCancel - # quick 261028-rqd (info findings): IN-03, IN-06. + # quick 261028-rqd (info findings): IN-03, IN-06, IN-02. TestPluginErrorWordedUnknownCommand TestExecuteKeepsRootHelp + TestErrorBlockUsesParsedGlobals ) DOCSITE_TESTS=( # Plan 04: the docs checker reads past the global options.