A release pull request proves its release builds, before the tag exists - #19
Conversation
release.yml's "WHAT IS AND IS NOT VERIFIED" block claimed "NOT run: this workflow. No dispatch has happened, in either mode." Two have, both dry, both green: ingot on ubuntu-24.04 (run 35762662020) and piri on macos-14 (run 35765210724). That block is the file's rule-5 claim about itself, so it being wrong in the direction of understatement still makes it a claim nobody can trust. Between them the two runs cover more than "it ran": the PyYAML derivation routed piri to macos-14 and ingot to ubuntu-24.04, the skip list kept goreleaser off docker on a runner that has a daemon, the assertion passed for both, and `require the tag` and `publish` both skipped. What is still unverified is now stated as three specific things rather than "this workflow". The piri entry's "Not measurable without dispatching it" is gone with it -- 8m47s, measured. The number moves to `timeout-minutes`, which is the decision it changes: 40 minutes for a job that takes nine. It is also what a release pull request will cost, which is the next thing built on top of this file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
release.yml was dispatch-only, so the only thing that ever proved a service could be released was someone remembering to dispatch it. Two dry runs did that yesterday; nothing keeps them true. This makes a pull request from a `release/<svc>` branch build that service and assert its version stamping, publishing nothing -- so the release path is checked continuously instead of when recalled. The selector is the branch name, and the alternative is worth recording. Firing on a change to `<svc>/version.json` looks more derived and is worse: a subtree resync carries upstream's own version bumps, so an unrelated pull request would pay for up to four release builds, piri's alone nine minutes. Narrowing it to "version bumps and nothing else" then excludes a release pull request that also edits a changelog. A branch name states the intent, which is what is actually being detected. It leaves a gap -- bump a version anywhere else and nothing here looks -- and that gap is where every service already is. The trap this had to avoid is `inputs`. It does not exist on a pull_request event, so `inputs.dry_run` is null there and `!inputs.dry_run` -- how both `if:`s were spelled -- evaluates TRUE. Unchanged, a pull request would have run `require the tag` against a tag that by design does not exist yet, and passed GITHUB_TOKEN to goreleaser on a build a fork can trigger by naming its branch `release/piri`. The mode is now resolved once in `plan` and read from there; no `inputs.dry_run` survives in the release job. The permissions comment said there was no fork-pull-request case to protect, which this is precisely what stops being true, so it says what holds it instead. New check, `plan` only: the version must NOT already be tagged, the mirror of the release job's `require the tag`. It cannot fail today -- `git ls-remote --tags origin` is empty -- and the comment says so rather than leaving a green check to imply otherwise. Verified both directions against a scratch tag. A local clone disagrees misleadingly, carrying v0.0.0, v0.2.4 and v1.13.4 from the upstream remotes; none is on origin. Compat on a release pull request is deliberately NOT here. It wants compat.yml, which is still open in #18, and wiring a workflow under review from another branch is how both get harder to review. It follows once #18 lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja
left a comment
There was a problem hiding this comment.
[From Claude:]
The gating is correct. No pull request path reaches require the tag, publish, or a GITHUB_TOKEN, and that is observed rather than reasoned — run 35773432670 is a pull_request on release/ingot at this exact head, and both steps report skipped. Every number in the header checks out against the run API. Five findings, none of which breaks the mechanism: one is the header's own verification block, two are residual exposure the file states incompletely, one is the concurrency keying, and one is outside the diff.
1. The "WHAT IS AND IS NOT VERIFIED" block cites two runs that executed none of this diff, and omits the two that did.
Lines 42-52 name runs 35762662020 and 35765210724. Both are workflow_dispatch, at shas whose release.yml differs from this head by the entire change:
$ git diff --stat 983c9d4c HEAD -- .github/workflows/release.yml # run 35762662020
1 file changed, 173 insertions(+), 26 deletions(-)
$ git diff --stat 699f929f HEAD -- .github/workflows/release.yml # run 35765210724
1 file changed, 173 insertions(+), 26 deletions(-)
So when line 45 offers those runs as covering "require the tag and publish both skipping", what skipped in them was if: ${{ !inputs.dry_run }} — the spelling this PR deletes. The new needs.plan.outputs.dry_run != 'true' at lines 453 and 532 was never evaluated in either run.
It has been evaluated, twice, at b9fa2298:
| run | event / branch | plan |
release |
|---|---|---|---|
| 35773432670 | pull_request / release/ingot |
success, 9s | success, ubuntu-24.04, 2m54s — require the tag skipped, publish skipped, assertion passed |
| 35773389770 | pull_request / claude/release-pr-gate |
success, 16s | skipped |
Neither appears in the block, and the NOT run: list at line 50 does not disclaim the pull_request path either — so the block neither claims the coverage it has nor says it is missing. A reader at this head is left with two run IDs that are evidence about a file that no longer exists in this form.
This is the same fault the parent commit 9287fbe8 ("The dry runs happened; the header still said they had not") was written to fix, one version later. The PR body has the right table; the file is where it has to live, since the file is what survives the merge.
2. A fork pull request can now put a macos-14 runner under its own build config; "Three things hold it" covers the token, not the compute.
Line 384's three mitigations all hold, and I traced each: the token is read-only for a fork PR, dry_run is hard-coded true at line 181 before any other output write, and line 495 resolves to '' in that mode. They are all about secrets and publishing.
What they do not cover is that the runner class is chosen from the fork's tree. plan checks out the merge ref, then derives the runner by parsing <svc>/.goreleaser.yaml (line 239). A fork PR on branch release/ingot that also edits ingot/.goreleaser.yaml to declare a darwin build with CGO_ENABLED=1 resolves runner=macos-14 (line 241), and then runs its own goreleaser config — before: hooks included — for up to timeout-minutes: 40.
fil-forge/forge is "visibility": "public" with "allow_forking": true, and this is the only workflow in the repo that can reach a non-ubuntu runner:
$ grep -rn macos .github/workflows/ | grep -v '^\.github/workflows/release\.yml'
(no output)
$ grep -n 'runs-on' .github/workflows/{ci,e2e,images,itest}.yml
ci.yml:52: runs-on: ubuntu-latest
ci.yml:105: runs-on: ubuntu-latest
e2e.yml:63: runs-on: ubuntu-24.04
images.yml:74: runs-on: ubuntu-24.04
itest.yml:70: runs-on: ubuntu-24.04
macOS minutes bill at 10x, and GitHub's default first-time-contributor approval covers a stranger's first pull request, not a contributor's second. The plan parse itself is fine — yaml.safe_load, and the macOS-plus-docker refusal at line 262 still applies. This is a sentence in that block rather than a code change, but the block's stated job is to say what the remaining exposure is, and this is the one thing the trigger newly grants that no other workflow does.
3. `dry_run` is fail-open on anything that is not the literal string `true`, and nothing validates it.
All three consumers spell it != 'true' (lines 453, 495, 532). On a pull request the value is hard-coded at line 181, so pull requests are safe — that part is airtight. The residual is the dispatch path:
line 162: DRY_RUN: ${{ inputs.dry_run }}
line 178: dry_run=$DRY_RUN
line 204: echo "dry_run=$dry_run" >> "$GITHUB_OUTPUT"
set -u does not fire on an empty inputs.dry_run, because the env key exists and is empty. An empty value is written verbatim and then resolves to real-release mode at all three reads — the wrong direction for a flag described as "Leave on unless you mean it."
Today this is latent rather than live, and worth saying so: require the tag runs before goreleaser and fails, because the tag namespace is empty (the tags API returns [], confirming line 337). So nothing dangerous happens; the run just goes red for the wrong reason. It stops being latent the day the first tag exists. Reachability is low — GitHub applies the declared default: true — so this is about polarity, not a live bug.
The contrast is what makes it worth closing: resolve the version rejects a version.json value outside [0-9A-Za-z.+-] on the grounds that "this value reaches both shell and GITHUB_OUTPUT", while the flag that gates the token accepts any string at all. Two lines after 178:
case "$dry_run" in true|false) ;; *) echo "::error::dry_run is '$dry_run'"; exit 1 ;; esac4. The concurrency keying lets a pull request cancel an in-flight dispatch, which the comment above it says must not happen.
line 104: group: ${{ github.workflow }}-${{ inputs.service || github.head_ref }}
line 105: cancel-in-progress: ${{ github.event_name == 'pull_request' }}
A dispatch of ingot gets release-ingot. A pull request whose head branch is named ingot also gets release-ingot — a release pull request is release-release/ingot, so the collision needs a branch named exactly after a service, which in a ten-service monorepo is not exotic. cancel-in-progress is a property of the run joining the group, not of the one already running, so the pull request run cancels the dispatch.
Lines 100-103 say superseding is "right for a pull request ... and wrong for a dispatch, which someone is waiting on". The keying does not deliver the second half. Same-named branches on a fork and on origin collide with each other too, though there both are pull requests and the loser simply re-runs.
Folding the event into the group (${{ github.workflow }}-${{ github.event_name }}-${{ inputs.service || github.head_ref }}) separates the two namespaces without changing either behaviour.
5. Outside the diff: AGENTS.md still says this workflow is not part of what a change costs.
AGENTS.md lines 194-197:
CI is four workflows —
ci(per-module build/vet/staticcheck/tidy/test plus theguardsjob),images,e2e,itest. A fifth,release, exists but is dispatch-only and never runs on a push, so it is not part of what a change costs.
After this PR it is not dispatch-only, and it is part of what every change costs — measured on this pull request's own run 35773389770: plan 16s wall, 11s of which is the new fetch-depth: 0 checkout. "Never runs on a push" survives literally, since pull_request is not push; "dispatch-only" and the cost clause do not.
The diff is one file and AGENTS.md is untouched. It is the file that tells the next session what a change costs, and it now understates the check list by one.
Verification: what I checked, and what checked out.
Every header number, against the run API. All correct.
| claim | line | measured |
|---|---|---|
| two dry-run dispatches, both green | 42 | both "conclusion": "success" |
| ingot on ubuntu-24.04, run 35762662020 | 43 | job log SERVICE: ingot, dir=ingot, labels ["ubuntu-24.04"] |
| piri on macos-14, run 35765210724 | 43 | release job labels ["macos-14"] |
| runner derivation in both directions | 44 | ubuntu on one run, macos on the other |
| the skip list | 44 | ingot is a dockers: service (grep -cE '^dockers(_v2)?:' ingot/.goreleaser.yaml → 1), needs_docker=true in the log, and the build went green with docker skipped |
| the assertion | 45 | assert the binaries report their version success in both |
require the tag and publish both skipping |
45 | both "conclusion": "skipped" in both runs |
| all four configs | 47 | releasable: indexing-service ingot piri sprue in the job log, matching find -mindepth 2 -maxdepth 2 on this tree |
| piri 8m47s on macos-14 | 354 | 18:08:45 → 18:17:32 = 8m47s |
| 6m56s of it goreleaser | 354 | build step 18:09:09 → 18:16:05 = 6m56s |
| 1m18s the setup-go cache save | 355 | Post setup-go 18:16:08 → 18:17:26 = 1m18s |
| ingot 3m11s on ubuntu-24.04 | 355 | 17:45:48 → 17:48:59 = 3m11s |
git ls-remote --tags origin is empty |
337 | tags API returns [] |
Can a pull request reach a state the file says it cannot? No.
dry_runis set totrueat line 181 on thepull_requestbranch, before any other output write, and all three consumers compare!= 'true'. Observed on run 35773432670:require the tagandpublishbothskipped, so the same expression that produced those skips is the one guarding the token at line 495.needs.plan.outputs.dry_runis non-empty on every path that reachesrelease. The job is gated ondir(line 348), anddry_runis written beforediron both paths — line 204 before line 220 on the resolve path, lines 185-186 on the skip path.- A
planthat fails after resolving the service cannot start a build:needs.plan.result == 'success'covers it under either reading of the default, which is what the comment says it is there for.
The plan shell.
GITHUB_EVENT_NAMEis a documented default environment variable set on every runner, so not declaring it inenv:is fine. Underset -uits absence would abort the step, which skipsreleasevia theresultclause — fail-closed either way.HEAD_REFnever reaches$GITHUB_OUTPUTexcept as an exact match.svcis written at line 220 only after[ "$r" = "$svc" ]against thefind-derived list. The one place the raw value is used is a quotedechoto the log, andgit check-ref-formatforbids:, space,~,^,?,*,[,\and control characters in a refname — so neither a::workflow-command::nor a newline into$GITHUB_OUTPUTis expressible as a branch name.release/ingot/fooandrelease/xboth fall through to theexit 1.runneris written on every path that can reachruns-on:—ubuntu-24.04at line 190 on the skip path, one of 241/243 otherwise. Confirmed empirically: run 35773389770 skippedreleasewith no runner error.- No key is written twice: the skip path
exit 0s before line 204, anddir,runnerandneeds_dockereach have exactly one write per path.
The "must not already be released" step. Correct for the documented scheme, and it ran green on run 35773432670 (plan step 5, success). fetch-depth: 0 does give it tags: actions/checkout puts +refs/tags/*:refs/tags/* in the refspec when fetchDepth <= 0, and that is also what suppresses its own --no-tags; the fetch-tags input only matters for a shallow fetch, so the comment's reasoning for rejecting that spelling lands even though its premise is about a different failure. The one case the check cannot see is an unprefixed tag — which is the shape the publish comment says goreleaser would create if it were ever armed — but that is moot while publishing is closed and the tag scheme is an open ownership call, so I am not raising it.
Would CI reject this, and what does an ordinary pull request cost?
$ actionlint .github/workflows/release.yml
$ echo $?
0
guards is green on this head, and the block in the body derives the skip list correctly. An ordinary pull request costs plan alone — 16s on run 35773389770, with release skipped. The checkout comment's "~6s" brackets the two observations at this head (6s on 35773432670, 11s on 35773389770), which is what a tilde is for.
Generated by Claude Code
…ted the
wrong runs
Five findings. The gating itself held -- no pull request path reaches
`require the tag`, `publish` or a GITHUB_TOKEN, and that was observed, not
reasoned -- so all five are about what surrounds it.
A FORK PULL REQUEST COULD HAVE REACHED A macos-14 RUNNER WITH ITS OWN BUILD
CONFIG. `plan` derives the runner by parsing `<svc>/.goreleaser.yaml` from the
pull request's own tree, so a fork could name a branch `release/ingot`, add a
darwin build with CGO_ENABLED=1 to that config, and run its own goreleaser
hooks for up to forty minutes. This repository is public, forkable, and has a
fork; every other pull-request workflow here is pinned to ubuntu, so this file
was the only route to a macOS runner. No secret was exposed -- the token is
read-only on a fork pull request and is not passed to goreleaser in dry-run
mode -- but arbitrary code on a runner we pay for is not something to leave
open because the credential half is covered. The build job now runs on a pull
request only from this repository. Cutting the job rather than pinning the
runner, because piri's darwin cgo build genuinely cannot be cross-compiled from
linux, so a pinned `release/piri` pull request would fail rather than be safe;
and cutting forks costs nothing, since cutting this repository's releases is
not something a fork does.
THE HEADER CITED TWO RUNS THAT EXECUTED A DIFFERENT FILE. Runs 35762662020 and
35765210724 ran a release.yml that differs from this head by the whole change:
their `require the tag` and `publish` skips came from `if: !inputs.dry_run`,
the spelling this commit's parent deletes. They remain the evidence for the
runner derivation, the skip list and the timings, and they say nothing about
the gating. The runs that exercised THAT are 35773432670 and 35773389770, and
the block now names both sets and what each covers. The parent commit was
written to fix exactly this class in this exact block.
`dry_run` FAILED OPEN ON ANYTHING BUT THE LITERAL `true`. Every downstream test
is `!= 'true'`, so an empty value -- reachable, since a dispatch through the
API need not send the declared default -- selected real-release mode, the mode
that passes GITHUB_TOKEN to goreleaser. `resolve the version` already refuses a
version.json value outside its charset while the flag gating the token took any
string at all. Now exactly true or false; measured, `""`, `yes` and `TRUE` all
exit 1 writing nothing.
A PULL REQUEST COULD CANCEL A DISPATCH. The concurrency group was
`release-${inputs.service || github.head_ref}`, so a dispatch of `ingot` and a
pull request from a branch named `ingot` shared one -- and the joining run's
`cancel-in-progress` decides, so the pull request would cancel the dispatch,
which is what that line exists to prevent. The event is in the key now.
AGENTS.md said `release` is "dispatch-only and never runs on a push, so it is
not part of what a change costs", which this pull request makes false. What it
costs an ordinary change is 16s of `plan`, 11s of that the fetch-depth: 0
checkout the tag check needs -- measured on run 35773389770.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
plan derives the release runner by parsing <svc>/.goreleaser.yaml from the pull request's own tree, so a fork could have named a branch release/ingot, declared a darwin CGO_ENABLED=1 build in that config, and run its own goreleaser hooks on macos-14 for forty minutes. Public repo, forking on, one fork; every other pull-request workflow here is ubuntu-pinned, so that file was the only route. No secret was exposed -- read-only token on a fork PR, and not passed to goreleaser in dry-run mode -- but arbitrary code on a runner we pay for is not something to leave open because the credential half is covered. The build job now requires a same-repository pull request. Cut the job rather than pin the runner: piri's darwin cgo build cannot be cross-compiled from linux, so a pinned release/piri PR would fail rather than be safe. Four more from the same round, all fixed at 2e1e690, including the header citing two runs that executed a different release.yml -- the same class the preceding commit was written to fix, in the same block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja
left a comment
There was a problem hiding this comment.
[From Claude:]
Round 2, narrow brief — not satisfied. 3 findings: one blocking (a fork can still run arbitrary code on a runner, by a different route), one that is round 1's finding (b) again one revision later, one minor.
Of round 1's five: (c) and (e) are fully fixed and check out against the tree and the run data. (a) and (b) are half-fixes. (d) is fixed as stated, with a residual noted below. No new defect found outside these.
(a) half-fixed: the gate is on `release` only — `plan` still runs for forks, and it interpolates the fork's branch name straight into a shell script
The if: expression itself is correct and complete for what it covers. It parses as one expression (the folded >- keeps one literal newline, because the last line is more-indented, and Actions treats that as whitespace — harmless), github.event_name != 'pull_request' short-circuits true for workflow_dispatch so dispatch is untouched, and a deleted fork gives head.repo == null, which fails closed.
But the gate is on the release job. plan carries no event or repository condition, so a fork pull request still runs it — and plan holds the one step in this file that interpolates a value into a run: script instead of passing it by env::
- name: resolve the version
id: ver
if: steps.svc.outputs.dir != ''
run: |
set -euo pipefail
f="${{ steps.svc.outputs.dir }}/version.json"(and again at echo "Releasing ${{ steps.svc.outputs.dir }} $v"). dir is ${HEAD_REF#release/} — fork-controlled. The comment above it argues the value is safe because "Nothing derived from it reaches $GITHUB_OUTPUT unless it is exactly equal to one of this repository's own directory names." On a fork pull request that is false: actions/checkout@v4 checks out refs/pull/N/merge, so the find that builds releasable enumerates the pull request's tree, fork-added directories included.
Measured locally, on the exact pipeline from this file:
$ mkdir -p 'evil$(id)x' && : > 'evil$(id)x/.goreleaser.yaml'
find raw: ./evil$(id)x
releasable: evil$(id)x
svc=evil$(id)x found=1
$ git check-ref-format --branch 'release/$(id>/tmp/pwned)'
release/$(id>/tmp/pwned) # accepted
$ f="$(id>/tmp/pwned)/version.json" # what the next step's script becomes
$ cat /tmp/pwned
uid=0(root) gid=0(root) groups=0(root)
$, (, ), a backtick, ;, | and & are all legal in a git ref name; only space is not, and ${IFS} or brace expansion covers that.
So the fork does not get macos-14 for 40 minutes any more — it gets ubuntu-24.04, which is the same harm the new comment is written to close ("arbitrary code on a runner we pay for is not something to leave open because the credential half is covered"), one runner class cheaper. No secret is reachable: plan is permissions: contents: read and a fork pull request's token is read-only regardless, exactly as the comment says of the other half.
Worth noting this is this pull request's exposure, not inherited: the interpolation predates the branch, but on: pull_request is added here, and before it only someone who could dispatch the workflow could reach that step.
Two fixes, and the first is what the rest of the file already does three times (DIR:/VERSION: by env: in the version must not already be released, require the tag and assert the binaries):
- pass it by env —
env: DIR: ${{ steps.svc.outputs.dir }}, thenf="$DIR/version.json"; - and/or put the same-repository condition on the
release/*arm ofresolve the service, so a fork pull request resolves no service at all andplanstays a 16-second no-op for it.
(b) not fixed: the block labels runs at `b9fa2298` "THIS file", and this head is `2e1e690c` — so the new gate has still never been evaluated
The block now opens:
# Run, THIS file: both pull-request paths, at b9fa2298.
b9fa2298 is this branch's parent, not this head. Between it and 2e1e690c the file changes in three functional places — the release job's if:, the case "$DRY_RUN", and the concurrency group: — so both cited runs executed a file with none of this push's changes in it. That is round 1's finding (b) recurring, in the same block, one revision later.
The consequence is not bookkeeping. The new if: has never been evaluated in a state where its new clause is reached:
| run | head | branch | release job |
|---|---|---|---|
| 35773432670 | b9fa2298 |
release/ingot |
ran, 2m54s |
| 35773389770 | b9fa2298 |
ordinary | skipped |
| 35776051465 | 2e1e690c |
ordinary | skipped (plan 11s) |
The only run at this head takes the ordinary-branch path, where needs.plan.outputs.dir != '' is false and short-circuits before the new parenthesised clause. A gate that skips when it should run reports green and silent — which is rule 5's exact shape — so the release/<svc> throwaway pull request wants redoing at 2e1e690c before the block can claim "THIS file" for the release/<svc> path, and the case and the concurrency key are unexercised at this head too.
Everything else in the block is accurate; the run-by-run check is in the verification section.
(d) fixed as stated, with a residual: `github.head_ref` is not unique per pull request
Across events the new key is sound — workflow_dispatch and pull_request can no longer share a group whatever the service and branch are named, and cancel-in-progress is still right for each (supersede a pull request, never a dispatch someone is waiting on).
Within pull_request it is still not unique. github.head_ref is the bare head branch name, unqualified by the head repository. Now that forks reach this workflow, a fork pull request from a branch called release/ingot shares release-pull_request-release/ingot with the internal release/ingot pull request, and cancel-in-progress: true means whichever joins second kills the other's in-flight release build. Same shape as the finding this fixed, one level in.
github.event.pull_request.number is unique per pull request and keeps the "two release pull requests do not serialise behind each other" property the comment is after. Non-blocking.
Verification: (c) and (e) traced in full, and every run-data claim in the header checked
(c) dry_run — fixed. Every writer and every reader traced; no downstream read can see an unvalidated value.
Writers, all four paths out of resolve the service:
| path | writes | then |
|---|---|---|
workflow_dispatch, $DRY_RUN ∈ {true,false} |
dry_run=$DRY_RUN |
continues |
workflow_dispatch, anything else (incl. empty) |
nothing | exit 1 → plan.result != 'success' → release skipped |
pull_request, non-release/* |
dry_run=true (explicit, before exit 0) |
dir empty → release skipped |
pull_request, release/* |
dry_run=true |
continues |
| any other event | nothing | exit 1 |
Readers — grep -n 'dry_run' release.yml gives exactly three outside plan, and all three are needs.plan.outputs.dry_run != 'true': the require the tag if:, the publish if:, and the GITHUB_TOKEN env on the goreleaser step. There is no path where plan succeeds and the output is unset, so the fail-open != 'true' can no longer be reached with an unvalidated value. TRUE, yes and "" all fall to *) and exit 1 — fail closed, which is the right direction. inputs. survives in only three places (group:, SERVICE:, DRY_RUN:), none of them a mode test.
(e) AGENTS.md — fixed, numbers confirmed. Both against run 35773389770 (pull_request, claude/release-pr-gate, head b9fa2298):
planjobstarted_at 19:22:43→completed_at 19:22:59= 16s ✓actions/checkout@v4step19:22:45→19:22:56= 11s ✓
and the rest of the paragraph: .github/workflows/ holds exactly ci.yml, e2e.yml, images.yml, itest.yml, release.yml — five ✓; on: is workflow_dispatch + pull_request, so "runs by hand and on a pull request" and the dropped "never runs on a push" both hold ✓; "on one that is not from a release/<svc> branch it resolves no service and its build job skips" — observed in that run, resolve the version and the version must not already be released skipped, release skipped ✓; "publishes nothing in either mode" — --skip=validate,publish,sign,announce,docker in both, and the publish step exit 1s ✓. No other workflow-count claim survives in AGENTS.md (grep finds one).
Header run claims, all checked against the API. The "AN EARLIER FILE" half is right:
| run | event | head | claim | holds |
|---|---|---|---|---|
| 35762662020 | workflow_dispatch |
983c9d4c |
ingot on ubuntu-24.04 | ✓ labels ubuntu-24.04, release 3m11s |
| 35765210724 | workflow_dispatch |
699f929f |
piri on macos-14 | ✓ labels macos-14, release 8m47s |
and git show 983c9d4c:.github/workflows/release.yml / 699f929f: both carry if: ${{ !inputs.dry_run }} at the require the tag and publish steps, so "their skips came from the spelling this file no longer uses" is exact, and "They say nothing about the current gating" is right. The timeout-minutes comment's numbers also still hold at these runs: piri 8m47s (goreleaser 18:09:09→18:16:05 = 6m56s, setup-go post 18:16:08→18:17:26 = 1m18s), ingot 3m11s.
The "THIS file" half's content is accurate too — 35773432670 resolved ingot, release built and asserted in 2m54s (19:27:55→19:30:49) with require the tag and publish both skipped; 35773389770 resolved nothing and skipped; and those two are indeed the first to exercise needs.plan.outputs.dry_run. Only the label "THIS file" is wrong, which is finding (b).
Also checked, no finding: the folded >- parses to one expression (one preserved newline from the more-indented last line, which Actions tokenises as whitespace); release is the only other job, and its permissions: contents: read and the GITHUB_TOKEN ternary are unchanged and still correct; no other ${{ }} reaches a run: script anywhere in the file; runs-on: ${{ needs.plan.outputs.runner }} is still reachable only via the two literals, and the version.json newline-injection guard on that output is intact.
Generated by Claude Code
…uns a
fork's shell
Three findings. The first is that round one's fix was half of one, and the
comment it came with asserted the missing half was impossible.
A FORK COULD STILL GET ARBITRARY CODE ONTO A RUNNER, through `plan`, which had
no event or repository condition at all. `resolve the version` interpolated
`${{ steps.svc.outputs.dir }}` straight into its script, and on a pull request
`dir` derives from the branch name. Reproduced end to end: a fork adds a
directory literally named `evil$(touch${IFS}PWNED)x` carrying a
.goreleaser.yaml, pushes the branch `release/evil$(touch${IFS}PWNED)x` -- git
accepts `$ ( ) { }` in a ref, only space is illegal and ${IFS} covers that --
`actions/checkout` takes refs/pull/N/merge so the `find` enumerates the FORK's
tree and the directory lands in `releasable`, the branch-derived name matches,
and the next step runs the substitution. The comment added last push said
nothing derived from HEAD_REF could reach $GITHUB_OUTPUT "unless it is exactly
equal to one of this repository's own directory names". On a fork pull request
the enumerated directories are the fork's, so that was false.
Fixed twice over, because one of the two does not depend on getting a condition
right. `dir` now reaches the script by `env:`, as the file's other three steps
already did -- and no `${{ }}` remains inside any `run:` in this file, checked
by parsing rather than by reading. And a fork pull request now resolves no
service at all: everything `plan` reads is the pull request's tree, so refusing
once is smaller than hardening the releasable list, the config that picks the
runner and the version.json separately.
THE HEADER CALLED b9fa229 "THIS file". It is the parent -- the commit that
added the trigger, before the fork gate, the dry_run validation and the
concurrency key. The runs it names remain correct about what they covered, and
the label is now what they are: an earlier revision.
A FORK'S `release/ingot` AND OURS ARE THE SAME `head_ref`, so a fork pull
request shared a concurrency group with our in-flight release build and would
have cancelled it -- the same failure the event key was added to stop, one step
further out. The pull request number is unique across both.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
#19's round 1 found that plan derives the runner from the pull request's own .goreleaser.yaml, so a fork could book a macos-14 runner. I gated the build job and wrote a comment claiming nothing else fork-controlled could get through. Round 2 proved that wrong: plan had no condition at all, and resolve-the-version interpolated ${{ steps.svc.outputs.dir }} into its shell. Reproduced end to end with a directory named evil$(touch${IFS}PWNED)x and a branch to match. Fixed twice over -- dir by env, and a fork resolves no service -- and no ${{ }} remains inside any run: in that file, checked by parsing. #18's round 2 found the same shape: round 1's fleet-list fix asked ghcr.io for upload, indexer and signing-service, which answer 403; the packages are sprue, indexing-service and piri-signing-service. Four of the nine findings across the two pull requests are in fixes made by the round before, which is the argument for the round and the thing worth recording about the practice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Round two's point was that the fork gate, the dry_run validation and the concurrency key had never been evaluated on a run where a service resolves -- every run at those heads came from an ordinary branch, where `dir != ''` short-circuits first, and a gate that wrongly skips is green and silent. Run 35777398448 closes that: a release/ingot pull request at 68dd95d, plan 11s, release 3m35s on ubuntu-24.04, `require the tag` and `publish` skipped. The release job running at all is the evidence -- it needs plan to have succeeded, `dir` non-empty, and the same-repository condition to hold. The earlier pair stays named, because run 35773389770 is where the 16s cost of `plan` on an ordinary pull request is measured and that path has not changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Round two's point on #19 was that the fork gate, the dry_run validation and the concurrency key had never run on a pull request where a service resolves: every run at those heads came from an ordinary branch, where dir != '' short-circuits first, and a gate that wrongly skips is green and silent. #20 reopened at 68dd95d closes that. plan 11s, release 3m35s on ubuntu-24.04, require-the-tag and publish both skipped. The release job running at all is the evidence -- it needs plan to have succeeded, dir non-empty, and the same-repository condition to hold. Closed again; the branch still cannot be deleted from this session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja
left a comment
There was a problem hiding this comment.
[From Claude:]
Round 3 at dcfbe0bc: three findings, none blocking. Round 2's three are all genuinely fixed and the half-fix pattern does not repeat — checked by parsing the YAML rather than reading it, by extracting the resolve step and running it over twelve event/repository/branch combinations including the injection branch, and against the run API for every run the header names. What is left is one load-bearing claim in a comment that is false about how GitHub runs fork pull requests, one claim in the verification block that this push's own diff contradicts, and a PR body that was not refreshed for this head.
The fork gate's justification — "this file would be the only way a fork reaches a macOS runner" — does not hold, because a fork pull request runs its own copy of the workflows
release.yml lines 421-424, and the round-1 section of the PR body, justify cutting the job rather than pinning the runner with:
This repository is public and forkable, and every other pull-request workflow here is pinned to ubuntu, so this file would be the only way a fork reaches a macOS runner.
The first half is true. All four other workflows trigger on pull_request and every job in them is ubuntu-pinned:
ci.yml on: push, pull_request, workflow_dispatch guards -> ubuntu-latest
unit -> ubuntu-latest
images.yml on: push, pull_request, workflow_dispatch build -> ubuntu-24.04
e2e.yml on: push, pull_request, workflow_dispatch e2e -> ubuntu-24.04
itest.yml on: push, pull_request, workflow_dispatch itest -> ubuntu-24.04
The conclusion does not follow. pull_request — unlike pull_request_target — runs the workflow definitions from the pull request's merge commit, so a fork's own edits under .github/workflows/ are in force for its own run. A fork therefore does not need this file's branch-name logic to book a macOS runner: it can set runs-on: macos-14 in its copy of this very file, where the gate we just added never executes, or add a new on: pull_request workflow of its own. Nothing written in this repository's copy can prevent either.
What actually governs it is the repository's Fork pull request workflows from outside collaborators setting (Settings → Actions → General). The public-repo default is "Require approval for first-time contributors", which leaves a returning contributor's fork pull request running unapproved. That setting is the control; this gate is not. Worth confirming which one is set, since the round-1 finding this branch is answering is only actually closed by the stricter one.
I am not asking for the gate back out — it closes the accidental case (a fork pull request that touches no workflow and happens to be branched release/<svc>), and it takes a branch-name-derived value out of runs-on: in our own copy, which is worth having on its own. It is the sentence justifying it that is wrong, and it is exactly the kind someone will rely on later when deciding whether some other workflow needs the same treatment. The narrow claim is true and checkable: of the workflows in this repository, this is the only one that could route a fork to macOS.
"that path has not changed since" is contradicted by this push's own diff — though the 16s/11s figure it supports is right
The verification block says of run 35773389770:
Kept because the second is where the 16s/11s cost of
planon an ordinary pull request is measured, and that path has not changed since.
The number is right (verified below). "That path has not changed since" is not, and the same sentence names one of the changes two lines earlier ("before the fork gate, the dry_run validation and the concurrency key"). Three of this push's non-comment hunks land on the ordinary-pull-request path:
$ git diff b9fa2298 dcfbe0bc -- .github/workflows/release.yml \
| grep -E '^[+-]' | grep -vE '^[+-][[:space:]]*#|^(\+\+\+|---)'
- group: ${{ github.workflow }}-${{ inputs.service || github.head_ref }}
+ group: ${{ github.workflow }}-${{ github.event_name }}-${{ inputs.service || github.event.pull_request.number }}
+ if [ "$HEAD_REPO" != "$GITHUB_REPOSITORY" ]; then <- executes on EVERY
+ ... pull request, ordinary
+ fi ones included
- echo "dir=" >> "$GITHUB_OUTPUT" <- the ordinary branch's
- echo "dry_run=true" >> "$GITHUB_OUTPUT" own early exit, rewritten
- echo "runner=ubuntu-24.04" >> "$GITHUB_OUTPUT"
+ { echo "dir="
+ echo "dry_run=true"
+ echo "runner=ubuntu-24.04"; } >> "$GITHUB_OUTPUT"
What is true is that the cost has not changed: what was added to that path is one string comparison, and one redirect where there were three. The distinction is worth getting right because this block is the file's own rule-5 claim about itself, and round 2's second finding was this same class — a run labelled as covering a revision it did not cover.
The PR body is stale for this head: the verification table still says "running", and its diffstat matches no commit on the branch
Two things in the body no longer describe dcfbe0bc.
The verification table's third row still reads running for the run at 68dd95d. That is run 35777398448, which completed green at 2026-09-22T20:08:21Z — and naming it is the entire content of the head commit. The table is where a reviewer looks for that answer before the header comment.
The skippable-checks block quotes a diffstat that matches no commit on this branch:
body says: .github/workflows/release.yml | 292
dcfbe0bc: .github/workflows/release.yml | 291 (263 insertions(+), 28 deletions(-))
68dd95d9: .github/workflows/release.yml | 288 (260 insertions(+), 28 deletions(-))
main and origin/main are both at 699f929f, so two-dot and three-dot agree and there is no reading that gives 292. AGENTS.md's rule for the block is "derive it; do not assert it", refreshed on every push.
The rest of the block holds and is reproducible. Both greps return exactly the files it lists, and the hits are prose inside comments — ci.yml:49 is a comment about timeouts, assert-released-version.sh:201 a comment about the reject class — so nothing parses either changed file, and "everything except release / plan" is the right call.
Verification: round 2's three findings, the run API, and what else was checked
Round 2 (1), the fork gate — both halves verified, and the half-fix pattern does not repeat.
No ${{ }} survives inside any run:, checked by parsing, not reading. All six run bodies extracted with PyYAML; none contains ${{. bash -n clean on all six; actionlint clean on the file.
run: steps parsed: 6
steps whose run body contains ${{ : NONE
dir now arrives by env:, and the injection round 2 reproduced is closed independently of the condition. Extracting the ver step and running it with the hostile value directly:
$ DIR='evil$(touch${IFS}PWNED)x' bash ver.sh
::error::evil$(touch${IFS}PWNED)x/version.json is missing, so there is no version to release.
rc=1 — no PWNED file: injection closed
A fork resolves nothing. Extracting the svc step and running it over twelve combinations (GITHUB_REPOSITORY=fil-forge/forge):
PR, fork, release/ingot rc=0 | dir= dry_run=true runner=ubuntu-24.04
PR, fork, injection branch rc=0 | dir= dry_run=true runner=ubuntu-24.04 (no PWNED)
PR, fork, head.repo null (empty) rc=0 | dir= dry_run=true runner=ubuntu-24.04
PR, ours, ordinary branch rc=0 | dir= dry_run=true runner=ubuntu-24.04
PR, ours, release/ingot rc=0 | dir=ingot dry_run=true runner=ubuntu-24.04 needs_docker=true
PR, ours, release/piri rc=0 | dir=piri dry_run=true runner=macos-14 needs_docker=false
PR, ours, release/nosuch rc=1 | dry_run=true
dispatch, piri, dry=true rc=0 | dir=piri dry_run=true runner=macos-14
dispatch, ingot, dry=false rc=0 | dir=ingot dry_run=false runner=ubuntu-24.04
dispatch, ingot, dry=EMPTY rc=1 |
dispatch, ingot, dry=TRUE rc=1 |
push (unhandled event) rc=1 |
HEAD_REPO is correct and non-spoofable for every event. It is github.event.pull_request.head.repo.full_name from the webhook payload, which a fork cannot set to ours, and it is read only inside the pull_request arm — so workflow_dispatch, where github.event.pull_request is null and the env var is the empty string, never reaches it, and the *) arm fails closed on anything else. A pull request whose head repository has been deleted yields "" and is refused (row 3 above).
Both early exits write every output that runs-on: or a downstream if: reads. runs-on ← runner ✓; the release job's if: ← result, dir ✓; require the tag and publish ← dry_run ✓; resolve the version and the version must not already be released ← dir ✓. That runs-on really is evaluated for a skipped job is not a reading of the docs here — run 35773389770's skipped release job reports labels: ["ubuntu-24.04"], resolved from the early exit's own write. needs_docker is declared as a job output and read by nothing, but that is pre-existing (main's release.yml, line 88), not this push's.
What a fork can still make plan do: ~16s of an ubuntu runner and a fetch-depth: 0 checkout per push, which is the accepted design. Nothing fork-controlled is read off disk or executed — the refusal precedes the find, the PyYAML runner derivation and the version.json read.
Round 2 (2), the run API. Every figure in the block, measured from job started_at/completed_at:
| run | event, head | header claims | measured |
|---|---|---|---|
| 35777398448 | pull_request, release/ingot, 68dd95d9 |
plan 11s; release 3m35s, ubuntu-24.04; tag + publish skipped | plan 20:01:29→20:01:40 = 11s; release 20:04:45→20:08:20 = 3m35s; labels: ["ubuntu-24.04"]; both steps skipped ✓ |
| 35773432670 | pull_request, release/ingot, b9fa2298 |
built and asserted in 2m54s | 19:27:55→19:30:49 = 2m54s, green ✓ |
| 35773389770 | pull_request, ordinary branch, b9fa2298 |
resolved nothing, release skipped; 16s/11s | plan 19:22:43→19:22:59 = 16s, checkout 11s; resolve the version and the tag check skipped; release skipped ✓ |
| 35762662020 | workflow_dispatch, 983c9d4c |
ingot on ubuntu-24.04 | ubuntu-24.04, release 3m11s — matches the timeout-minutes note ✓ |
| 35765210724 | workflow_dispatch, 699f929f |
piri on macos-14 | macos-14, release 8m47s, goreleaser 6m56s, setup-go cache save 1m18s — all three match ✓ |
The two dispatches' skips did come from the spelling this branch deletes: main's release.yml is on: workflow_dispatch: only and has if: ${{ !inputs.dry_run }} at lines 306 and 385 ✓.
"Run, THIS file" is accurate, and this is not a repeat of round 2's second finding. Run 35777398448 did run at 68dd95d9, the parent of this head — but git diff 68dd95d9 dcfbe0bc -- .github/workflows/release.yml falls entirely inside the # header block, so the executable file is byte-identical to what ran. That is the discriminator: b9fa2298 differed from its head in the gating itself.
Round 2 (3), concurrency. release-<event>-<service|number> gives release-pull_request-19 and release-workflow_dispatch-ingot. Well-formed and collision-free: the event name separates the two key spaces, and a pull request number is allocated by the base repository, so a fork cannot land on ours. inputs.service is required: true, so the || never falls through on a dispatch; even if it did, the right operand is null there and the group stays distinct from every pull-request group. inputs.service evaluating harmlessly on a pull_request event is not a docs reading either — the previous key used it and runs 35773389770 / 35773432670 were both pull_request. The new key was exercised on a pull_request event at 68dd95d9 (run 35777398448).
AGENTS.md. "CI is five workflows" ✓ — ls .github/workflows/ gives exactly ci.yml, e2e.yml, images.yml, itest.yml, release.yml. "16s its plan job takes, 11s of that the fetch-depth: 0 checkout" ✓ against run 35773389770, as above. One thing to be aware of rather than fix: release.yml line 162 says depth 0 "measures ~6s on this repository", and both numbers are real — the checkout ranged 2s to 11s across these five runs, and AGENTS.md happens to quote the slowest. "It publishes nothing in either mode" ✓ — publish is in --skip= for both modes and the publish step exits 1 in the real one.
Generated by Claude Code
A `pull_request` event runs the workflow definitions from the PULL REQUEST's commit -- that is what separates it from `pull_request_target`, and it is the same mechanism that let this trigger be tested before it merged. So a fork can write `runs-on: macos-14` into its own copy of this file, or add a workflow of its own, and the same-repository condition is simply not in the code that runs. The comment claimed this file "would be the only way a fork reaches a macOS runner". It is not. The gate is still worth having, for the half this file can own: after it, nothing a fork writes decides which runner we start or reaches the shell in the release job. What governs whether a fork's own workflow runs at all is the repository's "Fork pull request workflows from outside collaborators" setting, which the Actions permissions API is not reachable to read from here. That is named as worth confirming rather than assumed, since the public default approves a returning contributor automatically. Also: "that path has not changed since" is contradicted by this branch's own diff -- three non-comment hunks land on the ordinary-pull-request path, the fork check among them. What has not changed is the COST the cited run measures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Rounds 1 and 2 on each pull request found defects the previous round's fixes had introduced -- four of the eleven findings across the two. Round 3 on each found none of that shape: two counting errors on #18 (there are eight required image interpolations, and a flat systems/*/compose.yml glob misses INDEXER_IMAGE one level down), and three claims-about-claims on #19, the sharpest being that the fork gate does not do what its comment said. Also corrects this page's own count: five services lack a version-tagged image, not six. The indexer publishes 1.13.4; this entry and #18's comment both had it among the missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja
left a comment
There was a problem hiding this comment.
[From Claude:]
Round 4: three findings, none blocking. Both fixes this push claims hold. The corrected fork-gate comment is accurate: I walked every path plan and release take and found nothing a fork writes that decides a runner or reaches a shell on our side. The push is comment-only — with comments stripped, release.yml at 93930ec9 is byte-identical to dcfbe0bc and to 68dd95d9, the sha the newly cited run exercised, so "Run, THIS file" holds for the executed definition. Every figure in the verification block checks out against the run API, to the second.
The three findings: a comment 50 lines below the fixed one still describes the pre-gate world; the 16s/11s cost is a single-sample outlier this branch's own runs at this head contradict; and the dispatch-only claim was corrected in AGENTS.md and left standing in two other files.
1. The permissions: comment (L484-493) still says a fork can get the release job to run its own code — the gate 40 lines above stops exactly that
The gate comment now says, correctly, SAME-REPOSITORY PULL REQUESTS ONLY. Forty lines later the permissions: comment still describes the state before it existed:
# THE FORK-PULL-REQUEST CASE IS REAL HERE NOW, which it was not while this
# workflow was dispatch-only. A pull request from a fork can name its branch
# `release/piri` and get this job to run its own code. Three things hold it:
# GITHUB_TOKEN is read-only for a fork pull request whatever this block
# says, `dry_run` is resolved to true for every pull request regardless of
# the input, and the token is not passed to goreleaser at all in that mode.
At this head a fork cannot get this job to run at all, by two independent mechanisms — the same two the gate comment describes:
plan'spull_requestbranch exits before resolving anything whenHEAD_REPO != GITHUB_REPOSITORY, sodiris empty;- the job's own
if:requiresgithub.event.pull_request.head.repo.full_name == github.repository.
So the sentence is false of this file, and the list of "three things" names none of the two things that actually hold it.
It is also inaccurate about the one case where a fork does reach a runner — its own edited copy of this file, which is the case the new comment above was written to admit. In that case the fork controls the file, so two of the three protections listed (dry_run resolved to true, the token not passed to goreleaser) are lines the fork can delete. Only "GITHUB_TOKEN is read-only for a fork pull request" survives, because that one is not in the file.
This predates the push — it is not a regression — but it is the same defect class this push set out to fix, in the next comment block down.
2. "the 16s/11s COST … has not changed" is sound; the 16s/11s is a single-sample outlier, and the runs at this head measure 10s/6s
The substantive claim holds: the fork check is one string comparison, and nothing else this branch adds lands on the ordinary-pull-request path with measurable cost. So the cost did not change.
The number is the problem. It comes from one run at b9fa2298. The same path — same event, same ordinary branch claude/release-pr-gate, release job skipped — has since run twice more on this branch, including at this exact head:
| run | head | plan job |
actions/checkout step |
|---|---|---|---|
| 35773389770 (cited) | b9fa2298 |
16s | 11s |
| 35778421641 | dcfbe0bc |
9s | 5s |
| 35779874521 | 93930ec9 (this head) |
10s | 6s |
35779874521 is a run of this file on the ordinary-pull-request path, and the verification block does not cite it — although the previous push added a "Run, THIS file" entry for the release path as soon as one existed.
The file already disagrees with itself about the checkout: the fetch-depth: 0 comment says depth 0 "measures ~6s on this repository", which is what runs 35778421641 and 35779874521 measure, and which the 11s attribution contradicts. AGENTS.md then publishes 16s/11s as what release costs an ordinary change — an overstatement of roughly 60% against the head it ships with.
Either number is defensible on its own; both being in the same pull request is not, and the reader whose decision this changes is the one deciding whether an unfiltered release is worth paying for.
3. dispatch-only was fixed in AGENTS.md and left standing in two other files, one of them a guard's stated reason to exist
Round 1 caught AGENTS.md claiming release is "dispatch-only and never runs on a push", and this branch fixes it there. The class was not enumerated. One grep finds the rest:
$ grep -rn "dispatch-only\|four workflows" --include='*.md' --include='*.sh' --include='*.yml' .
./.github/workflows/release.yml:485: # workflow was dispatch-only. A pull request from a fork can name its branch
./.github/scripts/check-assert-released-version.sh:5:# is dispatch-only and has never been dispatched -- so until this guard, nothing
./MONOREPO_TODO.md:177:changed-files detection. Every push runs all four workflows and every job in
./MONOREPO_TODO.md:425:`release.yml` flow". **There was no `release.yml`**; `main` had four workflows —
.github/scripts/check-assert-released-version.sh:4-6— "assert-released-version.sh is called from release.yml, which is dispatch-only and has never been dispatched -- so until this guard, nothing in CI executed it." All three clauses are now false, and the third is the guard's justification. Run 35777398448 executedassert-released-version.shin CI, in the release job, step "assert the binaries report their version", success. The guard is still worth having — it runs the negative direction and covers services no release build reaches — but the reason written on it is not the reason any more.MONOREPO_TODO.md:431— "There is one now, and it is deliberately not armed — dispatch-only, dry-run by default, and it never creates a tag." The second clause is false at this head; the other two hold.MONOREPO_TODO.md:177— "Every push runs all four workflows and every job in them", in a section that measures cost per push to a pull-request branch (the next paragraph measures #8's "last push"). Under that usage it is five now. Literally, for a push tomain, it is still four:ci,e2e,imagesanditestarepush: [main]+pull_request;releaseispull_request+workflow_dispatchonly.
MONOREPO_TODO.md:425 ("main had four workflows") is past tense about the plan's starting point and is fine.
Verification, and what checked out
The push is comment-only. Stripping comment and blank lines from release.yml and comparing: 93930ec9 == dcfbe0bc == 68dd95d9. The YAML parses; plan is runs-on: ubuntu-24.04, release reads needs.plan.outputs.runner; both jobs are permissions: contents: read; no ${{ }} appears inside any run: block (checked by parsing the YAML, not by reading). So this push introduced no behavioural defect.
Round 3 (a), the corrected gate comment. "Nothing a fork writes decides which runner we start or reaches the shell in the job below" holds on every path:
plan'sruns-onis a literal.release's readsrunner, which our script writes as one of exactly two literals; the version-charset check (including thetr -d '\n'test) is what stops a craftedversion.jsonwriting a third.- On a fork pull request,
resolve the serviceexits 0 beforefind,python3andgrepever read the fork's tree, and beforeHEAD_REFreaches thecase. It writesrunner=ubuntu-24.04on the way out, so a skippedreleasestill has a valid label to read. HEAD_REPOis the only fork-controlled value that reaches that shell, quoted, in one[ ]comparison and oneecho.- Fail-closed on a deleted fork:
head.reponull makesHEAD_REPOempty, which is!= GITHUB_REPOSITORY, which takes the fork path. releasenever runs for a fork —if:requireshead.repo.full_name == github.repository.
The claim about the "Fork pull request workflows from outside collaborators" setting is a statement about the session, not about the tree; nothing here can confirm or refute it, which is what the comment says.
Malformed dispatch. dry_run is restricted to exactly true/false with exit 1 otherwise; service reaches find only after an exact match against the list derived from the tree; version is charset- and newline-checked; dir and version reach every shell by env:. Nothing new reachable.
The five cited runs, against the API. All figures correct.
| run | event / head | claim | measured |
|---|---|---|---|
| 35777398448 | pull_request, release/ingot, 68dd95d9 |
plan 11s; release 3m35s on ubuntu-24.04; require the tag + publish skipped |
plan 20:01:29→20:01:40 = 11s; release 20:04:45→20:08:20 = 3m35s; labels ubuntu-24.04; both steps skipped |
| 35773432670 | pull_request, release/ingot, b9fa2298 |
built and asserted in 2m54s | 19:27:55→19:30:49 = 2m54s |
| 35773389770 | pull_request, ordinary branch, b9fa2298 |
resolved nothing, release skipped, 16s/11s | resolve the version skipped, release job skipped; plan 16s, checkout 11s |
| 35762662020 | workflow_dispatch, 983c9d4c |
ingot 3m11s on ubuntu-24.04 | 17:45:48→17:48:59 = 3m11s, ubuntu-24.04 |
| 35765210724 | workflow_dispatch, 699f929f |
piri 8m47s on macos-14, 6m56s of it goreleaser, 1m18s the setup-go cache save | 18:08:45→18:17:32 = 8m47s, macos-14; build step 6m56s; Post setup-go 1m18s |
Both dispatch runs' plan jobs have no "the version must not already be released" step, which is consistent with their label as an earlier file, and both had require the tag and publish skipped.
AGENTS.md's workflow count. .github/workflows/ holds exactly five files: ci.yml, e2e.yml, images.yml, itest.yml, release.yml. "release runs by hand and on a pull request", "resolves no service and its build job skips", and "publishes nothing in either mode" are all accurate — the last because goreleaser runs with --skip=…,publish,… and the publish step exits 1 before anything is uploaded.
Generated by Claude Code
Three findings, none blocking, and the third is rule 5 happening to me: the
previous push corrected AGENTS.md's "dispatch-only and never runs on a push"
and did not run the command that enumerates the class. Three more copies stood.
check-assert-released-version.sh "release.yml, which is dispatch-only and
has never been dispatched -- so until this
guard, nothing in CI executed it". All
three clauses false, and the third is the
guard's stated reason: run 35777398448
executed the script in CI. The guard still
earns its place, for the paths a real run
does not reach, and now says so.
MONOREPO_TODO.md:431 "dispatch-only" in the Phase 1 entry.
MONOREPO_TODO.md:177 "Every push runs all four workflows" --
still true of a push to main, not of a
pull request, which now runs five.
THE PERMISSIONS COMMENT STILL DESCRIBED THE WORLD BEFORE THE FORK GATE, forty
lines below the comment the last push corrected. It said a fork "can name its
branch release/piri and get this job to run its own code", and named three
things holding it -- none of which is the gate, which is what actually holds it
now. It was also wrong about the case where a fork does reach a runner, its own
edited copy of this file: two of the three named protections are lines the fork
can delete, and only the read-only token survives.
16s/11s WAS THE SLOWEST OF FIVE SAMPLES, and it was published in two places as
the cost of an ordinary change. Measured across every run of that path on this
branch: 16s/11s, 11s/6s, 8s/4s, 9s/5s, 10s/6s. The first run is the outlier and
it overstated by about half; the file's own fetch-depth comment already said
~6s and disagreed with it. Both places now give the range and the sample rather
than a point estimate, since what a reader needs from this number is "under
twenty seconds" and no single figure supports more than that.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Peeja
left a comment
There was a problem hiding this comment.
[From Claude:]
Round 5 at d9b82b15: three findings, none blocking.
Round 4's three fixes check out. The permissions comment is accurate clause by clause, the 8-16s range reproduces exactly from the five runs it samples, the guard header's "covers the paths a real run does not reach" is true, and no fourth copy of the "dispatch-only" class survives anywhere in the repository. The push is comment-and-documentation-only in release.yml, verified by parsing both heads rather than by reading the diff.
All three findings are the same shape one step further out: the enumeration that fixed these numbers stopped at the tree. The figure this push replaced in two files is still standing in the PR body and on the wiki, and the range it published already excludes the run this head produced.
The skippable-checks block describes a two-file diff; this head is four files, and one of them is executed by ci / guards
The block at the top of the body opens "Everything except release / plan. Two files, and nothing in the tree reads or executes either" and shows a git diff --stat of release.yml | 305 and AGENTS.md | 14. That is the diff at an earlier head. At d9b82b15:
$ git rev-parse --short origin/main
699f929f # the PR's own base sha, per the API
$ git diff --stat origin/main HEAD
.github/scripts/check-assert-released-version.sh | 9 +-
.github/workflows/release.yml | 315 +++++++++++++++++++++--
AGENTS.md | 15 +-
MONOREPO_TODO.md | 12 +-
4 files changed, 312 insertions(+), 39 deletions(-)
The GitHub API agrees: changed_files: 4, additions: 312, deletions: 39.
The part that matters is not the stale numbers but the derived claim built on them. One of the four changed files is run by a check the block says a reviewer can skip:
$ grep -n check-assert-released-version .github/workflows/ci.yml
45: # check-assert-released-version.sh test 8 waits out the assertion's own
72: run: .github/scripts/check-assert-released-version.sh
So ci / guards executes a file this PR changes, and "nothing in the tree reads or executes either" is false at this head.
The claim happens to be harmless — the change to that script is comments only, so guards cannot behave differently:
$ diff <(git show 93930ec9:.github/scripts/check-assert-released-version.sh | grep -vE '^\s*#') \
<(grep -vE '^\s*#' .github/scripts/check-assert-released-version.sh)
# empty: identical
$ bash -n .github/scripts/check-assert-released-version.sh
# clean
But that is a different sentence from the one in the block, and AGENTS.md's rule for it is "Derive it; do not assert it" and "Refresh it on every push". A refreshed block would say four files, name check-assert-released-version.sh as executed by ci / guards, and rest the skip on the comment-only diff above rather than on nothing reading the file.
The 16s point estimate survives in two more places the fix did not enumerate: the PR body and the wiki
The commit message says the figure "was published in two places" and fixes both. There are two more, and both were written or refreshed in the same turn as the fix.
The PR body, in What this does:
An ordinary pull request costs 16s, 11s of that the
fetch-depth: 0checkout the tag check needs.
That is verbatim the claim release.yml and AGENTS.md just stopped making.
The wiki, Needs-Human-Work.md:109 (wiki branch head 7a9bd754, committed 20:41:35Z — after this push, and the same revision that records "Head d9b82b15, round 5 running"):
and
AGENTS.mdstill said this workflow "is not part of what a change costs", now measured at 16s ofplanper pull request.
"now measured at" is a present-tense claim about the current cost, not a quotation of what round 1 found.
This is the same miss as last round's, in a different class: the commit message calls out that the previous push "corrected AGENTS.md's 'dispatch-only and never runs on a push' and did not run the command that enumerates the class". The enumeration for the 16s class was run over the repository and stopped there, while AGENTS.md makes the wiki a same-turn artifact and the body a per-push one.
8-16s already excludes the run at this head, which measured 6s / 4s
Re-derived from the run API: every release.yml run on claude/release-pr-gate with event pull_request in fil-forge/forge. Six of them. plan is the job's started_at → completed_at; checkout is step 2's span — the methodology that reproduces the file's own five samples.
| # | run | head | plan |
checkout |
|---|---|---|---|---|
| 1 | 35773389770 | b9fa2298 |
16s | 11s |
| 2 | 35776051465 | 2e1e690c |
11s | 6s |
| 3 | 35777362433 | 68dd95d9 |
8s | 4s |
| 4 | 35778421641 | dcfbe0bc |
9s | 5s |
| 5 | 35779874521 | 93930ec9 |
10s | 6s |
| 6 | 35781536427 | d9b82b15 |
6s | 4s |
Rows 1–5 are the file's 16s/11s, 11s/6s, 8s/4s, 9s/5s, 10s/6s exactly, so the fix is right about what it measured. Row 6 is the run this push triggered, and it is below the published floor: six runs of that path is 6–16s, checkout 4–11s (unchanged). "The other four are 8-11s" becomes "the other five are 6-11s".
release.yml's "Five runs of that path across this branch measured 8-16s" and AGENTS.md's "8-16s over five runs" are both still true of the five runs they name, and both now read as a claim about the branch that the branch contradicts.
This recurs by construction: the run that measures a push always postdates the text that push publishes, so a sample range in the file is stale on arrival and the next push moves it again. The commit message already reaches the fix that survives that — "what a reader needs from this number is 'under twenty seconds' and no single figure supports more than that" — which argues for the bound rather than the interval.
One measurement note if the number is meant to answer "how long before this check clears": these are job execution times and exclude queueing, which at this head was the larger half — run 35781536427's plan was created at 20:37:54 and started at 20:38:32, 46s wall clock for the run against 6s of work.
Verification, and what checked out
(a) The permissions comment — accurate. Clause by clause:
- "THIS JOB DOES NOT RUN FOR A FORK at all — see the
if:above": theif:isneeds.plan.result == 'success' && needs.plan.outputs.dir != '' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository). It does require a same-repository pull request, andplan's ownpull_request)arm refuses a fork a second time by writing an emptydir. - "Only GITHUB_TOKEN being read-only for a fork pull request survives that, and it survives it regardless of what is granted here": checked that this does not rest on a repository setting. The API reports
"visibility": "public","private": false,forks_count: 1— the write-token/secrets toggles for fork PRs are a private-repository facility, and a job'spermissions:cannot exceed the token's ceiling either way. Accurate. - The characterisation of the three superseded protections (two are lines a fork can delete in its own copy, the read-only token is not) matches what the previous revision listed.
No other comment in the file overstates a gate. The two candidates both state their limits where they are: the if:'s own block carries "WHAT THIS DOES NOT DO ... it does not stop a fork reaching a macOS runner", and plan's "A FORK RESOLVES NOTHING" is scoped to what this code does with the pull request's tree, with the injection chain and the env: half spelled out beside it. The header, the concurrency key comment, require the tag, the goreleaser step and publish make no gate claims that the tree contradicts.
(c) The "dispatch-only" class — enumerated over the whole tree, no fourth copy in the repository:
$ git grep -nIiE "dispatch-only|dispatch only|only by hand|never runs on a push|never been dispatched|not part of what a change costs|(four|4) workflows|(five|5) workflows" -- .
.github/scripts/check-assert-released-version.sh:5:# for most of this script's life was dispatch-only and had never been
AGENTS.md:194:CI is five workflows — `ci` (per-module build/vet/staticcheck/tidy/test plus
MONOREPO_TODO.md:177:changed-files detection. Every push to `main` runs all four workflows that
MONOREPO_TODO.md:427:`release.yml` flow". **There was no `release.yml`**; `main` had four workflows —
MONOREPO_TODO.md:434:is dry-run by default, and never creates a tag. (It is no longer dispatch-only:
Line 427 is the past-tense account of what the consolidation plan assumed, immediately followed by "There is one now" — correct as history. The trigger counts are right: parsing all five workflow files, ci, e2e, images and itest carry push: branches: [main] and all five carry pull_request, so four on a push to main and five on a pull request.
Two wiki lines in this class were read and deliberately not raised: Needs-Human-Work.md:325 quotes the old script header inside a blockquote in a "since you last looked" retrospective, and Current-State.md:816's "still dispatch-only and publishes nothing" is scoped to release.yml "on main since #12", where it is still true.
The guard's new claim — true. Run 35777398448's release job ran assert the binaries report their version to success in 1s against ingot's dist, which is the ok path and nothing else; require the tag and publish both skipped. The fixtures cover what that cannot reach — the script's own verified-both-directions list names the FATAL unassertable exit, both fallback refusals, the </dev/null probe, the ERE escaping, the version-shaped test and both dist-filter arms (tests 3, 4b, 4c, 5, 6, 7, 9, 9b). So "covers the paths a real run does not reach" holds, and the header's "has executed the script in CI" is confirmed by that run.
The push is comment-and-documentation-only in release.yml. Compared against 93930ec9 two ways:
$ python3 -c "import yaml; print(yaml.safe_load(open('prev.yml')) == yaml.safe_load(open('head.yml')))"
True
$ diff prev.stripped head.stripped # comments and blank lines removed
# empty
Not raised, per the brief: prose, wording, line length and comment style, including where the new text narrates which earlier draft was wrong.
Generated by Claude Code
…t have
Petra asked how forks came into any of this. They do not, and two review rounds
went into a condition that answered a question nobody had asked.
Pull requests here come from the team, with push access. The only fork is
fil-forge/forge-2, our own archived copy of the migration. And ci, e2e, images
and itest have all triggered on pull_request since before this file did, so a
fork opening pull requests would already be spending 23 minutes of itest long
before it reached three minutes of ingot here. Nothing about adding this
trigger changed who can run what.
Nor is there anything to take. A pull_request from a fork runs in THIS
repository's Actions with no secrets and a read-only token -- which is what
pull_request_target exists to provide, and why that event and not this one is
the dangerous one. Audited rather than assumed: pull_request_target appears
nowhere in .github/; every runs-on is GitHub-hosted; the whole tree references
exactly one secret, GITHUB_TOKEN, which is provisioned rather than stored;
every permissions: block in every workflow is contents: read; and the
repository is public, so there is nothing to exfiltrate. What remains is
compute on free minutes.
The condition also could not do what it appeared to. A pull_request runs the
workflow definitions from the pull request's own commit, so a fork's copy of
this file is whatever the fork wrote and the condition is not in the code that
runs. That is a guard reading as total while covering nothing -- rule 5's case
for shipping none -- and it had already produced two wrong comments about
itself in consecutive rounds.
What survives is the half that was always right and never needed the threat
model: `dir` reaches a shell by env, never by an interpolated expression, in
every step of this file. A branch name can carry `$ ( ) { }`, git rejects only
a space, and the one step that did interpolate it ran the substitution.
Where forks would become real is arming publishing, which needs packages:write
and a registry credential. The permissions comment already says what that costs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
…xt push The cost of `plan` on an ordinary pull request was published first as a single number (the slowest of five samples, overstating by about half), then as the observed range. Round five caught the range already excluding the run at the head it shipped with -- 6s, below its own 8s floor. That recurs by construction: the run that measures the sentence always postdates it, so any interval is stale the moment it is written. Six runs now span 6-16s and the next will move it again. So both places give the bound -- under twenty seconds, every run so far -- and release.yml carries the command that lists the samples. That is what a reader's decision turns on, and it is the detail rule applied to the number that has broken it twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Fourteen behavioural findings in rounds 1-2 across #18 and #19. Zero in rounds 3-5, which produced nineteen findings, all about claims -- a comment, a count, a PR body -- rather than about what the code does. The claim-findings sustain themselves: each fix to a comment is a new comment the next round finds slightly off. The timing number could not converge at all, since the run that measures the sentence always postdates it, and the release-mechanism paragraph took three wrong versions before deleting it was the answer. The diagnosis is about rule 10 rather than these two pull requests. release.yml's diff is ~305 lines and mostly commentary, so the rounds reviewed the commentary; a reviewer told to verify every factual claim in 300 lines of prose will always find one, which makes the satisfied signal unreachable by construction on a comment-heavy diff. Possible fix recorded, not taken: scope a round to what changed since the last round and skip it when that is comments only. Petra's call. Both pull requests are now hers to read. CI: #18 24/24 green, #19 green bar one itest leg still running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
…open Petra decided a PR goes Open as soon as the rounds are done, whoever calls the stop, so #18 and #19 were un-drafted and #19 merged as 82aaa35. Rule 10 said the opposite in two places -- its headline and the two-signals bullet -- and forge#22 corrects both, opened without a round because it is a prose-only change to a prose file, which is the case the rule it adds says to skip. Her open question on #18 is recorded with my answer: run compat on the release pull request, yes, but keep the schedule. The pinned side moves independently of us -- compat-window.sh reads the registry live -- so an upstream release can break compat with no pull request of ours firing. The release PR answers whether our change broke it; only the schedule answers whether theirs did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
The Go test that replaced round one's shell guard still recognised less than
compose does: it matched only `${X:?msg}` and not `${X?msg}`, which compose
rejects identically, and it keyed its walk on the filename compose.yml, so an
included file with another name or a compose.yaml was invisible. Both proved
with the real client rather than argued. Now `:?\?` over every .yml and .yaml
under the smelt root.
The body also repeated #18's nested-path miscount -- "seven across
systems/*/compose.yml", a glob that matches thirteen files and reaches none of
the indexer's -- in the pull request that fixes the nesting.
And the round caught Current State: it said #18 was the only open forge pull
request, and a later paragraph still listed #19, which merged and is #23's
base. Both corrected; the open pair is now #18 and #23.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt
Checks a reviewer can merge without waiting for
Everything except
release / planandci / guards. Four files, not two — this block said two for several pushes and was wrong about which checks the diff can reach.release.yml— nothing in the tree reads or executes it. Every hit below is a prose mention inside a comment or a document:AGENTS.mdandMONOREPO_TODO.md— markdown; nothing compiles or parses either. Same grep shape, all hits in comments.check-assert-released-version.shIS executed, byci / guards(ci.yml:72). Soci / guardsis not skippable on this diff. The change to it is comment-only, which is checkable rather than asserted:What it costs if that is wrong: the ignored checks still run, and a red one after merge leaves
mainred.What this does
release.ymlwas dispatch-only, so the only thing that ever proved a service could be released was someone remembering to dispatch it. A pull request from arelease/<svc>branch now builds that service and asserts its version stamping. It publishes nothing, in either mode.The selector is the branch name. Firing on a change to
<svc>/version.jsonlooks more derived and is worse: a subtree resync carries upstream's own version bumps, so an unrelated pull request would pay for up to four release builds — piri's alone is 8m47s. Narrowing to "version bumps and nothing else" then excludes a release pull request that also edits a changelog. A branch name states the intent. It leaves a gap — bump a version elsewhere and nothing here looks — and that gap is where every service already is.An ordinary pull request costs one short
planjob, under twenty seconds every run so far. A bound rather than a figure, and that is deliberate: a single number was published first (the slowest sample, overstating by about half), the range that replaced it was stale by its own next push, because the run that measures the sentence always postdates it.What a reviewer should look at
The
inputstrap, which is why the mode is resolved once inplaninputsdoes not exist on apull_requestevent, soinputs.dry_runis null there and!inputs.dry_run— how bothif:s were originally spelled — evaluates true. Unchanged, a pull request would have runrequire the tagagainst a tag that by design does not exist yet, and passedGITHUB_TOKENto goreleaser.plannow resolves onedry_run, validated to exactlytrueorfalse; every downstream test reads that. All three reads are!= 'true', so any other string selects real-release mode, and an empty one is reachable — a dispatch through the API need not send the declared default. Measured:"",yesandTRUEall exit 1 writing nothing.A branch name reached a shell, and there is no longer any interpolation into a
run:in this fileresolve the versioninterpolated${{ steps.svc.outputs.dir }}into its script, anddiris derived from the branch name. Reproduced end to end:dirnow reaches the script byenv:, as the file's other steps already did, and no expression is interpolated into anyrun:in this file — checked by parsing the YAML rather than reading it. Unconditional and cheap, and it needed no threat model to justify.There is deliberately no fork condition, and that took two rounds to get right
A same-repository condition was added and then removed. Pull requests here come from the team, with push access; the only fork is
fil-forge/forge-2, our own archived copy of the migration.ci,e2e,imagesanditesthave all triggered onpull_requestsince before this file did, so a fork opening pull requests would already be spending 23 minutes ofitestbefore reaching three minutes of ingot here. Adding this trigger changed nothing about who can run what.Nor is there anything here to take. A
pull_requestfrom a fork runs in this repository's Actions with no secrets and a read-only token — which is whatpull_request_targetexists to provide, and why that event and not this one is the dangerous one. Audited rather than assumed:Public repo, GitHub-hosted ephemeral runners, nothing to exfiltrate. What remains is compute on free minutes.
The condition also could not do what it appeared to: a
pull_requestruns the workflow definitions from the pull request's own commit, so a fork's copy of this file is whatever the fork wrote and the condition is not in the code that runs. A guard that reads as total while covering nothing is rule 5's case for shipping none — and this one had already produced two wrong comments about itself in consecutive rounds.Where it would become real is arming publishing, which needs
packages: writeand a registry credential. Thepermissions:comment says what that costs.The new check: the version must not already be tagged
The mirror of
require the tag, inplan. It cannot fail today —git ls-remote --tags originis empty — and the comment says so rather than leaving a green check to imply otherwise. Verified in both directions against a scratch tag. A working clone disagrees misleadingly: adding the upstream polyrepos as remotes brings inv0.0.0,v0.2.4andv1.13.4, none of them this repository's and none on origin.Verification
releaseskippedb9fa229release/ingot→ builds and assertsb9fa229release/ingot, after thedry_runvalidation and the concurrency key68dd95dplan11s,releasegreen 3m35s,require the tagandpublishskippedThe third row exists because a gate that wrongly skips is green and silent: the runs at the later heads all came from ordinary branches, where
dir != ''short-circuits before reaching the new conditions.releaserunning at all is the evidence. Both were run from #20, a throwaway carrying these commits under arelease/ingotbranch name, now closed.Locally at the current head: the resolve step over every event/branch/input combination, including the injection branch above (rc 1, no file created);
dry_runof"",yes,TRUEall rc 1;actionlintclean; all sevencheck-*.shexit 0.Deliberately not here
Compat on a release pull request. It wants
compat.yml, which is open in #18. It follows once that lands.For a human
release/ingotandclaude/wiki-currentwant deleting by hand —git push origin --deletefails in this session, every attempt, for both refs.🤖 Generated with Claude Code
https://claude.ai/code/session_01CGAGAib517Ae1kg8SCdcEt