diff --git a/apps/sim/executor/execution/engine.test.ts b/apps/sim/executor/execution/engine.test.ts index 2677a9f89a6..49163f6b3ad 100644 --- a/apps/sim/executor/execution/engine.test.ts +++ b/apps/sim/executor/execution/engine.test.ts @@ -28,7 +28,7 @@ import { EDGE } from '@/executor/constants' import type { DAG, DAGNode } from '@/executor/dag/builder' import type { EdgeManager } from '@/executor/execution/edge-manager' import type { NodeExecutionOrchestrator } from '@/executor/orchestrators/node' -import type { ExecutionContext } from '@/executor/types' +import type { ExecutionContext, ExecutionResult } from '@/executor/types' import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' import type { SerializedBlock } from '@/serializer/types' import { ExecutionEngine } from './engine' @@ -275,6 +275,39 @@ describe('ExecutionEngine', () => { expect(provenance?.entries).toEqual([{ name: 'TOKEN', encryptedValue: 'ciphertext' }]) }) + /** + * The crossing at the copilot boundary reads the absence of an attached result as "no block + * ran", so the attach has to be total. A block failure is normalized on the way in, so only + * a non-Error raised by `run`'s own work — here the cancellation subscribe it awaits before + * the queue — reaches the catch untouched and exercises the guarantee. + */ + it('attaches the execution result to a non-Error thrown by its own work', async () => { + const node = createMockNode('function-1', 'function') + const registry = new ResolvedSecretTraceRegistry([ + { name: 'TOKEN', plaintext: 'secret-value-1234', encryptedValue: 'ciphertext' }, + ]) + registry.recordResolved('TOKEN', 'secret-value-1234') + const context = createMockContext({ + decisions: { router: new Map(), condition: new Map() }, + resolvedSecretTraceRegistry: registry, + }) + mockIsExecutionCancelled.mockRejectedValueOnce('cancellation lookup exploded') + + const engine = new ExecutionEngine( + context, + createMockDAG([node]), + createMockEdgeManager(), + createMockNodeOrchestrator() + ) + + const thrown = await engine.run(node.id).catch((error: unknown) => error) + + expect(thrown).toBeInstanceOf(Error) + const attached = (thrown as Error & { executionResult?: ExecutionResult }).executionResult + expect(attached).toBeDefined() + expect(attached?.executionState?.resolvedSecretTraceProvenance).toBeDefined() + }) + /** Deriving must not weaken the guarantee: a latched registry still exports incomplete. */ it('keeps the final output envelope incomplete when the registry latched', async () => { const node = createMockNode('loop-1', 'loop') diff --git a/apps/sim/executor/execution/engine.ts b/apps/sim/executor/execution/engine.ts index acfa7cfa42b..b6091f395e7 100644 --- a/apps/sim/executor/execution/engine.ts +++ b/apps/sim/executor/execution/engine.ts @@ -185,10 +185,17 @@ export class ExecutionEngine { metadata: this.context.metadata, } - if (error instanceof Error) { - attachExecutionResult(error, executionResult) - } - throw error + /** + * Normalized first so the attach is total rather than conditional on the throw already + * being an `Error`. A block failure is normalized on the way in, so the old guard held in + * practice; what it did not give was a guarantee. The copilot crossing reads a missing + * result as proof that no block ran, and that inference has to hold for every throw out of + * here, including a non-`Error` raised by this file's own synchronous work. `toError` + * returns an `Error` unchanged, so ordinary failures keep their identity and their type. + */ + const thrown = toError(error) + attachExecutionResult(thrown, executionResult) + throw thrown } finally { this.cleanup() } diff --git a/apps/sim/executor/utils/resolved-secret-trace-registry.ts b/apps/sim/executor/utils/resolved-secret-trace-registry.ts index 05e8d51cc71..432e0178a85 100644 --- a/apps/sim/executor/utils/resolved-secret-trace-registry.ts +++ b/apps/sim/executor/utils/resolved-secret-trace-registry.ts @@ -226,6 +226,18 @@ export interface ResolvedSecretIncompletenessDiagnostics { export const ANONYMOUS_SECRET_TRACE_REPLACEMENT = OPAQUE_RESOLVED_SECRET_REPLACEMENT export const RESOLVED_SECRET_TRACE_CHECKPOINT_VERSION = 1 +/** + * The envelope for content no secret ever reached: vouched for, naming nothing. + * + * Distinct from an incomplete envelope, which says the opposite — that something may be carried + * and cannot be named. A boundary that knows nothing was resolved should say so with this rather + * than latch, since latching is the claim that redaction is impossible. Returned fresh so no + * caller shares a value it may serialize or extend. + */ +export function emptyResolvedSecretTraceProvenance(): ResolvedSecretTraceProvenanceV1 { + return { version: 1, complete: true, entries: [] } +} + const MAX_PROVENANCE_ENTRIES = PROVENANCE_MAX_ENTRIES const MAX_SERIALIZED_PROVENANCE_BYTES = PROVENANCE_MAX_SERIALIZED_BYTES const MAX_TRACE_CATALOG_ENTRIES = PROVENANCE_MAX_ENTRIES diff --git a/apps/sim/lib/logs/execution/logging-session.ts b/apps/sim/lib/logs/execution/logging-session.ts index c2fe239248b..ece15ee9196 100644 --- a/apps/sim/lib/logs/execution/logging-session.ts +++ b/apps/sim/lib/logs/execution/logging-session.ts @@ -46,6 +46,7 @@ import type { SerializableExecutionState } from '@/executor/execution/types' import type { BlockLog } from '@/executor/types' import { projectResolvedSecretDiagnosticError } from '@/executor/utils/resolved-secret-content-projection' import { + emptyResolvedSecretTraceProvenance, isResolvedSecretTraceProvenanceV1, RESOLVED_SECRET_TRACE_CHECKPOINT_VERSION, type ResolvedSecretTraceProvenanceV1, @@ -124,10 +125,6 @@ function getActiveBlockDisplayProvenance( const logger = createLogger('LoggingSession') -function emptyResolvedSecretTraceProvenance(): ResolvedSecretTraceProvenanceV1 { - return { version: 1, complete: true, entries: [] } -} - type CompletionAttempt = 'complete' | 'error' | 'cancelled' | 'paused' export interface SecretSafeDisplayContent { diff --git a/apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts b/apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts index 593516912b4..d8976c7264e 100644 --- a/apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts +++ b/apps/sim/lib/workflows/application/run-workflow-from-copilot.test.ts @@ -391,4 +391,133 @@ describe('Copilot workflow run application commands', () => { expect(readAttemptedExecutionId(error)).toBeUndefined() }) }) + + describe('failed-run provenance crossing', () => { + function trackingLifecycle() { + const importCrossingProvenance = vi.fn().mockResolvedValue(true) + return { + importCrossingProvenance, + lifecycle: { + resolvedSecretTraceRegistry: { + exportProvenanceForValue: vi.fn(() => undefined), + beginPendingActivation: vi.fn(() => vi.fn()), + importCrossingProvenance, + }, + }, + } + } + + async function runExpectingFailure(input: { lifecycle: unknown }) { + await expect( + runWorkflowFromCopilot.execute({ + principal, + input: { + workflowId: 'workflow-1', + useDraftState: true, + lifecycle: input.lifecycle, + hasWorkflowInput: false, + useMockPayload: true, + }, + }) + ).rejects.toThrow() + } + + /** + * The executor attaches its result to every throw, so a failure without one never reached a + * block. Nothing crossed, and saying so keeps the caller's tool result — and the reason its + * run could not start — instead of reducing it to "result unavailable". + */ + it('vouches for a failure that never reached the engine', async () => { + const { importCrossingProvenance, lifecycle: tracked } = trackingLifecycle() + mocks.executeWorkflow.mockRejectedValueOnce(new Error('workflow is not deployed')) + + await runExpectingFailure({ lifecycle: tracked }) + + expect(importCrossingProvenance).toHaveBeenCalledWith( + { version: 1, complete: true, entries: [] }, + expect.objectContaining({ thrownMessage: 'workflow is not deployed' }), + expect.objectContaining({ origin: 'copilotWorkflowMutation.failedRunCrossing' }) + ) + }) + + /** + * The post-run crossing is inside the same try, so its failure reaches the catch with no + * execution result — the same evidence a never-started run leaves. An execution exists and + * its provenance was never imported, so this must not be vouched for. + */ + it('does not vouch when the crossing threw after the run returned', async () => { + const importCrossingProvenance = vi + .fn() + .mockImplementationOnce(() => { + throw new Error('crossing import failed') + }) + .mockResolvedValue(true) + + await runExpectingFailure({ + lifecycle: { + resolvedSecretTraceRegistry: { + exportProvenanceForValue: vi.fn(() => undefined), + beginPendingActivation: vi.fn(() => vi.fn()), + importCrossingProvenance, + }, + }, + }) + + expect(importCrossingProvenance).toHaveBeenNthCalledWith( + 2, + undefined, + expect.objectContaining({ thrownMessage: 'crossing import failed' }), + expect.objectContaining({ origin: 'copilotWorkflowMutation.failedRunCrossing' }) + ) + }) + + /** + * The executor's post-execution work can throw after a run has already produced a result. + * `executeWorkflow` carries it on that throw, so this reaches the catch with a result and + * must not be claimed as never-started. + */ + it('does not vouch when post-execution work threw after the engine ran', async () => { + const { importCrossingProvenance, lifecycle: tracked } = trackingLifecycle() + const incomplete = { version: 1 as const, complete: false, entries: [] } + mocks.executeWorkflow.mockRejectedValueOnce( + Object.assign(new Error('post-execution persistence failed'), { + executionResult: { + success: true, + output: { ran: true }, + executionState: { resolvedSecretTraceProvenance: incomplete }, + }, + }) + ) + + await runExpectingFailure({ lifecycle: tracked }) + + expect(importCrossingProvenance).toHaveBeenCalledWith( + incomplete, + expect.objectContaining({ output: { ran: true } }), + expect.objectContaining({ origin: 'copilotWorkflowMutation.failedRunCrossing' }) + ) + }) + + /** A run that did execute and could not vouch still hands back its incomplete envelope. */ + it('passes through an incomplete envelope from a run that did execute', async () => { + const { importCrossingProvenance, lifecycle: tracked } = trackingLifecycle() + const incomplete = { version: 1 as const, complete: false, entries: [] } + const failure = Object.assign(new Error('block failed'), { + executionResult: { + success: false, + output: { partial: true }, + executionState: { resolvedSecretTraceProvenance: incomplete }, + }, + }) + mocks.executeWorkflow.mockRejectedValueOnce(failure) + + await runExpectingFailure({ lifecycle: tracked }) + + expect(importCrossingProvenance).toHaveBeenCalledWith( + incomplete, + expect.objectContaining({ output: { partial: true } }), + expect.objectContaining({ origin: 'copilotWorkflowMutation.failedRunCrossing' }) + ) + }) + }) }) diff --git a/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts b/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts index 6467156f445..31ed03811fc 100644 --- a/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts +++ b/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts @@ -29,11 +29,14 @@ import { } from '@/lib/workflows/triggers/run-options' import type { SerializableExecutionState } from '@/executor/execution/types' import type { ExecutionResult } from '@/executor/types' -import { attachAttemptedExecutionId } from '@/executor/utils/errors' +import { attachAttemptedExecutionId, hasExecutionResult } from '@/executor/utils/errors' const logger = createLogger('CopilotWorkflowRun') -import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' +import { + emptyResolvedSecretTraceProvenance, + type ResolvedSecretTraceRegistry, +} from '@/executor/utils/resolved-secret-trace-registry' export interface CopilotWorkflowRunLifecycle { billingAttribution?: BillingAttributionSnapshot @@ -250,6 +253,13 @@ async function executeCopilotRun(params: { params.executionInput ) const completePendingActivation = registry?.beginPendingActivation() + /** + * The run's own result, once the executor returns it. The post-run crossing below is inside the + * same `try`, so its failure reaches the catch carrying nothing — and on that evidence alone it + * is indistinguishable from a run that never started. Holding the result here keeps the real + * envelope available to describe content that certainly exists. + */ + let runResult: ExecutionResult | undefined /** * The executor call is the first statement of this `try`, so everything caught below is * post-dispatch by construction, while authorization, admission and provenance export all @@ -302,6 +312,7 @@ async function executeCopilotRun(params: { }, childExecutionId ) + runResult = result if (registry) { await registry.importCrossingProvenance( result.executionState?.resolvedSecretTraceProvenance, @@ -325,16 +336,23 @@ async function executeCopilotRun(params: { * as never started and invite the duplicate this id exists to prevent. */ if (registry) { - const executionResult = - typeof error === 'object' && - error !== null && - 'executionResult' in error && - typeof error.executionResult === 'object' - ? (error.executionResult as ExecutionResult) - : undefined + /** + * Either source counts as proof a run exists: the error carries the result when the run or + * its post-execution work threw, and `runResult` holds it when the failure came later still + * — from the crossing below, after the executor had already returned. + */ + const executionResult = hasExecutionResult(error) ? error.executionResult : runResult try { + /** + * Only a failure with no result from either source can claim nothing ran, and saying so + * keeps the caller's failure reason instead of reducing the tool result to "result + * unavailable" for a message that named no secret because none had been resolved yet. + * Every other failure hands back the envelope it has, and an incomplete one still latches. + */ await registry.importCrossingProvenance( - executionResult?.executionState?.resolvedSecretTraceProvenance, + executionResult + ? executionResult.executionState?.resolvedSecretTraceProvenance + : emptyResolvedSecretTraceProvenance(), { output: executionResult?.output, logs: executionResult?.logs, diff --git a/apps/sim/lib/workflows/executor/execute-workflow.test.ts b/apps/sim/lib/workflows/executor/execute-workflow.test.ts index 4240058f7d3..f68fadb8a19 100644 --- a/apps/sim/lib/workflows/executor/execute-workflow.test.ts +++ b/apps/sim/lib/workflows/executor/execute-workflow.test.ts @@ -56,6 +56,7 @@ vi.mock('@/lib/workflows/executor/pause-persistence', () => ({ })) import { executeWorkflow } from '@/lib/workflows/executor/execute-workflow' +import { hasExecutionResult } from '@/executor/utils/errors' const workflowExecutionLoggerCallIndex = loggerMock.createLogger.mock.calls.findIndex( ([name]) => name === 'WorkflowExecution' @@ -296,6 +297,44 @@ describe('executeWorkflow', () => { expect(executionSettled).toBe(true) }) + /** + * Post-execution work runs after the core has produced a result and the executor never sees + * its failure, so this layer is the only one that can carry the result onto it. Callers read a + * missing result as proof that no block ran — a Copilot run would report an executed workflow + * as never started and vouch for content it cannot describe. + */ + it('carries the execution result onto a post-execution failure', async () => { + const result = { success: true, output: { ran: true }, logs: [] } + executeWorkflowCoreMock.mockResolvedValueOnce(result) + handlePostExecutionPauseStateMock.mockRejectedValueOnce(new Error('pause persistence failed')) + + const thrown = await executeWorkflow(workflow, 'request-1', undefined, 'actor-1', { + enabled: true, + principal, + billingAttribution, + }).catch((error: unknown) => error) + + expect(hasExecutionResult(thrown)).toBe(true) + expect((thrown as { executionResult?: unknown }).executionResult).toBe(result) + }) + + /** A non-Error cannot carry the result, so it is normalized before anything reads it. */ + it('normalizes a non-Error post-execution failure so it can carry the result', async () => { + const result = { success: true, output: { ran: true }, logs: [] } + executeWorkflowCoreMock.mockResolvedValueOnce(result) + handlePostExecutionPauseStateMock.mockRejectedValueOnce('pause persistence exploded') + + const thrown = await executeWorkflow(workflow, 'request-1', undefined, 'actor-1', { + enabled: true, + principal, + billingAttribution, + }).catch((error: unknown) => error) + + expect(thrown).toBeInstanceOf(Error) + expect(hasExecutionResult(thrown)).toBe(true) + expect((thrown as { executionResult?: unknown }).executionResult).toBe(result) + }) + it('transfers post-execution ownership with successful streaming metadata', async () => { const result = await executeWorkflow(workflow, 'request-1', undefined, 'actor-1', { enabled: true, diff --git a/apps/sim/lib/workflows/executor/execute-workflow.ts b/apps/sim/lib/workflows/executor/execute-workflow.ts index 8336ea332d2..b6779b5c673 100644 --- a/apps/sim/lib/workflows/executor/execute-workflow.ts +++ b/apps/sim/lib/workflows/executor/execute-workflow.ts @@ -1,5 +1,6 @@ import type { WorkflowExecutionPrincipal } from '@sim/auth/principal' import { createLogger } from '@sim/logger' +import { toError } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { assertBillingAttributionSnapshot, @@ -13,6 +14,7 @@ import { handlePostExecutionPauseState } from '@/lib/workflows/executor/pause-pe import { ExecutionSnapshot } from '@/executor/execution/snapshot' import type { ExecutionMetadata, SerializableExecutionState } from '@/executor/execution/types' import type { ExecutionResult, StreamingExecution } from '@/executor/types' +import { attachExecutionResult, hasExecutionResult } from '@/executor/utils/errors' import type { ResolvedSecretTraceProvenanceV1 } from '@/executor/utils/resolved-secret-trace-registry' import type { CoreTriggerType } from '@/stores/logs/filters/types' @@ -128,6 +130,12 @@ export async function executeWorkflow( loggingSession.setTrustedExecutionCorrelation(streamConfig.trustedExecutionCorrelation) } let postExecutionOwnershipTransferred = false + /** + * Held outside the `try` so the catch can carry it. The executor attaches its result when the + * run itself throws, but the post-execution work below can throw after a run has already + * produced one — and callers read a missing result as proof that no block ran. + */ + let executionResult: ExecutionResult | undefined try { const metadata: ExecutionMetadata = { @@ -169,7 +177,7 @@ export async function executeWorkflow( const executionStartMs = Date.now() - const result = await executeWorkflowCore({ + const result = (executionResult = await executeWorkflowCore({ snapshot, callbacks: { onStream: streamConfig?.onStream, @@ -197,7 +205,7 @@ export async function executeWorkflow( trustedInitialResolvedSecretTraceProvenance: streamConfig?.trustedInitialResolvedSecretTraceProvenance, runFromBlock: streamConfig?.runFromBlock, - }) + })) const blockTypes = [ ...new Set( @@ -240,7 +248,22 @@ export async function executeWorkflow( } return result - } catch (error: unknown) { + } catch (caught: unknown) { + /** + * Normalized before anything reads it, for the reason the executor normalizes its own throw: + * a value that cannot carry the result would otherwise reach callers bare, and they read a + * missing result as proof that no block ran. `toError` returns an `Error` unchanged, so a + * custom error class keeps its identity and every ordinary failure is untouched. + */ + const error = toError(caught) + /** + * Carries the run's result on a failure raised after it produced one — the post-execution + * work below the executor call can throw, and the executor never saw it. Skipped when the + * executor already attached its own, which is the more specific record. + */ + if (executionResult && !hasExecutionResult(error)) { + attachExecutionResult(error, executionResult) + } const errorDiagnostic = loggingSession.projectDiagnosticError(error) logger.error(`[${requestId}] Workflow execution failed`, errorDiagnostic)