Review summary: buildkite/agent PR #4350

Change reviewed

PR #4350 is a trusted-branch mirror of external contribution #4286. It fixes inconsistent hook bookkeeping in internal/job/executor.go: after an unwrapped script or binary hook finishes, runUnwrappedHook now derives the process status with shell.ExitCode and stores it in BUILDKITE_LAST_HOOK_EXIT_STATUS. This brings unwrapped hooks in line with the existing wrapped-shell-hook path and prevents subsequent hooks from seeing an unset or stale status.

The accompanying table-driven regression test in internal/job/executor_test.go exercises successful and failing unwrapped hooks, expecting statuses 0 and 7 respectively.

No Linear issue was linked. The branch name produced the candidate PR-4286, but linear issue view PR-4286 found no issue; the relevant context instead came from the current PR and original PR #4286.

What I examined

I read the complete changed files and traced the surrounding hook execution paths, including:

• Hook type dispatch in Executor.executeHook for interpreted scripts, binaries, and wrapped shell hooks.
• The existing runWrappedShellScriptHook status handling used as the consistency reference.
• Forward and reverse hook sequencing in internal/job/hooks.go, including how a stored status becomes visible to later hooks.
• Global, repository, and plugin HookConfig construction, plus environment merging through shell.WithExtraEnv.
• shell.ExitCode, including its handling of successful runs, process exit errors, and non-exit execution errors.
• Existing polyglot-hook coverage and the declared CI test environment in .buildkite/Dockerfile-compile, which explicitly installs Ruby but does not install Python.
• The current PR conversation, all reviews and inline threads, and the original PR description. There were no earlier buildsworth reviews or duplicate inline findings when this review was submitted.

The production change is narrow and follows the established wrapped-hook behavior. I did not find another correctness problem in that implementation.

Finding and verdict

I submitted CHANGES_REQUESTED with one blocking inline finding on internal/job/executor_test.go:419:

• The test fixture uses #!/usr/bin/env python, introducing an undeclared interpreter dependency. The repository's standard compile image installs Ruby for polyglot-hook tests but not Python. In an environment without the python alias, the fixture exits with status 127 before testing either intended status, so the regression test itself fails. The inline comment includes a suggestion to use the already-required Ruby interpreter (exit(N)) while preserving the unwrapped-hook code path.

Review: #4350 (review)

Inline finding: #4350 (comment)

Checks and evidence

Checks I ran locally with Go 1.26.5:

• git diff --check origin/main...HEAD — passed.
• go test ./internal/job -run '^TestRunUnwrappedHookSetsLastHookExitStatus$' -count=1 with the checked-in environment — failed reproducibly because /usr/bin/env could not find python; both subtests observed exit status 127.
• The same focused test with a temporary python shim pointing to the available python3 — passed, confirming the production status-handling behavior once the fixture interpreter is available.
• go test ./internal/job -count=1 with that temporary shim — the new test passed, but the package run failed on the environment-specific TestResolvingGitHostAliasesWithFlagSupport SSH aliases. This checkout lacks the /etc/ssh/ssh_config.d/ setup documented in AGENTS.md, so that unrelated failure was not attributed to the PR.
• go vet ./internal/job — passed.
• go tool gofumpt -extra -d internal/job/executor.go internal/job/executor_test.go — produced no diff.

I did not run the full go test ./... suite. At review time, GitHub reported the buildkite/agent CI status for Buildkite build #14330 as failed, while label and Socket checks had passed. I attempted to query the build and failed jobs with the Buildkite CLI, but the available token could not access the buildkite organization, so I could not inspect build #14330's job logs or independently attribute that CI failure to the same interpreter issue.

Trigger source: automatic.