feat(mcp): manage people (rename, register, remove) via MCP (#4862)#8491
feat(mcp): manage people (rename, register, remove) via MCP (#4862)#8491ZachL111 wants to merge 6 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
Confidence score: 1/5
backend/routers/mcp_sse.pyandbackend/routers/mcp.pycurrently bypasspeople.writeauthorization on destructive people operations, so any valid MCP API key could modify data if merged as-is. This is a merge-blocking access-control gap—enforce scope checks in both SSE tool execution and REST handlers (and add regression tests) before merging.- In
backend/routers/mcp_sse.py,create_personandupdate_personreturn read-level person data while requiring onlypeople.write, which can leak data to callers without read permission. Tighten response fields or requirepeople.readalongside write access 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/routers/mcp.py">
<violation number="1" location="backend/routers/mcp.py:541">
P1: People write routes bypass the people.write scope enforcement on REST endpoints.</violation>
</file>
<file name="backend/routers/mcp_sse.py">
<violation number="1" location="backend/routers/mcp_sse.py:97">
P1: New `people.write` scope is advertised in tool metadata but not enforced during SSE tool execution. Any valid MCP API key can invoke destructive people operations.</violation>
<violation number="2" location="backend/routers/mcp_sse.py:926">
P2: `create_person` and `update_person` expose read-level person data while requiring only `people.write` scope</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
|
|
||
| @router.post("/v1/mcp/people", tags=["mcp"]) | ||
| def mcp_create_person(request: McpCreatePerson, uid: str = Depends(get_uid_from_mcp_api_key)): |
There was a problem hiding this comment.
P1: People write routes bypass the people.write scope enforcement on REST endpoints.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/mcp.py, line 541:
<comment>People write routes bypass the people.write scope enforcement on REST endpoints.</comment>
<file context>
@@ -498,6 +499,69 @@ def get_people(uid: str = Depends(get_uid_from_mcp_api_key)):
+
+
+@router.post("/v1/mcp/people", tags=["mcp"])
+def mcp_create_person(request: McpCreatePerson, uid: str = Depends(get_uid_from_mcp_api_key)):
+ try:
+ return clean_person(mcp_people.create_person(uid, request.name))
</file context>
| CHAT_READ_SECURITY = [{"type": "oauth2", "scopes": ["chat.read"]}] | ||
| SCREEN_ACTIVITY_READ_SECURITY = [{"type": "oauth2", "scopes": ["screen_activity.read"]}] | ||
| PEOPLE_READ_SECURITY = [{"type": "oauth2", "scopes": ["people.read"]}] | ||
| PEOPLE_WRITE_SECURITY = [{"type": "oauth2", "scopes": ["people.write"]}] |
There was a problem hiding this comment.
P1: New people.write scope is advertised in tool metadata but not enforced during SSE tool execution. Any valid MCP API key can invoke destructive people operations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/mcp_sse.py, line 97:
<comment>New `people.write` scope is advertised in tool metadata but not enforced during SSE tool execution. Any valid MCP API key can invoke destructive people operations.</comment>
<file context>
@@ -92,6 +94,7 @@
CHAT_READ_SECURITY = [{"type": "oauth2", "scopes": ["chat.read"]}]
SCREEN_ACTIVITY_READ_SECURITY = [{"type": "oauth2", "scopes": ["screen_activity.read"]}]
PEOPLE_READ_SECURITY = [{"type": "oauth2", "scopes": ["people.read"]}]
+PEOPLE_WRITE_SECURITY = [{"type": "oauth2", "scopes": ["people.write"]}]
</file context>
| person = mcp_people.create_person(user_id, arguments.get("name")) | ||
| except mcp_people.PersonError as e: | ||
| raise _person_tool_error(e) | ||
| return {"success": True, "person": clean_person(person)} |
There was a problem hiding this comment.
P2: create_person and update_person expose read-level person data while requiring only people.write scope
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/routers/mcp_sse.py, line 926:
<comment>`create_person` and `update_person` expose read-level person data while requiring only `people.write` scope</comment>
<file context>
@@ -836,6 +904,41 @@ def execute_tool(user_id: str, tool_name: str, arguments: dict) -> dict:
+ person = mcp_people.create_person(user_id, arguments.get("name"))
+ except mcp_people.PersonError as e:
+ raise _person_tool_error(e)
+ return {"success": True, "person": clean_person(person)}
+
+ elif tool_name == "update_person":
</file context>
|
Thanks cubic. These three findings all come down to one design point about how the Omi MCP server does authorization, so let me address them together. MCP API keys are user-scoped bearer credentials, not per-scope ACLs. Two consequences for these findings: 1 and 2 (people.write not enforced): 3 (write tools return person data): returning the created or updated resource is standard create/update behavior and matches Happy to implement server-wide MCP scope enforcement (threading granted scopes from the key through |
Git-on-my-level
added
needs-maintainer-review
|
Thanks @ZachL111 — solid work here. The shared Two observations for the human maintainer before merge:
Feature direction looks good — people curation (relabel misidentified speakers, dedupe, register contacts) is a natural fit for the two-way memory bank in #4862. Flagging for maintainer review given this is part of a sequence of MCP-surface expansions and touches the auth/scope surface. |
|
Thanks David, agreed on both points. On the auth model, glad that lines up with the #8439 conclusion: the key is user-scoped, no per-key grants, and On the speech-sample cleanup, you're right that the MCP delete was orphaning the GCS blobs. |
|
Verified at head ( What I checked: all five tools route through a single Implementation looks clean and well-tested. Holding on formal approval since this is part of the MCP-surface expansion sequence and touches the auth/scope surface ( Thanks @ZachL111 — nice work on the shared orchestration layer. |
|
Thanks David. To put the scope model in one place for the maintainer review you are gating on: API keys here are user scoped and all access. There is no per key scope enforcement anywhere in the MCP surface today, so people.write behaves exactly like memories.write and the other write tools, declared in the metadata and in the tool securitySchemes and authorized by user identity rather than by a per key grant. delete_person carries the destructive write annotation and the speech sample cleanup now mirrors the REST delete_person_endpoint, so no new privilege boundary is introduced here beyond the existing user scoped key. A dedicated scope model pass is welcome. The broader question of server wide per key scope enforcement is a deliberate cross cutting change rather than part of this tool addition, and is the same design point raised on #8439; I am happy to support that as a separate effort if a maintainer wants to pursue it. |
3 participants
Summary
The MCP server can read the people Omi identifies from conversations (recurring speakers) via
get_people, but it cannot manage them. People are derived from speaker identification and frequently need curation: a speaker gets mislabeled, the same person shows up under two entries, or the user wants to register a known contact by name. This adds people management to the MCP server so an assistant can keep the user's people clean for the two-way memory bank (#4862).What this adds
Five new MCP tools, on both transports (REST
mcp.pyand SSEmcp_sse.py) through one shared util:get_person: fetch a single person by ID.find_person_by_name: look up a person by exact name (returns the person, or null when there is no match).create_person: register a known person by name. Idempotent by name (returns the existing person if one already has that name), matching the REST people endpoint, so an assistant cannot create duplicates.update_person: rename a person, for example to relabel a speaker Omi identified.delete_person: remove a person. Conversations attributed to them fall back to a generic speaker label.New OAuth scope:
people.write(people.readalready existed forget_people).Design
utils/mcp_people.pyis the single orchestration point both transports call, so REST and SSE cannot drift. It mirrors the REST people router: a missing person is 404 / -32001, and string inputs are type-checked so a non-string argument from the SSE transport returns a clean error rather than an uncaught exception.clean_personshaper, which already drops raw speech-sample audio URLs and speaker embeddings.Testing
Added tests to
tests/unit/test_mcp_data_endpoints.py(the canonical MCP data test, already wired intotest.sh), covering both transports: get/find/create/update/delete, idempotent-by-name create, the rename and delete not-found mappings, blank and non-string input rejection, find-by-name hit and miss, and tool/scope registration. The full file passes (33 tests). Black and the async-blocker lint are clean.