Skip to content

ci: harden reusable workflow execution - #92

Merged
kyle-sexton merged 6 commits into
mainfrom
codex/repo-governance-hardening
Jul 14, 2026
Merged

ci: harden reusable workflow execution#92
kyle-sexton merged 6 commits into
mainfrom
codex/repo-governance-hardening

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • centralize checksum-verified release installation in job-scoped runtime directories and derive immutable self-pins from tracked template workflows
  • bound read-only network operations and discard partial failed-attempt output before retries
  • add reusable Go quality execution with native Linux and Windows golangci-lint v2.12.2, exact official archive checksums, config verification, complete issue reporting, Linux race tests, Windows ordinary tests, gofmt/tidy/module verification, and no direct Staticcheck side policy
  • install govulncheck v1.6.0 through the authenticated Go module proxy/checksum database and classify validated SARIF so reachable findings fail product while tool/network/DB/malformed-output failures require an infrastructure rerun
  • track golangci-lint and govulncheck drift through their official release channels

Caller contract

jobs:
  go-quality:
    permissions:
      contents: read
    uses: melodic-software/ci-workflows/.github/workflows/go-quality.yml@<full-commit-sha>
    with:
      config: <standards-managed-config-path>

working-directory defaults to . and go-version-file defaults to go.mod. Tool versions, checksums, runners, package scope, and blocking posture are deliberately not caller-configurable.

ci-runner PR #35 remains the downstream consumer and will adopt the final immutable SHA after Standards materializes its managed config path. This branch does not edit ci-runner.

Verification

  • 195 Node tests passed
  • govulncheck SARIF classifier behavioral suite passed
  • OSV scan guard, Pulumi deployment guard, and Pulumi drift suites passed
  • actionlint passed for go-quality.yml
  • ShellCheck and shfmt passed for the new guard/tests
  • Biome 2.5.1 passed across all scripts
  • git diff --check passed

Authoritative references

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 28s —— View job


PR Review: ci: harden reusable workflow execution

  • Gather context and understand request
  • Review changed files
    • .github/actions/_shared/install-release.sh
    • .github/scripts/fetch-immutable-workflow-pins.cjs
    • .github/actions/pulumi-deploy-guard/guard.sh
    • .github/scripts/pulumi-version-drift.sh
    • Modified workflow files
    • Test files
  • Post review findings

This is a well-structured hardening PR. The core ideas are sound: job-scoped install paths, bounded retry/timeout for all network calls, output buffering to prevent partial-response corruption, and GA-verified object fetch. Here are the findings, most-severe first.


Findings

1. gh_read in pulumi-version-drift.sh streams stdout directly — partial output corruption possible on retry

Moderate.

pulumi-version-drift.sh:gh_read streams stdout through directly (unlike bounded_read in tool-version-drift-check.yml, which buffers to a temp file). When called in a capturing subshell like:

latest_tag="$(gh_read api repos/pulumi/pulumi/releases/latest --jq '...')"

If gh writes partial output to stdout before the timeout fires on attempt 1, and attempt 2 succeeds, latest_tag will contain <partial_attempt_1><full_attempt_2>. The same risk applies to gh_read api --paginate --slurp inside find_active_incidents().

The same streaming pattern exists in pulumi-version-drift-check.yml's inline gh_read (which mirrors the script). In practice gh rarely writes to stdout on connection failure, but this differs from the intent of the bounded_read approach used elsewhere in this PR. The temp-file pattern from bounded_read / bounded_docker_read is the correct fix. Fix this →


2. network-timeout-policy.test.cjs regex is fragile to indentation changes

Minor.

Lines 104–108:

assert.match(
  guard,
  /timeout --signal=TERM --kill-after=5s 60s[\s\\]+\n\s+"\$pulumi_bin" api/u,
);

This regex hard-codes the whitespace/continuation pattern. Any reformatting of guard.sh that doesn't change semantics — e.g., breaking the line differently — will break the test. The test correctly verifies that timeout bounds the Pulumi read, but a simpler check would be to assert that timeout --signal=TERM --kill-after=5s 60s and "$pulumi_bin" api both appear within the pulumi_api_read function, rather than requiring a specific line continuation form.


3. workflowSha duplicate-reference case is not directly unit-tested

Minor.

fetch-immutable-workflow-pins.test.cjs:47 only exercises workflowSha(source, "alpha.yaml") (unique match). The fixture has zeta.yml twice, so workflowSha(source, "zeta.yml") would throw ("expected exactly one…found 2"), but that path isn't exercised. The invariant is in the code; a 3-line assert.throws for the duplicate case would close the gap.


Positive Observations

  • Removing sudo install … /usr/local/bin in favour of a job-scoped $RUNNER_TEMP/ci-workflows/bin is the right move — drops a privilege escalation step and keeps the install blast radius within the job.
  • cat-file -e <sha>^{commit} verification after git fetch is a tight proof that the fetched object is a commit, not a tree or blob — stronger than trusting the fetch exit code alone.
  • bounded_read / bounded_docker_read temp-file pattern correctly prevents any partial output from a timed-out attempt from reaching downstream pipelines. The success-path catrm and failure-path rm are both handled.
  • gh_mutate / pulumi_api_read design intent — mutations are single-attempt (a timed-out write may have already succeeded server-side); reads get one bounded retry. This is the right model.
  • SHA256 format validation before sha256sum -c prevents hash-string injection. The BIN path validation (no /, no ..) prevents directory traversal in the install target. The ARCHIVE_MEMBER case check catches ../ and // traversal before extraction.
  • curl -q suppresses ~/.curlrc option injection; combined with --silent --show-error --proto '=https' --proto-redir '=https' this closes a class of curl config influence from untrusted environments.
  • Atomics.wait as synchronous sleep in Node.js is correct for a CJS CLI script (no event loop dependency); the injected sleepFor parameter in tests is a clean seam.
  • install-release.test.cjs idempotency test correctly models the GitHub Actions step-to-step GITHUB_PATH hand-off (fresh file each step; prior content appears in PATH), confirming the second install invocation skips writing the path entry.

Overall this is mergeable; the two moderate/minor items above are worth addressing before landing, but none are blockers if the team accepts the partial-output risk as theoretical.

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 2s —— View job


PR Review: ci: harden reusable workflow execution

  • Gather context
  • Review latest diff (HEAD: 20fa2cea)
  • Post findings

What changed in the fixup commit (20fa2cea)

The "satisfy workflow quality gates" commit is primarily a formatter/linter pass:

  • install-release.sh: Added *) ;; default arms to both case statements — satisfies shellcheck SC2078 / exhaustive-case requirement; no functional change.
  • guard.sh: Refactored pulumi_api_read() — the function now accepts the error message as a third parameter and calls fail "$error_message" directly instead of returning a status for the caller to check with || fail. Callers drop the || pattern. This is a positive change: the function is now self-contained, the else status=$? capture is gone, and fail (always exit 1) replaces propagating the raw timeout/process exit code. Correct.
  • fetch-immutable-workflow-pins.cjs / .test.cjs, install-release.test.cjs, zizmor-native.test.cjs: Pure line-length reformatting; no functional change.
  • network-timeout-policy.test.cjs: Formatting + two reordered assertions; bounded_read()[\s\S]*?>"\$output"[\s\S]*?cat -- "\$output" now also verified under "small JSON" test — tightens coverage of the temp-file buffering invariant.

Status of prior review findings

1. gh_read in pulumi-version-drift.sh streams stdout directly — still open

pulumi-version-drift.sh:18-32 is unchanged. gh_read() passes gh's stdout straight through; when used in a capturing subshell (latest_tag="$(gh_read api ...)" at line 56, and find_active_incidents piped at line 66), a timed-out first attempt that emitted partial bytes before SIGTERM will concatenate with the successful second attempt. The pulumi-version-drift-check.yml inline mirror has the same shape.

The guard.sh pulumi_api_read uses an explicit >"$output" temp file — the correct pattern. The gh_read function in pulumi-version-drift.sh should adopt the same temp-file buffering. Fix this →

2. Fragile line-continuation regex in network-timeout-policy.test.cjsstill open

Lines 84–90 still assert:

/timeout --signal=TERM --kill-after=5s 60s[\s\\]+\n\s+"\$pulumi_bin" api/u
/timeout --signal=TERM --kill-after=5s 300s[\s\\]+\n\s+"\$pulumi_bin" stack export/u

These pass against the current guard.sh formatting, but any reformatter that collapses the line continuation (e.g. 60s "$pulumi_bin" on one line) would break the test without changing behavior. Replacing with two independent assert.match calls — one for the timeout invocation and one for "$pulumi_bin" api within the same function — would make the assertion semantics-stable.

3. workflowSha duplicate-reference case untested — still open

The test fixture contains two zeta.yml references, but only the unique alpha.yaml case is exercised with workflowSha. A assert.throws(() => workflowSha(source, "zeta.yml"), /expected exactly one/) would close this gap.


No new issues from the fixup commit

The guard.sh pulumi_api_read refactor (moving fail inside the function) is correct. The behavioral difference — fail always exits 1 vs. propagating the original timeout/process exit code — is consistent with the rest of the script's fail convention and an improvement. All formatting-only changes are correct.


Overall: the three items above are carry-forwards from the initial review pass. None are blockers, but items 1 and 2 are worth resolving before merge. The fixup commit is clean and contains no regressions.

@kyle-sexton
kyle-sexton force-pushed the codex/repo-governance-hardening branch from ad319b5 to 30dd861 Compare July 14, 2026 22:00
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 31s —— View job


PR Review: ci: harden reusable workflow execution (commit 30dd861)

  • Gather context
  • Review new commit (30dd861 — "ci: add reusable Go quality contract")
  • Check status of prior open findings
  • Post findings

What changed in the new commit (30dd861)

This commit adds the go-quality.yml reusable workflow and its support files: govulncheck-sarif-guard.sh, govulncheck-sarif-guard.test.sh, go-quality.test.cjs, and drift-tracking additions in tool-version-drift-check.yml. The network-timeout-policy counter was bumped by 1 to account for the new curl_small_json call tracking golang.org/x/vuln.


Status of prior open findings

1. gh_read in pulumi-version-drift.sh streams stdout directly — still open

pulumi-version-drift.sh:18-32 is unchanged. gh_read() passes gh's stdout straight through; when called in a capturing subshell (latest_tag="$(gh_read api ...)" at line 56, find_active_incidents piped at line 66), a timed-out first attempt that emitted partial bytes before SIGTERM will concatenate with the successful second attempt. Fix this →

2. Fragile line-continuation regex in network-timeout-policy.test.cjsstill open

Lines 84–90 still assert the exact whitespace/continuation pattern between timeout … 60s and "$pulumi_bin" api. Any reformat that collapses the line continuation would break the test without changing behavior. Two independent assert.match calls — one for the timeout invocation and one for "$pulumi_bin" api within the same function — would make the assertions semantics-stable.

3. workflowSha duplicate-reference case untested — still open

fetch-immutable-workflow-pins.test.cjs still contains two zeta.yml references with no assert.throws covering the duplicate path.


New findings from this commit

4. go mod tidy -diff requires Go 1.23+

Moderate.

go-quality.yml:136 runs go mod tidy -diff, a flag added in Go 1.23. The workflow derives its Go version from the caller-supplied go-version-file (defaulting to go.mod), so a consumer pinning go 1.21 or go 1.22 will see flag provided but not defined: -diff — an unhelpful infrastructure error rather than a "module needs tidying" failure. No minimum Go version is documented or enforced in the input validation step.

Options: document the 1.23+ requirement in the input description, or detect the Go version and gate the flag. Fix this →


Positive observations from this commit

  • Generated guard sync testgo-quality.test.cjs:108–132 verifies that the inline # BEGIN GENERATED: govulncheck-sarif-guard.sh block in go-quality.yml exactly matches the standalone govulncheck-sarif-guard.sh source (minus shebang). If someone edits one without the other, CI catches it immediately. This is an effective synchronization guard without a build step.
  • govulncheck authentication chaingo mod download -json + jq checksum verification + go install + go version -m binary fingerprint check is a thorough three-stage proof that the installed binary matches the authenticated module. The GONOSUMDB: '' / GOPRIVATE: '' overrides ensure the sum DB is always consulted.
  • Input path validation on both platforms — The bash validate_relative and PowerShell Assert-RelativePath both block empty, absolute, backslash-containing, and ../-traversing paths before they reach filesystem calls.
  • --issues-exit-code=42 — Using an unusual exit code for the "found issues" case cleanly distinguishes golangci-lint product findings (42) from infrastructure failures (non-zero, non-42), with the classify step handling each case with a distinct exit code and error message.
  • Drift trackingtool-version-drift-check.yml now tracks both golangci-lint (via GitHub releases API) and govulncheck (via proxy.golang.org) alongside the existing tracked pins.
  • govulncheck SARIF provenance schema — The jq filter in the classify step validates the exact SARIF shape, tool driver identity, version, db URL, scan mode, and result level enumeration before touching any finding data. The test suite covers all failure branches (malformed, unknown-level, wrong-provenance, missing-results, scanner-error).

Overall: items 1–3 are carry-forwards from prior rounds; item 4 (go mod tidy -diff) is the only new moderate finding. The Go quality contract itself is well-structured. Finding 4 is worth addressing before consumers adopt the workflow, since it will produce a confusing failure against repos on Go 1.21/1.22.

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 11s —— View job


PR Review: ci: harden reusable workflow execution (commit 2a4f4e0)

  • Gather context
  • Review new commit (2a4f4e0 — "fix(ci): isolate retried GitHub read output")
  • Check status of prior open findings
  • Post findings

What changed in this commit

This commit addresses finding 1 (the gh_read partial-output risk) from prior review rounds. Changes across five files:

  • pulumi-version-drift.sh: gh_read() now allocates a temp file via mktemp "$RUNNER_TEMP/pulumi-gh-read.XXXXXX", redirects gh stdout into it, and only cats it on success — then rm -fs it in both the success and failure paths. This exactly mirrors the bounded_read/bounded_docker_read pattern used elsewhere in this PR. The RUNNER_TEMP guard was moved to just before the function definition (line 19 before line 24), which is marginally cleaner since the variable is now needed at gh_read call sites.
  • pulumi-version-drift-check.yml: The identical inline gh_read() receives the same fix.
  • pulumi-version-drift.test.sh: Adds a "failed read output cannot contaminate the successful retry" test — the mock writes v999. (intentionally partial) then exits 96 on attempt 1, and the test verifies the script still creates the correct drift issue on the retry and that the retry warning was emitted to stderr.
  • network-timeout-policy.test.cjs: The two fragile line-continuation regexes are updated from [\s\\]+\n\s+ to (?:[ \t]+|[ \t]*\\\r?\n[ \t]*) — now accepting both a single-line form and a backslash-continuation form without hard-coding the exact newline/indentation.
  • fetch-immutable-workflow-pins.test.cjs: Adds assert.throws(() => workflowSha(source, "zeta.yml"), /expected exactly one canonical zeta\.yml use; found 2/u) — the missing duplicate-reference case from the prior test gap.

Status of all prior findings

1. gh_read partial-output contamination on retry — RESOLVED

pulumi-version-drift.sh:21–43 now buffers gh stdout to a temp file and only surfaces it after a clean exit. The fix is correctly applied to both the script and its inline duplicate in pulumi-version-drift-check.yml. The new test exercises the partial-output isolation end-to-end.

2. Fragile line-continuation regex in network-timeout-policy.test.cjsSUBSTANTIALLY IMPROVED

Lines 83–90 now use (?:[ \t]+|[ \t]*\\\r?\n[ \t]*) in place of the previous [\s\\]+\n\s+. This is a meaningful improvement — it covers both single-line and backslash-continuation forms and handles CRLF endings. The assertions still bind the test to the timeout … 60s "$pulumi_bin" api adjacency requirement, but two independently positioned assert.match calls (one for the timeout invocation, one for "$pulumi_bin" api inside the same function) would make the semantics truly layout-independent. Low-priority; not a blocker.

3. workflowSha duplicate-reference case untested — RESOLVED

fetch-immutable-workflow-pins.test.cjs:36–39 now asserts the exact error message thrown for a duplicate zeta.yml reference.

4. go mod tidy -diff requires Go 1.23+ — STILL OPEN

go-quality.yml:136 still runs go mod tidy -diff without a minimum-version guard or documentation. A consumer with go 1.21 or go 1.22 in go.mod will get flag provided but not defined: -diff — an opaque infrastructure error rather than a "module needs tidying" diagnosis. This is the only remaining unaddressed finding. Fix this →


Positive observations

  • Test design for partial-output isolation: The pulumi-version-drift.test.sh mock writes genuinely malformed partial output (v999.) before exiting — this correctly proves that the temp-file boundary prevents contamination, not just that retries succeed. Most retry tests only simulate clean failures.
  • Symmetry: The inline gh_read() in pulumi-version-drift-check.yml is a byte-for-byte copy of the function in pulumi-version-drift.sh (modulo indentation). The sync is maintained correctly in this commit, and the go-quality.test.cjs-style generated-sync guard pattern would be worth applying here if the drift script continues to grow.
  • RUNNER_TEMP guard positioning: Moving the : "${RUNNER_TEMP:?...}" guard above the function definition (line 19 vs. the old line 49) is the right call — it makes the dependency explicit and fails early if the variable is absent.

Overall: this PR is ready to merge after addressing finding 4. Findings 1, 2 (substantially), and 3 are resolved. Finding 4 (go mod tidy -diff on Go < 1.23) is the only remaining action item.

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

PR Review: ci: harden reusable workflow execution (commit 4e289b8)

  • Gather context
  • Review new commit (4e289b8 — "fix(go): make module tidy check portable")
  • Check status of prior open findings
  • Post findings

View job run

@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 24s —— View job


PR Review: ci: harden reusable workflow execution (commits 4e289b8 + 0229ba7)

  • Gather context
  • Review new commits (4e289b8 — "fix(go): make module tidy check portable", 0229ba7 — "fix(go): isolate tidy check from workspaces")
  • Check status of prior open findings
  • Post findings

What changed in these two commits

4e289b8 — "fix(go): make module tidy check portable"

This addresses finding 4 (go mod tidy -diff requires Go 1.23+) by replacing the single-line go mod tidy -diff call with a standalone go-mod-tidy-check.sh script that works on any Go version supporting -modfile (Go 1.17+):

  1. Copies go.mod and go.sum to temp files (.ci-tidy.XXXXXX.mod / .sum)
  2. Runs go mod tidy -modfile=<temp> against the copies only — the originals are never modified
  3. Diffs the temp files against the originals; exits 1 if dirty, exits 2 on tool failure
  4. trap cleanup EXIT ensures temp files are removed in all paths, including tool failure

The go-quality.yml inline copy is kept in sync via the same generated-sync guard pattern used for govulncheck-sarif-guard.shgo-quality.test.cjs extracts the # BEGIN GENERATED / # END GENERATED block, strips indentation, and asserts byte equality with the source file. Tests cover all five go.sum lifecycle transitions: clean, changed-mod, changed-sum, created-sum (go.sum absent → tidy would create it), removed-sum (go.sum present → tidy would remove it), and tidy-tool-failure.

0229ba7 — "fix(go): isolate tidy check from workspaces"

Adds GOWORK=off as a per-command environment variable to the go mod tidy -modfile invocation. Without this, a go.work file in the consumer's repo would activate Go workspace mode, which ignores -modfile entirely — the alternate-copy approach would silently tidy the wrong file (the workspace-selected module, not the temp copy). The mock go binary in go-mod-tidy-check.test.sh now asserts [[ "${GOWORK:-}" == off ]] || exit 93, making any future regression an immediate test failure.


Status of all prior open findings

4. go mod tidy -diff requires Go 1.23+ — RESOLVED

go-quality.yml now embeds go-mod-tidy-check.sh under # BEGIN GENERATED. The go mod tidy -diff call is gone; go-quality.test.cjs explicitly asserts assert.doesNotMatch(workflow, /go mod tidy -diff/u) as a regression guard.

The -modfile flag requires Go 1.17 (released Aug 2021) rather than 1.23 — a meaningful improvement that covers all actively supported Go releases.

1–3 (from earlier rounds) — all RESOLVED ✅ (as confirmed in the prior review of commit 2a4f4e0)


Positive observations from these commits

  • GOWORK=off scope is surgical — applied only to the single go mod tidy invocation via the per-command env-var prefix, not exported globally. The surrounding cp, diff, and rm calls are unaffected.
  • Two-commit catch0229ba7 caught the workspace isolation gap in 4e289b8 promptly, and the test mock's new GOWORK=off assertion is the correct regression gate. The defense-in-depth is working.
  • compare_snapshot handles all go.sum lifecycle cases correctly — the elif [[ -f "$tidy" ]] branch catches the "go.sum absent, tidy would create it" transition (which go mod tidy -diff would also have caught but needed Go 1.23 to diagnose cleanly).
  • Symlink rejection[[ -f go.mod && ! -L go.mod ]] rejects symlinks before any file operations; since -f follows symlinks, the ! -L guard is necessary and correct.
  • alternate_sum naming"${alternate_mod%.mod}.sum" produces the exact path that go mod tidy -modfile will use for the companion sum file, so the cleanup and diff logic are correctly paired without any explicit path arithmetic.

Overall

All four tracked findings are resolved. No new issues identified in these commits. This PR is ready to merge.

@kyle-sexton
kyle-sexton marked this pull request as ready for review July 14, 2026 22:25
@kyle-sexton
kyle-sexton merged commit 9eb22ac into main Jul 14, 2026
33 checks passed
@kyle-sexton
kyle-sexton deleted the codex/repo-governance-hardening branch July 14, 2026 22:25
@claude

claude Bot commented Jul 14, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 6s —— View job


PR Review: ci: harden reusable workflow execution (final — merged at 0229ba7)

  • Gather context
  • Review changed files (final pass)
  • Post findings

This PR has already merged after five prior review rounds that drove findings 1–4 to resolution. This is an independent final-state review of the merged code.


Security and correctness findings

1. go-mod-tidy-check.sh creates temp files in the module root before registering trap

Minor.

go-mod-tidy-check.sh:18–23:

alternate_mod="$(mktemp .ci-tidy.XXXXXXXX.mod)"
alternate_sum="${alternate_mod%.mod}.sum"
cleanup() { rm -f -- "$alternate_mod" "$alternate_sum"; }
trap cleanup EXIT

The mktemp call creates the temp file in CWD (the consumer's module root) before trap cleanup EXIT is registered. If the process receives SIGKILL between those two lines — or if the script exits before the trap is set — .ci-tidy.*.mod persists in the workspace. A subsequent git diff --check or linting step that reads untracked files would observe it.

The safer pattern: declare the variable first, then register trap, then create the file:

alternate_mod=""
cleanup() { rm -f -- "${alternate_mod:-}" "${alternate_mod%.mod}.sum" 2>/dev/null; }
trap cleanup EXIT
alternate_mod="$(mktemp .ci-tidy.XXXXXXXX.mod)"

Alternatively, use $RUNNER_TEMP for the temp files to keep them outside the workspace entirely. In practice this only triggers on SIGKILL, which is uncommon, but it's a solvable inconsistency with the bounded_read / bounded_docker_read patterns elsewhere in this PR that use $RUNNER_TEMP.


2. install-release.sh sha256sum emits "OK" noise on success

Cosmetic.

install-release.sh:77:

printf '%s  %s\n' "$SHA256" "$asset" | sha256sum -c -

On success, sha256sum -c prints <filename>: OK to stdout. That string appears verbatim in Actions step logs. Using sha256sum --check --quiet --strict - suppresses the success line while still printing the failure message and returning a non-zero exit code. The --strict flag (also present in the golangci-lint inline install) is already in the description but not in the shared script. Consistent with the practice of having no extraneous output that could be mistaken for verification results.


Positive observations (independent of prior review)

  • govulncheck three-stage proofgo mod download -jsonjq sum/version check → go installgo version -m binary fingerprint is a complete authentication chain. Requiring GONOSUMDB: '' and GOPRIVATE: '' ensures the checksum DB is always consulted even in environments that set those variables. Clean.

  • --issues-exit-code=42 sentinel — cleanly disambiguates golangci-lint product findings (exit 42) from infrastructure failures (other non-zero). The classify case statement handles all three buckets. Reusing this exit code in both Linux bash and Windows PowerShell is consistent.

  • SARIF provenance schema validation — the jq -e filter in govulncheck-sarif-guard.sh validates tool name, version, db URL, scan level, scan mode, protocol version, result schema, and result level enumeration before touching any finding data. This ensures a future govulncheck SARIF format change surfaces as an infrastructure error (rerun) rather than a silent false-negative (missed vulnerability) or a false-positive (fabricated finding).

  • cat-file -e <sha>^{commit} post-fetch verification in fetch-immutable-workflow-pins.cjs — verifying the fetched object is typed as a commit (not a tree or blob) is a tight proof that can't be spoofed by a tag or non-commit object at that SHA.

  • GOWORK=off scoping in go-mod-tidy-check.sh — applied as a per-command env prefix, not exported, so it affects only the go mod tidy call and doesn't bleed into surrounding commands. The test mock's [[ "${GOWORK:-}" == off ]] || exit 93 assertion closes the regression loop.

  • Generated-sync guard pattern — the # BEGIN GENERATED / # END GENERATED blocks for both govulncheck-sarif-guard.sh and go-mod-tidy-check.sh in go-quality.yml are verified by go-quality.test.cjs at CI time. Any drift between the standalone script and the inlined copy fails the test suite before merge.

  • pulumi-version-drift.sh gh_read buffering — the fix from round 4 correctly mirrors the bounded_read pattern: stdout goes to mktemp "$RUNNER_TEMP/pulumi-gh-read.XXXXXX", only cat'd on success, removed in both paths. The find_active_incidents pipe through jq -ce and the active_incidents="$(find_active_incidents)" subshell are both correctly isolated from partial-output contamination.

  • compare_snapshot all-branches analysis — every code path that reaches case "$status" has assigned status=$? inside an else branch; all branches that get a 0 exit from diff take the early return 0. No uninitialized variable risk despite the local ... status declaration.


Overall

No new blockers. Finding 1 (temp file in module root) is the only item worth a follow-on cleanup; finding 2 is cosmetic. The core security posture — SHA-pinned installs, bounded retries with output isolation, govulncheck authentication chain, SARIF provenance validation, OIDC policy verification — is sound and well-tested. The PR was correct to merge.

kyle-sexton added a commit that referenced this pull request Jul 14, 2026
…contract

Lockstep repin: #95 made the cross-environment cache proofs
environment-invariant and #92 hardened the reusable execution, so the
canonical callers pin the merged commit 9eb22ac carrying both.

Part of melodic-software/github-iac#88.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D5wqdo4adEZmWgjpY9ZjVx
kyle-sexton added a commit that referenced this pull request Jul 22, 2026
…224)

Closes #217

## Related

- #209 / #212 (github-script port pattern this issue's fix depends on
for consumers)
- medley#1628 report Theme 1 (the exit-127 defect class that motivates
never routing a gh-CLI-dependent job self-hosted without porting it
first)
- medley#1632 (comment-review-gate.yml github-script port -- the
reviewed no-checkout consumer this change enables)

## Summary

- Adds `admits-comment-events` (workflow_call input, default `false`) to
`select-runner.yml`. When a caller sets it `true`, `issue_comment`,
`pull_request_review`, and `pull_request_review_comment` jobs become
eligible for local (self-hosted) routing, subject to every existing
policy/private-repo/fork gate.
- Fixes a second, independent event enumeration in the "Mint read-only
observer token" step (`prefer-self-hosted` policy path) that had its own
hardcoded event list and was otherwise unreachable for comment/review
events even with the flag set.

## Why (and the design this landed on)

Org GitHub Actions spending is capped at $0. The governed policy is
`self-hosted-only`, so private-repo required checks already run on the
fleet -- but comment/review-event jobs were categorically excluded from
local routing and fell back to GitHub-hosted runners, which now fail at
startup on a billing-limit error whenever free minutes are exhausted
(observed on medley's `comment-review-gate.yml`).

**Original-rationale finding:** `LOCAL_EVENT_ALLOWLIST` (git blame/log
through #92, #103, #123, #135) is a plain default-deny allowlist --
"only explicitly reviewed caller event classes may route locally" -- not
a comment-specific security ban. Comment/review events were simply never
reviewed, not deliberately blocked.

**Checkout-bearing consumer found, scoped out:** Auditing fleet
consumers surfaced medley's `claude-assistant.yml`, which triggers on
`issue_comment`/`pull_request_review_comment`, checks out the
repository, and runs Claude with `contents: write` and commit signing
via the same selector. Under the current `self-hosted-only` policy that
job is dormant (comment events route hosted, so its `route ==
'self-hosted'` gate never passes). A blanket "admit these event classes"
change -- the originally scoped approach -- would have made that dormant
job self-hosted-eligible as a side effect: comment-triggered checkout +
code execution + write perms on the fleet, without a security review of
that specific exposure.

Landed on a **per-caller opt-in** instead: `admits-comment-events`
defaults `false`, so `claude-assistant.yml` is untouched by this PR and
stays exactly as dormant as it is today. Its dormant assist job would
only ever go live by **its own deliberate future opt-in** -- a separate
decision requiring its own security review of checkout+write-on-fleet
exposure, not a side effect of this change. Recording this explicitly
here per the review discussion; will also leave a comment on #217 for
anyone who revisits `claude-assistant.yml`'s routing later.

`comment-review-gate.yml` (medley#1632) is the reviewed consumer this
opt-in is for: verified pure `gh api`/now `github-script` calls, zero
checkout anywhere in that workflow.

## Consumer follow-up (not in this PR)

medley pins `select-runner.yml` by full commit SHA (currently
`90f1c54935203fa31b5b3d1f41531228be2c2b7f # v0.6.1`). Once this merges
and cuts a tag, medley's normal repin process picks up the new SHA; only
*after* that repin can `comment-review-gate.yml` add
`admits-comment-events: true` to its `select-runner` call (the
pinned-SHA reusable doesn't know the input yet, so adding it any earlier
would be rejected as an unexpected input).

## Verification

- `node --test .github/scripts/select-runner.test.cjs` -- 112/112
passing, including new coverage: `admits-comment-events` true/false x
the three comment/review events x
`self-hosted-only`/`prefer-self-hosted`, plus a regression test proving
the flag does not widen an unrelated blocked event class
(`workflow_run`)
- `node --test .github/scripts/*.test.cjs` -- 247/247 passing repo-wide
- `actionlint .github/workflows/select-runner.yml` -- clean
- `node .github/scripts/render-select-runner-workflow.cjs --check` --
generated block in sync with `select-runner.cjs`

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant