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..716fbeb9ffd 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,26 @@ 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.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 9da0848f6ca..f645a5cb522 100644 --- a/apps/sim/app/api/copilot/tools/execute/route.ts +++ b/apps/sim/app/api/copilot/tools/execute/route.ts @@ -4,11 +4,13 @@ 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' import { withIncomingGoSpan } from '@/lib/copilot/request/otel' import { + describeWithholdingCause, inspectToolResultForCopilot, projectToolErrorMessageForCopilot, } from '@/lib/copilot/request/tools/resolved-secret-result' @@ -16,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' @@ -115,17 +118,43 @@ 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. + */ + logger.error('In-band egress registry unavailable; refusing the call', { toolName, toolCallId, + userId, + workspaceId, 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: 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 }, + }) } try { @@ -148,9 +177,23 @@ 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 (!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, diff --git a/apps/sim/executor/utils/errors.ts b/apps/sim/executor/utils/errors.ts index ba0d35d0ff1..deb1306e6ac 100644 --- a/apps/sim/executor/utils/errors.ts +++ b/apps/sim/executor/utils/errors.ts @@ -36,6 +36,55 @@ export function attachExecutionResult(error: Error, executionResult: ExecutionRe Object.assign(error, { executionResult }) } +/** + * 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. 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 (!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 { + 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: + * 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 isRecordedThrown(value: unknown): value is object { + /** 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 { 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..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 @@ -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,105 @@ 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: 'not-a-server-minted-id' } }, + }, + undefined, + 'run_workflow' + ) + ).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( + { + 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..65b704a06d0 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,38 @@ 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 + +/** 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) @@ -22,24 +56,101 @@ 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 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 [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 +} + +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 +165,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 +173,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 +185,7 @@ export function inspectToolResultForCopilot( } return { safe: true, result: projected } } catch { - return { safe: false, result: omittedResult(result, toolId) } + return withheld(result, registry, toolId) } } @@ -98,3 +209,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..47db6aa0d95 100644 --- a/apps/sim/lib/copilot/tool-executor/types.ts +++ b/apps/sim/lib/copilot/tool-executor/types.ts @@ -45,11 +45,52 @@ 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. + * + * 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', +} 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..9e11b107c6a 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,66 @@ 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: '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: () => + 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..61a417d148d 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,41 @@ 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) } +} + +/** + * 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. + * + * 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 settledPhase(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 @@ -46,7 +86,9 @@ function buildExecutionOutput( output?: unknown logs?: unknown[] error?: string + status?: ExecutionResultStatus }, + phase: ToolEffectPhase, extra?: Record ): ToolCallResult { return { @@ -59,21 +101,34 @@ function buildExecutionOutput( logs: stripBinaryFields(result.logs), }, error: result.success ? undefined : result.error || 'Workflow execution failed', + 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', + }, + settledPhase(error.executionResult.status) + ) } 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 +259,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 @@ -221,7 +276,7 @@ export async function executeRunWorkflow( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result) + return buildExecutionOutput(result, settledPhase(result.status)) } catch (error) { return buildExecutionError(error) } @@ -322,10 +377,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 @@ -343,7 +398,9 @@ export async function executeRunWorkflowUntilBlock( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result, { stoppedAfterBlockId: params.stopAfterBlockId }) + return buildExecutionOutput(result, settledPhase(result.status), { + stoppedAfterBlockId: params.stopAfterBlockId, + }) } catch (error) { return buildExecutionError(error) } @@ -401,10 +458,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 @@ -418,7 +475,9 @@ export async function executeRunFromBlock( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(result, { startBlockId: params.startBlockId }) + return buildExecutionOutput(result, settledPhase(result.status), { + startBlockId: params.startBlockId, + }) } catch (error) { return buildExecutionError(error) } @@ -487,10 +546,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 @@ -504,7 +563,7 @@ export async function executeRunBlock( lifecycle: copilotRunLifecycle(context), }) - return buildExecutionOutput(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 new file mode 100644 index 00000000000..793a3185a66 --- /dev/null +++ b/apps/sim/lib/copilot/tools/handlers/workflow/withheld-run-result.test.ts @@ -0,0 +1,229 @@ +/** + * @vitest-environment node + * + * What a caller can learn about a workflow run whose result the secret-egress boundary + * withholds. + * + * 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' +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 reads as masking rather than as a fallback. */ + messageForCopilotWorkflowError: (error: unknown, fallback = 'Workflow operation failed') => + getErrorMessage(error, 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' +/** + * 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 = { + 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 +} + +/** 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 +} + +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 the use case', async () => { + const rejected = await executeRunWorkflow({}, { ...context, workflowId: undefined }) + expect(mocks.executeWorkflowUseCase).not.toHaveBeenCalled() + + const { result } = inspectToolResultForCopilot( + rejected, + await latchedRegistry(), + 'run_workflow' + ) + + expect(result.output).toEqual({ resultWithheld: true, effect: 'not_attempted' }) + expect(result.error).toContain('nothing was created') + }) + + it.each(OUTCOMES)('discloses a run that was $label', async ({ arrange, effect, succeeded }) => { + arrange() + const result = await withhold() + + expect(result.success).toBe(succeeded) + expect(result.output).toEqual({ + resultWithheld: true, + effect, + // An id is present exactly when there is something to resolve. + ...(effect === 'not_attempted' ? {} : { 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()) + + expect(serialized).not.toContain(SECRET) + expect(serialized).not.toContain('PASS') + expect(serialized).not.toContain('Block failed') + expect(serialized).not.toContain('crashed') + }) + + /** + * 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) + } + }) + + /** 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/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}`) } 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..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 @@ -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,89 @@ describe('Copilot workflow run application commands', () => { }) ).rejects.toThrow('database unavailable') }) + + /** + * 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 = { + workflowId: 'workflow-1', + useDraftState: true, + lifecycle, + hasWorkflowInput: false, + useMockPayload: true, + } + + const failWith = (input = runInput) => + runWorkflowFromCopilot.execute({ principal, input }).catch((thrown) => thrown) + + 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') + }) + + /** + * 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') + ) + + 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 error the id is on. + 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 admission refused the run before it could start', async () => { + mocks.admission.mockRejectedValueOnce(new Error('Usage limit exceeded')) + + const error = await failWith() + + 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 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 32a4066041e..6467156f445 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' @@ -28,6 +29,10 @@ 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' + +const logger = createLogger('CopilotWorkflowRun') + import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry' export interface CopilotWorkflowRunLifecycle { @@ -245,6 +250,17 @@ async function executeCopilotRun(params: { params.executionInput ) const completePendingActivation = registry?.beginPendingActivation() + /** + * 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( { @@ -295,6 +311,19 @@ async function executeCopilotRun(params: { } return result } catch (error) { + /** + * `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) + /** + * 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' && @@ -303,18 +332,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?.() 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')