From 0d88a11be8549c253d80edd10383b368949594d5 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 15:07:30 -0700 Subject: [PATCH 01/16] fix(copilot): disclose how far a withheld run got instead of one opaque sentinel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tool result the egress projection cannot vouch for is reduced to a bare success or to `TOOL_RESULT_UNAVAILABLE_ERROR`. Both drop the execution id with the payload, and the sentinel also overwrites the real error text, so a call rejected on its own arguments and a run that already executed come back byte-identical. Those need opposite retry decisions. Reproduced against real code by latching a registry the way production latches one — a child run that returned no provenance envelope — and driving the real handler and the real projection: a pre-dispatch rejection and a post-dispatch failure were identical, and a completed run arrived as `{"success":true}` with nothing to look it up by. Two distinct shapes for three outcomes. The registry is right to fail closed; the boundary was discarding facts it never needed to redact. A tool may now declare a `ToolCallEffect` — a phase and server-minted ids — which the projection preserves when it withholds content, because neither is derived from that content. The exemption is enforced rather than asserted: ids must match the identifier shape this system mints, and one that does not voids the whole disclosure. The phase is attached in the application layer from dispatch onward and nowhere earlier, which is what makes the id's absence the positive statement that nothing was created rather than an admission of not knowing. Withholding also now reports its cause — a latched registry names the guard that tripped, an absent one means no catalog was built, and a content refusal means the registry was fine — so the next occurrence is diagnosable from the logs it already writes. Co-Authored-By: Claude Opus 5 (1M context) --- .../app/api/copilot/tools/execute/route.ts | 2 + apps/sim/executor/utils/errors.ts | 27 +++ .../sim/lib/copilot/request/tools/executor.ts | 12 +- .../tools/resolved-secret-result.test.ts | 87 +++++++++ .../request/tools/resolved-secret-result.ts | 137 ++++++++++++-- apps/sim/lib/copilot/tool-executor/types.ts | 34 ++++ .../tools/handlers/workflow/mutations.test.ts | 52 +++++- .../tools/handlers/workflow/mutations.ts | 47 ++++- .../workflow/withheld-run-result.test.ts | 173 ++++++++++++++++++ .../run-workflow-from-copilot.test.ts | 48 +++++ .../application/run-workflow-from-copilot.ts | 7 + 11 files changed, 604 insertions(+), 22 deletions(-) create mode 100644 apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts diff --git a/apps/sim/app/api/copilot/tools/execute/route.ts b/apps/sim/app/api/copilot/tools/execute/route.ts index 9da0848f6ca..dd49445de9c 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.ts @@ -9,6 +9,7 @@ import { TraceSpan } from '@/lib/copilot/generated/trace-spans-v1' import { checkInternalApiKey } from '@/lib/copilot/request/http' import { withIncomingGoSpan } from '@/lib/copilot/request/otel' import { + describeWithholdingCause, inspectToolResultForCopilot, projectToolErrorMessageForCopilot, } from '@/lib/copilot/request/tools/resolved-secret-result' @@ -158,6 +159,7 @@ export const POST = withRouteHandler((request: NextRequest) => error: projected.error, runtimeSucceeded: result.success, projectionSafe: projection.safe, + ...(projection.safe ? {} : describeWithholdingCause(projection.cause)), }) } if (result.success && chatId) { diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index ba0d35d0ff1..48927f749fa 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -36,6 +36,33 @@ export function attachExecutionResult(error: Error, executionResult: ExecutionRe Object.assign(error, { executionResult }) } +const ATTEMPTED_EXECUTION_ID = 'attemptedExecutionId' + +/** + * Names the run a failure belongs to once dispatch has been attempted. + * + * A caller that only sees the thrown error cannot tell an authorization refusal — which + * created nothing — from a crash after the run was already dispatched, and those need + * opposite retry decisions. Attaching the id at the point of no return makes its absence + * mean "nothing was started" rather than "we do not know", and its presence a key that + * resolves to zero or one executions. + * + * Distinct from {@link attachExecutionResult}: that says the workflow ran and produced a + * result, this says only that it was dispatched. + */ +export function attachAttemptedExecutionId(error: unknown, executionId: string): void { + if (!(error instanceof Error) || !executionId) return + if (ATTEMPTED_EXECUTION_ID in error) return + Object.assign(error, { [ATTEMPTED_EXECUTION_ID]: executionId }) +} + +/** Reads the dispatched-run id an error carries, if dispatch was reached at all. */ +export function readAttemptedExecutionId(error: unknown): string | undefined { + if (!(error instanceof Error) || !(ATTEMPTED_EXECUTION_ID in error)) return undefined + const value = (error as Error & Record)[ATTEMPTED_EXECUTION_ID] + return typeof value === 'string' && value.length > 0 ? value : undefined +} + export interface BlockExecutionErrorDetails { block: SerializedBlock error: Error | string diff --git a/apps/sim/lib/copilot/request/tools/executor.ts b/apps/sim/lib/copilot/request/tools/executor.ts index 127267555dc..f96192e579d 100644 --- a/apps/sim/lib/copilot/request/tools/executor.ts +++ b/apps/sim/lib/copilot/request/tools/executor.ts @@ -61,7 +61,10 @@ import { setTerminalToolCallState, } from '@/lib/copilot/request/tool-call-state' import { maybeWriteOutputToFile } from '@/lib/copilot/request/tools/files' -import { inspectToolResultForCopilot } from '@/lib/copilot/request/tools/resolved-secret-result' +import { + describeWithholdingCause, + inspectToolResultForCopilot, +} from '@/lib/copilot/request/tools/resolved-secret-result' import { handleResourceSideEffects } from '@/lib/copilot/request/tools/resources' import { maybeWriteOutputToTable, @@ -737,15 +740,20 @@ async function executeToolAndReportInner( toolSpan.attributes = { ...toolSpan.attributes, ...summarizeToolResultForSpan(copilotResult), - ...(projection.safe ? {} : { resultWithheld: true }), + ...(projection.safe + ? {} + : { resultWithheld: true, ...describeWithholdingCause(projection.cause) }), } if (!projection.safe) { // A withheld SUCCESS otherwise leaves no trace anywhere: the span reads // ok and the model just sees a bare `{success: true}` with no output. + // The cause is what says whether a guard latched, no catalog was built, + // or the payload itself was unprojectable — three different fixes. logger.warn('Tool result withheld by egress projection', { toolCallId: toolCall.id, toolName: toolCall.name, runtimeSucceeded: result.success, + ...describeWithholdingCause(projection.cause), }) } diff --git a/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts b/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts index 26285b0d7d5..decf9fe39d1 100644 --- a/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts +++ b/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts @@ -4,6 +4,8 @@ import { describe, expect, it } from 'vitest' import { RunCode, RunFunction } from '@/lib/copilot/generated/tool-catalog-v1' import { + describeWithholdingCause, + inspectToolResultForCopilot, projectToolResultForCopilot, READ_TOOL_RESULT_UNAVAILABLE_ERROR, TOOL_RESULT_UNAVAILABLE_ERROR, @@ -457,3 +459,88 @@ describe('projectToolResultForCopilot', () => { expect(toolResultUnavailableError(undefined)).toBe(TOOL_RESULT_UNAVAILABLE_ERROR) }) }) + +describe('effect disclosure on a withheld result', () => { + const EXECUTION_ID = '0f4d5a4c-6a1e-4c2f-9b7d-2c8f1a3e5d90' + + it('carries nothing extra for a tool that declared no effect', () => { + expect(projectToolResultForCopilot({ success: true, output: { a: 1 } }, undefined)).toEqual({ + success: true, + }) + expect(projectToolResultForCopilot({ success: false, error: 'why' }, undefined)).toEqual({ + success: false, + error: TOOL_RESULT_UNAVAILABLE_ERROR, + }) + }) + + /** + * The exemption is what makes the disclosure trustworthy, so it has to be all or + * nothing: a disclosure that silently dropped the id it could not vouch for would + * read exactly like one that never had a run to name. + */ + it('voids the whole disclosure when an id is not a shape this system mints', () => { + expect( + projectToolResultForCopilot( + { + success: false, + error: 'why', + effect: { phase: 'performed', ids: { executionId: 'sk-live-9Qv2XbTn4LmZa8Rd' } }, + }, + undefined, + 'run_workflow' + ) + ).toEqual({ success: false, error: TOOL_RESULT_UNAVAILABLE_ERROR }) + }) + + it('reports the phase and ids when every id is vouchable', () => { + expect( + projectToolResultForCopilot( + { + success: false, + error: 'why', + effect: { phase: 'attempted', ids: { executionId: EXECUTION_ID } }, + }, + undefined, + 'run_workflow' + ) + ).toEqual({ + success: false, + output: { resultWithheld: true, effect: 'attempted', executionId: EXECUTION_ID }, + error: expect.stringContaining('At most one run exists'), + }) + }) + + it('never leaks the disclosure into a result that projected cleanly', () => { + const registry = new ResolvedSecretTraceRegistry() + + expect( + projectToolResultForCopilot( + { + success: true, + output: { executionId: EXECUTION_ID }, + effect: { phase: 'performed', ids: { executionId: EXECUTION_ID } }, + }, + registry, + 'run_workflow' + ) + ).toEqual({ success: true, output: { executionId: EXECUTION_ID } }) + }) + + it('names why the content was withheld, for the surface about to log it', () => { + const latched = createRegistry() + latched.markIncomplete('source-provenance-incomplete', { origin: 'test.origin' }) + + const projection = inspectToolResultForCopilot({ success: false }, latched, 'run_workflow') + expect(projection.safe).toBe(false) + // The per-call fork adds its own propagation reason; the guard that originally + // tripped has to survive alongside it, or a refusal names only the messenger. + expect(projection.safe === false && describeWithholdingCause(projection.cause)).toEqual({ + withheldCause: 'registry-incomplete', + withheldReasons: expect.arrayContaining(['source-provenance-incomplete']), + withheldOrigins: ['test.origin'], + }) + + const absent = inspectToolResultForCopilot({ success: false }, undefined) + expect(absent.safe === false && absent.cause).toEqual({ kind: 'registry-absent' }) + }) +}) diff --git a/apps/sim/lib/copilot/request/tools/resolved-secret-result.ts b/apps/sim/lib/copilot/request/tools/resolved-secret-result.ts index f6785f60a8d..6c716c0e0cc 100644 --- a/apps/sim/lib/copilot/request/tools/resolved-secret-result.ts +++ b/apps/sim/lib/copilot/request/tools/resolved-secret-result.ts @@ -1,6 +1,10 @@ -import type { ToolExecutionResult } from '@/lib/copilot/tool-executor/types' +import type { ToolCallEffect, ToolExecutionResult } from '@/lib/copilot/tool-executor/types' +import { TOOL_EFFECT_PHASE } from '@/lib/copilot/tool-executor/types' import { projectResolvedSecretModelJsonContent } from '@/executor/utils/resolved-secret-content-projection' -import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' +import type { + ResolvedSecretIncompletenessReason, + ResolvedSecretTraceRegistry, +} from '@/executor/utils/resolved-secret-trace-registry' export const TOOL_RESULT_UNAVAILABLE_ERROR = 'Tool execution settled, but its result could not be returned safely. Do not retry a mutation automatically.' @@ -13,8 +17,35 @@ export const TOOL_RESULT_UNAVAILABLE_ERROR = export const READ_TOOL_RESULT_UNAVAILABLE_ERROR = 'Tool executed, but its result could not be returned safely. The call was read-only, so you may retry it or continue without the result.' +/** + * Withheld-result wording for a call that disclosed how far its side effect got. + * + * The generic message above has to cover both "nothing happened" and "it happened, + * you just cannot see it", which is why a caller could not build a retry policy from + * it: a rejected call and a completed mutation read identically. A tool that declares + * its {@link ToolCallEffect} gets the phrasing its phase actually warrants. + */ +const WITHHELD_ERROR_BY_EFFECT_PHASE: Record = { + [TOOL_EFFECT_PHASE.notAttempted]: + 'Tool call was rejected before it ran, so nothing was created or changed. The reason could not be returned safely — correct the call and try again.', + [TOOL_EFFECT_PHASE.attempted]: + 'Tool execution was dispatched but its outcome could not be returned safely. At most one run exists for the ids in this result — resolve it before retrying a mutation.', + [TOOL_EFFECT_PHASE.performed]: + 'Tool execution completed but its result could not be returned safely. Do not retry — read the outcome using the ids in this result.', +} + const READ_ONLY_RESULT_TOOLS = new Set(['read', 'glob', 'grep']) +/** + * The shape of an identifier this system mints — `generateId`'s UUID, and the + * database ids that share it. Effect ids bypass secret projection, so the set of + * values that may occupy one is pinned to a syntax no credential we issue or store + * takes. A caller with a differently shaped id has to widen this deliberately, + * where the exemption is reviewed, rather than by passing it. + */ +const SERVER_MINTED_ID_PATTERN = + /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i + /** Chooses the withheld-result message a tool's caller should surface. */ export function toolResultUnavailableError(toolId?: string): string { return toolId && READ_ONLY_RESULT_TOOLS.has(toolId) @@ -22,24 +53,95 @@ export function toolResultUnavailableError(toolId?: string): string { : TOOL_RESULT_UNAVAILABLE_ERROR } +/** + * Why complete content could not cross, for the caller that is about to log a refusal. + * + * The three causes need different fixes — a latched registry names the guard that tripped, + * an absent one means the surface never built a catalog, and a content refusal means the + * registry was fine and the payload itself was unprojectable — so they are not collapsed. + */ +export type ToolResultWithholdingCause = + | { + kind: 'registry-incomplete' + reasons: readonly ResolvedSecretIncompletenessReason[] + origins: readonly string[] + } + | { kind: 'registry-absent' } + | { kind: 'content-refused' } + +export type CopilotToolResultProjection = + | { safe: true; result: ToolExecutionResult } + | { safe: false; result: ToolExecutionResult; cause: ToolResultWithholdingCause } + function structuralResult(result: ToolExecutionResult): ToolExecutionResult { return { success: result.success === true } } +/** + * Reduces a withheld result to the facts the tool asserted about the call itself. + * + * Content is dropped because nothing here can prove it secret-free. The effect + * disclosure survives because it is not derived from content: the phase is a + * code-defined literal and every id is checked against {@link SERVER_MINTED_ID_PATTERN}. + * An id that fails that check voids the whole disclosure rather than being dropped + * on its own — a partially honoured exemption is the one shape a reader would + * misread as complete. + */ function omittedResult(result: ToolExecutionResult, toolId?: string): ToolExecutionResult { - if (result.success) return { success: true } - return { success: false, error: toolResultUnavailableError(toolId) } + const effect = vouchableEffect(result.effect) + if (!effect) { + return result.success + ? { success: true } + : { success: false, error: toolResultUnavailableError(toolId) } + } + + return { + success: result.success === true, + output: { resultWithheld: true, effect: effect.phase, ...effect.ids }, + ...(result.success ? {} : { error: WITHHELD_ERROR_BY_EFFECT_PHASE[effect.phase] }), + } } -export type CopilotToolResultProjection = - | { safe: true; result: ToolExecutionResult } - | { safe: false; result: ToolExecutionResult } +/** Returns the disclosure only when every id it carries is a shape this system mints. */ +function vouchableEffect(effect: ToolCallEffect | undefined): ToolCallEffect | undefined { + if (!effect) return undefined + for (const value of Object.values(effect.ids ?? {})) { + if (typeof value !== 'string' || !SERVER_MINTED_ID_PATTERN.test(value)) return undefined + } + return effect +} + +function withholdingCause( + registry: ResolvedSecretTraceRegistry | undefined +): ToolResultWithholdingCause { + if (!registry) return { kind: 'registry-absent' } + const diagnostics = registry.getIncompletenessDiagnostics() + return diagnostics + ? { + kind: 'registry-incomplete', + reasons: diagnostics.reasons, + origins: diagnostics.origins, + } + : { kind: 'content-refused' } +} + +function withheld( + result: ToolExecutionResult, + registry: ResolvedSecretTraceRegistry | undefined, + toolId: string | undefined +): CopilotToolResultProjection { + return { + safe: false, + result: omittedResult(result, toolId), + cause: withholdingCause(registry), + } +} /** * Projects terminal tool content and reports whether the complete content was safe to cross. * Callers that isolate provenance per tool call may merge that child registry only when `safe` * is true and the child is complete. The returned result is always safe to expose: an unsafe - * projection is reduced to a structural success or failure. + * projection is reduced to a structural success or failure, plus any effect the tool disclosed. */ export function inspectToolResultForCopilot( result: ToolExecutionResult, @@ -54,7 +156,7 @@ export function inspectToolResultForCopilot( if (Object.hasOwn(result, 'error')) content.error = result.error const projection = projectResolvedSecretModelJsonContent(content, resultRegistry) if (!projection.safe || !projection.value || typeof projection.value !== 'object') { - return { safe: false, result: omittedResult(result, toolId) } + return withheld(result, resultRegistry, toolId) } const projectedContent = projection.value as Record @@ -62,7 +164,7 @@ export function inspectToolResultForCopilot( if (Object.hasOwn(projectedContent, 'output')) projected.output = projectedContent.output if (Object.hasOwn(projectedContent, 'error')) { if (typeof projectedContent.error !== 'string') { - return { safe: false, result: omittedResult(result, toolId) } + return withheld(result, resultRegistry, toolId) } projected.error = projectedContent.error } @@ -74,7 +176,7 @@ export function inspectToolResultForCopilot( } return { safe: true, result: projected } } catch { - return { safe: false, result: omittedResult(result, toolId) } + return withheld(result, registry, toolId) } } @@ -98,3 +200,16 @@ export function projectToolErrorMessageForCopilot( ): string { return projectToolResultForCopilot({ success: false, error }, registry, toolId).error ?? '' } + +/** Flattens a withholding cause into log/span fields, so every surface reports it alike. */ +export function describeWithholdingCause( + cause: ToolResultWithholdingCause +): Record { + return cause.kind === 'registry-incomplete' + ? { + withheldCause: cause.kind, + withheldReasons: [...cause.reasons], + ...(cause.origins.length > 0 ? { withheldOrigins: [...cause.origins] } : {}), + } + : { withheldCause: cause.kind } +} diff --git a/apps/sim/lib/copilot/tool-executor/types.ts b/apps/sim/lib/copilot/tool-executor/types.ts index d774ca0999e..121e5a4f84e 100644 --- a/apps/sim/lib/copilot/tool-executor/types.ts +++ b/apps/sim/lib/copilot/tool-executor/types.ts @@ -45,11 +45,45 @@ export interface ToolExecutionContext { resolvedSecretTraceRegistry?: ResolvedSecretTraceRegistry } +/** + * How far a tool call got in performing its side effect. + * + * This is a property of the call, not of the content it produced, which is why it + * can still be reported when the content itself cannot cross the model boundary. + * It is the only thing that lets a caller decide about retry: a rejected call and a + * completed mutation are otherwise indistinguishable once their payloads are withheld. + */ +export const TOOL_EFFECT_PHASE = { + /** Rejected before anything could happen. Correcting the call and retrying is safe. */ + notAttempted: 'not_attempted', + /** Dispatched; zero or one effects may exist. Resolve by id before retrying. */ + attempted: 'attempted', + /** The effect ran to completion, whatever its outcome. Never retry blind. */ + performed: 'performed', +} as const +export type ToolEffectPhase = (typeof TOOL_EFFECT_PHASE)[keyof typeof TOOL_EFFECT_PHASE] + +export interface ToolCallEffect { + phase: ToolEffectPhase + /** + * Server-minted identifiers naming the effect, so an unreadable result stays + * resolvable. Values must be identifiers this system issues; the egress + * projection rejects the whole disclosure otherwise. + */ + ids?: Readonly> +} + export interface ToolExecutionResult { success: boolean output?: unknown error?: string resources?: MothershipResource[] + /** + * Declared by tools whose failure a caller cannot otherwise act on. Consumed by + * the egress projection and never returned to the model as-is — on a withheld + * result it becomes the disclosure record that replaces the dropped content. + */ + effect?: ToolCallEffect } export type ToolHandler = ( diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts index 37f6860601b..67d3ec5ed0e 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts @@ -9,6 +9,7 @@ const { mocks } = vi.hoisted(() => ({ apiKey: vi.fn(), executeWorkflowUseCase: vi.fn(), hasExecutionResult: vi.fn(), + readAttemptedExecutionId: vi.fn(), }, })) @@ -28,6 +29,7 @@ vi.mock('@/lib/workflows/sanitization/json-sanitizer', () => ({ vi.mock('@/executor/utils/errors', () => ({ hasExecutionResult: mocks.hasExecutionResult, + readAttemptedExecutionId: mocks.readAttemptedExecutionId, })) vi.mock('@/lib/core/telemetry', () => ({ @@ -57,6 +59,7 @@ describe('workflow mutation Copilot adapters', () => { beforeEach(() => { vi.clearAllMocks() mocks.hasExecutionResult.mockReturnValue(false) + mocks.readAttemptedExecutionId.mockReturnValue(undefined) }) it('maps encoded folder aliases into one create application command', async () => { @@ -259,6 +262,53 @@ describe('workflow mutation Copilot adapters', () => { const result = await executeRunWorkflow({ workflowId: 'workflow-1' }, context) - expect(result).toEqual({ success: false, error: 'Workflow execution failed' }) + expect(result).toEqual({ + success: false, + error: 'Workflow execution failed', + effect: { phase: 'not_attempted' }, + }) + }) + + /** + * How far the run got is the only thing a caller can act on once the egress boundary + * withholds the payload, so each of these must reach the projection distinguishable. + */ + it.each([ + { + label: 'refused on its own arguments', + arrange: () => {}, + run: () => executeRunWorkflow({}, { ...context, workflowId: undefined }), + effect: { phase: 'not_attempted' }, + }, + { + label: 'failed before dispatch', + arrange: () => mocks.executeWorkflowUseCase.mockRejectedValueOnce(new Error('denied')), + run: () => executeRunWorkflow({ workflowId: 'workflow-1' }, context), + effect: { phase: 'not_attempted' }, + }, + { + label: 'failed after dispatch', + arrange: () => { + mocks.executeWorkflowUseCase.mockRejectedValueOnce(new Error('crashed')) + mocks.readAttemptedExecutionId.mockReturnValue('execution-1') + }, + run: () => executeRunWorkflow({ workflowId: 'workflow-1' }, context), + effect: { phase: 'attempted', ids: { executionId: 'execution-1' } }, + }, + { + label: 'completed', + arrange: () => + mocks.executeWorkflowUseCase.mockResolvedValueOnce({ + success: true, + output: {}, + logs: [], + metadata: { executionId: 'execution-1' }, + }), + run: () => executeRunWorkflow({ workflowId: 'workflow-1' }, context), + effect: { phase: 'performed', ids: { executionId: 'execution-1' } }, + }, + ])('states that a run $label', async ({ arrange, run, effect }) => { + arrange() + expect((await run()).effect).toEqual(effect) }) }) diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts index 164574a84cb..897d2ef6fee 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts @@ -7,6 +7,11 @@ import { messageForCopilotWorkflowError, } from '@/lib/copilot/application/execute-workflow-use-case' import type { ExecutionContext, ToolCallResult } from '@/lib/copilot/request/types' +import { + TOOL_EFFECT_PHASE, + type ToolCallEffect, + type ToolEffectPhase, +} from '@/lib/copilot/tool-executor/types' import { requireCopilotWorkspace } from '@/lib/copilot/tools/server/workspace-scope' import { decodeVfsPathSegments, encodeVfsPathSegments } from '@/lib/copilot/vfs/path-utils' import { PlatformEvents } from '@/lib/core/telemetry' @@ -24,7 +29,7 @@ import { setWorkflowBlockEnabled, } from '@/lib/workflows/application/update-workflow-content' import { sanitizeForCopilot } from '@/lib/workflows/sanitization/json-sanitizer' -import { hasExecutionResult } from '@/executor/utils/errors' +import { hasExecutionResult, readAttemptedExecutionId } from '@/executor/utils/errors' import type { WorkflowState } from '@/stores/workflows/workflow/types' function stripBinaryFields(value: unknown): unknown { @@ -39,6 +44,22 @@ function stripBinaryFields(value: unknown): unknown { return out } +/** + * States how far a run got, so the answer survives a result the egress boundary withholds. + * + * Without it a withheld run reduces to a bare success or an opaque failure and takes the + * execution id with it, which is what left a caller unable to tell a rejected call from a + * completed run — and with nothing to look either one up by. + */ +function executionEffect(phase: ToolEffectPhase, executionId?: string): ToolCallEffect { + return { phase, ...(executionId ? { ids: { executionId } } : {}) } +} + +/** A run refused on its own arguments, before anything could be created. */ +function runRejected(error: string): ToolCallResult { + return { success: false, error, effect: executionEffect(TOOL_EFFECT_PHASE.notAttempted) } +} + function buildExecutionOutput( result: { success: boolean @@ -59,6 +80,7 @@ function buildExecutionOutput( logs: stripBinaryFields(result.logs), }, error: result.success ? undefined : result.error || 'Workflow execution failed', + effect: executionEffect(TOOL_EFFECT_PHASE.performed, result.metadata?.executionId), } } @@ -71,9 +93,18 @@ function buildExecutionError(error: unknown): ToolCallResult { }) } logger.error('Copilot workflow execution command failed', { error }) + /** + * Only failures raised after dispatch carry the id, so its absence is the positive + * statement that nothing was created rather than an admission of not knowing. + */ + const attemptedExecutionId = readAttemptedExecutionId(error) return { success: false, error: messageForCopilotWorkflowError(error, 'Workflow execution failed'), + effect: executionEffect( + attemptedExecutionId ? TOOL_EFFECT_PHASE.attempted : TOOL_EFFECT_PHASE.notAttempted, + attemptedExecutionId + ), } } @@ -204,7 +235,7 @@ export async function executeRunWorkflow( try { const workflowId = params.workflowId || context.workflowId if (!workflowId) { - return { success: false, error: 'workflowId is required' } + return runRejected('workflowId is required') } const useDraftState = !params.useDeployedState @@ -322,10 +353,10 @@ export async function executeRunWorkflowUntilBlock( try { const workflowId = params.workflowId || context.workflowId if (!workflowId) { - return { success: false, error: 'workflowId is required' } + return runRejected('workflowId is required') } if (!params.stopAfterBlockId) { - return { success: false, error: 'stopAfterBlockId is required' } + return runRejected('stopAfterBlockId is required') } const useDraftState = !params.useDeployedState @@ -401,10 +432,10 @@ export async function executeRunFromBlock( try { const workflowId = params.workflowId || context.workflowId if (!workflowId) { - return { success: false, error: 'workflowId is required' } + return runRejected('workflowId is required') } if (!params.startBlockId) { - return { success: false, error: 'startBlockId is required' } + return runRejected('startBlockId is required') } const useDraftState = !params.useDeployedState @@ -487,10 +518,10 @@ export async function executeRunBlock( try { const workflowId = params.workflowId || context.workflowId if (!workflowId) { - return { success: false, error: 'workflowId is required' } + return runRejected('workflowId is required') } if (!params.blockId) { - return { success: false, error: 'blockId is required' } + return runRejected('blockId is required') } const useDraftState = !params.useDeployedState diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts b/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts new file mode 100644 index 00000000000..9b14cc781e7 --- /dev/null +++ b/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts @@ -0,0 +1,173 @@ +/** + * @vitest-environment node + * + * Pins what a caller can learn about a workflow run whose result the secret-egress + * boundary withholds. + * + * The registry here is latched the way production latches it — a child run that handed + * back no provenance envelope — rather than by asserting an "unsafe" flag, so the test + * fails for the same reason the incident did. Three outcomes that need opposite retry + * decisions are driven through the real handler and the real projection: a call rejected + * on its arguments, a call that threw after dispatch, and a run that completed. All three + * used to arrive as the same sentence. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { inspectToolResultForCopilot } from '@/lib/copilot/request/tools/resolved-secret-result' +import type { ExecutionContext } from '@/lib/copilot/request/types' +import type { ToolExecutionResult } from '@/lib/copilot/tool-executor/types' +import { attachAttemptedExecutionId } from '@/executor/utils/errors' +import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' + +const { mocks } = vi.hoisted(() => ({ + mocks: { executeWorkflowUseCase: vi.fn() }, +})) + +vi.mock('@/lib/copilot/application/execute-workflow-use-case', () => ({ + executeCopilotWorkflowUseCase: mocks.executeWorkflowUseCase, + /** Passthrough, so a masked message is visible as masking rather than as a fallback. */ + messageForCopilotWorkflowError: (error: unknown, fallback = 'Workflow operation failed') => + error instanceof Error ? error.message : fallback, +})) + +vi.mock('@/lib/workflows/sanitization/json-sanitizer', () => ({ + sanitizeForCopilot: vi.fn((state) => state), +})) + +vi.mock('@/lib/core/telemetry', () => ({ + PlatformEvents: { apiKeyGenerated: vi.fn() }, +})) + +import { executeRunWorkflow } from '@/lib/copilot/tools/handlers/workflow/mutations' + +const EXECUTION_ID = '0f4d5a4c-6a1e-4c2f-9b7d-2c8f1a3e5d90' +const SECRET = 'sk-live-9Qv2XbTn4LmZa8Rd' + +const context = { + userId: 'user-1', + workspaceId: 'workspace-1', + toolCallId: 'tool-call-1', +} as ExecutionContext + +/** A registry latched exactly as `importCrossingProvenance` latches one in production. */ +async function latchedRegistry(): Promise { + const registry = new ResolvedSecretTraceRegistry([ + { name: 'API_KEY', plaintext: SECRET, encryptedValue: 'ciphertext' }, + ]) + registry.recordResolved('API_KEY', SECRET, { propagated: true }) + await registry.importCrossingProvenance( + undefined, + { output: {} }, + { trusted: true, origin: 'copilotWorkflowMutation.runCrossing' } + ) + expect(registry.isPermanentlyIncomplete()).toBe(true) + return registry +} + +async function withheld(result: ToolExecutionResult) { + const registry = await latchedRegistry() + const projection = inspectToolResultForCopilot(result, registry, 'run_workflow') + expect(projection.safe).toBe(false) + return projection +} + +function modelOutput(result: ToolExecutionResult): Record { + expect(result.output).toBeTypeOf('object') + return result.output as Record +} + +describe('a withheld run_workflow result', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('says nothing was created when the call never reached dispatch', async () => { + const rejected = await executeRunWorkflow({}, { ...context, workflowId: undefined }) + expect(mocks.executeWorkflowUseCase).not.toHaveBeenCalled() + + const { result } = await withheld(rejected) + + expect(result.success).toBe(false) + expect(modelOutput(result)).toEqual({ resultWithheld: true, effect: 'not_attempted' }) + expect(result.error).toContain('nothing was created') + }) + + it('names the run to resolve when the call threw after dispatch', async () => { + const error = new Error(`Execution crashed reading ${SECRET}`) + attachAttemptedExecutionId(error, EXECUTION_ID) + mocks.executeWorkflowUseCase.mockRejectedValue(error) + + const failed = await executeRunWorkflow({ workflowId: 'wf-1' }, context) + const { result } = await withheld(failed) + + expect(result.success).toBe(false) + expect(modelOutput(result)).toEqual({ + resultWithheld: true, + effect: 'attempted', + executionId: EXECUTION_ID, + }) + expect(result.error).toContain('At most one run exists') + }) + + it('names the run to read when it completed', async () => { + mocks.executeWorkflowUseCase.mockResolvedValue({ + success: true, + output: { report: `PASS ${SECRET}` }, + logs: [{ blockName: 'report', output: SECRET }], + metadata: { executionId: EXECUTION_ID, duration: 2800 }, + }) + + const completed = await executeRunWorkflow({ workflowId: 'wf-1' }, context) + const { result } = await withheld(completed) + + expect(result.success).toBe(true) + expect(modelOutput(result)).toEqual({ + resultWithheld: true, + effect: 'performed', + executionId: EXECUTION_ID, + }) + }) + + /** The defect itself: these three needed opposite retry decisions and read identically. */ + it('distinguishes the three outcomes from one another', async () => { + const rejected = await executeRunWorkflow({}, { ...context, workflowId: undefined }) + + const thrown = new Error('boom') + attachAttemptedExecutionId(thrown, EXECUTION_ID) + mocks.executeWorkflowUseCase.mockRejectedValue(thrown) + const failed = await executeRunWorkflow({ workflowId: 'wf-1' }, context) + + mocks.executeWorkflowUseCase.mockReset() + mocks.executeWorkflowUseCase.mockResolvedValue({ + success: true, + output: {}, + metadata: { executionId: EXECUTION_ID, duration: 1 }, + }) + const completed = await executeRunWorkflow({ workflowId: 'wf-1' }, context) + + const views = await Promise.all( + [rejected, failed, completed].map(async (r) => JSON.stringify((await withheld(r)).result)) + ) + + expect(new Set(views).size).toBe(3) + expect(views[0]).not.toContain(EXECUTION_ID) + expect(views[1]).toContain(EXECUTION_ID) + expect(views[2]).toContain(EXECUTION_ID) + }) + + it('never lets the withheld payload carry the run content past the boundary', async () => { + mocks.executeWorkflowUseCase.mockResolvedValue({ + success: false, + output: { report: `FAIL ${SECRET}` }, + logs: [{ output: SECRET }], + error: `Block failed with ${SECRET}`, + metadata: { executionId: EXECUTION_ID, duration: 10 }, + }) + + const { result } = await withheld(await executeRunWorkflow({ workflowId: 'wf-1' }, context)) + + const serialized = JSON.stringify(result) + expect(serialized).not.toContain(SECRET) + expect(serialized).not.toContain('FAIL') + expect(serialized).not.toContain('Block failed') + }) +}) 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 90d3bf6a3bf..f03486b0843 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 @@ -68,6 +68,7 @@ import { runFromBlockFromCopilot, runWorkflowFromCopilot, } from '@/lib/workflows/application/run-workflow-from-copilot' +import { readAttemptedExecutionId } from '@/executor/utils/errors' const principal = { kind: 'delegated' as const, @@ -305,4 +306,51 @@ describe('Copilot workflow run application commands', () => { }) ).rejects.toThrow('database unavailable') }) + + /** + * A caller whose result was withheld can only decide about retry from whether a run + * exists. Naming it from dispatch onward, and only from there, is what makes the id's + * absence the positive statement that nothing was created. + */ + describe('naming the run a failure belongs to', () => { + const runInput = { + workflowId: 'workflow-1', + useDraftState: true, + lifecycle, + hasWorkflowInput: false, + useMockPayload: true, + } + + it('names the dispatched run when execution itself fails', async () => { + mocks.executeWorkflow.mockRejectedValueOnce(new Error('database unavailable')) + + const error = await runWorkflowFromCopilot + .execute({ principal, input: runInput }) + .catch((thrown) => thrown) + + expect(readAttemptedExecutionId(error)).toBe('child-execution-1') + }) + + it('names nothing when admission refused the run before it could start', async () => { + mocks.admission.mockRejectedValueOnce(new Error('Usage limit exceeded')) + + const error = await runWorkflowFromCopilot + .execute({ principal, input: runInput }) + .catch((thrown) => thrown) + + expect(mocks.executeWorkflow).not.toHaveBeenCalled() + expect(readAttemptedExecutionId(error)).toBeUndefined() + }) + + it('names nothing when authorization refused the run', async () => { + mocks.permission.mockResolvedValue('read') + + const error = await runWorkflowFromCopilot + .execute({ principal, input: runInput }) + .catch((thrown) => thrown) + + expect(mocks.admission).not.toHaveBeenCalled() + expect(readAttemptedExecutionId(error)).toBeUndefined() + }) + }) }) 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 32a4066041e..889444bbe08 100644 --- a/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts +++ b/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts @@ -28,6 +28,7 @@ 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 type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' export interface CopilotWorkflowRunLifecycle { @@ -295,6 +296,12 @@ async function executeCopilotRun(params: { } return result } catch (error) { + /** + * Everything above this `try` — authorization, admission, provenance export — fails + * before a run can exist, so only failures from here carry the id. That asymmetry is + * what lets a caller read its absence as "nothing was created" instead of guessing. + */ + attachAttemptedExecutionId(error, childExecutionId) if (registry) { const executionResult = typeof error === 'object' && From 04f27ad2b3fe03519d668318b2afeb43fba1341e Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 15:31:35 -0700 Subject: [PATCH 02/16] fix(copilot): keep the withheld-result tests out of the secret scanners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The withheld-run fixture was shaped like a live provider key, which is exactly what a secret scanner is built to catch — it flagged the test file itself. The value only has to clear the eight-character substitution floor, so it says what it is instead. The id-shape guard likewise no longer needs a credential-looking string to prove it refuses one. Also routes the test's error-message mock through getErrorMessage rather than reimplementing it inline, which check:utils bans. Co-Authored-By: Claude Opus 5 (1M context) --- .../copilot/request/tools/resolved-secret-result.test.ts | 2 +- .../tools/handlers/workflow/withheld-run-result.test.ts | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts b/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts index decf9fe39d1..48d6ccfe1f5 100644 --- a/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts +++ b/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts @@ -484,7 +484,7 @@ describe('effect disclosure on a withheld result', () => { { success: false, error: 'why', - effect: { phase: 'performed', ids: { executionId: 'sk-live-9Qv2XbTn4LmZa8Rd' } }, + effect: { phase: 'performed', ids: { executionId: 'not-a-server-minted-id' } }, }, undefined, 'run_workflow' diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts b/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts index 9b14cc781e7..dbf32e59536 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts @@ -11,6 +11,7 @@ * on its arguments, a call that threw after dispatch, and a run that completed. All three * used to arrive as the same sentence. */ +import { getErrorMessage } from '@sim/utils/errors' import { beforeEach, describe, expect, it, vi } from 'vitest' import { inspectToolResultForCopilot } from '@/lib/copilot/request/tools/resolved-secret-result' import type { ExecutionContext } from '@/lib/copilot/request/types' @@ -26,7 +27,7 @@ vi.mock('@/lib/copilot/application/execute-workflow-use-case', () => ({ executeCopilotWorkflowUseCase: mocks.executeWorkflowUseCase, /** Passthrough, so a masked message is visible as masking rather than as a fallback. */ messageForCopilotWorkflowError: (error: unknown, fallback = 'Workflow operation failed') => - error instanceof Error ? error.message : fallback, + getErrorMessage(error, fallback), })) vi.mock('@/lib/workflows/sanitization/json-sanitizer', () => ({ @@ -40,7 +41,9 @@ vi.mock('@/lib/core/telemetry', () => ({ import { executeRunWorkflow } from '@/lib/copilot/tools/handlers/workflow/mutations' const EXECUTION_ID = '0f4d5a4c-6a1e-4c2f-9b7d-2c8f1a3e5d90' -const SECRET = 'sk-live-9Qv2XbTn4LmZa8Rd' +/** Above the 8-char substitution floor, and deliberately not shaped like any real + * provider credential — a realistic-looking fixture makes secret scanners flag this file. */ +const SECRET = 'fake-secret-for-test-only' const context = { userId: 'user-1', From d420e4ef5f414b572c4e67fcca177a77876e9065 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 15:45:50 -0700 Subject: [PATCH 03/16] fix(copilot): name the dispatched run from the boundary that owns it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the attempted-run id attached around the whole of executeWorkflow, which validates workspace and billing attribution before it can create anything. A preflight refusal therefore reported a run that never existed, telling a caller to resolve an id with nothing behind it and to skip a retry that was safe — the mirror of the defect this branch fixes. Move the attachment inside executeWorkflow, at the point it enters the execution core, which is the first moment a row may exist. Everything above it now correctly carries nothing, and the copilot layer keeps only the window executeWorkflow cannot see: a failure after the run already returned, where the crossing import threw and an execution certainly exists. Also from review: - Attach to any thrown object rather than only an Error, and normalize a thrown primitive past the dispatch boundary. Restricting to Error made the invariant silently invert for a thrown plain object — the id would not attach, its absence would read as "nothing started", and the caller would duplicate a real run. - Void the disclosure when an id would take one of the record's own field names. A valid uuid under `effect` overwrote the phase the retry decision reads, on the same all-or-nothing terms as an unvouchable id. - Drop `effect` from the provider model response. That path spreads every non-output field through verbatim, so the type's claim that the disclosure reaches the model only through the withheld-result projection was true by accident rather than by construction. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/executor/utils/errors.ts | 21 +++++-- .../request/tools/resolved-secret-result.ts | 13 ++++- .../run-workflow-from-copilot.test.ts | 57 ++++++++++++++----- .../application/run-workflow-from-copilot.ts | 11 ++-- .../workflows/executor/execute-workflow.ts | 19 ++++++- apps/sim/providers/runtime-context.ts | 12 +++- 6 files changed, 107 insertions(+), 26 deletions(-) diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index 48927f749fa..892579ed8b6 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -51,18 +51,31 @@ const ATTEMPTED_EXECUTION_ID = 'attemptedExecutionId' * result, this says only that it was dispatched. */ export function attachAttemptedExecutionId(error: unknown, executionId: string): void { - if (!(error instanceof Error) || !executionId) return + if (!isAttachableThrown(error) || !executionId) return if (ATTEMPTED_EXECUTION_ID in error) return Object.assign(error, { [ATTEMPTED_EXECUTION_ID]: executionId }) } -/** Reads the dispatched-run id an error carries, if dispatch was reached at all. */ +/** Reads the dispatched-run id a thrown value carries, if dispatch was reached at all. */ export function readAttemptedExecutionId(error: unknown): string | undefined { - if (!(error instanceof Error) || !(ATTEMPTED_EXECUTION_ID in error)) return undefined - const value = (error as Error & Record)[ATTEMPTED_EXECUTION_ID] + if (!isAttachableThrown(error) || !(ATTEMPTED_EXECUTION_ID in error)) return undefined + const value = (error as Record)[ATTEMPTED_EXECUTION_ID] return typeof value === 'string' && value.length > 0 ? value : undefined } +/** + * Any non-null object, not only an `Error`. + * + * Restricting this to `Error` would silently invert the invariant for a thrown plain object: + * the id would not attach, the absence would then read as "nothing was started", and the + * caller would retry a run that already exists — the exact duplicate-side-effect outcome + * this id exists to prevent. A thrown primitive cannot carry a property at all, so callers + * that must not lose the id normalize before attaching. + */ +function isAttachableThrown(value: unknown): value is Record { + return typeof value === 'object' && value !== null +} + export interface BlockExecutionErrorDetails { block: SerializedBlock error: Error | string diff --git a/apps/sim/lib/copilot/request/tools/resolved-secret-result.ts b/apps/sim/lib/copilot/request/tools/resolved-secret-result.ts index 6c716c0e0cc..65b704a06d0 100644 --- a/apps/sim/lib/copilot/request/tools/resolved-secret-result.ts +++ b/apps/sim/lib/copilot/request/tools/resolved-secret-result.ts @@ -46,6 +46,9 @@ const READ_ONLY_RESULT_TOOLS = new Set(['read', 'glob', 'grep']) const SERVER_MINTED_ID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[1-8][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i +/** Field names the disclosure record owns; an id may not take one. */ +const RESERVED_DISCLOSURE_KEYS = new Set(['resultWithheld', 'effect']) + /** Chooses the withheld-result message a tool's caller should surface. */ export function toolResultUnavailableError(toolId?: string): string { return toolId && READ_ONLY_RESULT_TOOLS.has(toolId) @@ -102,10 +105,16 @@ function omittedResult(result: ToolExecutionResult, toolId?: string): ToolExecut } } -/** Returns the disclosure only when every id it carries is a shape this system mints. */ +/** + * Returns the disclosure only when every id it carries is a shape this system mints and none + * of them would displace the record's own fields. An id named `effect` overwriting the phase + * would corrupt exactly the field the retry decision reads, so a collision voids the + * disclosure on the same all-or-nothing terms as an unvouchable id. + */ function vouchableEffect(effect: ToolCallEffect | undefined): ToolCallEffect | undefined { if (!effect) return undefined - for (const value of Object.values(effect.ids ?? {})) { + for (const [key, value] of Object.entries(effect.ids ?? {})) { + if (RESERVED_DISCLOSURE_KEYS.has(key)) return undefined if (typeof value !== 'string' || !SERVER_MINTED_ID_PATTERN.test(value)) return undefined } return effect 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 f03486b0843..1ccc4fc8cb3 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 @@ -308,9 +308,9 @@ describe('Copilot workflow run application commands', () => { }) /** - * A caller whose result was withheld can only decide about retry from whether a run - * exists. Naming it from dispatch onward, and only from there, is what makes the id's - * absence the positive statement that nothing was created. + * A caller whose result was withheld can only decide about retry from whether a run exists. + * `executeWorkflow` owns that boundary and names the run itself once it crosses it; this + * layer only covers the window it cannot see — a failure after the run already returned. */ describe('naming the run a failure belongs to', () => { const runInput = { @@ -321,22 +321,53 @@ describe('Copilot workflow run application commands', () => { useMockPayload: true, } - it('names the dispatched run when execution itself fails', async () => { - mocks.executeWorkflow.mockRejectedValueOnce(new Error('database unavailable')) + const failWith = (input = runInput) => + runWorkflowFromCopilot.execute({ principal, input }).catch((thrown) => thrown) - const error = await runWorkflowFromCopilot - .execute({ principal, input: runInput }) - .catch((thrown) => thrown) + it('passes through the id executeWorkflow attached at its dispatch boundary', async () => { + mocks.executeWorkflow.mockImplementationOnce(() => { + // Exactly what executeWorkflow does once it enters the core. + const dispatchFailure = new Error('database unavailable') + Object.assign(dispatchFailure, { attemptedExecutionId: 'child-execution-1' }) + throw dispatchFailure + }) + + expect(readAttemptedExecutionId(await failWith())).toBe('child-execution-1') + }) + + it('names the run when the crossing threw after it already returned', async () => { + // Only the post-run crossing throws; the catch re-enters this same method to + // record the failed crossing, and throwing again there would replace the very + // error the id was attached to. + let crossings = 0 + const registry = { + exportProvenanceForValue: () => undefined, + beginPendingActivation: () => () => {}, + importCrossingProvenance: () => { + if (crossings++ === 0) throw new Error('crossing import failed') + }, + } + + const error = await failWith({ + ...runInput, + lifecycle: { resolvedSecretTraceRegistry: registry }, + } as typeof runInput) expect(readAttemptedExecutionId(error)).toBe('child-execution-1') }) + it('names nothing when a preflight failure never reached dispatch', async () => { + mocks.executeWorkflow.mockRejectedValueOnce( + new Error('Billing attribution is required for workspace execution') + ) + + expect(readAttemptedExecutionId(await failWith())).toBeUndefined() + }) + it('names nothing when admission refused the run before it could start', async () => { mocks.admission.mockRejectedValueOnce(new Error('Usage limit exceeded')) - const error = await runWorkflowFromCopilot - .execute({ principal, input: runInput }) - .catch((thrown) => thrown) + const error = await failWith() expect(mocks.executeWorkflow).not.toHaveBeenCalled() expect(readAttemptedExecutionId(error)).toBeUndefined() @@ -345,9 +376,7 @@ describe('Copilot workflow run application commands', () => { it('names nothing when authorization refused the run', async () => { mocks.permission.mockResolvedValue('read') - const error = await runWorkflowFromCopilot - .execute({ principal, input: runInput }) - .catch((thrown) => thrown) + const error = await failWith() expect(mocks.admission).not.toHaveBeenCalled() expect(readAttemptedExecutionId(error)).toBeUndefined() 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 889444bbe08..0742a8ceb09 100644 --- a/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts +++ b/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts @@ -246,6 +246,7 @@ async function executeCopilotRun(params: { params.executionInput ) const completePendingActivation = registry?.beginPendingActivation() + let runReturned = false try { const result = await executeWorkflow( { @@ -287,6 +288,7 @@ async function executeCopilotRun(params: { }, childExecutionId ) + runReturned = true if (registry) { await registry.importCrossingProvenance( result.executionState?.resolvedSecretTraceProvenance, @@ -297,11 +299,12 @@ async function executeCopilotRun(params: { return result } catch (error) { /** - * Everything above this `try` — authorization, admission, provenance export — fails - * before a run can exist, so only failures from here carry the id. That asymmetry is - * what lets a caller read its absence as "nothing was created" instead of guessing. + * `executeWorkflow` names the run itself once it crosses its own dispatch boundary, so + * preflight failures inside it correctly carry nothing. This covers only the window it + * cannot see: a failure after the run already returned, where the crossing import is + * what threw and an execution certainly exists. */ - attachAttemptedExecutionId(error, childExecutionId) + if (runReturned) attachAttemptedExecutionId(error, childExecutionId) if (registry) { const executionResult = typeof error === 'object' && diff --git a/apps/sim/lib/workflows/executor/execute-workflow.ts b/apps/sim/lib/workflows/executor/execute-workflow.ts index 8336ea332d2..13b024a01ce 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 { attachAttemptedExecutionId } 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,7 @@ export async function executeWorkflow( loggingSession.setTrustedExecutionCorrelation(streamConfig.trustedExecutionCorrelation) } let postExecutionOwnershipTransferred = false + let dispatched = false try { const metadata: ExecutionMetadata = { @@ -169,6 +172,13 @@ export async function executeWorkflow( const executionStartMs = Date.now() + /** + * Entering the core is the point past which an execution row may exist. Everything above + * it — workspace and billing preflight, snapshot construction — fails without creating + * anything, so a caller can read the absence of this id as "nothing was started" rather + * than as an admission of not knowing. + */ + dispatched = true const result = await executeWorkflowCore({ snapshot, callbacks: { @@ -240,7 +250,14 @@ export async function executeWorkflow( } return result - } catch (error: unknown) { + } catch (thrown: unknown) { + /** + * A thrown primitive has nowhere to carry the dispatched-run id, and losing it would make + * a real run read as never started. Normalizing only past the dispatch boundary keeps the + * rethrown value identical on every other path. + */ + const error = dispatched && typeof thrown !== 'object' ? toError(thrown) : thrown + if (dispatched) attachAttemptedExecutionId(error, executionId) const errorDiagnostic = loggingSession.projectDiagnosticError(error) logger.error(`[${requestId}] Workflow execution failed`, errorDiagnostic) diff --git a/apps/sim/providers/runtime-context.ts b/apps/sim/providers/runtime-context.ts index 6d92ad924cb..2e602a83ba0 100644 --- a/apps/sim/providers/runtime-context.ts +++ b/apps/sim/providers/runtime-context.ts @@ -43,7 +43,17 @@ function toProviderModelResponse( rawResponse: ToolResponse, projectedResponse: ToolExecutionResult ): ToolResponse { - const { output: _output, error: _error, ...functionalFields } = rawResponse + /** + * `effect` is an input to the egress projection, not content — it reaches the model only as + * the disclosure record that replaces withheld output. This split spreads every other field + * through verbatim, so dropping it here is what keeps that true on the provider path too. + */ + const { + output: _output, + error: _error, + effect: _effect, + ...functionalFields + } = rawResponse as ToolResponse & { effect?: unknown } return { ...functionalFields, output: Object.hasOwn(projectedResponse, 'output') From dd676722f77786673a3c6c6d854189e7dec87b84 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 15:55:38 -0700 Subject: [PATCH 04/16] fix(copilot): refuse an in-band tool call whose egress catalog is unavailable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Production shows 88 of these in fourteen days, every one from this route and every one caused by a workspace id that no longer exists reaching the in-band lane. The handler ran anyway, which is the worst pair of outcomes available: the side effect happened, and because the projection can vouch for nothing without a catalog, the caller got a bare success or an opaque sentinel naming neither the cause nor whether anything had changed. It is also where the reported "cannot tell whether the mutation occurred" came from — of the tools affected, read and grep dominate, and the runs were bursts inside single sessions. Refuse before dispatch instead. Nothing runs, so there is nothing to be uncertain about, and the caller is told which workspace and why. A missing workspace also reported itself as an access denial, which sent every deleted-workspace call down a permissions path nobody could reproduce. `checkWorkspaceAccess` already distinguishes the two, so say which one it was. The refusal log now carries the user and workspace it refused; without them the only way to find the cause was to join by timestamp. Co-Authored-By: Claude Opus 5 (1M context) --- .../api/copilot/tools/execute/route.test.ts | 16 ++++++++-- .../app/api/copilot/tools/execute/route.ts | 31 ++++++++++++++++--- apps/sim/lib/environment/utils.ts | 9 ++++++ 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/apps/sim/app/api/copilot/tools/execute/route.test.ts b/apps/sim/app/api/copilot/tools/execute/route.test.ts index 87e7d1e88f2..afbcefdd2e6 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.test.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.test.ts @@ -95,12 +95,22 @@ describe('POST /api/copilot/tools/execute (in-band)', () => { expect(body.error).toBe('File not found: files/a.md') }) - it('withholds results when no egress registry can be built', async () => { - mockPrepareEnvironmentContext.mockRejectedValue(new Error('env unavailable')) + /** + * Running the tool without a catalog used to produce the worst pair of outcomes available: + * the side effect happened and the caller got a bare `{success: true}` naming neither the + * cause nor whether anything had changed. + */ + it('refuses the call, without running the tool, when no egress registry can be built', async () => { + mockPrepareEnvironmentContext.mockRejectedValue(new Error('Workspace ws-gone does not exist')) mockHandler.mockResolvedValue({ success: true, output: { content: 'sensitive' } }) + const res = await POST(makeRequest({ ...BASE_BODY, messageId: 'msg-no-registry' }) as never) const body = await res.json() - expect(body).toEqual({ success: true }) + + expect(mockHandler).not.toHaveBeenCalled() + expect(body.success).toBe(false) + expect(body.error).toContain('Workspace ws-gone does not exist') + expect(body.output).toEqual({ resultWithheld: true, effect: 'not_attempted' }) }) it('reuses one turn registry across calls that share a messageId', async () => { diff --git a/apps/sim/app/api/copilot/tools/execute/route.ts b/apps/sim/app/api/copilot/tools/execute/route.ts index dd49445de9c..3e4a7f299e9 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.ts @@ -4,6 +4,7 @@ import { type NextRequest, NextResponse } from 'next/server' import { copilotToolExecuteInternalBodySchema } from '@/lib/api/contracts/copilot' import { validationErrorResponse } from '@/lib/api/server' import { prepareCopilotEnvironmentContext } from '@/lib/copilot/environment-context' +import { MothershipStreamV1ToolOutcome } from '@/lib/copilot/generated/mothership-stream-v1' import { TraceAttr } from '@/lib/copilot/generated/trace-attributes-v1' import { TraceSpan } from '@/lib/copilot/generated/trace-spans-v1' import { checkInternalApiKey } from '@/lib/copilot/request/http' @@ -17,6 +18,7 @@ import { handleResourceSideEffects } from '@/lib/copilot/request/tools/resources import type { ToolCallResult } from '@/lib/copilot/request/types' import { ensureHandlersRegistered } from '@/lib/copilot/tool-executor' import { executeTool } from '@/lib/copilot/tool-executor/executor' +import { TOOL_EFFECT_PHASE } from '@/lib/copilot/tool-executor/types' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' @@ -116,16 +118,35 @@ export const POST = withRouteHandler((request: NextRequest) => [TraceAttr.UserId]: userId, }) - let toolRegistry: ResolvedSecretTraceRegistry | undefined - let turnRegistry: ResolvedSecretTraceRegistry | undefined + let toolRegistry: ResolvedSecretTraceRegistry + let turnRegistry: ResolvedSecretTraceRegistry try { turnRegistry = await getTurnEgressRegistry(userId, workspaceId, messageId) toolRegistry = turnRegistry.forkForInputPaths([]) } catch (err) { - logger.error('In-band egress registry unavailable; results will be withheld', { + /** + * Without a catalog the projection can vouch for nothing, so every result this call + * could produce would be withheld. Running the tool anyway was the worst of both + * outcomes: the side effect happened and the caller got an opaque sentinel that named + * neither the cause nor whether anything had changed. Refusing before dispatch is + * both truthful and the only answer that leaves nothing behind. + * + * The cause is almost always the workspace itself — a deleted or inaccessible id + * reaching this lane — which is actionable, so it is reported rather than swallowed. + */ + const reason = getErrorMessage(err) + logger.error('In-band egress registry unavailable; refusing the call', { toolName, toolCallId, - error: getErrorMessage(err), + userId, + workspaceId, + error: reason, + }) + rootSpan.setAttributes({ [TraceAttr.ToolOutcome]: MothershipStreamV1ToolOutcome.error }) + return NextResponse.json({ + success: false, + error: `${toolName} was not run: ${reason}`, + output: { resultWithheld: true, effect: TOOL_EFFECT_PHASE.notAttempted }, }) } @@ -149,7 +170,7 @@ export const POST = withRouteHandler((request: NextRequest) => }) const projection = inspectToolResultForCopilot(result, toolRegistry, toolName) const projected = projection.result - if (projection.safe && toolRegistry?.isComplete() && turnRegistry) { + if (projection.safe && toolRegistry.isComplete()) { turnRegistry.mergeToolCallRegistry(toolRegistry) } if (!projected.success) { diff --git a/apps/sim/lib/environment/utils.ts b/apps/sim/lib/environment/utils.ts index ea10b6d1546..065d610c0b0 100644 --- a/apps/sim/lib/environment/utils.ts +++ b/apps/sim/lib/environment/utils.ts @@ -151,6 +151,15 @@ export async function getPersonalAndWorkspaceEnv( let workspaceCanAdmin = false if (workspaceId) { const access = options?.workspaceAccess ?? (await checkWorkspaceAccess(workspaceId, userId)) + /** + * A workspace that no longer exists and one the caller may not read are different facts + * and take different corrections — stop using the id versus ask for access. Collapsing + * them sent every deleted-workspace call down the access-denied path, where it read as a + * permissions problem nobody could reproduce. + */ + if (!access.exists) { + throw new Error(`Workspace ${workspaceId} does not exist`) + } if (!access.hasAccess) { throw new Error(`Access denied to workspace ${workspaceId}`) } From f86e9b1b6ec4ac60a5a147513bae2a7c422d8e99 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 16:04:59 -0700 Subject: [PATCH 05/16] fix(copilot): put the dispatch boundary at the logging session, not the core MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review was right that entering the execution core is too early. The core loads custom blocks, workflow state, and the environment before `safeStart` writes a row, so a setup failure — which ran nothing and is safely retryable — still reported a dispatched run and sent the caller looking for it. Move the marker to `loggingStarted`, read before the catch's own recovery `safeStart` writes a row for the failure itself. That is the first point blocks may have executed, so it is the honest line, and it lets executeWorkflow go back to a plain rethrow. The thrown value stays exactly as received, including a non-Error one: the core's finalization guard identifies it, and three existing tests pin that. A thrown primitive therefore carries no id, which costs nothing today because every throw site past `safeStart` raises an Error — noted in the code rather than papered over. Also read the marker with `Object.hasOwn` rather than `in`, so an id reached through a prototype chain can never disclose an unrelated run, and cover the reserved-key branch of the disclosure guard. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/executor/utils/errors.ts | 8 ++++++-- .../tools/resolved-secret-result.test.ts | 17 +++++++++++++++++ .../run-workflow-from-copilot.test.ts | 2 +- .../workflows/executor/execute-workflow.ts | 19 +------------------ .../lib/workflows/executor/execution-core.ts | 14 +++++++++++++- 5 files changed, 38 insertions(+), 22 deletions(-) diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index 892579ed8b6..c653c22e65a 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -52,13 +52,17 @@ const ATTEMPTED_EXECUTION_ID = 'attemptedExecutionId' */ export function attachAttemptedExecutionId(error: unknown, executionId: string): void { if (!isAttachableThrown(error) || !executionId) return - if (ATTEMPTED_EXECUTION_ID in error) return + if (Object.hasOwn(error, ATTEMPTED_EXECUTION_ID)) return Object.assign(error, { [ATTEMPTED_EXECUTION_ID]: executionId }) } /** Reads the dispatched-run id a thrown value carries, if dispatch was reached at all. */ export function readAttemptedExecutionId(error: unknown): string | undefined { - if (!isAttachableThrown(error) || !(ATTEMPTED_EXECUTION_ID in error)) return undefined + /** + * Own property only. `in` would accept one reached through the prototype chain, which + * would let an unrelated run's id be disclosed for a refusal that started nothing. + */ + if (!isAttachableThrown(error) || !Object.hasOwn(error, ATTEMPTED_EXECUTION_ID)) return undefined const value = (error as Record)[ATTEMPTED_EXECUTION_ID] return typeof value === 'string' && value.length > 0 ? value : undefined } diff --git a/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts b/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts index 48d6ccfe1f5..7bd4eaa5109 100644 --- a/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts +++ b/apps/sim/lib/copilot/request/tools/resolved-secret-result.test.ts @@ -492,6 +492,23 @@ describe('effect disclosure on a withheld result', () => { ).toEqual({ success: false, error: TOOL_RESULT_UNAVAILABLE_ERROR }) }) + it.each(['effect', 'resultWithheld'])( + 'voids the disclosure when an id would take the reserved key %s', + (reserved) => { + expect( + projectToolResultForCopilot( + { + success: false, + error: 'why', + effect: { phase: 'performed', ids: { [reserved]: EXECUTION_ID } }, + }, + undefined, + 'run_workflow' + ) + ).toEqual({ success: false, error: TOOL_RESULT_UNAVAILABLE_ERROR }) + } + ) + it('reports the phase and ids when every id is vouchable', () => { expect( projectToolResultForCopilot( 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 1ccc4fc8cb3..aa254dda770 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 @@ -326,7 +326,7 @@ describe('Copilot workflow run application commands', () => { it('passes through the id executeWorkflow attached at its dispatch boundary', async () => { mocks.executeWorkflow.mockImplementationOnce(() => { - // Exactly what executeWorkflow does once it enters the core. + // Exactly what the execution core does once its logging session has started. const dispatchFailure = new Error('database unavailable') Object.assign(dispatchFailure, { attemptedExecutionId: 'child-execution-1' }) throw dispatchFailure diff --git a/apps/sim/lib/workflows/executor/execute-workflow.ts b/apps/sim/lib/workflows/executor/execute-workflow.ts index 13b024a01ce..8336ea332d2 100644 --- a/apps/sim/lib/workflows/executor/execute-workflow.ts +++ b/apps/sim/lib/workflows/executor/execute-workflow.ts @@ -1,6 +1,5 @@ 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, @@ -14,7 +13,6 @@ 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 { attachAttemptedExecutionId } from '@/executor/utils/errors' import type { ResolvedSecretTraceProvenanceV1 } from '@/executor/utils/resolved-secret-trace-registry' import type { CoreTriggerType } from '@/stores/logs/filters/types' @@ -130,7 +128,6 @@ export async function executeWorkflow( loggingSession.setTrustedExecutionCorrelation(streamConfig.trustedExecutionCorrelation) } let postExecutionOwnershipTransferred = false - let dispatched = false try { const metadata: ExecutionMetadata = { @@ -172,13 +169,6 @@ export async function executeWorkflow( const executionStartMs = Date.now() - /** - * Entering the core is the point past which an execution row may exist. Everything above - * it — workspace and billing preflight, snapshot construction — fails without creating - * anything, so a caller can read the absence of this id as "nothing was started" rather - * than as an admission of not knowing. - */ - dispatched = true const result = await executeWorkflowCore({ snapshot, callbacks: { @@ -250,14 +240,7 @@ export async function executeWorkflow( } return result - } catch (thrown: unknown) { - /** - * A thrown primitive has nowhere to carry the dispatched-run id, and losing it would make - * a real run read as never started. Normalizing only past the dispatch boundary keeps the - * rethrown value identical on every other path. - */ - const error = dispatched && typeof thrown !== 'object' ? toError(thrown) : thrown - if (dispatched) attachAttemptedExecutionId(error, executionId) + } catch (error: unknown) { const errorDiagnostic = loggingSession.projectDiagnosticError(error) logger.error(`[${requestId}] Workflow execution failed`, errorDiagnostic) diff --git a/apps/sim/lib/workflows/executor/execution-core.ts b/apps/sim/lib/workflows/executor/execution-core.ts index f5ec36774a7..9dd69e18b13 100644 --- a/apps/sim/lib/workflows/executor/execution-core.ts +++ b/apps/sim/lib/workflows/executor/execution-core.ts @@ -49,7 +49,7 @@ import type { SerializableExecutionState, } from '@/executor/execution/types' import type { ExecutionResult, StartBlockRunMetadata } from '@/executor/types' -import { hasExecutionResult } from '@/executor/utils/errors' +import { attachAttemptedExecutionId, hasExecutionResult } from '@/executor/utils/errors' import { projectResolvedSecretDiagnosticError } from '@/executor/utils/resolved-secret-content-projection' import { createResolvedSecretTraceRegistry, @@ -1097,6 +1097,18 @@ async function executeWorkflowCoreImpl( return result } catch (error: unknown) { + /** + * Whether the run had started when it failed, read before the recovery `safeStart` below + * writes a row for the failure itself. This — not entry into this function — is what says + * blocks may have executed: everything above it (custom-block loading, state loading, + * environment and secret setup) fails having run nothing, and a caller told otherwise + * would refuse a retry that was safe. + * + * The thrown value is rethrown exactly as received, including a non-Error one, because + * the finalization guard below identifies it. A primitive therefore carries no id, which + * costs nothing today: every throw site past `safeStart` raises an Error. + */ + if (loggingStarted) attachAttemptedExecutionId(error, executionId) const errorCause = describeErrorCause(error) logger.error( `[${requestId}] Execution failed:`, From 89abe98644e86d39043819f86a5a71a2196f29af Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 16:07:10 -0700 Subject: [PATCH 06/16] fix(copilot): log a withheld in-band result even when it withheld a success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cause was written only on the failure branch, but a withheld success keeps `projected.success` true — so the one case that leaves no other trace, where the model reads a bare success and nothing says why, was also the only one whose cause was never recorded. Report it on its own, as the resume driver already does. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/app/api/copilot/tools/execute/route.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/apps/sim/app/api/copilot/tools/execute/route.ts b/apps/sim/app/api/copilot/tools/execute/route.ts index 3e4a7f299e9..2f99e874ffb 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.ts @@ -173,6 +173,20 @@ export const POST = withRouteHandler((request: NextRequest) => if (projection.safe && toolRegistry.isComplete()) { turnRegistry.mergeToolCallRegistry(toolRegistry) } + if (!projection.safe) { + /** + * Reported on its own rather than folded into the failure branch below: a withheld + * SUCCESS keeps `projected.success` true, so gating on failure meant the one case + * that leaves no other trace — the model reads a bare success — was also the one + * case whose cause was never written down. + */ + logger.warn('In-band tool result withheld by egress projection', { + toolName, + toolCallId, + runtimeSucceeded: result.success, + ...describeWithholdingCause(projection.cause), + }) + } if (!projected.success) { logger.warn('In-band tool execution failed', { toolName, @@ -180,7 +194,6 @@ export const POST = withRouteHandler((request: NextRequest) => error: projected.error, runtimeSucceeded: result.success, projectionSafe: projection.safe, - ...(projection.safe ? {} : describeWithholdingCause(projection.cause)), }) } if (result.success && chatId) { From f3a47bfba4f6411d088356486339c64a7bf90fcf Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 16:18:33 -0700 Subject: [PATCH 07/16] fix(copilot): name the run from the executor, not the logging session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the logging session wrong in both directions, and it is: the result of `safeStart` is never checked, so blocks execute even when it fails — reporting that nothing started for a run that did, which is the direction that duplicates work — and it flips before trigger resolution and serialization, reporting a run for failures that never reached a block. A resume whose conditional update matches no row returns true and named a run that does not exist. Entering the executor is the only honest answer to "could a side effect have occurred", because side effects come from blocks rather than from log rows. Moving the marker there settles all three at once. Also from review: - Stop returning the thrown environment or database error to the model when the egress catalog is unavailable. Nothing there can project it — the catalog it would need is the very thing that is missing — so the reason stays in the log and the response carries fixed text plus the workspace id the caller itself supplied. - Guard the attach against a frozen failure, which would otherwise throw and replace the original error partway through cleanup, making a diagnostic aid the thing that loses the diagnosis. Co-Authored-By: Claude Opus 5 (1M context) --- .../api/copilot/tools/execute/route.test.ts | 6 ++++- .../app/api/copilot/tools/execute/route.ts | 13 +++++++--- apps/sim/executor/utils/errors.ts | 9 ++++++- .../lib/workflows/executor/execution-core.ts | 24 +++++++++++-------- 4 files changed, 37 insertions(+), 15 deletions(-) diff --git a/apps/sim/app/api/copilot/tools/execute/route.test.ts b/apps/sim/app/api/copilot/tools/execute/route.test.ts index afbcefdd2e6..716fbeb9ffd 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.test.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.test.ts @@ -109,8 +109,12 @@ describe('POST /api/copilot/tools/execute (in-band)', () => { expect(mockHandler).not.toHaveBeenCalled() expect(body.success).toBe(false) - expect(body.error).toContain('Workspace ws-gone does not exist') expect(body.output).toEqual({ resultWithheld: true, effect: 'not_attempted' }) + // The thrown reason is an unprojectable environment failure — the catalog that would + // vouch for it is the very thing missing — so it stays in the log. + expect(body.error).not.toContain('does not exist') + expect(body.error).toContain(BASE_BODY.workspaceId) + expect(body.error).toContain('could not be resolved') }) it('reuses one turn registry across calls that share a messageId', async () => { diff --git a/apps/sim/app/api/copilot/tools/execute/route.ts b/apps/sim/app/api/copilot/tools/execute/route.ts index 2f99e874ffb..f645a5cb522 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.ts @@ -134,18 +134,25 @@ export const POST = withRouteHandler((request: NextRequest) => * The cause is almost always the workspace itself — a deleted or inaccessible id * reaching this lane — which is actionable, so it is reported rather than swallowed. */ - const reason = getErrorMessage(err) logger.error('In-band egress registry unavailable; refusing the call', { toolName, toolCallId, userId, workspaceId, - error: reason, + error: getErrorMessage(err), }) rootSpan.setAttributes({ [TraceAttr.ToolOutcome]: MothershipStreamV1ToolOutcome.error }) + /** + * The thrown reason stays in the log. It is an environment or database failure that + * nothing here can project — the catalog it needed is the very thing that is missing — + * so this is the one message on this route that must be fixed text. The workspace id + * is echoed because the caller supplied it, and it is what makes this actionable. + */ return NextResponse.json({ success: false, - error: `${toolName} was not run: ${reason}`, + error: workspaceId + ? `${toolName} was not run: its workspace (${workspaceId}) could not be resolved. Check that the workspace exists and is accessible before retrying.` + : `${toolName} was not run: its execution environment could not be resolved.`, output: { resultWithheld: true, effect: TOOL_EFFECT_PHASE.notAttempted }, }) } diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index c653c22e65a..aac43574507 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -53,7 +53,14 @@ const ATTEMPTED_EXECUTION_ID = 'attemptedExecutionId' export function attachAttemptedExecutionId(error: unknown, executionId: string): void { if (!isAttachableThrown(error) || !executionId) return if (Object.hasOwn(error, ATTEMPTED_EXECUTION_ID)) return - Object.assign(error, { [ATTEMPTED_EXECUTION_ID]: executionId }) + /** + * A frozen or sealed failure would otherwise throw here and replace the original error + * partway through cleanup, turning a diagnostic aid into the thing that loses the + * diagnosis. Losing the id is the lesser failure, and the log still records the run. + */ + try { + Object.assign(error, { [ATTEMPTED_EXECUTION_ID]: executionId }) + } catch {} } /** Reads the dispatched-run id a thrown value carries, if dispatch was reached at all. */ diff --git a/apps/sim/lib/workflows/executor/execution-core.ts b/apps/sim/lib/workflows/executor/execution-core.ts index 9dd69e18b13..471d859962f 100644 --- a/apps/sim/lib/workflows/executor/execution-core.ts +++ b/apps/sim/lib/workflows/executor/execution-core.ts @@ -424,6 +424,7 @@ async function executeWorkflowCoreImpl( let processedInput = input || {} let deploymentVersionId: string | undefined let loggingStarted = false + let executorStarted = false let resolvedSecretTraceRegistry: ResolvedSecretTraceRegistry | undefined const pendingLifecycleCallbacks = new Set>() @@ -1051,6 +1052,14 @@ async function executeWorkflowCoreImpl( contextExtensions, }) + /** + * The last statement before a block can run, and therefore the only honest answer to + * "could a side effect have occurred". The logging session is the wrong proxy in both + * directions: `safeStart`'s result is never checked, so blocks execute even when it + * fails — reporting nothing started for a run that did — and it flips before trigger + * resolution and serialization, reporting a run for failures that never reached a block. + */ + executorStarted = true const result = runFromBlock ? ((await executorInstance.executeFromBlock( workflowId, @@ -1098,17 +1107,12 @@ async function executeWorkflowCoreImpl( return result } catch (error: unknown) { /** - * Whether the run had started when it failed, read before the recovery `safeStart` below - * writes a row for the failure itself. This — not entry into this function — is what says - * blocks may have executed: everything above it (custom-block loading, state loading, - * environment and secret setup) fails having run nothing, and a caller told otherwise - * would refuse a retry that was safe. - * - * The thrown value is rethrown exactly as received, including a non-Error one, because - * the finalization guard below identifies it. A primitive therefore carries no id, which - * costs nothing today: every throw site past `safeStart` raises an Error. + * Named only once a block could have run. The thrown value is rethrown exactly as + * received, including a non-Error one, because the finalization guard below identifies + * it; a primitive therefore carries no id, which costs nothing today because every throw + * site past this point raises an Error. */ - if (loggingStarted) attachAttemptedExecutionId(error, executionId) + if (executorStarted) attachAttemptedExecutionId(error, executionId) const errorCause = describeErrorCause(error) logger.error( `[${requestId}] Execution failed:`, From 1c3572a30a80f590c21ce40f29bf2b476f92c995 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 16:56:39 -0700 Subject: [PATCH 08/16] fix(copilot): key the dispatched-run id off the failure instead of writing to it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guarding the write was trading one failure for the worse one: a frozen or sealed error kept the process alive but dropped the marker, which turns "this run exists" into "nothing started" — the single direction that duplicates work. Record the id in a WeakMap keyed by the thrown value, the same shape markExecutionFinalizedByCore already keeps for the same reason. Nothing is written to the error, so a non-extensible one is recorded like any other and there is no throw to guard. Identity keying also retires the prototype-chain concern, and the error's own surface stays clean, so a serialized failure no longer carries a stray field. A thrown primitive still cannot be keyed, which costs nothing today because every throw site past the dispatch boundary raises an Error. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/executor/utils/errors.ts | 45 +++++++++---------- .../workflow/withheld-run-result.test.ts | 15 +++++++ .../run-workflow-from-copilot.test.ts | 6 +-- 3 files changed, 39 insertions(+), 27 deletions(-) diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index aac43574507..9b449034121 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -36,14 +36,25 @@ export function attachExecutionResult(error: Error, executionResult: ExecutionRe Object.assign(error, { executionResult }) } -const ATTEMPTED_EXECUTION_ID = 'attemptedExecutionId' +/** + * Dispatched-run ids, keyed by the thrown value itself. + * + * A side table rather than a property on the error, for the same reason + * {@link markExecutionFinalizedByCore} keeps one: a thrown value is not reliably writable. + * `Object.assign` throws on a frozen or sealed failure, and guarding that throw would drop + * the marker instead — silently converting "this run exists" into "nothing started", which + * is the one direction that duplicates work. Identity keying also means no id can arrive + * through a prototype chain, and nothing is added to the error's own surface, so a + * serialized error carries no stray field. + */ +const attemptedExecutionIds = new WeakMap() /** * Names the run a failure belongs to once dispatch has been attempted. * * A caller that only sees the thrown error cannot tell an authorization refusal — which * created nothing — from a crash after the run was already dispatched, and those need - * opposite retry decisions. Attaching the id at the point of no return makes its absence + * opposite retry decisions. Recording the id at the point of no return makes its absence * mean "nothing was started" rather than "we do not know", and its presence a key that * resolves to zero or one executions. * @@ -51,39 +62,25 @@ const ATTEMPTED_EXECUTION_ID = 'attemptedExecutionId' * result, this says only that it was dispatched. */ export function attachAttemptedExecutionId(error: unknown, executionId: string): void { - if (!isAttachableThrown(error) || !executionId) return - if (Object.hasOwn(error, ATTEMPTED_EXECUTION_ID)) return - /** - * A frozen or sealed failure would otherwise throw here and replace the original error - * partway through cleanup, turning a diagnostic aid into the thing that loses the - * diagnosis. Losing the id is the lesser failure, and the log still records the run. - */ - try { - Object.assign(error, { [ATTEMPTED_EXECUTION_ID]: executionId }) - } catch {} + if (!isRecordedThrown(error) || !executionId) return + if (attemptedExecutionIds.has(error)) return + attemptedExecutionIds.set(error, executionId) } /** Reads the dispatched-run id a thrown value carries, if dispatch was reached at all. */ export function readAttemptedExecutionId(error: unknown): string | undefined { - /** - * Own property only. `in` would accept one reached through the prototype chain, which - * would let an unrelated run's id be disclosed for a refusal that started nothing. - */ - if (!isAttachableThrown(error) || !Object.hasOwn(error, ATTEMPTED_EXECUTION_ID)) return undefined - const value = (error as Record)[ATTEMPTED_EXECUTION_ID] - return typeof value === 'string' && value.length > 0 ? value : undefined + return isRecordedThrown(error) ? attemptedExecutionIds.get(error) : undefined } /** * Any non-null object, not only an `Error`. * * Restricting this to `Error` would silently invert the invariant for a thrown plain object: - * the id would not attach, the absence would then read as "nothing was started", and the - * caller would retry a run that already exists — the exact duplicate-side-effect outcome - * this id exists to prevent. A thrown primitive cannot carry a property at all, so callers - * that must not lose the id normalize before attaching. + * no id would be recorded, its absence would read as "nothing was started", and the caller + * would retry a run that already exists. A thrown primitive cannot be keyed at all, which + * costs nothing today because every throw site past the dispatch boundary raises an `Error`. */ -function isAttachableThrown(value: unknown): value is Record { +function isRecordedThrown(value: unknown): value is object { return typeof value === 'object' && value !== null } diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts b/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts index dbf32e59536..6ee744a206d 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts @@ -157,6 +157,21 @@ describe('a withheld run_workflow result', () => { expect(views[2]).toContain(EXECUTION_ID) }) + /** A frozen failure cannot take a property, and losing the id here would invite a duplicate run. */ + it('still names the run when the failure cannot be written to', async () => { + const error = Object.freeze(new Error('crashed')) + attachAttemptedExecutionId(error, EXECUTION_ID) + mocks.executeWorkflowUseCase.mockRejectedValue(error) + + const { result } = await withheld(await executeRunWorkflow({ workflowId: 'wf-1' }, context)) + + expect(modelOutput(result)).toEqual({ + resultWithheld: true, + effect: 'attempted', + executionId: EXECUTION_ID, + }) + }) + it('never lets the withheld payload carry the run content past the boundary', async () => { mocks.executeWorkflowUseCase.mockResolvedValue({ success: false, 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 aa254dda770..5101fcd5e42 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 @@ -68,7 +68,7 @@ import { runFromBlockFromCopilot, runWorkflowFromCopilot, } from '@/lib/workflows/application/run-workflow-from-copilot' -import { readAttemptedExecutionId } from '@/executor/utils/errors' +import { attachAttemptedExecutionId, readAttemptedExecutionId } from '@/executor/utils/errors' const principal = { kind: 'delegated' as const, @@ -326,9 +326,9 @@ describe('Copilot workflow run application commands', () => { it('passes through the id executeWorkflow attached at its dispatch boundary', async () => { mocks.executeWorkflow.mockImplementationOnce(() => { - // Exactly what the execution core does once its logging session has started. + // Exactly what the execution core does once a block could have run. const dispatchFailure = new Error('database unavailable') - Object.assign(dispatchFailure, { attemptedExecutionId: 'child-execution-1' }) + attachAttemptedExecutionId(dispatchFailure, 'child-execution-1') throw dispatchFailure }) From cc012cc71b55d4791f158d11dd5c139bbc90aff8 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 17:06:44 -0700 Subject: [PATCH 09/16] fix(copilot): let the executor say when a block could first run Review was right that entering `execute` is still too early: DAG construction, snapshot restoration and pipeline assembly all happen inside it and reject a malformed graph having changed nothing, so a validation failure reported a run to resolve. Only the executor knows where that line falls, so it reports it. A `onBlocksMayRun` context extension fires immediately before `engine.run` on both entry points, and execution-core records the run from there rather than guessing at it from outside. A rejected graph now correctly says nothing started. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/executor/execution/executor.ts | 2 ++ apps/sim/executor/execution/types.ts | 10 +++++++++ .../lib/workflows/executor/execution-core.ts | 21 ++++++++++++------- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/apps/sim/executor/execution/executor.ts b/apps/sim/executor/execution/executor.ts index c567df277c8..84e6609d277 100644 --- a/apps/sim/executor/execution/executor.ts +++ b/apps/sim/executor/execution/executor.ts @@ -101,6 +101,7 @@ export class DAGExecutor { this.registerRestoredClonedSubflows(context.subflowParentMap, restoredClonedSubflows) const engine = this.buildExecutionPipeline(context, dag, state) + this.contextExtensions.onBlocksMayRun?.() return await engine.run(triggerBlockId) } @@ -258,6 +259,7 @@ export class DAGExecutor { context.subflowParentMap = this.buildSubflowParentMap(dag) const engine = this.buildExecutionPipeline(context, dag, state, filteredSnapshot) + this.contextExtensions.onBlocksMayRun?.() const result = await engine.run() if (result.metadata) { result.metadata.largeValueKeys = context.largeValueKeys diff --git a/apps/sim/executor/execution/types.ts b/apps/sim/executor/execution/types.ts index 35cf038381f..eb5e08b1d64 100644 --- a/apps/sim/executor/execution/types.ts +++ b/apps/sim/executor/execution/types.ts @@ -238,6 +238,16 @@ export interface PiiBlockOutputRedaction { } export interface ContextExtensions { + /** + * Fired once, immediately before the engine may run a block. + * + * Everything the executor does first — DAG construction, snapshot restoration, pipeline + * assembly — can reject a request having changed nothing, so a caller that needs to know + * whether a side effect was possible cannot infer it from having called `execute`. Only + * the executor knows where that line falls, so it reports it rather than being guessed at + * from the outside. + */ + onBlocksMayRun?: () => void workspaceId?: string executionId?: string largeValueExecutionIds?: string[] diff --git a/apps/sim/lib/workflows/executor/execution-core.ts b/apps/sim/lib/workflows/executor/execution-core.ts index 471d859962f..a3dee66041b 100644 --- a/apps/sim/lib/workflows/executor/execution-core.ts +++ b/apps/sim/lib/workflows/executor/execution-core.ts @@ -961,6 +961,19 @@ async function executeWorkflowCoreImpl( const principalSubject = resolvePrincipalSubject(metadata.principal) const contextExtensions: ContextExtensions = { + /** + * The only honest answer to "could a side effect have occurred", and the executor is + * the only thing that knows it: everything before this — DAG construction, snapshot + * restoration, pipeline assembly — can reject a request having changed nothing. + * + * The logging session was the wrong proxy in both directions. `safeStart`'s result is + * never checked, so blocks run even when it fails, reporting that nothing started for + * a run that did; and it flips before trigger resolution and serialization, reporting + * a run for failures that never reached a block. + */ + onBlocksMayRun: () => { + executorStarted = true + }, stream: !!onStream, selectedOutputs, executionId, @@ -1052,14 +1065,6 @@ async function executeWorkflowCoreImpl( contextExtensions, }) - /** - * The last statement before a block can run, and therefore the only honest answer to - * "could a side effect have occurred". The logging session is the wrong proxy in both - * directions: `safeStart`'s result is never checked, so blocks execute even when it - * fails — reporting nothing started for a run that did — and it flips before trigger - * resolution and serialization, reporting a run for failures that never reached a block. - */ - executorStarted = true const result = runFromBlock ? ((await executorInstance.executeFromBlock( workflowId, From 46b818b7b134b93c5c26c6c4041c9b5b8774cd4e Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 17:48:42 -0700 Subject: [PATCH 10/16] fix(copilot): report the run from the engine, and never let recovery erase it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings, both real. Firing before `engine.run` was still one step early: the engine's cancellation subscription is fallible and rejects having run nothing, so that failure claimed a run. The signal now fires inside the engine, immediately before the loop that processes blocks and past every startup step that can refuse a request — DAG construction, pipeline assembly and the subscription. The executor no longer guesses at the line from outside; the engine states it. Separately, the copilot catch path could throw while recording the failed crossing or releasing the execution slot. Either one propagated a different error — one the dispatched-run id was never recorded against — so an existing run reported itself as never started and invited the duplicate the id exists to prevent. Both are recovery work and neither may replace the failure it is describing, so both are contained and logged. Contained with try/catch rather than a rejection handler, since a synchronous throw has to be caught too. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/executor/execution/engine.ts | 9 ++++ apps/sim/executor/execution/executor.ts | 3 +- apps/sim/executor/types.ts | 2 + .../application/run-workflow-from-copilot.ts | 48 ++++++++++++++----- 4 files changed, 49 insertions(+), 13 deletions(-) diff --git a/apps/sim/executor/execution/engine.ts b/apps/sim/executor/execution/engine.ts index acfa7cfa42b..0aebeb7bace 100644 --- a/apps/sim/executor/execution/engine.ts +++ b/apps/sim/executor/execution/engine.ts @@ -107,6 +107,15 @@ export class ExecutionEngine { this.initializeQueue(triggerBlockId) await this.subscribeToCancellationSignal() + /** + * Past every fallible startup step — DAG construction, pipeline assembly and the + * cancellation subscription above all reject having run nothing — and immediately + * before the loop that processes blocks. This is the line a caller means by "could a + * side effect have occurred"; anything earlier reports a run for a request that was + * merely refused. + */ + this.context.onBlocksMayRun?.() + while (this.hasWork()) { if (this.checkCancellation() || this.errorFlag || this.stoppedEarlyFlag) { break diff --git a/apps/sim/executor/execution/executor.ts b/apps/sim/executor/execution/executor.ts index 84e6609d277..7aec5a2786e 100644 --- a/apps/sim/executor/execution/executor.ts +++ b/apps/sim/executor/execution/executor.ts @@ -101,7 +101,6 @@ export class DAGExecutor { this.registerRestoredClonedSubflows(context.subflowParentMap, restoredClonedSubflows) const engine = this.buildExecutionPipeline(context, dag, state) - this.contextExtensions.onBlocksMayRun?.() return await engine.run(triggerBlockId) } @@ -259,7 +258,6 @@ export class DAGExecutor { context.subflowParentMap = this.buildSubflowParentMap(dag) const engine = this.buildExecutionPipeline(context, dag, state, filteredSnapshot) - this.contextExtensions.onBlocksMayRun?.() const result = await engine.run() if (result.metadata) { result.metadata.largeValueKeys = context.largeValueKeys @@ -417,6 +415,7 @@ export class DAGExecutor { const context: ExecutionContext = { workflowId, + onBlocksMayRun: this.contextExtensions.onBlocksMayRun, workspaceId: this.contextExtensions.workspaceId, executionId: this.contextExtensions.executionId, largeValueExecutionIds: this.contextExtensions.largeValueExecutionIds, diff --git a/apps/sim/executor/types.ts b/apps/sim/executor/types.ts index 649398ed616..1e3ba1c3d9c 100644 --- a/apps/sim/executor/types.ts +++ b/apps/sim/executor/types.ts @@ -372,6 +372,8 @@ export interface ExecutorDelegationOrigin { } export interface ExecutionContext { + /** See {@link ContextExtensions.onBlocksMayRun}. Fired by the engine, once. */ + onBlocksMayRun?: () => void workflowId: string workspaceId?: string executionId?: string 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 0742a8ceb09..0f250679a91 100644 --- a/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts +++ b/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts @@ -1,4 +1,5 @@ import { type Principal, requirePrincipalSubjectUserId } from '@sim/auth/principal' +import { createLogger } from '@sim/logger' import { toError } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { mergeSubblockStateWithValues } from '@sim/workflow-persistence/subblocks' @@ -29,6 +30,9 @@ import { import type { SerializableExecutionState } from '@/executor/execution/types' import type { ExecutionResult } from '@/executor/types' import { attachAttemptedExecutionId } from '@/executor/utils/errors' + +const logger = createLogger('CopilotWorkflowRun') + import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' export interface CopilotWorkflowRunLifecycle { @@ -305,6 +309,12 @@ async function executeCopilotRun(params: { * what threw and an execution certainly exists. */ if (runReturned) attachAttemptedExecutionId(error, childExecutionId) + /** + * Recovery must never replace the failure it is describing. Both steps below run only to + * record and release, and either throwing would propagate a different error — one the + * dispatched-run id was never recorded against — so an existing run would report itself + * as never started and invite the duplicate this id exists to prevent. + */ if (registry) { const executionResult = typeof error === 'object' && @@ -313,18 +323,34 @@ async function executeCopilotRun(params: { typeof error.executionResult === 'object' ? (error.executionResult as ExecutionResult) : undefined - await registry.importCrossingProvenance( - executionResult?.executionState?.resolvedSecretTraceProvenance, - { - output: executionResult?.output, - logs: executionResult?.logs, - error: executionResult?.error, - thrownMessage: toError(error).message, - }, - { trusted: true, origin: 'copilotWorkflowMutation.failedRunCrossing' } - ) + try { + await registry.importCrossingProvenance( + executionResult?.executionState?.resolvedSecretTraceProvenance, + { + output: executionResult?.output, + logs: executionResult?.logs, + error: executionResult?.error, + thrownMessage: toError(error).message, + }, + { trusted: true, origin: 'copilotWorkflowMutation.failedRunCrossing' } + ) + } catch (importError) { + logger.error('Failed to record provenance for a failed Copilot run', { + executionId: childExecutionId, + error: toError(importError).message, + }) + } + } + if (admission.targetReservation) { + try { + await releaseExecutionSlot(childExecutionId) + } catch (releaseError) { + logger.error('Failed to release the execution slot for a failed Copilot run', { + executionId: childExecutionId, + error: toError(releaseError).message, + }) + } } - if (admission.targetReservation) await releaseExecutionSlot(childExecutionId) throw error } finally { completePendingActivation?.() From f28a7d7ecbdf08819c12c6d554932e11d0399a2a Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 18:23:41 -0700 Subject: [PATCH 11/16] fix(copilot): mark a run that failed after the core returned MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once `executeWorkflowCore` returns, the run happened. Everything after it in `executeWorkflow` — analytics, pause persistence, post-execution settling — is bookkeeping that can still throw, and the core's own catch no longer runs, so those failures named no run. The copilot handler then reported `not_attempted` for an execution that had already produced side effects, which is the one direction that duplicates work. Mark it as soon as the core settles, so any later failure carries it. The `finally` had the same shape and is now contained: a throw there replaces whatever the function was about to do, turning a successful run into an error or an error that names its run into one that does not. Settling post-execution work is bookkeeping and must not be able to do either. Co-Authored-By: Claude Opus 5 (1M context) --- .../workflows/executor/execute-workflow.ts | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/workflows/executor/execute-workflow.ts b/apps/sim/lib/workflows/executor/execute-workflow.ts index 8336ea332d2..8f498f5ac08 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 { getErrorMessage } 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 { attachAttemptedExecutionId } 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,7 @@ export async function executeWorkflow( loggingSession.setTrustedExecutionCorrelation(streamConfig.trustedExecutionCorrelation) } let postExecutionOwnershipTransferred = false + let coreReturned = false try { const metadata: ExecutionMetadata = { @@ -169,6 +172,12 @@ export async function executeWorkflow( const executionStartMs = Date.now() + /** + * Once the core returns, the run happened. Everything after it here — analytics, pause + * persistence, post-execution settling — is bookkeeping that can still throw, and a + * failure there names no run unless it is marked, so an execution that really occurred + * would report itself as never started and invite a duplicate. + */ const result = await executeWorkflowCore({ snapshot, callbacks: { @@ -198,6 +207,7 @@ export async function executeWorkflow( streamConfig?.trustedInitialResolvedSecretTraceProvenance, runFromBlock: streamConfig?.runFromBlock, }) + coreReturned = true const blockTypes = [ ...new Set( @@ -241,6 +251,7 @@ export async function executeWorkflow( return result } catch (error: unknown) { + if (coreReturned) attachAttemptedExecutionId(error, executionId) const errorDiagnostic = loggingSession.projectDiagnosticError(error) logger.error(`[${requestId}] Workflow execution failed`, errorDiagnostic) @@ -262,7 +273,19 @@ export async function executeWorkflow( throw error } finally { if (!postExecutionOwnershipTransferred) { - await loggingSession.waitForPostExecution() + /** + * A `finally` that throws replaces whatever the function was about to do — turning a + * successful run into an error, or an error that names its run into one that does not. + * Settling post-execution work is bookkeeping and must not be able to do either. + */ + try { + await loggingSession.waitForPostExecution() + } catch (postExecutionError) { + logger.error(`[${requestId}] Failed to settle post-execution work`, { + executionId, + error: getErrorMessage(postExecutionError), + }) + } } } } From 8c8ec84409fdc801a730b2b7c4f4a1c59fc29de7 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 18:31:06 -0700 Subject: [PATCH 12/16] fix(copilot): stop calling a cancelled run performed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `performed` claims the run reached the end of its work, so a caller reads it as "never retry, just read the outcome". Every returned result carried it, including a cancelled or paused one — which stopped partway and may have run every block, one, or none. Those are `attempted`: an execution exists under this id, resolve it before deciding anything. That is true whether the cancellation landed before the first block or after the last. Co-Authored-By: Claude Opus 5 (1M context) --- .../tools/handlers/workflow/mutations.test.ts | 13 +++++++++++++ .../tools/handlers/workflow/mutations.ts | 17 ++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts index 67d3ec5ed0e..9e11b107c6a 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts @@ -295,6 +295,19 @@ describe('workflow mutation Copilot adapters', () => { run: () => executeRunWorkflow({ workflowId: 'workflow-1' }, context), effect: { phase: 'attempted', ids: { executionId: 'execution-1' } }, }, + { + label: 'cancelled before it could finish', + arrange: () => + mocks.executeWorkflowUseCase.mockResolvedValueOnce({ + success: false, + output: {}, + logs: [], + status: 'cancelled', + metadata: { executionId: 'execution-1' }, + }), + run: () => executeRunWorkflow({ workflowId: 'workflow-1' }, context), + effect: { phase: 'attempted', ids: { executionId: 'execution-1' } }, + }, { label: 'completed', arrange: () => diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts index 897d2ef6fee..37cda4ea96c 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts @@ -60,6 +60,20 @@ function runRejected(error: string): ToolCallResult { return { success: false, error, effect: executionEffect(TOOL_EFFECT_PHASE.notAttempted) } } +/** + * `performed` claims the run reached the end of its work, so only a run that terminated on + * its own may carry it. A cancelled or paused one stopped partway — it may have run every + * block, one, or none — and `attempted` is the phase that says exactly that: an execution + * exists under this id, resolve it before deciding anything. + */ +function executionPhase(status: ExecutionResultStatus): ToolEffectPhase { + return status === 'cancelled' || status === 'paused' + ? TOOL_EFFECT_PHASE.attempted + : TOOL_EFFECT_PHASE.performed +} + +type ExecutionResultStatus = 'completed' | 'paused' | 'cancelled' | undefined + function buildExecutionOutput( result: { success: boolean @@ -67,6 +81,7 @@ function buildExecutionOutput( output?: unknown logs?: unknown[] error?: string + status?: ExecutionResultStatus }, extra?: Record ): ToolCallResult { @@ -80,7 +95,7 @@ function buildExecutionOutput( logs: stripBinaryFields(result.logs), }, error: result.success ? undefined : result.error || 'Workflow execution failed', - effect: executionEffect(TOOL_EFFECT_PHASE.performed, result.metadata?.executionId), + effect: executionEffect(executionPhase(result.status), result.metadata?.executionId), } } From 89cdb0ad172f88eac9162327075ded7a27424de9 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 19:07:55 -0700 Subject: [PATCH 13/16] fix(copilot): derive the run phase from what the executor saw, not the result's shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine review rounds found the same class of defect, which makes it a design problem rather than nine bugs. "Did a side effect occur" had two sources that disagreed: a precise marker on the thrown path, and on the returned path an inference from whatever the outcome happened to look like. Every property used for that inference is a proxy that breaks on the paths that matter — an engine failing before its first block still carries an ExecutionResult, and a run that ends without one still ran every block it had — so each round found another path where the proxy lied. There is now one source. The engine reports the moment a block handler is first about to run, which is terminal: no fallible step remains between it and the handler, so there is nothing left for a later reviewer to find in front of it. The signal is threaded to the caller and recorded against the outcome, and the copilot adapter reads it on every exit path instead of inspecting status or the presence of an attached result. The phase then follows from two stated facts rather than a guess: nothing dispatched is not_attempted whatever the result looks like, a run that stopped partway is attempted, and one that reached the end is performed. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/executor/execution/engine.ts | 25 +++++--- apps/sim/executor/utils/errors.ts | 21 +++++++ .../tools/handlers/workflow/mutations.test.ts | 18 ++++++ .../tools/handlers/workflow/mutations.ts | 58 ++++++++++++++----- .../application/run-workflow-from-copilot.ts | 16 ++++- .../workflows/executor/execute-workflow.ts | 7 +++ .../lib/workflows/executor/execution-core.ts | 2 + 7 files changed, 121 insertions(+), 26 deletions(-) diff --git a/apps/sim/executor/execution/engine.ts b/apps/sim/executor/execution/engine.ts index 0aebeb7bace..0f5aac1e1fb 100644 --- a/apps/sim/executor/execution/engine.ts +++ b/apps/sim/executor/execution/engine.ts @@ -38,6 +38,7 @@ export class ExecutionEngine { private cancellationController = new AbortController() private abortSignalListener: (() => void) | null = null private cancellationUnsubscribe: (() => void) | null = null + private reportedBlocksMayRun = false private execLogger: Logger constructor( @@ -62,6 +63,13 @@ export class ExecutionEngine { this.initializeAbortHandler() } + /** Fires the caller's dispatch observer exactly once, however many nodes follow. */ + private reportBlocksMayRun(): void { + if (this.reportedBlocksMayRun) return + this.reportedBlocksMayRun = true + this.context.onBlocksMayRun?.() + } + private async subscribeToCancellationSignal(): Promise { if (!this.context.executionId) return const executionId = this.context.executionId @@ -107,15 +115,6 @@ export class ExecutionEngine { this.initializeQueue(triggerBlockId) await this.subscribeToCancellationSignal() - /** - * Past every fallible startup step — DAG construction, pipeline assembly and the - * cancellation subscription above all reject having run nothing — and immediately - * before the loop that processes blocks. This is the line a caller means by "could a - * side effect have occurred"; anything earlier reports a run for a request that was - * merely refused. - */ - this.context.onBlocksMayRun?.() - while (this.hasWork()) { if (this.checkCancellation() || this.errorFlag || this.stoppedEarlyFlag) { break @@ -430,6 +429,14 @@ export class ExecutionEngine { private async executeNodeAsync(nodeId: string): Promise { try { const wasAlreadyExecuted = this.context.executedBlocks.has(nodeId) + /** + * The single moment a side effect becomes possible: the last statement before a block + * handler runs. Every earlier candidate was a proxy that a reviewer could then find a + * fallible step in front of — startup, the cancellation subscription, queue and + * subflow initialization all reject having run nothing. There is nothing between here + * and the handler, so there is nothing left to be in front of. + */ + this.reportBlocksMayRun() const result = await this.nodeOrchestrator.executeNode(this.context, nodeId) if (!wasAlreadyExecuted) { diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index 9b449034121..3125f484310 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -67,6 +67,27 @@ export function attachAttemptedExecutionId(error: unknown, executionId: string): attemptedExecutionIds.set(error, executionId) } +/** + * Whether a block handler ran, keyed by the outcome that carries it — a result or a throw. + * + * Recorded rather than inferred. Every property of an outcome that looks like it answers + * this is a proxy that disagrees on the paths that matter: an engine failing before its + * first block still carries an `ExecutionResult`, and a run that ends without one still ran + * every block it had. Only the executor knows, so only the executor says. + */ +const observedBlockDispatch = new WeakMap() + +/** Records the executor's answer against the outcome a caller will read it from. */ +export function recordBlocksMayHaveRun(outcome: unknown, blocksMayHaveRun: boolean): void { + if (!isRecordedThrown(outcome)) return + observedBlockDispatch.set(outcome, blocksMayHaveRun) +} + +/** Undefined when nothing observed this run, which callers must treat conservatively. */ +export function readBlocksMayHaveRun(outcome: unknown): boolean | undefined { + return isRecordedThrown(outcome) ? observedBlockDispatch.get(outcome) : undefined +} + /** Reads the dispatched-run id a thrown value carries, if dispatch was reached at all. */ export function readAttemptedExecutionId(error: unknown): string | undefined { return isRecordedThrown(error) ? attemptedExecutionIds.get(error) : undefined diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts index 9e11b107c6a..759cbdcd618 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts @@ -10,6 +10,7 @@ const { mocks } = vi.hoisted(() => ({ executeWorkflowUseCase: vi.fn(), hasExecutionResult: vi.fn(), readAttemptedExecutionId: vi.fn(), + readBlocksMayHaveRun: vi.fn(), }, })) @@ -30,6 +31,7 @@ vi.mock('@/lib/workflows/sanitization/json-sanitizer', () => ({ vi.mock('@/executor/utils/errors', () => ({ hasExecutionResult: mocks.hasExecutionResult, readAttemptedExecutionId: mocks.readAttemptedExecutionId, + readBlocksMayHaveRun: mocks.readBlocksMayHaveRun, })) vi.mock('@/lib/core/telemetry', () => ({ @@ -60,6 +62,8 @@ describe('workflow mutation Copilot adapters', () => { vi.clearAllMocks() mocks.hasExecutionResult.mockReturnValue(false) mocks.readAttemptedExecutionId.mockReturnValue(undefined) + // Default: the executor saw a block run, which is the ordinary case. + mocks.readBlocksMayHaveRun.mockReturnValue(true) }) it('maps encoded folder aliases into one create application command', async () => { @@ -295,6 +299,20 @@ describe('workflow mutation Copilot adapters', () => { run: () => executeRunWorkflow({ workflowId: 'workflow-1' }, context), effect: { phase: 'attempted', ids: { executionId: 'execution-1' } }, }, + { + label: 'refused by the engine before any block ran', + arrange: () => { + mocks.readBlocksMayHaveRun.mockReturnValue(false) + mocks.executeWorkflowUseCase.mockResolvedValueOnce({ + success: false, + output: {}, + logs: [], + metadata: { executionId: 'execution-1' }, + }) + }, + run: () => executeRunWorkflow({ workflowId: 'workflow-1' }, context), + effect: { phase: 'not_attempted', ids: { executionId: 'execution-1' } }, + }, { label: 'cancelled before it could finish', arrange: () => diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts index 37cda4ea96c..5205fd64b2d 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts @@ -29,7 +29,11 @@ import { setWorkflowBlockEnabled, } from '@/lib/workflows/application/update-workflow-content' import { sanitizeForCopilot } from '@/lib/workflows/sanitization/json-sanitizer' -import { hasExecutionResult, readAttemptedExecutionId } from '@/executor/utils/errors' +import { + hasExecutionResult, + readAttemptedExecutionId, + readBlocksMayHaveRun, +} from '@/executor/utils/errors' import type { WorkflowState } from '@/stores/workflows/workflow/types' function stripBinaryFields(value: unknown): unknown { @@ -61,12 +65,18 @@ function runRejected(error: string): ToolCallResult { } /** - * `performed` claims the run reached the end of its work, so only a run that terminated on - * its own may carry it. A cancelled or paused one stopped partway — it may have run every - * block, one, or none — and `attempted` is the phase that says exactly that: an execution - * exists under this id, resolve it before deciding anything. + * Derives the phase from two facts, neither of them guessed. + * + * `blocksMayHaveRun` comes from the executor and is the only thing that separates a run + * with side effects from a request that was refused; the status says whether that run + * reached the end of its work. Undefined means nothing observed the run, which is treated + * as "it may have" — over-reporting costs a lookup, under-reporting duplicates work. */ -function executionPhase(status: ExecutionResultStatus): ToolEffectPhase { +function executionPhase( + blocksMayHaveRun: boolean | undefined, + status: ExecutionResultStatus +): ToolEffectPhase { + if (blocksMayHaveRun === false) return TOOL_EFFECT_PHASE.notAttempted return status === 'cancelled' || status === 'paused' ? TOOL_EFFECT_PHASE.attempted : TOOL_EFFECT_PHASE.performed @@ -74,6 +84,11 @@ function executionPhase(status: ExecutionResultStatus): ToolEffectPhase { type ExecutionResultStatus = 'completed' | 'paused' | 'cancelled' | undefined +/** The phase of a run that returned, from what the executor observed about it. */ +function runPhase(result: { status?: ExecutionResultStatus }): ToolEffectPhase { + return executionPhase(readBlocksMayHaveRun(result), result.status) +} + function buildExecutionOutput( result: { success: boolean @@ -83,6 +98,7 @@ function buildExecutionOutput( error?: string status?: ExecutionResultStatus }, + phase: ToolEffectPhase, extra?: Record ): ToolCallResult { return { @@ -95,17 +111,25 @@ function buildExecutionOutput( logs: stripBinaryFields(result.logs), }, error: result.success ? undefined : result.error || 'Workflow execution failed', - effect: executionEffect(executionPhase(result.status), result.metadata?.executionId), + effect: executionEffect(phase, result.metadata?.executionId), } } function buildExecutionError(error: unknown): ToolCallResult { if (hasExecutionResult(error)) { - return buildExecutionOutput({ - ...error.executionResult, - success: false, - error: error.executionResult.error || 'Workflow execution failed', - }) + return buildExecutionOutput( + { + ...error.executionResult, + success: false, + error: error.executionResult.error || 'Workflow execution failed', + }, + /** + * Read off the error, not the spread copy above: the executor recorded its answer + * against the value it threw, and spreading makes a new object the record cannot + * follow. + */ + executionPhase(readBlocksMayHaveRun(error), error.executionResult.status) + ) } logger.error('Copilot workflow execution command failed', { error }) /** @@ -267,7 +291,7 @@ export async function executeRunWorkflow( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result) + return buildExecutionOutput(result, runPhase(result)) } catch (error) { return buildExecutionError(error) } @@ -389,7 +413,9 @@ export async function executeRunWorkflowUntilBlock( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result, { stoppedAfterBlockId: params.stopAfterBlockId }) + return buildExecutionOutput(result, runPhase(result), { + stoppedAfterBlockId: params.stopAfterBlockId, + }) } catch (error) { return buildExecutionError(error) } @@ -464,7 +490,7 @@ export async function executeRunFromBlock( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result, { startBlockId: params.startBlockId }) + return buildExecutionOutput(result, runPhase(result), { startBlockId: params.startBlockId }) } catch (error) { return buildExecutionError(error) } @@ -550,7 +576,7 @@ export async function executeRunBlock( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result, { blockId: params.blockId }) + return buildExecutionOutput(result, runPhase(result), { blockId: params.blockId }) } catch (error) { return buildExecutionError(error) } 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 0f250679a91..41c7761a786 100644 --- a/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts +++ b/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts @@ -29,7 +29,7 @@ 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, recordBlocksMayHaveRun } from '@/executor/utils/errors' const logger = createLogger('CopilotWorkflowRun') @@ -251,6 +251,7 @@ async function executeCopilotRun(params: { ) const completePendingActivation = registry?.beginPendingActivation() let runReturned = false + let blocksMayHaveRun = false try { const result = await executeWorkflow( { @@ -273,6 +274,17 @@ async function executeCopilotRun(params: { stopAfterBlockId: params.stopAfterBlockId, runFromBlock: params.runFromBlock, abortSignal: params.input.lifecycle.abortSignal, + /** + * Whether a block could have run, stated by the executor rather than inferred here. + * Every earlier attempt read it off the shape of the outcome — a status field, or + * whether an ExecutionResult rode along on the error — and those are proxies that + * disagree with reality on exactly the paths that matter: an engine that fails + * before its first block still carries a result, and a run that ends without one + * still ran every block it had. + */ + onBlocksMayRun: () => { + blocksMayHaveRun = true + }, billingAttribution: admission.billingAttribution, ...(trustedInitialResolvedSecretTraceProvenance ? { trustedInitialResolvedSecretTraceProvenance } @@ -293,6 +305,7 @@ async function executeCopilotRun(params: { childExecutionId ) runReturned = true + recordBlocksMayHaveRun(result, blocksMayHaveRun) if (registry) { await registry.importCrossingProvenance( result.executionState?.resolvedSecretTraceProvenance, @@ -309,6 +322,7 @@ async function executeCopilotRun(params: { * what threw and an execution certainly exists. */ if (runReturned) attachAttemptedExecutionId(error, childExecutionId) + recordBlocksMayHaveRun(error, blocksMayHaveRun) /** * Recovery must never replace the failure it is describing. Both steps below run only to * record and release, and either throwing would propagate a different error — one the diff --git a/apps/sim/lib/workflows/executor/execute-workflow.ts b/apps/sim/lib/workflows/executor/execute-workflow.ts index 8f498f5ac08..491834471ce 100644 --- a/apps/sim/lib/workflows/executor/execute-workflow.ts +++ b/apps/sim/lib/workflows/executor/execute-workflow.ts @@ -52,6 +52,12 @@ export interface ExecuteWorkflowOptions { useDraftState?: boolean /** Stop execution after this block completes. Used for "run until block" feature. */ stopAfterBlockId?: string + /** + * Fired once, when a block handler is first about to run. The only fact that answers + * "could a side effect have occurred", and the only one the executor can state rather + * than have inferred from the shape of a result. + */ + onBlocksMayRun?: () => void /** Run-from-block configuration using a prior execution snapshot. */ runFromBlock?: { startBlockId: string @@ -203,6 +209,7 @@ export async function executeWorkflow( base64MaxBytes: streamConfig?.base64MaxBytes, abortSignal: streamConfig?.abortSignal, stopAfterBlockId: streamConfig?.stopAfterBlockId, + onBlocksMayRun: streamConfig?.onBlocksMayRun, trustedInitialResolvedSecretTraceProvenance: streamConfig?.trustedInitialResolvedSecretTraceProvenance, runFromBlock: streamConfig?.runFromBlock, diff --git a/apps/sim/lib/workflows/executor/execution-core.ts b/apps/sim/lib/workflows/executor/execution-core.ts index a3dee66041b..994d922a13a 100644 --- a/apps/sim/lib/workflows/executor/execution-core.ts +++ b/apps/sim/lib/workflows/executor/execution-core.ts @@ -117,6 +117,7 @@ export interface ExecuteWorkflowCoreOptions { includeFileBase64?: boolean base64MaxBytes?: number stopAfterBlockId?: string + onBlocksMayRun?: () => void /** Trusted encrypted provenance captured by a server-only pre-execution boundary. */ trustedInitialResolvedSecretTraceProvenance?: ResolvedSecretTraceProvenanceV1 /** Immutable deployment admitted by the durable parent log for a resumed execution. */ @@ -973,6 +974,7 @@ async function executeWorkflowCoreImpl( */ onBlocksMayRun: () => { executorStarted = true + options.onBlocksMayRun?.() }, stream: !!onStream, selectedOutputs, From fd5039d4b58d6ee46be8257bb6bb4f21de0d7e3f Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 19:22:04 -0700 Subject: [PATCH 14/16] fix(copilot): report dispatch from the block handler itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I claimed last round that nothing could precede the signal. That was wrong: `executeNode` returns early on a cache hit, initializes loop and parallel scopes, and handles a sentinel that never reaches a handler — all after the point it fired. Both reviewers found the same thing. Move it to the line before `blockExecutor.execute`, which is the handler call. Nothing separates the two, so unlike every previous position this one cannot have something in front of it. Fired per block rather than once, since observers record a boolean and repeats cost nothing. Also accept functions as carriers of the run markers. They key a WeakMap exactly as objects do, so excluding them dropped the record for a thrown function and lost the distinction the markers exist to make. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/executor/execution/engine.ts | 16 ---------------- apps/sim/executor/orchestrators/node.ts | 10 ++++++++++ apps/sim/executor/utils/errors.ts | 3 ++- 3 files changed, 12 insertions(+), 17 deletions(-) diff --git a/apps/sim/executor/execution/engine.ts b/apps/sim/executor/execution/engine.ts index 0f5aac1e1fb..acfa7cfa42b 100644 --- a/apps/sim/executor/execution/engine.ts +++ b/apps/sim/executor/execution/engine.ts @@ -38,7 +38,6 @@ export class ExecutionEngine { private cancellationController = new AbortController() private abortSignalListener: (() => void) | null = null private cancellationUnsubscribe: (() => void) | null = null - private reportedBlocksMayRun = false private execLogger: Logger constructor( @@ -63,13 +62,6 @@ export class ExecutionEngine { this.initializeAbortHandler() } - /** Fires the caller's dispatch observer exactly once, however many nodes follow. */ - private reportBlocksMayRun(): void { - if (this.reportedBlocksMayRun) return - this.reportedBlocksMayRun = true - this.context.onBlocksMayRun?.() - } - private async subscribeToCancellationSignal(): Promise { if (!this.context.executionId) return const executionId = this.context.executionId @@ -429,14 +421,6 @@ export class ExecutionEngine { private async executeNodeAsync(nodeId: string): Promise { try { const wasAlreadyExecuted = this.context.executedBlocks.has(nodeId) - /** - * The single moment a side effect becomes possible: the last statement before a block - * handler runs. Every earlier candidate was a proxy that a reviewer could then find a - * fallible step in front of — startup, the cancellation subscription, queue and - * subflow initialization all reject having run nothing. There is nothing between here - * and the handler, so there is nothing left to be in front of. - */ - this.reportBlocksMayRun() const result = await this.nodeOrchestrator.executeNode(this.context, nodeId) if (!wasAlreadyExecuted) { diff --git a/apps/sim/executor/orchestrators/node.ts b/apps/sim/executor/orchestrators/node.ts index 56229908445..79c495a76aa 100644 --- a/apps/sim/executor/orchestrators/node.ts +++ b/apps/sim/executor/orchestrators/node.ts @@ -95,6 +95,16 @@ export class NodeExecutionOrchestrator { } } + /** + * The block handler, and therefore the first moment a side effect is possible. Every + * earlier position was a proxy with something in front of it — engine startup, the + * cancellation subscription, queue setup, and above this line a cache hit, loop and + * parallel scope initialization, and a sentinel that returns without reaching a handler. + * Nothing separates this call from the handler, so nothing can precede it. + * + * Fired per block rather than once; observers record a boolean, so repeats are free. + */ + ctx.onBlocksMayRun?.() const output = await this.blockExecutor.execute(ctx, node, node.block) const isFinalOutput = node.outgoingEdges.size === 0 return { diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index 3125f484310..711217a706b 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -102,7 +102,8 @@ export function readAttemptedExecutionId(error: unknown): string | undefined { * costs nothing today because every throw site past the dispatch boundary raises an `Error`. */ function isRecordedThrown(value: unknown): value is object { - return typeof value === 'object' && value !== null + /** Functions key a WeakMap as well as objects do, so excluding them would drop the record. */ + return (typeof value === 'object' || typeof value === 'function') && value !== null } export interface BlockExecutionErrorDetails { From 79401fd85e52c24c31c8a39bc5c6e82f0afc0906 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 19:49:33 -0700 Subject: [PATCH 15/16] refactor(copilot): decide the run phase where the caller lives, not in the executor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten review rounds chased the same question — when exactly may a side effect have occurred — through six positions in the executor, ending with a callback on every block of every execution in the product. Against 543k executions a week, serving a disclosure read about fifty times a week. The precision was never the point: `attempted` and `performed` both mean an execution exists under this id, and the caller was already handed the id that resolves it. Revert all of it. The engine, the orchestrator, both context types and the callback threading through execute-workflow and execution-core go back to staging untouched; the executor's only remaining change is the id carrier in utils/errors.ts. The phase now comes from what the copilot layer already holds. Its `try` opens on the executor call, so everything it catches is post-dispatch by construction while authorization, admission and provenance export throw past it having created nothing — no id means nothing exists, an id means resolve it. A result in hand says how the run ended, which separates cancelled and paused from completed. The harness that motivated this is now in the diff: every outcome the run path can produce, driven through the real handler and the real projection, asserted on the retry decision a caller can reach and on no run content crossing. Six mutations were used to confirm it fails for the right reasons; one of them found the dispatch flag this refactor introduced was already dead, and it is gone. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/executor/execution/executor.ts | 1 - apps/sim/executor/execution/types.ts | 10 - apps/sim/executor/orchestrators/node.ts | 10 - apps/sim/executor/types.ts | 2 - apps/sim/executor/utils/errors.ts | 21 -- .../tools/handlers/workflow/mutations.test.ts | 18 -- .../tools/handlers/workflow/mutations.ts | 47 ++-- .../workflow/withheld-run-result.test.ts | 262 ++++++++++-------- .../run-workflow-from-copilot.test.ts | 42 ++- .../application/run-workflow-from-copilot.ts | 31 +-- .../workflows/executor/execute-workflow.ts | 32 +-- .../lib/workflows/executor/execution-core.ts | 25 +- 12 files changed, 202 insertions(+), 299 deletions(-) diff --git a/apps/sim/executor/execution/executor.ts b/apps/sim/executor/execution/executor.ts index 7aec5a2786e..c567df277c8 100644 --- a/apps/sim/executor/execution/executor.ts +++ b/apps/sim/executor/execution/executor.ts @@ -415,7 +415,6 @@ export class DAGExecutor { const context: ExecutionContext = { workflowId, - onBlocksMayRun: this.contextExtensions.onBlocksMayRun, workspaceId: this.contextExtensions.workspaceId, executionId: this.contextExtensions.executionId, largeValueExecutionIds: this.contextExtensions.largeValueExecutionIds, diff --git a/apps/sim/executor/execution/types.ts b/apps/sim/executor/execution/types.ts index eb5e08b1d64..35cf038381f 100644 --- a/apps/sim/executor/execution/types.ts +++ b/apps/sim/executor/execution/types.ts @@ -238,16 +238,6 @@ export interface PiiBlockOutputRedaction { } export interface ContextExtensions { - /** - * Fired once, immediately before the engine may run a block. - * - * Everything the executor does first — DAG construction, snapshot restoration, pipeline - * assembly — can reject a request having changed nothing, so a caller that needs to know - * whether a side effect was possible cannot infer it from having called `execute`. Only - * the executor knows where that line falls, so it reports it rather than being guessed at - * from the outside. - */ - onBlocksMayRun?: () => void workspaceId?: string executionId?: string largeValueExecutionIds?: string[] diff --git a/apps/sim/executor/orchestrators/node.ts b/apps/sim/executor/orchestrators/node.ts index 79c495a76aa..56229908445 100644 --- a/apps/sim/executor/orchestrators/node.ts +++ b/apps/sim/executor/orchestrators/node.ts @@ -95,16 +95,6 @@ export class NodeExecutionOrchestrator { } } - /** - * The block handler, and therefore the first moment a side effect is possible. Every - * earlier position was a proxy with something in front of it — engine startup, the - * cancellation subscription, queue setup, and above this line a cache hit, loop and - * parallel scope initialization, and a sentinel that returns without reaching a handler. - * Nothing separates this call from the handler, so nothing can precede it. - * - * Fired per block rather than once; observers record a boolean, so repeats are free. - */ - ctx.onBlocksMayRun?.() const output = await this.blockExecutor.execute(ctx, node, node.block) const isFinalOutput = node.outgoingEdges.size === 0 return { diff --git a/apps/sim/executor/types.ts b/apps/sim/executor/types.ts index 1e3ba1c3d9c..649398ed616 100644 --- a/apps/sim/executor/types.ts +++ b/apps/sim/executor/types.ts @@ -372,8 +372,6 @@ export interface ExecutorDelegationOrigin { } export interface ExecutionContext { - /** See {@link ContextExtensions.onBlocksMayRun}. Fired by the engine, once. */ - onBlocksMayRun?: () => void workflowId: string workspaceId?: string executionId?: string diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index 711217a706b..deb1306e6ac 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -67,27 +67,6 @@ export function attachAttemptedExecutionId(error: unknown, executionId: string): attemptedExecutionIds.set(error, executionId) } -/** - * Whether a block handler ran, keyed by the outcome that carries it — a result or a throw. - * - * Recorded rather than inferred. Every property of an outcome that looks like it answers - * this is a proxy that disagrees on the paths that matter: an engine failing before its - * first block still carries an `ExecutionResult`, and a run that ends without one still ran - * every block it had. Only the executor knows, so only the executor says. - */ -const observedBlockDispatch = new WeakMap() - -/** Records the executor's answer against the outcome a caller will read it from. */ -export function recordBlocksMayHaveRun(outcome: unknown, blocksMayHaveRun: boolean): void { - if (!isRecordedThrown(outcome)) return - observedBlockDispatch.set(outcome, blocksMayHaveRun) -} - -/** Undefined when nothing observed this run, which callers must treat conservatively. */ -export function readBlocksMayHaveRun(outcome: unknown): boolean | undefined { - return isRecordedThrown(outcome) ? observedBlockDispatch.get(outcome) : undefined -} - /** Reads the dispatched-run id a thrown value carries, if dispatch was reached at all. */ export function readAttemptedExecutionId(error: unknown): string | undefined { return isRecordedThrown(error) ? attemptedExecutionIds.get(error) : undefined diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts index 759cbdcd618..9e11b107c6a 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.test.ts @@ -10,7 +10,6 @@ const { mocks } = vi.hoisted(() => ({ executeWorkflowUseCase: vi.fn(), hasExecutionResult: vi.fn(), readAttemptedExecutionId: vi.fn(), - readBlocksMayHaveRun: vi.fn(), }, })) @@ -31,7 +30,6 @@ vi.mock('@/lib/workflows/sanitization/json-sanitizer', () => ({ vi.mock('@/executor/utils/errors', () => ({ hasExecutionResult: mocks.hasExecutionResult, readAttemptedExecutionId: mocks.readAttemptedExecutionId, - readBlocksMayHaveRun: mocks.readBlocksMayHaveRun, })) vi.mock('@/lib/core/telemetry', () => ({ @@ -62,8 +60,6 @@ describe('workflow mutation Copilot adapters', () => { vi.clearAllMocks() mocks.hasExecutionResult.mockReturnValue(false) mocks.readAttemptedExecutionId.mockReturnValue(undefined) - // Default: the executor saw a block run, which is the ordinary case. - mocks.readBlocksMayHaveRun.mockReturnValue(true) }) it('maps encoded folder aliases into one create application command', async () => { @@ -299,20 +295,6 @@ describe('workflow mutation Copilot adapters', () => { run: () => executeRunWorkflow({ workflowId: 'workflow-1' }, context), effect: { phase: 'attempted', ids: { executionId: 'execution-1' } }, }, - { - label: 'refused by the engine before any block ran', - arrange: () => { - mocks.readBlocksMayHaveRun.mockReturnValue(false) - mocks.executeWorkflowUseCase.mockResolvedValueOnce({ - success: false, - output: {}, - logs: [], - metadata: { executionId: 'execution-1' }, - }) - }, - run: () => executeRunWorkflow({ workflowId: 'workflow-1' }, context), - effect: { phase: 'not_attempted', ids: { executionId: 'execution-1' } }, - }, { label: 'cancelled before it could finish', arrange: () => diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts index 5205fd64b2d..61a417d148d 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/mutations.ts @@ -29,11 +29,7 @@ import { setWorkflowBlockEnabled, } from '@/lib/workflows/application/update-workflow-content' import { sanitizeForCopilot } from '@/lib/workflows/sanitization/json-sanitizer' -import { - hasExecutionResult, - readAttemptedExecutionId, - readBlocksMayHaveRun, -} from '@/executor/utils/errors' +import { hasExecutionResult, readAttemptedExecutionId } from '@/executor/utils/errors' import type { WorkflowState } from '@/stores/workflows/workflow/types' function stripBinaryFields(value: unknown): unknown { @@ -65,18 +61,17 @@ function runRejected(error: string): ToolCallResult { } /** - * Derives the phase from two facts, neither of them guessed. + * The phase of a run whose result came back, from how that run ended. + * + * A result in hand means the executor reached a terminal state and recorded it, so the + * caller can read the whole story by id — `performed`. Cancelled and paused stopped partway + * and may have run every block, one, or none, which is exactly what `attempted` says. * - * `blocksMayHaveRun` comes from the executor and is the only thing that separates a run - * with side effects from a request that was refused; the status says whether that run - * reached the end of its work. Undefined means nothing observed the run, which is treated - * as "it may have" — over-reporting costs a lookup, under-reporting duplicates work. + * Deliberately does not separate "ran no blocks" from "ran some". Establishing that would + * take a callback on every block of every execution in the product, and buys the caller + * nothing it cannot get by resolving the id it was already handed. */ -function executionPhase( - blocksMayHaveRun: boolean | undefined, - status: ExecutionResultStatus -): ToolEffectPhase { - if (blocksMayHaveRun === false) return TOOL_EFFECT_PHASE.notAttempted +function settledPhase(status: ExecutionResultStatus): ToolEffectPhase { return status === 'cancelled' || status === 'paused' ? TOOL_EFFECT_PHASE.attempted : TOOL_EFFECT_PHASE.performed @@ -84,11 +79,6 @@ function executionPhase( type ExecutionResultStatus = 'completed' | 'paused' | 'cancelled' | undefined -/** The phase of a run that returned, from what the executor observed about it. */ -function runPhase(result: { status?: ExecutionResultStatus }): ToolEffectPhase { - return executionPhase(readBlocksMayHaveRun(result), result.status) -} - function buildExecutionOutput( result: { success: boolean @@ -123,12 +113,7 @@ function buildExecutionError(error: unknown): ToolCallResult { success: false, error: error.executionResult.error || 'Workflow execution failed', }, - /** - * Read off the error, not the spread copy above: the executor recorded its answer - * against the value it threw, and spreading makes a new object the record cannot - * follow. - */ - executionPhase(readBlocksMayHaveRun(error), error.executionResult.status) + settledPhase(error.executionResult.status) ) } logger.error('Copilot workflow execution command failed', { error }) @@ -291,7 +276,7 @@ export async function executeRunWorkflow( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result, runPhase(result)) + return buildExecutionOutput(result, settledPhase(result.status)) } catch (error) { return buildExecutionError(error) } @@ -413,7 +398,7 @@ export async function executeRunWorkflowUntilBlock( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result, runPhase(result), { + return buildExecutionOutput(result, settledPhase(result.status), { stoppedAfterBlockId: params.stopAfterBlockId, }) } catch (error) { @@ -490,7 +475,9 @@ export async function executeRunFromBlock( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result, runPhase(result), { startBlockId: params.startBlockId }) + return buildExecutionOutput(result, settledPhase(result.status), { + startBlockId: params.startBlockId, + }) } catch (error) { return buildExecutionError(error) } @@ -576,7 +563,7 @@ export async function executeRunBlock( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result, runPhase(result), { blockId: params.blockId }) + return buildExecutionOutput(result, settledPhase(result.status), { blockId: params.blockId }) } catch (error) { return buildExecutionError(error) } diff --git a/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts b/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts index 6ee744a206d..793a3185a66 100644 --- a/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts +++ b/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts @@ -1,15 +1,20 @@ /** * @vitest-environment node * - * Pins what a caller can learn about a workflow run whose result the secret-egress - * boundary withholds. + * What a caller can learn about a workflow run whose result the secret-egress boundary + * withholds. * - * The registry here is latched the way production latches it — a child run that handed - * back no provenance envelope — rather than by asserting an "unsafe" flag, so the test - * fails for the same reason the incident did. Three outcomes that need opposite retry - * decisions are driven through the real handler and the real projection: a call rejected - * on its arguments, a call that threw after dispatch, and a run that completed. All three - * used to arrive as the same sentence. + * The registry is latched the way production latches one — a child run that returned no + * provenance envelope — rather than by asserting an "unsafe" flag, so these fail for the + * same reason the incident did. Every outcome the copilot run path can produce is driven + * through the real handler and the real projection and asserted on two axes: the retry + * decision a caller can reach, which is the point of the disclosure, and that no run + * content crosses, which is the point of the boundary. + * + * The phases are deliberately coarse. `attempted` and `performed` both mean "an execution + * exists under this id". Separating "ran no blocks" from "ran some" would take a callback + * on every block of every execution in the product, and buys a caller nothing it cannot get + * by resolving the id it was handed. */ import { getErrorMessage } from '@sim/utils/errors' import { beforeEach, describe, expect, it, vi } from 'vitest' @@ -19,13 +24,11 @@ import type { ToolExecutionResult } from '@/lib/copilot/tool-executor/types' import { attachAttemptedExecutionId } from '@/executor/utils/errors' import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' -const { mocks } = vi.hoisted(() => ({ - mocks: { executeWorkflowUseCase: vi.fn() }, -})) +const { mocks } = vi.hoisted(() => ({ mocks: { executeWorkflowUseCase: vi.fn() } })) vi.mock('@/lib/copilot/application/execute-workflow-use-case', () => ({ executeCopilotWorkflowUseCase: mocks.executeWorkflowUseCase, - /** Passthrough, so a masked message is visible as masking rather than as a fallback. */ + /** Passthrough, so a masked message reads as masking rather than as a fallback. */ messageForCopilotWorkflowError: (error: unknown, fallback = 'Workflow operation failed') => getErrorMessage(error, fallback), })) @@ -34,15 +37,15 @@ vi.mock('@/lib/workflows/sanitization/json-sanitizer', () => ({ sanitizeForCopilot: vi.fn((state) => state), })) -vi.mock('@/lib/core/telemetry', () => ({ - PlatformEvents: { apiKeyGenerated: vi.fn() }, -})) +vi.mock('@/lib/core/telemetry', () => ({ PlatformEvents: { apiKeyGenerated: vi.fn() } })) import { executeRunWorkflow } from '@/lib/copilot/tools/handlers/workflow/mutations' const EXECUTION_ID = '0f4d5a4c-6a1e-4c2f-9b7d-2c8f1a3e5d90' -/** Above the 8-char substitution floor, and deliberately not shaped like any real - * provider credential — a realistic-looking fixture makes secret scanners flag this file. */ +/** + * Above the eight-character substitution floor, and deliberately not shaped like a real + * provider credential — a realistic fixture makes secret scanners flag this file. + */ const SECRET = 'fake-secret-for-test-only' const context = { @@ -66,126 +69,161 @@ async function latchedRegistry(): Promise { return registry } -async function withheld(result: ToolExecutionResult) { - const registry = await latchedRegistry() - const projection = inspectToolResultForCopilot(result, registry, 'run_workflow') - expect(projection.safe).toBe(false) - return projection +/** A run dense with the active secret, so a leak cannot pass unnoticed. */ +function secretBearingResult(extra: Record = {}) { + return { + success: true, + output: { report: `PASS ${SECRET}`, nested: { key: SECRET } }, + logs: [{ blockName: 'report', output: SECRET }], + metadata: { executionId: EXECUTION_ID, duration: 2800 }, + ...extra, + } +} + +function dispatchFailure(): Error { + const error = new Error(`crashed reading ${SECRET}`) + // What `executeCopilotRun` does once the run has been handed to the executor. + attachAttemptedExecutionId(error, EXECUTION_ID) + return error +} + +interface Outcome { + label: string + arrange: () => void + effect: string + /** Whether the caller may re-issue the call without resolving anything first. */ + safeToRetry: boolean + succeeded: boolean } -function modelOutput(result: ToolExecutionResult): Record { - expect(result.output).toBeTypeOf('object') - return result.output as Record +const OUTCOMES: Outcome[] = [ + { + label: 'refused before the executor was handed the run', + arrange: () => mocks.executeWorkflowUseCase.mockRejectedValue(new Error('Access denied')), + effect: 'not_attempted', + safeToRetry: true, + succeeded: false, + }, + { + label: 'failed after the executor was handed the run', + arrange: () => mocks.executeWorkflowUseCase.mockRejectedValue(dispatchFailure()), + effect: 'attempted', + safeToRetry: false, + succeeded: false, + }, + { + label: 'cancelled partway', + arrange: () => + mocks.executeWorkflowUseCase.mockResolvedValue( + secretBearingResult({ success: false, status: 'cancelled' }) + ), + effect: 'attempted', + safeToRetry: false, + succeeded: false, + }, + { + label: 'paused partway', + arrange: () => + mocks.executeWorkflowUseCase.mockResolvedValue( + secretBearingResult({ success: false, status: 'paused' }) + ), + effect: 'attempted', + safeToRetry: false, + succeeded: false, + }, + { + label: 'ran and failed', + arrange: () => + mocks.executeWorkflowUseCase.mockResolvedValue( + secretBearingResult({ success: false, error: `Block failed with ${SECRET}` }) + ), + effect: 'performed', + safeToRetry: false, + succeeded: false, + }, + { + label: 'ran and completed', + arrange: () => mocks.executeWorkflowUseCase.mockResolvedValue(secretBearingResult()), + effect: 'performed', + safeToRetry: false, + succeeded: true, + }, +] + +async function withhold(): Promise { + const settled = await executeRunWorkflow({ workflowId: 'wf-1' }, context) + const projection = inspectToolResultForCopilot(settled, await latchedRegistry(), 'run_workflow') + expect(projection.safe).toBe(false) + return projection.result } describe('a withheld run_workflow result', () => { beforeEach(() => { vi.clearAllMocks() + mocks.executeWorkflowUseCase.mockReset() }) - it('says nothing was created when the call never reached dispatch', async () => { + it('says nothing was created when the call never reached the use case', async () => { const rejected = await executeRunWorkflow({}, { ...context, workflowId: undefined }) expect(mocks.executeWorkflowUseCase).not.toHaveBeenCalled() - const { result } = await withheld(rejected) + const { result } = inspectToolResultForCopilot( + rejected, + await latchedRegistry(), + 'run_workflow' + ) - expect(result.success).toBe(false) - expect(modelOutput(result)).toEqual({ resultWithheld: true, effect: 'not_attempted' }) + expect(result.output).toEqual({ resultWithheld: true, effect: 'not_attempted' }) expect(result.error).toContain('nothing was created') }) - it('names the run to resolve when the call threw after dispatch', async () => { - const error = new Error(`Execution crashed reading ${SECRET}`) - attachAttemptedExecutionId(error, EXECUTION_ID) - mocks.executeWorkflowUseCase.mockRejectedValue(error) + it.each(OUTCOMES)('discloses a run that was $label', async ({ arrange, effect, succeeded }) => { + arrange() + const result = await withhold() - const failed = await executeRunWorkflow({ workflowId: 'wf-1' }, context) - const { result } = await withheld(failed) - - expect(result.success).toBe(false) - expect(modelOutput(result)).toEqual({ + expect(result.success).toBe(succeeded) + expect(result.output).toEqual({ resultWithheld: true, - effect: 'attempted', - executionId: EXECUTION_ID, + effect, + // An id is present exactly when there is something to resolve. + ...(effect === 'not_attempted' ? {} : { executionId: EXECUTION_ID }), }) - expect(result.error).toContain('At most one run exists') }) - it('names the run to read when it completed', async () => { - mocks.executeWorkflowUseCase.mockResolvedValue({ - success: true, - output: { report: `PASS ${SECRET}` }, - logs: [{ blockName: 'report', output: SECRET }], - metadata: { executionId: EXECUTION_ID, duration: 2800 }, - }) - - const completed = await executeRunWorkflow({ workflowId: 'wf-1' }, context) - const { result } = await withheld(completed) - - expect(result.success).toBe(true) - expect(modelOutput(result)).toEqual({ - resultWithheld: true, - effect: 'performed', - executionId: EXECUTION_ID, - }) - }) + it.each(OUTCOMES)('never leaks run content for a run that was $label', async ({ arrange }) => { + arrange() + const serialized = JSON.stringify(await withhold()) - /** The defect itself: these three needed opposite retry decisions and read identically. */ - it('distinguishes the three outcomes from one another', async () => { - const rejected = await executeRunWorkflow({}, { ...context, workflowId: undefined }) - - const thrown = new Error('boom') - attachAttemptedExecutionId(thrown, EXECUTION_ID) - mocks.executeWorkflowUseCase.mockRejectedValue(thrown) - const failed = await executeRunWorkflow({ workflowId: 'wf-1' }, context) - - mocks.executeWorkflowUseCase.mockReset() - mocks.executeWorkflowUseCase.mockResolvedValue({ - success: true, - output: {}, - metadata: { executionId: EXECUTION_ID, duration: 1 }, - }) - const completed = await executeRunWorkflow({ workflowId: 'wf-1' }, context) - - const views = await Promise.all( - [rejected, failed, completed].map(async (r) => JSON.stringify((await withheld(r)).result)) - ) - - expect(new Set(views).size).toBe(3) - expect(views[0]).not.toContain(EXECUTION_ID) - expect(views[1]).toContain(EXECUTION_ID) - expect(views[2]).toContain(EXECUTION_ID) + expect(serialized).not.toContain(SECRET) + expect(serialized).not.toContain('PASS') + expect(serialized).not.toContain('Block failed') + expect(serialized).not.toContain('crashed') }) - /** A frozen failure cannot take a property, and losing the id here would invite a duplicate run. */ - it('still names the run when the failure cannot be written to', async () => { - const error = Object.freeze(new Error('crashed')) - attachAttemptedExecutionId(error, EXECUTION_ID) - mocks.executeWorkflowUseCase.mockRejectedValue(error) - - const { result } = await withheld(await executeRunWorkflow({ workflowId: 'wf-1' }, context)) - - expect(modelOutput(result)).toEqual({ - resultWithheld: true, - effect: 'attempted', - executionId: EXECUTION_ID, - }) + /** + * The property the disclosure exists for: a caller can decide about retry from the + * response alone, and can never conclude "nothing happened" about a run that exists. + */ + it('lets a caller decide retry safety without resolving anything', async () => { + for (const outcome of OUTCOMES) { + mocks.executeWorkflowUseCase.mockReset() + outcome.arrange() + const output = (await withhold()).output as Record + + expect(output.effect === 'not_attempted', outcome.label).toBe(outcome.safeToRetry) + expect(Object.hasOwn(output, 'executionId'), outcome.label).toBe(!outcome.safeToRetry) + } }) - it('never lets the withheld payload carry the run content past the boundary', async () => { - mocks.executeWorkflowUseCase.mockResolvedValue({ - success: false, - output: { report: `FAIL ${SECRET}` }, - logs: [{ output: SECRET }], - error: `Block failed with ${SECRET}`, - metadata: { executionId: EXECUTION_ID, duration: 10 }, - }) - - const { result } = await withheld(await executeRunWorkflow({ workflowId: 'wf-1' }, context)) - - const serialized = JSON.stringify(result) - expect(serialized).not.toContain(SECRET) - expect(serialized).not.toContain('FAIL') - expect(serialized).not.toContain('Block failed') + /** The defect this replaced: every one of these arrived as the same sentence. */ + it('distinguishes outcomes that need different decisions', async () => { + const seen = new Set() + for (const outcome of OUTCOMES) { + mocks.executeWorkflowUseCase.mockReset() + outcome.arrange() + seen.add(JSON.stringify(await withhold())) + } + // Retry, resolve-then-decide, and read-the-result are the three distinct answers. + expect(seen.size).toBeGreaterThanOrEqual(3) }) }) 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 5101fcd5e42..4ae871e50b4 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 @@ -68,7 +68,7 @@ import { runFromBlockFromCopilot, runWorkflowFromCopilot, } from '@/lib/workflows/application/run-workflow-from-copilot' -import { attachAttemptedExecutionId, readAttemptedExecutionId } from '@/executor/utils/errors' +import { readAttemptedExecutionId } from '@/executor/utils/errors' const principal = { kind: 'delegated' as const, @@ -308,9 +308,13 @@ describe('Copilot workflow run application commands', () => { }) /** - * A caller whose result was withheld can only decide about retry from whether a run exists. - * `executeWorkflow` owns that boundary and names the run itself once it crosses it; this - * layer only covers the window it cannot see — a failure after the run already returned. + * A caller whose result was withheld decides about retry from one fact: whether a run + * exists. This layer owns that answer, because it is the last place that can distinguish + * "we never handed the work to the executor" from "we did". + * + * Deliberately coarse. A preflight refusal inside `executeWorkflow` also names the run, + * costing the caller one lookup; establishing anything finer would take a callback on + * every block of every execution in the product. */ describe('naming the run a failure belongs to', () => { const runInput = { @@ -324,21 +328,23 @@ describe('Copilot workflow run application commands', () => { const failWith = (input = runInput) => runWorkflowFromCopilot.execute({ principal, input }).catch((thrown) => thrown) - it('passes through the id executeWorkflow attached at its dispatch boundary', async () => { - mocks.executeWorkflow.mockImplementationOnce(() => { - // Exactly what the execution core does once a block could have run. - const dispatchFailure = new Error('database unavailable') - attachAttemptedExecutionId(dispatchFailure, 'child-execution-1') - throw dispatchFailure - }) + it('names the run once it has been handed to the executor', async () => { + mocks.executeWorkflow.mockRejectedValueOnce(new Error('database unavailable')) + + expect(readAttemptedExecutionId(await failWith())).toBe('child-execution-1') + }) + + it('names the run for a failure inside the executor call, whatever its cause', async () => { + mocks.executeWorkflow.mockRejectedValueOnce( + new Error('Billing attribution is required for workspace execution') + ) expect(readAttemptedExecutionId(await failWith())).toBe('child-execution-1') }) it('names the run when the crossing threw after it already returned', async () => { - // Only the post-run crossing throws; the catch re-enters this same method to - // record the failed crossing, and throwing again there would replace the very - // error the id was attached to. + // Only the post-run crossing throws; the catch re-enters this same method to record + // the failed crossing, and throwing again there would replace the error the id is on. let crossings = 0 const registry = { exportProvenanceForValue: () => undefined, @@ -356,14 +362,6 @@ describe('Copilot workflow run application commands', () => { expect(readAttemptedExecutionId(error)).toBe('child-execution-1') }) - it('names nothing when a preflight failure never reached dispatch', async () => { - mocks.executeWorkflow.mockRejectedValueOnce( - new Error('Billing attribution is required for workspace execution') - ) - - expect(readAttemptedExecutionId(await failWith())).toBeUndefined() - }) - it('names nothing when admission refused the run before it could start', async () => { mocks.admission.mockRejectedValueOnce(new Error('Usage limit exceeded')) 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 41c7761a786..6467156f445 100644 --- a/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts +++ b/apps/sim/lib/workflows/application/run-workflow-from-copilot.ts @@ -29,7 +29,7 @@ import { } from '@/lib/workflows/triggers/run-options' import type { SerializableExecutionState } from '@/executor/execution/types' import type { ExecutionResult } from '@/executor/types' -import { attachAttemptedExecutionId, recordBlocksMayHaveRun } from '@/executor/utils/errors' +import { attachAttemptedExecutionId } from '@/executor/utils/errors' const logger = createLogger('CopilotWorkflowRun') @@ -250,8 +250,17 @@ async function executeCopilotRun(params: { params.executionInput ) const completePendingActivation = registry?.beginPendingActivation() - let runReturned = false - let blocksMayHaveRun = false + /** + * 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 + * throw past this function having created nothing. That asymmetry is the whole of what a + * caller needs: no id means nothing exists, an id means resolve it before retrying. + * + * Deliberately no finer. Establishing whether a particular block ran would take a callback + * on every block of every execution in the product, to spare this one caller a lookup it + * can already make with the id it was handed. Keep the executor call first: anything + * inserted above it would be reported as a run that may exist. + */ try { const result = await executeWorkflow( { @@ -274,17 +283,6 @@ async function executeCopilotRun(params: { stopAfterBlockId: params.stopAfterBlockId, runFromBlock: params.runFromBlock, abortSignal: params.input.lifecycle.abortSignal, - /** - * Whether a block could have run, stated by the executor rather than inferred here. - * Every earlier attempt read it off the shape of the outcome — a status field, or - * whether an ExecutionResult rode along on the error — and those are proxies that - * disagree with reality on exactly the paths that matter: an engine that fails - * before its first block still carries a result, and a run that ends without one - * still ran every block it had. - */ - onBlocksMayRun: () => { - blocksMayHaveRun = true - }, billingAttribution: admission.billingAttribution, ...(trustedInitialResolvedSecretTraceProvenance ? { trustedInitialResolvedSecretTraceProvenance } @@ -304,8 +302,6 @@ async function executeCopilotRun(params: { }, childExecutionId ) - runReturned = true - recordBlocksMayHaveRun(result, blocksMayHaveRun) if (registry) { await registry.importCrossingProvenance( result.executionState?.resolvedSecretTraceProvenance, @@ -321,8 +317,7 @@ async function executeCopilotRun(params: { * cannot see: a failure after the run already returned, where the crossing import is * what threw and an execution certainly exists. */ - if (runReturned) attachAttemptedExecutionId(error, childExecutionId) - recordBlocksMayHaveRun(error, blocksMayHaveRun) + attachAttemptedExecutionId(error, childExecutionId) /** * Recovery must never replace the failure it is describing. Both steps below run only to * record and release, and either throwing would propagate a different error — one the diff --git a/apps/sim/lib/workflows/executor/execute-workflow.ts b/apps/sim/lib/workflows/executor/execute-workflow.ts index 491834471ce..8336ea332d2 100644 --- a/apps/sim/lib/workflows/executor/execute-workflow.ts +++ b/apps/sim/lib/workflows/executor/execute-workflow.ts @@ -1,6 +1,5 @@ import type { WorkflowExecutionPrincipal } from '@sim/auth/principal' import { createLogger } from '@sim/logger' -import { getErrorMessage } from '@sim/utils/errors' import { generateId } from '@sim/utils/id' import { assertBillingAttributionSnapshot, @@ -14,7 +13,6 @@ 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 { attachAttemptedExecutionId } from '@/executor/utils/errors' import type { ResolvedSecretTraceProvenanceV1 } from '@/executor/utils/resolved-secret-trace-registry' import type { CoreTriggerType } from '@/stores/logs/filters/types' @@ -52,12 +50,6 @@ export interface ExecuteWorkflowOptions { useDraftState?: boolean /** Stop execution after this block completes. Used for "run until block" feature. */ stopAfterBlockId?: string - /** - * Fired once, when a block handler is first about to run. The only fact that answers - * "could a side effect have occurred", and the only one the executor can state rather - * than have inferred from the shape of a result. - */ - onBlocksMayRun?: () => void /** Run-from-block configuration using a prior execution snapshot. */ runFromBlock?: { startBlockId: string @@ -136,7 +128,6 @@ export async function executeWorkflow( loggingSession.setTrustedExecutionCorrelation(streamConfig.trustedExecutionCorrelation) } let postExecutionOwnershipTransferred = false - let coreReturned = false try { const metadata: ExecutionMetadata = { @@ -178,12 +169,6 @@ export async function executeWorkflow( const executionStartMs = Date.now() - /** - * Once the core returns, the run happened. Everything after it here — analytics, pause - * persistence, post-execution settling — is bookkeeping that can still throw, and a - * failure there names no run unless it is marked, so an execution that really occurred - * would report itself as never started and invite a duplicate. - */ const result = await executeWorkflowCore({ snapshot, callbacks: { @@ -209,12 +194,10 @@ export async function executeWorkflow( base64MaxBytes: streamConfig?.base64MaxBytes, abortSignal: streamConfig?.abortSignal, stopAfterBlockId: streamConfig?.stopAfterBlockId, - onBlocksMayRun: streamConfig?.onBlocksMayRun, trustedInitialResolvedSecretTraceProvenance: streamConfig?.trustedInitialResolvedSecretTraceProvenance, runFromBlock: streamConfig?.runFromBlock, }) - coreReturned = true const blockTypes = [ ...new Set( @@ -258,7 +241,6 @@ export async function executeWorkflow( return result } catch (error: unknown) { - if (coreReturned) attachAttemptedExecutionId(error, executionId) const errorDiagnostic = loggingSession.projectDiagnosticError(error) logger.error(`[${requestId}] Workflow execution failed`, errorDiagnostic) @@ -280,19 +262,7 @@ export async function executeWorkflow( throw error } finally { if (!postExecutionOwnershipTransferred) { - /** - * A `finally` that throws replaces whatever the function was about to do — turning a - * successful run into an error, or an error that names its run into one that does not. - * Settling post-execution work is bookkeeping and must not be able to do either. - */ - try { - await loggingSession.waitForPostExecution() - } catch (postExecutionError) { - logger.error(`[${requestId}] Failed to settle post-execution work`, { - executionId, - error: getErrorMessage(postExecutionError), - }) - } + await loggingSession.waitForPostExecution() } } } diff --git a/apps/sim/lib/workflows/executor/execution-core.ts b/apps/sim/lib/workflows/executor/execution-core.ts index 994d922a13a..f5ec36774a7 100644 --- a/apps/sim/lib/workflows/executor/execution-core.ts +++ b/apps/sim/lib/workflows/executor/execution-core.ts @@ -49,7 +49,7 @@ import type { SerializableExecutionState, } from '@/executor/execution/types' import type { ExecutionResult, StartBlockRunMetadata } from '@/executor/types' -import { attachAttemptedExecutionId, hasExecutionResult } from '@/executor/utils/errors' +import { hasExecutionResult } from '@/executor/utils/errors' import { projectResolvedSecretDiagnosticError } from '@/executor/utils/resolved-secret-content-projection' import { createResolvedSecretTraceRegistry, @@ -117,7 +117,6 @@ export interface ExecuteWorkflowCoreOptions { includeFileBase64?: boolean base64MaxBytes?: number stopAfterBlockId?: string - onBlocksMayRun?: () => void /** Trusted encrypted provenance captured by a server-only pre-execution boundary. */ trustedInitialResolvedSecretTraceProvenance?: ResolvedSecretTraceProvenanceV1 /** Immutable deployment admitted by the durable parent log for a resumed execution. */ @@ -425,7 +424,6 @@ async function executeWorkflowCoreImpl( let processedInput = input || {} let deploymentVersionId: string | undefined let loggingStarted = false - let executorStarted = false let resolvedSecretTraceRegistry: ResolvedSecretTraceRegistry | undefined const pendingLifecycleCallbacks = new Set>() @@ -962,20 +960,6 @@ async function executeWorkflowCoreImpl( const principalSubject = resolvePrincipalSubject(metadata.principal) const contextExtensions: ContextExtensions = { - /** - * The only honest answer to "could a side effect have occurred", and the executor is - * the only thing that knows it: everything before this — DAG construction, snapshot - * restoration, pipeline assembly — can reject a request having changed nothing. - * - * The logging session was the wrong proxy in both directions. `safeStart`'s result is - * never checked, so blocks run even when it fails, reporting that nothing started for - * a run that did; and it flips before trigger resolution and serialization, reporting - * a run for failures that never reached a block. - */ - onBlocksMayRun: () => { - executorStarted = true - options.onBlocksMayRun?.() - }, stream: !!onStream, selectedOutputs, executionId, @@ -1113,13 +1097,6 @@ async function executeWorkflowCoreImpl( return result } catch (error: unknown) { - /** - * Named only once a block could have run. The thrown value is rethrown exactly as - * received, including a non-Error one, because the finalization guard below identifies - * it; a primitive therefore carries no id, which costs nothing today because every throw - * site past this point raises an Error. - */ - if (executorStarted) attachAttemptedExecutionId(error, executionId) const errorCause = describeErrorCause(error) logger.error( `[${requestId}] Execution failed:`, From 4d2ba50b984cc6200021c27466225e0d74bcf4b3 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Thu, 27 Aug 2026 20:21:27 -0700 Subject: [PATCH 16/16] docs(copilot): state that a named run may resolve to nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers read an id on a preflight failure as a defect. It is the one place this contract is deliberately coarse, so say so where each of them was looking rather than leave it to be rediscovered. `attempted` already means "zero or one executions exist under this id" — the id is a correlation key, not a promise that a row exists. A caller resolves it, finds nothing, and retries, which is the right outcome at the cost of one lookup. Buying that lookup back means an executor-side dispatch marker: a callback on every block of every execution in the product, which this branch just reverted for that reason. It would also gain nothing, since all four preflight throws are invariant violations — no workspace id, no billing attribution, no principal, attribution mismatch — and a retry fails identically. Co-Authored-By: Claude Opus 5 (1M context) --- apps/sim/lib/copilot/tool-executor/types.ts | 9 ++++++++- .../application/run-workflow-from-copilot.test.ts | 11 +++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/apps/sim/lib/copilot/tool-executor/types.ts b/apps/sim/lib/copilot/tool-executor/types.ts index 121e5a4f84e..47db6aa0d95 100644 --- a/apps/sim/lib/copilot/tool-executor/types.ts +++ b/apps/sim/lib/copilot/tool-executor/types.ts @@ -56,7 +56,14 @@ export interface ToolExecutionContext { export const TOOL_EFFECT_PHASE = { /** Rejected before anything could happen. Correcting the call and retrying is safe. */ notAttempted: 'not_attempted', - /** Dispatched; zero or one effects may exist. Resolve by id before retrying. */ + /** + * Dispatched; zero or one effects may exist. Resolve by id before retrying. + * + * Zero is a legitimate outcome here, not a defect: the id is a correlation key, not a + * promise that a row exists. Narrowing this to "a run definitely exists" would take + * per-block instrumentation across every execution in the product to spare one caller a + * lookup that answers the question definitively either way. + */ attempted: 'attempted', /** The effect ran to completion, whatever its outcome. Never retry blind. */ performed: 'performed', 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 4ae871e50b4..593516912b4 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 @@ -334,6 +334,17 @@ describe('Copilot workflow run application commands', () => { expect(readAttemptedExecutionId(await failWith())).toBe('child-execution-1') }) + /** + * Deliberate, and the one place this contract is deliberately coarse: `executeWorkflow` + * validates its own arguments before creating anything, and those failures still name + * the run. `attempted` means "zero or one executions exist under this id, resolve it", + * so the caller resolves, finds nothing, and retries — correct, at the cost of a lookup. + * + * Paying to avoid that lookup means an executor-side dispatch marker, which is a + * callback on every block of every execution in the product. It would also buy nothing: + * all four preflight throws are invariant violations — no workspace id, no billing + * attribution, no principal, attribution mismatch — so a retry fails identically. + */ it('names the run for a failure inside the executor call, whatever its cause', async () => { mocks.executeWorkflow.mockRejectedValueOnce( new Error('Billing attribution is required for workspace execution')