Skip to content

Commit 8d5b0ee

Browse files
committed
improvement(knowledge): read inline document payloads lazily and scope chunk reads to the base
Follow-ups from an independent review pass over the export path. - the document listing no longer selects fileUrl: a data: document can hold megabytes in that column, so the listing carries a flag and the archive reads one payload at a time when it reaches that entry - iterateDocumentChunks filters on knowledgeBaseId as well as documentId, so a future caller cannot reach another base's chunks through the bundle - tag definitions are validated by the single bundle gate, which answers 409 like every other undescribable value rather than throwing a bare ZodError - a consumer that abandons the download aborts the append loop and destroys the in-flight source instead of leaving it pending - direct tests for the knowledge use-case builder's authorize() on the workspace, organization, and legacy personal branches
1 parent ccaaafb commit 8d5b0ee

7 files changed

Lines changed: 278 additions & 63 deletions

File tree

Lines changed: 144 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,144 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const mocks = vi.hoisted(() => ({
7+
resolvePermission: vi.fn(),
8+
authorizeOrganization: vi.fn(),
9+
recordAudit: vi.fn(),
10+
}))
11+
12+
vi.mock('@sim/audit', () => ({
13+
AuditAction: { KNOWLEDGE_BASE_UPDATED: 'knowledge_base.updated' },
14+
AuditResourceType: { KNOWLEDGE_BASE: 'knowledge_base' },
15+
recordAudit: mocks.recordAudit,
16+
}))
17+
18+
vi.mock('@sim/platform-authz/workspace', () => ({
19+
permissionSatisfies: (actual: string | null, required: string) => {
20+
const rank = { read: 1, write: 2, admin: 3 } as const
21+
return (
22+
actual !== null && rank[actual as keyof typeof rank] >= rank[required as keyof typeof rank]
23+
)
24+
},
25+
resolveEffectiveWorkspacePermission: mocks.resolvePermission,
26+
}))
27+
28+
vi.mock('@/lib/core/application/organization-authorization', () => ({
29+
authorizeOrganizationOperation: mocks.authorizeOrganization,
30+
}))
31+
32+
import { defineAuthorizedKnowledgeUseCase } from '@/lib/knowledge/application/authorized-knowledge-use-case'
33+
import { knowledgeOperations } from '@/lib/knowledge/application/operations'
34+
35+
const workspaceContext = {
36+
workspaceId: 'workspace-1',
37+
workspaceOrganizationId: 'organization-1',
38+
allowPersonalApiKeys: true,
39+
billedAccountUserId: 'owner-1',
40+
knowledgeBaseId: 'knowledge-1',
41+
}
42+
const organizationContext = {
43+
workspaceId: undefined,
44+
organizationId: 'organization-1',
45+
knowledgeBaseId: 'knowledge-1',
46+
}
47+
const legacyContext = {
48+
workspaceId: undefined,
49+
legacyPersonalOwnerUserId: 'user-1',
50+
knowledgeBaseId: 'knowledge-1',
51+
}
52+
53+
const session = { kind: 'session', userId: 'user-1', sessionId: 'session-1' } as const
54+
55+
function useCaseFor(context: object) {
56+
const execute = vi.fn(async () => 'done')
57+
const useCase = defineAuthorizedKnowledgeUseCase({
58+
operation: knowledgeOperations.read,
59+
resolveContext: () => context as never,
60+
execute,
61+
projectAudit: () => ({
62+
action: 'knowledge_base.updated',
63+
resourceType: 'knowledge_base',
64+
resourceId: 'knowledge-1',
65+
resourceName: 'Docs',
66+
description: 'audited',
67+
metadata: {},
68+
}),
69+
})
70+
return { useCase, execute }
71+
}
72+
73+
describe('defineAuthorizedKnowledgeUseCase', () => {
74+
beforeEach(() => {
75+
vi.clearAllMocks()
76+
mocks.resolvePermission.mockResolvedValue('read')
77+
mocks.authorizeOrganization.mockResolvedValue(undefined)
78+
})
79+
80+
/** `authorize` must run the same funnel `execute` does, and nothing else. */
81+
it('authorizes a workspace base through the workspace funnel without executing', async () => {
82+
const { useCase, execute } = useCaseFor(workspaceContext)
83+
84+
await useCase.authorize({ principal: session, input: {} })
85+
expect(mocks.resolvePermission).toHaveBeenCalledTimes(1)
86+
expect(execute).not.toHaveBeenCalled()
87+
expect(mocks.recordAudit).not.toHaveBeenCalled()
88+
89+
mocks.resolvePermission.mockResolvedValue(null)
90+
await expect(useCase.authorize({ principal: session, input: {} })).rejects.toMatchObject({
91+
name: 'NoWorkspaceAccessError',
92+
})
93+
})
94+
95+
it('executes a workspace base and records its audit under the workspace', async () => {
96+
const { useCase } = useCaseFor(workspaceContext)
97+
98+
await expect(useCase.execute({ principal: session, input: {} })).resolves.toBe('done')
99+
expect(mocks.recordAudit).toHaveBeenCalledWith(
100+
expect.objectContaining({ workspaceId: 'workspace-1', resourceId: 'knowledge-1' })
101+
)
102+
})
103+
104+
it('authorizes and executes an organization base through the organization operation', async () => {
105+
const { useCase, execute } = useCaseFor(organizationContext)
106+
107+
await useCase.authorize({ principal: session, input: {} })
108+
expect(mocks.authorizeOrganization).toHaveBeenCalledWith(
109+
session,
110+
knowledgeOperations.read.organizationOperation,
111+
organizationContext
112+
)
113+
expect(execute).not.toHaveBeenCalled()
114+
expect(mocks.resolvePermission).not.toHaveBeenCalled()
115+
116+
await expect(useCase.execute({ principal: session, input: {} })).resolves.toBe('done')
117+
expect(mocks.recordAudit).toHaveBeenCalledWith(
118+
expect.objectContaining({
119+
workspaceId: undefined,
120+
resourceId: 'knowledge-1',
121+
metadata: expect.objectContaining({ organizationId: 'organization-1' }),
122+
})
123+
)
124+
})
125+
126+
it('admits only the owner of a legacy personal base', async () => {
127+
const { useCase, execute } = useCaseFor(legacyContext)
128+
129+
await useCase.authorize({ principal: session, input: {} })
130+
expect(execute).not.toHaveBeenCalled()
131+
await expect(useCase.execute({ principal: session, input: {} })).resolves.toBe('done')
132+
133+
const stranger = { kind: 'session', userId: 'user-2', sessionId: 'session-2' } as const
134+
await expect(useCase.authorize({ principal: stranger, input: {} })).rejects.toMatchObject({
135+
code: 'not_found',
136+
})
137+
await expect(
138+
useCase.authorize({
139+
principal: { kind: 'workspace_api_key', workspaceId: 'workspace-1', keyId: 'key-1' },
140+
input: {},
141+
})
142+
).rejects.toMatchObject({ code: 'not_found' })
143+
})
144+
})

apps/sim/lib/knowledge/application/exports.test.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,14 +135,14 @@ describe('exportKnowledgeBase', () => {
135135
input: { knowledgeBaseId: 'knowledge-1', vectors: true },
136136
})
137137
withVectors.chunks('doc-1')
138-
expect(mocks.iterateChunks).toHaveBeenLastCalledWith('doc-1', 1536)
138+
expect(mocks.iterateChunks).toHaveBeenLastCalledWith('knowledge-1', 'doc-1', 1536)
139139

140140
const textOnly = await exportKnowledgeBase.execute({
141141
principal,
142142
input: { knowledgeBaseId: 'knowledge-1', vectors: false },
143143
})
144144
textOnly.chunks('doc-1')
145-
expect(mocks.iterateChunks).toHaveBeenLastCalledWith('doc-1', null)
145+
expect(mocks.iterateChunks).toHaveBeenLastCalledWith('knowledge-1', 'doc-1', null)
146146
expect(textOnly.embedding.vectorsIncluded).toBe(false)
147147
})
148148

@@ -196,6 +196,19 @@ describe('exportKnowledgeBase', () => {
196196
expect(mocks.recordAudit).not.toHaveBeenCalled()
197197
})
198198

199+
it('refuses a stored tag definition the bundle format cannot describe', async () => {
200+
mocks.listTags.mockResolvedValueOnce([
201+
{ slot: 'tag1', displayName: 'Product', fieldType: 'mystery' },
202+
])
203+
204+
await expect(
205+
exportKnowledgeBase.execute({
206+
principal,
207+
input: { knowledgeBaseId: 'knowledge-1', vectors: true },
208+
})
209+
).rejects.toMatchObject({ code: 'conflict' })
210+
})
211+
199212
it('propagates an oversized base without recording audit', async () => {
200213
mocks.listDocuments.mockRejectedValueOnce(
201214
new OrchestrationError('payload_too_large', 'Knowledge base has 2001 documents')

apps/sim/lib/knowledge/application/exports.ts

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,10 @@ import { knowledgeOperations } from '@/lib/knowledge/application/operations'
66
import { KNOWLEDGE_BUNDLE_VERSION } from '@/lib/knowledge/constants'
77
import { toKbEmbeddingDimensions } from '@/lib/knowledge/embedding-models'
88
import {
9-
assertDescribableByBundle,
109
bundleEntryPaths,
1110
type KnowledgeBundleManifest,
1211
type KnowledgeBundleTag,
12+
parseDescribableBundle,
1313
toManifestDocument,
1414
} from '@/lib/knowledge/transfer/bundle'
1515
import {
@@ -57,32 +57,32 @@ export const exportKnowledgeBase = defineAuthorizedKnowledgeUseCase({
5757
listExportableTags(knowledgeBase.id),
5858
listExportableDocuments(knowledgeBase.id),
5959
])
60-
const bundle: KnowledgeBaseExportBundle = {
61-
knowledgeBase: {
62-
name: knowledgeBase.name,
63-
description: knowledgeBase.description,
64-
chunkingConfig: knowledgeBase.chunkingConfig,
65-
},
60+
const manifest = parseDescribableBundle({
61+
version: KNOWLEDGE_BUNDLE_VERSION,
62+
exportedAt: new Date().toISOString(),
6663
embedding: {
6764
model: knowledgeBase.embeddingModel,
6865
dimension,
6966
vectorsIncluded: input.vectors,
7067
},
71-
tags,
72-
documents,
73-
chunks: (documentId) => iterateDocumentChunks(documentId, input.vectors ? dimension : null),
74-
}
75-
assertDescribableByBundle({
76-
version: KNOWLEDGE_BUNDLE_VERSION,
77-
exportedAt: new Date().toISOString(),
78-
embedding: bundle.embedding,
79-
knowledgeBase: bundle.knowledgeBase,
68+
knowledgeBase: {
69+
name: knowledgeBase.name,
70+
description: knowledgeBase.description,
71+
chunkingConfig: knowledgeBase.chunkingConfig,
72+
},
8073
tags,
8174
documents: documents.map((document) =>
8275
toManifestDocument(document, bundleEntryPaths(document), 0)
8376
),
8477
})
85-
return bundle
78+
return {
79+
knowledgeBase: manifest.knowledgeBase,
80+
embedding: manifest.embedding,
81+
tags: manifest.tags,
82+
documents,
83+
chunks: (documentId) =>
84+
iterateDocumentChunks(knowledgeBase.id, documentId, input.vectors ? dimension : null),
85+
}
8686
},
8787
projectAudit: ({ context, input, result }) => ({
8888
action: AuditAction.KNOWLEDGE_BASE_EXPORTED,

apps/sim/lib/knowledge/transfer/bundle.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -220,13 +220,14 @@ export function bundleEntryPaths(
220220
}
221221

222222
/**
223-
* Refuses an export whose stored values the bundle format cannot describe,
224-
* before any byte streams: the manifest is written last, so a value the import
225-
* side would reject must surface as a clear error rather than a truncated archive.
223+
* Validates an export's stored values against the bundle format before any byte
224+
* streams, and returns them in their wire shape. The manifest is written last,
225+
* so a value the import side would reject must surface as a clear error rather
226+
* than a truncated archive.
226227
*/
227-
export function assertDescribableByBundle(manifest: unknown): void {
228+
export function parseDescribableBundle(manifest: unknown): KnowledgeBundleManifest {
228229
const result = knowledgeBundleManifestSchema.safeParse(manifest)
229-
if (result.success) return
230+
if (result.success) return result.data
230231
const [issue] = result.error.issues
231232
throw new OrchestrationError(
232233
'conflict',

apps/sim/lib/knowledge/transfer/export-archive.test.ts

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,23 @@
22
* @vitest-environment node
33
*/
44
import { Readable } from 'node:stream'
5+
import { sleep } from '@sim/utils/helpers'
56
import JSZip from 'jszip'
67
import { beforeEach, describe, expect, it, vi } from 'vitest'
78

89
const mocks = vi.hoisted(() => ({
910
downloadFileStream: vi.fn(),
11+
readInlineFileUrl: vi.fn(),
1012
}))
1113

1214
vi.mock('@/lib/uploads/core/storage-service', () => ({
1315
downloadFileStream: mocks.downloadFileStream,
1416
}))
1517

18+
vi.mock('@/lib/knowledge/transfer/export-source', () => ({
19+
readInlineFileUrl: mocks.readInlineFileUrl,
20+
}))
21+
1622
import type { KnowledgeBaseExportBundle } from '@/lib/knowledge/application/exports'
1723
import { decodeVectorBase64, knowledgeBundleManifestSchema } from '@/lib/knowledge/transfer/bundle'
1824
import {
@@ -72,10 +78,7 @@ function bundle(overrides: Partial<KnowledgeBaseExportBundle> = {}): KnowledgeBa
7278
id: INLINE_ID,
7379
filename: 'note.txt',
7480
mimeType: 'text/plain',
75-
file: {
76-
kind: 'data-uri',
77-
fileUrl: `data:text/plain;base64,${Buffer.from('hi').toString('base64')}`,
78-
},
81+
file: { kind: 'data-uri', documentId: INLINE_ID },
7982
hasChunks: false,
8083
}),
8184
exportableDocument({ id: TEXT_ONLY_ID, filename: 'wiki page', file: null, hasChunks: true }),
@@ -98,6 +101,9 @@ describe('buildKnowledgeBundleArchive', () => {
98101
beforeEach(() => {
99102
vi.clearAllMocks()
100103
mocks.downloadFileStream.mockImplementation(async () => Readable.from([Buffer.from('pdf')]))
104+
mocks.readInlineFileUrl.mockResolvedValue(
105+
`data:text/plain;base64,${Buffer.from('hi').toString('base64')}`
106+
)
101107
})
102108

103109
it('writes files, chunk lines, and a manifest that validates against the bundle schema', async () => {
@@ -190,6 +196,20 @@ describe('buildKnowledgeBundleArchive', () => {
190196
'entry:manifest.json',
191197
])
192198
})
199+
200+
/** A browser that abandons the download must not leave the append loop or its blob stream hanging. */
201+
it('releases the in-flight source and stops appending when the consumer goes away', async () => {
202+
const blob = new Readable({ read() {} })
203+
mocks.downloadFileStream.mockResolvedValue(blob)
204+
const archive = buildKnowledgeBundleArchive(bundle())
205+
await sleep(1)
206+
expect(mocks.downloadFileStream).toHaveBeenCalledTimes(1)
207+
208+
archive.destroy()
209+
await sleep(1)
210+
expect(blob.destroyed).toBe(true)
211+
expect(mocks.readInlineFileUrl).not.toHaveBeenCalled()
212+
})
193213
})
194214

195215
describe('knowledgeBundleFileName', () => {

0 commit comments

Comments
 (0)