Review summary: buildkite/agent PR #4206

Change reviewed

This PR makes default Windows .bat and .cmd hooks run explicitly through cmd.exe instead of relying on Windows' undocumented CreateProcessW compatibility behavior for batch files.

The implementation:

• adds an optional raw Windows command line to internal/process.Config and passes it to windows.SysProcAttr.CmdLine, bypassing Go's normal Windows argv quoting when needed;
• resolves cmd.exe from the Windows system directory with windows.GetSystemDirectory, rather than trusting PATH;
• detects .bat and .cmd extensions case-insensitively in shell.Script;
• constructs the cmd.exe /D /V:OFF /S /C command line using cmd-specific outer quoting;
• rejects hook paths containing %, because cmd.exe expands percent-delimited environment variables even within quotes;
• leaves explicit HooksShell overrides on their existing path; and
• adds Windows runtime tests for piped output, environment expansion, metacharacters, uppercase .CMD, non-zero exit propagation, and percent-path rejection.

What I examined

I read the complete six-file diff and the full changed files in internal/process and internal/shell. I also traced the important consumers of shell.Script, including agent lifecycle and pre-bootstrap hooks and the wrapped repository/plugin hook path in internal/job/executor.go.

I verified that the raw command line reaches SysProcAttr.CmdLine before process startup and that the normal Args remain available for logging and process metadata. I checked the relevant Go 1.25.12 Windows process source, which confirms that a non-empty SysProcAttr.CmdLine replaces Go's generated command line while retaining the configured executable path for CreateProcessW.

I also checked PTY handling because setupProcessGroup is where the new raw command line is installed. Windows PTY mode is disabled by the agent/start and bootstrap configuration paths, while lifecycle and pre-bootstrap shells default to non-PTY operation, so the affected supported Windows hook paths pass through this setup.

The PR has no Linear issue identifier in its branch name, title, or description, so no Linear issue was available to inspect. There were no inline or conversation comments, and the previous buildsworth reviews had not raised concerns.

Checks run

Using Go 1.25.12 locally on Linux:

• go test ./internal/shell ./internal/process passed.
• go test -race -p=1 ./internal/shell ./internal/process passed.
• Windows/amd64 test binaries for both internal/shell and internal/process cross-compiled successfully.

The initial combined command also attempted ./clicommand, but its build was killed while compiling the AWS EC2 SDK because of the session's memory limit. A second single-package, single-worker attempt failed for the same environmental reason. This was not a test assertion failure.

I could not execute the Windows-only runtime tests from this Linux environment. I did not run the full repository test suite or golangci-lint. At submission time, the GitHub status for Buildkite agent build #13875 was still pending. The Buildkite CLI token available to this session could not retrieve that build directly, so no job logs were inspected.

Findings and verdict

I found no concrete correctness, security, or compatibility issue in the current revision, and posted no inline findings. Resolving the command processor from the system directory avoids a PATH trust problem, and the /D and /V:OFF options plus explicit % rejection address the cmd-specific expansion mechanisms relevant to hook paths. The tests cover the main quoting-sensitive characters and exit/output behavior.

The main trade-off is the intentional rejection of otherwise-legal Windows paths containing %; this is explicit in the PR's scope and preferable to silently selecting a different path through environment expansion.

I submitted a COMMENT review rather than an approval because the shared raw cmd.exe launch and quoting path changes default Windows hook execution broadly enough to warrant a human Windows-focused sanity-check, especially while the current branch CI remained in progress.

Review: #4206 (review)

Trigger source: automatic.