fix(actions): Lopu repairs failed PR checks - #579
Conversation
|
These examples are warning-only. They do not fail a required check or block this PR.
Sanitized tail ' --effort low\n' +
' --max-turns 1\n' +
' --dangerously-skip-permissions\n' +
' --allowedTools ""\n' +
'\n' +
' - name: Report the live credential result\n' +
' env:\n' +
' CREDENTIAL_SLOT: ${{ steps.live_probe.outputs.claude-credential-slot }}\n' +
' CREDENTIAL_NAME: ${{ steps.live_probe.outputs.claude-credential-name }}\n' +
' run: echo "Live Claude authentication succeeded with $CREDENTIAL_SLOT ($CREDENTIAL_NAME)."\n' +
'\n' +
' route:\n' +
' if: >-\n' +
" inputs.promotion_source_pr == ''\n" +
" && inputs.promotion_plan_b64 == ''\n" +
" && (inputs.maintenance_operation == ''\n" +
" || inputs.maintenance_operation == 'manage-prs')\n" +
" && !(github.event_name == 'workflow_dispatch'\n" +
" && github.actor == 'github-actions[bot]'\n" +
" && github.ref_name == 'github-actions'\n" +
" && inputs.pr_number == ''\n" +
" && inputs.branch == 'lopu-internal-all-branch')\n" +
" && (github.event_name != 'issue_comment'\n" +
' || (github.event.issue.pull_request',
expected: /anthropic-api-key-fallback:/u,
operator: 'match',
diff: 'simple'
}
Node.js v22.23.2
Sanitized tail '# YAML is loaded from an arbitrary PR base or head ref.\n' +
'# - `ai-merge-paused` is a user-controlled, durable stop signal. Automation\n' +
'# never creates, adds, removes, or treats it as stale: when present, every\n' +
'# detector and worker abstains until a user removes it. This prevents base\n' +
'# branch movement from silently re-spending AI/Vercel/GitHub compute.\n' +
'#\n' +
"# graphify-out/** is never given to the AI. The repo's merge driver for\n" +
'# graph.json is not configured in CI, so git would silently text-merge it into\n' +
'# a mixed base+head union (the poisoned-pair state CLAUDE.md forbids). Instead,\n' +
'# when BOTH sides touched graphify-out since the merge base, the whole\n' +
'# directory is deterministically reset to the base side before anything else.\n' +
'# AFTER the resolution is verified and committed, the graph is refreshed on a\n' +
'# pristine reset tree and committed separately, so the pushed graph reflects\n' +
'# the merged code. That ordering is required: the verify step asserts the\n' +
'# staged graphify-out subtree still equals the base side, so refreshing\n' +
'# earlier would fail its own check.\n' +
'#\n' +
'# The refresh includes LLM SEMANTIC extraction when a configured Lopu provider\n' +
'# credential exists: `graphify extract` + `graphify cluster-only` through\n' +
'# OPENAI_API_KEY, ANTHROPIC_API_KEY, or CLAUDE_CODE_OAUTH_TOKEN. The configured\n' +
'# review backend is preferred, so Terra/Sol-based Lopu review and Graphify use\n' +
'# the same OpenAI project credential while Claude remains a fallback. Extract\n' +
'# is manifest-incremental, and unchanged\n' +
'# content is served from the tracked content-addressed semantic cach'... 487078 more characters,
expected: /feature_stack_merge:[\s\S]*?if: >-\s*!cancelled\(\)\s*&& needs\.feature_stack_plan\.result == 'success'\s*&& needs\.model_config\.result == 'success'/,
operator: 'match',
diff: 'simple'
}
Node.js v22.23.2
|
github-actions
Bot
added
the
lopu: mergeable
🤖 Lopu review — approve; the premise is visible in the live data
The consequence is observable on #557: its aggregate ImplementationBoth sides accept either accepted base while still requiring the live head parent — that's the property that matters, since widening the base must not widen what counts as "this PR's head".
ValidationAll seven control-plane PRs in this batch (#565, #573, #574, #575, #576, #577, #579) apply cleanly onto One note for the record: this removes the systematic cause, but #557's specific check is now a stale terminal state — a successful re-analysis did not reopen it, so that one needs a fresh scan on a new head rather than a code change. |
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 #579 ·
|
🤖 Lopu review — PR #579Verdict: sound. No changes made to this branch. I re-derived the diagnosis Verified independently
Running both predicates directly against those live parent arrays:
Because One correction to the PR description
That is no longer true of #560 — its merge ref has since been recomputed Also checked, all fine
Residual limitation (not blocking)If the base advances twice in quick succession the merge ref can be parented on Validation: Separate finding — a control-plane defect found during this reviewNot part of this PR and deliberately not placed on this branch. While merge_sha="$(gh api "repos/$REPO/git/ref/pull/$number/merge" --jq '.object.sha' 2>/dev/null || true)"
if [ -n "$merge_sha" ] && ! [[ "$merge_sha" =~ ^[0-9a-f]{40}$ ]]; then
echo "::error::PR #$number returned an invalid merge-ref SHA."
exit 1
fi
Fixed in the trusted controller checkout, with a replay against a mock I also found — and deliberately did not change — a pre-existing contract Posted by Lopu ( |
🌸 Lopu — the guard is widened in exactly one dimension, and the self-test proves itMerge order. The control-plane contracts run in a non-blocking advisory job, so green checks here don't certify them. Two are already red at On the fix. The failure chain is precisely stated and matches what I can reproduce from the code: The property that keeps this from being a hole is that only the first parent gains a second acceptable value; The five surrounding cases cover the shapes that actually occur (live-tip parent, cached-pointer parent, unavailable tip, neither, malformed tip), and the malformed-tip case specifically pins that a bad value can never widen the guard. Good. Validation is symmetric on both sides, which is what I checked next: And the No changes made. Lopu · automated repository review · 0 open CodeQL alerts on |
|
🤖 Lopu review — ✅ validated, no changes requested. The base-pointer skew diagnosis is right: What I checked most carefully is that the widening stays tight, and it does:
The self-test covers the two cases that matter most: a merge parented on neither 💭 Minor, not worth changing now: |
🌸 Lopu — the change is right; one correction to the motivating case, and a complementary fixThe change is correct. The correction. This wouldn't, on its own, have prevented the #557 timeout it cites. The numbers:
The merge ref matched neither accepted base — GitHub hadn't recomputed it for seven hours while What actually closes it. The fallback fires for a mergeable PR whose own if [ "$base_has_pr_listener" = true ] \
&& [ -n "$merge_sha" ] \ # <-- wrong fact
&& [ "$BACKFILL_LISTENER_OWNED" != true ]; then
analyze=false"GitHub published a merge ref" and "that merge ref is current" are different facts, and the freshness check clears Verified you two coexist, rather than assuming it. Applying my patch on top of a synthesized merge already containing this PR:
Nothing to change here. Note the two failing contract advisories on this head fail identically on |
github-actions
Bot
added
the
lopu: overlapping files
🌸 Lopu — heads-up: #588 lands on the same lines, and the two composeFollow-on to the earlier notes on this branch. Nothing here changes my read of #579 itself — the widened parent check is still correct, and What's new since those notes is #588, opened after them. It fixes the same #557 CodeQL Verified: they conflict textually, not semantically
The only real overlap is one line. This PR deletes assert.match(codeql, /\.\[0\] == \$base and \.\[1\] == \$head/u);and it has to, because this PR rewrites the I applied that union in a scratch clone — The two are complementaryThis PR stops a current merge ref being misread as stale. #588 stops staleness deciding ownership in the first place. #588's is the stronger guarantee — it removes the freshness dependency entirely — but this one still prevents the unnecessary exact-head fallback that created the duplicate scan. Worth landing both; the cost is the one line above. Full detail is on #588, including the live check-run timings that confirm the incident. Posted by Lopu, Thingtime's automated principal reviewer. |
🌸 Lopu — the #588 collision has a mechanical resolution; I built it and it's greenShort follow-up to my earlier "#588 lands on the same lines" note, now with the merge actually run rather than predicted. Replaying the batch into The only collision is #588 landing afterwards, and it is confined to one hunk of
Resolution: keep both sides, drop #588's re-add of the line this PR replaces — the And the merged One thing I checked because it is the usual way a pair like this goes wrong: neither PR's contract fails on the other's workflow change. This PR asserts the No changes to this branch; nothing here needs repair. — Lopu, reviewing at |
|
Lopu repository review — batch review of 16 open PRs. Fixing this in both places that make the decision — the inline The self-tests cover the cases that decide whether this is a fix or a hole: live-tip parent accepted, cached-pointer parent still accepted, unavailable tip falls back to the original behaviour, a merge ref matching neither still rejected, a malformed tip never widens the guard, and the live-head second parent stays required throughout. That last one is what stops this degrading into "accept any merge commit." Ordering note against #588These two fix opposite halves of the same #557 incident and collide. Verified:
This is the side to merge first — it's the one that retires that assertion — then rebase #588's Validation: full blocking No changes made — the only finding is the ordering constraint, which isn't a defect in this branch. |
|
🤖 Lopu review — correct fix, plus a merge-order hazard with #588 The diagnosis holds:
Merge-order note. #588 fixes the same PR #557 incident from the other end
This PR deletes the Verified resolution (real merge, then run): take this PR's three Also worth stating for anyone reading a base-tip diff of this branch: the |
|
Lopu review — no defects. This fixes the false "stale merge ref" verdict at its root, and the guards around the widening are right. The core insight is correct and worth restating: What I checked specifically, because widening an acceptance predicate is where this kind of change goes wrong:
One note, not an objection: the new workflow read uses a bare Mechanical conflict with #588 — both are wanted#588 attacks the same incident from the other side: it separates "GitHub published a merge ref" from "the ref we captured is current", so ownership stays correct even when the ref genuinely is stale. Yours reduces how often it's wrongly called stale. Land both. The conflict is in Validation
Recommended batch order, validated end-to-end: #565 → #574 → #573 → #577 → #580 → #584 → #579 → #588. Posted by Lopu, Thingtime's PR manager. |
Lopu · heads-up on the #588 interaction (no change needed here)This PR and #588 both repair the PR #557 CodeQL They conflict, though. I ran the merges rather than guessing: The hazard is the resolution, not the conflict. This PR correctly replaces assert.match(codeql, /\.\[0\] == \$base and \.\[1\] == \$head/u);with three stronger assertions, because the yml no longer carries that single-line spelling. #588 keeps that line untouched. Take both sides — the natural resolution — and the stale assertion survives against a yml that no longer matches it. I built the union tree and confirmed the contract fails there at line 893. Advisory-only, so it would not block anything; it would just warn forever about a property that is fully intact. I fixed it on #588's branch, not this one — its line 893 now pins On this PR itselfNo changes needed. A few things I checked rather than assumed:
Contract self-test is green in this worktree. The two advisory contracts that do fail here ( Merge order: this one, then #588, taking both blocks in the contract conflict. Either direction works. |
🌸 Lopu — this PR now has a position in the queue, and the
|
github-actions
Bot
removed
the
lopu: mergeable
github-actions
Bot
removed
the
lopu: overlapping files
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 owners from a stale base comparison
Scope note, stated up front
This fixes a real, log-verified controller defect found while investigating
PR #557's red
CodeQLcheck. It is not the cause of that red check, and Ido not claim it will turn the check green. The red check is driven by a GitHub
comparison that cannot be generated for that head; see "What this does not
fix". The defect below is worth repairing on its own merits — it burns roughly
45 minutes of serialized runner time per PR head and violates the single-owner
invariant this workflow documents for itself.
Defect
.github/workflows/codeql-analysis.ymldecides whether a PR's ownpull_requestrun already owns analysis by checking the parents ofrefs/pull/N/mergeagainstpulls/N.base.sha:pulls/N.base.shais GitHub's cached base pointer.refs/pull/N/mergeisrecomputed independently. Once the base branch advances the two skew, in both
directions. For PR #557 the merge ref had been refreshed onto the live
developtip while the cached pointer lagged:refs/pull/557/merge5d4493390464947c7d04e7ddc063e05b33706b197b6418bd…(livedeveloptip),ebb640e5…(live head)pulls/557.base.sha(cached)f31864b2…7b6418bd… != f31864b2…, so a perfectly current merge ref was rejected. Allfour dispatched backfill runs (
33570993922,33571026172,33571689494,33571733288) logged it verbatim:Consequences, all observed on this one head:
refs/pull/557/headeven thoughdevelopcarriesthe
pull_request:listener and PR docs: grow Thingtime's world-domination TODO garden #557's own run33571087941alreadyowned analysis.
refs/pull/557/headholds 8 analyses forebb640e5— 4actionsand 4javascript-typescript— where the design intends none.analysisSnapshotsdedupe, whichcannot see an analysis that is still running. So the 10-minute controller
tick re-selected the PR three more times.
queue: maxthen serialized fourjavascript-typescriptjobs into onequeue:
23:26:16→23:38:57,23:39:01→23:47:08,23:47:12→00:00:01,00:00:05→00:05:06. About 45 minutes of runner time for one head, threequarters of it redundant.
The workflow's own comment already names head-ref uploads at a live PR head as
a hazard to avoid, but its guard covers only the
pushpath(
if: github.event_name == 'push' && …). Theworkflow_dispatchbackfill pathis unguarded, and that is the path that fired here.
.github/scripts/codeql-open-pr-backfill.mjscarried the identical defect inanalysisSnapshotForPullRequest(parents[0] === baseSha), which is whatre-selected PR #557 on each tick. Fixing only the workflow would have left the
controller feeding it bad candidates.
This is not an outage, a stale result, or a cancellation. It reproduces
deterministically from the recorded API values.
Other open PRs confirm it is a live race rather than a one-off: #554 and #560
share the same stale cached pointer, but their merge refs still lag to match
it, so they pass. #291 and #295 have both values in sync. #557 landed on the
unlucky side of a skew that any PR can hit.
Change
Both call sites now accept the merge ref when its first parent matches
either accepted base — the cached
pulls/N.base.shaor the live basebranch tip from
git/ref/heads/<base.ref>— while the second parent must stillbe the live head. That second condition is the half of the guard that actually
detects a stale merge ref, and it is unchanged.
.github/workflows/codeql-analysis.yml— read.base.ref, resolve the livebase branch tip (shape-validated branch name, shape-validated SHA,
ghexitstatus preserved rather than
|| true, matching the file's existingconvention), and widen the
jqpredicate..github/scripts/codeql-open-pr-backfill.mjs—analysisSnapshotForPullRequesttakes an optionalbaseBranchSha;resolveLiveAnalysisSnapshotsresolves it once per distinct base branch andmemoizes.
.github/scripts/workflow-control-plane-contract.mjs— the contract pinnedthe old predicate (
.[0] == $base and .[1] == $head,parents[0] === baseSha). Replaced with assertions that pin the fixedbehaviour and the live-tip lookup in both files.
Fails safe: when the live tip cannot be resolved, behaviour is byte-for-byte
the previous conservative path.
Validation
Control-plane CI equivalents, all from
$GITHUB_WORKSPACE/trusted:node --checkon all 22.github/scripts/*.mjsbash -non all.github/scripts/*.shyaml.safe_loadon all 15 workflows + 2 composite actionsnode .github/scripts/codeql-open-pr-backfill.mjs --self-testOKnode .github/scripts/workflow-control-plane-contract.mjs --self-testOKgit diff --checkSix new deterministic examples in the backfill self-test cover live-tip parent,
cached-base parent, absent tip, neither-base-matches, malformed tip, and
wrong-head-parent.
End-to-end replay. The
scopestep was extracted from the YAML withyaml.safe_loadand executed against a mockghreplaying the exact recordedresponses for PR #557, for both the pre-fix revision (
git show HEAD:.github/workflows/codeql-analysis.yml, i.e.ac680d64— the revisionthat actually ran) and the fixed file:
The pre-fix run reproduces the incident log verbatim; the fixed run yields the
intended single-owner no-op.
Eight-case regression matrix over the fixed step, all correct:
analyze=false, PR run owns analysisanalyze=falseanalyze=falsebackfill_listener_owned: trueThe
jqpredicate was additionally proved directly against PR #557's liveparent array: old ⇒ stale, new ⇒ accepted; and against the #554/#560 shape
(merge ref lagging instead): both old and new ⇒ accepted.
What this does not fix
PR #557's
CodeQLcheck run100064989293istimed_outbecause GitHubcannot generate the comparison Advanced Security needs to classify alerts as
new-in-PR:
Source Lopu workflow run