feat(store): journaled multi-note transactions and omind recover (v8.0.0) - #213
Conversation
Multi-file updates cannot be truly atomic on the filesystems omind runs on. This does not pretend otherwise; it provides the contract that can actually be kept: every target's pre-image is captured and fsynced BEFORE the first write, the writes go through the store's existing atomic per-file replace, a commit record marks the point of no return, and an interrupted run is rolled back deterministically. omind had the first half already — same-dir temp + os.replace, an advisory write lock, version preconditions — and no journal, so an interrupted multi-note operation left partial state with no way back. `store.create_and_disable_sources` conceded exactly this in its own docstring: "a process crash can still leave extra recoverable copies". That fails toward keeping data, which is the right direction, but a human still had to notice and reconcile by hand. The rule that makes recovery safe: a pre-image is restored only when the file still holds either that pre-image (nothing to do) or exactly the bytes the interrupted run meant to write (ours to undo). Anything else means someone edited the note after the crash, and their edit is newer information than our pre-image — so it is reported as a conflict, left alone, and its journal kept for inspection. `omind recover` exits 1 in that case. Blind rollback would be data loss wearing recovery's clothes. In-process failures roll themselves back, so consolidate needs no manual recover; the journal is for when the process dies outright. Journals live in the state dir, never the vault: they describe this machine's interrupted filesystem work, are meaningless to a mesh peer, and must not replicate (invariant 1). AGENTS.md gains invariant 3 so the next multi-note operation does not quietly reintroduce the gap. Skipped their approved_plan_sha256 handshake as the issue planned: right for an LLM-driven research vault, wrong in front of routine memory writes. Closes #194. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 44 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 (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded durable journaled transactions for multi-note writes. The implementation captures pre-images, applies atomic file replacements, supports rollback and conflict-safe recovery, migrates source consolidation, and adds the ChangesJournaled transactions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant CLI
participant WriteLock
participant Recovery
participant FileSystem
participant Index
Operator->>CLI: Run omind recover
CLI->>WriteLock: Acquire write lock
CLI->>Recovery: Recover pending journals
Recovery->>FileSystem: Restore pre-images or remove new files
FileSystem-->>Recovery: Report restored, removed, skipped, or conflicting files
Recovery-->>CLI: Return recovery reports
CLI->>Index: Update index after recovery
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 10
🤖 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`:
- Line 10: Add one blank line immediately after the “### Added” and “###
Changed” headings in CHANGELOG.md to satisfy markdownlint MD022, without
altering the release-note content.
In `@src/omind/cli.py`:
- Around line 1254-1255: Update the recover command flow around txn.recover to
catch TransactionError, convert it into the same user-facing error message and
nonzero return behavior used by _run_import, and prevent an uncaught traceback
while preserving the existing write lock and recovery behavior for successful
transactions.
- Around line 1269-1271: Update _run_recover after the non-dry-run recovery
update to call store._signal_write() in addition to store.update_index(),
ensuring search-index invalidation and mesh synchronization receive the restored
note content.
In `@src/omind/txn.py`:
- Around line 251-255: Preserve conflicted rollback evidence: in
Transaction.rollback, discard the journal and pre-images only when the
RecoveryReport is clean; update src/omind/txn.py lines 251-255 accordingly. In
src/omind/store.py lines 1321-1328, inspect the returned report, log conflicting
note names with guidance to run omind recover, and suppress rollback failures so
they do not replace the exception that caused apply to abort.
- Around line 90-92: Update the documentation comment for the content field in
Transaction.write to describe only its actual str payload semantics. Remove the
claim that None requests deletion, and clarify that file removal decisions
during recovery use the existed field instead.
- Around line 271-277: Update pending() to retain directories whose journal
cannot be read or parsed instead of silently continuing, so _run_recover can
report those directories as recovery candidates and never claim there are no
interrupted transactions while unreadable journal data remains.
- Around line 202-203: Update Transaction.prepare() and the corresponding
transaction writers so newline handling is consistent: either open writer text
streams with newline translation disabled, or normalize content identically
before computing new_sha and comparing it during _rollback_journal(). Ensure
interrupted transactions produce matching digests and restore entries on
Windows.
- Around line 337-349: Update the dry-run branch in recover to inspect each
journal entry’s note and classify it using the same rules as _rollback_journal,
while avoiding any filesystem writes. Populate RecoveryReport restored, removed,
skipped, and conflict information consistently with the predicted rollback, and
update _run_recover output to print conflicts when present.
In `@tests/test_txn.py`:
- Around line 94-127: Add a test alongside the recovery tests named
test_a_failing_restore_is_reported_as_a_conflict that prepares and applies a
transaction, then calls txn.recover with a writer raising OSError. Assert the
report marks A.md as a conflict, the applied content remains unchanged, and
txn.pending(omi) confirms the journal is retained.
- Around line 21-25: Update the omi fixture in tests/test_txn.py to set
XDG_STATE_HOME to a directory under tmp_path before creating or using the test
OMI directory, following the isolation pattern used by the vault fixture in
tests/test_consolidate.py. Preserve the fixture’s returned OMI path while
ensuring txn.Transaction state journals and pre-images remain within the
temporary test directory.
🪄 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: 4b617469-b2d2-45c1-ae5c-388f792e1155
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
AGENTS.mdBACKLOG.mdCHANGELOG.mdREADME.mdpyproject.tomlsrc/omind/__init__.pysrc/omind/cli.pysrc/omind/paths.pysrc/omind/server.pysrc/omind/store.pysrc/omind/txn.pytests/test_txn.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: test (ubuntu-latest, 3.10)
- GitHub Check: test (ubuntu-latest, 3.13)
- GitHub Check: test (windows-latest, 3.14)
- GitHub Check: test (macos-latest, 3.14)
- GitHub Check: test (ubuntu-latest, 3.12)
- GitHub Check: test (windows-latest, 3.10)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Retrieval must fail open: every search layer returnsNoneon errors, allowing fallback to an older path; test failure branches as well as successful searches.
Operations writing multiple notes must journal pre-images throughtxn.Transactionwhile holdingstore.write_lock(); recovery must not overwrite notes edited after the crash.
All note writes must go throughOmiStore; external writers should usenotes.upsert_note, preserving flocking, atomic rename, LamportRev:stamping, and soft deletion. Deletes archive notes withDisabled: true; onlyomind mesh purgepermanently removes them.
UseOmiStore.safe_namefor every note read and write so path traversal is impossible.
De-prioritize credential notes in search and gate suggestions usingretrieve._CREDENTIAL_PENALTY, unless the query concerns credentials; never steer agents into secrets notes.
MCP tools must never return unbounded output. Every list-shaped tool must paginate throughserver._pageand exposelimit,offset,total, andhas_more.
Treatindex.mdandMemory Template.mdas scaffolding rather than memories; reading them must not clear the consult gate, as defined bypaths.NON_CONSULT_FILENAMES.
search()must refresh the search index on every call so results remain correct after writes from another process.
Recency may only re-rank notes already matched by content legs; it must never add otherwise-unmatched notes.
Do not strip code fences from wikilinks in the search index;lint.pyremains responsible for that behavior.
link_targets()must preserve wikilink case for reporting, while only resolution lowercases links.
Never mutate aNoteSummaryreturned from_cached_summary; usedataclasses.replace, as instore._indexed_search.
Pass embedding results throughsearchindex._query_vector; do not assumeembed.encodereturns an object with.shape, since test backends may return a plain list.
Do not reintroduce document-frequency term filtering; use grade...
Files:
src/omind/server.pysrc/omind/__init__.pysrc/omind/paths.pysrc/omind/cli.pysrc/omind/store.pytests/test_txn.pysrc/omind/txn.py
**/*.{py,md}
📄 CodeRabbit inference engine (AGENTS.md)
Retrieval changes must preserve both indexed and fallback behavior; verify normal search and
OMI_INDEX_DISABLE=1fallback paths.
Files:
src/omind/server.pysrc/omind/__init__.pyREADME.mdBACKLOG.mdsrc/omind/paths.pysrc/omind/cli.pyCHANGELOG.mdAGENTS.mdsrc/omind/store.pytests/test_txn.pysrc/omind/txn.py
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Treat Markdown vault files as the source of truth; keep indexes, caches, vectors, and other derived data inpaths.state_dir(), never in the vault.
Documentation files must include the exact Proudly Made in Nebraska footer; the README uses the centered banner variant.
Files:
README.mdBACKLOG.mdCHANGELOG.mdAGENTS.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: CryptoJones/omind
Timestamp: 2026-08-02T19:54:07.233Z
Learning: Commit and push only when explicitly asked; branch from `main` using `feat/`, `fix/`, `docs/`, `chore/`, or `refactor/` prefixes and use conventional-commit subjects.
Learnt from: CR
Repo: CryptoJones/omind
Timestamp: 2026-08-02T19:54:07.233Z
Learning: Report failed gates and skipped scope honestly, including the relevant command output or explanation.
🪛 ast-grep (0.45.0)
src/omind/txn.py
[info] 183-183: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload, indent=2)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🪛 LanguageTool
BACKLOG.md
[grammar] ~70-~70: Ensure spelling is correct
Context: ...rability)_ — shipped as omind.txn + omind recover. Pre-images captured and fsynced before ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 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)
[warning] 49-49: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (15)
README.md (1)
56-61: LGTM!pyproject.toml (1)
3-3: LGTM!src/omind/__init__.py (1)
5-5: LGTM!src/omind/server.py (1)
474-474: LGTM!src/omind/txn.py (3)
58-78: LGTM!
109-134: LGTM!
149-187: LGTM!src/omind/paths.py (1)
114-122: LGTM!src/omind/cli.py (1)
69-69: LGTM!Also applies to: 480-500, 1591-1592
tests/test_txn.py (1)
32-91: LGTM!Also applies to: 130-165, 205-259
src/omind/store.py (2)
30-30: LGTM!Also applies to: 1276-1282
1313-1320: LGTM!AGENTS.md (2)
33-39: LGTM!
40-55: 🩺 Stability & AvailabilityNo numbered invariant references to update.
The repository contains no non-AGENTS.md references that cite invariants by number, so there is no renumbering correction to apply.
BACKLOG.md (1)
69-76: LGTM!
|
|
||
| ## [8.0.0] - 2026-08-02 | ||
|
|
||
| ### Added |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines after the release-note headings.
Add one blank line after ### Added at Line 10 and after ### Changed at Line 49. This resolves the reported markdownlint MD022 warnings.
Also applies to: 49-49
🧰 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` at line 10, Add one blank line immediately after the “###
Added” and “### Changed” headings in CHANGELOG.md to satisfy markdownlint MD022,
without altering the release-note content.
Source: Linters/SAST tools
| with store.write_lock(): | ||
| reports = txn.recover(omi_dir, _atomic_write, dry_run=args.dry_run) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle TransactionError from txn.recover.
_rollback_journal raises TransactionError when a journal cannot be read or parsed. txn.recover does not catch it, so omind recover exits with an uncaught traceback. Every other runner in this file converts its module error into a message and a nonzero code; _run_import (Line 1036) is the pattern.
🐛 Proposed fix
with store.write_lock():
- reports = txn.recover(omi_dir, _atomic_write, dry_run=args.dry_run)
+ try:
+ reports = txn.recover(omi_dir, _atomic_write, dry_run=args.dry_run)
+ except txn.TransactionError as exc:
+ print(f"error: {exc}", file=sys.stderr)
+ return 1📝 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.
| with store.write_lock(): | |
| reports = txn.recover(omi_dir, _atomic_write, dry_run=args.dry_run) | |
| with store.write_lock(): | |
| try: | |
| reports = txn.recover(omi_dir, _atomic_write, dry_run=args.dry_run) | |
| except txn.TransactionError as exc: | |
| print(f"error: {exc}", file=sys.stderr) | |
| return 1 |
🤖 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 1254 - 1255, Update the recover command flow
around txn.recover to catch TransactionError, convert it into the same
user-facing error message and nonzero return behavior used by _run_import, and
prevent an uncaught traceback while preserving the existing write lock and
recovery behavior for successful transactions.
| if not args.dry_run: | ||
| store.update_index() | ||
| return 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm _signal_write is the standard post-write nudge and how other CLI write paths call it.
rg -nP --type=py -C4 '_signal_write' src/omindRepository: CryptoJones/omind
Length of output: 2900
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cli relevant section =="
sed -n '1200,1285p' src/omind/cli.py
echo
echo "== store _signal_write and update_index =="
sed -n '840,865p' src/omind/store.py
rg -n -C 3 'def update_index|def update_index_locked|_signal_write|_invalidate|mesh|sync' src/omind/store.py
echo
echo "== programmatic probe for _run_recover return path signal_write calls =="
python3 - <<'PY'
from pathlib import Path
import ast
cli = Path('src/omind/cli.py').read_text()
tree = ast.parse(cli)
class Finder(ast.NodeVisitor):
def __init__(self):
self.in_func = set()
self.finders = []
def visit_FunctionDef(self, node):
old = self.in_func.copy()
self.in_func.add(node.name)
self.generic_visit(node)
self.in_func = old
def visit_Attribute(self, node):
if node.attr == '_signal_write' and isinstance(node.value, ast.Name):
pos = node.lineno, node.col_offset
enclosing = sorted(self.in_func, key=lambda x:-len(x))
self.finders.append((pos, node.value.id + '.' + node.attr, enclosing))
self.generic_visit(node)
f = Finder()
f.visit(tree)
print("All _signal_write attribute accesses:")
for pos, expr, stack in f.finders:
print(f" line {pos[0]}: {expr} (enclosing: {stack})")
target_lines = {1269,1270,1271}
runs = [n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == '_run_recover']
if not runs:
print("ERROR: _run_recover not found")
raise SystemExit(1)
store_update = []
for n in ast.walk(runs[0]):
if isinstance(n, ast.Attribute) and n.attr == 'update_index':
store_update.append(n)
print(f"\n_run_recover update_index calls: {[s.lineno for s in store_update]}")
print(f"_run_recover contains _signal_write: {any(pos[0] in target_lines for pos,_,_ in f.finders)}")
PYRepository: CryptoJones/omind
Length of output: 15504
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cli relevant section =="
sed -n '1200,1285p' src/omind/cli.py
echo
echo "== store _signal_write and update_index =="
sed -n '840,865p' src/omind/store.py
rg -n -C 3 'def update_index|def update_index_locked|_signal_write|_invalidate|mesh|sync' src/omind/store.py
echo
echo "== programmatic probe for _run_recover return path signal_write calls =="
python3 - <<'PY'
from pathlib import Path
import ast
cli = Path('src/omind/cli.py').read_text()
tree = ast.parse(cli)
class Finder(ast.NodeVisitor):
def __init__(self):
self.in_func = set()
self.finders = []
def visit_FunctionDef(self, node):
old = self.in_func.copy()
self.in_func.add(node.name)
self.generic_visit(node)
self.in_func = old
def visit_Attribute(self, node):
if node.attr == '_signal_write' and isinstance(node.value, ast.Name):
pos = node.lineno, node.col_offset
enclosing = sorted(self.in_func, key=lambda x:-len(x))
self.finders.append((pos, node.value.id + '.' + node.attr, enclosing))
self.generic_visit(node)
f = Finder()
f.visit(tree)
print("All _signal_write attribute accesses:")
for pos, expr, stack in f.finders:
print(f" line {pos[0]}: {expr} (enclosing: {stack})")
target_lines = {1269,1270,1271}
runs = [n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == '_run_recover']
if not runs:
print("ERROR: _run_recover not found")
raise SystemExit(1)
store_update = []
for n in ast.walk(runs[0]):
if isinstance(n, ast.Attribute) and n.attr == 'update_index':
store_update.append(n)
print(f"\n_run_recover update_index calls: {[s.lineno for s in store_update]}")
print(f"_run_recover contains _signal_write at lines {target_lines}: {any(pos[0] in target_lines for pos,_,_ in f.finders)}")
PYRepository: CryptoJones/omind
Length of output: 15532
Signal the write after recovery updates notes.
store._signal_write() is the nudge for search-index invalidation and mesh sync. _run_recover restores note content but only calls store.update_index(), which regenerates index.md. The derived search index can keep stale pre-rollback content until another write happens.
🐛 Recommended fix
if not args.dry_run:
store.update_index()
+ store._signal_write()
return 0📝 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.
| if not args.dry_run: | |
| store.update_index() | |
| return 0 | |
| if not args.dry_run: | |
| store.update_index() | |
| store._signal_write() | |
| return 0 |
🤖 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 1269 - 1271, Update _run_recover after the
non-dry-run recovery update to call store._signal_write() in addition to
store.update_index(), ensuring search-index invalidation and mesh
synchronization receive the restored note content.
| #: Bytes to write. ``None`` means "this file should not exist afterwards", | ||
| #: which recovery reads as "delete it on rollback if we created it". | ||
| content: str |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the content field comment.
The comment states that None means the file should not exist afterwards. The field is typed str and Transaction.write accepts only str. No deletion path exists. The comment describes behavior that the code does not implement, and recovery reads existed (not content) to decide removal.
📝 Proposed comment fix
- #: Bytes to write. ``None`` means "this file should not exist afterwards",
- #: which recovery reads as "delete it on rollback if we created it".
+ #: Text to write. Recovery uses ``existed`` (not this field) to decide
+ #: whether rollback deletes a file this transaction created.
content: str📝 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.
| #: Bytes to write. ``None`` means "this file should not exist afterwards", | |
| #: which recovery reads as "delete it on rollback if we created it". | |
| content: str | |
| #: Text to write. Recovery uses ``existed`` (not this field) to decide | |
| #: whether rollback deletes a file this transaction created. | |
| content: str |
🤖 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/txn.py` around lines 90 - 92, Update the documentation comment for
the content field in Transaction.write to describe only its actual str payload
semantics. Remove the claim that None requests deletion, and clarify that file
removal decisions during recovery use the existed field instead.
| def rollback(self, writer: Any) -> RecoveryReport: | ||
| """Undo a prepared-but-uncommitted transaction in this process.""" | ||
| report = _rollback_journal(self._dir(), writer) | ||
| self.discard() | ||
| return report |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
A conflicted in-process rollback destroys its own evidence and reports nothing. Transaction.rollback deletes the journal and pre-images whatever the outcome, and the single caller drops the returned RecoveryReport. When a rollback cannot restore every note, the pre-images are gone, omind recover finds no pending journal, and the operator learns only that the original write failed. recover() deliberately keeps a conflicted journal (src/omind/txn.py Lines 352-357); the in-process path must match that rule.
src/omind/txn.py#L251-L255: callself.discard()only whenreport.cleanis true, so a conflicted rollback keeps the journal and pre-images.src/omind/store.py#L1321-L1328: inspect the returned report, log the conflicting note names with a pointer toomind recover, and suppress a rollback failure so it cannot replace the exception that aborted the apply.
📍 Affects 2 files
src/omind/txn.py#L251-L255(this comment)src/omind/store.py#L1321-L1328
🤖 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/txn.py` around lines 251 - 255, Preserve conflicted rollback
evidence: in Transaction.rollback, discard the journal and pre-images only when
the RecoveryReport is clean; update src/omind/txn.py lines 251-255 accordingly.
In src/omind/store.py lines 1321-1328, inspect the returned report, log
conflicting note names with guidance to run omind recover, and suppress rollback
failures so they do not replace the exception that caused apply to abort.
| try: | ||
| payload = json.loads(journal.read_text(encoding="utf-8")) | ||
| except (OSError, ValueError): | ||
| continue | ||
| if str(payload.get("state")) != COMMITTED: | ||
| found.append(directory) | ||
| return found |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
An unparseable journal is invisible to the operator.
pending() skips a journal that fails to parse. _run_recover then prints "nothing to recover: no interrupted transactions" while a journal directory with pre-images still exists on disk. The operator gets a false all-clear.
_record uses temp file plus os.replace, so a torn journal is unlikely, but the failure mode is silent when it happens. Expose the unreadable directories so the CLI can name them.
♻️ Proposed change
+def unreadable(omi_dir: Path | str) -> list[Path]:
+ """Journal directories whose journal.json cannot be parsed."""
+ root = paths.transaction_dir(Path(omi_dir))
+ if not root.is_dir():
+ return []
+ bad: list[Path] = []
+ for directory in sorted(root.iterdir()):
+ journal = directory / "journal.json"
+ if not journal.is_file():
+ continue
+ try:
+ json.loads(journal.read_text(encoding="utf-8"))
+ except (OSError, ValueError):
+ bad.append(directory)
+ return bad🤖 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/txn.py` around lines 271 - 277, Update pending() to retain
directories whose journal cannot be read or parsed instead of silently
continuing, so _run_recover can report those directories as recovery candidates
and never claim there are no interrupted transactions while unreadable journal
data remains.
| @pytest.fixture | ||
| def omi(tmp_path: Path) -> Path: | ||
| d = tmp_path / "OMI" | ||
| d.mkdir() | ||
| return d |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
The omi fixture does not isolate the state directory.
txn.Transaction stores journals under paths.state_dir(), which resolves to $XDG_STATE_HOME/omind or ~/.local/state/omind. This fixture sets only tmp_path, so every test in this file writes journals and pre-images into the developer's real home directory and leaves them there.
The existing convention is in tests/test_consolidate.py (Lines 17-18), whose vault fixture sets XDG_STATE_HOME under tmp_path. Apply the same isolation here.
💚 Proposed fix
`@pytest.fixture`
-def omi(tmp_path: Path) -> Path:
+def omi(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path:
+ # Journals live in the state dir; keep them inside tmp_path so the tests
+ # neither pollute nor depend on the real home directory.
+ monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "state"))
d = tmp_path / "OMI"
d.mkdir()
return d📝 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.
| @pytest.fixture | |
| def omi(tmp_path: Path) -> Path: | |
| d = tmp_path / "OMI" | |
| d.mkdir() | |
| return d | |
| `@pytest.fixture` | |
| def omi(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: | |
| # Journals live in the state dir; keep them inside tmp_path so the tests | |
| # neither pollute nor depend on the real home directory. | |
| monkeypatch.setenv("XDG_STATE_HOME", str(tmp_path / "state")) | |
| d = tmp_path / "OMI" | |
| d.mkdir() | |
| return d |
🤖 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_txn.py` around lines 21 - 25, Update the omi fixture in
tests/test_txn.py to set XDG_STATE_HOME to a directory under tmp_path before
creating or using the test OMI directory, following the isolation pattern used
by the vault fixture in tests/test_consolidate.py. Preserve the fixture’s
returned OMI path while ensuring txn.Transaction state journals and pre-images
remain within the temporary test directory.
| def test_recovery_refuses_to_clobber_an_edit_made_after_the_crash(omi: Path) -> None: | ||
| """Someone's later edit is newer information than our pre-image (#194). | ||
|
|
||
| Blind rollback here would be data loss wearing recovery's clothes. | ||
| """ | ||
| a = omi / "A.md" | ||
| _write(a, "old A\n") | ||
| t = txn.Transaction(omi) | ||
| t.write(a, "new A\n") | ||
| t.prepare() | ||
| t.apply(_atomic_write) # applied, but the process died before commit() | ||
|
|
||
| a.write_text("a human fixed this by hand\n", encoding="utf-8") | ||
|
|
||
| reports = txn.recover(omi, _atomic_write) | ||
| assert len(reports) == 1 | ||
| assert reports[0].conflicts == ["A.md"] | ||
| assert not reports[0].clean | ||
| assert a.read_text() == "a human fixed this by hand\n" # preserved | ||
| assert txn.pending(omi) # journal kept, so the evidence survives for a human | ||
|
|
||
|
|
||
| def test_recovery_is_idempotent(omi: Path) -> None: | ||
| a = omi / "A.md" | ||
| _write(a, "old A\n") | ||
| t = txn.Transaction(omi) | ||
| t.write(a, "new A\n") | ||
| t.prepare() | ||
| t.apply(_atomic_write) | ||
|
|
||
| assert txn.recover(omi, _atomic_write)[0].clean | ||
| assert a.read_text() == "old A\n" | ||
| assert txn.recover(omi, _atomic_write) == [] # second pass finds nothing | ||
| assert a.read_text() == "old A\n" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a case where the restore writer itself fails.
_rollback_journal converts a writer OSError or a UnicodeDecodeError into a conflict (src/omind/txn.py Lines 317-319). That arm decides whether a failed restore preserves data or loses it, and no test covers it. The other failure paths in this file are each pinned by a test.
💚 Proposed test
def test_a_failing_restore_is_reported_as_a_conflict(omi: Path) -> None:
a = omi / "A.md"
_write(a, "old A\n")
t = txn.Transaction(omi)
t.write(a, "new A\n")
t.prepare()
t.apply(_atomic_write)
def failing_writer(path: Path, content: str) -> None:
raise OSError("read-only filesystem")
reports = txn.recover(omi, failing_writer)
assert reports[0].conflicts == ["A.md"]
assert a.read_text() == "new A\n" # untouched
assert txn.pending(omi) # journal kept for a human🤖 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_txn.py` around lines 94 - 127, Add a test alongside the recovery
tests named test_a_failing_restore_is_reported_as_a_conflict that prepares and
applies a transaction, then calls txn.recover with a writer raising OSError.
Assert the report marks A.md as a conflict, the applied content remains
unchanged, and txn.pending(omi) confirms the journal is retained.
Both windows-latest legs failed. `_atomic_write` writes in text mode, so on Windows every `\n` reaches the disk as `\r\n`. `new_sha` was computed over the pre-translation string, so the bytes on disk matched neither the pre-image nor what we had just written — recovery classified every file as "edited after the crash", refused to touch any of them, and rolled back nothing. The whole feature was inert on Windows, and silent about it: `omind recover` would report conflicts and exit 1 forever. Same class as #202 — a platform-specific path that turns into permanent no-op rather than an error. Content identity is now hashed over line-ending-normalized text. Pre-images are still stored and restored as exact bytes; only the *comparison* normalizes. `test_rollback_recognizes_its_own_write_after_line_ending_translation` reproduces it on any platform by writing the translated bytes directly — verified to fail without this fix and pass with it, so a Linux-only run still catches a regression. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second Windows failure, different cause. Rollback restored a pre-image by decoding it and handing the string to the store's `_atomic_write`, which is TEXT mode — so on Windows the `\n` was translated a second time. A pre-image of b"old A\r\n" was written back as b"old A\r\r\n", meaning every rollback silently grew a blank line in the note it was supposed to be restoring. `read_text()` saw 'old A\n\n'. A pre-image is bytes. Recovery now writes it back with a binary same-dir temp + os.replace, so what was there is exactly what comes back. The `writer` argument to recover()/rollback() is gone with it — restoration was never the caller's business. The first version of this test asserted on bytes and passed against the bug, because on Linux the text writer translates nothing. Replaced with one that installs a Windows-like translating writer and asserts recovery never routes through it — verified to fail without this fix and pass with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes #194.
store.py— the single write path to the source of truth (invariant 4) — so it deserves a real read, not a merge-on-green. Flagging that explicitly.The honest contract
Multi-file updates cannot be truly atomic on the filesystems omind runs on. This doesn't pretend otherwise. What it provides is the contract that can be kept:
An interrupted apply therefore lands in one of two states, never a third: fully applied, or fully rolled back once
omind recoverruns.What was actually missing
omind had the first half — same-dir temp +
os.replace, an advisory write lock, version preconditions. It had no journal, so a multi-note operation interrupted midway left partial state with no recovery path.store.create_and_disable_sourcesconceded it in its own docstring:Failing toward keeping data is the right direction, but "extra recoverable copies" still means a human notices and reconciles by hand. That docstring is now gone, and so is the behavior behind it.
The design decision worth reviewing
Recovery refuses to clobber an edit made after the crash.
A pre-image is restored only when the file still holds either that pre-image (nothing to do) or exactly the bytes the interrupted run intended to write (ours to undo — that's what
new_shais for). Anything else means someone edited the note after the crash, and their edit is newer information than our pre-image. So it's reported as a conflict, left untouched, and its journal is kept rather than deleted, so the evidence survives for a human.omind recoverexits 1.Blind rollback would be data loss wearing recovery's clothes. Verified end-to-end: a hand-edit made post-crash survives recovery intact.
Verified on a real vault, not just fixtures
And the conflict path:
Tests — 13 new, mostly failure cases
The one that matters is
test_an_interrupted_apply_is_rolled_back: it kills the writer between two real writes, asserts the vault is genuinely half-applied, then asserts recovery restores it. Plus: prepare changes nothing on disk; recovery is idempotent; a committed transaction is never rolled back (crash between commit record and cleanup); dry-run changes nothing; the conflict refusal; the journal never lands in the vault (invariant 1); a corrupt journal raises rather than silently skipping; andcreate_and_disable_sourcesself-rolls-back a failed merge with no stray note and no half-archived source.Scope notes
consolidate --applyneeds no manualrecover. The journal is for when the process dies outright.approved_plan_sha256handshake, as the issue planned — right for an LLM-driven research vault, wrong in front of routine memory writes.omind migrate. Both are named in the issue as callers; doing them in the same PR as the primitive would make this reviewable-by-nobody. Happy to follow up.AGENTS.mdgains invariant 3 so the next multi-note operation doesn't quietly reintroduce the gap.Version
8.0.0 — new store-level primitive and a new command.
Gates
ruff check .·mypy src(strict, 47 files) ·pytest(882 passed) ·pip-audit— green locally.🤖 Generated with Claude Code