Skip to content

Commit 5701bd5

Browse files
hannesrudolphhannesrudolph
committed
fix: remove silent error swallowing and retry band-aid
- Replace all 11 .catch(() => {}) on presentAssistantMessage() calls with error-logging handlers that log non-abort errors via console.error - Remove 500ms retry band-aid in AttemptCompletionTool.delegateToParent(); simplify to single attemptDelegation() call with parent repair on failure - Remove retry test and fake timers from attemptCompletionTool.spec.ts
1 parent 7a8bf0d commit 5701bd5

File tree

4 files changed

+61
-55
lines changed

4 files changed

+61
-55
lines changed

‎src/core/assistant-message/presentAssistantMessage.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -974,7 +974,11 @@ export async function presentAssistantMessage(cline: Task) {
974974
if (cline.currentStreamingContentIndex < cline.assistantMessageContent.length) {
975975
// There are already more content blocks to stream, so we'll call
976976
// this function ourselves.
977-
presentAssistantMessage(cline).catch(() => {})
977+
presentAssistantMessage(cline).catch((err) => {
978+
if (!cline.abort) {
979+
console.error("[presentAssistantMessage] Unhandled error:", err)
980+
}
981+
})
978982
return
979983
} else {
980984
// CRITICAL FIX: If we're out of bounds and the stream is complete, set userMessageContentReady
@@ -987,7 +991,11 @@ export async function presentAssistantMessage(cline: Task) {
987991

988992
// Block is partial, but the read stream may have finished.
989993
if (cline.presentAssistantMessageHasPendingUpdates) {
990-
presentAssistantMessage(cline).catch(() => {})
994+
presentAssistantMessage(cline).catch((err) => {
995+
if (!cline.abort) {
996+
console.error("[presentAssistantMessage] Unhandled error:", err)
997+
}
998+
})
991999
}
9921000
}
9931001

‎src/core/task/Task.ts‎

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -464,7 +464,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
464464
// Add to content and present
465465
this.assistantMessageContent.push(partialToolUse)
466466
this.userMessageContentReady = false
467-
presentAssistantMessage(this).catch(() => {})
467+
presentAssistantMessage(this).catch((err) => {
468+
if (!this.abort) {
469+
console.error("[presentAssistantMessage] Unhandled error:", err)
470+
}
471+
})
468472
} else if (event.type === "tool_call_delta") {
469473
// Process chunk using streaming JSON parser
470474
const partialToolUse = NativeToolCallParser.processStreamingChunk(event.id, event.delta)
@@ -480,7 +484,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
480484
this.assistantMessageContent[toolUseIndex] = partialToolUse
481485

482486
// Present updated tool use
483-
presentAssistantMessage(this).catch(() => {})
487+
presentAssistantMessage(this).catch((err) => {
488+
if (!this.abort) {
489+
console.error("[presentAssistantMessage] Unhandled error:", err)
490+
}
491+
})
484492
}
485493
}
486494
} else if (event.type === "tool_call_end") {
@@ -506,7 +514,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
506514
this.userMessageContentReady = false
507515

508516
// Present the finalized tool call
509-
presentAssistantMessage(this).catch(() => {})
517+
presentAssistantMessage(this).catch((err) => {
518+
if (!this.abort) {
519+
console.error("[presentAssistantMessage] Unhandled error:", err)
520+
}
521+
})
510522
} else if (toolUseIndex !== undefined) {
511523
// finalizeStreamingToolCall returned null (malformed JSON or missing args)
512524
// Mark the tool as non-partial so it's presented as complete, but execution
@@ -525,7 +537,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
525537
this.userMessageContentReady = false
526538

527539
// Present the tool call - validation will handle missing params
528-
presentAssistantMessage(this).catch(() => {})
540+
presentAssistantMessage(this).catch((err) => {
541+
if (!this.abort) {
542+
console.error("[presentAssistantMessage] Unhandled error:", err)
543+
}
544+
})
529545
}
530546
}
531547
}
@@ -3111,7 +3127,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
31113127

31123128
// Present the tool call to user - presentAssistantMessage will execute
31133129
// tools sequentially and accumulate all results in userMessageContent
3114-
presentAssistantMessage(this).catch(() => {})
3130+
presentAssistantMessage(this).catch((err) => {
3131+
if (!this.abort) {
3132+
console.error("[presentAssistantMessage] Unhandled error:", err)
3133+
}
3134+
})
31153135
break
31163136
}
31173137
case "text": {
@@ -3130,7 +3150,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
31303150
})
31313151
this.userMessageContentReady = false
31323152
}
3133-
presentAssistantMessage(this).catch(() => {})
3153+
presentAssistantMessage(this).catch((err) => {
3154+
if (!this.abort) {
3155+
console.error("[presentAssistantMessage] Unhandled error:", err)
3156+
}
3157+
})
31343158
break
31353159
}
31363160
case "response_message":
@@ -3457,7 +3481,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
34573481
this.userMessageContentReady = false
34583482

34593483
// Present the finalized tool call
3460-
presentAssistantMessage(this).catch(() => {})
3484+
presentAssistantMessage(this).catch((err) => {
3485+
if (!this.abort) {
3486+
console.error("[presentAssistantMessage] Unhandled error:", err)
3487+
}
3488+
})
34613489
} else if (toolUseIndex !== undefined) {
34623490
// finalizeStreamingToolCall returned null (malformed JSON or missing args)
34633491
// We still need to mark the tool as non-partial so it gets executed
@@ -3476,7 +3504,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
34763504
this.userMessageContentReady = false
34773505

34783506
// Present the tool call - validation will handle missing params
3479-
presentAssistantMessage(this).catch(() => {})
3507+
presentAssistantMessage(this).catch((err) => {
3508+
if (!this.abort) {
3509+
console.error("[presentAssistantMessage] Unhandled error:", err)
3510+
}
3511+
})
34803512
}
34813513
}
34823514
}
@@ -3710,7 +3742,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
37103742
// If there is content to update then it will complete and
37113743
// update `this.userMessageContentReady` to true, which we
37123744
// `pWaitFor` before making the next request.
3713-
presentAssistantMessage(this).catch(() => {})
3745+
presentAssistantMessage(this).catch((err) => {
3746+
if (!this.abort) {
3747+
console.error("[presentAssistantMessage] Unhandled error:", err)
3748+
}
3749+
})
37143750
}
37153751

37163752
if (hasTextContent || hasToolUses) {

‎src/core/tools/AttemptCompletionTool.ts‎

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -182,26 +182,13 @@ export class AttemptCompletionTool extends BaseTool<"attempt_completion"> {
182182
}
183183

184184
try {
185-
try {
186-
await attemptDelegation()
187-
} catch (firstError) {
188-
// Retry once after a short delay to handle transient races
189-
console.warn(
190-
`[AttemptCompletionTool] First delegation attempt failed for task ${childTaskId}, retrying: ${
191-
firstError instanceof Error ? firstError.message : String(firstError)
192-
}`,
193-
)
194-
await new Promise((resolve) => setTimeout(resolve, 500))
195-
await attemptDelegation()
196-
}
197-
198-
// Only push tool result after successful delegation
185+
await attemptDelegation()
199186
pushToolResult("")
200187
return true
201188
} catch (error) {
202-
// Both attempts failed — repair parent status so it doesn't stay "delegated"
189+
// Delegation failed — repair parent status so it doesn't stay "delegated"
203190
console.error(
204-
`[AttemptCompletionTool] Delegation failed after retry for task ${childTaskId}: ${
191+
`[AttemptCompletionTool] Delegation failed for task ${childTaskId}: ${
205192
error instanceof Error ? error.message : String(error)
206193
}. Repairing parent and falling through to standalone completion.`,
207194
)

‎src/core/tools/__tests__/attemptCompletionTool.spec.ts‎

Lines changed: 3 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -492,8 +492,6 @@ describe("attemptCompletionTool", () => {
492492
let callbacks: AttemptCompletionCallbacks
493493

494494
beforeEach(() => {
495-
vi.useFakeTimers()
496-
497495
mockProvider = {
498496
getTaskWithId: vi.fn(),
499497
reopenParentFromDelegation: vi.fn(),
@@ -533,23 +531,6 @@ describe("attemptCompletionTool", () => {
533531
}
534532
})
535533

536-
afterEach(() => {
537-
vi.useRealTimers()
538-
})
539-
540-
it("retries delegation once on first failure and succeeds", async () => {
541-
mockProvider.reopenParentFromDelegation
542-
.mockRejectedValueOnce(new Error("transient error"))
543-
.mockResolvedValueOnce(undefined)
544-
545-
const promise = attemptCompletionTool.handle(mockTask as Task, block, callbacks)
546-
await vi.advanceTimersByTimeAsync(500)
547-
await promise
548-
549-
expect(mockProvider.reopenParentFromDelegation).toHaveBeenCalledTimes(2)
550-
expect(mockPushToolResult).toHaveBeenCalledWith("")
551-
})
552-
553534
it("repairs parent to active when both delegation attempts fail", async () => {
554535
mockProvider.reopenParentFromDelegation.mockRejectedValue(new Error("persistent error"))
555536

@@ -563,9 +544,7 @@ describe("attemptCompletionTool", () => {
563544
mockProvider.updateTaskHistory.mockResolvedValue([])
564545
mockProvider.persistDelegationMeta.mockResolvedValue(undefined)
565546

566-
const promise = attemptCompletionTool.handle(mockTask as Task, block, callbacks)
567-
await vi.advanceTimersByTimeAsync(500)
568-
await promise
547+
await attemptCompletionTool.handle(mockTask as Task, block, callbacks)
569548

570549
expect(mockProvider.updateTaskHistory).toHaveBeenCalledWith(
571550
expect.objectContaining({ status: "active", awaitingChildId: undefined }),
@@ -587,9 +566,7 @@ describe("attemptCompletionTool", () => {
587566

588567
mockProvider.updateTaskHistory.mockRejectedValue(new Error("repair failed"))
589568

590-
const promise = attemptCompletionTool.handle(mockTask as Task, block, callbacks)
591-
await vi.advanceTimersByTimeAsync(500)
592-
await promise // should NOT throw — repair error is caught internally
569+
await attemptCompletionTool.handle(mockTask as Task, block, callbacks) // should NOT throw — repair error is caught internally
593570

594571
expect(mockPushToolResult).toHaveBeenCalledWith(expect.stringContaining("Delegation to parent task failed"))
595572
})
@@ -604,9 +581,7 @@ describe("attemptCompletionTool", () => {
604581

605582
mockProvider.persistDelegationMeta.mockResolvedValue(undefined)
606583

607-
const promise = attemptCompletionTool.handle(mockTask as Task, block, callbacks)
608-
await vi.advanceTimersByTimeAsync(500)
609-
await promise
584+
await attemptCompletionTool.handle(mockTask as Task, block, callbacks)
610585

611586
expect(mockProvider.persistDelegationMeta).toHaveBeenCalledWith("parent-123", {
612587
status: "active",

0 commit comments

Comments
 (0)