Release/v2.36.0 - #128
Conversation
The concept graph only grew when someone ran a build, so it was stale until a human remembered to refresh it. Storing a memory now queues it and a background worker turns it into a node about 30 seconds later, on both transports. Automation: - Durable outbox in the memory DB, enqueued in the same transaction as the memory, so a memory cannot exist without its indexing task and a killed process loses nothing. Teardown stops the worker rather than waiting on extraction. - Per-memory build outcomes (indexed/no_entities/failed/vanished). The aggregate counters cannot report a failed extraction, and settling queue tasks on them would delete work promised for retry. - Failures retry with backoff and park after CONCEPT_INDEX_MAX_ATTEMPTS rather than blocking the queue. Nothing here can block a write. - Console Explorer polls a cheap change marker and refreshes the graph without a reload, stopping when its tab is not showing. Fixes: - Builds truncated at the 500 newest rows, leaving older memories permanently unreachable. They now page through the whole scope; CONCEPT_BUILD_ROW_CAP is a page size, not a ceiling. - Builds indexed compaction summaries instead of their sources, so concepts were attributed to a paraphrase. Inverted, which is also what makes a memory graph-eligible the moment it is written. - init_concept_database stamped the current schema version on every ConceptDB construction, so one console delete could mark a stale graph as current and skip its rebuild. - The Console restated the schema version as a literal and would have called every rebuilt graph stale after the bump. Concurrency: - HTTP and STDIO are two processes over one database and the build lock was an asyncio.Lock. A rebuild drops the graph tables, so both writers now take a leased row in the memory DB, renewed while work runs and always acquired before the in-process lock. - Losing that lease stops the build at the next memory and settles nothing, rather than writing alongside the new owner. BREAKING CHANGE: existing graphs report rebuild_required and need one marm_concept_build(search_all=True). They hold summary-derived entities the new rule would never produce and there is no selective retraction. The old graph is backed up beside the database first.
The suite disables the encoder for isolation, so nothing exercised store and recall latency while the background indexer runs. Adds the benchmark that does, and acts on what it found. Measured on a 400-memory corpus, worker draining a full backlog: - recall 12ms -> 20ms median, p95 18ms -> 33ms - writes 11ms -> 19ms median The spec named the encoder lock as the main risk. Half right. Disabling entity-name embedding drops the write cost from +65% to +13% and leaves recall unchanged at +82%, so recall contention is CPU competition from spaCy extraction, not lock waiting. - New CONCEPT_INDEX_BATCH_PAUSE_MS (250) pauses between batches. It does not move p95 (32.6 -> 32.0ms) and costs 18% drain time, but it cuts worst-case recall from ~270ms to ~80ms, reproducibly across runs. The tail is what a user feels, so it is on by default; 0 disables it. - Smaller batches were tried and are worse: batch 5 with the same pause took p95 to 100ms, so batch size stays at 20. - bench_concept_worker.py runs the two-phase comparison or a pause sweep, against a synthetic corpus or a copy of the live one.
Running the benchmark with --from-live exposed a bug in the benchmark itself: it enqueued COALESCE(content_hash, id), so every memory whose content_hash column was NULL got a hash that could never match. The worker extracted all of them, saw a mismatch, and discarded every result as superseded. It paid the CPU cost and wrote nothing, and the run reported timings for a worker that never indexed anything. - Backfill the real hash when queueing legacy rows. - Count progress against the ids actually queued. Probe writes enqueue themselves, so raw queue depth grew while the worker was clearing it and the "still pending" line was meaningless. - Guard the worker against a NULL stored hash. Treating it as a mismatch never settles the task and never counts a failure, so that memory is re-extracted forever without ever being written or parked. Unreachable from the shipped write paths, which always compute a hash, but it cost a whole benchmark run to notice. Corrected numbers, real 768-memory corpus, two runs: - store_memory 10.2-11.2ms -> 10.8-11.1ms (-0.5% to +6%) - recall_similar 8.3-8.5ms -> 15.0-17.7ms (+80% to +109%) The published write regression was an artifact of short synthetic memories generating entity embeddings much faster than real content does. Writes are effectively unaffected on a real corpus. README section 5 now records this, and the benchmarking README says to prefer --from-live before publishing.
The dedicated benchmarks section for concept indexing was more detail than the cost warranted. Removing it left four references pointing at a heading that no longer exists, plus a gap in the section numbering. - Renumber LoCoMo and the competitor matrix back to 5 and 6, and restore the intro sentence that described what each section covers. - Point the three README bullets at the benchmark script instead of the removed anchor. The measured numbers stay where a reader meets the feature; only the deep-dive is gone. - Drop the same anchor from the CHANGELOG and the benchmarking README. No code changes.
📝 WalkthroughWalkthroughAutomatic concept-graph indexing now uses a durable SQLite queue, retries, leases, cross-process graph locking, background workers, paginated builds, and graph-version polling for Explorer refreshes. Release metadata, documentation, lifecycle wiring, tests, and benchmarking were updated for v2.36.0. ChangesConcept indexing
Release documentation
Estimated code review effort: 5 (Critical) | ~120 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: e4ca0f8fc1
ℹ️ 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".
| task.cancel() | ||
| try: | ||
| await task |
There was a problem hiding this comment.
When shutdown occurs while _process is awaiting build_for_memory_ids, this cancellation only cancels the await around asyncio.to_thread; the _run_build thread continues writing. Unwinding _drain then stops both heartbeats and releases the cross-process graph lock, allowing the other transport to acquire it and reset or write the same concept database concurrently. Signal the abort event and retain the lock until the extraction thread observes it or completes.
Useful? React with 👍 / 👎.
| conn.execute("DELETE FROM memory_chunks WHERE memory_id = ?", (memory_id,)) | ||
| # A console edit replaces the content wholesale, so the graph holds | ||
| # entities from text that no longer exists until this is re-indexed. | ||
| enqueue_concept_index(conn, memory_id, content_hash) |
There was a problem hiding this comment.
When an existing memory is replaced through the Console, this queues its new content, but the targeted build only adds entities and relationships and never removes the memory ID from concepts derived from the previous content. The same issue affects merges and document resaves, so after the task completes concept recall can permanently return facts that are no longer present in the memory. Remove the old provenance under the graph build lock before extracting the replacement.
Useful? React with 👍 / 👎.
| if "entities" not in existing_tables: | ||
| conn.execute( | ||
| "INSERT INTO concept_schema_metadata (key, value) VALUES (?, ?) " | ||
| "ON CONFLICT(key) DO NOTHING", | ||
| (_SCHEMA_VERSION_KEY, str(CONCEPT_SCHEMA_VERSION)), | ||
| ) |
There was a problem hiding this comment.
During the required v2-to-v3 upgrade, _prepare_build_schema drops the old graph before calling this initializer, so existing_tables lacks entities and these lines stamp schema version 3 before any corpus rows are rebuilt. If the process exits, loses its lock, or raises during the subsequent build, inspect_concept_schema reports current after restart and no longer prompts for the required rebuild; legacy memories also have no automatic-index queue rows to recover the missing portion. Preserve a rebuild-required marker until the full search_all build succeeds.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
marm-mcp-server/tests/test_concept_endpoints.py (1)
285-316: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the pagination baseline with
_BUILD_ROW_FILTERS.
_BUILD_ROW_FILTERSkeeps rows whencompaction_roleisNULLor not"summary", whilemarm-mcp-server/tests/test_concept_build_pagination.py:96-99hand-makes(compaction_role IS NULL OR compaction_role != 'source'). Update the baseline SQL to match the production filter.🤖 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_concept_endpoints.py` around lines 285 - 316, The test setup for test_fetch_memory_pages_indexes_sources_and_skips_compaction_summaries uses the wrong compaction-role condition. Update the hand-built baseline SQL in the related pagination test to retain rows where compaction_role is NULL or not 'summary', matching _BUILD_ROW_FILTERS, rather than excluding 'source'.
🤖 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/concept_build_lock.py`:
- Around line 168-184: Update the _keep_alive heartbeat to track the timestamp
of the last successful renewal and set lease.lost, log the lock loss, and stop
looping once the elapsed time since that success exceeds ttl_seconds, including
when renew raises exceptions. Preserve the existing immediate lease.lost
behavior when renew returns False, while resetting the successful-renewal
timestamp after each renewal.
In `@marm-mcp-server/README.md`:
- Line 161: Restore the missing Section 5 benchmark results, or remove its
reference, consistently in both marm-mcp-server/README.md (line 161) and
marm-mcp-server/marm_mcp_server/resources/marm-docs/README.md (line 135). Ensure
the subsequent headings remain correctly numbered: retain Section 6 at lines 205
and 179 and Section 7 at lines 217 and 191 only when their preceding sections
exist; otherwise renumber them sequentially.
In `@marm-mcp-server/tests/test_concept_two_process.py`:
- Around line 188-222: Make test_work_outliving_its_ttl_keeps_both_locks
reliable by preventing the synchronous _run_in_second_process call from blocking
the event loop: execute the child-process invocation via asyncio.to_thread while
preserving its existing assertions and lock/claim scenario. Keep the TTL
behavior under test unchanged.
In `@marm-mcp-server/tests/test_concept_worker_wiring.py`:
- Around line 17-31: Update _reload_settings and its callers to require a pytest
tmp_path fixture and set MARM_DB_PATH and MARM_API_KEY to paths under that
temporary directory before reloading marm_mcp_server.config.settings. Ensure
both the initial reload and restore_settings reload use isolated test paths,
preventing DEFAULT_DB_PATH or the API key path from touching ~/.marm or real
parent directories.
In `@marm-mcp-server/tests/test_graph_context.py`:
- Around line 236-241: Strengthen the test around
memory_endpoints._cleanup_deleted_concepts by asserting the returned status
equals the cleanup success value rather than merely not being failed. Ensure the
test creates or uses the concept database so cleanup is not skipped, then verify
the provenance data for concept “m1” has been removed.
In `@README.md`:
- Line 912: Update the README section describing concept indexing to qualify the
durable outbox and background-worker behavior as automatic mode, while stating
that CONCEPT_AUTO_INDEX=false still creates concept_index_queue rows. Update the
corresponding CHANGELOG.md entry at lines 8-10 to clarify that manual mode
disables processing but does not stop queued work.
In `@scripts/benchmarking/performance/bench_concept_worker.py`:
- Around line 388-414: Update the contended benchmark flow around the warm-up
loop and still_working calculation to track whether queued work actually
decreased before timing. When no memories are indexed, report the worker as
idle/no-progress rather than “busy throughout,” and ensure the contention
measurements are not presented as valid busy-worker results; preserve the
existing reporting for workers that drain at least one memory.
In `@scripts/benchmarking/README.md`:
- Around line 45-47: Update the benchmark description near the “worker stopped”
and “drains a queue” comparison to limit the full-corpus backlog claim to
upgrades with an existing corpus; do not state that every installation enters
this state, while preserving the existing explanation of the upgrade rebuild
path.
- Around line 54-55: Update the --from-live benchmark setup to create a
consistent SQLite snapshot before copying marm_memory.db and its sidecars, using
sqlite3.backup semantics or explicitly requiring the database to be quiescent
during the copy; preserve the guarantee that the original database is never
modified.
---
Outside diff comments:
In `@marm-mcp-server/tests/test_concept_endpoints.py`:
- Around line 285-316: The test setup for
test_fetch_memory_pages_indexes_sources_and_skips_compaction_summaries uses the
wrong compaction-role condition. Update the hand-built baseline SQL in the
related pagination test to retain rows where compaction_role is NULL or not
'summary', matching _BUILD_ROW_FILTERS, rather than excluding 'source'.
🪄 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 Plus
Run ID: 9cee15d8-0f88-40b4-aee4-a965735cdccb
📒 Files selected for processing (45)
CHANGELOG.mdCONTRIBUTORS.mdREADME.mdRELEASE-NOTES-v2.35.0.mddocs/INSTALL-DOCKER.mddocs/INSTALL-LINUX.mddocs/INSTALL-PLATFORMS.mddocs/INSTALL-WINDOWS.mdmarm-console/artifacts/marm-console/src/components/knowledge/ExplorerTab.tsxmarm-console/artifacts/marm-console/src/hooks/use-marm-queries.tsmarm-console/artifacts/marm-console/src/lib/marm-api.tsmarm-console/artifacts/marm-console/src/lib/marm-types.tsmarm-mcp-server/Dockerfilemarm-mcp-server/README.mdmarm-mcp-server/docker-compose.ymlmarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/console/concept_store.pymarm-mcp-server/marm_mcp_server/console/endpoints/concepts.pymarm-mcp-server/marm_mcp_server/core/concept_build_lock.pymarm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/marm_mcp_server/core/concept_queue.pymarm-mcp-server/marm_mcp_server/core/concept_worker.pymarm-mcp-server/marm_mcp_server/core/memory_db.pymarm-mcp-server/marm_mcp_server/core/memory_delete.pymarm-mcp-server/marm_mcp_server/core/memory_ops.pymarm-mcp-server/marm_mcp_server/core/shutdown_manager.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.pymarm-mcp-server/marm_mcp_server/resources/marm-docs/README.mdmarm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/marm_mcp_server/server_stdio.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pymarm-mcp-server/pyproject.tomlmarm-mcp-server/server.jsonmarm-mcp-server/tests/test_concept_build_pagination.pymarm-mcp-server/tests/test_concept_endpoints.pymarm-mcp-server/tests/test_concept_incremental_build.pymarm-mcp-server/tests/test_concept_queue.pymarm-mcp-server/tests/test_concept_two_process.pymarm-mcp-server/tests/test_concept_worker.pymarm-mcp-server/tests/test_concept_worker_wiring.pymarm-mcp-server/tests/test_console_graph_version.pymarm-mcp-server/tests/test_graph_context.pyscripts/benchmarking/README.mdscripts/benchmarking/performance/bench_concept_worker.py
💤 Files with no reviewable changes (1)
- RELEASE-NOTES-v2.35.0.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (14)
{README.md,marm-mcp-server/README.md,marm-mcp-server/marm_mcp_server/resources/marm-docs/README.md,docs/INSTALL-*.md}
📄 CodeRabbit inference engine (AGENTS.md)
When bumping the version, update the README h1 in all three README variants and the version headers in installation documents.
Files:
docs/INSTALL-DOCKER.mddocs/INSTALL-LINUX.mddocs/INSTALL-WINDOWS.mddocs/INSTALL-PLATFORMS.mdmarm-mcp-server/README.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/README.mdREADME.md
**/*.{py,md,json,toml,yml,yaml,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Use SemVer: MAJOR for breaking changes, MINOR for new tools, parameters, or features, and PATCH for fixes and documentation updates.
Files:
docs/INSTALL-DOCKER.mdmarm-mcp-server/pyproject.tomldocs/INSTALL-LINUX.mddocs/INSTALL-WINDOWS.mdmarm-mcp-server/server.jsonmarm-mcp-server/marm_mcp_server/console/endpoints/concepts.pymarm-mcp-server/docker-compose.ymlmarm-mcp-server/marm_mcp_server/core/memory_delete.pymarm-mcp-server/tests/test_concept_two_process.pymarm-mcp-server/tests/test_concept_build_pagination.pymarm-mcp-server/tests/test_console_graph_version.pymarm-mcp-server/marm_mcp_server/core/memory_db.pyCONTRIBUTORS.mdmarm-mcp-server/marm_mcp_server/core/concept_db.pyCHANGELOG.mdmarm-mcp-server/tests/test_graph_context.pymarm-mcp-server/marm_mcp_server/core/memory_ops.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pymarm-mcp-server/marm_mcp_server/core/shutdown_manager.pymarm-mcp-server/marm_mcp_server/server_stdio.pymarm-mcp-server/marm_mcp_server/server.pyscripts/benchmarking/README.mddocs/INSTALL-PLATFORMS.mdmarm-mcp-server/marm_mcp_server/console/concept_store.pymarm-mcp-server/tests/test_concept_incremental_build.pymarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/tests/test_concept_queue.pymarm-mcp-server/tests/test_concept_worker_wiring.pymarm-mcp-server/marm_mcp_server/core/concept_queue.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/core/concept_build_lock.pymarm-mcp-server/marm_mcp_server/core/concept_worker.pymarm-mcp-server/tests/test_concept_worker.pymarm-mcp-server/tests/test_concept_endpoints.pymarm-mcp-server/README.mdscripts/benchmarking/performance/bench_concept_worker.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.pymarm-mcp-server/marm_mcp_server/resources/marm-docs/README.mdREADME.md
**/*.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:
docs/INSTALL-DOCKER.mddocs/INSTALL-LINUX.mddocs/INSTALL-WINDOWS.mdCONTRIBUTORS.mdCHANGELOG.mdscripts/benchmarking/README.mddocs/INSTALL-PLATFORMS.mdmarm-mcp-server/README.mdmarm-mcp-server/marm_mcp_server/resources/marm-docs/README.mdREADME.md
marm-mcp-server/{pyproject.toml,server.json,Dockerfile,docker-compose.yml}
📄 CodeRabbit inference engine (AGENTS.md)
When bumping the version, update the package version, all three
server.jsonoccurrences, the Docker version label and compose configuration.
Files:
marm-mcp-server/pyproject.tomlmarm-mcp-server/server.jsonmarm-mcp-server/docker-compose.ymlmarm-mcp-server/Dockerfile
marm-mcp-server/{server.py,server_stdio.py,server.json}
📄 CodeRabbit inference engine (AGENTS.md)
When adding or removing an MCP tool, update the endpoint implementation, HTTP route and whitelist, STDIO wrapper or service registration, and the
server.jsontools array.
Files:
marm-mcp-server/server.json
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: Keep changes surgical, touch only what the task requires, match existing style, and preserve behavior during refactors.
All memory writes must use the serialized asynchronous write queue and must not bypass its single worker.
marm_log_entrymust dual-write alog_entriesrow and semantic memory, but semantic-store failure must never fail the log write.
Graph and concept failures must never break the seven core memory tools; graph-aware recall must keep primary memory ranking authoritative and treat graph enrichment as bounded, read-only, and fail-open.
Keep the memory and concept graph SQLite databases isolated and never share connections between their pools.
Writes must succeed when the embedding encoder is unavailable; use the single lazy-loaded 512-dimensionaljinaai/jina-embeddings-v2-small-enencoder behind a lock.
Keep orchestration in its current owner file and extract modules only at real boundaries; avoid speculative abstractions and unnecessary configuration flags.
Use minimal comments only for non-obvious reasons; never add comments that narrate the next line.
Files:
marm-mcp-server/marm_mcp_server/console/endpoints/concepts.pymarm-mcp-server/marm_mcp_server/core/memory_delete.pymarm-mcp-server/marm_mcp_server/core/memory_db.pymarm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/marm_mcp_server/core/memory_ops.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pymarm-mcp-server/marm_mcp_server/core/shutdown_manager.pymarm-mcp-server/marm_mcp_server/server_stdio.pymarm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/marm_mcp_server/console/concept_store.pymarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/marm_mcp_server/core/concept_queue.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/core/concept_build_lock.pymarm-mcp-server/marm_mcp_server/core/concept_worker.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.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/console/endpoints/concepts.pymarm-mcp-server/marm_mcp_server/core/memory_delete.pymarm-mcp-server/tests/test_concept_two_process.pymarm-mcp-server/tests/test_concept_build_pagination.pymarm-mcp-server/tests/test_console_graph_version.pymarm-mcp-server/marm_mcp_server/core/memory_db.pymarm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/tests/test_graph_context.pymarm-mcp-server/marm_mcp_server/core/memory_ops.pymarm-mcp-server/marm_mcp_server/services/runtime_status.pymarm-mcp-server/marm_mcp_server/core/shutdown_manager.pymarm-mcp-server/marm_mcp_server/server_stdio.pymarm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/marm_mcp_server/console/concept_store.pymarm-mcp-server/tests/test_concept_incremental_build.pymarm-mcp-server/marm_mcp_server/__init__.pymarm-mcp-server/tests/test_concept_queue.pymarm-mcp-server/tests/test_concept_worker_wiring.pymarm-mcp-server/marm_mcp_server/core/concept_queue.pymarm-mcp-server/marm_mcp_server/config/settings.pymarm-mcp-server/marm_mcp_server/core/concept_build_lock.pymarm-mcp-server/marm_mcp_server/core/concept_worker.pymarm-mcp-server/tests/test_concept_worker.pymarm-mcp-server/tests/test_concept_endpoints.pyscripts/benchmarking/performance/bench_concept_worker.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.py
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Run tests withpytestfrommarm-mcp-server/; use real FastAPI endpoints and real SQLite, mocking only when it meaningfully speeds testing and matches real behavior with at least 95% fidelity.
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, and usepytest.mark.skiponly for genuinely unavailable dependencies.
Files:
marm-mcp-server/tests/test_concept_two_process.pymarm-mcp-server/tests/test_concept_build_pagination.pymarm-mcp-server/tests/test_console_graph_version.pymarm-mcp-server/tests/test_graph_context.pymarm-mcp-server/tests/test_concept_incremental_build.pymarm-mcp-server/tests/test_concept_queue.pymarm-mcp-server/tests/test_concept_worker_wiring.pymarm-mcp-server/tests/test_concept_worker.pymarm-mcp-server/tests/test_concept_endpoints.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_concept_two_process.pymarm-mcp-server/tests/test_concept_build_pagination.pymarm-mcp-server/tests/test_console_graph_version.pymarm-mcp-server/tests/test_graph_context.pymarm-mcp-server/tests/test_concept_incremental_build.pymarm-mcp-server/tests/test_concept_queue.pymarm-mcp-server/tests/test_concept_worker_wiring.pymarm-mcp-server/tests/test_concept_worker.pymarm-mcp-server/tests/test_concept_endpoints.py
marm-mcp-server/marm_mcp_server/server_stdio.py
📄 CodeRabbit inference engine (AGENTS.md)
STDIO owns the
FastMCPapp and core tool wrappers; graph and concept services must be explicitly registered after core tools sotools/listorder remains stable. Never fork behavior between transports.
Files:
marm-mcp-server/marm_mcp_server/server_stdio.py
marm-mcp-server/marm_mcp_server/server.py
📄 CodeRabbit inference engine (AGENTS.md)
HTTP MCP tools must be registered in the route definitions and
MCP_TOOL_OPERATIONSwhitelist; unlisted tools do not exist over HTTP.
Files:
marm-mcp-server/marm_mcp_server/server.py
marm-mcp-server/marm_mcp_server/{__init__.py,server.py}
📄 CodeRabbit inference engine (AGENTS.md)
When bumping the version, update
__version__and its docstring in__init__.py, plus the server docstring.
Files:
marm-mcp-server/marm_mcp_server/server.pymarm-mcp-server/marm_mcp_server/__init__.py
marm-mcp-server/marm_mcp_server/endpoints/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep endpoint logic in
endpoints/, split by surface, and keep shared helpers incore/. New tools should be implemented in the appropriate endpoint module.
Files:
marm-mcp-server/marm_mcp_server/endpoints/concepts.py
README.md
📄 CodeRabbit inference engine (AGENTS.md)
The root
README.mdis the single source of truth for README content.
Files:
README.md
🧠 Learnings (1)
📚 Learning: 2026-07-31T08:30:32.056Z
Learnt from: Lyellr88
Repo: Lyellr88/marm-memory PR: 125
File: docs/INSTALL-LINUX.md:323-323
Timestamp: 2026-07-31T08:30:32.056Z
Learning: In the Linux and Windows installation documentation, treat http://localhost:8001 as the primary MARM MCP Server endpoint. Do not validate its response contract against the standalone marm-mcp-server/marm_graph/server.py health route, because marm-graph is embedded in the primary marm_mcp_server service.
Applied to files:
docs/INSTALL-LINUX.mddocs/INSTALL-WINDOWS.md
🪛 ast-grep (0.45.0)
marm-mcp-server/tests/test_concept_two_process.py
[error] 40-46: Command coming from incoming request
Context: subprocess.run(
[sys.executable, "-c", script],
capture_output=True,
text=True,
timeout=120,
env=env,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
scripts/benchmarking/performance/bench_concept_worker.py
[info] 143-143: use secrets package over random package
Context: random.Random(i)
Note: [CWE-330] Use of Insufficiently Random Values.
(avoid-random-python)
🪛 OpenGrep (1.26.0)
marm-mcp-server/marm_mcp_server/core/concept_queue.py
[ERROR] 90-92: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 130-137: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
[ERROR] 323-325: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🔇 Additional comments (51)
marm-console/artifacts/marm-console/src/components/knowledge/ExplorerTab.tsx (1)
2-2: LGTM!Also applies to: 97-99
marm-console/artifacts/marm-console/src/hooks/use-marm-queries.ts (1)
1-1: LGTM!Also applies to: 24-24, 297-334
marm-console/artifacts/marm-console/src/lib/marm-api.ts (1)
18-18: LGTM!Also applies to: 209-210
marm-console/artifacts/marm-console/src/lib/marm-types.ts (1)
294-300: LGTM!marm-mcp-server/marm_mcp_server/console/concept_store.py (1)
12-17: LGTM!Also applies to: 61-91
marm-mcp-server/marm_mcp_server/console/endpoints/concepts.py (1)
84-89: LGTM!marm-mcp-server/tests/test_concept_build_pagination.py (1)
18-147: LGTM!marm-mcp-server/tests/test_concept_endpoints.py (2)
55-79: LGTM!Also applies to: 111-123, 319-365, 1022-1025
230-238: 📐 Maintainability & Code QualityNo change needed:
marm_concept_buildcalls_run_buildpositionally, so this stub name change does not affect the tested FastAPI endpoint path.> Likely an incorrect or invalid review comment.marm-mcp-server/tests/test_concept_incremental_build.py (1)
57-374: LGTM!marm-mcp-server/tests/test_concept_queue.py (1)
44-316: LGTM!marm-mcp-server/tests/test_concept_two_process.py (1)
21-53: LGTM!Also applies to: 79-186, 226-291
marm-mcp-server/tests/test_concept_worker.py (1)
19-521: LGTM!marm-mcp-server/tests/test_concept_worker_wiring.py (1)
75-233: LGTM!marm-mcp-server/tests/test_console_graph_version.py (1)
16-107: LGTM!marm-mcp-server/tests/test_graph_context.py (1)
190-215: LGTM!scripts/benchmarking/performance/bench_concept_worker.py (1)
27-386: LGTM!Also applies to: 416-426
CHANGELOG.md (1)
3-7: LGTM!Also applies to: 11-36
CONTRIBUTORS.md (1)
15-15: LGTM!README.md (1)
8-8: LGTM!Also applies to: 89-89, 143-144, 699-702, 868-868, 877-887, 903-903, 982-988, 1065-1081
docs/INSTALL-DOCKER.md (1)
5-5: LGTM!marm-mcp-server/marm_mcp_server/__init__.py (1)
17-20: LGTM!marm-mcp-server/pyproject.toml (1)
7-7: LGTM!marm-mcp-server/server.json (1)
6-6: LGTM!Also applies to: 20-25
marm-mcp-server/docker-compose.yml (1)
8-8: LGTM!Also applies to: 21-21
docs/INSTALL-LINUX.md (1)
5-5: LGTM!Also applies to: 323-323
docs/INSTALL-PLATFORMS.md (1)
1-1: LGTM!docs/INSTALL-WINDOWS.md (1)
5-5: LGTM!Also applies to: 297-297
marm-mcp-server/Dockerfile (1)
76-76: LGTM!marm-mcp-server/README.md (1)
10-10: LGTM!Also applies to: 91-91, 145-146, 701-702, 704-704, 870-870, 879-889, 905-905, 914-914, 984-990, 1067-1083
marm-mcp-server/marm_mcp_server/resources/marm-docs/README.md (1)
1-1: LGTM!Also applies to: 65-65, 119-120, 675-676, 678-678, 844-844, 853-863, 879-879, 888-888, 958-964, 1041-1057
scripts/benchmarking/README.md (1)
11-13: LGTM!Also applies to: 39-43, 49-52, 57-77
marm-mcp-server/marm_mcp_server/config/settings.py (1)
58-84: LGTM!Also applies to: 209-209, 313-371
marm-mcp-server/marm_mcp_server/core/memory_db.py (1)
381-415: LGTM!marm-mcp-server/marm_mcp_server/core/concept_queue.py (2)
81-92: LGTM!Also applies to: 95-143, 315-326
232-281: 🗄️ Data Integrity & IntegrationNo change needed.
Parked rows are reported in queue status and are recovered automatically by later writes via
enqueue; there is no manual retry or build retirement path shown to un-park them.marm-mcp-server/marm_mcp_server/core/concept_build_lock.py (1)
61-93: LGTM!Also applies to: 96-141
marm-mcp-server/marm_mcp_server/core/concept_db.py (1)
24-27: LGTM!Also applies to: 164-173
marm-mcp-server/marm_mcp_server/core/memory_ops.py (1)
15-15: LGTM!Also applies to: 126-130, 270-274, 361-363, 484-486
marm-mcp-server/marm_mcp_server/endpoints/concepts.py (2)
12-16: LGTM!Also applies to: 34-39, 55-69, 98-123, 126-173, 195-249, 268-268, 312-312, 338-357, 451-481, 557-561, 598-680
682-710: 🩺 Stability & AvailabilityNo change needed. The worker holds
concept_build_lock("auto_index", ...)aroundbuild_for_memory_ids, andabortis bound fromlease.lost.marm-mcp-server/marm_mcp_server/core/memory_delete.py (1)
6-7: LGTM!Also applies to: 173-176
marm-mcp-server/marm_mcp_server/core/concept_worker.py (4)
34-71: LGTM!
91-152: LGTM!
226-263: LGTM!
73-89: 🩺 Stability & AvailabilityDo not return from
concept_worker.stop()until in-flightto_threadwork stops using the memory SQLite connection.
to_threadcannot cancel a worker thread once it starts;task.cancel()only cancels theawait.stop()still waits for that await, so this is not the orphan-thread issue described. The remaining concern is whetherconcept_queuehelpers close the sharedmemory.connection_poolrow immediately afterstop()returns while a settlement thread is still running.marm-mcp-server/marm_mcp_server/core/shutdown_manager.py (1)
7-7: LGTM!Also applies to: 71-78
marm-mcp-server/marm_mcp_server/server.py (2)
25-25: LGTM!Also applies to: 68-68
8-8: 📐 Maintainability & Code QualityVersion metadata is consistent.
marm-mcp-server/marm_mcp_server/server_stdio.py (1)
38-38: LGTM!Also applies to: 68-68, 84-90
marm-mcp-server/marm_mcp_server/services/runtime_status.py (1)
18-18: LGTM!Also applies to: 120-141
| `--from-live` copies `~/.marm/marm_memory.db` (and its WAL sidecars) into a temp | ||
| directory and measures against a real corpus. It never writes to the original. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -t f -a 'bench_concept_worker.py' scripts/benchmarking | head -n1)"
test -n "$file"
rg -n -C 8 'from.live|from-live|copy|backup|wal|WAL|sqlite3' "$file"Repository: Lyellr88/marm-memory
Length of output: 4350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -t f -a 'bench_concept_worker.py' scripts/benchmarking | head -n1)"
test -n "$file"
echo "## File excerpt around live copy"
sed -n '80,105p' "$file" | cat -n -v
echo
echo "## Search for SQLite backup API and transaction locking related docs/code"
rg -n -C 3 'sqlite3\.backup|Backup|BEGIN IMMEDIATE|BEGIN EXCLUSIVE|BEGIN DEFERRED|LOCKED\s+database|is_locked|db\.locked|lock_mode|busy_timeout' .Repository: Lyellr88/marm-memory
Length of output: 45878
🌐 Web query:
SQLite WAL mode database locking backup WAL file not copied by operating system file copy during write
💡 Result:
Using operating system file copy commands (such as cp, rsync, or snapshotting) to back up an SQLite database in WAL (Write-Ahead Logging) mode is highly unreliable and generally discouraged because it creates an internally inconsistent database [1][2][3]. In WAL mode, the database state is split across the main database file, the -wal file (containing committed transactions not yet flushed), and the -shm shared-memory file [1][2]. Because these files are updated independently, a naive file copy captures them at different moments [2]. If a write or a checkpoint operation occurs between the copying of the main file and the WAL file, the resulting backup will be corrupted or unusable [4][1][2]. The correct and authoritative ways to back up an SQLite database in WAL mode are: 1. Online Backup API: This is the recommended method [1][2]. SQLite provides an official Online Backup API (available as sqlite3_backup_init/step/finish in C or Connection.backup in languages like Python) that copies the database page-by-page [5][2][6]. It holds a shared read-lock only briefly during each page-copy step and is restart-aware—if a concurrent writer modifies a page that has already been copied, SQLite transparently restarts that part of the process [5][2]. This ensures a consistent point-in-time snapshot without needing to stop database writes [5][6]. 2. Full Checkpoint and Copy: If you must use file-level copying, you must first perform a full checkpoint (e.g., using PRAGMA wal_checkpoint(FULL) or RESTART) to flush all data from the WAL file into the main database file [1][3]. Once the checkpoint is complete and the WAL file is empty/truncated, you can safely copy the main database file [3]. To ensure no new WAL data is written during the copy, all writing connections must be closed or prevented from writing until the copy is finished [3]. 3. Volume Shadow Copy (VSS/Snapshots): On systems that support it, file-level snapshots (like Windows VSS or APFS snapshots) can provide a consistent point-in-time copy of the entire file system or volume, including all associated database files, provided the snapshot mechanism is truly atomic [3]. However, simply relying on standard file-copy utilities without such infrastructure is unsafe [1][2].
Citations:
- 1: https://greyforge.tech/chronicles/sqlite-checkpoint-atomic-backups
- 2: https://www.productionhardening.org/backup-recovery-data-integrity/
- 3: https://sqlite.org/forum/forumpost/f173a78e2e9517c2
- 4: https://www.sqlite.org/wal.html
- 5: https://sqlite.org/backup.html
- 6: https://www2.sqlite.org/c3ref/backup_finish.html
Use a consistent SQLite snapshot before copying marm_memory.db.
File-level copies of marm_memory.db, -wal, and -shm can capture independent points in time if MARM writes, which may omit committed WAL frames or produce an unusable benchmark corpus. Use sqlite3.backup semantics or require the database to be quiescent before copying.
🤖 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 `@scripts/benchmarking/README.md` around lines 54 - 55, Update the --from-live
benchmark setup to create a consistent SQLite snapshot before copying
marm_memory.db and its sidecars, using sqlite3.backup semantics or explicitly
requiring the database to be quiescent during the copy; preserve the guarantee
that the original database is never modified.
Source: Path instructions
Ten valid findings, one refuted, one already handled. Concurrency: - stop() now signals the in-flight build before cancelling. Cancelling only cancels the await around asyncio.to_thread; the extraction thread kept writing while unwinding released the cross-process graph lock, so another transport could reset the concept database underneath it. - The lock heartbeat gave up only when a renewal was refused. A renewal that kept raising left the holder writing past its own expiry, which is the collision the lock exists to prevent. It now abandons at the TTL either way. Schema: - The reset no longer stamps the new version. A rebuild that died partway left an empty graph reporting current, so nothing prompted for the rebuild again and the corpus was silently absent with no queue rows to recover it. The version is written by the build that finishes, via mark_schema_current(). - A doc resave now retracts provenance first, matching the Console replace path. Indexing only adds, so repeated saves accumulated concepts from doc versions that no longer existed. Tests: - The two-process TTL test ran a synchronous subprocess on the event loop, blocking the heartbeat it was trying to prove. It goes through a thread now. - Settings reload tests pin every path they resolve at tmp_path instead of relying on the session HOME sandbox. - The console-delete cleanup test accepted status="skipped", which a missing concept database returns. It now asserts the provenance actually went. Benchmark: - Zero indexed memories reported "worker was busy throughout" and presented the deltas as valid. That is exactly how the superseded-hash bug produced a believable but meaningless run. - --from-live uses sqlite3 backup() instead of copying the database and its WAL sidecars separately, which could capture three different instants. Docs: package README section numbering was left broken when the indexing benchmark section was removed, and manual mode still records queue rows, which was undocumented. Refuted: Codex's claim that Console replace never retracts provenance. endpoints/memory.py:154 already does, before the queued reindex runs. Merges are deliberate, since appended content keeps its earlier entities valid.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/concept_db.py`:
- Around line 278-279: Update the reset flow around init_concept_database() and
clear_schema_marker() so database initialization does not recreate the schema
marker after entities are dropped. Ensure the marker remains absent throughout
rebuilding, and only write it via mark_schema_current() once the complete build
succeeds.
In `@marm-mcp-server/marm_mcp_server/services/notebook.py`:
- Around line 261-272: The resave cleanup currently runs after
store_doc_mirror() queues indexing, allowing it to delete newly indexed
entities. Move the best-effort _cleanup_deleted_concepts_async call before
store_doc_mirror() commits the replacement, and pass doc_row.memory_id rather
than memory_id; preserve the existing exception handling and only apply this to
resaves.
🪄 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 Plus
Run ID: 641adc7d-c0f8-4dda-abcf-3342e566281e
📒 Files selected for processing (16)
CHANGELOG.mdREADME.mdmarm-mcp-server/README.mdmarm-mcp-server/marm_mcp_server/core/concept_build_lock.pymarm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/marm_mcp_server/core/concept_worker.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.pymarm-mcp-server/marm_mcp_server/resources/marm-docs/README.mdmarm-mcp-server/marm_mcp_server/services/notebook.pymarm-mcp-server/tests/test_concept_incremental_build.pymarm-mcp-server/tests/test_concept_two_process.pymarm-mcp-server/tests/test_concept_worker.pymarm-mcp-server/tests/test_concept_worker_wiring.pymarm-mcp-server/tests/test_graph_context.pyscripts/benchmarking/README.mdscripts/benchmarking/performance/bench_concept_worker.py
🚧 Files skipped from review as they are similar to previous changes (12)
- CHANGELOG.md
- marm-mcp-server/tests/test_concept_worker_wiring.py
- scripts/benchmarking/README.md
- marm-mcp-server/tests/test_concept_incremental_build.py
- marm-mcp-server/marm_mcp_server/core/concept_build_lock.py
- marm-mcp-server/tests/test_concept_two_process.py
- README.md
- marm-mcp-server/marm_mcp_server/core/concept_worker.py
- marm-mcp-server/tests/test_concept_worker.py
- marm-mcp-server/marm_mcp_server/endpoints/concepts.py
- scripts/benchmarking/performance/bench_concept_worker.py
- marm-mcp-server/marm_mcp_server/resources/marm-docs/README.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: Keep changes surgical, touch only what the task requires, match existing style, and preserve behavior during refactors.
All memory writes must use the serialized asynchronous write queue and must not bypass its single worker.
marm_log_entrymust dual-write alog_entriesrow and semantic memory, but semantic-store failure must never fail the log write.
Graph and concept failures must never break the seven core memory tools; graph-aware recall must keep primary memory ranking authoritative and treat graph enrichment as bounded, read-only, and fail-open.
Keep the memory and concept graph SQLite databases isolated and never share connections between their pools.
Writes must succeed when the embedding encoder is unavailable; use the single lazy-loaded 512-dimensionaljinaai/jina-embeddings-v2-small-enencoder behind a lock.
Keep orchestration in its current owner file and extract modules only at real boundaries; avoid speculative abstractions and unnecessary configuration flags.
Use minimal comments only for non-obvious reasons; never add comments that narrate the next line.
Files:
marm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/marm_mcp_server/services/notebook.py
**/*.{py,md,json,toml,yml,yaml,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Use SemVer: MAJOR for breaking changes, MINOR for new tools, parameters, or features, and PATCH for fixes and documentation updates.
Files:
marm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/marm_mcp_server/services/notebook.pymarm-mcp-server/tests/test_graph_context.pymarm-mcp-server/README.md
**/*.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/concept_db.pymarm-mcp-server/marm_mcp_server/services/notebook.pymarm-mcp-server/tests/test_graph_context.py
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Run tests withpytestfrommarm-mcp-server/; use real FastAPI endpoints and real SQLite, mocking only when it meaningfully speeds testing and matches real behavior with at least 95% fidelity.
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, and usepytest.mark.skiponly for genuinely unavailable dependencies.
Files:
marm-mcp-server/tests/test_graph_context.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_graph_context.py
{README.md,marm-mcp-server/README.md,marm-mcp-server/marm_mcp_server/resources/marm-docs/README.md,docs/INSTALL-*.md}
📄 CodeRabbit inference engine (AGENTS.md)
When bumping the version, update the README h1 in all three README variants and the version headers in installation documents.
Files:
marm-mcp-server/README.md
**/*.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:
marm-mcp-server/README.md
🔇 Additional comments (4)
marm-mcp-server/README.md (3)
10-10: LGTM!Also applies to: 91-91, 145-146
161-161: LGTM!Also applies to: 205-205, 217-217
701-704: LGTM!Also applies to: 870-870, 879-889, 905-905, 914-914, 984-990
marm-mcp-server/tests/test_graph_context.py (1)
11-11: LGTM!Also applies to: 186-195, 196-221, 224-253, 279-282
Both valid, both narrow, both in code added by this branch. - The reset wrote the schema version and deleted it again a moment later. init_concept_database takes mark_current now and the reset passes False, so the marker is never present on an unbuilt graph. Before this, a crash between the two calls, or another process reading the schema state in between, saw an empty graph reported as current and never asked for the rebuild again. clear_schema_marker is gone with its only caller. - Doc resave cleanup ran after store_doc_mirror had already queued the replacement, and passed the id that call returned. Two problems: the returned id is a new one when the previous mirror row was deleted, so the stale provenance under the old id survived while the cleanup did nothing useful; and running after the enqueue can delete entities the worker has already written for the new content, leaving the queue row settled with nothing to re-index it. Cleanup now runs before the write, keyed on the previous id.
All three findings valid. The first was my incomplete fix, not a new issue. - stop() set the abort flag and then cancelled, which unwound the lock and released it while the extraction thread was still writing. The flag shrank the window to one memory but did not close it. _run_build now sets a finished event in a finally, the only place that can know the thread stopped, and stop() waits on it for ABORT_GRACE_SECONDS before releasing. Bounded on purpose: waiting out a full extraction would put spaCy back on the teardown path that v2.35.0 exists to bound. Past the grace the lock is released anyway, and the residual risk is stray entities or a logged write failure in a graph another process now owns, not corruption. - Console replace cleaned old provenance after queuing the replacement, the same ordering just fixed for promoted-doc resaves. Cleanup now runs first, guarded on the memory existing, which that path does not need: a replace against a missing id is an ordinary 404, and cleaning first would strip a live memory's provenance on the way to returning one. - The pagination baseline still excluded compaction sources while production excludes summaries. Seeded rows are all NULL so it masked nothing, but the regression baseline disagreed with the code exactly where this feature changed behavior. The stop() contract test was asserting an instant return. It now asserts the bound instead, and that a build ignoring the abort is not waited out.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
marm-mcp-server/marm_mcp_server/endpoints/concepts.py (1)
218-236: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
finishedis not signalled on every exit path of the build call chain.ConceptWorker.stop()waits up toABORT_GRACE_SECONDS + 1wheneverself._build_finishedis unset, so any path that returns or raises without setting the event adds a pointless bounded delay to shutdown. The contract stated on Line 356 ofmarm-mcp-server/marm_mcp_server/endpoints/concepts.pyis that the event always marks the end of graph writing.
marm-mcp-server/marm_mcp_server/endpoints/concepts.py#L218-L236: move_get_concept_db()andis_graph_available()inside thetryso asqlite3.ErrororOSErrorduring setup still reaches thefinallythat setsfinished.marm-mcp-server/marm_mcp_server/endpoints/concepts.py#L725-L737: setfinishedin afinallyaround the wholebuild_for_memory_idsbody so the empty-batch return, therebuild_requiredraise, and the unavailable-database raise all signal 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/endpoints/concepts.py` around lines 218 - 236, Ensure the build completion event is signalled on every exit path: in concepts.py lines 218-236, move _get_concept_db() and is_graph_available() inside the try so setup errors reach the existing completion finally; in concepts.py lines 725-737, wrap the entire build_for_memory_ids body in a finally that sets finished, covering empty-batch returns and all raised errors.
🤖 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_concept_worker.py`:
- Around line 458-469: In the slow_build work function, update the completion
sequence so still_running.clear() executes before finished.set(). Keep the
existing delay and abort behavior unchanged, ensuring the worker no longer
reports completion until it has stopped marking itself as active.
---
Outside diff comments:
In `@marm-mcp-server/marm_mcp_server/endpoints/concepts.py`:
- Around line 218-236: Ensure the build completion event is signalled on every
exit path: in concepts.py lines 218-236, move _get_concept_db() and
is_graph_available() inside the try so setup errors reach the existing
completion finally; in concepts.py lines 725-737, wrap the entire
build_for_memory_ids body in a finally that sets finished, covering empty-batch
returns and all raised errors.
🪄 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 Plus
Run ID: 9c7b4c7c-5fa2-4f88-8020-fb5ca50d997c
📒 Files selected for processing (8)
marm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/marm_mcp_server/core/concept_worker.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.pymarm-mcp-server/marm_mcp_server/endpoints/memory.pymarm-mcp-server/marm_mcp_server/services/notebook.pymarm-mcp-server/tests/test_concept_build_pagination.pymarm-mcp-server/tests/test_concept_worker.pymarm-mcp-server/tests/test_graph_context.py
🚧 Files skipped from review as they are similar to previous changes (3)
- marm-mcp-server/tests/test_graph_context.py
- marm-mcp-server/tests/test_concept_build_pagination.py
- marm-mcp-server/marm_mcp_server/core/concept_worker.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
marm-mcp-server/marm_mcp_server/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/marm_mcp_server/**/*.py: Keep changes surgical, touch only what the task requires, match existing style, and preserve behavior during refactors.
All memory writes must use the serialized asynchronous write queue and must not bypass its single worker.
marm_log_entrymust dual-write alog_entriesrow and semantic memory, but semantic-store failure must never fail the log write.
Graph and concept failures must never break the seven core memory tools; graph-aware recall must keep primary memory ranking authoritative and treat graph enrichment as bounded, read-only, and fail-open.
Keep the memory and concept graph SQLite databases isolated and never share connections between their pools.
Writes must succeed when the embedding encoder is unavailable; use the single lazy-loaded 512-dimensionaljinaai/jina-embeddings-v2-small-enencoder behind a lock.
Keep orchestration in its current owner file and extract modules only at real boundaries; avoid speculative abstractions and unnecessary configuration flags.
Use minimal comments only for non-obvious reasons; never add comments that narrate the next line.
Files:
marm-mcp-server/marm_mcp_server/services/notebook.pymarm-mcp-server/marm_mcp_server/endpoints/memory.pymarm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.py
**/*.{py,md,json,toml,yml,yaml,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Use SemVer: MAJOR for breaking changes, MINOR for new tools, parameters, or features, and PATCH for fixes and documentation updates.
Files:
marm-mcp-server/marm_mcp_server/services/notebook.pymarm-mcp-server/marm_mcp_server/endpoints/memory.pymarm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.pymarm-mcp-server/tests/test_concept_worker.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/services/notebook.pymarm-mcp-server/marm_mcp_server/endpoints/memory.pymarm-mcp-server/marm_mcp_server/core/concept_db.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.pymarm-mcp-server/tests/test_concept_worker.py
marm-mcp-server/marm_mcp_server/endpoints/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Keep endpoint logic in
endpoints/, split by surface, and keep shared helpers incore/. New tools should be implemented in the appropriate endpoint module.
Files:
marm-mcp-server/marm_mcp_server/endpoints/memory.pymarm-mcp-server/marm_mcp_server/endpoints/concepts.py
marm-mcp-server/tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
marm-mcp-server/tests/**/*.py: Run tests withpytestfrommarm-mcp-server/; use real FastAPI endpoints and real SQLite, mocking only when it meaningfully speeds testing and matches real behavior with at least 95% fidelity.
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, and usepytest.mark.skiponly for genuinely unavailable dependencies.
Files:
marm-mcp-server/tests/test_concept_worker.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_concept_worker.py
🔇 Additional comments (9)
marm-mcp-server/marm_mcp_server/services/notebook.py (1)
241-259: LGTM!marm-mcp-server/marm_mcp_server/endpoints/memory.py (1)
139-165: LGTM!marm-mcp-server/marm_mcp_server/endpoints/concepts.py (1)
688-702: LGTM!marm-mcp-server/marm_mcp_server/core/concept_db.py (3)
43-50: LGTM!
170-179: LGTM!
261-285: LGTM!marm-mcp-server/tests/test_concept_worker.py (3)
152-159: LGTM!Also applies to: 179-188, 286-291
375-408: LGTM!
411-441: 🩺 Stability & AvailabilityNo change needed.
stop()has a fixed 3-second bounded timeout, so this test does not “pay” the fullABORT_GRACE_SECONDSas an unbounded teardown step.> Likely an incorrect or invalid review comment.
| async def slow_build(memory_ids, abort=None, finished=None): | ||
| # Stands in for a thread that notices the abort and then takes a | ||
| # moment to unwind, which is when the lock must still be held. | ||
| def work(): | ||
| abort.wait(5) | ||
| time.sleep(0.4) | ||
| if finished is not None: | ||
| finished.set() | ||
| still_running.clear() | ||
|
|
||
| await asyncio.to_thread(work) | ||
| return {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reorder finished.set() and still_running.clear() to remove a flake window.
stop() unblocks as soon as finished is set. still_running.clear() runs after that on the worker thread. Between the two statements, stop() can cancel the task, unwind the lock context, and call watching_release while still_running is still set. The test then records a false violation.
The window is small, so the test passes on an idle machine. On a loaded CI runner the worker thread can be descheduled between the two statements.
Clear still_running first. The event models "the thread is still writing", so it must be cleared before the thread announces it has finished.
🔧 Proposed fix
def work():
abort.wait(5)
time.sleep(0.4)
+ still_running.clear()
if finished is not None:
finished.set()
- still_running.clear()As per path instructions: "Focus on tests that are flaky, non-isolated, incorrectly asserting behavior, or missing coverage for a changed high-risk path."
📝 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.
| async def slow_build(memory_ids, abort=None, finished=None): | |
| # Stands in for a thread that notices the abort and then takes a | |
| # moment to unwind, which is when the lock must still be held. | |
| def work(): | |
| abort.wait(5) | |
| time.sleep(0.4) | |
| if finished is not None: | |
| finished.set() | |
| still_running.clear() | |
| await asyncio.to_thread(work) | |
| return {} | |
| async def slow_build(memory_ids, abort=None, finished=None): | |
| # Stands in for a thread that notices the abort and then takes a | |
| # moment to unwind, which is when the lock must still be held. | |
| def work(): | |
| abort.wait(5) | |
| time.sleep(0.4) | |
| still_running.clear() | |
| if finished is not None: | |
| finished.set() | |
| await asyncio.to_thread(work) | |
| return {} |
🤖 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_concept_worker.py` around lines 458 - 469, In the
slow_build work function, update the completion sequence so
still_running.clear() executes before finished.set(). Keep the existing delay
and abort behavior unchanged, ensuring the worker no longer reports completion
until it has stopped marking itself as active.
Source: Path instructions
1 participant
v2.36.0: Automatic Concept Graph Indexing
The concept graph builds itself now. Storing a memory queues it, and a background worker turns it into a node about 30 seconds later on both transports, so the graph stops being stale until someone remembers to run a build.
marm_concept_buildremains for rebuilds and for indexing memories written before this release.CONCEPT_AUTO_INDEX=falseturns automation off.CONCEPT_BUILD_ROW_CAPis a page size, not a ceiling.build_in_progress.scripts/benchmarking/performance/bench_concept_worker.py --from-live, two runs): recall median moves from 8.3-8.5ms to 15.0-17.7ms, writes are unaffected. The cause is CPU contention from entity extraction, not lock waiting. It applies only while a backlog is draining, not in steady state.Upgrade Note
This release requires one graph rebuild. Existing graphs hold entities extracted from compaction summaries that the new rule would never produce, and there is no way to remove only those. MARM reports
rebuild_requireduntil you run:marm_concept_build(search_all=True)
The old graph is backed up beside the database first. The build clears the queue it covered, so the worker does not immediately re-extract the same corpus.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation