Skip to content

01: CI runs the repo's checks on push and PR - #11

Merged
T-Crypt merged 7 commits into
oh-my-pocketfrom
improve/01-ci
Oct 13, 2026
Merged

T-Crypt merged 7 commits into
oh-my-pocketfrom
improve/01-ci

Conversation

@T-Crypt

@T-Crypt T-Crypt commented Oct 13, 2026 •

Copy link
Copy Markdown
Owner

CI runs the repo's own checks on every push and pull request. Before this they ran by hand.

What changed

.github/workflows/ci.yml runs npm ci, npm run check, npm run lint, npm run format:check,
and npm test on Node 22.19 and Node 24. Actions are pinned to their current majors. The token is
read-only.

push names the base branches, so a branch with an open PR runs once through pull_request, and a
gh-pages push from site:publish does not trigger a run. package.json gains packageManager,
which setup-node needs before it caches npm.

The job now names the browser it uses. PI_POCKET_BROWSER: /usr/bin/google-chrome picks the deb's
Google Chrome. The image's own chromium is a snapshot build unpacked under
/usr/local/share/chromium and reached through the /usr/bin/chromium symlink, which PATH order
makes findBrowser() return first; it has none of its dependencies installed and no AppArmor
profile, so Ubuntu 24.04's restriction on unprivileged user namespaces kills it as it starts, and
the DevTools pipe closes. That is the failure of run 38350258246, both legs: 628 tests, 491 pass,
137 fail, every browser file dying in its before hook with The browser closed.
PI_POCKET_BROWSER_ARGS: --no-sandbox is the remedy the README already documents for a machine
where sandboxes are unavailable, which a CI runner is.

A step before npm test runs test -x /usr/bin/google-chrome. With PI_POCKET_BROWSER naming a
path that is not a file, findBrowser() returns undefined and every browser test skips itself, so a
job that had lost its browser would go green with 137 tests doing nothing. The guard turns that
silent loss into a red step.

test/visible-models-ui.test.ts counted the picker's router rows as model rows. The picker's
Routers group is rendered as soon as the api("routers") fetch resolves, so the test failed
whenever that won the race, 3 of 5 single-file runs here, with standard on no model yet in the
rows. Its menu() helper now reads .pop-menu-list .pop-menu-row:not(.router-row). Without this the
workflow cannot be green whatever the browser does, so it rides here rather than waiting for ticket
06.

Proof

Run in the worktree on Node 26.11.1:

Command Result
npm ci 244 packages, exit 0
npm run check exit 0
npm run lint exit 0
npm run format:check all files match, exit 0
npm test 628 tests, 628 pass, 0 fail, 0 skipped, exit 0
PI_POCKET_BROWSER=/nonexistent-browser-on-ci npm test 628 tests, 491 pass, 137 skipped, 0 fail, exit 0
node --test test/visible-models-ui.test.ts, five runs after the fix pass 1, fail 0, five times; before it, 3 of 5 runs failed
python3 -c 'import yaml; yaml.safe_load(open(".github/workflows/ci.yml"))' parses; the guard step is in the step list

The second-to-last line is why the fallback is safe before it is needed, and the counts are the
measure that the browser tests ran rather than skipped: on a machine with no browser this suite is
137 skipped and 0 fail.

Review

Clean on the workflow itself. Pinned majors checked against the GitHub API, the Node matrix matches
engines, permissions: contents: read is right, and format:check does cover YAML.

Open

  • The other 15 browser test files have never run on a runner; every one of them was masked by its
    before hook in run 38350258246. If one of them turns out to be runner-specific (fonts, layout,
    timing), the fallback is one line: replace the env: block with
    PI_POCKET_BROWSER: /nonexistent-browser-on-ci and delete the guard step, which is the state
    measured above.
  • --no-sandbox in CI. Reversible in one line; if the deb's Chrome keeps its sandbox on the runner,
    dropping the line is the stronger posture.
  • The message The browser closed. cannot name its own cause, which is why the run above was
    diagnosed from the image's build script rather than from the log. The DevTools pipe's close event
    (cdp.ts:31-33) closes the connection before the child's exit handler (cdp.ts:218-223) can
    report the exit code and the browser's stderr, so the reason a browser died never surfaces. Worth
    a ticket of its own.
  • LAUNCH_ARGS in src/server/browser/cdp.ts never passes --no-sandbox by itself, which is
    correct: the flag comes from the environment, which is what PI_POCKET_BROWSER_ARGS is for.
  • /usr/bin/google-chrome comes from reading the image's build script, not from running the image.
    A wrong path fails loudly at the guard step rather than skipping silently; the cheap correction is
    to drop PI_POCKET_BROWSER and keep only the flag, which then applies to the snapshot Chromium.

Ticket 01. Run folder: ~/handoffs/oh-my-pocket/2026-10-13-pr11-13-rerun/.

Two more fixes, found by running the workflow here

format:check failed on three files the base had left unformatted: .ome/project.json, test/browser.test.ts and test/session-row-ui.test.ts. It failed on both legs in 40 seconds, before the tests could run at all. Whitespace only.

With a browser that launches, one test failed on its teardown: a browser closed just before the test's temporary folder is removed can still be writing its profile, so the removal raced it and the run ended ENOTEMPTY: directory not empty, rmdir .../browser/profile/Default (run 38424667589, 631 tests, 630 pass, 1 fail). test/helpers.ts's cleanUp now retries the removal, which is what Node's maxRetries is for.

The base branch carried three files Prettier would rewrite, so format:check
failed on every run of this workflow: .ome/project.json, test/browser.test.ts
and test/session-row-ui.test.ts. Whitespace only.
A browser closed just before the test's folder is removed can still be writing
its profile, which left the folder non-empty and failed the run with ENOTEMPTY
(run 38424667589, visible-models-ui). The removal now retries.
@T-Crypt
T-Crypt marked this pull request as ready for review October 13, 2026 16:20
@T-Crypt
T-Crypt merged commit 66ecee5 into oh-my-pocket Oct 13, 2026
2 checks passed
@T-Crypt
T-Crypt deleted the improve/01-ci branch October 13, 2026 16:20
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