-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5698
#5698 fix(ci): checkout publisher for preview recovery by cixzhang (bucket: the maintainer)
5dac75c903665ce2471f39402d2e1d92c5ab413b
LOOP VERSION: 1.9.1 AUDIT RUBRIC: 1.15.1
LANE: full
WHY: the code delta is small, but this is a manual workflow with contents: write, fork PR artifacts, and a trusted-code/no-PR-execution boundary. Review scope included permission, ordering, fork safety, workflow syntax, regression strength, and live failed recovery runs.
PRIOR REVIEW: checked; no human or Copilot reviews existed at this head, only automated Vercel and PR enrichment comments.
WHY 1: Manual preview recovery runs could find the PR artifacts but fail before publishing because the runner never checked out the repository code containing the shared publisher. WHY 2: Maintainers trying to recover previews for review then still had no refreshed Storybook/Sandbox preview, even though the artifacts existed. WHY 3: Preview recovery must stay safe for fork PRs: it should republish already-built artifacts without checking out or executing PR-controlled code with the repo token.
USER-FACING PROBLEM: A maintainer using manual Re-deploy Preview for a PR with existing CI artifacts gets a failed recovery run instead of a refreshed preview.
PROBLEM SEVERITY: broken task — four live recovery runs failed at the publisher invocation with MODULE_NOT_FOUND after artifact download succeeded.
NEW FEATURE CASE: not a new feature.
EARLY STOP: clear — the live run failure and affected maintainer task are proven.
VERDICT: clear
The workflow now checks out trusted main before it resolves the PR run, downloads artifacts, verifies them, and invokes the shared publisher. That gives the runner the publisher script while preserving the existing model: PR content is only downloaded as static artifacts and is copied, not executed. The job permissions remain scoped to the existing needs: read PR/run state and write the Pages branch.
SOLUTION (1 decision · 5 workflow lines of 10 added lines)
- Checkout trusted
mainas the first job step so all later lookup and publication logic comes from default-branch code.
BURDEN: low — one existing checkout action, no package/runtime API, dependency, schema, state, or component surface; one focused workflow-regression assertion.
BURDEN MATCH: proportionate — the manual workflow needs repository code to invoke the shared publisher, and ref: main preserves the trust boundary.
VERDICT: clear
OWNER: the Re-deploy Preview workflow owns manual preview recovery orchestration; the shared gh-pages publisher owns mutation of gh-pages preview subtrees.
TIER 1: existing shared gh-pages publisher and Actions artifact APIs.
TIER 2: none.
SEAMS: manual workflow_dispatch, typed PR number, trusted PR head lookup, CI run lookup, artifact download, artifact presence check, publisher invocation, and fork PR head content.
BEHAVIOR UNIT: workflow step order plus static workflow-regression test.
COMPLEXITY BUDGET: 1 workflow step, 1 trusted code source, 0 new writable state, 0 new publisher commands, 0 PR-code execution.
ACTUAL BURDEN: 2 changed files; 5 workflow lines; 5 test lines; existing job permissions retained (contents: write, pull-requests: read, actions: read).
BURDEN TREND: first reviewed head → current head: flat; this is the first review of this head.
RESET TRIGGER: not triggered — one missing checkout is fixed without adding another owner or representation.
| domain fact | one authoritative writable source | generated / immutable projections | other writable copies |
|---|---|---|---|
| recovery workflow code | default-branch main checkout |
runner workspace used by later shell steps | none |
| PR preview payload | completed PR CI artifacts for the PR's current head SHA | downloaded storybook-dist / sandbox-dist directories |
none |
| preview publication | shared gh-pages publisher |
pr/<number>/ Pages subtree |
no direct workflow push |
| seam | driven result |
|---|---|
| failed live recovery runs | four runs failed after artifact download at Cannot find module ... gh-pages-publisher.mjs
|
| checkout ordering |
Checkout publisher is line 46, before Locate the PR's CI run, both artifact downloads, verification, and gh-pages-publisher.mjs pr-preview
|
| fork safety | workflow checks out ref: main; it never checks out the PR head and only passes downloaded static directories to the publisher |
| permissions | global permissions remain {}; job grants only contents: write, pull-requests: read, and actions: read
|
| publisher behavior |
publishPrPreview() validates the PR number and source directories, copies bytes into a Pages checkout, commits, and pushes; it does not execute artifact contents |
| regression strength | focused test now requires checkout, ref: main, and checkout before publisher; non-blocking gap: it does not explicitly assert checkout before the locate step |
VERDICT: clear — implementation satisfies the trust boundary; regression has one non-blocking precision gap.
Maintainers can recover a PR preview from already-built artifacts instead of getting a missing-publisher failure. Fork PR authors are not exposed to a new privileged execution path because the workflow checks out trusted main, not the fork head, before using repository code.
NEW FEATURE IMPACT: not a new feature.
VERDICT: clear
No package or consumer API change. The only operator surface is the existing manual workflow_dispatch input.
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
~ |
Re-deploy Preview workspace now contains trusted default-branch code before publication | repository workflow | existing manual recovery workflow | workflow comments and tests | ok |
OSSIFICATION: internal repository automation only; the behavior matches the existing safety comment that the workflow uses built artifacts and default-branch code rather than PR checkout.
VERDICT: clear
No component CSS, tokens, variables, theme targets, or rendered styles changed.
VERDICT: clear
BEHAVIOR: yes, intended — the manual workflow now has repository code available and should publish instead of failing at missing module. API: no consumer API change. VISUAL: no component/story pixels changed; previews are publication payloads only. THEME: no theme surface change.
VERDICT: clear
EFFECTS: zero. RENDER: no render path. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package bundle change. CI/runtime cost: one default-branch checkout added to a manual recovery job; no recurring CI path cost.
VERDICT: clear
VISUAL CHECK: not applicable WHY: both changed files are CI workflow/test files; no component output, DOM, CSS, Storybook story, or visual acceptance surface changed. Exact-head visual/a11y/RTL jobs were skipped by scope, and the PR enrichment reported no modified components.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
No rendered elements, roles, focus behavior, accessible strings, locale formatting, or directional layout changed.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear; note on regression precision |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable |
| A11Y & I18N | clear |
GOAL: met — the missing publisher failure is addressed by a trusted main checkout before lookup/download/publish, without adding PR-code execution.
DISPOSITION: approve once undrafted. The test precision note can be handled in this or a follow-up commit.
ADVICE: if touching this before merge, assert the step order as checkout < Locate the PR's CI run < Download Storybook artifact < Download Sandbox artifact < Deploy to GitHub Pages so the regression preserves the full invariant named by the workflow.
AUTHOR CAN PROCEED: yes.
WORST OUTCOME: the current head ships a weaker-than-ideal regression test, not a broken recovery path.
JUDGEMENT NEEDED: none.
approve once undrafted
- Non-blocking regression note: the test checks checkout before publisher but not before PR lookup/artifact locating. The workflow itself has the right order, so this does not block the draft.
Semantic verdict: approve once undrafted
This looks right for the recovery path. The failed manual runs were all getting through artifact download and then dying because the runner had no checked-out .github/scripts/gh-pages-publisher.mjs; this head checks out trusted main before the PR lookup, artifact downloads, and publisher invocation, so the job has the publisher without checking out fork code.
One non-blocking thing I’d tighten while you’re here: the regression locks checkout before gh-pages-publisher.mjs, but not specifically before the “Locate the PR's CI run” step. The workflow itself has the right order, so I would not hold the draft on it; the test would just preserve the full trust-boundary invariant more directly.
[Reviewed by Robohands]
None.
- Live failed recovery runs: 33321176378, 33321177704, 33321179157, and 33321180475 all failed in
Deploy to GitHub PageswithCannot find module '/home/runner/work/astryx/astryx/.github/scripts/gh-pages-publisher.mjs'after artifact download steps succeeded. - Exact-head focused local test:
.github/scripts/visual-gate/workflow-concurrency.test.mjspassed 20/20 in 20.41s. - Exact-head CI: run 33321672492 passed, including the workflow-concurrency test and 626 passed test files; lint, dependency-check, smoke-test, build-storybook, build-sandbox, docsite-test, theme-layers, fixture-contrast, check-scope, and check-components were green.
- Workflow syntax: local actionlint reached only the existing ShellCheck style warning about grouped redirects; remote exact-head lint passed.
- No live manual redeploy was dispatched because that would mutate the shared public Pages branch.
TIME total 18m
- setup/rules: 4m — rules, owner bucket, prior reviews/comments, exact head, and untrusted diff surface
- worktree/install: 2m — detached exact-head worktree and dependency materialization for focused Vitest
- code/history/runs: 5m — workflow order, publisher path, permissions, failed recovery logs, and exact-head CI
- focused tests: 2m — local workflow-concurrency test and syntax check
- critique/wiki/delivery: 5m — review text, fork-wiki record, GitHub review, readback, cleanup
- I did not dispatch
workflow_dispatchfor a live redeploy; that would mutategh-pages. The failure evidence comes from the four live failed runs, and the success evidence comes from exact-head workflow order plus focused/CI tests.