diff --git a/src/core/sdk-session-contract.test.ts b/src/core/sdk-session-contract.test.ts index 55b260f..01867a9 100644 --- a/src/core/sdk-session-contract.test.ts +++ b/src/core/sdk-session-contract.test.ts @@ -495,7 +495,7 @@ describe('SDK session contract', () => { expect(initialSession.close).toHaveBeenCalledTimes(1); }); - it('clears stuck shared conversation during proactive recovery when details include invalid tool call IDs', async () => { + it('keeps shared conversation during proactive recovery when details include invalid tool call IDs', async () => { const initialSession = { initialize: vi.fn(async () => undefined), bootstrapState: vi.fn(async () => ({ hasPendingApproval: true, conversationId: 'conv-stuck' })), @@ -552,7 +552,7 @@ describe('SDK session contract', () => { ); expect(vi.mocked(resumeSession)).toHaveBeenCalledTimes(2); expect(vi.mocked(resumeSession).mock.calls[0][0]).toBe('conv-stuck'); - expect(vi.mocked(resumeSession).mock.calls[1][0]).toBe('agent-contract-test'); + expect(vi.mocked(resumeSession).mock.calls[1][0]).toBe('conv-stuck'); expect(initialSession.close).toHaveBeenCalledTimes(1); }); @@ -1238,7 +1238,7 @@ describe('SDK session contract', () => { expect(sentTexts).toContain('after default recovery'); }); - it('clears stuck shared conversation during reactive conflict recovery when details include invalid tool call IDs', async () => { + it('keeps shared conversation during reactive conflict recovery when details include invalid tool call IDs', async () => { const conflictError = new Error( 'CONFLICT: Cannot send a new message: The agent is waiting for approval on a tool call.' ); @@ -1297,7 +1297,7 @@ describe('SDK session contract', () => { expect(recoverOrphanedConversationApproval).toHaveBeenCalledWith('agent-contract-test', 'conv-stuck'); expect(vi.mocked(resumeSession)).toHaveBeenCalledTimes(2); expect(vi.mocked(resumeSession).mock.calls[0][0]).toBe('conv-stuck'); - expect(vi.mocked(resumeSession).mock.calls[1][0]).toBe('agent-contract-test'); + expect(vi.mocked(resumeSession).mock.calls[1][0]).toBe('conv-stuck'); expect(stuckSession.close).toHaveBeenCalledTimes(1); }); diff --git a/src/core/session-manager.ts b/src/core/session-manager.ts index 279a1d8..70a3792 100644 --- a/src/core/session-manager.ts +++ b/src/core/session-manager.ts @@ -8,7 +8,7 @@ import { createAgent, createSession, resumeSession, type Session, type SendMessage, type CanUseToolCallback } from '@letta-ai/letta-code-sdk'; import type { BotConfig, StreamMsg } from './types.js'; -import { isApprovalConflictError, isConversationMissingError, isAgentMissingFromInitError, isInvalidToolCallIdsError } from './errors.js'; +import { isApprovalConflictError, isConversationMissingError, isAgentMissingFromInitError } from './errors.js'; import { Store } from './store.js'; import { updateAgentName, recoverOrphanedConversationApproval, isRecoverableConversationId, recoverPendingApprovalsForAgent } from '../tools/letta-api.js'; import { installSkillsToAgent, prependSkillDirsToPath } from '../skills/loader.js'; @@ -399,18 +399,6 @@ export class SessionManager { } else { log.warn(`Proactive approval recovery did not find resolvable approvals: ${result.details}`); } - // Even on partial recovery, if any denial failed with mismatched IDs the - // conversation may still be stuck. Clear it so the retry creates a fresh one. - // TEMP(letta-code-sdk): remove this detail-string fallback once the SDK - // exposes typed terminal approval conflicts with built-in recovery policy. - if (isInvalidToolCallIdsError(result.details)) { - log.warn(`Clearing stuck conversation (key=${key}) due to invalid tool call IDs mismatch`); - if (key !== 'shared') { - this.store.clearConversation(key); - } else { - this.store.conversationId = null; - } - } return this._createSessionForKey(key, true, generation); } } @@ -597,19 +585,6 @@ export class SessionManager { const result = isRecoverableConversationId(convId) ? await recoverOrphanedConversationApproval(this.store.agentId, convId) : await recoverPendingApprovalsForAgent(this.store.agentId); - // Even on partial recovery, if any denial failed with mismatched IDs the - // conversation may still be stuck. Clear it so the retry creates a fresh one. - // TEMP(letta-code-sdk): remove this detail-string fallback once the SDK - // exposes typed terminal approval conflicts with built-in recovery policy. - if (isInvalidToolCallIdsError(result.details)) { - log.warn(`Clearing stuck conversation (key=${convKey}) due to invalid tool call IDs mismatch, retrying with fresh conversation`); - if (convKey !== 'shared') { - this.store.clearConversation(convKey); - } else { - this.store.conversationId = null; - } - return this.runSession(message, { retried: true, canUseTool, convKey }); - } if (result.recovered) { log.info(`Recovery succeeded (${result.details}), retrying...`); return this.runSession(message, { retried: true, canUseTool, convKey }); diff --git a/src/tools/letta-api.test.ts b/src/tools/letta-api.test.ts index eef8893..c0fbfcf 100644 --- a/src/tools/letta-api.test.ts +++ b/src/tools/letta-api.test.ts @@ -282,8 +282,42 @@ describe('recoverOrphanedConversationApproval', () => { expect(approvals[0].tool_call_id).toBe('tc-dup'); }); - it('continues recovery if batch denial API call fails', async () => { - // Two runs with approvals -- first batch fails, second should still succeed + it('recovers remaining approvals by submitting denials sequentially', async () => { + // Parallel tool calls can fail when denied as one batch. Verify we keep + // progressing by submitting one tool_call_id per request. + mockConversationsMessagesList.mockReturnValue(mockPageIterator([ + { + message_type: 'approval_request_message', + tool_calls: [ + { tool_call_id: 'tc-a', name: 'Bash' }, + { tool_call_id: 'tc-b', name: 'Read' }, + { tool_call_id: 'tc-c', name: 'Grep' }, + ], + run_id: 'run-parallel', + id: 'msg-parallel', + }, + ])); + mockRunsRetrieve.mockResolvedValue({ status: 'failed', stop_reason: 'error' }); + mockConversationsMessagesCreate + .mockRejectedValueOnce(new Error("Invalid tool call IDs. Expected '['tc-b']', but received '['tc-a']'")) + .mockResolvedValueOnce({}) + .mockResolvedValueOnce({}); + mockRunsList.mockReturnValue(mockPageIterator([])); + + const resultPromise = recoverOrphanedConversationApproval('agent-1', 'conv-1'); + await vi.advanceTimersByTimeAsync(10000); + const result = await resultPromise; + + expect(result.recovered).toBe(true); + expect(result.details).toContain('Failed to deny approval tc-a from run run-parallel'); + expect(result.details).toContain('Denied 2 approval(s) from failed run run-parallel'); + expect(mockConversationsMessagesCreate).toHaveBeenCalledTimes(3); + expect(mockConversationsMessagesCreate.mock.calls.map((call) => call[1].messages[0].approvals[0].tool_call_id)) + .toEqual(['tc-a', 'tc-b', 'tc-c']); + }); + + it('continues recovery if approval denial API call fails for one run', async () => { + // Two runs with approvals -- first denial fails, second should still succeed mockConversationsMessagesList.mockReturnValue(mockPageIterator([ { message_type: 'approval_request_message', diff --git a/src/tools/letta-api.ts b/src/tools/letta-api.ts index 02dd9b9..9a572f6 100644 --- a/src/tools/letta-api.ts +++ b/src/tools/letta-api.ts @@ -892,18 +892,36 @@ export async function recoverOrphanedConversationApproval( reason: `Auto-denied: originating run was ${status}/${stopReason}`, })); - try { - await client.conversations.messages.create(conversationId, { - messages: [{ - type: 'approval', - approvals: approvalResponses, - }], - streaming: false, - }); - } catch (batchError) { - const batchErrMsg = batchError instanceof Error ? batchError.message : String(batchError); - log.warn(`Failed to submit approval denial batch for run ${runId} (${approvals.length} tool call(s)):`, batchError); - details.push(`Failed to deny ${approvals.length} approval(s) from run ${runId}: ${batchErrMsg}`); + let deniedForRun = 0; + for (let i = 0; i < approvalResponses.length; i++) { + const approvalResponse = approvalResponses[i]; + try { + // Letta surfaces one pending approval at a time for parallel tool calls, + // so submit denials sequentially instead of as a single multi-ID batch. + await client.conversations.messages.create(conversationId, { + messages: [{ + type: 'approval', + approvals: [approvalResponse], + }], + streaming: false, + }); + deniedForRun += 1; + } catch (approvalError) { + const approvalErrMsg = approvalError instanceof Error ? approvalError.message : String(approvalError); + log.warn( + `Failed to submit approval denial for run ${runId} (tool_call_id=${approvalResponse.tool_call_id}):`, + approvalError, + ); + details.push(`Failed to deny approval ${approvalResponse.tool_call_id} from run ${runId}: ${approvalErrMsg}`); + continue; + } + + if (i < approvalResponses.length - 1) { + await new Promise(resolve => setTimeout(resolve, 1500)); + } + } + + if (deniedForRun === 0) { continue; } @@ -925,9 +943,9 @@ export async function recoverOrphanedConversationApproval( log.info(`No active runs to cancel for conversation ${conversationId}`); } - recoveredCount += approvals.length; + recoveredCount += deniedForRun; const suffix = cancelled ? ' (runs cancelled)' : ''; - details.push(`Denied ${approvals.length} approval(s) from ${status} run ${runId}${suffix}`); + details.push(`Denied ${deniedForRun} approval(s) from ${status} run ${runId}${suffix}`); } else { details.push(`Run ${runId} is ${status}/${stopReason} - not orphaned`); }