Skip to content

Review 5565

Jump to bottom
Cindy Zhang edited this page Aug 27, 2026 · 2 revisions

#5565 — fix(ci): stop shipping template-assets into every PR preview

cixzhang · open · view on GitHub

Verdict: COMMENT — semantic approve (drafted; GitHub cannot accept the author's self-approval, and no public action was authorized on this run)

HEAD REVIEWED: e50cb76515d4a343efe3915dc82af6fac8b0b254 — every claim below was verified at this commit. Single commit; head re-fetched at the end of the run and unmoved. Merge-base 408cef2192f8ce6bbce31d6bfd2c1a9b39ab7053.

Versions. Review Loop 1.5.0 · Component Audit Rubric 1.12.

Bucket: the maintainer. The author is cixzhang, who is also in .github/ENGOWNERS. This remains a real review written to another person; an accepted verdict must be posted mechanically as a comment because GitHub rejects self-approval.

LANE: full. The diff is entirely .github/workflows/**. A trust/CI surface is a mandatory full-lane promotion regardless of size, and this one also carries a delete path against shared state (gh-pages). Fast was never opened.

Risk class: LOW-RISK, with one behaviour change noted. No new API surface · no performance or resource regression (every measured number moves down) · nothing got bigger · one behaviour widening (top-level dotfiles are now copied), driven on a fixture and inert on the real export.

Description → test plan → code. The body states the failure with the exact artifact size, separates the size problem from the 404 problem, quotes a Chromium run showing the media is unreachable, and puts the real fix explicitly out of scope. The test plan offers three things: actionlint before/after, a fixture exercise of the copy logic, and "real behaviour is CI on this PR". Each was re-run here rather than taken on trust. Code inspection followed that framing.

Prior review: none. pulls/5565/reviews → 0 and pulls/5565/comments → 0. There is no colleague's conclusion to extend, narrow, or contradict, and no standing review of our own to reconcile. The only PR comments are the Vercel bot and the PR Analysis Report.

Main did not invalidate this. origin/main has advanced 18 commits since the merge-base, and git log <base>..origin/main -- <the three workflow files> is empty. The mechanism reviewed still exists exactly as reviewed.

Commit type vs diff: honest. fix(ci): claims a CI repair with nothing consumer-visible. The diff is three workflow files, 0 paths under packages/, and 0 changesets — and check-scope and lint are green on this head, so the changeset gate agrees.

STEP 0 — trust review (done before any execution)

gh pr diff 5565 --name-only → three files, all .github/workflows/*.yml. All three were read in full at head before anything was run. Head repo is facebook/astryx, not a fork.

what could change trust result
triggers (on:) unchanged in all three — diffing the on: / permissions: / checkout / ref: / secrets. lines between origin/main and head is empty for each file
pull_request_target introduced no — none of the three uses it
top-level + job permissions: unchanged: permissions: {} top-level; jobs keep contents: write (+actions: read, +pull-requests: read)
PR-head checkout added no — cleanup-previews.yml:65 is still ref: gh-pages and nothing else; neither deploy workflow checks out anything at all
PR code executed no — both deploy paths still only copy pre-downloaded artifacts
new secret / token use no — secrets.GITHUB_TOKEN only, at the same call sites
new action, or version bump no
artifact provenance unchanged: sandbox-<hash> / storybook-<hash> from the same run-id, still name-validated — deploy-preview.yml:70 if ! echo "$PR_NUMBER" | grep -qE '^[0-9]+$'; then, with the ^[0-9a-f]{7,40}$ hash guard beside it

Nothing PR-authored was executed. All reproduction below is static or read-only: actionlint/shellcheck over the YAML, a synthetic fixture exercising the copy line, GitHub read APIs, and HTTP GETs.

VERDICT: clear — no trust-boundary change

PROBLEM

WHY 1: On 2026-08-26 01:04 UTC the github-pages deploy job errored — ##[error]Artifact could not be deployed. Please ensure the content does not contain any hard links, symlinks and total size is less than 10GB. — after uploading a 7,537,214,270-byte artifact, with ##[warning]Uploaded artifact size of 7537214270 bytes exceeds the allowed size of 1 GB. Deployment might fail. (run 32917561958; build succeeded, deploy failed). That push never published.

WHY 2: When the Pages deploy fails, the site keeps serving the previous deploy. A contributor who pushes and then clicks the Storybook/sandbox link the PR Analysis Report posts gets the old build, or a 404 for a brand-new preview — and nothing on the PR says the publish failed, so they cannot tell whether they are looking at their own change.

WHY 3: That preview link is the only way to see a change rendered without checking the repo out. When it silently lags, review falls back to reading the diff and trusting it — the posture this repo's whole screenshot-and-preview discipline exists to prevent.

USER-FACING PROBLEM: a contributor pushes a fix, opens their own preview link, and sees a stale page or a 404, with nothing telling them the publish failed.

PROBLEM SEVERITY: harmful friction — measured, not asserted. Over 2026-08-24T19:52Z → 2026-08-26T23:39Z there were 110 completed pages deploys: 109 success, 1 failure (plus 267 cancelled-superseded). Nobody is blocked — the next deploy publishes minutes later — but the artifact grows monotonically with the open-PR count, which is the argument for fixing it now rather than at the next failure. Deliberately not inflated to broken task: no evidence anyone could not complete a review, and the site never went down.

VERDICT: clear

SOLUTION

A preview is built by copying two already-built folders into a per-pull-request directory on the published site. One of those folders carries a large set of demo photos and videos, and the pages that show them ask for those files by an address that ignores which preview the page is in — so the copy sitting next to the page is never the one requested. The change stops copying that folder into new previews, and deletes the folders earlier copies already left behind. The published site therefore carries one set of the media in total instead of one set per open pull request, and no page loses anything, because no page was reaching the per-preview copy in the first place.

SOLUTION (2 decisions · ~21 executable lines of +33/−2; 11 of the added lines are comments)

  1. Preview assembly skips sandbox/template-assets/ — deploy-preview.yml:154 find sandbox-dist -mindepth 1 -maxdepth 1 ! -name template-assets \, and redeploy-preview.yml:130, byte-identical. → stated problem: "125 MB of each 196 MB preview is sandbox/template-assets/". The automatic and manual paths assemble a preview identically, so they cannot ship apart — one decision, not two.
  2. Cleanup sweeps the copies earlier deploys already pushed — cleanup-previews.yml:163 for ta in pr/*/sandbox/template-assets; do. → stated problem: "so the size drops on the next run instead of after 41 PRs redeploy".

Both trace to a problem written down in the body. No unrecorded decision.

BURDEN: low — no runtime state, no Effects/listeners/timers/observers, no public surface, no dependency, no new trigger, permission, token, or action. Two workflows lose a copy; one gains a bounded for loop that reuses the file's existing DELETED_PATHS replay contract (cleanup-previews.yml:265) rather than inventing a second deletion path.

BURDEN MATCH: proportionate — a delete-only change against a size problem, with the deletion proven unreachable before it is made.

VERDICT: clear

ARCHITECTURE

  • OWNER: deploy-preview.yml / redeploy-preview.yml own what a preview contains; cleanup-previews.yml owns garbage-collecting gh-pages. The change sits in both owners and nowhere else.
  • TIER 1: the existing CI artifact contract — ci.yml:386 name: sandbox-${{ … short_hash }}, ci.yml:387 path: apps/sandbox/out/ — and the existing gh-pages clone/push-retry contract. No new mechanism, registry, or protocol.
  • TIER 2: none
  • SEAMS: automatic deploy (workflow_run on CI) · manual re-deploy (workflow_dispatch) · cleanup (workflow_run + daily cron + dry-run dispatch) · cleanup's push-retry replay path
  • BEHAVIOR UNIT: inline shell — correct here. Each change is a few lines inside the step that already owns that tree; a composite action for one find and one for would be indirection, not architecture.
seam driven result
automatic deploy — copy line works. Fixture: template-assets/ excluded; _next/ (underscore-prefixed), nested dirs, empty dirs, symlinks and top-level files all still copied
manual re-deploy — copy line works. Byte-identical to the automatic path, so the fixture covers both
cleanup — sweep, previews present works. Glob pr/*/sandbox/template-assets + [ -d "$ta" ] || continue matches only real dirs
cleanup — sweep, no matches works. No nullglob, so the literal pattern is tested by [ -d ] and skipped; the loop is a no-op after the first sweep
cleanup — PR already deleted as closed works. The closed-PR block runs first (:147 git rm -rf --quiet "pr/${pr_num}" removes the working-tree files), so [ -d ] fails and no path is queued twice
cleanup — push rejected, retry replay works. Swept paths are appended to DELETED_PATHS and replayed at :265 with --ignore-unmatch, which tolerates paths already gone upstream
cleanup — gh pr list fails works. The guard at :137 empties PR_DIRS so no live preview is mass-deleted; the template-assets sweep sits outside that guard and is still safe, because it deletes unreachable media rather than a preview
cleanup — dry run works. git rm is skipped and the script exits at :238 before committing
working directory on retry works. cd "$WORK_DIR" (deploy-preview.yml:142, redeploy-preview.yml:118) precedes the find, so relative sandbox-dist still resolves on attempts 2–5

Is this THE fix, or A fix? The root cause is upstream: scripts/sync-templates.js:258-265 — fs.cpSync(TEMPLATE_ASSETS_SRC, TEMPLATE_ASSETS_DEST, {recursive: true}); — mirrors the 120 MB docsite set into apps/sandbox/public/template-assets at generate time, so every sandbox-<hash> artifact carries it and so does the published /astryx/sandbox/. An unevadable fix would live there, or in the references. This PR fixes it at the deploy call sites instead — and that is a step, not a detour: it freezes no surface, is one revert away, and the follow-up the PR itself names (point previews at the single /astryx/sandbox/template-assets/ copy) requires that shared copy to keep existing, which deleting the mirror would remove. Not a finding.

The next place the same failure can happen. Searched for a second instance of the shape "large root-absolute asset dir shipped per preview, unreachable under the basePath": besides the mirrored set, apps/sandbox/public/ holds only avatars/ (84 KB) and templates/ (172 KB), and the 31.29 MB templates/ in the deployed sandbox is exported HTML that IS served (/astryx/pr/5567/sandbox/templates/AvatarShowcase/ → 200). There is no second instance. The shape is genuinely closed on this axis, which is the tell of a scoped fix rather than a patch.

VERDICT: clear

IMPACT

End users of apps built with Astryx: nobody. No published package byte changes; the diff touches no packages/** path.

Contributors and reviewers clicking a preview link: nothing they can see changes. The 125.17 MB being removed is already unreachable at the address the pages request. Driven live at review time, read-only GETs:

URL code
/template-assets/DATA-Ana-Thomas.png — what a template actually requests, under any basePath 404
/astryx/pr/5567/sandbox/template-assets/DATA-Ana-Thomas.png — the per-preview copy this PR stops shipping 200 (present, and nothing asks for it)
/astryx/sandbox/template-assets/DATA-Ana-Thomas.png — the one shared copy the follow-up would point at 200 (exists today: 101 files, 125,170,054 bytes)
/astryx/pr/5567/sandbox/templates/AvatarShowcase/ — the page that renders them 200 (renders; its <img>s are the ones that 404)

Mechanically why: the sandbox is exported with basePath = /astryx/pr/<n>/sandbox — apps/sandbox/next.config.mjs:33 basePath: process.env.SANDBOX_BASE_PATH || '', fed by ci.yml:380 SANDBOX_BASE_PATH: ${{ steps.urls.outputs.sandbox_base_path }}, and ci.yml:378 says it in the file itself: "Built without the /<repo> prefix, the sandbox's root-absolute asset URLs…". Next's basePath does not rewrite a literal string in a src attribute. All 168 template-assets references in the repo are the root-absolute /template-assets/ form; zero are relative — so not one of them can resolve to the sibling copy.

What landing this newly exposes: nothing. The demo images are already broken on the published sandbox and in every preview, and are exactly as broken afterwards. This is the rare case where fixing half a surface exposes nothing, because the half left alone was never visible. The PR says so itself under "Out of scope", which is the right place for it.

VERDICT: clear

API

No API change. The complete changed-path set is three .github/workflows/*.yml files; gh pr diff --name-only | grep -vc '\.yml$' → 0. No published package, barrel, prop, export, type, or default is touched, and no consumer can be broken because nothing consumable moved.

OSSIFICATION: nothing is added that could freeze. The find expression and the sweep loop are local to the step that runs them; neither is a name, contract, or seam anything else can reach for. No new concept, so nothing to escalate.

VERDICT: clear

THEMING

n/a — structural only, no styles.

grep -nE "#[0-9a-fA-F]{3,8}|rgba?\(|hsla?\(|boxShadow|light-dark\(|stylex\.|xstyle" <3 files>  → 1

The single hit is cleanup-previews.yml:9 — an issue reference inside a comment, not a colour. Real hits: 0. No theme target, token, variable, or style contract is added, changed, or removed.

VERDICT: clear

BREAKING

Is there a consumer who can be broken? No published package is touched, so there is no installed consumer by construction. The affected surface is the repo's own preview site.

BEHAVIOR: one reachable change beyond the intended exclusion, found by running the two copy lines against a fixture rather than reading them:

OLD  cp -r sandbox-dist/* dest/
NEW  find sandbox-dist -mindepth 1 -maxdepth 1 ! -name template-assets -exec cp -r {} dest/ \;

diff old -> new
  + ./.nojekyll                        <- dotfiles are NOW copied
  - ./template-assets/**               <- the intended exclusion
  (identical otherwise: _next/, nested dirs, empty dirs, symlinks, top-level files)

A shell glob skips dot-prefixed entries; find -mindepth 1 does not. So the copy widens to include top-level dotfiles. Inert today, and checked rather than assumed: apps/sandbox/public/ contains only avatars/ and templates/ (no dotfiles); the Next export emits no top-level dotfile; and gh-pages already carries a root .nojekyll (verified present, size 0) which disables Jekyll for the whole site including _next/, so a per-sandbox one would change nothing even if one appeared.

The PR's test plan says dotfiles are "still copied", which reads as unchanged. They were not copied before. The conclusion (nothing breaks) is right; the wording is off by a step. Not raised in the comment — it would spend the word cap for no reader benefit.

  • API: no — nothing consumable changed.
  • VISUAL: no — nothing rendered by any package changes, and no preview page's DOM changes. Nothing grew: there is no outer dimension to grow, and the only delta is bytes removed from a static host.
  • THEME: no — no target, token, or override is touched, so every existing one still resolves.

VERDICT: note — the copy now includes top-level dotfiles; driven on a fixture, inert on the real export

PERFORMANCE & RESOURCES

EFFECTS: zero. No React, no .tsx, no hook in the diff — the changed set is three .yml files. The mandatory added/changed-Effect evidence gate does not fire.

RENDER: no reachable change — nothing renders. LISTENERS/OBSERVERS: none added or removed. LAYOUT: none. BUNDLE: no dependency change; no published package byte changes.

The resource delta is the point of the PR, so it is measured rather than described.

measured source
per preview, total 196.3 MB median (181.7 MB for the fully-sampled pr/4782) git/trees/gh-pages?recursive=1, blob sizes
per preview, template-assets 125,170,054 bytes, identical in every preview sampled same, plus contents/pr/<n>/sandbox/template-assets
share of a preview removed 64% 125.17 / 196.3
the shared /sandbox/template-assets/ copy 101 files, 125,170,054 bytes contents/sandbox/template-assets?ref=gh-pages
failing Pages artifact 7,537,214,270 bytes run 32917561958 upload log
Pages "allowed size" in that run's warning 1 GB run 32917561958 deploy log
artifact after the sweep, at the PR's 41 previews ~2.41 GB 7.537 − 41 × 0.12517
previews on gh-pages now 29 (the closed-PR sweep has run since the PR was written) contents/pr?ref=gh-pages

What does the new sweep catch, and are the hits real? Run against the live tree: 29 preview dirs, and every one sampled carries exactly 125,170,054 bytes of sandbox/template-assets (pr/5018, pr/5150, pr/5230, pr/5268, pr/5272, pr/5281 all byte-identical). So the first sweep deletes ~3.63 GB across 29 real hits — zero false positives, because each hit is verified unreachable at the address its pages request. This is a migration that runs once and is then a no-op, not a rule that will keep firing.

Reproduction, all read-only:

gh api "repos/facebook/astryx/git/trees/gh-pages?recursive=1"                 # blob sizes; note truncated:true
gh api "repos/facebook/astryx/contents/pr?ref=gh-pages" --jq length           # live preview count
gh api "repos/facebook/astryx/contents/pr/5018/sandbox/template-assets?ref=gh-pages" --jq '[.[].size]|add'
gh run view 32917561958 --repo facebook/astryx --log | grep -iE "exceed|Final size|could not be deployed"

CI cost added: one for loop per cleanup run; after the first sweep it matches nothing and costs a glob expansion. One-time cost: a single commit deleting 29 × 101 files over the same git rm + push-retry path the closed-PR sweep already exercises at the same order of magnitude. Not driven in a real run — that requires merging — and said plainly rather than implied.

Who pays, and is anything measurably worse? Nothing is worse. Every number moves down.

VERDICT: clear

VISUAL EVIDENCE

VISUAL CHECK: not applicable

WHY: a genuinely non-rendering change, proved by the changed paths rather than by a label. The complete changed-file set is .github/workflows/{cleanup-previews,deploy-preview,redeploy-preview}.yml and nothing else — gh pr diff 5565 --name-only | grep -vc '\.yml$' → 0. No package, story, template, theme, or docsite path is touched, so no story exists that could render a difference; the styles grep is 0 real hits and the a11y/i18n grep is 0.

Recorded rather than assumed: pr-visual did NOT run on this head. gh pr checks 5565 reports it skipping (path-filtered), along with pr-a11y and pr-rtl. This review therefore does not claim "visual gate sufficient" — green CI here is not evidence the visual job ran. The obligation is discharged by the non-rendering path proof above, which depends on no gate.

No frames, and the reason is the acceptable one — nothing rendered changes. What frames would have shown is instead carried by the live HTTP status table in IMPACT, which is the right sensor for "is this byte reachable"; a screenshot cannot answer that question and a status code can. No sensor receipt is owed because no frame was captured.

VERDICT: clear

A11Y & I18N

n/a — structural only.

grep -nE "aria-|role=|useTranslator|t\('@astryx" <3 files>  → 0

No string, role, ARIA attribute, focus path, keyboard path, or rendered element exists in the diff, so §1 and §9 have nothing to reach. No preview page's DOM changes, so the published previews' own accessibility is untouched — the change removes files nothing links to. pr-a11y and pr-rtl report skipping on this head; recorded as did not run, never read as a pass.

VERDICT: clear

STEP 7 — the five checks, run on the draft

7a — every decision traces to a sentence in the body saying why the old behaviour was wrong. Not "none" for either: decision 1 → "Under a preview basePath of /astryx/pr/<n>/sandbox, /template-assets/* resolves to facebook.github.io/template-assets/*, not to the copy sitting next to the page. Verified in Chromium — every one of them 404s"; decision 2 → "sweep the copies earlier deploys already pushed, so the size drops on the next run instead of after 41 PRs redeploy". No change in the diff is unargued, so there is no unrecorded-change finding and nothing to split.

7b — does the closing line match the verdict? The verdict is approve; the review's last sentence is the single nit, explicitly labelled Nit:. No design question is left as the last thing read.

7c — can the author tell what is meant? One finding, anchored to the body's own Expected: … Pages artifact 7.5 GB → ~2.4 GB line. Nothing is expressed as a count.

7c2 — grep the diff for the fix about to be asked for. sync-templates → 0 hits, upload-pages-artifact → 0, NEXT_PUBLIC_BASE_PATH → 0. The author did not already do the upstream fix, so the observation stands — and it is not asked for, because it is out of scope by the PR's own statement and would be a detour.

7d — do the findings share a parent? Early hypothesis, written before the slots: "every finding will be a child of — the fix is at the deploy step rather than at the build step." The slots refuted it, and that is worth saying plainly rather than defending: the deploy-step placement turned out to be the right seam, because the named follow-up depends on the shared copy that removing the mirror would delete. The two surviving notes — residual artifact size, and dotfile widening — share no parent with each other and neither is a child of the placement. No cluster, so no redirect.

JUDGEMENT

slot verdict
STEP 0 trust clear — no trigger, permission, checkout, token, or action change
PROBLEM clear
SOLUTION clear — 2 decisions, both traced
ARCHITECTURE clear
IMPACT clear
API clear — no API change
THEMING clear — n/a, structural
BREAKING note — dotfile copying widens; inert
PERFORMANCE clear — zero Effects; 29 real hits, zero false positives; every number moves down
VISUAL clear — not applicable, non-rendering paths proved
A11Y & I18N clear — n/a, structural

Read the bodies, not the labels. Three things only visible from the prose, none of them in a verdict line: (a) the only sentence anywhere describing someone worse off sits in BREAKING, and the fixture plus the real export show that state is unreachable; (b) nothing is filed in the wrong slot — the dotfile widening was noticed by driving the copy line and belongs to BREAKING, and the residual size is a GOAL fact rather than a PERFORMANCE regression, which is why it is carried in GOAL and not laundered into a note; (c) the two notes do not compound — one is inert, the other is a residual of a pre-existing condition that this change reduces.

GOAL: partly met. The stated dead weight is provably removed — 125,170,054 bytes per preview, 64% of each, across 29 live previews, verified unreachable by a 404 at the address the pages request. The stated failure is not provably gone: at the PR's own projection of ~2.41 GB the artifact is still 2.4× the 1 GB allowed size that the failing run's own warning names, so Deployment might fail still applies. The uncovered member is the remaining ~71 MB × N of storybook + sandbox per preview; it can follow separately because it needs a different lever (retain fewer previews, or stop publishing a per-PR sandbox), not a larger version of this change.

DISPOSITION:

  • Artifact still above the 1 GB allowed size after the fix → accepted, non-blocking, and said in the posted review. Who pays: the same contributor, on the next occurrence. Why acceptable: the change strictly reduces the exposure (−68%) and introduces none of it, and the residual is pre-existing and not this diff's to carry. Not accepted silently — a reviewer may not pocket a cost decision, so it is in the comment rather than only here.
  • Copy widens to top-level dotfiles → accepted. Inert on the real export (checked, not assumed), and the root .nojekyll makes it harmless if it ever fires.
  • Test plan says dotfiles are "still copied" → accepted. Conclusion correct, wording off by a step; not worth a round trip or a word of the cap.

No orphan findings: every negative fact stated anywhere above appears exactly once here.

ADVICE: omitted, deliberately. The finding is proven and the acceptance criterion is stateable without designing anything, so no remedy is prescribed. The next lever has not been built here, and an unbuilt fix is authorship rather than review; a specific prescription would anchor the author on a guess and buy nothing.

AUTHOR CAN PROCEED: yes — nothing is withheld and no acceptance criterion is imposed. The change is complete as scoped, every verifiable question is settled above, and the single nit is informational: a fact for the body, not work that must land before merge. No decision was reserved, so there is nothing for the author to wait on.

WORST OUTCOME: "at the PR's own projection of ~2.41 GB the artifact is still 2.4× the 1 GB allowed size the deploy's own warning names, so Deployment might fail still applies" → compatible with approve. That sentence describes a residual of a pre-existing condition which this change reduces by 68% and does not create. No slot contains a sentence saying a person is stuck, cannot reach something, hears nothing, loses their place, or loses data.

JUDGEMENT NEEDED: none — a size fix against an existing contract. No new surface, no new concept, no uncovered design choice, and no measured degradation to accept. The language gate was run over the draft: no occurrence of "maintainer's decision", "Cindy's call", "not yours to own/decide", "we'll decide", or "ours to call".

approve and merge (semantic verdict). Mechanically this posts as COMMENT: the author is cixzhang and GitHub will not accept a self-approval, exactly as on Review 5514. No public action was authorized on this run, so nothing was posted.

  1. [not blocking] A contributor who pushes and opens their own preview link gets a stale page — or a 404, if the preview is brand new — with nothing on the PR saying the publish failed. That is the same outcome as before this change, on the next Pages deploy that trips the limit. → because the projected ~2.41 GB artifact is still 2.4× the 1 GB allowed size named in the failing run's own warning; the PR's Expected line stops one step short of saying so · PR body, "## Change"

Consequence-first, per the standing impact requirement: the person, the state they are in, and what they get instead lead the finding; the mechanism and the anchor follow the arrow. This inverts the usual template order deliberately.

REVIEW

Thanks—this is an approve. Verifying the 404s first makes this safe. Nit: contributors can still get stale or missing previews because ~2.4 GB exceeds the 1 GB limit.

[Reviewed by Robohands]

29 words, cap 30. Revised under the standing impact requirement (see Round 2); the earlier 28-word wording is preserved there. No code block and no closing question — approve rules. No [Full review] link: the brief omits it on the maintainer's own PR. No Discord clause: approve, internal author.

INLINE

None. The one finding is about the body's Expected line rather than a line of code, and the dotfile note does not benefit from a hunk-level thread.

EVIDENCE NOT SPENT

  • actionlint (with shellcheck) over the three files: 13 findings at origin/main, 13 at head. Diffing the two sets with line numbers stripped leaves only three entries that moved line (cleanup-previews SC2034 119→142, deploy-preview SC2086 23→27, redeploy-preview SC2086 20→24). The PR's test-plan claim is exactly true — confirmed rather than taken on trust. The SC2086 hits are the pre-existing unquoted ${PR_NUMBER}, which the ^[0-9]+$ guard already makes safe and which is not this diff's to fix.
  • compact-gh-pages.yml compacts gh-pages history, not tree size, so no existing mechanism already reached this problem — the change is not a duplicate seam.
  • The gh-pages tree API response is truncated: true at 57,584 entries, so the site totals come from 13 fully-sampled previews; the per-preview figure the arithmetic rests on (125,170,054 bytes) is exact and was re-confirmed against six more previews through the contents API.

TIME

TIME  total 21m
  setup       2m   fresh fork-wiki clone, public-wiki clone, exact-head worktree (warm main reused: no — non-rendering CI review, no Storybook or build needed)
  reading     5m   brief + critic in full, loop mechanics, presentation, evidence templates, Rulings, Running-It, repo rubric from main, all three workflows at head
  measuring   6m   trust diff, gh-pages tree + contents APIs, failing-run log, deploy-history stats, 6 live HTTP probes, copy-logic fixture, actionlint before/after, anchor verification, CI check state
  writing     8m   gate-1 draft, critic pass 1, gate-2 rewrite, critic pass 2; then round 2 — impact-rule re-critique, six candidate comments word-counted, gate-3 rewrite, critic pass 3
  waste       1m   three commands re-run: the six HTTP probes (curl options passed through an unquoted variable, so every probe errored and the batch was repeated), the structural-only grep (same word-splitting mistake), and one attempt to page the pages-deployment history by workflow id, which 404s because `pages-build-deployment` is a dynamic workflow

WHAT COULD NOT BE VERIFIED

  • Whether the Pages deploy actually stops failing. That needs a post-merge deploy at the reduced size; it cannot be driven from here, and one intermittent failure in 110 completed deploys is not a baseline a single run could move measurably. This is exactly why GOAL is partly met and not met.
  • The first sweep's real cost — one commit deleting 29 × 101 files, pushed against a concurrently-written gh-pages. It reuses the closed-PR sweep's existing git rm + retry path at the same order of magnitude, but it was not executed.
  • The copy-logic fixture ran under macOS/BSD find; CI runs GNU find on ubuntu-slim. -mindepth, -maxdepth, -name and -exec … \; are POSIX and behave identically for the three properties tested (name exclusion, dotfile inclusion, recursive copy), but the engine differs.

What changed before posting

Not posted. Public actions were explicitly forbidden on this run; only this wiki record was authorized. The REVIEW block above is the exact text for a later authorized comment — revised in Round 2 under the standing impact requirement, with the earlier wording preserved there.

Critic passes

Pass 1 — verdict CORRECT, length PASS, ten artifact FAILs. The verdict check held: the maintainer's own PR, no shipped defect, every slot clear or note, so semantic approve with a mechanical COMMENT. The 28-word review passed the 30-word cap, opened with thanks, carried no rubric id, no private merge syntax, no human-hold phrase, no [Full review] link on the maintainer's own PR, no Discord clause on an approve, and ended with the required signature. No finding was invented and none was withdrawn. What failed was the artifact around it:

# rule what was wrong
1 R33 ADVICE said "a roadmap call, not a code call" — hold-shaped phrasing in a review that holds nothing. Rewritten as an evidenced omission.
2 R37 / 7b AUTHOR CAN PROCEED imposed an acceptance criterion ("the body says…") the REVIEW never asks for. Now: nothing withheld, nit informational.
3 STEP 7 Gate 1 referenced "STEP 7a below" and had no STEP 7 section — a dangling pointer. All five checks now recorded, including 7d's refuted early hypothesis.
4 R1e No line saying prior review was checked and none exists. R1e is a gate, not a reminder. Added with the counts.
5 R31c Never checked whether origin/main moved under the three files. It advanced 18 commits and touched none of them; now stated.
6 R14e Anchors in ci.yml, sync-templates.js and next.config.mjs were cited but never opened. All 22 anchors opened at head and their line text pasted inline. One was wrong — sync-templates.js:266 is the readdirSync; the cpSync is :265. Repointed.
7 R19 The low-risk class was implied, not stated. Now stated against all four conditions, with the one behaviour widening called out rather than hidden inside "no regression".
8 R1f The fix(ci): type claim was never tested against the diff. Now checked and stated honest.
9 R14b The sweep is an enforcement mechanism; its catch count was measured but not framed. Now: 29 real hits, zero false positives, one-shot migration.
10 R20 waste 0m on a first run through unfamiliar ground is not honest. Three commands were re-run; named specifically.

Rewrite → gate 2. All ten closed. No slot verdict moved, no finding was added or dropped, and the REVIEW text is unchanged.

Pass 2 — PASS. Re-graded against R0–R42 and STEP 7. Verdict still correct. Length still 28/30. Every slot carries exactly one verdict line from the permitted five. No not looked at on a slot the diff touches. WORST OUTCOME quotes rather than paraphrases and is compatible with the verdict. GOAL is partly met, which may approve — only not met may not. Every negative finding has exactly one disposition and no orphan is left in prose. The visual obligation is decided with evidence and explicitly declines the "gate sufficient" route because pr-visual skipped. BREAKING is not "no" and reaches JUDGEMENT. No [BLOCKS] line exists and the verdict is not request-changes, which is consistent. All ten pass-1 items re-checked individually and closed. No new violation and no invented finding. Would she have posted it: yes.

Pattern

Two failure modes across the two passes, both about the artifact rather than the judgement. First: the frame gates get skipped on a PR that feels simple. R1e, R31c, R1f and R19 are each one command, and all four were missing from gate 1 because a three-file CI diff did not feel like it needed a frame. They are cheapest exactly there, and their value is that a clean answer is itself evidence. Second: consistency between JUDGEMENT and REVIEW is checked by no single rule — gate 1 approved while AUTHOR CAN PROCEED quietly imposed an ask the comment never made. The single change to the brief that would prevent the most of this: make STEP 7b read the AUTHOR CAN PROCEED line and the REVIEW's closing line together and require that the asks match, rather than checking the closing line against the verdict alone.

Round 2 — revised under the standing impact requirement

Applied 2026-08-26, after the record above was first pushed (138ee2a). The standing requirement: every comment and every finding states who is affected, in what state, and what breaks or worsens; lead with the consequence; cut findings with no impact. Nothing about the PR was re-reviewed and no new evidence was gathered — this is a revision of the review's delivery, so the head, the versions, the slot verdicts and the measurements above all stand unchanged.

What the rule caught in the posted wording

The comment as first drafted:

Thanks—this is an approve. Verifying the 404s first is what makes this safe. Nit: ~2.4 GB is still over the 1 GB limit, so deploys can fail.

"so deploys can fail" is a statement about the system, not about a person. It names no one, puts nobody in a state, and describes a job going red rather than an outcome anyone experiences. The evidence for the human consequence was already in the record — the PROBLEM slot says it outright: "a contributor pushes a fix, opens their own preview link, and sees a stale page or a 404, with nothing telling them the publish failed" — and the comment simply did not carry it down. That is the exact failure the requirement exists to catch: the consequence was known, written, and then dropped on the way to the reader.

Candidates, word-counted against the 30-word cap

Counted as whitespace-separated tokens, with the em dash in Thanks—this split into two words (the conservative reading).

wording of the nit words verdict
A "~2.4 GB is still over the 1 GB limit, so deploys can fail" 28 fails the rule — system outcome, no person
B "contributors may still get stale or missing previews because ~2.4 GB exceeds the 1 GB limit" (the suggested shape, verbatim) 29 passes — who, what worsens, consequence-first
C B with may → can 29 chosen
D C plus "after pushing" 31 over cap
E D minus "or missing" 29 passes, but drops the 404 half
F C plus state, unhedged "still get" 30 at cap, and inaccurate — see below

The suggested shape passes the rule as written. One word changed, and one rejected:

  • may → can. "May" is the hedge R2k names — it softens without adding information. "Can" is the honest modality: this outcome is reachable, not certain.
  • can still get was NOT tightened to still get (candidate F). Unhedged, it claims the outcome happens on every push. It does not: the measured rate is 1 failed deploy in 110 completed over the sampled window. Directness may not buy accuracy, so the modal stays.

Why the state is implicit rather than spelled out

The requirement asks for the state, and the explicit form — "after pushing" — costs two words that put the comment at 31 (candidate D). Buying it means dropping either "or missing" (the 404 a brand-new preview returns, which is the more visible half of the outcome) or part of the mechanism. Neither trade is worth it, because the state is already carried by the sentence: a contributor only gets a preview by opening one after a push, so "can still get … previews" names the reachable state without spending the words. The fully explicit form lives in the JUDGEMENT finding above, which has no cap.

Saying this plainly rather than claiming the cap was met without compromise: the comment names who (contributors) and what worsens (a stale or missing preview) outright, and implies when. The record carries all three.

The cut test, applied to every finding in this review

The rule requires cutting findings with no impact. Each negative fact was re-tested by asking "name the person, the state, and what they get instead":

fact person and state survives as a finding?
Artifact still ~2.4 GB, over the 1 GB allowed size a contributor, after a push, on a deploy that trips the limit → a stale page or a 404 yes — it is the one nit in the comment
The copy now includes top-level dotfiles nobody. No dotfile exists in the export; a root .nojekyll already covers the site no — demoted to recorded evidence
Test plan says dotfiles are "still copied" nobody. A wording imprecision in the body with no reachable outcome no — stays cut, now for a stated reason

The two demoted items keep their place in BREAKING and DISPOSITION as evidence, because they were found by driving the copy line and a future reader should not have to rediscover them. What changes is their status: they are observations, not findings, and neither may ever reach a comment. BREAKING's verdict stays note — a note records a non-blocking fact, and that is exactly what it is; a slot verdict is a floor and this round lowers nothing.

Critic pass 3 — PASS

Graded the revised comment and the revised finding only; the untouched slots keep their pass from round 1.

  • The impact rule — the comment names the person (contributors) and the worsening (a stale or missing preview) and leads with them, with the mechanism trailing behind "because". The JUDGEMENT finding names person, state and outcome in its first sentence. Pass.
  • R2 length — 29 words against a cap of 30. Pass.
  • R12b — opens with thanks, before anything critical. Pass.
  • R2k — the nit starts at the subject and states the outcome; no "might be worth", no "smaller than it sounds". Pass.
  • R2j — no sentence restates the author's own mechanism back at them. Pass.
  • R12d — "previews", "1 GB limit" and "404s" are all things a reader who has not read the diff can identify. No loop shorthand. Pass.
  • R12c — no rubric id, grade, or severity. Pass.
  • R2c — signature present and correct ([Reviewed by Robohands], drafted unedited).
  • R31 / R33 — no private merge control field; language gate re-run over the new text, no hold-shaped phrase. Pass.
  • 7b — closing line versus verdict — the last sentence is still the single nit, still marked Nit:, and it is still informational rather than an ask. It remains consistent with AUTHOR CAN PROCEED: yes, which is the pairing that failed in round 1 and must not regress. Pass.
  • R16g — no [BLOCKS] finding exists, so no second confirmation is owed. Pass.

No finding was invented and none was withdrawn. Would she have posted it: yes.

What did not change

The semantic verdict is approve and merge, and mechanically it is still COMMENT, because the author is cixzhang and GitHub rejects a self-approval. Every slot verdict, the GOAL: partly met call, all three dispositions, AUTHOR CAN PROCEED: yes, JUDGEMENT NEEDED: none, and every measurement are unchanged. This round changed how the finding is said, not what the review concluded.

Clone this wiki locally