fix(actions): CodeQL aggregate check times out on matrix language skew - #686
3 commits merged into
Conversation
github-actions
Bot
added
lopu: mergeable
🦄 Lopu review — #686: correct diagnosis, but it is a rival of #663. Recommend closing this one.Reviewed The diagnosis holdsI could not find a hole in it. Advanced Security opens the aggregate The loop is safe as written: only non-slowest legs wait (no leg can wait on itself), every exit path The measured skew table is the best part of this PR and I would like it preserved regardless of what happens to the code. Blocking: #663 fixes the same defect in the same hunk#663 was opened ~21 hours earlier and rewrites the same region of
Without contract coverage, a later prompt/workflow reflow can delete this step silently — which is the failure mode One real defect, if this branch is landed anywayThe guard step can turn the status="$(jq -r --arg name "$PACING_JOB" '
[.jobs[] | select(.name | endswith($name))] | .[0].status // "absent"
' <<<"$jobs_json")"
Reproduced with the exact program from the diff, against #663 already guards this with Why I did not patch it hereHardening a branch whose recommended disposition is closure would deepen the duplication, and the surviving implementation already carries the guard. I also did not open a third controller-repair PR for this defect and left Worth recording for #677: I verified that the reviewer-prompt paragraph #677 adds — "Before writing that fix, inventory the repairs already open with RecommendationClose #686 as superseded by #663, after folding this description's skew table into #663 as corroborating evidence. Lopu — Thingtime principal developer & PR manager. Handing over to the serialized fleet after this PR; the remaining candidates in this batch are untouched and unreviewed. |
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 #686 "fix(actions): CodeQL aggregate check times out on matrix language skew"Head What I comparedFull head against Key finding — the fix this PR exists to deliver is already merged into its own baseThe PR body states "File changed: Every commit on this head that touched
The base branch already carries the mitigation the body describes. I read it in the worktree's own copy of the workflow: the Ancestry makes the delivery route explicit — the fix reached Secondary finding — all that remains is Graphify churnThe ten remaining files are five The base is an ancestor of the head here, so unlike #709 this is a clean redundancy rather than a stale-snapshot regression — but it is still a large write with zero value. Changes madeNone, deliberately. There is no defect left to repair: the workflow fix is already in the base, and the diagnosis in the PR body is sound and was acted on. Re-applying it would duplicate merged code. The worktree is left clean and I inventoried the open repairs against RecommendationClose PR #686 without merging. Its diagnosis was correct and its fix has already shipped via #663/#671/#677. Leaving it open costs an ~81-check matrix and a Lopu review session per wake-up to re-review an empty control-plane diff. Validation
|
|
Lopu review — PR #686 ( Verdict: the diagnosis holds, but this is a rival repair of #663. Recommend closing #686 in favour of #663.The diagnosis is correctI re-derived the matrix-language-skew claim against live job timings on product-branch PRs, and it stands:
The one red The mechanism also demonstrably engages. On this PR's own Why I am not recommending this PR#663 — "CodeQL aggregate check times out on the largest PRs", opened 2026-09-06 00:50Z, 20 hours before this PR — repairs the same defect in the same file, is That is exactly the publisher fall-through #677 documents: two independent rewrites of the same lines never 3-way apply onto each other, so the second review pass opens a rival PR instead of folding into the first. #686 was authored 2026-09-07 06:57Z, after #677 was opened but before its prompt guard could land — this is the third instance of that pattern (#671/#675 were the first two), and the first to hit Where they differ, #663 is strictly stronger:
That last row is the one I would flag even if #663 did not exist. This PR adds a load-bearing, non-obvious ordering guard to the control plane with nothing in What I didNo changes. The worktree is clean and left at Deliberately so: hardening this PR toward parity with #663 would deepen a conflict that a human then has to resolve by hand, which is the specific outcome #677 argues against. There is also no failing check to repair here — all of #686's own checks are green. I ran RecommendationLand #663 and close #686 as superseded. If any part of this PR is judged worth keeping, the cheap version is to graft it onto #663 as a follow-up rather than to reconcile two whole-step rewrites. No CodeQL alerts are open on this head (trusted snapshot is — Lopu, automated repository review |
|
Lopu review — this fix works, but it collides with #663 I reviewed The mechanism demonstrably works. From this head's own run
The pacing step ran The blocking problem is that #663 fixes the same defect in the same file. I verified with a read-only They cannot both land — merging either strands the other. On the merits I'd keep #663: it adds a pre- Recommendation: close this as superseded by #663. I've deliberately made no edits here — the fix is correct as written, and rewriting these lines a third time is exactly the failure mode #677 exists to stop. Worth noting this is the second instance of that pattern in one batch: #671 and #675 both independently rewrite One small accuracy note for the record: the step comment says the skew "collapses to seconds". Measured it's ≈62s, floored by the fast leg's own analyze+upload after the barrier lifts. The conclusion still holds; #663's comment phrases it accurately as "about a minute later". Posted by Lopu ( |
|
🤖 Lopu review — post-batch review pass, dispatch The diagnosis is right and the code is correct — but this PR is a rival of #663, which has had the same fix open since run Both hold the fast Both read This is the class of defect #677 documents for #671 ↔ #675 — the reviewer prompt never tells Lopu to inventory the controller repairs already open, so each pass diagnoses from scratch and writes its own patch. #677's body predicted a recurrence; this PR is that recurrence, and it's the pair #677 doesn't yet mention. #677 should land first. Recommendation: close in favour of #663 — but keep two thingsI'd land #663 on the strength of its 151 lines of
I'm flagging the trade-off for a human rather than silently closing either PR. Review of the changeNo defects found.
Validation: existing The fix is self-demonstrating here too: |
|
🐙 Lopu — repository review of All checks green, 0 CodeQL alerts, control-plane contract still passes on this head. The fix is correct — but it is one of two rival PRs for the same defect. #686 and #663 cannot both land. Verified mechanically: Both rewrite the same region of My recommendation: keep #686, close #663. Same race removed with 70 lines instead of 252. The key reason: waiting on the sibling job to reach Things this gets right that are easy to get wrong:
I also checked the late-runner-allocation case that #663 adds a second barrier for: a not-yet-allocated matrix leg is listed as Two minor, non-blocking notes:
No changes made. I left both notes unapplied deliberately — the counter is cosmetic, and the contract assertion only makes sense once the #663/#686 selection is settled; editing now would deepen the conflict rather than resolve it. |
|
Lopu review — same defect as #663; I recommend closing this in favour of it The diagnosis here is the best-evidenced of the five open controller repairs. The skew/outcome table across eleven PR heads — every skew ≤199s passing, both timeouts in the 387–392s band, #660 passing at 388s, #649 losing by one second ( The problem is that #663 (opened a day earlier) repairs the same defect in the same region of the same file: Neither can be folded into the other. Comparing on the merits, #663 wins on three substantive points:
Smaller point: the jobs lookup is unscoped ( Recommendation: close as superseded by #663 — but carry this PR's measured skew table across to it. That evidence is the better of the two and should outlive the patch. If the 403-line surface of #663 is judged too much and this smaller step is preferred instead, then #663's start barrier and language cross-check should be brought over here rather than dropped. This is good work; it collided only because the review session that authored it had no instruction to inventory open controller repairs first — the gap #677 closes. Validation at |
|
Lopu review — #663 and #686 are rival repairs for one defect. Exactly one must land. Both patch The defect is real — I reproduced it live on #557, whose check concluded Merging both is actively harmful, not just redundant. The
That is exactly the My recommendation: land #686, close #663. Both work; the tiebreaker is a real correctness property rather than size. #663 waits on the #663 is the more thorough engineering and its contract test is the one asset #686 lacks. Best path: land #686, then port #663's contract assertions (renamed to #686's step) as a small follow-up so the fix stays regression-locked. Choosing #663 instead is entirely defensible — the only non-negotiable is that one of the two closes before either merges. Full reasoning is in my review reports for both PRs. I made no changes to either branch: the diffs are correct, the problem is coordination. |
|
🐙 Lopu — gap to absorb before this lands: the aggregate check latches red From reviewing #557, whose Both analyses succeeded ( The gap: the upload-ordering fix here prevents recurrence on future scans, but it does not reopen an aggregate check that has already closed on an existing head. On #557 there is exactly one Net effect: #557 stays red even after this merges, until something forces a fresh analysis for its current revision. Worth either handling here (a way to force a re-upload for an already-analyzed SHA — note the scope job's "already has both Lopu CodeQL language analyses" guard blocks a plain re-dispatch) or tracking explicitly as a follow-up, so the fix isn't judged by a PR it cannot repair. I did not open a third controller PR for this — #663 and #686 are already two repairs for one defect, which is what #677 exists to prevent. Recommendation unchanged: land #677, then pick exactly one of #663/#686 and close the other. |
github-actions
Bot
added
the
lopu: queued
🤖 Lopu live PR updateStatus: ✅ Lopu finished — this PR is mergeable Current phase: GitHub verified the published branch result Estimated completion: Done — no further active-work ETA. Time conversion (UTC source)
Los Angeles and Melbourne use their real IANA time zones, so PDT/PST and AEST/AEDT offsets change automatically. Lopu queue and PR pulse
Related PR context
Exact branch pair: Timeline
Technical run details — optional; this comment is the human-facing source of truth. |
github-actions
Bot
added
lopu: conflicting
|
🤝 Merged Conflicted files:
Please review the merge commit before relying on it. |
github-actions
Bot
added
lopu: resolving
…olved conflicts) Conflicted paths: .github/workflows/codeql-analysis.yml Resolved by the resolve-pr-conflicts workflow: https://github.com/lopugit/thingtime/actions/runs/34323168283 Lopu-Conflict-Resolution: run=34323168283 pr=686 Co-Authored-By: Lopu <github-actions[bot]@users.noreply.github.com>
Structural `graphify update` followed by LLM semantic `graphify extract` (graphify 0.9.4, claude-cli backend); unchanged content served from the tracked semantic cache. Refreshed by the resolve-pr-conflicts workflow: https://github.com/lopugit/thingtime/actions/runs/34323168283 Lopu-Conflict-Resolution: run=34323168283 pr=686
github-actions
Bot
added
the
lopu: overlapping files
github-actions
Bot
added
lopu: mergeable
|
Lopu · repository review This PR no longer carries its fix, and I think it should be closed. The deliverable was the 70-line pacing step The merge The difference matters. This branch's step releases the fast So the fix isn't merely redundant — it's the weaker of the two, and the better one is already live. The aggregate What remains is ~20k lines of regenerated |
|
Lopu review — this PR is superseded and should be closed. I compared the full head The AI-resolved merge Both hold the fast
Landing What's left is only generated churn. The Checks are green, including the aggregate No changes made. Editing this branch just to produce a commit would be churn. Recommendation: close as superseded by #671; there is nothing here to salvage into another PR. Lopu · automated repository review |
|
Lopu review — this PR's fix is already on Comparing That is not a lost fix. The AI-resolved merge (
Ordering on the upload rather than on job completion is the right call: a job can complete having declined its upload, and this branch's version would have treated that as a satisfied wait — reopening the exact Checks are green (23 pass / 58 skipping / 0 failing) and the CodeQL snapshot for this head is empty, so there is nothing red to diagnose and no alert to fix or dispose. Recommendation: close without merging. Merging now would land only a graphify snapshot rotation, and leaving it open keeps a second open repair touching |
🦄 Lopu review — this PR no longer contains its repairRecommendation: close as superseded. The diff against Everything remaining is regenerated The repair commit did carry a real fix — Dropping it was correct.
Re-adding the step would layer a second, narrower wait on top of a better one — the "serializing trades Why not just merge it: the only payload is a Graphify snapshot re-key, so merging retires the base branch's active snapshot and installs this branch's — zero controller change, under a title claiming a fix the commit no longer contains. The changelog would credit a repair that merged as a no-op. The reason this redundant PR never closed itself (Graphify refreshes keep the diff non-empty) is already fixed in open repair #724, so I deliberately did not write a rival controller fix for those lines. Checks were all green and CodeQL reported 0 open alerts on this head — nothing to diagnose or dispose. |
|
Lopu review — this PR is now a no-op and should be closed as superseded. I compared head
That merge resolution was correct — do not restore the hunk. Base already implements this exact guard, better, at
That guard reached base as Restoring the dropped step would re-add a weaker duplicate of a guard that is already live, putting two independent waits on the same matrix leg. I made no code change here; there is no defect left on this branch to repair. Validation run: Recommended disposition: close #686 as superseded by #663. Its remaining diff is only a graphify refresh and carries no controller change worth merging. |
Lopu review — this fix has already landed; the PR is now emptyI compared head Every commit on this head that touched I confirmed the mitigation is genuinely present rather than merely claimed, by reading the base's own Independent corroboration: The ten remaining files are five Recommendation: close without merging. The diagnosis in this body was correct and was acted on. Leaving it open costs an ~81-check matrix and a Lopu review session per wake-up to re-review an empty control-plane diff. I made no changes and wrote no — Lopu, automated repository review |
github-actions
Bot
removed
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 — CodeQL aggregate check times out on matrix language skew
File changed:
.github/workflows/codeql-analysis.yml(only)Found while reviewing: PR #649 (
claude/subspace-communities→develop), headcb9d095dAlso currently affecting: PR #683. Previously recorded in this file's own comments as the PR #557 incident.
Diagnosis
PR #649's only failing check is
CodeQL, owned by thegithub-advanced-securityapp (check run
101637358172), conclusiontimed_out. It is not a findingand not a code defect:
(
825e20669a,refs/pull/649/merge) succeeded and uploaded:/language:actionsat05:54:39Z(0 results) and/language:javascript-typescriptat06:01:06Z(12 results).(
js/insecure-randomness×10,js/stack-trace-exposure,js/xss-through-dom),none of them in a file this PR touches. Open alerts on the PR ref: 0,
which matches the trusted snapshot.
Analyzejobs both reportedsuccess(
actions60s,javascript-typescript452s).Advanced Security opens a PR's aggregate
CodeQLcheck when the firstanalysis for the ref lands and closes it
timed_outif the remaining languageshave not answered inside its own budget. On this tree
actionsfinishes inabout a minute while
javascript-typescriptnow needs seven or more, so thefast language opens the window roughly 390s before the slow one can answer it.
Measured upload skew (
actions→javascript-typescript) against the liveoutcome across the repository's open PRs:
Every skew ≤199s passed; both timeouts sit in the 387–392s band, and #660 at
388s shows the cliff is not a clean threshold but a coin flip once the skew
approaches it. PR #649 lost that flip by one second: the check closed at
06:01:05Zand thejavascript-typescriptanalysis landed at06:01:06Z.This is the same failure the
scopejob above already guards against — quotedin its own comments as PR #557, where the check closed
timed_outat11:36:46Z, exactly 387s after the fast analysis opened it — but reachedthrough matrix language skew rather than through duplicate scans. The
existing guard cannot see it, because here there is no duplicate scan: a single
pull_requestrun, two languages, one of which is simply far slower.It is therefore a control-plane defect rather than a PR defect, and it is not
self-correcting:
javascript-typescriptanalysis time grows with the tree, sothe skew drifts further into the failing band with every large PR. A bare
re-run is not a repair either — it re-rolls the same coin flip.
Fix
Hold the fast language until the slow one has uploaded, so the aggregate check
opens on the slow analysis and the residual skew collapses to seconds —
deep inside the band that has never timed out. New step
Let the slowest language open the aggregate checkin theanalyzejob,gated on
matrix.language != 'javascript-typescript', polling the siblingmatrix job in the same run via the
actions: readpermission the job alreadyholds.
Design points:
Initialize CodeQLand before the ownership re-check, so thefast language's database build still overlaps the slow analysis, and so
Confirm this push still owns the analysiskeeps its "last moment beforeupload" meaning instead of going stale across the wait.
completedregardless of conclusion. A failed sibling has noupload left to wait for; stalling would hide a broken analysis behind a slow job.
timeout-minutes, with a 120sgrace for an absent sibling. Every degraded path uploads rather than letting
the runner kill the job, which would discard a built database and produce
exactly the red check this step prevents.
gh's exit status (never|| true), matching the conventionthe
scopepre-flight and the ownership step already follow; a transientlookup failure falls back to the prior unpaced behaviour.
workflow_callruns prefix job names(
control-plane / Analyze (javascript-typescript)).Job names, check contexts, the matrix, permissions and the concurrency group are
all unchanged, so no required-status-check configuration is affected.
Expected cost: the
actionsjob idles instead of finishing early; totalwall-clock for the run is unchanged (it was already bounded by the slow language).
Validation run
yaml.safe_load; asserted the new step isordered before
Confirm this push still owns the analysis; confirmed matrix,actions: readpermission andtimeout-minutes: 60unchanged. Pass.run:block,bash -n. Pass.jqfilter against thelive jobs JSON of the failing run
34088527888. Resolvedcontrol-plane / Analyze (javascript-typescript)→completed, matchedexactly one job, and did not match
Analyze (actions). Pass.scenarios: sibling already done → paced at 0s; sibling finishes at 450s →
paced at 450s; sibling absent → unpaced at 120s; sibling stuck → uploads with
a warning at 1800s (inside the 3600s job timeout);
ghlookup fails →unpaced immediately. All five terminate and upload. Pass.
actionlintis not installed on this runner, so that check was skipped ratherthan reported as passing.
Not done deliberately
649.jsondispositions remain[]: thereare no open alerts on this PR head, so there is nothing to dispose of.
carries none of this fix. That worktree is clean.
the dedicated Lopu fix branch against
github-actions.Source Lopu workflow run