first nao MCP version#708
Conversation
🚀 Preview Deployment
Preview will be automatically removed when this PR is closed. |
There was a problem hiding this comment.
16 issues found across 53 files
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="apps/backend/src/mcp/tools/stories.ts">
<violation number="1" location="apps/backend/src/mcp/tools/stories.ts:115">
P1: `create_story` should not upsert by slug; slug collisions currently update an existing story instead of creating a new one.</violation>
</file>
<file name="apps/backend/src/mcp/tools/files.ts">
<violation number="1" location="apps/backend/src/mcp/tools/files.ts:45">
P2: Clamp `max_results` to a positive integer before passing it to grep. Values <= 0 currently produce empty `matches` even when grep finds results.</violation>
</file>
<file name="apps/backend/src/mcp/routes.ts">
<violation number="1" location="apps/backend/src/mcp/routes.ts:29">
P1: When reusing an existing session, the code does not verify that the authenticated `userId` matches `session.userId`. This allows an authenticated user to hijack another user's session if they know the session ID.</violation>
<violation number="2" location="apps/backend/src/mcp/routes.ts:83">
P1: The DELETE endpoint does not authenticate the caller. Unlike POST and GET, it skips `resolveUserId`, allowing any unauthenticated request with a valid `Mcp-Session-Id` to terminate another user's session.</violation>
</file>
<file name="apps/backend/migrations-postgres/0038_mcp_endpoint.sql">
<violation number="1" location="apps/backend/migrations-postgres/0038_mcp_endpoint.sql:67">
P2: Missing index on `mcp_call_log.user_id`. Every other table in this migration indexes its `user_id` FK column, but `mcp_call_log` does not. Without it, cascading deletes from the `user` table will require a sequential scan of `mcp_call_log`, and any user-scoped queries on the call log will be slow.</violation>
</file>
<file name="apps/backend/src/mcp/logging.ts">
<violation number="1" location="apps/backend/src/mcp/logging.ts:26">
P1: `build_chart` is missing from `TOOL_MODE_MAP`, so tools-mode disabling does not apply to that tool.</violation>
<violation number="2" location="apps/backend/src/mcp/logging.ts:51">
P2: Call log success is misreported when handlers return `isError: true` without throwing.</violation>
</file>
<file name="apps/backend/migrations-sqlite/0038_mcp_endpoint.sql">
<violation number="1" location="apps/backend/migrations-sqlite/0038_mcp_endpoint.sql:94">
P2: Missing index on `story.project_id`. The other two FK columns (`chat_id`, `user_id`) both have indexes, but `project_id` does not. This will hurt query performance when filtering stories by project and slow down cascade deletes from the `project` table.</violation>
</file>
<file name="apps/backend/src/db/sqlite-schema.ts">
<violation number="1" location="apps/backend/src/db/sqlite-schema.ts:523">
P2: The `story_chat_slug_unique` constraint on `(chatId, slug)` does not enforce slug uniqueness for standalone stories. In SQLite, NULL values are always considered distinct in unique indexes, so multiple rows with `chatId = NULL` can share the same slug. Consider adding a partial unique constraint or a CHECK + separate unique index for standalone stories (e.g., on `(projectId, userId, slug)` where `chatId IS NULL`).</violation>
</file>
<file name="apps/backend/src/trpc/chat-fork.routes.ts">
<violation number="1" location="apps/backend/src/trpc/chat-fork.routes.ts:41">
P1: Validate that the requested story belongs to the selected project before reusing or creating a chat; otherwise this endpoint can link a story across projects.</violation>
</file>
<file name="apps/backend/src/db/pg-schema.ts">
<violation number="1" location="apps/backend/src/db/pg-schema.ts:497">
P2: Making `chatId` nullable breaks the `story_chat_slug_unique` constraint for standalone stories. In PostgreSQL, `UNIQUE(chatId, slug)` allows unlimited rows with `(NULL, same_slug)` because NULLs are considered distinct. Consider adding a partial unique index for standalone stories, e.g. on `(projectId, userId, slug)` where `chatId IS NULL`.</violation>
</file>
<file name="apps/backend/src/mcp/tools/data.ts">
<violation number="1" location="apps/backend/src/mcp/tools/data.ts:23">
P2: Validate `limit` with a lower bound; currently negative values bypass validation and produce incorrect slicing behavior.</violation>
</file>
<file name="apps/backend/src/queries/story.queries.ts">
<violation number="1" location="apps/backend/src/queries/story.queries.ts:167">
P2: Race condition: `createStandaloneVersion` can create duplicate stories under concurrent calls. Unlike `getOrCreateStory` which uses `onConflictDoNothing`, this plain insert has no conflict handling and there's no unique constraint protecting `(projectId, userId, slug)` when `chatId` is NULL (SQLite treats NULLs as distinct in unique indexes).</violation>
</file>
<file name="apps/frontend/src/components/auth-form.tsx">
<violation number="1" location="apps/frontend/src/components/auth-form.tsx:18">
P1: Thread the callback URL through Microsoft sign-in too; otherwise that provider still breaks the MCP authorize redirect.</violation>
</file>
<file name="apps/backend/src/trpc/story.routes.ts">
<violation number="1" location="apps/backend/src/trpc/story.routes.ts:53">
P2: Standalone project routes are not constrained to the current project, allowing cross-project story access by ID.</violation>
</file>
<file name="apps/backend/src/mcp/tools/agent.ts">
<violation number="1" location="apps/backend/src/mcp/tools/agent.ts:67">
P2: Return a generic failure message instead of exposing raw exception text to clients.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
3 issues found across 21 files (changes from recent commits).
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="apps/backend/src/db/pg-schema.ts">
<violation number="1" location="apps/backend/src/db/pg-schema.ts:518">
P2: This partial unique index does not fully enforce standalone slug uniqueness because `projectId` and `userId` are nullable. Add a constraint so standalone rows require non-null `projectId`/`userId` (or make those columns non-null in that mode).</violation>
</file>
<file name="apps/backend/migrations-sqlite/0038_mcp_endpoint.sql">
<violation number="1" location="apps/backend/migrations-sqlite/0038_mcp_endpoint.sql:94">
P2: This unique index can still allow duplicate standalone slugs when `project_id` or `user_id` is NULL.</violation>
</file>
<file name="apps/backend/src/mcp/tools/stories.ts">
<violation number="1" location="apps/backend/src/mcp/tools/stories.ts:112">
P2: `getUniqueStandaloneSlug` uses a non-atomic check-then-create flow, so concurrent `create_story` calls can still collapse into the same story instead of creating separate stories.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
440f5b2 to
41064c6
Compare
…CP client side too
Required by routes.ts and mcp-endpoint.routes.ts after the real-time MCP settings update; unblocks tsc/lint and the Docker frontend build. Co-authored-by: Cursor <cursoragent@cursor.com>
41064c6 to
24f2c8d
Compare
Bl3f
left a comment
There was a problem hiding this comment.
First review of the backend, I think there are a few things to fix still (a lot of duplication)) and I'm afraid also having a lot of execution code that is similar but not factorised to execute things.
| @@ -1,5 +1,7 @@ | |||
| import { APIError, betterAuth } from 'better-auth'; | |||
| import { drizzleAdapter } from 'better-auth/adapters/drizzle'; | |||
| import { mcp as mcpPlugin } from 'better-auth/plugins'; | |||
There was a problem hiding this comment.
its written this plugin will be deprecated here https://better-auth.com/docs/plugins/mcp
| ); | ||
|
|
||
| const rows = result.data.slice(0, limit); | ||
| const queryId = `query_${crypto.randomUUID().slice(0, 8)}`; |
There was a problem hiding this comment.
queryId is already returned by executeQuery
| const userId = await resolveUserId(request); | ||
| if (!userId) { | ||
| return replyUnauthorized(reply); | ||
| } |
There was a problem hiding this comment.
This pattern is written 4 times, can it be wrapped in a fastify middleware?
| return replyUnauthorized(reply); | ||
| } | ||
|
|
||
| const sessionId = request.headers['mcp-session-id'] as string | undefined; |
| } | ||
|
|
||
| const sessionId = request.headers['mcp-session-id'] as string | undefined; | ||
| const session = sessionId ? sessions.get(sessionId) : undefined; |
|
|
||
| const existingSessionId = request.headers['mcp-session-id'] as string | undefined; | ||
| if (existingSessionId) { | ||
| const session = sessions.get(existingSessionId); |
There was a problem hiding this comment.
why here we check different things for the mcp session?
|
|
||
| const projectId = await resolveProjectId(userId); | ||
| const settings = await getMcpEndpointSettings(projectId); | ||
| if (!settings.enabled) { |
There was a problem hiding this comment.
i think for the get or delete we should also check if enabled (and btw what the get and delete endpoint are for?)
Summary
Adds an HTTP MCP server (Streamable + SSE) on the backend to expose nao to external AI clients (Claude, Cursor, Codex…), with session management, TTL, and Better-Auth (Bearer + mcp plugin).
Exposes 4 families of MCP tools:
ask_nao(runs the agent and streams progress),execute_sql+build_chart(data warehouse),grep+ls(project files), and full story CRUD (list, get, create, update, archive, delete).Introduces standalone stories (stories without a parent chat): new userId column on story, new queries, dedicated frontend route, and integration into the stories list.
Adds an MCP settings page : toggles for enabled / agent mode / tools mode / objects mode, Bearer token display, and a recent call logs table.
Related issue
#648