Skip to content

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

Merged
lopugit merged 2 commits into
github-actionsfrom
lopu/workflow-check-fix-33580883897
Sep 3, 2026
Merged

fix(actions): Lopu repairs failed PR checks#580
lopugit merged 2 commits into
github-actionsfrom
lopu/workflow-check-fix-33580883897

Conversation

@lopugit

@lopugit lopugit commented Sep 2, 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 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, so
it 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 review job, step "Prepare complete PR worktrees for one Lopu review
session"
— the step that builds the review manifest every Lopu review session
consumes:

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

gh api writes GitHub's JSON error body to stdout on failure. Verified
against the live API from this runner:

$ gh api repos/lopugit/thingtime/git/ref/pull/999999/merge --jq .object.sha 2>/dev/null
exit=1
stdout={"message":"Not Found","documentation_url":"...","status":"404"}

|| true discards that non-zero exit and captures the error body. merge_sha
is then non-empty and not a 40-hex SHA, so the shape check fires exit 1 — and
because this is a single while loop over all candidates in one step, one
PR's 404 aborts worktree preparation for the entire batch, not just that PR.

Three things make this disproportionate:

  1. merge_sha is optional enrichment. Its only downstream use is widening
    the CodeQL alert filter, which already guards on $merge_sha != "". An
    empty value is a fully supported state; a hard abort is not the intended
    response to "this PR has no merge ref".
  2. The 404 is a normal race, not corruption. Candidates are filtered on a
    gh pr list snapshot (mergeable != CONFLICTING/UNKNOWN), but GitHub drops
    refs/pull/N/merge while it recomputes after the base branch advances. The
    loop does two fetch_ref calls per candidate before this read, so the window
    between snapshot and read grows with batch size. This repository's base
    branches advance constantly — during this review, develop had already moved
    from the cached f31864b2… to 7b6418bd…, and merge refs for PRs docs: grow Thingtime's world-domination TODO garden #557 and
    feat: rename Thingtime plugin and add login command #560 had been recomputed onto the new tip mid-flight.
  3. This repo already documents the hazard. codeql-analysis.yml carries the
    comment "gh api writes GitHub's JSON error body to stdout for a missing
    synthetic merge ref. Do not capture that body through || true…" and uses
    the exit-status-preserving form. This call site is the remaining copy of the
    anti-pattern; it is the only git/ref/pull/*/merge read in the control plane
    still using || true.

Change

.github/workflows/resolve-pr-conflicts.yml — keep gh's exit status, leave
merge_sha empty when the read fails, and reserve the hard error for a
successful response carrying a malformed SHA. This is the same shape
codeql-analysis.yml already uses.

.github/scripts/resolve-pr-conflicts-routing-contract.mjs — pin the fixed
behaviour, per this repo's convention of asserting on controller text:
a doesNotMatch forbidding the || true capture on the merge ref, and a
match requiring 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:

Check Result
yaml.safe_load on all workflows + composite actions pass
node --check on all .github/scripts/*.mjs pass
bash -n on the extracted run: block of the modified step pass (exit 0)
Both new contract assertions evaluated against the extracted reviewBlock both PASS
resolve-pr-conflicts-routing-contract.mjs --self-test fails on a pre-existing unrelated assertion (see below)

Behavioural replay against a mock gh reproducing the exact observed
responses:

Case Before After
404 (real error body, exit 1) ::error:: + exit 1, whole batch aborts continues, merge_sha=""
200 with a valid merge SHA continues, SHA captured identical
200 with a malformed SHA ::error:: + exit 1 ::error:: + exit 1 (preserved)

Pre-existing, unrelated advisory (reported, not changed)

resolve-pr-conflicts-routing-contract.mjs --self-test already fails on
github-actions@ac680d64 before any edit of mine — confirmed by stashing my
changes and re-running. The failing assertion is at
resolve-pr-conflicts.yml:733: "every Lopu call receives the secondary API-key
slot"
. The step "Verify the credential waterfall with one live Claude turn"
uses ./.github/actions/lopu-agent without anthropic-api-key-fallback.

History shows this is a regression in the step, not an aspirational contract:

  • assertion added fab0538d (2026-08-25, "Lopu: fail over Claude credentials")
  • probe step added 51813961 (2026-09-01, "fix(ci): verify Claude vault
    credentials live") — a week later, without satisfying the existing contract.

It is currently invisible because control-plane-ci.yml runs every contract
through run_contract, which downgrades failures to a ⚠️ advisory and does not
fail 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:

  • Narrow the contract to exempt this probe. That job's purpose is to verify
    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.
  • Pass the fallback at the call site, if the contract's "every Lopu call"
    invariant is meant to be absolute.

Source Lopu workflow run

@github-actions

github-actions Bot commented Sep 2, 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'... 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
- ✅ 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 2, 2026
@github-actions

github-actions Bot commented Sep 2, 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 #580 · lopu/workflow-check-fix-33580883897github-actions

Compared 2ee33365 against github-actions @ 9f7f4fa3.

Check state

gh pr checks 580: 27 pass / 52 skipping / 0 fail, MERGEABLE / CLEAN. No failure to
diagnose. CodeQL snapshot for this head is empty; 580.json stays [].

The two red advisory contracts on this tree are pre-existing on the base — I confirmed by
running them against 9f7f4fa3 directly. #584 fixes them.

What the PR does

In the Lopu-review candidate-preparation step, the merge-ref read was

merge_sha="$(gh api "repos/$REPO/git/ref/pull/$number/merge" --jq '.object.sha' 2>/dev/null || true)"

gh api writes GitHub's JSON error body to stdout, so || true captured that body, the
^[0-9a-f]{40}$ shape check then failed, and exit 1 took down the whole review batch — for one
PR whose merge ref GitHub happened to be recomputing. The PR keeps gh's exit status, routes the
read through gh_read_retry, leaves merge_sha empty on a failed read, and reserves the hard
error for a successful response carrying a malformed SHA.

Findings

No defects. What I checked:

  1. Right helper, right call shape. This step's gh_read_retry copy (defined ~90 lines above at
    the lopu-review-prepare-gh temp prefix) wraps gh "$@", so gh_read_retry api "repos/…" is
    correct. The sibling copy at line ~1424 wraps gh api "$@" and is called without the api verb —
    the two spellings are not a mistake, they match their respective helpers.
  2. Failing soft is safe here. merge_sha only widens the CodeQL alert filter downstream and is
    optional there, so an empty value narrows the snapshot rather than corrupting it.
  3. …and the narrowing is announced. The ::notice:: on the empty branch is what stops a silent
    degradation, and pairing it with gh_read_retry means a 502 or transport reset is replayed rather
    than being indistinguishable from a PR that genuinely has no merge ref. Both properties are pinned
    by the three new contract assertions, including a doesNotMatch on the old || true shape.
  4. The invalid-SHA path still fails hard, which is the correct direction: a 200 response with a
    non-SHA body is a real contract violation, not a race.

Cross-PR interaction

Merges cleanly with #584, #577 and #573 (all verified in a scratch clone), and lands clean in the
whole-batch simulation. No interaction with #588/#579, which touch codeql-analysis.yml rather than
resolve-pr-conflicts.yml.

Worth noting the family resemblance: #580, #577 and #573 are three different repairs of the same
class of bug
— a gh read whose failure mode reached the wrong stream or the wrong severity. They
are complementary, not redundant, and all three land clean together.

Changes made

None.

Validation run

  • gh pr checks 580 / gh pr view 580.
  • workflow-control-plane-contract.mjs --self-test → OK on this head.
  • resolve-pr-conflicts.yml YAML parse + bash -n on all 61 run: blocks → 0 syntax failures.
  • Confirmed the two red advisories are pre-existing on 9f7f4fa3.
  • Scratch-clone pairwise merges #580+#577, #580+#573, #584+#580 → all clean; whole-batch
    simulation → clean.

View Lopu workflow run

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🌸 Lopu — second review pass on a76a7887 (dispatch 33586132102)

The fix in this PR is correct, and I found one thing to add to it. I've left
the change in the worktree for the publisher.

The PR's own change holds up

gh api writes GitHub's JSON error body to stdout, || true discarded the
exit status and captured that body, and the ^[0-9a-f]{40}$ check then exit 1'd
the whole batch on one PR's normal 404. Keeping gh's exit status is the right
repair, and I confirmed merge_sha really is optional downstream — the alert
filter guards on $merge_sha != "", and the only other consumer
(expected_analysis_sha, line 5064) is unreachable for a merge-ref alert when
merge_sha is empty, because such an alert can never enter the authority
snapshot in the first place.

What I added: the new soft-fail path is unretried and silent

The rewrite collapses every failure of that read into merge_sha="" — not
just the 404 it's about. A retryable 502 or an http2 GOAWAY now looks exactly
like a PR that genuinely has no merge ref, with no retry and no log line. Every
merge-ref CodeQL alert then drops out of that PR's snapshot, and out of the
disposition authority derived from it, for that pass.

That's out of step with its own surroundings: the same step defines
gh_read_retry() for precisely this class of read, and the routing contract
already pins that this block "routes both initial metadata reads through the
retry helper"
(resolve-pr-conflicts-routing-contract.mjs:687). The merge-ref
read is the third idempotent metadata read in that loop and was the only one
bypassing it — while #577 is concurrently hardening that same helper because an
unretried read of this class took the repository-wide scan down in run
33565995120.

codeql-analysis.yml isn't a counter-precedent, by the way: it has no
gh_read_retry in scope (0 occurrences), so plain gh api is correct there.

The risk direction is coverage, not safety — a narrowed snapshot can only make
Lopu miss an alert, never dismiss one it can't see, since the authority
binding rejects out-of-snapshot proposals. Still worth closing.

Change (same lines this PR already rewrites): route the read through
gh_read_retry, drop the now-harmful 2>/dev/null (the helper already keeps
diagnostics off stdout, and suppressing stderr would hide its retry notices),
and ::notice:: when merge_sha ends up empty. Contract: repoint the new
match from gh api to gh_read_retry api, and pin the announcement. The
doesNotMatch on || true is untouched.

Replay

Real gh_read_retry + the real merge-ref block, extracted verbatim from the
YAML, against a mock gh reproducing GitHub's actual shapes (error body on
stdout, gh: line on stderr, exit 1):

Mock response base ac680d64 #580 as submitted with this fix
404 (real error body) ::error:: + exit 1, batch aborts merge_sha="", silent merge_sha="", notice logged
200, valid SHA captured captured captured
200, malformed SHA ::error:: + exit 1 ::error:: + exit 1 ::error:: + exit 1
502 → 200 ::error:: + exit 1 merge_sha="", 1 attempt, silent retried, SHA recovered (3 attempts)
GOAWAY (persistent) merge_sha="", silent merge_sha="", silent 4 attempts, then notice logged

The middle three rows confirm the PR's intended semantics are preserved exactly.

Also: yaml.safe_load over all 17 workflows/actions ✅ · node --check over all
23 scripts ✅ · bash -n on the extracted 255-line run: block ✅ ·
workflow-control-plane-contract --self-test ✅ ·
resolve-pr-conflicts-routing-contract --self-test passes in full once the one
pre-existing base assertion is skipped.

✅ The decision this PR's description defers is already made — by #575

The description asks the owner to choose between narrowing the
anthropic-api-key-fallback contract or wiring the fallback into the credential
probe. #575 already implements the option this PR recommends: it exempts the
probe keyed on the verify_credential_vault job boundary (not a spoofable
id: live_probe), and pins thingtime-ci-router-secret + the absence of every
GitHub-secret slot as the property that earns the exemption. It fixes the
promotion-worker advisory too. No owner decision is outstanding — landing
#575 clears both reds.

I checked the stack actually composes: a real 3-way git merge-file of this
contract file against #575's head 66c60b12 over their shared ancestor
ac680d64 merges cleanly, 0 conflicts, and in that composed tree
resolve-pr-conflicts-routing-contract --self-test and
promotion-worker-routing-contract both pass.

Checks on this head: 27 pass, 52 skipping, 0 failing. CodeQL open alerts on
a76a7887: 0 — dispositions left [], nothing dismissed.

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.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🌸 Lopu — the second-order reasoning here is what makes this correct, and it holds

Merge order. The contracts run in a non-blocking advisory job, so green checks here don't certify them. Two are already red at github-actions tip 9f7f4fa3, and #575 is the only open PR that fixes either. Land it first. This PR merges cleanly regardless, and the full seven-PR combined merge is conflict-free with all fourteen contracts passing.

The bug. gh api writes GitHub's JSON error body to stdout, so || true captured that body when the synthetic merge ref was missing, and the ^[0-9a-f]{40}$ check then aborted the whole review batch. Since GitHub drops refs/pull/N/merge while recomputing after the base advances, that 404 is a routine race on a candidate the snapshot correctly saw as mergeable. Nasty, and the diagnosis is exact.

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:

  1. routing through gh_read_retry, which replays a 5xx or transport reset and never puts a failing response body on stdout at all;
  2. ::notice::PR #N has no readable merge ref — so a narrowed snapshot is legible in the log rather than an invisible degradation.

Both are contract-pinned, and I'd single out that the contract asserts the absence of the old shape (assert.doesNotMatch(… /merge"[^\n]*\|\| true/)) alongside the presence of the new one. Pinning the bug's shape, not just the fix's, is what stops a future rewrite from reintroducing it in a differently-spelled form.

Interaction with #573 — checked, not assumed. #573 adds a guard forbidding a captured function from calling a helper that annotates on stdout. The new ::notice:: here is emitted at step scope rather than inside a captured function, so it isn't a violation — and I confirmed empirically that with both merged, resolve-pr-conflicts-routing-contract.mjs --self-test passes.

No changes made.

Lopu · automated repository review · 0 open CodeQL alerts on 2ee33365

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

🌸 Lopu — I replayed the fix instead of reading it, and it holds

Fourth pass on 2ee33365 (dispatch 33591845011). No changes made, and no check is failing.
0 open CodeQL alerts on this head. Two things below are new to this PR's thread.

The replay

Earlier passes reasoned about this change; this pass executed it. I extracted the real
gh_read_retry definition and the real merge-ref block out of the step's run: string via
yaml.safe_load, and drove both the old and new shapes against a mock gh reproducing GitHub's
actual responses:

Case Old New
404 (JSON error body on stdout, exit 1) ::error:: + exit 1, batch aborts merge_sha="", ::notice::, exit 0
200, valid SHA captured identical
200, malformed SHA ::error:: + exit 1 ::error:: + exit 1 — preserved
502 ×2 then 200 ::error:: + exit 1 retries, captures the SHA
502 ×4, retries exhausted ::error:: + exit 1 merge_sha="", ::notice::, exit 0

The blast radius is confirmed too: the candidate loop is done < <(…), so the body runs in the
current shell and that exit 1 really did take down worktree prep for every remaining PR, not
just the one that 404'd. I also re-derived the reviewBlock slice the way the contract does and
evaluated the three new assertions against it — 3/3 pass, and they sit at contract lines
1210–1231, before the assertWorkflowSource() throw at 1605, so they genuinely run today.

One thing I considered and chose not to change

Retries exhausted on a 502 lands on the same ::notice:: as a benign 404, while the sibling
CodeQL-alert read 20 lines down uses ::warning:: for a failed read. I left it alone:
gh_read_retry deliberately hides the HTTP status from its caller, so splitting the two means
either editing that shared helper (whose other two callers already fail hard) or issuing a second
status-bearing request — and raising the single branch to ::warning:: would fire on the common,
expected path instead. "No readable merge ref" is honest about both. Good trade-off as written.

New finding: the "secondary API-key slot" is inert for lopu-agent

This PR's description — and #575's exemption comment — justify exempting the vault probe with
"handing it GitHub-secret fallbacks would let a broken vault pass a green probe." That is right
for the token slots. For the API-key slot, which is the one the failing assertion actually
names, it doesn't hold, because the input goes nowhere:

  • .github/actions/lopu-agent/action.yml declares anthropic-api-key (L12) and
    anthropic-api-key-fallback (L21) and never references either in runs:.
  • Four of the five lopu-agent call sites pass real secrets into them anyway
    (resolve-pr-conflicts.yml 3908/3911, 4666/4669, 6592/6595, 8021/8024), and
    rebase-conflict-round/action.yml:320,323 forwards them into the same dead inputs.
  • No workflow- or job-level env: supplies ANTHROPIC_API_KEY to those steps (checked by parsing
    the workflow, not grepping), so the composite can't pick it up ambiently.
  • lopu-credential-vault.mjs reads only LOPU_LEGACY_CLAUDE_TOKEN_{PREFERRED,PRIMARY,FALLBACK}
    and throws on any row whose credentialType !== 'claude-code-oauth-token'.

So the waterfall is OAuth-token-only by construction, and that contract line pins wiring with no
effect. This does not undermine #575 — its exemption also covers the claude-code-oauth-token*
slots, which are live, so the fix is right. It does mean the follow-up question is a product call
for @lopugit: drop the dead inputs and the assertion, or actually implement API-key auth in the
vault. Not something I'd put on this branch.

Merge ordering

Both red advisories are pre-existing and identical at base tip 9f7f4fa3 — I ran all eleven
contracts at both revisions (base extracted with git archive, no repo state touched): 9 pass, the
same 2 fail. This PR causes neither. #575 fixes both; land it first if you want the advisory green,
though nothing here depends on it.

Lopu · automated repository review · Claude Opus 5 · validated: 17 workflows/actions YAML-parse, node --check all scripts, bash -n on the modified step, 5-case behavioural replay, 11-contract suite at two revisions

@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 — confirmed against this step's own output; no changes needed

The bug is real and the fix is right on all three axes. gh api writes GitHub's JSON error body to stdout on a 404, || true captured it, and the shape check then exit 1'd — aborting the whole review batch, not just that candidate. I checked the surrounding step: there's no per-candidate error trap, so the blast radius really was the entire batch.

What I verified rather than assumed:

  • gh_read_retry is in scope — defined at the top of this same run: block, so the call resolves.
  • A 404 body can never reach stdout through it. The helper redirects to $output/$errors files and only cats $output on a successful gh invocation. That's the property the whole fix depends on, so it was worth reading rather than trusting.
  • The hard error is narrowed correctly::error:: + exit 1 now fires only for a successful response carrying a malformed SHA, which is a genuine invariant violation. A missing ref degrades to a ::notice:: naming the consequence, which is the right severity given merge_sha only widens the downstream CodeQL alert filter and is optional there.
  • Adding the retry alongside the soft failure is the correct pairing. A soft-failing read without retry would be a quiet regression — a 5xx blip would silently narrow the alert snapshot instead of aborting loudly.

One live corroboration worth recording: this session's own lopu-review-manifest.json was produced by this exact step, and all 13 records carry a merge_sha. So the soft path wasn't exercised today — this is pre-emptive for the race, not a response to a currently-broken read. Which is the right time to fix it.

Overlap checked: #580, #577 and #573 all edit resolve-pr-conflicts.yml in different regions. Verified #584 → #573 → #577 → #580 → #579 → #574 → #565 merges cleanly in sequence with all 15 control-plane contracts green on the combined tree, so no coordination is needed between them.

The two failing advisories on this head fail identically on github-actions@9f7f4fa3 — inherited; #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.

Good catch, and the fix is the right shape. Two things went wrong together in the old code: gh api writes GitHub's JSON error body to stdout for a missing ref, so || true captured that body into merge_sha and the shape check then exit 1'd the whole review batch — and the 404 itself is normal, since GitHub drops refs/pull/N/merge while recomputing after the base advances. A candidate that was mergeable at gh pr list time can legitimately 404 moments later, so a routine race took down every PR in the run.

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 merge_sha only widens the CodeQL alert filter downstream and is optional there, that's correct. Routing through gh_read_retry matters for the same reason the soft path does — a 5xx would otherwise silently narrow the alert snapshot instead of being retried — and the ::notice:: keeps the narrowed snapshot visible rather than turning this into an invisible degradation.

Validation: full blocking control-plane-ci.yml verify suite passes in this worktree. Merges cleanly with all eight sibling controller PRs (pairwise git merge-tree) — it touches a different region of resolve-pr-conflicts.yml than #577 and #573, so the three are independent despite sharing the file.

FYI: the two contract advisories failing in this worktree (resolve-pr-conflicts-routing, promotion-worker-routing) fail identically on the unmodified base at 9f7f4fa3. Pre-existing, not yours — #584 fixes both.

No changes made.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Lopu review — no defects found. Good catch, and the fix goes two steps further than the obvious one.

The bug itself is the classic gh api trap: it writes GitHub's JSON error body to stdout, so || true captured that body into merge_sha, the 40-hex shape check then failed, and exit 1 took down the whole review batch — over a 404 that's an ordinary race, since GitHub drops refs/pull/N/merge while it recomputes after a base advance.

Three things make this the right fix rather than a narrower one:

  • The hard error is correctly kept for a successful response carrying a malformed SHA. Softening only the failed-read path preserves the signal that something is genuinely wrong, rather than blanket-swallowing.
  • Routing the read through gh_read_retry is the non-obvious part and you got it right. Once the empty outcome became soft, an unretried read would degrade silently — a 502 or transport reset would drop every merge-ref CodeQL alert from the snapshot and from the disposition authority derived from it, indistinguishably from a PR that genuinely has no merge ref. That's a worse failure than the one being fixed, and it's easy to miss.
  • The narrowed snapshot is announced. The ::notice:: makes the degradation visible in the log instead of silent, and the contract pins the notice rather than just the retry.

Since merge_sha only widens the alert filter downstream and is optional there, degrading to head-ref alerts is a sound fallback rather than a correctness loss.

Related: this is the same defect class #588 fixes one layer up in codeql-analysis.yml (gh api … || true capturing an error body for a missing synthetic merge ref). Complementary, different files, no conflict.

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.

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. Two real bugs in four lines, both caught. gh api writes GitHub's JSON error body to stdout, so || true captured that body into merge_sha and the shape check aborted the whole review batch — on a routine race, since GitHub drops refs/pull/N/merge while recomputing after a base advance. And making the empty outcome soft would then have hidden a genuine 502 behind "this PR has no merge ref". Routing through gh_read_retry and reserving the hard error for a successful response with a malformed SHA gets both. I verified in-tree that gh_read_retry is defined earlier in the same run: block, returns 1 on final failure, and keeps stdout clean — so the call site's semantics are what the comment claims. This is the step that produced the manifest for this very review session.

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 21252a7 into github-actions Sep 3, 2026
79 checks passed
@github-actions github-actions Bot removed lopu: overlapping files This PR changes files also changed by another open PR lopu: mergeable The PR branches can currently be merged without conflicts 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