Skip to content

Commit 3d0be98

Browse files
committed
fix(logs): keep run identity on every remaining failure line
Fixes the type error on the external failure body, attributes a provider error payload on a 2xx to the provider, and adds workflow, execution, and block ids to the MCP failure lines that replace the block executor's. The condition batch carries its failed result's marks and passes its block id. Pi GitHub calls carry no run identity, so their errors now carry only the tool layer's attribution and the block executor still logs them with ids. Restores the HITL notification warning, which covers soft failures the tool layer never logs, and attributes refused tool and proxy URLs to the author.
1 parent 1da0d7a commit 3d0be98

6 files changed

Lines changed: 73 additions & 33 deletions

File tree

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

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,14 @@ type ConditionEvaluation =
4040
| { status: 'matched'; index: number }
4141
| { status: 'no-match' }
4242
| { status: 'expression-threw'; index: number; message: string }
43-
| { status: 'no-verdict'; message: string; retryable: boolean; timedOut: boolean }
43+
| {
44+
status: 'no-verdict'
45+
message: string
46+
retryable: boolean
47+
timedOut: boolean
48+
/** The failed tool result's output, so a terminal error carries its logged mark. */
49+
output?: unknown
50+
}
4451

4552
/**
4653
* Wraps one expression as a boolean test, on its own line so a trailing line
@@ -173,6 +180,7 @@ async function runConditionCode(
173180
userId: ctx.userId,
174181
isDeployedContext: ctx.isDeployedContext,
175182
enforceCredentialAccess: ctx.enforceCredentialAccess,
183+
blockId: currentNodeId,
176184
},
177185
},
178186
{ executionContext: ctx }
@@ -215,6 +223,7 @@ async function evaluateConditionList(
215223
message,
216224
retryable: result.retryable !== false,
217225
timedOut: isTimeoutFailure(result.error),
226+
output: result.output,
218227
}
219228
}
220229

@@ -515,8 +524,11 @@ export class ConditionBlockHandler implements BlockHandler {
515524
)
516525
case 'no-verdict':
517526
if (!evaluation.retryable) {
518-
throw new NonRetryableExecutionError(
519-
`Evaluation error in condition "${conditions[0].title}": ${evaluation.message}`
527+
throw adoptToolFailure(
528+
new NonRetryableExecutionError(
529+
`Evaluation error in condition "${conditions[0].title}": ${evaluation.message}`
530+
),
531+
evaluation
520532
)
521533
}
522534
// Retrying one branch at a time is what recovers a batch the sandbox
@@ -526,7 +538,7 @@ export class ConditionBlockHandler implements BlockHandler {
526538
// failure as it stands. The whole list was one call, so no single
527539
// branch owns that failure; name the first, where evaluation started.
528540
if (evaluation.timedOut || ctx.abortSignal?.aborted) {
529-
throw conditionError(conditions[0], evaluation.message)
541+
throw adoptToolFailure(conditionError(conditions[0], evaluation.message), evaluation)
530542
}
531543
logger.warn('Batched condition evaluation produced no verdict, retrying one at a time', {
532544
conditionCount: conditions.length,

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -561,6 +561,10 @@ export class HumanInTheLoopBlockHandler implements BlockHandler {
561561
const durationMs = Date.now() - startTime
562562

563563
if (!result.success) {
564+
logger.warn('Notification tool execution failed', {
565+
toolId,
566+
error: result.error,
567+
})
564568
return {
565569
toolId,
566570
title: toolConfig.title,

‎apps/sim/executor/handlers/pi/cloud/babysit/github.ts‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { getErrorMessage } from '@sim/utils/errors'
22
import { isRecordLike } from '@sim/utils/object'
33
import { truncate } from '@sim/utils/string'
4-
import { adoptToolFailure } from '@/lib/core/errors/failure-log'
4+
import { classifyFailure, markFailureKind } from '@/lib/core/errors/failure-log'
55
import type { BabysitRoundDecision } from '@/executor/handlers/pi/cloud/babysit/round'
66
import {
77
fetchPrSnapshot,
@@ -24,6 +24,7 @@ import type {
2424
StatusCheckRollupContext,
2525
SubmittedReviewSummary,
2626
} from '@/tools/github/types'
27+
import type { ToolResponse } from '@/tools/types'
2728

2829
const MAX_PAGES = 10
2930
const MAX_COMMENTS_PER_THREAD = 50
@@ -205,6 +206,11 @@ function toolFailure(label: string, error: unknown): Error {
205206
)
206207
}
207208

209+
/** Carries the tool layer's attribution; these calls carry no run identity, so not its logged mark. */
210+
function attributedToolFailure(label: string, result: ToolResponse): Error {
211+
return markFailureKind(toolFailure(label, result.error), classifyFailure(result.output))
212+
}
213+
208214
function parseReviewThread(value: unknown, index: number): ReviewThread {
209215
if (!isRecordLike(value)) throw new Error(`Review thread ${index} must be an object`)
210216
const commentsValue = value.comments
@@ -289,8 +295,7 @@ export async function fetchBabysitThreads(
289295
},
290296
{ signal }
291297
)
292-
if (!result.success)
293-
throw adoptToolFailure(toolFailure('Failed to fetch review threads', result.error), result)
298+
if (!result.success) throw attributedToolFailure('Failed to fetch review threads', result)
294299
const output = result.output
295300
if (!isRecordLike(output) || !Array.isArray(output.threads)) {
296301
throw new Error('Review thread response is incomplete')
@@ -425,8 +430,7 @@ export async function fetchBabysitCheckState(
425430
},
426431
{ signal }
427432
)
428-
if (!result.success)
429-
throw adoptToolFailure(toolFailure('Failed to fetch checks', result.error), result)
433+
if (!result.success) throw attributedToolFailure('Failed to fetch checks', result)
430434
const output = result.output
431435
if (!isRecordLike(output) || !Array.isArray(output.contexts)) {
432436
throw new Error('Check response is incomplete')

‎apps/sim/executor/handlers/pi/cloud/shared.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
* security-sensitive details.
66
*/
77

8-
import { adoptToolFailure } from '@/lib/core/errors/failure-log'
8+
import { classifyFailure, markFailureKind } from '@/lib/core/errors/failure-log'
99
import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
1010
import { resolvePiSandboxLifetimeMs } from '@/lib/execution/remote-sandbox/pi-lifetime'
1111
import { PI_EVENT_FILTER_PATH } from '@/executor/handlers/pi/cloud/event-filter-source'
@@ -224,9 +224,13 @@ export function scrubGitSecrets(text: string, token: string): string {
224224
}
225225

226226
/**
227-
* The error a backend throws for a failed GitHub tool call. Carries the tool layer's marks so the
228-
* failure `executeTool` already logged is not logged again at error.
227+
* The error a backend throws for a failed GitHub tool call. Carries the tool layer's attribution
228+
* but not its logged mark: these calls carry no run identity, so the block executor's line, which
229+
* does, must still be written.
229230
*/
230231
export function toolResultError(label: string, result: ToolResponse): Error {
231-
return adoptToolFailure(new Error(`${label}: ${result.error ?? 'unknown error'}`), result)
232+
return markFailureKind(
233+
new Error(`${label}: ${result.error ?? 'unknown error'}`),
234+
classifyFailure(result.output)
235+
)
232236
}

‎apps/sim/executor/handlers/pi/core/redaction.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { getErrorMessage } from '@sim/utils/errors'
2-
import { inheritFailureMarks } from '@/lib/core/errors/failure-log'
2+
import { classifyFailure, markFailureKind } from '@/lib/core/errors/failure-log'
33
import type { PiEvent } from '@/executor/handlers/pi/core/events'
44

55
/**
@@ -38,13 +38,16 @@ export function getScrubbedPiErrorMessage(
3838
}
3939

4040
/**
41-
* Creates a boundary-safe error without retaining a potentially secret-bearing cause. The failure
42-
* marks still cross, so a GitHub tool failure the tool layer logged is not logged again.
41+
* Creates a boundary-safe error without retaining a potentially secret-bearing cause. The failure's
42+
* attribution still crosses, so a GitHub 404 is not logged as a Sim fault.
4343
*/
4444
export function createScrubbedPiError(
4545
error: unknown,
4646
secrets: readonly string[],
4747
fallback?: string
4848
): Error {
49-
return inheritFailureMarks(new Error(getScrubbedPiErrorMessage(error, secrets, fallback)), error)
49+
return markFailureKind(
50+
new Error(getScrubbedPiErrorMessage(error, secrets, fallback)),
51+
classifyFailure(error)
52+
)
5053
}

‎apps/sim/tools/index.ts‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1155,12 +1155,13 @@ async function readToolResponseBody(
11551155
* is logged: an internal operation's or a Function's body carries the author's data, stdout, and
11561156
* source lines.
11571157
*/
1158-
const externalHttpFailures = new WeakSet<Error>()
1158+
const externalHttpFailures = new WeakSet<object>()
11591159

11601160
function createExternalHttpFailure(errorInfo?: ErrorInfo, extractorId?: string): Error {
11611161
const failure = createTransformedErrorFromErrorInfo(errorInfo, extractorId)
11621162
externalHttpFailures.add(failure)
1163-
return failure
1163+
/** An error payload on a 2xx carries no status but is still the provider refusing. */
1164+
return errorInfo?.status === undefined ? markFailureKind(failure, 'third_party_client') : failure
11641165
}
11651166

11661167
/**
@@ -2334,9 +2335,7 @@ async function executeToolImplementation(
23342335
: {
23352336
error: normalizedError.message,
23362337
stack: error instanceof Error ? error.stack : undefined,
2337-
...(error instanceof Error && externalHttpFailures.has(error)
2338-
? { errorData: error.data }
2339-
: {}),
2338+
...(externalHttpFailures.has(error) ? { errorData: error.data } : {}),
23402339
}),
23412340
},
23422341
resolvedSecretTraceRegistry,
@@ -2839,6 +2838,11 @@ async function executeDeclaredInternalOperation({
28392838
}
28402839
}
28412840

2841+
/** A tool or proxy URL the author configured that Sim refuses to call; the author's to fix. */
2842+
function invalidToolTarget(message: string): Error {
2843+
return markFailureKind(new Error(message), 'user')
2844+
}
2845+
28422846
/** Executes one external tool request with DNS validation and IP pinning. */
28432847
async function executeToolRequest(
28442848
toolId: string,
@@ -2856,7 +2860,7 @@ async function executeToolRequest(
28562860
const targetsThisSimInstance = isSelfOriginUrl(fullUrl)
28572861

28582862
if (targetsThisSimInstance && tool.request.allowSameOrigin !== true) {
2859-
throw new Error(SAME_ORIGIN_EXTERNAL_TOOL_ERROR_MESSAGE)
2863+
throw invalidToolTarget(SAME_ORIGIN_EXTERNAL_TOOL_ERROR_MESSAGE)
28602864
}
28612865

28622866
if (targetsThisSimInstance) {
@@ -2888,14 +2892,14 @@ async function executeToolRequest(
28882892
try {
28892893
const urlValidation = await validateUrlWithDNS(fullUrl, 'toolUrl', 'requestTarget')
28902894
if (!urlValidation.isValid) {
2891-
throw new Error(`Invalid tool URL: ${urlValidation.error}`)
2895+
throw invalidToolTarget(`Invalid tool URL: ${urlValidation.error}`)
28922896
}
28932897

28942898
let proxyOption: string | undefined
28952899
if (requestParams.proxyUrl) {
28962900
const proxyValidation = await validateAndPinProxyUrl(requestParams.proxyUrl)
28972901
if (!proxyValidation.isValid) {
2898-
throw new Error(`Invalid proxy URL: ${proxyValidation.error}`)
2902+
throw invalidToolTarget(`Invalid proxy URL: ${proxyValidation.error}`)
28992903
}
29002904
proxyOption = proxyValidation.pinnedProxyUrl
29012905
}
@@ -2916,7 +2920,7 @@ async function executeToolRequest(
29162920
? undefined
29172921
: (redirectUrl) => {
29182922
if (isSelfOriginUrl(redirectUrl)) {
2919-
throw new Error(SAME_ORIGIN_EXTERNAL_TOOL_ERROR_MESSAGE)
2923+
throw invalidToolTarget(SAME_ORIGIN_EXTERNAL_TOOL_ERROR_MESSAGE)
29202924
}
29212925
},
29222926
})
@@ -3310,6 +3314,13 @@ async function executeMcpTool(
33103314
const endTimeISO = endTime.toISOString()
33113315
const duration = endTime.getTime() - new Date(actualStartTime).getTime()
33123316

3317+
/** These lines replace the block executor's, so they carry the run identity themselves. */
3318+
const blockId = toRecord(params._context).blockId
3319+
const runIdentity = {
3320+
workflowId: context?.workflowId,
3321+
executionId: context?.executionId,
3322+
blockId: typeof blockId === 'string' ? blockId : undefined,
3323+
}
33133324
const errorMsg = toError(error).message
33143325
if (isBodySizeLimitError(errorMsg)) {
33153326
const failure = bodySizeLimitError()
@@ -3318,14 +3329,14 @@ async function executeMcpTool(
33183329
`[${actualRequestId}] Request body size limit exceeded for mcp:${toolId}:`,
33193330
failure,
33203331
{
3321-
metadata: () =>
3322-
projectToolLogMetadata(
3332+
metadata: () => ({
3333+
...runIdentity,
3334+
...projectToolLogMetadata(
33233335
{ originalError: errorMsg },
33243336
context?.resolvedSecretTraceRegistry,
3325-
{
3326-
hasOriginalError: errorMsg.length > 0,
3327-
}
3337+
{ hasOriginalError: errorMsg.length > 0 }
33283338
),
3339+
}),
33293340
}
33303341
)
33313342
return {
@@ -3342,8 +3353,9 @@ async function executeMcpTool(
33423353

33433354
const normalizedError = toError(error)
33443355
logFailureOnce(logger, `[${actualRequestId}] Error executing MCP tool ${toolId}:`, error, {
3345-
metadata: () =>
3346-
projectToolLogMetadata(
3356+
metadata: () => ({
3357+
...runIdentity,
3358+
...projectToolLogMetadata(
33473359
{
33483360
error: normalizedError.message,
33493361
stack: error instanceof Error ? error.stack : undefined,
@@ -3354,6 +3366,7 @@ async function executeMcpTool(
33543366
hasStack: Boolean(error instanceof Error && error.stack),
33553367
}
33563368
),
3369+
}),
33573370
})
33583371

33593372
const errorMessage = getErrorMessage(error, `Failed to execute MCP tool ${toolId}`)

0 commit comments

Comments
 (0)