fix(doctor): stop printing the embed install hint twice (v8.1.1) - #217
Conversation
`embed.status()` already puts "pip install 'omind[embed]'" in its reason for the common cause (model2vec missing), and 8.1.0's new warning appended the same command unconditionally, so the line read: semantic search: off (keyword path) — model2vec not importable (ModuleNotFoundError); pip install 'omind[embed]'; install the extra for ~20pp better recall: pip install 'omind[embed]' Only visible by running the command on a machine where the extra is genuinely missing — this machine has it installed, so local `omind doctor` never showed the warning at all. Found on the Win11 VM. Co-Authored-By: Claude Opus 5 <[email protected]>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe semantic-search doctor warning now avoids duplicate ChangesSemantic-search warning correction
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-11: Add a blank line immediately after the `### Fixed` heading
in the changelog, before its list item, so the Markdown structure satisfies
MD022.
In `@src/omind/cli.py`:
- Around line 784-791: Update the warning message constructed in the semantic
search check to place the cost/recall impact immediately after the
semantic-search status, followed by the existing reason and optional
installation hint. Preserve the current values and formatting for reason, hint,
and cost while changing only their order in the CheckResult message.
In `@tests/test_cli.py`:
- Around line 303-307: Update the test around the mocked reason for
search_semantic so it includes “omind[embed]”, exercising the de-duplication
path rather than only the fallback append path. Keep the existing assertion that
message.count("omind[embed]") equals one, and preserve coverage for the append
behavior if it is not already tested separately.
🪄 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: d8abfc06-3b0e-4780-b58b-5e02cdfce625
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.mdpyproject.tomlsrc/omind/__init__.pysrc/omind/cli.pytests/test_cli.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: test (ubuntu-latest, 3.11)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (ubuntu-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Keep Markdown vault files as the source of truth; store all derived indexes, caches, and vectors underpaths.state_dir(), never in the vault.
Any operation writing multiple notes must journal pre-images throughtxn.Transactionwhile holdingstore.write_lock(); recovery must not overwrite notes edited after the crash.
Route all note writes throughOmiStore; external writers should usenotes.upsert_note. Preserve flocking, atomic rename, LamportRev:stamping, and soft-delete behavior. Deletes archive notes withDisabled: true; onlyomind mesh purgepermanently removes them.
UseOmiStore.safe_namefor every note read and write so path traversal remains impossible.
Keepstore.pyframework-free; it must not depend on FastAPI or MCP because both the CLI and web app build on it.
De-prioritize credential notes in search and gate suggestions usingretrieve._CREDENTIAL_PENALTY, unless the query is about credentials; never steer agents into secrets notes.
MCP tools must not return unbounded output. Every list-shaped tool must paginate withlimit,offset,total, andhas_moreviaserver._page.
Treatindex.mdandMemory Template.mdas scaffolding rather than memories; reading them must not clear the consult gate, as represented bypaths.NON_CONSULT_FILENAMES.
Index retrieval must preserve the fail-open fallback, including when disabled withOMI_INDEX_DISABLE=1; verify both indexed and fallback search paths.
Recency may only re-rank notes matched by content legs; it must never add unmatched notes to search results.
Do not strip code fences from[[wikilinks]]in the search index;lint.pyintentionally remains the independent full-vault scanner.
link_targets()must preserve the author’s link casing for dangling-link reports; only link resolution should lowercase names.
Never mutate aNoteSummaryreturned from_cached_summary; usedataclasses.replace, as instore._indexed_search.
Coerce embedding results through `searc...
Files:
src/omind/__init__.pysrc/omind/cli.pytests/test_cli.py
**/*.{py,md}
📄 CodeRabbit inference engine (AGENTS.md)
Retrieval must fail open: every search layer returns
Noneon errors and falls back to the older search path; test failure branches as well as successful searches.
Files:
src/omind/__init__.pyCHANGELOG.mdsrc/omind/cli.pytests/test_cli.py
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Documentation files must include the footer
*Proudly Made in Nebraska. Go Big Red! 🌽 <https://xkcd.com/2347/>*; the README uses the centered banner variant.
Files:
CHANGELOG.md
🪛 markdownlint-cli2 (0.23.1)
CHANGELOG.md
[warning] 10-10: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (2)
pyproject.toml (1)
3-3: LGTM!src/omind/__init__.py (1)
5-5: LGTM!
| ### Fixed | ||
| - `omind doctor` printed the `pip install 'omind[embed]'` hint **twice** in the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line after the ### Fixed heading.
markdownlint-cli2 reports MD022 because the heading is not followed by a blank line.
Proposed fix
### Fixed
+
- `omind doctor` printed the `pip install 'omind[embed]'` hint **twice** in the📝 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.
| ### Fixed | |
| - `omind doctor` printed the `pip install 'omind[embed]'` hint **twice** in the | |
| ### Fixed | |
| - `omind doctor` printed the `pip install 'omind[embed]'` hint **twice** in the |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[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` around lines 10 - 11, Add a blank line immediately after the
`### Fixed` heading in the changelog, before its list item, so the Markdown
structure satisfies MD022.
Source: Linters/SAST tools
| reason = str(semantic["reason"]) | ||
| cost = "worth ~20pp of recall@1 on a real vault" | ||
| hint = "" if "omind[embed]" in reason else "; pip install 'omind[embed]'" | ||
| results.append( | ||
| CheckResult( | ||
| "search_semantic", | ||
| "warn", | ||
| f"semantic search: off (keyword path) — {semantic['reason']}; " | ||
| "install the extra for ~20pp better recall: pip install 'omind[embed]'", | ||
| f"semantic search: off (keyword path) — {reason}{hint} ({cost})", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Lead with the recall impact required by the PR objective.
The warning places {reason} and the installation hint before {cost}. The estimated recall improvement therefore appears last. Move {cost} immediately after the semantic-search status, then append the reason and optional installation hint.
🤖 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/cli.py` around lines 784 - 791, Update the warning message
constructed in the semantic search check to place the cost/recall impact
immediately after the semantic-search status, followed by the existing reason
and optional installation hint. Preserve the current values and formatting for
reason, hint, and cost while changing only their order in the CheckResult
message.
| message = checks["search_semantic"].message | ||
| assert "omind[embed]" in message | ||
| # The reason already carries the install command; saying it twice is what a | ||
| # real Windows install actually printed. | ||
| assert message.count("omind[embed]") == 1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the de-duplication branch.
The mocked reason at Line 298 does not contain omind[embed], so this test covers only the fallback append branch. An implementation that always appends the hint would still pass. Include the installation command in the mocked reason or add a second test case where the reason already contains it, then assert that the count remains one.
🤖 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_cli.py` around lines 303 - 307, Update the test around the mocked
reason for search_semantic so it includes “omind[embed]”, exercising the
de-duplication path rather than only the fallback append path. Keep the existing
assertion that message.count("omind[embed]") equals one, and preserve coverage
for the append behavior if it is not already tested separately.
1 participant
Found while validating v8.1.0 on the Win11 QEMU VM on pluto — a real install where the
[embed]extra is genuinely absent.What it printed
embed.status()already carries the install command in itsreasonfor the common cause. The warning I added in 8.1.0 appended it again unconditionally.Why local testing couldn't catch it
This dev machine has
model2vecinstalled — installing it was one of today's findings — soomind doctorhere takes theavailablebranch and never renders the warning at all. The unit test asserted"omind[embed]" in message, which passes just as happily when it appears twice.Test now asserts
message.count("omind[embed]") == 1.The message also now leads with the cost rather than repeating the remedy:
Gates
ruff check .·mypy src(strict) ·pytest(891 passed) ·pip-audit— green locally.🤖 Generated with Claude Code