fix(actions): Lopu repairs failed PR checks - #588
Conversation
github-actions
Bot
added
lopu: mergeable
🌸 Lopu — approve, but land this together with #579: they collide textually and compose semanticallyFirst review note on this branch. The change is right, and the incident it cites is real — I pulled the live records rather than taking the comment's word for it:
The aggregate check opened against the first snapshot to land and closed ~5.5 minutes before the slow language arrived. That is exactly the mechanism described, so gating ownership on The part no single PR view shows: #588 and #579 conflictBoth are
The conflict is textual, and the resolution is one lineBoth PRs insert assertions at the same point in assert.match(codeql, /\.\[0\] == \$base and \.\[1\] == \$head/u);which #579 must delete, because #579 rewrites the I applied exactly that in a scratch integration clone and verified: and the merged Why both are worth havingThey are different defenses for the same incident, not duplicates:
#588 is the more robust of the two, since it removes the dependency on freshness entirely — but #579 still prevents the unnecessary exact-head fallback that produced the duplicate scan. Landing both costs one line of conflict resolution. One caveat on the green checks
Suggested order: #584 → #565/#573/#574/#577/#580 → #579 + #588 with the union resolution above. Posted by Lopu, Thingtime's automated principal reviewer. |
Lopu repository reviewLopu reviewed this PR against github-actions as Thingtime's principal PR and repository manager. Using Claude Opus 5. Lopu found no justified local change to publish from this review pass. Lopu review — PR #588 ·
|
| base listener | merge ref | backfill | pre-fix | post-fix |
|---|---|---|---|---|
| yes | fresh | no | analyze=false |
analyze=false |
| yes | stale | no | analyze=true ref=head |
analyze=false |
| yes | absent | no | analyze=true ref=head |
unchanged |
| yes | fresh | yes | analyze=true ref=merge |
unchanged |
| yes | stale | yes | analyze=true ref=head (+::warning::) |
unchanged |
| yes | absent | yes | analyze=true ref=head |
unchanged |
| no | fresh / stale / absent | — | merge / head / head |
unchanged |
| yes | malformed SHA | no | analyze=true ref=head |
unchanged |
Exactly one row changes, and it is the row the PR claims. The refless case — a
conflicted PR, the only kind with no pull_request run of its own — still reaches
the exact-head fallback. Reachability of the elif [ -n "$merge_sha" ] arm is
unchanged.
2. The new contract assertions have teeth — 6/6 mutations caught
I injected each regression the contract claims to prevent and re-ran
--self-test. All six fail the contract:
| mutation | result |
|---|---|
| revert the whole block to the pre-fix file | fails (bites) |
revert the gate to -n "$merge_sha" |
fails (bites) |
re-collapse: add mergeable_pr=false beside the freshness merge_sha="" |
fails (bites) |
hoist mergeable_pr=true out of the SHA-validated branch |
fails (bites) |
drop .[0] == $base from the freshness jq |
fails (bites) |
drop .[1] == $head from the freshness jq |
fails (bites) |
| rename the variable | fails (bites) |
The assert.deepEqual over the assignment list is the load-bearing one: it pins
the variable's whole lifecycle (initialized false, set true exactly once, only
where a well-formed published merge SHA was captured), which is what catches the
write-side re-collapse that the three read-side assertions alone would miss.
Note on method: my first mutation run reported three assertions "blind." That
was my harness, not the contract — "\$" inside a Python string literal is a
literal backslash-dollar, so three of the replacements silently matched nothing
and were no-ops. Re-run with the mutations written to a file and asserted
non-empty (assert t != orig), all six bite. Recorded because a mutation test
that silently mutates nothing always reports "guard blind."
3. New: this is the majority state of the repo, not a rare race
No previous pass measured the population; they established the mechanism on
#557 specifically. I evaluated the live freshness predicate against every open PR:
- 19 open PRs. 0 are refless. 8 fresh. 11 (58%) stale.
- Every one of the 11 is a first-parent (base) mismatch. The second-parent
(head) comparison never fires — head skew does not occur, because a head change
retriggerspull_requestand GitHub recomputes the ref.
So pre-fix, 11 of 19 open PRs would take the exact-head fallback on a handoff
dispatch, beside their own live pull_request merge-ref run — #557's split-ref
shape, right now, on more than half the repo. That materially raises this PR's
value above "fixes one incident."
It also isolates which half of the freshness test is load-bearing: .[1] == $head
is the real safety property; .[0] == $base is the noise source. That is why
gating ownership on it was the wrong cut, and why #579 widening only .[0]
targets the correct half.
4. New: how #579 and #588 compose, quantified
The "land both" constraint has been asserted repeatedly on this PR but never
measured. Replaying #579's widened predicate (accept the live base branch tip as
.[0]) over the same 11:
| count | |
|---|---|
| fresh today | 8 |
| stale today, rescued by #579 alone | 8 |
| stale today, still stale after #579 | 3 (#568, #564, #499) |
Those 3 have merge refs whose first parent is neither the cached base.sha nor
the live tip — lazily computed against an intermediate base state, exactly the
class #579 cannot close because a lazily recomputed ref can be arbitrarily far
behind. #579 takes exposure 11 → 3; #588 takes it to 0. Both should land;
#588 is the backstop and the one that closes the class.
5. Gate input verified sound
The fix makes base_has_pr_listener the sole remaining gate on whether a
mergeable PR is centrally re-analyzed, so I verified its input rather than
assuming it. It is computed by grep -Eq '^ pull_request:$' against the base's
workflow file at the cached pulls/N.base.sha — and that pointer is stale for
14 of 19 open PRs. In practice it does not bite: the anchored grep matches at all
three live base tips (main, develop, github-actions) and at all 19 cached
base SHAs — 19/19 detected. The anchor also correctly excludes the
pull_request_target: trigger two lines below it on main/develop. The
residual risk is confined to a listener activation/deactivation window, which is
precisely what backfill_listener_owned exists to cover.
6. Residual — unchanged, deliberate, and now visible
backfill_listener_owned=true + mergeable + stale still routes to
refs/pull/N/head, structurally the same split-ref shape as #557. It is opted
into, it is bounded (the completion check suppresses the repeat), it is
pre-existing and untouched by this PR, and it is now the one state that emits a
real ::warning::. Narrowing it risks leaving a PR unscanned. Correctly left
alone.
Changes made
None. No defect found in this head, and no speculative churn. The one
diagnostic defect a previous pass identified — the concurrency alarm firing on the
no-listener path, where no pull_request run can exist — is already fixed on this
branch in 6b79b4bc; I re-ran the 12-state replay against it and every analyze /
analysis_ref decision is identical to the pre-split behaviour, so that hunk is
diagnostics-only as claimed.
Worktree left clean (git status --porcelain empty). No controller fix written:
the root cause is not a live failure in .github/workflows/**, .github/actions/**
or .github/scripts/**, so no lopu-workflow-fix.md was produced.
Operational finding (not a code defect, no action taken)
Conversation comment 5511880220 on this PR, authored by github-actions[bot],
has a 25-character body that is literally @/tmp/lopu-588-comment.md — a prior
Lopu publication passed an @file argument to an endpoint that took it literally,
so the intended review text was never posted. It carries no
<!-- thingtime-lopu-conversation:v1 --> marker, so per the editing rule I did not
touch it. Worth repairing in whatever Lopu step posts these: build the JSON body
and pipe it (gh api --input -) rather than relying on @-expansion. I used that
form for this pass's comment.
Validation run
| check | result |
|---|---|
python3 yaml.safe_load(codeql-analysis.yml) |
parses; jobs ['scope', 'analyze'] |
node .github/scripts/workflow-control-plane-contract.mjs --self-test |
workflow control-plane contract: self-test OK |
node --check workflow-control-plane-contract.mjs |
OK |
| ownership-block replay, 10 input states, head vs base | 1 row changes, as documented |
| contract mutation suite, 6 injected regressions | 6/6 caught |
build-all-branch --self-test |
OK |
deploy-develop-pr-preview --self-test |
88/88 passed |
codeql-open-pr-backfill --self-test |
OK |
merge-main-develop-sync-pr --self-test |
OK |
promote-features-to-main --self-test |
OK |
git diff --check |
clean |
git status --porcelain |
empty |
gh pr checks 588 |
0 failing / 0 cancelled / 0 timed out |
| CodeQL snapshot | empty; dispositions left [] |
Verdict: approve, no changes. Land #579 first (using the corrected
keep-both-blocks-plus-one-); resolution already recorded on this PR), then this.
— Lopu, reviewing 6b79b4bc against github-actions@9f7f4fa3.
|
@/tmp/lopu-588-comment.md |
🌸 Lopu — I simulated the whole batch landing. Seven merge clean; this PR and #584 are the only two that don't.Earlier notes on this branch already say "land #588 with #579, they collide textually and compose semantically." This is the follow-through: I ran the merge instead of predicting it, and I have the exact resolution. The batch merge order, verifiedReplaying every open
I then materialised the 7-PR stack and ran the full control-plane suite on it — all 13 contracts plus Worth stating plainly: #573's new The #588 ↔ #579 conflict, and its exact resolution
Resolution: keep both sides, and drop #588's re-add of the replaced line. #579 rewrites the I built that union and ran it: I also read the auto-merged base_branch_sha="" # ← #579: live base tip as a 2nd accepted parent
...
mergeable_pr=false # ← #588: "GitHub published a merge ref"
if candidate_merge_sha="$(gh api ...)"; then
if [[ ... ]]; then merge_sha=...; mergeable_pr=true; fi
fi
if [ -n "$merge_sha" ]; then # ← #579 widens this freshness test
... .[1] == $head and (.[0] == $base or ($branch_base != "" and .[0] == $branch_base))
fi
if [ "$base_has_pr_listener" = true ] \
&& [ "$mergeable_pr" = true ] \ # ← #588: ownership no longer reads the cleared merge_sha#579 makes the false-stale read rarer; #588 makes it stop mattering when it still happens (the base can always advance between the two reads). Neither subsumes the other. What I'd doLand the seven clean ones, then #588 with the union resolution above. #584/#575 need a close/keep decision first — see my note on #584. No changes made to this branch: the change is correct as written, and a merge-time conflict is not something to pre-empt on the branch. — Lopu, reviewing at |
🌸 Lopu — the workflow fix is right; the contract that shipped with it did not actually hold it in place. Strengthened on this branch.Earlier notes on this branch verified the #557 incident and the #579 interaction. I re-ran both from scratch rather than inheriting them, and then went after the part no previous pass tested: does the new guard catch the regression it was written to catch? It did not. The gap, demonstratedThe PR adds three assertions to One line added to the freshness check: merge_sha=""
mergeable_pr=false # <-- re-collapses the two facts this PR separatedreproduces #557 exactly — and the contract stays green:
Mutation A is the whole incident back, under a guard reporting What I changed (+19 lines, contract only)Pin the variable's lifecycle rather than its individual reads: assert.deepEqual(
codeql.match(/^\s*mergeable_pr=\S+$/gmu)?.map((assignment) => assignment.trim()),
["mergeable_pr=false", "mergeable_pr=true"],
"`mergeable_pr` is initialized false and set true exactly once, so the freshness check cannot clear it",
);
assert.match(
codeql,
/merge_sha="\$candidate_merge_sha"\n\s+mergeable_pr=true$/mu,
"`mergeable_pr` records the published merge SHA GitHub actually returned, never an unconditional default",
);Two assignments, in that order, with the
After: A ❌, B ❌, C ❌, and a control mutation (rewording an operator #579 union: re-verified, resolution unchangedReproduced the merge in an isolated clone. #579 into Validation
One housekeeping noteFour comments across #573 (2), #575 (1) and #588 (1) have a body of literally Verdict: approve. The workflow change fixes the real cause of the #557 aggregate-check timeout; the guard now genuinely prevents its return. — Lopu, reviewing at |
|
Lopu repository review — batch review of 16 open PRs. The diagnosis here is right, and the lifecycle pin is the good part: rather than only constraining how This PR and #579 are not independently mergeableThey fix opposite halves of the same #557 incident, and they collide in a way that's easy to miss. I checked rather than assumed: 1. They conflict in the contract file. (That and #584↔#575 are the only conflicting pairs among the nine controller PRs.) 2. So resolving the Suggested orderMerge #579 first — it's the side that rewrites the I deliberately have not pre-adopted #579's changes here. This PR is correct and green against its own base, and rewriting it to accommodate an unmerged sibling would be speculative churn. Validation: full blocking No changes made. |
🌸 Lopu — I checked which dispatcher actually produced the #557 duplicates. All three came from the lane this PR gates.Earlier passes on this branch established the mechanism from the run logs and then strengthened the contract so the regression that matters — re-collapsing the two facts with a It didn't. Attribution
It recurred, and the merge-ref lane was healthy the whole timeFrom And the previous head Why declining at the gate is the right cut, rather than tightening the idempotency guard
That three-second cadence is a queue, not variance. So once one duplicate head-ref scan exists, each repeat handoff dispatch for the same head adds another ~10–13 minutes to the head-ref queue instead of converging. The existing One thing that reads backwards and is worth writing downThe incident runs say Two follow-ups, neither blocking this PR1. The backfill lane keeps the residual — and its "don't pile on" guard has a blind spot. 2. Validation
I also re-derived the reachability of the rewritten block: Verdict: approve, unchanged. Sequencing constraint from the earlier pass still stands — merge #579 first, then rebase this onto it. One cosmetic note for whoever rebases: the new exact-head fallback message says "a pull_request run may be analyzing its merge ref concurrently", but that arm is also reached when — Lopu, reviewing |
|
🤖 Lopu review — merge-order hazard with #579 This PR is correct and I have no changes to request. Flagging one thing that #579 fixes the same PR #557 incident from the other end — it teaches the They do not compose as text, and the dangerous half merges silently:
The trap is that this PR keeps Because the YAML half resolves quietly, a conflict-marker-only resolution looks Verified resolution — I performed the real merge and ran it: keep #579's Whichever of the two merges second needs to reconcile the assertions, not Two asides on this branch, neither caused by it: the |
|
Lopu review — no defects. Careful work, and the cited incident is real — I verified it against the live API rather than taking the commit message for it: On PR #557 @ The separation you've drawn is the right one: "did GitHub publish a merge ref at all" and "is the ref we captured current" are different facts, and only the first should decide listener ownership. Gating on the freshness-cleared The lifecycle assertion is the strongest part of the contract: assert.deepEqual(
codeql.match(/^\s*mergeable_pr=\S+$/gmu)?.map(a => a.trim()),
["mergeable_pr=false", "mergeable_pr=true"],
)The other three assertions pin how Splitting the log message so a stale ref no longer reports as absent is a real improvement too — the old log claimed the ref was absent on the line directly after announcing it was stale, which is what hid the concurrent One mechanical conflict with #579 — both are wanted#579 attacks the same incident from the other side: it stops a freshly-recomputed merge ref being misread as stale in the first place ( They conflict in
Resolution: keep #579's three replacements, drop the retained one-liner from this side, keep your five additions. I applied exactly that in a scratch merge and the combined contract passes against the combined Validation
Recommended batch order, validated end-to-end: #565 → #574 → #573 → #577 → #580 → #584 → #579 → #588. All five control-plane contracts pass at the end of it, versus two red on the tip today. Posted by Lopu, Thingtime's PR manager. |
Lopu · this PR and #579 fix the same incident and break each otherBoth this PR and #579 repair the PR #557 CodeQL
Both fixes are worth having, and I would keep both. But they collide, and I ran the merge rather than reasoning about it:
The conflict itself is easy. The resolution is the hazard. #579 rewrites the merge-parent jq program into a multi-line two-base form and deletes the assertion that pinned the old single-line spelling. This PR keeps that assertion untouched at line 893: assert.match(codeql, /\.\[0\] == \$base and \.\[1\] == \$head/u);Take both sides — the obvious resolution — and that assertion survives against a yml that no longer contains the text. I built exactly that union tree (git's own auto-merged Advisory lane, so it would not block a merge — it would just start posting a contract-advisory comment on every control-plane PR, about a property that is completely intact. Exactly the "permanently red contract has stopped checking anything" failure that #584 and #575 are separately fixing elsewhere in this queue. What I changed on this branchI replaced that one brittle assertion with two on the invariant itself: assert.match(codeql, /\.\[0\] == \$base/u, "…first parent against the PR base");
assert.match(codeql, /\.\[1\] == \$head/u, "…second parent must be the live head");Both spellings satisfy these, so the check keeps testing the property without pinning one rendering of the jq program. Verified: this worktree's contract is green, and the union tree of #579 + this PR is now green too (it failed before the change). I deliberately left #579 alone — its three replacement assertions are strictly stronger than what they replaced, so there is nothing to fix on that side. Merge either order; take both blocks in the contract conflict. Nothing else in the two PRs interacts: #579's other assertions target |
🌸 Lopu — the fix holds up; one diagnostic was wrong, and the merge recipe I left earlier does not compileRe-reviewed The core fix is right, and I checked it the hard wayI replayed the edited ownership block over all 12 reachable input states instead of re-reading the matrix in the description. The delta against the pre-fix block is exactly one row:
Eleven states unchanged. Separating "GitHub published a merge ref" from "that ref is current" is the right cut, and a conflicted PR — the only one with no The incident evidence checks out live, not just in prose: And the new contract assertions have teeth — I injected each regression they claim to prevent and confirmed the intended one fires: the pre-fix yml, One defect, fixed on this branchThe second hunk exists to make the logs legible, but the line it added says
whenever
So the alarm fired on the ordinary no-listener path, where nothing competes — in the one hunk whose entire purpose is diagnostic accuracy for this incident class. I split it into three accurate cases and promoted the single genuine hazard to a real
|
🌸 Lopu — I measured the population. This isn't a rare race: 58% of open PRs are in the state this PR fixes, right now.Re-reviewed Earlier passes established the mechanism on #557 and then strengthened the contract. I re-ran both from scratch rather than inheriting them — both hold — and then went at the one thing no pass had done: nobody measured how often the fixed path is taken. It turns out to be the common case. Every open PR, evaluated against the live freshness predicate
Pre-fix, all 11 take And every one of the 11 is a first-parent mismatch. The #579 relationship, finally with numbersThis PR has said "land both, they compose" several times (mine included) without anyone quantifying it. Replaying #579's widened predicate — accept the live base branch tip as
Their merge refs were computed against an intermediate base state — the class #579 structurally cannot close, because a lazily recomputed ref can sit arbitrarily far behind. #579 takes exposure 11 → 3. This PR takes it to 0. That's the argument for both, with a number on it. I also verified the input the fix now leans onThis change makes It doesn't bite: the anchored grep matches at all three live base tips and at all 19 cached base SHAs, 19/19. The Replay and mutation results, re-derivedBlock extracted programmatically from the YAML (not transcribed), stubs only on the two
VerdictApprove, no changes. The diagnostic defect flagged last pass is already fixed in Housekeeping: comment — Lopu, reviewing |
github-actions
Bot
added
lopu: conflicting
…ve-pr588-controller # Conflicts: # .github/scripts/workflow-control-plane-contract.mjs
github-actions
Bot
added
lopu: unknown state
github-actions
Bot
removed
the
lopu: mergeable
1 participant
Lopu controller check repair
Lopu identified a failed PR check whose root cause is in the protected controller/workflow code.
Lopu controller fix — duplicate CodeQL analysis owner closes a PR's aggregate check
timed_outSymptom
PR #557 (
docs: grow Thingtime's world-domination TODO garden, headbb151336b799a1cde9f6add600f1e5976357bc2b) carries a red required-lookingCodeQLcheck:100230355382(appgithub-advanced-security)conclusion: timed_out, opened2026-09-02T11:30:19Z, closed11:36:46ZThe PR is documentation-only and its trusted CodeQL alert snapshot is empty, so
nothing in the PR's contents explains the failure.
Root cause
.github/workflows/codeql-analysis.yml,scopejob,workflow_dispatchbranch.Two facts were collapsed into one variable:
refs/pull/N/merge(i.e. the PR is mergeable, so apull_requestrun exists and is analyzing that merge ref itself).The freshness check clears
merge_shawhen (2) fails. The listener-ownershipdecision then read that cleared value as if (1) had failed:
GitHub recomputes
refs/pull/N/mergelazily. For #557 it was last computed at04:25Z(first parent814ebfdc,develop's tip at that time) whiledevelopadvanced repeatedly through the morning, so at
11:29Zthe ref was genuinelystale — but the PR was still mergeable and its
pull_requestrun(
33624831516) was analyzingrefs/pull/557/mergenormally.Verbatim from the dispatched run's
Select one analysis ownerjob(run
33625651090, job100232646196):So one PR head was analyzed under two different refs at once:
33624831516pull_requestrefs/pull/557/merge33624842347workflow_dispatchrefs/pull/557/head33624901816workflow_dispatchrefs/pull/557/head33625651090workflow_dispatchrefs/pull/557/headAdvanced Security opened the PR's aggregate
CodeQLcheck on the firsthead-ref analysis (
11:30:19Z) and then waited for the second configuredlanguage on that same ref. The duplicate scans were themselves the
contention that stretched
Analyze (javascript-typescript)from 7m02s (on theuncontended
pull_requestrun) to 12m43s, so it landed at11:41:49Z— fiveminutes after the check had already closed
timed_outat11:36:46Z. GitHubdoes not reopen a completed check run, so the later successful analyses could
not clear it.
This is the identical hazard the workflow already documents and guards for the
pushpath ("Confirm this push still owns the analysis"), one event away andunguarded.
Fix (in
$GITHUB_WORKSPACE/trustedonly).github/workflows/codeql-analysis.yml— recordmergeable_prwhen GitHubpublishes a well-formed merge SHA, and keep it set when the separate
freshness check clears
merge_sha. Gate listener ownership onmergeable_pr.A listener-owned mergeable PR now always declines the dispatched scan,
fresh ref or stale, because its own
pull_requestrun owns analysis. Only aPR GitHub cannot merge (no ref at all, hence no
pull_requestrun) stilltakes the deliberate exact-head fallback.
.github/scripts/workflow-control-plane-contract.mjs— pin the rule: the twofacts must stay separately recorded, ownership must be decided by
mergeable_pr, and it must not regress to-n "$merge_sha".No behaviour change for: PRs targeting a branch without the listener (all three
ref states unchanged), conflicted PRs (still head-scanned), and
backfill_listener_owned=trueactivation backfills (still analyze).Validation run
python3 -c "yaml.safe_load(...)"oncodeql-analysis.yml— parses, jobs['scope', 'analyze'].node .github/scripts/workflow-control-plane-contract.mjs --self-test—workflow control-plane contract: self-test OK(fails on the pre-fix file viathe three new assertions).
node .github/scripts/build-all-branch.mjs --self-test— OK.node .github/scripts/deploy-develop-pr-preview.mjs --self-test— OK.git diff --check— clean.Decision-matrix simulation of the edited block, 8 cases, all pass:
analyze=false(pull_request owns)analyze=false(washead— the bug)analyze=true,headanalyze=true,mergeanalyze=true,headanalyze=true,mergeanalyze=true,headanalyze=true,headReplaying the pre-fix block on row 2 reproduces
analyze=true ref=head.Relationship to open PR #579
PR #579 (
lopu/workflow-check-fix-33571033461→github-actions) widens thefreshness check to also accept the live base branch tip as a valid first
parent. That is a correct and complementary improvement — it reduces how often
the stale verdict is reached — but it would not have prevented this failure:
#557's merge-ref first parent (
814ebfdc, computed04:25Z) matched neitherthe cached
pulls/557.base.sha(f31864b2) nordevelop's live tip at11:29Z(59f7c5f8). A lazily recomputed merge ref can always be arbitrarilyfar behind, so the ownership gate is the part that has to be correct.
Verified interaction (synthesized merge of
github-actions@9f7f4fa+#584 + #573 + #577 + #580 + #579 + #574 + #565, then this patch applied with
git apply --3way):.github/workflows/codeql-analysis.yml— applies cleanly; the two editsare in different parts of the
scopestep..github/scripts/workflow-control-plane-contract.mjs— one trivial conflict:fix(actions): Lopu repairs failed PR checks #579 replaces the single
assert.match(codeql, /\.\[0\] == \$base and \.\[1\] == \$head/u);line that this patch anchors after. Resolution is to keepfix(actions): Lopu repairs failed PR checks #579's three replacement assertions and append this patch's three after them.
With that resolution,
node .github/scripts/workflow-control-plane-contract.mjs --self-testreports
workflow control-plane contract: self-test OK, so both changescoexist.
They are independent in intent and both should land.
Source Lopu workflow run