fix(copilot): key the dispatched-run id off the failure instead of writing to it

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

Author: icecrasher321

apps/sim/executor/utils/errors.ts

@@ -36,54 +36,51 @@ 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<object, string>()
 
 /**
  * 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.
  *
  * 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 (!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<string, unknown>)[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<string, unknown> {
+function isRecordedThrown(value: unknown): value is object {
   return typeof value === 'object' && value !== null
 }
 

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,

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