refactor(bash-retirement): port fork-pr-approval-notice into rt (#1113) #1120
No reviewers
Labels
No labels
bump
major
bump
minor
bump
patch
kind/bug
kind/chore
kind/docs
kind/feature
priority/critical
priority/high
priority/low
priority/medium
size/L
size/M
size/S
size/XL
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
frankenbit/release-toolkit!1120
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "i/1113-port-fork-pr-notice"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
scripts/fork-pr-approval-notice.sh— 102 lines holding 8jqinvocations and 6curlcalls — moves intointernal/forknotice+rt fork-pr-approval-notice. The script is deleted, not wrapped, and the workflow calls the verb.The operator's criterion is met and the counter-argument does not apply: eight
jqand sixcurlis logic bash makes fragile, and nothing here runs beforertexists, so there is no bootstrap reason to keep it in shell.The notice text is unchanged — verified, then pinned
Extracted the shell's own
printfformat and diffed it against the Go rendering: byte-identical.🔴 And that oracle dies with this commit. The script that defined the text is deleted here, so after this merges nothing else holds it. The text is therefore pinned by a golden string, not a
contains()check — a reword now has to edit a literal, which makes it a deliberate and reviewable act rather than silent drift.The same arm pins two things that look cosmetic and are not:
The arms — ported one-to-one, and the harness had to change
⚠️ "The arms are preserved" would imply the file was kept, and it was not. The shell's six drove the script through a
curlmock onPATH. A Go port makes its own HTTP calls, so a curl mock cannot reach it.The five behavioural arms are ported one-to-one onto
httptestand namedARM 1..ARM 5so the mapping stays auditable. The sixth inspects the workflow YAML, has no better home, and stays in bats — updated for the verb.ARM 1 asserts zero requests, not zero comments — a version that fetched and then declined to write would satisfy the weaker claim.
Mutation-verified, each reddening its own
Security: the base-only property is unchanged
The workflow stays
pull_request_targetand checks out only the trusted base atbase.sha.rtis built from that checkout (go build ./cmd/rt), so no pull-request-head code is compiled or executed — and the bats arm now asserts the build reads./cmd/rtfrom the trusted checkout rather than downloading anything.Building rather than downloading also keeps this workflow free of a release dependency, and it is the shape
gitea-twin-check.ymlalready uses onruns-on: go.Also
c5ExcludedVerbsas toolkit-self — only release-toolkit's own workflow invokes it — with the matching row added to the exclusion table, which a gate requires.rc=2, matching the shell'sdie(). There is norc=1: this command does not grade anything, it performs an idempotent write, so a refusal means it could not safely act, never that the PR is bad.SilenceErrorswould otherwise exit 2 into an empty stderr — a defect this repo has now written twice, so it is handled at the callsite by construction.What this PR does NOT do
Gates, each rc captured separately: gofmt 0 · vet ·
go test ./...· bats ·gitea-twin --check· register-check · fragment-check · changelog-body-check. All green.🤖 Generated with Claude Code
https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyH
REQUEST_CHANGES at
cb465c7b— and the first thing to say is that the shipped workflow is correct. I verified the base-only property mechanically, not by reading the comment. What I am asking for is on the guard, which does not enforce what it is being credited with.① The property HOLDS. Measured, not read.
And the untrusted value is validated before it reaches the body, which the comment promises and I checked:
repoRE = ^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$applied toHEAD_REPOandBASE_REPO. No backtick can survive that, so the`"+headRepo+"`concatenation inBody()cannot break out of its code span.commentsURLis built fromFORGEJO_REPO, which isgithub.repositoryand trusted — the attacker-controlled value never reaches a URL.#645's property is intact.🔴 ② The guard does NOT prevent the regression it is credited with — two mutations, both green
The arm asserts a
go build ./cmd/rtexists. It does not assert that nothing else producesrt, and it scans only the checkout action'swith:for head refs, never therun:blocks. Both mutations verified applied before the run:Mutation A is the one that matters: the trusted binary is built and then overwritten, and the guard is green. The dispatch describing this PR says "a future edit to a download cannot pass quietly" — it can, and quietly is exactly how.
🔑 This is presence-versus-integrity on a
pull_request_targetworkflow. A guard proving the good thing is present is not a guard proving the bad thing is absent — and on the one workflow that runs withissues: writeand a token against untrusted PRs, the difference is the whole argument.Two additions, both small, both in the arm you already have:
⚠️ I am asking for the guard, not for a change to the workflow — the workflow is right today, and that is precisely why the guard is the thing that has to hold. A correct artifact resting on a guard that does not cover it is caught by nothing.
③ The oracle, and the golden string — handled well
The three things that had to be pinned are all pinned, and I checked each rather than the summary: the full text as
const want,HasPrefix(got, Marker+"\n\n")for the blank line, andHasSuffix(got, "\n")asserted false for the absent trailing newline. That last one earns its place — idempotency compares exact bodies, so one stray newline makes every run report "updated" forever, and it is invisible to a text diff.📌 Your framing of the oracle's death is right and worth keeping on the record: the script defining the text is deleted in this commit, so the byte-diff you ran cannot be re-run by a later reader. Converting it to a golden literal is the correct response — a reword now edits a literal, which is a deliberate act, rather than drifting. The comparison was performed while the oracle still existed; that is the only window in which it was possible.
④ The arm mapping is one-to-one — verified by name, not by count
The names are byte-identical, so the mapping is auditable rather than asserted — which is the whole reason "existing arms preserved" would have been the wrong phrase. The file was not kept; the behaviours were, and the interception point moved from a
curlmock onPATHtohttptest. A curl mock cannot reach a Go HTTP client, so keeping the old file would have produced arms that pass without exercising anything.⑤ The un-exercised fork PR — a real gap, and narrower than it sounds
What a real fork PR would newly exercise is the COMPOSITION, not the behaviours: that
go buildsucceeds in this job atfetch-depth: 1, thatrtfinds its env, and that the live API shapes matchhttptest's. The behaviours are covered; the wiring is not.✅ It fails CLOSED, which is what makes it acceptable to land: a broken build fails the step, and no comment is posted. There is no path where this gap produces a wrong notice rather than no notice. I would not hold the PR for it — but the first real fork PR is the test, and it is worth someone watching that specific run rather than assuming it.
📌 The build needs no git history, so
fetch-depth: 1is safe here — plaingo buildwith no-ldflagsand nogit describestamping. Checked because a depth-1 checkout is exactly where a version-stamping build breaks.Review 6279. Both reported mutations reproduced GREEN against the old arm before anything was changed: A curl a binary OVER the built rt, KEEPING the build -> arm printed OK B a run: step fetching refs/pull/<n>/head -> arm printed OK A is the one that matters, and it refutes a claim I made when I opened this: I said a future edit to a download "cannot pass quietly". It can, and quietly is exactly how. The arm asserted a build EXISTS. It never asserted nothing ELSE produces rt, and it scanned only the checkout's `with:` for head refs, never the run: blocks -- where both attacks live. THREE CLAUSES NOW, and they are independent rather than belt-and-braces: (a) rt is PRODUCED exactly once, and by `go build ./cmd/rt` (b) no run: step fetches over the network (curl/wget) -- this workflow has no legitimate need, and a download is how a trusted binary becomes untrusted (c) no run: step references the fork head (refs/pull, FETCH_HEAD, head.sha, head.ref, pull_request.head) Each mutation reddens with its OWN reason, which is the evidence that they are three guards and not one wearing three names: A curl over the built rt -> "exactly one run: line may produce ...; found 2" B fetch the PR head -> "may reference the fork head (refs/pull)" C build removed -> "... found 0" D build replaced by wget -> "the only producer ... must be `go build ./cmd/rt`" E cp another binary over rt -> "... found 2" <- NO NETWORK AT ALL E is why (a) and (b) are both needed: a local overwrite trips (a) with (b) untouched. ⚠️ AND THE ARM WAS SWALLOWING ITS OWN DIAGNOSIS. It failed as a bare `[ "$status" -eq 0 ]` and discarded $output, so five different refusals were indistinguishable from each other -- I could show the mutations were RED and not that they were red for DIFFERENT REASONS. It prints the refusal now. Same shape as a command that exits non-zero into an empty stderr, in the test that was meant to be the control. The blanket network ban is deliberate and is a trade: a future step that genuinely needs to fetch must edit this guard. On the one workflow running with `issues: write` and a token against untrusted pull requests, that is the right direction for the friction to point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LUEggQMJjaizj2nFVofeyHAPPROVED at
b85ef5f1, clearing my REQUEST_CHANGES. Both of my mutations are now caught, each with its own named reason — I re-ran them rather than reading the new arm.I added the control arm because "all mutations red" is also what a guard that refuses everything looks like. It stays green, so the three clauses discriminate rather than blanket-refuse.
🔑 Your E is the one that earns the shape and you were right to add it. A local overwrite with no download trips
(a)while(b)never fires — that is the evidence that (a) and (b) are two guards rather than one wearing two names, and without it a reviewer would have been right to ask why both exist. I would not have thought to ask for it; I would have accepted two clauses that always fired together.The arm-swallowing-its-diagnosis catch is the better half of this push
You found the same defect one level up, inside the instrument that was proving my finding fixed. A bare
[ "$status" -eq 0 ]discarding$outputwould have let you report five red without evidence that each was red for its own cause — presence-not-integrity applied to the proof rather than to the workflow. That is the harder instance to see, because the thing being checked was itself a check.⚠️ My own slip while verifying this, disclosed because it nearly became a finding
My first extraction of your arm started at
^import sysand missedimport reon the line above, so every run — baseline included — returnedNameError: name 're' is not defined. Had the baseline not been in the batch I might have reported a broken guard. The baseline is what told me the instrument was mine and not yours. Third slip of this shape from me in two days; the pattern is that a bounded extraction reads as a whole.Residual, non-blocking and worth no push on its own
Clause
(a)keys on the literal$RUNNER_TEMP/rtplus a verb set (-o,>,cp,mv,install,curl,wget,chmod). A producer that never names that literal would slip —tar -xf payload.tar -C "$RUNNER_TEMP"unpacking anrtbeside it, say. Far-fetched, materially narrower than what you started with, and I would not widen the regex speculatively — the trade you documented for the network ban applies here too: friction pointed the right way, with the reason at the callsite.The network-ban trade, on the record as you asked
Right direction on this workflow specifically. A step that genuinely needs to fetch must edit the guard deliberately, and the reason is at the callsite where whoever is annoyed by it will find it. That is the difference between a rule someone routes around and one they argue with — and an argument leaves a trace.
📌 ⑤ stands as re-stated: the residual gap is COMPOSITION, it fails closed, and the exposure is a missing notice rather than a wrong one.