fix(provision,guard,store): pin hooks to a canonical omind, fix title recall - #229
Conversation
… recall
Six fixes from an audit of a live install (docs/install-verification.md).
Hook commands baked shutil.which("omind") into settings.json as an absolute
path frozen at setup time. On a dev box that captured an editable checkout's
venv, so `omind self-update` upgraded ~/.local/bin while all five hooks kept
executing a months-old build -- silently, because the wiring still looked
correct. Pin to ~/.local/bin/omind instead: on a uv install that symlink is
retargeted on every upgrade, giving an absolute path (fires without
~/.local/bin on PATH) that is also stable (an update never strands it).
Applied to all 13 which("omind") sites so every harness gets the property,
not just Claude Code, plus a doctor check that fails on a non-canonical pin.
store.safe_name now falls back to the sanitized title. The guard names notes
by TITLE in block messages and 73/831 notes have '/' in their title, so
recall-note rejected them for path separators -- the guard demanded a retry
that could not be satisfied. The fallback requires the note to exist and
re-runs full validation, so traversal stays impossible and creates still fail.
Also: self-update re-provisions and warms the index (fail-open, opt out with
OMIND_NO_AUTOHEAL); guard pause capped at 4h after a box was found paused for
185h with enforcement silently off; a PAUSED banner in SessionStart priming;
and doctor reports the compliance deny RATE, warning above 25%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T3gMTVBvDX84KKe2h8HXvJ
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change standardizes ChangesInstallation Reliability and Runtime Guardrails
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UpdateCommand
participant Provisioning
participant SearchIndex
participant UpdateLogger
UpdateCommand->>Provisioning: re-run provisioning
UpdateCommand->>SearchIndex: refresh shared search index
Provisioning-->>UpdateLogger: log changes or warnings
SearchIndex-->>UpdateLogger: log refresh or migration results
UpdateCommand->>UpdateLogger: log restart instruction
Possibly related PRs
🚥 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from omind.paths import INDEX_FILENAME | ||
| from omind.proc import DEFAULT_TIMEOUT, run_command | ||
| from omind.provision import CheckResult, Logger, SetupConfig | ||
| from omind.provision import CheckResult, Logger, SetupConfig, canonical_omind_exe |
| raise MeshError(f"not a mesh node yet — run `omind mesh init` first ({omi_dir})") | ||
| omind_exe = shutil.which("omind") or "omind" | ||
| # Imported lazily: provision imports mesh, so a module-level import cycles. | ||
| from omind.provision import canonical_omind_exe |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/install-verification.md (1)
174-177: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required documentation footer.
The file ends without:
*Proudly Made in Nebraska. Go Big Red! 🌽 <https://xkcd.com/2347/>*Append this exact footer after the final paragraph. The centered banner exception applies only to
README.As per coding guidelines: Documentation files must include the required footer.
🤖 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/install-verification.md` around lines 174 - 177, Append the exact required “Proudly Made in Nebraska. Go Big Red!” footer, including the corn emoji and URL, after the final paragraph in docs/install-verification.md; do not center it, since the README-only exception does not apply.Source: Coding guidelines
src/omind/provision.py (1)
603-614: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftQuote the executable at every string-command boundary.
An absolute executable path can contain spaces. The raw path breaks the executable token in the hook, OpenClaw, Gemini, backup, mesh, and checkpoint command strings. Encode it for each target parser. Serialize the Windows
schtasks /TRcommand separately because it contains nested quoting.Keep executable paths raw in MCP argv arrays. Codex already quotes its executable. Add tests with an executable path that contains spaces.
🤖 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/provision.py` around lines 603 - 614, Quote the executable path at every string-command boundary so paths containing spaces remain a single command token. Update the hook command built by the relevant provision method, the OpenClaw/Gemini/backup/mesh/checkpoint command builders in src/omind/agents.py (lines 333-345, 738, and 808-814), src/omind/backup.py (lines 413-420), and src/omind/mesh.py (lines 891-894); serialize the Windows schtasks /TR command separately for its nested quoting. Keep executable paths raw in MCP argv arrays, preserve Codex’s existing quoting, and add tests using an executable path containing spaces.
🤖 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 `@docs/install-verification.md`:
- Line 156: Correct the I4 verification step in the documentation by replacing
the invalid git -C status reference with a valid command such as git -C
"$TESTVAULT/OMI" status --short, and ensure the before-and-after checks compare
the vault state without mutation.
- Line 166: Update the verification summary’s B-group total to account for all
13 defined B checks, changing the example from B: 11/11 to B: 13/13; do not
exclude B7a or B7b.
- Around line 19-23: Add blank lines immediately before and after the shell code
fence in the installation verification instructions, and label the reporting
example fence around the reporting section with the text language. Preserve the
existing command and reporting content unchanged.
- Around line 124-134: Add a retrieval acceptance row in section G that runs a
known-phrase search with OMI_INDEX_DISABLE=1 and verifies the expected note is
returned through the fallback path. If the test harness supports index-error
injection, add coverage for that failure case as well, confirming search still
falls back successfully and preserves the documented fail-open behavior.
- Around line 43-59: Update the verification matrix so B3 and B5 require MCP and
hook commands to resolve to the canonical ~/.local/bin/omind executable, not
merely the command name. Add equivalent wiring checks for every supported
harness exercised by D1: Hermes, OpenCode, Codex, Gemini, and OpenClaw. Reuse
the existing desired_server_entry() comparison behavior from agents.py to detect
stale virtual-environment binaries and non-Claude drift.
In `@src/omind/guard.py`:
- Around line 1578-1584: Update the pause boundary in pause_gate to enforce
_MAX_PAUSE_SECONDS for direct callers, applying the same capped duration before
persisting or starting the pause. If pause_gate is not intended as a supported
API, rename it to _pause_gate and ensure supported callers continue routing
through _run_pause.
In `@src/omind/provision.py`:
- Around line 316-323: Update the hook executable detection around shlex.split
and the token scan to use platform-correct parsing, preserve Windows
backslashes, and only return the executable when its path is absolute rather
than merely containing os.sep. Extend the regression coverage for relative
commands such as ./venv/bin/omind and Windows absolute paths such as
C:\Users\u\Scripts\omind.EXE.
In `@src/omind/store.py`:
- Around line 951-956: Update OmiStore.safe_name around _validated_name and
_name_from_title to reject absolute paths and any input containing a “..” path
segment before title fallback; preserve NoteError for these traversal-shaped
names. In tests/test_store.py lines 902-909, create matching sanitized files
under tmp_path and assert that ../outside and /etc/passwd still raise NoteError.
In `@src/omind/update.py`:
- Around line 226-235: Update the post-update provisioning flow around
Provisioner.run to load the persisted active SetupConfig instead of constructing
one with default_vault_path(). Use the configured vault, folder, and agent
values when re-provisioning; if no saved setup configuration exists, skip
provisioning and emit a warning rather than modifying the defaults.
In `@tests/test_guard.py`:
- Around line 1386-1397: Strengthen test_guard_pause duration assertions by
verifying the effective remaining time is approximately _MAX_PAUSE_SECONDS for
the capped 185h request and approximately 1,800 seconds for the 30m request,
using a small timing tolerance or deterministic clock injection. Keep the
existing cap-output and resume behavior checks intact.
- Around line 1383-1398: Ensure both pause tests always clean up the pause
sentinel by moving guard.resume_gate() into a finally block surrounding each
test’s assertions and output checks. Update test_guard_pause_is_capped and
test_guard_pause_under_the_cap_is_untouched without changing their existing
validation behavior.
---
Outside diff comments:
In `@docs/install-verification.md`:
- Around line 174-177: Append the exact required “Proudly Made in Nebraska. Go
Big Red!” footer, including the corn emoji and URL, after the final paragraph in
docs/install-verification.md; do not center it, since the README-only exception
does not apply.
In `@src/omind/provision.py`:
- Around line 603-614: Quote the executable path at every string-command
boundary so paths containing spaces remain a single command token. Update the
hook command built by the relevant provision method, the
OpenClaw/Gemini/backup/mesh/checkpoint command builders in src/omind/agents.py
(lines 333-345, 738, and 808-814), src/omind/backup.py (lines 413-420), and
src/omind/mesh.py (lines 891-894); serialize the Windows schtasks /TR command
separately for its nested quoting. Keep executable paths raw in MCP argv arrays,
preserve Codex’s existing quoting, and add tests using an executable path
containing spaces.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 085de806-9150-4646-a96b-1892e1095adc
📒 Files selected for processing (13)
docs/install-verification.mdsrc/omind/agents.pysrc/omind/backup.pysrc/omind/guard.pysrc/omind/hooks.pysrc/omind/mesh.pysrc/omind/provision.pysrc/omind/store.pysrc/omind/update.pytests/test_agents.pytests/test_guard.pytests/test_provision.pytests/test_store.py
📜 Review details
⏰ Context from checks skipped due to timeout. (10)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (ubuntu-latest, 3.11)
- GitHub Check: test (ubuntu-latest, 3.14)
- GitHub Check: test (macos-latest, 3.10)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (windows-latest, 3.10)
- GitHub Check: test (macos-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.13)
- 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/backup.pysrc/omind/hooks.pytests/test_agents.pysrc/omind/update.pysrc/omind/store.pysrc/omind/guard.pytests/test_guard.pytests/test_store.pytests/test_provision.pysrc/omind/provision.pysrc/omind/mesh.pysrc/omind/agents.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/backup.pysrc/omind/hooks.pytests/test_agents.pysrc/omind/update.pydocs/install-verification.mdsrc/omind/store.pysrc/omind/guard.pytests/test_guard.pytests/test_store.pytests/test_provision.pysrc/omind/provision.pysrc/omind/mesh.pysrc/omind/agents.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:
docs/install-verification.md
🪛 markdownlint-cli2 (0.23.2)
docs/install-verification.md
[warning] 20-20: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 22-22: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 164-164: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
src/omind/guard.py (2)
470-476: LGTM!
1562-1563: LGTM!src/omind/hooks.py (1)
377-396: LGTM!Also applies to: 575-576
tests/test_agents.py (1)
28-36: LGTM!
| 2. **Write tests go to a throwaway vault**, never the real OMI folder: | ||
| ```sh | ||
| export TESTVAULT="$(mktemp -d)/vault"; mkdir -p "$TESTVAULT/OMI" | ||
| ``` | ||
| Pass `--vault "$TESTVAULT" --folder OMI` on every write/index/mesh row. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the reported Markdown fence violations.
markdownlint-cli2 reports MD031 because the shell fence at Lines 20-22 lacks surrounding blank lines. It reports MD040 because the reporting example at Line 164 has no language. Add blank lines around the shell fence and mark the reporting fence as text.
Also applies to: 164-172
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 20-20: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 22-22: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 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/install-verification.md` around lines 19 - 23, Add blank lines
immediately before and after the shell code fence in the installation
verification instructions, and label the reporting example fence around the
reporting section with the text language. Preserve the existing command and
reporting content unchanged.
Source: Linters/SAST tools
| ## B. Wiring — what `omind setup` is supposed to have installed | ||
|
|
||
| | ID | Command | Pass criterion | | ||
| |----|---------|----------------| | ||
| | B1 | `omind doctor` | Exit 0, **zero problems**. Warnings triaged individually against this table | | ||
| | B2 | `omind doctor` line "MCP server 'omi'" | `[✓]`, not "differs from the expected `omind node` command". This is the single most common drift after a manual edit of `~/.claude.json` | | ||
| | B3 | `jq '.mcpServers.omi' ~/.claude.json` | Command array is `[<omind>, node, --vault, <vault>, --folder, OMI]` — an absolute interpreter path pointing into a venv that no longer exists is the classic post-reinstall break | | ||
| | B4 | `jq '.hooks \| keys' ~/.claude/settings.json` | Contains `PreToolUse`, `PostToolUse`, `Stop`, `SessionStart`, `UserPromptSubmit` | | ||
| | B5 | `jq -r '.hooks[][]\|.hooks[]?.command' ~/.claude/settings.json` | Contains `omind hook PostToolUse`, `omind hook Stop`, `omind hook SessionStart`, plus the four managed scripts: `omi-guard.sh`, `omi-gate-reset.sh`, `omi-enforce.py`, `secret-output-guard.sh` (and `git-fresh-base.sh` where repo rules apply) | | ||
| | B6 | `ls -l ~/.claude/hooks/` | All hook scripts present and executable (`0755`). Managed guard scripts are expected to be root-owned/write-protected — a user-writable `omi-guard.sh` defeats self-protection | | ||
| | B7 | `cat ~/.claude/hooks/.omind-provision.json` | Manifest present; its recorded SHAs match the shipped hook resources. Mismatch = **hookset drift** (someone hand-edited a managed script); repair with `omind setup` | | ||
| | B7a | `omind doctor` line "auto-memory hooks run a non-canonical omind" | Absent. Present = hooks are pinned to a different install than `~/.local/bin/omind`, so `self-update` will never reach them — the failure that let one box run 8.1.1 hooks under an 8.2.0 binary indefinitely | | ||
| | B7b | Confirm `omind setup` can actually write `~/.claude/settings.json` | Writable by the user. If it is root-owned **and** immutable (`chattr +i`), setup dies with `PermissionError` and every "run `omind setup`" repair instruction in this matrix is impossible until an operator clears it — verify before trusting any B/C row's remediation | | ||
| | B8 | `test -f ~/.claude/skills/omind/SKILL.md` | Present — this is how agents discover authoritative command syntax | | ||
| | B9 | `omind doctor` line "seed files present" | `[✓]` — `index.md` + note template exist in the OMI folder | | ||
| | B10 | `test -f "$OMI/.obsidian/app.json"` | Obsidian config seeded | | ||
| | B11 | `git -C "$OMI" config --get-regexp 'merge\.(omi\|ours)'` + `cat "$OMI/.gitattributes"` | Both merge drivers configured and `.gitattributes` routes `*.md` to the `omi` driver — without this, mesh sync silently produces conflict markers in notes | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Verify the canonical executable for every supported harness.
The B3 and B5 criteria accept <omind> and omind hook ... without checking the exact ~/.local/bin/omind path. They inspect only Claude configuration, while D1 exercises Hermes, OpenCode, Codex, Gemini, and OpenClaw without checking their installed wiring. A stale virtual-environment binary or non-Claude drift can pass this matrix and remain unreachable by self-update. Compare each MCP and hook command with the canonical executable, and add equivalent checks for every supported harness. src/omind/agents.py:1602-1655 already compares registration with desired_server_entry().
Also applies to: 80-84
🤖 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/install-verification.md` around lines 43 - 59, Update the verification
matrix so B3 and B5 require MCP and hook commands to resolve to the canonical
~/.local/bin/omind executable, not merely the command name. Add equivalent
wiring checks for every supported harness exercised by D1: Hermes, OpenCode,
Codex, Gemini, and OpenClaw. Reuse the existing desired_server_entry()
comparison behavior from agents.py to detect stale virtual-environment binaries
and non-Claude drift.
| ## G. Retrieval | ||
|
|
||
| | ID | Command | Pass criterion | | ||
| |----|---------|----------------| | ||
| | G1 | `omind doctor` line "search index: FTS5 available" | `[✓]`. FTS5 missing = the keyword path is degraded to a scan | | ||
| | G2 | `omind doctor` line "semantic search" | `[✓]` if `omind[embed]` was intended. "off (keyword path) — model2vec not importable" is a legitimate SKIP only if the install deliberately omitted the extra; it costs ~20pp recall@1 | | ||
| | G3 | `omind doctor` line "search index: … stale note(s)" | Zero stale notes and index age consistent with the write timer. Non-zero stale = run `omind reindex --rebuild` and re-check | | ||
| | G4 | `omind search '<known phrase>'` **RO** | Returns the expected note in the top hits | | ||
| | G5 | `omind reindex --vault "$TESTVAULT" --folder OMI` | Exits 0; index rebuilt under the write lock | | ||
| | G6 | `omind bench --vault "$TESTVAULT" --folder OMI` | Reports index-build, search latency, capsule build, recall token cost. Record as the **install's baseline** — this row's value is the number, compared over time | | ||
| | G7 | `omind graph stats` / `graph orphans` / `graph dangling` **RO** | All exit 0 and return structured output | |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a fallback-search acceptance row.
The G section tests indexed success, stale notes, and reindex success, but it does not run OMI_INDEX_DISABLE=1 or force an index error. A broken index can pass this matrix even though fail-open behavior is untested. Add a known-phrase search with the index disabled and verify that the fallback returns the expected note. Also cover an injected index failure if the test harness supports it.
As per coding guidelines: Retrieval must fail open: every search layer returns None on errors and falls back to the older search path; test failure branches as well as successful searches.
🤖 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/install-verification.md` around lines 124 - 134, Add a retrieval
acceptance row in section G that runs a known-phrase search with
OMI_INDEX_DISABLE=1 and verifies the expected note is returned through the
fallback path. If the test harness supports index-error injection, add coverage
for that failure case as well, confirming search still falls back successfully
and preserves the documented fail-open behavior.
Source: Coding guidelines
| | I1 | `omind checkpoint --vault "$TESTVAULT" --folder OMI` | Produces/updates a daily worklog note | | ||
| | I2 | `systemctl --user list-timers \| grep -i omind` (or the platform equivalent) | The checkpoint timer is installed and scheduled if `install-timer` was intended | | ||
| | I3 | `omind rollup --vault "$TESTVAULT" --folder OMI` | Compacts dailies; default archives rather than deletes | | ||
| | I4 | `omind consolidate --vault "$TESTVAULT" --folder OMI` | Proposes merges **without mutating** the vault (verify with `git -C status`/checksums before and after) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the git -C verification command.
git -C status treats status as the repository path. It does not run git status. The I4 check therefore cannot verify that consolidation did not mutate the vault. Use git -C "$TESTVAULT/OMI" status --short before and after, or provide a valid checksum comparison.
🤖 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/install-verification.md` at line 156, Correct the I4 verification step
in the documentation by replacing the invalid git -C status reference with a
valid command such as git -C "$TESTVAULT/OMI" status --short, and ensure the
before-and-after checks compare the vault state without mutation.
|
|
||
| ``` | ||
| omind install verification — <host> — <version> — <date> | ||
| A: 5/5 B: 11/11 C: 6/7 (C7 FAIL) D: 11/13 (D0,D1 FAIL) E: 6/6 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the B-group denominator.
The table defines 13 B rows: B1-B11, B7a, and B7b. The example reports B: 11/11, so it omits two checks. Change it to B: 13/13, or explicitly exclude B7a and B7b from group totals.
🤖 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/install-verification.md` at line 166, Update the verification summary’s
B-group total to account for all 13 defined B checks, changing the example from
B: 11/11 to B: 13/13; do not exclude B7a or B7b.
| try: | ||
| tokens = shlex.split(command_text) | ||
| except ValueError: # unbalanced quotes in a hand-edited settings.json | ||
| return None | ||
| for token, following in zip(tokens, tokens[1:], strict=False): | ||
| if following == "hook" and os.sep in token: | ||
| return token | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make hook executable detection platform-aware and require an absolute path.
Line 321 treats any path containing os.sep as absolute. A relative command such as ./venv/bin/omind hook Stop is reported as a stale absolute pin.
shlex.split() also consumes Windows backslashes. A command such as C:\Users\u\Scripts\omind.EXE hook Stop is not detected. doctor can then report healthy hooks after an update leaves a Windows hook pinned to an old executable.
Use platform-correct command parsing and an absolute-path check. Add regression cases for relative paths and Windows paths.
🤖 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/provision.py` around lines 316 - 323, Update the hook executable
detection around shlex.split and the token scan to use platform-correct parsing,
preserve Windows backslashes, and only return the executable when its path is
absolute rather than merely containing os.sep. Extend the regression coverage
for relative commands such as ./venv/bin/omind and Windows absolute paths such
as C:\Users\u\Scripts\omind.EXE.
| try: | ||
| return self._validated_name(name) | ||
| except NoteError: | ||
| fallback = self._name_from_title(name) | ||
| if fallback is not None: | ||
| return fallback |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject traversal-shaped names before title fallback.
Lines 951-956 sanitize every rejected name. If outside.md exists in the vault, safe_name("../outside") resolves it. If etc passwd.md exists, safe_name("/etc/passwd") resolves it. The path does not escape the vault, but invalid traversal-shaped input becomes an alias that write paths can update, disable, or purge.
src/omind/store.py#L951-L956: Reject absolute paths and names with a..path segment before calling_name_from_title.tests/test_store.py#L902-L909: Create matching sanitized files insidetmp_pathand assert that../outsideand/etc/passwdstill raiseNoteError.
Proposed fix
def _name_from_title(self, name: str) -> Path | None:
"""The existing note whose filename this *title* sanitizes to, if any."""
+ raw = (name or "").strip()
+ if raw.startswith(("/", "\\")) or ".." in re.split(r"[\\/]+", raw):
+ return None
try:
candidate = self._validated_name(self.filename_for_title(name))As per coding guidelines, OmiStore.safe_name must keep path traversal impossible.
📍 Affects 2 files
src/omind/store.py#L951-L956(this comment)tests/test_store.py#L902-L909
🤖 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 951 - 956, Update OmiStore.safe_name around
_validated_name and _name_from_title to reject absolute paths and any input
containing a “..” path segment before title fallback; preserve NoteError for
these traversal-shaped names. In tests/test_store.py lines 902-909, create
matching sanitized files under tmp_path and assert that ../outside and
/etc/passwd still raise NoteError.
Source: Coding guidelines
| from omind.provision import Provisioner, SetupConfig, default_vault_path | ||
|
|
||
| vault = default_vault_path() | ||
| # Hook scripts, the MCP entry, and the skill are all rewritten by the new | ||
| # binary — otherwise a release that changes any of them lands only on boxes | ||
| # where someone remembered to re-run `omind setup` by hand. | ||
| try: | ||
| actions = Provisioner(config=SetupConfig(vault=vault), log=lambda _m: None).run() | ||
| if actions: | ||
| log(f"re-provisioned wiring ({len(actions)} change(s)).") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use the installed setup configuration for post-update healing.
This always creates SetupConfig(vault=default_vault_path()). It therefore resets folder to OMI and agent to claude.
If the user configured another vault, folder, or agent harness, a successful update can create or modify the default Claude installation instead of healing the active installation. Resolve the persisted active setup configuration before provisioning. If no configuration is available, skip provisioning and report a warning.
🤖 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/update.py` around lines 226 - 235, Update the post-update
provisioning flow around Provisioner.run to load the persisted active
SetupConfig instead of constructing one with default_vault_path(). Use the
configured vault, folder, and agent values when re-provisioning; if no saved
setup configuration exists, skip provisioning and emit a warning rather than
modifying the defaults.
| def test_guard_pause_is_capped(capsys: pytest.CaptureFixture[str]) -> None: | ||
| """A week-long pause is a disable with extra steps: it silently masks the | ||
| enforcement check for the duration. One box was found paused for 185h.""" | ||
| assert guard.run_guard("pause", duration="185h") == 0 | ||
| out = capsys.readouterr().out | ||
| assert "cap" in out | ||
| remaining = guard.pause_remaining() | ||
| assert 0 < remaining <= guard._MAX_PAUSE_SECONDS | ||
| guard.resume_gate() | ||
|
|
||
|
|
||
| def test_guard_pause_under_the_cap_is_untouched(capsys: pytest.CaptureFixture[str]) -> None: | ||
| assert guard.run_guard("pause", duration="30m") == 0 | ||
| assert 0 < guard.pause_remaining() <= 1800 | ||
| assert "cap" not in capsys.readouterr().out | ||
| guard.resume_gate() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guarantee pause cleanup after assertion failures.
guard.resume_gate() runs only after the assertions at Line [1388] and Line [1397]. If an assertion fails, the pause sentinel remains active and can affect later tests. Put each cleanup call in a finally block, or use an autouse fixture that re-arms the gate.
🤖 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_guard.py` around lines 1383 - 1398, Ensure both pause tests always
clean up the pause sentinel by moving guard.resume_gate() into a finally block
surrounding each test’s assertions and output checks. Update
test_guard_pause_is_capped and test_guard_pause_under_the_cap_is_untouched
without changing their existing validation behavior.
| assert guard.run_guard("pause", duration="185h") == 0 | ||
| out = capsys.readouterr().out | ||
| assert "cap" in out | ||
| remaining = guard.pause_remaining() | ||
| assert 0 < remaining <= guard._MAX_PAUSE_SECONDS | ||
| guard.resume_gate() | ||
|
|
||
|
|
||
| def test_guard_pause_under_the_cap_is_untouched(capsys: pytest.CaptureFixture[str]) -> None: | ||
| assert guard.run_guard("pause", duration="30m") == 0 | ||
| assert 0 < guard.pause_remaining() <= 1800 | ||
| assert "cap" not in capsys.readouterr().out |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert the effective duration, not only the upper bound.
The capped test passes for any positive duration at Line [1389]. The under-cap test passes for any positive duration up to 1,800 seconds at Line [1396]. Compare the remaining duration with _MAX_PAUSE_SECONDS and 1,800 using a small tolerance, or inject a deterministic clock. Otherwise, an implementation that under-pauses every request can pass both tests.
🤖 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_guard.py` around lines 1386 - 1397, Strengthen test_guard_pause
duration assertions by verifying the effective remaining time is approximately
_MAX_PAUSE_SECONDS for the capped 185h request and approximately 1,800 seconds
for the 30m request, using a small timing tolerance or deterministic clock
injection. Keep the existing cap-output and resume behavior checks intact.
`_hook_exe_path` tested `os.sep in token`, so a POSIX-style pin read on Windows (or the reverse) was missed -- precisely the stale-install case the check exists to catch. settings.json is portable data; both separators count. Matched with a regex rather than shlex for the same reason: POSIX-mode shlex treats the backslashes in a Windows path as escapes and silently flattens the path away, which is how the Windows CI job caught this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T3gMTVBvDX84KKe2h8HXvJ
Patch release rolling up #229: hooks pinned to a canonical omind so an update actually reaches them, recall-note accepting titles that contain '/', a re-provision + index warm on self-update, a 4h cap on guard pause, and a deny-rate health metric in doctor. Adds docs/install-verification.md. Version bumped in lockstep: pyproject.toml, src/omind/__init__.py, uv.lock. Claude-Session: https://claude.ai/code/session_01T3gMTVBvDX84KKe2h8HXvJ Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2 participants
Six fixes from an audit of a live install. The new
docs/install-verification.mdis the matrix that found them: ~70 agent-executable rows checking that a new install's features, controls, and hooks are actually present and behaving — not merely wired.The root cause
Hook commands baked
shutil.which("omind")intosettings.jsonas an absolute path frozen at setup time. On a dev box that captured an editable checkout's venv, soomind self-updateupgraded~/.local/binwhile all five hooks kept executing a months-old build — silently, because the wiring still looked correct.Fixed by pinning to
~/.local/bin/omind. On auv tool installbox that is a symlink uv retargets on every upgrade, which gives both properties the old code was chasing separately: absolute (fires in a shell without~/.local/binon PATH) and stable (an update never strands it). Resolution now happens once at install time instead of being frozen into five hook entries. Applied to all 13which("omind")sites, so hermes/opencode/codex/gemini/openclaw get it too — not just Claude Code — plus adoctorcheck that fails on a non-canonical pin.The other five
store.safe_nametitle fallback. The guard names notes by TITLE in block messages, and 73/831 notes on the audited vault have/in their title, sorecall-noterejected them for path separators. The guard would block an action, instruct a recall of "NEVER offer to end/pause the session …", and then refuse it — a remediation loop impossible to satisfy. The fallback requires the note to exist and re-runs full validation, so traversal stays impossible and creates still fail loudly.self-updatere-provisions and warms the index. Opening the index runs the existingSCHEMA_VERSION/model check, so a format migration is paid during the update the user is already waiting on instead of ambushing the next search. Not an unconditional rebuild — the schema check already decides. Both steps fail-open, opt out with the existingOMIND_NO_AUTOHEAL.guard pausecapped at 4h. A box was found paused for 185h with the consult-gate and verifier off; the only visible trace wasguard status, which nobody runs unprompted.doctorreports the deny RATE, warning above 25%. The audited install sat at 51% — one action in two — which the raw counts hid. Precision, not volume, decides whether the guard is helping.Testing
928 tests pass; ruff clean. New regression tests cover the title fallback (including four traversal attempts and the create path), the canonical resolver and its fallback,
_hook_exe_pathparsing, and both pause-cap branches.Both new doctor checks were verified firing against the real install that motivated them.
Proudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/