Skip to content

chore(ci): stop gh rejecting --slurp with --jq in PR image publish - #146

Merged
joryirving merged 2 commits into
mainfrom
courier/misospace/pinchflat/issue-145
Oct 11, 2026
Merged

joryirving merged 2 commits into
mainfrom
courier/misospace/pinchflat/issue-145

Conversation

@itsmiso-ai

@itsmiso-ai itsmiso-ai commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

What

Fixes #145. Two commits:

  1. b39173a — the Publish PR Image workflow's Push PR Image to GHCR job failed repeatedly on the default branch. The Check for image artifact step combined gh api --paginate --slurp with gh's own --jq flag, which gh rejects at runtime:

    the --slurp option is not supported with --jq or --template

    The fix pipes the --slurp-ed output to an external jq process, keeping the intended filter byte-for-byte. This is the same pattern the sibling Resolve trusted PR identity step already uses.

  2. bedf622 — unblocks the required Lint and Test gate, which turned red on this branch through no fault of the diff: mix hex.audit began failing on advisory EEF-CVE-2026-97853 (decimal >= 0.1.0, < 3.1.2, "Unbounded allocation in Decimal.round/3", published 2026-10-10T19:43Z — ~25 min after main's green run at 19:16Z and ~15h before this PR's red run). Locked decimal 3.1.1 is inside the range, so the failure is deterministic and external-advisory drift, not the workflow diff. Bumps mix.lock decimal 3.1.1 → 3.2.0 (newest version satisfying every consumer constraint; also clears the two older < 3.0.0 decimal advisories).

Why the workflow fix is correct

From gh's source (pkg/cmd/api/api.go), --jq (FilterOutput) is applied to each page individually during --paginate. So:

  • --paginate --jq (no --slurp) would emit one count per page, not a total.
  • --paginate --slurp + gh's --jq — the combination that was there — is the exact case gh errors on.

--paginate --slurp wraps all pages into one outer JSON array; feeding that to an external jq produces a single total across every page, which is what the author's original filter ([.[] | .artifacts[] | select(.name == "pr-image")] | length) was written for. The /actions/runs/{id}/artifacts endpoint returns an object per page ({total_count, artifacts:[...]}), so the filter traverses .[].artifacts[]; the sibling step counts over /pulls, which returns an array per page, hence its .[][] — the two differ on purpose.

Why the decimal lock line is trustworthy without a Mix toolchain

No Elixir toolchain exists in this lane, so the lock line was derived and cryptographically verified rather than generated by mix deps.update:

  • Trailing checksum 190bca80… is hex.pm's authoritative release checksum (/api/packages/decimal/releases/3.2.0).
  • 4th field f1235f05… is the canonical inner checksum recomputed from decimal-3.2.0.tar per hexpm/hex mix_hex_tarball.erl (sha256 over VERSION ++ metadata.config ++ contents.tar.gz), matching the tarball's own CHECKSUM member.
  • The method was validated by exact reproduction of the locked decimal 3.1.1 and bunt 1.0.0 entries, and independently recomputed by the review pass from fresh tarball fetches.
  • metadata.config confirms build_tools [mix] and empty requirements → lock fields [:mix], []. ecto/ecto_sql/ecto_sqlite3 require ~> 3.0; jason's optional ~> 1.0 or ~> 2.0 or ~> 3.0 — all permit 3.2.0; no upper bound is violated.

Invariants kept, and the other paths that enforce each

  • Never combine --paginate --slurp with gh's own --jq/--template — aggregate in an external jq. Enforced by this step and by Resolve trusted PR identity (~line 103). Kept: the sibling is already correct and is left untouched; a grep of .github/workflows/ confirms these are the only --slurp uses and no --paginate gh call anywhere combines --slurp with --jq/--template.
  • The existence check counts pr-image across every page, not just the first 30. Kept: --paginate --slurp still wraps every page in an outer array and the external jq reduces over all page-objects, so count is a single overall integer (verified with a multi-page / no-match / empty simulation → 1 / 0 / 0, matching the exists=true / exists=false branches).
  • Fail-closed: a failed gh/jq aborts the step rather than silently reporting exists=false. Kept: the step runs under set -euo pipefail; with pipefail a failed gh in the pipeline exits non-zero and set -e stops the step. A genuine no-match emits 0 (never empty output), so only a real zero count reaches exists=false.
  • Trust model (issue [P1] Block shell injection through PR-image artifact metadata #139): no untrusted artifact byte reaches shell source; validated values reach run blocks only via env vars with quoted expansions. Kept: the changed line interpolates only ${REPOSITORY} / ${RUN_ID} (trusted github.repository / github.event.workflow_run.id run context) into the URL argument, double-quoted; the jq program is a single-quoted literal; artifact JSON enters only via stdin and leaves only as an integer. The gating script (tooling/pr_image_metadata.sh) and the download-to-$RUNNER_TEMP isolation are untouched.
  • Downstream gating is unchanged. Kept: steps.check.outputs.exists still gates every later step; the trusted-identity resolution, metadata validation, push, and PR comment are all unchanged.
  • mix hex.audit passes with only the two documented Cowlib exceptions (HEX_IGNORE_ADVISORIES=GHSA-w4f7-4cxr-rv3c,GHSA-g2wm-735q-3f56 in tooling/.check.exs; the rationale for that exception lives there). Kept: the decimal bump moves to 3.2.0, outside EEF-CVE-2026-97853 (< 3.1.2) and the two older < 3.0.0 decimal advisories; no new ignore entries were added. The only other mix.lock-wide audit path is mix deps.audit (Mix advisory DB), which does not yet carry the decimal advisory; the bump satisfies both regardless.
  • mix.lock stays exactly what mix deps.update decimal would produce (single-line change, canonical checksums, [:mix]/[] from metadata.config). Kept: verified as described above; no other dependency line moved.

jq is preinstalled on ubuntu-latest, so the external binary is available where the workflow runs.

Local validation run

  • yarn run lint:check (Prettier — the CI gate that covers .github/workflows/*.yml): exit 0 on head bedf622.
  • bash -n on the extracted Check for image artifact script: syntax OK.
  • Lock line: hex inner/outer checksums recomputed independently from the published tarballs by a review pass and matched the lock byte-for-byte; method validated against two untouched lock entries (decimal 3.1.1 pre-bump, bunt 1.0.0).
  • Independent review of the final diff at bedf622: verdict SHIP, no findings.
  • Not runnable in this lane: mix check / mix test / mix hex.audit — no Elixir, Erlang, or Docker in any environment available here. CI is the first real execution of mix deps.get against the new lock line; that verification is pending, and I am not claiming CI green.

Still to verify externally

  • Lint and Test on bedf622: expected green (decimal leaves the advisory range; everything else is unchanged from main's green e9b39a6), pending CI execution — deps.get checksum validation of the constructed lock line is the one thing only Mix can confirm.
  • This workflow fires via workflow_run and executes only on the default branch, so the fixed step cannot run from a PR branch. The green Push PR Image to GHCR run is confirmed after merge.

Head: bedf622

The Publish PR Image workflow's "Check for image artifact" step combined
gh's own --jq flag with --paginate --slurp, which gh rejects at runtime
("the --slurp option is not supported with --jq or --template"), failing
the job on runs that produce a pr-image artifact.

Pipe the slurped output to an external jq instead, preserving the exact
intended filter. This matches the sibling "Resolve trusted PR identity"
step, which already pipes --paginate --slurp output to jq. gh applies its
own --jq per page during --paginate, so the external jq (over the slurped
outer array of page objects) is also what makes the count cover every
page rather than only the last one.

Fixes #145
its-saffron[bot]

This comment was marked as outdated.

mix hex.audit fails deterministically on head: decimal 3.1.1 falls inside
the vulnerable range (>= 0.1.0, < 3.1.2) of advisory EEF-CVE-2026-97853
(Unbounded allocation in Decimal.round/3, published 2026-10-10T19:43Z),
which turned the required Lint and Test gate red ~15h after main's green
run. 3.2.0 is the newest version satisfying every consumer constraint
(ecto/ecto_sql/ecto_sqlite3 require ~> 3.0).

No Mix toolchain exists in this lane, so the lock line was derived from
the published decimal-3.2.0.tar: the trailing checksum is hex.pm's
authoritative release checksum (190bca80), the fourth field is the
canonical inner checksum recomputed from the tarball (sha256 over
VERSION, metadata.config and contents.tar.gz per hexpm/hex
mix_hex_tarball.erl) and equals the tarball's own CHECKSUM member. The
method was validated by exact reproduction of the locked decimal 3.1.1
and bunt 1.0.0 entries. metadata.config confirms no deps, build_tools
[mix].
@itsmiso-ai

Copy link
Copy Markdown
Contributor Author

Response to the AI review's single blocker ("Lint and Test failed on b39173a, no logs in corpus") — diagnosed and resolved in bedf622.

Root cause, with evidence:

  • Main (e9b39a6) was green on Lint and Test at 2026-10-10T19:16Z; this branch turned red at 2026-10-11T10:17Z in the Run code checks step (~31s, so an early mix check stage, not ex_unit), with the diff touching only workflow YAML and Prettier passing locally.
  • hexpm published advisory EEF-CVE-2026-97853 (decimal >= 0.1.0, < 3.1.2, "Unbounded allocation in Decimal.round/3") at 2026-10-10T19:43:45Z — ~25 minutes after main's green run. Locked decimal 3.1.1 is inside the range, so mix hex.audit began failing deterministically for every run from that moment, on any branch. Reproducible via https://hex.pm/api/packages/decimal (security_advisories).
  • Not the two Cowlib exceptions: HEX_IGNORE_ADVISORIES in tooling/.check.exs already covers those, and they matched before main's green run too.

Fix: chore(deps): update dependency decimal to v3.2.0 (newest version satisfying all consumer constraints; clears all three decimal advisories). No entries were added to HEX_IGNORE_ADVISORIES — the fix is the patched version, not a wider ignore. Because no Elixir toolchain exists in this lane, the single mix.lock line was derived from the published decimal-3.2.0.tar and cryptographically verified (canonical inner checksum per hexpm/hex mix_hex_tarball.erl, matching the tarball's own CHECKSUM member; method validated by exact reproduction of untouched lock entries decimal 3.1.1 and bunt 1.0.0, then independently recomputed by a review pass). PR body updated to describe the new scope, invariants, and the checks that could not run locally (mix check / mix test / mix hex.audit), so Lint and Test on bedf622 is honestly pending, not claimed green.

@its-saffron
its-saffron Bot dismissed their stale review October 11, 2026 11:13

Superseded by a newer automated review for this pull request.

@its-saffron its-saffron Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Automated Review

Full PR review.

Analysis engine: glm-5.3-flash (openai) — routed smart (risk match: linked_priority_p1) · pr-reviewer-action v3.2.0

Partial coverage: required-check coverage is incomplete — this review did not resolve every required check and must not be read as a complete pass.

Findings (1 info)

Severity Location Finding
Info .github/workflows/pr-image-publish.yml:51 The new comment's claim that gh would apply --jq per page during --paginate is consistent with gh's flag documentation but was not directly verified against gh's source in this review; the rejection itself is confirmed by the runtime error in issue PR 145.

Recommendation: Approve

Both changes are correct, minimal, and verified. The workflow fix resolves the exact runtime failure captured in issue PR 145 (gh rejecting --slurp combined with its own --jq), preserves the intended filter byte-for-byte, and keeps the step fail-closed. The decimal 3.1.1 → 3.2.0 lock bump clears the newly published advisory, satisfies every consumer constraint in mix.lock/mix.exs, and is validated end-to-end by a green Lint and Test run on the head commit (which includes mix deps.get, mix hex.audit, and the full ex_unit suite).

Change-by-change findings

1. .github/workflows/pr-image-publish.yml — Check for image artifact step

The change replaces gh's own --jq with a pipe to an external jq, keeping the filter [.[] | .artifacts[] | select(.name == "pr-image")] | length identical. Verified against the full workflow file:

  • The step still runs under set -euo pipefail; with pipefail, a failed gh or jq in the pipeline makes the command substitution (and thus the step) exit non-zero, so the fail-closed property is preserved.
  • The sibling Resolve trusted PR identity step (~line 105) already used exactly this pattern (gh api --paginate --slurp … | jq --arg repo … '[.[][] | …]') and is untouched, as the PR states.
  • A grep of .github/workflows/ for --slurp|--paginate|--jq|--template returns matches only in this file (lines 49, 51, 53, 105): the two --slurp call sites both pipe to external jq, and no --paginate gh invocation anywhere combines --slurp with --jq/--template. The PR's grep assertion holds.
  • The jq filter is semantically correct for this endpoint: /actions/runs/{id}/artifacts returns a page object ({total_count, artifacts:[…]}) per page, --slurp wraps all pages into one outer array (confirmed by gh's own flag help captured in the issue log: "return an array of all pages of either JSON arrays or objects"), so .[] | .artifacts[] reduces over every artifact on every page and length yields one total. The sibling step's .[][] differs because /pulls returns an array per page — the asymmetry is intentional and correct.

Claim scorecard:

  • Claim 1 (gh applies --jq per page during --paginate): partially verified. The load-bearing half — gh rejects --slurp with --jq/--template — is confirmed by the runtime error captured in issue PR 145's log. The per-page application detail was not directly observed: the fetched cli/cli pkg/cmd/api/api.go blob (both via the contents API and the git-blobs API) was truncated before the paginate loop, though the ApiOptions struct with Slurp/FilterOutput fields and the flag help text were seen. This detail appears only in an explanatory comment and does not affect the fix's correctness.
  • Claim 2 (--paginate --slurp + external jq yields a single total across pages): held, 3/3 workflow items. The fourth listed item (mix.lock:L12) is the decimal lock line, not a call site of this filter — an extraction artifact, not a counterexample.
  • Claim 3 (no --paginate --slurp combined with gh's --jq/--template anywhere in .github/workflows/; sibling already correct): held, 3/3 items — independently reproduced by grep.
  • Claim 4 (count covers every page, not just the first 30): held for all workflow items; mix.lock:L12 is again not a call site of the count logic. The multi-page/no-match/empty simulations described in the PR body match the filter's semantics as read.
  • Claim 5 (fail-closed under set -euo pipefail): held, 4/4 items — verified in the file; mix.lock:L12 is not a shell call site.

2. mix.lock — decimal 3.1.1 → 3.2.0

  • Consumer constraints: verified directly in mix.lock — ecto ~> 3.0, ecto_sql ~> 3.0, ecto_sqlite3 ~> 3.0, and jason's optional ~> 1.0 or ~> 2.0 or ~> 3.0 all permit 3.2.0. mix.exs pins {:decimal, "~> 3.0", override: true}, also satisfied. No upper bound is violated.
  • Release existence and content: the v3.2.0 tag exists on ericmj/decimal (published 2026-10-10T21:14:30Z) and its notes include multiple Decimal.round/3 fixes, consistent with the advisory's subject ("Unbounded allocation in Decimal.round/3", fixed in ≥ 3.1.2 per the quoted range; 3.2.0 > 3.1.2).
  • Lock-line integrity: I could not independently recompute the hex.pm checksums in this lane, but this is covered by stronger evidence: CI's Lint and Test on the head commit succeeded, and its mix deps.get step verifies the fetched tarball against the lock's outer checksum — a fabricated or corrupted lock line would have failed there, before mix check even ran.
  • Behavioral changes in 3.2.0: the release notes list real behavior changes (subnormal/:clamped signaling, round/3 now raising Decimal.Error in some cases, compare/2 raising on NaN). These are mitigated here: per the mix.exs comment, no schema or migration uses the :decimal type, and the full test suite passed on the head commit.

Standards Compliance

  • The PR does not touch Elixir source, schemas, workers, queues, or migrations, so the aliasing, module-naming, queue-name, and @allowed_fields conventions are not engaged.
  • Trace-scope requirement req-cec06a117f01 (never assume the checkout lives at a particular path): not_applicable — the diff adds no fixtures or tests and no absolute paths; the workflow change uses env-var interpolation only. See requirement_coverage.
  • CI convention compliance: the required Lint and Test gate is green on the head commit per the authoritative CI status results.
  • No documented behaviour in AGENTS.md/CODEBASE.md/DEVELOPMENT.md is invalidated, so no doc updates are required.

Linked Issue Fit

Issue PR 145 reports Publish PR Image failing repeatedly on the default branch with exactly the --slurp option is not supported with --jq or --template in the Check for image artifact step. The PR fixes precisely that step and nothing else in the workflow, preserving the pagination/slurp intent stated in the issue's own inline comment. The second commit addresses the downstream consequence the issue's fix exposed (the required gate turning red on an external advisory), which the PR body documents with timestamps. The issue's acceptance criterion — the workflow no longer failing on this error — is directly satisfied by the diff, and the head commit's CI is green.

Tool Harness Findings

  • read_file on .github/workflows/pr-image-publish.yml returned the full file, confirming the new pipeline form, the untouched sibling step, and set -euo pipefail.
  • git_grep over .github/workflows confirmed the only --slurp/--paginate/--jq/--template occurrences are the two call sites in this workflow, both piping to external jq.
  • git_grep over mix.lock and read_file on mix.exs confirmed all decimal consumer constraints permit 3.2.0 and the override: true pin is ~> 3.0.
  • web_fetch of the decimal v3.2.0 GitHub release confirmed the release exists, its publish timestamp, and its round/3-focused fix list.
  • Attempts to read cli/cli's api.go paginate loop were truncated in both fetch paths, and several gh_api calls failed with transport errors (code search, check-runs, PR files/commits); none of these blocked the verdict, since the rejection is proven by the issue's runtime log and the lock line by CI.

Unknowns or Needs Verification

  • The claim that gh's --jq is applied per page during --paginate (Claim 1's second half) was not directly observed in the gh source fetched here; the fetched blob was truncated before the paginate loop. It is consistent with gh's documented flag behavior and is immaterial to the fix, but it remains an unverified explanatory detail in the new comment.
  • The hex.pm checksums in the new mix.lock line were not independently recomputed in this review; they are validated indirectly by the green CI run, whose mix deps.get step fails on any checksum mismatch.
  • The advisory EEF-CVE-2026-97853's exact text was taken from the PR body/comment (hex.pm API fetch was not reachable in this lane); the version range quoted is internally consistent with the bump chosen.

Requirement trace

1 of 5 requirement(s) not fully traced to enforcement and a test:

  • req-cec06a117f01 — unverifiable (no valid enforcement location)

Approval withheld: this review's coverage is incomplete — required-check coverage or the tool-loop investigation did not finish, so it is publishing as an advisory comment rather than an approval.

@joryirving
joryirving merged commit 94a8223 into main Oct 11, 2026
4 checks passed
@joryirving
joryirving deleted the courier/misospace/pinchflat/issue-145 branch October 11, 2026 19:03
@its-miso its-miso Bot mentioned this pull request Oct 10, 2026
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.

CI: Publish PR Image failing on the default branch (Push PR Image to GHCR)

2 participants