fix: switch BrainBar FTS5 search from OR to AND matching#134
Conversation
Root cause: BrainBar's sanitizeFTS5Query joined terms with OR, so "overnight hardening sprint" matched any chunk containing ANY of those words. Python _escape_fts5_query uses implicit AND (space = AND in FTS5), requiring ALL terms to be present. This caused BrainBar to return irrelevant January chunks that only mentioned "sprint" while the Python MCP correctly returned only March 29 chunks containing all three words. Fix: Change from `tokens.joined(separator: " OR ")` to `tokens.joined(separator: " ")` (implicit AND). 68/68 tests green (1 new: testMultiWordSearchUsesAND). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThe PR switches FTS5 full-text search semantics from disjunctive (OR) to conjunctive (AND) matching by joining query tokens with spaces instead of "OR" separators. Changes include updating query construction in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
@coderabbitai review |
|
@codex review |
|
@greptileai review |
✅ Actions performedReview triggered.
|
|
The GitHub CLI doesn't have write permissions. That's fine - the review document is committed and pushed. Let me create a summary of the review findings: SummaryI've completed a comprehensive bug review of PR #134 (fix: switch BrainBar FTS5 search from OR to AND matching). Here's what I found: ✅ APPROVED - Ready to MergeRisk Level: LOW Key FindingsStrengths:
Minor Issues (non-blocking):
Edge Cases Validated
Deployment Risk Assessment
Full review document:   |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Co-authored-by: Etan Heyman <EtanHey@users.noreply.github.com>
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BUGBOT_REVIEW.md`:
- Around line 42-48: The PR should only contain the intended single-line fix
switching the separator from " OR " to " " — remove any unrelated changes that
made it look like f.rank, the conditional ORDER BY logic, and the score
calculation were introduced by this PR; revert those pre-existing edits so the
diff only changes the separator string literal (" OR " -> " ") in the
query-building site, and leave existing symbols like f.rank, the ORDER BY
branch, and the score calculation untouched.
- Around line 51-54: Fix the Markdown formatting issues flagged by the linter:
add a blank line above and below the fenced code block containing the Swift
snippet that includes let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank",
add the language specifier (```swift) to that fenced block, remove the trailing
space on the reported line, and ensure there is a blank line before/after nearby
headings; update the BUGBOT_REVIEW.md file so all fenced blocks and headings
follow MD031/MD022/MD009/MD040 rules.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a369a255-5c2e-4816-b895-0db25bbf0500
📒 Files selected for processing (3)
BUGBOT_REVIEW.mdbrain-bar/Sources/BrainBar/BrainDatabase.swiftbrain-bar/Tests/BrainBarTests/DatabaseTests.swift
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: EtanHey/brainlayer PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-14T02:20:54.656Z
Learning: Request codex review, cursor review, and bugbot review for BrainLayer PRs
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In `brain-bar/Sources/BrainBar/BrainDatabase.swift` (Swift, BrainBar module), the `search()` function uses `maxRowID` as a delivery watermark cursor in the `unreadOnly` path. The result set **must** be ordered by `c.rowid ASC` when `unreadOnly=true`, so that `maxRowID` correctly represents the contiguous delivery frontier. Using a relevance-based ordering (e.g., `ORDER BY f.rank`) in the `unreadOnly` path would cause gaps in the watermark and mark unseen rows as delivered. The correct pattern is: `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`. Flag any future change to the `ORDER BY` clause in this function that applies relevance sorting unconditionally.
📚 Learning: 2026-03-18T00:12:08.774Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 87
File: brain-bar/Sources/BrainBar/BrainBarServer.swift:118-129
Timestamp: 2026-03-18T00:12:08.774Z
Learning: In Swift files under brain-bar/Sources/BrainBar, enforce that when a critical dependency like the database is nil due to startup ordering (socket before DB), any tool handler that accesses the database must throw an explicit error (e.g., ToolError.noDatabase) instead of returning a default/empty value. Do not allow silent defaults (e.g., guard let db else { return ... }). Flag patterns that silently return defaults when db is nil, as this masks startup timing issues. This guidance applies broadly to similar Swift files in the BrainBar module, not just this one location.
Applied to files:
brain-bar/Sources/BrainBar/BrainDatabase.swift
📚 Learning: 2026-03-29T18:45:40.988Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In the BrainBar module’s Swift database layer (notably BrainDatabase.swift), ensure that the `search()` function’s `unreadOnly=true` path orders results by the delivery frontier cursor so the watermark `maxRowID` stays contiguous. Specifically, when `unreadOnly` is enabled, the query must include `ORDER BY c.rowid ASC` (e.g., via `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`). Do not replace the unread-only ordering with relevance-based sorting (e.g., `f.rank`) unconditionally or for the unread-only path, as it can introduce gaps in the watermark and incorrectly mark unseen rows as delivered. Flag any future change to the `ORDER BY` clause in this function that makes relevance sorting apply to the unread-only case.
Applied to files:
brain-bar/Sources/BrainBar/BrainDatabase.swift
📚 Learning: 2026-03-29T18:45:40.988Z
Learnt from: EtanHey
Repo: EtanHey/brainlayer PR: 133
File: brain-bar/Sources/BrainBar/BrainDatabase.swift:0-0
Timestamp: 2026-03-29T18:45:40.988Z
Learning: In `brain-bar/Sources/BrainBar/BrainDatabase.swift` (Swift, BrainBar module), the `search()` function uses `maxRowID` as a delivery watermark cursor in the `unreadOnly` path. The result set **must** be ordered by `c.rowid ASC` when `unreadOnly=true`, so that `maxRowID` correctly represents the contiguous delivery frontier. Using a relevance-based ordering (e.g., `ORDER BY f.rank`) in the `unreadOnly` path would cause gaps in the watermark and mark unseen rows as delivered. The correct pattern is: `let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"`. Flag any future change to the `ORDER BY` clause in this function that applies relevance sorting unconditionally.
Applied to files:
brain-bar/Tests/BrainBarTests/DatabaseTests.swiftBUGBOT_REVIEW.md
📚 Learning: 2026-03-14T02:20:54.656Z
Learnt from: CR
Repo: EtanHey/brainlayer PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-14T02:20:54.656Z
Learning: Request codex review, cursor review, and bugbot review for BrainLayer PRs
Applied to files:
BUGBOT_REVIEW.md
🪛 markdownlint-cli2 (0.22.0)
BUGBOT_REVIEW.md
[warning] 24-24: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 52-52: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 56-56: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 66-66: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 67-67: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 78-78: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 85-85: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 85-85: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 102-102: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 133-133: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 135-135: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 137-137: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 143-143: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 145-145: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 147-147: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 153-153: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 155-155: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 160-160: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 166-166: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 176-176: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 195-195: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 198-198: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 202-202: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 212-212: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 214-214: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 219-219: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 225-225: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 243-243: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 247-247: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 254-254: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 265-265: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 267-267: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 277-277: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 279-279: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 289-289: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 291-291: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 325-325: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 332-332: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 342-342: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 349-349: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (2)
brain-bar/Sources/BrainBar/BrainDatabase.swift (1)
786-789: LGTM! Correct switch from OR to AND matching.The change from
" OR "to" "separator correctly implements FTS5 implicit AND semantics, matching Python MCP behavior. The explanatory comments are helpful.The existing token sanitization (stripping quotes/wildcards, quoting each token) and empty-query fallback (
"") remain intact, preserving safety against FTS5 injection.brain-bar/Tests/BrainBarTests/DatabaseTests.swift (1)
327-337: LGTM! Well-designed test for AND semantics.The test correctly validates the core behavioral change:
- Inserts a positive case (chunk with all 3 query terms) and a negative case (chunk with only 1 term)
- Asserts that only the chunk containing ALL terms is returned
- Clear descriptive comments explaining expected behavior
This provides good coverage for the PR's main fix.
| ### 2. Search Ranking Enhancement: `BrainDatabase.swift` (lines 231-241, 277-295) | ||
|
|
||
| # After | ||
| if not query or not query.strip(): | ||
| return "" # Empty string signals "skip FTS" | ||
| **Changes:** | ||
| 1. Added `f.rank` to SELECT clause (line 236) | ||
| 2. Added conditional ORDER BY logic (line 233) | ||
| 3. Added score calculation and result field (lines 280-282, 294) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarification: ranking/score code appears to be pre-existing.
The review correctly analyzes the ranking behavior, but note that the f.rank column, ORDER BY conditional logic, and score calculation (lines 231-241, 277-295) appear to be pre-existing code, not changes introduced by this PR. The PR's actual change is the single-line fix at line 789 (switching from " OR " to " " separator).
This doesn't affect the approval verdict, but clarifies the scope for readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BUGBOT_REVIEW.md` around lines 42 - 48, The PR should only contain the
intended single-line fix switching the separator from " OR " to " " — remove any
unrelated changes that made it look like f.rank, the conditional ORDER BY logic,
and the score calculation were introduced by this PR; revert those pre-existing
edits so the diff only changes the separator string literal (" OR " -> " ") in
the query-building site, and leave existing symbols like f.rank, the ORDER BY
branch, and the score calculation untouched.
| #### 2.1 Conditional Ordering | ||
| ```swift | ||
| let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank" | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor markdown formatting issues flagged by linter.
Static analysis detected several markdown formatting inconsistencies throughout this document:
- Missing blank lines around fenced code blocks (MD031)
- Missing blank lines around headings (MD022)
- Trailing space on line 56 (MD009)
- Missing language specifier on code block at line 85 (MD040)
These are cosmetic issues that don't affect the content accuracy.
Example fix for lines 51-54
#### 2.1 Conditional Ordering
+
```swift
let orderByClause = unreadOnly ? "c.rowid ASC" : "f.rank"</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
---
[warning] 52-52: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @BUGBOT_REVIEW.md around lines 51 - 54, Fix the Markdown formatting issues
flagged by the linter: add a blank line above and below the fenced code block
containing the Swift snippet that includes let orderByClause = unreadOnly ?
"c.rowid ASC" : "f.rank", add the language specifier (```swift) to that fenced
block, remove the trailing space on the reported line, and ensure there is a
blank line before/after nearby headings; update the BUGBOT_REVIEW.md file so all
fenced blocks and headings follow MD031/MD022/MD009/MD040 rules.
</details>
<!-- fingerprinting:phantom:poseidon:ocelot -->
<!-- This is an auto-generated comment by CodeRabbit -->
2 participants
Summary
sanitizeFTS5Queryjoined terms withOR— "overnight hardening sprint" matched ANY chunk with "sprint" alone (January planning chunks). Python_escape_fts5_queryuses implicit AND (space = AND in FTS5).tokens.joined(separator: " OR ")→tokens.joined(separator: " ")(1 line change)Before/After
Test plan
testMultiWordSearchUsesAND)🤖 Generated with Claude Code
Note
Fix BrainBar FTS5 multi-word search to use AND instead of OR matching
Changes the token joiner in
BrainDatabase.sanitizeFTS5Queryfrom an explicitORseparator to a space, which leverages FTS5's implicit AND semantics. Multi-word queries now return only documents containing all terms, improving precision and matching Python parity. A new test inDatabaseTestsvalidates this behavior. Behavioral Change: existing queries that relied on OR (any-term) matching will now return fewer, more precise results.Macroscope summarized d309f69.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests