Skip to content

Commit da895f4

Browse files
committed
fix local codex review comment
- [P0] Preserve pending MCP approvals when resuming — packages/agents-core/src/runImplementation.ts:414-418 When we resume an interrupted turn, this block removes every RunToolApprovalItem from originalPreStepItems, but we only re-add the ones tied to function tools. For hosted MCP approvals without an on_approval handler, if the user has not yet approved, approved stays undefined, so nothing gets pushed back into newItems. As a result maybeCompleteTurnFromToolResults sees no interruptions, returns next_step_run_again, and the runner keeps looping until it hits the max-turns guard. In practice you can reproduce this with any HostedMCPTool HITL flow: run once, do not call state.approve, resume the run, and the approval request disappears so the user can no longer respond. We need to keep outstanding hosted approval items in preStepItems (or push them back into newItems) whenever they are still waiting on human input; otherwise HITL with hosted MCP tools is broken.
1 parent 98d56c0 commit da895f4

File tree

3 files changed

+149
-21
lines changed

3 files changed

+149
-21
lines changed

packages/agents-core/src/runImplementation.ts

Lines changed: 56 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -370,8 +370,21 @@ export async function resolveInterruptedTurn<TContext>(
370370
state,
371371
);
372372

373-
// Create the initial set of the output items
374-
const newItems: RunItem[] = functionResults.map((r) => r.runItem);
373+
// When resuming we receive the original RunItem references; suppress duplicates so history and streaming do not double-emit the same items.
374+
const originalPreStepItemSet = new Set(originalPreStepItems);
375+
const newItems: RunItem[] = [];
376+
const newItemsSet = new Set<RunItem>();
377+
const appendIfNew = (item: RunItem) => {
378+
if (originalPreStepItemSet.has(item) || newItemsSet.has(item)) {
379+
return;
380+
}
381+
newItems.push(item);
382+
newItemsSet.add(item);
383+
};
384+
385+
for (const result of functionResults) {
386+
appendIfNew(result.runItem);
387+
}
375388

376389
// Run MCP tools that require approval after they get their approval results
377390
const mcpApprovalRuns = processedResponse.mcpApprovalRequests.filter(
@@ -383,6 +396,9 @@ export async function resolveInterruptedTurn<TContext>(
383396
);
384397
},
385398
);
399+
// Hosted MCP approvals may still be waiting on a human decision when the turn resumes.
400+
const pendingHostedMCPApprovals = new Set<RunToolApprovalItem>();
401+
const pendingHostedMCPApprovalIds = new Set<string>();
386402
for (const run of mcpApprovalRuns) {
387403
// the approval_request_id "mcpr_123..."
388404
const approvalRequestId = run.requestItem.rawItem.id!;
@@ -398,23 +414,49 @@ export async function resolveInterruptedTurn<TContext>(
398414
reason: undefined,
399415
};
400416
// Tell Responses API server the approval result in the next turn
401-
newItems.push(
402-
new RunToolCallItem(
403-
{
404-
type: 'hosted_tool_call',
405-
name: 'mcp_approval_response',
406-
providerData,
407-
},
408-
agent as Agent<unknown, 'text'>,
409-
),
417+
const responseItem = new RunToolCallItem(
418+
{
419+
type: 'hosted_tool_call',
420+
name: 'mcp_approval_response',
421+
providerData,
422+
},
423+
agent as Agent<unknown, 'text'>,
410424
);
425+
appendIfNew(responseItem);
426+
} else {
427+
pendingHostedMCPApprovals.add(run.requestItem);
428+
pendingHostedMCPApprovalIds.add(approvalRequestId);
429+
functionResults.push({
430+
type: 'hosted_mcp_tool_approval',
431+
tool: run.mcpTool,
432+
runItem: run.requestItem,
433+
});
434+
appendIfNew(run.requestItem);
411435
}
412436
}
413437

414-
// Exclude the tool approval items, which should not be sent to Responses API,
415-
// from the SingleStepResult's preStepItems
438+
// Server-managed conversations rely on preStepItems to re-surface pending approvals.
439+
// Keep unresolved hosted MCP approvals in place so HITL flows still have something to approve next turn.
416440
const preStepItems = originalPreStepItems.filter((item) => {
417-
return !(item instanceof RunToolApprovalItem);
441+
if (!(item instanceof RunToolApprovalItem)) {
442+
return true;
443+
}
444+
445+
if (
446+
item.rawItem.type === 'hosted_tool_call' &&
447+
item.rawItem.providerData?.type === 'mcp_approval_request'
448+
) {
449+
if (pendingHostedMCPApprovals.has(item)) {
450+
return true;
451+
}
452+
const approvalRequestId = item.rawItem.id;
453+
if (approvalRequestId) {
454+
return pendingHostedMCPApprovalIds.has(approvalRequestId);
455+
}
456+
return false;
457+
}
458+
459+
return false;
418460
});
419461

420462
const completedStep = await maybeCompleteTurnFromToolResults({

packages/agents-core/test/run.test.ts

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import { handoff } from '../src/handoff';
3131
import {
3232
RunMessageOutputItem as MessageOutputItem,
3333
RunToolApprovalItem as ToolApprovalItem,
34+
RunToolCallOutputItem as ToolCallOutputItem,
3435
} from '../src/items';
3536
import { getTurnInput, selectModel } from '../src/run';
3637
import { RunContext } from '../src/runContext';
@@ -2389,14 +2390,23 @@ describe('Runner.run', () => {
23892390
conversationId: 'conv-mixed',
23902391
});
23912392

2392-
expect(model.requests).toHaveLength(2);
2393-
const secondItems = model.requests[1].input as AgentInputItem[];
2394-
expect(secondItems).toHaveLength(1);
2395-
expect(secondItems[0]).toMatchObject({
2396-
type: 'function_call_result',
2397-
callId: 'call-mixed',
2393+
expect(model.requests).toHaveLength(1);
2394+
2395+
const toolOutputs = secondResult.newItems.filter(
2396+
(item) =>
2397+
item instanceof ToolCallOutputItem &&
2398+
item.rawItem.type === 'function_call_result' &&
2399+
item.rawItem.callId === 'call-mixed',
2400+
);
2401+
expect(toolOutputs).toHaveLength(1);
2402+
2403+
expect(secondResult.interruptions).toHaveLength(1);
2404+
expect(secondResult.interruptions[0].rawItem).toMatchObject({
2405+
providerData: { id: 'approval-id', type: 'mcp_approval_request' },
23982406
});
2399-
expect(secondResult.finalOutput).toBe('still waiting');
2407+
expect(secondResult.state._currentStep?.type).toBe(
2408+
'next_step_interruption',
2409+
);
24002410
});
24012411

24022412
it('sends full history when no server-managed state is provided', async () => {

packages/agents-core/test/runImplementation.test.ts

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1915,4 +1915,80 @@ describe('resolveTurnAfterModelResponse', () => {
19151915
});
19161916
}
19171917
});
1918+
1919+
it('preserves pending hosted MCP approvals when resuming an interrupted turn', async () => {
1920+
const approvalAgent = new Agent({ name: 'MCPAgent', outputType: 'text' });
1921+
const mcpTool = hostedMcpTool({
1922+
serverLabel: 'demo_server',
1923+
serverUrl: 'https://example.com',
1924+
requireApproval: {
1925+
always: { toolNames: ['demo_tool'] },
1926+
},
1927+
});
1928+
1929+
const approvalRequest: protocol.HostedToolCallItem = {
1930+
type: 'hosted_tool_call',
1931+
id: 'approval1',
1932+
name: 'demo_tool',
1933+
status: 'in_progress',
1934+
providerData: {
1935+
type: 'mcp_approval_request',
1936+
server_label: 'demo_server',
1937+
name: 'demo_tool',
1938+
id: 'approval1',
1939+
arguments: '{}',
1940+
},
1941+
} as protocol.HostedToolCallItem;
1942+
1943+
const approvalItem = new ToolApprovalItem(approvalRequest, approvalAgent);
1944+
const originalPreStepItems = [approvalItem];
1945+
1946+
const processedResponse: ProcessedResponse = {
1947+
newItems: [],
1948+
handoffs: [],
1949+
functions: [],
1950+
computerActions: [],
1951+
mcpApprovalRequests: [
1952+
{
1953+
requestItem: approvalItem,
1954+
mcpTool,
1955+
},
1956+
],
1957+
toolsUsed: [],
1958+
hasToolsOrApprovalsToRun() {
1959+
return true;
1960+
},
1961+
};
1962+
1963+
const resumedResponse: ModelResponse = {
1964+
output: [],
1965+
usage: new Usage(),
1966+
} as any;
1967+
1968+
const resumedState = new RunState(
1969+
new RunContext(),
1970+
'test input',
1971+
approvalAgent,
1972+
1,
1973+
);
1974+
1975+
const runner = new Runner();
1976+
1977+
const result = await resolveInterruptedTurn(
1978+
approvalAgent,
1979+
'test input',
1980+
originalPreStepItems,
1981+
resumedResponse,
1982+
processedResponse,
1983+
runner,
1984+
resumedState,
1985+
);
1986+
1987+
expect(result.nextStep.type).toBe('next_step_interruption');
1988+
if (result.nextStep.type === 'next_step_interruption') {
1989+
expect(result.nextStep.data.interruptions).toContain(approvalItem);
1990+
}
1991+
expect(result.preStepItems).toContain(approvalItem);
1992+
expect(result.newStepItems).not.toContain(approvalItem);
1993+
});
19181994
});

0 commit comments

Comments
 (0)