Skip to content

feat(mcp): organize conversations into folders via MCP (#4862)#8489

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

feat(mcp): organize conversations into folders via MCP (#4862)#8489
ZachL111 wants to merge 9 commits into
BasedHardware:mainfrom
ZachL111:zach/mcp-folders

Conversation

@ZachL111

@ZachL111 ZachL111 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Summary

The MCP server already lets an assistant read conversations and create or edit memories, but it cannot organize anything. The folders system (database/folders.py, the /v1/folders REST API) is fully built yet has zero MCP exposure, so a connected assistant (ChatGPT, Claude) cannot file conversations into Work/Personal/Social or custom folders. This adds folder management to the MCP server so the two-way memory bank (#4862) can keep itself organized.

What this adds

Five new MCP tools, available on both transports (REST mcp.py and SSE mcp_sse.py) and kept in sync through one shared util:

  • get_folders: list the user's folders.
  • create_folder: create a custom folder.
  • update_folder: rename, recolor, redescribe, or change the icon.
  • delete_folder: delete a folder and move its conversations to a target folder (or the default folder when none is given). System folders cannot be deleted.
  • move_conversation_to_folder: file a conversation into a folder, or omit the folder to remove it from any folder.

It also adds a folder_id filter to the existing get_conversations tool, so "show me my Work conversations" works without a new tool (the database query already supported the filter, it just was not exposed).

New OAuth scopes: folders.read and folders.write.

Design

  • utils/mcp_folders.py is the single orchestration point both transports call, so REST and SSE cannot drift. It mirrors the validation the REST folders router already enforces: a missing folder or conversation is 404 / -32001, a locked conversation is 402 / -32002, deleting a system folder or exceeding the 50 custom-folder cap is rejected, and a missing move target is 404.
  • utils/mcp_data.clean_folder shapes folder docs for output, matching the other clean_* helpers.
  • Each transport maps the orchestration errors through one small helper (HTTP status for REST, JSON-RPC code for SSE).

Testing

Added 20 tests to tests/unit/test_mcp_data_endpoints.py (the canonical MCP data test, already wired into test.sh): both transports, the create/update/delete/move flows, the system-folder and paywall guards, the not-found mappings, the folder_id passthrough on get_conversations, the clean_folder shape, and tool/scope registration. The full file passes (37 tests). 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.

2 issues found across 5 files

Confidence score: 3/5

  • In backend/utils/mcp_folders.py, the shared orchestration path does not validate name at runtime, so a non-string value from SSE can trigger an uncaught AttributeError and fail the request unexpectedly for users—add explicit input type checks (and a graceful error response) before merging.
  • In backend/utils/mcp_folders.py (create_folder), the custom_count < 50 guard is non-atomic, so concurrent requests can both pass the check and create folders beyond MAX_CUSTOM, causing inconsistent enforcement of limits—make the limit check + create step atomic (transaction/lock/DB constraint) before merging.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="backend/utils/mcp_folders.py">

<violation number="1" location="backend/utils/mcp_folders.py:61">
P2: Folder limit check in `create_folder` is non-atomic and susceptible to a race condition: two concurrent requests can both read `custom_count < 50` and both proceed to create a folder, exceeding the intended `MAX_CUSTOM_FOLDERS` cap. The underlying `database/folders.py` `create_folder` does not enforce the limit either.</violation>
</file>

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

Re-trigger cubic

Comment thread backend/utils/mcp_folders.py
name = (name or "").strip()
if not name:
raise InvalidFolderRequest("Folder name is required")
custom_count = len([f for f in folders_db.get_folders(uid) if not f.get("is_system")])

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: Folder limit check in create_folder is non-atomic and susceptible to a race condition: two concurrent requests can both read custom_count < 50 and both proceed to create a folder, exceeding the intended MAX_CUSTOM_FOLDERS cap. The underlying database/folders.py create_folder does not enforce the limit either.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/utils/mcp_folders.py, line 61:

<comment>Folder limit check in `create_folder` is non-atomic and susceptible to a race condition: two concurrent requests can both read `custom_count < 50` and both proceed to create a folder, exceeding the intended `MAX_CUSTOM_FOLDERS` cap. The underlying `database/folders.py` `create_folder` does not enforce the limit either.</comment>

<file context>
@@ -0,0 +1,118 @@
+    name = (name or "").strip()
+    if not name:
+        raise InvalidFolderRequest("Folder name is required")
+    custom_count = len([f for f in folders_db.get_folders(uid) if not f.get("is_system")])
+    if custom_count >= MAX_CUSTOM_FOLDERS:
+        raise FolderLimitReached(f"Maximum folder limit reached ({MAX_CUSTOM_FOLDERS} custom folders)")
</file context>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right that the check is non-atomic. I kept it intentionally so the MCP path matches the existing REST folders router (routers/folders.py), which does the same count-then-create check without a transaction. This cap is a soft UX limit, not a security or data-integrity boundary: in a rare concurrent race a user could end up with 51 custom folders, which is harmless. True atomicity would need a transaction or a counter document inside the shared database/folders.create_folder, which both the REST router and the MCP tool call, so it belongs in that shared layer as its own change rather than diverging the MCP wrapper from the router. Happy to do that follow-up if you want the cap enforced atomically across both surfaces.

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

Copy link
Copy Markdown
Collaborator

Thanks @ZachL111 — this is a really clean contribution, and a natural extension of the two-way memory bank direction in #4862. A few notes from review:

What looks good

  • Single orchestration point (utils/mcp_folders.py) shared by both the REST and SSE transports is the right call — it prevents drift, and the typed error hierarchy maps cleanly to both HTTP statuses and JSON-RPC codes.
  • Mirroring the existing REST folders router's validation (50-folder soft cap, system-folder protection, target-folder existence checks, self-move guard) keeps the MCP surface consistent with what the app already does. Good instinct matching the existing behavior rather than inventing stricter rules here.
  • The SSE type-validation work you added after cubic's P1 is exactly right — raw JSON args bypassing Pydantic is a real trap, and rejecting non-strings at the boundary is the correct fix. The added tests for non-string inputs are appreciated.
  • clean_folder follows the established clean_* pattern. The folder_id filter on get_conversations reuses an existing DB query rather than adding a new tool — nice scope discipline.
  • New OAuth scopes (folders.read / folders.write) follow the existing per-resource read/write split.

Notes for the human maintainer

  • This is a new write surface over user data (folder create/update/delete, conversation moves), exposed via MCP to connected assistants (ChatGPT, Claude). The code looks correct, but whether and how Omi wants assistants to reorganize conversations is a product-direction call I'm flagging rather than deciding.
  • Worth confirming the OAuth scope enforcement story end to end: the new folders.read/folders.write scopes are declared in MCP_SCOPES_SUPPORTED and the tool securitySchemes, and the REST MCP endpoints rely on the API-key dependency — please verify that the runtime scope checks (if any) cover the folder operations the same way they cover the existing read/write tools, so a read-only token can't delete a folder.
  • delete_folder is a destructive, irreversible-ish operation (conversations get bulk-moved to a fallback folder). The DESTRUCTIVE_WRITE_ANNOTATIONS hint is set on the SSE tool, which is good; confirm that's enough for connected clients to gate it.
  • The folder-limit check is count-then-create and non-atomic, matching the existing REST router. Your reasoning (soft UX limit, not a security boundary) is sound and I agree it's fine to match the existing behavior here, but noting it so a maintainer can decide if it's worth hardening in both places later.

Not approving — this is a non-trivial feature touching a new user-data write surface and the MCP permission model, so it needs a human product/security judgment before merge. Tests and CI look solid (E2E + lint passing). Tagging needs-maintainer-review and feature-fit-review.

@ZachL111

Copy link
Copy Markdown
Contributor Author

Thanks David, appreciate the thorough pass. Confirming the points you flagged:

Scope enforcement (consistency): There is no per-key runtime scope enforcement for any MCP tool today. authenticate_api_key returns only a user_id, the MCP API key carries no per-scope grants, and handle_mcp_message passes just that user_id into execute_tool with no scope check, the same for the existing memories.write write tools (create_memory/edit_memory/delete_memory). So folders.write is consistent with the existing surface rather than diverging, and because keys are user-scoped there is no cross-user path. There is also no read-only-token concept yet, so "a read-only token can't delete a folder" is not something the server can enforce for folders or for any other tool. Real per-key scope enforcement is the server-wide follow-up we aligned on in #8439 and #8491, applied across all tools at once.

Destructive annotation: delete_folder uses DESTRUCTIVE_WRITE_ANNOTATIONS (destructiveHint: true), the same hint as delete_memory, which is the standard MCP signal connected clients use to gate a destructive call. So it is annotated the same way as the existing destructive tools.

Non-atomic cap: Agreed, it matches the REST folders router's count-then-create, and I noted in the cubic thread that atomic enforcement belongs in the shared database.folders layer as its own change so both surfaces stay in lockstep.

Product direction: Understood, whether assistants should reorganize conversations is a maintainer call; I scoped it additive and enrollment-gated and am happy to defer that decision.

@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: organize conversations into folders via MCP (#4862) — 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)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants