Conversation
…e result IDs
`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 <noreply@anthropic.com>
shibayan
added
the
bug
github-actions
Bot
added
documentation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This pull request fixes tool-approval decision correlation when providers reuse callIds by binding approvals to per-occurrence requests derived from transcript order (instead of a transcript-wide “answered callIds” set). This aligns withToolApproval behavior with the underlying invocation loop and reference implementations, preventing approve→re-ask loops and unintended loss of pending approvals.
Changes:
- Reworked inbound approval binding to scan “approval occurrences” from message order, merge in stored pending requests, and bind decisions to the correct open occurrence.
- Added regression tests covering call-id reuse, duplicate/replayed requests, destructive store semantics, batch ordering, and session serialization/resume.
- Documented the behavioral fix in
CHANGELOG.md.
File summaries
| File | Description |
|---|---|
| packages/core/src/client/tool-approval.ts | Implements occurrence-aware approval scanning/binding and correct pending-request write-back semantics. |
| packages/core/src/client/tool-approval.test.ts | Adds regression coverage for call-id reuse and occurrence correlation across replay/store/serialization scenarios. |
| CHANGELOG.md | Records the behavior change and its user-visible impact under “Unreleased”. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Fixes #98.
withToolApprovalcorrelated inbound decisions against a transcript-wide set ofanswered
function_result.callIds, so an approval granted for a call id that an earlier completedcall had already used was silently discarded — a permanent approve → re-ask loop, or a lost
decision — 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 appearing after the latest result for its call
opens a new one. Request ids cannot stand in for occurrence identity, because a local id is derived
from the call id and repeats whenever the call id does. Nothing positional is persisted — the
boundaries are recomputed every run, so serialized sessions keep their current shape.
Parity
ApprovalResponseBindingChatClient, which binds the requests it issuedand never purges pending requests from a transcript-wide result-id set; Python, which derives
logical occurrences from transcript order and has regression coverage for call-id reuse. The rule
matches the one
isActionableResponsealready applies in the invocation loop underneath.Checklist
pnpm checkpasses (lint, typecheck, build, test)