Skip to content

Commit 934d122

Browse files
committed
test(logs): attribute the serializer refusal at its owner and pin block log redaction
Moves the missing-required-fields attribution check to the serializer that throws it, drops an execution-core case whose internal row passed by default, and covers the block failure line's secret projection now that it, not the Agent handler, logs provider errors. Tightens comments the diff added.
1 parent 4e66f3b commit 934d122

9 files changed

Lines changed: 59 additions & 40 deletions

File tree

‎apps/sim/executor/execution/block-executor.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,48 @@ describe('BlockExecutor', () => {
622622
expect(blockFailureLogsSince(loggerIndex)).toHaveLength(2)
623623
})
624624

625+
it.each([
626+
['projects secrets and runtime identifiers out of', false],
627+
['fails closed to a structural', true],
628+
] as const)('%s the block failure line for a provider error', async (_name, incomplete) => {
629+
const secret = 'block-failure-secret'
630+
/** The Agent handler hands its provider error registry to the block executor this way. */
631+
const errorRegistry = new ResolvedSecretTraceRegistry([
632+
{ name: 'TOKEN', plaintext: secret, encryptedValue: 'encrypted-block-failure-secret' },
633+
])
634+
errorRegistry.recordResolved('TOKEN', secret)
635+
if (incomplete) errorRegistry.markIncomplete('unspecified')
636+
const block = createBlock()
637+
const workflow: SerializedWorkflow = {
638+
version: '1',
639+
blocks: [block],
640+
connections: [],
641+
loops: {},
642+
parallels: {},
643+
}
644+
const loggerIndex = blockExecutorBaseLogger.withMetadata.mock.results.length
645+
const state = new ExecutionState()
646+
const handler: BlockHandler = {
647+
canHandle: () => true,
648+
execute: async (ctx) => {
649+
ctx.errorResolvedSecretTraceRegistry = errorRegistry
650+
throw new Error(`provider failed with ${secret} __var_TOKEN __sim_runtime_test_1`)
651+
},
652+
}
653+
const executor = new BlockExecutor(
654+
[handler],
655+
new VariableResolver(workflow, {}, state),
656+
{},
657+
state
658+
)
659+
660+
await executor.execute(createContext(state), createNode(block), block).catch(() => undefined)
661+
662+
const logged = JSON.stringify(blockFailureLogsSince(loggerIndex))
663+
expect(logged).toContain('Block execution failed')
664+
for (const leaked of [secret, '__var_', '__sim_']) expect(logged).not.toContain(leaked)
665+
})
666+
625667
it('logs an internal child workflow fault with the block and run identity', async () => {
626668
const loggerIndex = blockExecutorBaseLogger.withMetadata.mock.results.length
627669

‎apps/sim/executor/execution/block-executor.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -789,7 +789,7 @@ export class BlockExecutor {
789789
}
790790
}
791791

792-
/** Projected only when logged: a tool failure arrives already logged and skips it. */
792+
/** Lazy, so a failure a tool already logged skips the secret projection. */
793793
let errorDiagnostic: Record<string, unknown> | undefined
794794
const getErrorDiagnostic = () => {
795795
if (errorDiagnostic) return errorDiagnostic
@@ -887,7 +887,7 @@ export class BlockExecutor {
887887
executionTime: duration,
888888
},
889889
})
890-
/** A thrown primitive has no `cause` link back to the value logged above. */
890+
/** The raw thrown value is never marked logged, so the fresh block error carries the mark. */
891891
markFailureLogged(blockError)
892892
throw blockError
893893
}

‎apps/sim/executor/handlers/agent/agent-handler.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,6 @@ function describeProviderTransportFailure(error: Error): string | null {
327327
return null
328328
}
329329

330-
/** A provider refusing the key it was given. */
331330
function isProviderKeyRejection(error: unknown): boolean {
332331
const status = (error as { status?: unknown } | null)?.status
333332
return status === 401 || status === 402 || status === 403

‎apps/sim/executor/handlers/human-in-the-loop/human-in-the-loop-handler.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -560,7 +560,6 @@ export class HumanInTheLoopBlockHandler implements BlockHandler {
560560
const result = await executeTool(toolId, toolParams, { executionContext: ctx })
561561
const durationMs = Date.now() - startTime
562562

563-
/** `executeTool` already logged the failure with its tool id and attribution. */
564563
if (!result.success) {
565564
return {
566565
toolId,

‎apps/sim/executor/handlers/workflow/workflow-handler.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1200,7 +1200,6 @@ export class WorkflowBlockHandler implements BlockHandler {
12001200
? `Custom block execution failed (ref: ${ref})`
12011201
: 'Custom block execution failed'
12021202

1203-
/** No `cause` crosses the boundary, so the failure's marks are carried explicitly. */
12041203
return inheritFailureMarks(
12051204
new ChildWorkflowError({
12061205
message,

‎apps/sim/lib/core/errors/failure-log.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,9 @@ export function markFailureKind<T>(error: T, kind: FailureKind): T {
5757
}
5858

5959
/**
60-
* Attributes `error` from its cause chain. A database failure anywhere is always internal,
61-
* then the outermost link with an explicit mark, a {@link UserFailure}, a Sim `HttpError` status,
62-
* or an upstream `status` decides. Anything unattributed is internal.
60+
* Attributes `error` from its cause chain. A database or retryable setup failure is always
61+
* internal, then the outermost link with an explicit mark, a {@link UserFailure}, a Sim `HttpError`
62+
* status, or an upstream `status` decides. Anything unattributed is internal.
6363
*/
6464
export function classifyFailure(error: unknown): FailureKind {
6565
if (findDatabaseQueryError(error)) return 'internal'

‎apps/sim/lib/workflows/executor/execution-core.test.ts‎

Lines changed: 0 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -160,13 +160,11 @@ vi.mock('@/serializer', () => ({
160160
},
161161
}))
162162

163-
import { classifyFailure } from '@/lib/core/errors/failure-log'
164163
import {
165164
executeWorkflowCore,
166165
FINALIZED_EXECUTION_ID_TTL_MS,
167166
wasExecutionFinalizedByCore,
168167
} from '@/lib/workflows/executor/execution-core'
169-
import { MissingRequiredFieldsError } from '@/serializer/errors'
170168

171169
const uploadWorkflowInputMock = uploadsExecutionMockFns.mockUploadExecutionFile
172170
largeValueMetadataMockFns.mockRegisterLargeValueOwner.mockResolvedValue(true)
@@ -703,32 +701,6 @@ describe('executeWorkflowCore terminal finalization sequencing', () => {
703701
)
704702
})
705703

706-
it.each([
707-
[
708-
'a block missing a required field',
709-
new MissingRequiredFieldsError('Slack', ['Slack Account']),
710-
'user',
711-
],
712-
[
713-
'an unknown block type, a registry regression',
714-
new Error('Invalid block type: retired'),
715-
'internal',
716-
],
717-
] as const)('attributes a serializer refusal for %s', async (_name, refusal, kind) => {
718-
serializeWorkflowMock.mockImplementationOnce(() => {
719-
throw refusal
720-
})
721-
722-
const thrown = await executeWorkflowCore({
723-
snapshot: createSnapshot() as any,
724-
callbacks: {},
725-
loggingSession: loggingSession as any,
726-
}).catch((error: unknown) => error)
727-
728-
expect(thrown).toBe(refusal)
729-
expect(classifyFailure(thrown)).toBe(kind)
730-
})
731-
732704
it('activates trusted pre-execution provenance on the installed execution registry', async () => {
733705
executorExecuteMock.mockResolvedValue({
734706
success: true,

‎apps/sim/serializer/index.test.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import {
2323
toolsUtilsMock,
2424
} from '@sim/testing/mocks'
2525
import { describe, expect, it, vi } from 'vitest'
26+
import { classifyFailure } from '@/lib/core/errors/failure-log'
2627
import { DAGBuilder } from '@/executor/dag/builder'
2728
import { Serializer } from '@/serializer/index'
2829
import { getToolMetadata, getToolParams } from '@/tools/metadata'
@@ -356,15 +357,22 @@ describe('Serializer', () => {
356357
enabled: true,
357358
}
358359

359-
expect(() => {
360+
let refusal: unknown
361+
try {
360362
serializer.serializeWorkflow(
361363
{ 'test-block': blockWithMissingUserOnlyField },
362364
[],
363365
{},
364366
undefined,
365367
true
366368
)
367-
}).toThrow('Test Jina Block is missing required fields: API Key')
369+
} catch (error) {
370+
refusal = error
371+
}
372+
expect(refusal).toBeInstanceOf(Error)
373+
expect((refusal as Error).message).toBe('Test Jina Block is missing required fields: API Key')
374+
/** The author's configuration, so execution logs it at info rather than paging at error. */
375+
expect(classifyFailure(refusal)).toBe('user')
368376
})
369377

370378
it.concurrent('should not validate user-or-llm fields during serialization', () => {

‎apps/sim/tools/index.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1059,7 +1059,7 @@ const RESPONSE_SIZE_LIMIT_ERROR_MESSAGE =
10591059
const SAME_ORIGIN_EXTERNAL_TOOL_ERROR_MESSAGE =
10601060
'External integration tools cannot target this Sim instance; use an internal operation'
10611061

1062-
/** The author's workflow data is too large to send; their fault, logged once by the tool catch. */
1062+
/** The author's data is too large to send, so it is a user failure. */
10631063
function bodySizeLimitError(): Error {
10641064
return markFailureKind(new Error(BODY_SIZE_LIMIT_ERROR_MESSAGE), 'user')
10651065
}
@@ -2424,7 +2424,7 @@ async function executeToolImplementation(
24242424
...errorDetails,
24252425
...(functionSandboxCost ? { cost: functionSandboxCost } : {}),
24262426
}
2427-
/** A handler rebuilding this result as a thrown error carries `output`, and with it both marks. */
2427+
/** Lets `adoptToolFailure` carry both marks onto a handler's rebuilt error. */
24282428
markFailureLogged(failureOutput)
24292429
markFailureKind(failureOutput, loggedKind ?? classifyFailure(error))
24302430
return {

0 commit comments

Comments
 (0)