PR #4145 review summary

Change reviewed

This PR changes Git branch/commit verification for agent v4. The current revision defaults --git-commit-verification to strict, replaces the prior warn override with an explicit off mode, and validates the effective startup value from flags, environment variables, and the agent config file. At executor level, strict and the zero value perform the ancestry check, off skips it, and unknown dynamically supplied values fail the checkout.

The checkout still blocks only a definitive branch/commit mismatch. Verification that cannot complete because of an infrastructure or Git error remains warning-only, and the existing skip cases for HEAD, tags, pull requests, custom refspecs, and missing branches remain. The change also updates propagation through AgentConfiguration, checkout-override handling, runtime environment refresh, and affected checkout tests.

Context and evidence examined

• Read the full 10-file PR diff and all changed files.
• Traced the setting through clicommand/global.go, clicommand/agent_start.go, clicommand/bootstrap.go, agent/job_runner.go, internal/job/config.go, internal/job/checkout.go, and internal/job/commit_verification.go.
• Checked how backend job environment, hooks/plugins/Job API refreshes, secrets, and the strict/from-job/none checkout-override modes affect the final executor value.
• Reviewed the focused unit and integration coverage for the strict default, explicit off, zero-value fail-closed behavior, short commit checkout, definitive mismatches, shallow clones, and unavailable verification. I also inspected the runtime dispatch that rejects unknown dynamically supplied modes.
• Read Linear issue A-1592 and its discussion, including the later proposal to retain an explicit escape hatch.
• Read the PR conversation and both earlier buildsworth reviews. The earlier warning-message duplication concern is already fixed and its thread is resolved; no human review threads were present.
• Inspected the linked companion documentation PR, buildkite/docs#3145.
• git diff --check origin/v4...HEAD passed.

I could not run the Go tests locally because the checkout environment does not have the go executable installed. At submission time, GitHub showed the category and Socket checks passing, while the buildkite/agent build #13761 was still pending. Direct Buildkite CLI lookup was also unavailable because the configured token could not access the buildkite organization, so no Buildkite job logs were inspected.

Finding

One non-blocking inline finding was submitted on clicommand/global.go: the implementation and CLI help now expose strict|off, but this PR's description and companion docs still document strict|warn and explicitly say there is no off mode. An operator following that migration guidance would configure warn, which the new validator rejects at startup. The PR description and companion documentation should be synchronized with the final contract before release.

The implementation itself consistently applies the current strict|off contract across CLI validation, config-file validation, job environment propagation, runtime executor dispatch, and tests. Treating the executor zero value as strict also avoids restoring the former empty-string bypass for direct executor consumers.

Verdict

Submitted a COMMENT review with one non-blocking inline comment. I found no code-level blocker in the current revision, but this changes a security policy and adds verification work to every eligible checkout, so it requires human sign-off rather than an automated approval. The agent CI build was still running when the review was posted.

Trigger source: automatic.