fix(recovery): deny orphaned approvals sequentially for parallel tool calls (#580)
Co-authored-by: Letta Code <noreply@letta.com>
This commit is contained in:
@@ -495,7 +495,7 @@ describe('SDK session contract', () => {
|
|||||||
expect(initialSession.close).toHaveBeenCalledTimes(1);
|
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 = {
|
const initialSession = {
|
||||||
initialize: vi.fn(async () => undefined),
|
initialize: vi.fn(async () => undefined),
|
||||||
bootstrapState: vi.fn(async () => ({ hasPendingApproval: true, conversationId: 'conv-stuck' })),
|
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)).toHaveBeenCalledTimes(2);
|
||||||
expect(vi.mocked(resumeSession).mock.calls[0][0]).toBe('conv-stuck');
|
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);
|
expect(initialSession.close).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -1238,7 +1238,7 @@ describe('SDK session contract', () => {
|
|||||||
expect(sentTexts).toContain('after default recovery');
|
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(
|
const conflictError = new Error(
|
||||||
'CONFLICT: Cannot send a new message: The agent is waiting for approval on a tool call.'
|
'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(recoverOrphanedConversationApproval).toHaveBeenCalledWith('agent-contract-test', 'conv-stuck');
|
||||||
expect(vi.mocked(resumeSession)).toHaveBeenCalledTimes(2);
|
expect(vi.mocked(resumeSession)).toHaveBeenCalledTimes(2);
|
||||||
expect(vi.mocked(resumeSession).mock.calls[0][0]).toBe('conv-stuck');
|
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);
|
expect(stuckSession.close).toHaveBeenCalledTimes(1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -8,7 +8,7 @@
|
|||||||
|
|
||||||
import { createAgent, createSession, resumeSession, type Session, type SendMessage, type CanUseToolCallback } from '@letta-ai/letta-code-sdk';
|
import { createAgent, createSession, resumeSession, type Session, type SendMessage, type CanUseToolCallback } from '@letta-ai/letta-code-sdk';
|
||||||
import type { BotConfig, StreamMsg } from './types.js';
|
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 { Store } from './store.js';
|
||||||
import { updateAgentName, recoverOrphanedConversationApproval, isRecoverableConversationId, recoverPendingApprovalsForAgent } from '../tools/letta-api.js';
|
import { updateAgentName, recoverOrphanedConversationApproval, isRecoverableConversationId, recoverPendingApprovalsForAgent } from '../tools/letta-api.js';
|
||||||
import { installSkillsToAgent, prependSkillDirsToPath } from '../skills/loader.js';
|
import { installSkillsToAgent, prependSkillDirsToPath } from '../skills/loader.js';
|
||||||
@@ -399,18 +399,6 @@ export class SessionManager {
|
|||||||
} else {
|
} else {
|
||||||
log.warn(`Proactive approval recovery did not find resolvable approvals: ${result.details}`);
|
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);
|
return this._createSessionForKey(key, true, generation);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -597,19 +585,6 @@ export class SessionManager {
|
|||||||
const result = isRecoverableConversationId(convId)
|
const result = isRecoverableConversationId(convId)
|
||||||
? await recoverOrphanedConversationApproval(this.store.agentId, convId)
|
? await recoverOrphanedConversationApproval(this.store.agentId, convId)
|
||||||
: await recoverPendingApprovalsForAgent(this.store.agentId);
|
: 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) {
|
if (result.recovered) {
|
||||||
log.info(`Recovery succeeded (${result.details}), retrying...`);
|
log.info(`Recovery succeeded (${result.details}), retrying...`);
|
||||||
return this.runSession(message, { retried: true, canUseTool, convKey });
|
return this.runSession(message, { retried: true, canUseTool, convKey });
|
||||||
|
|||||||
@@ -282,8 +282,42 @@ describe('recoverOrphanedConversationApproval', () => {
|
|||||||
expect(approvals[0].tool_call_id).toBe('tc-dup');
|
expect(approvals[0].tool_call_id).toBe('tc-dup');
|
||||||
});
|
});
|
||||||
|
|
||||||
it('continues recovery if batch denial API call fails', async () => {
|
it('recovers remaining approvals by submitting denials sequentially', async () => {
|
||||||
// Two runs with approvals -- first batch fails, second should still succeed
|
// 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([
|
mockConversationsMessagesList.mockReturnValue(mockPageIterator([
|
||||||
{
|
{
|
||||||
message_type: 'approval_request_message',
|
message_type: 'approval_request_message',
|
||||||
|
|||||||
@@ -892,18 +892,36 @@ export async function recoverOrphanedConversationApproval(
|
|||||||
reason: `Auto-denied: originating run was ${status}/${stopReason}`,
|
reason: `Auto-denied: originating run was ${status}/${stopReason}`,
|
||||||
}));
|
}));
|
||||||
|
|
||||||
try {
|
let deniedForRun = 0;
|
||||||
await client.conversations.messages.create(conversationId, {
|
for (let i = 0; i < approvalResponses.length; i++) {
|
||||||
messages: [{
|
const approvalResponse = approvalResponses[i];
|
||||||
type: 'approval',
|
try {
|
||||||
approvals: approvalResponses,
|
// Letta surfaces one pending approval at a time for parallel tool calls,
|
||||||
}],
|
// so submit denials sequentially instead of as a single multi-ID batch.
|
||||||
streaming: false,
|
await client.conversations.messages.create(conversationId, {
|
||||||
});
|
messages: [{
|
||||||
} catch (batchError) {
|
type: 'approval',
|
||||||
const batchErrMsg = batchError instanceof Error ? batchError.message : String(batchError);
|
approvals: [approvalResponse],
|
||||||
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}`);
|
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;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -925,9 +943,9 @@ export async function recoverOrphanedConversationApproval(
|
|||||||
log.info(`No active runs to cancel for conversation ${conversationId}`);
|
log.info(`No active runs to cancel for conversation ${conversationId}`);
|
||||||
}
|
}
|
||||||
|
|
||||||
recoveredCount += approvals.length;
|
recoveredCount += deniedForRun;
|
||||||
const suffix = cancelled ? ' (runs cancelled)' : '';
|
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 {
|
} else {
|
||||||
details.push(`Run ${runId} is ${status}/${stopReason} - not orphaned`);
|
details.push(`Run ${runId} is ${status}/${stopReason} - not orphaned`);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user