Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hanydd wants to merge 3 commits into
base: main
Choose a base branch
from
+208 −10
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02ce18a
fix(core): prevent pending recovery from consuming HITL approvals
hanydd Jul 11, 2026
8418787
test(core): cover modified and partial HITL confirmations
hanydd Jul 11, 2026
a183e46
fix(core): guard null confirmation metadata
hanydd Jul 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions agentscope-core/src/main/java/io/agentscope/core/ReActAgent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1501,9 +1501,21 @@ private Mono<Msg> doCallInner(List<Msg> msgs) {
msgs = List.of();
}

List<ConfirmResult> confirmResults = extractConfirmResults(msgs);
boolean hasConfirmResultInput =
msgs != null

@AgentScopeJavaBot AgentScopeJavaBot Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hanydd hanydd Jul 12, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@AgentScopeJavaBot AgentScopeJavaBot Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

&& msgs.stream()
.map(Msg::getMetadata)
.filter(Objects::nonNull)
.anyMatch(
metadata ->
metadata.containsKey(
Msg.METADATA_CONFIRM_RESULTS));

// Pending-tool-call recovery: auto-patch orphaned pending tool calls with synthetic
// error results so the agent can continue instead of crashing.
if (enablePendingToolRecovery) {
// error results so the agent can continue instead of crashing. Confirmation input
// must be handled by the permission HITL flow below instead of recovery.
if (enablePendingToolRecovery && !hasConfirmResultInput) {
maybePatchPendingToolCalls(msgs);
}

Expand All @@ -1519,7 +1531,6 @@ private Mono<Msg> doCallInner(List<Msg> msgs) {
// ConfirmResults (via Msg.METADATA_CONFIRM_RESULTS) before we can proceed.
List<ToolUseBlock> asking = askingToolCalls();
if (!asking.isEmpty()) {
List<ConfirmResult> confirmResults = extractConfirmResults(msgs);
if (confirmResults.isEmpty()) {
String pendingSummary =
asking.stream()
Expand Down Expand Up @@ -1702,18 +1713,22 @@ private void maybePatchPendingToolCalls(List<Msg> msgs) {
if (userProvidedResults) {
return;
}
log.warn(
"Pending tool calls detected without results, auto-generating error results."
+ " Pending IDs: {}",
pendingIds);
Msg lastAssistant = findLastAssistantMsg();
if (lastAssistant == null) {
return;
}
List<ToolUseBlock> pendingToolCalls =
lastAssistant.getContentBlocks(ToolUseBlock.class).stream()
.filter(toolUse -> pendingIds.contains(toolUse.getId()))
.filter(toolUse -> toolUse.getState() != ToolCallState.ASKING)
.toList();
if (pendingToolCalls.isEmpty()) {
return;
}
log.warn(
"Pending tool calls detected without results, auto-generating error results."
+ " Pending IDs: {}",
pendingToolCalls.stream().map(ToolUseBlock::getId).toList());
for (ToolUseBlock toolCall : pendingToolCalls) {
ToolResultBlock errorResult =
buildErrorToolResult(
Expand Down
189 changes: 186 additions & 3 deletions agentscope-core/src/test/java/io/agentscope/core/agent/ReActAgentHitlTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,16 @@ private static ReActAgent buildAgent(ChatModelBase model, Toolkit toolkit) {
return ReActAgent.builder().name("asst").model(model).toolkit(toolkit).build();
}

private static ReActAgent buildAgentWithPendingToolRecovery(
ChatModelBase model, Toolkit toolkit) {
return ReActAgent.builder()
.name("asst")
.model(model)
.toolkit(toolkit)
.enablePendingToolRecovery(true)
.build();
}

private static int indexOf(List<AgentEvent> events, Class<?> type) {
for (int i = 0; i < events.size(); i++) {
if (type.isInstance(events.get(i))) {
Expand All @@ -200,10 +210,12 @@ private static int countOf(List<AgentEvent> events, Class<?> type) {
}

private static Msg confirmMsg(boolean confirmed, ToolUseBlock toolCall) {
return confirmMsg(List.of(new ConfirmResult(confirmed, toolCall, null)));
}

private static Msg confirmMsg(List<ConfirmResult> confirmResults) {
Map<String, Object> meta = new HashMap<>();
meta.put(
Msg.METADATA_CONFIRM_RESULTS,
List.of(new ConfirmResult(confirmed, toolCall, null)));
meta.put(Msg.METADATA_CONFIRM_RESULTS, confirmResults);
return Msg.builder()
.name("user")
.role(MsgRole.USER)
Expand Down Expand Up @@ -320,6 +332,177 @@ void askingToolResumeWithDeniedConfirmResultProducesDeniedToolResult() {
assertTrue(foundDenied, "expected a DENIED ToolResultBlock for the rejected tool");
}

@Test
void pendingToolRecoveryDoesNotConsumeConfirmedAskingTool() {
ChatModelBase model =
new ScriptedModel(
List.of(
() -> Flux.just(toolUseResponse("tc1", "ask", "ping")),
() -> Flux.just(textResponse("done"))));
ReActAgent agent =
buildAgentWithPendingToolRecovery(model, toolkitWith(new AskingTool("ask")));

Msg first = agent.call(List.of()).block();
ToolUseBlock asking = first.getContentBlocks(ToolUseBlock.class).get(0);

agent.call(List.of(confirmMsg(true, asking))).block();

boolean foundExecutedResult =
agent.getAgentState().getContext().stream()
.flatMap(m -> m.getContentBlocks(ToolResultBlock.class).stream())
.filter(tr -> "tc1".equals(tr.getId()))
.flatMap(tr -> tr.getOutput().stream())
.filter(TextBlock.class::isInstance)
.map(TextBlock.class::cast)
.anyMatch(text -> "executed:ping".equals(text.getText()));
assertTrue(foundExecutedResult, "confirmed ASKING tool should execute normally");
}

@Test
void pendingToolRecoveryDoesNotConsumeDeniedAskingTool() {
ChatModelBase model =
new ScriptedModel(
List.of(
() -> Flux.just(toolUseResponse("tc1", "ask", "ping")),
() -> Flux.just(textResponse("done"))));
ReActAgent agent =
buildAgentWithPendingToolRecovery(model, toolkitWith(new AskingTool("ask")));

Msg first = agent.call(List.of()).block();
ToolUseBlock asking = first.getContentBlocks(ToolUseBlock.class).get(0);

agent.call(List.of(confirmMsg(false, asking))).block();

boolean foundDenied =
agent.getAgentState().getContext().stream()
.flatMap(m -> m.getContentBlocks(ToolResultBlock.class).stream())
.anyMatch(
tr ->
"tc1".equals(tr.getId())
&& tr.getState() == ToolResultState.DENIED);
assertTrue(foundDenied, "denied ASKING tool should produce a DENIED result");
}

@Test
void pendingToolRecoveryPreservesModifiedArgumentsFromConfirmation() {
ChatModelBase model =
new ScriptedModel(
List.of(
() -> Flux.just(toolUseResponse("tc1", "ask", "original")),
() -> Flux.just(textResponse("done"))));
ReActAgent agent =
buildAgentWithPendingToolRecovery(model, toolkitWith(new AskingTool("ask")));

Msg first = agent.call(List.of()).block();
ToolUseBlock asking = first.getContentBlocks(ToolUseBlock.class).get(0);
ToolUseBlock modified =
ToolUseBlock.builder()
.id(asking.getId())
.name(asking.getName())
.input(Map.of("query", "modified"))
.content(asking.getContent())
.metadata(asking.getMetadata())
.state(asking.getState())
.build();

agent.call(List.of(confirmMsg(true, modified))).block();

boolean foundModifiedResult =
agent.getAgentState().getContext().stream()
.flatMap(m -> m.getContentBlocks(ToolResultBlock.class).stream())
.filter(tr -> "tc1".equals(tr.getId()))
.flatMap(tr -> tr.getOutput().stream())
.filter(TextBlock.class::isInstance)
.map(TextBlock.class::cast)
.anyMatch(text -> "executed:modified".equals(text.getText()));
assertTrue(foundModifiedResult, "confirmed tool should execute with modified arguments");
}

@Test
void pendingToolRecoveryReasksForUnconfirmedAskingTools() {
ChatModelBase model =
new ScriptedModel(
List.of(
() ->
Flux.just(
ChatResponse.builder()
.content(
List.<ContentBlock>of(
ToolUseBlock.builder()
.id("tc1")
.name("ask")
.input(
Map.of(
"query",
"first"))
.build(),
ToolUseBlock.builder()
.id("tc2")
.name("ask")
.input(
Map.of(
"query",
"second"))
.build()))
.build())));
ReActAgent agent =
buildAgentWithPendingToolRecovery(model, toolkitWith(new AskingTool("ask")));

Msg first = agent.call(List.of()).block();
List<ToolUseBlock> asking =
first.getContentBlocks(ToolUseBlock.class).stream()
.filter(t -> t.getState() == ToolCallState.ASKING)
.toList();
assertEquals(2, asking.size());

Msg resumed =
agent.call(
List.of(
confirmMsg(
List.of(
new ConfirmResult(
true, asking.get(0), null)))))
.block();

assertEquals(GenerateReason.PERMISSION_ASKING, resumed.getGenerateReason());
List<ToolUseBlock> remaining =
resumed.getContentBlocks(ToolUseBlock.class).stream()
.filter(t -> t.getState() == ToolCallState.ASKING)
.toList();
assertEquals(1, remaining.size());
assertEquals("tc2", remaining.get(0).getId());

long remainingResults =
agent.getAgentState().getContext().stream()
.flatMap(m -> m.getContentBlocks(ToolResultBlock.class).stream())
.filter(tr -> "tc2".equals(tr.getId()))
.count();
assertEquals(0, remainingResults, "unconfirmed ASKING tool must not be auto-patched");
}

@Test
void pendingToolRecoveryDoesNotSilentlySkipAskingToolForRegularPrompt() {
ChatModelBase model =
new ScriptedModel(List.of(() -> Flux.just(toolUseResponse("tc1", "ask", "ping"))));
ReActAgent agent =
buildAgentWithPendingToolRecovery(model, toolkitWith(new AskingTool("ask")));

agent.call(List.of()).block();

assertThrows(
Throwable.class,
() ->
agent.call(
List.of(
Msg.builder()
.name("user")
.role(MsgRole.USER)
.textContent("new prompt")
.build()))
.block(),
"regular input must not bypass an ASKING tool call");
}

@Test
void askingToolWithoutConfirmResultOnResumeThrows() {
ChatModelBase model =
Expand Down
Loading