Improvement/improve retrieval - #66
Merged
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.
|
Warning Review limit reached
More reviews will be available in 52 minutes and 38 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 (1)
WalkthroughThis PR introduces a two-tier keyword search strategy ( ChangesKeyword Search Strategy Refactor
Benchmark Infrastructure
Version Bumps and Scripts
Sequence Diagram(s)sequenceDiagram
participant Client as CLI / WebMCP
participant AutoSearch as searchWithAutoFallback
participant RemoteBackend as RemoteBackend
participant TRPC as tRPC solutions.search
participant DB as PostgreSQL searchSolutions
Client->>AutoSearch: query, limit, allowHybridFallback
AutoSearch->>RemoteBackend: searchExactKeyword({ query, limit })
RemoteBackend->>TRPC: mode:"keyword", keywordStrategy:"exact"
TRPC->>DB: strategy:"exact"
DB-->>TRPC: results
TRPC-->>RemoteBackend: results
RemoteBackend-->>AutoSearch: results
alt exact results found
AutoSearch-->>Client: results + attempts[keyword exact]
else empty
AutoSearch->>RemoteBackend: search({ mode:"hybrid" })
RemoteBackend->>TRPC: mode:"hybrid"
TRPC-->>RemoteBackend: hybridResults
alt hybrid succeeds
AutoSearch-->>Client: hybridResults + attempts[keyword exact, hybrid]
else hybrid unavailable or error
AutoSearch->>RemoteBackend: search({ mode:"keyword", keywordStrategy:"tiered" })
RemoteBackend->>TRPC: mode:"keyword", keywordStrategy:"tiered"
TRPC->>DB: strategy:"tiered"
DB-->>AutoSearch: tieredResults
AutoSearch-->>Client: tieredResults + attempts[keyword exact, hybrid?, keyword tiered]
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 11
🧹 Nitpick comments (1)
packages/db/tsconfig.json (1)
10-11: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winKeep benchmark sources in a typechecked TS project.
This excludes
benchmarks/**from the package TS project while the same package now ships a benchmark entry script. Consider a dedicatedtsconfig.benchmarks.json(or include this folder) so CI catches drift before runtime.🤖 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/tsconfig.json` around lines 10 - 11, The exclude array in the tsconfig.json file currently excludes the benchmarks directory from type checking. Since the package now includes a benchmark entry script, either remove "benchmarks" from the exclude array to include benchmarks in the main type checking, or create a dedicated tsconfig.benchmarks.json file that includes the benchmarks folder so TypeScript type checking can catch any drift in the benchmarks before runtime.
🤖 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 `@apps/web/src/components/webmcp-provider.tsx`:
- Around line 74-77: When recording the attempt result in the return statement
for non-auto mode keyword searches, the attempts array is missing the
keywordStrategy field. Since the search function is called with "tiered" as the
strategy when mode is "keyword" (as seen on line 75), add the keywordStrategy
property to the attempts object to include this execution metadata. The
keywordStrategy should be set to "tiered" when mode equals "keyword", otherwise
it should be undefined or omitted, to ensure the recorded metadata matches what
was actually passed to the search function.
In `@packages/cli/benchmarks/local-embeddings/metrics.ts`:
- Around line 56-85: The summarizeMetrics function does not validate the
bootstrapSamples parameter, so if it is set to 0 or a negative number, the
samples array becomes empty and the quantile function silently returns 0 for
both low and high confidence interval bounds, producing misleading results. Add
a validation check at the beginning of the summarizeMetrics function (similar to
the existing check for queries.length === 0) to ensure bootstrapSamples is
greater than 0 and throw an appropriate error if the validation fails.
In `@packages/cli/benchmarks/local-embeddings/models.ts`:
- Around line 78-83: Add a bounded timeout to prevent indefinite hangs during
model downloads in the fetch call. Implement an AbortController with a timeout
that will abort the request after a specified duration. Wrap the entire download
block (the fetch, pipeline, and fileSha256 operations) in a try-catch block, and
in the catch handler, clean up the temporary file at temporaryPath using
filesystem operations like unlink or rm before re-throwing the error. This
ensures that partial downloads are removed when the operation fails due to
timeout or network errors.
In `@packages/cli/benchmarks/local-embeddings/README.md`:
- Around line 27-29: The README.md file at lines 27-29 contains inaccurate
documentation about the download verification process. The text claims that
downloads are checked against both SHA-256 and byte size, but the actual
implementation in models.ts only validates SHA-256. Update the wording in the
README to accurately reflect that only SHA-256 verification is performed,
removing the reference to byte size validation. This ensures the documentation
accurately describes the current behavior of the benchmark tool.
In `@packages/cli/benchmarks/local-embeddings/worker.ts`:
- Around line 106-123: The runCold function does not handle exceptions safely
because cleanup code is only executed on the happy path. If any operation like
loaded.embed(query) or result assignments throws an exception, the dispose calls
for loaded.context, loaded.model, and loaded.llama will never execute, causing
resource leaks. Refactor the function by wrapping all operations starting from
loadEmbedder through the result object creation in a try block, then move all
three dispose calls (loaded.context.dispose, loaded.model.dispose, and
loaded.llama.dispose) into a finally block to ensure cleanup always runs
regardless of whether an exception occurs. Apply this same pattern to the other
worker functions mentioned in the comment (around lines 153-220).
In `@packages/cli/src/index.test.ts`:
- Around line 348-373: The test "passes a leading-dash query via the --
separator" does not actually test a leading-dash query because it uses "version"
as the positional argument after the -- separator. To fix this, replace the
string "version" with an actual leading-dash query string (such as "-version" or
"-mode") in the searchProgram.parseAsync call to properly validate that
leading-dash queries are handled correctly through the -- separator.
In `@packages/cli/src/index.ts`:
- Around line 590-599: The limit validation in the options parsing has a bug
where Number.parseInt truncates decimal inputs (e.g., "2.5" becomes 2), allowing
them to pass the Number.isInteger() check when they should fail. Replace the
Number.parseInt(String(options.limit), 10) call with
Number(String(options.limit)) to preserve decimal values so that inputs like 2.5
remain as floats and correctly fail the integer validation, ensuring the error
message about requiring an integer is properly enforced.
In `@packages/cli/src/mcp/local-backend.ts`:
- Around line 125-127: The tokenizeFtsQuery function at line 125 splits
parentheses into separate tokens, which prevents NEAR(...) syntax from being
recognized as a single token and prevents the NEAR passthrough logic at line 252
from executing. Additionally, the runtime search paths at lines 390/394 and 490
use localFtsQuery and localRelaxedFtsQuery which bypass the operator-aware
parsing validation in the main ftsQuery function, causing advanced FTS5 syntax
like AND/OR/NOT, column filters, and NEAR(...) to not be consistently applied.
Modify tokenizeFtsQuery to preserve parentheses as part of NEAR tokens instead
of splitting them, and refactor localFtsQuery and localRelaxedFtsQuery to apply
the same operator-aware parsing and validation logic as ftsQuery so that
advanced syntax is consistently handled across all search modes (keyword,
hybrid, and auto).
In `@packages/db/benchmarks/hosted-retrieval.ts`:
- Around line 54-63: The post function lacks a timeout mechanism on its fetch
call, which can cause the benchmark to hang indefinitely if a request stalls.
Add a timeout configuration to the fetch call in the post function by including
an AbortSignal with a reasonable deadline (typically a few seconds for API
calls). You can create an AbortController, set a timeout using setTimeout to
abort it after the desired duration, and pass the controller's signal in the
fetch options to ensure requests don't hang the benchmark run.
In `@packages/infra/src/retrieval-benchmark-worker.ts`:
- Around line 41-43: The validation for the documents payload only checks if it
is a non-empty array, but does not validate the structure of individual document
items within that array. Malformed items (missing the text field or other
required properties) can pass through and cause errors later. Add additional
validation logic after the array length check to iterate through each document
and verify that required properties (like text) are present. If any document
item is malformed, return a 400 error with an appropriate message indicating the
invalid document structure. Apply the same validation pattern to the other
request payload validation around lines 61-67 to ensure consistency across all
ingestion points.
- Around line 71-77: The code at line 76 in the query loop assumes
vectors[index] exists without validation, which can lead to unclear errors if
Workers AI returns fewer vectors or wrong dimensions. Add the same embedding
sanity check that already exists in the /seed path to validate the vectors array
and its dimensions before accessing vectors[index] in the for loop that queries
SOLUTION_VECTORS. This will ensure the code fails fast with a clear error
message rather than throwing during the vector query operation.
---
Nitpick comments:
In `@packages/db/tsconfig.json`:
- Around line 10-11: The exclude array in the tsconfig.json file currently
excludes the benchmarks directory from type checking. Since the package now
includes a benchmark entry script, either remove "benchmarks" from the exclude
array to include benchmarks in the main type checking, or create a dedicated
tsconfig.benchmarks.json file that includes the benchmarks folder so TypeScript
type checking can catch any drift in the benchmarks before runtime.
🪄 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: 7b7dbbbe-83b4-40e3-9f3b-c35948ef6036
📒 Files selected for processing (53)
.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/.claude-plugin/plugin.jsonpackages/cli/.codex-plugin/plugin.jsonpackages/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/openclaw.plugin.jsonpackages/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
+125
to
+127
| if (char === "(" || char === ")") { | ||
| flushWord(); | ||
| tokens.push(char === "(" ? { type: "lparen" } : { type: "rparen" }); |
There was a problem hiding this comment.
Advanced FTS5 behavior is not applied on runtime search paths.
Line 390/Line 394 and Line 490 route keyword execution through localFtsQuery/localRelaxedFtsQuery, so operator-aware parsing/validation in ftsQuery never executes for actual keyword/hybrid searches. Also, the NEAR passthrough at Line 252 is unreachable because tokenizeFtsQuery splits ( into a separate token at Line 125, so NEAR(...) cannot remain a single token.
This creates a user-visible contract gap for advanced queries (AND/OR/NOT, column filters, NEAR(...)) and for syntax-rejection behavior.
Proposed direction
-export function searchLocalKeywordExact(db: LocalDb, queryText: string, limit: number) {
- return searchLocalKeywordExpression(db, localFtsQuery(queryText.trim()), limit);
-}
+export function searchLocalKeywordExact(db: LocalDb, queryText: string, limit: number) {
+ return searchLocalKeywordExpression(db, ftsQuery(queryText.trim()), limit);
+}Then align the hybrid keyword leg and relaxed fallback strategy so advanced syntax is preserved (or explicitly disallowed) consistently across keyword, hybrid, and auto.
Also applies to: 252-254, 389-395, 490-503
🤖 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
tokenizeFtsQuery function at line 125 splits parentheses into separate tokens,
which prevents NEAR(...) syntax from being recognized as a single token and
prevents the NEAR passthrough logic at line 252 from executing. Additionally,
the runtime search paths at lines 390/394 and 490 use localFtsQuery and
localRelaxedFtsQuery which bypass the operator-aware parsing validation in the
main ftsQuery function, causing advanced FTS5 syntax like AND/OR/NOT, column
filters, and NEAR(...) to not be consistently applied. Modify tokenizeFtsQuery
to preserve parentheses as part of NEAR tokens instead of splitting them, and
refactor localFtsQuery and localRelaxedFtsQuery to apply the same operator-aware
parsing and validation logic as ftsQuery so that advanced syntax is consistently
handled across all search modes (keyword, hybrid, and auto).
- webmcp-provider: record keywordStrategy in non-auto keyword attempts - metrics: validate bootstrapSamples is an integer >= 1 - models: add 5-min download timeout + temp-file cleanup on failure - worker: dispose model handles and temp dirs in try/finally - index: use Number() not parseInt() for embed --limit (decimals rejected) - index.test: actually exercise leading-dash query via -- separator - hosted-retrieval: add 60s timeout to benchmark post() requests - retrieval-benchmark-worker: validate item shapes + /query dimension check - db: add tsconfig.benchmarks.json + check-types script for benchmark typecheck - README: correct download verification wording (SHA-256 only)
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.
Summary by CodeRabbit
--for leading-dash queries.