Skip to content

Commit 2579549

Browse files
waleedlatif1waleedlatif1
committed
fix(agents): retain legacy failures and continue output cleanup
1 parent 54172b7 commit 2579549

5 files changed

Lines changed: 257 additions & 2 deletions

File tree

‎apps/sim/lib/function-execution/execute-request.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2091,8 +2091,19 @@ async function discardUploadedSandboxFiles(files: readonly UserFile[]): Promise<
20912091
)
20922092
if (context === 'copilot') {
20932093
const failedKeys = new Set(result.failed.map((failure) => failure.key))
2094+
let metadataFailures = 0
20942095
for (const file of files) {
2095-
if (!failedKeys.has(file.key)) await deleteFileMetadata(file.key)
2096+
if (failedKeys.has(file.key)) continue
2097+
try {
2098+
await deleteFileMetadata(file.key)
2099+
} catch {
2100+
metadataFailures++
2101+
}
2102+
}
2103+
if (metadataFailures > 0) {
2104+
logger.warn('Could not remove some sandbox output file metadata', {
2105+
fileCount: metadataFailures,
2106+
})
20962107
}
20972108
}
20982109
if (result.failed.length > 0) {

‎apps/sim/lib/tool-execution/application/direct-function.test.ts‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -476,6 +476,39 @@ describe('direct Function execution', () => {
476476
expect(mocks.deleteFiles).toHaveBeenCalledWith(['copilot/file/report.txt'], 'copilot')
477477
expect(mocks.deleteMetadata).not.toHaveBeenCalled()
478478
})
479+
it('continues metadata cleanup after an earlier metadata update fails', async () => {
480+
mocks.deleteFiles.mockResolvedValue({ deleted: 2, failed: [] })
481+
mocks.deleteMetadata.mockRejectedValueOnce(new Error('metadata unavailable'))
482+
mocks.sandbox.mockResolvedValue({
483+
result: null,
484+
stdout: '',
485+
sandboxId: 'sandbox',
486+
collectedFiles: ['first.txt', 'second.txt', 'secret.txt'].map((name, index) => {
487+
const content = index === 2 ? 'audit-secret' : 'hi'
488+
return {
489+
path: `/tmp/sim/outputs/${name}`,
490+
relativePath: name,
491+
contentBase64: Buffer.from(content).toString('base64'),
492+
byteLength: Buffer.byteLength(content),
493+
}
494+
}),
495+
})
496+
const result = await run({
497+
code: 'token = {{TOKEN}}',
498+
language: 'python',
499+
secretScope: 'selected',
500+
mountedSecrets: ['TOKEN'],
501+
})
502+
expect(result.status).toBe('failed')
503+
expect(result.error?.message).toContain('contains a resolved secret value')
504+
expect(mocks.deleteFiles).toHaveBeenCalledWith(
505+
['copilot/file/first.txt', 'copilot/file/second.txt'],
506+
'copilot'
507+
)
508+
expect(mocks.deleteMetadata).toHaveBeenNthCalledWith(1, 'copilot/file/first.txt')
509+
expect(mocks.deleteMetadata).toHaveBeenNthCalledWith(2, 'copilot/file/second.txt')
510+
expect(JSON.stringify(result)).not.toContain('audit-secret')
511+
})
479512
it('refuses a secret-bearing filename before personal upload or storage logging', async () => {
480513
mocks.sandbox.mockResolvedValue({
481514
result: null,

‎packages/sim-cli/src/output/run-diagnostics.test.ts‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,182 @@ describe('compact run diagnostics', () => {
4646
expect(result.files).toEqual([{ id: 'file-1', name: 'test.pdf', base64: '[binary omitted]' }])
4747
})
4848

49+
it('retains explicit failures from legacy tool calls on a successful span', () => {
50+
const result = summarizeRun({
51+
status: 'completed',
52+
traceSpans: [
53+
{
54+
blockId: 'agent-1',
55+
name: 'Agent',
56+
type: 'agent',
57+
status: 'success',
58+
toolCalls: [
59+
{
60+
name: 'slack_message',
61+
status: 'error',
62+
error: 'not_in_channel',
63+
input: { channel: 'C123', text: 'Hello' },
64+
output: { ok: false },
65+
},
66+
{
67+
name: 'slack_message',
68+
status: 'success',
69+
output: { error: 'An ordinary output field' },
70+
},
71+
],
72+
},
73+
],
74+
})
75+
76+
expect(result.executionStatus).toBe('completed')
77+
expect(result.observedBlocks).toEqual([
78+
{ blockId: 'agent-1', name: 'Agent', status: 'success' },
79+
])
80+
expect(result.failures).toEqual([
81+
{
82+
blockId: 'agent-1',
83+
name: 'slack_message',
84+
status: 'error',
85+
error: 'not_in_channel',
86+
handled: false,
87+
input: { channel: 'C123', text: 'Hello' },
88+
output: { ok: false },
89+
},
90+
])
91+
expect(result.truncated).toBe(false)
92+
})
93+
94+
it('preserves explicit recovery and bounds legacy tool-call input and output', () => {
95+
const result = summarizeRun({
96+
traceSpans: [
97+
{
98+
blockId: 'agent-1',
99+
status: 'success',
100+
errorHandled: true,
101+
toolCalls: [
102+
{
103+
name: 'render',
104+
error: 'Invalid export',
105+
input: { prompt: 'x'.repeat(500) },
106+
output: { fileBase64: 'FILE_BYTES' },
107+
},
108+
],
109+
},
110+
],
111+
})
112+
113+
expect(result.failures).toMatchObject([
114+
{
115+
blockId: 'agent-1',
116+
name: 'render',
117+
error: 'Invalid export',
118+
handled: true,
119+
output: { fileBase64: '[binary omitted]' },
120+
},
121+
])
122+
expect(JSON.stringify(result)).not.toContain('FILE_BYTES')
123+
expect(JSON.stringify(result)).not.toContain('x'.repeat(401))
124+
expect(result.truncated).toBe(true)
125+
})
126+
127+
it('does not duplicate a span failure with its legacy tool-call error', () => {
128+
const result = summarizeRun({
129+
traceSpans: [
130+
{
131+
blockId: 'agent-1',
132+
name: 'Agent',
133+
status: 'error',
134+
errorMessage: 'not_in_channel',
135+
errorHandled: true,
136+
toolCalls: [{ name: 'slack_message', error: 'not_in_channel' }],
137+
},
138+
],
139+
})
140+
141+
expect(result.failures).toMatchObject([
142+
{ blockId: 'agent-1', name: 'Agent', error: 'not_in_channel', handled: true },
143+
])
144+
expect(result.failures).toHaveLength(1)
145+
})
146+
147+
it('retains distinct legacy failures alongside modern tool children', () => {
148+
const result = summarizeRun({
149+
traceSpans: [
150+
{
151+
blockId: 'agent-1',
152+
name: 'Agent',
153+
status: 'success',
154+
errorHandled: true,
155+
toolCalls: [{ name: 'lookup', error: 'Unavailable' }],
156+
children: [
157+
{
158+
type: 'tool',
159+
name: 'lookup',
160+
status: 'error',
161+
errorMessage: 'Unavailable',
162+
errorHandled: true,
163+
},
164+
],
165+
},
166+
],
167+
})
168+
169+
expect(result.failures).toHaveLength(2)
170+
expect(result.failures).toMatchObject([
171+
{ blockId: 'agent-1', name: 'lookup', error: 'Unavailable', handled: true },
172+
{ name: 'lookup', error: 'Unavailable', handled: true },
173+
])
174+
expect(result.truncated).toBe(false)
175+
})
176+
177+
it('bounds legacy call inspection across spans without reading past the limit', () => {
178+
const firstCalls = Array.from({ length: 60 }, () => ({ name: 'lookup' }))
179+
const lastCalls = Array.from({ length: 40 }, () => ({ name: 'lookup' }))
180+
const beyondLimit = vi.fn(() => {
181+
throw new Error('Tool calls beyond the diagnostic limit must not be read')
182+
})
183+
Object.defineProperty(lastCalls, 40, { get: beyondLimit })
184+
185+
const result = summarizeRun({
186+
traceSpans: [
187+
{ blockId: 'first', toolCalls: firstCalls },
188+
{ blockId: 'last', toolCalls: lastCalls },
189+
],
190+
})
191+
192+
expect(beyondLimit).not.toHaveBeenCalled()
193+
expect(result.observedBlocks).toHaveLength(2)
194+
expect(result.failures).toEqual([])
195+
expect(result.truncated).toBe(true)
196+
})
197+
198+
it('shares the failure budget with tool calls and ignores malformed or error-shaped data', () => {
199+
const result = summarizeRun({
200+
traceSpans: [
201+
{ status: 'error', errorMessage: 'Block failed' },
202+
{
203+
blockId: 'agent-1',
204+
toolCalls: [
205+
null,
206+
{ name: 'lookup', error: '' },
207+
{ name: 'lookup', output: { error: 'Ordinary data' } },
208+
...Array.from({ length: 20 }, (_, index) => ({
209+
name: `lookup_${index}`,
210+
error: 'Provider rejected the call',
211+
})),
212+
],
213+
},
214+
],
215+
})
216+
217+
expect(result.failures).toHaveLength(10)
218+
expect(result.failures).toMatchObject([
219+
{ error: 'Block failed' },
220+
...Array.from({ length: 9 }, (_, index) => ({ name: `lookup_${index}` })),
221+
])
222+
expect(result.truncated).toBe(true)
223+
})
224+
49225
it('bounds wide/deep traces, long text, and nested output values', () => {
50226
const result = summarizeRun({
51227
traceSpans: Array.from({ length: 1000 }, (_, i) => ({

‎packages/sim-cli/src/output/run-diagnostics.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { isRecordLike } from '@sim/utils/object'
22
import { truncate } from '@sim/utils/string'
33

44
const MAX_SPANS = 100
5+
const MAX_TOOL_CALLS = 100
56
const MAX_FAILURES = 10
67
const MAX_FIELDS = 12
78
const MAX_DEPTH = 4
@@ -65,6 +66,7 @@ export function summarizeRun(log: unknown): Record<string, unknown> {
6566
/** Iterator frames keep traversal memory proportional to depth, not trace width. */
6667
const pending = [(Array.isArray(log.traceSpans) ? log.traceSpans : [])[Symbol.iterator]()]
6768
let visited = 0
69+
let visitedToolCalls = 0
6870
while (pending.length > 0 && visited < MAX_SPANS) {
6971
const next = pending[pending.length - 1].next()
7072
if (next.done) {
@@ -90,6 +92,30 @@ export function summarizeRun(log: unknown): Record<string, unknown> {
9092
output: compact(span.output),
9193
})
9294
} else truncated = true
95+
} else if (Array.isArray(span.toolCalls)) {
96+
/** Older persisted traces store tool calls here instead of in child spans. */
97+
for (let index = 0; index < span.toolCalls.length; index++) {
98+
if (visitedToolCalls === MAX_TOOL_CALLS) {
99+
truncated = true
100+
break
101+
}
102+
visitedToolCalls++
103+
const call = span.toolCalls[index]
104+
if (!isRecordLike(call) || typeof call.error !== 'string' || !call.error) continue
105+
if (failures.length === MAX_FAILURES) {
106+
truncated = true
107+
break
108+
}
109+
failures.push({
110+
blockId: identity.blockId,
111+
name: compact(call.name),
112+
status: compact(call.status),
113+
error: compact(call.error),
114+
handled: span.errorHandled === true,
115+
input: compact(call.input),
116+
output: compact(call.output),
117+
})
118+
}
93119
}
94120
if (Array.isArray(span.children)) pending.push(span.children[Symbol.iterator]())
95121
}

‎packages/sim-cli/src/runtime/execute.test.ts‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,15 @@ it('runs compact log diagnostics through the generated authenticated read', asyn
118118
data: {
119119
runId: 'run-1',
120120
status: 'completed',
121-
traceSpans: [],
121+
traceSpans: [
122+
{
123+
blockId: 'agent-1',
124+
name: 'Agent',
125+
status: 'success',
126+
errorHandled: true,
127+
toolCalls: [{ name: 'lookup', error: 'Rate limited' }],
128+
},
129+
],
122130
finalOutput: { requiresClarification: true },
123131
workflowState: { source: 'hidden body' },
124132
},
@@ -136,6 +144,7 @@ it('runs compact log diagnostics through the generated authenticated read', asyn
136144
expect(JSON.parse(String(stdout.mock.calls[0][0]))).toMatchObject({
137145
executionStatus: 'completed',
138146
finalOutput: { requiresClarification: true },
147+
failures: [{ blockId: 'agent-1', name: 'lookup', error: 'Rate limited', handled: true }],
139148
})
140149
expect(String(stdout.mock.calls[0][0])).not.toContain('hidden body')
141150
})

0 commit comments

Comments
 (0)