Reject unsupported Windows shells before workflow jobs start - #387
Review submitted — changes requested
Review summary
PR #387 moves unsupported PowerShell and Windows-shell detection from job runtime into workflow processing when the shell can be resolved statically. It extracts shell-template parsing and compatibility classification into internal/shell, calls that classifier while constructing plans, keeps runtime validation for expressions that still depend on runtime context, and updates the compatibility guide. The new compiler tests cover explicit step shells, job defaults, matrix-resolved shells, accepted custom templates, and retained runtime expressions. Runtime coverage verifies that a dynamic unsupported shell still fails before its step runs.
What I reviewed
I read the full diff and traced the changed paths through:
reducePlanEventExpressionsandvalidateShellCompatibilityininternal/compiler/plan_builder.goEvaluateAvailableCompileTemplateand the compile-time expression context- program-site lowering and source attribution
ProcessingFindingconversion into compatibility diagnostics- runtime shell selection, custom-template parsing, and process execution in
internal/runtime/action_execution.go - existing tests for event-derived diagnostic sanitization, event-backed shell reduction, workflow defaults, custom shells, and compatibility-gap fixtures
docs/compatibility.mdand PB-3021
The extracted parser preserves the prior custom-template behavior while giving compile-time and runtime checks one classifier. The implementation also correctly leaves unresolved env-dependent shells in plans and continues to reject them at runtime. The PR's Buildkite checks were successful in GitHub, including buildkite/buildkite-gha build 1827 and the public-actions and shell smoke proofs.
I attempted the focused Go tests, go vet, and golangci-lint, but this review environment did not contain mise or go, so I could not run them locally. The Buildkite API token could not read the supplied review build directly; I verified available CI state through GitHub instead.
Blocking finding
internal/compiler/plan_builder.go:193 copies the complete resolved shell error into ProcessingFinding.Message. Before that point, reducePlanEventExpressions has folded available event values into the shell string. A shell such as:
shell: pwsh -File {0} ${{ github.event.pull_request.title }}therefore carries PR-controlled title text into the generated workflow failure report. This conflicts with the explicit ProcessingFinding.Message invariant that report messages must not contain event-derived data; existing runner diagnostics deliberately suppress resolved labels when their provenance is not workflow-authored.
The review requests a bounded correction: report fixed guidance or the normalized blocked command, or include the full shell only when its provenance is known to be workflow-authored. I submitted REQUEST_CHANGES with one inline blocking comment.
Trigger source: automatic.