fix: audit and port bare-gh CLI shell invocations to github-script - #212
Conversation
) The 2026-07-18 do-not-merge fleet spike (~191 failures, exit 127) traced to a `gh api` shell step in a reusable running on a caller-selected runner without the gh CLI; PR #144 fixed that one reusable and left the rest of the fleet unaudited. This audits all nine gh-calling reusables and ports every confirmed at-risk instance to actions/github-script (the #144 pattern), so a routing shift to a gh-less runner image can no longer fail these checks with exit 127. Ported (workflow_call + runs-on: ${{ inputs.runner }}, caller-selected): claude-review.yml, claude-security-review.yml, claude-e2e-verify.yml, link-check.yml, pulumi-version-drift-check.yml. link-check.yml's port also drops it from find-tracking-issue.sh's generated-block consumers, and pulumi-version-drift-check.yml's port retires pulumi-version-drift.sh and its generator/test now that the workflow no longer embeds a shell copy to keep in sync. Cleared (fixed ubuntu-24.04 runner, not caller-selected, gh guaranteed by the hosted image): queue-monitor-liveness.yml, tool-version-drift-check.yml, release.yml. do-not-merge-gate.yml's residual audit found no remaining shell gh calls — #144 already fully ported it. Adds a README triage note (check commit history and runner routing on a fleet-wide single-workflow spike before attributing it to infra flake) and a REVIEW.md "Always check" line codifying the github-script-or-guard requirement for CLI-shelling steps on caller-selected runners. Closes #209 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYvF6bWGqemS9aYFfWJRiW
PR #212 Review — porting bare
|
|
Claude finished @kyle-sexton's task in 5m 22s —— View job Security Review
SummaryThis PR ports FindingsSUGGESTION · Low confidence ·
|
| Area | Verdict |
|---|---|
pull_request_target / workflow_run tripwire |
Still present and unchanged in both claude-review.yml and claude-security-review.yml |
Script injection via ${{ github.* }} in run: blocks |
All new run: steps continue the safe pattern: PR-author-controlled values flow through env: into shell variables or process.env.*, never interpolated directly into the script body |
Script injection in github-script blocks |
All new github-script steps access event data through context.* and process.env.*, with zero ${{ }} interpolations inside any script: body — the canonical safe pattern |
| Action pins | All new uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 are full-SHA pins per CLAUDE.md policy |
| Permission grants | No permission widening; all modified jobs retain contents: read + pull-requests: write + id-token: write |
REVIEW_DETAIL / RUN_URL env interpolation |
Both passed through env: blocks to process.env.* in JS, not used in any execution path — safe |
| Token exposure | GH_TOKEN removed from shell env: blocks (old); github.token is now the implicit token for github-script via the action's own runtime — not logged |
Third-party data in pulumi-version-drift-check.yml |
Pulumi API response fields used in issue body are validated by SemVer regex before use; mutation calls (create/update/close) use typed Octokit parameters, not string-interpolated shell commands |
ISSUE_NUMBER / ISSUE_TYPE in link-check.yml |
ISSUE_NUMBER is coerced with Number() before use as an API parameter; ISSUE_TYPE is passed as a typed request body field — no injection surface |
Deleted scripts (pulumi-version-drift.sh, render-pulumi-version-drift.cjs) |
Removal reduces attack surface; the single JS implementation in the workflow is cleaner and has fewer injection surfaces than the shell original |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4dddb4dfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CI's comment-hygiene and biome lanes on PR #212 caught two house-style violations in the previous commit: three code comments carried a bare "issue #209" back-reference (only TODO(#issue) is a sanctioned tracker reference; commit/PR history owns the rest), and four .cjs test files had formatting biome's `ci --error-on-warnings` check rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYvF6bWGqemS9aYFfWJRiW
|
Claude finished @kyle-sexton's task in 4m 47s —— View job PR #212 Security Review
SummaryThis PR ports bare Items reviewed and cleared
Note on the Codex
|
|
Claude finished @kyle-sexton's task in 5m 5s —— View job PR #212 Review — porting bare
|
| Area | Verdict |
|---|---|
pull_request_target / workflow_run tripwires |
Present and unchanged in both claude-review.yml (line 192) and claude-security-review.yml (line 323) |
${{ }} interpolation inside script: bodies |
Zero instances across all 11 new github-script steps — all caller-controlled values go through env: → process.env.*, never interpolated into the script body |
| SHA pinning | All new uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 — consistent across all 5 files (11 call sites) |
| Permission grants | No widening; all modified jobs retain contents: read + pull-requests: write + id-token: write |
| Bounded reads / mutation semantics | boundedRead (retry once) wraps all reads; mutations not retried — matches the retired gh_read/gh_mutate semantics exactly |
latestTag URL usage in pulumi-version-drift-check.yml |
latestTag is used in the release URL only after latest (its v-stripped form) passes the SemVer regex, so the constraint is transitive and correct |
Label injection in link-check.yml |
Labels are passed as a typed array to the Octokit client, which handles URL encoding internally — no injection surface |
ISSUE_NUMBER / ISSUE_TYPE in link-check.yml |
ISSUE_NUMBER coerced with Number() before use as a typed API parameter; ISSUE_TYPE passed as a typed body field |
| Deleted scripts | pulumi-version-drift.sh, render-pulumi-version-drift.cjs, pulumi-version-drift.test.sh retired cleanly; ci.yml drops the now-gone test step; render-find-tracking-issue.cjs and find-tracking-issue-render.test.cjs updated to remove link-check.yml from the generated-block consumer list |
| Test coverage | Tests updated for all new API call patterns; structural invariants (github-script pin, no bare gh/jq in script bodies, bounded-read semantics, label-filter fail-closed) verified across link-check.test.cjs, pulumi-deploy-guard.test.cjs, network-timeout-policy.test.cjs, claude-review-superseded-guard.test.cjs, find-tracking-issue-render.test.cjs |
reopen + update sequencing in pulumi drift |
Bash gh issue reopen --comment was also two API calls under the hood; splitting them in JS is not a new atomicity regression |
The github-script port used .trim() to read .pulumi.version, which
strips all leading/trailing whitespace. The retired bash implementation
read the file via `$(<file)` command substitution, which strips only
trailing newline(s) and leaves any other whitespace intact for the
strict SemVer regex to reject. .trim() therefore silently accepted a
whitespace-padded reviewed version pin (leading space, trailing space,
a stray \r) that the prior implementation would fail closed on.
Replace .trim() with a trailing-newline-only strip that reproduces
$(<file) exactly, and add a runtime test proving the specific
padding cases the regex must still reject. Audited the other four
ported workflows for the same pattern: link-check.yml's label .trim()
is a different, correct case (mirrors the original jq
gsub("^\s+|\s+$"; "") per-label trim), not a validation bypass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KYvF6bWGqemS9aYFfWJRiW
|
Claude finished @kyle-sexton's task in 2m 52s —— View job PR #212 Review — porting bare
|
| Area | Verdict |
|---|---|
pull_request_target / workflow_run tripwires |
Present and unchanged in claude-review.yml, claude-security-review.yml, claude-e2e-verify.yml |
${{ }} interpolation inside script: bodies |
Zero instances across all 11 new github-script steps — all caller-controlled values route through env: → process.env.* |
| SHA pinning | All new uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 — consistent across all 5 files, 11 call sites; compliant with CLAUDE.md |
| Permission grants | No widening; all modified jobs retain contents: read + pull-requests: write + id-token: write; link-check.yml keeps issues: write; pulumi-version-drift-check.yml keeps contents: read + issues: write |
GH_TOKEN env removal |
Removed from all converted shell steps; github-script uses implicit job token which is never logged |
| Fork guard on comment steps | All three new comment-marker steps carry the head.repo.full_name == github.repository condition |
boundedRead / mutation semantics |
Single-retry wraps reads; mutations are not retried — matches the retired gh_read/gh_mutate bounded-read semantics exactly |
label.trim() in link-check.yml |
Correct normalization of caller-configured label input (mirrors original `jq gsub("^\s+ |
latestTag URL usage |
Constrained transitively by the latest SemVer regex before any downstream use |
ISSUE_NUMBER / ISSUE_TYPE in link-check.yml |
Number() coercion before typed API call; ISSUE_TYPE passed as a typed request body field |
findActiveIncidents paginate timeout |
request: { timeout } on paginated calls applies per-page; consistent with the original bash per-invocation timeout and appropriate for the scale of issue lists in practice |
Biome / comment hygiene (40fa0a7) |
Purely mechanical; CI-flagged violations corrected, no logic changes |
| Test coverage | pulumi-deploy-guard.test.cjs adds a standalone readVersion behavior test proving the exact whitespace-rejection cases, plus a doesNotMatch guard against .trim() regression; all five test files updated to match new API call patterns |
|
Claude finished @kyle-sexton's task in 4m 35s —— View job PR #212 Security Review
SummaryNo CRITICAL or IMPORTANT findings. The porting is secure: all caller-controlled values route through Resolved since prior reviews
const current = fs.readFileSync(versionFile, "utf8").replace(/\n+$/, "");This exactly mirrors bash command-substitution semantics (trailing newlines only) and the strict SemVer regex on line 98 rejects any remaining whitespace. Cleared. SUGGESTION ·
|
| Area | Verdict |
|---|---|
pull_request_target / workflow_run tripwires |
Present and unchanged in claude-review.yml (line 192) and claude-security-review.yml (line 323); claude-e2e-verify.yml uses the env-based ${EVENT_NAME} pattern (the safe form) |
${{ }} interpolation inside new script: bodies |
Zero — all 11 new github-script steps access caller data exclusively through context.* and process.env.* |
${{ }} interpolation inside new run: blocks |
Zero new run: steps with expression interpolation; PATHS, EVENT_NAME, FILES_LIST_PATH, FILES_LIST_FAILED all go through env: |
actions/github-script SHA pins |
All uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 — full 40-char SHA, consistent across all 5 files and 11 call sites |
GITHUB_TOKEN / token handling |
GH_TOKEN: ${{ github.token }} removed from the ported shell steps; github-script uses the implicit job token, which is never logged |
| Permission grants | No permission widening: all modified jobs retain their prior grants (contents: read + pull-requests: write + id-token: write, plus issues: write where applicable) |
REVIEW_DETAIL env variable |
Set from jq -c '{subtype, is_error, num_turns, duration_ms, total_cost_usd}' output — structured metadata only, no model-authored text; used as process.env.REVIEW_DETAIL in a JS template literal, not in a shell execution context |
PATHS input in claude-security-review.yml |
Multiline string flows through env: block; processed per-line with printf '%s\n' "$PATHS" | while IFS= read -r pattern; written to .gitignore file, not executed; no shell injection surface |
Changed-files listing (file.previous_filename) |
GitHub path names cannot contain newlines; filenames are written one-per-line to a temp file and consumed by git check-ignore --stdin as paths, not patterns — !-prefixed filenames treated as literal paths, not negations |
Label injection in link-check.yml |
Labels passed as a typed array to Octokit; ISSUE_LABELS split and filtered before use, not interpolated into a URL or shell command |
ISSUE_NUMBER / ISSUE_TYPE in link-check.yml |
ISSUE_NUMBER coerced with Number() before use as a typed API parameter; ISSUE_TYPE passed as a typed request body field |
| Fork guard on marker-comment steps | All three new comment steps carry github.event.pull_request.head.repo.full_name == github.repository guard — fork-triggered runs skip those steps correctly |
comment.user?.login === "github-actions[bot]" filter |
Double check (bot login + marker prefix) prevents automation from targeting comments from other users; marker strings are specific enough to avoid accidental collision |
| Deleted scripts | pulumi-version-drift.sh, render-pulumi-version-drift.cjs, pulumi-version-drift.test.sh removed — reduces attack surface; the single JS implementation has a narrower injection profile than the retired shell original |
…se onto #212) ci-workflows#212 (merged during this branch's lifetime) ported link-check.yml's gh-CLI-based tracking-issue lookup/close steps to actions/github-script, because it runs on a caller-selected runner where the gh CLI is not guaranteed present, and dropped it from the shared find-tracking-issue.sh codegen consumers accordingly. standards-sync-stuck-automerge-alert.yml is the same shape (workflow_call, runs-on: inputs.runner), so it needed the same port rather than joining the shared bash consumer list as originally written. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KYvF6bWGqemS9aYFfWJRiW
…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)
1 participant
Closes #209
Summary
The 2026-07-18
do-not-mergefleet spike (~191 failures across 6 repos,exit 127: gh: command not found) traced to agh apishell step in a reusable workflow running on a caller-selected runner, exposed when routing shifted callers onto a self-hosted image that does not ship the gh CLI. PR #144 fixed that one reusable (do-not-merge-gate.yml) and named four sibling reusables still out of scope; this issue expanded the sweep to all nine gh-calling reusables in the fleet and ports every confirmed at-risk instance toactions/github-script(the #144 pattern — bundled Node runtime, no runner-image tooling dependency).Per-reusable audit verdict
runs-onclaude-review.yml${{ inputs.runner }}claude-security-review.yml${{ inputs.runner }}changesjob's PR-file listing (split into a github-script fetch + unchangedgit check-ignorematching), freshness check, both comment-marker stepsclaude-e2e-verify.yml${{ inputs.runner }}gh pr diff/gh pr commenttext is agent-environment usage (the Claude agent's own tool-use, not a workflowrun:step) — out of scope by the same principle decision 1 applied toclaude-review.yml's prompt text, just not previously spelled out for this filelink-check.yml${{ inputs.runner }}find-tracking-issue.sh's resolver logic — dropped from that generated block's consumer list since only a fixed-runner consumer still needs the shared bash source), "Assert native issue type" (rawPATCHrequest — thetypefield isn't guaranteed on every octokit release's typedissues.update), "Close recovered tracking issue"pulumi-version-drift-check.yml${{ inputs.runner }}gh_read/gh_mutatebounded-read-with-single-retry semantics in JS. Retirespulumi-version-drift.sh,render-pulumi-version-drift.cjs, andpulumi-version-drift.test.sh— the workflow no longer embeds a generated shell copy to keep in syncqueue-monitor-liveness.ymlubuntu-24.04(hardcoded)workflow_call— a standalone scheduled workflow, always on the fixed GitHub-hosted image, which ships gh. Not caller-selectabletool-version-drift-check.ymlubuntu-24.04(hardcoded)release.ymlubuntu-24.04(hardcoded)workflow_dispatch-only, notworkflow_call; same reasoningdo-not-merge-gate.yml${{ inputs.runner }}ghcall per the issue's "(residual)" flag — none found; #144 already fully ported this fileOther changes
pulumi-version-drift-check.ymlbullet reflecting the github-script implementation.actions/github-scriptor carry a justified guard, citingconventions/review/cross-platform.md#tools-processes-and-committed-artifacts.Verification
node --test .github/scripts/*.test.cjs— 209/209 passing, including rewritten assertions for the ported steps (link-check.test.cjs,pulumi-deploy-guard.test.cjs,network-timeout-policy.test.cjs,find-tracking-issue-render.test.cjs,claude-review-superseded-guard.test.cjs)bash .github/scripts/find-tracking-issue.test.sh— passing (unchanged shared source, now consumed only by the two fixed-runner workflows)actionlint— clean on all five ported workflowsshellcheck— clean on the remaining bash sourceszizmor— identical finding count before and after (18 findings: 1 low, 3 medium, 1 high, all pre-existing and unrelated to this change — verified by diffing against theorigin/mainversion of each ported file)markdownlint-cli2— clean on README.md and REVIEW.mdRelated