Skip to content

chore: document shipped tooling and move actions off Node 20 - #5

Merged
PeterGuy326 merged 1 commit into
mainfrom
chore/4-ci-followups
Sep 10, 2026
Merged

PeterGuy326 merged 1 commit into
mainfrom
chore/4-ci-followups

Conversation

@Bindy-lbb

@Bindy-lbb Bindy-lbb commented Sep 7, 2026 •

Copy link
Copy Markdown
Collaborator

Tracking record

Closes #4

Summary

Two follow-ups from the verification tooling that merged with #3. No change to index.html, so verify going green is the whole acceptance signal.

  1. CHANGELOG.md documented none of the tooling that shipped. main carries the workflow, both check scripts, the manifest and the PR template; the changelog stopped at the page fixes. The commit that added those files staged them individually and left CHANGELOG.md out of the git add, so the entries never left my working tree. CONTRIBUTING.md requires changelog files to be updated alongside a change, so this is a gap against that rule, not a cosmetic one. Entries added for the workflow, contrast-check.js, render-check.js, the pinned manifest, and the PR template.

  2. The workflow's actions targeted the deprecated Node 20 runtime. The first run annotated actions/cache@v4, actions/checkout@v4, actions/setup-node@v4, actions/upload-artifact@v4 as being forced onto Node 24. They still execute, but the warning would appear on every future run. Bumped to current majors — checkout v7, setup-node v7, cache v6, upload-artifact v7 — using only inputs that are stable across those bumps (node-version, cache, path, key, name, if-no-files-found, retention-days).

Verification

CI (verify) must be green; it re-runs the whole suite on this push and, in doing so, verifies the action bump itself.

Local reproduction

All commands run from the repository root. npm ci installs playwright-core but does not download a browser — npm run browser is the separate step, and skipping it fails with Executable doesn't exist at .../chrome-headless-shell rather than anything about a missing install. Confirmed locally by pointing PLAYWRIGHT_BROWSERS_PATH at an empty directory, which reproduces exactly that error and exit 1.

cd /path/to/ordane          # repository root
npm ci                      # installs playwright-core 1.62.1 from the lockfile
npm run browser             # playwright-core install chromium — required, not covered by npm ci
npm run verify              # contrast + render
npm run verify:failclose    # expected to exit 1

Environment used for the run below:

  • OS / arch: Ubuntu 24.04.1 LTS on WSL2, Linux 5.15.146.1-microsoft-standard-WSL2 x86_64
  • Node / npm: v22.23.2 / 10.9.8
  • playwright-core: 1.62.1 (from package-lock.json)
  • Chromium: 151.0.7922.34, as reported by browser.version()

Results:

Command Expected Actual
npm run verify:contrast all pairs pass, exit 0 CHECKED=44 FAILED=0, RESULT=PASS, exit 0
npm run verify:render renders, all reveal blocks trigger, exit 0 REVEALS=9/9, PAGE_ERRORS=none, RESULT=PASS, exit 0
npm run verify:failclose non-zero, proving the guard still bites RESULT=FAIL, exit 1

Nothing CI cannot see changed: no page markup, styles or copy in this diff.

Factual accuracy

No capability claims, pricing or screenshots are touched by this diff.

Risk and rollback

Low. Two files, neither served to visitors. If an action bump misbehaves, CI fails on this PR before merge; after merge, rollback is reverting the merge commit.

Note on branch hygiene

The action bump was originally pushed onto feat/2-publish-marketing-site after that branch had already been squash-merged and deleted, which recreated it as an orphan. That branch is being deleted; this PR is the same change re-done from main through the normal flow.

Two follow-ups from the tooling that merged with #3. No change to the page.

- CHANGELOG.md was missing every tooling entry. The commit that added the
  workflow, the two check scripts, the manifest and the PR template staged
  those files individually and left CHANGELOG.md out of the git add, so the
  entries never left the working tree and main documents none of what
  shipped. CONTRIBUTING.md requires changelog files to be updated alongside
  a change, so this closes that gap rather than being cosmetic.

- The workflow's actions targeted the deprecated Node 20 runtime, which
  annotated the first run and would annotate every future one. Bumped to
  the current majors: checkout v7, setup-node v7, cache v6,
  upload-artifact v7. Only inputs stable across those majors are used
  (node-version, cache, path, key, name, if-no-files-found,
  retention-days).

Verified locally before pushing: verify:contrast 44/0 exit 0,
verify:render RESULT=PASS exit 0 with 9/9 reveal blocks, verify:failclose
exit 1. CI re-verifies the action bump on this push.

Closes #4

@PeterGuy326 PeterGuy326 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no blocking defect in the two-file change. The four upgraded references resolve to official Node24 actions, retain the inputs used here, and executed successfully in the current-head verify run.

Please complete the local reproduction instructions before approval: add your OS/architecture, Node/npm versions, Chromium version and installation step (the existing npm run browser script is available), and state that commands run from the repository root. npm ci with playwright-core does not itself install Chromium.

Independent reproduction on macOS arm64, Node 24.14.1 / npm 11.11.0, playwright-core 1.62.1, Chromium 151.0.7922.34:

  • npm ci: exit 0, pinned version resolved, zero vulnerabilities.
  • npm run verify:contrast: exit 0, CHECKED=44 FAILED=0.
  • npm run verify:render: exit 0, REVEALS=9/9, tab t2, no page or webfont errors.
  • npm run verify:failclose: expected exit 1; both deliberate errors were captured.

These original commands matched the reported results using an existing browser cache. A separate empty-cache diagnostic failed with Executable doesn't exist; this demonstrates the missing setup instruction, not a PR regression. The workflow already installs Chromium, and the page, scripts and package manifest/lockfile are unchanged from base.

Reviewed head: c079e961128282113258af69fba1c07c5c8fc786; base: f796973512184da912dd20368ac553b4cf7f237e. Independent automated review submitted at the maintainer's request. This is a non-approving comment pending the reproduction-instructions correction; no code or workflow change is requested. Please update the description, confirm the documented sequence and request re-review.

@Bindy-lbb

Copy link
Copy Markdown
Collaborator Author

Reproduction instructions corrected — you were right that they were incomplete, and I reproduced the failure you hit before writing the fix.

Pointing PLAYWRIGHT_BROWSERS_PATH at an empty directory and running npm run verify:render fails with exactly Executable doesn't exist at .../chromium_headless_shell-1234/chrome-headless-shell-linux64/chrome-headless-shell, exit 1. So the gap was mine: npm ci installs playwright-core but downloads no browser, and I had omitted the separate npm run browser step.

The PR body now carries a Local reproduction block with the full sequence, stated to run from the repository root:

cd /path/to/ordane
npm ci                      # playwright-core 1.62.1 from the lockfile
npm run browser             # playwright-core install chromium — required, not covered by npm ci
npm run verify
npm run verify:failclose    # expected to exit 1

plus the environment: Ubuntu 24.04.1 LTS on WSL2, Linux 5.15.146.1-microsoft-standard-WSL2 x86_64, Node v22.23.2 / npm 10.9.8, playwright-core 1.62.1, Chromium 151.0.7922.34 as reported by browser.version().

I then ran that exact sequence against a fresh clone with an empty browser cache, rather than asserting it from the warm-cache run:

npm ci                    exit=0
npm run browser           exit=0
npm run verify            exit=0    CHECKED=44 FAILED=0 · REVEALS=9/9 · RESULT=PASS
npm run verify:failclose  exit=1    (expected)

No code or workflow change, as you noted none was needed. Requesting re-review.

@PeterGuy326 PeterGuy326 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review result

APPROVE for the current head c079e961128282113258af69fba1c07c5c8fc786.

The change is limited to the verification workflow action refresh and the corresponding changelog/tooling documentation. The head-matched verify check is green, and the diff does not change the page itself or the verification assertions.

@waterbro-8

Copy link
Copy Markdown

Verification of the supplement only — I did not re-open the code review. This comment carries no vote and is not an approval.

What the 2026-09-08 review asked for, item by item

Each of the five requested additions is now present in the Local reproduction block:

Asked Now in the body
OS / architecture Ubuntu 24.04.1 LTS on WSL2, Linux 5.15.146.1-microsoft-standard-WSL2 x86_64
Node / npm v22.23.2 / 10.9.8
Chromium version 151.0.7922.34, as reported by browser.version()
Installation step npm run browser, with the trap named: npm ci installs playwright-core but downloads no browser
Commands run from repo root stated verbatim, and repeated in the cd line of the recipe

The gap being closed here was documentation, and the diagnosis in the author's 2026-09-08 comment is the right one — npm ci succeeding and then failing with Executable doesn't exist at …/chrome-headless-shell really does read like a broken install rather than a missing step.

What I re-ran myself, on exact head c079e961128282113258af69fba1c07c5c8fc786

Fetched from that head's own blobs, nothing written to the repo:

  • node scripts/contrast-check.js index.html → CHECKED=44 FAILED=0, RESULT=PASS, exit 0. The 44 decomposes as 22 rows in LIGHT + 22 rows in DARK, which is exactly what the new CHANGELOG bullet claims (22 对文字/背景 × 明暗两套共 44 项). Lowest margin in the run is 4.60:1 against a 4.5 threshold — that is building state / amber on band, so the set is not padded with easy pairs.
  • Every command the body documents exists and means what the body says: package.json defines verify = verify:contrast && verify:render, plus verify:contrast, verify:render, verify:failclose, and browser = playwright-core install chromium.
  • playwright-core is an exact pin (1.62.1, no caret) in devDependencies, and package-lock.json resolves node_modules/playwright-core → 1.62.1 from registry.npmjs.org. So npm ci reproduces the version the environment block claims.
  • All four bumped major tags exist upstream as real refs: actions/checkout@v7 → 3d3c42e5aac5ba805825da76410c181273ba90b1, actions/setup-node@v7 → 820762786026740c76f36085b0efc47a31fe5020, actions/cache@v6 → 55cc8345863c7cc4c66a329aec7e433d2d1c52a9, actions/upload-artifact@v7 → 043fb46d1a93c77aae656e7c1c64a875d1fc6a0a.
  • Each new CHANGELOG entry names a file that is actually present at this head: scripts/contrast-check.js (122 lines), scripts/render-check.js (146 lines), .github/PULL_REQUEST_TEMPLATE.md (48 lines), .github/workflows/verify.yml, package.json + lock. And the two CI claims check out against the workflow text: three screenshots (hero.png / runtime.png / ladder.png) with retention-days: 14, and the fail-close step inverts the exit code (if npm run verify:failclose; then ::error …; exit 1; fi).
  • The branch-hygiene note is true: repos/bytefolk/ordane/branches now returns only main and chore/4-ci-followups, so the orphaned feat/2-publish-marketing-site really was deleted.

I could not verify two rows of the results table locally — verify:render and verify:failclose need a browser download, so I did not run them. For those two, the independent macOS arm64 reproduction in the 2026-09-08 review is the evidence, not this comment.

Two nits, neither blocking

  1. These four references are tag-pinned, not SHA-pinned. That is the one place this workflow diverges from the org's own baseline (mem#166 AC-005, landed as mem#167, pins github/codeql-action by commit sha). This PR touches precisely those four lines, so it is the cheapest moment to close the gap. Caveat if someone does take it: don't copy the org template's pin comment style blindly — three of its # v4.37.4 comments describe a sha that is actually v4.37.9, so resolve each tag through git/ref/tags/<tag> (peel it if object.type == "tag") rather than trusting a version annotation.
  2. The documented local step is not the command CI runs. CI does npx --no-install playwright-core install --with-deps chromium; npm run browser is playwright-core install chromium without --with-deps. --with-deps is what pulls the system libraries, so on a bare Ubuntu/WSL2 box the recipe as written can complete the download and still fail at launch on a missing .so — a different failure from the Executable doesn't exist one this round exists to explain, and one a reader would again have to guess at. One sentence saying "locally, add --with-deps (needs sudo) if launch fails on a missing library" closes it.

Status, and what I did not do

Closes #4 is correct — #4 is still open and names exactly these two problems, so merging this PR closes it. verify is green on this head (run 34133918907, attempt 1), mergeable_state=clean, and there is already an APPROVED from @PeterGuy326 on this same head, submitted after the supplement.

I did not submit a review, did not vote, did not merge, did not touch labels or assignees, and did not update this branch.

@PeterGuy326
PeterGuy326 merged commit fb9bca0 into main Sep 10, 2026
1 check passed
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.

CI follow-ups: undocumented tooling in CHANGELOG, deprecated action runtime

3 participants