Skip to content

Execute Windows batch hooks through the system command processor - #4206

Open
lox wants to merge 3 commits into
mainfrom
fix-windows-batch-script-execution
Open

Execute Windows batch hooks through the system command processor#4206
lox wants to merge 3 commits into
mainfrom
fix-windows-batch-script-execution

Conversation

@lox

@lox lox commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • execute default Windows .bat and .cmd hooks through the Windows command processor
  • resolve cmd.exe from the Windows system directory rather than PATH
  • preserve cmd.exe quoting for script paths containing spaces and shell metacharacters
  • reject script paths containing % rather than allowing environment-variable expansion to select a different path

Why

The agent currently passes default Windows batch hooks to CreateProcessW as if they were executable images. This usually works because Windows has an undocumented compatibility behavior that converts a .bat or .cmd application into a cmd.exe /c invocation. Rust documents the same behavior, warns that it may be removed, and handles batch files explicitly.

That implicit behavior is not the supported Windows contract. Microsoft's CreateProcessW documentation says that callers must start the command interpreter to run a batch file. Go's os/exec.Command documentation also calls out cmd.exe and batch files as exceptions to its normal Windows argument quoting and directs callers to provide the appropriate raw command line.

This change makes that contract explicit at the shared internal/shell boundary. Default lifecycle, bootstrap, repository, and plugin batch hooks run through the trusted system command processor with cmd-specific quoting, while explicit HooksShell overrides remain unchanged.

Literal % is legal in a Windows path but is expanded by cmd.exe even inside quotes. Until there is a verified literal encoding for script paths, the agent rejects these paths with an actionable error rather than potentially executing a different path.

Validation

  • go test ./internal/shell ./internal/process ./clicommand
  • go test -race ./internal/shell ./internal/process ./clicommand
  • golangci-lint run ./internal/shell ./internal/process ./clicommand
  • Windows cross-compilation of internal/shell and internal/process test binaries
  • Windows runtime coverage for .bat and .cmd, piped stdout/stderr, non-zero exit propagation, environment expansion, paths containing spaces, &, parentheses, !, and ^, and deterministic rejection of % in paths

@lox
lox marked this pull request as ready for review August 8, 2026 08:41
@lox
lox requested review from a team as code owners August 8, 2026 08:41

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found no concrete issues. The shared Windows process-launch and cmd.exe quoting path is subtle enough that it still warrants a human sanity-check. CI exercised the new Windows batch regression coverage successfully; the overall build remains red on the separately documented lifecycle-hook flake.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

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

Install the reading-buildsworth-logs skill to run this.

About buildsworth

Model: gpt-5.6-sol with xhigh thinking.

How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.

Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. To let it approve, grant L2 approval by mentioning @buildsworth-bk (see L2 approval grant):

  • L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
  • L2 — Standard risk (new UI, additive API fields, refactors). Approved only with an L2 grant; otherwise comment-only.
  • L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No concrete issues in the implementation or the follow-up metacharacter and exit-status coverage. Targeted and race tests passed locally, as did Windows cross-compilation. The raw cmd.exe launch path affects Windows hook execution broadly enough that it still needs a human sanity-check. The branch Buildkite check is red, but its job logs were unavailable to this session.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

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

Install the reading-buildsworth-logs skill to run this.

ampagent and others added 3 commits August 9, 2026 04:41
@lox
lox force-pushed the fix-windows-batch-script-execution branch from e4d35fb to e338d7e Compare August 9, 2026 04:42

@buildsworth-bk-app buildsworth-bk-app Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No concrete issues in this revision. The raw cmd.exe launch and quoting path affects all default Windows batch hooks, so the execution approach still warrants a human sanity-check. go test and go test -race passed locally for internal/shell and internal/process, and both packages' Windows test binaries cross-compiled; branch CI is still running.

Want to dig deeper?

Paste this into your agent to explore the findings from this review's Buildkite build:

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

Install the reading-buildsworth-logs skill to run this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants