Repository navigation
ci: restore a working CI pipeline (pnpm setup, build order, Node 22, types) - #4
Merged
Merged
Conversation
pnpm/action-setup now hard-errors when the version is given both as the action's `version` input and as `packageManager` in package.json. Since v4 is a floating tag, the action got stricter underneath us and every job died at setup — typecheck, lint, test, and the db-schema drift check have produced no real signal since mid-July. Drop the `version: 9` input at all five sites and let packageManager be the single source of truth. Also bump checkout v4->v7, setup-node v4->v7, and pnpm/action-setup v4->v6, clearing the Node 20 runtime deprecation. The breaking changes in those majors do not apply here: checkout v7 only restricts fork checkouts under pull_request_target/workflow_run, and the setup-node v5/v6 changes concern automatic caching, which these jobs do not use. node-version stays at 20 (the project's documented floor) and the KVM job still queues without a self-hosted runner, which is the intended fail-closed shape (docs/deploy.md 9).
Two pre-existing bugs that the pnpm setup fix uncovered — both were masked for weeks because every job died before running anything. CI had no build step. Cross-package types resolve only through each package's built dist/*.d.ts (no project references, no path mapping) and dist/ is gitignored, so a clean checkout failed with TS2307 on every @pi-managed/* import. Reproduced locally by moving packages/contracts/dist aside; `pnpm build` before typecheck/test fixes it. Added to the typecheck, test, and kvm jobs; lint needs no build and is left alone. SpawnExecutor wrote the payload to child.stdin with no error listener on that stream. When the spawn script exits before reading stdin — which the close handler already treats as a legitimate error result — the pipe closes and the write raises EPIPE, which without a listener becomes an unhandled error that takes the process down instead. Verified with a forced-race repro: without the listener the process dies with EPIPE before the close handler finishes; with it, the intended error result is returned. This crashed the worker in production, not just in tests.
`pnpm -r build` cannot order this workspace. testkit depends on backend for port types while backend keeps testkit as a devDependency, and pnpm resolves that cycle by building testkit first — which fails on any clean tree, since cross-package types resolve only through the gitignored dist/*.d.ts (no project references, no path mapping). This is why CI still failed after adding the build step, and it means `pnpm build` has never worked on a fresh clone either; every local run succeeded only against stale dist artifacts. Build everything except testkit first, then testkit. Verified by removing all packages/*/dist and running the script from scratch: all 7 packages build, typecheck is clean. Documented the constraint in architecture.md 3.2 next to the cycle it follows from.
The embedded Pi agent (@earendil-works/pi-coding-agent) pulls undici 8.5.0, whose engines field is >=22.19.0. On Node 20 it throws "webidl.util.markAsUncloneable is not a function" at import time, which is what was failing the client-extension suites in CI — the declared floor was simply below what a dependency required. Node 20 is also EOL. Raise engines to >=22.19.0, move every workflow to node-version 22, and update the prerequisite wherever it is stated (README, getting-started, deploy x2, user-journeys, spec.md, and the KVM runner notes in ci.yml), recording the reason so the number is not later lowered on a guess.
@types/node was pinned at ^26.1.1 while the runtime floor is 22.19, so typecheck was validating against APIs the target runtime does not have — the same class of gap that let the undici/Node 20 break reach CI. Pin all three declarations (root, worker, billing-adapter) to ^22.20.1. Doing so surfaced one real error: fileStream() in jsonl-sync typed its data handler as (chunk: Buffer), but Readable's data event is string | Buffer. The stream is created locally with no encoding so chunks are always Buffer; narrow at that boundary and say why.
The large-streaming ObjectStore case asserted with toEqual on a 2 MiB Uint8Array, which vitest walks element by element: 2432ms of the suite's 2551ms, and a timeout against the 5s default on a shared CI runner. Buffer.compare gives the identical byte-exact guarantee natively — the test drops to 9ms. The small-payload case keeps toEqual, where the element-wise diff is cheap and worth having on failure.
Both surfaced on the first backend run in weeks; neither is a weakening. contract.test.ts duplicates the testkit ObjectStore conformance suite by hand rather than importing the kit, so it carried the same 2 MiB toEqual that timed out the 5s default — here three times over (Fake, Filesystem, GCS). Same native Buffer.compare fix, same guarantee. Worth noting the duplication itself: the two copies must be kept in sync by hand. The ROB-22 catch-up storm enumerates 5,200 minute-occurrences and inserts 5,200 rows, twice. Its author already flagged it slow and set 60s, which held on developer hardware but not on a 2-core runner sharing the box with the parallel suites. Raised to the package's 180s hookTimeout.
The concurrency test asserted that the entry last in (created_at DESC, id DESC) carries the final balance. Nothing can identify the last-APPLIED entry that way: created_at defaults to now(), which is transaction-START time in Postgres and so does not track the order 40 concurrent transactions take the FOR UPDATE lock, and id is a random newId(). The assertion only ever passed on timing luck; on CI it read 1000100. The ledger itself is correct — appendEntry serializes every credit and debit under FOR UPDATE on tenant_billing, and the two authoritative assertions (final balance 1_000_000, all 41 entries present) both passed. Drop the ordering assertion and its now-dead subquery, recording why it cannot be asserted so it is not reintroduced.
The pool test sampled rtB.status() once, immediately after sendEvent resolved. sendEvent returns when the event is accepted; the turn settles back to idle asynchronously after that, so on a loaded CI runner the sample caught 'running'. Poll with the waitFor helper already used a few lines above for the respawn. This keeps the assertion's strength — a session that never leaves 'running' still fails when waitFor times out at 10s.
Every test file passed but the job still exited 1: vitest caught two unhandled errors, both Postgres 57P01 (admin_shutdown) raised when testcontainers stopped the database while pooled connections were open. The cause is production code, not the tests. createPool never attached an 'error' listener, and pg emits that event on IDLE pooled clients whenever the server drops them — a restart, a failover, or pg_terminate_backend. In Node an 'error' event with no listener is fatal, so a routine Postgres restart would have taken the backend process down. The pool discards the dead client itself and stays usable, and an error on a checked-out client still rejects that client's own query, so the listener hides nothing a caller would otherwise observe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI had been fully red since mid-July. Every job died at the environment-setup step, so the checks carried no signal at all — and that silence was hiding four further defects. This branch fixes them in order of discovery, each one only visible once the previous was cleared.
1. pnpm setup (
010b5dd)pnpm/action-setup@v4began hard-erroring when the version is given both as the action'sversioninput and aspackageManagerinpackage.json.v4is a floating tag, so the action got stricter underneath the repo with no commit here. Droppedversion: 9at all 5 sites;packageManageris now the single source of truth. Also bumpedcheckoutv4→v7,setup-nodev4→v7,pnpm/action-setupv4→v6, clearing the Node 20 runtime deprecation.2. Workspace build order (
a5f8f2a)pnpm -r buildcannot order this workspace:testkitdepends onbackendfor port types whilebackendkeepstestkitas a devDependency, and pnpm broke that cycle by buildingtestkitfirst. Since cross-package types resolve only through the gitignoreddist/*.d.ts,pnpm buildhad never worked on a clean clone — every green local build was running against stale artifacts. The rootbuildscript now builds everything excepttestkit, thentestkit. Constraint documented inarchitecture.md§3.2 beside the cycle it follows from. CI also gained the missingpnpm buildstep before typecheck/test.3. Node floor (
2fbe517)The declared floor (
>=20) was below what a dependency required: the embedded Pi agent pulls[email protected], whoseenginesis>=22.19.0. On Node 20 it throwswebidl.util.markAsUncloneable is not a functionat import. Raised to>=22.19.0and updated all 13 declarations — workflows,README,getting-started,deploy×2,user-journeys,spec.md— recording why, so the number isn't lowered later on a guess. Node 20 is also EOL.4.
@types/nodemismatch + the bug it hid (6801854)Types were pinned at
^26.1.1against a Node 22 runtime, so typecheck validated APIs the target runtime lacks. Aligned to^22.20.1, which immediately surfaced a real error:fileStream()injsonl-sync.tstyped its handler(chunk: Buffer)whereReadable'sdataisstring | Buffer.5. Worker EPIPE crash (
f02efaa)SpawnExecutorwrote tochild.stdinwith no error listener. When a spawn script exits before reading stdin — which theclosehandler already treats as a legitimate error result — the write raises EPIPE and, unhandled, took the process down instead. Verified with a forced-race repro. This crashed the worker in production, not just in tests.6. Conformance test timeout (
b400a2d)toEqualon a 2 MiBUint8Arraycost 2432ms of the suite's 2551ms and timed out the 5s default on a shared runner.Buffer.compareis the identical guarantee natively: 9ms.Result: Typecheck, Lint, and DB Schema Doc green; Test exercising the container-backed backend suites for the first time in weeks. The
KVM testscheck stays permanently queued — that is the intended fail-closed shape (docs/deploy.md§9), not a regression.