fix: emit permission-denied tool result events - #3099
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@guslegend0510 This PR currently conflicts with git fetch origin
git checkout codex/fix-3097-denied-tool-events
git rebase origin/main
# resolve conflicts, then:
git push --force-with-leaseThis is a one-time reminder. Feel free to @mention me for a re-review once conflicts are resolved. Automated notification by github-manager-bot |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds a shared deniedToolResultEvents(...) helper and routes every permission-denial path (user confirm, auto-deny during acting, rule deny in runToolBatch) through it, with parameterized tests — a genuinely more complete treatment of the denied-event gap than the current state on main. COMMENT because three things need resolution first: this PR currently has merge conflicts, the AG-UI half duplicates machinery that already landed, and the synthesized replyId changes correlation semantics.
Findings
- [Critical]
ReActAgent.java:1913— a randomreplyIdfor the metadata-missing case produces results keyed to an id the client never saw; also now publishesUserConfirmResultEventunconditionally. - [Warning]
AguiStreamContext.java:235—resumeToolCalloverlaps withadoptToolCall+hasKnownToolCall+ the existingPermissionConfirmEventConverteradoption onmain; likely the conflict source. - [Warning]
maven-ci.yml:89— unrelated Windows mvnd→mvn switch that also removes the documentedmvnd#161workaround. - [Info]
ReActAgent.java:2927— denied triples are emitted beforeRequireUserConfirmEvent. - [Info] test — blank-
replyIdbranch not covered.
Cross-PR Note
PR #3104 covers the same user-denied gap with a narrower change. Suggest the maintainers pick one home for the shared helper: this PR's deniedToolResultEvents(...) factoring (which also fixes the auto-denied path) looks like the better base once the conflict is resolved and the AG-UI hunks are dropped.
Automated review by github-manager-bot
| # -T1: single-threaded build so reactor order is respected (see Linux step). | ||
| # -Dmvnd.maxLostKeepAlive=120: avoid StaleAddressException on long javadoc phases (mvnd#161). | ||
| & "~\.mvnd\bin\mvnd.cmd" -B -T1 "-Dmvnd.maxLostKeepAlive=120" clean verify | ||
| # Use standard Maven to avoid mvnd daemon startup/communication failures. |
There was a problem hiding this comment.
Two things here: (1) this CI change is unrelated to the PR's stated purpose (permission-denied tool result events) — it belongs in its own PR so it can be reverted independently; (2) dropping mvnd on Windows also drops the workaround the removed comment described (-Dmvnd.maxLostKeepAlive=120 for StaleAddressException during long javadoc phases, apache/maven-mvnd#161). Falling back to plain mvn -B -T1 clean verify is a legitimate choice, but please confirm Windows CI is green with it and say so in the description, otherwise the next long javadoc build may flake and the cause will be hard to trace back to this PR.
| clearPendingRequestReplyId(Msg.METADATA_CONFIRM_REQUEST_REPLY_ID); | ||
| if (replyId.isEmpty()) { | ||
| // Older persisted ASKING calls may not carry request correlation metadata. | ||
| replyId = UUID.randomUUID().toString().replace("-", ""); |
There was a problem hiding this comment.
Inventing a fresh replyId when the persisted METADATA_CONFIRM_REQUEST_REPLY_ID is absent breaks the correlation this PR is trying to restore: the client saw RequireUserConfirmEvent under the original reply id, so the ToolResult* events (and the ToolCallResult messageId keyed as replyId + ":" + toolCallId in the AG-UI adapter) will land under an id the client has never seen — an orphan result rather than a resolved pending call. It also makes UserConfirmResultEvent publish unconditionally now, where before it was suppressed for the blank case. Could the blank-metadata case instead recover the id from the last assistant message (the same place persistPendingRequestReplyId writes it), or fall back to the previous no-op behaviour plus a log.warn? A test for the metadata-missing resume path would pin the intent either way.
| "permission asking", | ||
| GenerateReason.PERMISSION_ASKING)); | ||
| return Flux.fromIterable(toolCalls) | ||
| .filter(tc -> autoDenied.contains(tc.getId())) |
There was a problem hiding this comment.
Emission ordering detail: the auto-denied ToolResult* triples are concatenated before RequireUserConfirmEvent / RequestStopEvent. A consumer that opens a tool-call card on RequireUserConfirmEvent will therefore receive results for tool calls it has not been told are pending yet. Emitting the denied triples after the confirm/stop pair (or right after writeAutoDeniedResults) reads more naturally; either is fine as long as it is deliberate.
| } | ||
| } | ||
|
|
||
| /** Recognize a validated permission resume without replaying the previous run's call events. */ |
There was a problem hiding this comment.
resumeToolCall overlaps with what is already on main: AguiStreamContext.adoptToolCall(toolCallId) plus the hasKnownToolCall(...) guard on beginToolResult / appendToolResultText / endToolResult, and PermissionConfirmEventConverter already calls adoptToolCall(...) for every validated ConfirmResult. This is very likely the source of the current merge conflict. Please rebase onto main and drop the AguiStreamContext / PermissionConfirmEventConverter hunks — the core-side fix plus the existing adoption path should be sufficient. Note the difference in semantics too: resumeToolCall writes both startedToolCalls and endedToolCalls (which suppresses a later ToolCallEnd for a call that is re-executed in this run), whereas adoptToolCall keeps those sets separate.
| assertTrue(foundDenied, "expected a DENIED ToolResultBlock for the rejected tool"); | ||
| } | ||
|
|
||
| @ParameterizedTest |
There was a problem hiding this comment.
The parameterized user-denied / rule-denied coverage and assertDeniedEvents are a good fit for this change, and promoting AskingTool to assert execution counts is a nice way to prove the denied path never invokes the tool. The one missing case is the blank-replyId resume discussed above, which is exactly the branch whose behaviour changed.
3 participants
Fixes #3097
User-denied permission confirmations previously updated the message context without emitting tool-result events, leaving event-driven clients without a tool outcome.
This change:
Validation: