fix(agent): keep user messages sent with a permission-HITL resume by Ashfaqbs · Pull Request #3295 · agentscope-ai/agentscope-java

Ashfaqbs · 2026-09-25T16:09:32Z

AgentScope-Java Version

2.0.4-SNAPSHOT (main @ ea78c31)

Description

When a run is paused on a permission prompt, the askingToolCalls() branch of ReActAgent only pulled the ConfirmResults out of the incoming messages and discarded the rest. A user message sent in the same request as the resume never reached the model, in that turn or any later one, and no error was raised.

This follows option A from the maintainer evaluation on the issue, with its ordering constraint:

The non-confirm messages of the resume request are held on the call scope (deferredResumeMsgs). They are not written before resumeAgent(), because the approved tools' results are written by acting(0) and a user turn would end up between the assistant tool_use and its tool_result.
They are appended at the start of the next reasoning() step, and only once no tool call is pending.
If the turn ends without another reasoning step (return-direct, a stop request, a suspended tool), they are appended before the state is saved, again only when nothing is pending.
If tool calls are still pending when the turn ends (for example a second permission prompt), the messages cannot be placed without breaking the tool_use/tool_result pairing. They are dropped with a WARN log instead of silently. I did not try to solve that case here; say if you would rather reject the request in that situation.

Tests (ReActAgentHitlTest): the scripted model now records the messages of each request, and two new tests cover an approved and a denied resume that carry a user message. They assert the text is in the next model request and in the persisted context, and that it comes after the tool result. Both fail on main (user message sent with the resume must not be dropped) and pass with this change.

Not covered: the AG-UI level case suggested in the evaluation (AguiPermissionResumeTest resuming with a user message). This PR only touches ReActAgent; I have not run that path end to end.

Checklist

Code has been formatted with mvn spotless:apply
All tests are passing (mvn test) - I ran ReActAgent*Test, *Permission*Test and *Hitl*Test in agentscope-core, all green, not the full suite
Javadoc comments are complete and follow project conventions
Related documentation has been updated (e.g. links, examples, etc.) - none needed
Code is ready for review

When a run was paused on a permission prompt, the resume branch of ReActAgent only read the ConfirmResults out of the incoming messages and dropped everything else, so a user message sent in the same request never reached the model, in this turn or in later ones. Hold those messages until the resumed tool calls have their results and append them at the next reasoning step, so a user turn never sits between a tool_use and its tool_result. If tool calls are still pending when the turn ends (for example a second permission prompt), log a warning instead of dropping them silently. Fixes agentscope-ai#3294

CLAassistant · 2026-09-25T16:09:40Z

All committers have signed the CLA.

oss-maintainer · 2026-09-25T16:46:09Z

CLA Not Signed

The Contributor License Agreement (CLA) check is currently pending on this PR (license/cla: Contributor License Agreement is not signed yet.). This PR cannot be merged until the CLA is signed.

@Ashfaqbs please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the license/cla status will turn green.

Automated check by github-manager-bot

oss-maintainer

Summary

Fixes a real HITL gap: user messages sent together with a permission resume are now deferred until the resumed tool calls have results, so they never land between a tool_use and its tool_result. The approach (defer → flush at the next reasoning step → discard on turn end) is sound and well tested.

Two things keep me from approving this run:

CLA is not signed yet (license/cla: pending) — the PR cannot be merged until it is; see the reminder comment.
CI — build (ubuntu-latest) failed on ToolConfirmationCoordinatorTest.replacementTurnLeaseCannotReleaseOldTicketAndMayReuseToolUseId (CannotStubVoidMethodWithReturnValue on appendSessionEvent), in agentscope-service, i.e. outside this PR's diff. main is green at ea78c3172, so please rebase onto the latest main and re-run; if the failure persists after rebase, flag it — it would look like a cross-module flake worth its own issue.

Inline notes cover the ArrayList cross-thread mutation, the silent-discard data-loss path, and a missing discard-path test.

Automated review by github-manager-bot

oss-maintainer · 2026-09-25T16:47:22Z

+         * enter the context before the resumed tool calls have their results, so they wait here
+         * until the ReAct loop reaches its next reasoning step.
+         */
+        private final List<Msg> deferredResumeMsgs = new ArrayList<>();


[Warning] deferredResumeMsgs is a plain ArrayList mutated from more than one reactive path: addAll runs on the caller thread in doCallInner, while flushDeferredResumeMsgs() runs from doOnNext (possibly a scheduler/IO thread) and from reasoning(). ArrayList gives no cross-thread visibility or atomicity guarantee. If the resumed stream can emit or run the next reasoning step on different threads, this is a data race that could duplicate or miss messages. Consider CopyOnWriteArrayList (small lists, read-mostly) or draining via synchronized + List.copyOf, and note the chosen concurrency contract in the field javadoc.

oss-maintainer · 2026-09-25T16:47:23Z

+         */
+        private void discardDeferredResumeMsgs() {
+            if (!deferredResumeMsgs.isEmpty()) {
+                log.warn(


[Warning] discardDeferredResumeMsgs() drops user input with a WARN log as the only trace. If the resumed tool calls raise a second permission prompt, the user's accompanying message disappears from the conversation permanently. Suggestion: keep the deferred messages in the persisted pending-resume state (or attach them to the returned Msg / an event) so the next resume can flush them, instead of deleting them. If dropping is intentional product behavior, please say so in the PR description — it is a silent data-loss path otherwise.

oss-maintainer · 2026-09-25T16:47:25Z

+    }
+
+    @Test
+    void userMessageSentWithDeniedResumeReachesTheNextModelRequest() {


[Info] Test coverage is good (approved + denied resume, context and model-request assertions). One gap: there is no test for the discard path — a resumed tool call that triggers a second ask, so the turn ends with pending tool calls. Worth pinning whatever the intended behavior is (drop with warn vs. carry over), because it exercises doOnSuccess + discardDeferredResumeMsgs.

oss-maintainer · 2026-09-25T16:54:33Z

Review summary (accompanies my inline review comments: #3295 (review))

Summary

Fixes a real HITL gap: user messages sent together with a permission resume are now deferred until the resumed tool calls have results, so they never land between a tool_use and its tool_result. The approach (defer → flush at the next reasoning step → discard on turn end) is sound and well tested.

Two things keep me from approving this run:

CLA is not signed yet (license/cla: pending) — the PR cannot be merged until it is; see the reminder comment.
CI — build (ubuntu-latest) failed on ToolConfirmationCoordinatorTest.replacementTurnLeaseCannotReleaseOldTicketAndMayReuseToolUseId (CannotStubVoidMethodWithReturnValue on appendSessionEvent), in agentscope-service, i.e. outside this PR's diff. main is green at ea78c3172, so please rebase onto the latest main and re-run; if the failure persists after rebase, flag it — it would look like a cross-module flake worth its own issue.

Inline notes cover the ArrayList cross-thread mutation, the silent-discard data-loss path, and a missing discard-path test.

Automated review by github-manager-bot

Access deferredResumeMsgs through synchronized methods, and stop discarding the messages when the turn ends with pending tool calls: they stay queued until the next flush. Add a test for a resume that raises a second prompt.

Ashfaqbs · 2026-09-26T07:16:55Z

Pushed e706863 for the review points.

Thread safety: deferredResumeMsgs is now only touched through synchronized methods, and the field javadoc says so.
Discard path: I removed discardDeferredResumeMsgs(), so messages are no longer dropped. If tool calls are still pending when a turn ends, they stay queued until the next flush. They are held in memory only and are not persisted.
Test for a second prompt: added userMessageIsPlacedBeforeTheSecondPromptWhenTheResumedCallAsksAgain. While writing it I found that after an approved resume the message is flushed at the start of the next reasoning step, before the second tool call. It therefore does not reach the discard path in that scenario. The path only applies when several tool calls are asking and only some are confirmed, and I have not added a test for that partial-confirm case.

ReActAgentHitlTest passes locally (22 tests). The two build jobs failed in service-dataplane, in ToolConfirmationCoordinatorTest.replacementTurnLeaseCannotReleaseOldTicketAndMayReuseToolUseId with a Mockito CannotStubVoidMethodWithReturnValue. That test is not touched by this PR, and I have not checked whether it also fails on main.

codecov · 2026-09-26T07:32:01Z

Codecov Report

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

Files with missing lines	Patch %	Lines
...e/src/main/java/io/agentscope/core/ReActAgent.java	80.00%	1 Missing and 3 partials ⚠️

📢 Thoughts on this report? Let us know!

oss-maintainer

Summary

Re-review of e706863e (previous review was on 91486570). The fix is correct and all earlier review points are handled: deferredResumeMsgs is now only touched through synchronized deferResumeMsgs() / flushDeferredResumeMsgs(), so the caller-thread vs. doOnNext race is gone; the discard path (silent loss of a user message with only a WARN) was replaced by keep-and-flush-after-the-next-resume; and userMessageIsPlacedBeforeTheSecondPromptWhenTheResumedCallAsksAgain now covers the second-prompt case that had no coverage before. The flush guard on MessageUtils.pendingToolUseIds() keeps the user turn from landing between a tool_use and its tool_result, which is the Anroid/Anthropic-style ordering constraint this PR targets.

[Info] agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java:1964 — the deferred list is per-CallExecution and in-memory only, which the javadoc now states explicitly. Worth being aware that a resume which terminates before reaching the next reasoning step (exception path, or a returnDirect tool that ends the turn) leaves those messages unflushed with the execution instance; the current tests cover the ask/ask/deny paths, not that edge. Not blocking.

LGTM, CLA signed.

CI

build (ubuntu-latest), Check License, codecov/patch ✅; build (windows-latest) still in progress at the time of this review.

Automated review by github-manager-bot

…ps-user-messages # Conflicts: # agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java

Ashfaqbs · 2026-10-10T14:48:32Z

Merged main to resolve the conflict. Only one file conflicted (ReActAgent.java): main added soForceToolChoiceCount right where this PR adds deferredResumeMsgs on CallExecution — unrelated fields landing in the same spot, so both are kept.

Verified: mvn -T1 -pl agentscope-core -am test (243 passed, 1 pre-existing skip) and spotless:check both green after the merge.

oss-maintainer reviewed Sep 25, 2026

View reviewed changes

oss-maintainer approved these changes Sep 26, 2026

View reviewed changes

oss-maintainer mentioned this pull request Sep 28, 2026

[Bug]: User messages sent together with a permission-HITL resume are silently dropped #3294

Open

Merge remote-tracking branch 'upstream/main' into fix/hitl-resume-dro…

08b32c1

…ps-user-messages # Conflicts: # agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java

Navigation Menu

Repository navigation

fix(agent): keep user messages sent with a permission-HITL resume - #3295

fix(agent): keep user messages sent with a permission-HITL resume#3295
Ashfaqbs wants to merge 3 commits into
agentscope-ai:mainfrom
Ashfaqbs:fix/hitl-resume-drops-user-messages

Ashfaqbs commented Sep 25, 2026

Uh oh!

CLAassistant commented Sep 25, 2026 •

edited

Loading

Uh oh!

oss-maintainer commented Sep 25, 2026

Uh oh!

oss-maintainer left a comment

Uh oh!

oss-maintainer Sep 25, 2026

Uh oh!

oss-maintainer Sep 25, 2026

Uh oh!

oss-maintainer Sep 25, 2026

Uh oh!

oss-maintainer commented Sep 25, 2026

Uh oh!

Ashfaqbs commented Sep 26, 2026

Uh oh!

codecov Bot commented Sep 26, 2026

Uh oh!

oss-maintainer left a comment

Uh oh!

Ashfaqbs commented Oct 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Ashfaqbs commented Sep 25, 2026

AgentScope-Java Version

Description

Checklist

Uh oh!

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

Uh oh!

Uh oh!

oss-maintainer commented Sep 25, 2026

Uh oh!

oss-maintainer left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

oss-maintainer Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

oss-maintainer Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

oss-maintainer Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

oss-maintainer commented Sep 25, 2026

Review summary (accompanies my inline review comments: #3295 (review))

Summary

Uh oh!

Ashfaqbs commented Sep 26, 2026

Uh oh!

codecov Bot commented Sep 26, 2026

Codecov Report

Uh oh!

oss-maintainer left a comment

Choose a reason for hiding this comment

Summary

CI

Uh oh!

Ashfaqbs commented Oct 10, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CLAassistant commented Sep 25, 2026 •

edited

Loading