fix(mcp): defer vector-store init past signal-routing branches in _brain_search#272
Conversation
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.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 37 minutes and 57 seconds.Comment |
Review Complete ✅I've completed a comprehensive review of PR #272 as requested. Here's what I found: Verdict: APPROVEDRisk Level: LOW Key FindingsTest Isolation Fix ✅
Behavioral Safety Analysis ✅
Performance Improvement ✅
Test Coverage ✅
Observable Behavior ChangesOutside Test Scope: NONE The only potential behavior change (chunk ID pattern + signal phrase) cannot occur in practice due to mutual exclusion on the space constraint. All other routing paths remain unchanged or improve (file_path precedence better aligns with user intent). DocumentationI've created two documents in the repository:
Both are now committed and pushed to the RecommendationShip it. The refactor achieves its goal (test isolation) without introducing production risk, and actually improves performance for signal-routed queries. |
- Verified test isolation fix (signal routing before DB access) - Analyzed precedence change: chunk ID vs signal routing (mutual exclusion on space constraint) - Confirmed behavioral safety via control flow analysis - Identified performance improvement (3 DB ops saved per signal-routed query) - Risk assessment: LOW (pure reordering, no logic changes) - Recommendation: APPROVED ✅ Co-authored-by: Etan Heyman <EtanHey@users.noreply.github.com>
Co-authored-by: Etan Heyman <EtanHey@users.noreply.github.com>


Summary
_brain_search()until after current-context, think, and recall signal-routing branches have had a chance to return.tests/test_phase6_critical.py::TestSearchRoutingwas hitting the canonical DB before mocked routing handlers, which also overlaps the test_post_commit_hook ordering bug family.Test plan
uv run pytest -vv tests/test_phase6_critical.py -k "current_context_signal or think_signal or recall_signal"failed withapsw.BusyError: database is lockedbefore the fix.uv run pytest -vv tests/test_phase6_critical.py -k "current_context_signal or think_signal or recall_signal"passed.uv run pytest -vv tests/test_phase6_critical.pypassed with 14 tests.com.brainlayer.enrichment, reran the three routing tests, confirmed they still passed, then bootstrapped enrichment again.test_fts5_determinism.sh.Note
Low Risk
Low risk reorder of
_brain_searchcontrol flow; main impact is avoiding early vector-store/DB access, which could subtly change precedence only if a query both matches a routing signal and looks like a chunk id.Overview
Defers
_get_vector_store()initialization (and the dependent exact chunk-id lookup / FTS query expansion) in_brain_searchuntil after early-return routing branches likecurrent_context,think,recall, and file/regression handlers.This reduces unintended DB/vector-store access during signal-routed queries (improving test isolation and avoiding lock contention) while keeping the same downstream search behavior for non-routed queries.
Reviewed by Cursor Bugbot for commit 2af5a57. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Defer vector-store init in
_brain_searchuntil after signal-routing early returns_get_vector_store(),_exact_chunk_lookup_result(), and_expanded_fts_query()were called before the regression, think, and recall routing checks in search_handler.py.Macroscope summarized 2af5a57.