Skip to content

Commit 6eb1a57

Browse files
seratchseratch
committed
fix local codex review comment:
 - [P1] Rewind only pending approval items — packages/agents-core/src/runImplementation.ts:352-358 In this resume path we subtract approvalItemCount computed over all RunToolApprovalItems in originalPreStepItems. When a turn experiences multiple approval interruptions in sequence, _generatedItems already contains older approval placeholders plus their resolved outputs. Subtracting the total count drops _currentTurnPersistedItemCount below the run items we already persisted (e.g., the first tool’s output). The next call to saveToSession then slices from that lower index and re-adds those older outputs to the session, producing duplicate tool results in persisted history. You can reproduce by running with session persistence, triggering two approvals back-to-back, approving the first, letting the agent request a second, then approving again — the session now contains the first tool result twice. Please only rewind the counter for the approval items that are still pending (e.g., the trailing approvals for the current interruption) rather than every approval seen so far.
1 parent edaad7c commit 6eb1a57

File tree

2 files changed

+183
-11
lines changed

2 files changed

+183
-11
lines changed

‎packages/agents-core/src/runImplementation.ts‎

Lines changed: 105 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,68 @@ export type ProcessedResponse<TContext = UnknownContext> = {
9393
hasToolsOrApprovalsToRun(): boolean;
9494
};
9595

96+
type ApprovalItemLike =
97+
| RunToolApprovalItem
98+
| {
99+
rawItem?: protocol.FunctionCallItem | protocol.HostedToolCallItem;
100+
agent?: Agent<any, any>;
101+
};
102+
103+
function isApprovalItemLike(value: unknown): value is ApprovalItemLike {
104+
if (!value || typeof value !== 'object') {
105+
return false;
106+
}
107+
108+
if (!('rawItem' in value)) {
109+
return false;
110+
}
111+
112+
const rawItem = (value as { rawItem?: unknown }).rawItem;
113+
if (!rawItem || typeof rawItem !== 'object') {
114+
return false;
115+
}
116+
117+
const itemType = (rawItem as { type?: unknown }).type;
118+
return itemType === 'function_call' || itemType === 'hosted_tool_call';
119+
}
120+
121+
function getApprovalIdentity(approval: ApprovalItemLike): string | undefined {
122+
const rawItem = approval.rawItem;
123+
if (!rawItem) {
124+
return undefined;
125+
}
126+
127+
if (rawItem.type === 'function_call' && rawItem.callId) {
128+
return `function_call:${rawItem.callId}`;
129+
}
130+
131+
if ('callId' in rawItem && rawItem.callId) {
132+
return `${rawItem.type}:${rawItem.callId}`;
133+
}
134+
135+
const id = 'id' in rawItem ? rawItem.id : undefined;
136+
if (id) {
137+
return `${rawItem.type}:${id}`;
138+
}
139+
140+
const providerData =
141+
typeof rawItem.providerData === 'object' && rawItem.providerData
142+
? (rawItem.providerData as { id?: string })
143+
: undefined;
144+
if (providerData?.id) {
145+
return `${rawItem.type}:provider:${providerData.id}`;
146+
}
147+
148+
const agentName =
149+
'agent' in approval && approval.agent ? approval.agent.name : '';
150+
151+
try {
152+
return `${agentName}:${rawItem.type}:${JSON.stringify(rawItem)}`;
153+
} catch {
154+
return `${agentName}:${rawItem.type}`;
155+
}
156+
}
157+
96158
/**
97159
* @internal
98160
* Walks a raw model response and classifies each item so the runner can schedule follow-up work.
@@ -350,16 +412,49 @@ export async function resolveInterruptedTurn<TContext>(
350412
// We already persisted the turn once when the approval interrupt was raised, so the
351413
// counter reflects the approval items as "flushed". When we resume the same turn we need
352414
// to rewind it so the eventual tool output for this call is still written to the session.
353-
const approvalItemCount = originalPreStepItems.filter(
354-
(item) => item instanceof RunToolApprovalItem,
355-
).length;
356-
// Persisting the approval request already advanced the counter once, so undo the increment
357-
// to make sure we write the final tool output back to the session when the turn resumes.
358-
if (approvalItemCount > 0) {
359-
state._currentTurnPersistedItemCount = Math.max(
360-
0,
361-
state._currentTurnPersistedItemCount - approvalItemCount,
362-
);
415+
const pendingApprovalItems = state
416+
.getInterruptions()
417+
.filter(isApprovalItemLike);
418+
419+
if (pendingApprovalItems.length > 0) {
420+
const pendingApprovalIdentities = new Set<string>();
421+
for (const approval of pendingApprovalItems) {
422+
const identity = getApprovalIdentity(approval);
423+
if (identity) {
424+
pendingApprovalIdentities.add(identity);
425+
}
426+
}
427+
428+
if (pendingApprovalIdentities.size > 0) {
429+
let rewindCount = 0;
430+
for (let index = originalPreStepItems.length - 1; index >= 0; index--) {
431+
const item = originalPreStepItems[index];
432+
if (!(item instanceof RunToolApprovalItem)) {
433+
break;
434+
}
435+
436+
const identity = getApprovalIdentity(item);
437+
if (!identity || !pendingApprovalIdentities.has(identity)) {
438+
break;
439+
}
440+
441+
rewindCount++;
442+
pendingApprovalIdentities.delete(identity);
443+
444+
if (pendingApprovalIdentities.size === 0) {
445+
break;
446+
}
447+
}
448+
449+
// Persisting the approval request already advanced the counter once, so undo the increment
450+
// to make sure we write the final tool output back to the session when the turn resumes.
451+
if (rewindCount > 0) {
452+
state._currentTurnPersistedItemCount = Math.max(
453+
0,
454+
state._currentTurnPersistedItemCount - rewindCount,
455+
);
456+
}
457+
}
363458
}
364459
// Run function tools that require approval after they get their approval results
365460
const functionToolRuns = processedResponse.functions.filter((run) => {

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

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,14 @@ describe('saveToSession', () => {
194194
providerData: {},
195195
};
196196

197-
state._generatedItems = [new ToolApprovalItem(functionCall, textAgent)];
197+
const approvalItem = new ToolApprovalItem(functionCall, textAgent);
198+
state._generatedItems = [approvalItem];
199+
state._currentStep = {
200+
type: 'next_step_interruption',
201+
data: {
202+
interruptions: [approvalItem],
203+
},
204+
};
198205

199206
const preApprovalResult = new RunResult(state);
200207
await saveToSession(
@@ -2025,6 +2032,76 @@ describe('resolveTurnAfterModelResponse', () => {
20252032
});
20262033

20272034
describe('resolveInterruptedTurn', () => {
2035+
it('rewinds persisted count only for pending approval placeholders', async () => {
2036+
const textAgent = new Agent<UnknownContext, 'text'>({
2037+
name: 'SequentialApprovalsAgent',
2038+
outputType: 'text',
2039+
});
2040+
const agent = textAgent as unknown as Agent<
2041+
UnknownContext,
2042+
AgentOutputType
2043+
>;
2044+
const firstCall: protocol.FunctionCallItem = {
2045+
...TEST_MODEL_FUNCTION_CALL,
2046+
id: 'call-first',
2047+
callId: 'call-first',
2048+
};
2049+
const secondCall: protocol.FunctionCallItem = {
2050+
...TEST_MODEL_FUNCTION_CALL,
2051+
id: 'call-second',
2052+
callId: 'call-second',
2053+
};
2054+
2055+
const firstApproval = new ToolApprovalItem(firstCall, agent);
2056+
const firstOutputRaw = getToolCallOutputItem(firstCall, 'done');
2057+
const firstOutput = new ToolCallOutputItem(firstOutputRaw, agent, 'done');
2058+
const secondApproval = new ToolApprovalItem(secondCall, agent);
2059+
2060+
const generatedItems = [firstApproval, firstOutput, secondApproval];
2061+
const state = new RunState(new RunContext(), 'hello', agent, 5);
2062+
state._generatedItems = generatedItems;
2063+
state._currentTurnPersistedItemCount = generatedItems.length;
2064+
state._currentStep = {
2065+
type: 'next_step_interruption',
2066+
data: {
2067+
interruptions: [secondApproval],
2068+
},
2069+
};
2070+
2071+
const processedResponse: ProcessedResponse = {
2072+
newItems: [],
2073+
handoffs: [],
2074+
functions: [],
2075+
computerActions: [],
2076+
mcpApprovalRequests: [],
2077+
toolsUsed: [],
2078+
hasToolsOrApprovalsToRun() {
2079+
return false;
2080+
},
2081+
};
2082+
2083+
const runner = new Runner({ tracingDisabled: true });
2084+
const modelResponse: ModelResponse = {
2085+
output: [],
2086+
usage: new Usage(),
2087+
} as any;
2088+
2089+
const result = await resolveInterruptedTurn(
2090+
agent,
2091+
'hello',
2092+
generatedItems,
2093+
modelResponse,
2094+
processedResponse,
2095+
runner,
2096+
state,
2097+
);
2098+
2099+
expect(state._currentTurnPersistedItemCount).toBe(
2100+
generatedItems.length - 1,
2101+
);
2102+
expect(result.preStepItems).toEqual([firstOutput]);
2103+
});
2104+
20282105
it('dispatches approved computer actions when resuming an interruption', async () => {
20292106
const fakeComputer: Computer = {
20302107
environment: 'mac',

0 commit comments

Comments
 (0)