Replace sqlite-lembed local embedder - #62
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 5 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughReplaces the Changessqlite-lembed → node-llama-cpp Embedder Replacement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/e2e/Dockerfile.local-mode (1)
1-2: ⚡ Quick winDrop root privileges for the runtime container.
Line 2 inherits the default root user. Add a non-root
USERbeforeCMDto reduce blast radius if dependency scripts or test payloads are compromised.Suggested patch
RUN pnpm --filter `@clankeroverflow/cli` run build +RUN chown -R node:node /workspace +USER node + CMD ["node", "packages/cli/e2e/local-mode.mjs"]🤖 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 `@packages/cli/e2e/Dockerfile.local-mode` around lines 1 - 2, The Dockerfile currently runs as the default root user, which poses a security risk. Add a USER directive before the CMD instruction in the Dockerfile to specify a non-root user for the runtime container. You can either create a new unprivileged user (with appropriate RUN mkdir and useradd commands) or use an existing non-root user that may be available in the base node image. This will ensure the container runs with reduced privileges and limits the blast radius if the runtime environment is compromised.Source: Linters/SAST tools
🤖 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 `@packages/cli/src/mcp/local-backend.ts`:
- Around line 260-264: The resolveEmbedder() method has a race condition where
concurrent calls can both pass the initial check and trigger
createLocalEmbedder() separately, causing duplicate model loads. Fix this by
assigning the promise from createLocalEmbedder() to this.embedder immediately
before awaiting it, rather than assigning only after the await completes. This
way, subsequent concurrent calls will detect the in-progress promise and await
it instead of starting new embedder creation.
In `@scripts/test-cli-local-e2e.ts`:
- Around line 6-8: Add a guard check after the NODE_IMAGES initialization (after
the filter operation on lines 6-8) to ensure the array is not empty. If
NODE_IMAGES resolves to an empty array, the script should throw an error or exit
with a non-zero code to prevent false-green test runs. This will catch cases
where the CLANKER_LOCAL_E2E_NODE_IMAGES environment variable is blank, contains
only commas, or is otherwise invalid.
---
Nitpick comments:
In `@packages/cli/e2e/Dockerfile.local-mode`:
- Around line 1-2: The Dockerfile currently runs as the default root user, which
poses a security risk. Add a USER directive before the CMD instruction in the
Dockerfile to specify a non-root user for the runtime container. You can either
create a new unprivileged user (with appropriate RUN mkdir and useradd commands)
or use an existing non-root user that may be available in the base node image.
This will ensure the container runs with reduced privileges and limits the blast
radius if the runtime environment is compromised.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a75db137-31ef-4c30-9c95-7172f07653e1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
README.mdpackages/cli/e2e/Dockerfile.local-modepackages/cli/e2e/local-mode.mjspackages/cli/package.jsonpackages/cli/src/index.tspackages/cli/src/mcp/local-backend.test.tspackages/cli/src/mcp/local-backend.tspackages/cli/src/mcp/local-semantic.tspackages/cli/src/mcp/server.tspackages/cli/src/package.test.tspnpm-workspace.yamlscripts/test-cli-local-e2e.ts
| private async resolveEmbedder() { | ||
| if (this.embedder) return this.embedder; | ||
| if (!this.semantic?.enabled) throw new LocalSemanticSearchNotConfiguredError(); | ||
| this.embedder = await createSqliteEmbedder(this.db, this.semantic); | ||
| this.embedder = await createLocalEmbedder(this.semantic); | ||
| return this.embedder; |
There was a problem hiding this comment.
Serialize embedder initialization to prevent duplicate GGUF model loads.
Line 263 assigns this.embedder only after await createLocalEmbedder(...) resolves. If two requests hit resolveEmbedder() concurrently on cold start, both can create separate embedder/model contexts. That can cause avoidable memory pressure and unstable startup under concurrent MCP traffic.
💡 Suggested fix
export class LocalBackend implements SolutionBackend {
private db: LocalDb;
private semantic?: LocalSemanticConfig;
private embedder?: Embedder;
+ private embedderPromise?: Promise<Embedder>;
@@
private async resolveEmbedder() {
if (this.embedder) return this.embedder;
if (!this.semantic?.enabled) throw new LocalSemanticSearchNotConfiguredError();
- this.embedder = await createLocalEmbedder(this.semantic);
- return this.embedder;
+ if (!this.embedderPromise) {
+ this.embedderPromise = createLocalEmbedder(this.semantic)
+ .then((embedder) => {
+ this.embedder = embedder;
+ return embedder;
+ })
+ .catch((error) => {
+ this.embedderPromise = undefined;
+ throw error;
+ });
+ }
+ return this.embedderPromise;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async resolveEmbedder() { | |
| if (this.embedder) return this.embedder; | |
| if (!this.semantic?.enabled) throw new LocalSemanticSearchNotConfiguredError(); | |
| this.embedder = await createSqliteEmbedder(this.db, this.semantic); | |
| this.embedder = await createLocalEmbedder(this.semantic); | |
| return this.embedder; | |
| private async resolveEmbedder() { | |
| if (this.embedder) return this.embedder; | |
| if (!this.semantic?.enabled) throw new LocalSemanticSearchNotConfiguredError(); | |
| if (!this.embedderPromise) { | |
| this.embedderPromise = createLocalEmbedder(this.semantic) | |
| .then((embedder) => { | |
| this.embedder = embedder; | |
| return embedder; | |
| }) | |
| .catch((error) => { | |
| this.embedderPromise = undefined; | |
| throw error; | |
| }); | |
| } | |
| return this.embedderPromise; | |
| } |
🤖 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 `@packages/cli/src/mcp/local-backend.ts` around lines 260 - 264, The
resolveEmbedder() method has a race condition where concurrent calls can both
pass the initial check and trigger createLocalEmbedder() separately, causing
duplicate model loads. Fix this by assigning the promise from
createLocalEmbedder() to this.embedder immediately before awaiting it, rather
than assigning only after the await completes. This way, subsequent concurrent
calls will detect the in-progress promise and await it instead of starting new
embedder creation.
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Documentation
Refactor
node-llama-cpp. Thelocal statusandlocal doctoroutput now reflects the updated embedder name visible to users.