Skip to content

fix(actions): Lopu controller repair β€” control-plane PRs are authorized into a product build they cannot possibly run - #625

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

fix(actions): Lopu controller repair β€” control-plane PRs are authorized into a product build they cannot possibly run#625
lopugit merged 1 commit into
github-actionsfrom
lopu/workflow-check-fix-33907643740

Conversation

@lopugit

@lopugit lopugit commented Sep 4, 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 repair β€” control-plane PRs are authorized into a product build they cannot possibly run

Files changed (in $GITHUB_WORKSPACE/trusted only):

  • .github/scripts/deploy-develop-pr-preview.mjs β€” preview eligibility now also
    requires 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:

<!-- thingtime-develop-pr-preview -->
### ❌ Develop S3 preview failed
- Commit: `12597008`
The ordinary generated Vercel Preview remains available on the shared development
runtime. Re-run this workflow after correcting the deployment, DNS, or CORS
configuration.

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 624 is 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_target handoff (33907601208) succeeded and
dispatched the privileged worker (33907615031, head_branch=main), whose
Build exact PR bundle without secrets job failed:

ERROR  ENOENT: no such file or directory, lstat
       '/home/runner/work/thingtime/thingtime/product/remix'
##[error]Process completed with exit code 1.

The build job checks the PR head out into the product/ directory and then
runs corepack pnpm --dir product/remix install --frozen-lockfile. PR #624 is a
control-plane PR: base github-actions, whose tree is only .github, the root
docs, PRs/, graphify-out/, and vercel.json. There is no remix/, so
product/remix cannot exist and the install always fails.

assertTrustedPullRequestStack is the only gate in front of the build, and it
answers a different question. Its own comment says so:

Preview eligibility belongs to the exact PR being built. Its base may be
develop, main, another feature branch … none of those shapes changes the trust
of this PR's same-repository author, ref, or immutable head SHA.

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-actions is affected, and none based on develop is:

PR base remix/package.json at head preview
624, 623, 621, 619, 617, 614 github-actions missing (404) ❌ false failure
613, 612, 610 develop / feature present βœ…

That 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:

const PREVIEW_BUNDLE_PATH = 'remix/package.json';

const headHasPreviewBundle = async (repository, headSha, request = githubRequest) => {
  if (!/^[0-9a-f]{40}$/.test(headSha ?? '')) throw new EligibilityError('invalid-head-sha');
  try {
    const entry = await request(`/repos/${repository}/contents/${PREVIEW_BUNDLE_PATH}?ref=${headSha}`);
    return entry?.type === 'file';
  } catch (error) {
    if (error instanceof HttpError && error.status === 404) return false;
    throw error;
  }
};

Called from prepareBuildPlan() and from main(), inside the try blocks that
already classify EligibilityError. Both call sites are required:

  • prepareBuildPlan() alone would set should_build=false and skip the build β€”
    but the controller's Reconcile or clean up without a new build step re-enters
    main(), which would fall straight through to deploy() with no prebuilt
    bundle and fail again.
  • With both, a control-plane PR takes the existing ineligible path:
    synchronize/opened log Skipped unrelated PR #N: head-has-no-preview-bundle
    and post nothing; edited/closed reach handleIneligible, which only
    comments when it actually removed an alias or deployment β€” never the case here.

Deliberate choices:

  • Head-content probe, not a base-ref filter. The caller workflow on the
    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.
  • Only a 404 means "nothing to preview." Any other HTTP error propagates, so
    a transport fault or a permissions regression cannot be silently misread as an
    empty head and quietly disable previews repository-wide.
  • assertTrustedPullRequestStack is untouched, so assertCurrentPullRequest,
    deploy(), and the scheduled reconcile() sweep keep their existing
    semantics.
  • The path is repository-relative. product/ is the checkout directory, not
    part of the repository layout β€” see the negative result below.

Validation

Check Result
deploy-develop-pr-preview.mjs --self-test 138/138 pass (was 120/120)
workflow-control-plane-contract.mjs --self-test OK
resolve-pr-conflicts-routing-contract.mjs --self-test OK
node --test resolve-pr-conflicts-routing-contract.test.mjs 10/10 pass
promotion-worker-routing-contract.mjs OK
rebase-ownership-routing-contract.sh OK
promotion-worker-contract.sh OK

Live end-to-end replay. The shipped headHasPreviewBundle /
assertPreviewBundle were imported and run against the real API and real PR
heads (read-only GET; no live state was mutated):

probe path: remix/package.json
PR #610 3eea1252 hasBundle=true   ELIGIBLE (build authorized)
PR #612 1ca8bcc4 hasBundle=true   ELIGIBLE (build authorized)
PR #613 bf3498fb hasBundle=true   ELIGIBLE (build authorized)
PR #624 12597008 hasBundle=false  INELIGIBLE (head-has-no-preview-bundle)
                                  -> should_build=false, no failure comment

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 to
remix/package.json and re-verified above.

Negative control 2 β€” inverted the 404 branch to return true (the
pre-fix behaviour): self-test fails with true !== false. Restored β†’ 138/138.

Negative control 3 β€” removed the prepareBuildPlan() call site: only one
call 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

@lopugit

lopugit commented Sep 4, 2026

Copy link
Copy Markdown
Owner Author

❌ 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.

@github-actions github-actions Bot added the lopu: mergeable The PR branches can currently be merged without conflicts label Sep 4, 2026
@lopugit
lopugit temporarily deployed to develop-pr-625 September 4, 2026 19:07 Destroyed
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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 build job checks the head out into product/ and unconditionally runs corepack pnpm --dir product/remix install --frozen-lockfile, but github-actions carries no remix/ workspace, so it dies with ENOENT … /product/remix and the controller publishes a failure comment telling the operator to fix deployment/DNS/CORS β€” none of which is the problem.

Confirming the scale independently: every control-plane preview worker today failed this way β€” 33909394105, 33909387490, 33907615031, and 33888936460 / 33887678048 / 33886516079 / 33885726085 / 33885038530 / 33883924348 / 33882576994 for codex/preview-manager-reliability β€” interleaved with successes for product PRs. Spot-checked jobs 101075627620 and 101054590543: same step, same error. Not a flake and not an outage.

Why I dropped my version for this one

I had written a base.ref == 'github-actions' ineligibility gate. This is better on four counts, so I reverted mine rather than open a competing change:

  • Probing the head for remix/package.json is general to any product-less head and doesn't hardcode a branch name.
  • Mapping 404 β†’ "no bundle" while rethrowing every other status is the detail that matters. A base-ref gate never needed it; a probe does, and reading a 403 or a 5xx as "nothing to preview" would silently disable previews repo-wide. Good call pinning it in the self-test.
  • Validating the head SHA shape before interpolating into the contents URL.
  • Calling the probe in both prepareBuildPlan and main() β€” the reconcile/report step re-enters main(), so classifying before deploy() is reached is necessary, and the comment saying so is worth keeping.

Keeping it as an EligibilityError rather than a workflow-level if: is also right: --prepare returns before upsertComment(state: 'deploying'), and main()'s cleanupRelevant branch stays intact, so a PR retargeted from develop onto the control plane still cleans up resources it had already created.

Verified independently: fetched .github/scripts/deploy-develop-pr-preview.mjs from this head, node --check passes, --self-test reports 138/138.

Heads-up on this PR's own red check

The same Contract advisories cancellation affects both PRs. control-plane-ci.yml uses the constant concurrency group workflow-control-plane-ci with cancel-in-progress: true, so this PR's run 33909382726 cancelled #624's 33909377070 four seconds after it started β€” #624's run had every step green. I've opened a separate controller repair scoping that group per PR/ref. It touches only control-plane-ci.yml and workflow-control-plane-contract.mjs, so it's disjoint from this PR and the two can land in either order.

One loose end worth a decision, not changed here: neither fix retracts the stale ❌ preview comments already sitting on #624 and #625. handleIneligible only comments when it actually cleaned something up, which is correct for PRs that never had a preview β€” but it does leave a misleading red status on these two. Retracting it would be a change to preview-comment ownership, so it's your call.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Lopu review β€” the red check was a GitHub outage, not this diff. Re-ran it; PR #625 is green and CLEAN.

Following up on my earlier comment on this PR: the Contract advisories cancellation I flagged there has cleared (that repair is #626). The check that was still red just now is a different one, so I diagnosed it from the logs rather than assuming it was the same thing.

Find merge-conflicting PRs β€” run 33909379288, attempt 1

::notice::GitHub API read hit a transient failure (attempt 1/4); retrying in 2s.
::notice::GitHub API read hit a transient failure (attempt 2/4); retrying in 4s.
::notice::GitHub API read hit a transient failure (attempt 3/4); retrying in 8s.
::error::GitHub API read still failed after 4 transient attempts.
gh: HTTP 502
::error::Could not read the open PR inventory from GitHub; the API read failed before any response was parsed.

That is gh_read_retry wrapping the all_open_prs() GraphQL inventory read, doing exactly its job: classified the 502 as transient, retried on the 2/4/8s ladder, exhausted it, failed loudly with a precise message. Nothing to repair in the controller.

It is request-level flakiness, not a query this repo has outgrown β€” the same read succeeded and failed inside the same minute:

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, head 2af16fcf β†’ 404. The repo-relative path is right.
  • Both call sites are genuinely required: the controller job's Reconcile or clean up without a new build step is gated if: needs.prepare.outputs.should_build != 'true' and re-enters main() without VERCEL_PREBUILT_DIR in its env, so a prepareBuildPlan()-only gate would reach deploy() with no bundle.
  • The ineligible routing does not start commenting on control-plane PRs: opened/synchronize on base github-actions misses every cleanupRelevant arm and is log-only; the paths that do reach handleIneligible only comment when aliasRemoved || 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 assertTrustedPullRequestStack at both sites, so a fork head SHA is never probed against the base repo, and headSha is 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.

@github-actions

github-actions Bot commented Sep 4, 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 #625 Β· preview eligibility requires a buildable head

lopu/workflow-check-fix-33907643740 β†’ github-actions @ 2af16fcf
Compared against base 82786d9f (current github-actions tip).

What I compared

  • Full head-vs-base diff (1 file, +83/βˆ’0).
  • The whole eligibility β†’ build β†’ reconcile path in
    .github/scripts/deploy-develop-pr-preview.mjs, in particular what happens to
    a PR that trips the new EligibilityError on each action.
  • develop-pr-preview.yml's five invocation sites (--prepare, the worker, the
    reconcile/report steps) to confirm the probe is reached on every entry.
  • control-plane-ci.yml line 154, which runs this file's self-test.

Findings

The root cause is real and the probe is the right shape. The build job checks
the PR head out into product/ and then runs
pnpm --dir product/remix install. A control-plane PR (base github-actions)
has no remix/ in its tree at all, so the install could only ever fail with
ENOENT … /product/remix β€” and the failure surfaced as a "Develop S3 preview
failed" comment whose remediation text blamed deployment/DNS/CORS, none of which
were involved. Probing the head for remix/package.json answers the question
assertTrustedPullRequestStack was never asking (buildability, not trust).

The failure mode is correctly non-silent. headHasPreviewBundle returns
false only on a 404; every other HttpError re-throws, so a 403/500 from the
contents API cannot be misread as "nothing to preview" and quietly suppress a
legitimate preview. The self-test pins exactly that (the 403 and 500 cases).

Both call sites are needed. prepareBuildPlan gates the build, and main()
gates the reconcile/report re-entry, which arrives with no prebuilt bundle. If
only the first were guarded, the second path would still reach deploy() for an
unbuildable head. The comment on the main() call says this; I confirmed it by
reading the two entry points rather than taking it on faith.

The ineligible path is a clean skip, not a red comment. For a control-plane
PR with a non-cleanup action, cleanupRelevant is false (base.ref is
github-actions, not develop), so main() logs
Skipped unrelated PR #N: head-has-no-preview-bundle and returns. No status
comment is written. That is the intended outcome.

No regression for the merged-into-develop path. When action === 'closed'
the reason is overridden to 'closed' before handleIneligible, so a merged
develop PR whose head SHA has become unreachable takes the same cleanup branch it
took before, and the mergedIntoDevelop alias reconcile still runs afterwards.

One thing worth knowing (not a defect)

PREVIEW_BUNDLE_PATH is a content probe rather than a base-ref filter, which
the comment justifies (retarget/close events must still reach cleanup). The
consequence is one extra GitHub contents call per preview event. That is
negligible against the token exchange and build the same run performs, and it
buys correctness for a stacked PR whose base is neither develop nor
github-actions. I agree with the trade.

Minor cosmetic note, deliberately not changed: the block of four
headHasPreviewBundle assertions increments checks once rather than four
times. Since the summary line prints ${checks}/${checks}, the count is
decorative either way and no assertion is skipped.

Interaction with the sibling controller PRs

Disjoint from #624 and #626 (see #626's report for the file-level breakdown).
All three are MERGEABLE / CLEAN and can land in any order.

Changes made

None. The implementation is correct and already carries its own coverage.

Validation run

$ node .github/scripts/deploy-develop-pr-preview.mjs --self-test
develop PR preview self-test: 138/138 passed          (exit 0)

Recommendation

Approve and merge.

View Lopu workflow run

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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 ENOENT ... /product/remix in Build exact PR bundle without secrets:

run head built PR
33907615031 1259700 #624 (earlier head)
33909387490 a546525 #624
33909394105 2af16fc #625 β€” this PR
33911073742 e4991e9 #626

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:

  • git ls-tree at a5465255 and at base ae9a113a returns only .gitattributes, .github, .gitignore, the root docs, PRs, graphify-out, vercel.json β€” no remix/, so the probe correctly reports the head as unbuildable.
  • repos/…/contents/remix/package.json?ref=develop returns type: file, so product heads keep building. The probe path is right: product/ is the checkout directory, not repository layout β€” the distinction your comment calls out is the one that matters, and it's correct.

Two details I liked, since they're the ones that usually get missed: raising on a non-404 HttpError rather than reading a transport fault as "nothing to preview", and applying the probe in the reconcile path too, so retarget/close events still reach cleanup instead of dying in deploy(). deploy-develop-pr-preview.mjs --self-test passes 120/120 at #624's head, i.e. before your additions, so the 5 new checks are additive to a green baseline.

One knock-on worth knowing: until this lands, every failing preview posts a <!-- thingtime-develop-pr-preview --> comment, and under the current conversation gate each of those comments spends a full Lopu review session on an unchanged head. #624 fixes that half. The two are independent fixes but the same loop β€” merging them together closes it.

πŸ€– Posted by Lopu, Thingtime's automated PR reviewer.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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 ❌ Develop S3 preview failed comment on this PR. Three earlier Lopu comments here already cover the root cause, the scale, and why both call sites are needed β€” I re-derived all of it and it holds, so I won't restate it. Below is only what those comments don't have.

Why this PR's ❌ cannot clear before it merges

develop-pr-preview.yml's prepare and controller jobs both check out ref: github-actions, so the controller always runs the base-branch script, never the PR head. The blob at .github/scripts/deploy-develop-pr-preview.mjs@github-actions is 6fee4588 β€” exactly the index 6fee4588..e1f70337 pre-image in this diff. The deployed controller is byte-identical to the unfixed version, so every control-plane PR keeps posting this comment until the fix lands. Run 33909394105 is this PR's own worker and reads as a textbook reproduction: GitHub prebuild authorized for PR #625 at 2af16fcf… β†’ ENOENT … '/product/remix' β†’ ❌ comment at 19:07:11 β†’ Missing required workflow setting: VERCEL_PREBUILT_DIR at 19:07:12.

That last line is worth pausing on: it is empirical proof of the main() call site, not just an argument for it. The report step re-entered main(), sailed past assertTrustedPullRequestStack, refreshed the stable alias, and died inside deploy() with no bundle. A prepareBuildPlan()-only gate leaves that exact path live.

Regression sweep β€” 23/23 open heads, zero false negatives

The 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 PREVIEW_BUNDLE_PATH / headHasPreviewBundle / assertPreviewBundle verbatim from this head and ran them against the live API for every open PR (read-only GET, nothing mutated):

Group PRs Result
base github-actions #626, #625, #624 hasBundle=false β†’ INELIGIBLE (head-has-no-preview-bundle)
base develop / main / feature #613 #612 #611 #610 #607 #602 #596 #595 #592 #590 #578 #568 #564 #560 #557 #554 #499 #295 #291 #10 hasBundle=true β†’ ELIGIBLE, all 20

Heads disagreeing with base-shape expectation: 0. All seven main-based PRs stay eligible, so this doesn't quietly narrow previews to develop.

The blast radius is tighter than the description claims

pullRequestShapeIssue returns not-open and draft before any principal check, and the probe runs after assertTrustedPullRequestStack at both sites. So closed, merged, and draft PRs never reach the probe at all β€” the cleanup path isn't merely preserved by the cleanupRelevant arms, it's untouched, and no API call is spent on an untrusted head. That's a stronger guarantee than the description sells.

One behavioural delta nobody has flagged

Control-plane PR events currently reach reconcileStableDevelopAlias(config) right before failing in deploy() β€” visible at 19:07:05 in the run above. After this change they return from the stackEligibilityError branch and no longer refresh the stable develop alias. This is fine: that alias belongs to the schedule sweep and to every real develop deploy, and the old path always ended red anyway. Recording it so the change in ownership isn't a surprise later.

Validation

Ran the full control-plane-ci.yml contract set from the worktree, not a subset β€” all 12 green, including --self-test at 138/138 (base ae9a113a: 120/120). The +18 reconciles exactly: 8 new top-level assertions plus 10 equal calls inside the bundleProbe/contentsProbe fixtures (2Γ—3 + 1Γ—4), which confirms every new fixture was actually invoked rather than silently skipped. CodeQL snapshot for this head is empty. gh pr checks is 55 skipped + 27 success, 0 failures, CLEAN.

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.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

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 (a5465255), this PR (2af16fcf), #626 (e4991e96). All three share one cause β€” develop-pr-preview.yml runs corepack pnpm --dir product/remix install --frozen-lockfile unconditionally, and github-actions has no product/ in its tree. From the worker log for #626 (job 101147416668):

ERROR ENOENT: no such file or directory, lstat '.../product/remix'

2. This PR cannot clear its own ❌. The prepare job checks out the controller with ref: github-actions, so deploy-develop-pr-preview.mjs always runs from github-actions, never from the PR head. The head probe therefore takes effect only once merged β€” so the ❌ above will persist on this PR until then, and re-running the preview beforehand will not clear it. Worth knowing so the red comment is not read as the fix failing.

Validated independently against head 2af16fcf: node --check passes and --self-test reports 138/138 passed. I also traced the control flow β€” assertPreviewBundle raises EligibilityError('head-has-no-preview-bundle'), which drives writePrepareOutputs({ shouldBuild: false }), so the build job is skipped and the Report failed GitHub prebuild guard (should_build == 'true' && ...) is false. The ❌ is not published and the controller reconciles instead. Calling the probe in both prepareBuildPlan and main() is necessary, and the comment explaining the reconcile re-entry earns its place.

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.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

βœ… Lopu β€” reviewed and validated; no changes needed

Compared the full head (2af16fcf) against github-actions @ ae9a113a.
The framing is right: trust and buildability are different questions. The
build job checks the head out into product/ and runs
pnpm --dir product/remix install, so a head without remix/ cannot build no
matter how trusted its author is.

I checked the integration, which is where a gate like this usually goes wrong:

  • Cleanup is preserved. The probe runs inside the same try as
    assertTrustedPullRequestStack, and an EligibilityError only sets
    stackEligibilityError. cleanupRelevant is unchanged, so retarget-away and
    close events still reach handleIneligible; a control-plane PR takes the
    console.log skip branch, which is correct β€” it never had resources.
  • No misleading comment. head-has-no-preview-bundle is unmapped in
    cleanupComment, so it falls through to the generic "skipped" text β€” and
    handleIneligible only comments when
    cleaned.aliasRemoved || cleaned.deleted > 0. A control-plane PR created
    nothing, so nothing is posted. Correct on both counts.
  • Fails open on transport faults, not closed-as-ineligible: only HTTP 404
    becomes false; 403/500 re-throw. The self-test pins exactly that β€” and it's
    the distinction that matters, since silently reading a 500 as "nothing to
    preview" would have been the subtle bug here.
  • Choosing a head-content probe over a base-ref filter is right: the caller
    workflow doesn't filter by base ref either, so retarget/close must still reach
    cleanup.
  • Definitions sit after runSelfTest in the module, but it's only invoked after
    top-level evaluation, so no TDZ issue.
$ node .github/scripts/deploy-develop-pr-preview.mjs --self-test
develop PR preview self-test: 138/138 passed

Two cosmetic notes, no behaviour impact: one extra contents read per
develop-PR run, and checks += 1 increments once for the three-assertion
headHasPreviewBundle block.

Checks: 27 pass / 0 fail. Ready to merge.

Lopu Β· automated repository review

@github-actions github-actions Bot added lopu: overlapping files This PR changes files also changed by another open PR lopu: queued The current PR snapshot is waiting in Lopu's PR-management queue and removed lopu: overlapping files This PR changes files also changed by another open PR labels Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

πŸ€– Lopu detected an out-of-date PR branch

Status: 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)

Moment UTC Los Angeles Melbourne
Updated 2026-09-05 06:08 UTC (UTC+00:00) 2026-09-04 23:08 PDT (UTC-07:00) 2026-09-05 16:08 AEST (UTC+10:00)
Estimated finish 2026-09-05 06:28 UTC (UTC+00:00) 2026-09-04 23:28 PDT (UTC-07:00) 2026-09-05 16:28 AEST (UTC+10:00)

Los Angeles and Melbourne use their real IANA time zones, so PDT/PST and AEST/AEDT offsets change automatically.

Lopu queue and PR pulse

Scope Metric Count
Repository Open PRs 25
Repository Conflicting 1
Repository Out-of-date with target 0
Repository GitHub state unknown 7
Repository Part of an open stack 2
Repository Touch files changed by another open PR 15
Repository Target a non-root branch without an open parent PR 0
This resolver batch Admitted snapshots 3
This resolver batch Currently resolving 0
This resolver batch Waiting 3
This resolver batch Finished 0

Related PR context

  • Stack: No open parent or child PR currently links to this branch.
  • Target: github-actions is a repository root/integration branch.
  • Changed-file overlap: No changed paths overlap another open PR in this snapshot.

Exact branch pair: github-actions β†’ lopu/workflow-check-fix-33907643740.

Timeline

  • 06:08 UTC β€” Detected that github-actions needs to be merged into lopu/workflow-check-fix-33907643740; assigning the exact snapshot to the resolver queue.

@github-actions github-actions Bot added lopu: queued The current PR snapshot is waiting in Lopu's PR-management queue and removed lopu: queued The current PR snapshot is waiting in Lopu's PR-management queue labels Sep 5, 2026
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

πŸ•΅οΈ Lopu β€” verified the two claims that carry this fix

Root cause confirmed: the build job checks the head out into product/ then runs pnpm --dir product/remix install, and a control-plane PR's tree has no remix/ at all β€” so the install could only ever ENOENT. Worse than failing was failing misleadingly: the status comment told the operator to correct the deployment, DNS or CORS, none of which were ever reached. Probing the head answers the question assertTrustedPullRequestStack was never asking. Trust and buildability are genuinely different predicates.

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, cleanupRelevant is false (base.ref is github-actions, not develop), so main() logs Skipped unrelated PR #N: head-has-no-preview-bundle and returns. No comment is written. And action === 'closed' overrides the reason to 'closed' before handleIneligible, so a merged develop PR whose head SHA has become unreachable takes exactly the cleanup branch it took before, with the mergedIntoDevelop alias reconcile still running after. No regression on that path.

2. Both call sites are needed. prepareBuildPlan gates the build; main() gates the reconcile/report re-entry, which arrives with no prebuilt bundle. Guarding only the first would still let the second reach deploy() for an unbuildable head. Read both entry points to confirm.

The failure-mode polarity is also right, and it's the detail that would have been easy to get wrong: headHasPreviewBundle returns false only on 404 and re-throws every other HttpError, so a 403 or 500 from the contents API can't be misread as "nothing to preview" and silently suppress a legitimate preview. The self-test pins exactly those two cases.

$ node .github/scripts/deploy-develop-pr-preview.mjs --self-test
develop PR preview self-test: 138/138 passed   (exit 0)

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 develop nor github-actions. Right call.

Merge-order note for all three controller repairs is on #626 β€” disjoint files, any order works.

No changes made to this branch.

@lopugit
lopugit merged commit 16cdc74 into github-actions Sep 5, 2026
110 of 111 checks passed
@github-actions github-actions Bot removed the lopu: mergeable The PR branches can currently be merged without conflicts label Sep 5, 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