-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5514
#5514 — ci(visual-gate): build the theme dists the gate needs instead of crashing
cixzhang · open · view on GitHub
Verdict: comment — semantic approve (drafted; GitHub cannot accept the author's self-approval, and no public action was authorized)
HEAD REVIEWED: 765b432ad0cf5158686901f947378fff080edfa2 — every claim below was verified at this commit. Parent 41494fcd41310c819f0501df2e798235c4b41135.
Versions. Review Loop 1.4.0 · Component Audit Rubric 1.12.
Bucket: the maintainer. The author is cixzhang. This remains a real review; an accepted verdict must be posted mechanically as a comment because GitHub rejects self-approval.
LANE: full. The diff changes a workflow and an executable CI module. That trust surface is a mandatory full-lane promotion even though the runtime decision is narrow.
Description → test plan → code. The body establishes the failure with a denominator (15 of 21 open PRs), distinguishes the two missing artifacts, explains why re-runs are frozen to the old merge commit, and says what is and is not fixed retroactively. The test evidence is split between four focused unit tests and two real pr-visual runs in the PR comment: a forced missing-artifact run and an artifact-present control. Code inspection followed that framing.
Prior review: none. The PR has no submitted reviews. Review-5514.md and an index row were absent from the fresh fork-wiki clone.
Main did not invalidate this. The workflow and source-loader blobs are byte-identical between the PR parent and current origin/main (90bf6c9cd10); no later main commit touched the reviewed surface. #5482 is already in the parent and its unconditional Core build remains at ci.yml:602-603.
A component PR can receive a red pr-visual before the visual gate runs because the job expects built theme and Core files supplied by another job's artifact. The soft gate then teaches contributors to ignore red. This is reproduced from a clean parent tree in both historical failure shapes:
- no built dists →
Theme butter is not built (.../packages/themes/butter/dist/source.mjs missing); - themes present but Core absent (the exact #5477 half-fix state) →
ERR_MODULE_NOT_FOUNDfor@astryxdesign/core/dist/theme/index.js.
The stated 15/21 incidence is supported by the body; the review does not depend on that count because both mechanisms were independently reproduced.
VERDICT: clear
The gate first tries the same built theme data it already used. If that import fails, it builds the missing Core dependency once, builds only each missing theme once, retries through a fresh module URL, and reports a persistent unreadable build differently from a missing one. The workflow makes only the dists download non-fatal so this recovery can run; analysis and Storybook artifacts remain required. When all artifacts are present, the loader performs no builds.
SOLUTION (4 decisions · ~56 runtime lines of 203 added)
- Treat the dists artifact as an optimization, not a correctness dependency.
- Build missing prerequisites in Core-before-theme order, at most once per package.
- Retry the import under a cache-busting URL and preserve the underlying error if it still fails.
- Inject the build side effect so the orchestration is testable without spawning package builds.
Every decision traces to the missing-artifact failure. No unrelated runtime decision is bundled.
VERDICT: clear
OWNER: the visual gate's source loader, which already owns reading built theme component maps
TIER 1: GitHub Actions artifacts and each package's existing build script; no new build system or registry
TIER 2: none
SEAMS: artifact present; themes missing; Core missing; import still unreadable after a build; ESM retry caching
BEHAVIOR UNIT: inline — one loader owns the import/build/retry lifecycle, with an injected builder as the focused test seam
| seam | driven result |
|---|---|
| all Core/theme dists absent | parent throws on Butter; exact head builds Core plus eight themes and loads seven shipped themes (Probe excluded) |
| theme dists present, Core absent | parent reproduces #5482's ERR_MODULE_NOT_FOUND; exact head builds Core only and loads all seven |
| all dists present | exact head loads all seven with zero visual gate: build lines |
| dists artifact missing in GitHub Actions | run 32900359861 records the artifact miss, builds Core plus eight themes, captures 16/16, and passes pr-visual
|
| artifact present in GitHub Actions | run 32901113865 captures 16/16 and passes with zero fallback-build lines |
| build callback returns without a readable theme | focused test preserves a diagnostic saying the rebuild was attempted |
Trust boundary. Step-0 inspection covered the complete workflow/executable diff before checkout or execution. The head is facebook/astryx, not a fork. ci.yml runs on pull_request/merge_group, starts with permissions: {}, and grants pr-visual only contents: read; the changed job references no secrets.*, id-token, or write permission. It executes checked-out PR package scripts, but this job already installs, builds Core, and runs the PR's gate code under the same read-only token; the fallback adds no privilege. actions/download-artifact@v8, actions/checkout@v7, and actions/upload-artifact@v7 remain tag-pinned rather than SHA-pinned, but this PR changes none of those references and does not widen that pre-existing risk.
VERDICT: clear
Every component PR using pr-visual gains a recovery path when only the dists artifact is unavailable or incomplete. A healthy run still downloads and imports the artifact without spawning a fallback build. A fully missing artifact adds nine serial package builds before the same visual plan; an old PR whose event captured the prior workflow is unchanged until a new push creates a new merge commit. Analysis and Storybook artifact failures still stop the job, correctly: the gate cannot infer touched components or capture stories without them.
VERDICT: clear
Production usage remains unchanged:
const overrides = await loadThemeOverrides(REPO_ROOT, config.probeTheme);| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
loadThemeOverrides(repoRoot, probeTheme = 'probe', build = pnpmBuild) |
no — .github/scripts only |
existing injected-side-effect test seam | JSDoc | ok |
OSSIFICATION: the optional third parameter is reachable only inside the repository's CI module and tests; it is absent from package barrels and shipped types. If the seam is wrong, changing it costs the gate call sites and this test file, not a consumer migration.
VERDICT: clear
No theme target, token, CSS variable, component style contract, or painted element changes. The code builds existing theme packages and reads the same built component maps the gate already consumed. Artifact-present and recovery CI runs both produce the same 16-shot gate plan and a passing verdict.
VERDICT: clear
BEHAVIOR: intentionally changed only on the failure path — a missing dists artifact now triggers package builds instead of aborting before the gate; artifact-present behavior remains zero-build API: no public API or package export changes VISUAL: no product render, DOM, CSS, geometry, color, or story changes THEME: no target, token, override, or generated theme meaning changes
VERDICT: clear
EFFECTS: zero; no React code RENDER: no product runtime path LISTENERS/OBSERVERS: zero LAYOUT: no browser layout or paint work BUNDLE: no dependency or shipped package change
The count is the primary evidence: artifact present = 0 builds; Core missing = 1 build; all dists missing = 9 builds (Core plus eight themes). On the real 2-core Actions runner, the forced recovery's build lines span about 48 seconds before the 16-shot gate; the artifact-present control emits no fallback lines. On this M5, the clean exact-head recovery took 101.46 seconds and the Core-only recovery 64.65 seconds; those clocks are environment color, not an assertion. Each build has a 300-second timeout. The added cost is confined to the path that previously crashed, so no healthy PR pays it.
VERDICT: clear
VISUAL CHECK: not applicable
WHY: all three changed paths are CI workflow/script/test files under .github; they do not alter a component, story, DOM, CSS, theme output, or rendered product path. Exact-head pr-visual skipped because no component changed, which is the workflow's intended scope. The historical forced 16-shot runs validate gate behavior, not pixels for this review, so no manual frame is claimed or required.
VERDICT: clear
No interactive element, accessible name/state, focus/keyboard behavior, user-facing or AT-facing string, locale formatter, directional glyph, or logical layout is reachable from this workflow-only diff. Exact-head pr-a11y and pr-rtl skipped because no component changed; that is expected rather than evidence of a pass. No browser check is applicable.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable |
| A11Y & I18N | clear |
GOAL: met — both parent failure shapes reproduce from explicitly missing dists; exact-head recovery loads all seven shipped themes after the required builds; the artifact-present path emits zero builds; and the real forced Actions run reaches a 16/16 passing gate.
DISPOSITION:
- Unit coverage does not model the exact #5482 state: the “Core missing” case starts with the theme missing too, and its generated theme module does not import Core (
sources.test.mjs:73-84). → accepted as an optional nit. The exact state was independently driven with the real built Butter theme and default Core build, so merge safety does not depend on the fixture. - Direct Prettier checks flag line wrapping in both changed
.mjsfiles. → accepted as mechanical maintainer polish. Exact-head lint is green and this has no runtime consequence; mention it as an optional nit rather than hold a working CI repair.
ADVICE: proven remedy — keep the theme fixture prebuilt in the Core-missing case so the test reaches the same import failure as #5482; run the repository formatter on the two .mjs files. Both remedies were verified locally and change no public shape.
AUTHOR CAN PROCEED: yes — both asks are optional, mechanically bounded, and require no withheld API, design, ownership, or cost decision.
WORST OUTCOME: “A fully missing artifact adds nine serial package builds before the same visual plan” → clear for this fix because only the path that previously failed pays it, while the normal path remains zero-build. No new builder or end-user failure was found.
JUDGEMENT NEEDED: none
comment — semantic approve; GitHub cannot accept the author's self-approval
- [not blocking] The Core-missing test proves build order but not #5482's exact prebuilt-theme import state.
→ a future regression could make the test stay green while built themes still fail to resolve Core; the real-state reproduction passes today ·
.github/scripts/visual-gate/lib/sources.test.mjs:73 - [not blocking] Both changed
.mjsfiles fail the repository's direct Prettier check. → maintainers see inconsistent wrapping in executable CI code; runtime is unaffected ·.github/scripts/visual-gate/lib/sources.mjs:60
Thanks—this is an approve. The clean-tree fallback works; the fast path stays zero-build. Nits: model #5482's prebuilt-theme state in its test, and run Prettier on both .mjs files.
[Reviewed by Robohands]
None. Both optional nits fit the summary and neither benefits from a hunk-level thread.
- The exact-head unit test passes 4/4; exact-head CI's test, build, lint, docsite, Storybook, sandbox, smoke, dependency, and theme-layer jobs are green.
- The recovery CI run's overall workflow was canceled by a later push, but its
pr-visualjob completed successfully; the executable bytes match the exact head except for a comment wording change. - The action version tags are mutable, but unchanged by this PR and outside its behavioral delta.
TIME total 17m setup 3m trust review, fresh wiki clones, exact-head worktree, clone-install; warm main reused: no (non-rendering CI review) reading 4m description/test plan/code, prior repairs, current main, workflow privileges, kit and rulings measuring 7m two parent failures, clean recovery, Core-only recovery, happy path, 4 focused tests, formatter check, CI-log verification writing 3m draft, two critic passes, wiki record waste 1m first Core-missing assertion matched the symlink-resolved path too narrowly; rerun corrected only the harness assertion
- Exact-head
pr-visualend to end on GitHub Actions because a workflow-only PR intentionally skips it. The forced recovery ran functionally identical executable code (the only later source change was comment wording), and the exact head was driven locally from the two missing-dist states. - No manual visual frame: this is a genuinely non-rendering CI path, so visual evidence is not applicable rather than unverified.
Not posted. Public actions were explicitly forbidden. The proposed comment above is the exact text for a later authorized comment.
Pass 1 — two FAILs, verdict unchanged. Verdict check was correct: maintainer-owned, no shipped defect, semantic approve with a mechanical comment. The 26-word review passed the 30-word cap, carried the exact #5482 link, contained no private merge syntax or human-hold phrase, and ended with the required signature. It failed R12b because it opened with “Approve” rather than thanks. The Prettier finding cited sources.mjs:59, which names the function but not the unformatted line, so R14e failed the anchor.
Rewrite. Opened with thanks and kept the semantic verdict explicit; repointed the formatter anchor to the actual long statement at sources.mjs:60.
Pass 2 — PASS. The rewritten comment is 28 words, starts warmly, says this is an approve, keeps both nits visibly optional, and contains no unsupported remedy. The two anchors were reopened at the exact head and contain the described test and formatting lines. JUDGEMENT reads the trust, failure-mode, test-seam, and resource evidence rather than only its verdict table. No blocking finding exists, so no second block confirmation is owed.
# In the dedicated exact-head worktree, after the safety review:
python3 ~/astryx/tools/fast-install.py <worktree>
# Parent clean-tree failure; loadThemeOverrides throws the Butter missing-dist error.
git checkout --detach 41494fcd41310c819f0501df2e798235c4b41135
rm -rf packages/core/dist packages/themes/*/dist
node --input-type=module -e "import('./.github/scripts/visual-gate/lib/sources.mjs').then(m => m.loadThemeOverrides(process.cwd()))"
# Exact-head recovery; builds Core + eight themes, then loads seven shipped themes.
git checkout --detach 765b432ad0cf5158686901f947378fff080edfa2
rm -rf packages/core/dist packages/themes/*/dist
node --input-type=module -e "import('./.github/scripts/visual-gate/lib/sources.mjs').then(m => m.loadThemeOverrides(process.cwd())).then(x => console.log(Object.keys(x)))"
# Focused unit and formatting checks.
npx --yes pnpm@11.10.0 exec vitest run .github/scripts/visual-gate/lib/sources.test.mjs
npx --yes pnpm@11.10.0 exec prettier --check .github/scripts/visual-gate/lib/sources.mjs .github/scripts/visual-gate/lib/sources.test.mjs .github/workflows/ci.yml