fix(copilot): name the dispatched run from the boundary that owns it

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) <noreply@anthropic.com>

Author: Vikhyath Mondreti

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<string, unknown>)[ATTEMPTED_EXECUTION_ID]
+  if (!isAttachableThrown(error) || !(ATTEMPTED_EXECUTION_ID in error)) return undefined
+  const value = (error as Record<string, unknown>)[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<string, unknown> {
+  return typeof value === 'object' && value !== null
+}
+
 export interface BlockExecutionErrorDetails {
   block: SerializedBlock
   error: Error | string

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

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()

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' &&

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)
 

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')