feat(ai): migrate localNlpService onto WorkerBus v2, delete v1 inference worker - #290
feat(ai): migrate localNlpService onto WorkerBus v2, delete v1 inference worker#290qnbs wants to merge 2 commits into
Conversation
…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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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
🧹 Nitpick comments (2)
tests/unit/localNlpService.test.ts (1)
45-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage of sentiment parsing/fallback; missing a dedicated "pool unavailable" test.
The suite exercises label parsing, truncation, and enqueue wiring well. It doesn't add a test for
ensureInferencePool()resolving tonull(the!busthrow branch inrequestInference), which is a distinct, real-world path (e.g., unsupported environment) beyond a generic rejected task. It's implicitly covered via the shared catch behavior, but an explicit test would pin down that specific contract.🧪 Example addition
+ it('returns NEUTRAL fallback when the inference pool is unavailable', async () => { + mockEnsureInferencePool.mockResolvedValue(null); + const r = await analyzeSentiment('anything'); + expect(r.label).toBe('NEUTRAL'); + });🤖 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 `@tests/unit/localNlpService.test.ts` around lines 45 - 100, Add a dedicated test in the analyzeSentiment suite that makes ensureInferencePool resolve to null, exercising requestInference’s !bus throw branch. Assert that analyzeSentiment preserves the same NEUTRAL fallback contract as task rejection, including score 0.5 and normalized 0.services/ai/localNlpService.ts (1)
51-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCatch blocks silently swallow all inference failures without logging.
Both fallbacks catch every error (OOM, pool unavailable, malformed result, etc.) and return a static fallback with no call into
services/logger.ts. Per the**/*.{ts,tsx,js,jsx}guideline, "technical details may only be emitted throughservices/logger.tsusing Biome-compliantwarnorerror" and silent swallowing is only permitted for "documented aborts." Additionally,summarizeText's catch (lines 65-67) lacks the QNBS-v3 doc comment thatanalyzeSentiment's catch (line 52) has, so it isn't even textually documented.Add a
logger.warn(with the caught error, no payload/PII) in both catches, and givesummarizeText's catch a QNBS-v3 comment matching the one onanalyzeSentiment.♻️ Example
} catch { + // QNBS-v3: [Graceful degrade — same fallback v1 returned on any worker failure.] return text.slice(0, 280); // graceful degrade }Also applies to: 65-67
🤖 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 `@services/ai/localNlpService.ts` around lines 51 - 54, Update the catch blocks in analyzeSentiment and summarizeText to accept the caught error and call services/logger.ts through logger.warn with the error only, avoiding payload or PII. Add the matching QNBS-v3 graceful-degrade comment to summarizeText’s catch while preserving both existing fallback results.Source: Coding guidelines
🤖 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 `@services/ai/localNlpService.ts`:
- Around line 15-32: Update requestInference so the enqueue payload omits
inferenceOptions when it is undefined, while preserving the property when
options are provided; adjust the payload construction around bus.enqueue and
keep the existing request type contract intact.
---
Nitpick comments:
In `@services/ai/localNlpService.ts`:
- Around line 51-54: Update the catch blocks in analyzeSentiment and
summarizeText to accept the caught error and call services/logger.ts through
logger.warn with the error only, avoiding payload or PII. Add the matching
QNBS-v3 graceful-degrade comment to summarizeText’s catch while preserving both
existing fallback results.
In `@tests/unit/localNlpService.test.ts`:
- Around line 45-100: Add a dedicated test in the analyzeSentiment suite that
makes ensureInferencePool resolve to null, exercising requestInference’s !bus
throw branch. Assert that analyzeSentiment preserves the same NEUTRAL fallback
contract as task rejection, including score 0.5 and normalized 0.
🪄 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: 8cc231a7-0b99-4d4c-8a6e-61a62455d021
📒 Files selected for processing (7)
services/ai/localNlpService.tsservices/ai/pipelineLruCache.tsservices/voice/wasmSttEngine.tstests/unit/inferenceWorker.test.tstests/unit/localNlpService.test.tstests/unit/services/localNlpService.test.tsworkers/inference.worker.ts
💤 Files with no reviewable changes (3)
- tests/unit/inferenceWorker.test.ts
- tests/unit/services/localNlpService.test.ts
- workers/inference.worker.ts
…tOptionalPropertyTypes) localNlpService.requestInference() spread inferenceOptions into the enqueue payload via object-literal shorthand, so calls without a 4th arg (analyzeSentiment) set the key to an explicit `undefined`. With exactOptionalPropertyTypes the optional payload field only accepts the property being absent, not present-with-undefined. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Fourth and final migration PR of the worker-generation consolidation (ADR-0014). Stacked on #288 → #287 → #286. Plan:
.claude/plans/worker-generation-v1-vs-enumerated-fox.md.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 inservices/ai/index.ts, so this is the lowest-blast-radius of the three service migrations. Internals swapped from a dedicatednew Worker(inference.worker.ts)instance to the shared WorkerBus v2'inference'pool viaensureInferencePool()(added in feat(ai): migrate localEmbeddingService onto WorkerBus v2 #288), routing to the'inference.text'capability.analyzeSentiment/summarizeTextalready had their own app-level graceful-degrade behavior on failure (NEUTRAL sentiment / truncated text) — preserved unchanged via try/catch around the new throw-on-failurerequestInference()call.localEmbeddingService.ts(feat(ai): migrate localEmbeddingService onto WorkerBus v2 #288) 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.tsrewritten against aworkerBusManagermock;tests/unit/services/localNlpService.test.tsdeleted as a near-total duplicate (same module, overlappingclassifyWritingTopic/analyzeSentimentcoverage the rewritten suite already has).Verified via typecheck and a full local
pnpm run build:edgereproduction of Vercel's exact build command — the lesson from #288's missing-export incident (typecheck/Vitest alone didn't catch that one; only the real build did).Test plan
pnpm exec vitest run tests/unit/localNlpService.test.ts tests/unit/inferenceWorkerHandlerV2.test.ts— 30/30 passingpnpm run typecheck(exact CI command) — cleanNODE_OPTIONS=--max-old-space-size=3072 pnpm run build:edge(exact Vercel build command) — clean, verified end-to-endpnpm run lint(Biome) — clean via pre-commit hook🤖 Generated with Claude Code
Summary by CodeRabbit