fix(actions): Lopu repairs failed PR checks - #580
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'... 488605 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 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 #580 ·
|
|
🌸 Lopu — second review pass on The fix in this PR is correct, and I found one thing to add to it. I've left The PR's own change holds up
What I added: the new soft-fail path is unretried and silentThe rewrite collapses every failure of that read into That's out of step with its own surroundings: the same step defines
The risk direction is coverage, not safety — a narrowed snapshot can only make Change (same lines this PR already rewrites): route the read through ReplayReal
The middle three rows confirm the PR's intended semantics are preserved exactly. Also: ✅ The decision this PR's description defers is already made — by #575The description asks the owner to choose between narrowing the I checked the stack actually composes: a real 3-way Checks on this head: 27 pass, 52 skipping, 0 failing. CodeQL open alerts on Correcting my earlier pass on this head (comment 5504305889), which concluded no change was needed. It was right on everything it examined — it enumerated the guard's four branches but treated "failed read" as one case, which is where the unretried-transient path slipped through. |
🌸 Lopu — the second-order reasoning here is what makes this correct, and it holdsMerge order. The contracts run in a non-blocking advisory job, so green checks here don't certify them. Two are already red at The bug. Why the fix is more than "make it soft". Making an empty outcome soft is only safe if a transport failure can't masquerade as "this PR has no merge ref" — otherwise a 502 silently drops every merge-ref CodeQL alert from the snapshot and from the disposition authority derived from it, indistinguishably from the benign case. This PR closes both halves:
Both are contract-pinned, and I'd single out that the contract asserts the absence of the old shape ( Interaction with #573 — checked, not assumed. #573 adds a guard forbidding a captured function from calling a helper that annotates on stdout. The new No changes made. Lopu · automated repository review · 0 open CodeQL alerts on |
🌸 Lopu — I replayed the fix instead of reading it, and it holdsFourth pass on The replayEarlier passes reasoned about this change; this pass executed it. I extracted the real
The blast radius is confirmed too: the candidate loop is One thing I considered and chose not to changeRetries exhausted on a 502 lands on the same New finding: the "secondary API-key slot" is inert for
|
|
🤖 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 — confirmed against this step's own output; no changes neededThe bug is real and the fix is right on all three axes. What I verified rather than assumed:
One live corroboration worth recording: this session's own Overlap checked: #580, #577 and #573 all edit The two failing advisories on this head fail identically on |
|
Lopu repository review — batch review of 16 open PRs. Good catch, and the fix is the right shape. Two things went wrong together in the old code: The error-vs-degrade line is drawn in the right place: a malformed SHA from a successful response is a real anomaly and still fails hard, while an absent ref degrades. Since Validation: full blocking FYI: the two contract advisories failing in this worktree ( No changes made. |
|
Lopu review — no defects found. Good catch, and the fix goes two steps further than the obvious one. The bug itself is the classic Three things make this the right fix rather than a narrower one:
Since Related: this is the same defect class #588 fixes one layer up in Validation: 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. Two real bugs in four lines, both caught. 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 controller fix — a missing PR merge ref aborts the whole Lopu review batch
Scope note
This was found while reviewing PR #579, whose own change is sound and needs no
edits. The defect below is in a different controller file
(
.github/workflows/resolve-pr-conflicts.yml) that PR #579 does not touch, soit does not belong on that PR's branch. No PR check is currently red because of
it — this is a latent race with a batch-wide blast radius, not a reproduction of
an observed failure.
Defect
In the
reviewjob, step "Prepare complete PR worktrees for one Lopu reviewsession" — the step that builds the review manifest every Lopu review session
consumes:
gh apiwrites GitHub's JSON error body to stdout on failure. Verifiedagainst the live API from this runner:
|| truediscards that non-zero exit and captures the error body.merge_shais then non-empty and not a 40-hex SHA, so the shape check fires
exit 1— andbecause this is a single
whileloop over all candidates in one step, onePR's 404 aborts worktree preparation for the entire batch, not just that PR.
Three things make this disproportionate:
merge_shais optional enrichment. Its only downstream use is wideningthe CodeQL alert filter, which already guards on
$merge_sha != "". Anempty value is a fully supported state; a hard abort is not the intended
response to "this PR has no merge ref".
gh pr listsnapshot (mergeable != CONFLICTING/UNKNOWN), but GitHub dropsrefs/pull/N/mergewhile it recomputes after the base branch advances. Theloop does two
fetch_refcalls per candidate before this read, so the windowbetween snapshot and read grows with batch size. This repository's base
branches advance constantly — during this review,
develophad already movedfrom the cached
f31864b2…to7b6418bd…, and merge refs for PRs docs: grow Thingtime's world-domination TODO garden #557 andfeat: rename Thingtime plugin and add login command #560 had been recomputed onto the new tip mid-flight.
codeql-analysis.ymlcarries thecomment "
gh apiwrites GitHub's JSON error body to stdout for a missingsynthetic merge ref. Do not capture that body through
|| true…" and usesthe exit-status-preserving form. This call site is the remaining copy of the
anti-pattern; it is the only
git/ref/pull/*/mergeread in the control planestill using
|| true.Change
.github/workflows/resolve-pr-conflicts.yml— keepgh's exit status, leavemerge_shaempty when the read fails, and reserve the hard error for asuccessful response carrying a malformed SHA. This is the same shape
codeql-analysis.ymlalready uses..github/scripts/resolve-pr-conflicts-routing-contract.mjs— pin the fixedbehaviour, per this repo's convention of asserting on controller text:
a
doesNotMatchforbidding the|| truecapture on the merge ref, and amatchrequiring the exit-status-preserving read.Genuine corruption still fails loudly; only the "no merge ref right now" case
becomes soft.
Validation
Run from
$GITHUB_WORKSPACE/trusted:yaml.safe_loadon all workflows + composite actionsnode --checkon all.github/scripts/*.mjsbash -non the extractedrun:block of the modified stepreviewBlockresolve-pr-conflicts-routing-contract.mjs --self-testBehavioural replay against a mock
ghreproducing the exact observedresponses:
::error::+ exit 1, whole batch abortsmerge_sha=""::error::+ exit 1::error::+ exit 1 (preserved)Pre-existing, unrelated advisory (reported, not changed)
resolve-pr-conflicts-routing-contract.mjs --self-testalready fails ongithub-actions@ac680d64before any edit of mine — confirmed by stashing mychanges and re-running. The failing assertion is at
resolve-pr-conflicts.yml:733: "every Lopu call receives the secondary API-keyslot". The step "Verify the credential waterfall with one live Claude turn"
uses
./.github/actions/lopu-agentwithoutanthropic-api-key-fallback.History shows this is a regression in the step, not an aspirational contract:
fab0538d(2026-08-25, "Lopu: fail over Claude credentials")51813961(2026-09-01, "fix(ci): verify Claude vaultcredentials live") — a week later, without satisfying the existing contract.
It is currently invisible because⚠️ advisory and does not
control-plane-ci.ymlruns every contractthrough
run_contract, which downgrades failures to afail the job — which is why control-plane CI reports success on
ac680d64.I did not change this, because the two resolutions have opposite security
implications and the choice is the owner's:
the Thingtime vault waterfall (it passes
thingtime-ci-router-secret);wiring in the GitHub-secret fallback would let a broken vault pass the probe.
This is my recommendation.
invariant is meant to be absolute.
Source Lopu workflow run