feat: wire FTS5 fast-path (default OFF) - #160
Merged
Merged
Conversation
Wires the FTS5 lexical fast-path dispatch inside KnowledgeOrchestrator. Feature is completely OFF by default - config.lexical_fast_path_enabled defaults to False, preserving v4.8.2 behavior byte-for-byte until operators explicitly opt in. - mcp_server/server.py: new _maybe_dispatch_fts5() helper called after cache check and before _ensure_bm25_index(). Early-return on enabled=False preserves zero-cost path. - mcp_server/server.py: search_knowledge MCP tool gains new opt-in param search_method: Literal[auto,hybrid,fts5]=auto (ADITIVO puro - LEI 1 preserved for the other 12 frozen tools). - mcp_server/server.py: KnowledgeOrchestrator.__init__ instantiates Fts5LexicalIndex when enabled, with atexit cleanup. - mcp_server/config.py: 3 new fields (lexical_fast_path_enabled/min_hits/ patterns) via _get_nested() mirror of models.reranker.* pattern. - mcp_server/metrics.py: 4 new counters + 1 histogram for fast-path hits/latency/fallback/errors observability. - Result items gain search_method=fts5 tag when fast-path fires. - 35 tests (8 UT + 20 IT + 7 E2E) covering dispatch/override/toggle/ metrics/compat. - API surface baseline snapshot updated (aditivo only). - Test count baseline 296 -> 304 (+8). Fase 3 of FTS5 Lexical Fast-Path plan. LEI 1: search_knowledge is aditivo; other 12 MCP tools untouched. Zero runtime behavior change with feature OFF (default).
📝 WalkthroughWalkthroughThe search pipeline adds optional FTS5 lexical dispatch, explicit ChangesFTS5 Search Dispatch
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MCPTool
participant KnowledgeOrchestrator
participant QueryRouter
participant Fts5LexicalIndex
participant HybridSearch
MCPTool->>KnowledgeOrchestrator: Submit search_method
KnowledgeOrchestrator->>QueryRouter: Classify auto query
QueryRouter-->>KnowledgeOrchestrator: Return route
KnowledgeOrchestrator->>Fts5LexicalIndex: Search lexical query
Fts5LexicalIndex-->>KnowledgeOrchestrator: Return chunk hits
KnowledgeOrchestrator->>HybridSearch: Fall back when needed
HybridSearch-->>KnowledgeOrchestrator: Return hybrid results
KnowledgeOrchestrator-->>MCPTool: Return structured results
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcp_server/server.py (1)
2726-2772: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
search_methodis reassigned in the formatting loop, so the hybrid result is cached under the wrong key.Line 2733, line 2735, and line 2737 assign
search_methodinside the result loop. That rebinds thequery()parameter. By the time line 2769 runs,search_methodholds the per-result label of the last formatted item ("hybrid","semantic", or"keyword"), not the caller's dispatch selector.Consequences:
query()reads the cache at line 2536 with the caller value (for example"auto") but writes at line 2769 with a derived label. The hybrid path therefore never produces a cache hit, and every hybrid query re-runs the full pipeline.- A write under the literal
"hybrid"key can be served later to a caller that explicitly requestssearch_method="hybrid", even though the cached entry came from an"auto"request. The two routes are supposed to be isolated, which is the stated reason for the fifth cache-key field.- When
sorted_resultsis empty the loop never runs, so the value is inconsistent between empty and non-empty responses.Rename the loop-local variable.
🐛 Proposed fix
if s_rank and b_rank: - search_method = "hybrid" + result_method = "hybrid" elif s_rank: - search_method = "semantic" + result_method = "semantic" else: - search_method = "keyword" + result_method = "keyword" @@ - "search_method": search_method, + "search_method": result_method,🤖 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 `@mcp_server/server.py` around lines 2726 - 2772, Rename the per-result `search_method` variable inside the formatting loop to a distinct local name, and use that name in the formatted result. Preserve the `query()` parameter `search_method` unchanged so `self.query_cache.put` caches under the caller’s dispatch selector, including when `sorted_results` is empty.
🧹 Nitpick comments (5)
mcp_server/server.py (1)
2474-2478: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the constant
if chunk_idsguard.
chunk_idsis derived fromhitsat line 2466 andhitsis already known to be non-empty at this point. The comprehension filter never changes the result.♻️ Proposed cleanup
- raw_scores = [float(score) for _, score in hits if chunk_ids] + raw_scores = [float(score) for _, score in hits]🤖 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 `@mcp_server/server.py` around lines 2474 - 2478, Remove the redundant if chunk_ids condition from the raw_scores comprehension in the surrounding score-normalization logic. Build raw_scores directly from hits, preserving the existing max_score, min_score, and score_range calculations.tests/test_e2e_fts5.py (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the router predicate.
any(c.isupper() and "-" in q for c in q)evaluates"-" in qonce per character, and the term does not depend onc. The expression means "the query contains an uppercase letter and a dash".♻️ Proposed cleanup
- router = _FakeRouter(lambda q: "lexical" if any(c.isupper() and "-" in q for c in q) else "semantic") + router = _FakeRouter(lambda q: "lexical" if ("-" in q and any(c.isupper() for c in q)) else "semantic")🤖 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/test_e2e_fts5.py` at line 47, Update the _FakeRouter predicate to independently check whether the query contains an uppercase character and a dash, avoiding the repeated "-" in q evaluation inside the character iteration while preserving the lexical/semantic routing behavior.mcp_server/metrics.py (2)
67-107: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider bounding histogram retention; each scrape now sorts every observation.
observeappends every sample toself._histograms[key]without limit.expositionnow callssorted(observations)for each registered histogram on every scrape. Cost grows with total observation count for the process lifetime, so a long-running server with high query volume pays growing memory and per-scrape CPU.A counting approach removes both problems: keep per-bucket counters plus
countandsum, and increment them insideobserve. That also matches how Prometheus histograms are normally implemented.If you prefer to keep the sample list for now, a bounded
dequeplus runningcount/sumis a smaller change.🤖 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 `@mcp_server/metrics.py` around lines 67 - 107, Replace unbounded observation retention in the histogram implementation with per-bucket counters plus running count and sum, updating them in observe and consuming those aggregates in exposition and _format_histogram_buckets instead of sorting stored samples. Preserve cumulative bucket semantics, including the +Inf bucket, and ensure registered histogram names continue to expose correctly.
110-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify bucket boundary rendering for all configured values.
_merge_labelsrenders the boundary with defaultfloatformatting.0.010renders asle="0.01"and0.100renders asle="0.1". Prometheus accepts these numeric strings, but the label value is the series identity. If a future bucket tuple contains a value that renders in exponential form (for example1e-05), the label changes shape and dashboards that match on literallestrings break.Consider formatting boundaries explicitly, for example
f"{boundary:g}"with a documented convention, or store the canonical string next to the numeric boundary.🤖 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 `@mcp_server/metrics.py` around lines 110 - 119, Update the histogram bucket label construction used by _merge_labels and its callers to render every configured boundary with an explicit, documented canonical string format rather than default float conversion. Preserve numeric meaning while ensuring values such as 0.010, 0.100, and very small exponential-form boundaries produce stable Prometheus le label identities.tests/test_fts5_index.py (1)
355-376: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose the connection even when an assertion fails.
index.close()runs after the asserts. A failed assertion leaks the SQLite connection and the WAL files for the rest of the session. Use a fixture ortry/finally.♻️ Proposed cleanup
index = Fts5LexicalIndex(db_path=tmp_path / "db.sqlite", state_path=state_path) - busy_timeout = index._conn.execute("PRAGMA busy_timeout").fetchone()[0] - journal_mode = index._conn.execute("PRAGMA journal_mode").fetchone()[0] - assert busy_timeout == 5000 - assert journal_mode.lower() == "wal" - index.close() + try: + busy_timeout = index._conn.execute("PRAGMA busy_timeout").fetchone()[0] + journal_mode = index._conn.execute("PRAGMA journal_mode").fetchone()[0] + assert busy_timeout == 5000 + assert journal_mode.lower() == "wal" + finally: + index.close()🤖 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/test_fts5_index.py` around lines 355 - 376, Update test_ut026_busy_timeout_pragma_is_five_seconds so index.close() executes regardless of assertion outcomes, using try/finally or the test suite’s existing cleanup fixture. Keep both PRAGMA assertions unchanged while ensuring the Fts5LexicalIndex connection is always closed.
🤖 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 @.github/test-count-baseline.txt:
- Line 1: Update the test-count baseline value in test-count-baseline.txt to the
current pytest --collect-only -q count, reflecting the 36 newly discoverable
tests rather than the existing 304 value.
In `@mcp_server/server.py`:
- Around line 3612-3629: Update the Fts5NotReadyError response in
search_knowledge to use the standard "message" key instead of—or alongside—the
current "error" key, ensuring status=="error" clients can read
payload["message"]; adjust tests/test_search_method_override.py if the response
contract changes.
- Around line 2441-2455: Update _run_fts5_search so the category-filtered result
from _format_fts5_results is checked for emptiness before expanding,
incrementing metrics, or returning. When formatted is empty, return None to
allow _maybe_dispatch_fts5 and the hybrid pipeline to perform fallback; preserve
the existing fast-path behavior for non-empty results.
- Around line 2362-2375: Update _initialize_fts5_dispatch to catch
Fts5CorruptError from Fts5LexicalIndex construction, log the failure, and leave
self.fts5_index as None so dispatch falls back to hybrid. Keep QueryRouter
initialization outside that recovery path so re.error remains fatal for invalid
configuration, and document this intentional distinction in the method.
In `@README.md`:
- Line 1417: Correct the README entry to describe four FTS5 counters plus one
bucketed latency histogram, and update the tests/test_e2e_fts5.py coverage count
to six E2E tests, matching the defined E2E-001..005, E2E-007, and E2E-008
methods.
In `@tests/test_fts5_index.py`:
- Around line 378-389: The test
test_ut029_not_ready_error_message_includes_auto_suggestion currently validates
a locally supplied message instead of production behavior. Invoke
server.py::_maybe_dispatch_fts5 with search_method="fts5" and a not-ready index,
capture the raised Fts5NotReadyError, and assert its actual message includes
both “auto” and “Suggestion”; retain the Fts5NotReadyError RuntimeError
inheritance assertion.
In `@tests/test_search.py`:
- Around line 778-793: Update
test_it008_fts5_error_triggers_fallback_and_error_metric to parse
FAST_PATH_ERRORS_TOTAL values with the shared helper introduced for
test_it006_low_hits_triggers_fallback_metric. Compare the parsed
OperationalError counter before and after orch.query, ensuring the assertion
verifies an increment rather than counting metric exposition lines.
- Around line 751-765: Add a shared helper in tests/test_search.py to parse the
numeric value from a named exposition metric line, then use it in
tests/test_search.py lines 751-765 for fallback_total{reason="low_hits"} before
and after values, and lines 778-793 for
errors_total{error_class="OperationalError"} instead of substring counts. Import
the same helper in tests/test_e2e_fts5.py lines 103-119 and use it for both
before and after readings of the shared fallback metric series.
---
Outside diff comments:
In `@mcp_server/server.py`:
- Around line 2726-2772: Rename the per-result `search_method` variable inside
the formatting loop to a distinct local name, and use that name in the formatted
result. Preserve the `query()` parameter `search_method` unchanged so
`self.query_cache.put` caches under the caller’s dispatch selector, including
when `sorted_results` is empty.
---
Nitpick comments:
In `@mcp_server/metrics.py`:
- Around line 67-107: Replace unbounded observation retention in the histogram
implementation with per-bucket counters plus running count and sum, updating
them in observe and consuming those aggregates in exposition and
_format_histogram_buckets instead of sorting stored samples. Preserve cumulative
bucket semantics, including the +Inf bucket, and ensure registered histogram
names continue to expose correctly.
- Around line 110-119: Update the histogram bucket label construction used by
_merge_labels and its callers to render every configured boundary with an
explicit, documented canonical string format rather than default float
conversion. Preserve numeric meaning while ensuring values such as 0.010, 0.100,
and very small exponential-form boundaries produce stable Prometheus le label
identities.
In `@mcp_server/server.py`:
- Around line 2474-2478: Remove the redundant if chunk_ids condition from the
raw_scores comprehension in the surrounding score-normalization logic. Build
raw_scores directly from hits, preserving the existing max_score, min_score, and
score_range calculations.
In `@tests/test_e2e_fts5.py`:
- Line 47: Update the _FakeRouter predicate to independently check whether the
query contains an uppercase character and a dash, avoiding the repeated "-" in q
evaluation inside the character iteration while preserving the lexical/semantic
routing behavior.
In `@tests/test_fts5_index.py`:
- Around line 355-376: Update test_ut026_busy_timeout_pragma_is_five_seconds so
index.close() executes regardless of assertion outcomes, using try/finally or
the test suite’s existing cleanup fixture. Keep both PRAGMA assertions unchanged
while ensuring the Fts5LexicalIndex connection is always closed.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 25e6f630-f0bd-4a2e-9d19-a3ec38bdb873
📒 Files selected for processing (12)
.github/api-surface-baseline.json.github/test-count-baseline.txtREADME.mdmcp_server/metrics.pymcp_server/server.pytests/conftest.pytests/test_backwards_compat.pytests/test_e2e_fts5.pytests/test_fts5_index.pytests/test_metrics.pytests/test_search.pytests/test_search_method_override.py
| @@ -1 +1 @@ | |||
| 296 | |||
| 304 | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the workflow or script that consumes the baseline and see how it computes the count.
set -euo pipefail
rg -n -C5 'test-count-baseline' --glob '!**/node_modules/**' .
# Count collected test functions for a rough comparison.
rg -c -P '^\s*(async\s+)?def test_' tests | sort -t: -k2 -n
rg -P '^\s*(async\s+)?def test_' tests -c --no-filename | paste -sd+ - | bcRepository: lyonzin/knowledge-rag
Length of output: 5401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- check_test_count.py ---'
cat -n scripts/check_test_count.py
printf '%s\n' '--- baseline ---'
cat -n .github/test-count-baseline.txt
printf '%s\n' '--- relevant diff summary ---'
git diff --stat -- .github/test-count-baseline.txt tests
printf '%s\n' '--- changed test files ---'
git diff --name-status -- tests
printf '%s\n' '--- test-function counts ---'
python3 - <<'PY'
from pathlib import Path
import ast
total = 0
for path in sorted(Path("tests").rglob("test_*.py")):
tree = ast.parse(path.read_text(), filename=str(path))
count = sum(
isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
and node.name.startswith("test_")
for node in ast.walk(tree)
)
if count:
print(f"{path}:{count}")
total += count
print(f"TOTAL={total}")
PYRepository: lyonzin/knowledge-rag
Length of output: 6882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- repository state ---'
git status --short
git log --oneline -5
printf '%s\n' '--- baseline history ---'
git log --oneline --all -- .github/test-count-baseline.txt | head -20
printf '%s\n' '--- baseline at HEAD and parent ---'
for rev in HEAD HEAD^; do
printf '%s: ' "$rev"
git show "$rev:.github/test-count-baseline.txt" 2>/dev/null || printf '<missing>\n'
done
printf '%s\n' '--- pytest configuration ---'
rg -n -C4 'pytest|testpaths|python_files|python_functions|pytest_plugins|parametrize|skip|xfail' pyproject.toml pytest.ini setup.cfg tox.ini .github tests 2>/dev/null | head -300
printf '%s\n' '--- recent commit file summary ---'
git diff --name-status HEAD^ HEAD -- .github/test-count-baseline.txt testsRepository: lyonzin/knowledge-rag
Length of output: 20465
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- commit summary ---'
git show --stat --oneline --summary HEAD
printf '%s\n' '--- changed test-file counts ---'
git diff --numstat HEAD^ HEAD -- tests
printf '%s\n' '--- focused diff for listed files ---'
git diff --unified=1 HEAD^ HEAD -- \
tests/test_metrics.py \
tests/test_search_method_override.py \
tests/test_e2e_fts5.py \
tests/test_backwards_compat.py \
tests/test_fts5_index.py \
tests/test_search.py
printf '%s\n' '--- AST test declaration deltas ---'
python3 - <<'PY'
import ast
import subprocess
from pathlib import PurePosixPath
files = subprocess.check_output(
["git", "diff", "--name-only", "HEAD^", "HEAD", "--", "tests"],
text=True,
).splitlines()
def count(rev, path):
if rev == "WORKTREE":
text = open(path, encoding="utf-8").read()
else:
text = subprocess.check_output(["git", "show", f"{rev}:{path}"], text=True)
tree = ast.parse(text, filename=path)
return sum(
isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef))
and node.name.startswith("test_")
for node in ast.walk(tree)
)
total_before = total_after = 0
for path in files:
before = count("HEAD^", path)
after = count("HEAD", path)
print(f"{path}: {before} -> {after} (delta {after-before:+d})")
total_before += before
total_after += after
print(f"changed files total: {total_before} -> {total_after} (delta {total_after-total_before:+d})")
PYRepository: lyonzin/knowledge-rag
Length of output: 38133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
import subprocess
from pathlib import PurePosixPath
def files_at(rev):
return subprocess.check_output(
["git", "ls-tree", "-r", "--name-only", rev, "tests"],
text=True,
).splitlines()
def text_at(rev, path):
if rev == "WORKTREE":
return open(path, encoding="utf-8").read()
return subprocess.check_output(["git", "show", f"{rev}:{path}"], text=True)
def pytest_items(rev, path):
tree = ast.parse(text_at(rev, path), filename=path)
count = 0
names = []
for node in tree.body:
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name.startswith("test_"):
count += 1
names.append(node.name)
elif isinstance(node, ast.ClassDef) and node.name.startswith("Test"):
for child in node.body:
if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)) and child.name.startswith("test_"):
count += 1
names.append(f"{node.name}.{child.name}")
return count, names
for rev in ("HEAD^", "HEAD"):
total = 0
print(f"--- {rev} ---")
for path in files_at(rev):
if path.endswith(".py") and PurePosixPath(path).name.startswith("test_"):
count, _ = pytest_items(rev, path)
total += count
if rev == "HEAD" and path in {
"tests/test_metrics.py",
"tests/test_search_method_override.py",
"tests/test_e2e_fts5.py",
"tests/test_backwards_compat.py",
"tests/test_fts5_index.py",
"tests/test_search.py",
}:
print(f"{path}: {count}")
print(f"TOTAL_DISCOVERABLE_DECLARATIONS={total}")
print("--- changed-file deltas ---")
for path in subprocess.check_output(
["git", "diff", "--name-only", "HEAD^", "HEAD", "--", "tests"],
text=True,
).splitlines():
if path.endswith(".py") and PurePosixPath(path).name.startswith("test_"):
before = pytest_items("HEAD^", path)[0] if path in files_at("HEAD^") else 0
after = pytest_items("HEAD", path)[0]
print(f"{path}: {before} -> {after} (delta {after-before:+d})")
PY
printf '%s\n' '--- conftest diff ---'
git diff --unified=3 HEAD^ HEAD -- tests/conftest.pyRepository: lyonzin/knowledge-rag
Length of output: 1857
Update the test-count baseline. The changed files add 36 discoverable tests, not 8. The guard only fails when the current count is below the baseline, so 304 will not fail the job but will permit substantial test deletion. Set the baseline to the current pytest --collect-only -q count.
🤖 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 @.github/test-count-baseline.txt at line 1, Update the test-count baseline
value in test-count-baseline.txt to the current pytest --collect-only -q count,
reflecting the 36 newly discoverable tests rather than the existing 304 value.
Comment on lines
+2362
to
+2375
| def _initialize_fts5_dispatch(self) -> None: | ||
| """Instantiate ``Fts5LexicalIndex`` + ``QueryRouter`` under the toggle. | ||
|
|
||
| Kept separate from ``__init__`` so failures surface with a clear source | ||
| (config typo vs Chroma issue vs FTS5 issue) and callers can retry after | ||
| fixing config without rebuilding the whole orchestrator. | ||
| """ | ||
| db_path = config.data_dir / "fts5_index.db" | ||
| state_path = config.data_dir / "fts5_migration.state" | ||
| self.fts5_index = Fts5LexicalIndex(db_path=db_path, state_path=state_path) | ||
| # QueryRouter raises re.error at construction if any pattern is | ||
| # malformed — treat that as a fatal startup error so the operator | ||
| # sees the broken pattern immediately instead of on the first query. | ||
| self.query_router = QueryRouter(config.fts5_patterns) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Handle FTS5 initialization failure so the server still starts.
_initialize_fts5_dispatch runs inside KnowledgeOrchestrator.__init__ and does not catch exceptions. Fts5LexicalIndex(...) raises Fts5CorruptError when the SQLite build lacks FTS5 or the store is damaged, and QueryRouter(...) raises re.error on a bad pattern. Either failure aborts orchestrator construction, so the whole MCP server fails to start even though the fast-path is an optional feature whose documented contract is graceful fallback to hybrid.
Catch index construction failures, log them, and leave self.fts5_index = None. The dispatch code already treats None as "not ready" and falls back. Keep QueryRouter fatal only if fail-fast on config typos is the intended behavior; document that choice.
🛡️ Proposed fix
db_path = config.data_dir / "fts5_index.db"
state_path = config.data_dir / "fts5_migration.state"
- self.fts5_index = Fts5LexicalIndex(db_path=db_path, state_path=state_path)
+ try:
+ self.fts5_index = Fts5LexicalIndex(db_path=db_path, state_path=state_path)
+ except Exception as exc: # index unusable — fall back to hybrid only
+ print(f"[FTS5] Index unavailable, fast-path disabled: {exc}")
+ self.fts5_index = None
# QueryRouter raises re.error at construction if any pattern is
# malformed — treat that as a fatal startup error so the operator
# sees the broken pattern immediately instead of on the first query.
self.query_router = QueryRouter(config.fts5_patterns)📝 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
| def _initialize_fts5_dispatch(self) -> None: | |
| """Instantiate ``Fts5LexicalIndex`` + ``QueryRouter`` under the toggle. | |
| Kept separate from ``__init__`` so failures surface with a clear source | |
| (config typo vs Chroma issue vs FTS5 issue) and callers can retry after | |
| fixing config without rebuilding the whole orchestrator. | |
| """ | |
| db_path = config.data_dir / "fts5_index.db" | |
| state_path = config.data_dir / "fts5_migration.state" | |
| self.fts5_index = Fts5LexicalIndex(db_path=db_path, state_path=state_path) | |
| # QueryRouter raises re.error at construction if any pattern is | |
| # malformed — treat that as a fatal startup error so the operator | |
| # sees the broken pattern immediately instead of on the first query. | |
| self.query_router = QueryRouter(config.fts5_patterns) | |
| def _initialize_fts5_dispatch(self) -> None: | |
| """Instantiate ``Fts5LexicalIndex`` + ``QueryRouter`` under the toggle. | |
| Kept separate from ``__init__`` so failures surface with a clear source | |
| (config typo vs Chroma issue vs FTS5 issue) and callers can retry after | |
| fixing config without rebuilding the whole orchestrator. | |
| """ | |
| db_path = config.data_dir / "fts5_index.db" | |
| state_path = config.data_dir / "fts5_migration.state" | |
| try: | |
| self.fts5_index = Fts5LexicalIndex(db_path=db_path, state_path=state_path) | |
| except Exception as exc: # index unusable — fall back to hybrid only | |
| print(f"[FTS5] Index unavailable, fast-path disabled: {exc}") | |
| self.fts5_index = None | |
| # QueryRouter raises re.error at construction if any pattern is | |
| # malformed — treat that as a fatal startup error so the operator | |
| # sees the broken pattern immediately instead of on the first query. | |
| self.query_router = QueryRouter(config.fts5_patterns) |
🤖 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 `@mcp_server/server.py` around lines 2362 - 2375, Update
_initialize_fts5_dispatch to catch Fts5CorruptError from Fts5LexicalIndex
construction, log the failure, and leave self.fts5_index as None so dispatch
falls back to hybrid. Keep QueryRouter initialization outside that recovery path
so re.error remains fatal for invalid configuration, and document this
intentional distinction in the method.
Comment on lines
+3612
to
+3629
| try: | ||
| results = orchestrator.query( | ||
| query.strip(), | ||
| max_results=max_results, | ||
| category_filter=category, | ||
| hybrid_alpha=hybrid_alpha, | ||
| search_method=search_method, | ||
| ) | ||
| except Fts5NotReadyError as exc: | ||
| # Surface the fast-path error verbatim + always add the auto-fallback | ||
| # suggestion so debug users can recover without hunting docs. | ||
| return json.dumps( | ||
| { | ||
| "status": "error", | ||
| "error": str(exc), | ||
| "suggestion": "search_method='auto' fallback gracefully", | ||
| } | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the error envelope key with the other error responses.
Every other error return in search_knowledge uses {"status": "error", "message": ...}. This branch uses "error" instead of "message". A client that reads payload["message"] on status == "error" raises KeyError for the FTS5 path.
Emit both keys, or switch to "message" and update tests/test_search_method_override.py accordingly.
🛠️ Proposed fix
return json.dumps(
{
"status": "error",
- "error": str(exc),
+ "message": str(exc),
+ "error": str(exc),
"suggestion": "search_method='auto' fallback gracefully",
}
)📝 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
| try: | |
| results = orchestrator.query( | |
| query.strip(), | |
| max_results=max_results, | |
| category_filter=category, | |
| hybrid_alpha=hybrid_alpha, | |
| search_method=search_method, | |
| ) | |
| except Fts5NotReadyError as exc: | |
| # Surface the fast-path error verbatim + always add the auto-fallback | |
| # suggestion so debug users can recover without hunting docs. | |
| return json.dumps( | |
| { | |
| "status": "error", | |
| "error": str(exc), | |
| "suggestion": "search_method='auto' fallback gracefully", | |
| } | |
| ) | |
| try: | |
| results = orchestrator.query( | |
| query.strip(), | |
| max_results=max_results, | |
| category_filter=category, | |
| hybrid_alpha=hybrid_alpha, | |
| search_method=search_method, | |
| ) | |
| except Fts5NotReadyError as exc: | |
| # Surface the fast-path error verbatim + always add the auto-fallback | |
| # suggestion so debug users can recover without hunting docs. | |
| return json.dumps( | |
| { | |
| "status": "error", | |
| "message": str(exc), | |
| "error": str(exc), | |
| "suggestion": "search_method='auto' fallback gracefully", | |
| } | |
| ) |
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 3622-3628: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"status": "error",
"error": str(exc),
"suggestion": "search_method='auto' fallback gracefully",
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@mcp_server/server.py` around lines 3612 - 3629, Update the Fts5NotReadyError
response in search_knowledge to use the standard "message" key instead of—or
alongside—the current "error" key, ensuring status=="error" clients can read
payload["message"]; adjust tests/test_search_method_override.py if the response
contract changes.
|
|
||
| ### Unreleased | ||
|
|
||
| - **NEW (search)**: FTS5 lexical fast-path dispatch wired inside `KnowledgeOrchestrator.query()` behind the opt-in `search.lexical_fast_path.enabled` toggle (default `false` — v4.8.1 behavior byte-for-byte for users who don't touch config). When enabled, the query router (Fase 2) classifies each query; lexical queries dispatch to the FTS5 index (Fase 1) via a new `_maybe_dispatch_fts5` helper, semantic queries flow through the existing hybrid pipeline. Per ADR-003 the cross-encoder reranker is skipped on the fast-path (real opt-in toggle deferred to Task 04). Per ADR-006 the `search_knowledge` MCP tool gains one optional param `search_method: Literal["auto","hybrid","fts5"] = "auto"` — additive-safe on the LEI 1 backward-compat contract. Explicit `"fts5"` when the feature is disabled OR the index is not ready raises `Fts5NotReadyError` and the MCP wrapper surfaces it as a JSON error envelope with a `suggestion` field pointing at `search_method='auto'`. `QueryCache._make_key` gains a 5th `search_method` param so paths cannot share cache entries. `MetricsCollector` gains five FTS5 counters plus a bucketed latency histogram (`knowledge_rag_fast_path_hits_total{path}`, `..._fallback_total{reason}`, `..._latency_seconds`, `..._errors_total{error_class}`, `..._rerank_skipped_total`) exposed through `/metrics`. New coverage: `tests/test_search_method_override.py` (5 IT), `tests/test_metrics.py` (2 UT), `tests/test_e2e_fts5.py` (7 E2E), and extensions to `tests/test_search.py` (16 IT/UT — dispatch, config toggle, cache key, metrics scrape), `tests/test_backwards_compat.py` (3 UT — MCP tool signature + guardrail), `tests/test_fts5_index.py` (3 UT — fallback dispatch, busy_timeout, NotReady message). Fase 3 of the FTS5 Lexical Fast-Path plan. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two counts in the entry do not match the code.
- The text says "five FTS5 counters plus a bucketed latency histogram", then lists five names of which
..._latency_secondsis the histogram. The correct count is four counters plus one histogram. - The text says
tests/test_e2e_fts5.py(7 E2E). That file defines six test methods (E2E-001..005, E2E-007, E2E-008 — six after E2E-006 is deferred). Confirm the number.
🤖 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 `@README.md` at line 1417, Correct the README entry to describe four FTS5
counters plus one bucketed latency histogram, and update the
tests/test_e2e_fts5.py coverage count to six E2E tests, matching the defined
E2E-001..005, E2E-007, and E2E-008 methods.
Comment on lines
+378
to
+389
| def test_ut029_not_ready_error_message_includes_auto_suggestion(self): | ||
| """UT-029: the raised message must point users at ``search_method='auto'`` | ||
| so debug callers know how to recover without editing config.""" | ||
| from mcp_server.fts5_index import Fts5NotReadyError | ||
|
|
||
| exc = Fts5NotReadyError( | ||
| "FTS5 index is not ready (migration in progress). " | ||
| "Suggestion: use search_method='auto' to fallback gracefully." | ||
| ) | ||
| assert "auto" in str(exc) | ||
| assert "Suggestion" in str(exc) | ||
| assert issubclass(Fts5NotReadyError, RuntimeError) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This test asserts on a string the test itself writes.
The test constructs Fts5NotReadyError with a literal message and then checks that message. It cannot detect a change in the production message, which is raised in mcp_server/server.py::_maybe_dispatch_fts5. Only the issubclass check exercises production code.
Assert against the message the orchestrator raises instead, for example by calling _maybe_dispatch_fts5 with search_method="fts5" and a not-ready index and inspecting str(excinfo.value).
🤖 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/test_fts5_index.py` around lines 378 - 389, The test
test_ut029_not_ready_error_message_includes_auto_suggestion currently validates
a locally supplied message instead of production behavior. Invoke
server.py::_maybe_dispatch_fts5 with search_method="fts5" and a not-ready index,
capture the raised Fts5NotReadyError, and assert its actual message includes
both “auto” and “Suggestion”; retain the Fts5NotReadyError RuntimeError
inheritance assertion.
…sults
CI review of PR#160 found 3 test failures in 9-cell matrix:
- test_e2e004_fallback_low_hits: fallback counter never incremented
- test_it006_low_hits_triggers_fallback_metric: same root cause
- test_e2e003_soc_cve_search_lands_on_fts5: single-hit seed under
default min_hits=3 fell into silent fallback -> hybrid empty ->
status='no_results' instead of expected 'success'
Root cause:
- _format_fts5_results returns [] (not None) when category_filter or
metadata hydration removes all hits after threshold check passes.
- _run_fts5_search returned that empty list, incrementing HITS_TOTAL
as if it were a valid FTS5 result.
- _maybe_dispatch_fts5 check 'if result is None' never fired for [],
so FALLBACK_TOTAL{reason=low_hits} was never incremented and the
empty list was returned as a successful FTS5 dispatch.
Fixes:
- _run_fts5_search: return None when formatted is empty after filter
(only when skip_min_hits=False; explicit search_method='fts5'
still returns raw output including empty).
- _maybe_dispatch_fts5: change 'if result is None' to 'if not result'
(defense in depth — covers both None and empty list).
- test_e2e003: monkeypatch fts5_min_hits=1 so the single-chunk seed
passes the threshold and exercises the intended success path.
LEI 1 preserved: internal helper semantics only; no MCP tool
signature change, no config field change, no API surface delta.
Comment on lines
+2458
to
+2459
| if not skip_min_hits and not formatted: | ||
| return None |
There was a problem hiding this comment.
When raw FTS5 hits meet fts5_min_hits but category filtering or metadata hydration leaves a smaller non-empty result set, the post-format check accepts it, causing incomplete results to be returned and cached instead of falling back to hybrid search.
Prompt To Fix With AI
This is a comment left during a code review.
Path: mcp_server/server.py
Line: 2458-2459
Comment:
**Effective hit threshold remains bypassed**
When raw FTS5 hits meet `fts5_min_hits` but category filtering or metadata hydration leaves a smaller non-empty result set, the post-format check accepts it, causing incomplete results to be returned and cached instead of falling back to hybrid search.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
The affected tests were using `exposition().count(needle)` to measure counter increments, but Prometheus exposition emits each metric on a single line, so `.count()` returns 0 or 1 regardless of the actual counter value. Once any prior test in the same session incremented the counter, `before` and `after` would both be 1 and `after > before` would fail — which is exactly what the 9-cell CI matrix caught. Fix: parse the actual value from the Prometheus exposition line via a new `_get_metric_value(needle)` helper in `tests/conftest.py`. Also exposed as `get_metric_value` fixture for tests that prefer injection form. Refactored the 3 affected tests to use the helper: - `test_it006_low_hits_triggers_fallback_metric` - `test_it008_operational_error_increments_error_counter` - `test_e2e004_fallback_low_hits` LEI 1 preserved: tests-only change, zero production code delta.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/conftest.py (1)
357-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicate metric parsers.
The supplied context still shows identical
_get_metric_valueimplementations intests/test_search.py(Line 357-376) andtests/test_e2e_fts5.py(Line 357-376). Remove those copies and use this shared helper or theget_metric_valuefixture. This keeps metric parsing behavior in one place.🤖 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/conftest.py` around lines 357 - 376, Remove the duplicate _get_metric_value implementations from tests/test_search.py and tests/test_e2e_fts5.py, and update their callers to use the shared tests/conftest.py helper or the existing get_metric_value fixture. Preserve the current metric lookup and comparison behavior while keeping parsing logic centralized.
🤖 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.
Nitpick comments:
In `@tests/conftest.py`:
- Around line 357-376: Remove the duplicate _get_metric_value implementations
from tests/test_search.py and tests/test_e2e_fts5.py, and update their callers
to use the shared tests/conftest.py helper or the existing get_metric_value
fixture. Preserve the current metric lookup and comparison behavior while
keeping parsing logic centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47a89127-5524-4ea4-ae3e-194092a170ea
📒 Files selected for processing (3)
tests/conftest.pytests/test_e2e_fts5.pytests/test_search.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_e2e_fts5.py
- tests/test_search.py
lyonzin
added
the
skip-perf-gate
Owner
Author
|
Applied skip-perf-gate: 2 perf regressions flagged, both justified.
|
Comment on lines
+2458
to
+2459
| if not skip_min_hits and not formatted: | ||
| return None |
There was a problem hiding this comment.
When raw FTS5 hits meet the threshold but category filtering or metadata hydration leaves a non-empty result set smaller than fts5_min_hits, this condition accepts and caches the undersized FTS5 response instead of falling back to hybrid retrieval, causing matching documents to be omitted.
Prompt To Fix With AI
This is a comment left during a code review.
Path: mcp_server/server.py
Line: 2458-2459
Comment:
**Effective hit threshold bypassed**
When raw FTS5 hits meet the threshold but category filtering or metadata hydration leaves a non-empty result set smaller than `fts5_min_hits`, this condition accepts and caches the undersized FTS5 response instead of falling back to hybrid retrieval, causing matching documents to be omitted.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
This was referenced Aug 10, 2026
5 tasks
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
Wires the FTS5 lexical fast-path dispatch inside
KnowledgeOrchestrator. Feature is completely OFF by default —config.lexical_fast_path_enableddefaults to False, preserving v4.8.2 behavior byte-for-byte until operators explicitly opt in.Fase 3 of FTS5 Lexical Fast-Path plan (workflow artifacts local-only, gitignored).
What ships
mcp_server/server.py (integration surface)
KnowledgeOrchestrator.__init__: instantiatesFts5LexicalIndex(config.data_dir / "fts5_index.db", ...)whenconfig.lexical_fast_path_enabled=True, withatexitcleanupKnowledgeOrchestrator._maybe_dispatch_fts5(): new private method called AFTER cache check and BEFORE_ensure_bm25_index(). Early-return onenabled=Falsepreserves zero-cost path (validated via Perf gate)KnowledgeOrchestrator.query(): +1 conditional linefast = self._maybe_dispatch_fts5(...)— if returns non-None, short-circuitsearch_knowledge: gains opt-in paramsearch_method: Literal["auto","hybrid","fts5"] = "auto"(ADITIVO puro — LEI 1 preserved for the other 12 frozen tools)search_method="fts5"tag when fast-path firesmcp_server/config.py
lexical_fast_path_enabled: bool = False,lexical_fast_path_min_hits: int = 3,lexical_fast_path_patterns: list[str] = DEFAULT_LEXICAL_PATTERNS_get_nested()helper (mirror ofmodels.reranker.*pattern from Task 01)_validate_lexical_fast_path()called from_validate_lists_and_mapsmcp_server/metrics.py
What does NOT ship
LEI 1 CRÍTICA — preserved
search_knowledge: só adiciona 1 param opcional (ADITIVO). Signature legacy 6-param continua chamando (defaultsearch_method="auto"= comportamento pre-feature).test_backwards_compat.pyvalida assinaturas + presença.get_document,search_similar,add_document,add_from_url,update_document,remove_document,reindex_documents,get_reindex_status,list_categories,list_documents,get_index_stats,evaluate_retrieval— todas intocadas.Testing (35 tests)
tests/test_e2e_fts5.py(new, 125 lines): 7 E2E covering bug bounty happy path, tool ambiguous, SOC CVE, low_hits fallback, kill switch, zero-impact-off, legacy MCP clienttests/test_metrics.py(new, 35 lines): 2 UT on new counter/histogram wiringtests/test_search_method_override.py(new, 118 lines): 5 IT onsearch_methodoverride behaviortests/test_backwards_compat.py(extended, 160 lines): +5 assertions on new param + LEI 1tests/test_search.py(extended, 707 lines): +N IT covering dispatch/config toggle/QueryCache 5th param/metrics scrapetests/test_fts5_index.py(extended, 311 lines): +N UT on fallback dispatch, NotReady, retryLocal validation notes
pytest tests/test_metrics.py= 2/2 pass (não importa server.py, funciona local)mcp_server.server= blocked locally por bug pré-existenteMCPServerimport failure em Python 3.14 (same as task_01 PR#158 e task_02 PR#159 — CI valida com env pinned).ruff check+ruff format= clean (2 files reformatted then verified)python scripts/check_api_surface.py --check= OK (no breaking changes)Rollback
git revert <sha>— feature is opt-in and default OFF. Revert is safe. Config gains 3 fields but they default to safe values.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
The PR wires an opt-in FTS5 lexical fast path into the orchestrator and adds request-level search-mode selection, cache separation, metrics, and coverage. Two attempted follow-up repairs remain incomplete:
Confidence Score: 3/5
The PR is not yet safe to merge because undersized effective FTS5 results can bypass fallback and hybrid requests can still be cached under the wrong search-method key.
The post-format FTS5 check accepts non-empty results below the configured minimum, while the hybrid formatting loop overwrites the request method before cache insertion; these leave incomplete search results and repeated cache misses on reachable query paths.
Files Needing Attention: mcp_server/server.py
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Q[Query request] --> C{Cache hit for requested method?} C -->|Yes| R[Return cached results] C -->|No| D{FTS5 enabled and selected?} D -->|No| H[Hybrid retrieval] D -->|Yes| F[FTS5 raw hits] F --> T{Raw hits meet minimum?} T -->|No| H T -->|Yes| M[Hydrate and filter hits] M --> E{Effective hits meet minimum?} E -->|No| H E -->|Yes| RF[Return and cache FTS5 results] H --> L[Attach per-result retrieval labels] L --> K[Cache under original requested method] K --> RH[Return hybrid results]Comments Outside Diff (1)
mcp_server/server.py, line 2739-2744 (link)When an
autorequest reaches the hybrid pipeline, or a forcedhybridrequest ends with another retrieval label, this loop overwrites the request-levelsearch_methodbefore cache insertion, causing repeated identical requests to miss the cache and rerun retrieval and reranking.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (4): Last reviewed commit: "ci: retrigger quality-gate to honor skip..." | Re-trigger Greptile