docs: ADR-0015, CLAUDE.md/AGENTS.md doc-truth, TODO/AUDIT closeout, run log - #291
Merged
Conversation
…ests Neither workers/v2/duckdb.worker.ts's handlers nor workers/v2/inference.worker.ts's handleInference were exported or unit-tested against real logic (only webllm.worker.ts had that, tests/unit/webllmWorkerHandler.test.ts) despite both already being registered as live WorkerBus v2 pools with zero production callers — docs/adr/0014-worker-generation-duplication.md's deferred migration needs these to actually match v1 before any consumer cuts over. Testing surfaced a real gap: initDuckDb()'s OPFS-unavailable catch block silently swallowed the failure, unlike v1's out-of-band OPFS_FALLBACK message that lets the UI warn users their analytics won't persist. Fixed by threading ctx.emitProgress into initDuckDb() and emitting an 'opfs-fallback' progress stage from the catch block — reuses the existing progress channel WebLLM already uses for download progress, no new message-protocol/schema change needed. First PR of the ADR-0014 consolidation sprint (5 PRs total, see .claude/plans/worker-generation-v1-vs-enumerated-fox.md). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…S-v3 format CodeRabbit review on #286 found a real bug: initDuckDb()'s OPFS catch block overwrote `connection` with the fallback connection without closing the first (partially-attached) one, leaking a DuckDB connection on every OPFS-attach failure. Fixed by tracking the OPFS attempt in its own local variable and closing it in the catch path before falling back. New regression test asserts close() is called. Also collapsed several multi-line QNBS-v3 comments in workers/v2/duckdb.worker.ts, workers/v2/inference.worker.ts, and both new test files to the required single bracketed-line format. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Second of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication
(ADR-0014). services/duckdb/duckdbClient.ts's public API (init/query/
exec/shutdown/terminate/setOpfsFallbackHandler) is unchanged — its 5
real consumers (useDuckDb.ts, duckdbAnalytics.ts, duckdbMigration.ts,
ragVectorMigration.ts, telemetryService.ts) need no changes. Only the
internals swapped from a raw `new Worker(duckdbWorker.ts)` to routing
through the shared WorkerBus v2 'duckdb' pool via a new
ensureDuckDbPool() (mirrors ensureWebLlmPool()'s decoupling from the
enableWorkerBusV2 flag — analytics is core, not experimental infra).
The v2 worker returns raw values and throws on failure, unlike v1's
{ok,rows,error} wrapper — the adapter in duckdbClient.ts's send()
translates at that one boundary instead of touching ~20 call sites
across 5 files. WorkerBus's default 2-retry is disabled per-call
(retryPolicy: {maxRetries:0}) to avoid stacking under
duckdbAnalytics.ts's existing withDuckDbRetry() app-level retry.
Two things surfaced while wiring this up, both fixed here:
- WorkerBus.shutdown() tears down all 4 pools with no way to scope to
just 'duckdb' — added WorkerBus.terminatePool(poolId) so
duckdbClient.terminate() doesn't kill in-flight inference/webllm/
plugin work on the shared bus.
- send() is async and awaits ensureDuckDbPool() before a task handle
exists; naively attaching the AbortSignal listener after that await
(like a first draft did) can lose a same-tick abort, unlike v1's
synchronous Promise-executor version. Fixed by attaching the
listener synchronously up front and applying it once the handle is
ready — regression-tested.
workers/duckdbWorker.ts deleted; features/featureCatalog.ts's
enableDuckDbAnalytics entry retargeted to workers/v2/duckdb.worker.ts.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ailure CodeRabbit's second pass on #286 flagged this as an outside-diff-range comment (not a resolvable inline thread) on bcc41c2's own fix: the new `await opfsConnection?.close()` call could itself reject, which would skip emitProgress('opfs-fallback', ...) and the fallback newDb.connect(), and replace the original ATTACH error with the close failure — turning a graceful degrade into a hard init failure. Wrapped the cleanup in its own try/catch (best-effort, logged via console.warn, never blocks the fallback path) so a failing close() can't prevent the fallback connection or swallow the real error. New regression test asserts both still happen when close() rejects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Third of 5 PRs consolidating the v1/WorkerBus-v2 worker duplication (ADR-0014). services/ai/localEmbeddingService.ts's public API (embedText/embedBatch/cosineSimilarity/clearEmbeddingCache) is unchanged — its consumers (localRagService.ts, crossProjectIndexService.ts, ragPromptAssembly.ts, ragVectorMigration.ts, proForgeMemoryBank.ts, loraEvaluationService.ts, loraDatasetBuilder.ts, inferenceGateway.ts) need no changes. Internals swapped from a dedicated `new Worker(inference.worker.ts)` instance to the shared WorkerBus v2 'inference' pool via ensureInferencePool() (same decoupling-from- enableWorkerBusV2 pattern as ensureWebLlmPool/ensureDuckDbPool). v2's handler already throws on failure (matching embedText's existing throw-on-failure contract), so no response-shape translation is needed here — just swap the transport. Deleted ~70 lines of hand-rolled health-check/restart/backoff logic (30s ping, exponential backoff up to 5 attempts), superseded by WorkerPool's generic health check. Capped the shared 'inference' pool's maxWorkers to 2 (was MAX_WORKERS_INFERENCE=4) in workerBusManager.ts: each pool replica independently loads its own transformers.js pipeline with no cross-replica cache sharing, so 4 concurrent workers under a burst (e.g. embedBatch's micro-batching) could mean 4x the model memory footprint. workers/inference.worker.ts (v1) is NOT deleted yet — localNlpService.ts still uses it directly; removal is PR 4 of this sequence, after that migration lands too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…QL param binding Three real findings from CodeRabbit's review of #287 (duckdbClient migration onto WorkerBus v2), all confirmed against current code before fixing: 1. CRITICAL — params silently dropped (workers/v2/duckdb.worker.ts). duckdbClient.send() enqueues {sql, params}, but handleQuery/ handleExec only read req.sql and called connection.query(sql) directly, ignoring params entirely and leaving an unbound-query/ SQL-injection surface flagged by static analysis. This was already live: services/ai/telemetryService.ts's writeToDuckDb() passes real bound params ([taskType, backend, modelId, latencyMs, success]) that were being silently discarded. Fixed via DuckDB-WASM prepared statements (connection.prepare(sql) + stmt.query(...params) + stmt.close()) when params is non-empty; unchanged direct connection.query(sql) otherwise. Needed a narrow structural type (PreparableConnection) since tsgo doesn't resolve AsyncDuckDBConnection.prepare() through this package's nested `export *` chain, despite it being present in the package's own .d.ts — same class of tsgo/external-package gap CLAUDE.md already documents for the transformers.js path alias. 2. MAJOR — DuckDB pool worker losing its connection on respawn (services/duckdb/duckdbClient.ts). The duckdb pool's minWorkers:0 lets its worker idle-terminate; a freshly spawned replacement worker's module state has no `connection` until INIT runs again on it, so the next QUERY/EXEC after a respawn failed with "DuckDB not initialized". Fixed by transparently re-sending INIT once and retrying the original call when send() sees that specific error message (bounded to one retry, INIT/SHUTDOWN excluded from the check to avoid any recursive loop). 3. MAJOR — terminatePool() had no lifecycle contract (packages/worker-bus/src/workerBus.ts + services/workerBusManager.ts). Removing a pool didn't settle tasks already routed to it — their result promises would await a RESULT message from a worker that no longer exists. Fixed by tracking task->pool assignment (new taskPools map, set/cleared around runTask's pool-acquire phase) and cancelling matching tasks before deletion. Separately, ensureDuckDbPool() assumed a non-null bus always has every pool, so any call after duckdbClient.terminate() would fail with NO_POOL forever; added WorkerBus.hasPool() and a re-registration path (duckDbPoolOptions()/reRegisterDuckDbPool(), shared with doInitWorkerBus() to avoid option drift between the two registration sites). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…uilds Root cause of both Vercel deployment failures on this branch/PR chain: services/ai/localEmbeddingService.ts imports and calls ensureInferencePool() from workerBusManager.ts, but that function was never actually added there — only ensureDuckDbPool() was. Vitest never caught this because localEmbeddingService.test.ts mocks the entire workerBusManager module (vi.mock(...) replaces it wholesale), so the real file's missing export was invisible to that suite. tsgo's typecheck also passed clean locally for reasons still unclear (worth a follow-up look), but rolldown's stricter static bundling in the actual `pnpm run build:edge` (Vercel's exact build command) correctly failed hard with [MISSING_EXPORT] — reproduced locally with `NODE_OPTIONS=--max-old-space-size=3072 pnpm run build:edge`, which is what actually surfaced this. Added ensureInferencePool(), mirroring ensureDuckDbPool()'s shape (decoupled from enableWorkerBusV2, re-registers the pool if it was removed via terminatePool() while the bus stays alive). Refactored doInitWorkerBus()'s inline 'inference' pool registration to share the same inferencePoolOptions() the new function uses, avoiding the same kind of drift the duckdb pool options already guard against. New tests in workerBusManager.test.ts import the *real* module (only @domain/worker-bus is mocked) — this exact suite would have caught the missing export immediately, unlike the fully-mocked consumer-side test. Verified fixed via a full local `pnpm run build:edge` reproduction of Vercel's exact build command (previously failed with [MISSING_EXPORT], now builds clean). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nce worker Fourth and final migration PR of the worker-generation consolidation (ADR-0014). services/ai/localNlpService.ts's public API (analyzeSentiment/summarizeText/classifyWritingTopic) is unchanged — confirmed zero real production callers beyond its own tests and a barrel re-export in services/ai/index.ts, so this is the lowest-blast-radius of the three migrations. Internals swapped from a dedicated `new Worker(inference.worker.ts)` instance to the shared WorkerBus v2 'inference' pool via ensureInferencePool() (added in the previous PR), routing to the 'inference.text' capability. analyzeSentiment/summarizeText already had their own app-level graceful-degrade behavior on failure (NEUTRAL sentiment / truncated text) — that's preserved unchanged via try/catch around the new throw-on-failure requestInference() call, replacing the old ok:false-check. With both localEmbeddingService.ts (previous PR) and this migrated, workers/inference.worker.ts (v1) has no remaining callers — deleted, along with its dedicated test (tests/unit/inferenceWorker.test.ts). tests/unit/localNlpService.test.ts rewritten against a workerBusManager mock; tests/unit/services/localNlpService.test.ts deleted as a near-total duplicate of the rewritten suite (same module, overlapping classifyWritingTopic/analyzeSentiment coverage, the comprehensive file already had everything it did). Verified via typecheck and a full local `pnpm run build:edge` reproduction of Vercel's exact build command (the lesson from the previous PR's missing-export incident) — both clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…un log Closes out the worker-generation consolidation sprint (PRs #286-288, #290) at the documentation layer. - New docs/adr/0015-worker-generation-consolidation.md — the decision record for the completed migration (target generation, per-workload changes, parity gaps found and fixed, deliberately-unchanged WebLLM-fallback-shape rationale, the timeoutMs follow-up). - docs/adr/0014-worker-generation-duplication.md: status line changed to "Superseded by 0015" (body untouched, per this repo's own "supersede, don't edit history" convention). - docs/adr/README.md: index gained rows 0010-0015 — 0010-0014 were pre-existing files missing from the index before this sprint, unrelated drift fixed opportunistically while editing the file. - CLAUDE.md/AGENTS.md: directory map, WorkerBus v2 section, DuckDB section, and Known Technical Debt all updated to the post-migration state. Also fixed 2 pre-existing doc-truth bugs found while reading the WorkerBus v2 source directly: WorkerHandlerContext.emitProgress is a flat function, not context.progress.emit(...); there is no 'background' priority tier, only critical > high > normal > low. - TODO.md: F-14 entry marked done with the PR chain and correction- loop findings. Also caught and fixed a genuinely stale, unrelated entry while in the file — "Tag v1.24.2 + publish release" was still marked open, but v1.24.2 was already tagged/released (verified via git tag/gh release list). - AUDIT.md: F-14 and F-11 (the same stale-tag issue) findings updated to resolved. - CHANGELOG.md: [Unreleased] section added, covering the full sprint (Changed/Fixed/Removed). - New docs/audit/WS-RUN-LOG-2026-07-29-worker-consolidation.md — execution log in this repo's established WS-RUN-LOG format: baseline, the 4-PR breakdown, every correction-loop finding with its fix, the Vercel missing-export incident, final verification, and what's deliberately left unresolved (timeoutMs enforcement; why local tsgo typecheck didn't catch the missing export). Verified: pnpm exec tsx scripts/audit-feature-parity.ts (0 drifts), checkDocMetrics.test.ts + featureCatalog.test.ts (28/28), typecheck clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@CLAUDE.md`:
- Line 409: Update the CLAUDE.md entry for workers/v2/inference.worker.ts to
remove the recommendation to restore `@ts-expect-error`. State that any
`@huggingface/transformers` v3 path-alias issue must be resolved by fixing the
tsconfig.json alias or the relevant type declaration, without introducing
suppression directives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52cf6d5b-7f45-4114-9aed-3c596ef18a10
📒 Files selected for processing (9)
AGENTS.mdAUDIT.mdCHANGELOG.mdCLAUDE.mdTODO.mddocs/adr/0014-worker-generation-duplication.mddocs/adr/0015-worker-generation-consolidation.mddocs/adr/README.mddocs/audit/WS-RUN-LOG-2026-07-29-worker-consolidation.md
CodeRabbit review on #291 correctly flagged that the Known Technical Debt line I edited (only updating the file path to workers/v2/inference.worker.ts) still carried a pre-existing recommendation to "restore @ts-expect-error" if the transformers.js path alias breaks — in direct tension with this repo's own suppression-ratchet policy documented a few sections above in the same file. Reworded to point at fixing the alias/type declaration directly instead, consistent with the structural-type workaround this same sprint used for a different tsgo/external-package resolution gap (workers/v2/duckdb.worker.ts's PreparableConnection, PR #287) rather than reaching for a suppression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…leanup # Conflicts: # packages/worker-bus/tests/workerBus.test.ts # services/ai/localNlpService.ts # services/duckdb/duckdbClient.ts # services/workerBusManager.ts # tests/unit/duckdbClient.test.ts # tests/unit/inferenceWorkerHandlerV2.test.ts # tests/unit/localNlpService.test.ts # tests/unit/workerBusManager.test.ts # workers/v2/duckdb.worker.ts
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
1 participant
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.
Summary
Closes out the worker-generation consolidation sprint (#286-288, #290) at the documentation layer. Stacked on #290 → #288 → #287 → #286 (merged).
docs/adr/0015-worker-generation-consolidation.md— the decision record for the completed migration.docs/adr/0014-*.md: status → "Superseded by 0015" (body untouched, per this repo's "supersede, don't edit history" convention).docs/adr/README.md: index gained rows 0010-0015 (0010-0014 were pre-existing files missing from the index before this sprint — unrelated drift, fixed opportunistically).CLAUDE.md/AGENTS.md: directory map, WorkerBus v2 section, DuckDB section, Known Technical Debt updated to post-migration state. Also fixed 2 pre-existing doc-truth bugs found while reading the WorkerBus v2 source directly (emitProgressis a flat function notcontext.progress.emit(...); nobackgroundpriority tier).TODO.md: F-14 marked done with the PR chain. Also caught and fixed an unrelated stale entry — "Tag v1.24.2 + publish release" was still marked open, butv1.24.2was already tagged/released.AUDIT.md: F-14 and F-11 (same stale-tag issue) updated to resolved.CHANGELOG.md:[Unreleased]section added covering the full sprint.docs/audit/WS-RUN-LOG-2026-07-29-worker-consolidation.md— full execution log (baseline, the 4-PR breakdown, every correction-loop finding with its fix, the Vercel missing-export incident, final verification, deliberately-unresolved items).Test plan
pnpm exec tsx scripts/audit-feature-parity.ts— 0 driftspnpm exec vitest run tests/unit/checkDocMetrics.test.ts tests/unit/featureCatalog.test.ts— 28/28 passingpnpm run typecheck(exact CI command) — cleanpnpm run lint(Biome) — clean via pre-commit hook🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation