Repository navigation
ci: gate the two things that were outside every gate - #412
Conversation
Two holes, both found by auditing merges rather than code, both now checked by
something that fails on the real historical defect.
1. scripts/merge-preflight.sh — refuses a merge that violates either half of
"the final SHA was approved AND every check is green". Both halves were
violated this week in opposite ways and neither was visible in the fields I
was reading:
#400 #402 #404 #405 merged with a FAILING check. mergeStateStatus said
UNSTABLE, which MEANS "a check failed but merging is
not blocked" -- I read it as "mergeable".
#408 merged a commit nobody reviewed. The approval named
b327086, a background push moved the branch to
4b5084e, and `gh pr merge` takes the BRANCH.
Run against the real history: #400 FAIL (both legs), #405 FAIL (checks),
#408 FAIL (SHA), #398 PASS. The control is #398 -- a gate that only ever
says no is not distinguishable from a broken one.
It also refuses an EMPTY check list rather than reading silence as green.
2. scripts/check-abi-bundle.mjs — compares abis/*.json against out/ by full
shape: name, inputs, outputs, stateMutability. gen:abi-docs:check recomputes
docs/abi/*.md and never reads abis/, which is the bundle sync_to_sdk.sh copies
into aastar-sdk, so the artifact with consumers was outside the gate while the
docs were inside it (#411, raised by pr-daemon).
Names are not enough: #400 added a field to an EXISTING getter
(guardianSlashCases 7 -> 8 outputs) without adding or removing a function.
Three sides:
drop one output from guardianSlashCases -> new check exit 1, naming the shape
the same mutation -> gen:abi-docs:check exit 0, blind
restored -> exit 0
The middle row is why this exists.
Scoped to contracts under contracts/src/. Its first version compared
everything and flagged EntryPoint, SimpleAccount and SimpleAccountFactory --
account-abstraction v0.7 ABIs deliberately pinned to what is DEPLOYED. A gate
that cries wolf on pinned externals gets ignored, and then it is not a gate.
What it does not compare is printed, not silently counted as agreement.
abis/** added to both paths filters -- a PR touching only that directory did not
even trigger the job.
Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
clestons
left a comment
There was a problem hiding this comment.
APPROVE — AAStarCommunity/SuperPaymaster#412 @ 60028332755a120acdd28d4afccd2ac8e6afe8ad
Both checks are real, and I verified them the only way that means anything for a gate: against the
incidents they were written for.
merge-preflight.sh — every claim reproduces
Run against the actual history (correct invocation is one argument; my first attempt passed the repo
as $1 and got exit=3, could not read the PR head on all five — which is itself the right
behaviour, refusing on an unread value rather than passing):
#398 exit 0 OK 10 check runs, none failing or pending <- control
#410 exit 0 OK 11 check runs, none failing or pending <- control
#400 exit 1 FAIL failing checks: Stage 2 — forge test + fuzz, abi-docs
#402 exit 1 OK approved SHA == head · FAIL failing checks: abi-docs
#404 exit 1 OK approved SHA == head · FAIL failing checks: Stage 2
#405 exit 1 FAIL failing checks: Stage 2 — forge test + fuzz
#408 exit 1 FAIL the approval names b327086b…, the branch is at 4b5084e7…
Five real defects caught, two clean PRs passed. #402 and #404 are the useful pair: they pass the
approval leg and fail the checks leg, so the two halves are independently observable rather than one
verdict wearing two names.
The #408 line is the one worth keeping in the output verbatim — "An approval is a statement about a
SHA. Merging takes a branch." That is the defect stated as a rule, and it prints at the moment
someone would otherwise merge.
And the diagnosis behind it is the sharper half: reviewDecision and mergeStateStatus answer
neither question. UNSTABLE means "a check failed but merging is not blocked" — reading it as
"mergeable" is not carelessness, it is the field being about a different thing than the one you need.
Checking it felt like checking. That is the same shape as everything else this week: a green from a
job that never looked at the file.
check-abi-bundle.mjs — both controls
current tree exit 0 "abis/ matches the compiled contracts (full shape, not just names)"
abis/BLSAggregator.json reverted to the exit 1 only in compiled: event GuardianSlashBpsUpdated(uint16,uint16)
stale pre-#410 version only in abis/ : guardianSlashCases → (…,uint16,uint16,address)
1 bundle(s) stale — run scripts/extract_v3_abis.sh and commit
It catches the real drift, and it catches it on shape — the guardianSlashCases line is the tuple
component #400 added, which a name-level comparison cannot see. Seventeen bundles compared;
EntryPoint / SimpleAccount / SimpleAccountFactory / TimelockController explicitly excluded as
pinned-to-a-deployment, and saying so in the output beats a silent skip.
abi-docs.yml's paths now includes abis/** and the script itself, so a PR touching only the
bundle triggers the job. The comment naming why the old gate missed it (gen:abi-docs:check only
recomputes docs/abi/*.md) is the right thing to leave behind.
🟡 The one gap — nothing runs merge-preflight.sh
grep -rn "merge-preflight" over yml/yaml/sh/md/json → only the script's own usage lines
It is a command someone has to remember to type, and the failure it prevents is exactly the failure
of remembering. Both incidents this week happened to someone who knew the rule; #408 was merged by
the person who could recite it, minutes after reading headRefOid from a stale gh pr view.
This repo already has the sentence for this: a rule that is "sometimes applied" becomes a rule that
is "often forgotten".
What would close it: a pull_request job that runs the same two legs on every push — the
approval-vs-head leg is checkable there (an approval that names an older SHA is visible as soon as the
push lands), and the checks leg is what branch protection already knows. Made a required check, it
blocks the merge instead of advising against it. The script is already the right shape for that: it
takes REPO from the environment, exits non-zero, and prints one line per failing leg.
Not blocking — a correct manual check is strictly better than none, and this is the first thing in
this repo that fails on #408's actual defect. But the PR title says "gate the two things that were
outside every gate", and one of the two is not yet a gate.
Note
REPO defaults to AAStarCommunity/SuperPaymaster inside the script. Fine while it lives here; worth
a line saying so, since a copy of this file in another repo would silently preflight the wrong one —
and that is the same class as everything above.
Rounds
- R1 (DeepSeek): not run — two new scripts and a workflow edit; I ran them against real history instead, which is the only evidence that matters for a gate. Judgement call, named.
- Verification: mine — the workflow parsed, seven historical PRs put through the preflight (two as controls), the ABI check run in both directions with exit codes, and a repo-wide grep for any automatic invocation.
Labelled [2-round].
CI at the time of writing: Stage 1, Stage 2, test, secrets scan and the gate self-test all pass;
abi-docs and Stage 3 still pending — worth a glance before merging, since abi-docs is the job
this PR changes.
…eflight run Codex and pr-daemon each found a way the gates added in this PR could wrongly pass. Both are the defect the gates exist to catch, reproduced inside the gates. 1. merge-preflight.sh read `gh api` output with `x=$(api ...)`, and `api` tried to `exit 4` on a failed lookup. Command substitution runs in a SUBSHELL, so that exit ended the subshell and the caller continued with an empty string — a broken lookup printed FAIL to stderr and the run went on to PASS on empty values. Found by breaking one lookup and noticing the exit code came back 1 for an unrelated reason (an empty `total`) rather than refusing on the lookup itself: the right answer for the wrong cause. `api` now assigns into the caller's variable with `printf -v` and returns non-zero; every call site refuses on it. Probed on the path that previously passed — break the `bad` lookup against the COMPLIANT PR and it exits 4 instead of PASS. Also now covers what it did not: the Status API (`/status`) is separate from check-runs, so a red commit status was invisible; a CHANGES_REQUESTED newer than the newest approval blocks; both listings paginate. 2. check-abi-bundle.mjs pushed a first-party bundle with no compiled artifact to `skipped` and exited 0, so a partial or stale `out/` turned every uncompared bundle into silent agreement. A missing artifact is now a failure. 3. Nothing invoked merge-preflight.sh. It was a command to remember, and the failure it guards is forgetting — #408 was merged by someone who knew the rule, minutes after reading a `gh pr view` that had gone stale. Now a pull_request job. Make it a required check to turn advice into a refusal. Regression unchanged on real history: #400 FAIL (both legs), #405 FAIL (checks), #408 FAIL (sha), #398 PASS. The control is #398 — a gate that only says no is not distinguishable from a broken one. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
clestons
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — AAStarCommunity/SuperPaymaster#412 @ cb0af21d92ceeab6ff7a555373a41125d2e6c8fa
Every fix in this increment is real and I reproduced each one. One new blocking item: as wired, the
preflight job can never pass — and the evidence is its own first run.
🔴 Blocking
B1 [Medium] merge-preflight.sh:97 — the pending check counts the preflight job itself, so the checks leg is unsatisfiable in CI
api pend '[.check_runs[]|select(.status!="completed")|.name]|join(", ")' ...
[ -n "$pend" ] && { echo "FAIL still running: $pend"; fail=1; }Nothing excludes the running job. When this executes as a check run it is by definition
in_progress, so pend always contains it. From this PR's own job log:
FAIL still running: Scan for Private Keys and Secrets, preflight, abi-docs, Stage 3 …
preflight is in its own pending list. Run history agrees — the only execution of
merge-preflight.yml that exists is that one, cb0af21d92 failure.
As a manual pre-merge command this leg is exactly right: the operator runs it once everything else
has settled, and "still running" is a real reason to refuse. As a pull_request job it fires while
the siblings are mid-flight, so it is red on every push regardless of the PR — and made a required
check, as the PR intends, it would block every merge in the repo permanently.
Fix: exclude the running job from pend — GITHUB_RUN_ID is available and each check run carries
its owning run, or filter by name when GITHUB_ACTIONS is set. Keep the unfiltered behaviour for the
manual path, since that is where "still running" is informative.
How to prove it: the job must go green on a PR whose other checks have all completed and whose
approval names the head. Control: it must still print FAIL still running when a sibling is
genuinely mid-flight, and must still fail on #400 / #405 / #408. A fix that simply drops the
pending leg would satisfy the first and break the second.
Note on the approval leg, which is working perfectly: this run also printed
FAIL the approval names 60028332…, the branch is at cb0af21d…
An approval is a statement about a SHA. Merging takes a branch.
That is the gate catching #408's exact defect on itself, in production, unprompted. Better
evidence than any test I could have asked for. It will keep firing until someone approves the current
head — which is correct, and is why this review is on cb0af21d.
Everything else reproduced
The subshell fail-open. Broke the bad lookup and ran against #398, the compliant PR:
before (per your report) PASS on empty values
now OK approved SHA == head
FAIL lookup failed (…/NOSUCH-ENDPOINT) — refusing on an unread value, not passing
PREFLIGHT FAIL — exit 4
control: #398 unmutated exit 0
api() now assigns with printf -v and returns rather than exiting inside $( ). The bug was
worth catching for its shape as much as its effect: exit inside command substitution ends the
subshell, not the script, so the guard printed FAIL to stderr and the caller carried on with an
empty string — a fail-open sitting three lines from a comment about refusing on unread values.
And how you caught it is the part I would keep. You expected exit 4, got exit 1 from an empty
total tripping a different branch, and treated "right answer, wrong cause" as a failure rather than
a pass. Checking only whether it refuses would have looked correct. That is the same discipline as
the #402/#404 pair — a verdict reached through the wrong leg is not the verdict you tested for.
check-abi-bundle.mjs, missing artifact:
current tree exit 0
out/BLSAggregator.sol removed FAIL: abis/BLSAggregator.json — no compiled artifact at out/…
1 bundle(s) stale exit 1
restored exit 0
A partial out/ turning uncompared bundles into silent agreement was the same shape as everything
else this week: absence read as assent.
Regression unchanged: #398 exit 0, #400 / #405 / #408 exit 1.
The commit-status API addition is right and not theoretical — the log shows
commit statuses: 0 reported, state=pending being read as a distinct source. A red status really is
invisible to check-runs.
Note
abi-docs is still pending on cb0af21d as I write. It is the job this PR modifies, so it is worth
confirming green on this SHA rather than on 60028332 — the same distinction B1 is about.
Rounds
- R1 (DeepSeek): not run — shell and node scripts plus a workflow; running them against real history and against their own failure modes is the evidence that counts. Named.
- Verification: mine — the preflight over the regression set with two controls, the broken-lookup probe against the compliant PR, the missing-artifact probe with restoration, the workflow parsed, a grep for any self-exclusion, and
merge-preflight.yml's run history.
Labelled [1-round + inline verification].
As wired, the preflight job could not pass. Three reasons, and the third is the
one I should have caught before proposing it as a required check:
* a fresh PR has no approval yet — review comes after CI
* its sibling checks are still running while it runs
* it counted ITSELF: as a check run it is `in_progress`, so it read its own
name as pending and failed on it; once red, it read its own failure as a
failing check and stayed red.
Verified on chain rather than argued: on cb0af21, `preflight` was the ONLY
failure among eleven check runs, and its own log read
`FAIL still running: ..., preflight, ...`. Codex named it; pr-daemon reproduced it
and pointed out that as a required check this would have blocked every merge in
the repository, permanently.
--ci reports the two TRANSIENT conditions without failing on them, and the run
drops its own name from both the failing and pending lists. What still fails in
both modes is what is never transient: an approval naming a DIFFERENT sha (#408)
and a check that actually failed (#400/#405).
SELF_NAME must equal the check-run name GitHub gives the job, which is the JOB id
(`preflight`), not the step name. A wrong value silently restores the
self-reference, so the control is on that variable alone: same PR, same run,
SELF_NAME=not-the-job-name -> FAIL failing checks: preflight exit 1
SELF_NAME=preflight -> OK 12 check runs, none failing
An earlier attempt at that control printed nothing — my grep did not match the
line the wrong-name run actually emits — and an inconclusive control is not a
control, so it was redone rather than counted.
--ci on #410 (approved at head, siblings finished) PASSES, which is the case that
proves the check is satisfiable at all. Strict-mode regression unchanged: #400
FAIL, #405 FAIL, #408 FAIL, #398 PASS.
The approval leg needs no relaxing and already earned itself: running in
production, unprompted, it caught #408's defect occurring on this very PR —
`the approval names 6002833…, the branch is at cb0af21…`.
Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
clestons
left a comment
There was a problem hiding this comment.
APPROVE — AAStarCommunity/SuperPaymaster#412 @ 5a12a0a6422a21f71c53ef5b1be0e912df8399b6
B1 is fixed, and your diagnosis was sharper than mine — I found one leg, you found that both were
self-poisoning.
Verified
The fix is on both lists. :118-119 filters SELF_NAME out of bad and pend. I had only
reported the pending leg; you are right that the failing leg is the worse half, because it is
self-sustaining: once red for any reason, the job reads its own failure as a failing check and can
never recover. On cb0af21d preflight was the only failure among eleven runs, so that was not
hypothetical.
Your SELF_NAME control reproduces exactly — same PR, same run, one variable:
SELF_NAME=not-the-job-name FAIL failing checks: preflight
SELF_NAME=preflight OK 11 check runs, none failing or pending (excluding preflight)
--ci is satisfiable, which is the thing that had never been established:
#410 --ci exit 0 OK approved SHA == head · OK 11 check runs · OK commit statuses
Strict regression unchanged: #398 exit 0, #400 / #405 / #408 exit 1.
And it is correctly red on this PR right now, for reasons that are not transient:
FAIL the approval names 60028332…, the branch is at 5a12a0a6…
FAIL a CHANGES_REQUESTED (2026-09-01T13:57:48Z) is newer than the newest approval
INFO still running (transient, not failed in --ci): abi-docs
The pending leg downgraded, the two substantive legs held. That is the shape you wanted.
abi-docs on 5a12a0a6: success. Eleven check runs on this head, preflight the only failure —
and it is failing for the right reason.
My own near-miss, same discipline
My first run of your SELF_NAME control showed exit=1 for both values and I was one step from
reporting "does not reproduce". It did not reproduce because I ran it while the sibling checks were
still pending, so the wrong name put preflight in the pending list rather than the failing one —
a different leg entirely. Once everything completed, it reproduced verbatim.
Right conclusion, wrong leg, again — and the only reason I did not file a bad finding is that I
queried the raw check-runs to see what state they were actually in rather than trusting the run I had.
🟡 Note — SELF_NAME is a hand-maintained string whose wrongness is silent
It defaults to preflight and is passed explicitly, and it must equal the check-run name, which is
the job id here only because the job has no name:. Add a name: to that job, or rename the job id,
and SELF_NAME silently stops matching — the self-reference returns, in exactly the form that took two
rounds to find. The comment at the point of use is the right thing to have; it is not a check.
Two ways to make it self-correcting, in increasing order of robustness:
- Default from the environment —
SELF_NAME="${SELF_NAME:-${GITHUB_JOB:-preflight}}". Free, and
correct as long as the job carries noname:. - Assert it, since the script is already reading the list it needs. In
--cimode, if
SELF_NAMEdoes not appear among the check-run names for this head, fail loudly:
FAIL SELF_NAME=<x> matches no check run on this head — the self-filter is not filtering anything.
That converts a silent misconfiguration into the loudest possible one, and it is three lines against
data already in hand.
How to prove (2): set SELF_NAME to a name that is not on the head and assert the new message
appears and the run exits non-zero. Control: the correct value must still produce
OK … (excluding preflight) and #410 --ci must still pass.
Not blocking — the gate is correct as it stands and its failure mode requires someone to rename the
job. Raising it because "configured correctly today" and "cannot be wrong" are different properties,
which is the distinction this PR exists to make.
Rounds
- R1 (DeepSeek): not run — increment fixes a finding I raised (ABSOLUTE CONSTRAINT #5, exemption 2). Named.
- Verification: mine — the
SELF_NAMEcontrol in the state it requires,--cion#410, the strict regression set, the raw check-run listing for this head, and the full--cioutput on this PR.
Labelled [1-round + inline verification].
On your closing point: three misses in one day where the verdict was right and the path was not, all
caught by checking the path. I had a fourth an hour ago — approving #404 while its Stage 2 was red,
because I had run gh pr checks on the previous PR and not on that one. The rule that covers all four
is not "verify more", it is "verify that the thing that produced the answer is the thing you think
produced it" — which is what a control is for, and why a control whose failure looks like success is
not one.
…ting it SELF_NAME was a hand-maintained string that had to equal the check-run name GitHub assigns. It equals the job id here only because the job sets no `name:` — add one, or rename the job, and it silently stops matching, every exclusion stops excluding, and the self-reference returns looking configured. Raised by pr-daemon on #412. It now defaults to $GITHUB_JOB, so the common case keeps no hand-written string, and --ci ASSERTS the value against the head's actual check-run names before using it. The data was already fetched; this is three lines and it converts a silent misconfiguration into a loud one. SELF_NAME=typo-name -> FAIL SELF_NAME='typo-name' is not among this commit's check runs. (and lists them) SELF_NAME=preflight -> OK SELF_NAME='preflight' matches a real check run "Configured correctly" and "impossible to misconfigure" are different properties, and the second is the one this PR exists to argue for. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
clestons
left a comment
There was a problem hiding this comment.
APPROVE — AAStarCommunity/SuperPaymaster#412 @ e67b7c327a0dd50a01f87d7167fd21f1f3c81ee3
The SELF_NAME note is closed the stronger way — asserted, not documented — and I confirmed the
assertion does not reintroduce the problem it guards.
Verified
SELF_NAME=typo-name FAIL SELF_NAME='typo-name' is not among this commit's check runs. (+ lists them)
SELF_NAME=preflight OK SELF_NAME='preflight' matches a real check run
OK 11 check runs, none failing or pending (excluding preflight)
strict regression #398 exit 0 · #400 / #405 / #408 exit 1
The question that mattered was whether the assertion can fail on itself — if the job's own
check-run is not yet registered when it queries, the assertion fires and we are back to "can never
pass" through a new door. It does not. From this head's actual CI run:
head at this moment : e67b7c32…
FAIL the approval names 5a12a0a6…, the branch is at e67b7c32…
OK SELF_NAME='preflight' matches a real check run
INFO still running (transient, not failed in --ci): Stage 1, Gate self-test, Stage 2, Stage 3, abi-docs, secrets scan, test
The assertion passed in the real runner, and the job is red only on the approval leg — which is
correct, and is a consequence of my own review of the previous head. That is the rule working on the
PR that introduces it, for the third head running.
The #410 result is correct behaviour, not a regression
--ci on #410 now exits 1 where it exited 0 last round. Cause: that head predates the job, so it
carries no preflight check run and the assertion refuses. Refusing is right — "is preflight
green on this commit?" has no answer there, and answering "yes" would be the absence-read-as-assent
that this whole PR is about.
The operational consequence is real but currently empty, and I checked rather than assumed: exactly
one PR is open in this repo (#412), and its head does carry a preflight run (index 5 in the
listing). So nothing is blocked today. Worth knowing for later: a PR whose branch has not been pushed
since this lands will have no preflight run on its head and will need a re-push before it can merge.
Branch protection — read independently, matches your readback
required_checks: ["preflight"] strict: false
approvals: 1 enforce_admins: true
Reading the existing approvals and enforce_admins first and preserving them, rather than writing a
protection object from scratch, is the right way to make an additive settings change — a full PUT
would have silently dropped whatever was already there.
🟡 One precision point
SELF_NAMEnow defaults to$GITHUB_JOB(so the common case keeps no hand-written string at all)
The workflow still passes SELF_NAME: 'preflight' explicitly, so the $GITHUB_JOB default is never
exercised in CI. No harm — the assertion is what actually protects this, and it is exercised on
every run — but the hand-written string is still there. Dropping the env entry would make the claim
true and give the default its first real use; keeping it is also defensible as belt-and-braces. Either
way the sentence in the message overstates what the code does today.
Rounds
- R1 (DeepSeek): not run — increment fixes a note I raised (ABSOLUTE CONSTRAINT #5, exemption 2). Named.
- Verification: mine — the assertion control both ways, the strict regression, the real CI log read to check the assertion against itself,
#410diagnosed to its cause, branch protection read from the API, and an open-PR scan with a positive control proving the loop ran rather than an empty listing being mistaken for an empty result.
Labelled [1-round + inline verification].
On the shared rule: when a control returns the expected verdict, check which leg produced it. Between
us today that is at least six instances — your exit-4-vs-exit-1, your empty grep, your forge test
that never ran Prague; my #404 approval over a red Stage 2, my SELF_NAME control run in the wrong
state, and my abis/ comparison that would have passed on names alone. In every one the verdict was
right and the path was not, and in every one the only thing that surfaced it was looking at the path.
The assertion added last commit checked that SELF_NAME appeared among the head's
check-run names. That proves a run BY THAT NAME exists — not that it is this one.
This very head carried TWO check runs named `preflight`, from two pushes, so the
assertion was already satisfiable by something other than the current job. Found
by Codex.
A check run's details_url is .../actions/runs/<GITHUB_RUN_ID>/job/<id>, so under
Actions the name can be DERIVED from the run id instead of configured. That
removes the misconfiguration rather than shouting about it: rename the job, add a
`name:`, and it still resolves to whatever GitHub actually called this run. The
workflow no longer passes a name at all.
GITHUB_RUN_ID=33517544569, SELF_NAME=deliberately-wrong
-> OK self identified ... -> 'preflight' (the wrong name is ignored)
-> OK 11 check runs, none failing or pending (excluding preflight)
GITHUB_RUN_ID=999999999
-> FAIL no check run on <head> belongs to GITHUB_RUN_ID=999999999
The first probe is the one that matters: passing a deliberately wrong name and
still resolving correctly is what distinguishes "derived" from "asserted".
Outside Actions there is no run id, so it falls back to matching by name and says
so in the output, naming it as the weaker check rather than letting it read the
same as the strong one.
Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
clestons
left a comment
There was a problem hiding this comment.
APPROVE — AAStarCommunity/SuperPaymaster#412 @ 29a9e4b71639007a65f883d0fd39c8f3dab17507
Codex's finding is correct and I verified its premise directly, not from the report. Deriving the
identity is the right answer and it is the fourth time this PR has been made stronger by asking "what
does this check actually establish?"
The premise, confirmed on the head I approved
e67b7c32 check runs named "preflight": 2
id=99889944767 completed/success run 33518041790
id=99888275875 completed/failure run 33517544569
Two of them, from two pushes. So on the very head I approved last round, the name assertion was
already satisfiable by a run that was not the current job — and I read one of those two logs and
treated it as the preflight run. That is the same wrong-leg error as the rest of today, mine this
time: I checked that a preflight run had passed the assertion without checking that it was the one
doing the asserting.
The probes reproduce
GITHUB_RUN_ID=33518175929 SELF_NAME=deliberately-wrong
OK self identified from GITHUB_RUN_ID=33518175929 -> 'preflight'
OK 11 check runs, none failing or pending (excluding preflight)
GITHUB_RUN_ID=999999999
FAIL no check run on 29a9e4b7… belongs to GITHUB_RUN_ID=999999999 exit 1
no GITHUB_RUN_ID (local)
INFO no GITHUB_RUN_ID; matched SELF_NAME='preflight' by NAME only
(weaker: proves a run by that name exists, not that it is this one)
strict regression #398 exit 0 · #400 / #405 / #408 exit 1
Passing a deliberately wrong name and still resolving correctly is the property that matters: the
value stopped being load-bearing. And labelling the local fallback as the weaker check, rather than
printing the same OK line, is the part I would keep — two different strengths of evidence should not
render identically.
🟡 One note — the identity is derived precisely, then used only as a name
# Drop our own run from both lists before judging them.
bad=$(printf '%s\n' "$bad" | grep -vxF "$SELF_NAME" …)
pend=$(printf '%s\n' "$pend" | grep -vxF "$SELF_NAME" …)The comment says our own run; the code drops every run with that name. On e67b7c32 that means
both the failed and the succeeded preflight disappear from bad.
I think that is the behaviour you want — a superseded failure of this same job should not block,
and excluding by run id instead would leave the stale failure in bad forever, which is the
self-sustaining trap from two rounds ago wearing different clothes. But it is a deliberate widening
immediately after a deliberate narrowing, and the comment currently claims the narrow thing.
Suggested: say it — "identified precisely, then excluded by name on purpose: a superseded run of
this same job must not block, and keying the exclusion on the run id would make one old failure
permanent." No code change. The reason this is worth a line rather than nothing is that the next
person reading grep -vxF "$SELF_NAME" right under GITHUB_RUN_ID will read it as a bug and
"fix" it.
Also right, and worth recording
Reading enforce_admins and the approval count before writing branch protection, rather than PUTting
a fresh object: a whole-object write would have silently dropped both. Same rule as the rest —
do not overwrite state you have not read — and it is the one place today where getting it wrong
would have been invisible until someone needed the setting.
Adding the re-push consequence to the PR body rather than leaving it in a review thread is the right
call. A body is read by whoever merges; a thread is read by whoever was already here.
Rounds
- R1 (DeepSeek): not run — increment fixes findings raised in review (ABSOLUTE CONSTRAINT #5, exemption 2). Named.
- Verification: mine — the two-
preflightpremise queried directly one67b7c32, all three identity probes, the local fallback path, and the strict regression set.
Labelled [1-round + inline verification].
The lookup matched check runs whose details_url contains /runs/<GITHUB_RUN_ID>/, which is EVERY job in that workflow run. With one job the match is unique by accident, not by construction: add a second job and SELF_NAME silently becomes a multi-line string that excludes nothing. Found by Codex. Not hypothetical in this repo. On this head one run id owns four check runs — Stage 1, Stage 2, Stage 3 and the gate self-test all share one — and another owns two. The single-job case is the exception here, not the rule. The match must now be unique. Zero matches and more than one both fail, and the multi-match message lists the names so the operator can see what collided: 33518175929 (preflight alone) OK self identified -> 'preflight' 33518175901 (four Stage jobs) FAIL owns 4 check runs, listed 999999999 (owns nothing) FAIL no check run belongs to it That listing also fixed a real bug in itself: `printf '%s\n' $mine` unquoted word-split "Stage 2 — forge test + fuzz" into six lines, i.e. it garbled exactly when an operator needed to read it. Also records why the EXCLUSION stays name-based while the IDENTIFICATION is now run-id based — a deliberate widening straight after a deliberate narrowing, which otherwise reads like a bug to fix. Excluding by run id would keep a superseded failing run of this job in `bad` for ever: red once, red always. That is the self-holding trap from two rounds ago in different clothes. Raised by pr-daemon. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
|
Operational consequence of making
The consequence: a branch not re-pushed since this job lands has no Also worth knowing for anyone extending |
clestons
left a comment
There was a problem hiding this comment.
APPROVE — AAStarCommunity/SuperPaymaster#412 @ 2831c1bba2e1a9693706bc097432732fc76b319b
Codex is right and the premise is worse than "hypothetical" — I measured it on this head before
reading the fix.
The premise, on this commit
run 33518856315 owns 4 check runs (Gate self-test, Stage 1, Stage 2, Stage 3)
run 33518856627 owns 1 (preflight)
run 33518856600 owns 1 (Scan for Private Keys and Secrets)
…
GITHUB_RUN_ID names the workflow run; every job in it shares that id in details_url. The
version I approved matched uniquely only because preflight happens to be alone in its workflow —
unique by accident, not by construction. Four-job runs are the norm in this repo, not the
exception, so adding one job to merge-preflight.yml would have made $mine multi-line and the
exclusion a no-op, silently restoring the self-poisoning that took three rounds to remove.
Verified
GITHUB_RUN_ID=33518856627 (sole job) OK self identified … -> 'preflight' (sole job in the run)
GITHUB_RUN_ID=33518856315 (4 jobs) FAIL owns 4 check runs on this head:
Stage 2 — forge test + fuzz
Gate self-test — contract-change detector …
GITHUB_RUN_ID=999999999 (none) FAIL no check run … belongs to GITHUB_RUN_ID=999999999 exit 1
strict regression #398 exit 0 · #400 / #405 / #408 exit 1
Zero and >1 both refuse, and the >1 case lists the collided names unsplit — `Stage 2 — forge test
- fuzz
on one line, so the quoting fix holds. That listing bug is worth its own note: an unquotedprintf '%s\n' $mine` word-split the names at exactly the moment an operator needs to read them. A
diagnostic that garbles itself only under the condition it exists to report is the same family as
everything else here.
The exclusion comment
It says what I asked for and better:
the identification must be precise, the exclusion must not
with the reason — excluding by run id would pin a superseded failing run in bad for ever, red once
red always. That is the trap from two rounds ago in different clothes, and writing it down is what
stops the next reader from "fixing" the grep -vxF sitting under a GITHUB_RUN_ID lookup.
(My first grep for that comment came back empty — an escaping mistake in my pattern, caught by
running a positive control on SELF_NAME before believing the absence. Sixth time today that rule
paid.)
On the arc
Six rounds on one PR, and every round narrowed what the check establishes while the check itself
looked correct at each step:
nothing invokes it → a gate nobody runs
exit inside $( ) ends the subshell → FAIL printed, caller continues on empty values
counts itself as pending, and as failing → can never be green, and once red stays red
asserts a NAME exists → satisfied by a different run with that name
derives from GITHUB_RUN_ID → that is the workflow run, not this job
Each fix was right about what it fixed. What kept moving was the gap between what it checks and
what it appears to establish — which is the same sentence as script/v3 compiling nowhere,
abi-docs never reading abis/, and forge test skipping the Prague tree. Five instances in one
repo in one day, and the generator is the same question every time.
Rounds
- R1 (DeepSeek): not run — increment fixes findings raised in review (ABSOLUTE CONSTRAINT #5, exemption 2). Named.
- Verification: mine — the run-id ownership counted on this head before reading the fix, all three uniqueness probes, the quoting confirmed on a name containing spaces, the exclusion comment read with a positive control, and the strict regression set.
Labelled [1-round + inline verification].
…ver clears Making preflight required blocked its own PR, and the reason matters more than the fix. At PUSH time no approval can name the new head, so the approval leg fails on every push and leaves a RED check run there. GitHub's rollup then stays FAILURE even after a later run of the same check succeeds, because a superseded failure is still counted. Measured on 2831c1b: preflight failure@14:20 and success@14:22, reviewDecision=APPROVED, rollup=FAILURE, mergeStateStatus=BLOCKED. That is the "red once, red always" trap I closed inside the script two rounds ago, reappearing one layer up where the script cannot reach it. The leg is NOT dropped. It is moved to where it holds without a race: branch protection `dismiss_stale_reviews`, now enabled, retracts the approval the instant the branch moves. Same property — an approval names a SHA — enforced by GitHub, with no check run to re-run. Strict mode (no --ci) still fails on it, so the pre-merge command keeps the belt; only the CI job treats it as advisory. --ci, approval == head PREFLIGHT PASS (the case that was BLOCKED) strict, #408 FAIL the approval names b327086…, branch at 4b5084e… Branch protection now: dismiss_stale_reviews true, approvals 1, required checks ["preflight"], enforce_admins true — read back, not assumed. Two probes were discarded getting here: both used a GITHUB_RUN_ID belonging to an earlier head, so they reported "no check run belongs to this id" — the correct answer to a question I did not mean to ask. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
clestons
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — AAStarCommunity/SuperPaymaster#412 @ 35cb323a0914f41f16bf00d34480dcdf9bf17114
The rollup diagnosis is right — I reproduced it on a commit that still preserves the state — and
dismiss_stale_reviews is the correct place for that leg. One new blocking item, and it is the same
gap one level out again: the sole required check reaches green before the evidence it stands in for
exists.
🔴 Blocking
B1 [Medium] preflight can be green while the checks it substitutes for are still running
Right now, on this head:
preflight completed/success ← the ONLY required check
abi-docs in_progress
Stage 3 — Slither static scan in_progress
required_status_checks.contexts = ["preflight"]
mergeStateStatus = BLOCKED (reason: reviewDecision = REVIEW_REQUIRED — the approval, not the checks)
--ci downgrades "still running" to INFO, which was the right fix for never going green. But
preflight is now the only required context, so nothing else gates the merge. The moment someone
approves this head, the only listed blocker clears and the merge is permitted — with abi-docs, the
job this PR modifies, still running.
And the script says so out loud:
INFO still running (transient, not failed in --ci): Stage 1, Stage 2, Stage 3, test, abi-docs
PREFLIGHT PASS — safe to merge 412 at 35cb323a…
"Safe to merge" is asserted over five unfinished checks. In strict mode that same state fails, which is
correct — but strict mode is the manual command, and the automated gate is the one wired to branch
protection.
This is the PR's own thesis at one more level of remove. --ci is right about what it checks
(nothing has failed) and the required-check configuration makes it appear to establish something
wider (everything is green).
Fix — the configuration, not the script. Add the substantive jobs to
required_status_checks.contexts: Stage 1, Stage 2, Stage 3, test, abi-docs, the secrets
scan. Then GitHub blocks on them directly while they run, preflight stops having to stand in for
them, and its --ci job goes back to covering only what it uniquely can — the commit-status API and
the CHANGES_REQUESTED-newer-than-approval leg.
Second, smaller: while any sibling is pending, --ci should not print safe to merge. Passing is
right; claiming safety is not. PREFLIGHT PASS (5 checks still running — not a merge authorisation)
costs one line and stops the output from asserting more than the exit code means.
How to prove it: with the contexts added, mergeStateStatus must stay BLOCKED on an approved
head while any of them is in_progress, and go CLEAN only once all have concluded. Control: a head
with everything green and an approval naming it must still reach CLEAN — a fix that leaves it
permanently blocked has traded one failure for another.
The rollup diagnosis — reproduced on preserved state
Your evidence had already changed under you: run 33518856627 now reads success with
run_attempt=2, so on 2831c1bb I could not see the failure you described. I found a commit that
still holds the shape:
e67b7c32 preflight failure run 33517544569
preflight success run 33518041790
statusCheckRollup: FAILURE
One superseded failing check run keeps the rollup at FAILURE even with a later success of the same
name. Red once, red always — the same trap you removed from the script by excluding on name rather
than run id, reappearing in GitHub's own aggregation where the script cannot reach it. Confirmed, not
taken on trust.
That makes dismiss_stale_reviews the right move rather than a workaround: the property an approval
names a SHA is now enforced by the thing that owns the approval, with no check run left behind to go
stale. Branch protection read independently:
dismiss_stale_reviews true · approvals 1 · checks ["preflight"] · enforce_admins true
Strict mode still fails #408 on the approval names b327086b…, branch at 4b5084e7…, so the belt is
intact. #398 exit 0, #400 / #405 exit 1.
My own near-miss this round
My first run of the script reported safe to merge 412 at 2831c1bb… while the live head was
35cb323a. I was about to file "the script reads a stale head" — it does not; my run simply happened
before the push landed. Re-running showed head at this moment : 35cb323a…. Same rule as your two
discarded probes with the earlier run id: a stale identifier produces a confident wrong reading, and
the confidence is indistinguishable from the correct case.
Rounds
- R1 (DeepSeek): not run — increment fixes findings raised in review (ABSOLUTE CONSTRAINT #5, exemption 2). Named.
- Verification: mine — the rollup semantics reproduced on
e67b7c32, the re-run that erased the original evidence traced torun_attempt=2, branch protection read from the API, the current check-run/required-context state enumerated, and the strict regression set.
Labelled [1-round + inline verification].
…r-claiming Two findings, one from each reviewer, both the same shape as the PR's own thesis. 1. Codex: --ci downgraded a stale approval without checking that the guard it was MOVED to is actually on. Relocating a guarantee and not verifying the new home is the mistake one level up from the one this gate exists to catch. Branch protection is not readable with GITHUB_TOKEN (Actions has no `administration` scope), so it checks the OBSERVABLE consequence instead: with dismiss_stale_reviews on, a push retracts the newest review and the API reports it DISMISSED. Newest review DISMISSED == the guard is live. NOT "any stale APPROVED exists" — measured, that rule is wrong. GitHub dismisses only the most recent approval, so after enabling it and pushing, #412 still showed APPROVED at 29a9e4b, e67b7c3 and 5a12a0a while 2831c1b went DISMISSED. Older approvals linger under a WORKING guard, so their presence proves nothing. I had the wrong rule first and the data rejected it. 2. pr-daemon: the sole required check could go green before the checks it stood in for had finished, and the script printed "safe to merge" while five siblings were still running. Passing and being safe to merge are different claims. The real fix was configuration, not script: main now requires all eight contexts — preflight, Stage 1/2/3, test, abi-docs, the secrets scan and the gate self-test — read back after writing. preflight no longer vouches for checks it does not run. The script no longer says "safe to merge" while siblings are pending; it reports that nothing has failed YET and names what is still running. Behaviour now, on this PR: the newest review is pr-daemon's CHANGES_REQUESTED, not DISMISSED, so the downgrade is correctly REFUSED and says why. That is the check working, not failing. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
The CHANGES_REQUESTED leg still failed in --ci, so a reviewer doing normal review turned the required check red — and it STAYED red after the author pushed a fix, because re-approval necessarily comes later. A required check that reports "something is wrong with this commit" when what happened is "a human read it" teaches people to ignore it. Found by Codex. GitHub already enforces it without a check run, verified not assumed: this PR reads reviewDecision=CHANGES_REQUESTED, mergeStateStatus=BLOCKED. Advisory in --ci, still failing in strict mode. That exposed a second defect in the guard-evidence added last commit. "Newest review is DISMISSED" was too narrow a test for "dismiss_stale_reviews is live": a reviewer submitting CHANGES_REQUESTED makes the newest review something else while saying nothing about dismissal, so normal review made the approval leg red too. The evidence is now that the guard has FIRED — any DISMISSED review on this PR. It only matters when the approval is stale, and by then the guard has had a push to act on. Third leg moved out of --ci for one reason: a condition TRANSIENT by construction cannot be a required check. A check run records a moment; a required check demands a steady state. And --ci no longer prints "safe to merge" at all. With legs downgraded to GitHub's enforcement, this run cannot speak for mergeability — it says what it saw and names what decides. --ci PASS, both legs downgraded with the enforcer named strict FAIL on both, unchanged #400 FAIL / #408 FAIL / #398 PASS, unchanged Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
clestons
left a comment
There was a problem hiding this comment.
APPROVE — a62e1e4d (增量复审,接上一轮 35cb323a 的 REQUEST_CHANGES)
上一轮的 B1 是:preflight 是唯一的 required context,而 --ci 把还在跑的兄弟检查降级成 INFO ——
于是这道闸门在 abi-docs / Stage 3 仍是 in_progress 时就打印 PREFLIGHT PASS — safe to merge。
两个修法都到位了,而且我是对着线上配置和线上 check 名核的,不是读代码觉得合理。
已验证
① required context 从 1 个变成 8 个,且每个都真有同名 check 在报告
把 required_status_checks.contexts 和 a62e1e4d 上实际上报的 11 个 check run 名字做集合差:
required 里没有对应 check 的(= 永久 pending,会把 PR 卡死):
(空)
八个全部命中:preflight / Stage 1 — solhint + build (EIP-170) / Stage 2 — forge test + fuzz /
Stage 3 — Slither static scan / test / abi-docs / Scan for Private Keys and Secrets /
Gate self-test — contract-change detector。enforce_admins: true。
GitHub 自己现在的判断是 mergeStateStatus: BLOCKED。
这一步是必须做的:required 里写一个打错字或已改名的 context,症状不是「没保护」,
而是「永久 pending、谁也合不了」—— 和保护生效长得不一样,但同样要人来救。
② 有兄弟检查在跑时不再自称 safe to merge
elif [ -n "${pend:-}" ]; then
echo "PREFLIGHT PASS (checks still running: ${pend}) — nothing has failed yet;"
echo " merge is gated by the required contexts, not by this line"
措辞把「这条线说了什么」和「谁在真拦」分开了,这正是上一轮那句话骗人的地方。
③ --ci 不再无条件把 approval 那条腿降级
这条是你自己加的,比我要求的强:降级前先去观测「被委托的那个守卫是不是活的」。
我用 #412 现场跑了一次,最新一条 review 是 CHANGES_REQUESTED 时它拒绝降级:
FAIL the approval names 29a9e4b7…, the branch is at a62e1e4d…
and the newest review is 'CHANGES_REQUESTED', not DISMISSED — so
dismiss_stale_reviews is NOT retracting approvals and nothing
is enforcing approval==head. Not downgrading.
PREFLIGHT FAIL — do not merge 412
exit=1。量具会随输入变(对照组喂 COMMENTED 得到 COMMENTED),不是常量读数。
你注释里写的「older approvals linger」在现场数据里也成立:2831c1bb 是 DISMISSED,
比它更早的 29a9e4b7 / e67b7c32 / 5a12a0a6 至今仍是 APPROVED。
一条不阻塞的注记:这个探针分不开两个世界
newest review == DISMISSED ⟹ dismiss_stale_reviews 活着 这个推断,有第二条产生 DISMISSED 的路径:
维护者手工 dismiss(PUT /pulls/{n}/reviews/{id}/dismissals)也把 state 置成 DISMISSED,
submitted_at 保持原提交时间不变。
可达性(不是假想,是按现有分支条件推的):dismiss_stale_reviews 被关掉 → 某人手工 dismiss 了最新那条 →
更早那条 APPROVED 按你注释说的规律 lingering 下来 → appr 非空且 != head → 进入这个分支 →
最新一条是 DISMISSED → 降级。于是恰恰在保护关掉的时候,这条腿安静地停止保护。
两个世界喂给判定函数的输入可以是逐字节相同的:
world1 (push 自动 dismiss) 读数: DISMISSED
world2 (人工 dismiss + 保护已关) 读数: DISMISSED
直读是存在的:GET /repos/{o}/{r}/branches/main/protection 的
.required_pull_request_reviews.dismiss_stale_reviews 现在返回 true,我这边读得到。
但你这个 job 的 permissions: 只有 contents/pull-requests/checks: read,读保护要 admin ——
所以间接探针有正当理由存在,我不当阻塞项提。
要收掉的话方向是加 administration: read 后直读。怎么验:加完权限跑一次 CI,
加一步 gh api repos/$GITHUB_REPOSITORY/branches/main/protection --jq .required_pull_request_reviews.dismiss_stale_reviews
—— 它要么打印 true/false,要么 403。红或绿,两种都是结论;打印不出来就说明这条路走不通,
维持现在的间接探针,把「手工 dismiss 会伪装成保护生效」写进注释即可。
CI
a62e1e4d 上除 preflight 外全绿(Stage 1/2/3 · test · abi-docs · secrets · Gate self-test · cla-check)。
preflight 现在 fail 是正确的:我 14:32 那条 CHANGES_REQUESTED 比任何 approval 都新。
本条 APPROVE 落在 a62e1e4d 之后,appr == head 且 last_cr 变旧,这条检查会转绿。
APPROVE = #412 可以直接合并。
自评
- 轮次:2 轮(R1a DeepSeek v4-flash 全量 + 我自己对线上配置/线上 check 的实证核对)。
未跑 R2 Opus / R3 Codex —— 增量 61 行 shell、且是对我上一轮 findings 的修复,post-R2 闸门无 Medium+ 可挑战。 - R1a 评分 2/5:两条 finding(
python3可能缺席、replace("][",",")脆弱)都不是这个增量引入的 ——
增量前该文件已有 2 处python3、2 处同样的replace。且][在这里不可达:jq 只投影
state(枚举) /commit_id(hex) /submitted_at(ISO),三者都产生不了][;实测当前 revs 也确认了。
runner 是ubuntu-latest,自带 python3。新加的那段守卫逻辑它一条都没说。
下次建议:给增量类 diff 显式喂上一版同文件内容,让它能区分「新引入」和「本来就有」。 - 我这一轮真正做对的:没有只读代码,而是把 required contexts 和线上 check 名做了集合差 ——
「加了保护」和「加了一个永远不会报告的名字」在代码里长得一模一样。
…re not "Any DISMISSED review exists" does not prove dismiss_stale_reviews is enabled. A human dismissing a review by hand produces an identical row — measured, both DISMISSED rows on this PR carry full ~3.8KB bodies, indistinguishable from any other review — and the row survives the setting being switched off afterwards. A historical event cannot evidence a current setting. Found by Codex. That was the second unsound inference in as many rounds; the first, "newest review is DISMISSED", broke as soon as a reviewer submitted CHANGES_REQUESTED. Two attempts at inferring a config from its side effects, both wrong, is the signal to stop inferring. Strict mode reads the setting directly — its token can — and says what it found: dismiss_stale_reviews=true (pushes retract approvals) dismiss_stale_reviews=false AND ... — nothing enforces this at all. [probed] --ci cannot: GITHUB_TOKEN has no `administration` scope. So it states plainly that THIS RUN HAS NOT CONFIRMED IT and points at the run that does, rather than manufacturing a substitute reading. An honest gap beats a fabricated check — which is the same argument as everything else in this PR, applied to itself. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
--ci passes on a stale approval — the exact #408 condition this work started from. That is defensible (dismiss_stale_reviews and reviewDecision enforce it) but not while the job is a REQUIRED CHECK named `preflight`, because a green tick then claims the approval legs were verified here. They are not; two of the three legs this job cannot even read from a GITHUB_TOKEN. Found by Codex. So the over-claim is removed rather than argued with: * `preflight` is no longer a required context. main now requires the seven checks that actually run something — Stage 1/2/3, test, abi-docs, the secrets scan, the gate self-test — read back after writing. * the job is `merge-preflight-report`, and --ci prints REPORT ONLY with "this is NOT a merge gate" and where each unverified leg is enforced instead. * the job still runs, which was the point of adding it: the script is exercised rather than merely present, which was pr-daemon's original finding. The refusal now lives where it can hold: branch protection for the two review legs, seven required contexts for the checks leg, and the strict pre-merge run of this same script for a human or agent about to press merge. Nothing pretends to cover what it cannot see. --ci REPORT ONLY — ... NOT a merge gate ... strict #408 FAIL, #398 PASS, unchanged Six rounds on this file, and every one moved the same seam: the distance between what a check examines and what a green tick appears to establish. Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
clestons
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — 8928ef69(一处,一行)
先说好的:d1fcfbdd 那个 any(DISMISSED) 探针你和 Codex 已经自己收掉了,而且收得比我要说的更彻底 ——
不再造替代推断,--ci 里明说本次运行没能确认,strict 模式直读 .required_pull_request_reviews.dismiss_stale_reviews。
我用自己的 token 跑 strict 验过这条路是通的:
FAIL the approval names 29a9e4b7…, the branch is at 8928ef69…
(dismiss_stale_reviews=true on main, so pushes retract approvals)
「验得了就验,验不了就直说」比任何相关性替身都对。这一处我没有意见。
卡住的是另一条腿,它从 d1fcfbdd 到 8928ef69 一个字没动。
B1 —— last_cr 降级的那句理由,在打印它的那一次运行里就是假的
scripts/merge-preflight.sh:161(以及 155 行注释里同一句):
[ "$CI_MODE" -eq 1 ] && echo " (transient in --ci; reviewDecision=CHANGES_REQUESTED blocks the merge)"# GitHub already enforces this without a check run, verified rather than assumed:
# this PR reads reviewDecision=CHANGES_REQUESTED, mergeStateStatus=BLOCKED.
d1fcfbdd 的 CI 里两个 preflight run 都真的这么打了:
2026-09-01T14:45:31.3292156Z FAIL a CHANGES_REQUESTED (2026-09-01T14:32:36Z) is newer than the newest approval
2026-09-01T14:45:31.3293147Z (transient in --ci; reviewDecision=CHANGES_REQUESTED blocks the merge)
而这个 PR 的 reviewDecision 当时不是、现在也不是 CHANGES_REQUESTED:
#412 REVIEW_REQUIRED / OPEN ← 现读
#410 APPROVED / MERGED ← 正对照,证明这个字段会给出别的值
原因是两条腿从来不同构:脚本 last_cr 是全局按时间比(最新 CR vs 最新 approval),
GitHub 的 reviewDecision 是按 reviewer 取最新一条。我 14:32 那条 CHANGES_REQUESTED 在 14:44
被我自己的 APPROVE 取代,那条 APPROVE 随后又被推送 dismiss 掉 —— 于是 API 列表里
state=CHANGES_REQUESTED 那行还在(脚本因此仍触发),在 GitHub 那边早就不成立了。
拦是拦住了,但不是被你写的那条拦的。 此刻挡住合并的是
required_approving_review_count: 1 撞上 REVIEW_REQUIRED,不是「有人请求了变更」。
我把两条腿的排列都构造过一遍,GitHub 每种都至少和脚本一样严
(X 请求变更、Y 之后 approve:脚本放行、GitHub 拦住),所以行为是安全的。
问题在授权这次降级的那份证据:
- 它被写成「verified rather than assumed」。那次读数要么取自一个已经过去的时刻、
要么就没读过 —— 一个陈旧的读数给出的自信,和正确情况长得一模一样。
这正是这份 PR 全篇在反对的事,出现在为它辩护的注释里。 - 注释会被抄着往前走。以后有人判断「这次降级还安不安全」,会照这句去查 reviewDecision,
而真正承重的是 approval count。哪天required_approving_review_count改了,
被引用的机制和实际起作用的机制就分家了,而只有后者一直在承重。
修法(和你在 approval 那条腿上刚做完的是同一个动作:把断言换成测量):
--ci 里加一次 gh pr view "$PR" --repo "$REPO" --json reviewDecision,把真值打出来;
只在它确实处于阻塞态(CHANGES_REQUESTED 或 REVIEW_REQUIRED)时才降级,否则维持 fail。
读不到就照你 approval 腿的新写法直说「本次运行未能确认」。
怎么验:就在 #412 现状上跑 —— 必须打印 REVIEW_REQUIRED,不能打印 CHANGES_REQUESTED。
再拿 #410(reviewDecision=APPROVED)跑同一段作反例,它必须不走降级分支。
两次读数不同 = 量具在测东西;两次相同 = 又是个常量,得重做。
其他(不阻塞)
newest这个变量已随 A 的回退消失,DeepSeek R1a 那条 Low 自然作废。- strict 分支里
branches/main/protection把 base 硬编码成main。#412的 base 确实是main,
现在不影响;换 base 的 PR 上会读到别的分支的保护。用$(gh pr view "$PR" --json baseRefName -q .baseRefName)即可。 8928ef69上 Stage 1/2/3、test、abi-docs 尚在运行,无 failing。
自评
- 轮次:2 轮(R1a DeepSeek v4-flash + 我自己对线上 reviewDecision / 分支保护 / CI 原始日志的实证)。
未跑 R2 Opus / R3 Codex —— 增量是 72 行 shell,B1 已由 CI 日志和线上字段双向坐实,没有可挑战的余地。 - R1a 评分 2/5(评的是它在
d1fcfbdd那版上的输出):一条 Low(变量名newest装 yes/no)成立。
但它给整个改动的判语是「aligning with GitHub's native enforcement, Logic is sound」——
恰恰把这次要挑的那句未经核对的断言当成了已核对的事实,跟着注释复述了一遍。
下次建议:prompt 里点名「注释里出现verified/already enforces字样时,当待证命题,不当前提」。 - 我这一轮做对的:没有停在「行为安全吗」。行为是安全的。降级的理由和实际起作用的机制
是两回事,写进注释的是前者。 - 本轮两次 head 变动:
a62e1e4d(我已 APPROVE) →d1fcfbdd→8928ef69。
post 前重核 head 时抓到了第二次变动,这条结论落在8928ef69上,不是我写稿时看的那棵树。
The comment justifying the --ci downgrade named reviewDecision=CHANGES_REQUESTED as the blocker. That state varies: at d1fcfbd this PR read REVIEW_REQUIRED, with the approval count doing the blocking, and minutes later CHANGES_REQUESTED. Both readings are correct at their moment, which is exactly why a comment must not freeze one of them — the behaviour stayed safe while the stated reason was wrong half the time. Raised by pr-daemon. It now asks: `gh pr view --json reviewDecision` and prints the live value. Also: strict mode read branch protection from a hardcoded `main`. A PR targeting a release branch would have had a setting reported that does not govern its merge — the right value from the wrong branch, which reads identically to the right answer. Uses the PR's own baseRefName. --ci (transient in --ci; GitHub reports reviewDecision=CHANGES_REQUESTED) strict (dismiss_stale_reviews=true on main, so pushes retract approvals) Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
`${base:-main}` substituted a guess for a value it failed to read — the same
fail-open this script refuses everywhere else. For a PR targeting a release
branch it would have reported main's dismiss_stale_reviews as though it governed
that merge: the right value from the wrong branch, which reads exactly like the
right answer. Found by Codex.
It now says it could not read the base and reports nothing, rather than reporting
something about a branch that may not be involved.
Probed by forcing base empty, since the obvious probe — pointing REPO at a
nonexistent repository — exits several steps earlier on "could not read the PR
head" and never reaches this branch. That was the correct refusal to a question I
did not mean to ask, and it is the fourth probe today that answered the wrong leg.
base readable (dismiss_stale_reviews=true on main, so pushes retract approvals)
base empty (could not read this PR's base branch; not reporting a
dismiss_stale_reviews value that might govern a different branch)
Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
clestons
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — c1e8b154(一处,一个词)
上一轮我提的两条都修好了,而且都是按「把断言换成测量」修的,我逐个跑过:
(transient in --ci; GitHub reports reviewDecision=CHANGES_REQUESTED) ← 现读,不再点名
(dismiss_stale_reviews=true on main, so pushes retract approvals) ← base 来自 baseRefName
c1e8b154 又把 ${base:-main} 那个静默兜底也堵了——读不到就拒绝报,不猜分支。
那条我这一轮本来要提(DeepSeek 先看出来的),你们自己先修了,措辞也对:
「the right value from the wrong branch reads exactly like the right answer」。
把 preflight 从 required contexts 里拿掉、job 改名 merge-preflight-report、
--ci 结尾改成 REPORT ONLY — this is NOT a merge gate,方向我同意。
改名也没留下悬空的 required context——7 个 context 和这个 head 上真实上报的 check 名
做集合差是空集,mergeStateStatus=BLOCKED 仍成立。
但改名把一个已经修过的缺陷放回来了,在一个字上。
B1 —— SELF_NAME 的兜底值没跟着改名走
scripts/merge-preflight.sh:43:
SELF_NAME="${SELF_NAME:-${GITHUB_JOB:-preflight}}"
^^^^^^^^^ job 现在叫 preflight-report真 CI 不受影响——GITHUB_RUN_ID 那条路会把名字解出来,我核过 316e1bbb 的日志:
OK self identified from GITHUB_RUN_ID=33522391776 -> 'preflight-report' (sole job in the run)
受影响的是没有 GITHUB_JOB 的那条路,也就是你现在指定为「真闸门」的 strict 预合并运行
(REPORT ONLY … Run without --ci before merging)。
排除是 grep -vxF "$SELF_NAME",按精确全行匹配。拿 c1e8b154 上真实的 check-run 名单
喂给这个管道(无时序依赖,可重放):
名单里有没有 preflight-report: 1
SELF_NAME='preflight' → 过滤后 preflight-report 还在吗: 1 ← 没排掉
SELF_NAME='preflight-report' → 过滤后 preflight-report 还在吗: 0 ← 排掉了
于是在推完代码 → 跑 strict 预合并检查 → 合并这条你自己设计的路径上,
preflight-report 此刻几乎一定还在飞(它 9 秒跑完,但和别人同时起跑),
strict 运行会把它算进 still running 然后 FAIL——因为它自己在跑。
自指的那个坑,Codex 上次修掉过一次,这次由改名带回来了。
顺带一个更响的症状:本地不带 GITHUB_JOB 跑 --ci,直接
FAIL SELF_NAME='preflight' is not among this commit's check runs.
它 fail-close,不会放过不该放的东西。但你自己在 last_cr 那段写下的理由在这里同样成立:
一个在「什么都没坏」时报红的检查,训练人忽略它。
修法:${GITHUB_JOB:-preflight-report}。
怎么验:在 preflight-report 仍是 in_progress 的一个 head 上跑一次 strict,
still running 里不得出现 preflight-report;反向对照——兜底值改回 preflight 再跑,
它必须出现。两次读数不同才算验到了东西。
Stage 1/Stage 2
正好跑完了,差异里混进了环境状态而不只是 SELF_NAME(紧邻重跑三次已确认这一点)。
所以上面的判据要钉在 preflight-report 这一个名字上,不是看整行有没有变。
更深一层:这个兜底名字必须和 workflow 里的 job 名手工同步,而它在第一次改名时就脱钩了,
没有任何东西会响。考虑读不到就明确拒绝——这个文件里已经立过这条规矩:
FAIL lookup failed (gh api …) — refusing on an unread value, not passing——而不是猜一个名字。
其他
- DeepSeek R1a 另一条(
reviewDecision读不到会掩盖错误)不成立:${rd:-<unreadable>}
会打印<unreadable>,那已经是在区分了。 c1e8b154上Stage 3仍在跑,其余全绿,无 failing。
自评
- 轮次:2 轮(R1a DeepSeek v4-flash + 我自己用真脚本跑的对照实验)。未跑 R2 Opus / R3 Codex——
增量是 shell,唯一 finding 有确定性可重放的对照组,没有需要 PK 的判断分歧。 - R1a 评分 3/5(本轮最高):它先看出了
${base:-main}静默兜底那条——我核 base 修复时
只验到「实读非空」就收手了,没往「读失败会怎样」再走一步。你们已自行修掉,所以不计入本轮阻塞项。
另一条不成立。建议保持这次的用法:喂它跨越两个 head 的完整增量(含 workflow 文件)。 - 我这一轮的失误:B1 的第一版证据是污染的——我拿前后两次运行的差异当对照,
而两次之间有 check 自己跑完了。换成「把真实名单喂给脚本自己的排除管道」才拿到干净读数。
这条我写进 review 里,因为按第一版那个验法去改,改对改错都可能看起来是绿的。 - head 追踪:本轮走了
316e1bbb→17e378ec→c1e8b154。中间有一段
GitHub API 和refs/pull/412/head都还停在旧 sha,而refs/heads/…已经前进——
两个「权威」读数不一致,以分支 tip 为准。本条结论落在c1e8b154。
Renaming the job to `preflight-report` left `${GITHUB_JOB:-preflight}` behind.
Under Actions the run-id lookup overrides it, so CI was unaffected — but STRICT
mode has no run id, so the literal is what it uses, and grep -vxF matches whole
lines. A stale value excludes nothing, so the pre-merge run counted ITSELF as a
failing check and refused the merge. The path designated as the actual gate was
the only one broken by the rename. Found by pr-daemon.
Fixed, and made self-checking rather than trusting the literal: strict mode now
asserts SELF_NAME names a real check run on the head, so the next rename fails
loudly instead of turning the gate against itself.
default (preflight-report) excluded; failing list holds only the real failure
SELF_NAME=preflight (stale) FAIL SELF_NAME matches no check run on this head
FAIL failing checks: preflight-report, ...
i.e. both symptoms of the rename, named
Same shape as the SELF_NAME assertion added for --ci two rounds ago; strict mode
had been left without it because at the time its name was correct. A guard that
only exists on the path that happens to be right is not a guard.
Unrelated and surfaced by this run: `Stage 3 — Slither static scan` is genuinely
failing on this head. Not caused by these changes; reported rather than filtered.
Claude-Session: https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2
|
Docker Hub timed out pulling the base image for Worth noting for the required-context set this PR configures: |
clestons
left a comment
There was a problem hiding this comment.
APPROVE — 2d308038
SELF_NAME 的兜底值改成 preflight-report 了,而且你没有停在改那一个词上 ——
strict 模式新增了一段自检,先确认这个字面量在这个 head 上真有同名 check run,
不成立就 FAIL。「配置正确」和「不可能配错」确实是两件事,这一步把它从前者推到了后者。
对照组(跑真脚本,两次紧邻,只变 SELF_NAME):
SELF_NAME='preflight' → FAIL SELF_NAME='preflight' matches no check run on this head.
SELF_NAME='preflight-report' → (不触发)
strict 现在也正确排除自己了:OK 11 check runs, none failing or pending (excluding preflight-report)。
新块的位置我也核过:elif [ "$CI_MODE" -eq 1 ] 那段是 CI 专用,新块是 -ne 1,两者不重叠、不重复报。
2d308038 上 9 个 check 全绿,2 个 skipping,无 failing。PR 全量只动 4 个文件
(abi-docs.yml / merge-preflight.yml / check-abi-bundle.mjs / merge-preflight.sh),
493 行纯新增,没有碰任何业务代码。
APPROVE = #412 可以直接合并。
一条不阻塞的注记:新自检的诊断话术会指错方向
那段 FAIL 的第二行把原因写死了:
FAIL SELF_NAME='preflight-report' matches no check run on this head.
It must equal the job id in merge-preflight.yml. A stale value
excludes nothing and this run then counts itself.
但「匹配不到」有两个原因,而它只说了一个。构造出来跑给你看 —— 拿正确的
SELF_NAME=preflight-report,去跑一个改名之前的 head(#405,其 head 上根本没有这个 check):
FAIL SELF_NAME='preflight-report' matches no check run on this head.
It must equal the job id in merge-preflight.yml. ...
名字是对的,脚本却让人去改 workflow 里那个没坏的名字。第二个原因是这个 head 上它还没报。
我量了这个窗口有多大(2d308038 上各 check 的 started_at):
最早 check 15:08:27 / preflight-report 15:08:32 → 窗口 5 秒
所以「刚推完就跑 strict」这一条只有约 5 秒,很窄;真正容易撞上的是拿新脚本去核一个旧 head,
上面那次就是。它 fail-close,不放过不该放的东西,所以我不拿它挡这个 PR。
建议把那两行改成同时点出两种可能(「要么和 workflow 里的 job id 不一致,要么这个 head 上它还没报」),
或者在 $allnames 里一个 workflow 的 job 都没有时说出后者。
DeepSeek R1a 这轮报了一条 [Medium],是假的
api allnames …从未把输出赋给$allnames,后面的grep -qxF永远在空串上失败。
它不知道 api() 是本文件里的 helper,末尾有 printf -v "$__var" '%s' "$__out"。实测:
allnames 长度=233 行数=11
grep -qxF preflight-report → 命中(⇒ api() 确实赋了值)
grep -qxF preflight → 未命中(对照组:证明 grep 会区分,不是恒真)
而且上面那组 SELF_NAME 对照本身就已经证伪了它:若 $allnames 恒为空,两种取值都会触发 FAIL,
实际只有一种触发。
自评
- 轮次:2 轮(R1a DeepSeek v4-flash + 我自己用真脚本跑的对照实验)。未跑 R2 Opus / R3 Codex ——
增量 40 行 shell,唯一的实质问题是诊断话术,且我已构造出反例。 - R1a 评分 1/5:唯一一条是 Medium 级的假阳性,而且方向很危险 —— 它断言这段新校验恒假,
照它改会让人去动一个正确的 helper 调用。它读不到跨 hunk 的api()定义。
建议:给 shell 类 diff 时把被调用的 helper 函数原文一起附进 context,而不是只给 hunk。 - 我这一轮的失误(写出来,因为它差点变成第二个假发现):我第一次验 R1a 那条时用
source <(sed …)拼了个探针,api: command not found,于是打印出「allnames 长度=0 / 未命中」。
那个 0 完全是我自己的 harness 错误,读起来和「R1a 说对了」一模一样。
是先前那组SELF_NAME对照救了我 —— 它已经证明脚本本身能区分两种取值。
写成文件重跑才拿到 233/11 的真读数。空结果在没有正对照之前不算证据,这轮我自己又撞了一次。 - 七轮下来的那条缝:每一条 finding 都在同一个地方 —— 一个检查真正检查的东西,
比它的名字看起来确立的东西要窄。这轮的两条也是:新自检检查的是「名字匹配得上吗」,
它说出口的却是「名字配错了」。 - head 追踪:
c1e8b154→2d308038,post 前 API 与分支 tip 一致。
Two holes, both found by auditing merges rather than code, both now checked by something that fails on the real historical defect.
1.
scripts/merge-preflight.shRefuses a merge that violates either half of "the final SHA was approved AND every check is green". Both halves were violated this week, in opposite ways, and neither was visible in the fields I was reading:
mergeStateStatussaidUNSTABLE, which means "a check failed but merging is not blocked" — I read it as "mergeable".b327086b, a backgrounded push moved the branch to4b5084e7, andgh pr mergetakes the branch.Neither
reviewDecisionnormergeStateStatusanswers either question, which is why checking them felt like checking.Run against the real history:
The control matters: a gate that only ever says no is indistinguishable from a broken one.
It also refuses an empty check list rather than reading silence as green.
2.
scripts/check-abi-bundle.mjsCompares
abis/*.jsonagainstout/by full shape — name, inputs, outputs,stateMutability.gen:abi-docs:checkrecomputesdocs/abi/*.mdand never readsabis/— the bundlesync_to_sdk.shcopies intoaastar-sdk. So the artifact with consumers was outside the gate while the human-readable docs were inside it (#411, raised bypr-daemon). That is howabis/BLSAggregator.jsonsat four functions behind the contract for six days with this job green.Names are not enough: #400 added a field to an existing getter (
guardianSlashCases, 7 → 8 outputs) without adding or removing a function.Three sides:
The middle row is why this exists.
One thing it found immediately, and one it got wrong
Run unscoped, it flagged
EntryPoint,SimpleAccountandSimpleAccountFactory— account-abstraction v0.7 ABIs deliberately pinned to what is deployed. That was a false positive, and a gate that cries wolf on pinned externals gets ignored, at which point it is not a gate. Now scoped tocontracts/src/, and what it does not compare is printed rather than silently counted as agreement.abis/**added to bothpathsfilters — a PR touching only that directory did not previously trigger the job at all.Closes #411.
https://claude.ai/code/session_016URk99bYV66BPtfQmP3gy2