Skip to content

Commit 4260004

Browse files
committed
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.
1 parent 60a77cc commit 4260004

3 files changed

Lines changed: 64 additions & 21 deletions

File tree

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -471,6 +471,33 @@ describe('Copilot workflow run application commands', () => {
471471
)
472472
})
473473

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+
474501
/** A run that did execute and could not vouch still hands back its incomplete envelope. */
475502
it('passes through an incomplete envelope from a run that did execute', async () => {
476503
const { importCrossingProvenance, lifecycle: tracked } = trackingLifecycle()

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

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -254,11 +254,12 @@ async function executeCopilotRun(params: {
254254
)
255255
const completePendingActivation = registry?.beginPendingActivation()
256256
/**
257-
* Whether the executor returned. The post-run crossing is inside the same `try`, so its own
258-
* failure lands in the catch carrying no execution result — indistinguishable, on that
259-
* evidence alone, from a run that never started. This records the difference the error cannot.
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.
260261
*/
261-
let executed = false
262+
let runResult: ExecutionResult | undefined
262263
/**
263264
* The executor call is the first statement of this `try`, so everything caught below is
264265
* post-dispatch by construction, while authorization, admission and provenance export all
@@ -311,7 +312,7 @@ async function executeCopilotRun(params: {
311312
},
312313
childExecutionId
313314
)
314-
executed = true
315+
runResult = result
315316
if (registry) {
316317
await registry.importCrossingProvenance(
317318
result.executionState?.resolvedSecretTraceProvenance,
@@ -335,24 +336,23 @@ async function executeCopilotRun(params: {
335336
* as never started and invite the duplicate this id exists to prevent.
336337
*/
337338
if (registry) {
338-
const executionResult = hasExecutionResult(error) ? error.executionResult : 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
339345
try {
340346
/**
341-
* A run that never started carried nothing back, and saying so keeps the caller's
342-
* failure reason instead of reducing the tool result to "result unavailable" for a
343-
* message that named no secret because none had been resolved yet.
344-
*
345-
* Both conditions are required to claim it. `executed` rules out the post-run window,
346-
* where the crossing import is what threw: an execution exists and its provenance was
347-
* never imported, so its content is exactly what cannot be vouched for. The absent
348-
* execution result then rules out the engine having run at all — it attaches one to
349-
* every throw. Anything else hands back whatever envelope it has, and an incomplete one
350-
* still latches.
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.
351351
*/
352352
await registry.importCrossingProvenance(
353-
!executed && !executionResult
354-
? emptyResolvedSecretTraceProvenance()
355-
: executionResult?.executionState?.resolvedSecretTraceProvenance,
353+
executionResult
354+
? executionResult.executionState?.resolvedSecretTraceProvenance
355+
: emptyResolvedSecretTraceProvenance(),
356356
{
357357
output: executionResult?.output,
358358
logs: executionResult?.logs,

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import { handlePostExecutionPauseState } from '@/lib/workflows/executor/pause-pe
1313
import { ExecutionSnapshot } from '@/executor/execution/snapshot'
1414
import type { ExecutionMetadata, SerializableExecutionState } from '@/executor/execution/types'
1515
import type { ExecutionResult, StreamingExecution } from '@/executor/types'
16+
import { attachExecutionResult, hasExecutionResult } from '@/executor/utils/errors'
1617
import type { ResolvedSecretTraceProvenanceV1 } from '@/executor/utils/resolved-secret-trace-registry'
1718
import type { CoreTriggerType } from '@/stores/logs/filters/types'
1819

@@ -128,6 +129,12 @@ export async function executeWorkflow(
128129
loggingSession.setTrustedExecutionCorrelation(streamConfig.trustedExecutionCorrelation)
129130
}
130131
let postExecutionOwnershipTransferred = false
132+
/**
133+
* Held outside the `try` so the catch can carry it. The executor attaches its result when the
134+
* run itself throws, but the post-execution work below can throw after a run has already
135+
* produced one — and callers read a missing result as proof that no block ran.
136+
*/
137+
let executionResult: ExecutionResult | undefined
131138

132139
try {
133140
const metadata: ExecutionMetadata = {
@@ -169,7 +176,7 @@ export async function executeWorkflow(
169176

170177
const executionStartMs = Date.now()
171178

172-
const result = await executeWorkflowCore({
179+
const result = (executionResult = await executeWorkflowCore({
173180
snapshot,
174181
callbacks: {
175182
onStream: streamConfig?.onStream,
@@ -197,7 +204,7 @@ export async function executeWorkflow(
197204
trustedInitialResolvedSecretTraceProvenance:
198205
streamConfig?.trustedInitialResolvedSecretTraceProvenance,
199206
runFromBlock: streamConfig?.runFromBlock,
200-
})
207+
}))
201208

202209
const blockTypes = [
203210
...new Set(
@@ -241,6 +248,15 @@ export async function executeWorkflow(
241248

242249
return result
243250
} catch (error: unknown) {
251+
/**
252+
* Carries the run's result on a failure raised after it produced one — the post-execution
253+
* work below the executor call can throw, and callers read a missing result as proof that no
254+
* block ran. Skipped when the executor already attached its own, which is the more specific
255+
* record, and when the throw is not an object to carry it.
256+
*/
257+
if (executionResult && error instanceof Error && !hasExecutionResult(error)) {
258+
attachExecutionResult(error, executionResult)
259+
}
244260
const errorDiagnostic = loggingSession.projectDiagnosticError(error)
245261
logger.error(`[${requestId}] Workflow execution failed`, errorDiagnostic)
246262

0 commit comments

Comments
 (0)