From e227fce88d2417cce318ee04f1a7397e3ce927eb Mon Sep 17 00:00:00 2001 From: Gordon Chan Date: Fri, 3 Jul 2026 11:37:56 +1200 Subject: [PATCH 1/5] [TE-6257] Isolate per-command cli.Flag parse state in run/plan flag helpers runCommandFlags() and planCommandFlags() returned shared package-global cli.Flag pointers, so urfave/cli's per-flag parse state (hasBeenSet, applied, count, value) leaked between commands built in the same process. Once a test parsed --selector-splitting on the CLI, hasBeenSet stayed set on the shared selectorSplittingFlag, suppressing the env var source on a later command and making TestRunCommandEnvVarsBindToConfig see cfg.SelectorSplitting=false. Add freshFlag/freshFlags to hand each command shallow copies with distinct backing structs, keeping the shared Destination pointers so parsed values still land in cfg. Add a regression test asserting parse state does not leak between commands. --- cli.go | 44 ++++++++++++++++++++++++++++++++++++++++++-- cli_test.go | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/cli.go b/cli.go index 7e3c50aa..97a31c48 100644 --- a/cli.go +++ b/cli.go @@ -613,6 +613,46 @@ func previewSelectionFlags() []cli.Flag { } } +// freshFlag returns a shallow copy of a flag with a distinct backing struct, so +// urfave/cli's per-flag parse state (hasBeenSet, applied, count, value) does not +// leak between commands built from the same package-global flag definitions. The +// copy keeps the shared Destination pointer, so parsed values still land in cfg. +func freshFlag(f cli.Flag) cli.Flag { + switch v := f.(type) { + case *cli.BoolFlag: + c := *v + return &c + case *cli.StringFlag: + c := *v + return &c + case *cli.IntFlag: + c := *v + return &c + case *cli.DurationFlag: + c := *v + return &c + case *cli.StringSliceFlag: + c := *v + return &c + case *cli.BoolWithInverseFlag: + c := *v + return &c + default: + // Unknown flag type: return as-is rather than silently dropping it. This + // preserves behaviour if a new flag type is introduced, at the cost of + // that flag not being isolated until added above. + return f + } +} + +func freshFlags(flags []cli.Flag) []cli.Flag { + out := make([]cli.Flag, len(flags)) + for i, f := range flags { + out[i] = freshFlag(f) + } + return out +} + func runCommandFlags() []cli.Flag { flags := []cli.Flag{ filesFlag, @@ -626,7 +666,7 @@ func runCommandFlags() []cli.Flag { flags = append(flags, failOnNoTestsFlag) flags = append(flags, promiseFailureFlag) flags = append(flags, previewSelectionFlags()...) - return flags + return freshFlags(flags) } func planCommandFlags() []cli.Flag { @@ -645,7 +685,7 @@ func planCommandFlags() []cli.Flag { flags = append(flags, runnerEnvironmentFlags...) flags = append(flags, parallelismFlag) flags = append(flags, previewSelectionFlags()...) - return flags + return freshFlags(flags) } var cliCommand = &cli.Command{ diff --git a/cli_test.go b/cli_test.go index 63811481..7dfbdfbb 100644 --- a/cli_test.go +++ b/cli_test.go @@ -306,3 +306,51 @@ func TestSelectorSplittingFlagBindsToPlanConfig(t *testing.T) { t.Fatalf("cfg.SelectorSplitting = false, want true") } } + +// TestRunCommandFlagsDoNotShareParseState guards against the order-dependent +// failure from TE-6257: runCommandFlags() must hand out fresh flag instances so +// that explicitly setting a flag on one command does not leave urfave/cli's +// hasBeenSet state on a shared flag object, which would make a later command +// ignore the flag's env var source. +func TestRunCommandFlagsDoNotShareParseState(t *testing.T) { + cfg = config.New() + t.Cleanup(func() { cfg = config.New() }) + + // First command parses --selector-splitting on the CLI. + first := &cli.Command{ + Name: "bktec", + Commands: []*cli.Command{ + { + Name: "run", + Action: func(ctx context.Context, cmd *cli.Command) error { return nil }, + Flags: runCommandFlags(), + }, + }, + } + if err := first.Run(context.Background(), []string{"bktec", "run", "--selector-splitting"}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Second command, built independently, relies on the env var source. If the + // two commands shared the same flag instance, the flag's hasBeenSet state + // from the first parse would suppress the env var here. + cfg = config.New() + t.Setenv("BUILDKITE_TEST_ENGINE_SELECTOR_SPLITTING", "true") + second := &cli.Command{ + Name: "bktec", + Commands: []*cli.Command{ + { + Name: "run", + Action: func(ctx context.Context, cmd *cli.Command) error { return nil }, + Flags: runCommandFlags(), + }, + }, + } + if err := second.Run(context.Background(), []string{"bktec", "run"}); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if !cfg.SelectorSplitting { + t.Fatalf("cfg.SelectorSplitting = false, want true; flag parse state leaked between commands") + } +} From 0cc19a7c9fe3ea9e418b653ea4a17fef5555bcd7 Mon Sep 17 00:00:00 2001 From: Gordon Chan Date: Fri, 3 Jul 2026 11:45:06 +1200 Subject: [PATCH 2/5] [TE-6257] Extend flag isolation to plan-output flags and guard against regressions Route the plan command's mutually-exclusive PLAN OUTPUT flags through freshFlag so the isolation guarantee covers the whole plan flag tree, not just the run/plan helper sets. Build the debug flag in TestRunCommandEnvVarsBindToConfig from a fresh copy for the same reason. Add TestCommandFlagsAllReturnFreshInstances, which fails if a new flag type is added without a matching freshFlag case and silently falls through to default. --- cli.go | 6 +++--- cli_test.go | 27 ++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/cli.go b/cli.go index 97a31c48..f6b85f2f 100644 --- a/cli.go +++ b/cli.go @@ -711,9 +711,9 @@ var cliCommand = &cli.Command{ Required: true, Category: "PLAN OUTPUT", Flags: [][]cli.Flag{ - {jsonFlag}, - {planOutFlag}, - {pipelineUploadFlag}, + {freshFlag(jsonFlag)}, + {freshFlag(planOutFlag)}, + {freshFlag(pipelineUploadFlag)}, }, }, }, diff --git a/cli_test.go b/cli_test.go index 7dfbdfbb..1bbd27c9 100644 --- a/cli_test.go +++ b/cli_test.go @@ -194,7 +194,7 @@ func TestRunCommandEnvVarsBindToConfig(t *testing.T) { cmd := &cli.Command{ Name: "bktec", - Flags: []cli.Flag{debugFlag}, + Flags: freshFlags([]cli.Flag{debugFlag}), Commands: []*cli.Command{ { Name: "run", @@ -354,3 +354,28 @@ func TestRunCommandFlagsDoNotShareParseState(t *testing.T) { t.Fatalf("cfg.SelectorSplitting = false, want true; flag parse state leaked between commands") } } + +// TestCommandFlagsAllReturnFreshInstances asserts every flag handed out by the +// run and plan helpers is a distinct instance, i.e. freshFlag matched a concrete +// case rather than falling through to its default (which returns the shared +// global unchanged). This catches a new flag type being added without a +// corresponding freshFlag case, which would silently reintroduce the TE-6257 +// parse-state leak for that flag. +func TestCommandFlagsAllReturnFreshInstances(t *testing.T) { + // Enable preview so the selection flags are included in the sweep. + t.Setenv(previewSelectionEnvVar, "true") + + for _, tc := range []struct { + name string + flags []cli.Flag + }{ + {"run", runCommandFlags()}, + {"plan", planCommandFlags()}, + } { + for _, f := range tc.flags { + if freshFlag(f) == f { + t.Errorf("%s: freshFlag(%v) returned the shared global (type %T); add a case to freshFlag so its parse state is isolated", tc.name, f.Names(), f) + } + } + } +} From ef397f6d19f4032ca0276ee0013227390e6cd910 Mon Sep 17 00:00:00 2001 From: Gordon Chan Date: Fri, 3 Jul 2026 12:34:11 +1200 Subject: [PATCH 3/5] [TE-6257] Clarify freshFlag default-case comment Reword to stop claiming the default branch preserves behaviour: the flag still works but is not isolated, and TestCommandFlagsAllReturnFreshInstances is what surfaces an unhandled type in CI. --- cli.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/cli.go b/cli.go index f6b85f2f..932d218f 100644 --- a/cli.go +++ b/cli.go @@ -638,9 +638,11 @@ func freshFlag(f cli.Flag) cli.Flag { c := *v return &c default: - // Unknown flag type: return as-is rather than silently dropping it. This - // preserves behaviour if a new flag type is introduced, at the cost of - // that flag not being isolated until added above. + // Unknown flag type: return the shared global as-is rather than dropping + // it, so the flag still registers and works. It is NOT isolated; its + // parse state can leak between commands until a case is added above. + // TestCommandFlagsAllReturnFreshInstances fails when an unhandled type + // reaches this branch, so the gap surfaces in CI rather than silently. return f } } From 650fb4227db2158cfe885e16c72098c92c319067 Mon Sep 17 00:00:00 2001 From: Gordon Chan Date: Wed, 8 Jul 2026 17:46:14 +1200 Subject: [PATCH 4/5] [TE-6257] Note urfave/cli has no public flag-state reset in freshFlag comment Record why freshFlag copies rather than resets: urfave/cli v3 keeps hasBeenSet/applied/count/value unexported with no reset method, and the library's own tests build a fresh command with fresh flag instances per case. --- cli.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cli.go b/cli.go index 932d218f..2bdceef6 100644 --- a/cli.go +++ b/cli.go @@ -617,6 +617,8 @@ func previewSelectionFlags() []cli.Flag { // urfave/cli's per-flag parse state (hasBeenSet, applied, count, value) does not // leak between commands built from the same package-global flag definitions. The // copy keeps the shared Destination pointer, so parsed values still land in cfg. +// urfave/cli v3 exposes no public reset for that state, so a fresh copy per +// command is the mechanism, matching the library's own per-command flag pattern. func freshFlag(f cli.Flag) cli.Flag { switch v := f.(type) { case *cli.BoolFlag: From 65ba5ecd35ecc06e7689d6ffb9b5306a8cb70a1f Mon Sep 17 00:00:00 2001 From: Gordon Chan Date: Thu, 9 Jul 2026 09:54:02 +1200 Subject: [PATCH 5/5] [TE-6257] Cite urfave/cli test precedent for flag reset in freshFlag comment Link the upstream command_test.go where urfave reassigns fresh flag literals before re-running a command, as precedent for resetting per-flag parse state between runs. --- cli.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cli.go b/cli.go index 2bdceef6..2f3da915 100644 --- a/cli.go +++ b/cli.go @@ -619,6 +619,9 @@ func previewSelectionFlags() []cli.Flag { // copy keeps the shared Destination pointer, so parsed values still land in cfg. // urfave/cli v3 exposes no public reset for that state, so a fresh copy per // command is the mechanism, matching the library's own per-command flag pattern. +// urfave/cli's own tests reset flags the same way, reassigning fresh flag +// literals before re-running a command "since they are set previously": +// https://github.com/urfave/cli/blob/4937c163f8e8bd88fdb06c4eab67cde61436c205/command_test.go#L3202 func freshFlag(f cli.Flag) cli.Flag { switch v := f.(type) { case *cli.BoolFlag: