fix(core): prevent pending recovery from consuming HITL approvals

[hanydd]

AgentScope-Java Version

2.0.1-SNAPSHOT

Description

Problem

When enablePendingToolRecovery is enabled, a permission-gated tool call in the ASKING state is treated as an ordinary dangling tool call before the incoming ConfirmResult is read. Recovery writes a synthetic failure result first, clears the pending ID, and routes the approval message through normal user-message processing.

As a result:

• approving the tool does not execute it;
• denying the tool does not produce a DENIED result;
• modified arguments and accepted permission rules are ignored; and
• regular user input can silently bypass a pending confirmation instead of reporting that approval is still required.

Background

Pending tool recovery was introduced in #956 as an explicit opt-in mechanism for repairing orphaned tool calls. During that PR, maintainers called out that automatic result patching must not hide incorrect HITL usage or create unintended resume paths. The merged implementation therefore skipped recovery when callers supplied manual ToolResultBlocks and kept the feature disabled by default.

Permission HITL now resumes through ConfirmResult objects stored under Msg.METADATA_CONFIRM_RESULTS, while pending recovery still recognizes only ToolResultBlock input. With enablePendingToolRecovery=true, an ASKING tool call is consequently patched as a failed orphan before its ConfirmResult can be applied. Approval, denial, modified arguments, and accepted permission rules are then ignored.

Relevant #956 discussions:

• HITL concerns around automatic result patching: fix: handle tool execution timeout/error causing IllegalStateExceptio… #956 (comment)
• Proposed separation between HITL pauses and orphan recovery: fix: handle tool execution timeout/error causing IllegalStateExceptio… #956 (comment)
• Decision to keep pending recovery explicitly opt-in: fix: handle tool execution timeout/error causing IllegalStateExceptio… #956 (comment)

Changes

• Detect confirmation metadata before running pending tool recovery, so approval input reaches the existing Permission HITL flow.
• Exclude ToolCallState.ASKING calls from orphan recovery as a state-level safeguard.
• Add regression coverage for approval, denial, and regular user input while recovery is enabled.

This does not change the recovery default, introduce a new policy, or alter recovery for ordinary orphaned tool calls.

Validation

Result: all tests passed, including the existing pending recovery scenarios.

Checklist

Please check the following items before code is ready to be reviewed.

• Code has been formatted with mvn spotless:apply
• All tests are passing (mvn test) — focused HITL and pending recovery tests pass; the full repository suite was not run
• Javadoc comments are complete and follow project conventions (no public API changes)
• Related documentation has been updated (not needed for this internal bug fix)
• Code is ready for review (draft)

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...e/src/main/java/io/agentscope/core/ReActAgent.java	92.30%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

This PR modifies 2 file(s) (+207 -10).

Reviewed the changes and they look reasonable overall. No critical issues found.

---

Automated review by github-manager-bot

[AgentScopeJavaBot]

🤖 AI Review

Fixes pending-tool-recovery consuming HITL approvals. Dual-layer protection: outer skip on ConfirmResult, inner filter ASKING tools. 3 tests. Concern: hasConfirmResultInput lacks null guard on getMetadata().

[AgentScopeJavaBot]

[minor] msg.getMetadata().containsKey() lacks null guard (getMetadata() can return null). Use already-extracted confirmResults.isEmpty() instead.

[hanydd]

正常的 Msg 构造路径不会在这里产生 NPE：构造器始终将 metadata 初始化为 HashMap，传入 null 时得到的也是空 map。这里已经补上 null guard，与 getMetadata() 当前的 Javadoc 保持一致。没有改成 confirmResults.isEmpty()，因为两者语义不同：空或格式错误的确认载荷会让 confirmResults 为空，但 metadata key 的存在仍表示这是一次审批输入。此时必须跳过 recovery，避免同一批次中的其他 pending tool call 被提前补成失败结果。

[AgentScopeJavaBot]

✅ Verified as addressed in f3ba2a3: Null guard added via .filter(Objects::nonNull) at line 1510, resolving the NPE risk. The alternative suggestion (use confirmResults.isEmpty()) was deliberately rejected with valid semantic reasoning — metadata key presence vs confirmResults emptiness serve different purposes for recovery logic.

[AgentScopeJavaBot]

All previously raised review comments have been addressed.