Hardening/sqlite write atomicity - #96
Conversation
MARM's pooled connections run in full autocommit mode (isolation_level= None), so a multi-statement write sequence followed by conn.commit() was never actually atomic -- each statement committed immediately on its own, and a failure partway through left earlier statements permanently applied with nothing to roll back. Wrapped every meaningful multi-statement mutation path in explicit BEGIN IMMEDIATE / COMMIT / ROLLBACK-on-exception, matching the pattern already established by _store_memory and apply_compaction_write: - services/log_entry.py: log-entry creation, session-switch, dated- fallback session, single-entry delete, whole-session delete, notebook delete - endpoints/session.py: marm_start's session activation - services/notebook.py: notebook add's update-then-insert - services/documentation.py: legacy system-notebook cleanup + marker insert - core/memory_ops.py: _update_memory, which used to hold the write lock open across an await for embedding generation (a real event-loop-stall risk under concurrent load) -- restructured to compute the merge and its embedding before acquiring the lock, then re-verify under the lock that neither content nor metadata changed concurrently before writing, and folded the memory-chunks cleanup into the same transaction as the content update Audited and left unchanged: endpoints/compaction.py (every mutation path is already single-statement, hence already atomic; wrapping the whole per-candidate loop in one transaction would change intended per-candidate- independent semantics) and core/memory_db.py (isolation_level untouched by design; no shared transaction helper added since the inline pattern stayed consistent and small across every file that needed it). Work was split into 3 packets to keep the diff reviewable: two spawned agents each owned a non-overlapping file pair, the third packet (memory_ ops.py + the compaction.py/memory_db.py audit) was implemented directly. Every packet got an independent review before being accepted: - Packet A's own review surfaced no code issues; running the full suite against it did surface one pre-existing test relying on an implementation detail (intercepting .commit() directly) that no longer applied once commits went through conn.execute(COMMIT) -- fixed the test, not the production code. - Packet B's review found nothing to fix. - Packet C (implemented by the coordinator) was reviewed by Agent A, which found one real, narrow TOCTOU gap: _update_memory's post-lock re-check originally compared only content against the unlocked pre-read, missing a concurrent metadata-only write from _restore_sources_from_deleted_ summary/_update_summary_metadata_after_delete. Fixed by widening the re-check to compare metadata too. - The coordinator's own first draft of _update_memory also had a real bug caught by the full test suite (not by review): the re-check compared the re-read row against the truncated existing_content instead of the original, causing every large merge to silently no-op. Fixed by preserving the untruncated original for comparison. Every new transaction boundary has a mutation-tested regression test in the new tests/test_sqlite_write_atomicity.py: each test forces a specific SQL statement to fail and asserts the earlier statement(s) in the same block did not durably apply. Every test was verified to actually fail against the pre-hardening code before being accepted -- a happy-path-only test doesn't prove rollback. No schema changes, no new endpoints, no MCP tool-surface changes. Full suite: 636 passed, 20 skipped, 8 slow_stdio passed, zero regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
- endpoints/compaction.py: documented the audit conclusion inline (every mutation path is single-statement, batch-wrapping the per-candidate loop would regress intended per-candidate-independent semantics, not harden them) and added a real regression test proving it: one expired candidate in the same request as one valid candidate must not block the valid one from staging. - services/documentation.py: documented why _index_doc's cleanup block is intentionally left unwrapped (single statement per branch, already atomic, no gap). Refuted the third finding (log_entry.py's guarded session_summary_cache update swallowing exceptions): confirmed via git history that this exact guard predates the write-atomicity branch entirely (already on origin/MARM-main before this branch started) and is explicitly documented elsewhere in the same file as deliberate -- cache dirty-marking is best-effort and must never abort a real log write. The spec's testing-checklist wording that prompted this finding was imprecise; the actual rollback-worthy failure (session upsert) is already covered by test_create_log_entry_rollback_no_partial_row_on_ second_statement_failure. Full suite: 637 passed, 20 skipped, zero regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011s8tBNnQTmb1q5nRAKwhvv
📝 WalkthroughWalkthroughSQLite write paths now use explicit ChangesSQLite atomicity hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc93d39065
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| current is None | ||
| or current[0] != original_existing_content | ||
| or current[1] != metadata_json | ||
| ): | ||
| conn.execute("ROLLBACK") | ||
| return |
There was a problem hiding this comment.
When direct writes run concurrently (for example WRITE_QUEUE_ENABLED=0 or callers using MARMMemory.store_memory directly) and two semantic duplicates target the same existing row, both can pre-read the old content while embedding generation yields; the first merge commits, then the second hits this stale check, rolls back, and returns as if the store succeeded. _store_memory still returns the existing id and increments write accounting, but the second entry's content is never stored. This should retry against the fresh row or signal failure instead of silently dropping the merge.
Useful? React with 👍 / 👎.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@marm-mcp-server/marm_mcp_server/core/memory_ops.py`:
- Around line 112-118: Update the concurrency check in _store_memory so a
changed snapshot does not return silently: on mismatch, reload the latest row,
recompute the merge and embedding inputs, and retry the transaction with a
bounded attempt count. If all retries encounter contention, raise or propagate
an error instead of reporting the existing ID as successful; preserve the
current commit path when the snapshot remains unchanged.
In `@marm-mcp-server/marm_mcp_server/services/documentation.py`:
- Around line 112-119: Update the documentation replacement flow around
store_memory_queued so the new memory is successfully stored before deleting the
existing documentation memory. Once storage succeeds, atomically update
doc_index and remove the old row through the serialized asynchronous write
queue, ensuring failures leave the current memory and index intact and that no
direct memory writes bypass the queue.
In `@marm-mcp-server/marm_mcp_server/services/log_entry.py`:
- Around line 166-202: Route every listed transaction through the existing
serialized asynchronous write queue instead of directly executing BEGIN
IMMEDIATE on the async paths: update log_entry.py lines 166-202, 49-89, 116-132,
324-359, 360-387, and 401-410; session.py lines 25-38; notebook.py lines 31-53;
and documentation.py lines 274-289. Preserve each operation’s current SQL,
ordering, error behavior, and results while ensuring all memory writes are
submitted to the queue.
In `@marm-mcp-server/tests/test_sqlite_write_atomicity.py`:
- Line 95: Add failure-injection rollback tests in the SQLite write atomicity
test suite for session-switch creation, dated fallback creation, targeted log
deletion, and notebook deletion. Exercise each path through the real FastAPI
endpoints backed by real SQLite, verify the injected failure rolls back all
mutations, and cover the corresponding service flows in services/log_entry.py
and the additional referenced branch.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af53f954-44d4-440b-ad88-33d1ffdd95fc
📒 Files selected for processing (9)
CHANGELOG.mdmarm-mcp-server/marm_mcp_server/core/memory_ops.pymarm-mcp-server/marm_mcp_server/endpoints/compaction.pymarm-mcp-server/marm_mcp_server/endpoints/session.pymarm-mcp-server/marm_mcp_server/services/documentation.pymarm-mcp-server/marm_mcp_server/services/log_entry.pymarm-mcp-server/marm_mcp_server/services/notebook.pymarm-mcp-server/tests/test_sqlite_write_atomicity.pymarm-mcp-server/tests/test_stdio_transport.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
marm-mcp-server/marm_mcp_server/endpoints/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/endpoints/**/*.py: Endpoint logic belongs underendpoints/, split by surface; shared helpers belong incore/.
New MCP tools must be implemented in the appropriateendpoints/<surface>.pymodule.
Files:
marm-mcp-server/marm_mcp_server/endpoints/session.pymarm-mcp-server/marm_mcp_server/endpoints/compaction.py
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: All memory writes must use the serialized asynchronous write queue; do not bypass it.
Amarm_log_entrysemantic-store failure must never fail the corresponding log write.
Never share database connections between the memory SQLite database and the concept-graph SQLite database.
Graph and concept failures must not break the seven core memory tools; the graph supervisor starts lazily and operates degraded on failure.
Use one lazy-loadedall-MiniLM-L6-v2fastembed encoder serialized behind a lock; writes must succeed when embeddings are unavailable.
Prefer the smallest implementation that solves the problem; avoid speculative abstractions and unrequested configuration flags.
Use minimal comments only for non-obvious rationale; do not add comments narrating the next line.
Keep orchestration in its current owner file and extract modules only at genuine boundaries.
Files:
marm-mcp-server/marm_mcp_server/endpoints/session.pymarm-mcp-server/marm_mcp_server/endpoints/compaction.pymarm-mcp-server/marm_mcp_server/services/notebook.pymarm-mcp-server/marm_mcp_server/services/documentation.pymarm-mcp-server/marm_mcp_server/core/memory_ops.pymarm-mcp-server/marm_mcp_server/services/log_entry.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
marm-mcp-server/marm_mcp_server/endpoints/session.pymarm-mcp-server/marm_mcp_server/endpoints/compaction.pymarm-mcp-server/marm_mcp_server/services/notebook.pymarm-mcp-server/marm_mcp_server/services/documentation.pymarm-mcp-server/marm_mcp_server/core/memory_ops.pymarm-mcp-server/tests/test_stdio_transport.pymarm-mcp-server/marm_mcp_server/services/log_entry.pymarm-mcp-server/tests/test_sqlite_write_atomicity.py
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Only flag documentation issues that are materially wrong, misleading for installation/release behavior, or inconsistent with live MCP behavior. Skip style, phrasing, formatting, and wording preferences.
Files:
CHANGELOG.md
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Run tests withpytestfrommarm-mcp-server/; tests should use real FastAPI endpoints and real SQLite, mocking only when it closely matches real behavior.
Every new MARM Console API route must have at least one happy-path FastAPI response-contract test with the MCP adapter stubbed.
Do not write existence-check or coded-to-pass tests; prefer deep tests exercising real paths over broad shallow coverage.
Usepytest.mark.skiponly for genuinely unavailable dependencies, never merely to avoid implementation effort.
Files:
marm-mcp-server/tests/test_stdio_transport.pymarm-mcp-server/tests/test_sqlite_write_atomicity.py
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-mcp-server/tests/test_stdio_transport.pymarm-mcp-server/tests/test_sqlite_write_atomicity.py
🪛 ast-grep (0.44.1)
marm-mcp-server/marm_mcp_server/core/memory_ops.py
[info] 124-124: use jsonify instead of json.dumps for JSON output
Context: json.dumps(metadata)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 136-136: use jsonify instead of json.dumps for JSON output
Context: json.dumps(metadata)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (3)
marm-mcp-server/tests/test_stdio_transport.py (1)
325-356: LGTM!marm-mcp-server/marm_mcp_server/endpoints/compaction.py (1)
1-17: LGTM!CHANGELOG.md (1)
5-11: LGTM!Also applies to: 13-15
| if ( | ||
| current is None | ||
| or current[0] != original_existing_content | ||
| or current[1] != metadata_json | ||
| ): | ||
| conn.execute("ROLLBACK") | ||
| return |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Retry instead of silently dropping a concurrent merge.
If the snapshot changes during embedding generation, this returns without storing new_content; _store_memory still reports the existing ID as successful. Recompute from the latest row with a bounded retry, and surface an error if contention persists.
🤖 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 `@marm-mcp-server/marm_mcp_server/core/memory_ops.py` around lines 112 - 118,
Update the concurrency check in _store_memory so a changed snapshot does not
return silently: on mismatch, reload the latest row, recompute the merge and
embedding inputs, and retry the transaction with a bounded attempt count. If all
retries encounter contention, raise or propagate an error instead of reporting
the existing ID as successful; preserve the current commit path when the
snapshot remains unchanged.
| # Audited under the SQLite write-atomicity hardening effort | ||
| # (docs/current/sqlite-write-atomicity-hardening.md): no BEGIN | ||
| # IMMEDIATE needed here. Exactly one of the two branches below | ||
| # runs per call, and each is a single statement -- a lone | ||
| # statement is already atomic under SQLite regardless of | ||
| # isolation_level, so there's no multi-statement sequence to | ||
| # protect. store_memory_queued below intentionally stays outside | ||
| # any transaction (it awaits and does its own internal locking). |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not delete the current documentation memory before its replacement exists.
The old memory is committed as deleted before store_memory_queued runs. If storage fails, documentation disappears and doc_index remains stale until a later retry. Store first, then atomically swap the index and remove the old row through the write queue.
As per coding guidelines, all memory writes must use the serialized asynchronous write queue; do not bypass it.
🤖 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 `@marm-mcp-server/marm_mcp_server/services/documentation.py` around lines 112 -
119, Update the documentation replacement flow around store_memory_queued so the
new memory is successfully stored before deleting the existing documentation
memory. Once storage succeeds, atomically update doc_index and remove the old
row through the serialized asynchronous write queue, ensuring failures leave the
current memory and index intact and that no direct memory writes bypass the
queue.
Source: Coding guidelines
PR #96 review findings (CodeRabbit + Codex): - core/memory_ops.py: _update_memory now returns bool indicating whether it actually wrote. _store_memory previously reported a stale semantic merge as successful even when _update_memory's lock re-check bailed on a concurrent change -- silently dropping the caller's content. Falls through to storing as a new memory instead. - tests/test_sqlite_write_atomicity.py: added mutation-tested rollback coverage for the session-switch, dated-fallback, targeted log-delete, and notebook-delete transaction boundaries (previously untested despite the CHANGELOG's coverage claim). - tests/test_consolidation_write_time.py: regression test for the dropped-write fix; tightened the missing-id test to assert the new bool contract. Deferred (documented, not fixed): documentation.py's delete-before-store ordering in doc re-indexing -- the suggested reorder would expose the new content to the semantic-duplicate scan when CONSOLIDATION_ENABLED=1, causing merge-append corruption instead of clean replacement. Self-heals today via the existing stale-doc_index recovery path. Refuted: routing log/session/notebook SQL through WriteQueue -- contradicts the hardening spec's explicit rejection of that approach; WriteQueue serializes writes, it doesn't make a SQL sequence atomic.
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@marm-mcp-server/tests/test_sqlite_write_atomicity.py`:
- Around line 285-316: Update
test_dated_fallback_rollback_keeps_previous_active_session so
memory.active_log_session remains "main" when /marm_log_entry is called,
triggering the dated-fallback insert. Remove or avoid the preceding /marm_start
setup and adjust assertions/setup as needed while preserving verification that
the prior active session remains active and no dated fallback row is created.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e8859bc-e743-452d-b765-9e75cacce9d2
📒 Files selected for processing (5)
CHANGELOG.mdmarm-mcp-server/marm_mcp_server/core/memory.pymarm-mcp-server/marm_mcp_server/core/memory_ops.pymarm-mcp-server/tests/test_consolidation_write_time.pymarm-mcp-server/tests/test_sqlite_write_atomicity.py
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- marm-mcp-server/marm_mcp_server/core/memory_ops.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: All memory writes must use the serialized asynchronous write queue; do not bypass it.
Amarm_log_entrysemantic-store failure must never fail the corresponding log write.
Never share database connections between the memory SQLite database and the concept-graph SQLite database.
Graph and concept failures must not break the seven core memory tools; the graph supervisor starts lazily and operates degraded on failure.
Use one lazy-loadedall-MiniLM-L6-v2fastembed encoder serialized behind a lock; writes must succeed when embeddings are unavailable.
Prefer the smallest implementation that solves the problem; avoid speculative abstractions and unrequested configuration flags.
Use minimal comments only for non-obvious rationale; do not add comments narrating the next line.
Keep orchestration in its current owner file and extract modules only at genuine boundaries.
Files:
marm-mcp-server/marm_mcp_server/core/memory.py
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Prioritize runtime correctness, async/concurrency safety, SQLite transaction safety, auth/rate-limit behavior, release-breaking packaging issues, and MCP protocol compatibility.
Files:
marm-mcp-server/marm_mcp_server/core/memory.pymarm-mcp-server/tests/test_consolidation_write_time.pymarm-mcp-server/tests/test_sqlite_write_atomicity.py
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Run tests withpytestfrommarm-mcp-server/; tests should use real FastAPI endpoints and real SQLite, mocking only when it closely matches real behavior.
Every new MARM Console API route must have at least one happy-path FastAPI response-contract test with the MCP adapter stubbed.
Do not write existence-check or coded-to-pass tests; prefer deep tests exercising real paths over broad shallow coverage.
Usepytest.mark.skiponly for genuinely unavailable dependencies, never merely to avoid implementation effort.
Files:
marm-mcp-server/tests/test_consolidation_write_time.pymarm-mcp-server/tests/test_sqlite_write_atomicity.py
**/tests/**
⚙️ CodeRabbit configuration file
**/tests/**: Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path. Skip minor naming, comments, and layout preferences.
Files:
marm-mcp-server/tests/test_consolidation_write_time.pymarm-mcp-server/tests/test_sqlite_write_atomicity.py
🔇 Additional comments (4)
marm-mcp-server/marm_mcp_server/core/memory.py (1)
247-248: LGTM!marm-mcp-server/tests/test_consolidation_write_time.py (2)
57-60: LGTM!
259-304: LGTM!marm-mcp-server/tests/test_sqlite_write_atomicity.py (1)
256-279: 🎯 Functional CorrectnessThis assertion matches the
Session:path. BothSession:andTopic:append-{today}to the new session name, so the test’snew_sessioncheck is correct.> Likely an incorrect or invalid review comment.
| def test_dated_fallback_rollback_keeps_previous_active_session(monkeypatch, tmp_path): | ||
| """create_log_entry's dated-fallback block (no session_name given and | ||
| memory.active_log_session is still "main"): UPDATE sessions SET | ||
| marm_active = FALSE, then an INSERT for the new dated session. Force | ||
| that insert to fail -- the previously active session's marm_active | ||
| flag must not have been cleared, and the dated fallback session row | ||
| must not exist.""" | ||
| server = load_isolated_server(monkeypatch, tmp_path) | ||
| client = local_client(server.app) | ||
|
|
||
| first = client.post("/marm_start", json={"session_name": "atomicity-a5-old"}) | ||
| assert first.status_code == 200 | ||
|
|
||
| db_path = tmp_path / "marm_memory.db" | ||
| with sqlite3.connect(db_path) as conn: | ||
| active_before = conn.execute( | ||
| "SELECT marm_active FROM sessions WHERE session_name = ?", | ||
| ("atomicity-a5-old",), | ||
| ).fetchone()[0] | ||
| assert active_before in (1, True) | ||
|
|
||
| import marm_mcp_server.services.log_entry as log_entry | ||
|
|
||
| # First occurrence of "INSERT INTO sessions" in this call path is the | ||
| # dated-fallback block's own insert (the later per-entry upsert further | ||
| # down in create_log_entry is a second, distinct occurrence that never | ||
| # gets reached once this one raises). | ||
| _fail_on(monkeypatch, log_entry.memory, "INSERT INTO sessions") | ||
|
|
||
| resp = client.post( | ||
| "/marm_log_entry", json={"entry": "plain entry with no session prefix"} | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the fallback block logic in create_log_entry to confirm the active session condition.
rg -A 10 -B 2 'active_log_session' marm-mcp-server/marm_mcp_server/services/log_entry.pyRepository: Lyellr88/marm-memory
Length of output: 2714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test and its helpers around the cited lines.
ast-grep outline marm-mcp-server/tests/test_sqlite_write_atomicity.py --view expanded || true
printf '\n--- relevant slice ---\n'
sed -n '250,340p' marm-mcp-server/tests/test_sqlite_write_atomicity.py
printf '\n--- helper search ---\n'
rg -n "def load_isolated_server|def local_client|/marm_start|active_log_session" marm-mcp-server/tests marm-mcp-server/marm_mcp_server -g '!**/__pycache__/**'Repository: Lyellr88/marm-memory
Length of output: 9687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the dated-fallback implementation directly for the session-selection condition.
sed -n '120,220p' marm-mcp-server/marm_mcp_server/services/log_entry.pyRepository: Lyellr88/marm-memory
Length of output: 4306
Trigger the dated-fallback branch in this test. /marm_start switches memory.active_log_session off "main", so the later /marm_log_entry call reuses atomicity-a5-old instead of hitting the dated fallback insert. Start this case with "main" (or skip the prior start call) so the rollback path is actually covered.
🤖 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 `@marm-mcp-server/tests/test_sqlite_write_atomicity.py` around lines 285 - 316,
Update test_dated_fallback_rollback_keeps_previous_active_session so
memory.active_log_session remains "main" when /marm_log_entry is called,
triggering the dated-fallback insert. Remove or avoid the preceding /marm_start
setup and adjust assertions/setup as needed while preserving verification that
the prior active session remains active and no dated fallback row is created.
Source: Coding guidelines
2 participants
Summary by CodeRabbit
Reliability Improvements
Tests
Documentation