Skip to content

Commit 8e9758a

Browse files
committed
fix(files): address second search review round
1 parent d770429 commit 8e9758a

12 files changed

Lines changed: 158 additions & 35 deletions

File tree

apps/sim/blocks/blocks/file.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,16 @@ describe('FileV5Block', () => {
8383
expect(maxResults?.value?.()).toBe('50')
8484
})
8585

86+
it('uses the default search cap when the builder field is cleared', () => {
87+
expect(
88+
buildParams({
89+
operation: 'file_search',
90+
query: 'needle',
91+
maxResults: '',
92+
})
93+
).toEqual({ query: 'needle', maxResults: 50 })
94+
})
95+
8696
it.each(['10.5', '10results', '0', '201'])(
8797
'rejects invalid builder-configured search cap %s',
8898
(maxResults) => {

apps/sim/blocks/blocks/file.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1254,7 +1254,9 @@ export const FileV5Block: BlockConfig<FileParserV3Output> = {
12541254
const operation = params.operation || 'file_read'
12551255

12561256
if (operation === 'file_search') {
1257-
const maxResults = Number(params.maxResults ?? '50')
1257+
const maxResultsInput =
1258+
params.maxResults == null || params.maxResults === '' ? 50 : params.maxResults
1259+
const maxResults = Number(maxResultsInput)
12581260
if (!Number.isInteger(maxResults) || maxResults < 1 || maxResults > 200) {
12591261
throw new Error('Maximum Results must be an integer between 1 and 200')
12601262
}

apps/sim/lib/internal/file/execute-tool.test.ts

Lines changed: 65 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,26 @@ const BILLING_ATTRIBUTION = {
7070
payerSubscription: null,
7171
} satisfies BillingAttributionSnapshot
7272

73+
const SEARCH_RESULT = {
74+
results: [{ fileId: 'file-1', lineNumber: 2, text: 'needle' }],
75+
count: 1,
76+
truncated: false,
77+
complete: true,
78+
indexStatus: {
79+
readyFiles: 1,
80+
pendingFiles: 0,
81+
failedFiles: 0,
82+
skippedFiles: 0,
83+
partialFiles: 0,
84+
},
85+
sources: [
86+
{
87+
identity: { fileId: 'file-1', key: 'workspace/workspace-1/file.txt' },
88+
ownerUserId: 'user-1',
89+
},
90+
],
91+
}
92+
7393
function request(
7494
toolId: string,
7595
input: unknown,
@@ -109,25 +129,7 @@ describe('executeFileTool', () => {
109129
})
110130
mocks.executeManage.mockResolvedValue(Response.json({ success: true }))
111131
mocks.executeParser.mockResolvedValue(Response.json({ success: true }))
112-
mocks.searchContent.mockResolvedValue({
113-
results: [{ fileId: 'file-1', lineNumber: 2, text: 'needle' }],
114-
count: 1,
115-
truncated: false,
116-
complete: true,
117-
indexStatus: {
118-
readyFiles: 1,
119-
pendingFiles: 0,
120-
failedFiles: 0,
121-
skippedFiles: 0,
122-
partialFiles: 0,
123-
},
124-
sources: [
125-
{
126-
identity: { fileId: 'file-1', key: 'workspace/workspace-1/file.txt' },
127-
ownerUserId: 'user-1',
128-
},
129-
],
130-
})
132+
mocks.searchContent.mockResolvedValue(SEARCH_RESULT)
131133
mocks.getProvenance.mockResolvedValue({ version: 1, complete: true, entries: [] })
132134
})
133135

@@ -146,7 +148,7 @@ describe('executeFileTool', () => {
146148
})
147149
expect(mocks.searchContent).toHaveBeenCalledWith({
148150
principal: expect.objectContaining({ serviceId: 'executor' }),
149-
input: { workspaceId: 'workspace-1', query: 'needle', maxResults: 25 },
151+
input: { workspaceId: 'workspace-1', query: 'needle', maxResults: 25, signal: undefined },
150152
})
151153
expect(mocks.executeManage).not.toHaveBeenCalled()
152154
})
@@ -176,7 +178,8 @@ describe('executeFileTool', () => {
176178
expect.objectContaining({
177179
identity: expect.objectContaining({ fileId: 'file-1' }),
178180
}),
179-
])
181+
]),
182+
undefined
180183
)
181184
const body = await response.json()
182185
expect(body.data.sources).toBeUndefined()
@@ -207,6 +210,47 @@ describe('executeFileTool', () => {
207210
})
208211
})
209212

213+
it('propagates cancellation that arrives while search work is running', async () => {
214+
const controller = new AbortController()
215+
mocks.searchContent.mockImplementationOnce(async () => {
216+
controller.abort(new DOMException('cancelled', 'AbortError'))
217+
return SEARCH_RESULT
218+
})
219+
220+
await expect(
221+
executeFileTool(request('file_search', { query: 'needle' }, { signal: controller.signal }))
222+
).rejects.toMatchObject({ name: 'AbortError' })
223+
expect(mocks.searchContent).toHaveBeenCalledWith(
224+
expect.objectContaining({
225+
input: expect.objectContaining({ signal: controller.signal }),
226+
})
227+
)
228+
expect(mocks.getProvenance).not.toHaveBeenCalled()
229+
})
230+
231+
it('propagates cancellation that arrives while search provenance is loading', async () => {
232+
const controller = new AbortController()
233+
mocks.getProvenance.mockImplementationOnce(async () => {
234+
controller.abort(new DOMException('cancelled', 'AbortError'))
235+
return { version: 1, complete: true, entries: [] }
236+
})
237+
238+
await expect(
239+
executeFileTool(
240+
request(
241+
'file_search',
242+
{ query: 'needle' },
243+
{
244+
signal: controller.signal,
245+
headers: new Headers({
246+
'x-sim-request-private-tool-metadata': 'resolved-secret-provenance-v1',
247+
}),
248+
}
249+
)
250+
)
251+
).rejects.toMatchObject({ name: 'AbortError' })
252+
})
253+
210254
it.each(Object.entries(MANAGE_INPUTS))('validates and dispatches %s', async (toolId, input) => {
211255
const response = await executeFileTool(request(toolId, input))
212256

apps/sim/lib/internal/file/execute-tool.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,16 +107,19 @@ export const executeFileTool: InternalToolOperationHandler = async (request) =>
107107
workspaceId,
108108
query: searchInput.data.query,
109109
maxResults: searchInput.data.maxResults,
110+
signal: request.signal,
110111
},
111112
})
113+
request.signal?.throwIfAborted()
112114
const { sources, ...data } = result
113115
const includePrivateProvenance = requestsPrivateToolMetadata(
114116
request.headers,
115117
RESOLVED_SECRET_PROVENANCE_METADATA_V1
116118
)
117119
const provenance = includePrivateProvenance
118-
? await getFileContentProvenance(principal, workspaceId, sources)
120+
? await getFileContentProvenance(principal, workspaceId, sources, request.signal)
119121
: undefined
122+
request.signal?.throwIfAborted()
120123
return fileContentJsonResponse(
121124
{ success: true, data },
122125
includePrivateProvenance,

apps/sim/lib/internal/file/operations.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -345,8 +345,10 @@ async function bindSelectedContentFile(
345345
export async function getFileContentProvenance(
346346
principal: Principal,
347347
workspaceId: string,
348-
sources: readonly FileContentProvenanceSource[]
348+
sources: readonly FileContentProvenanceSource[],
349+
signal?: AbortSignal
349350
): Promise<ResolvedSecretTraceProvenanceV1> {
351+
signal?.throwIfAborted()
350352
const ownerIds = new Set(
351353
sources
352354
.map((source) => source.ownerUserId)
@@ -359,6 +361,7 @@ export async function getFileContentProvenance(
359361
const accumulator = new ResolvedSecretTraceProvenanceAccumulator(scope)
360362

361363
for (const source of sources) {
364+
signal?.throwIfAborted()
362365
if (!source.identity || !source.ownerUserId) {
363366
accumulator.markIncomplete('file-source-unidentified')
364367
continue
@@ -371,6 +374,7 @@ export async function getFileContentProvenance(
371374
expectedContentUpdatedAt: source.identity.contentUpdatedAt,
372375
},
373376
})
377+
signal?.throwIfAborted()
374378
/**
375379
* `unrecorded` is a more specific `unknown`, and this accumulator has not opted into the
376380
* workspace file surface's policy, so it latches exactly as it did before.
@@ -766,7 +770,7 @@ export async function executeFileManageOperation(
766770

767771
logger.info('File content extracted', { count: contents.length })
768772
const provenance = includePrivateContentProvenance
769-
? await getFileContentProvenance(principal, workspaceId, sources)
773+
? await getFileContentProvenance(principal, workspaceId, sources, signal)
770774
: undefined
771775

772776
return contentResponse({ success: true, data: { contents } }, undefined, provenance)

apps/sim/lib/workspace-files/application/search-workspace-file-content.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,23 +9,27 @@ export interface SearchWorkspaceFileContentInput {
99
workspaceId: string
1010
query: string
1111
maxResults: number
12+
signal?: AbortSignal
1213
}
1314

14-
async function resolveSearchWorkspaceFileContext(workspaceId: string) {
15-
const workspace = await loadActiveWorkspaceContext(workspaceId)
15+
async function resolveSearchWorkspaceFileContext(input: SearchWorkspaceFileContentInput) {
16+
input.signal?.throwIfAborted()
17+
const workspace = await loadActiveWorkspaceContext(input.workspaceId)
18+
input.signal?.throwIfAborted()
1619
if (!workspace) throw new OrchestrationError('not_found', 'Workspace not found')
1720
return workspace
1821
}
1922

2023
export const searchWorkspaceFileContent = defineAuthorizedWorkspaceFileUseCase({
2124
operation: fileOperations.searchContent,
2225
resolveContext: ({ input }: { input: SearchWorkspaceFileContentInput }) =>
23-
resolveSearchWorkspaceFileContext(input.workspaceId),
26+
resolveSearchWorkspaceFileContext(input),
2427
execute: ({ input, context }) =>
2528
searchWorkspaceFileIndex({
2629
workspaceId: context.workspaceId,
2730
query: input.query,
2831
maxResults: input.maxResults,
2932
caseSensitive: isFileSearchCaseSensitive(input.query),
33+
signal: input.signal,
3034
}),
3135
})

apps/sim/lib/workspace-files/search/repository.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,17 @@ interface SearchWorkspaceFileIndexInput {
4242
query: string
4343
maxResults: number
4444
caseSensitive: boolean
45+
signal?: AbortSignal
4546
}
4647

4748
export async function searchWorkspaceFileIndex({
4849
workspaceId,
4950
query,
5051
maxResults,
5152
caseSensitive,
53+
signal,
5254
}: SearchWorkspaceFileIndexInput): Promise<WorkspaceFileSearchResult> {
55+
signal?.throwIfAborted()
5356
const escapedPattern = `%${escapeFileSearchLikePattern(query)}%`
5457
const matchExpression = caseSensitive
5558
? sql`${workspaceFileSearchSegment.content} LIKE ${escapedPattern} ESCAPE '\\'`
@@ -110,6 +113,7 @@ export async function searchWorkspaceFileIndex({
110113
)
111114
.limit(maxResults + 1)
112115

116+
signal?.throwIfAborted()
113117
const coverageRows = await db
114118
.select({
115119
readyFiles: sql<number>`count(*) filter (where ${workspaceFileSearchIndex.status} = 'ready')::int`,
@@ -134,6 +138,7 @@ export async function searchWorkspaceFileIndex({
134138
)
135139
)
136140

141+
signal?.throwIfAborted()
137142
const resultRows = rows.slice(0, maxResults)
138143
const indexStatus = coverageRows[0] ?? {
139144
readyFiles: 0,
@@ -163,6 +168,7 @@ export async function searchWorkspaceFileIndex({
163168
suffixOmitted: row.segmentStart + row.content.length < row.lineLength,
164169
}),
165170
}))
171+
signal?.throwIfAborted()
166172
return {
167173
results,
168174
count: results.length,

apps/sim/lib/workspace-files/search/text.test.ts

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { Buffer } from 'node:buffer'
2-
import { describe, expect, it } from 'vitest'
2+
import { describe, expect, it, vi } from 'vitest'
33
import {
44
createFileSearchPreview,
55
escapeFileSearchLikePattern,
@@ -51,6 +51,22 @@ describe('workspace file search text utilities', () => {
5151
expect(Buffer.byteLength(preview, 'utf8')).toBeLessThanOrEqual(2048)
5252
})
5353

54+
it('centers previews with locale-independent case folding', () => {
55+
const localeLowerCase = vi
56+
.spyOn(String.prototype, 'toLocaleLowerCase')
57+
.mockImplementation(function (this: string) {
58+
return String(this).replaceAll('I', 'ı').toLowerCase()
59+
})
60+
61+
try {
62+
const line = `${'x'.repeat(1500)}I${'y'.repeat(1500)}`
63+
const preview = createFileSearchPreview(line, 'i', false, 128)
64+
expect(preview).toContain('I')
65+
} finally {
66+
localeLowerCase.mockRestore()
67+
}
68+
})
69+
5470
it('shows omitted logical-line content beyond the selected segment', () => {
5571
expect(
5672
createFileSearchPreview('needle and nearby text', 'needle', false, 2048, {

apps/sim/lib/workspace-files/search/text.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -120,8 +120,8 @@ function findFileSearchMatchRange(
120120
return { start, end: Math.min(line.length, start + query.length) }
121121
}
122122

123-
const searchableLine = line.toLocaleLowerCase()
124-
const searchableQuery = query.toLocaleLowerCase()
123+
const searchableLine = line.toLowerCase()
124+
const searchableQuery = query.toLowerCase()
125125
const foldedStart = searchableLine.indexOf(searchableQuery)
126126
if (foldedStart < 0) return { start: 0, end: Math.min(line.length, query.length) }
127127

@@ -131,7 +131,7 @@ function findFileSearchMatchRange(
131131
const codePoint = line.codePointAt(offset)
132132
if (codePoint === undefined) break
133133
const character = String.fromCodePoint(codePoint)
134-
const foldedCharacter = character.toLocaleLowerCase()
134+
const foldedCharacter = character.toLowerCase()
135135
const end = offset + character.length
136136
for (let foldedOffset = 0; foldedOffset < foldedCharacter.length; foldedOffset += 1) {
137137
originalStarts.push(offset)

helm/sim/templates/_helpers.tpl

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,19 @@ If release name contains chart name it will be used as a full name.
2323
{{- end }}
2424
{{- end }}
2525

26+
{{/*
27+
Create a CronJob name that leaves room for the Job controller's generated suffix.
28+
Long names retain a stable hash so independently configured jobs cannot collide.
29+
*/}}
30+
{{- define "sim.cronjobName" -}}
31+
{{- $name := printf "%s-%s" (include "sim.fullname" .root) .jobName -}}
32+
{{- if gt (len $name) 52 -}}
33+
{{- printf "%s-%s" ($name | trunc 43 | trimSuffix "-") ($name | sha256sum | trunc 8) -}}
34+
{{- else -}}
35+
{{- $name -}}
36+
{{- end -}}
37+
{{- end }}
38+
2639
{{/*
2740
Create chart name and version as used by the chart label.
2841
*/}}
@@ -669,4 +682,4 @@ Validate Copilot configuration
669682
{{- end -}}
670683
{{- end -}}
671684
{{- end -}}
672-
{{- end }}
685+
{{- end }}

0 commit comments

Comments
 (0)