fix(store): link brain_store writes to MCP sessions + make deferred BrainBar stores idempotent (items 6+7) - #641
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_04e6202c-e517-4538-bd46-9278955a7c0b) |
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟠 High
content_hash is added only in the CREATE TABLE for fresh databases; existing databases never get the column. Because ensureSchema is skipped when chunks already exists and ensureChunkColumns doesn't add it, writes that reference content_hash fail with SQLite's "no such column: content_hash" instead of storing the memory. Add an ALTER TABLE chunks ADD COLUMN content_hash TEXT migration in ensureChunkColumns for existing databases.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @brain-bar/Sources/BrainBar/BrainDatabase.swift around line 817:
`content_hash` is added only in the `CREATE TABLE` for fresh databases; existing databases never get the column. Because `ensureSchema` is skipped when `chunks` already exists and `ensureChunkColumns` doesn't add it, writes that reference `content_hash` fail with SQLite's "no such column: content_hash" instead of storing the memory. Add an `ALTER TABLE chunks ADD COLUMN content_hash TEXT` migration in `ensureChunkColumns` for existing databases.
There was a problem hiding this comment.
🟡 Medium
When brain_store is called from a session whose content duplicates an existing chunk, the conversation_id is silently dropped. store_memory only assigns conversation_id and position in its new-row branch; the find_duplicate / merge_duplicate_chunk path discards the incoming session entirely. The store call succeeds and returns the old canonical chunk ID, but no row is created in the current session — so session expansion/recall omits this store and the chunk stays associated with the earlier session (or NULL). Consider propagating conversation_id (and recomputing position) when merging a duplicate, so the canonical chunk is linked to the producing session.
Also found in 1 other location(s)
src/brainlayer/store.py:351
conversation_idis applied only in the new-row branch. Iffind_duplicatereturns an existing canonical chunk (for example, the same note is stored in a later MCP session),merge_duplicate_chunkruns instead and the incoming session and position are discarded. The successfulbrain_storecall therefore does not appear in the producing session's context/recall, and the returned chunk remains associated with the earlier session (or withNULL).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/brainlayer/mcp/store_handler.py around line 1:
When `brain_store` is called from a session whose content duplicates an existing chunk, the `conversation_id` is silently dropped. `store_memory` only assigns `conversation_id` and `position` in its new-row branch; the `find_duplicate` / `merge_duplicate_chunk` path discards the incoming session entirely. The store call succeeds and returns the old canonical chunk ID, but no row is created in the current session — so session expansion/recall omits this store and the chunk stays associated with the earlier session (or `NULL`). Consider propagating `conversation_id` (and recomputing `position`) when merging a duplicate, so the canonical chunk is linked to the producing session.
Also found in 1 other location(s):
- src/brainlayer/store.py:351 -- `conversation_id` is applied only in the new-row branch. If `find_duplicate` returns an existing canonical chunk (for example, the same note is stored in a later MCP session), `merge_duplicate_chunk` runs instead and the incoming session and position are discarded. The successful `brain_store` call therefore does not appear in the producing session's context/recall, and the returned chunk remains associated with the earlier session (or with `NULL`).
| // DEFERRED is a durable success receipt, but a client may still retry it. | ||
| // Reuse the exact-content row within this server-owned session so replay plus retry | ||
| // cannot double-store, while distinct legacy queue entries remain distinct. | ||
| if let conversationID { |
There was a problem hiding this comment.
🟡 Medium BrainBar/BrainDatabase.swift:1330
The session-level dedupe in store treats any later write with the same contentHash, project, and conversationID as a retry of the original, returning the existing StoredChunk instead of inserting a new row. So storing the same short message twice in one conversation silently drops the second write — including its tags, importance, source, timestamp, and position — and returns the first row's identity. The dedupe lookup has no queueID in its match criteria, so it cannot distinguish a genuine retry/replay from a legitimately repeated same-content store. Consider including queueID (or another retry identifier) in the dedupe match so only actual replays collapse, not distinct same-content writes.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @brain-bar/Sources/BrainBar/BrainDatabase.swift around line 1330:
The session-level dedupe in `store` treats any later write with the same `contentHash`, `project`, and `conversationID` as a retry of the original, returning the existing `StoredChunk` instead of inserting a new row. So storing the same short message twice in one conversation silently drops the second write — including its tags, importance, source, timestamp, and position — and returns the first row's identity. The dedupe lookup has no `queueID` in its match criteria, so it cannot distinguish a genuine retry/replay from a legitimately repeated same-content store. Consider including `queueID` (or another retry identifier) in the dedupe match so only actual replays collapse, not distinct same-content writes.
![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: 206b570a35
ℹ️ 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".
| INSERT INTO chunks (id, content, metadata, source_file, project, tags, importance, source, content_type, char_count, created_at, preview_text, conversation_id, position, content_hash) | ||
| VALUES (?, ?, ?, 'brainbar-store', ?, ?, ?, ?, 'user_message', ?, ?, ?, ?, ?, ?) |
There was a problem hiding this comment.
When BrainBar opens a database created by the previous release, chunks has no content_hash column: existing databases skip ensureSchema(), and ensureChunkColumns() never adds this newly required column. Consequently every brain_store reaches this INSERT and fails with no column named content_hash rather than storing or deferring the write. Add the column in the existing-database migration path before relying on it.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
| if let existing = try storedChunk( | ||
| contentHash: contentHash, | ||
| project: project, | ||
| contentType: "user_message", | ||
| conversationID: conversationID, |
There was a problem hiding this comment.
When two identical brain_store calls from one session are deferred before the queue drains, they receive different promised chunk IDs, but flushing the second entry finds the first row by content hash and returns it here; the second queue line is then removed even though its promised ID was never persisted. The same lookup also silently discards changed tags or importance on a later exact-content call. Deduplication must use a retry identity such as the queue/chunk ID, or otherwise preserve each durable receipt and merge the incoming metadata.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
The 7 admin merges of 2026-08-04 (#645 #644 #643 #641 #633 #605 #602) bypassed CI, landing import-order and formatting drift on main. Every PR's lint job inherits it. Rebased onto #647 (949334a), which cleared the watchdog test blocking this push. ruff check --fix + ruff format only; no behavioural change.
1 participant
Sprint lane R1-C of the BrainLayer cleanup sprint (
docs.local/plan/cleanup-2026-08-02/). Items 6 + 7, paired because both are defects in the same store write path.What was broken
conversation_idNULL on every store. Measured 11,181 / 11,181 = 100% NULL. Stored memories could not be tied back to the session that produced them.content_hashgroups. The retry path re-inserted instead of reconciling.Commits
f631dfb4fix: link brain_store writes to MCP sessions206b570afix: make deferred BrainBar stores idempotentVerification
Proven against a rebuilt daemon on an isolated socket — not a mocked path. Per the sprint's standing rule, mock-green is not live-green.
Review status — READ BEFORE MERGING
🛑 This PR has NO routed review yet. It is pushed and open so it can be reviewed, not because it is ready to merge. Per sprint law an ACCEPT binds to an exact commit (
206b570a), never to a PR number, and nobody reviews their own code.hooks/dedup_coordination.py:33pins_COORD_DIR = "/tmp"against a constant test session id, so concurrent runs read each other's coordination file). Checkpgrep -f pytestbefore pushing.Gate passed serially at
206b570a.Note
Medium Risk
Touches the primary memory write path and deduplication semantics; incorrect idempotency or session binding could drop or mis-order stored chunks, but changes are narrow and heavily regression-tested.
Overview
MCP
brain_storewrites are tied to a server-owned session, not client-suppliedconversation_id. SwiftPaletteSessionand Python_calling_session_id()assign one stable ID per MCP connection; that value flows through direct writes, busy queues, and drain so chunks getconversation_idand monotonicpositionfor ordered before/after expansion.BrainBar
store()is idempotent within a session: whenconversation_idis set, it hashes body content and returns an existing active chunk on replay (same hash, project, type, session) instead of inserting again—covering DEFERRED receipt retries after flush. Pending-store JSONL carriesconversation_idend-to-end.The Python stack mirrors the same plumbing (
store_memory, queue IO, drain, MCP handlers). Tests assert client-forged IDs are ignored, retry does not create a second row, and expand context follows store order.Reviewed by Cursor Bugbot for commit 206b570. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Link brain_store writes to MCP session IDs and make deferred store operations idempotent
PaletteSession(Swift) and MCP connection (Python) now generates a stable server-ownedconversation_id, overriding any client-supplied value, so allbrain_storewrites within a session are tagged to that session.positionperconversation_id, enabling ordered session context and expand-by-neighbor lookups.conversation_idin the queue file so that on retry/drain the chunk is linked to the originating session rather than creating a duplicate.content_hashcolumn and deduplication query inBrainDatabase.swiftprevent duplicate chunks when a deferred store is retried within the same session.conversation_idarguments supplied by MCP clients are silently ignored; the server-assigned value is always used.📊 Macroscope summarized 206b570. 7 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.