[codex] improve keyword and hybrid retrieval - #65
Merged
Conversation
…rgonomics Address 8 local-search bugs: - Empty/whitespace query no longer crashes the embedder; rejected upfront (CLI parseSearchQuery, MCP zod schema, local-backend guard). - --limit now enforces integer 1-20 (was silently 0/empty); local embed --limit requires >= 1. Docs corrected (default 1, not 3). - Leading-dash queries (`-1`, `v2.0-beta-1`) surface a `--` separator hint instead of a bare unknown-option error. - ftsQuery rewritten to parse and honor FTS5 operators (AND/OR/NOT, NEAR, column filters, prefix terms, groups) or reject malformed syntax with a clear message; auto mode falls through to hybrid on a keyword syntax error. - Semantic ranking improved by prefixing query embeddings with the required BGE v1.5 retrieval instruction (document side unchanged, no re-embedding). - Documented `clanker mcp` for batch/repeated queries to avoid cold starts, and the benign node-llama-cpp tokenizer warning.
WalkthroughThis PR introduces a two-tier keyword search strategy ( ChangesKeyword Strategy Routing and Auto-Mode Fallback Chain
Local and Hosted Embedding Benchmark Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Client
participant searchWithAutoFallback
participant LocalBackend
participant RemoteBackend
participant TRPC as tRPC solutions.search
rect rgba(100, 149, 237, 0.5)
note over Client,TRPC: Auto-mode search flow
Client->>searchWithAutoFallback: query, allowHybridFallback
searchWithAutoFallback->>LocalBackend: searchExactKeyword (strategy=exact)
LocalBackend->>LocalBackend: localFtsQuery → AND match
alt results found
LocalBackend-->>searchWithAutoFallback: results
searchWithAutoFallback-->>Client: exact keyword results
else empty results + hybrid allowed
searchWithAutoFallback->>RemoteBackend: search(mode=hybrid)
RemoteBackend->>TRPC: query {mode:hybrid}
TRPC-->>RemoteBackend: hybrid results
RemoteBackend-->>searchWithAutoFallback: hybrid results
searchWithAutoFallback-->>Client: hybrid results
else empty results + hybrid unavailable or throws
searchWithAutoFallback->>LocalBackend: search(mode=keyword, keywordStrategy=tiered)
LocalBackend->>LocalBackend: localRelaxedFtsQuery → OR prefix match
LocalBackend-->>searchWithAutoFallback: tiered results
searchWithAutoFallback-->>Client: tiered keyword results
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 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 |
bernoussama
marked this pull request as ready for review
June 21, 2026 22:28
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/benchmarks/local-embeddings/metrics.ts`:
- Around line 59-60: The bootstrapSamples parameter lacks validation, allowing
values less than or equal to zero to be passed, which results in empty bootstrap
draws and misleading confidence interval values (low/high forced to 0). Add
validation logic in the function or method that uses the bootstrapSamples
parameter (around the parameter definition or at the start of the function
logic) to ensure bootstrapSamples is greater than zero. If an invalid value is
provided, either throw an error with a clear message or provide a sensible
default. Apply this same validation check to any other places where bootstrap
sampling occurs, as indicated by the "Also applies to" section spanning lines
68-81.
In `@packages/cli/benchmarks/local-embeddings/models.ts`:
- Around line 76-90: The temporary file handling in the ensureModel function has
two issues: the temp path uses only the process ID, causing concurrent downloads
to collide and overwrite each other, and temporary files are only cleaned up
when a checksum mismatch occurs, leaving stale files behind on other failures
like fetch or pipeline errors. Replace the PID-based temporaryPath with a unique
identifier per call (such as a UUID), and wrap the fetch, pipeline, and checksum
verification logic in a try-finally block to ensure the temporary file is always
cleaned up with rmSync regardless of whether an error occurs or succeeds.
In `@packages/cli/benchmarks/local-embeddings/run.ts`:
- Around line 115-137: The runWorker function lacks a timeout mechanism, which
means if the child process hangs (due to model load deadlock or stuck I/O), the
entire benchmark run will hang indefinitely. Add a timeout to the Promise that
waits for the child process to exit. Use Promise.race to combine the exit
promise with a timeout promise, and if the timeout fires, kill the child process
using child.kill() and throw an appropriate error message indicating the worker
timed out. This ensures the benchmark run can recover from hung worker
processes.
In `@packages/cli/benchmarks/local-embeddings/worker.ts`:
- Around line 106-123: The runCold function (and similarly runFull as mentioned
in the review) only disposes of resources when execution completes successfully.
If any error occurs during loadEmbedder, formatQuery, embed, or performance
measurements, the cleanup calls to loaded.context.dispose(),
loaded.model.dispose(), and loaded.llama.dispose() will not execute, causing
resource leaks. Wrap the main function body in a try/finally block, moving all
three dispose calls into the finally block to ensure cleanup always runs
regardless of whether an error was thrown.
In `@packages/cli/src/index.test.ts`:
- Around line 348-381: The test named "passes a leading-dash query via the --
separator" does not actually test parsing a leading-dash query because it uses
"version" (without a leading dash) in the searchProgram.parseAsync arguments.
Replace "version" with a dash-prefixed argument such as "--version" or "-v" to
properly test the intended behavior of parsing arguments that start with dashes
when passed after the "--" separator.
In `@packages/cli/src/index.ts`:
- Around line 592-599: The limit option validation in the CLI is accepting
malformed input because Number.parseInt() truncates invalid values like "2.5" or
"2abc" to valid integers like 2. Replace the current parsing mechanism with
strict numeric parsing that rejects any input that is not a valid integer string
(e.g., rejects strings with decimal points or non-numeric characters). Ensure
the validation checks the original string format before or during parsing to
prevent silent truncation of invalid values, so that inputs like "2.5" or "2abc"
are properly rejected with an error message instead of being silently converted.
In `@packages/cli/src/mcp/local-backend.ts`:
- Around line 125-127: The tokenization at the character comparison for "(" and
")" produces separate tokens, which means NEAR( is never captured as a single
word token. This causes the regex pattern checks at lines 165 and 252 that look
for ^NEAR\s*\( to never match, making NEAR(...) queries unreachable in the
advanced FTS path. Instead of matching NEAR\s*\( within a single word token,
modify the parsing logic to detect when a NEAR token is immediately followed by
an lparen token in the token stream, and handle it appropriately as a NEAR
expression rather than treating it as separate terms or groups.
In `@packages/db/benchmarks/hosted-retrieval.ts`:
- Around line 54-63: The post function's fetch call lacks a timeout mechanism,
which could cause the benchmark to hang indefinitely on network stalls. Add an
AbortController to the post function, set up a timeout that aborts the request
after a reasonable duration (e.g., 30 seconds), pass the abort signal to the
fetch options, and ensure any AbortError exceptions are properly caught and
handled along with other fetch errors.
- Around line 27-51: The runAlchemy function spawns a child process without any
timeout protection, which can cause the benchmark workflow to hang indefinitely
if the subprocess gets stuck. Add a timeout mechanism to the Promise in the
runAlchemy function that will reject the promise if the child process does not
complete within a reasonable time limit. Ensure the timeout is properly cleared
when the process exits successfully to avoid any lingering timers, and update
the rejection logic to distinguish between timeout failures and normal exit code
failures.
In `@packages/infra/src/retrieval-benchmark-worker.ts`:
- Around line 61-77: Add input validation for the topK parameter to ensure it is
a positive integer within acceptable bounds, similar to how queries is validated
with Array.isArray. Additionally, before calling env.SOLUTION_VECTORS.query with
the vectors returned from the embed function, validate that the embeddings array
has the correct length and dimensions match expectations, preventing invalid
queries from reaching the vector database and causing 500 errors.
🪄 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: 163322b0-e8a2-4f6d-aaa9-233e749e4269
📒 Files selected for processing (50)
.gitignoreapps/web/src/app/(site)/solutions/solutions-page.tsxapps/web/src/components/webmcp-provider.test.tsapps/web/src/components/webmcp-provider.test.tsxapps/web/src/components/webmcp-provider.tsxpackage.jsonpackages/api/src/routers/solutions.test.tspackages/api/src/routers/solutions.tspackages/api/src/semantic/search.test.tspackages/api/src/semantic/search.tspackages/cli/benchmarks/local-embeddings/README.mdpackages/cli/benchmarks/local-embeddings/benchmark.test.tspackages/cli/benchmarks/local-embeddings/corpus.tspackages/cli/benchmarks/local-embeddings/metrics.tspackages/cli/benchmarks/local-embeddings/models.tspackages/cli/benchmarks/local-embeddings/profiles.tspackages/cli/benchmarks/local-embeddings/quality-gate.tspackages/cli/benchmarks/local-embeddings/run.tspackages/cli/benchmarks/local-embeddings/tsconfig.jsonpackages/cli/benchmarks/local-embeddings/types.tspackages/cli/benchmarks/local-embeddings/worker.tspackages/cli/commands/search-solutions.mdpackages/cli/package.jsonpackages/cli/skills/clankeroverflow-cli/SKILL.mdpackages/cli/skills/clankeroverflow-mcp/SKILL.mdpackages/cli/src/index.test.tspackages/cli/src/index.tspackages/cli/src/mcp/auto-search.test.tspackages/cli/src/mcp/auto-search.tspackages/cli/src/mcp/backend.tspackages/cli/src/mcp/format.tspackages/cli/src/mcp/local-backend.test.tspackages/cli/src/mcp/local-backend.tspackages/cli/src/mcp/local-semantic.test.tspackages/cli/src/mcp/local-semantic.tspackages/cli/src/mcp/remote-backend.tspackages/cli/src/mcp/server.test.tspackages/cli/src/mcp/server.tspackages/db/benchmarks/hosted-retrieval.tspackages/db/package.jsonpackages/db/src/create-search-indexes.tspackages/db/src/migrations/0006_unicode_search.sqlpackages/db/src/migrations/meta/_journal.jsonpackages/db/src/search.test.tspackages/db/src/search.tspackages/db/tsconfig.jsonpackages/infra/retrieval-benchmark.run.tspackages/infra/src/alchemy-run.test.tspackages/infra/src/retrieval-benchmark-worker.tspackages/infra/tsconfig.json
Comment on lines
+59
to
+60
| bootstrapSamples = 10_000, | ||
| seed = 20_260_621, |
There was a problem hiding this comment.
Validate bootstrapSamples to avoid silent invalid confidence intervals.
bootstrapSamples <= 0 currently yields empty bootstrap draws and forces low/high to 0, producing misleading metric summaries.
Suggested fix
export function summarizeMetrics(
queries: readonly BenchmarkQuery[],
rankings: ReadonlyMap<string, readonly string[]>,
bootstrapSamples = 10_000,
seed = 20_260_621,
): MetricSummary {
if (queries.length === 0) throw new Error("Cannot summarize an empty query set");
+ if (!Number.isInteger(bootstrapSamples) || bootstrapSamples <= 0) {
+ throw new Error("bootstrapSamples must be a positive integer");
+ }
const rows = queries.map((query) => queryMetrics(query, rankings.get(query.id) ?? []));Also applies to: 68-81
🤖 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/benchmarks/local-embeddings/metrics.ts` around lines 59 - 60,
The bootstrapSamples parameter lacks validation, allowing values less than or
equal to zero to be passed, which results in empty bootstrap draws and
misleading confidence interval values (low/high forced to 0). Add validation
logic in the function or method that uses the bootstrapSamples parameter (around
the parameter definition or at the start of the function logic) to ensure
bootstrapSamples is greater than zero. If an invalid value is provided, either
throw an error with a clear message or provide a sensible default. Apply this
same validation check to any other places where bootstrap sampling occurs, as
indicated by the "Also applies to" section spanning lines 68-81.
Comment on lines
+76
to
+90
| const temporaryPath = `${modelPath}.tmp-${process.pid}`; | ||
| rmSync(temporaryPath, { force: true }); | ||
| const response = await fetch(model.url); | ||
| if (!response.ok || !response.body) { | ||
| throw new Error(`Unable to download ${model.label} (${response.status})`); | ||
| } | ||
| await pipeline(Readable.fromWeb(response.body as any), createWriteStream(temporaryPath)); | ||
| const actualHash = await fileSha256(temporaryPath); | ||
| if (actualHash !== model.sha256) { | ||
| rmSync(temporaryPath, { force: true }); | ||
| throw new Error( | ||
| `Checksum mismatch for ${model.fileName}: expected ${model.sha256}, got ${actualHash}`, | ||
| ); | ||
| } | ||
| renameSync(temporaryPath, modelPath); |
There was a problem hiding this comment.
Harden temporary artifact handling in ensureModel to avoid collisions and stale files.
Line 76 uses a PID-only temp path, so concurrent downloads in the same process can clobber each other. Also, failures before checksum mismatch (e.g., fetch/pipeline errors) leave temp files behind. Use a per-call unique temp name and unconditional cleanup in finally.
Suggested patch
-import { createHash } from "node:crypto";
+import { createHash, randomUUID } from "node:crypto";
@@
- const temporaryPath = `${modelPath}.tmp-${process.pid}`;
- rmSync(temporaryPath, { force: true });
- const response = await fetch(model.url);
- if (!response.ok || !response.body) {
- throw new Error(`Unable to download ${model.label} (${response.status})`);
- }
- await pipeline(Readable.fromWeb(response.body as any), createWriteStream(temporaryPath));
- const actualHash = await fileSha256(temporaryPath);
- if (actualHash !== model.sha256) {
- rmSync(temporaryPath, { force: true });
- throw new Error(
- `Checksum mismatch for ${model.fileName}: expected ${model.sha256}, got ${actualHash}`,
- );
- }
- renameSync(temporaryPath, modelPath);
- return modelPath;
+ const temporaryPath = `${modelPath}.tmp-${process.pid}-${randomUUID()}`;
+ let committed = false;
+ try {
+ const response = await fetch(model.url);
+ if (!response.ok || !response.body) {
+ throw new Error(`Unable to download ${model.label} (${response.status})`);
+ }
+ await pipeline(Readable.fromWeb(response.body as any), createWriteStream(temporaryPath));
+ const actualHash = await fileSha256(temporaryPath);
+ if (actualHash !== model.sha256) {
+ throw new Error(
+ `Checksum mismatch for ${model.fileName}: expected ${model.sha256}, got ${actualHash}`,
+ );
+ }
+ renameSync(temporaryPath, modelPath);
+ committed = true;
+ return modelPath;
+ } finally {
+ if (!committed) rmSync(temporaryPath, { force: true });
+ }📝 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.
Suggested change
| const temporaryPath = `${modelPath}.tmp-${process.pid}`; | |
| rmSync(temporaryPath, { force: true }); | |
| const response = await fetch(model.url); | |
| if (!response.ok || !response.body) { | |
| throw new Error(`Unable to download ${model.label} (${response.status})`); | |
| } | |
| await pipeline(Readable.fromWeb(response.body as any), createWriteStream(temporaryPath)); | |
| const actualHash = await fileSha256(temporaryPath); | |
| if (actualHash !== model.sha256) { | |
| rmSync(temporaryPath, { force: true }); | |
| throw new Error( | |
| `Checksum mismatch for ${model.fileName}: expected ${model.sha256}, got ${actualHash}`, | |
| ); | |
| } | |
| renameSync(temporaryPath, modelPath); | |
| const temporaryPath = `${modelPath}.tmp-${process.pid}-${randomUUID()}`; | |
| let committed = false; | |
| try { | |
| const response = await fetch(model.url); | |
| if (!response.ok || !response.body) { | |
| throw new Error(`Unable to download ${model.label} (${response.status})`); | |
| } | |
| await pipeline(Readable.fromWeb(response.body as any), createWriteStream(temporaryPath)); | |
| const actualHash = await fileSha256(temporaryPath); | |
| if (actualHash !== model.sha256) { | |
| throw new Error( | |
| `Checksum mismatch for ${model.fileName}: expected ${model.sha256}, got ${actualHash}`, | |
| ); | |
| } | |
| renameSync(temporaryPath, modelPath); | |
| committed = true; | |
| return modelPath; | |
| } finally { | |
| if (!committed) rmSync(temporaryPath, { force: true }); | |
| } |
🤖 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/benchmarks/local-embeddings/models.ts` around lines 76 - 90, The
temporary file handling in the ensureModel function has two issues: the temp
path uses only the process ID, causing concurrent downloads to collide and
overwrite each other, and temporary files are only cleaned up when a checksum
mismatch occurs, leaving stale files behind on other failures like fetch or
pipeline errors. Replace the PID-based temporaryPath with a unique identifier
per call (such as a UUID), and wrap the fetch, pipeline, and checksum
verification logic in a try-finally block to ensure the temporary file is always
cleaned up with rmSync regardless of whether an error occurs or succeeds.
Comment on lines
+115
to
+137
| async function runWorker<T>(input: Record<string, unknown>): Promise<T> { | ||
| const workerPath = join(dirname(fileURLToPath(import.meta.url)), "worker.ts"); | ||
| const child = spawn(process.execPath, [...process.execArgv, workerPath], { | ||
| cwd: process.cwd(), | ||
| env: { ...process.env, CLANKER_BENCHMARK_WORKER_INPUT: JSON.stringify(input) }, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); | ||
| let stdout = ""; | ||
| let stderr = ""; | ||
| child.stdout.setEncoding("utf8"); | ||
| child.stderr.setEncoding("utf8"); | ||
| child.stdout.on("data", (chunk) => (stdout += chunk)); | ||
| child.stderr.on("data", (chunk) => (stderr += chunk)); | ||
| const exitCode = await new Promise<number>((resolveExit, reject) => { | ||
| child.on("error", reject); | ||
| child.on("exit", (code) => resolveExit(code ?? 1)); | ||
| }); | ||
| if (exitCode !== 0) | ||
| throw new Error(`Benchmark worker failed (${exitCode})\n${stderr}\n${stdout}`); | ||
| const marker = stdout.split("\n").find((line) => line.startsWith("CLANKER_BENCHMARK_RESULT=")); | ||
| if (!marker) throw new Error(`Benchmark worker returned no result\n${stderr}\n${stdout}`); | ||
| return JSON.parse(marker.slice("CLANKER_BENCHMARK_RESULT=".length)) as T; | ||
| } |
There was a problem hiding this comment.
Add a timeout for worker subprocess execution (Line 115).
runWorker can hang forever if the child never exits (model load deadlock, stuck I/O), which blocks the entire benchmark run.
Proposed fix
async function runWorker<T>(input: Record<string, unknown>): Promise<T> {
const workerPath = join(dirname(fileURLToPath(import.meta.url)), "worker.ts");
const child = spawn(process.execPath, [...process.execArgv, workerPath], {
@@
child.stdout.on("data", (chunk) => (stdout += chunk));
child.stderr.on("data", (chunk) => (stderr += chunk));
+ const timeoutMs = 10 * 60 * 1000;
+ const timer = setTimeout(() => {
+ child.kill("SIGKILL");
+ }, timeoutMs);
const exitCode = await new Promise<number>((resolveExit, reject) => {
child.on("error", reject);
child.on("exit", (code) => resolveExit(code ?? 1));
});
+ clearTimeout(timer);
if (exitCode !== 0)
throw new Error(`Benchmark worker failed (${exitCode})\n${stderr}\n${stdout}`);🤖 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/benchmarks/local-embeddings/run.ts` around lines 115 - 137, The
runWorker function lacks a timeout mechanism, which means if the child process
hangs (due to model load deadlock or stuck I/O), the entire benchmark run will
hang indefinitely. Add a timeout to the Promise that waits for the child process
to exit. Use Promise.race to combine the exit promise with a timeout promise,
and if the timeout fires, kill the child process using child.kill() and throw an
appropriate error message indicating the worker timed out. This ensures the
benchmark run can recover from hung worker processes.
Comment on lines
+106
to
+123
| async function runCold(input: WorkerInput): Promise<ColdWorkerResult> { | ||
| const loaded = await loadEmbedder(input); | ||
| const query = formatQuery(input.model, "native", benchmarkCorpus.queries[0]!.text); | ||
| const started = performance.now(); | ||
| await loaded.embed(query); | ||
| const firstQueryMs = performance.now() - started; | ||
| const result: ColdWorkerResult = { | ||
| model: input.model, | ||
| backend: input.backend, | ||
| resolvedBackend: String(loaded.llama.gpu), | ||
| loadMs: loaded.loadMs, | ||
| firstQueryMs, | ||
| peakRssBytes: peakRssBytes(), | ||
| }; | ||
| await loaded.context.dispose(); | ||
| await loaded.model.dispose(); | ||
| await loaded.llama.dispose(); | ||
| return result; |
There was a problem hiding this comment.
Add try/finally cleanup around benchmark execution paths.
Line 120-122 and Line 215-219 dispose resources only on success. If any step throws, model/context/llama handles and temp DB directories can leak. Wrap runCold/runFull bodies with try/finally so cleanup always runs.
Suggested cleanup structure
async function runCold(input: WorkerInput): Promise<ColdWorkerResult> {
- const loaded = await loadEmbedder(input);
- const query = formatQuery(input.model, "native", benchmarkCorpus.queries[0]!.text);
- const started = performance.now();
- await loaded.embed(query);
- const firstQueryMs = performance.now() - started;
- const result: ColdWorkerResult = {
- model: input.model,
- backend: input.backend,
- resolvedBackend: String(loaded.llama.gpu),
- loadMs: loaded.loadMs,
- firstQueryMs,
- peakRssBytes: peakRssBytes(),
- };
- await loaded.context.dispose();
- await loaded.model.dispose();
- await loaded.llama.dispose();
- return result;
+ const loaded = await loadEmbedder(input);
+ try {
+ const query = formatQuery(input.model, "native", benchmarkCorpus.queries[0]!.text);
+ const started = performance.now();
+ await loaded.embed(query);
+ const firstQueryMs = performance.now() - started;
+ return {
+ model: input.model,
+ backend: input.backend,
+ resolvedBackend: String(loaded.llama.gpu),
+ loadMs: loaded.loadMs,
+ firstQueryMs,
+ peakRssBytes: peakRssBytes(),
+ };
+ } finally {
+ await loaded.context.dispose();
+ await loaded.model.dispose();
+ await loaded.llama.dispose();
+ }
} async function runFull(input: WorkerInput): Promise<FullWorkerResult> {
const loaded = await loadEmbedder(input);
- const indexRuns: FullWorkerResult["indexRuns"] = [];
- let retained: Awaited<ReturnType<typeof createIndexedDb>> | undefined;
- ...
- retained.db.close();
- rmSync(retained.directory, { recursive: true, force: true });
- await loaded.context.dispose();
- await loaded.model.dispose();
- await loaded.llama.dispose();
- return result;
+ let retained: Awaited<ReturnType<typeof createIndexedDb>> | undefined;
+ try {
+ const indexRuns: FullWorkerResult["indexRuns"] = [];
+ ...
+ return result;
+ } finally {
+ if (retained) {
+ retained.db.close();
+ rmSync(retained.directory, { recursive: true, force: true });
+ }
+ await loaded.context.dispose();
+ await loaded.model.dispose();
+ await loaded.llama.dispose();
+ }
}Also applies to: 153-220
🤖 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/benchmarks/local-embeddings/worker.ts` around lines 106 - 123,
The runCold function (and similarly runFull as mentioned in the review) only
disposes of resources when execution completes successfully. If any error occurs
during loadEmbedder, formatQuery, embed, or performance measurements, the
cleanup calls to loaded.context.dispose(), loaded.model.dispose(), and
loaded.llama.dispose() will not execute, causing resource leaks. Wrap the main
function body in a try/finally block, moving all three dispose calls into the
finally block to ensure cleanup always runs regardless of whether an error was
thrown.
Comment on lines
+348
to
+381
| test("passes a leading-dash query via the -- separator", async () => { | ||
| await withLocalCliEnv(async () => { | ||
| const logProgram = createProgram(); | ||
| await logProgram.parseAsync([ | ||
| "node", | ||
| "test", | ||
| "log", | ||
| "--problem", | ||
| "Negative version string mismatch", | ||
| "--solution", | ||
| "Parse the version as a SemVer range", | ||
| "--tags", | ||
| "versioning", | ||
| ]); | ||
| consoleLogMock.mockClear(); | ||
|
|
||
| const searchProgram = createProgram(); | ||
| await searchProgram.parseAsync([ | ||
| "node", | ||
| "test", | ||
| "search", | ||
| "--", | ||
| "version", | ||
| "--mode", | ||
| "keyword", | ||
| ]); | ||
|
|
||
| expect(fetchMock).not.toHaveBeenCalled(); | ||
| expect(consoleLogMock).toHaveBeenCalledWith( | ||
| expect.stringContaining("Negative version string mismatch"), | ||
| ); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Leading-dash test does not actually test a leading-dash query.
The test uses "version" (not a dash-prefixed token) and puts --mode after --, so it doesn’t validate the intended parsing path.
Proposed fix
const searchProgram = createProgram();
await searchProgram.parseAsync([
"node",
"test",
"search",
+ "--mode",
+ "keyword",
"--",
- "version",
- "--mode",
- "keyword",
+ "-1",
]);📝 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.
Suggested change
| test("passes a leading-dash query via the -- separator", async () => { | |
| await withLocalCliEnv(async () => { | |
| const logProgram = createProgram(); | |
| await logProgram.parseAsync([ | |
| "node", | |
| "test", | |
| "log", | |
| "--problem", | |
| "Negative version string mismatch", | |
| "--solution", | |
| "Parse the version as a SemVer range", | |
| "--tags", | |
| "versioning", | |
| ]); | |
| consoleLogMock.mockClear(); | |
| const searchProgram = createProgram(); | |
| await searchProgram.parseAsync([ | |
| "node", | |
| "test", | |
| "search", | |
| "--", | |
| "version", | |
| "--mode", | |
| "keyword", | |
| ]); | |
| expect(fetchMock).not.toHaveBeenCalled(); | |
| expect(consoleLogMock).toHaveBeenCalledWith( | |
| expect.stringContaining("Negative version string mismatch"), | |
| ); | |
| }); | |
| }); | |
| test("passes a leading-dash query via the -- separator", async () => { | |
| await withLocalCliEnv(async () => { | |
| const logProgram = createProgram(); | |
| await logProgram.parseAsync([ | |
| "node", | |
| "test", | |
| "log", | |
| "--problem", | |
| "Negative version string mismatch", | |
| "--solution", | |
| "Parse the version as a SemVer range", | |
| "--tags", | |
| "versioning", | |
| ]); | |
| consoleLogMock.mockClear(); | |
| const searchProgram = createProgram(); | |
| await searchProgram.parseAsync([ | |
| "node", | |
| "test", | |
| "search", | |
| "--mode", | |
| "keyword", | |
| "--", | |
| "-1", | |
| ]); | |
| expect(fetchMock).not.toHaveBeenCalled(); | |
| expect(consoleLogMock).toHaveBeenCalledWith( | |
| expect.stringContaining("Negative version string mismatch"), | |
| ); | |
| }); | |
| }); |
🤖 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/index.test.ts` around lines 348 - 381, The test named
"passes a leading-dash query via the -- separator" does not actually test
parsing a leading-dash query because it uses "version" (without a leading dash)
in the searchProgram.parseAsync arguments. Replace "version" with a
dash-prefixed argument such as "--version" or "-v" to properly test the intended
behavior of parsing arguments that start with dashes when passed after the "--"
separator.
Comment on lines
+592
to
+599
| if (options.limit !== undefined) { | ||
| if (limit === undefined || !Number.isInteger(limit) || limit < SEARCH_LIMIT_MIN) { | ||
| console.error( | ||
| pc.red(pc.bold("✖ Error: ")) + | ||
| pc.red(`--limit must be an integer of at least ${SEARCH_LIMIT_MIN}`), | ||
| ); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Use strict numeric parsing for local embed --limit.
Number.parseInt() truncates invalid values ("2.5", "2abc") so they pass validation as 2. This silently accepts malformed input and changes requested behavior.
Proposed fix
- const limit =
- options.limit === undefined ? undefined : Number.parseInt(String(options.limit), 10);
+ const limit = options.limit === undefined ? undefined : Number(options.limit);
if (options.limit !== undefined) {
if (limit === undefined || !Number.isInteger(limit) || limit < SEARCH_LIMIT_MIN) {
console.error(
pc.red(pc.bold("✖ Error: ")) +
pc.red(`--limit must be an integer of at least ${SEARCH_LIMIT_MIN}`),
);
process.exit(1);
}
}🤖 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/index.ts` around lines 592 - 599, The limit option
validation in the CLI is accepting malformed input because Number.parseInt()
truncates invalid values like "2.5" or "2abc" to valid integers like 2. Replace
the current parsing mechanism with strict numeric parsing that rejects any input
that is not a valid integer string (e.g., rejects strings with decimal points or
non-numeric characters). Ensure the validation checks the original string format
before or during parsing to prevent silent truncation of invalid values, so that
inputs like "2.5" or "2abc" are properly rejected with an error message instead
of being silently converted.
Comment on lines
+125
to
+127
| if (char === "(" || char === ")") { | ||
| flushWord(); | ||
| tokens.push(char === "(" ? { type: "lparen" } : { type: "rparen" }); |
There was a problem hiding this comment.
NEAR(...) parsing is currently unreachable in the advanced FTS path.
At Line 125-127, tokenization splits ( into a separate token, so NEAR( is never present inside a single word token. Because of that, the ^NEAR\s*\( checks at Line 165 and Line 252 never match, and NEAR(...) queries are rendered as ordinary terms/groups instead of NEAR expressions.
Also applies to: 157-166, 252-254
🤖 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 125 - 127, The
tokenization at the character comparison for "(" and ")" produces separate
tokens, which means NEAR( is never captured as a single word token. This causes
the regex pattern checks at lines 165 and 252 that look for ^NEAR\s*\( to never
match, making NEAR(...) queries unreachable in the advanced FTS path. Instead of
matching NEAR\s*\( within a single word token, modify the parsing logic to
detect when a NEAR token is immediately followed by an lparen token in the token
stream, and handle it appropriately as a NEAR expression rather than treating it
as separate terms or groups.
Comment on lines
+27
to
+51
| function runAlchemy(command: "deploy" | "destroy") { | ||
| return new Promise<string>((resolveRun, reject) => { | ||
| const child = spawn("pnpm", ["exec", "alchemy", command, entrypoint, "--stage", stage], { | ||
| cwd: infraDirectory, | ||
| env: { ...process.env, RETRIEVAL_BENCHMARK_TOKEN: token }, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }); | ||
| let stdout = ""; | ||
| let stderr = ""; | ||
| child.stdout.setEncoding("utf8"); | ||
| child.stderr.setEncoding("utf8"); | ||
| child.stdout.on("data", (chunk) => { | ||
| stdout += chunk; | ||
| process.stdout.write(chunk); | ||
| }); | ||
| child.stderr.on("data", (chunk) => { | ||
| stderr += chunk; | ||
| process.stderr.write(chunk); | ||
| }); | ||
| child.on("error", reject); | ||
| child.on("exit", (code) => { | ||
| if (code === 0) resolveRun(stdout); | ||
| else reject(new Error(`Alchemy ${command} failed (${code})\n${stderr}`)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Guard alchemy deploy/destroy subprocesses with a timeout (Line 27).
A stuck pnpm exec alchemy ... process currently has no upper bound and can wedge the benchmark workflow.
Proposed fix
function runAlchemy(command: "deploy" | "destroy") {
return new Promise<string>((resolveRun, reject) => {
const child = spawn("pnpm", ["exec", "alchemy", command, entrypoint, "--stage", stage], {
@@
let stdout = "";
let stderr = "";
+ const timeout = setTimeout(() => {
+ child.kill("SIGKILL");
+ reject(new Error(`Alchemy ${command} timed out`));
+ }, 10 * 60 * 1000);
@@
child.on("exit", (code) => {
+ clearTimeout(timeout);
if (code === 0) resolveRun(stdout);
else reject(new Error(`Alchemy ${command} failed (${code})\n${stderr}`));
});
});
}🤖 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/db/benchmarks/hosted-retrieval.ts` around lines 27 - 51, The
runAlchemy function spawns a child process without any timeout protection, which
can cause the benchmark workflow to hang indefinitely if the subprocess gets
stuck. Add a timeout mechanism to the Promise in the runAlchemy function that
will reject the promise if the child process does not complete within a
reasonable time limit. Ensure the timeout is properly cleared when the process
exits successfully to avoid any lingering timers, and update the rejection logic
to distinguish between timeout failures and normal exit code failures.
Comment on lines
+54
to
+63
| async function post<T>(url: string, path: string, body: unknown): Promise<T> { | ||
| const response = await fetch(new URL(path, url), { | ||
| method: "POST", | ||
| headers: { authorization: `Bearer ${token}`, "content-type": "application/json" }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
| const payload = await response.json(); | ||
| if (!response.ok) | ||
| throw new Error(`${path} failed (${response.status}): ${JSON.stringify(payload)}`); | ||
| return payload as T; |
There was a problem hiding this comment.
Add HTTP timeouts for benchmark worker calls (Line 54).
fetch without an abort timeout can block the run indefinitely on network stalls.
Proposed fix
async function post<T>(url: string, path: string, body: unknown): Promise<T> {
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 30_000);
const response = await fetch(new URL(path, url), {
method: "POST",
headers: { authorization: `Bearer ${token}`, "content-type": "application/json" },
body: JSON.stringify(body),
+ signal: controller.signal,
});
+ clearTimeout(timeout);
const payload = await response.json();🤖 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/db/benchmarks/hosted-retrieval.ts` around lines 54 - 63, The post
function's fetch call lacks a timeout mechanism, which could cause the benchmark
to hang indefinitely on network stalls. Add an AbortController to the post
function, set up a timeout that aborts the request after a reasonable duration
(e.g., 30 seconds), pass the abort signal to the fetch options, and ensure any
AbortError exceptions are properly caught and handled along with other fetch
errors.
Comment on lines
+61
to
+77
| const { queries, topK = 20 } = (await request.json()) as { | ||
| queries?: Array<{ id: string; text: string }>; | ||
| topK?: number; | ||
| }; | ||
| if (!Array.isArray(queries) || queries.length === 0) { | ||
| return json({ error: "queries are required" }, 400); | ||
| } | ||
| const rankings: Array<{ queryId: string; ids: string[] }> = []; | ||
| for (let start = 0; start < queries.length; start += BATCH_SIZE) { | ||
| const batch = queries.slice(start, start + BATCH_SIZE); | ||
| const vectors = await embed( | ||
| env, | ||
| batch.map((query) => query.text), | ||
| ); | ||
| for (let index = 0; index < batch.length; index += 1) { | ||
| const result = await env.SOLUTION_VECTORS.query(vectors[index]!, { | ||
| topK: Math.min(50, Math.max(1, topK)), |
There was a problem hiding this comment.
Harden /query input and embedding validation (Line 61).
topK is not type-validated, and /query doesn’t verify embedding count/dimensions before querying. Both paths can produce avoidable 500s.
Proposed fix
if (request.method === "POST" && url.pathname === "/query") {
const { queries, topK = 20 } = (await request.json()) as {
queries?: Array<{ id: string; text: string }>;
topK?: number;
};
+ const parsedTopK = Number(topK);
+ if (!Number.isFinite(parsedTopK) || parsedTopK <= 0) {
+ return json({ error: "topK must be a positive number" }, 400);
+ }
+ const clampedTopK = Math.min(50, Math.max(1, Math.trunc(parsedTopK)));
if (!Array.isArray(queries) || queries.length === 0) {
return json({ error: "queries are required" }, 400);
}
@@
const vectors = await embed(
env,
batch.map((query) => query.text),
);
+ if (vectors.length !== batch.length || vectors.some((vector) => vector.length !== 768)) {
+ throw new Error("Unexpected embedding dimensions");
+ }
for (let index = 0; index < batch.length; index += 1) {
const result = await env.SOLUTION_VECTORS.query(vectors[index]!, {
- topK: Math.min(50, Math.max(1, topK)),
+ topK: clampedTopK,
});🤖 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/infra/src/retrieval-benchmark-worker.ts` around lines 61 - 77, Add
input validation for the topK parameter to ensure it is a positive integer
within acceptable bounds, similar to how queries is validated with
Array.isArray. Additionally, before calling env.SOLUTION_VECTORS.query with the
vectors returned from the embed function, validate that the embeddings array has
the correct length and dimensions match expectations, preventing invalid queries
from reaching the vector database and causing 500 errors.
Merged
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.
What changed
Why
The previous strict FTS strategy returned almost no benchmark results, so hybrid retrieval contributed effectively nothing. It also stripped non-ASCII text in hosted search and could interpret technical leading hyphens poorly. This change improves lexical recall without allowing relaxed results to outrank exact matches.
Impact
Explicit keyword search returns exact matches first and fills remaining slots with relaxed prefix/fuzzy matches. Auto search preserves precision while gaining a useful fallback when semantic infrastructure is unavailable. Hosted RRF remains disabled until the disposable benchmark passes its nDCG, MRR, Recall@10, and slice-regression thresholds.
Validation
oxlintpassed with existing unrelated warningsgit diff --checkpassedThe credentialed disposable Cloudflare benchmark was not run in this environment, so semantic-first remains the hosted default.
Summary by CodeRabbit
New Features
Improvements