mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-21 13:00:04 +08:00
improvement(copilot): trim copilot_chats reads to lean projections (#4629)
* improvement(copilot): trim copilot_chats reads to lean projections * fix(copilot): exercise idempotent-delete guard via the lean auth mock
This commit is contained in:
@@ -7,14 +7,16 @@ import { authMockFns, dbChainMock, dbChainMockFns } from '@sim/testing'
|
||||
import { NextRequest } from 'next/server'
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
const { mockGetAccessibleCopilotChat } = vi.hoisted(() => ({
|
||||
const { mockGetAccessibleCopilotChat, mockGetAccessibleCopilotChatAuth } = vi.hoisted(() => ({
|
||||
mockGetAccessibleCopilotChat: vi.fn(),
|
||||
mockGetAccessibleCopilotChatAuth: vi.fn(),
|
||||
}))
|
||||
|
||||
vi.mock('@sim/db', () => dbChainMock)
|
||||
|
||||
vi.mock('@/lib/copilot/chat/lifecycle', () => ({
|
||||
getAccessibleCopilotChat: mockGetAccessibleCopilotChat,
|
||||
getAccessibleCopilotChatAuth: mockGetAccessibleCopilotChatAuth,
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/copilot/tasks', () => ({
|
||||
@@ -39,6 +41,7 @@ describe('Copilot Chat Delete API Route', () => {
|
||||
|
||||
dbChainMockFns.returning.mockResolvedValue([{ workspaceId: 'ws-1' }])
|
||||
mockGetAccessibleCopilotChat.mockResolvedValue({ id: 'chat-123', userId: 'user-123' })
|
||||
mockGetAccessibleCopilotChatAuth.mockResolvedValue({ id: 'chat-123', userId: 'user-123' })
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
@@ -140,7 +143,7 @@ describe('Copilot Chat Delete API Route', () => {
|
||||
it('should delete chat even if it does not exist (idempotent)', async () => {
|
||||
authMockFns.mockGetSession.mockResolvedValue({ user: { id: 'user-123' } })
|
||||
|
||||
mockGetAccessibleCopilotChat.mockResolvedValueOnce(null)
|
||||
mockGetAccessibleCopilotChatAuth.mockResolvedValueOnce(null)
|
||||
|
||||
const req = createMockRequest('DELETE', {
|
||||
chatId: 'non-existent-chat',
|
||||
|
||||
@@ -6,7 +6,7 @@ import { type NextRequest, NextResponse } from 'next/server'
|
||||
import { deleteCopilotChatContract } from '@/lib/api/contracts/copilot'
|
||||
import { parseRequest } from '@/lib/api/server'
|
||||
import { getSession } from '@/lib/auth'
|
||||
import { getAccessibleCopilotChat } from '@/lib/copilot/chat/lifecycle'
|
||||
import { getAccessibleCopilotChatAuth } from '@/lib/copilot/chat/lifecycle'
|
||||
import { taskPubSub } from '@/lib/copilot/tasks'
|
||||
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
|
||||
|
||||
@@ -30,7 +30,7 @@ export const DELETE = withRouteHandler(async (request: NextRequest) => {
|
||||
if (!validated.success) return validated.response
|
||||
const parsed = validated.data.body
|
||||
|
||||
const chat = await getAccessibleCopilotChat(parsed.chatId, session.user.id)
|
||||
const chat = await getAccessibleCopilotChatAuth(parsed.chatId, session.user.id)
|
||||
if (!chat) {
|
||||
return NextResponse.json({ success: true })
|
||||
}
|
||||
|
||||
@@ -51,6 +51,21 @@ function transformChat(chat: {
|
||||
}
|
||||
}
|
||||
|
||||
type CopilotChatListRow = Pick<
|
||||
typeof copilotChats.$inferSelect,
|
||||
'id' | 'title' | 'model' | 'createdAt' | 'updatedAt'
|
||||
>
|
||||
|
||||
function transformChatListItem(chat: CopilotChatListRow) {
|
||||
return {
|
||||
id: chat.id,
|
||||
title: chat.title,
|
||||
model: chat.model,
|
||||
createdAt: chat.createdAt,
|
||||
updatedAt: chat.updatedAt,
|
||||
}
|
||||
}
|
||||
|
||||
export async function GET(req: NextRequest) {
|
||||
try {
|
||||
const { searchParams } = new URL(req.url)
|
||||
@@ -166,9 +181,6 @@ export async function GET(req: NextRequest) {
|
||||
id: copilotChats.id,
|
||||
title: copilotChats.title,
|
||||
model: copilotChats.model,
|
||||
messages: copilotChats.messages,
|
||||
planArtifact: copilotChats.planArtifact,
|
||||
config: copilotChats.config,
|
||||
createdAt: copilotChats.createdAt,
|
||||
updatedAt: copilotChats.updatedAt,
|
||||
})
|
||||
@@ -181,7 +193,7 @@ export async function GET(req: NextRequest) {
|
||||
|
||||
return NextResponse.json({
|
||||
success: true,
|
||||
chats: chats.map(transformChat),
|
||||
chats: chats.map(transformChatListItem),
|
||||
})
|
||||
} catch (error) {
|
||||
logger.error('Error fetching copilot chats:', error)
|
||||
|
||||
@@ -6,7 +6,7 @@ import { type NextRequest, NextResponse } from 'next/server'
|
||||
import { renameCopilotChatContract } from '@/lib/api/contracts/copilot'
|
||||
import { parseRequest, validationErrorResponse } from '@/lib/api/server'
|
||||
import { getSession } from '@/lib/auth'
|
||||
import { getAccessibleCopilotChat } from '@/lib/copilot/chat/lifecycle'
|
||||
import { getAccessibleCopilotChatAuth } from '@/lib/copilot/chat/lifecycle'
|
||||
import { taskPubSub } from '@/lib/copilot/tasks'
|
||||
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
|
||||
|
||||
@@ -30,7 +30,7 @@ export const PATCH = withRouteHandler(async (request: NextRequest) => {
|
||||
if (!parsed.success) return parsed.response
|
||||
const { chatId, title } = parsed.data.body
|
||||
|
||||
const chat = await getAccessibleCopilotChat(chatId, session.user.id)
|
||||
const chat = await getAccessibleCopilotChatAuth(chatId, session.user.id)
|
||||
if (!chat) {
|
||||
return NextResponse.json({ success: false, error: 'Chat not found' }, { status: 404 })
|
||||
}
|
||||
|
||||
@@ -5,7 +5,7 @@ import { eq } from 'drizzle-orm'
|
||||
import { type NextRequest, NextResponse } from 'next/server'
|
||||
import { updateCopilotMessagesContract } from '@/lib/api/contracts/copilot'
|
||||
import { parseRequest } from '@/lib/api/server'
|
||||
import { getAccessibleCopilotChat } from '@/lib/copilot/chat/lifecycle'
|
||||
import { getAccessibleCopilotChatAuth } from '@/lib/copilot/chat/lifecycle'
|
||||
import { normalizeMessage, type PersistedMessage } from '@/lib/copilot/chat/persisted-message'
|
||||
import {
|
||||
authenticateCopilotRequestSessionOnly,
|
||||
@@ -66,7 +66,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
|
||||
}
|
||||
|
||||
// Verify that the chat belongs to the user
|
||||
const chat = await getAccessibleCopilotChat(chatId, userId)
|
||||
const chat = await getAccessibleCopilotChatAuth(chatId, userId)
|
||||
|
||||
if (!chat) {
|
||||
return createNotFoundResponse('Chat not found or unauthorized')
|
||||
|
||||
@@ -36,6 +36,7 @@ vi.mock('@/lib/workflows/utils', () => workflowsUtilsMock)
|
||||
|
||||
vi.mock('@/lib/copilot/chat/lifecycle', () => ({
|
||||
getAccessibleCopilotChat: mockGetAccessibleCopilotChat,
|
||||
getAccessibleCopilotChatAuth: mockGetAccessibleCopilotChat,
|
||||
}))
|
||||
|
||||
vi.mock('@sim/db', () => ({
|
||||
|
||||
@@ -7,7 +7,7 @@ import { type NextRequest, NextResponse } from 'next/server'
|
||||
import { revertCopilotCheckpointContract } from '@/lib/api/contracts/copilot'
|
||||
import type { CleanedWorkflowState } from '@/lib/api/contracts/workflows'
|
||||
import { parseRequest } from '@/lib/api/server'
|
||||
import { getAccessibleCopilotChat } from '@/lib/copilot/chat/lifecycle'
|
||||
import { getAccessibleCopilotChatAuth } from '@/lib/copilot/chat/lifecycle'
|
||||
import {
|
||||
authenticateCopilotRequestSessionOnly,
|
||||
createInternalServerErrorResponse,
|
||||
@@ -57,7 +57,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
|
||||
return createNotFoundResponse('Checkpoint not found or access denied')
|
||||
}
|
||||
|
||||
const chat = await getAccessibleCopilotChat(checkpoint.chatId, userId)
|
||||
const chat = await getAccessibleCopilotChatAuth(checkpoint.chatId, userId)
|
||||
if (!chat) {
|
||||
return createNotFoundResponse('Checkpoint not found or access denied')
|
||||
}
|
||||
|
||||
@@ -44,6 +44,7 @@ vi.mock('drizzle-orm', () => ({
|
||||
|
||||
vi.mock('@/lib/copilot/chat/lifecycle', () => ({
|
||||
getAccessibleCopilotChat: mockGetAccessibleCopilotChat,
|
||||
getAccessibleCopilotChatAuth: mockGetAccessibleCopilotChat,
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/workflows/utils', () => workflowsUtilsMock)
|
||||
|
||||
@@ -9,7 +9,7 @@ import {
|
||||
listCopilotCheckpointsContract,
|
||||
} from '@/lib/api/contracts/copilot'
|
||||
import { getValidationErrorMessage, parseRequest, validationErrorResponse } from '@/lib/api/server'
|
||||
import { getAccessibleCopilotChat } from '@/lib/copilot/chat/lifecycle'
|
||||
import { getAccessibleCopilotChatAuth } from '@/lib/copilot/chat/lifecycle'
|
||||
import {
|
||||
authenticateCopilotRequestSessionOnly,
|
||||
createBadRequestResponse,
|
||||
@@ -60,7 +60,7 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
|
||||
})
|
||||
|
||||
// Verify that the chat belongs to the user
|
||||
const chat = await getAccessibleCopilotChat(chatId, userId)
|
||||
const chat = await getAccessibleCopilotChatAuth(chatId, userId)
|
||||
|
||||
if (!chat) {
|
||||
return createBadRequestResponse('Chat not found or unauthorized')
|
||||
@@ -159,7 +159,7 @@ export const GET = withRouteHandler(async (req: NextRequest) => {
|
||||
chatId,
|
||||
})
|
||||
|
||||
const chat = await getAccessibleCopilotChat(chatId, userId)
|
||||
const chat = await getAccessibleCopilotChatAuth(chatId, userId)
|
||||
if (!chat) {
|
||||
return createBadRequestResponse('Chat not found or unauthorized')
|
||||
}
|
||||
|
||||
@@ -48,6 +48,7 @@ vi.mock('@/lib/copilot/request/http', () => copilotHttpMock)
|
||||
|
||||
vi.mock('@/lib/copilot/chat/lifecycle', () => ({
|
||||
getAccessibleCopilotChat: mockGetAccessibleCopilotChat,
|
||||
getAccessibleCopilotChatAuth: mockGetAccessibleCopilotChat,
|
||||
}))
|
||||
|
||||
vi.mock('@/lib/copilot/chat/stream-liveness', () => ({
|
||||
|
||||
@@ -12,7 +12,10 @@ import {
|
||||
import { parseRequest } from '@/lib/api/server'
|
||||
import { getLatestRunForStream } from '@/lib/copilot/async-runs/repository'
|
||||
import { buildEffectiveChatTranscript } from '@/lib/copilot/chat/effective-transcript'
|
||||
import { getAccessibleCopilotChat } from '@/lib/copilot/chat/lifecycle'
|
||||
import {
|
||||
getAccessibleCopilotChat,
|
||||
getAccessibleCopilotChatAuth,
|
||||
} from '@/lib/copilot/chat/lifecycle'
|
||||
import { normalizeMessage } from '@/lib/copilot/chat/persisted-message'
|
||||
import { reconcileChatStreamMarkers } from '@/lib/copilot/chat/stream-liveness'
|
||||
import {
|
||||
@@ -238,7 +241,7 @@ export const DELETE = withRouteHandler(
|
||||
if (!parsed.success) return parsed.response
|
||||
const { chatId } = parsed.data.params
|
||||
|
||||
const chat = await getAccessibleCopilotChat(chatId, userId)
|
||||
const chat = await getAccessibleCopilotChatAuth(chatId, userId)
|
||||
if (!chat || chat.type !== 'mothership') {
|
||||
return NextResponse.json({ success: true })
|
||||
}
|
||||
|
||||
@@ -418,10 +418,6 @@ const copilotChatGetListItemSchema = z
|
||||
id: z.string(),
|
||||
title: z.string().nullable(),
|
||||
model: z.string().nullable(),
|
||||
messages: z.array(z.unknown()),
|
||||
messageCount: z.number(),
|
||||
planArtifact: z.unknown().nullable(),
|
||||
config: z.unknown().nullable(),
|
||||
createdAt: z.string().nullable(),
|
||||
updatedAt: z.string().nullable(),
|
||||
})
|
||||
|
||||
@@ -20,13 +20,30 @@ export interface ChatLoadResult {
|
||||
isNew: boolean
|
||||
}
|
||||
|
||||
export async function getAccessibleCopilotChat(chatId: string, userId: string) {
|
||||
const [chat] = await db
|
||||
.select()
|
||||
.from(copilotChats)
|
||||
.where(and(eq(copilotChats.id, chatId), eq(copilotChats.userId, userId)))
|
||||
.limit(1)
|
||||
/**
|
||||
* Minimal column set needed to perform workflow/workspace authorization for a
|
||||
* copilot chat. Heavy TOAST-able columns (messages, planArtifact, previewYaml,
|
||||
* config, resources) are intentionally excluded — callers that only need to
|
||||
* verify ownership should not pay the detoast cost for those fields.
|
||||
*/
|
||||
const copilotChatAuthColumns = {
|
||||
id: copilotChats.id,
|
||||
userId: copilotChats.userId,
|
||||
workflowId: copilotChats.workflowId,
|
||||
workspaceId: copilotChats.workspaceId,
|
||||
type: copilotChats.type,
|
||||
} as const
|
||||
|
||||
type CopilotChatAuthRow = Pick<
|
||||
typeof copilotChats.$inferSelect,
|
||||
'id' | 'userId' | 'workflowId' | 'workspaceId' | 'type'
|
||||
>
|
||||
|
||||
async function authorizeCopilotChatRow<T extends CopilotChatAuthRow>(
|
||||
chat: T | undefined,
|
||||
chatId: string,
|
||||
userId: string
|
||||
): Promise<T | null> {
|
||||
if (!chat) {
|
||||
logger.warn('Copilot chat not found or not owned by user', { chatId, userId })
|
||||
return null
|
||||
@@ -61,6 +78,40 @@ export async function getAccessibleCopilotChat(chatId: string, userId: string) {
|
||||
return chat
|
||||
}
|
||||
|
||||
/**
|
||||
* Verify a copilot chat exists, is owned by the user, and the user has access
|
||||
* to its workflow/workspace. Selects only the columns required for the
|
||||
* authorization check — use this for routes that only need ownership
|
||||
* verification before a mutation (rename, delete, update-messages).
|
||||
*/
|
||||
export async function getAccessibleCopilotChatAuth(
|
||||
chatId: string,
|
||||
userId: string
|
||||
): Promise<CopilotChatAuthRow | null> {
|
||||
const [chat] = await db
|
||||
.select(copilotChatAuthColumns)
|
||||
.from(copilotChats)
|
||||
.where(and(eq(copilotChats.id, chatId), eq(copilotChats.userId, userId)))
|
||||
.limit(1)
|
||||
|
||||
return authorizeCopilotChatRow(chat, chatId, userId)
|
||||
}
|
||||
|
||||
/**
|
||||
* Load the full copilot chat row after authorization. Use this only when the
|
||||
* caller actually consumes the heavy columns (`messages`, `planArtifact`,
|
||||
* `config`, etc.) — for example, chat resume or the GET-by-id endpoint.
|
||||
*/
|
||||
export async function getAccessibleCopilotChat(chatId: string, userId: string) {
|
||||
const [chat] = await db
|
||||
.select()
|
||||
.from(copilotChats)
|
||||
.where(and(eq(copilotChats.id, chatId), eq(copilotChats.userId, userId)))
|
||||
.limit(1)
|
||||
|
||||
return authorizeCopilotChatRow(chat, chatId, userId)
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve or create a copilot chat session.
|
||||
* If chatId is provided, loads the existing chat. Otherwise creates a new one.
|
||||
|
||||
Reference in New Issue
Block a user