From bd33dceccd5e4bed6737e27ef3e3674f88b095f3 Mon Sep 17 00:00:00 2001 From: Tatsuro Shibamura Date: Sun, 30 Aug 2026 16:24:32 +0900 Subject: [PATCH 1/2] Correlate tool approvals by call occurrence instead of transcript-wide result IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `withToolApproval` collected every historical `function_result.callId` into one set, deleted the pending approval requests whose wrapped call used any of those ids, and then dropped the decisions those requests would have bound. When a provider reused a call id from an occurrence that had already completed, the approval a human had just granted was silently discarded: the composition `withToolApproval(withFunctionInvocation(...))` either re-asked forever or lost the decision outright, and a turn carrying no decision at all deleted the stored request for the same reason. The transcript-wide purge is replaced by occurrence-aware correlation. One ordered pass over the messages of the run derives the logical occurrences: a request opens one, a `function_result` naming its call closes every occurrence for that call, and a decision settles the one still open under its id. A request that appears after the latest result for its call is therefore a new occurrence that neither the older result nor the decision that closed the older occurrence can reach — the same rule `isActionableResponse` already applies in the invocation loop underneath, and the correlation .NET's `ApprovalResponseBindingChatClient` and Python's transcript-order occurrences use. Request ids cannot stand in for occurrence identity, because a local id is derived as `ficc_${callId}` and repeats whenever a call id does. Nothing positional is persisted: the boundaries are recomputed from the input transcript on every run, so a serialized session still carries only the request snapshots. Stored requests stay the authoritative record of what a human was shown — they now bind to the newest occurrence still open under their id rather than to whichever copy the caller replayed — and the destructive store read is written back with every request that remains unanswered. Replayed copies of one still-open request coalesce into a single occurrence whose first copy stays canonical, so a doctored replay can no longer displace the request a decision binds against. Partial-batch behavior is unchanged: answering part of a batch re-surfaces only the remainder. Fixes #98 Co-authored-by: Claude Opus 5 --- CHANGELOG.md | 13 + .../core/src/client/tool-approval.test.ts | 339 ++++++++++++++++++ packages/core/src/client/tool-approval.ts | 194 ++++++++-- 3 files changed, 509 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f0d402..0ae4f2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,19 @@ The umbrella `@polymind-inc/agent-framework` package and all `@polymind-inc/agen packages are versioned in lockstep; one entry here covers the set. During 0.x, **minor releases may contain breaking changes**; patch releases are fixes only. +## Unreleased + +- **`@polymind-inc/agent-framework-core`** — an approval granted for a call id that an earlier + completed call had already used is no longer discarded. The approval layer correlated decisions + against a transcript-wide set of answered call ids, so a provider that reused a call id produced + a permanent approve → re-ask loop, or lost the decision outright, and a turn carrying no decision + deleted the stored request for the same reason. Decisions now bind per call occurrence, derived + from the order of the run's own messages: a result closes only the occurrence before it, and a + request after the latest result for its call opens a new one — the rule the function-calling loop + already applied underneath. Replayed copies of one still-open request coalesce with the first copy + canonical, so a doctored replay cannot displace the request a decision binds against. Nothing + positional is persisted; serialized sessions are unchanged in shape. + ## 0.4.0 A hardening and consolidation release: ten breaking changes tighten types, credentials, telemetry diff --git a/packages/core/src/client/tool-approval.test.ts b/packages/core/src/client/tool-approval.test.ts index 315a108..244a85d 100644 --- a/packages/core/src/client/tool-approval.test.ts +++ b/packages/core/src/client/tool-approval.test.ts @@ -3,6 +3,7 @@ import { Agent } from '../agent/agent.js'; import { AgentSession } from '../agent/session.js'; import { approvalResponse, + functionApprovalRequestContent, isApprovalRequest, isApprovalResponse, isHostedApproval, @@ -16,7 +17,11 @@ import type { } from '../types/content.js'; import { textContent } from '../types/content.js'; import type { Message } from '../types/message.js'; +import type { ChatClient, ChatOptions } from './chat-client.js'; +import { withFunctionInvocation } from './function-invocation.js'; import { MockChatClient } from './test-support.js'; +import type { ApprovalStateStore } from './tool-approval.js'; +import { sessionApprovalStore, withToolApproval } from './tool-approval.js'; const deleteAll = tool({ name: 'delete_all', @@ -662,3 +667,337 @@ describe('tool approval', () => { ]); }); }); + +describe('approval occurrences of a reused call id', () => { + /** A gated tool that records the `value` argument of every call it actually runs. */ + function recordingGate(executed: string[], name = 'gated') { + return tool({ + name, + description: 'Needs a human', + parameters: { type: 'object', properties: { value: { type: 'string' } } }, + approvalMode: 'always_require', + execute: async (input) => { + const value = String(input.value); + executed.push(value); + return `ran ${value}`; + }, + }); + } + + /** `call()` with the arguments a gated tool records. */ + function valued(callId: string, name: string, value: string): FunctionCallContent { + return { type: 'function_call', callId, name, arguments: `{"value":"${value}"}` }; + } + + /** The production composition, driven with an explicit transcript instead of through `Agent`. */ + function composed(mock: MockChatClient, store: ApprovalStateStore): ChatClient { + return withToolApproval(withFunctionInvocation(mock), store); + } + + /** A model that gates the same call id twice, then finishes. */ + function reusingModel(): MockChatClient { + return new MockChatClient([ + { contents: [valued('reused', 'gated', 'first')], finishReason: 'tool_calls' }, + { contents: [valued('reused', 'gated', 'second')], finishReason: 'tool_calls' }, + { contents: [textContent('done')], finishReason: 'stop' }, + ]); + } + + it('executes a later approval whose call id an earlier completed call already used', async () => { + const executed: string[] = []; + const agent = new Agent({ client: reusingModel(), tools: [recordingGate(executed)] }); + const session = agent.createSession(); + + const first = await agent.run('go', { session }); + const firstRequest = approvals(first)[0]; + assert.exists(firstRequest); + + const second = await agent.run(approvalResponse(firstRequest, true), { session }); + const secondRequest = approvals(second)[0]; + assert.exists(secondRequest); + // Same id, because a local request id is derived from the call id. + expect(secondRequest.id).toBe(firstRequest.id); + expect(secondRequest.functionCall.arguments).toBe('{"value":"second"}'); + + const third = await agent.run(approvalResponse(secondRequest, true), { session }); + expect(executed).toEqual(['first', 'second']); + expect(third.text).toBe('done'); + }); + + it('executes a reused call id whose later occurrence names a different tool', async () => { + const executed: string[] = []; + const mock = new MockChatClient([ + { contents: [valued('reused', 'gated', 'first')], finishReason: 'tool_calls' }, + { contents: [valued('reused', 'other_gate', 'second')], finishReason: 'tool_calls' }, + { contents: [textContent('done')], finishReason: 'stop' }, + ]); + const agent = new Agent({ + client: mock, + tools: [recordingGate(executed), recordingGate(executed, 'other_gate')], + }); + const session = agent.createSession(); + + const first = await agent.run('go', { session }); + const firstRequest = approvals(first)[0]; + assert.exists(firstRequest); + const second = await agent.run(approvalResponse(firstRequest, true), { session }); + const secondRequest = approvals(second)[0]; + assert.exists(secondRequest); + expect(secondRequest.functionCall.name).toBe('other_gate'); + + await agent.run(approvalResponse(secondRequest, true), { session }); + expect(executed).toEqual(['first', 'second']); + }); + + it('does not let the completed occurrence decision authorize the reused call id', async () => { + const executed: string[] = []; + const agent = new Agent({ client: reusingModel(), tools: [recordingGate(executed)] }); + const session = agent.createSession(); + + const first = await agent.run('go', { session }); + const firstRequest = approvals(first)[0]; + assert.exists(firstRequest); + await agent.run(approvalResponse(firstRequest, true), { session }); + + // No new decision. The transcript still replays the one that authorized the completed + // occurrence, and it names the same request id as the occurrence now waiting for a human. + const third = await agent.run('anything else?', { session }); + expect(executed).toEqual(['first']); + expect(approvals(third).map((request) => request.functionCall.arguments)).toEqual(['{"value":"second"}']); + + const resurfaced = approvals(third)[0]; + assert.exists(resurfaced); + await agent.run(approvalResponse(resurfaced, true), { session }); + expect(executed).toEqual(['first', 'second']); + }); + + it('keeps an unanswered stored request through a turn that carries no decision', async () => { + const executed: string[] = []; + const session = new AgentSession(); + const store = sessionApprovalStore(session); + const request = functionApprovalRequestContent(valued('reused', 'gated', 'second')); + store.addPending([request]); + const mock = new MockChatClient([{ contents: [textContent('nothing to do')], finishReason: 'stop' }]); + + await composed(mock, store).getResponse( + [ + { role: 'assistant', contents: [valued('reused', 'gated', 'first')] }, + { role: 'tool', contents: [{ type: 'function_result', callId: 'reused', result: 'ran first' }] }, + { role: 'user', contents: [textContent('anything else?')] }, + ], + { tools: [recordingGate(executed)] }, + ); + + expect(executed).toEqual([]); + expect(session.state._toolApproval).toEqual({ pending: [request] }); + }); + + it('writes every unanswered request back into a destructively read store', async () => { + const executed: string[] = []; + const reads: number[] = []; + let pending: FunctionApprovalRequestContent[] = []; + let autoApproved: FunctionApprovalRequestContent[] = []; + const store: ApprovalStateStore = { + takePending(): FunctionApprovalRequestContent[] { + reads.push(pending.length); + const taken = pending; + pending = []; + return taken; + }, + addPending(requests: readonly FunctionApprovalRequestContent[]): void { + for (const request of requests) { + if (!pending.some((known) => known.id === request.id)) { + pending.push(request); + } + } + }, + takeAutoApproved(): FunctionApprovalRequestContent[] { + const taken = autoApproved; + autoApproved = []; + return taken; + }, + setAutoApproved(requests: readonly FunctionApprovalRequestContent[]): void { + autoApproved = [...requests]; + }, + }; + const answered = functionApprovalRequestContent(valued('c1', 'gated', 'one')); + const unanswered = functionApprovalRequestContent(valued('c2', 'gated', 'two')); + store.addPending([answered, unanswered]); + const mock = new MockChatClient([{ contents: [textContent('done')], finishReason: 'stop' }]); + + await composed(mock, store).getResponse( + [ + // `c2` was used by an older call that already completed, so the transcript carries a + // result for it while the request the human still owes an answer to is only in the store. + { role: 'assistant', contents: [valued('c2', 'gated', 'older')] }, + { role: 'tool', contents: [{ type: 'function_result', callId: 'c2', result: 'ran older' }] }, + { role: 'user', contents: [approvalResponse(answered, true)] }, + ], + { tools: [recordingGate(executed)] }, + ); + + expect(reads.length).toBeGreaterThan(0); + expect(executed).toEqual(['one']); + expect(pending.map((request) => request.id)).toEqual([unanswered.id]); + }); + + it('coalesces replayed copies of one still-open request into a single execution', async () => { + const executed: string[] = []; + const session = new AgentSession(); + const store = sessionApprovalStore(session); + const request = functionApprovalRequestContent(valued('c1', 'gated', 'only')); + store.addPending([request]); + const mock = new MockChatClient([{ contents: [textContent('done')], finishReason: 'stop' }]); + + await composed(mock, store).getResponse( + [ + { role: 'assistant', contents: [request] }, + { role: 'assistant', contents: [request] }, + { role: 'user', contents: [approvalResponse(request, true)] }, + ], + { tools: [recordingGate(executed)] }, + ); + + expect(executed).toEqual(['only']); + expect(session.state._toolApproval).toBeUndefined(); + }); + + it('keeps the first copy canonical when a replay doctors a still-open request', async () => { + const executed: string[] = []; + // Nothing in the store: the transcript is the only record, so the copy the caller replays + // first is the one the decision has to bind against. + const store = sessionApprovalStore(new AgentSession()); + const request = functionApprovalRequestContent(valued('c1', 'gated', 'safe')); + const doctored: FunctionApprovalRequestContent = { + ...request, + functionCall: valued('c1', 'gated', 'everything'), + }; + const mock = new MockChatClient([{ contents: [textContent('done')], finishReason: 'stop' }]); + + await composed(mock, store).getResponse( + [ + { role: 'assistant', contents: [request] }, + { role: 'assistant', contents: [doctored] }, + { role: 'user', contents: [approvalResponse(doctored, true)] }, + ], + { tools: [recordingGate(executed)] }, + ); + + expect(executed).toEqual(['safe']); + }); + + it('resumes a reused call id in a service-managed session', async () => { + // The service owns the transcript, so history is never replayed: every occurrence is known + // only from the store, and the decision arrives with nothing beside it. + const executed: string[] = []; + const agent = new Agent({ client: reusingModel(), tools: [recordingGate(executed)] }); + const session = agent.createSession({ serviceSessionId: 'conv_1' }); + + const first = await agent.run('go', { session }); + const firstRequest = approvals(first)[0]; + assert.exists(firstRequest); + const second = await agent.run(approvalResponse(firstRequest, true), { session }); + const secondRequest = approvals(second)[0]; + assert.exists(secondRequest); + + const third = await agent.run(approvalResponse(secondRequest, true), { session }); + expect(executed).toEqual(['first', 'second']); + expect(third.text).toBe('done'); + }); + + it('binds a batch of decisions whatever order they arrive in', async () => { + const executed: string[] = []; + const mock = new MockChatClient([ + { + contents: [valued('c1', 'gated', 'first'), valued('c2', 'gated', 'second')], + finishReason: 'tool_calls', + }, + { contents: [textContent('done')], finishReason: 'stop' }, + ]); + const agent = new Agent({ client: mock, tools: [recordingGate(executed)] }); + const session = agent.createSession(); + + const first = await agent.run('do both', { session }); + const [one, two] = approvals(first); + assert.exists(one); + assert.exists(two); + + const resumed = await agent.run([approvalResponse(two, true), approvalResponse(one, true)], { + session, + }); + expect([...executed].sort()).toEqual(['first', 'second']); + expect(resumed.text).toBe('done'); + }); + + it('re-surfaces only the unanswered remainder when an older result reuses a batch call id', async () => { + const executed: string[] = []; + const mock = new MockChatClient([ + { + contents: [valued('c1', 'gated', 'first'), valued('c2', 'gated', 'second')], + finishReason: 'tool_calls', + }, + { contents: [textContent('done')], finishReason: 'stop' }, + ]); + const agent = new Agent({ client: mock, tools: [recordingGate(executed)] }); + const session = agent.createSession(); + + // An older, completed call already used `c2`, so the transcript carries a result for it + // before the batch that is waiting for a human. + const first = await agent.run( + [ + { role: 'assistant', contents: [valued('c2', 'gated', 'older')] }, + { role: 'tool', contents: [{ type: 'function_result', callId: 'c2', result: 'ran older' }] }, + { role: 'user', contents: [textContent('do both')] }, + ], + { session }, + ); + const [one, two] = approvals(first); + assert.exists(one); + assert.exists(two); + + const partial = await agent.run(approvalResponse(one, true), { session }); + expect(executed).toEqual(['first']); + expect(approvals(partial).map((request) => request.functionCall.callId)).toEqual(['c2']); + + const resumed = await agent.run(approvalResponse(two, true), { session }); + expect(executed).toEqual(['first', 'second']); + expect(resumed.text).toBe('done'); + }); + + it('resumes a reused call id across a serialized session', async () => { + const executed: string[] = []; + const agent = new Agent({ client: reusingModel(), tools: [recordingGate(executed)] }); + const session = agent.createSession(); + + const first = await agent.run('go', { session }); + const firstRequest = approvals(first)[0]; + assert.exists(firstRequest); + const second = await agent.run(approvalResponse(firstRequest, true), { session }); + const secondRequest = approvals(second)[0]; + assert.exists(secondRequest); + + const serialized = JSON.parse(JSON.stringify(session)) as { state: Record }; + // Deep equality, so a transcript index, message position or turn number added to the record + // would fail here: occurrence boundaries are re-derived from the input transcript instead. + expect(serialized.state._toolApproval).toEqual({ + pending: [ + { + type: 'function_approval_request', + id: 'ficc_reused', + userInputRequest: true, + functionCall: { + type: 'function_call', + callId: 'reused', + name: 'gated', + arguments: '{"value":"second"}', + }, + }, + ], + }); + + const restored = AgentSession.fromJSON(serialized); + const third = await agent.run(approvalResponse(secondRequest, true), { session: restored }); + expect(executed).toEqual(['first', 'second']); + expect(third.text).toBe('done'); + }); +}); diff --git a/packages/core/src/client/tool-approval.ts b/packages/core/src/client/tool-approval.ts index 5bd13cd..3f1a70f 100644 --- a/packages/core/src/client/tool-approval.ts +++ b/packages/core/src/client/tool-approval.ts @@ -9,7 +9,12 @@ import { } from '../tools/approval.js'; import type { AnyFunctionTool, Tool } from '../tools/tool.js'; import { isFunctionTool } from '../tools/tool.js'; -import type { Content, FunctionApprovalRequestContent, FunctionCallContent } from '../types/content.js'; +import type { + Content, + FunctionApprovalRequestContent, + FunctionApprovalResponseContent, + FunctionCallContent, +} from '../types/content.js'; import { isRecord } from '../types/content.js'; import type { Message } from '../types/message.js'; import type { ChatResponseUpdate } from '../types/response.js'; @@ -134,57 +139,173 @@ function sameCall(a: FunctionCallContent, b: FunctionCallContent): boolean { } /** - * Binds each inbound approval decision to the request the framework actually issued. + * One logical approval: a request that was issued, and everything that has settled it since. * - * Mirrors .NET `ApprovalResponseBindingChatClient`. Requests are known from two places: the store - * (for callers that echo only the decision) and the message history itself (for callers that - * replay the whole transcript). A decision whose id is not known is dropped, and one whose - * `functionCall` differs from the recorded call is rebound to the recorded one. + * A request id cannot stand in for this. Local ids are derived from the call id, so a provider + * that reuses a call id issues a second request under the *same* id — two occurrences that a + * decision names identically. + */ +interface ApprovalOccurrence { + /** The id a decision names. Repeats across occurrences of a reused call id. */ + readonly id: string; + /** The call this occurrence gates. A `function_result` naming it closes the occurrence. */ + readonly callId: string; + /** The record a decision binds against — the stored snapshot whenever there is one. */ + request: FunctionApprovalRequestContent; + /** A `function_result` for {@link callId} arrived after the request. */ + closed: boolean; + /** A decision has already been bound here; one decision settles one occurrence. */ + answered: boolean; +} + +/** What one pass over the input transcript says about the approvals it describes. */ +interface ApprovalScan { + /** Every occurrence the transcript opened, in the order it opened them. */ + readonly occurrences: ApprovalOccurrence[]; + /** The occurrence each decision settles, keyed by the decision object itself. */ + readonly bound: Map; + /** Decisions naming an id the transcript has no open occurrence for, in transcript order. */ + readonly unbound: FunctionApprovalResponseContent[]; + /** Whether the turn carries any decision of ours at all. */ + hasDecision: boolean; +} + +/** + * Derives the approval occurrences of the current input transcript, in transcript order. * - * ## Security considerations + * A request opens an occurrence; a `function_result` naming its call closes every occurrence for + * that call; a decision settles the one still open under its id. So a request that appears after + * the latest result for its call is a *new* occurrence, and neither an older result nor the + * decision that closed the older occurrence reaches it. * - * **The store wins.** A request that came out of the store is the record of what a human was - * actually shown; one read out of the message history is whatever the caller sent this turn. When - * both claim the same id the stored one is kept, so replaying a doctored copy of a request cannot - * change the arguments an already-granted decision authorizes. + * Nothing derived here is persisted. The boundaries are recomputed from the messages of every + * run, which is what keeps a serialized session free of transcript positions. */ -function bindInboundDecisions(messages: readonly Message[], store: ApprovalStateStore): Message[] { - const known = new Map(); - const fromStore = new Set(); - const answeredCallIds = new Set(); - for (const request of store.takePending()) { - known.set(request.id, request); - fromStore.add(request.id); - } +function scanOccurrences(messages: readonly Message[]): ApprovalScan { + const scan: ApprovalScan = { occurrences: [], bound: new Map(), unbound: [], hasDecision: false }; + /** The occurrence a decision would currently settle, per id. */ + const open = new Map(); - let hasDecision = false; for (const msg of messages) { for (const content of msg.contents) { if (content.type === 'function_result') { - answeredCallIds.add(content.callId); + // An empty call id names nothing — wire items that lost theirs must not close an + // unrelated occurrence. + if (content.callId !== '') { + for (const [id, occurrence] of open) { + if (occurrence.callId === content.callId) { + occurrence.closed = true; + open.delete(id); + } + } + } + continue; } if (isHostedApproval(content)) { - // Not ours to bind: the provider issued it and the provider checks it. + // Not ours to bind: the provider issued it and the provider settles it. continue; } if (isApprovalRequest(content)) { - if (!fromStore.has(content.id)) { - known.set(content.id, content); + const current = open.get(content.id); + if (current !== undefined && !current.answered && current.callId === content.functionCall.callId) { + // A replayed copy of an occurrence that is still open. The first copy stays canonical, + // so a doctored replay neither displaces it nor asks the human a second time. + continue; } + const occurrence: ApprovalOccurrence = { + id: content.id, + callId: content.functionCall.callId, + request: content, + closed: false, + answered: false, + }; + scan.occurrences.push(occurrence); + open.set(content.id, occurrence); } else if (isApprovalResponse(content)) { - hasDecision = true; + scan.hasDecision = true; + const current = open.get(content.id); + if (current === undefined || current.answered) { + scan.unbound.push(content); + } else { + current.answered = true; + scan.bound.set(content, current); + } } } } - for (const [id, request] of known) { - if (answeredCallIds.has(request.functionCall.callId)) { - known.delete(id); + return scan; +} + +/** + * Folds the requests the store surfaced on an earlier turn into the scanned occurrences. + * + * A stored request records what a human was actually shown, so it outranks any copy the caller + * replayed — but only for the occurrence it was surfaced for, which is the newest one still open + * under its id. A store entry the transcript does not mention at all (a caller that echoes only + * the decision, or a service-managed transcript that never replays history) becomes an occurrence + * of its own, and the decision that arrived with nothing to bind against settles it. + */ +function mergeStoredRequests(scan: ApprovalScan, stored: readonly FunctionApprovalRequestContent[]): void { + for (const request of stored) { + let target: ApprovalOccurrence | undefined; + for (const occurrence of scan.occurrences) { + if (occurrence.id === request.id && !occurrence.closed) { + target = occurrence; + } + } + if (target !== undefined) { + target.request = request; + continue; + } + const occurrence: ApprovalOccurrence = { + id: request.id, + callId: request.functionCall.callId, + request, + closed: false, + answered: false, + }; + scan.occurrences.push(occurrence); + const index = scan.unbound.findIndex((response) => response.id === request.id); + const response = index < 0 ? undefined : scan.unbound.splice(index, 1)[0]; + if (response !== undefined) { + occurrence.answered = true; + scan.bound.set(response, occurrence); } } - if (!hasDecision) { +} + +/** + * Binds each inbound approval decision to the request occurrence the framework actually issued. + * + * Mirrors .NET `ApprovalResponseBindingChatClient`. Requests are known from two places: the store + * (for callers that echo only the decision) and the message history itself (for callers that + * replay the whole transcript). A decision that settles no open occurrence is dropped, and one + * whose `functionCall` differs from the recorded call is rebound to the recorded one. + * + * Correlation is per occurrence rather than per call id: a completed call cannot retire the + * request a provider raised afterwards under the same id, and the decision that authorized the + * completed one cannot authorize the new one, because it settled an occurrence the result closed. + * This matches how the invocation loop underneath decides which decisions are actionable. + * + * ## Security considerations + * + * **The store wins.** A request that came out of the store is the record of what a human was + * actually shown; one read out of the message history is whatever the caller sent this turn. When + * both describe the same occurrence the stored one is kept, so replaying a doctored copy of a + * request cannot change the arguments an already-granted decision authorizes. + */ +function bindInboundDecisions(messages: readonly Message[], store: ApprovalStateStore): Message[] { + const scan = scanOccurrences(messages); + mergeStoredRequests(scan, store.takePending()); + const stillPending = (): FunctionApprovalRequestContent[] => + scan.occurrences + .filter((occurrence) => !occurrence.closed && !occurrence.answered) + .map((occurrence) => occurrence.request); + + if (!scan.hasDecision) { // `takePending` is destructive, but an unrelated turn must not consume approvals that the // caller has not answered yet. - store.addPending([...known.values()]); + store.addPending(stillPending()); return [...messages]; } @@ -197,20 +318,19 @@ function bindInboundDecisions(messages: readonly Message[], store: ApprovalState kept.push(content); continue; } - const matched = known.get(content.id); + const matched = scan.bound.get(content); if (matched === undefined) { - // A decision for a request this session never issued cannot authorize anything. + // A decision for a request this session never issued — or a replayed duplicate that finds + // its occurrence already settled — cannot authorize anything. changed = true; continue; } - // One decision per request: a replayed duplicate finds nothing left to bind against. - known.delete(content.id); - if (sameCall(content.functionCall, matched.functionCall)) { + if (sameCall(content.functionCall, matched.request.functionCall)) { kept.push(content); continue; } changed = true; - kept.push({ ...content, functionCall: matched.functionCall }); + kept.push({ ...content, functionCall: matched.request.functionCall }); } if (!changed) { out.push(msg); @@ -222,7 +342,7 @@ function bindInboundDecisions(messages: readonly Message[], store: ApprovalState // A caller may answer only part of a batch. Keep the remaining requests durable and inject // them into this turn so the invocation loop can surface them again instead of silently // consuming them when `takePending` cleared the store. - const pending = [...known.values()]; + const pending = stillPending(); store.addPending(pending); if (pending.length > 0) { out.push({ role: 'assistant', contents: pending, messageId: crypto.randomUUID() }); From 9228c3b10560d5371f6b6af6ad09e18f810a62ee Mon Sep 17 00:00:00 2001 From: Tatsuro Shibamura Date: Mon, 31 Aug 2026 11:26:34 +0900 Subject: [PATCH 2/2] Let only a result end an approval occurrence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both layers still let something other than a `function_result` end an occurrence, so a request copy replayed after the decision that answered it was read as a second, unanswered ask. `scanOccurrences` coalesced a replayed copy only while its occurrence was unanswered, and the invocation loop's `lastRequestPosition` always moved to the newest copy, which put the request after the decision meant for it. Together they dropped the decision and asked the human again for a call they had just approved: for `request → decision → request` with no result between, the gated tool ran zero times. An occurrence now ends only where the rule says it does. A replayed copy is coalesced whether or not a decision has settled it, and the recorded request position moves only when a result for that call has closed the previous occurrence — so a genuinely reused call id after a result still opens a new occurrence a stale decision cannot reach. Co-authored-by: Claude Opus 5 --- .../core/src/client/function-invocation.ts | 15 ++++++++++-- .../core/src/client/tool-approval.test.ts | 24 +++++++++++++++++++ packages/core/src/client/tool-approval.ts | 7 ++++-- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/packages/core/src/client/function-invocation.ts b/packages/core/src/client/function-invocation.ts index 2d2f0f9..0d64a76 100644 --- a/packages/core/src/client/function-invocation.ts +++ b/packages/core/src/client/function-invocation.ts @@ -364,8 +364,19 @@ export function createFunctionInvocationClientFactory { expect(executed).toEqual(['first', 'second']); expect(third.text).toBe('done'); }); + + it('honours a decision that a replayed copy of its own still-open request follows', async () => { + const executed: string[] = []; + const session = new AgentSession(); + const store = sessionApprovalStore(session); + const request = functionApprovalRequestContent(valued('c1', 'gated', 'only')); + store.addPending([request]); + const mock = new MockChatClient([{ contents: [textContent('done')], finishReason: 'stop' }]); + + // A caller that rebuilds the transcript can put the request back after the decision that + // answered it. No result separates the copies, so both are the same open occurrence: the + // decision still settles it, and the human is not asked again for what they just granted. + await composed(mock, store).getResponse( + [ + { role: 'assistant', contents: [request] }, + { role: 'user', contents: [approvalResponse(request, true)] }, + { role: 'assistant', contents: [request] }, + ], + { tools: [recordingGate(executed)] }, + ); + + expect(executed).toEqual(['only']); + expect(session.state._toolApproval).toBeUndefined(); + }); }); diff --git a/packages/core/src/client/tool-approval.ts b/packages/core/src/client/tool-approval.ts index 3f1a70f..53abcce 100644 --- a/packages/core/src/client/tool-approval.ts +++ b/packages/core/src/client/tool-approval.ts @@ -207,9 +207,12 @@ function scanOccurrences(messages: readonly Message[]): ApprovalScan { } if (isApprovalRequest(content)) { const current = open.get(content.id); - if (current !== undefined && !current.answered && current.callId === content.functionCall.callId) { + if (current !== undefined && current.callId === content.functionCall.callId) { // A replayed copy of an occurrence that is still open. The first copy stays canonical, - // so a doctored replay neither displaces it nor asks the human a second time. + // so a doctored replay neither displaces it nor asks the human a second time. A decision + // having already settled it makes no difference: only a result ends an occurrence, so a + // copy replayed after its own decision is still that same occurrence, and opening a new + // one would ask again for what the human just granted. continue; } const occurrence: ApprovalOccurrence = {