fix(actions): Lopu controller repair β control-plane PRs are authorized into a product build they cannot possibly run - #625
Conversation
β Develop S3 preview failed
The ordinary generated Vercel Preview remains available on the shared development runtime. Re-run this workflow after correcting the deployment, DNS, or CORS configuration. Generic Vercel Preview deployments use the shared development runtime; this controller adds the stable exact-SHA alias and marker-scoped cleanup. |
|
Lopu review β independently reproduced this diagnosis, and I'm deferring to this fix over my own. I was woken by the β develop-preview comment on #624 and reached the same root cause before finding this PR: the Confirming the scale independently: every control-plane preview worker today failed this way β Why I dropped my version for this oneI had written a
Keeping it as an Verified independently: fetched Heads-up on this PR's own red checkThe same One loose end worth a decision, not changed here: neither fix retracts the stale β preview comments already sitting on #624 and #625. |
|
Lopu review β the red check was a GitHub outage, not this diff. Re-ran it; PR #625 is green and Following up on my earlier comment on this PR: the
|
| Run | Event / branch | Result |
|---|---|---|
33909379288 att.1 |
push / this head |
502 β 19:06β19:07 |
33909384333 |
pull_request_target / identical head SHA |
β 51s, 19:06β19:07 |
33910581652 |
schedule / main |
502 β 19:24β19:25 |
33911056736 |
push / another branch |
β 19:25 |
Re-ran the failed job. Attempt 2 passed in 30s; every check on this PR is now green and mergeStateStatus is CLEAN.
On the diff itself
I re-derived the load-bearing claims from live state rather than re-reading the description, and they hold:
GET /contents/remix/package.jsonβdevelopβfile,mainβfile,github-actionsβ 404, head2af16fcfβ 404. The repo-relative path is right.- Both call sites are genuinely required: the
controllerjob'sReconcile or clean up without a new buildstep is gatedif: needs.prepare.outputs.should_build != 'true'and re-entersmain()withoutVERCEL_PREBUILT_DIRin its env, so aprepareBuildPlan()-only gate would reachdeploy()with no bundle. - The ineligible routing does not start commenting on control-plane PRs:
opened/synchronizeon basegithub-actionsmisses everycleanupRelevantarm and is log-only; the paths that do reachhandleIneligibleonly comment whenaliasRemoved || deleted > 0, which is never true for a PR that never built.cleanupComment()also has a safe fallback branch, so the new reason string can't produce a broken message where it is reachable. - The probe runs after
assertTrustedPullRequestStackat both sites, so a fork head SHA is never probed against the base repo, andheadShais shape-validated before it reaches the URL.
Self-test 138/138 (120/120 at base), node --check clean, control-plane contract + routing contract + node --test 10/10 all pass from the worktree. No changes needed; I made none. CodeQL snapshot for this head is empty.
One correction to my earlier note
I described the stale β preview comments as sitting on "#624 and #625". It is wider than that β #617, #619, #621, #623, #624 and this PR all still carry ### β Develop S3 preview failed telling the operator to correct deployment, DNS, or CORS. The part worth your decision: after this fix those PRs take the log-only "Skipped unrelated PR" path, so nothing will ever update those comments again β they are now permanent unless retraction is added deliberately. Still your call on comment ownership; I have not touched it.
Also worth a separate change, not this one: deploy-admin-pr-previews.mjs has the same unguarded shape β admin_build checks out into product/ and runs pnpm --dir product/remix install with no bundle probe. Much smaller blast radius, since it only fires on an explicit admin dispatch naming a PR.
β Lopu, Thingtime's PR and repository manager. Using Claude Opus 5.
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 #625 Β· preview eligibility requires a buildable head
What I compared
FindingsThe root cause is real and the probe is the right shape. The build job checks The failure mode is correctly non-silent. Both call sites are needed. The ineligible path is a clean skip, not a red comment. For a control-plane No regression for the merged-into-develop path. When One thing worth knowing (not a defect)
Minor cosmetic note, deliberately not changed: the block of four Interaction with the sibling controller PRsDisjoint from #624 and #626 (see #626's report for the file-level breakdown). Changes madeNone. The implementation is correct and already carries its own coverage. Validation runRecommendationApprove and merge. |
|
Lopu β independent corroboration from the #624 review I diagnosed this same defect from the other end while reviewing #624 (that session was woken by the failing-preview comment it produces), reached the same root cause, and deliberately made no controller edit because this PR already fixes it in the right place. Recording the evidence here so it's attached to the fix. Four independent reproductions, all
So it reproduces on every control-plane head, including this branch's own β the worker runs the controller from the default branch, so the fix isn't live until it merges. Worth expecting this PR to stay red on the preview until then; that's the defect, not a regression in the change. Two premise checks I ran independently, both of which hold:
Two details I liked, since they're the ones that usually get missed: raising on a non-404 One knock-on worth knowing: until this lands, every failing preview posts a π€ Posted by Lopu, Thingtime's automated PR reviewer. |
|
Lopu review β answering the β above: that comment is the bug this PR fixes, posted by the unfixed controller onto this PR's own head. Approving; no changes made. I was woken by the Why this PR's β cannot clear before it merges
That last line is worth pausing on: it is empirical proof of the Regression sweep β 23/23 open heads, zero false negativesThe dangerous direction for this change is the inverse of the bug: a probe that's too strict silently disables previews repo-wide, which is what your negative control 1 caught pre-ship. I extracted
Heads disagreeing with base-shape expectation: 0. All seven The blast radius is tighter than the description claims
One behavioural delta nobody has flaggedControl-plane PR events currently reach ValidationRan the full Two small things
No defect found and nothing worth changing, so I made no edits β the worktree is clean and there's no commit to push. π€ Lopu, Thingtime's PR and repository manager. Using Claude Opus 5. |
|
Lopu β cross-PR note from the #626 review (no action needed on this PR). While reviewing #626 I reproduced the failure this PR fixes, and confirmed independently that your fix is the right one. Two things worth recording here. 1. This PR is the single blocker for the β preview comment on all three open control-plane PRs. Each carries the identical comment, one per head: #624 ( 2. This PR cannot clear its own β. The Validated independently against head This PR and #626 touch disjoint files, so they can land in either order β but landing this one first clears the red comment on the other two immediately. |
β Lopu β reviewed and validated; no changes neededCompared the full head ( I checked the integration, which is where a gate like this usually goes wrong:
Two cosmetic notes, no behaviour impact: one extra Checks: 27 pass / 0 fail. Ready to merge. Lopu Β· automated repository review |
π€ Lopu detected an out-of-date PR branchStatus: Work detected β Lopu is taking ownership. Current phase: Entering Lopu's serialized PR-resolution queue. Estimated completion: around 06:28 UTC (~20 minutes; this adjusts as the queue moves). Next automatic check-in: within 10 minutes while work remains active. You can stay on this PR; there is no need to find the Actions run. Time conversion (UTC source)
Los Angeles and Melbourne use their real IANA time zones, so PDT/PST and AEST/AEDT offsets change automatically. Lopu queue and PR pulse
Related PR context
Exact branch pair: Timeline
|
π΅οΈ Lopu β verified the two claims that carry this fixRoot cause confirmed: the build job checks the head out into Two claims in the diff are the ones the fix rests on, so I checked them rather than reading them: 1. The ineligible path is a clean skip, not another red comment. For a control-plane PR on a non-cleanup action, 2. Both call sites are needed. The failure-mode polarity is also right, and it's the detail that would have been easy to get wrong: One trade worth stating out loud since it's a deliberate design choice: this is a content probe rather than a base-ref filter, so it costs one extra contents call per preview event. Negligible against the token exchange and build in the same run, and it buys correctness for a stacked PR whose base is neither Merge-order note for all three controller repairs is on #626 β disjoint files, any order works. No changes made to this branch. |
Lopu controller check repair
Lopu identified a failed PR check whose root cause is in the protected controller/workflow code.
Lopu controller repair β control-plane PRs are authorized into a product build they cannot possibly run
Files changed (in
$GITHUB_WORKSPACE/trustedonly):.github/scripts/deploy-develop-pr-preview.mjsβ preview eligibility now alsorequires that the exact PR head actually carries the bundle the build job
installs, plus self-test coverage for the new probe.
No product branch was touched, and this is not a defect in PR #624's diff.
PR #624's own branch carries an unrelated fix (its conversation gate); mixing an
unrelated controller change onto it would have been scope creep.
Diagnosis
PR #624 shows a red develop-preview status comment:
That remediation text is wrong: nothing is wrong with the deployment, DNS, or
CORS. The build never got far enough to touch any of them.
gh pr checks 624is entirely green β the preview is reported only as a comment,so it is invisible to the check surface and easy to mistake for a real
infrastructure fault.
Root cause. The
pull_request_targethandoff (33907601208) succeeded anddispatched the privileged worker (
33907615031,head_branch=main), whoseBuild exact PR bundle without secretsjob failed:The build job checks the PR head out into the
product/directory and thenruns
corepack pnpm --dir product/remix install --frozen-lockfile. PR #624 is acontrol-plane PR: base
github-actions, whose tree is only.github, the rootdocs,
PRs/,graphify-out/, andvercel.json. There is noremix/, soproduct/remixcannot exist and the install always fails.assertTrustedPullRequestStackis the only gate in front of the build, and itanswers a different question. Its own comment says so:
That is correct about trust and silent about buildability. A trusted
author with a head that has no product tree passes it.
Scale. Every open/synchronize on a control-plane PR pays a failed 45-minute-
timeout build job plus a privileged publish job, and posts a false β. Every PR
based on
github-actionsis affected, and none based ondevelopis:remix/package.jsonat headgithub-actionsdevelop/ featureThat is 11 consecutive control-plane PRs (#614β#624), each with a red preview
comment that instructs the operator to fix infrastructure that is not broken.
Fix
Buildability is now asserted next to trust, at the two points that authorize
work, as a head-content probe:
Called from
prepareBuildPlan()and frommain(), inside thetryblocks thatalready classify
EligibilityError. Both call sites are required:prepareBuildPlan()alone would setshould_build=falseand skip the build βbut the controller's
Reconcile or clean up without a new buildstep re-entersmain(), which would fall straight through todeploy()with no prebuiltbundle and fail again.
synchronize/openedlogSkipped unrelated PR #N: head-has-no-preview-bundleand post nothing;
edited/closedreachhandleIneligible, which onlycomments when it actually removed an alias or deployment β never the case here.
Deliberate choices:
default branch explicitly refuses to filter by base ref ("Do not filter by base
branch here: edited/closed events must still remove resources after a PR is
retargeted away from develop"), and a retargeted PR must still reach cleanup.
a transport fault or a permissions regression cannot be silently misread as an
empty head and quietly disable previews repository-wide.
assertTrustedPullRequestStackis untouched, soassertCurrentPullRequest,deploy(), and the scheduledreconcile()sweep keep their existingsemantics.
product/is the checkout directory, notpart of the repository layout β see the negative result below.
Validation
deploy-develop-pr-preview.mjs --self-testworkflow-control-plane-contract.mjs --self-testresolve-pr-conflicts-routing-contract.mjs --self-testnode --test resolve-pr-conflicts-routing-contract.test.mjspromotion-worker-routing-contract.mjsrebase-ownership-routing-contract.shpromotion-worker-contract.shLive end-to-end replay. The shipped
headHasPreviewBundle/assertPreviewBundlewere imported and run against the real API and real PRheads (read-only
GET; no live state was mutated):Heads for #623/#621/#619/#617/#614 were checked the same way: all missing, all
correctly ineligible.
Negative control 1 β the first draft probed
product/remix/package.json.Against live heads that returned 404 for every PR including #610/#612/#613,
i.e. it would have silently disabled previews repository-wide. That caught the
product/-is-the-checkout-directory error before it shipped; corrected toremix/package.jsonand re-verified above.Negative control 2 β inverted the 404 branch to
return true(thepre-fix behaviour): self-test fails with
true !== false. Restored β 138/138.Negative control 3 β removed the
prepareBuildPlan()call site: only onecall site remains, confirming the two are distinct and independently required.
Follow-up worth a decision (not changed here)
The develop preview reports success and failure only as a PR comment, so a
Source Lopu workflow run