Skip to content

Commit 95d1969

Browse files
fix(provenance): let a run that never started report why it failed (#7281)
* fix(provenance): let a run that never started report why it failed A copilot-run workflow that fails before reaching the engine crossed back with no provenance, which latched the tool's registry and reduced the result to "result unavailable". The caller was told its run failed but not that the workflow was undeployed, or the input invalid, or the slot unavailable — the reasons this layer produces before any block runs, naming no secret because none had been resolved yet. The executor attaches its execution result to every throw, so the absence of one is proof that no block ran: output, logs and error are all undefined and the only content is a message this layer wrote. That is an absence, not an inability to vouch, so the crossing now carries an exact-empty envelope. The message still passes the tool boundary's egress projection against the same registry, so anything that registry knows is still redacted. A run that did execute and could not vouch hands back its incomplete envelope exactly as before, and that still latches. Make the attach total rather than conditional to keep that inference sound. A block failure is already normalized on the way in, so the old `instanceof Error` guard held in practice; what it did not give was a guarantee covering a non-Error raised by the engine's own synchronous work. toError is identity-preserving, so ordinary failures keep their type. The empty envelope moves to the registry module, which owns the vocabulary, replacing a private copy in the logging session so one definition states what "vouched for, naming nothing" is. * fix(provenance): keep the post-run crossing window out of the never-started claim Review round 1, both findings accepted. The post-run crossing runs inside the same try as the executor call, so when that import is what throws, the catch sees an error carrying no execution result — the same evidence a run that never started leaves. The previous condition read that as "nothing crossed" and vouched for it, when in fact an execution exists and its provenance was never imported, which is exactly the content that cannot be vouched for. Record whether the executor returned and require both facts before claiming the absence: not past the executor, and no result attached. Everything else hands back whatever envelope it has, and an incomplete one still latches. The executor test also could not fail against the old gated attach: a block failure is normalized on the way in, so its rejection already arrived as an Error. Drive it through the cancellation subscribe run() awaits before the queue instead, which is its own synchronous work and reaches the catch untouched — the case the total attach exists for. * fix(provenance): carry the run's result through post-execution failures Round 2, cubic's finding accepted — and it was a distinct window, not a restatement of round 1. The executor's post-execution work runs after the run has produced a result but before `executeWorkflow` returns, so a failure there reached callers with no result attached: the run threw nothing itself, and the flag added last round could not be set yet. Every consumer that reads a missing result as "no block ran" was wrong in that window, this crossing included. Fix it where the result lives rather than at each reader. The executor attaches its own on the throws it raises; `executeWorkflow` now does the same for failures raised after it holds one, skipping the case the executor already recorded. Logging and trace spans get the same benefit for free — they read the identical signal. That makes an absent result total again, so the boolean flag goes and the crossing reads one thing: the result from the error, or the one already returned when the failure came later still, from the crossing itself. Only a failure with neither can claim nothing ran. The post-return case now describes content with the run's real envelope rather than latching blind, which is strictly more accurate than either prior behaviour. * fix(provenance): normalize a post-execution failure so it can carry the result Round 3, cubic's finding accepted. The guard added last round required the caught value to already be an `Error`, so a non-Error raised by post-execution work skipped the attach and was rethrown bare — the same hole this branch closed in the executor, left open one layer up by my own change. A Copilot run would have reported an executed workflow as never started and vouched for content it cannot describe. Normalize once at the top of the catch and use that value throughout, including the rethrow, matching what the executor does. `toError` returns an `Error` unchanged, so a custom error class keeps its identity and every ordinary failure is untouched — the existing identity assertion on the rejection path still holds. Two tests: the result reaches an ordinary post-execution failure, and a non-Error one is normalized so it can carry the result too. The second fails against the previous guard.
1 parent d7c9f51 commit 95d1969

8 files changed

Lines changed: 280 additions & 22 deletions

File tree

apps/sim/executor/execution/engine.test.ts

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import { EDGE } from '@/executor/constants'
2828
import type { DAG, DAGNode } from '@/executor/dag/builder'
2929
import type { EdgeManager } from '@/executor/execution/edge-manager'
3030
import type { NodeExecutionOrchestrator } from '@/executor/orchestrators/node'
31-
import type { ExecutionContext } from '@/executor/types'
31+
import type { ExecutionContext, ExecutionResult } from '@/executor/types'
3232
import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
3333
import type { SerializedBlock } from '@/serializer/types'
3434
import { ExecutionEngine } from './engine'
@@ -275,6 +275,39 @@ describe('ExecutionEngine', () => {
275275
expect(provenance?.entries).toEqual([{ name: 'TOKEN', encryptedValue: 'ciphertext' }])
276276
})
277277

278+
/**
279+
* The crossing at the copilot boundary reads the absence of an attached result as "no block
280+
* ran", so the attach has to be total. A block failure is normalized on the way in, so only
281+
* a non-Error raised by `run`'s own work — here the cancellation subscribe it awaits before
282+
* the queue — reaches the catch untouched and exercises the guarantee.
283+
*/
284+
it('attaches the execution result to a non-Error thrown by its own work', async () => {
285+
const node = createMockNode('function-1', 'function')
286+
const registry = new ResolvedSecretTraceRegistry([
287+
{ name: 'TOKEN', plaintext: 'secret-value-1234', encryptedValue: 'ciphertext' },
288+
])
289+
registry.recordResolved('TOKEN', 'secret-value-1234')
290+
const context = createMockContext({
291+
decisions: { router: new Map(), condition: new Map() },
292+
resolvedSecretTraceRegistry: registry,
293+
})
294+
mockIsExecutionCancelled.mockRejectedValueOnce('cancellation lookup exploded')
295+
296+
const engine = new ExecutionEngine(
297+
context,
298+
createMockDAG([node]),
299+
createMockEdgeManager(),
300+
createMockNodeOrchestrator()
301+
)
302+
303+
const thrown = await engine.run(node.id).catch((error: unknown) => error)
304+
305+
expect(thrown).toBeInstanceOf(Error)
306+
const attached = (thrown as Error & { executionResult?: ExecutionResult }).executionResult
307+
expect(attached).toBeDefined()
308+
expect(attached?.executionState?.resolvedSecretTraceProvenance).toBeDefined()
309+
})
310+
278311
/** Deriving must not weaken the guarantee: a latched registry still exports incomplete. */
279312
it('keeps the final output envelope incomplete when the registry latched', async () => {
280313
const node = createMockNode('loop-1', 'loop')

apps/sim/executor/execution/engine.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -185,10 +185,17 @@ export class ExecutionEngine {
185185
metadata: this.context.metadata,
186186
}
187187

188-
if (error instanceof Error) {
189-
attachExecutionResult(error, executionResult)
190-
}
191-
throw error
188+
/**
189+
* Normalized first so the attach is total rather than conditional on the throw already
190+
* being an `Error`. A block failure is normalized on the way in, so the old guard held in
191+
* practice; what it did not give was a guarantee. The copilot crossing reads a missing
192+
* result as proof that no block ran, and that inference has to hold for every throw out of
193+
* here, including a non-`Error` raised by this file's own synchronous work. `toError`
194+
* returns an `Error` unchanged, so ordinary failures keep their identity and their type.
195+
*/
196+
const thrown = toError(error)
197+
attachExecutionResult(thrown, executionResult)
198+
throw thrown
192199
} finally {
193200
this.cleanup()
194201
}

apps/sim/executor/utils/resolved-secret-trace-registry.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,18 @@ export interface ResolvedSecretIncompletenessDiagnostics {
226226
export const ANONYMOUS_SECRET_TRACE_REPLACEMENT = OPAQUE_RESOLVED_SECRET_REPLACEMENT
227227
export const RESOLVED_SECRET_TRACE_CHECKPOINT_VERSION = 1
228228

229+
/**
230+
* The envelope for content no secret ever reached: vouched for, naming nothing.
231+
*
232+
* Distinct from an incomplete envelope, which says the opposite — that something may be carried
233+
* and cannot be named. A boundary that knows nothing was resolved should say so with this rather
234+
* than latch, since latching is the claim that redaction is impossible. Returned fresh so no
235+
* caller shares a value it may serialize or extend.
236+
*/
237+
export function emptyResolvedSecretTraceProvenance(): ResolvedSecretTraceProvenanceV1 {
238+
return { version: 1, complete: true, entries: [] }
239+
}
240+
229241
const MAX_PROVENANCE_ENTRIES = PROVENANCE_MAX_ENTRIES
230242
const MAX_SERIALIZED_PROVENANCE_BYTES = PROVENANCE_MAX_SERIALIZED_BYTES
231243
const MAX_TRACE_CATALOG_ENTRIES = PROVENANCE_MAX_ENTRIES

apps/sim/lib/logs/execution/logging-session.ts

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import type { SerializableExecutionState } from '@/executor/execution/types'
4646
import type { BlockLog } from '@/executor/types'
4747
import { projectResolvedSecretDiagnosticError } from '@/executor/utils/resolved-secret-content-projection'
4848
import {
49+
emptyResolvedSecretTraceProvenance,
4950
isResolvedSecretTraceProvenanceV1,
5051
RESOLVED_SECRET_TRACE_CHECKPOINT_VERSION,
5152
type ResolvedSecretTraceProvenanceV1,
@@ -124,10 +125,6 @@ function getActiveBlockDisplayProvenance(
124125

125126
const logger = createLogger('LoggingSession')
126127

127-
function emptyResolvedSecretTraceProvenance(): ResolvedSecretTraceProvenanceV1 {
128-
return { version: 1, complete: true, entries: [] }
129-
}
130-
131128
type CompletionAttempt = 'complete' | 'error' | 'cancelled' | 'paused'
132129

133130
export interface SecretSafeDisplayContent {

apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -391,4 +391,133 @@ describe('Copilot workflow run application commands', () => {
391391
expect(readAttemptedExecutionId(error)).toBeUndefined()
392392
})
393393
})
394+
395+
describe('failed-run provenance crossing', () => {
396+
function trackingLifecycle() {
397+
const importCrossingProvenance = vi.fn().mockResolvedValue(true)
398+
return {
399+
importCrossingProvenance,
400+
lifecycle: {
401+
resolvedSecretTraceRegistry: {
402+
exportProvenanceForValue: vi.fn(() => undefined),
403+
beginPendingActivation: vi.fn(() => vi.fn()),
404+
importCrossingProvenance,
405+
},
406+
},
407+
}
408+
}
409+
410+
async function runExpectingFailure(input: { lifecycle: unknown }) {
411+
await expect(
412+
runWorkflowFromCopilot.execute({
413+
principal,
414+
input: {
415+
workflowId: 'workflow-1',
416+
useDraftState: true,
417+
lifecycle: input.lifecycle,
418+
hasWorkflowInput: false,
419+
useMockPayload: true,
420+
},
421+
})
422+
).rejects.toThrow()
423+
}
424+
425+
/**
426+
* The executor attaches its result to every throw, so a failure without one never reached a
427+
* block. Nothing crossed, and saying so keeps the caller's tool result — and the reason its
428+
* run could not start — instead of reducing it to "result unavailable".
429+
*/
430+
it('vouches for a failure that never reached the engine', async () => {
431+
const { importCrossingProvenance, lifecycle: tracked } = trackingLifecycle()
432+
mocks.executeWorkflow.mockRejectedValueOnce(new Error('workflow is not deployed'))
433+
434+
await runExpectingFailure({ lifecycle: tracked })
435+
436+
expect(importCrossingProvenance).toHaveBeenCalledWith(
437+
{ version: 1, complete: true, entries: [] },
438+
expect.objectContaining({ thrownMessage: 'workflow is not deployed' }),
439+
expect.objectContaining({ origin: 'copilotWorkflowMutation.failedRunCrossing' })
440+
)
441+
})
442+
443+
/**
444+
* The post-run crossing is inside the same try, so its failure reaches the catch with no
445+
* execution result — the same evidence a never-started run leaves. An execution exists and
446+
* its provenance was never imported, so this must not be vouched for.
447+
*/
448+
it('does not vouch when the crossing threw after the run returned', async () => {
449+
const importCrossingProvenance = vi
450+
.fn()
451+
.mockImplementationOnce(() => {
452+
throw new Error('crossing import failed')
453+
})
454+
.mockResolvedValue(true)
455+
456+
await runExpectingFailure({
457+
lifecycle: {
458+
resolvedSecretTraceRegistry: {
459+
exportProvenanceForValue: vi.fn(() => undefined),
460+
beginPendingActivation: vi.fn(() => vi.fn()),
461+
importCrossingProvenance,
462+
},
463+
},
464+
})
465+
466+
expect(importCrossingProvenance).toHaveBeenNthCalledWith(
467+
2,
468+
undefined,
469+
expect.objectContaining({ thrownMessage: 'crossing import failed' }),
470+
expect.objectContaining({ origin: 'copilotWorkflowMutation.failedRunCrossing' })
471+
)
472+
})
473+
474+
/**
475+
* The executor's post-execution work can throw after a run has already produced a result.
476+
* `executeWorkflow` carries it on that throw, so this reaches the catch with a result and
477+
* must not be claimed as never-started.
478+
*/
479+
it('does not vouch when post-execution work threw after the engine ran', async () => {
480+
const { importCrossingProvenance, lifecycle: tracked } = trackingLifecycle()
481+
const incomplete = { version: 1 as const, complete: false, entries: [] }
482+
mocks.executeWorkflow.mockRejectedValueOnce(
483+
Object.assign(new Error('post-execution persistence failed'), {
484+
executionResult: {
485+
success: true,
486+
output: { ran: true },
487+
executionState: { resolvedSecretTraceProvenance: incomplete },
488+
},
489+
})
490+
)
491+
492+
await runExpectingFailure({ lifecycle: tracked })
493+
494+
expect(importCrossingProvenance).toHaveBeenCalledWith(
495+
incomplete,
496+
expect.objectContaining({ output: { ran: true } }),
497+
expect.objectContaining({ origin: 'copilotWorkflowMutation.failedRunCrossing' })
498+
)
499+
})
500+
501+
/** A run that did execute and could not vouch still hands back its incomplete envelope. */
502+
it('passes through an incomplete envelope from a run that did execute', async () => {
503+
const { importCrossingProvenance, lifecycle: tracked } = trackingLifecycle()
504+
const incomplete = { version: 1 as const, complete: false, entries: [] }
505+
const failure = Object.assign(new Error('block failed'), {
506+
executionResult: {
507+
success: false,
508+
output: { partial: true },
509+
executionState: { resolvedSecretTraceProvenance: incomplete },
510+
},
511+
})
512+
mocks.executeWorkflow.mockRejectedValueOnce(failure)
513+
514+
await runExpectingFailure({ lifecycle: tracked })
515+
516+
expect(importCrossingProvenance).toHaveBeenCalledWith(
517+
incomplete,
518+
expect.objectContaining({ output: { partial: true } }),
519+
expect.objectContaining({ origin: 'copilotWorkflowMutation.failedRunCrossing' })
520+
)
521+
})
522+
})
394523
})

apps/sim/lib/workflows/application/run-workflow-from-copilot.ts

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,14 @@ import {
2929
} from '@/lib/workflows/triggers/run-options'
3030
import type { SerializableExecutionState } from '@/executor/execution/types'
3131
import type { ExecutionResult } from '@/executor/types'
32-
import { attachAttemptedExecutionId } from '@/executor/utils/errors'
32+
import { attachAttemptedExecutionId, hasExecutionResult } from '@/executor/utils/errors'
3333

3434
const logger = createLogger('CopilotWorkflowRun')
3535

36-
import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
36+
import {
37+
emptyResolvedSecretTraceProvenance,
38+
type ResolvedSecretTraceRegistry,
39+
} from '@/executor/utils/resolved-secret-trace-registry'
3740

3841
export interface CopilotWorkflowRunLifecycle {
3942
billingAttribution?: BillingAttributionSnapshot
@@ -250,6 +253,13 @@ async function executeCopilotRun(params: {
250253
params.executionInput
251254
)
252255
const completePendingActivation = registry?.beginPendingActivation()
256+
/**
257+
* The run's own result, once the executor returns it. The post-run crossing below is inside the
258+
* same `try`, so its failure reaches the catch carrying nothing — and on that evidence alone it
259+
* is indistinguishable from a run that never started. Holding the result here keeps the real
260+
* envelope available to describe content that certainly exists.
261+
*/
262+
let runResult: ExecutionResult | undefined
253263
/**
254264
* The executor call is the first statement of this `try`, so everything caught below is
255265
* post-dispatch by construction, while authorization, admission and provenance export all
@@ -302,6 +312,7 @@ async function executeCopilotRun(params: {
302312
},
303313
childExecutionId
304314
)
315+
runResult = result
305316
if (registry) {
306317
await registry.importCrossingProvenance(
307318
result.executionState?.resolvedSecretTraceProvenance,
@@ -325,16 +336,23 @@ async function executeCopilotRun(params: {
325336
* as never started and invite the duplicate this id exists to prevent.
326337
*/
327338
if (registry) {
328-
const executionResult =
329-
typeof error === 'object' &&
330-
error !== null &&
331-
'executionResult' in error &&
332-
typeof error.executionResult === 'object'
333-
? (error.executionResult as ExecutionResult)
334-
: undefined
339+
/**
340+
* Either source counts as proof a run exists: the error carries the result when the run or
341+
* its post-execution work threw, and `runResult` holds it when the failure came later still
342+
* — from the crossing below, after the executor had already returned.
343+
*/
344+
const executionResult = hasExecutionResult(error) ? error.executionResult : runResult
335345
try {
346+
/**
347+
* Only a failure with no result from either source can claim nothing ran, and saying so
348+
* keeps the caller's failure reason instead of reducing the tool result to "result
349+
* unavailable" for a message that named no secret because none had been resolved yet.
350+
* Every other failure hands back the envelope it has, and an incomplete one still latches.
351+
*/
336352
await registry.importCrossingProvenance(
337-
executionResult?.executionState?.resolvedSecretTraceProvenance,
353+
executionResult
354+
? executionResult.executionState?.resolvedSecretTraceProvenance
355+
: emptyResolvedSecretTraceProvenance(),
338356
{
339357
output: executionResult?.output,
340358
logs: executionResult?.logs,

apps/sim/lib/workflows/executor/execute-workflow.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ vi.mock('@/lib/workflows/executor/pause-persistence', () => ({
5656
}))
5757

5858
import { executeWorkflow } from '@/lib/workflows/executor/execute-workflow'
59+
import { hasExecutionResult } from '@/executor/utils/errors'
5960

6061
const workflowExecutionLoggerCallIndex = loggerMock.createLogger.mock.calls.findIndex(
6162
([name]) => name === 'WorkflowExecution'
@@ -296,6 +297,44 @@ describe('executeWorkflow', () => {
296297
expect(executionSettled).toBe(true)
297298
})
298299

300+
/**
301+
* Post-execution work runs after the core has produced a result and the executor never sees
302+
* its failure, so this layer is the only one that can carry the result onto it. Callers read a
303+
* missing result as proof that no block ran — a Copilot run would report an executed workflow
304+
* as never started and vouch for content it cannot describe.
305+
*/
306+
it('carries the execution result onto a post-execution failure', async () => {
307+
const result = { success: true, output: { ran: true }, logs: [] }
308+
executeWorkflowCoreMock.mockResolvedValueOnce(result)
309+
handlePostExecutionPauseStateMock.mockRejectedValueOnce(new Error('pause persistence failed'))
310+
311+
const thrown = await executeWorkflow(workflow, 'request-1', undefined, 'actor-1', {
312+
enabled: true,
313+
principal,
314+
billingAttribution,
315+
}).catch((error: unknown) => error)
316+
317+
expect(hasExecutionResult(thrown)).toBe(true)
318+
expect((thrown as { executionResult?: unknown }).executionResult).toBe(result)
319+
})
320+
321+
/** A non-Error cannot carry the result, so it is normalized before anything reads it. */
322+
it('normalizes a non-Error post-execution failure so it can carry the result', async () => {
323+
const result = { success: true, output: { ran: true }, logs: [] }
324+
executeWorkflowCoreMock.mockResolvedValueOnce(result)
325+
handlePostExecutionPauseStateMock.mockRejectedValueOnce('pause persistence exploded')
326+
327+
const thrown = await executeWorkflow(workflow, 'request-1', undefined, 'actor-1', {
328+
enabled: true,
329+
principal,
330+
billingAttribution,
331+
}).catch((error: unknown) => error)
332+
333+
expect(thrown).toBeInstanceOf(Error)
334+
expect(hasExecutionResult(thrown)).toBe(true)
335+
expect((thrown as { executionResult?: unknown }).executionResult).toBe(result)
336+
})
337+
299338
it('transfers post-execution ownership with successful streaming metadata', async () => {
300339
const result = await executeWorkflow(workflow, 'request-1', undefined, 'actor-1', {
301340
enabled: true,

0 commit comments

Comments
 (0)