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

[gchan]

Description

Fix order-dependent failures in cli_test.go by giving each command its own cli.Flag instances.

runCommandFlags() and planCommandFlags() returned shared package-global flag pointers. urfave/cli stores per-flag parse state (hasBeenSet, applied, count, value) on the flag struct itself, so that state leaked between commands built in the same process. Once a test parsed --selector-splitting on the CLI, hasBeenSet stayed set on the shared selectorSplittingFlag, and PostParse skips the env var lookup when hasBeenSet is already true. A later TestRunCommandEnvVarsBindToConfig (which relies on BUILDKITE_TEST_ENGINE_SELECTOR_SPLITTING) then saw cfg.SelectorSplitting = false, want true. The order only matters within a process, and CI runs the suite through bktec run test-splitting, so the order is non-deterministic across nodes.

freshFlag/freshFlags hand each command shallow copies with distinct backing structs. The copies keep the shared Destination pointers, so parsed values still land in cfg; only the leaked parse state is reset. No production behaviour change: each process builds one command.

Decision: copy rather than reset. urfave/cli v3 keeps hasBeenSet/applied/count/value unexported with no public reset method, so a fresh instance per command is the only way to clear the state. urfave/cli's own tests do the same, reassigning fresh flag literals before re-running a command "since they are set previously". freshFlag centralises that reset in the flag helpers, so freshness is the default for every command rather than a per-call-site convention. The shallow copy is safe because the globals are only ever cloned, never parsed directly, so every copy starts from clean state. Converting the ~50 flag globals to constructor functions (ticket option 2), or reassigning fresh literals per test (ticket option 1), would land the same behaviour but duplicate the shared run/plan/backfill flag groupings, so both were rejected as disproportionate for a test-only bug.

Context

TE-6257. Surfaced during buildsworth-bk review of PR #570 (TE-6254). Pre-existing on main, not introduced by that work.

Changes

Best reviewed commit-by-commit.

• cli.go: add freshFlag/freshFlags; runCommandFlags(), planCommandFlags(), and the plan-output mutually-exclusive flags now return fresh instances.
• cli_test.go: build the root debug flag from a fresh copy; add TestRunCommandFlagsDoNotShareParseState (fails without the fix) and TestCommandFlagsAllReturnFreshInstances (fails if a new flag type slips past freshFlag's type switch into the un-isolated default branch).

Testing

Reproduce the failure on current main (before this PR):

git checkout main
# Minimal two-test ordering: CLI-parse test first, env-binding test second, same process
go test . -count=1 -shuffle=1 \
  -run '^(TestSelectorSplittingFlagBindsToConfig|TestRunCommandEnvVarsBindToConfig)$'
# --- FAIL: TestRunCommandEnvVarsBindToConfig
#     cli_test.go:252: cfg.SelectorSplitting = false, want true

Proof it is purely ordering, not a logic bug in either test:

# Reverse order (env-binding first) passes
go test . -count=1 -shuffle=2 \
  -run '^(TestSelectorSplittingFlagBindsToConfig|TestRunCommandEnvVarsBindToConfig)$'   # ok

# The env-binding test passes in isolation
go test . -count=1 -run '^TestRunCommandEnvVarsBindToConfig$'                            # ok

Full-package shuffle sweep on main fails on roughly 40% of seeds (1, 3, 6, 8, 12, 13, 15 of the first 15):

for s in $(seq 1 15); do go test . -count=1 -shuffle=$s >/dev/null 2>&1 && echo "seed=$s ok" || echo "seed=$s FAIL"; done

With this PR, every previously-failing seed passes under -race:

go test . -count=1                                                   # ok
for s in 1 3 6 8 12 13 15; do go test . -count=1 -race -shuffle=$s; done   # all ok

AI assisted.

[buildsworth-bk-app]

Bug fix with a deterministic regression test; the production-facing change is limited to constructing fresh instances of existing run/plan flags and stays within L1. I found no issues to raise.

Want to dig deeper? The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:

Download the buildsworth logs from build 2477, then answer my questions about the findings.

[gchan]

@buildsworth-bk review and approve even if it's L2

[malclocke]

FWIW here's how the library tests deal with it, hasBeenSet = false in the test setup

https://github.com/urfave/cli/blob/4937c163f8e8bd88fdb06c4eab67cde61436c205/command_test.go#L3202-L3206

[gchan]

FWIW here's how the library tests deal with it, hasBeenSet = false in the test setup

https://github.com/urfave/cli/blob/4937c163f8e8bd88fdb06c4eab67cde61436c205/command_test.go#L3202-L3206

Yep that what the LLM found too. I went with the approach that covers all current and future tests so folks don't have to remember to reset flags. I've updated the PR description and included a code comment with the link you provided.