[TE-6257] Isolate per-command cli.Flag parse state to fix order-dependent test failures

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)},
 					},
 				},
 			},

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)
+			}
+		}
+	}
+}