Feat/hybrid retrieval token budgets - #179
Conversation
Search read and parsed every note on every query to run `needle in haystack`, then sorted hits by DATE. A natural-language question with no literal substring returned nothing at all. Meanwhile `list-notes` returned the whole vault in one MCP result — ~90,800 tokens on 744 notes — and `read-note` sent every body through the context twice. Add `omind.searchindex`: one disposable SQLite file per vault, in the state dir, with FTS5/BM25 over heading-split chunks, packed float32 chunk vectors, and the resolved [[wikilink]] graph. Queries fuse a keyword leg, a semantic leg and a weak recency leg with Reciprocal Rank Fusion; hits carry the matched excerpt. Notes stay the source of truth — the index is never committed, never mesh-synced, refreshes only what changed, and every path fails open to the old scan when it is unavailable, disabled or corrupt. Page every list-shaped MCP tool; give `read-note` one representation; serve backlinks and the graph from the index instead of two more full-vault scanners; move the per-prompt gate suggestion off its two full listings. Retire `omind.vectorindex` (metadata-only embeddings, JSON float storage, refresh-per-query, pure-Python cosine). Measured on the live 744-note vault via the new `omind bench`: search 268 ms -> 18 ms natural-language 0 hits -> 10 ranked hits index build 1.5 s (5,678 chunks); incremental refresh 5 ms list-notes payload ~90,800 -> 3,136 tokens Design and sources: docs/retrieval.md. Next tier filed as #167-#178. Co-Authored-By: Claude Opus 5 <[email protected]>
Captures what CONTRIBUTING.md deliberately leaves out: the invariants that break silently (derived data never in the vault, retrieval fails open, credential de-prioritization, no unbounded MCP payload), the state of the in-flight hybrid-index branch, the gotchas found while building it, and the document-frequency dead end so nobody walks it twice. Points at #167-#178 with a recommended order, and flags the two items (#172, #177) that need a decision before any code. Co-Authored-By: Claude Opus 5 <[email protected]>
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntroduces a disposable SQLite hybrid search index with incremental indexing, BM25, optional embeddings, recency fusion, excerpts, backlinks, and graph support. Store, CLI, retrieval, and MCP layers use indexed results with fallbacks. MCP list tools are paged, ChangesHybrid retrieval and bounded access
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OmiStore
participant SearchIndex
participant SQLiteFTS5
participant embed.encode
Client->>OmiStore: submit search query
OmiStore->>SearchIndex: refresh and search
SearchIndex->>SQLiteFTS5: rank keyword matches
SearchIndex->>embed.encode: encode query when enabled
SearchIndex-->>OmiStore: fused hits with excerpts and scores
OmiStore-->>Client: ranked search results
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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 `@AGENTS.md`:
- Around line 58-61: Update the handoff status in the section containing the
feat/hybrid-retrieval-token-budgets branch details to reflect that the branch is
already published and associated with PR `#179`, or clearly mark the existing “not
pushed” statement as historical. Keep the commit, test, version, and changelog
details accurate.
In `@BACKLOG.md`:
- Around line 88-89: Update the “Adopt an external memory framework” backlog
entry to include a valid GitHub issue link, creating the corresponding issue
first if necessary; otherwise remove the entry. Preserve BACKLOG.md and GitHub
Issues as synchronized views of the same work list.
In `@CHANGELOG.md`:
- Line 10: Add a blank line immediately after each changelog heading—“###
Added”, “### Changed”, and “### Removed”—before the corresponding list entries,
preserving all existing changelog content.
In `@docs/retrieval.md`:
- Line 8: Update the fenced code block in the retrieval documentation to specify
the text language immediately after the opening fence, using the existing block
content unchanged.
In `@README.md`:
- Around line 389-390: Update the README pagination statement to include total
alongside limit, offset, and has_more. Ensure the documented contract reflects
that every list-shaped MCP tool uses all four metadata fields through
server._page.
In `@src/omind/bench.py`:
- Around line 126-138: Close each throwaway cold-start SearchIndex created in
the query loop after its timed search completes, ensuring cleanup also occurs if
the search raises. Update the loop around the fresh SearchIndex and cold _timed
call; leave the reusable warm index and scan behavior unchanged.
- Around line 159-166: Update the `list-notes, one page` label to derive its
page-size value from the imported `DEFAULT_PAGE` constant instead of hardcoding
25, keeping the label synchronized with the `rows[:DEFAULT_PAGE]` pagination
behavior.
In `@src/omind/cli.py`:
- Around line 924-950: Update OmiStore.index to obtain the index through
searchindex.shared(self.omi_dir) instead of constructing a new SearchIndex
directly. Preserve the existing unavailable-index behavior by propagating
shared()'s None result, and keep index reuse semantics for callers such as
OmiStore.search. Do not change the explain, graph, or retrieve paths.
In `@src/omind/retrieve.py`:
- Around line 196-206: Update the credential filtering around _looks_credential
and _NoteRow so indexed results retain and evaluate each note’s summary in
addition to its title and tags. Propagate the summary through
searchindex._materialize and the notes index data, then pass it to the
credential classifier while preserving the existing task_is_cred bypass and hard
exclusion for credential notes.
In `@src/omind/searchindex.py`:
- Around line 546-562: Update _embed_chunks to process pending chunks in
fixed-size batches rather than calling embed.encode for the entire collection at
once. Encode and insert each batch independently, bound temporary matrix and
packed-row memory, and return the total number of chunks successfully persisted
so partial progress is counted while preserving the existing unavailable/backend
error behavior.
- Around line 654-671: Move allowed-filename filtering into the SQL queries
before ranking and LIMIT in _bm25, _vector_leg, and _recency_leg, so eligible
matches are selected from the full result set. Preserve unrestricted behavior
when allowed is None, and use a scalable join or equivalent mechanism for large
allowed sets rather than exceeding SQLite variable limits. Remove the
corresponding post-LIMIT filtering.
- Around line 465-476: Update _ingest to accept and reuse the already-fetched
stat result from refresh instead of calling path.stat() after _read_text; pass
that st from the existing refresh call site. Ensure the ingest/refresh error
handling rolls back the transaction for any exception, including OSError, and
preserves fail-open behavior by returning None rather than propagating scan
errors.
In `@src/omind/server.py`:
- Around line 125-138: Update read_note to validate representation before
selecting the payload: accept only "fields" and "raw", and raise a clear error
for any other value instead of falling through to the fields response. Preserve
the existing raw and parsed-fields behavior for supported values.
In `@src/omind/store.py`:
- Around line 978-985: Update the note reads in the search result flow around
`_cached_summary` and in `_indexed_backlinks` to resolve every index-supplied
filename through `OmiStore.safe_name` before joining it with `self.omi_dir`.
Catch and skip `NoteError` for invalid filenames so malformed index rows do not
abort the read path, while preserving the existing handling for deleted notes
and valid results.
In `@tests/test_searchindex.py`:
- Around line 79-89: Gate the index-dependent tests for missing FTS5 support: in
tests/test_searchindex.py lines 79-89, add module-level pytest skipif using
searchindex._fts5_available(), while preserving the fail-open tests at lines
261-291; in tests/test_bench.py lines 34-44, leave capsule/token assertions
unconditional and conditionally perform the index-build, search measurement, and
“ms” unit assertions using searchindex.available(); in tests/test_bench.py lines
53-60, skip the test when the index is unavailable because scan fallback makes
both results empty.
- Around line 79-89: Add an availability guard for the indexed-path tests in
tests/test_searchindex.py, using searchindex.available() to skip tests that
require FTS5 when unavailable. Keep the fail-open tests covering unavailable
behavior outside this guard so they continue to run on both supported and
unsupported SQLite configurations.
In `@tests/test_server.py`:
- Around line 167-198: Extend the bounded list-tool coverage in
test_every_list_tool_is_bounded to include graph-path and verify its response
uses result, count, offset, total, and has_more. Update graph-path to return its
path through the shared server._page pagination with limit clamped at MAX_PAGE,
then create more than 100 graph rows and a path exceeding 100 nodes so the test
proves the hard cap rather than merely returning all available entries.
🪄 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 Plus
Run ID: 9de2220f-8a13-488d-90ca-327c46c49db9
📒 Files selected for processing (20)
AGENTS.mdBACKLOG.mdCHANGELOG.mdCONTRIBUTING.mdREADME.mddocs/retrieval.mdsrc/omind/bench.pysrc/omind/cli.pysrc/omind/embed.pysrc/omind/graph.pysrc/omind/retrieve.pysrc/omind/searchindex.pysrc/omind/server.pysrc/omind/store.pysrc/omind/vectorindex.pytests/test_bench.pytests/test_searchindex.pytests/test_server.pytests/test_store.pytests/test_vectorindex.py
💤 Files with no reviewable changes (2)
- src/omind/vectorindex.py
- tests/test_vectorindex.py
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (ubuntu-latest, 3.14)
- GitHub Check: test (windows-latest, 3.14)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Treat Markdown notes in the Obsidian vault as the source of truth; keep indexes, caches, and vectors derived and disposable, and store all derived state underpaths.state_dir(), never in the vault.
Documentation files must carry the specified Nebraska footer; the README uses the centered banner variant.
Files:
CONTRIBUTING.mdAGENTS.mdCHANGELOG.mdREADME.mddocs/retrieval.mdBACKLOG.md
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Do not merge or consolidate notes destructively without proposal and review; incorrect merges can destroy memory that exists nowhere else.
Treat changes that rename or consolidate graph tools as coordinated fleet changes requiring user approval, because tool names are referenced by vault playbooks, managed skills, and other machines.
Keep BACKLOG.md and GitHub Issues synchronized as two views of the same work list; link each backlog item to an issue and update both when work ships.
Commit or push only when explicitly asked; branch frommainusing the prescribed prefixes and use conventional-commit subjects.
Report failed quality gates and skipped scope honestly, including the relevant output or reason.
Files:
CONTRIBUTING.mdAGENTS.mdtests/test_store.pysrc/omind/embed.pytests/test_bench.pyCHANGELOG.mdREADME.mddocs/retrieval.mdsrc/omind/graph.pysrc/omind/bench.pysrc/omind/retrieve.pytests/test_server.pytests/test_searchindex.pyBACKLOG.mdsrc/omind/server.pysrc/omind/cli.pysrc/omind/store.pysrc/omind/searchindex.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Retrieval must fail open: every search layer returns
Noneon errors and falls back to the older path. Handle missing models, corrupt indexes, locked databases, and missing FTS5 builds without breaking search, and test failure branches.
Files:
tests/test_store.pysrc/omind/embed.pytests/test_bench.pysrc/omind/graph.pysrc/omind/bench.pysrc/omind/retrieve.pytests/test_server.pytests/test_searchindex.pysrc/omind/server.pysrc/omind/cli.pysrc/omind/store.pysrc/omind/searchindex.py
**/retrieve.py
📄 CodeRabbit inference engine (AGENTS.md)
Credential notes must be de-prioritised in search and gate suggestions unless the query concerns credentials; preserve the behavior represented by
retrieve._CREDENTIAL_PENALTY.
Files:
src/omind/retrieve.py
**/server.py
📄 CodeRabbit inference engine (AGENTS.md)
MCP tools must never return unbounded output. Every list-shaped tool must paginate using
limit,offset,total, andhas_morethroughserver._page.
Files:
src/omind/server.py
**/{notes,store}.py
📄 CodeRabbit inference engine (AGENTS.md)
Route every note write through
OmiStore; external writers should usenotes.upsert_note. Preserve flocking, atomic rename, LamportRev:stamping, and soft deletion. Deletes must archive withDisabled: true; onlyomind mesh purgemay permanently remove notes.
Files:
src/omind/store.py
**/store.py
📄 CodeRabbit inference engine (AGENTS.md)
**/store.py: UseOmiStore.safe_namefor every note read and write; never bypass it, because path traversal must remain impossible.
Keepstore.pyframework-free: it must not depend on FastAPI or MCP, because both the CLI and web application build on it.
Files:
src/omind/store.py
**/{store,searchindex}.py
📄 CodeRabbit inference engine (AGENTS.md)
Do not mutate a
NoteSummaryreturned from_cached_summary; usedataclasses.replacewhen creating modified summaries.
Files:
src/omind/store.pysrc/omind/searchindex.py
**/searchindex.py
📄 CodeRabbit inference engine (AGENTS.md)
**/searchindex.py: Keep recency as a re-ranking leg only: it may reorder notes matched by content legs but must never add otherwise-unmatched notes to results.
Preserve case inlink_targets()for dangling-link reports; only link resolution should lowercase targets.
Convert embedding results throughsearchindex._query_vector; do not assumeembed.encodereturns an object with.shape, because test backends may return plain lists.
Do not reintroduce document-frequency threshold filtering of query terms; use threshold-free graded matching where all-words matches rank above any-words matches.
When changing retrieval, preserve both indexed and fallback paths and verify behavior withOMI_INDEX_DISABLE=1; changes must not work only when the index is healthy.
Files:
src/omind/searchindex.py
🪛 ast-grep (0.44.1)
src/omind/bench.py
[info] 164-164: use jsonify instead of json.dumps for JSON output
Context: json.dumps(rows[:DEFAULT_PAGE], ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 165-165: use jsonify instead of json.dumps for JSON output
Context: json.dumps(rows, ensure_ascii=False)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
src/omind/cli.py
[info] 960-960: use jsonify instead of json.dumps for JSON output
Context: json.dumps(report.to_dict(), indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
src/omind/searchindex.py
[warning] 246-246: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: retrieve._WORD_RE.findall(query.lower())
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
[warning] 1078-1078: XPath query is request-/variable-derived; use parameterized XPath to prevent injection.
Context: _WIKILINK_RE.findall(md)
Note: [CWE-643] Improper Neutralization of Data within XPath Expressions ('XPath Injection').
(xpath-injection-python)
🪛 GitHub Check: CodeQL
src/omind/retrieve.py
[notice] 182-182: Cyclic import
Import of module omind.searchindex begins an import cycle.
src/omind/store.py
[notice] 941-941: Cyclic import
Import of module omind.searchindex begins an import cycle.
src/omind/searchindex.py
[notice] 58-58: Cyclic import
Import of module omind.retrieve begins an import cycle.
[notice] 397-397: Cyclic import
Import of module omind.store begins an import cycle.
[notice] 463-463: Cyclic import
Import of module omind.store begins an import cycle.
[notice] 1011-1011: Cyclic import
Import of module omind.store begins an import cycle.
[notice] 1076-1076: Cyclic import
Import of module omind.store begins an import cycle.
🪛 LanguageTool
AGENTS.md
[locale-violation] ~96-~96: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...e to check the model revision (cached afterwards). It fails open when offline. ### Dead...
(AFTERWARDS_US)
BACKLOG.md
[style] ~61-~61: To elevate your writing, consider using more formal language here.
Context: ...ps code fences before extracting links, and the index does not, so a naive swap w...
(AND_WHEREAS)
🪛 markdownlint-cli2 (0.23.0)
CHANGELOG.md
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 26-26: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
docs/retrieval.md
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 OpenGrep (1.25.0)
src/omind/searchindex.py
[ERROR] 365-365: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 853-856: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 958-958: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🔇 Additional comments (34)
src/omind/server.py (1)
21-21: LGTM!Also applies to: 41-62, 229-242, 257-266, 286-295, 305-318, 334-352
tests/test_server.py (1)
98-101: LGTM!Also applies to: 201-212
AGENTS.md (1)
1-53: LGTM!Also applies to: 63-160
BACKLOG.md (1)
5-7: LGTM!Also applies to: 8-85, 91-94
CHANGELOG.md (1)
11-24: LGTM!Also applies to: 27-44, 46-50
CONTRIBUTING.md (1)
7-9: LGTM!README.md (1)
385-387: LGTM!Also applies to: 392-408
src/omind/searchindex.py (9)
90-106: LGTM!Also applies to: 153-198
256-305: LGTM!
330-392: LGTM!
566-612: LGTM!
702-759: LGTM!
761-837: LGTM!
841-946: LGTM!
977-999: LGTM!
1002-1097: LGTM!src/omind/embed.py (1)
153-154: LGTM!tests/test_searchindex.py (3)
24-73: LGTM!
147-215: LGTM!
221-304: LGTM!docs/retrieval.md (1)
60-104: LGTM!src/omind/store.py (3)
22-22: LGTM!Also applies to: 134-140, 250-254, 750-752
932-948: LGTM!
1025-1027: LGTM!Also applies to: 1050-1061, 1065-1068
src/omind/retrieve.py (1)
216-225: LGTM!src/omind/graph.py (2)
70-82: LGTM!
84-118: LGTM!tests/test_store.py (1)
621-626: LGTM!src/omind/cli.py (4)
276-291: LGTM!Also applies to: 335-360
856-885: LGTM!
953-962: LGTM!
1166-1175: LGTM!Also applies to: 1309-1310
src/omind/bench.py (1)
38-85: LGTM!tests/test_bench.py (1)
19-31: 📐 Maintainability & Code Quality
tests/conftest.pyalready isolatespaths.state_dir()for every test by settingXDG_STATE_HOMEto a per-test temp dir, so these bench tests won't touch the developer state dir.> Likely an incorrect or invalid review comment.
| Branch **`feat/hybrid-retrieval-token-budgets`**, one commit (`2eea146`), | ||
| **not pushed to either mirror**. 808 tests + ruff + mypy strict green. | ||
| Version deliberately **not bumped** — changes sit under `## [Unreleased]` in | ||
| `CHANGELOG.md`; the next release is 4.3.0 (module docstrings already say so). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Refresh the stale handoff state.
This says the source branch was “not pushed to either mirror,” but PR #179 is already open from that branch on GitHub as of July 25, 2026. Mark this as historical or update it whenever the handoff changes so agents do not repeat or avoid publishing work.
🤖 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 `@AGENTS.md` around lines 58 - 61, Update the handoff status in the section
containing the feat/hybrid-retrieval-token-budgets branch details to reflect
that the branch is already published and associated with PR `#179`, or clearly
mark the existing “not pushed” statement as historical. Keep the commit, test,
version, and changelog details accurate.
| - [ ] **Long game: fine-tune a model on the accumulated violation corpus** ([#91](https://github.com/CryptoJones/omind/issues/91), closed not-planned) — _roadmap (Phase 4)_ — deferred: the blocker is data, not compute. The live `compliance.jsonl` corpus is ~91% relevance-noise, ~6% real denies, and 100% DENY (zero ALLOW), so training on it as-is yields an always-deny model. Revisit only after `export-corpus` is reworked to synthesize balanced ALLOW examples (from the deterministic `guard.decide()`) and split the relevance corpus from the action corpus. The mechanical guard remains the backstop. | ||
| - [ ] **Adopt an external memory framework (Mem0 / Cognee / Zep) as the storage layer** — _rejected_ — evaluated during the 2026-07-24 survey. Every one of them wants to own storage, and omind's whole premise is that the Markdown vault is the source of truth: plain files, git-replicated across the mesh, readable in Obsidian, with no service to run. The techniques are worth copying; the dependency is not. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Link the new backlog item to a GitHub issue.
Adopt an external memory framework has no issue reference, despite this file requiring every backlog item to correspond to a GitHub issue. Create/link the issue or remove the item.
As per coding guidelines, keep BACKLOG.md and GitHub Issues synchronized as two views of the same work list.
🤖 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 `@BACKLOG.md` around lines 88 - 89, Update the “Adopt an external memory
framework” backlog entry to include a valid GitHub issue link, creating the
corresponding issue first if necessary; otherwise remove the entry. Preserve
BACKLOG.md and GitHub Issues as synchronized views of the same work list.
Source: Coding guidelines
|
|
||
| ## [Unreleased] | ||
|
|
||
| ### Added |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines after the changelog headings.
### Added, ### Changed, and ### Removed each need a blank line before their lists.
Proposed fix
### Added
+
- **A derived hybrid search index (`omind.searchindex`).**
### Changed
+
- **Search is relevance-ranked, not substring-filtered.**
### Removed
+
- `omind.vectorindex`.Also applies to: 26-26, 45-45
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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 `@CHANGELOG.md` at line 10, Add a blank line immediately after each changelog
heading—“### Added”, “### Changed”, and “### Removed”—before the corresponding
list entries, preserving all existing changelog content.
Source: Linters/SAST tools
| whole design: the notes in your vault are the source of truth, and the index is | ||
| a disposable cache you can delete at any time. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced block (markdownlint MD040).
🧹 Fix
-```
+```text
~/Documents/Obsidian Vault/OMI/*.md source of truth — synced, committed, yours🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/retrieval.md` at line 8, Update the fenced code block in the retrieval
documentation to specify the text language immediately after the opening fence,
using the existing block content unchanged.
Source: Linters/SAST tools
| Every list-shaped MCP tool is paged (`limit`, `offset`, `has_more`), so no tool | ||
| can return the whole vault in a single result. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document the complete pagination contract.
Add total to this list. The required metadata is limit, offset, total, and has_more; omitting total makes the README inconsistent with the MCP API contract.
As per coding guidelines, every list-shaped MCP tool must paginate using limit, offset, total, and has_more through server._page.
🤖 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` around lines 389 - 390, Update the README pagination statement to
include total alongside limit, offset, and has_more. Ensure the documented
contract reflects that every list-shaped MCP tool uses all four metadata fields
through server._page.
Source: Coding guidelines
| @staticmethod | ||
| def _bm25(db: sqlite3.Connection, expr: str, allowed: set[str] | None) -> list[int]: | ||
| """Chunk ids for one MATCH expression, best BM25 first. Column weights put | ||
| a title/tag match well above a body mention of the same word.""" | ||
| try: | ||
| rows = db.execute( | ||
| "SELECT f.rowid AS id, c.filename AS filename FROM chunks_fts f" | ||
| " JOIN chunks c ON c.id = f.rowid" | ||
| " WHERE chunks_fts MATCH ?" | ||
| " ORDER BY bm25(chunks_fts, 5.0, 2.0, 4.0, 1.0, 1.0)" | ||
| " LIMIT ?", | ||
| (expr, _LEG_DEPTH * 3), | ||
| ) | ||
| except sqlite3.Error: | ||
| return [] # an unparseable expression is no matches, never a crash | ||
| return [ | ||
| int(r["id"]) for r in rows if allowed is None or str(r["filename"]) in allowed | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tag/archive filtering is applied after the SQL LIMIT, so filtered queries can lose real hits.
_bm25 takes the global top _LEG_DEPTH * 3 rows and only then drops rows outside allowed. On a vault where the tag is a small slice, every one of those 180 rows can belong to other notes, and search("query", tag="pets") returns [] while matching chunks sit at rank 181+. _vector_leg (Lines 694-698) and _recency_leg (Lines 740-742) truncate-then-filter the same way.
Push the filter into the query instead of post-filtering. The existing tests only cover a tag with an empty query (the _listing path), so this gap is untested.
🛠️ Sketch: filter inside SQL
`@staticmethod`
def _bm25(db: sqlite3.Connection, expr: str, allowed: set[str] | None) -> list[int]:
try:
+ sql = (
+ "SELECT f.rowid AS id FROM chunks_fts f"
+ " JOIN chunks c ON c.id = f.rowid"
+ " WHERE chunks_fts MATCH ?"
+ )
+ args: list[object] = [expr]
+ if allowed is not None:
+ sql += f" AND c.filename IN ({', '.join('?' * len(allowed))})"
+ args.extend(sorted(allowed))
+ sql += " ORDER BY bm25(chunks_fts, 5.0, 2.0, 4.0, 1.0, 1.0) LIMIT ?"
+ args.append(_LEG_DEPTH * 3)
- rows = db.execute(
- "SELECT f.rowid AS id, c.filename AS filename FROM chunks_fts f"
- " JOIN chunks c ON c.id = f.rowid"
- " WHERE chunks_fts MATCH ?"
- " ORDER BY bm25(chunks_fts, 5.0, 2.0, 4.0, 1.0, 1.0)"
- " LIMIT ?",
- (expr, _LEG_DEPTH * 3),
- )
+ rows = db.execute(sql, args)
except sqlite3.Error:
return []
- return [
- int(r["id"]) for r in rows if allowed is None or str(r["filename"]) in allowed
- ]
+ return [int(r["id"]) for r in rows](allowed can be large; a temp table of allowed filenames joined in is the scalable form if you hit SQLite's variable limit.)
🤖 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 `@src/omind/searchindex.py` around lines 654 - 671, Move allowed-filename
filtering into the SQL queries before ranking and LIMIT in _bm25, _vector_leg,
and _recency_leg, so eligible matches are selected from the full result set.
Preserve unrestricted behavior when allowed is None, and use a scalable join or
equivalent mechanism for large allowed sets rather than exceeding SQLite
variable limits. Remove the corresponding post-LIMIT filtering.
| def read_note(name: str, representation: str = "fields") -> dict[str, object]: | ||
| raw = store.read_note(name) | ||
| # One read + one parse: read_fields would re-read the file just read. | ||
| return { | ||
| # ONE representation, never both: returning `raw` and `fields` together | ||
| # sent every note body through the context twice, and the editing caller | ||
| # only ever uses one of them. | ||
| payload: dict[str, object] = { | ||
| "filename": store.safe_name(name).name, | ||
| "raw": raw, | ||
| "fields": parse_note(raw).to_dict(), | ||
| "version": store.note_version(name), | ||
| } | ||
| if representation == "raw": | ||
| payload["raw"] = raw | ||
| else: | ||
| payload["fields"] = parse_note(raw).to_dict() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject unsupported representation values.
Any value other than "raw" silently returns "fields", so a caller requesting an invalid representation receives an unexpected payload. Validate against {"fields", "raw"} and raise a clear error.
🤖 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 `@src/omind/server.py` around lines 125 - 138, Update read_note to validate
representation before selecting the payload: accept only "fields" and "raw", and
raise a clear error for any other value instead of falling through to the fields
response. Preserve the existing raw and parsed-fields behavior for supported
values.
| results: list[NoteSummary] = [] | ||
| for hit in hits: | ||
| summary = self._cached_summary(self.omi_dir / hit.filename) | ||
| if summary is None: | ||
| continue # indexed a note that has since been deleted | ||
| # replace(), not mutation: ``summary`` is the shared cached instance. | ||
| results.append(replace(summary, excerpt=hit.excerpt, score=round(hit.score, 6))) | ||
| return results |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Resolve index-supplied filenames through safe_name.
self.omi_dir / hit.filename bypasses safe_name, so the path-traversal invariant now depends on the derived SQLite file being trustworthy rather than on the store's own check. The index lives in the state dir and is machine-local, but it is also disposable, third-party-writable state feeding a read path — the exact case the rule covers. _indexed_backlinks (Line 1064) does the same.
Swallow NoteError so a bad row is skipped rather than breaking search (fail-open).
As per coding guidelines, "Use OmiStore.safe_name for every note read and write; never bypass it, because path traversal must remain impossible."
🛡️ Proposed fix
results: list[NoteSummary] = []
for hit in hits:
- summary = self._cached_summary(self.omi_dir / hit.filename)
+ try:
+ path = self.safe_name(hit.filename)
+ except NoteError:
+ continue # a corrupt/hostile index row is not a note
+ summary = self._cached_summary(path)
if summary is None:
continue # indexed a note that has since been deleted📝 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.
| results: list[NoteSummary] = [] | |
| for hit in hits: | |
| summary = self._cached_summary(self.omi_dir / hit.filename) | |
| if summary is None: | |
| continue # indexed a note that has since been deleted | |
| # replace(), not mutation: ``summary`` is the shared cached instance. | |
| results.append(replace(summary, excerpt=hit.excerpt, score=round(hit.score, 6))) | |
| return results | |
| results: list[NoteSummary] = [] | |
| for hit in hits: | |
| try: | |
| path = self.safe_name(hit.filename) | |
| except NoteError: | |
| continue # a corrupt/hostile index row is not a note | |
| summary = self._cached_summary(path) | |
| if summary is None: | |
| continue # indexed a note that has since been deleted | |
| # replace(), not mutation: ``summary`` is the shared cached instance. | |
| results.append(replace(summary, excerpt=hit.excerpt, score=round(hit.score, 6))) | |
| return results |
🤖 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 `@src/omind/store.py` around lines 978 - 985, Update the note reads in the
search result flow around `_cached_summary` and in `_indexed_backlinks` to
resolve every index-supplied filename through `OmiStore.safe_name` before
joining it with `self.omi_dir`. Catch and skip `NoteError` for invalid filenames
so malformed index rows do not abort the read path, while preserving the
existing handling for deleted notes and valid results.
Source: Coding guidelines
| def test_refresh_is_incremental(omi: Path) -> None: | ||
| _note(omi, "Release Guide", "how to cut a release", ["release"]) | ||
| _note(omi, "Smoothie", "banana smoothie recipe", ["smoothie"]) | ||
| idx = searchindex.SearchIndex(omi) | ||
| first = idx.refresh() | ||
| assert first is not None and first.reindexed == 2 | ||
| again = idx.refresh() | ||
| assert again is not None and again.reindexed == 0 # nothing changed | ||
| _note(omi, "Smoothie", "banana and mango smoothie", ["smoothie"]) | ||
| third = idx.refresh() | ||
| assert third is not None and third.reindexed == 1 # only the edited note |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
New tests assume FTS5 is present. All three sites assert the indexed code path unconditionally, but searchindex.available() is False on a Python whose sqlite3 was built without FTS5 — the configuration this PR's fail-open design exists to support — so the suite fails there instead of skipping. One shared fix: gate the index-dependent assertions on availability.
tests/test_searchindex.py#L79-L89: add a module-levelpytestmark = pytest.mark.skipif(not searchindex._fts5_available(), ...), leaving the fail-open tests (Lines 261-291) working as they already assert the unavailable behavior.tests/test_bench.py#L34-L44: keep the capsule/token assertions unconditional and gate"index build (from scratch)", thesearch …measurement, and the"ms"unit onsearchindex.available().tests/test_bench.py#L53-L60: skip this test when the index is unavailable — the scan fallback makes both sides[].
As per coding guidelines, "Handle missing models, corrupt indexes, locked databases, and missing FTS5 builds without breaking search, and test failure branches."
📍 Affects 2 files
tests/test_searchindex.py#L79-L89(this comment)tests/test_bench.py#L34-L44tests/test_bench.py#L53-L60
🤖 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_searchindex.py` around lines 79 - 89, Gate the index-dependent
tests for missing FTS5 support: in tests/test_searchindex.py lines 79-89, add
module-level pytest skipif using searchindex._fts5_available(), while preserving
the fail-open tests at lines 261-291; in tests/test_bench.py lines 34-44, leave
capsule/token assertions unconditional and conditionally perform the
index-build, search measurement, and “ms” unit assertions using
searchindex.available(); in tests/test_bench.py lines 53-60, skip the test when
the index is unavailable because scan fallback makes both results empty.
Source: Coding guidelines
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add an FTS5 availability guard for this module.
Every test here asserts the indexed path (assert first is not None, search(...) == [...]), but searchindex.available() is False on a Python whose sqlite3 lacks FTS5 — exactly the environment the fail-open design exists for. The suite then fails on a supported configuration rather than skipping. The semantic fixture already models this with importorskip for numpy.
💚 Suggested guard
from omind import embed, searchindex
+#: The indexed assertions below require FTS5; without it every path fails open
+#: to scanning, which is covered by the fail-open tests in test_store.py.
+pytestmark = pytest.mark.skipif(
+ not searchindex._fts5_available(), reason="sqlite3 built without FTS5"
+)
+
#: A tiny fixed-vocabulary "embedding": a normalised bag-of-words over these terms.Note the fail-open tests at Lines 261-291 intentionally exercise the unavailable path and would need to stay outside the mark (or keep working, since they assert None/fallback either way).
🤖 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_searchindex.py` around lines 79 - 89, Add an availability guard
for the indexed-path tests in tests/test_searchindex.py, using
searchindex.available() to skip tests that require FTS5 when unavailable. Keep
the fail-open tests covering unavailable behavior outside this guard so they
continue to run on both supported and unsupported SQLite configurations.
Source: Coding guidelines
| def test_every_list_tool_is_bounded(server: FastMCP) -> None: | ||
| """No tool may return the whole vault in one result. | ||
|
|
||
| `list-notes` used to: ~348 KB / 87k tokens on a 744-note vault, in a single | ||
| tool payload. Every list-shaped tool now pages, and the caps are asserted | ||
| here so a new one cannot quietly go unbounded again. | ||
| """ | ||
| for number in range(40): | ||
| call(server, "create-note", {"title": f"Note {number:02d}", "summary": "body"}) | ||
| call(server, "create-note", {"title": "Linker", "summary": "see [[Note 00]] and [[Ghost]]"}) | ||
|
|
||
| listed = call(server, "list-notes", {}) | ||
| assert listed["count"] == 25 and listed["has_more"] is True # default page | ||
| assert listed["total"] == 41 | ||
| assert call(server, "list-notes", {"limit": 5})["count"] == 5 | ||
| assert call(server, "list-notes", {"limit": 9_999})["count"] == 41 # clamped to MAX_PAGE | ||
| second = call(server, "list-notes", {"limit": 25, "offset": 25}) | ||
| assert second["count"] == 16 and second["has_more"] is False | ||
| assert {n["filename"] for n in listed["result"]}.isdisjoint( | ||
| n["filename"] for n in second["result"] | ||
| ) | ||
|
|
||
| for tool, args in ( | ||
| ("backlinks", {"name": "Note 00.md", "limit": 1}), | ||
| ("list-tags", {"limit": 1}), | ||
| ("graph-orphans", {"limit": 1}), | ||
| ("graph-dangling", {"limit": 1}), | ||
| ("graph-neighbors", {"name": "Linker", "limit": 1}), | ||
| ): | ||
| page = call(server, tool, args) | ||
| assert set(page) >= {"result", "count", "offset", "total", "has_more"}, tool | ||
| assert page["count"] <= 1, tool |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cover graph-path and prove the hard cap.
graph-path still returns its full path list, but this “every list tool” test omits it. Also, 41 rows cannot verify clamping to MAX_PAGE = 100; limit=9_999 legitimately returns all 41. Convert graph-path to the shared paged response, add it here, and create more than 100 rows plus a >100-node path.
As per coding guidelines, “MCP tools must never return unbounded output. Every list-shaped tool must paginate using limit, offset, total, and has_more through server._page.”
🤖 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_server.py` around lines 167 - 198, Extend the bounded list-tool
coverage in test_every_list_tool_is_bounded to include graph-path and verify its
response uses result, count, offset, total, and has_more. Update graph-path to
return its path through the shared server._page pagination with limit clamped at
MAX_PAGE, then create more than 100 graph rows and a path exceeding 100 nodes so
the test proves the hard cap rather than merely returning all available entries.
Source: Coding guidelines
2 participants
No description provided.