fix(core): emit tool result events for user-denied HITL calls by CryoThrust · Pull Request #3104 · agentscope-ai/agentscope-java

CryoThrust · 2026-09-11T07:32:00Z

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

Carry the persisted HITL reply id into confirmation-result application.
Emit the same start → text delta → end sequence for user-denied calls, with ToolResultState.DENIED.
Add an end-to-end regression test covering correlation, ordering, tool-call id, and denied state.

Verification

mvn -pl agentscope-core -DskipTests -Dspotless.check.skip=false spotless:check
mvn -pl agentscope-core -Dtest=ReActAgentHitlTest test (14 tests passed)

Signed-off-by: Yohanes [email protected]

Signed-off-by: Yohanes <[email protected]>

codecov · 2026-09-11T07:49:22Z

Codecov Report

❌ Patch coverage is 80.00000% with 31 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ain/java/io/agentscope/core/util/MessageUtils.java	72.94%	6 Missing and 17 partials ⚠️
...e/src/main/java/io/agentscope/core/ReActAgent.java	88.57%	7 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

oss-maintainer

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-replyId guard 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

oss-maintainer · 2026-09-11T12:49:55Z

                        ToolResultMessageBuilder.buildToolResultMsg(
                                deniedResult, denied, getName());
                state.contextMutable().add(deniedMsg);
+                if (replyId != null && !replyId.isEmpty()) {


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?

oss-maintainer · 2026-09-11T12:49:55Z

+                                    replyId,
+                                    denied.getId(),
+                                    denied.getName(),
+                                    "Permission denied by user"));


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.

oss-maintainer · 2026-09-11T12:49:55Z

            }

-            applyConfirmResults(normalized);
+            applyConfirmResults(normalized, replyId);


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.

oss-maintainer · 2026-09-11T12:49:55Z

        assertEquals("tc1", confirm.getConfirmResults().get(0).getToolCall().getId());
    }

+    @Test


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.

oss-maintainer

Summary

Right fix for the right symptom: a user-denied HITL tool call previously wrote a DENIED ToolResultBlock into context without ever telling the event stream, so AG-UI style consumers were left with a tool call that started and never ended. Sourcing replyId from the pending confirm request and correlating start/delta/end with the same toolCallId + ToolResultState.DENIED is exactly the contract the rule-denied path already uses, and the new test pins the ordering and the correlation. CI green on 8fa232e, CLA signed.

Caveat for the maintainers: this overlaps with PR #3099 ("fix: emit permission-denied tool result events"), which is currently in conflict with main — please land one of the two rather than both.

Findings

[Warning] agentscope-core/.../ReActAgent.java:2040-2055 — third copy of the DENIED start/delta/end triple and of the "Permission denied by user" literal (runToolBatch() already does this for rule-denied calls); see inline comment on extracting one helper.
[Info] ReActAgent.java:2040 — the blank-replyId guard silently skips emission and no test covers it.
[Info] ReActAgent.java:2051 — overloaded signature + unchanged javadoc leave a hook for a future caller to drop the events.

Not approving — this is review feedback only; the merge decision stays with the maintainers.

Automated review by github-manager-bot

oss-maintainer · 2026-09-14T04:45:17Z

+                if (replyId != null && !replyId.isEmpty()) {
+                    publishEvent(
+                            new ToolResultStartEvent(replyId, denied.getId(), denied.getName()));
+                    publishEvent(
+                            new ToolResultTextDeltaEvent(
+                                    replyId,
+                                    denied.getId(),
+                                    denied.getName(),
+                                    "Permission denied by user"));
+                    publishEvent(
+                            new ToolResultEndEvent(
+                                    replyId,
+                                    denied.getId(),
+                                    denied.getName(),
+                                    ToolResultState.DENIED));
+                }


[Performance / maintainability] This is the third copy of the "synthesise a DENIED tool-result event triple" shape — runToolBatch() already builds the identical ToolResultStartEvent / ToolResultTextDeltaEvent / ToolResultEndEvent sequence for rule-denied calls (around ReActAgent.java:3006-3021 on main), and the literal "Permission denied by user" is now duplicated with line 2033 of this file. Please extract one private helper (e.g. Flux<AgentEvent> deniedToolResultEvents(String replyId, ToolUseBlock call, String reason) or a small publishDeniedToolResult(...)), so the user-denied, rule-denied and any future deny path cannot drift apart in payload text or event ordering.

oss-maintainer · 2026-09-14T04:45:17Z

                        ToolResultMessageBuilder.buildToolResultMsg(
                                deniedResult, denied, getName());
                state.contextMutable().add(deniedMsg);
+                if (replyId != null && !replyId.isEmpty()) {


[Correctness] The replyId != null && !replyId.isEmpty() guard is the only place in this file where event emission is skipped on a missing reply id, and the no-event branch has no test. Consequence of falling into it: the DENIED ToolResultBlock is written to context but the UI/AG-UI stream never sees a ToolResultEndEvent, which is exactly the desynchronisation this PR sets out to fix. Worth either (a) a test that pins the silent-resume behaviour, or (b) an explicit contract assertion that replyId is always non-null on this path (it comes from resolvePendingRequestReplyId() at the caller) so the guard can be dropped rather than left untested.

oss-maintainer · 2026-09-14T04:45:17Z

+                                    "Permission denied by user"));
+                    publishEvent(
+                            new ToolResultEndEvent(
+                                    replyId,


[Compatibility] Overloading applyConfirmResults(List<ConfirmResult>) rather than changing its signature would silently leave a future caller on the single-arg version and drop the events again; the javadoc block above ("write a DENIED ToolResultBlock to context so the tool will no longer be pending on resume") also does not mention that the method now publishes tool-result events as a side effect. Since there is exactly one call site (validateAndAcceptConfirmResults), consider changing the signature outright and extending that javadoc to state the event contract.

The ubuntu build failed in ToolConfirmationCoordinatorTest .replacementTurnLeaseCannotReleaseOldTicketAndMayReuseToolUseId with "CannotStubVoidMethodWithReturnValue: 'appendSessionEvent' is a void method" while the failing line stubs managedExecutionScope. That is the signature of the shared Mockito mock being called from the coordinator's worker thread while the test thread sets up a stub: the void appendSessionEvent call is recorded and blamed instead of the stub. The test comes from upstream main (agentscope-ai#3104), this branch does not touch service-dataplane, and it passes 10/10 locally. Re-running for a clean signal.

fix(core): emit events for denied HITL tool calls

0f4e15f

Signed-off-by: Yohanes <[email protected]>

CryoThrust mentioned this pull request Sep 11, 2026

Permission HITL: user-denied tool calls emit no tool-result events (rule-denied ones do)[Bug]: #3097

Closed

oss-maintainer reviewed Sep 11, 2026

View reviewed changes

This was referenced Sep 11, 2026

fix: emit permission-denied tool result events #3099

Closed

feat(streaming): add semantic text output disposition events #3013

Open

Merge branch 'main' into fix/hitl-denied-events

8fa232e

oss-maintainer reviewed Sep 14, 2026

View reviewed changes

Merge branch 'main' into fix/hitl-denied-events

4989dfd

This comment was marked as abuse.

Sign in to view

fix: improve

0ec1861

This comment was marked as abuse.

Sign in to view

fix(core): refactor ReActAgent message helper methods into MessageUtils

14a05cc

jujn approved these changes Sep 17, 2026

View reviewed changes

jujn merged commit 70686e4 into agentscope-ai:main Sep 17, 2026
6 checks passed

DennisWLX mentioned this pull request Sep 18, 2026

feat(service): run Personal Weixin iLink channels end to end #3186

Open

Navigation Menu

Repository navigation

fix(core): emit tool result events for user-denied HITL calls - #3104

fix(core): emit tool result events for user-denied HITL calls#3104
jujn merged 5 commits into
agentscope-ai:mainfrom
CryoThrust:fix/hitl-denied-events

CryoThrust commented Sep 11, 2026

Uh oh!

codecov Bot commented Sep 11, 2026 •

edited

Loading

Uh oh!

oss-maintainer left a comment

Uh oh!

oss-maintainer Sep 11, 2026

Uh oh!

oss-maintainer Sep 11, 2026

Uh oh!

oss-maintainer Sep 11, 2026

Uh oh!

oss-maintainer Sep 11, 2026

Uh oh!

oss-maintainer left a comment

Uh oh!

oss-maintainer Sep 14, 2026

Uh oh!

oss-maintainer Sep 14, 2026

Uh oh!

oss-maintainer Sep 14, 2026

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

CryoThrust commented Sep 11, 2026

What problem does this PR solve?

What changed

Verification

Uh oh!

codecov Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

oss-maintainer left a comment

Choose a reason for hiding this comment

Summary

Findings

Cross-PR Note

Uh oh!

oss-maintainer Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

oss-maintainer Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

oss-maintainer Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

oss-maintainer Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

oss-maintainer left a comment

Choose a reason for hiding this comment

Summary

Findings

Uh oh!

oss-maintainer Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

oss-maintainer Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

oss-maintainer Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

This comment was marked as abuse.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 11, 2026 •

edited

Loading