feat(mcp): organize conversations into folders via MCP (#4862)#8489
feat(mcp): organize conversations into folders via MCP (#4862)#8489ZachL111 wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
2 issues found across 5 files
Confidence score: 3/5
- In
backend/utils/mcp_folders.py, the shared orchestration path does not validatenameat runtime, so a non-string value from SSE can trigger an uncaughtAttributeErrorand 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), thecustom_count < 50guard is non-atomic, so concurrent requests can both pass the check and create folders beyondMAX_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
| 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")]) |
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
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.
|
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
Notes for the human maintainer
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 |
|
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. Destructive annotation: 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 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. |
# Conflicts: # backend/routers/mcp.py # backend/routers/mcp_sse.py
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/foldersREST 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.pyand SSEmcp_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_idfilter to the existingget_conversationstool, 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.readandfolders.write.Design
utils/mcp_folders.pyis 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_foldershapes folder docs for output, matching the otherclean_*helpers.Testing
Added 20 tests to
tests/unit/test_mcp_data_endpoints.py(the canonical MCP data test, already wired intotest.sh): both transports, the create/update/delete/move flows, the system-folder and paywall guards, the not-found mappings, thefolder_idpassthrough onget_conversations, theclean_foldershape, and tool/scope registration. The full file passes (37 tests). Black and the async-blocker lint are clean.