diff --git a/cli.go b/cli.go index 7e3c50aa..2f3da915 100644 --- a/cli.go +++ b/cli.go @@ -613,6 +613,53 @@ 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. +// 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: + 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 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 + } +} + +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 +673,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 +692,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{ @@ -671,9 +718,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 63811481..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", @@ -306,3 +306,76 @@ 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") + } +} + +// 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) + } + } + } +}