Skip to content

Commit ebf3ece

Browse files
committed
Fix
1 parent 7e8e77b commit ebf3ece

File tree

7 files changed

+318
-32
lines changed

7 files changed

+318
-32
lines changed

apps/sim/lib/copilot/tools/handlers/vfs.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,8 @@ describe('vfs handlers oversize policy', () => {
5252
)
5353

5454
expect(result.success).toBe(false)
55-
expect(result.error).toContain('smaller grep')
55+
expect(result.error).toContain('more specific pattern')
56+
expect(result.error).toContain('context window')
5657
})
5758

5859
it('fails oversized read results with grep guidance', async () => {
@@ -67,6 +68,8 @@ describe('vfs handlers oversize policy', () => {
6768

6869
expect(result.success).toBe(false)
6970
expect(result.error).toContain('Use grep')
71+
expect(result.error).toContain('offset/limit')
72+
expect(result.error).toContain('context window')
7073
})
7174

7275
it('fails file-backed oversized read placeholders with grep guidance', async () => {
@@ -85,5 +88,7 @@ describe('vfs handlers oversize policy', () => {
8588

8689
expect(result.success).toBe(false)
8790
expect(result.error).toContain('Use grep')
91+
expect(result.error).toContain('offset/limit')
92+
expect(result.error).toContain('context window')
8893
})
8994
})

apps/sim/lib/copilot/tools/handlers/vfs.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ export async function executeVfsGrep(
3737
if (!pattern) {
3838
return { success: false, error: "Missing required parameter 'pattern'" }
3939
}
40+
const outputMode = (params.output_mode as string) ?? 'content'
4041

4142
const workspaceId = context.workspaceId
4243
if (!workspaceId) {
@@ -47,12 +48,11 @@ export async function executeVfsGrep(
4748
const vfs = await getOrMaterializeVFS(workspaceId, context.userId)
4849
const result = vfs.grep(pattern, params.path as string | undefined, {
4950
maxResults: (params.maxResults as number) ?? 50,
50-
outputMode: (params.output_mode as 'content' | 'files_with_matches' | 'count') ?? 'content',
51+
outputMode: outputMode as 'content' | 'files_with_matches' | 'count',
5152
ignoreCase: (params.ignoreCase as boolean) ?? false,
5253
lineNumbers: (params.lineNumbers as boolean) ?? true,
5354
context: (params.context as number) ?? 0,
5455
})
55-
const outputMode = (params.output_mode as string) ?? 'content'
5656
const key =
5757
outputMode === 'files_with_matches' ? 'files' : outputMode === 'count' ? 'counts' : 'matches'
5858
const matchCount = Array.isArray(result)
@@ -65,7 +65,7 @@ export async function executeVfsGrep(
6565
return {
6666
success: false,
6767
error:
68-
'Grep result too large to return inline. Use a smaller grep by narrowing the pattern/path, reducing context, or lowering maxResults.',
68+
'Grep result too large to return inline. Retry grep with a more specific pattern or narrower path, and reduce context or maxResults. Avoid catch-all greps because smaller searches save context window and make follow-up reads cheaper.',
6969
}
7070
}
7171
logger.debug('vfs_grep result', { pattern, path: params.path, outputMode, matchCount })
@@ -168,7 +168,7 @@ export async function executeVfsRead(
168168
return {
169169
success: false,
170170
error:
171-
'Read result too large to return inline. Use grep on this path instead of reading it directly.',
171+
'Read result too large to return inline. Use grep with a more specific pattern or narrower path to locate the relevant section, then retry read with offset/limit. Avoid catch-all greps or full-file reads because they waste context window.',
172172
}
173173
}
174174
const windowedUpload = applyWindow(uploadResult)
@@ -199,7 +199,7 @@ export async function executeVfsRead(
199199
return {
200200
success: false,
201201
error:
202-
'Read result too large to return inline. Use grep on this path instead of reading it directly.',
202+
'Read result too large to return inline. Use grep with a more specific pattern or narrower path to locate the relevant section, then retry read with offset/limit. Avoid catch-all greps or full-file reads because they waste context window.',
203203
}
204204
}
205205
const windowedFileContent = applyWindow(fileContent)
@@ -231,7 +231,7 @@ export async function executeVfsRead(
231231
return {
232232
success: false,
233233
error:
234-
'Read result too large to return inline. Use grep on this path instead of reading it directly.',
234+
'Read result too large to return inline. Use grep with a more specific pattern or narrower path to locate the relevant section, then retry read with offset/limit. Avoid catch-all greps or full-file reads because they waste context window.',
235235
}
236236
}
237237
logger.debug('vfs_read result', { path, totalLines: result.totalLines, offset, limit })

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/index.ts

Lines changed: 4 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,9 @@ export const editWorkflowServerTool: BaseServerTool<EditWorkflowParams, unknown>
9292
throw new Error(authorization.message || 'Unauthorized workflow access')
9393
}
9494

95+
const workspaceId = authorization.workflow?.workspaceId ?? undefined
96+
const workflowName = authorization.workflow?.name ?? undefined
97+
9598
logger.info('Executing edit_workflow', {
9699
operationCount: operations.length,
97100
workflowId,
@@ -124,7 +127,7 @@ export const editWorkflowServerTool: BaseServerTool<EditWorkflowParams, unknown>
124127
if (context?.userId) {
125128
const { filteredOperations, errors: credErrors } = await preValidateCredentialInputs(
126129
operations,
127-
{ userId: context.userId },
130+
{ userId: context.userId, workspaceId },
128131
workflowState
129132
)
130133
operationsToApply = filteredOperations
@@ -141,20 +144,6 @@ export const editWorkflowServerTool: BaseServerTool<EditWorkflowParams, unknown>
141144
// Add credential validation errors
142145
validationErrors.push(...credentialErrors)
143146

144-
let workspaceId: string | undefined
145-
let workflowName: string | undefined
146-
try {
147-
const [workflowRecord] = await db
148-
.select({ workspaceId: workflowTable.workspaceId, name: workflowTable.name })
149-
.from(workflowTable)
150-
.where(eq(workflowTable.id, workflowId))
151-
.limit(1)
152-
workspaceId = workflowRecord?.workspaceId ?? undefined
153-
workflowName = workflowRecord?.name ?? undefined
154-
} catch (error) {
155-
logger.warn('Failed to get workflow metadata for validation', { error, workflowId })
156-
}
157-
158147
// Validate selector IDs exist in the database
159148
if (context?.userId) {
160149
try {

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.test.ts

Lines changed: 70 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
/**
22
* @vitest-environment node
33
*/
4-
import { describe, expect, it, vi } from 'vitest'
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
55
import { normalizeConditionRouterIds } from './builders'
6-
import { validateInputsForBlock } from './validation'
6+
7+
const { mockValidateSelectorIds } = vi.hoisted(() => ({
8+
mockValidateSelectorIds: vi.fn(),
9+
}))
710

811
const conditionBlockConfig = {
912
type: 'condition',
@@ -12,6 +15,13 @@ const conditionBlockConfig = {
1215
subBlocks: [{ id: 'conditions', type: 'condition-input' }],
1316
}
1417

18+
const oauthBlockConfig = {
19+
type: 'slack',
20+
name: 'Slack',
21+
outputs: {},
22+
subBlocks: [{ id: 'credential', type: 'oauth-input' }],
23+
}
24+
1525
const routerBlockConfig = {
1626
type: 'router_v2',
1727
name: 'Router',
@@ -23,12 +33,33 @@ vi.mock('@/blocks/registry', () => ({
2333
getBlock: (type: string) =>
2434
type === 'condition'
2535
? conditionBlockConfig
26-
: type === 'router_v2'
27-
? routerBlockConfig
28-
: undefined,
36+
: type === 'slack'
37+
? oauthBlockConfig
38+
: type === 'router_v2'
39+
? routerBlockConfig
40+
: undefined,
41+
}))
42+
43+
vi.mock('@/lib/copilot/validation/selector-validator', () => ({
44+
validateSelectorIds: mockValidateSelectorIds,
2945
}))
3046

47+
vi.mock('@/lib/core/config/feature-flags', () => ({
48+
isHosted: false,
49+
}))
50+
51+
vi.mock('@/providers/utils', () => ({
52+
getHostedModels: () => [],
53+
}))
54+
55+
import { preValidateCredentialInputs, validateInputsForBlock } from './validation'
56+
3157
describe('validateInputsForBlock', () => {
58+
beforeEach(() => {
59+
vi.clearAllMocks()
60+
mockValidateSelectorIds.mockResolvedValue({ valid: [], invalid: [] })
61+
})
62+
3263
it('accepts condition-input arrays with arbitrary item ids', () => {
3364
const result = validateInputsForBlock(
3465
'condition',
@@ -88,3 +119,37 @@ describe('normalizeConditionRouterIds', () => {
88119
expect(normalizeConditionRouterIds('block-1', 'code', input)).toBe(input)
89120
})
90121
})
122+
123+
describe('preValidateCredentialInputs', () => {
124+
beforeEach(() => {
125+
vi.clearAllMocks()
126+
mockValidateSelectorIds.mockResolvedValue({ valid: ['shared-cred-1'], invalid: [] })
127+
})
128+
129+
it('passes workspace context when validating shared oauth credentials', async () => {
130+
const operations = [
131+
{
132+
operation_type: 'edit' as const,
133+
block_id: 'block-1',
134+
params: {
135+
type: 'slack',
136+
inputs: {
137+
credential: 'shared-cred-1',
138+
},
139+
},
140+
},
141+
]
142+
143+
const result = await preValidateCredentialInputs(operations, {
144+
userId: 'user-1',
145+
workspaceId: 'workspace-1',
146+
})
147+
148+
expect(mockValidateSelectorIds).toHaveBeenCalledWith('oauth-input', ['shared-cred-1'], {
149+
userId: 'user-1',
150+
workspaceId: 'workspace-1',
151+
})
152+
expect(result.filteredOperations[0]?.params?.inputs?.credential).toBe('shared-cred-1')
153+
expect(result.errors).toHaveLength(0)
154+
})
155+
})

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/validation.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -792,14 +792,14 @@ export async function validateWorkflowSelectorIds(
792792

793793
/**
794794
* Pre-validates credential and apiKey inputs in operations before they are applied.
795-
* - Validates oauth-input (credential) IDs belong to the user
795+
* - Validates oauth-input (credential) IDs are accessible to the user in the workflow workspace
796796
* - Filters out apiKey inputs for hosted models when isHosted is true
797797
* - Also validates credentials and apiKeys in nestedNodes (blocks inside loop/parallel)
798798
* Returns validation errors for any removed inputs.
799799
*/
800800
export async function preValidateCredentialInputs(
801801
operations: EditWorkflowOperation[],
802-
context: { userId: string },
802+
context: { userId: string; workspaceId?: string },
803803
workflowState?: Record<string, unknown>
804804
): Promise<{ filteredOperations: EditWorkflowOperation[]; errors: ValidationError[] }> {
805805
const { isHosted } = await import('@/lib/core/config/feature-flags')
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mockDbSelect } = vi.hoisted(() => ({
7+
mockDbSelect: vi.fn(),
8+
}))
9+
10+
vi.mock('@sim/db', () => ({
11+
db: {
12+
select: (...args: unknown[]) => mockDbSelect(...args),
13+
},
14+
}))
15+
16+
vi.mock('@sim/db/schema', () => ({
17+
account: {
18+
id: 'account.id',
19+
userId: 'account.userId',
20+
providerId: 'account.providerId',
21+
},
22+
credential: {
23+
id: 'credential.id',
24+
accountId: 'credential.accountId',
25+
displayName: 'credential.displayName',
26+
providerId: 'credential.providerId',
27+
workspaceId: 'credential.workspaceId',
28+
type: 'credential.type',
29+
},
30+
credentialMember: {
31+
credentialId: 'credentialMember.credentialId',
32+
userId: 'credentialMember.userId',
33+
status: 'credentialMember.status',
34+
},
35+
document: {
36+
id: 'document.id',
37+
userExcluded: 'document.userExcluded',
38+
archivedAt: 'document.archivedAt',
39+
deletedAt: 'document.deletedAt',
40+
},
41+
knowledgeBase: {
42+
id: 'knowledgeBase.id',
43+
deletedAt: 'knowledgeBase.deletedAt',
44+
workspaceId: 'knowledgeBase.workspaceId',
45+
},
46+
mcpServers: {
47+
id: 'mcpServers.id',
48+
enabled: 'mcpServers.enabled',
49+
deletedAt: 'mcpServers.deletedAt',
50+
workspaceId: 'mcpServers.workspaceId',
51+
},
52+
workflow: {
53+
id: 'workflow.id',
54+
archivedAt: 'workflow.archivedAt',
55+
},
56+
}))
57+
58+
vi.mock('@sim/logger', () => ({
59+
createLogger: () => ({
60+
warn: vi.fn(),
61+
error: vi.fn(),
62+
}),
63+
}))
64+
65+
vi.mock('drizzle-orm', () => ({
66+
and: vi.fn((...args: unknown[]) => ({ type: 'and', args })),
67+
eq: vi.fn((...args: unknown[]) => ({ type: 'eq', args })),
68+
inArray: vi.fn((...args: unknown[]) => ({ type: 'inArray', args })),
69+
isNull: vi.fn((field: unknown) => ({ type: 'isNull', field })),
70+
or: vi.fn((...args: unknown[]) => ({ type: 'or', args })),
71+
}))
72+
73+
import { validateSelectorIds } from './selector-validator'
74+
75+
function createSelectChain(result: unknown) {
76+
const chain: Record<string, unknown> = {}
77+
Object.assign(chain, {
78+
from: vi.fn().mockReturnValue(chain),
79+
innerJoin: vi.fn().mockReturnValue(chain),
80+
leftJoin: vi.fn().mockReturnValue(chain),
81+
where: vi.fn().mockResolvedValue(result),
82+
})
83+
return chain
84+
}
85+
86+
describe('validateSelectorIds', () => {
87+
beforeEach(() => {
88+
vi.clearAllMocks()
89+
})
90+
91+
it('accepts shared workspace credential ids and legacy account ids for oauth-input', async () => {
92+
mockDbSelect.mockReturnValueOnce(
93+
createSelectChain([{ credentialId: 'cred-1', accountId: 'acct-1' }])
94+
)
95+
96+
const result = await validateSelectorIds('oauth-input', ['cred-1', 'acct-1'], {
97+
userId: 'user-1',
98+
workspaceId: 'workspace-1',
99+
})
100+
101+
expect(result).toEqual({
102+
valid: ['cred-1', 'acct-1'],
103+
invalid: [],
104+
})
105+
expect(mockDbSelect).toHaveBeenCalledTimes(1)
106+
})
107+
108+
it('reports accessible workspace credentials in warnings for invalid oauth-input ids', async () => {
109+
mockDbSelect.mockReturnValueOnce(createSelectChain([])).mockReturnValueOnce(
110+
createSelectChain([
111+
{
112+
id: 'cred-2',
113+
displayName: 'Shared Gmail',
114+
accountId: 'acct-2',
115+
credentialProviderId: null,
116+
accountProviderId: 'google-email',
117+
},
118+
])
119+
)
120+
121+
const result = await validateSelectorIds('oauth-input', 'missing-cred', {
122+
userId: 'user-1',
123+
workspaceId: 'workspace-1',
124+
})
125+
126+
expect(result.valid).toEqual([])
127+
expect(result.invalid).toEqual(['missing-cred'])
128+
expect(result.warning).toContain('Accessible workspace credentials:')
129+
expect(result.warning).toContain('Shared Gmail [cred-2]')
130+
})
131+
})

0 commit comments

Comments
 (0)