feat(backend): expand Omi MCP data surface (action items, goals, chat, people, screen activity, daily summaries)#7817
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ity, daily summaries via MCP REST Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…people, screen activity, daily summaries Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR expands the Omi MCP server with six new data domains (action items, goals, chat, people, screen activity, daily summaries), wiring each into both the REST router and the MCP SSE tool surface via a shared
Confidence Score: 3/5Needs fixes before merging — two tool dispatch branches have unguarded parse calls that leak exceptions to callers, and the daily-summaries path bypasses date validation that all other tools enforce. Three tool branches in mcp_sse.py have correctness gaps: get_goals and get_screen_activity both call parse_mcp_bool() without a try/except, so a bad boolean argument produces an unhandled ValueError instead of a proper MCP error; get_daily_summaries forwards raw date strings to Firestore without running them through _parse_mcp_date(), silently returning wrong data on invalid input. The REST endpoint compounds the last issue by typing its date params as Optional[str] rather than Optional[datetime]. The privacy-sensitive people-record stripping and the overall scope design are solid, but these dispatch gaps affect real runtime behavior on the new tool surface. backend/routers/mcp_sse.py (the get_goals, get_screen_activity, and get_daily_summaries dispatch branches) and the get_daily_summaries endpoint signature in backend/routers/mcp.py. Important Files Changed
Sequence DiagramsequenceDiagram
participant C as MCP Client / REST caller
participant SSE as mcp_sse.execute_tool
participant Shape as utils/mcp_data.py
participant DB as database modules
C->>SSE: get_action_items(completed, due_start_date, limit)
SSE->>SSE: parse_mcp_int / parse_optional_mcp_bool / _parse_mcp_date
SSE->>DB: action_items_db.get_action_items(uid, ...)
DB-->>SSE: raw Firestore docs
SSE->>Shape: clean_action_item(item)
SSE-->>C: "{action_items: [...]}"
C->>SSE: "get_goals(include_inactive=maybe)"
SSE->>SSE: parse_mcp_bool raises unhandled ValueError
SSE-->>C: unhandled exception / 500
C->>SSE: "get_screen_activity(summary=bad)"
SSE->>SSE: parse_mcp_bool raises unhandled ValueError
SSE-->>C: unhandled exception / 500
C->>SSE: "get_daily_summaries(start_date=not-a-date)"
SSE->>SSE: No _parse_mcp_date - raw string forwarded
SSE->>DB: "daily_summaries_db.get_daily_summaries(start_date=not-a-date)"
DB-->>SSE: empty / wrong results silent
SSE-->>C: "{daily_summaries: []}"
Reviews (1): Last reviewed commit: "test(backend): run test_mcp_data_endpoin..." | Re-trigger Greptile |
| elif tool_name == "get_goals": | ||
| include_inactive = parse_mcp_bool(arguments.get("include_inactive"), "include_inactive", default=False) | ||
| return {"goals": goals_db.get_all_goals(user_id, include_inactive=include_inactive)} |
There was a problem hiding this comment.
parse_mcp_bool can raise ValueError for invalid inputs (e.g., "maybe"), but the get_goals branch doesn't wrap it in a try/except. Every other tool that calls parsing helpers wraps them in try/except ValueError as e: raise ToolExecutionError(str(e), code=-32602). Without that guard, a malformed bool argument escapes execute_tool as an unhandled ValueError rather than producing the expected MCP error response.
| elif tool_name == "get_goals": | |
| include_inactive = parse_mcp_bool(arguments.get("include_inactive"), "include_inactive", default=False) | |
| return {"goals": goals_db.get_all_goals(user_id, include_inactive=include_inactive)} | |
| elif tool_name == "get_goals": | |
| try: | |
| include_inactive = parse_mcp_bool(arguments.get("include_inactive"), "include_inactive", default=False) | |
| except ValueError as e: | |
| raise ToolExecutionError(str(e), code=-32602) | |
| return {"goals": goals_db.get_all_goals(user_id, include_inactive=include_inactive)} |
| elif tool_name == "get_screen_activity": | ||
| start = _parse_mcp_date(arguments.get("start_date"), "start_date") | ||
| end = _parse_mcp_date(arguments.get("end_date"), "end_date") | ||
| app = arguments.get("app") | ||
| summary = parse_mcp_bool(arguments.get("summary"), "summary", default=False) | ||
| if summary: | ||
| return screen_activity_db.get_screen_activity_summary(user_id, start_date=start, end_date=end) | ||
| try: | ||
| limit = parse_mcp_int(arguments.get("limit"), "limit", default=200, minimum=1, maximum=1000) | ||
| except ValueError as e: | ||
| raise ToolExecutionError(str(e), code=-32602) |
There was a problem hiding this comment.
parse_mcp_bool for summary can raise ValueError on a bad input and is unguarded here. Wrapping it in a try/except that covers all the parsing in this branch keeps the error path consistent with every other tool.
| elif tool_name == "get_screen_activity": | |
| start = _parse_mcp_date(arguments.get("start_date"), "start_date") | |
| end = _parse_mcp_date(arguments.get("end_date"), "end_date") | |
| app = arguments.get("app") | |
| summary = parse_mcp_bool(arguments.get("summary"), "summary", default=False) | |
| if summary: | |
| return screen_activity_db.get_screen_activity_summary(user_id, start_date=start, end_date=end) | |
| try: | |
| limit = parse_mcp_int(arguments.get("limit"), "limit", default=200, minimum=1, maximum=1000) | |
| except ValueError as e: | |
| raise ToolExecutionError(str(e), code=-32602) | |
| elif tool_name == "get_screen_activity": | |
| try: | |
| start = _parse_mcp_date(arguments.get("start_date"), "start_date") | |
| end = _parse_mcp_date(arguments.get("end_date"), "end_date") | |
| app = arguments.get("app") | |
| summary = parse_mcp_bool(arguments.get("summary"), "summary", default=False) | |
| except (ValueError, ToolExecutionError) as e: | |
| raise ToolExecutionError(str(e), code=-32602) | |
| if summary: | |
| return screen_activity_db.get_screen_activity_summary(user_id, start_date=start, end_date=end) | |
| try: | |
| limit = parse_mcp_int(arguments.get("limit"), "limit", default=200, minimum=1, maximum=1000) | |
| except ValueError as e: | |
| raise ToolExecutionError(str(e), code=-32602) |
| elif tool_name == "get_daily_summaries": | ||
| try: | ||
| limit = parse_mcp_int(arguments.get("limit"), "limit", default=30, minimum=1, maximum=100) | ||
| offset = parse_mcp_int(arguments.get("offset"), "offset", default=0, minimum=0, maximum=100000) | ||
| except ValueError as e: | ||
| raise ToolExecutionError(str(e), code=-32602) | ||
| summaries = daily_summaries_db.get_daily_summaries( | ||
| user_id, | ||
| limit=limit, | ||
| offset=offset, | ||
| start_date=arguments.get("start_date"), | ||
| end_date=arguments.get("end_date"), | ||
| ) | ||
| return {"daily_summaries": summaries} |
There was a problem hiding this comment.
start_date and end_date are forwarded to daily_summaries_db.get_daily_summaries() as raw, unvalidated strings. The DB function passes them straight into Firestore FieldFilter string comparisons, so an invalid value like "not-a-date" silently produces wrong results instead of a proper MCP error. Every other tool with date params calls _parse_mcp_date() first.
| elif tool_name == "get_daily_summaries": | |
| try: | |
| limit = parse_mcp_int(arguments.get("limit"), "limit", default=30, minimum=1, maximum=100) | |
| offset = parse_mcp_int(arguments.get("offset"), "offset", default=0, minimum=0, maximum=100000) | |
| except ValueError as e: | |
| raise ToolExecutionError(str(e), code=-32602) | |
| summaries = daily_summaries_db.get_daily_summaries( | |
| user_id, | |
| limit=limit, | |
| offset=offset, | |
| start_date=arguments.get("start_date"), | |
| end_date=arguments.get("end_date"), | |
| ) | |
| return {"daily_summaries": summaries} | |
| elif tool_name == "get_daily_summaries": | |
| try: | |
| limit = parse_mcp_int(arguments.get("limit"), "limit", default=30, minimum=1, maximum=100) | |
| offset = parse_mcp_int(arguments.get("offset"), "offset", default=0, minimum=0, maximum=100000) | |
| except ValueError as e: | |
| raise ToolExecutionError(str(e), code=-32602) | |
| start = _parse_mcp_date(arguments.get("start_date"), "start_date") | |
| end = _parse_mcp_date(arguments.get("end_date"), "end_date") | |
| summaries = daily_summaries_db.get_daily_summaries( | |
| user_id, | |
| limit=limit, | |
| offset=offset, | |
| start_date=start.strftime("%Y-%m-%d") if start else None, | |
| end_date=end.strftime("%Y-%m-%d") if end else None, | |
| ) | |
| return {"daily_summaries": summaries} |
| @router.get("/v1/mcp/daily-summaries", tags=["mcp"]) | ||
| def get_daily_summaries( | ||
| limit: int = 30, | ||
| offset: int = 0, | ||
| start_date: Optional[str] = None, | ||
| end_date: Optional[str] = None, | ||
| uid: str = Depends(get_uid_from_mcp_api_key), | ||
| ): |
There was a problem hiding this comment.
Every other date-filtering endpoint in this file uses
Optional[datetime] so FastAPI validates the format automatically. Using Optional[str] here means a caller can pass "not-a-date" and it reaches Firestore unchanged, silently returning wrong results instead of a 422.
| @router.get("/v1/mcp/daily-summaries", tags=["mcp"]) | |
| def get_daily_summaries( | |
| limit: int = 30, | |
| offset: int = 0, | |
| start_date: Optional[str] = None, | |
| end_date: Optional[str] = None, | |
| uid: str = Depends(get_uid_from_mcp_api_key), | |
| ): | |
| @router.get("/v1/mcp/daily-summaries", tags=["mcp"]) | |
| def get_daily_summaries( | |
| limit: int = 30, | |
| offset: int = 0, | |
| start_date: Optional[datetime] = None, | |
| end_date: Optional[datetime] = None, | |
| uid: str = Depends(get_uid_from_mcp_api_key), | |
| ): |
| ACTION_ITEMS_READ_SECURITY = [{"type": "oauth2", "scopes": ["action_items.read"]}] | ||
| GOALS_READ_SECURITY = [{"type": "oauth2", "scopes": ["goals.read"]}] | ||
| 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"]}] |
There was a problem hiding this comment.
The PR description says each domain gets its own OAuth scope, and five new scopes are correctly added. But
get_daily_summaries reuses CONVERSATIONS_READ_SECURITY, so any OAuth client with only conversations.read can also read daily life summaries — a distinct, high-sensitivity data domain. A dedicated daily_summaries.read scope should be added to match the stated design.
| ACTION_ITEMS_READ_SECURITY = [{"type": "oauth2", "scopes": ["action_items.read"]}] | |
| GOALS_READ_SECURITY = [{"type": "oauth2", "scopes": ["goals.read"]}] | |
| 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"]}] | |
| ACTION_ITEMS_READ_SECURITY = [{"type": "oauth2", "scopes": ["action_items.read"]}] | |
| GOALS_READ_SECURITY = [{"type": "oauth2", "scopes": ["goals.read"]}] | |
| 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"]}] | |
| DAILY_SUMMARIES_READ_SECURITY = [{"type": "oauth2", "scopes": ["daily_summaries.read"]}] |
What
Expands the Omi MCP server to expose six data domains it already stores but never surfaced, wired in both the REST API (`routers/mcp.py`) and the MCP tool surface AI clients see (`routers/mcp_sse.py`):
Shared response shaping lives in `utils/mcp_data.py` so both routers reuse identical shapes without cross-importing (routers must not import each other). Each domain gets its own OAuth scope (`action_items.read`, `goals.read`, `chat.read`, `screen_activity.read`, `people.read`) so a future scoped-key model can gate them — today's `omi_mcp_` key still grants all.
Testing
🤖 Generated with Claude Code