Repository navigation
feat(search): offline BM25 lexical provider + pm eval relevance harness (pm-75k9, pm-u8n5) - #314
Merged
Merged
Conversation
Deliver two coherent post-v0.1 search capabilities under epic pm-ydp6
(Search & semantic retrieval), both validated end-to-end and at 100%
coverage.
Offline BM25 provider (pm-75k9)
- New pure module src/core/search/bm25.ts: Okapi BM25 (IDF + k1
term-frequency saturation + b length-normalization) scoring the
in-memory corpus with NO embedding service or vector store.
- Activated via search.provider=bm25 (always) or auto (offline fallback
only when no embedding/extension provider is available); plain keyword
mode is unchanged. Wired into runSearch for --semantic (pure BM25,
matched_fields:[bm25]) and --hybrid (blended with the keyword scorer
via combineHybridHits, which gained a dense-component label). The
exact full-ID/short-ID guarantee is preserved in both modes.
- Validated/persisted tuning knobs search.bm25.{k1,b}, settable via
pm config set search_bm25_k1/b.
Search relevance eval harness (pm-u8n5)
- New pure module src/core/search/eval.ts: nDCG@k, MRR@k, precision@k,
recall@k over a binary-relevance golden set + parseEvalQuerySet
validator.
- New core command `pm eval` (src/cli/commands/eval.ts): runs each
golden query through the live search path, reports per-query +
macro-aggregate metrics, and --fail-under turns it into a CI gate
(prints the report, exits non-zero on aggregate-nDCG regression).
- Registered as a core command (enum-contracts, EVAL_FLAG_CONTRACTS,
help-content, regenerated contracts fixture). Golden set lives at
<pmRoot>/search/eval-queries.json (gitignore negation keeps the
search cache dir ignored while tracking the curated file).
Docs: COMMANDS.md + CONFIGURATION.md cover the BM25 provider, the
search.provider bm25/auto values, the bm25 tuning knobs, and pm eval.
ADR pm-f2al records the design. All gates green: 100/100/100/100
coverage, static + docs-skills + typecheck + contracts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WiNZNrE3rjftCC1udbBLGt
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
/gemini review |
Owner
Author
|
@codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
✅ Action performedFull review finished. |
|
Warning Review limit reached
More reviews will be available in 40 minutes and 48 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: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (32)
📝 WalkthroughWalkthroughAdds two new features: (1) an in-process offline Okapi BM25 lexical ranker that activates for semantic/hybrid modes when ChangesOffline BM25 provider and
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as register-list-query (eval)
participant runEval
participant loadEvalQuerySet
participant runSearch
participant evaluateRanking
participant aggregateEvalMetrics
User->>CLI: pm eval --mode hybrid --k 10 --fail-under 0.5
CLI->>runEval: EvalOptions
runEval->>loadEvalQuerySet: resolve queries path, readFile
loadEvalQuerySet-->>runEval: EvalQuerySet
loop per EvalQuery
runEval->>runSearch: query text, mode, limit=k
runSearch-->>runEval: ranked SearchResult ids
runEval->>evaluateRanking: rankedIds, relevantIds, k
evaluateRanking-->>runEval: QueryEvalMetrics
end
runEval->>aggregateEvalMetrics: QueryEvalMetrics[]
aggregateEvalMetrics-->>runEval: AggregateEvalMetrics
runEval-->>CLI: EvalResult { passed, fail_under, queries }
alt passed = false
CLI-->>User: throw PmCliError (EXIT_CODE.GENERIC_FAILURE)
else passed = true
CLI-->>User: print EvalResult JSON
end
flowchart TD
runSearch --> resolveBuiltInBm25Mode
resolveBuiltInBm25Mode -- provider=bm25 --> explicit
resolveBuiltInBm25Mode -- provider=auto no embedding --> auto_fallback
resolveBuiltInBm25Mode -- other --> null_path
explicit --> computeBuiltInBm25Hits
auto_fallback --> emitOfflineWarning
emitOfflineWarning --> computeBuiltInBm25Hits
computeBuiltInBm25Hits --> flattenSearchCorpusText
computeBuiltInBm25Hits --> buildBm25Index
computeBuiltInBm25Hits --> scoreBm25Query
computeBuiltInBm25Hits -- hybrid mode --> combineHybridHits
combineHybridHits --> forceExactIdHitsToTop
null_path --> semanticProviderBlock
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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. 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 |
|
To use Codex here, create a Codex account and connect to github. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
tests/unit/core/search/eval-metrics.spec.ts (1)
132-143: ⚡ Quick winAdd a regression test for mixed-type
relevant_ids.Current parser tests cover missing/empty arrays but not
[string, non-string], which is a contract-sensitive validation edge case.Suggested test case
it("rejects a missing relevant_ids array", () => { expect(() => parseEvalQuerySet([{ query: "x" }])).toThrow(/"relevant_ids" array/); }); + it("rejects non-string relevant_ids entries", () => { + expect(() => parseEvalQuerySet([{ query: "x", relevant_ids: ["pm-1", 7] }])).toThrow(/array of strings/); + }); + it("rejects an empty relevant_ids list (after trimming)", () => { expect(() => parseEvalQuerySet([{ query: "x", relevant_ids: ["", " "] }])).toThrow(/at least one relevant id/); });🤖 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/core/search/eval-metrics.spec.ts` around lines 132 - 143, Add a new test case in the parseEvalQuerySet test suite to validate that mixed-type relevant_ids arrays are rejected. Create an it statement that calls parseEvalQuerySet with a query object containing relevant_ids with both string and non-string values (for example, strings mixed with numbers or objects), and expect it to throw an appropriate error. This test should be placed alongside the existing validation tests for missing arrays, empty lists, and invalid modes to ensure the parser properly validates the type consistency of relevant_ids elements.
🤖 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 `@CHANGELOG.md`:
- Line 8: In the CHANGELOG.md file, the offline/fallback embedding provider
entry currently mentions "TF-IDF or BM25" but only BM25 was actually implemented
and shipped. Remove the "TF-IDF or" portion from the changelog entry so it
accurately reflects that only the BM25 implementation was delivered, keeping the
rest of the description about the offline/fallback embedding provider feature
intact.
In `@src/cli/commands/eval.ts`:
- Around line 190-215: The issue is that metrics are being rounded before
aggregation, which can cause false pass/fail decisions around the failUnder
threshold. Store unrounded metrics from the evaluateRanking call (ndcg, mrr,
precision, recall) in the reports array without applying roundMetric() at the
point of insertion into the reports object. Then pass these unrounded metrics to
aggregateEvalMetrics() instead of the rounded ones. Only apply roundMetric() to
the final aggregateNdcg result after aggregateEvalMetrics() returns, just before
comparing it against the failUnder threshold in the passed variable assignment.
This ensures aggregate calculations work with full precision values rather than
pre-rounded ones.
In `@src/core/search/bm25.ts`:
- Around line 197-202: The resolveClampedParam function currently returns the
fallback value when a candidate is a finite number but outside the valid range,
which silently ignores user-configured values. Instead of rejecting out-of-range
finite values, clamp them to the min-max boundaries using Math.max(min,
Math.min(max, candidate)) to preserve user intent. Only return the fallback
value when the candidate is not a finite number, ensuring that values like
search.bm25.k1=5000 are clamped to the maximum allowed value rather than being
reset to defaults.
In `@src/core/search/eval.ts`:
- Around line 221-233: The current validation in the relevant_ids processing
silently filters out non-string values instead of rejecting malformed data.
After checking that entry.relevant_ids is an array, add explicit validation to
ensure all entries are strings, and throw an EvalQuerySetError with the query
index if any non-string values are found. Then apply the existing trim and
empty-string filtering only to the validated string array. This prevents
malformed golden data from passing validation undetected.
In `@src/sdk/cli-contracts.ts`:
- Around line 1393-1399: The EVAL_FLAG_CONTRACTS constant is defined but not
integrated into the command-name resolver logic, causing eval command contract
queries to default to an empty flag set. Locate the resolver logic (likely a
switch statement handling command names) and add a case "eval" branch that
returns EVAL_FLAG_CONTRACTS to make these flag contracts reachable for eval
command processing.
---
Nitpick comments:
In `@tests/unit/core/search/eval-metrics.spec.ts`:
- Around line 132-143: Add a new test case in the parseEvalQuerySet test suite
to validate that mixed-type relevant_ids arrays are rejected. Create an it
statement that calls parseEvalQuerySet with a query object containing
relevant_ids with both string and non-string values (for example, strings mixed
with numbers or objects), and expect it to throw an appropriate error. This test
should be placed alongside the existing validation tests for missing arrays,
empty lists, and invalid modes to ensure the parser properly validates the type
consistency of relevant_ids elements.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3ba64422-645f-4852-ae31-f550e2d6f64d
📒 Files selected for processing (32)
.agents/pm/decisions/pm-f2al.toon.agents/pm/features/pm-75k9.toon.agents/pm/features/pm-u8n5.toon.agents/pm/history/pm-75k9.jsonl.agents/pm/history/pm-f2al.jsonl.agents/pm/history/pm-u8n5.jsonl.agents/pm/search/eval-queries.json.gitignoreCHANGELOG.mddocs/COMMANDS.mddocs/CONFIGURATION.mdsrc/cli/commands/contracts.tssrc/cli/commands/eval.tssrc/cli/commands/index.tssrc/cli/commands/search.tssrc/cli/help-content.tssrc/cli/register-list-query.tssrc/core/config/nested-settings.tssrc/core/search/bm25.tssrc/core/search/eval.tssrc/core/store/settings-validator.tssrc/sdk/cli-contracts.tssrc/sdk/cli-contracts/enum-contracts.tssrc/types.tstests/fixtures/contracts/full.jsontests/unit/cli/register-list-query.spec.tstests/unit/commands/config-key-aliases-search.spec.tstests/unit/commands/eval-command.spec.tstests/unit/commands/search-bm25.spec.tstests/unit/commands/search-command.spec.tstests/unit/core/search/bm25.spec.tstests/unit/core/search/eval-metrics.spec.ts
- eval gate: aggregate and gate on UNROUNDED metrics; round only for display so --fail-under decisions can't flip on display precision. - eval: reject non-string relevant_ids entries (fail loudly) instead of silently dropping malformed golden data. - bm25: clamp finite out-of-range k1/b to their bounds (preserve intent, e.g. k1=5000 -> 1000) rather than resetting to default; non-finite / non-number still falls back to the default. - cli-contracts: add `case "eval"` to the command-name flag resolver so EVAL_FLAG_CONTRACTS is reachable (pm contracts --command eval). - changelog: regenerate from the corrected pm-75k9 title (BM25-only, dropping the overstated "TF-IDF or BM25" wording). All gates green: 100/100/100/100 coverage (4768 tests), static + docs-skills + typecheck + contracts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WiNZNrE3rjftCC1udbBLGt
Owner
Author
|
Addressed all 5 review findings in c9930f0 (unrounded gate, strict relevant_ids, BM25 clamp, eval flag resolver, changelog wording). @coderabbitai full review |
Owner
Author
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
✅ Action performedFull review finished. |
unbraind
added a commit
that referenced
this pull request
Jun 20, 2026
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WiNZNrE3rjftCC1udbBLGt
This was referenced Jul 12, 2026
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
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
Two coherent post-v0.1 search capabilities under epic pm-ydp6 (Search & semantic retrieval), bundled because the eval harness is the natural validation vehicle for the new provider. Both ship at 100% coverage with full sandbox verification.
Offline BM25 provider (pm-75k9)
src/core/search/bm25.ts— Okapi BM25 (IDF +k1term-frequency saturation +blength-normalization) scoring the in-memory corpus with no embedding service or vector store, for air-gapped / CI / zero-setup retrieval.search.provider=bm25(always) orauto(offline fallback only when no embedding/extension provider is available). Plainkeywordmode is unchanged.runSearch:--semanticreturns pure BM25 hits (matched_fields:["bm25"]);--hybridblends BM25 with the keyword scorer viacombineHybridHits(now carries a dense-component label so hits read honestly). The exact full-ID / short-ID ranking guarantee is preserved in both modes.search.bm25.{k1,b}, settable viapm config set search_bm25_k1/b.Search relevance eval harness (pm-u8n5)
src/core/search/eval.ts— nDCG@k / MRR@k / precision@k / recall@k over a binary-relevance golden set +parseEvalQuerySetvalidator.pm eval— runs each golden query through the live search path, reports per-query + macro-aggregate metrics;--fail-underturns it into a CI gate (prints the report, exits non-zero on aggregate-nDCG regression).<pmRoot>/search/eval-queries.json(git-tracked; a.gitignorenegation keeps the search cache dir ignored while tracking the curated file). A 4-query golden set for this repo is included (aggregate nDCG ≈ 0.74).Docs & governance
docs/COMMANDS.md+docs/CONFIGURATION.mddocument the BM25 provider,search.providerbm25/auto, the bm25 tuning knobs, andpm eval.enum-contracts,EVAL_FLAG_CONTRACTS,help-content, regenerated contracts fixture). ADR pm-f2al records the design.Verification
100/100/100/100coverage (4766 tests).static+docs-skills+docs-links+typecheck+contracts:checkall green.pm evalgate pass & fail (non-zero exit).Closes pm-75k9, pm-u8n5. Records ADR pm-f2al.
🤖 Generated with Claude Code