test: migrate to Vitest + node:sqlite; daemon boots under Node (Bun→Deno step 1) - #2364
Conversation
|
Too many files changed for review. ( Bypass the limit by tagging |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Zhipu)
Model: glm-5.1 | Client: HyperNeo | Provider: Zhipu
Recommendation: REQUEST_CHANGES — one P0 blocker (a test-coverage regression that leaves CI misleadingly green) plus P1 findings. The load-bearing migration code itself is sound and independently verified: the daemon boots under Bun (3s, re-verified locally — no node:sqlite crash) and under Node (the 19 online jobs exercise the Node WS path). The sqlite-compat runtime-dispatch, runtime-server abstraction, lazy Bun.spawn wrappers, FTS5 rowid dedup, undefined→null bind coercion, and vitest shim are all correct, and the deferred #837–#842 runtime items are correctly untouched.
P0 — 45 migration test files (~395 test cases) silently dropped from CI
packages/daemon/vitest.config.ts:16 — include: ['tests/**/*.test.ts'] does not match *_test.ts. scripts/test-daemon.sh:43 (migration_shard_paths) explicitly globs migrations/*_test.ts (clear intent to run them), but Vitest applies the include filter even to explicitly-passed CLI paths, so those files are silently skipped and the shard exits 0 → CI stays green.
Verified empirically:
vitest run <one *_test.ts>→No test files found, exiting with code 1, printsinclude: tests/**/*.test.ts.vitest run <one .test.ts> <one _test.ts>→ runs only the.test.ts(7 tests), drops the_test.ts, exits 0.
Impact: only 19 of 64 migration test files actually run. The dropped 45 hold 395 it/test calls — the very safety net that proves all 167 migrations work under node:sqlite. This is the PR's central purpose and the explicit review focus, and right now only ~30% of that suite executes. It's also a regression: bun:test matched *_test.ts by default, so these ran before this PR.
Fix: include: ['tests/**/*.test.ts', 'tests/**/*_test.ts'] in packages/daemon/vitest.config.ts (and packages/shared, packages/cli for consistency). After the fix, re-run the 4-space-migrations-* shards and confirm the test count jumps substantially.
P1 — --rerun / --show-failures are broken (vitest junit has no file= attribute)
scripts/test-daemon.sh:324 extracts failing files via grep -o 'file="[^"]*"'; :174 uses tc.get('file'). But Vitest's junit <testcase> emits classname (the file path) and no file= attribute (verified: 0 file= occurrences in generated junit). Result: FAILURES_FILE is always empty → --rerun always reports "No previous failures found", and --show-failures prints ?:?.
Fix: parse classname instead of file in both spots.
P1 — Scope: unrelated feature work bundled in (~670 lines)
This PR is scoped as a test/runtime migration, but it carries feature work that belongs in its own PRs:
- GitHub
statuswebhook event —github-normalizer.ts(+90,normalizeGitHubStatus),github-event-extension.ts(+162,handleStatusWebhook+'status'registered at :166),event-essence.ts(+9). ~261 lines of new production event-handling. docs/design/github-events-merge-blocking-spec.md— a 349-line NEW design doc for that feature.- Migration #169 (
message_subtype_normVIRTUAL generated column + index) — a query-perf optimization; its own comment notes it was authored on this branch independently of the migration ("Originally M168 on this branch; renumbered to M169 because dev shipped M168 … in #2343"). Mixing a schema migration into a test-infra PR also complicates the revert story.
(One borderline item — websocket-client-transport.ts connectResolved (+20/-5) — is plausibly exposed by running under Node and could stay; the three above should be split out.)
Verified sound (no action needed)
- sqlite-compat runtime-dispatch split — boots under Bun (3s) and Node; no
instanceof Databaseanywhere; noStatementSyncvalue-import bypass of the coercion. - runtime-server/ (bun + node backends) — both statically importable under either runtime; Node
'upgrade'-event divergence correct; singlefetch(req, upgrade)contract preserved;upgrade()is called synchronously inapp.ts(matches the Node backend's assumption). - Lazy
Bun.spawnwrappers (production/presets/github-connector/pr-ready-validator) — all module-scope Bun defaults made lazy; no remaining import-time crash sites. - FTS5 rowid dedup, undefined→null bind coercion, vitest shim,
undicidevDep,tsconfig types:["bun"]. - Deferred #837–#842 runtime items correctly untouched (CLI still Bun, space-runtime still
Bun.spawn, worktree stillBun.hash, no Deno backend).
Minor (P3, non-blocking): CompatStatement.iterate() in sqlite-node.ts:85 doesn't apply the undefined→null coercion like get/all/run do — unused today, but worth a one-line fix for consistency.
|
Pushed fixes for the review findings (commits ea3023a + 5506fee); replied on the two inline threads. Two more items here since they weren't threaded: P3 (iterate coercion) — fixed in 5506fee. P1 (scope) — I think this is a stale-diff artifact rather than bundled feature work; nothing to split. All four referenced commits are already on
They reached this branch via the |
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (Zhipu)
Model: glm-5.1 | Client: HyperNeo | Provider: Zhipu
Recommendation: APPROVE. All round-1 findings resolved and independently re-verified on 5506fee4b; CI 32/32 green (4 skipped), zero unresolved threads.
- P0 (dropped
*_test.tsmigration files) — fixed & verified.vitest.config.ts:20nowinclude: ['tests/**/*.test.ts', 'tests/**/*_test.ts']. Empirically confirmed:vitest run <a _test.ts>now executes (previously "No test files found"). The 45 previously-skipped migration files (~395 cases) now run in CI — the migration safety net is real. - P1 (
--rerun/--show-failures) — fixed & verified.test-daemon.shnow parsesclassnamein all three junit spots (line 176, 333-334, 352). - P3 (
iteratecoercion) — fixed & verified.sqlite-node.tsiterate()now appliesbindArgs. - P1 (scope) — withdrawn, my error. I had diffed against a stale local
devthat was 4 commits behindorigin/dev. Those four items (#2347 status webhook, #2346 migration #169, #2342 design doc, #2348) are already onorigin/dev; the true net diff vs the PR base is 0 lines for 3 of the files and just thebun:sqlite → sqlite-compatimport repoint for the other two. Squash-merge therefore lands only the migration changes — no rebase required (it would only refresh the Files-changed UI and re-run a green CI).
No outstanding findings. The migration's load-bearing code remains sound (daemon boots under Bun and Node; sqlite-compat runtime-dispatch, runtime-server/, lazy Bun.spawn, FTS5 dedup, undefined→null coercion, vitest shim all correct; deferred #837–#842 untouched). Ship it.
…(Bun→Deno step 1) First step of the Bun→Deno migration. Moves the test suite off bun:test/bun:sqlite onto Vitest + node:sqlite (runs under Node 24, Deno-ready) and ports the daemon's HTTP/WebSocket server to a runtime-agnostic abstraction so the daemon boots under both Node and Bun. Test runner (bun:test → Vitest): the shim maps the full bun:test API + custom matchers + mock.restore + it/test arg-reorder onto Vitest (~525 files, no per-file edit); global SDK mock via resolve.alias; test-daemon.sh rewired to vitest per shard; include matches both .test.ts and _test.ts (45 migration *_test.ts files / ~395 cases otherwise silently dropped). Storage (bun:sqlite → node:sqlite): sqlite-compat is a runtime-dual entry — top-level- await dynamic import of bun:sqlite under Bun (the native contract) or ./sqlite-node.ts under Node (wraps DatabaseSync: null get(), FK-OFF default, nested transaction via SAVEPOINT, query/run, <TRow,TParams> generics, undefined→null bind coercion). ~300 imports repointed. Bun.CryptoHasher→WebCrypto; Buffer↔BLOB→Uint8Array. Runtime server: runtime-server/ (Bun + Node backends); createHttpWsServer is async (awaits 'listening'); the Node backend routes WS upgrades through the 'upgrade' event (opposite of Bun) while preserving one fetch(req, upgrade) contract. app.ts, setup-websocket.ts, websocket-server-transport.ts rewired to RuntimeSocket. Online tests + helpers: daemon-server spawns under Node via tsx; mock-api-server→ node:http; waitForWebSocketMessage per-connection queue; client transport rejects on early close; Bun.$/Bun.sleep→node:fs/setTimeout. WS protocol online suite 16/16. CI: test-cli & test-daemon-online → vitest; vitest.online.config.ts; daemon tsconfig types:["bun"]; added ws/tsx/@types/ws deps; test-daemon.sh extracts junit failures by classname for --rerun/--show-failures (Vitest has no file= attribute). Verified: unit suite green (~12,800) + cli (71); lint/typecheck/knip/session-guards/ db-schema-parity pass; daemon boots under Node (tsx, all 13 startup phases + 167 migrations on node:sqlite) and Bun (CLI / make dev, on bun:sqlite). node:sqlite emits an ExperimentalWarning (stable in practice, and stable in Deno 2.9). Out of scope (later phases): CLI/bridge Bun.serve servers, Bun.spawn in space runtime (76 gated tests), deno compile, worktree Bun.hash identity, SDK-on-Deno spike.
5506fee to
af09eec
Compare
dev advanced with the Vitest/node:sqlite migration (#2364) and the post-approval sibling-quiesce fix (#2348). One conflict: built-in-workflows.test.ts — kept both my ChannelResolver import and dev's isBun gate-script guard. All other files auto-merged (my Post-Approval node + channels + post_review intact; verified by the full daemon suite — my shards green).
Resolve conflict in built-in-workflows.test.ts (dev kept the 4 review-posted bash behavioral tests under test.skipIf(!isBun); this branch removed them since the gate no longer has a bash script — coverage ported to the review_posted preset tests). Adopt dev's lazy Bun.spawn default in createReviewPostedValidator (required by #2364's Vitest/Node migration).
…Node createReviewPostedValidator used an eager `= Bun.spawn` default arg, which throws `ReferenceError: Bun is not defined` at module load under Vitest/Node (the daemon test runner since #2364) when registerProductionBuiltInValidators imports it. Match the other connector factories in this file (createPrReadyValidatorV2, createPrMergedValidator, createCodexReviewBotValidator, registerGithubConnector), which already use the lazy `((...args) => Bun.spawn(...args))` form that defers the Bun reference until call. [#835]
(Vitest migration) Conflicts were additive in the shared github-events files — both this PR's merge_state kind/essence/normalizer and dev's status kind/essence/normalizer land in the same unions, mapEventType switch, and event-essence branch list; resolved by keeping both. github-repository.ts auto-merged (adopted the sqlite-compat import) and the mergeStateSeq doc was updated to reflect that seq is now retained across rebuilds (P2-4 fix). bun install picks up dev's new deps (ws, tsx).
What
First step of the Bun→Deno migration. Migrates the test suite off
bun:test/bun:sqliteonto Vitest +node:sqliteso it runs under Node 24 (and is Deno-ready), and ports the daemon's HTTP/WebSocket server to a runtime-agnostic abstraction so the daemon now boots under Node (previously Bun-only).Per the user's note: "get the fresh data/docs/code online for deno 2.9.4 and compare with hyperneo implementation" — a full migration review was done first (8 subagents: codebase deep-dives + fresh Deno 2.9.4 research). This PR lands the foundation: runtime-agnostic tests + a bootable-under-Node daemon. Deno itself is a later step via the same seams.
Changes
Test runner (bun:test → Vitest) —
packages/daemon/{vitest.config.ts, vitest.online.config.ts, tests/bun-test-shim.ts, tests/sdk-mock.ts, tests/vitest.setup.ts},packages/shared/vitest.config.ts,packages/cli/vitest.config.ts. The shim maps the full bun:test API + custom matchers +mock.restore+ anit/testarg-reorder proxy onto Vitest, so ~525 test files needed no per-file import edit. Global SDK mock moved to aresolve.alias(sidesteps Vitest hoisting).scripts/test-daemon.shrewired tovitest runper shard.Storage (bun:sqlite → node:sqlite) —
packages/daemon/src/storage/sqlite-compat.ts:Database extends DatabaseSyncpreserving bun's contract (get()→null, FK-OFF default, nestedtransaction()via SAVEPOINT,query/run,<TRow,TParams>generics). ~300 imports repointed.Bun.CryptoHasher→WebCrypto;Buffer↔BLOB→Uint8Array.Runtime server (Bun.serve → abstraction) —
packages/daemon/src/lib/runtime-server/with Bun + Node backends.createHttpWsServeris async (awaits'listening'). The Node backend routes WS upgrades through the'upgrade'event (opposite of Bun) while preserving onefetch(req, upgrade)contract.app.ts,routes/setup-websocket.ts,lib/websocket-server-transport.tsrewired toRuntimeSocket. Daemon boots under Node through all 13 startup phases incl. 167 migrations onnode:sqlite.Online tests + helpers —
daemon-serverspawns the daemon under Node viatsx;mock-api-server→node:http;waitForWebSocketMessageuses a per-connection queue (the one-shot listener raced under Node's native WS); client transport rejects on early close;Bun.$/Bun.sleep→node:fs/setTimeout. WS protocol online suite: 16/16 green (was 0/16).CI —
test-cli&test-daemon-online→vitest run. Addedws,tsx,@types/wsdeps.Verification
./scripts/test-daemon.sh(~12,400 tests) + cli (71).lint/typecheck/knip/session-guards/db-schema-parityall pass.Note on
node:sqliteNode 24 emits an
ExperimentalWarning: SQLite is an experimental feature— the API is stable in practice (and stable in Deno 2.9, the migration target). CI already pinsnode-version: '24'.Out of scope (follow-up phases)
CLI servers + 4 provider bridge servers still use
Bun.serve(dev/prod);Bun.spawnin space runtime (76 gated tests re-enable when ported);deno compile; worktreeBun.hashidentity; SDK-on-Deno spike. These are tracked as later steps in the migration plan.