Skip to content

Commit 60a77cc

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

3 files changed

Lines changed: 56 additions & 17 deletions

File tree

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -277,9 +277,11 @@ describe('ExecutionEngine', () => {
277277

278278
/**
279279
* The crossing at the copilot boundary reads the absence of an attached result as "no block
280-
* ran". That inference is only total if every throw out of `run` carries one.
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.
281283
*/
282-
it('attaches the execution result to every throw out of a failed run', async () => {
284+
it('attaches the execution result to a non-Error thrown by its own work', async () => {
283285
const node = createMockNode('function-1', 'function')
284286
const registry = new ResolvedSecretTraceRegistry([
285287
{ name: 'TOKEN', plaintext: 'secret-value-1234', encryptedValue: 'ciphertext' },
@@ -289,14 +291,13 @@ describe('ExecutionEngine', () => {
289291
decisions: { router: new Map(), condition: new Map() },
290292
resolvedSecretTraceRegistry: registry,
291293
})
292-
const nodeOrchestrator = createMockNodeOrchestrator()
293-
vi.mocked(nodeOrchestrator.executeNode).mockRejectedValue(new Error('block exploded'))
294+
mockIsExecutionCancelled.mockRejectedValueOnce('cancellation lookup exploded')
294295

295296
const engine = new ExecutionEngine(
296297
context,
297298
createMockDAG([node]),
298299
createMockEdgeManager(),
299-
nodeOrchestrator
300+
createMockNodeOrchestrator()
300301
)
301302

302303
const thrown = await engine.run(node.id).catch((error: unknown) => error)

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,37 @@ describe('Copilot workflow run application commands', () => {
440440
)
441441
})
442442

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

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

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -253,6 +253,12 @@ async function executeCopilotRun(params: {
253253
params.executionInput
254254
)
255255
const completePendingActivation = registry?.beginPendingActivation()
256+
/**
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.
260+
*/
261+
let executed = false
256262
/**
257263
* The executor call is the first statement of this `try`, so everything caught below is
258264
* post-dispatch by construction, while authorization, admission and provenance export all
@@ -305,6 +311,7 @@ async function executeCopilotRun(params: {
305311
},
306312
childExecutionId
307313
)
314+
executed = true
308315
if (registry) {
309316
await registry.importCrossingProvenance(
310317
result.executionState?.resolvedSecretTraceProvenance,
@@ -331,21 +338,21 @@ async function executeCopilotRun(params: {
331338
const executionResult = hasExecutionResult(error) ? error.executionResult : undefined
332339
try {
333340
/**
334-
* A run that never reached the engine carried nothing back: the executor attaches its
335-
* result to every throw, so its absence means no block ran, and the three content fields
336-
* below are all undefined. The only content is `thrownMessage`, which this layer produced
337-
* — an admission, validation, or setup failure — and which the tool boundary still
338-
* projects against this registry before any of it reaches a model.
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.
339344
*
340-
* Reporting that as unvouchable cost the caller the reason its run failed: the crossing
341-
* latched, and the tool result was reduced to "result unavailable" for a message that
342-
* named no secret because none had been resolved yet. An engine that did run and could
343-
* not vouch still hands back an incomplete envelope, and that still latches.
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.
344351
*/
345352
await registry.importCrossingProvenance(
346-
executionResult
347-
? executionResult.executionState?.resolvedSecretTraceProvenance
348-
: emptyResolvedSecretTraceProvenance(),
353+
!executed && !executionResult
354+
? emptyResolvedSecretTraceProvenance()
355+
: executionResult?.executionState?.resolvedSecretTraceProvenance,
349356
{
350357
output: executionResult?.output,
351358
logs: executionResult?.logs,

0 commit comments

Comments
 (0)