diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index 8de3ee7cb16..65cbbbb47be 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -566,7 +566,7 @@ "get": { "operationId": "downloadFile", "summary": "Download File", - "description": "Download the current file bytes from a workspace.", + "description": "Download the current file bytes from a workspace. A generated document is served as its compiled artifact, so it returns `409` while that artifact is still compiling and `413` if it renders past the size ceiling.", "tags": ["Files"], "parameters": [ { @@ -638,6 +638,12 @@ "404": { "$ref": "#/components/responses/NotFound" }, + "409": { + "$ref": "#/components/responses/Conflict" + }, + "413": { + "$ref": "#/components/responses/PayloadTooLarge" + }, "429": { "$ref": "#/components/responses/RateLimited" }, diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index 8ca5e1e66aa..d2fd212d5b6 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -273,7 +273,7 @@ } } }, - "put": { + "patch": { "operationId": "updateKnowledgeBase", "summary": "Update Knowledge Base", "description": "Update a knowledge base name, description, chunking configuration, or folder placement.", diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index 215dab6bafe..8af5c3373f1 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -864,7 +864,7 @@ } } }, - "put": { + "patch": { "operationId": "updateTableRows", "summary": "Update Rows by Filter", "description": "Apply the same partial data patch to every row matching a non-empty predicate.", diff --git a/apps/sim/app/api/files/serve/[...path]/route.ts b/apps/sim/app/api/files/serve/[...path]/route.ts index a94eee6b48d..7ba54de8e06 100644 --- a/apps/sim/app/api/files/serve/[...path]/route.ts +++ b/apps/sim/app/api/files/serve/[...path]/route.ts @@ -4,10 +4,8 @@ import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' import { fileServeParamsSchema, fileServeQuerySchema } from '@/lib/api/contracts/storage-transfer' import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' -import { - DocCompileUserError, - resolveServableDocBytes, -} from '@/lib/copilot/tools/server/files/doc-compile' +import { resolveServableDocBytes } from '@/lib/copilot/tools/server/files/doc-compile' +import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { CopilotFiles, isUsingCloudStorage } from '@/lib/uploads' import type { StorageContext } from '@/lib/uploads/config' diff --git a/apps/sim/app/api/v1/files/[fileId]/route.test.ts b/apps/sim/app/api/v1/files/[fileId]/route.test.ts index 34cbd6a1e79..8e4927490e3 100644 --- a/apps/sim/app/api/v1/files/[fileId]/route.test.ts +++ b/apps/sim/app/api/v1/files/[fileId]/route.test.ts @@ -37,7 +37,7 @@ vi.mock('@sim/audit', () => ({ })) vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() })) -import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile' +import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error' import { GET } from '@/app/api/v1/files/[fileId]/route' const WORKSPACE_ID = 'ws-1' diff --git a/apps/sim/app/api/v1/files/[fileId]/route.ts b/apps/sim/app/api/v1/files/[fileId]/route.ts index 89c63c37d31..1e9b084e680 100644 --- a/apps/sim/app/api/v1/files/[fileId]/route.ts +++ b/apps/sim/app/api/v1/files/[fileId]/route.ts @@ -10,7 +10,7 @@ import { fetchServableWorkspaceFileBuffer, getWorkspaceFile, } from '@/lib/uploads/contexts/workspace' -import { docNotReadyMessage, isDocNotReadyError } from '@/lib/uploads/utils/servable-file-response' +import { docNotReadyMessage, isDocNotReadyError } from '@/lib/uploads/utils/doc-not-ready' import { performDeleteWorkspaceFileItems } from '@/lib/workspace-files/orchestration' import { checkRateLimit, diff --git a/apps/sim/app/api/v2/files/[fileId]/route.test.ts b/apps/sim/app/api/v2/files/[fileId]/route.test.ts index fffb6da3349..907303c57b0 100644 --- a/apps/sim/app/api/v2/files/[fileId]/route.test.ts +++ b/apps/sim/app/api/v2/files/[fileId]/route.test.ts @@ -111,6 +111,8 @@ describe('v2 single-file routes', () => { mocks.download.mockResolvedValue({ file: fileRecord(), stream: new Blob(['id,name\n']).stream(), + contentType: 'text/csv', + contentLength: 'id,name\n'.length, }) mocks.rename.mockResolvedValue({ file: fileRecord({ name: 'renamed.csv' }) }) mocks.deleteFile.mockResolvedValue({ diff --git a/apps/sim/app/api/v2/files/[fileId]/route.ts b/apps/sim/app/api/v2/files/[fileId]/route.ts index 5db1ff967de..10b1ca6c94d 100644 --- a/apps/sim/app/api/v2/files/[fileId]/route.ts +++ b/apps/sim/app/api/v2/files/[fileId]/route.ts @@ -25,6 +25,8 @@ export const revalidate = 0 * The response carries no JSON envelope, so rate-limit state is surfaced via * `X-RateLimit-*` headers. Errors still render the canonical v2 JSON error body. * Lookups are workspace-scoped (IDOR-safe): a file in another workspace 404s. + * + * A generated doc whose artifact is still compiling renders `CONFLICT`; retry. */ export const GET = defineV2BinaryRoute({ contract: v2DownloadFileContract, @@ -37,11 +39,11 @@ export const GET = defineV2BinaryRoute({ assertedWorkspaceId: query.workspaceId, }), useCase: downloadWorkspaceFileStream, - present: ({ file, stream }) => ({ + present: ({ file, stream, contentType, contentLength }) => ({ body: stream, - contentType: file.type || 'application/octet-stream', + contentType, contentDisposition: `attachment; filename="${file.name.replace(/[^\w.-]/g, '_')}"; filename*=UTF-8''${encodeURIComponent(file.name)}`, - contentLength: file.size, + contentLength, }), }) diff --git a/apps/sim/app/api/v2/knowledge/[id]/route.ts b/apps/sim/app/api/v2/knowledge/[id]/route.ts index dce1a6ccccb..c2047280fc9 100644 --- a/apps/sim/app/api/v2/knowledge/[id]/route.ts +++ b/apps/sim/app/api/v2/knowledge/[id]/route.ts @@ -35,8 +35,8 @@ export const GET = defineV2JsonRoute({ }), }) -/** PUT /api/v2/knowledge/[id] — Update a knowledge base. */ -export const PUT = defineV2JsonRoute({ +/** PATCH /api/v2/knowledge/[id] — Partially update a knowledge base. */ +export const PATCH = defineV2JsonRoute({ contract: v2UpdateKnowledgeBaseContract, auth: v2ApiKeyAuth, operation: knowledgeOperations.update, diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts index 11ea0842bfb..05c89c2d310 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/route.test.ts @@ -42,7 +42,7 @@ vi.mock('@/lib/table/application/rows', () => ({ deleteTableRows: { operation: { id: 'tables.rows.delete_many' }, execute: mocks.deleteRows }, })) -import { DELETE, GET, POST, PUT } from '@/app/api/v2/tables/[tableId]/rows/route' +import { DELETE, GET, PATCH, POST } from '@/app/api/v2/tables/[tableId]/rows/route' const WORKSPACE_ID = 'workspace-1' const PRINCIPAL = { @@ -76,7 +76,7 @@ const ROW = { } const CONTEXT = { params: Promise.resolve({ tableId: 'table-1' }) } -function request(method: 'GET' | 'POST' | 'PUT' | 'DELETE', body?: unknown, query = '') { +function request(method: 'GET' | 'POST' | 'PATCH' | 'DELETE', body?: unknown, query = '') { return new NextRequest(`http://localhost/api/v2/tables/table-1/rows${query}`, { method, headers: { @@ -159,12 +159,12 @@ describe('/api/v2/tables/[tableId]/rows', () => { it('preserves authoritative bulk update counts including a zero-match result', async () => { mocks.updateRows.mockResolvedValue({ table: TABLE, affectedCount: 0, affectedRowIds: [] }) - const req = request('PUT', { + const req = request('PATCH', { workspaceId: WORKSPACE_ID, filter: { all: [{ field: 'name', op: 'eq', value: 'missing' }] }, data: { name: 'Grace' }, }) - const response = await PUT(req, CONTEXT) + const response = await PATCH(req, CONTEXT) expect(response.status).toBe(200) expect(await response.json()).toEqual({ data: { updatedCount: 0, updatedRowIds: [] } }) diff --git a/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts b/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts index d597c5bdc8e..3d3382cfcff 100644 --- a/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts +++ b/apps/sim/app/api/v2/tables/[tableId]/rows/route.ts @@ -76,7 +76,7 @@ export const POST = defineV2JsonRoute({ }, }) -export const PUT = defineV2JsonRoute({ +export const PATCH = defineV2JsonRoute({ contract: v2UpdateRowsByFilterContract, operation: tableOperations.updateRows, auth: v2ApiKeyAuth, diff --git a/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts b/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts new file mode 100644 index 00000000000..4c48242ec33 --- /dev/null +++ b/apps/sim/lib/api/contracts/v2/__tests__/list-pagination.test.ts @@ -0,0 +1,196 @@ +/** + * @vitest-environment node + */ +import { readdirSync } from 'node:fs' +import path from 'node:path' +import { describe, expect, it } from 'vitest' +import type { z } from 'zod' + +/** + * Pins which v2 lists are paged. + * + * Every v2 list returns the same `{ data, nextCursor }` envelope, but only some + * accept `limit` + `cursor` and can return a non-null `nextCursor`. That split + * is a documented part of the public contract (`v2/shared.ts`) and has already + * drifted once — `GET /api/v2/tables` gained real pagination while its contract + * docstring still claimed a single full page. Enumerating the two sets here + * makes the next such change fail a test instead of rotting a comment, and + * makes flipping a shipped list from full-set to paged a deliberate edit: a + * `limit` with a default silently truncates callers that read the full set + * today. + */ + +const CONTRACTS_DIR = path.resolve(import.meta.dirname, '..', '..') + +/** Lists that accept `limit` + `cursor` and can return a non-null `nextCursor`. */ +const PAGED_LISTS = [ + 'GET /api/v2/audit-logs', + 'GET /api/v2/billing/logs', + 'GET /api/v2/files', + 'GET /api/v2/knowledge/[id]/documents', + 'GET /api/v2/logs', + 'GET /api/v2/tables', + 'GET /api/v2/tables/[tableId]/rows', + 'POST /api/v2/tables/[tableId]/query', + 'GET /api/v2/workflows', + 'GET /api/v2/workflows/[id]/runs', + 'GET /api/v2/workflows/[id]/versions', + 'GET /api/v2/workspaces/[workspaceId]/members', +] as const + +/** + * Lists that accept neither param and always return `nextCursor: null`, because + * the set is small and bounded per workspace or per table. + */ +const FULL_SET_LISTS = [ + 'GET /api/v2/credentials', + 'GET /api/v2/custom-tools', + 'GET /api/v2/files/folders', + 'GET /api/v2/knowledge', + 'GET /api/v2/knowledge/folders', + 'GET /api/v2/mcp-servers', + 'GET /api/v2/secrets', + 'GET /api/v2/skills', + 'GET /api/v2/tables/[tableId]/groups', + 'GET /api/v2/tables/[tableId]/views', + 'GET /api/v2/tables/folders', + 'GET /api/v2/workflows/folders', +] as const + +interface ContractLike { + method: string + path: string + query?: z.ZodType + body?: z.ZodType + response?: { mode: string; schema?: z.ZodType } +} + +function isContract(value: unknown): value is ContractLike { + return ( + !!value && + typeof value === 'object' && + typeof (value as ContractLike).method === 'string' && + typeof (value as ContractLike).path === 'string' && + typeof (value as ContractLike).response === 'object' + ) +} + +/** Unwraps `.optional()` / `.default()` / `.superRefine()` / pipe wrappers to the object underneath. */ +function objectShape(schema: z.ZodType | undefined): Record | null { + let current: unknown = schema + for (let depth = 0; current && depth < 8; depth++) { + const def = (current as { def?: Record }).def + if (!def) return null + if (def.type === 'object') return def.shape as Record + current = def.innerType ?? def.in ?? def.schema + } + return null +} + +/** A `{ data: T[], nextCursor: string | null }` response. */ +function isListResponse(schema: z.ZodType | undefined): boolean { + const shape = objectShape(schema) + return !!shape && 'data' in shape && 'nextCursor' in shape +} + +function listContractFiles(dir: string): string[] { + const files: string[] = [] + for (const entry of readdirSync(dir, { withFileTypes: true }).sort((a, b) => + a.name.localeCompare(b.name) + )) { + const full = path.join(dir, entry.name) + if (entry.isDirectory()) { + if (entry.name === '__tests__') continue + files.push(...listContractFiles(full)) + } else if (entry.name.endsWith('.ts') && !entry.name.endsWith('.test.ts')) { + files.push(full) + } + } + return files +} + +interface V2ListContract { + key: string + name: string + paginationParams: string[] +} + +/** + * Sweeping the contracts tree costs a few hundred dynamic imports, so it is done + * once for the whole file rather than repeated per test. + */ +let contractsPromise: Promise | null = null +function loadV2ListContracts(): Promise { + contractsPromise ??= sweepV2ListContracts() + return contractsPromise +} + +async function sweepV2ListContracts(): Promise { + const found = new Map() + for (const file of listContractFiles(CONTRACTS_DIR)) { + const mod = (await import(file)) as Record + for (const [name, value] of Object.entries(mod)) { + if (!isContract(value)) continue + if (!value.path.startsWith('/api/v2/')) continue + if (!isListResponse(value.response?.schema)) continue + const key = `${value.method.toUpperCase()} ${value.path}` + if (found.has(key)) continue + const inputShape = { ...objectShape(value.query), ...objectShape(value.body) } + found.set(key, { + key, + name, + paginationParams: ['limit', 'cursor'].filter((param) => param in inputShape), + }) + } + } + return [...found.values()].sort((a, b) => a.key.localeCompare(b.key)) +} + +/** + * Only the first test pays for the sweep; the rest await the memoized promise. + * It costs ~2s standalone but has exceeded the default 10s under full-suite + * thread contention, so it gets headroom the other two do not need. + */ +const SWEEP_TIMEOUT_MS = 60_000 + +describe('v2 list pagination split', () => { + it( + 'classifies every v2 list as paged or full-set', + async () => { + const contracts = await loadV2ListContracts() + const classified = new Set([...PAGED_LISTS, ...FULL_SET_LISTS]) + const unclassified = contracts.filter((c) => !classified.has(c.key)).map((c) => c.key) + + expect( + unclassified, + 'A new v2 list must be classified in PAGED_LISTS or FULL_SET_LISTS.' + ).toEqual([]) + expect(contracts.map((c) => c.key).sort()).toEqual([...classified].sort()) + }, + SWEEP_TIMEOUT_MS + ) + + it('gives every paged list both limit and cursor', async () => { + const contracts = await loadV2ListContracts() + const byKey = new Map(contracts.map((c) => [c.key, c])) + + for (const key of PAGED_LISTS) { + expect(byKey.get(key)?.paginationParams, `${key} is declared paged`).toEqual([ + 'limit', + 'cursor', + ]) + } + }) + + it('gives every full-set list neither limit nor cursor', async () => { + const contracts = await loadV2ListContracts() + const byKey = new Map(contracts.map((c) => [c.key, c])) + + for (const key of FULL_SET_LISTS) { + expect( + byKey.get(key)?.paginationParams, + `${key} returns the full set; adding a defaulted limit would truncate existing callers` + ).toEqual([]) + } + }) +}) diff --git a/apps/sim/lib/api/contracts/v2/knowledge.ts b/apps/sim/lib/api/contracts/v2/knowledge.ts index 9a078e7524e..e250daca6ec 100644 --- a/apps/sim/lib/api/contracts/v2/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/knowledge.ts @@ -649,8 +649,12 @@ export const v2GetKnowledgeBaseContract = defineRouteContract({ }, }) +/** + * PATCH, not PUT: every mutable field is optional and a `superRefine` requires + * at least one, so this is a partial update rather than a replacement. + */ export const v2UpdateKnowledgeBaseContract = defineRouteContract({ - method: 'PUT', + method: 'PATCH', path: '/api/v2/knowledge/[id]', params: v2KnowledgeBaseParamsSchema, body: v2UpdateKnowledgeBaseBodySchema, diff --git a/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts b/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts index 1370c7fa8d7..a9a0138ca4d 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/files-audit.ts @@ -324,8 +324,9 @@ const routes = [ filesOperation({ operationId: 'downloadFile', summary: 'Download File', - description: 'Download the current file bytes from a workspace.', - errors: [...WORKSPACE_ERRORS, 'NotFound'], + description: + 'Download the current file bytes from a workspace. A generated document is served as its compiled artifact, so it returns `409` while that artifact is still compiling and `413` if it renders past the size ceiling.', + errors: [...WORKSPACE_ERRORS, 'NotFound', 'Conflict', 'PayloadTooLarge'], success: { description: 'The file bytes.', headers: ['Content-Type', 'Content-Disposition', 'Content-Length'], diff --git a/apps/sim/lib/api/contracts/v2/shared.ts b/apps/sim/lib/api/contracts/v2/shared.ts index ad6d1e31529..132c81cc3ed 100644 --- a/apps/sim/lib/api/contracts/v2/shared.ts +++ b/apps/sim/lib/api/contracts/v2/shared.ts @@ -11,9 +11,14 @@ import { FolderPathError, parseFolderPath, requireNonRootFolderPath } from '@/li * - list: `{ data: T[], nextCursor: string | null }` * - error: `{ error: { code, message, details? } }` * - * Every list uses the opaque-cursor envelope (Stripe/Slack-style): `limit` + - * `cursor` in, `{ data, nextCursor }` out. Cursors are opaque so the underlying - * scheme (keyset / offset / full-set) can change without a contract change. + * Every list returns the opaque-cursor envelope (Stripe/Slack-style) + * `{ data, nextCursor }`, but not every list is *paged*. A paged list also + * accepts `limit` + `cursor` and can return a non-null `nextCursor`; a list + * whose result set is small and bounded by construction accepts neither and + * returns the whole set as one page with `nextCursor` always `null`. Sharing + * the envelope regardless is what lets a full-set list gain real pages later + * without a contract change: the cursor is opaque, so the scheme behind it + * (keyset / offset / full-set) is not part of the interface. * Total counts are not returned on lists — they're available on the parent * resource where relevant (e.g. `rowCount` on a table, `docCount` on a KB). * @@ -46,17 +51,30 @@ import { FolderPathError, parseFolderPath, requireNonRootFolderPath } from '@/li * on the surface (`scope`, `folderPath`, `deployedOnly`, `type`, `providerId`, * `resourceType`). No generic filter expression. * - * Every one of these is pushed into SQL. No v2 list fetches a full result set - * to filter or sort it in memory. + * Every one of these is pushed into SQL, except on `GET /skills` (which merges + * the static builtin registry into the DB rows, then re-filters and re-sorts the + * merged array) and `GET /files/folders` (which applies `parentPath`, `search`, + * and the sort in JS). Both read a full result set to produce a page; neither is + * a pattern to copy. + * + * ## Which lists are paged + * + * The authoritative split is pinned in `v2/__tests__/list-pagination.test.ts`, + * not restated here. Adding `limit`/`cursor` to a full-set list is additive, + * but making a `limit` *default* would silently truncate callers that rely on + * the full set today, so a default page size cannot be introduced without a + * version bump. * * ## Sort and the opaque cursor * - * On the lists that paginate ({@link v2CursorListResponse} with a non-null - * `nextCursor` — files and workflows), the cursor is a keyset over the *active* - * sort, so its keys change when the sort does. The sort is therefore encoded - * into the cursor and re-checked on the way back in: replaying a cursor under a - * different `sortBy`/`sortOrder` is a 400, not a silently duplicated or skipped - * page. Change the sort by restarting pagination without a cursor. + * Lists using the shared keyset codec (`encodeSortedCursor` / + * `decodeSortedCursor` in `app/api/v2/lib/response.ts`) carry a cursor that is + * a keyset over the *active* sort, so its keys change when the sort does. The + * sort is therefore encoded into the cursor and re-checked on the way back in: + * replaying a cursor under a different `sortBy`/`sortOrder` is a 400, not a + * silently duplicated or skipped page. Change the sort by restarting pagination + * without a cursor. The rest delegate to their domain's own cursor codec, which + * is opaque in exactly the same way. */ /** Canonical v2 error envelope. */ diff --git a/apps/sim/lib/api/contracts/v2/tables.ts b/apps/sim/lib/api/contracts/v2/tables.ts index 94eb8724fc3..aae2fe53172 100644 --- a/apps/sim/lib/api/contracts/v2/tables.ts +++ b/apps/sim/lib/api/contracts/v2/tables.ts @@ -408,12 +408,10 @@ export const v2CreateTableBodySchema = v1CreateTableBodySchema export type V2CreateTableBody = z.input /** - * Table list. `listTables` returns every table in the workspace (a small, - * bounded per-workspace set), so today the cursor list is a single full page - * (`nextCursor` is always `null`). Using the canonical cursor envelope keeps the - * whole v2 list surface uniform, and real pagination can be added later behind - * the opaque cursor without an interface change. Search, folder filter, and - * sort all run in that query, not over its result. + * Table list. Keyset-paged over the active sort: `limit` (default 100, clamped + * to 1..1000) plus an opaque sort-stamped `cursor`, so `nextCursor` is a real + * cursor and not always `null`. Search, folder filter, sort, and the keyset all + * run in the query, not over its result. */ export const v2ListTablesContract = defineRouteContract({ method: 'GET', @@ -767,8 +765,13 @@ export const v2UpdateRowsByPredicateBodySchema = updateRowsByFilterBodySchema.ex }) export type V2UpdateRowsByPredicateBody = z.input +/** + * PATCH, not PUT: the body carries a row-data *patch* applied to every matching + * row, never a replacement row. Pairs with `PATCH /rows/[rowId]` so partial + * update is the same verb whether it targets one row or a predicate's worth. + */ export const v2UpdateRowsByFilterContract = defineRouteContract({ - method: 'PUT', + method: 'PATCH', path: '/api/v2/tables/[tableId]/rows', params: tableIdParamsSchema, body: v2UpdateRowsByPredicateBodySchema, diff --git a/apps/sim/lib/copilot/tools/server/files/doc-compile-error.ts b/apps/sim/lib/copilot/tools/server/files/doc-compile-error.ts new file mode 100644 index 00000000000..c9aa344c63a --- /dev/null +++ b/apps/sim/lib/copilot/tools/server/files/doc-compile-error.ts @@ -0,0 +1,15 @@ +/** + * Signals that a generated document's artifact is still compiling, so the caller + * should retry rather than treat the read as a fault. + * + * Lives in its own leaf module — with no imports — so that callers which only + * need to *recognize* this error do not pull in `doc-compile`, which reaches + * `app/api/**` and therefore `next/server`. Application modules must not take + * that edge. + */ +export class DocCompileUserError extends Error { + constructor(message: string) { + super(message) + this.name = 'DocCompileUserError' + } +} diff --git a/apps/sim/lib/copilot/tools/server/files/doc-compile.ts b/apps/sim/lib/copilot/tools/server/files/doc-compile.ts index 77f5fa70cd9..72029072f53 100644 --- a/apps/sim/lib/copilot/tools/server/files/doc-compile.ts +++ b/apps/sim/lib/copilot/tools/server/files/doc-compile.ts @@ -1,6 +1,7 @@ import type { Principal } from '@sim/auth/principal' import { createLogger } from '@sim/logger' import { sha256Hex } from '@sim/security/hash' +import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error' import { isDocSandboxEnabled } from '@/lib/core/config/env-flags' import { CodeLanguage } from '@/lib/execution/languages' import { @@ -18,20 +19,6 @@ import { loadCompiledDoc, storeCompiledDoc } from './doc-compiled-store' const logger = createLogger('CopilotDocCompile') -/** - * Thrown when the user-authored Python script itself fails (raised an exception - * or produced no output) — i.e. an error the agent should fix by editing the - * script. Infra failures (E2B sandbox create/timeout, S3) propagate as plain - * Errors so callers can return 5xx instead of telling the agent its script was - * wrong. - */ -export class DocCompileUserError extends Error { - constructor(message: string) { - super(message) - this.name = 'DocCompileUserError' - } -} - const PPTX_MIME = 'application/vnd.openxmlformats-officedocument.presentationml.presentation' const DOCX_MIME = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' const XLSX_MIME = 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet' diff --git a/apps/sim/lib/copilot/tools/server/files/doc-recalc.ts b/apps/sim/lib/copilot/tools/server/files/doc-recalc.ts index 08dc72e16d4..700d00746dd 100644 --- a/apps/sim/lib/copilot/tools/server/files/doc-recalc.ts +++ b/apps/sim/lib/copilot/tools/server/files/doc-recalc.ts @@ -1,8 +1,9 @@ import type { Principal } from '@sim/auth/principal' import { createLogger } from '@sim/logger' +import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error' import { CodeLanguage } from '@/lib/execution/languages' import { executeInSandbox } from '@/lib/execution/remote-sandbox' -import { compileDoc, DocCompileUserError } from './doc-compile' +import { compileDoc } from './doc-compile' const logger = createLogger('CopilotDocRecalc') diff --git a/apps/sim/lib/copilot/tools/server/files/doc-servable.test.ts b/apps/sim/lib/copilot/tools/server/files/doc-servable.test.ts index 3798aa237f1..d7b4081a700 100644 --- a/apps/sim/lib/copilot/tools/server/files/doc-servable.test.ts +++ b/apps/sim/lib/copilot/tools/server/files/doc-servable.test.ts @@ -49,12 +49,8 @@ vi.mock('@/app/api/files/utils', () => ({ : 'application/octet-stream', })) -import { - compileDoc, - DocCompileUserError, - resolveServableDoc, - resolveServableDocBytes, -} from './doc-compile' +import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error' +import { compileDoc, resolveServableDoc, resolveServableDocBytes } from './doc-compile' const WORKSPACE_ID = '550e8400-e29b-41d4-a716-446655440000' const FILE_PRINCIPAL = { kind: 'session', userId: 'user-1' } as const diff --git a/apps/sim/lib/copilot/tools/server/files/workspace-file.ts b/apps/sim/lib/copilot/tools/server/files/workspace-file.ts index 3ee928009de..f40fbe00652 100644 --- a/apps/sim/lib/copilot/tools/server/files/workspace-file.ts +++ b/apps/sim/lib/copilot/tools/server/files/workspace-file.ts @@ -16,6 +16,7 @@ import { type BaseServerTool, type ServerToolContext, } from '@/lib/copilot/tools/server/base-tool' +import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error' import { isDocSandboxEnabled } from '@/lib/core/config/env-flags' import { asOrchestrationError } from '@/lib/core/orchestration/types' import { runSandboxTask } from '@/lib/execution/sandbox/run-task' @@ -33,7 +34,6 @@ import type { SandboxTaskId } from '@/sandbox-tasks/registry' import { compileDoc, DOCXJS_SOURCE_MIME, - DocCompileUserError, getE2BDocFormat, PPTXGENJS_SOURCE_MIME, } from './doc-compile' diff --git a/apps/sim/lib/logs/application/get-public-log.ts b/apps/sim/lib/logs/application/get-public-log.ts index 4f9cdc0b3d7..c4ff2b8fb2b 100644 --- a/apps/sim/lib/logs/application/get-public-log.ts +++ b/apps/sim/lib/logs/application/get-public-log.ts @@ -2,7 +2,7 @@ import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { loadActiveFolderPathIndex } from '@/lib/folders/queries' import { logOperations } from '@/lib/logs/application/operations' -import { materializeExecutionData } from '@/lib/logs/execution/trace-store' +import { materializeExecutionDataForDisplay } from '@/lib/logs/execution/trace-store' import { getPublicWorkflowLog, getPublicWorkflowLogScope } from '@/lib/logs/public-queries' import { type ActiveWorkspaceApplicationContext, @@ -36,7 +36,7 @@ export const getPublicLog = defineAuthorizedWorkspaceUseCase({ return { ...workspace, executionId: scope.executionId, workflowId: scope.workflowId } }, authorizationOptions: {}, - execute: async ({ context }): Promise => { + execute: async ({ principal, context }): Promise => { const log = await getPublicWorkflowLog( { column: 'executionId', value: context.executionId }, context.workspaceId @@ -45,12 +45,13 @@ export const getPublicLog = defineAuthorizedWorkspaceUseCase({ throw new OrchestrationError('not_found', 'Log not found') } const folderIndex = await loadActiveFolderPathIndex(context.workspaceId, 'workflow') - const executionData = await materializeExecutionData( + const executionData = await materializeExecutionDataForDisplay( log.executionData as Record | null, { workspaceId: context.workspaceId, workflowId: log.workflowId, executionId: log.executionId, + userId: principal.kind === 'personal_api_key' ? principal.userId : undefined, } ) if (log.workflowUserId && !log.workflowOwnerEmail) { diff --git a/apps/sim/lib/logs/application/list-public-logs.ts b/apps/sim/lib/logs/application/list-public-logs.ts index e75fca02cee..12b0c062c6a 100644 --- a/apps/sim/lib/logs/application/list-public-logs.ts +++ b/apps/sim/lib/logs/application/list-public-logs.ts @@ -4,7 +4,7 @@ import { MATERIALIZE_CONCURRENCY, mapWithConcurrency } from '@/lib/core/utils/co import { ROOT_FOLDER_PATH } from '@/lib/folders/paths' import { loadActiveFolderPathIndex } from '@/lib/folders/queries' import { logOperations } from '@/lib/logs/application/operations' -import { materializeExecutionData } from '@/lib/logs/execution/trace-store' +import { materializeExecutionDataForDisplay } from '@/lib/logs/execution/trace-store' import type { LogFilters } from '@/lib/logs/public-filters' import { listPublicWorkflowLogs } from '@/lib/logs/public-queries' import { loadActiveWorkspaceApplicationContext } from '@/lib/workspaces/application/workspace-context' @@ -42,7 +42,7 @@ export const listPublicLogs = defineAuthorizedWorkspaceUseCase({ return context }, authorizationOptions: {}, - execute: async ({ input, context }): Promise => { + execute: async ({ principal, input, context }): Promise => { const folderIndex = input.folderPaths ? await loadActiveFolderPathIndex(context.workspaceId, 'workflow') : null @@ -65,17 +65,19 @@ export const listPublicLogs = defineAuthorizedWorkspaceUseCase({ folderScope: input.folderPaths ? { includesRoot, folderIds: folderIds ?? [] } : undefined, }) + const userId = principal.kind === 'personal_api_key' ? principal.userId : undefined const items = needsMaterialization ? await mapWithConcurrency(data, MATERIALIZE_CONCURRENCY, async (log) => { if (!log.executionData) return { log } return { log, - executionData: await materializeExecutionData( + executionData: await materializeExecutionDataForDisplay( log.executionData as Record, { workspaceId: log.workspaceId, workflowId: log.workflowId, executionId: log.executionId, + userId, } ), } diff --git a/apps/sim/lib/logs/application/public-log-use-cases.test.ts b/apps/sim/lib/logs/application/public-log-use-cases.test.ts index ef568c35394..f6af0bc84f7 100644 --- a/apps/sim/lib/logs/application/public-log-use-cases.test.ts +++ b/apps/sim/lib/logs/application/public-log-use-cases.test.ts @@ -35,8 +35,12 @@ vi.mock('@/lib/folders/queries', () => ({ loadActiveFolderPathIndex: mocks.loadFolders, })) +/** + * Only the display projection is exposed: these public readers must never reach + * for raw `materializeExecutionData`, which skips resolved-secret redaction. + */ vi.mock('@/lib/logs/execution/trace-store', () => ({ - materializeExecutionData: mocks.materialize, + materializeExecutionDataForDisplay: mocks.materialize, })) vi.mock('@sim/audit', () => ({ recordAudit: mocks.recordAudit })) @@ -111,12 +115,48 @@ describe('public log application use cases', () => { ) expect(mocks.materialize).toHaveBeenCalledWith( { pointer: true }, - { workspaceId: 'workspace-1', workflowId: 'workflow-1', executionId: 'run-1' } + { + workspaceId: 'workspace-1', + workflowId: 'workflow-1', + executionId: 'run-1', + userId: undefined, + } ) expect(result.workflowFolderPath).toBe('/agents') expect(mocks.recordAudit).not.toHaveBeenCalled() }) + it('passes the personal-key subject through as the projection reader', async () => { + await getPublicLog.execute({ + principal: { kind: 'personal_api_key', userId: 'user-9', keyId: 'key-9' }, + input: { runId: 'run-1' }, + }) + + expect(mocks.materialize).toHaveBeenCalledWith( + { pointer: true }, + expect.objectContaining({ userId: 'user-9' }) + ) + }) + + it('projects listed runs for display when trace spans are requested', async () => { + await listPublicLogs.execute({ + principal: { kind: 'personal_api_key', userId: 'user-9', keyId: 'key-9' }, + input: { + workspaceId: 'workspace-1', + filters: {}, + limit: 50, + includeFullDetails: false, + includeFinalOutput: false, + includeTraceSpans: true, + }, + }) + + expect(mocks.materialize).toHaveBeenCalledWith( + { pointer: true }, + expect.objectContaining({ executionId: 'run-1', userId: 'user-9' }) + ) + }) + it('rejects a workspace key outside the run workspace before materialization', async () => { await expect( getPublicLog.execute({ diff --git a/apps/sim/lib/uploads/utils/doc-not-ready.ts b/apps/sim/lib/uploads/utils/doc-not-ready.ts new file mode 100644 index 00000000000..5e5798eac9c --- /dev/null +++ b/apps/sim/lib/uploads/utils/doc-not-ready.ts @@ -0,0 +1,18 @@ +import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error' + +/** True when `error` means a generated document's artifact is still compiling. */ +export function isDocNotReadyError(error: unknown): error is DocCompileUserError { + return error instanceof DocCompileUserError +} + +/** + * Message for a still-compiling generated document. Batch callers pass the names + * they resolved so the copy says which documents to wait on. + */ +export function docNotReadyMessage(fileNames?: string[]): string { + if (!fileNames || fileNames.length === 0) { + return 'A document is still being generated. Wait for it to finish, then try again.' + } + const subject = fileNames.length === 1 ? 'A document is' : `${fileNames.length} documents are` + return `${subject} still being generated: ${fileNames.join(', ')}. Wait for them to finish, then try again.` +} diff --git a/apps/sim/lib/uploads/utils/file-utils.ts b/apps/sim/lib/uploads/utils/file-utils.ts index da2c51c0f91..6d04e92c7ef 100644 --- a/apps/sim/lib/uploads/utils/file-utils.ts +++ b/apps/sim/lib/uploads/utils/file-utils.ts @@ -261,6 +261,22 @@ export function isRenderableDocumentName(fileName: string): boolean { return RENDERABLE_DOCUMENT_EXTENSIONS.has(getFileExtension(fileName)) } +/** + * True when a stored file must be resolved to its rendered artifact before being + * handed out. The recorded content type is the authoritative signal — a genuinely + * uploaded `.pdf` carries `application/pdf` and must NOT be routed through the + * generation-source path — so the extension is consulted only for records that + * carry no type at all. + */ +export function needsRenderedArtifact( + contentType: string | null | undefined, + fileName: string +): boolean { + return contentType + ? isGeneratedDocumentSourceType(contentType) + : isRenderableDocumentName(fileName) +} + const ARCHIVE_EXTENSIONS = new Set(SUPPORTED_ARCHIVE_EXTENSIONS) /** diff --git a/apps/sim/lib/uploads/utils/servable-file-response.ts b/apps/sim/lib/uploads/utils/servable-file-response.ts index 63ffb0223f0..9dfae37d1d2 100644 --- a/apps/sim/lib/uploads/utils/servable-file-response.ts +++ b/apps/sim/lib/uploads/utils/servable-file-response.ts @@ -1,33 +1,18 @@ import { NextResponse } from 'next/server' -import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile' - -/** True when `error` means a generated document's artifact is still compiling. */ -export function isDocNotReadyError(error: unknown): error is DocCompileUserError { - return error instanceof DocCompileUserError -} - -/** - * Message for a still-compiling generated document. Batch callers pass the names - * they resolved so the copy says which documents to wait on. - */ -export function docNotReadyMessage(fileNames?: string[]): string { - if (!fileNames || fileNames.length === 0) { - return 'A document is still being generated. Wait for it to finish, then try again.' - } - const subject = fileNames.length === 1 ? 'A document is' : `${fileNames.length} documents are` - return `${subject} still being generated: ${fileNames.join(', ')}. Wait for them to finish, then try again.` -} +import { docNotReadyMessage, isDocNotReadyError } from '@/lib/uploads/utils/doc-not-ready' /** * Canonical retryable response for an attachment/upload whose generated-document * artifact is still compiling. Returns the 409 when `error` is a - * {@link DocCompileUserError} (thrown by `downloadServableFileFromStorage`), - * otherwise `null` so the caller falls through to its own error handling. Shared - * by every tool route that downloads workspace files so the status, body shape, - * and user-facing copy stay identical instead of being re-typed per route. + * `DocCompileUserError` (thrown by `downloadServableFileFromStorage`), otherwise + * `null` so the caller falls through to its own error handling. Shared by every + * tool route that downloads workspace files so the status, body shape, and + * user-facing copy stay identical instead of being re-typed per route. * * Routes whose error envelope differs, or that resolved a batch and want the pending - * files named, build the 409 themselves from {@link docNotReadyMessage}. + * files named, build the 409 themselves from `docNotReadyMessage`. Non-route callers + * import those two helpers from `@/lib/uploads/utils/doc-not-ready` directly, which + * carries no `next/server` dependency. */ export function docNotReadyResponse(error: unknown): NextResponse | null { if (isDocNotReadyError(error)) { diff --git a/apps/sim/lib/workspace-files/application/download-workspace-file-items.test.ts b/apps/sim/lib/workspace-files/application/download-workspace-file-items.test.ts index 459baa164cb..ae370bc4038 100644 --- a/apps/sim/lib/workspace-files/application/download-workspace-file-items.test.ts +++ b/apps/sim/lib/workspace-files/application/download-workspace-file-items.test.ts @@ -45,8 +45,10 @@ vi.mock('@/lib/uploads/utils/file-utils', () => ({ isGeneratedDocumentSourceType: mockIsGenerated, isRenderableDocumentName: mockIsRenderable, MAX_RENDERED_DOCUMENT_BYTES: 50 * 1024 * 1024, + needsRenderedArtifact: (contentType: string | null | undefined, fileName: string) => + contentType ? mockIsGenerated(contentType) : mockIsRenderable(fileName), })) -vi.mock('@/lib/uploads/utils/servable-file-response', () => ({ +vi.mock('@/lib/uploads/utils/doc-not-ready', () => ({ docNotReadyMessage: (names: string[]) => `Pending: ${names.join(', ')}`, isDocNotReadyError: mockIsDocNotReady, })) diff --git a/apps/sim/lib/workspace-files/application/download-workspace-file-items.ts b/apps/sim/lib/workspace-files/application/download-workspace-file-items.ts index 4509350875a..98611e62a0c 100644 --- a/apps/sim/lib/workspace-files/application/download-workspace-file-items.ts +++ b/apps/sim/lib/workspace-files/application/download-workspace-file-items.ts @@ -9,13 +9,12 @@ import { loadWorkspaceFileOperationContext, type WorkspaceFileRecord, } from '@/lib/uploads/contexts/workspace' +import { docNotReadyMessage, isDocNotReadyError } from '@/lib/uploads/utils/doc-not-ready' import { formatFileSize, - isGeneratedDocumentSourceType, - isRenderableDocumentName, MAX_RENDERED_DOCUMENT_BYTES, + needsRenderedArtifact, } from '@/lib/uploads/utils/file-utils' -import { docNotReadyMessage, isDocNotReadyError } from '@/lib/uploads/utils/servable-file-response' import { defineAuthorizedWorkspaceFileUseCase } from '@/lib/workspace-files/application/authorized-workspace-file-use-case' import { fileOperations } from '@/lib/workspace-files/application/operations' @@ -37,10 +36,6 @@ export interface DownloadWorkspaceFileItemsResult { declaredBytes: number } -function needsRendering(file: WorkspaceFileRecord): boolean { - return file.type ? isGeneratedDocumentSourceType(file.type) : isRenderableDocumentName(file.name) -} - function collectDescendantFolderIds( selectedFolderIds: string[], folders: Array<{ id: string; parentId: string | null }> @@ -120,14 +115,14 @@ async function executeDownloadWorkspaceFileItems({ } const reservedForStreamed = filesToZip - .filter((file) => !needsRendering(file)) + .filter((file) => !needsRenderedArtifact(file.type, file.name)) .reduce((sum, file) => sum + file.size, 0) const renderedDocuments = new Map() const pendingNames: string[] = [] let renderedBytes = 0 for (const file of filesToZip) { - if (!needsRendering(file)) continue + if (!needsRenderedArtifact(file.type, file.name)) continue const remaining = Math.max(0, MAX_ZIP_DOWNLOAD_BYTES - reservedForStreamed - renderedBytes) const allowance = Math.min(remaining, MAX_RENDERED_DOCUMENT_BYTES) try { diff --git a/apps/sim/lib/workspace-files/application/download-workspace-file.test.ts b/apps/sim/lib/workspace-files/application/download-workspace-file.test.ts index 37e932ca15f..0b563b59afe 100644 --- a/apps/sim/lib/workspace-files/application/download-workspace-file.test.ts +++ b/apps/sim/lib/workspace-files/application/download-workspace-file.test.ts @@ -4,8 +4,11 @@ import { Readable } from 'node:stream' import { beforeEach, describe, expect, it, vi } from 'vitest' +class FakeDocNotReadyError extends Error {} + const mocks = vi.hoisted(() => ({ downloadStream: vi.fn(), + fetchServable: vi.fn(), getFile: vi.fn(), loadContext: vi.fn(), recordAudit: vi.fn(), @@ -24,6 +27,7 @@ vi.mock('@sim/platform-authz/workspace', () => ({ })) vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({ + fetchServableWorkspaceFileBuffer: mocks.fetchServable, getWorkspaceFile: mocks.getFile, loadActiveWorkspaceFileContext: mocks.loadContext, })) @@ -32,6 +36,12 @@ vi.mock('@/lib/uploads/core/storage-service', () => ({ downloadFileStream: mocks.downloadStream, })) +vi.mock('@/lib/uploads/utils/doc-not-ready', () => ({ + isDocNotReadyError: (error: unknown) => error instanceof FakeDocNotReadyError, + docNotReadyMessage: () => 'A document is still being generated.', +})) + +import { MAX_RENDERED_DOCUMENT_BYTES } from '@/lib/uploads/utils/file-utils' import { downloadWorkspaceFile, downloadWorkspaceFileStream, @@ -54,6 +64,48 @@ const file = { storageContext: 'workspace', } +/** A name the generated-doc resolver does not claim, so it streams raw. */ +const plainFile = { + id: 'file-2', + workspaceId: 'workspace-1', + name: 'data.csv', + key: 'workspace/workspace-1/data.csv', + size: 12, + type: 'text/csv', + storageContext: 'workspace', +} + +/** Stored bytes are a generation source, so the artifact must be resolved. */ +const generatedDoc = { + ...file, + id: 'file-3', + type: 'text/x-python-pdf', +} + +/** + * A real uploaded PDF. Shares the generated doc's extension but carries final + * bytes, so it must keep streaming rather than being materialized. + */ +const uploadedPdf = { + id: 'file-4', + workspaceId: 'workspace-1', + name: 'manual.pdf', + key: 'workspace/workspace-1/manual.pdf', + size: 900, + type: 'application/pdf', + storageContext: 'workspace', +} + +const SESSION = { kind: 'session', userId: 'user-1', sessionId: 'session-1' } as const + +async function readAll(stream: ReadableStream): Promise { + return new Response(stream).text() +} + +function downloadStream(fileId: string) { + return downloadWorkspaceFileStream.execute({ principal: SESSION, input: { fileId } }) +} + describe('workspace file downloads', () => { beforeEach(() => { vi.clearAllMocks() @@ -61,6 +113,10 @@ describe('workspace file downloads', () => { mocks.resolvePermission.mockResolvedValue('admin') mocks.getFile.mockResolvedValue(file) mocks.downloadStream.mockResolvedValue(Readable.from(Buffer.from('pdf'))) + mocks.fetchServable.mockResolvedValue({ + buffer: Buffer.from('%PDF-compiled'), + contentType: 'application/pdf', + }) }) it('returns the authoritative file and records its semantic download audit', async () => { @@ -89,16 +145,95 @@ describe('workspace file downloads', () => { }) it('does not audit a streaming download when storage acquisition fails', async () => { + mocks.getFile.mockResolvedValue(plainFile) const failure = new Error('storage unavailable') mocks.downloadStream.mockRejectedValueOnce(failure) - await expect( - downloadWorkspaceFileStream.execute({ - principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, - input: { fileId: 'file-1' }, - }) - ).rejects.toBe(failure) + await expect(downloadStream('file-2')).rejects.toBe(failure) + expect(mocks.recordAudit).not.toHaveBeenCalled() + }) + + /** Resolving an artifact costs a full buffer, so it is reserved for generation sources. */ + it.each([ + { label: 'an ordinary file', record: plainFile, type: 'text/csv', size: 12 }, + { label: 'a genuinely uploaded pdf', record: uploadedPdf, type: 'application/pdf', size: 900 }, + ])('streams $label straight from storage', async ({ record, type, size }) => { + mocks.getFile.mockResolvedValue(record) + + const result = await downloadStream(record.id) + expect(mocks.fetchServable).not.toHaveBeenCalled() + expect(mocks.downloadStream).toHaveBeenCalledWith({ + key: record.key, + context: 'workspace', + }) + expect(result.contentType).toBe(type) + expect(result.contentLength).toBe(size) + }) + + /** + * Regression: a raw stream of a generated doc's key yields its generation + * source under a `.pdf` name — a file the recipient cannot open. + */ + it('serves the compiled artifact for a generated doc rather than its source', async () => { + mocks.getFile.mockResolvedValue(generatedDoc) + + const result = await downloadStream('file-3') + + expect(mocks.downloadStream).not.toHaveBeenCalled() + expect(await readAll(result.stream)).toBe('%PDF-compiled') + expect(result.contentType).toBe('application/pdf') + expect(result.contentLength).toBe('%PDF-compiled'.length) + expect(result.contentLength).not.toBe(generatedDoc.size) + }) + + it('caps the artifact it will materialize', async () => { + mocks.getFile.mockResolvedValue(generatedDoc) + + await downloadStream('file-3') + + expect(mocks.fetchServable).toHaveBeenCalledWith( + generatedDoc, + expect.objectContaining({ maxBytes: MAX_RENDERED_DOCUMENT_BYTES }) + ) + }) + + /** Legacy records carry no type, so the extension is the only signal left. */ + it('routes a record with no content type by its extension', async () => { + await downloadStream('file-1') + + expect(mocks.fetchServable).toHaveBeenCalledWith(file, expect.anything()) + expect(mocks.downloadStream).not.toHaveBeenCalled() + }) + + it('surfaces a still-compiling generated doc as a retryable conflict', async () => { + mocks.getFile.mockResolvedValue(generatedDoc) + mocks.fetchServable.mockRejectedValueOnce(new FakeDocNotReadyError('still compiling')) + + await expect(downloadStream('file-3')).rejects.toMatchObject({ + name: 'OrchestrationError', + code: 'conflict', + }) expect(mocks.recordAudit).not.toHaveBeenCalled() }) + + it('propagates a genuine artifact-resolution failure unchanged', async () => { + mocks.getFile.mockResolvedValue(generatedDoc) + const failure = new Error('artifact store unavailable') + mocks.fetchServable.mockRejectedValueOnce(failure) + + await expect(downloadStream('file-3')).rejects.toBe(failure) + }) + + it('audits the bytes actually served, not the generation source size', async () => { + mocks.getFile.mockResolvedValue(generatedDoc) + + await downloadStream('file-3') + + expect(mocks.recordAudit).toHaveBeenCalledWith( + expect.objectContaining({ + metadata: expect.objectContaining({ bytes: '%PDF-compiled'.length }), + }) + ) + }) }) diff --git a/apps/sim/lib/workspace-files/application/download-workspace-file.ts b/apps/sim/lib/workspace-files/application/download-workspace-file.ts index 70be04fbe93..2613293d629 100644 --- a/apps/sim/lib/workspace-files/application/download-workspace-file.ts +++ b/apps/sim/lib/workspace-files/application/download-workspace-file.ts @@ -2,11 +2,19 @@ import { AuditAction, AuditResourceType } from '@sim/audit' import type { AuthorizedWorkspaceUseCaseContext } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { nodeReadableToWebStream } from '@/lib/core/utils/node-stream' +import { isPayloadSizeLimitError } from '@/lib/core/utils/stream-limits' import { type ActiveWorkspaceFileContext, + fetchServableWorkspaceFileBuffer, getWorkspaceFile, } from '@/lib/uploads/contexts/workspace/workspace-file-manager' import { downloadFileStream } from '@/lib/uploads/core/storage-service' +import { docNotReadyMessage, isDocNotReadyError } from '@/lib/uploads/utils/doc-not-ready' +import { + formatFileSize, + MAX_RENDERED_DOCUMENT_BYTES, + needsRenderedArtifact, +} from '@/lib/uploads/utils/file-utils' import { defineAuthorizedWorkspaceFileUseCase } from '@/lib/workspace-files/application/authorized-workspace-file-use-case' import { fileOperations } from '@/lib/workspace-files/application/operations' import { resolveActiveWorkspaceFileContext } from '@/lib/workspace-files/application/workspace-file-context' @@ -22,9 +30,17 @@ export interface DownloadWorkspaceFileResult { export interface DownloadWorkspaceFileStreamResult extends DownloadWorkspaceFileResult { stream: ReadableStream + /** + * What to advertise for the bytes actually served. Both differ from the record + * for a generated doc, whose compiled artifact is neither the stored source's + * size nor its MIME. + */ + contentLength: number + contentType: string } -function projectDownloadAudit(file: DownloadWorkspaceFileResult['file']) { +/** Audits the bytes actually handed out, which for a generated doc is not `file.size`. */ +function projectDownloadAudit(file: DownloadWorkspaceFileResult['file'], servedBytes?: number) { return { action: AuditAction.FILE_DOWNLOADED, resourceType: AuditResourceType.FILE, @@ -34,7 +50,7 @@ function projectDownloadAudit(file: DownloadWorkspaceFileResult['file']) { metadata: { fileId: file.id, fileName: file.name, - bytes: file.size, + bytes: servedBytes ?? file.size, }, } } @@ -60,6 +76,37 @@ export const downloadWorkspaceFile = defineAuthorizedWorkspaceFileUseCase({ projectAudit: ({ result }) => projectDownloadAudit(result.file), }) +/** + * Resolves a generation-source record to its compiled artifact. + * + * The record's declared size bounds nothing here — a source is text and orders + * of magnitude smaller than what it renders to — so the artifact is checked + * against its own ceiling. Note this rejects an oversized artifact rather than + * preventing it being read: the artifact store fetch is not itself streaming- + * bounded, so the bytes are resident before the check rejects them. + * + * An artifact that is still compiling is retryable rather than a fault, so it + * surfaces as `conflict` — a 500 would give the caller no reason to try again. + */ +async function resolveRenderedArtifact(file: DownloadWorkspaceFileResult['file']) { + try { + return await fetchServableWorkspaceFileBuffer(file, { + maxBytes: MAX_RENDERED_DOCUMENT_BYTES, + }) + } catch (error) { + if (isDocNotReadyError(error)) { + throw new OrchestrationError('conflict', docNotReadyMessage()) + } + if (isPayloadSizeLimitError(error)) { + throw new OrchestrationError( + 'payload_too_large', + `"${file.name}" renders to more than ${formatFileSize(MAX_RENDERED_DOCUMENT_BYTES)} and is too large to download.` + ) + } + throw error + } +} + async function executeDownloadWorkspaceFileStream({ context, }: AuthorizedWorkspaceUseCaseContext< @@ -71,17 +118,52 @@ async function executeDownloadWorkspaceFileStream({ throwOnError: true, }) if (!file) throw new OrchestrationError('not_found', 'File not found') + + /** + * AI-generated docs store their generation SOURCE as the primary file and keep + * the rendered binary in a separate artifact store, so streaming `file.key` + * raw would hand back source text under a `.pdf` name. Resolving that costs a + * buffer, so it is gated on the recorded generation-source type — a genuinely + * uploaded `.pdf` keeps streaming and never materializes. The artifact is + * enqueued as a view over that buffer, not a copy, which would otherwise + * double peak memory. + */ + if (needsRenderedArtifact(file.type, file.name)) { + const { buffer, contentType } = await resolveRenderedArtifact(file) + return { + file, + stream: new ReadableStream({ + start(controller) { + controller.enqueue( + new Uint8Array(buffer.buffer as ArrayBuffer, buffer.byteOffset, buffer.byteLength) + ) + controller.close() + }, + }), + contentLength: buffer.length, + contentType, + } + } + const stream = await downloadFileStream({ key: file.key, context: file.storageContext ?? 'workspace', }) - return { file, stream: nodeReadableToWebStream(stream) } + return { + file, + stream: nodeReadableToWebStream(stream), + contentLength: file.size, + contentType: file.type || 'application/octet-stream', + } } -/** Authorized and audited binary download without materializing the file in memory. */ +/** + * Authorized and audited binary download. Ordinary files stream without being + * materialized; generated docs resolve to their compiled artifact first. + */ export const downloadWorkspaceFileStream = defineAuthorizedWorkspaceFileUseCase({ operation: fileOperations.download, resolveContext: ({ input }) => resolveActiveWorkspaceFileContext(input), execute: executeDownloadWorkspaceFileStream, - projectAudit: ({ result }) => projectDownloadAudit(result.file), + projectAudit: ({ result }) => projectDownloadAudit(result.file, result.contentLength), }) diff --git a/scripts/check-openapi-specs.ts b/scripts/check-openapi-specs.ts index dd1bc4645bb..442dff4a3b5 100644 --- a/scripts/check-openapi-specs.ts +++ b/scripts/check-openapi-specs.ts @@ -12,10 +12,15 @@ * 2. v2 conventions: every published operation is under `/api/v2/`, documents * 401, 429, and 503, and resolves every documented 4xx/5xx response to the * canonical error envelope `{ error: { code, message } }`. - * 3. Contract cross-check: every contract exported from - * `lib/api/contracts/v2/*` must be documented, every documented `/api/v2/` - * operation must have a contract, and for each pair the query params, - * body fields, and response fields are diffed via `z.toJSONSchema`. + * 3. Contract cross-check: every route contract anywhere under + * `lib/api/contracts/**` whose path is under `/api/v2/` must be documented + * (or listed in `UNDOCUMENTED_V2_ROUTES` with a reason), every documented + * `/api/v2/` operation must have a contract, and for each pair the query + * params, body fields, and response fields are diffed via + * `z.toJSONSchema`. The sweep is recursive and rooted at the whole + * contracts tree, not the flat `v2/` directory — a contract in a + * subdirectory, or one that lives beside its non-v2 siblings, must never + * be able to escape coverage by virtue of where its file sits. * 4. Examples: documented request/response examples are parsed with the * matching contract's actual Zod schemas — a doc example that the runtime * would reject fails the build. @@ -29,10 +34,27 @@ import { OPENAPI_SPEC_FILES } from '../apps/docs/lib/openapi-specs' const ROOT = path.resolve(import.meta.dir, '..') const DOCS_DIR = path.join(ROOT, 'apps/docs') -const V2_CONTRACTS_DIR = path.join(ROOT, 'apps/sim/lib/api/contracts/v2') +const CONTRACTS_DIR = path.join(ROOT, 'apps/sim/lib/api/contracts') const SPEC_FILES = OPENAPI_SPEC_FILES +/** + * `/api/v2/` routes that are deliberately absent from the public OpenAPI + * specs, each with the reason it is not public API surface. Anything not + * listed here fails the build, so an undocumented v2 route is always a + * conscious, reviewed decision rather than an accident of file layout. + * + * A stale entry — one whose contract no longer exists, or which has since + * been documented — also fails, so the list cannot rot into a blanket + * exemption. + */ +const UNDOCUMENTED_V2_ROUTES: Readonly> = { + 'PUT /api/v2/uploads/{uploadId}': + 'Local-storage data plane for a signed whole-object upload. Authenticated by the short-lived upload-token minted by the documented session-create operation, not by an API key; carries no v2 feature gate and returns bare error bodies rather than the canonical v2 envelope. The URL is handed to the client by the session response and is never constructed from docs.', + 'PUT /api/v2/uploads/{uploadId}/parts/{partNumber}': + 'Local-storage data plane for a signed multipart part upload. Authenticated by a per-part signed `token` query param minted by the documented part-URL operation, not by an API key; same non-canonical envelope and self-describing URL as the whole-object PUT above.', +} + /** * Every operation removed with the unversioned core specification has a * public v2 replacement. Keeping this mapping executable prevents a future @@ -109,15 +131,31 @@ function isContract(value: unknown): value is ContractLike { const contractKey = (c: ContractLike) => `${c.method.toUpperCase()} ${c.path.replace(/\[([^\]]+)\]/g, '{$1}')}` +/** Every non-test `.ts` file under `dir`, recursively, in stable order. */ +function listContractFiles(dir: string): string[] { + const files: string[] = [] + for (const entry of readdirSync(dir, { withFileTypes: true }).sort((a, b) => + a.name.localeCompare(b.name) + )) { + const full = path.join(dir, entry.name) + if (entry.isDirectory()) { + if (entry.name === '__tests__') continue + files.push(...listContractFiles(full)) + } else if (entry.name.endsWith('.ts') && !entry.name.endsWith('.test.ts')) { + files.push(full) + } + } + return files +} + +/** Every `/api/v2/` route contract exported anywhere in the contracts tree. */ async function loadContracts(): Promise> { const registry = new Map() - const files = readdirSync(V2_CONTRACTS_DIR) - .filter((f) => f.endsWith('.ts') && f !== 'shared.ts') - .map((f) => path.join(V2_CONTRACTS_DIR, f)) - for (const file of files) { + for (const file of listContractFiles(CONTRACTS_DIR)) { const mod = (await import(file)) as Record for (const [name, value] of Object.entries(mod)) { if (!isContract(value)) continue + if (!value.path.startsWith('/api/v2/')) continue const key = contractKey(value) const existing = registry.get(key) if (existing) { @@ -659,8 +697,20 @@ for (const specFile of SPEC_FILES) { } for (const [key, { name }] of registry) { - if (!documentedKeys.has(key)) { - errors.push(`registry: ${name} (${key}) is not documented in any OpenAPI spec`) + if (documentedKeys.has(key) || key in UNDOCUMENTED_V2_ROUTES) continue + errors.push(`registry: ${name} (${key}) is not documented in any OpenAPI spec`) +} + +for (const [key, reason] of Object.entries(UNDOCUMENTED_V2_ROUTES)) { + if (!reason.trim()) { + errors.push(`undocumented v2 allowlist: ${key} needs a reason explaining why it is not public`) + } + if (!registry.has(key)) { + errors.push(`undocumented v2 allowlist: ${key} matches no contract — remove the stale entry`) + } else if (documentedKeys.has(key)) { + errors.push( + `undocumented v2 allowlist: ${key} is documented after all — remove it from the allowlist` + ) } } @@ -733,6 +783,7 @@ if (errors.length > 0) { for (const message of errors) console.error(` - ${message}`) process.exit(1) } +const exemptCount = Object.keys(UNDOCUMENTED_V2_ROUTES).length console.log( - `OpenAPI spec validation passed: ${SPEC_FILES.length} specs, ${documentedKeys.size} operations, ${registry.size} contracts cross-checked.` + `OpenAPI spec validation passed: ${SPEC_FILES.length} specs, ${documentedKeys.size} operations, ${registry.size} contracts cross-checked (${exemptCount} explicitly undocumented).` )