fix(harness): register web::fetch worker in composite entry#203
Conversation
The web::fetch worker (#202) shipped its own files and standalone main.ts but was never wired into src/index.ts, so `pnpm dev:all` and `start:all` (which run the composite all-in-one process) never started it. Add the registration so the worker boots with the rest. Also backfill the missing dev:web / iii-web wiring in package.json, plus the same gap for provider-llamacpp, and add a regression test asserting every runnable worker folder (src/*/main.ts) is wired into the composite manifest, dev scripts, and bin entries.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR integrates a new ChangesWeb Worker Integration
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
skill-check — worker0 verified, 13 skipped (no docs/).
Note 17 stale rendered artifact(s) detected on main, unrelated to this PR. This PR is fine; the drift was already there. A maintainer should open a chore PR to re-render these.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
harness/tests/composite-manifest.test.ts (1)
32-36: ⚡ Quick winGuard the synchronous
tsxspawn against Vitest’s default 5s timeout
compositeManifestNames()runsexecFileSync(tsxBin, [indexEntry, '--manifest'], { encoding: 'utf8' })(no timeout). Withtsxcold-start/transpile, this can exceed Vitest’s default 5,000ms (Node.js) and make the “registers every runnable worker folder…” test flaky. Add anexecFileSynctimeout (and/or set a per-test timeout for thatit(...)viait(name, { timeout: ... }, fn)).♻️ Example: explicit timeouts
-function compositeManifestNames(): string[] { - const out = execFileSync(tsxBin, [indexEntry, '--manifest'], { encoding: 'utf8' }); +function compositeManifestNames(): string[] { + const out = execFileSync(tsxBin, [indexEntry, '--manifest'], { + encoding: 'utf8', + timeout: 20_000, + }); const manifest = JSON.parse(out) as Array<{ name: string }>; return manifest.map((w) => w.name).sort(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harness/tests/composite-manifest.test.ts` around lines 32 - 36, The synchronous spawn in compositeManifestNames() currently calls execFileSync(tsxBin, [indexEntry, '--manifest'], { encoding: 'utf8' }) with no timeout and can exceed Vitest's 5s default; fix by adding a timeout option to that execFileSync call (e.g., include timeout: 30000 in the options object) and/or set a per-test timeout on the Vitest it(...) that uses compositeManifestNames (e.g., it(name, { timeout: 30000 }, ...)), referencing compositeManifestNames, execFileSync, tsxBin and indexEntry so the change is made where the manifest is spawned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@harness/tests/composite-manifest.test.ts`:
- Around line 32-36: The synchronous spawn in compositeManifestNames() currently
calls execFileSync(tsxBin, [indexEntry, '--manifest'], { encoding: 'utf8' })
with no timeout and can exceed Vitest's 5s default; fix by adding a timeout
option to that execFileSync call (e.g., include timeout: 30000 in the options
object) and/or set a per-test timeout on the Vitest it(...) that uses
compositeManifestNames (e.g., it(name, { timeout: 30000 }, ...)), referencing
compositeManifestNames, execFileSync, tsxBin and indexEntry so the change is
made where the manifest is spawned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb2a19fa-644e-431f-9d3b-4d65f8ae7334
📒 Files selected for processing (3)
harness/package.jsonharness/src/index.tsharness/tests/composite-manifest.test.ts
2 participants
Summary
The
web::fetchworker added in #202 shipped its source files and a standalonesrc/web/main.ts, but was never wired intosrc/index.ts— the composite all-in-one entry thatpnpm dev:all(bun --watch src/index.ts) andstart:all(node dist/index.js) run. As a result the web worker silently never started in the all-in-one process.This registers it alongside the other 15 workers and backfills the wiring that #202 missed.
Changes
src/index.ts: importregisterWeband add thewebworker to theWORKERSarray. Composite manifest now reports 16 workers (was 15).package.json: add the missingdev:webscript +iii-webbin. Also backfill the identical gap forprovider-llamacpp(it was in the composite array but had nodev:/iii-entry either).tests/composite-manifest.test.ts(new): regression guard asserting every runnable worker folder (src/*/main.ts) is wired into (a) the composite--manifest, (b) adev:<name>script, and (c) aniii-<name>bin. This is the check that was missing — per-folder tests existed, but nothing verified composite wiring.Root cause
#202's diff touched onlysrc/web/*andtests/web/*;src/index.tswas last modified in #195, before the web worker existed. TheWORKERSarray is a hardcoded list, so any new worker folder must be added manually — and there was no test catching the omission.Test plan
pnpm test— 1033 passing (121 files), including the newcomposite-manifesttestpnpm run typecheck— cleanwebandprovider-llamacpp) and passes with itbun src/index.ts --manifestlistswebSummary by CodeRabbit
New Features
Tests