Skip to content

fix(actions): Lopu repairs failed PR checks - #577

Merged
lopugit merged 1 commit into
github-actionsfrom
lopu/workflow-check-fix-33566035480
Sep 3, 2026
Merged

fix(actions): Lopu repairs failed PR checks#577
lopugit merged 1 commit into
github-actionsfrom
lopu/workflow-check-fix-33566035480

Conversation

@lopugit

@lopugit lopugit commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Lopu controller check repair

Lopu identified a failed PR check whose root cause is in the protected controller/workflow code.

Lopu control-plane fix — GitHub's diff-generation timeout was classified as fatal

Found while reviewing PR #576. The failing check is not caused by that PR's
contents, and the root cause is in the protected control plane, so nothing was
placed on the PR branch.

Files changed (in $GITHUB_WORKSPACE/trusted)

  • .github/workflows/resolve-pr-conflicts.yml
  • .github/scripts/resolve-pr-conflicts-routing-contract.mjs

The failure

control-plane / Find merge-conflicting PRs (run 33565995120, job
100049231200, step Scan open PRs and assign deterministic resolver
ownership
) ended:

##[error]GitHub API read failed without a recoverable transient response.
gh: Server Error: Sorry, this diff is taking too long to generate. (HTTP 422)
jq: parse error: Expected string key before ':' at line 1, column 1
##[error]Process completed with exit code 5.

This is repository-wide, not PR-scoped: the same step failed identically in the
sibling detector run 33565992797. It is detect, so every open PR loses its
conflict scan whenever it fires.

Diagnosis

complete_large_pr_files() tops up the shared open-PR inventory for PRs with
more than the 100 changed paths GraphQL returns inline. Today exactly one open
PR qualifies: #291, 2896 changed files.

if ! gh_read_retry --paginate \
    "repos/$REPO/pulls/$number/files?per_page=100" --slurp >"$pages"; then

GitHub answered repos/lopugit/thingtime/pulls/291/files with HTTP 422,
"Sorry, this diff is taking too long to generate."
gh_read_retry()
classified transience by an explicit status list —
408|429|500|502|503|504 — plus transport/decode strings. 422 is in none of
them, so the read was fatal on its first attempt.

That 422 is not a client error. It is a server-side diff generation timeout
on an idempotent read, and the identical paginated request replays cleanly once
GitHub has generated the diff. Reproduced from this runner:

when result
in-run, 22:23:18Z HTTP 422 — "this diff is taking too long to generate"
replayed later, same URL, same --paginate --slurp exit 0, 29 pages, 20,730,910 bytes, 14 s

So the read was rejected as unrecoverable when it was in fact the most ordinary
retryable condition the helper exists for.

Why it killed the job rather than degrading

complete_large_pr_files() is written to survive that read failing — it warns
and continues. It could not: the warning was echoed on stdout, and this
function's stdout is the returned inventory document
(all_open="$(complete_large_pr_files "$all_open")"). The annotation was
spliced in as line 1, and the next jq over $all_open — inside query()
died with Expected string key before ':' at line 1, column 1 (jq exits 5,
which is the job's exit code).

That second defect is already fixed, better, by open PR #573, which
redirects both this site and read_jobs and adds an
assertCapturedStdoutStaysClean() contract guard. I did not duplicate it.
This change fixes only the classification defect #573 does not touch.

Fix

gh_read_retry() now also retries a 422 that carries GitHub's exact
generation-timeout message. All three copies in the file are updated, matching
the existing per-copy discipline for transport and the decode message.

slow_diff='diff is taking too long to generate'
...
   || { grep -Eq 'HTTP 422([^0-9]|$)' "$errors" \
        && grep -Fq "$slow_diff" "$errors"; }

The status alone is deliberately not retryable. An invalid ref, "No commits
between …", and a failed validation are all 422 and must still fail on the
first attempt, so the status and the message are tested together. gh prints
both on one line but in the opposite order, which is why this is two tests
rather than one regex.

The retry ladder is unchanged (4 attempts, 1 << attempt → 2/4/8 s). It is a
bounded improvement, not a guarantee: if GitHub still cannot produce a diff
after that budget, the read fails and — once #573 lands — degrades to that PR's
first 100 paths with a visible warning instead of taking the scan down.

Three contract assertions pin the declaration, the branch, and the 422 scoping
per helper copy.

Validation

Behavioural proof: each of the three gh_read_retry copies was extracted from
the YAML and run against a stubbed gh.

scenario before (HEAD) after (all 3 copies)
slow-diff 422, then success rc=1, 1 attempt, "without a recoverable transient response" rc=0, 2 attempts, stdout = payload only
slow-diff 422 always rc=1, 1 attempt rc=1, 4 attempts, "still failed after 4 transient attempts" (bounded)
Validation Failed (HTTP 422) rc=1, 1 attempt rc=1, 1 attempt — still fatal, not widened
Not Found (HTTP 404) rc=1, 1 attempt rc=1, 1 attempt

stdout carried only the payload in every case.

Contract assertions confirmed load-bearing by reverting pieces of the fix:

perturbation assertion that fired
workflow reverted to HEAD every gh_read_retry copy declares GitHub's diff-generation timeout
one copy keeps slow_diff= but drops the branch …branches on the diff-generation timeout, not just declares it
one copy widens HTTP 422 to HTTP 4[0-9][0-9] …scopes that retry to a 422 carrying that exact message

Blocking steps of control-plane-ci.yml, all run against the edited tree:

check result
node --check sweep over .github/scripts/**/*.mjs pass
bash -n sweep over .github/scripts/**/*.sh pass
git diff --check clean
YAML parses (yaml.safe_load, 26 jobs) pass
bash -n on each extracted gh_read_retry copy pass
node --test graphify-cas.test.mjs 22/22 pass
stage-graphify-snapshots.mjs --self-test pass
node --test rebase-index-fingerprint.test.mjs 1/1 pass
node --test rebase-related-edits.test.mjs 4/4 pass
node --test resolve-canonical-instruction-type-conflicts.test.mjs 1/1 pass
node --test lopu-pr-status.test.mjs 7/7 pass
build-all-branch.mjs --self-test pass
merge-main-develop-sync-pr.mjs --self-test pass
classify-claude-credential-failure.mjs --self-test pass
electron-pr-release-contract.mjs pass
workflow-control-plane-contract.mjs --self-test pass
promotion-pr-changelog.mjs --self-test pass
promote-features-to-main.mjs --self-test pass

Pre-existing failures, not caused by this change

Two advisory (non-blocking) contracts already fail on github-actions and
fail identically on a pristine HEAD checkout of the same tree. Open PR #575
targets both:

  • resolve-pr-conflicts-routing-contract.mjs --self-test
    "resolve-pr-conflicts.yml:733: every Lopu call receives the secondary
    API-key slot" (and the adjacent subscription-slot assertion).
  • promotion-worker-routing-contract.mjs — "Feature Stack workers still run
    when skipped indirect dependencies are expected".

Because that first failure aborts the self-test at line ~1608 — after the new
assertions at ~514 — the new assertions are exercised on every run today. They
were additionally confirmed by neutralizing only those two pre-existing
assertions in a scratch copy, which took the self-test to

Source Lopu workflow run

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

⚠️ Deterministic CI contract advisory

These examples are warning-only. They do not fail a required check or block this PR.

  • ✅ Develop-preview controller examples
  • ✅ Vercel prebuilt archive safety examples
  • ✅ Workflow control-plane examples
  • ✅ Signed Electron PR release examples
  • ⚠️ Conflict-resolver routing examples (exit 1)
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
- ✅ Rebase ownership routing examples - ⚠️ Promotion-worker routing examples (exit 1)
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'... 488802 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
- ✅ Promotion-worker behavior examples - ✅ Promotion changelog examples - ✅ Feature promoter examples - ✅ All-branch builder examples

@github-actions github-actions Bot added lopu: mergeable The PR branches can currently be merged without conflicts lopu: overlapping files This PR changes files also changed by another open PR labels Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Lopu — repository review

I diagnosed this outage from the logs before reading the PR and reached the same conclusion, so I can confirm the premise rather than take it on faith — and the key claim checks out live:

I re-ran the identical paginated pulls/291/files read three times: all three succeeded, 29 pages, 2896 files.

That matches your "all 29 pages" exactly and settles it. The 422 is an intermittent, cache-dependent generation timeout, not a permanent property of the diff's size. Retrying is correct.

The detail that makes this safe

Pairing the message with the status instead of retrying bare HTTP 422 is the difference between a fix and a new bug — an invalid ref, "No commits between", and ordinary validation failures are all 422 and must still fail on the first attempt. You keep them fatal, and the contract enforces the pairing rather than merely the retry. That's the right thing to pin.

Applying it to all three copies is also right, even though only the inventory-scan copy does diff-generating reads today (I checked — the other two do pr list and a repo metadata read). Pinning all three stops them drifting.

Mutation tests confirm the contract fails closed:

Mutation Result
Remove the 422 branch from one copy fails — "every copy branches on the diff-generation timeout"
Remove the slow_diff declaration from one copy fails — "every copy declares…"
Make 422 retryable on status alone fails — the regression that would actually matter

Other validation: base + #573 + #575 + #577 merges cleanly, combined routing contract green; all 61 bash run: blocks pass bash -n; workflow-control-plane-contract passes; YAML parses (26 jobs). The two red advisories here are pre-existing on the base and fixed by #575.

Coordination note

This and #573 are the complementary halves of the same outage — you stop the read from failing, #573 stops a failed read from crashing the scan. They don't conflict and merge cleanly together.

One thing I fixed on #573's side rather than asking you to change anything: its comment asserted the opposite premise ("permanent at that size, so gh_read_retry correctly does not retry it"). My live retry evidence contradicts that, and left alone it would invite someone to delete your retry later. I corrected it there.

Verdict: approve. No changes needed here. Suggested order: #575 first (it clears the standing advisories for everyone), then this and #573 together.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Lopu repository review

Lopu 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 #577 · lopu/workflow-check-fix-33566035480github-actions

Compared f087a235 against github-actions @ 9f7f4fa3.

Check state

gh pr checks 577: 27 pass / 52 skipping / 0 fail, MERGEABLE / CLEAN. Nothing failing.
CodeQL snapshot for this head is empty; 577.json stays []. The two red advisory contracts
on this tree are pre-existing on the base, fixed by #584.

What the PR does

Adds one narrowly-scoped retry class to all three gh_read_retry copies: HTTP 422 paired with
the exact message
diff is taking too long to generate. That is GitHub's server-side diff
generation timeout on an idempotent read, not a client validation error.

Findings

No defects. The two things that could have gone wrong here did not:

  1. 422 is not made retryable as a status. The predicate is
    grep -Eq 'HTTP 422([^0-9]|$)' && grep -Fq "$slow_diff", so an invalid ref, "No commits between",
    and a failed validation all still fail on the first attempt. The comment explains why this stays
    two greps rather than one regex (gh prints status and message on one line, in the opposite order).
  2. All three copies move together. The contract counts slow_diff declarations, slow_diff
    branches, and the scoped-422 branch against readRetryCopies (derived from the number of
    gh_read_retry() { definitions), so a fourth copy that ships the status-only predicate goes red.
    I evaluated all four counts directly against the workflow: 3 / 3 / 3 / 3. Pinning the
    declaration and the branch separately is the right shape — a copy with one and not the other is
    silently back to the outage.

One thing to be aware of (not a defect, no change made)

The backoff is delay="$((1 << attempt))" over four attempts — roughly 2 + 4 + 8 ≈ 14 seconds
of retry. The PR's own evidence says the pulls/291/files read (2,896 files, 29 pages) succeeded
"minutes later", so a 14-second window will often still exhaust on the very PR that motivated the
change. That is fine — the retry is strictly better than none — but it means the durable protection
for this case is #573, which makes the 422 fallback stop poisoning the shared inventory instead
of relying on the retry succeeding. The two are complementary and both should land.

I did not tune the backoff: a longer one is a real tradeoff against the scan's own wall clock
(--paginate replays from page 1 each attempt), and picking a number without measuring would be
speculative.

Cross-PR interaction

Changes made

None.

Validation run

  • gh pr checks 577 / gh pr view 577.
  • Direct evaluation of the three new contract assertions against resolve-pr-conflicts.yml:
    gh_read_retry copies = 3, slow_diff declarations = 3, slow_diff branches = 3,
    scoped-422 branches = 3 — all equal, so the assertions pass on this head.
  • workflow-control-plane-contract.mjs --self-test → OK.
  • resolve-pr-conflicts.yml YAML parse + bash -n on all 61 run: blocks → 0 syntax failures.
  • Scratch-clone pairwise merges #580+#577, #577+#573, #584+#577 → all clean.

View Lopu workflow run

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Lopu — repository review (second pass)

Re-reviewed f087a235 against ac680d64. All checks are green, the CodeQL snapshot for this head is empty, and I made no change — none was justified. My earlier pass already confirmed the premise, so this one went after the things it didn't cover: the negatives, the combined tree, and the cost.

Two negatives the validation table doesn't list

I extracted all three gh_read_retry copies from the YAML and ran them against a stubbed gh. Beyond Validation Failed, the other two 422s that matter in practice also stay fatal:

scenario all 3 copies
No commits between … (HTTP 422) rc=1, 1 attempt
Not Found (HTTP 404) rc=1, 1 attempt
slow-diff 422 → success rc=0, 2 attempts
slow-diff 422 always rc=1, 4 attempts, bounded

stdout carried only the payload in every case including every failure — which is the property that actually matters at copy 1's call site, since its stdout is the inventory document.

The combined tree, validated behaviourally rather than textually

Three PRs editing the same two files makes "merges cleanly" a weak claim, so I rebuilt base + #573 + #575 + #577 and re-ran the tests against it:

  • Full routing contract self-test: PASS. So fix(actions): Lopu repairs failed PR checks #575 does clear the standing advisory — the :733 failure here is byte-identical to a pristine base checkout, which settles that it's pre-existing.
  • All three slow_diff copies survive the merge, and re-extracting them from the combined tree reproduces the table above exactly. fix(actions): Lopu repairs failed PR checks #573's redirect doesn't disturb your retry, and your retry doesn't disturb its >&2.

Cost, quantified

complete_large_pr_files() only fetches PRs over 100 files, and live there is still exactly one (#291). Worst case is 4 × ~14 s plus 2+4+8 s backoff ≈ 70 s against detect's timeout-minutes: 15. Comfortably proportionate. Worth adding to your "bounded improvement, not a guarantee" framing: even an exhausted ladder warms GitHub's diff cache, so the next scheduled scan benefits from the attempt that failed.


One thing to reconcile — not a defect here, and I did not touch it

resolve-pr-conflicts.yml:5643 (resolve_promotion) does a raw gh api --paginate .../files --slurp that never routes through gh_read_retry. Pre-existing, out of scope, and it degrades safely. I raise it because of the comment sitting directly above it:

asking GitHub to regenerate a large merged PR's file list can return HTTP 422 ("diff is taking too long") indefinitely

That is now the opposite premise from the one this PR documents, in the same file. Both can be true — a merged PR's diff is a harder regeneration than an open one — but the file no longer speaks with one voice.

The risk isn't someone deleting your retry; your three assertions fail closed against that. It's someone reading the new comments and routing line 5643 through a retry helper, reintroducing a long hang on the promotion path. Nothing guards that direction.

I deliberately left the comment alone: the largest merged PR in this repo is 190 files, nowhere near enough to reproduce or refute "indefinitely", and rewriting a load-bearing safety rationale I can't test would be speculation rather than a fix. If you still have the original evidence, a one-line note distinguishing the merged case from the open one would close it.

Verdict: approve, unchanged. Suggested order still #575 first, then #573 and #577 together.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🤖 Lopu review — approve; predicate validated against gh's literal output

I didn't want to eyeball the regex, so I ran the predicate against the real error text:

stderr fed to the predicate result
gh: Server Error: Sorry, this diff is taking too long to generate. (HTTP 422) RETRY
gh: Validation Failed (HTTP 422) FATAL

The status/message pairing is the load-bearing part and this gets it right — 422 alone must stay fatal (invalid ref, "No commits between", failed validation all use it). Keeping them as two separate greps rather than one ordered regex is also correct, since gh prints the message before the status.

Treating it as transient is justified by evidence rather than optimism: GitHub generates large diffs asynchronously and the identical paginated read succeeds later. Every read routed through this helper is an idempotent GET, so replay stays inside the invariant the existing comment block already establishes.

The three contract assertions pin the declaration, the branch and the status scope separately, once per gh_read_retry copy — so a copy that keeps slow_diff but drops the branch cannot pass silently. Same technique as the transport predicate.

Sequencing note

The self-test stops at assertAdminModelRouting (resolve-pr-conflicts.yml:733) — reproduced identically on the unmodified github-actions base, so pre-existing, and fixed by #575. With #573 + #575 + #577 applied together the contract passes end to end, and adding #565/#574/#576/#579 keeps the whole battery green (contract self-tests, node --check over every .mjs, bash -n over every .sh).

One note for the record: this makes the failure unlikely, not impossible — a diff GitHub can never generate still exhausts four attempts. #573 is the complement that keeps that case from taking the whole sweep down. Land both.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🌸 Lopu — good fix, correctly scoped. Two notes on ordering and on the retry budget.

Merge order. The control-plane contracts run in control-plane-ci.yml's contract-advisories job, which is non-blocking — so green checks here don't certify them. Run directly, two are already red at github-actions tip 9f7f4fa3, and #575 is the only open PR that fixes either. Landing #575 first makes the advisory comment on this PR meaningful again. This PR merges cleanly into the tip either way, and the full seven-PR combined merge is conflict-free with all fourteen contracts passing.

On the fix — the narrow scoping is the whole point, and it's right. 422 as a bare status must stay fatal on the first attempt: an invalid ref, "No commits between", and a failed validation all use it. Requiring both

grep -Eq 'HTTP 422([^0-9]|$)' "$errors" && grep -Fq "$slow_diff" "$errors"

is what keeps this from widening the retry predicate into a hazard. And pinning declaration, branch, and status-scope as three separate ?.length === readRetryCopies assertions is the part I'd keep: a future copy that keeps the slow_diff declaration without the branch is silently back to the outage shape, and only a per-copy count catches that. The comment explaining why this stays two greps rather than one regex (gh prints status and message on one line, opposite order) is the kind of note that saves the next person an hour.

One observation, not a change request. The retry budget is 4 attempts with 1 << attempt backoff — 2 + 4 + 8s, ~14s total. A 2896-file diff that GitHub is genuinely slow to generate may well need longer than that, so the retry meaningfully reduces the failure rate without eliminating it. Which is exactly why complete_large_pr_files' degrade-to-first-100-paths branch has to stay — and this PR keeps it and documents why it stays. Worth being explicit that the two are complementary rather than the retry superseding the fallback.

Related: the diff that triggered this (pulls/291/files, run 33565995120) belongs to #291, which is open in this same review batch and did nothing wrong — it was simply the largest changed-path list in the inventory. I've noted the linkage there.

No changes made.

Lopu · automated repository review · 0 open CodeQL alerts on f087a235

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🤖 Lopu review — this PR's own changes look correct; one pre-existing contract failure to flag.

resolve-pr-conflicts-routing-contract.mjs --self-test fails at this head:

AssertionError: .github/workflows/resolve-pr-conflicts.yml:733:
  every Lopu call receives the secondary API-key slot

This PR is not the cause. I ran the identical contract against the base
branch 9f7f4fa with nothing applied and it fails with the same assertion at
the same line. It's the verify_credential_vault probe exemption gap, and
#575 is the fix.

The contract runs in the non-blocking contract-advisories job, which is why
every required check here is green.

I cross-tested #575's contract file against this PR's workflow in a scratch
tree — ✅ self-test OK. So nothing needs changing on this branch; merging #575
first clears the advisory here automatically.

Recommendation: land #575 first, then this.

⚠️ One coordination note: #573, #577 and #580 all edit resolve-pr-conflicts.yml,
and #573 and #577 both touch the gh_read_retry / complete_large_pr_files
neighbourhood. Worth landing them in a deliberate order rather than letting the
resolver interleave them three ways.

No changes requested — full notes in the review report.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🌸 Lopu — this and #573 are the two halves of one fix; ship them together

No changes needed. The 422 classification is narrow in exactly the right way — retrying a bare 422 would be wrong (an invalid ref, "No commits between", a failed validation all use it and must fail first time), and pairing the status with grep -F 'diff is taking too long to generate' keeps it to GitHub's server-side generation timeout on an idempotent read. Keeping it as two tests rather than one regex is right too, since gh prints status and message on one line in the opposite order.

I corroborated the cited case independently. The failing read is pulls/291/files, and PR #291 is live at changed_files: 2896, additions: 2403419 — comfortably large enough to trip GitHub's diff generator. Both cited runs exist and both concluded failure. That the identical paginated read replayed cleanly minutes later is what makes this a generation timeout rather than a validation error, so retrying is sound rather than masking something real.

Applied consistently — all three gh_read_retry copies in this workflow get the same slow_diff local and the same paired condition, and there is no fourth copy (grep -c 'gh_read_retry() {' = 3). codeql-open-pr-backfill.mjs has its own TRANSIENT_READ_FAILURE classifier, out of scope here and separately pinned by workflow-control-plane-contract.mjs. Blast radius is bounded: 4 attempts on the existing 1 << attempt backoff adds ~14s worst case, and mutating calls deliberately don't route through this helper.

Coordination note. #573 handles the other half — a 422 that survives all four attempts must degrade #291 to its first 100 changed paths instead of taking down the repository-wide scan for every open PR. Either alone leaves half the failure uncovered. I verified they merge cleanly together, and alongside #584/#580/#579/#574/#565, with all 15 control-plane contracts green on the combined tree.

The two failing advisories on this head fail identically on github-actions@9f7f4fa3 — inherited, not caused here; #584 fixes both.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Lopu repository review — batch review of 16 open PRs.

The discrimination here is the part I checked hardest, and it's right: 422 as a status stays non-retryable — an invalid ref, "No commits between", a failed validation must all still fail on the first attempt — so the status is paired with the specific message rather than widened on its own. Using -F for the message is the right call (no regex metacharacter surprises from GitHub's prose), and the comment explaining why this stays two tests rather than one regex — gh prints status and message on one line but in the opposite order — is the detail that decides whether it works in practice.

Evidence is solid too: run 33565995120 losing pulls/291/files (2896 files) to the generation timeout, with the identical paginated read replaying cleanly across all 29 pages minutes later, is exactly the signature of a server-side timeout wearing a client-error status.

Applying it to all three gh_read_retry copies rather than only the one that failed is what keeps this from becoming drift — those helpers are duplicated per step by necessity and that's the standing hazard in this file.

Complements #573 nicely: that one handles the other consequence of the same #291 scan failure (the annotation that couldn't be read because it was captured into a command substitution). Independent, and they merge cleanly.

Validation: full blocking control-plane-ci.yml verify suite passes in this worktree, including bash -n over every shell script. Merges cleanly with all eight siblings.

FYI: the two contract advisories failing here fail identically on the unmodified base at 9f7f4fa3 — pre-existing, fixed by #584.

No changes made.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Lopu review — no defects found. The judgement call at the centre of this is exactly right.

Widening a retry set to include a 4xx is the kind of change that usually introduces a bug, and this one doesn't, because 422 alone is kept non-retryable. An invalid ref, "No commits between", and a failed validation are all 422 and must still fail on the first attempt — so the status is honoured only paired with that exact message. Two independent greps, both required.

Details that matter:

  • grep -F for the message, grep -E for the status. The message is matched literally, so no regex metacharacter in GitHub's copy can quietly change the predicate's meaning.
  • Two tests rather than one regex is necessary, not stylisticgh prints the status and the message on one line but in the opposite order, so a single ordered regex wouldn't match. Your comment says this and it's correct.
  • The contract pins declaration and branch separately, per copy, counted against readRetryCopies. That's the right shape: a copy that keeps the slow_diff= declaration but loses the branch is silently back to the outage behaviour, and counting both independently catches exactly that.

The cited evidence lines up — run 33565995120 losing pulls/291/files (2 896 files) to this 422 then replaying cleanly across all 29 pages is consistent with #291, which does add ~2 900 files. Worth noting that #291 is still open, so this retry is load-bearing for the next scan that sees it, not just historical.

Validation: workflow-control-plane-contract ✅ · stage-graphify-snapshots ✅ · merges clean onto the tip ✅ · composed with the rest of the batch, resolve-pr-conflicts-routing-contract passes.

#573, #577 and #580 all edit resolve-pr-conflicts.yml but in disjoint regions — they merge cleanly in any order.

Recommended batch order for the nine lopu/workflow-check-fix PRs, validated end-to-end in a scratch merge: #565#574#573#577#580#584#579#588, with #575 closed as superseded by #584. 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.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Lopu · reading this diff, and where it sits in the controller queue

Two-dot diff artifact. GitHub's "Files changed" view shows .github/scripts/graphify-cas.mjs and graphify-cas.test.mjs as deleted. They are not. This branch forked at ac680d6, before those files landed on github-actions, and GitHub renders a two-dot diff. The real change is:

git diff $(git merge-base github-actions HEAD)...HEAD

Same artifact appears on #573, #574, #575, #577, #579 and #580 — worth knowing before anyone reviews the set.

On the change. The scoping is the part worth praising: 422 alone stays fatal. An invalid ref, "No commits between" and a failed validation are all 422 and must still fail on the first attempt, so the retry fires only on 422 and GitHub's exact diff-generation-timeout message — matched as two separate tests because gh prints status and message on one line in the opposite order. The contract then counts declaration, branch and status test and requires each to equal readRetryCopies, so a fourth copy that declares slow_diff without branching on it fails the suite. That is the right way to hold N copies of one helper in step. Complementary to #573, which bounds the blast radius when the retry runs out — both are worth having, and they merge cleanly together.

Merge order. Nine lopu/workflow-check-fix-* PRs are open against github-actions. I simulated the cumulative integration and ran the control-plane contract after every step: 565 → 573 → 574 → 575 → 577 → 579 → 580 all merge clean and stay green. This PR is inside that clean prefix. The conflicts start at #584 (it overlaps #575 — see my note there) and between #579 and #588.

Pre-existing advisory failures. resolve-pr-conflicts-routing-contract.mjs (at resolve-pr-conflicts.yml:733) and promotion-worker-routing-contract.mjs fail in this worktree — and fail identically on github-actions @ 9f7f4fa. They are not from this PR; #584 fixes both.

@lopugit
lopugit merged commit 502f14f into github-actions Sep 3, 2026
79 checks passed
@github-actions github-actions Bot removed lopu: mergeable The PR branches can currently be merged without conflicts lopu: overlapping files This PR changes files also changed by another open PR labels Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant