Repository navigation
fix(agent): validate approval decisions and keep decision-less resumes paused - #131
Conversation
…s paused De-duplicate approveToolCalls/rejectToolCalls in the ModelResult constructor so a repeated call ID cannot execute an approval-gated tool twice, and throw if one ID appears in both lists (typescript-sdk#518). Resuming an awaiting_approval/awaiting_hitl conversation with no decisions now stays paused instead of flipping to in_progress and sending a new model request (typescript-sdk#517). getResponse()/getText() reject with an explicit "awaiting approval" error listing the pending call IDs instead of the generic "Response not available". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-dedup-and-paused-resume
…very getter Review follow-ups for the decision-less pause: - awaiting_hitl resumes again when input carries a function_call_output for a pending call (the documented HITL resume). It stays paused only when no such output and no decision is supplied. awaiting_approval still pauses unconditionally. - A paused status with no pending tool calls continues with a normal request instead of stranding the conversation. - getToolCalls() and the stream getters reject with the same awaiting error as getResponse()/getText(). The message caps the listed pending IDs at 10 and says when supplied input was dropped. - Document de-duplication and the conflict throw on approveToolCalls / rejectToolCalls, and expand the changeset.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if (loadedState.status !== 'awaiting_hitl') { | ||
| return true; |
There was a problem hiding this comment.
🟡 Partial HITL resumes reject human outputs
After approveToolCalls re-pauses one HITL call and leaves another pending, supplying outputs for both keeps the conversation paused. processApprovalDecisions records this mixed pending set as awaiting_approval, so the HITL outputs never reach the model.
Learn more
HITL tools can return null from onToolCalled, leaving a call for a human to answer. processApprovalDecisions retains re-paused HITL calls alongside undecided calls, and nextStatus labels the combined pending set awaiting_approval. The new pause check uses that overall status rather than distinguishing HITL calls from approval-gated calls. Consequently, it rejects a later input containing valid outputs for all outstanding HITL calls, even though the normal HITL resume path can consume them.
Example: Two HITL calls a and b pause. Approving a invokes onToolCalled again, which returns null; b remains undecided. Supplying human function_call_output items for both a and b now leaves the run paused instead of delivering their results.
Recommended fix: Preserve the distinction between HITL and approval-gated pending calls when persisting partial decisions. Accept matching HITL outputs only when all outstanding approval gates are resolved, without treating their outputs as approval decisions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ```ts | ||
| const result = callModel(client, { model, input: [], tools, state }); | ||
| // state is awaiting_approval and no decisions were passed: | ||
| await result.requiresApproval(); // true, and no request was sent | ||
| await result.getResponse(); // throws: Conversation is awaiting approval ... Pending tool call IDs: call_1 | ||
| ``` |
There was a problem hiding this comment.
🟡 Missing required PR API example
The callModel approval options change behavior, but the PR description lacks the required ### API example section. Its inline snippets do not show consumer usage or annotate the old and new behavior.
Learn more
The public-API examples rule requires a fenced TypeScript usage example in both the changeset and the PR description. It also requires a ### API example section in the PR description and old-versus-new comments for behavioral changes. The changeset has an example, but the supplied PR description has no such section or fenced usage example.
Example: Show a persisted awaiting_approval conversation resumed without decisions, where the old behavior sent a request and the new behavior remains paused.
Recommended fix: Add a ### API example section with a fenced ts example to the PR description, annotated with the old and new observable behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /** | ||
| * Tool call IDs to approve when resuming a paused conversation | ||
| * (`awaiting_approval`, or `awaiting_hitl` to re-run `onToolCalled`). | ||
| * | ||
| * Duplicate IDs are ignored: each approved call executes once. An ID that | ||
| * also appears in `rejectToolCalls` makes `callModel` throw before any | ||
| * state is loaded or any tool runs. | ||
| * | ||
| * Resuming an `awaiting_approval` conversation without any decision leaves | ||
| * it paused: no model request is sent, and `input` passed with that call is | ||
| * neither sent nor saved. | ||
| */ | ||
| approveToolCalls?: string[]; |
There was a problem hiding this comment.
🟡 Approval guide still shows invalid resume
The approveToolCalls behavior changes, but the Tool Approval guide still shows a callback without state. The required README update is missing, leaving consumers with an unusable approval example.
Learn more
The public-API examples rule requires updating the existing README section when a documented API changes behavior. The Tool Approval guide documents approveToolCalls, but still passes an async callback without a state accessor. The option accepts IDs for a state-backed paused resume instead. This change updates the option documentation and behavior without correcting that consumer-facing guide.
Example: Copying the current README example supplies approveToolCalls as an async callback and omits state; it does not demonstrate how to approve the paused call IDs.
Recommended fix: Update the existing Tool Approval section with a state-backed pause/resume example that supplies pending call IDs as arrays, and show how decision-less resumes remain paused.
Was this helpful? React with 👍 or 👎 to provide feedback.
Fixes #157 (moved from OpenRouterTeam/typescript-sdk#518)
Fixes #156 (moved from OpenRouterTeam/typescript-sdk#517)
What
Duplicate / conflicting decisions (#157). The
ModelResultconstructor de-duplicatesapproveToolCallsandrejectToolCalls, soapproveToolCalls: ['call_1', 'call_1']executes the gated tool once and sends onefunction_call_output. An ID in both lists throws before any state is loaded or any tool runs:Tool call IDs cannot be both approved and rejected: call_2. ...De-duplication also closes a second hole: when a
PreToolUsehook mutated the arguments on resume, the first copy of a duplicated ID sent the call back for re-approval and the second copy executed it with the unreviewed arguments.Resume without decisions (#156). Before this change, loading a paused state with no decisions fell through to the normal path, set the status to
in_progress, and sent a fresh model request past calls that still needed a decision. The rules are now:awaiting_approval, at least one pending callawaiting_hitl,inputhas afunction_call_outputfor a pending callonResponseReceivedand on to the model. This is the documented HITL resume.awaiting_hitl, no such outputpendingToolCallsClearer error from every getter. When a resume leaves the conversation paused (no decisions, or decisions for only some pending calls),
getResponse(),getText(),getToolCalls()and all stream getters reject with the same message, for example:Conversation is awaiting approval (status 'awaiting_approval'); no response is available. Pending tool call IDs: call_1. Resume with approveToolCalls and/or rejectToolCalls for the pending calls. ...Before, they rejected with
Response not availableorStream not initialized. The message lists at most 10 pending IDs (... (N more)after that) and, when the call passedinput, adds that the input was not sent to the model and not saved to state. Forawaiting_hitlit explains thefunction_call_outputresume.requiresApproval(),getPendingToolCalls()andgetState()do not throw.Behavior notes for reviewers
awaiting_approvalplus a caller-suppliedfunction_call_outputfor the gated call stays paused. Onmainthis input went straight to the model, so a caller could answer for a gated tool with no approve/reject on record. Nothing documents that as a way to resolve an approval pause (the docs describe it for HITL and manual tools only), so the gate now holds until a decision is recorded. The output is dropped and the error says so.inputand no decisions stays paused, and the new input is neither sent nor saved. Callers have to approve or reject first (or, for HITL, supply the output).inputis now resolved before the pause check on anawaiting_hitlresume. It is still resolved only once per call.Error. The repo's typed errors (InvalidStateError,ToolTaskAlreadySettledError) describe unparseable state and settled async tasks, and neither fits a paused conversation, so this PR adds no new public error class.patch: these are bug fixes with no new API, and the HITL and empty-pending paths behave as onmain.Not changed here
These behave the same on
mainand on this branch: duplicate decisions across separate or concurrent resumes, replayed approvals on a completed conversation,inputbeing dropped when decisions are supplied, duplicate pending IDs from the model (#128), stalependingToolCallsleft on a state after a HITL output resume completes, and the README example with a function-valuedapproveToolCalls.Test plan
approval decision validation and paused resumes (#517, #518)block intests/unit/approval-gate-regressions.test.ts(16 tests). Original 6: duplicate approve, duplicate reject, approve+reject conflict,awaiting_approvalno-decision resume,awaiting_hitlno-decision resume, partial-decision error. Added 10: stateful HITL output resume throughonResponseReceived, function-valuedinputresolved once,awaiting_hitlstays paused without a matching output,awaiting_approvalstays paused despite a caller-supplied output, every response/stream getter rejects with the awaiting error while inspection APIs work, empty/missingpendingToolCallscontinues (3 cases), ID list cap and input note, duplicated ID cannot skip re-approval after aPreToolUsemutation.main. 9 of the added 10 fail on the previous head of this branch (1601bff); thePreToolUseone fails onmain.pnpm install --frozen-lockfile,pnpm lint,pnpm typecheck,pnpm test(agent: 1265 passed, mcp: 12 passed, coverage thresholds met),pnpm verify:packages,sentrux gate.🤖 Generated with Claude Code