fix(actions): Lopu repairs failed PR checks - #577
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'... 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
|
|
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:
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 safePairing the message with the status instead of retrying bare 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 Mutation tests confirm the contract fails closed:
Other validation: Coordination noteThis 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 Verdict: approve. No changes needed here. Suggested order: #575 first (it clears the standing advisories for everyone), then this and #573 together. |
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 #577 ·
|
|
Lopu — repository review (second pass) Re-reviewed Two negatives the validation table doesn't listI extracted all three
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 textuallyThree PRs editing the same two files makes "merges cleanly" a weak claim, so I rebuilt
Cost, quantified
One thing to reconcile — not a defect here, and I did not touch it
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. |
🤖 Lopu review — approve; predicate validated against gh's literal outputI didn't want to eyeball the regex, so I ran the predicate against the real error text:
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 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 Sequencing noteThe self-test stops at 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. |
🌸 Lopu — good fix, correctly scoped. Two notes on ordering and on the retry budget.Merge order. The control-plane contracts run in 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 One observation, not a change request. The retry budget is 4 attempts with Related: the diff that triggered this ( No changes made. Lopu · automated repository review · 0 open CodeQL alerts on |
|
🤖 Lopu review — this PR's own changes look correct; one pre-existing contract failure to flag.
This PR is not the cause. I ran the identical contract against the base The contract runs in the non-blocking I cross-tested #575's contract file against this PR's workflow in a scratch Recommendation: land #575 first, then this.
No changes requested — full notes in the review report. |
🌸 Lopu — this and #573 are the two halves of one fix; ship them togetherNo 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 I corroborated the cited case independently. The failing read is Applied consistently — all three 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 |
|
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 Evidence is solid too: run Applying it to all three 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 FYI: the two contract advisories failing here fail identically on the unmodified base at No changes made. |
|
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:
The cited evidence lines up — run 33565995120 losing Validation: #573, #577 and #580 all edit Recommended batch order for the nine Posted by Lopu, Thingtime's PR manager. |
Lopu · reading this diff, and where it sits in the controller queueTwo-dot diff artifact. GitHub's "Files changed" view shows git diff $(git merge-base github-actions HEAD)...HEADSame 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 Merge order. Nine Pre-existing advisory failures. |
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.mjsThe failure
control-plane / Find merge-conflicting PRs(run 33565995120, job100049231200, step Scan open PRs and assign deterministic resolver
ownership) ended:
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 itsconflict scan whenever it fires.
Diagnosis
complete_large_pr_files()tops up the shared open-PR inventory for PRs withmore than the 100 changed paths GraphQL returns inline. Today exactly one open
PR qualifies: #291, 2896 changed files.
GitHub answered
repos/lopugit/thingtime/pulls/291/fileswith 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 ofthem, 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:
HTTP 422— "this diff is taking too long to generate"--paginate --slurpSo 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 warnsand
continues. It could not: the warning was echoed on stdout, and thisfunction's stdout is the returned inventory document
(
all_open="$(complete_large_pr_files "$all_open")"). The annotation wasspliced in as line 1, and the next
jqover$all_open— insidequery()—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_jobsand adds anassertCapturedStdoutStaysClean()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 exactgeneration-timeout message. All three copies in the file are updated, matching
the existing per-copy discipline for
transportand the decode message.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.
ghprintsboth 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 abounded 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_retrycopies was extracted fromthe YAML and run against a stubbed
gh.Validation Failed (HTTP 422)Not Found (HTTP 404)stdout carried only the payload in every case.
Contract assertions confirmed load-bearing by reverting pieces of the fix:
slow_diff=but drops the branchHTTP 422toHTTP 4[0-9][0-9]Blocking steps of
control-plane-ci.yml, all run against the edited tree:node --checksweep over.github/scripts/**/*.mjsbash -nsweep over.github/scripts/**/*.shgit diff --checkyaml.safe_load, 26 jobs)bash -non each extractedgh_read_retrycopynode --test graphify-cas.test.mjsstage-graphify-snapshots.mjs --self-testnode --test rebase-index-fingerprint.test.mjsnode --test rebase-related-edits.test.mjsnode --test resolve-canonical-instruction-type-conflicts.test.mjsnode --test lopu-pr-status.test.mjsbuild-all-branch.mjs --self-testmerge-main-develop-sync-pr.mjs --self-testclassify-claude-credential-failure.mjs --self-testelectron-pr-release-contract.mjsworkflow-control-plane-contract.mjs --self-testpromotion-pr-changelog.mjs --self-testpromote-features-to-main.mjs --self-testPre-existing failures, not caused by this change
Two advisory (non-blocking) contracts already fail on
github-actionsandfail identically on a pristine
HEADcheckout of the same tree. Open PR #575targets both:
resolve-pr-conflicts-routing-contract.mjs --self-test—"
resolve-pr-conflicts.yml:733: every Lopu call receives the secondaryAPI-key slot" (and the adjacent subscription-slot assertion).
promotion-worker-routing-contract.mjs— "Feature Stack workers still runwhen 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