fix(core): emit tool result events for user-denied HITL calls - #3104
fix(core): emit tool result events for user-denied HITL calls#3104CryoThrust wants to merge 1 commit into
Conversation
Signed-off-by: Yohanes <CryoThrust@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Emitting the missing ToolResultStartEvent / TextDeltaEvent / ToolResultEndEvent triple for user-denied HITL tool calls is the right fix, it mirrors the existing rule-denied emission in runToolBatch, and it ships with a focused regression test. Left as COMMENT because two questions are open (blank-replyId no-op, and publishing events from a synchronous helper rather than the stream).
Findings
- [Warning]
ReActAgent.java:2014— the blank-replyIdguard silently emits nothing, so the stuck-pending symptom survives for older persisted sessions. - [Warning]
ReActAgent.java:1916— events are published outside the reactive chain; please confirm subscriber ordering. - [Info]
ReActAgent.java:2022— duplicate literal for the denial reason. - [Info] test — no coverage for the blank-replyId branch or the auto-denied path.
Cross-PR Note
PR #3099 (also "emit permission-denied tool result events") overlaps heavily with this change and takes a different approach: it extracts a shared deniedToolResultEvents(...) helper, covers the auto-denied path, and mints a reply id when the correlation metadata is missing. Maintainers may want to land one of the two and fold the useful parts of the other in, rather than both.
Automated review by github-manager-bot
| ToolResultMessageBuilder.buildToolResultMsg( | ||
| deniedResult, denied, getName()); | ||
| state.contextMutable().add(deniedMsg); | ||
| if (replyId != null && !replyId.isEmpty()) { |
There was a problem hiding this comment.
Skipping emission whenever replyId is blank leaves exactly the symptom this PR fixes: for an ASKING tool call persisted before the correlation metadata existed (or after clearPendingRequestReplyId), no ToolResult* events are published, so an AG-UI client still renders a permanently pending tool call. Could this path emit under a freshly minted reply id (as PR #3099 does) or at least log at warn level so the silent no-op is diagnosable?
| replyId, | ||
| denied.getId(), | ||
| denied.getName(), | ||
| "Permission denied by user")); |
There was a problem hiding this comment.
The reason string is now duplicated in two places (ToolResultBlock.text(...) above and this delta). If they ever drift, the message written to context and the one streamed to the client disagree. Worth extracting a private static final String DENIED_BY_USER = "Permission denied by user"; and reusing it in both spots.
| } | ||
|
|
||
| applyConfirmResults(normalized); | ||
| applyConfirmResults(normalized, replyId); |
There was a problem hiding this comment.
applyConfirmResults is a synchronous state-mutation helper, so these events are pushed through publishEvent while the resume call is still validating input, i.e. outside the reactive chain that emits RequireUserConfirmEvent / RequestStopEvent. Please confirm the ordering guarantees: a subscriber that starts collecting after validateAndAcceptConfirmResults returns (or a replay from persisted state) may never observe the denied triple. Returning Flux<AgentEvent> from the helper — as runToolBatch does — would keep emission inside the stream.
| assertEquals("tc1", confirm.getConfirmResults().get(0).getToolCall().getId()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Nice regression test. Two gaps worth covering: (1) resume without METADATA_CONFIRM_REQUEST_REPLY_ID (the blank-replyId branch, which today emits nothing), and (2) the rule-denied path — writeAutoDeniedResults still writes DENIED blocks without publishing events, so an auto-denied tool has the same stuck-pending symptom this PR addresses for user denials.
2 participants
What problem does this PR solve?
When a user denies a permission-mode HITL tool call, the resumed run writes a DENIED ToolResultBlock to context but emits no ToolResultStartEvent/ToolResultTextDeltaEvent/ToolResultEndEvent. This leaves AG-UI and other event-driven consumers unable to observe the outcome. Rule-denied calls already emit the corresponding sequence.
What changed
ToolResultState.DENIED.Verification
mvn -pl agentscope-core -DskipTests -Dspotless.check.skip=false spotless:checkmvn -pl agentscope-core -Dtest=ReActAgentHitlTest test(14 tests passed)Signed-off-by: Yohanes CryoThrust@users.noreply.github.com