From e410fc78b09afa07f5ffadef4b01ca7430bc59c8 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 19 Oct 2026 13:04:52 -0700 Subject: [PATCH 1/2] fix(execution): precheck the default video target and attribute 4xx resume preprocessing refusals to the run --- apps/sim/app/api/mcp/tools/discover/route.ts | 6 +++--- .../tools/server/media/generate-video.ts | 15 ++++++-------- .../server/media/model-boundaries.test.ts | 2 ++ .../write-target-precheck.integration.ts | 16 +++++++++++++++ .../human-in-the-loop-manager.test.ts | 20 +++++++++++++++++++ .../executor/human-in-the-loop-manager.ts | 6 +++++- 6 files changed, 52 insertions(+), 13 deletions(-) diff --git a/apps/sim/app/api/mcp/tools/discover/route.ts b/apps/sim/app/api/mcp/tools/discover/route.ts index c6ed21a0dd1..dded0044831 100644 --- a/apps/sim/app/api/mcp/tools/discover/route.ts +++ b/apps/sim/app/api/mcp/tools/discover/route.ts @@ -32,9 +32,9 @@ export const dynamic = 'force-dynamic' * reached, times out on connect or on `tools/list`, answers with a protocol or HTTP * error, or is cooling down after a recent failure is a fault of that server rather * than of Sim: it answers 422 (the request was valid; the dependency it names cannot - * serve it), the status this API family already uses for an upstream that cannot - * complete the operation, and logs at warn. Anything else keeps {@link categorizeError}'s - * status and logs at error only when that status is a 5xx. + * serve it), the status `oauth/start` answers when a server's authorization server + * refuses it, and logs at warn. The v2 surface keeps its documented 503. Anything else + * keeps {@link categorizeError}'s status and logs at error only when that status is a 5xx. */ function discoveryFailureResponse(requestId: string, error: unknown) { if (isMcpServerUnavailableError(error)) { diff --git a/apps/sim/lib/mothership/tools/server/media/generate-video.ts b/apps/sim/lib/mothership/tools/server/media/generate-video.ts index 680518350fe..71e2f216e40 100644 --- a/apps/sim/lib/mothership/tools/server/media/generate-video.ts +++ b/apps/sim/lib/mothership/tools/server/media/generate-video.ts @@ -68,12 +68,12 @@ export const generateVideoServerTool: BaseServerTool recordServiceCost('falai_video', cost.costDollars), }) - const outputPath = outputFile?.path || 'files/generated-video.mp4' - const mode = outputFile?.mode ?? 'create' - assertServerToolNotAborted(context) // The prompt is the only secret-bearing input; recording its provenance // keeps the written file model-readable (see generate-image). diff --git a/apps/sim/lib/mothership/tools/server/media/model-boundaries.test.ts b/apps/sim/lib/mothership/tools/server/media/model-boundaries.test.ts index 96f62494678..d1e9d401050 100644 --- a/apps/sim/lib/mothership/tools/server/media/model-boundaries.test.ts +++ b/apps/sim/lib/mothership/tools/server/media/model-boundaries.test.ts @@ -21,6 +21,7 @@ vi.mock('@google/genai', () => ({ })) vi.mock('@/lib/core/config/api-keys', () => ({ getRotatingApiKey: vi.fn(() => 'api-key') })) vi.mock('@/lib/mothership/vfs/resource-writer', () => ({ + validateCopilotWorkspaceFileWriteTarget: vi.fn(), writeCopilotWorkspaceFileByPath: mockWriteWorkspaceFileByPath, })) vi.mock('@/lib/media/falai-audio', () => ({ generateFalAudio: mockGenerateFalAudio })) @@ -118,6 +119,7 @@ describe('Mothership media model boundaries', () => { generator().mockImplementationOnce( ({ signal }: { signal?: AbortSignal }) => new Promise((_resolve, reject) => { + if (signal?.aborted) return reject(signal.reason) signal?.addEventListener('abort', () => reject(signal.reason), { once: true }) }) ) diff --git a/apps/sim/lib/mothership/tools/server/media/write-target-precheck.integration.ts b/apps/sim/lib/mothership/tools/server/media/write-target-precheck.integration.ts index 2456b795998..f8505521287 100644 --- a/apps/sim/lib/mothership/tools/server/media/write-target-precheck.integration.ts +++ b/apps/sim/lib/mothership/tools/server/media/write-target-precheck.integration.ts @@ -181,4 +181,20 @@ describe('media generation output target precheck', () => { expect(written).toEqual([{ name: file }]) } ) + + it('refuses a taken default video target before generating or billing', async () => { + const { context } = await seedMovedFolder() + const first = await generateVideoServerTool.execute({ prompt: 'p' }, context) + expect(first.success, first.message).toBe(true) + mockGenerateFalVideo.mockClear() + const charges = vi.fn().mockResolvedValue(undefined) + + const second = await observeServiceCosts(charges, () => + generateVideoServerTool.execute({ prompt: 'p' }, context) + ) + + expect(second.success).toBe(false) + expect(mockGenerateFalVideo).not.toHaveBeenCalled() + expect(charges).not.toHaveBeenCalled() + }) }) diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts index 5711a55b671..d976b8fdcff 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts @@ -2273,6 +2273,26 @@ describe('repeated human review pauses', () => { }) } + it.each([ + { statusCode: 402, kind: 'user' }, + { statusCode: 404, kind: 'user' }, + { statusCode: 429, kind: 'user' }, + { statusCode: 500, kind: 'internal' }, + ])('attributes a $statusCode preprocessing refusal as $kind', async ({ statusCode, kind }) => { + dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'log-1', deploymentVersionId: null }]) + mockPreprocessExecution.mockResolvedValueOnce({ + success: false, + error: { message: 'refused', statusCode }, + }) + + const error = await runResumeExecution(createRepeatedReviewResumeArgs()).catch( + (thrown: unknown) => thrown + ) + + expect(error).toMatchObject({ name: 'ResumeAdmissionError', statusCode }) + expect(classifyFailure(error)).toBe(kind) + }) + it('keeps the next pause snapshot attached to the log that admission claimed', async () => { mockCompletedResumeRun() diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts index b8bfe1f744c..b83d53902f4 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts @@ -1561,11 +1561,15 @@ export class PauseResumeManager { error: errorMessage, }) - throw new ResumeAdmissionError( + const refusal = new ResumeAdmissionError( errorMessage, preprocessingResult.error?.statusCode ?? 500, preprocessingResult.error?.retryable ?? false ) + /** A 4xx is the run's own state (usage limit, rate limit, archived workflow), not a Sim fault. */ + throw refusal.statusCode >= 400 && refusal.statusCode < 500 + ? markFailureKind(refusal, 'user') + : refusal } logger.info('Preprocessing checks passed for resume', { From c2524bb0fa3cb5d5d759a40d1ac73fec768e4cde Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Mon, 19 Oct 2026 13:22:15 -0700 Subject: [PATCH 2/2] fix(resume): keep retryable preprocessing refusals internal and log a run-state refusal once at info --- .../human-in-the-loop-manager.test.ts | 37 +++++++++++-------- .../executor/human-in-the-loop-manager.ts | 25 +++++++------ 2 files changed, 35 insertions(+), 27 deletions(-) diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts index d976b8fdcff..d6f07c4a21d 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts @@ -2274,24 +2274,29 @@ describe('repeated human review pauses', () => { } it.each([ - { statusCode: 402, kind: 'user' }, - { statusCode: 404, kind: 'user' }, - { statusCode: 429, kind: 'user' }, - { statusCode: 500, kind: 'internal' }, - ])('attributes a $statusCode preprocessing refusal as $kind', async ({ statusCode, kind }) => { - dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'log-1', deploymentVersionId: null }]) - mockPreprocessExecution.mockResolvedValueOnce({ - success: false, - error: { message: 'refused', statusCode }, - }) + { statusCode: 402, retryable: false, kind: 'user' }, + { statusCode: 404, retryable: false, kind: 'user' }, + { statusCode: 429, retryable: false, kind: 'user' }, + /** An unreadable usage ledger fails closed with a retryable 402: no verdict on the payer. */ + { statusCode: 402, retryable: true, kind: 'internal' }, + { statusCode: 500, retryable: false, kind: 'internal' }, + ])( + 'attributes a $statusCode (retryable: $retryable) preprocessing refusal as $kind', + async ({ statusCode, retryable, kind }) => { + dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'log-1', deploymentVersionId: null }]) + mockPreprocessExecution.mockResolvedValueOnce({ + success: false, + error: { message: 'refused', statusCode, retryable }, + }) - const error = await runResumeExecution(createRepeatedReviewResumeArgs()).catch( - (thrown: unknown) => thrown - ) + const error = await runResumeExecution(createRepeatedReviewResumeArgs()).catch( + (thrown: unknown) => thrown + ) - expect(error).toMatchObject({ name: 'ResumeAdmissionError', statusCode }) - expect(classifyFailure(error)).toBe(kind) - }) + expect(error).toMatchObject({ name: 'ResumeAdmissionError', statusCode }) + expect(classifyFailure(error)).toBe(kind) + } + ) it('keeps the next pause snapshot attached to the log that admission claimed', async () => { mockCompletedResumeRun() diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts index b83d53902f4..a4963191ec4 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts @@ -1554,22 +1554,25 @@ export class PauseResumeManager { if (!preprocessingResult.success) { const errorMessage = preprocessingResult.error?.message || 'Preprocessing check failed for resume execution' - logger.error('Resume preprocessing failed', { - resumeExecutionId, - workflowId: pausedExecution.workflowId, - userId: effectiveUserId, - error: errorMessage, - }) - const refusal = new ResumeAdmissionError( errorMessage, preprocessingResult.error?.statusCode ?? 500, preprocessingResult.error?.retryable ?? false ) - /** A 4xx is the run's own state (usage limit, rate limit, archived workflow), not a Sim fault. */ - throw refusal.statusCode >= 400 && refusal.statusCode < 500 - ? markFailureKind(refusal, 'user') - : refusal + /** + * A final 4xx is a verdict on the run (usage limit, rate limit, archived workflow). A + * retryable one, such as an unreadable usage ledger, is no verdict on the caller and stays + * a Sim failure. + */ + const refusedByRunState = + refusal.statusCode >= 400 && refusal.statusCode < 500 && !refusal.retryable + logger[refusedByRunState ? 'info' : 'error']('Resume preprocessing failed', { + resumeExecutionId, + workflowId: pausedExecution.workflowId, + userId: effectiveUserId, + error: errorMessage, + }) + throw refusedByRunState ? markFailureKind(refusal, 'user') : refusal } logger.info('Preprocessing checks passed for resume', {