fix(ci): unbreak build — separate workerd test pool, correct phantom dep version - #132
fix(ci): unbreak build — separate workerd test pool, correct phantom dep version#132chitcommit wants to merge 3 commits into
Conversation
…dep version The `build` job has been red on every PR opened or updated since the DO coordinator merge (c979f6a, Aug 2). The cause was not in any of those PRs. 1. vitest config overlap. vitest.config.ts globs `tests/**/*.test.ts`, which also matched tests/workers/coordinator.test.ts — a workerd-pool test that imports `cloudflare:test`. That specifier only resolves under @cloudflare/vitest-pool-workers (vitest.workers.config.mts), so the node suite died at import with ERR_MODULE_NOT_FOUND before running an assertion. Excluded tests/workers/** from the node suite, preserving vitest's default node_modules/dist excludes. 2. hono-agents "^0.19.0" never existed. a60d0e1 applied the agents-SDK version to the wrong package; the registry tops out at 3.0.12 and the lockfile already pins 3.0.7. Fresh `npm i` failed outright. Aligned the manifest to the locked version — the resolved version is still 3.0.7, so this is a manifest-only correction with no runtime change. 3. The workers suite ran nowhere in CI. `npm run test:workers` was never invoked by any workflow, so the DO coordinator tests had never executed. Excluding them from `npm test` would have converted "loudly broken" into "silently unrun", so they are now wired into the build job. Booting them required @cloudflare/vitest-pool-workers 0.22: the pinned 0.20.1 bundles a workerd supporting compatibility dates only through 2026-08-06, one day behind the 2026-08-07 date a60d0e1 set. Dev-only; the lockfile change is confined to that chain (pool-workers, miniflare, wrangler, workerd). Verified locally, full build sequence: tsc --noEmit clean; npm test 68 passed; npm run test:workers 24 passed (previously 0 — the suite could not boot). Not addressed here: the production advisories (hono, js-yaml, fast-uri, ip-address, nanoid) that fail the Dependency Audit gate are pre-existing on main and are what PR #128 already fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpK3ddoHzUqgW3keYafHsD
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
To use Codex here, create a Codex account and connect to github. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR updates worker testing dependencies, excludes worker tests from the Node Vitest suite, and adds a dedicated CI step to run them. ChangesWorker test isolation
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This PR makes localized CI and test-infrastructure changes and corrects a development dependency version; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Finding from separated adversarial review (claude-sonnet-4-5 via chittyclaw, reviewer != implementer): setting `exclude` replaces vitest's built-in default list wholesale instead of appending to it. The previous commit kept only node_modules and dist, silently dropping the cypress, dot-directory, and *.config.* patterns. Verified inert today — nothing under tests/ matches any dropped pattern, and `include` is scoped to tests/** so root config files were never candidates. It is restated anyway so a future file under e.g. tests/.cache/ cannot quietly re-enter the node suite. Also verified from the same review, empirically rather than by assertion: - `npm run test:workers` exits 1 on a broken config, so the new CI step is a real gate and not decorative. Checked by temporarily breaking the workers config and reading the exit code. - `npm ls @cloudflare/workers-types` reports no unmet peer dependency. wrangler 4.124 (pulled by pool-workers 0.22) resolves its own nested workers-types v5 while the project continues to resolve v4; tsc --noEmit passes against v4. Re-verified after this change: tsc clean, npm test 68 passed, test:workers 24 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpK3ddoHzUqgW3keYafHsD
|
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
`npm audit --audit-level=high --omit=dev` is the first step of the CI build job, so it gates every other check. It currently fails on `main` for reasons no PR introduced: npm audit is a moving gate, and advisories published after a branch last ran will fail it retroactively. hono 4.12.23 -> 4.13.3 is entirely inside the already-declared "^4.12.23", so this is a lockfile-only change with no manifest edit and no range widening. It clears 12 hono advisories, including CORS middleware reflecting any Origin with credentials when origin defaults to wildcard, hono/jsx cross-request context leakage, memo() retaining SSR output across requests, and ReDoS in the CORS and language middlewares. Production advisory count drops 7 -> 5. Validated after the bump: tsc --noEmit clean, npm test 68 passed, npm run test:workers 24 passed. The gate is still red on the remaining 4 (fast-uri, ip-address, js-yaml, nanoid). Those are transitive under agents/partyserver and each needs a forced MAJOR override (js-yaml 4->5, nanoid 5->6, fast-uri 3->4) pushed into packages that expect the older major. That is a distinct change with real silent-breakage risk and belongs in its own reviewed PR, not bundled into a CI fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SpK3ddoHzUqgW3keYafHsD
|
|
To use Codex here, create a Codex account and connect to github. |
|
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
chittycommand | 13f61ca | Aug 18 2026, 11:21 PM |
1 participant
Why
The
buildjob has been red on every PR opened or updated since the DO coordinator merge (c979f6a, Aug 2). The cause was in none of those PRs — it is onmain.Root causes
1. vitest config overlap (the actual test blocker)
vitest.config.tsglobstests/**/*.test.ts, which also matchedtests/workers/coordinator.test.ts— a workerd-pool test importingcloudflare:test. That specifier only resolves under@cloudflare/vitest-pool-workers(vitest.workers.config.mts), so the node suite died at import:It failed before running a single assertion. Excluded
tests/workers/**from the node suite, preserving vitest's defaultnode_modules/distexcludes.2.
hono-agents@^0.19.0never existeda60d0e1applied the agents-SDK version to the wrong package. The registry tops out at3.0.12; the lockfile already pinned3.0.7. Freshnpm ifailed outright withETARGET. Aligned the manifest to the locked version — resolved version is still3.0.7, so this is a manifest-only correction with no runtime change.3. The workers suite ran nowhere in CI
npm run test:workerswas never invoked by any workflow, so the DO coordinator tests had never executed in CI. Simply excluding them fromnpm testwould have converted "loudly broken" into "silently unrun", so they are now wired into the build job.Booting them required
@cloudflare/vitest-pool-workers0.22 — the pinned 0.20.1 bundles a workerd supporting compatibility dates only through2026-08-06, one day behind the2026-08-07datea60d0e1set:Dev-only; the lockfile change is confined to that chain (pool-workers, miniflare, wrangler, workerd).
hono-agentsstays at3.0.7.Verification
Full sequence run locally against real infrastructure — no mocks, no stubs:
tsc -p tsconfig.json --noEmitnpm testnpm run test:workersThis PR will still show
buildred — deliberatelybuildruns Dependency Audit before Typecheck/Test, so it fails at 22s and never reaches the fix above. That audit failure is independent of this PR and pre-existing onmain.Worth flagging clearly:
npm auditis a moving gate. #128 passed it on Aug 14, but advisories published since (hono <=4.12.33, plusfast-uri,ip-address,js-yaml,nanoid,@hono/node-server,body-parser) now fail it on any branch — I verified this by merging #128 into this branch locally and re-running the gate; it still failed. #128 as it stands will not turn the queue green.Clearing it requires bumping
honopast 4.12.33 — a web-framework upgrade with real runtime risk that does not belong bundled inside a CI fix. Left as separate work, deliberately.Suggested review path: confirm the test-infra reasoning here on its own merits; land the hono/advisory bump separately, after which this PR's
buildshould go fully green.🤖 Generated with Claude Code
https://claude.ai/code/session_01SpK3ddoHzUqgW3keYafHsD
Summary by CodeRabbit
Tests
Chores