fix(eve): own HITL approval closure end-to-end in the harness#590
Closed
ruiconti wants to merge 5 commits into
Closed
fix(eve): own HITL approval closure end-to-end in the harness#590ruiconti wants to merge 5 commits into
ruiconti wants to merge 5 commits into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
A parked local tool call is a provider-enforced obligation: its tool_use id must be closed by exactly one terminal tool-result before any replay. Closure was split across two owners — eve synthesized deny-side results while the approve side was delegated to AI SDK collectToolApprovals (which only inspects messages.at(-1)) with result persistence riding stream capture. Fixes #236, #460, #529, #533; turns the stacked red evals green. Two mechanisms, both harness-owned: - approved-tool-execution.ts: resolvePendingInput returns the approved calls (approvedActions) and closeApprovedActionBatch executes them at resume time via the same wrapToolExecute + toModelOutput normalization the SDK path uses (extracted as resolveExecutedToolModelOutput). Every approved call yields exactly one terminal closure — real output, tool-error on throw, error result when the tool is missing or has no local execute — appended to the same tool message as the approval response before any model request is assembled. action.result is emitted against the parked batch's turn coordinates, and an AuthorizationSignal parks via the shared parkOnAuthorization (also adopted by the post-step in-stream path). Closure no longer depends on the approval message being the request tail, so the post-resolution message deferral (and the channel-context ordering hazard from #529) is deleted; a message sent with an approval response now rides the same model call. The unanswered-approval deferral (unrelated follow-ups never auto-deny) is unchanged. - transcript-obligations.ts: one pure reconciliation pass over the assembled messages immediately before every model call closes any remaining dangling local tool-call with a synthetic error-text result placed adjacent to its assistant message. No exemptions: anything still dangling at that point is an orphan nothing else will close. The repair is durable, so histories already poisoned by a missed closure heal on their next turn instead of replaying the same 400 forever. Signed-off-by: Rui Conti <ruiconti@gmail.com>
840a634 to
07b4f9a
Compare
Generalize the reconciler into closeDanglingToolCalls(messages, resolveClosure): one primitive that finds every local tool-call without a terminal tool-result and records the supplied closure adjacent to the calling assistant message. Calls whose closure resolves to undefined pass through untouched, so legitimately open obligations stay open. Every synthesized transcript closure now flows through it: - handleStepResult closes invalid-input tool calls (#576's sibling obligation) with their specific, model-actionable parse error, placed adjacently instead of via the insertInlineToolResultMessages trailing append. The emission-layer trailingInlineToolResultParts push for invalid input is dropped — that site only emits the action.result event; closure is re-derived from the step result via getInvalidToolCallInputErrors. - reconcileToolTranscript becomes a thin caller that closes every remaining dangler with the generic interrupted text. Also trims the red-eval descriptions and updates the research doc's point-fix history to record the fold. Signed-off-by: Rui Conti <ruiconti@gmail.com>
Review follow-ups, behavior-preserving: - Delete the reconcileToolTranscript wrapper: it had become a constant resolver plus a closed->repaired field rename. The tool loop's guard calls closeDanglingToolCalls directly with the interrupted-text policy; the invariant documentation moves onto the primitive. - Replace the flush-callback walk with an indexed lookahead loop so the provider adjacency rule reads literally: push the message, then merge its closures into the next tool message or insert one. - Define 'approved response' once: collectApprovedRequestIds feeds both recordApprovedTools and buildApprovedActionBatch instead of each re-deriving the predicate. Signed-off-by: Rui Conti <ruiconti@gmail.com>
Classify resume-time authorization parks before emitting terminal action results, and retain challenges from every approved call. Route approved, denied, and answered input results through call-ID transcript closure so results stay adjacent to their originating calls. Signed-off-by: Rui Conti <ruiconti@gmail.com>
Remove the Eve-owned executor and transcript-closure changes from #590. Approval collection with trailing context belongs in AI SDK and is now tracked in vercel/ai#17033; the stacked red-eval base remains unchanged. Signed-off-by: Rui Conti <ruiconti@gmail.com>
Contributor
Author
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.
Stacked on #588 (red evals + problem analysis). Fixes #236, #460, #529, #533 — the four red evals from the base PR go green without touching them.
What
harness/approved-tool-execution.ts— the harness executes approved parked tool calls itself on resume; results are durable before any model request exists.harness/transcript-obligations.ts— one pure guard before every model call: no localtool-callreaches a provider without a terminaltool-result. No exemptions.Why execution moves out of the AI SDK
Post-approval tool execution has always happened before the next model call — the provider contract forbids a request containing a
tool_usewith no adjacenttool_result. On main, that execution lived inside the AI SDK:streamText/generateTextbegin by scanning the initial messages for approval responses (collectToolApprovals) and, if found, run the tools in a synthetic pre-stream step with no model call, before issuing the first real request.That black-box shim had two failure modes:
queueDeferredStepInputdeferral existed to guard, and what the Linear channel's per-promptcontextslipped past (Linear channel: approving a HITL tool call permanently kills the session — channel context is appended after the approval response, socollectToolApprovalsnever sees it (danglingtool_use→ Anthropic 400) #529).tool_useand a guaranteed provider rejection on resume (Approved HITL tool's execution result missing from history — "tool_use ids were found without tool_result blocks" #460).This PR keeps the timing identical but moves ownership: the harness executes approved parked calls itself on resume (
closeApprovedActionBatch), appends and persists theirtool-results before any model request is assembled, and parks on any authorization challenge raised mid-execute — exactly mirroring the in-stream path (sharedparkOnAuthorization). The SDK path isn't just bypassed; it's structurally inert:collectToolApprovalsskips any approval whose call already has atool-resultin the trailing tool message, and owned closure writes into exactly that message, so double execution is impossible.Closure semantics
Execution goes through the same
wrapToolExecute+toModelOutputnormalization the SDK path uses (extracted asresolveExecutedToolModelOutputso both paths record identical results). Every approved call gets exactly one terminal closure: the real output, an error result whenexecutethrows, and an error result when the tool is missing or has no localexecute— an execute-less approved call previously produced no result from any owner and was a guaranteed provider rejection, so closing it honestly replaces a 400 with a model-visible error.action.resultis emitted against the parked turn's coordinates.Because closure no longer requires the approval message to be the request tail, the "AI SDK cannot process an approval response and a new user message in the same request" deferral is deleted: a message or channel context sent with the approving prompt rides the same model call, one fewer round-trip. The unanswered-approval deferral (unrelated follow-ups never auto-deny a pending approval) is unchanged.
The invariant guard
All synthesized transcript closure flows through one primitive,
closeDanglingToolCalls(messages, resolveClosure): it finds every localtool-callwithout a terminaltool-resultand records the supplied closure adjacent to the calling assistant message (calls resolving toundefinedstay open). Two callers:handleStepResultcloses invalid-input tool calls with their specific parse error — folding #576's sibling obligation out of theinsertInlineToolResultMessagestrailing append and the emission layer, which now only emits the event — and the tool loop's request-assembly guard, which runs once after all message assembly (pending resolution, context/message appends, compaction): any localtool-callwithout a latertool-resultis closed with a syntheticerror-textresult placed adjacent to its assistant message, plus a warn log. Zero exemptions — resume closure above guarantees anything still dangling here is an orphan nothing else will close.The repair is durable, which is the part park-time fixes can't deliver: sessions already poisoned by a missed closure (the permanently-dead sessions reported in #460/#529/#533) heal on their next turn instead of replaying the same 400 forever.
Scope
emitStreamContent's inline-result capture (still used for provider-executed and in-stream error results; its approval-resume capture branch is now dead for local tools and can be deleted in a follow-up once the e2e suite has soaked).