Skip to content

feat(mcp): ChatGPT/Claude connector search and fetch over the memory bank (#4862)#8492

Open
ZachL111 wants to merge 9 commits into
BasedHardware:mainfrom
ZachL111:zach/mcp-chatgpt-connector
Open

feat(mcp): ChatGPT/Claude connector search and fetch over the memory bank (#4862)#8492
ZachL111 wants to merge 9 commits into
BasedHardware:mainfrom
ZachL111:zach/mcp-chatgpt-connector

Conversation

@ZachL111

@ZachL111 ZachL111 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Advances #4862 (Omi needs to become an easily integratable memory bank for chatgpt/claude), in the "Great, sharable product" milestone.

Claude already speaks MCP, so the Omi MCP server's existing tools work there directly. ChatGPT is the gap: OpenAI's ChatGPT connectors and deep-research models consume an MCP server through a standard pair of read-only tools, search and fetch, with a fixed result shape. Omi only exposed domain-specific search tools (search_memories, search_conversations), so it was not directly usable as a ChatGPT connector. This adds the standard contract over the user's memory bank.

What this adds

Two new MCP tools following OpenAI's connector contract:

  • search(query): unified search across the user's memories and conversations (the two richest text sources), returning {results: [{id, title, url, text}]}. Result ids are namespaced (memory:<id> / conversation:<id>).
  • fetch(id): returns the full document for a result id as {id, title, text, url, metadata}.

Both are read-only. The tools/call response echoes search and fetch results as structuredContent alongside the JSON text, which the connector contract expects. Matching REST endpoints (GET /v1/mcp/search, GET /v1/mcp/fetch) are added for parity.

Design

  • utils/mcp_search.py is the single orchestration both transports call. It reuses the existing vector search (find_similar_memories, query_vectors), applies the same safety filters search_memories already uses (never surfaces rejected, locked, or superseded facts, and skips locked conversations), interleaves the two sources so both are represented, and namespaces ids so fetch knows which store to read.
  • fetch maps a missing item to 404 / -32001 and a locked item to 402 / -32002, and validates the id and query types so a non-string argument returns a clean error instead of an uncaught one.
  • Citation urls use the existing h.omi.me web host (for example https://h.omi.me/conversations/<id>).

Combined with the read and write tools already on the server (memories, conversations, action items, goals, people, folders, screen activity, daily summaries), this makes the Omi MCP server a drop-in memory bank for both Claude (native MCP) and ChatGPT (search/fetch connector).

Testing

Added 14 tests to tests/unit/test_mcp_data_endpoints.py (already wired into test.sh): search merges and ranks both sources, the safety filters drop locked and rejected items, fetch returns the right shape for memories and conversations, not-found is 404 / -32001, locked is 402 / -32002, bad and non-string ids are rejected, the tools are registered, and tools/call emits structuredContent for search. The full file passes (31 tests), and the adjacent MCP tests still pass. Black and the async-blocker lint are clean.

Review in cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 4 files

Confidence score: 2/5

  • In backend/routers/mcp.py, the search endpoint puts sensitive query text in GET URL parameters, which can bypass existing PII log sanitization and leak user data via access logs/history; this is the highest-risk issue in the PR — move the payload to a sanitized POST body (or equivalent) before merging.
  • In _search_conversations in backend/utils/mcp_search.py, using only structured.overview for result text can return empty previews even when transcripts exist, so users may see blank/low-quality search results while fetch returns content — add transcript fallback logic for preview text before merging.
  • Also in _search_conversations (backend/utils/mcp_search.py), not over-fetching candidates when top-k items are locked can under-fill returned results and make search appear incomplete — over-fetch and then filter (as done for memories) to keep result counts stable.
  • In ID parsing paths in backend/utils/mcp_search.py, prefixed-but-empty IDs like memory: are treated as missing items (404) instead of invalid requests (400), and backend/tests/unit/test_mcp_data_endpoints.py does not exercise rejected-item filtering as named, increasing behavior drift risk — add explicit empty-ID validation and extend the test to cover rejected filtering before merging.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread backend/routers/mcp.py Outdated
Comment thread backend/utils/mcp_search.py Outdated
Comment thread backend/utils/mcp_search.py
Comment thread backend/utils/mcp_search.py Outdated
Comment thread backend/tests/unit/test_mcp_data_endpoints.py
@ZachL111

Copy link
Copy Markdown
Contributor Author

Thanks cubic, all five were valid and are fixed.

  1. (P1) Search query in the GET URL: moved the REST search endpoint to POST so the query travels in the request body and never lands in URL-based access logs. fetch stays GET since its id is an opaque memory:/conversation: reference. The actual ChatGPT connector path is the SSE tools/call, which already carries the query in the JSON-RPC body, so this only tightens the REST parity endpoint.

  2. (P2) Conversation preview used only structured.overview: added a fallback so the search stub text uses the overview, then a transcript snippet, then the title. A conversation with segments but no overview now has a non-empty preview that foreshadows what fetch returns.

  3. (P2) Conversation search did not over-fetch: it now over-fetches candidates (limit times 3, capped at 60) and returns up to limit after dropping locked items, the same way the memory path works, so locked hits no longer under-fill the result count.

  4. (P2) Empty raw id like memory: or conversation:: these now raise InvalidRequest (400 / -32602) before any database call, instead of being treated as not-found.

  5. (P2) Test coverage: the filter test now includes a rejected memory (user_review false) and asserts it is dropped alongside the locked memory and locked conversation; there is a new test that a conversation preview falls back to transcript text; and the empty-id cases are now exercised. Full file passes (32 tests), black and async-lint clean. Pushed in 4050997.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks for this, @ZachL111 — really clean work and squarely on-mission for #4862 (making Omi's memory bank directly consumable by ChatGPT/Claude via the standard MCP search/fetch connector contract). The implementation is coherent, well-scoped, and well-tested.

I did a static pass over the full diff against main and a couple of things are worth surfacing:

Verified good

  • The memory safety filter in _search_memories (user_review is False | is_locked | invalid_at is not None) matches the existing search_memories dispatch exactly — rejected/locked/superseded facts don't surface.
  • fetch is uid-scoped throughout (get_memory(uid, …), get_conversation(uid, …)), so there's no cross-user read path.
  • Moving the REST search endpoint to POST so the query stays out of URL/access logs is a good privacy call.
  • Locked conversations are dropped in search and raise a paywall (402 / -32002) in fetch — stricter than search_conversations, which is the right posture for a connector.

One consistency gap worth hardening (low severity)
search hides memories where user_review is False or invalid_at is not None, but fetch("memory:<id>") only checks is_locked. So a rejected/superseded memory's full text is reachable via fetch even though search hides it. In practice this is low impact — access is uid-scoped, ids are non-enumerable UUIDs, and a ChatGPT connector only ever learns ids from search (which already filters them) — but the two entry points really shouldn't disagree. Suggested fix: factor the visibility check into a shared helper (e.g. _memory_visible(mem)) used by both _search_memories and the memory branch of fetch, and add a test that fetch on a rejected memory behaves consistently with search.

Status
Flagging this for security + maintainer review before merge rather than approving — it adds a read path over users' memories and conversations (full-text fetch) plus a combined memories.read + conversations.read scope, so I'd want a maintainer sign-off on the data-access direction in addition to the code looking correct. Not blocking on the gap above; it can land as a follow-up.

@Git-on-my-level Git-on-my-level added feature-fit-review PR touches feature direction; qualitative fit assessed security-review Needs security/privacy review needs-maintainer-review Needs a human maintainer to review/approve (e.g. stacked, product, or architecture judgment) labels Jun 28, 2026
@ZachL111

Copy link
Copy Markdown
Contributor Author

Thanks David, good catch on the search/fetch disagreement. search hid rejected and superseded memories but fetch only checked the lock, so a rejected fact's full text was reachable by id. I factored the check into a shared _memory_visible(mem) helper used by both _search_memories and the memory branch of fetch: a rejected (user_review is False) or superseded (invalid_at set) memory now returns not-found from fetch too, consistent with search. Locking stays a separate paywall (402 / -32002) in fetch. Added a test that fetch on a rejected and on a superseded memory both behave like search. Pushed in bbcde6f.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks @ZachL111 — confirmed the bbcde6f35 fix looks correct: _memory_visible is now shared by both _search_memories and the fetch memory branch, so rejected/superseded memories return not-found from fetch consistently with search, with locking staying a separate 402/-32002 paywall. Good catch on your own follow-through.

Static pass against main looks sound to me:

  • All DB calls are uid-scoped (get_memory(uid, …), get_conversation(uid, …), etc.) — no cross-user read path.
  • The memory visibility filter mirrors the existing search_memories filter exactly.
  • POST for the REST search keeps queries out of URL/access logs.
  • The connector drops locked conversations in search and paywalls them in fetch, which is a sensible stricter posture for the connector context.

This is squarely on-mission for #4862 and well-tested. I'm not approving it myself, though: it adds a new full-text fetch read path over users' memories and conversations plus a combined memories.read + conversations.read scope, so I'd want a maintainer + security sign-off on the data-access direction (not just code correctness) before merge. Keeping the security-review / needs-maintainer-review labels for that. The points above are not blocking; the consistency fix can stay as-is.

@ZachL111

Copy link
Copy Markdown
Contributor Author

Thanks David, that matches the intent. The new fetch path is read only and every DB call is uid scoped, so it adds reach over the caller's own memories and conversations but no new cross user surface, which is why I kept the connector stricter than REST (drops locked conversations in search, paywalls them in fetch). I agree the data access direction deserves a maintainer plus security sign off rather than just a code pass, so the security-review and needs-maintainer-review labels make sense to me.

If a maintainer would rather split memories.read and conversations.read into two separate connector scopes, or gate the combined scope behind an explicit opt in, I am happy to make that change in this PR. Leaving it as is for now per your note that the consistency fix can stay.

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCP feature: connector search/fetch over memory base — approve only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature-fit-review PR touches feature direction; qualitative fit assessed needs-maintainer-review Needs a human maintainer to review/approve (e.g. stacked, product, or architecture judgment) security-review Needs security/privacy review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants