Preview: test selection flags in plan/run by pda · Pull Request #449 · buildkite/test-engine-client

pda · 2026-02-24T06:33:25Z

New flags in the plan and run subcommands available when BKTEC_PREVIEW_SELECTION is enabled via environment:

--selection-strategy
--selection-param
--metadata

These are sent to the server, the effect of which is currently undefined behaviour. A subsequent release and product announcement will clarify.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cb692f9e96

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

internal/api/create_test_plan.go

New flags in the `plan` and `run` subcommands available when BKTEC_PREVIEW_SELECTION is enabled via environment: --selection-strategy --selection-param --metadata These are sent to the server, the effect of which is currently undefined behaviour.

gchan · 2026-02-25T03:34:54Z

README.md

+export BUILDKITE_TEST_ENGINE_SELECTION_PARAMS='{"percent":"40"}'
+export BUILDKITE_TEST_ENGINE_METADATA='{"commit_message":"fix flaky tests","git_diff":"..."}'


Do we want to hold back the documentation while we continue to iterate on this feature? The percent strategy hasn't yet been implemented.

I think there is enough words to indicate that the API is unstable, subject to change and no one should be using it in their critical paths.

Yeah I was unsure, so biased towards transparency.

gchan · 2026-02-25T03:47:22Z

cli.go

+var selectionParamFlag = &cli.StringSliceFlag{
+	Name:     "selection-param",
+	Category: "PREVIEW: TEST SELECTION",
+	Usage:    "Additional selection strategy parameter key=value sent to the test plan API. Repeat for multiple entries.",
+}
+
+var metadataFlag = &cli.StringSliceFlag{
+	Name:     "metadata",
+	Category: "PREVIEW: TEST SELECTION",
+	Usage:    "Additional metadata key=value sent to the test plan API. Repeat for multiple entries.",
+}


Sources is deliberately omitted because the env vars are represented as JSON objects not key=value pairs.

gchan · 2026-02-25T03:51:53Z

cli.go

+		accessTokenFlag,
+		suiteSlugFlag,
+	}
+	flags = append(flags, previewSelectionFlags()...)


Is there a reason the selection flags are inserted between suiteSlugFlag and baseURLFlag?

No good reason that I'm aware of. I'm going to move it to the end so that there's one slice literal and one append().

gchan · 2026-02-25T03:54:15Z

cli.go

+			Name:                      "run",
+			Usage:                     "Run tests",
+			Action:                    run,
+			DisableSliceFlagSeparator: true,


Commas are treated as literal characters.

e.g. --metadata git_diff="foo,bar" should produce one entry with value "foo,bar", not two entries.

gchan · 2026-02-25T03:55:14Z

cli.go

+	Category: "PREVIEW: TEST SELECTION",
+	Usage:    "Additional metadata key=value sent to the test plan API. Repeat for multiple entries.",
+}
+


Similar question why this new category of flags is in the middle of another category (i.e. between base-url and suite-slug)

No strong reason, other than that they probably do belong in this section:

// Test Engine specific flags

… but they could be added to the end of that section, bumped down one to be after baseURLFlag, although arguably that flag is special because it's Hidden: true so maybe it belongs last? 🤷🏼‍♂️

Usually I like to alphabetically sort things where the order doesn't otherwise matter, to avoid these conundrums 😁

gchan · 2026-02-25T04:01:58Z

cli.go

+func validateSelectionConfig(strategy string, params map[string]string) error {
+	if strategy == "" && len(params) > 0 {
+		return fmt.Errorf("invalid selection params: selection strategy must be set when selection params are provided")
+	}
+
+	return nil
+}


Can we move this validation to where all the other validation is occurring, to the validate function on Config?

nprizal · 2026-02-25T04:02:07Z

cli.go

+		return nil
+	}
+
+	selectionParams, err := buildStringMap(


can we do the parsing in the flag definition instead?

var selectionParamFlag = &cli.StringSliceFlag{ Name: "selection-param", Category: "PREVIEW: TEST SELECTION", Usage: "Additional selection strategy parameter key=value sent to the test plan API. Repeat for multiple entries.", Sources: cli.EnvVars("BUILDKITE_TEST_ENGINE_SELECTION_PARAMS"), Action: func(ctx context.Context, cmd *cli.Command, v []string) error { if !previewSelectionEnabled() { return nil } if !cmd.IsSet("selection-strategy") { return fmt.Errorf("invalid selection params: selection strategy must be set when selection params are provided") } // parse the value, and set it in the config return nil }, }

Sonnet 4.6 reckons that would have some issues re. handling a single JSON value from env as being equivalent to multiple args.

It sounds legit to me.

LLM output

The suggestion is partially feasible but has a notable limitation.**

The Action callback on a flag fires when the flag is set — but selectionParamFlag uses Sources: cli.EnvVars(...), and the env var (BUILDKITE_TEST_ENGINE_SELECTION_PARAMS) supplies JSON, not repeated key=value entries. The current code handles these two sources differently:

--selection-param key=value → parsed by parseKeyValueEntries

BUILDKITE_TEST_ENGINE_SELECTION_PARAMS={"key":"val"} → parsed by parseStringMapJSON

The Action callback on the flag only receives the slice of string values from the flag itself (merged with env var entries as raw strings). If the env var contains JSON, cli/v3 would pass the raw JSON string as one entry in v, and the key=value parser would reject it.

The current design intentionally separates the two: buildStringMap calls parseStringMapJSON on the env var separately and parseKeyValueEntries on the flag values.

What the suggestion could work for: the --selection-param flag-only path — parsing key=value entries and setting cfg.SelectionParams within Action. But you'd still need to handle the JSON env var elsewhere, and the validation (cmd.IsSet("selection-strategy")) in Action would fire before applyPlanRequestContext even runs, which is actually fine for ordering.

The bigger issue: the Action approach would split the logic — flag parsing in the flag definition, env var JSON parsing still in applyPlanRequestContext — making it harder to follow. The current approach keeps all of that in one place with clear separation of concerns.

Summary for your colleague: The suggestion has merit for the key=value flag parsing, but because the env var uses a different format (JSON object vs repeated key=value), moving parsing into Action would require keeping the JSON env var handling in applyPlanRequestContext anyway, splitting the logic rather than simplifying it. The current centralised approach in applyPlanRequestContext is cleaner.

I wonder if we could just not accept JSON 🤔
But it's hard to e.g. split on commas or newlines when there can be commas in the actual input data.
You end up needing some kind of parser that handles escaping etc.

I guess the question then is whether we want multiple args/JSONs? Can't we just add all params in one arg/JSON? It looks like we are merging the JSONs anyway.

For CLI economics, I greatly prefer this:

bktec \ --selection-strategy demo \ --selection-param hello=world \ --selection-param foo=bar \ --metadata diffstat="$(git diff --stat)"

Over this:

bktec \ --selection-strategy demo \ --selection-params '{"hello": "world", "foo": "bar"}' \ --metadata "{\"diffstat\": \"$(git diff --stat)\"}"

And IMO CLI ergonomics is important!

aaah! so we are accepting 2 formats. JSON from env vars BUILDKITE_TEST_ENGINE_SELECTION_PARAMS= '{ "percent": 40 }' and key=value format for cli flag --selection-param percent=40.

I think it will be nice if the format is consistent between env var and cli flag.

I just wish urfav/cli had a better out-of-the-box way to do multi-arg from env!

nice if the format is consistent between env var and cli flag

I agree!

Maybe we just rip out ENV support for these flags for now, and come back to it later.

I really want to support non-JSON CLI args more than I want to be able to pass them via ENV.

I've removed json-from-env support entirely, for now.

gchan · 2026-02-25T04:08:08Z

cli.go

+	}
+}
+
+func applyPlanRequestContext(cmd *cli.Command) error {


I wonder if this function and other new ones can be moved to another file? Perhaps inside internal/config?

It's coupled to github.com/urfave/cli/v3 which is currently only imported in package main (e.g. cli.go) and is specific to CLI arg parsing.

So I think we'd only move it if we created a new CLI-arg-parsing internal package. For now, the separate cli.go in package main is the equivalent of that.

I see, applyPlanRequestContext should probably stay in this file however there is an option to shift the parsing functions to an internal parsing package (or config/selection.go?) or leave it here.

I'm going to rip out the JSON-from-env stuff entirely, it's awkward and I don't like it 😅

nprizal · 2026-02-25T04:20:43Z

cli.go

+)
+
+func previewSelectionEnabled() bool {
+	switch strings.ToLower(strings.TrimSpace(os.Getenv(previewSelectionEnvVar))) {


[nit] Maybe we can make this a hidden flag? The cli library will handle the bool flag so we don't to manually parse it.

I wasn't sure if we could use a CLI flag to drive which CLI flags were available… it seemed chicken-and-egg but I didn't go deep on whether it's possible. What do you think?

If we do the validation inside the flag action, I think we can do this. But happy to leave it for now.

Yeah I'll leave it as is… we'll rip it out soon anyway when it's no longer in preview.

gchan · 2026-02-25T04:20:48Z

cli.go

+		if key == "" {
+			return nil, fmt.Errorf("%s %q has empty key", fieldName, entry)
+		}


Should we also ensure values are non-empty too?

I can't think of a strong reason to disallow empty values.
Unix env vars work this way, e.g:

$ USER= env | grep ^USER USER=

That's a good point. Do we want to ignore keys (and not include them in result) where the value is empty?

If you can think of a reason to do that :)
Otherwise I'll just allow empty values for now. Maybe they'll be useful? 🤷🏼‍♂️

Unix-ish ENV doesn't disallow them… they comes through as an empty string, which is distinct from unsetting them.

$ ruby -e 'p ENV["HOME"]' "/Users/pda" $ env HOME= ruby -e 'p ENV["HOME"]' "" $ env -u HOME ruby -e 'p ENV["HOME"]' nil

The implementation is messy, and the UX inconsistency is unpleasant. We'll come up with a better way in future, for now just the CLI flags is enough.

gchan

lgtm, no major blockers. Does it look good to you @nprizal ?

pda requested a review from a team as a code owner February 24, 2026 06:33

chatgpt-codex-connector bot reviewed Feb 24, 2026

View reviewed changes

internal/api/create_test_plan.go Outdated Show resolved Hide resolved

pda force-pushed the preview-test-selection branch 2 times, most recently from 051b10f to d0869a9 Compare February 25, 2026 01:47

pda force-pushed the preview-test-selection branch from d0869a9 to 6d22176 Compare February 25, 2026 01:51

gchan reviewed Feb 25, 2026

View reviewed changes

nprizal reviewed Feb 25, 2026

View reviewed changes

gchan reviewed Feb 25, 2026

View reviewed changes

Simplify previewSelectionFlags() append (no functional change)

37fe7b1

nprizal reviewed Feb 25, 2026

View reviewed changes

gchan reviewed Feb 25, 2026

View reviewed changes

pda added 2 commits February 25, 2026 15:07

validation of test selection flags/config moved to Config.validate()

ef46e46

test selection: for repeatable key=value CLI flags, remove JSON-in-env

768f200

The implementation is messy, and the UX inconsistency is unpleasant. We'll come up with a better way in future, for now just the CLI flags is enough.

pda requested a review from a team February 25, 2026 05:46

gchan approved these changes Feb 25, 2026

View reviewed changes

pda merged commit 325e41e into main Feb 26, 2026
1 check passed

pda deleted the preview-test-selection branch February 26, 2026 02:42

		export BUILDKITE_TEST_ENGINE_SELECTION_PARAMS='{"percent":"40"}'
		export BUILDKITE_TEST_ENGINE_METADATA='{"commit_message":"fix flaky tests","git_diff":"..."}'

Conversation

pda commented Feb 24, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nprizal Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nprizal Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nprizal Feb 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

nprizal Feb 25, 2026 •

edited

Loading

nprizal Feb 25, 2026 •

edited

Loading

nprizal Feb 25, 2026 •

edited

Loading