feat(SubAgentTool) Human-in-the-Loop (HITL) Support for SubAgentTool#723
feat(SubAgentTool) Human-in-the-Loop (HITL) Support for SubAgentTool#723wuji1428 wants to merge 8 commits intoagentscope-ai:mainfrom
Conversation
…d context classes and test cases.
…action (HITL) with sub-agents.
Summary of ChangesHello @wuji1428, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the AgentScope-Java framework by introducing comprehensive Human-in-the-Loop (HITL) support for Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for Human-in-the-Loop (HITL) support in SubAgentTool. The changes are extensive, adding new classes like SubAgentContext, SubAgentHook, and SubAgentPendingStore to manage the state of suspended sub-agents. The implementation correctly propagates suspension states and allows for resumption. Overall, this is a well-designed feature. I've identified a few areas for improvement, including a critical bug related to state loading that could cause a NullPointerException, a race condition in state consumption, and some minor issues in documentation and error messages. Addressing these points will make the implementation more robust and reliable.
| public void loadFrom(Session session, SessionKey sessionKey) { | ||
| this.pendingStore = null; | ||
| session.get(sessionKey, "subagent_context", SubAgentPendingStore.class) | ||
| .ifPresent(state -> this.pendingStore = state); | ||
| } |
There was a problem hiding this comment.
There is a critical bug here. If the session does not contain the subagent_context state, this.pendingStore will remain null after being explicitly set to null on line 307. Any subsequent call to a method on this SubAgentContext instance will result in a NullPointerException.
You should ensure pendingStore is always initialized, for example by providing a default new instance if one is not found in the session.
public void loadFrom(Session session, SessionKey sessionKey) {
this.pendingStore = session.get(sessionKey, "subagent_context", SubAgentPendingStore.class)
.orElse(new SubAgentPendingStore());
}| public Optional<SubAgentPendingContext> consumePendingResult(String toolId) { | ||
| if (!pendingStore.contains(toolId)) { | ||
| return Optional.empty(); | ||
| } | ||
| SubAgentPendingContext pending = | ||
| new SubAgentPendingContext( | ||
| toolId, | ||
| pendingStore.getSessionId(toolId), | ||
| pendingStore.getPendingResults(toolId)); | ||
| clearToolResult(toolId); | ||
| return Optional.of(pending); | ||
| } |
There was a problem hiding this comment.
This implementation of consumePendingResult is not atomic and has a race condition. Between checking for the tool's existence and clearing it, another thread could modify the state. This can lead to inconsistent state or lost updates in a concurrent environment.
To fix this, you should make the consumption atomic. I recommend modifying SubAgentPendingStore.remove() to return the removed SubAgentPendingContext (since ConcurrentHashMap.remove() does this atomically). Then, this method can be simplified to a single, thread-safe call.
public Optional<SubAgentPendingContext> consumePendingResult(String toolId) {
if (toolId == null) {
return Optional.empty();
}
return Optional.ofNullable(pendingStore.remove(toolId));
}| public void remove(String toolId) { | ||
| if (toolId != null) { | ||
| toolIdToContext.remove(toolId); | ||
| } | ||
| } |
There was a problem hiding this comment.
To support an atomic consume operation and fix the race condition in SubAgentContext, this remove method should return the value that was removed. ConcurrentHashMap.remove() already provides this functionality atomically.
| public void remove(String toolId) { | |
| if (toolId != null) { | |
| toolIdToContext.remove(toolId); | |
| } | |
| } | |
| public SubAgentPendingContext remove(String toolId) { | |
| if (toolId != null) { | |
| return toolIdToContext.remove(toolId); | |
| } | |
| return null; | |
| } |
| throw new IllegalStateException( | ||
| "SubAgent HITL is not enabled. Please enable it via" | ||
| + " builder.enableSubAgentHitl(true)"); |
There was a problem hiding this comment.
There's a typo in the exception message. The builder method is named enableSubAgentHITL, but the message suggests enableSubAgentHitl. To avoid confusion, it's best to match the method name exactly.
"SubAgent HITL is not enabled. Please enable it via" + " builder.enableSubAgentHITL(true)");| * to handle sub-agent suspension and resumption. This allows sub-agents to be | ||
| * suspended when they need user input and resumed when the user provides results. | ||
| * | ||
| * @param enableSubAgentHITL true to enable sub-agent HITL support (default: true) |
There was a problem hiding this comment.
The Javadoc states that the default value for enableSubAgentHITL is true, but the field is initialized to false on line 1122. The pull request description also mentions that HITL is disabled by default. Please correct the Javadoc to reflect the actual default value.
* @param enableSubAgentHITL true to enable sub-agent HITL support (default: false)| metadata.put(PREVIOUS_TOOL_RESULT, pendingResult.get()); | ||
| Map<String, Object> newInput = new HashMap<>(toolUse.getInput()); | ||
| newInput.put("session_id", pendingContext.get().sessionId()); | ||
| context.clearToolResult(toolUse.getId()); |
There was a problem hiding this comment.
The call to context.clearToolResult(toolUse.getId()) is redundant. The consumePendingResult method on line 129 already removes the pending context from the store. Removing this line will make the logic clearer.
docs/en/multi-agent/agent-as-tool.md
Outdated
| List<ToolResultBlock> cancelResults = pendingTools.stream() | ||
| .map(t -> ToolResultBlock.of(t.getId(), t.getName(), | ||
| TextBlock.builder().text("Operation cancelled").build())) | ||
| .toList(); | ||
| mainAgent.submitSubAgentResult(resultBlock.getId(), cancelResults); |
There was a problem hiding this comment.
There's a typo in the method name. For submitting a list of results, the method is submitSubAgentResults (plural). The example here uses submitSubAgentResult.
| List<ToolResultBlock> cancelResults = pendingTools.stream() | |
| .map(t -> ToolResultBlock.of(t.getId(), t.getName(), | |
| TextBlock.builder().text("Operation cancelled").build())) | |
| .toList(); | |
| mainAgent.submitSubAgentResult(resultBlock.getId(), cancelResults); | |
| List<ToolResultBlock> cancelResults = pendingTools.stream() | |
| .map(t -> ToolResultBlock.of(t.getId(), t.getName(), | |
| TextBlock.builder().text("Operation cancelled").build())) | |
| .toList(); | |
| mainAgent.submitSubAgentResults(resultBlock.getId(), cancelResults); |
docs/en/multi-agent/agent-as-tool.md
Outdated
| **Resume methods**: | ||
| - `mainAgent.call()` — Continue executing pending tools | ||
| - `mainAgent.submitSubAgentResult(String, ToolResultBlock)` — Submit single tool result | ||
| - `mainAgent.submitSubAgentResult(String, List<ToolResultBlock>)` — Submit multiple tool results No newline at end of file |
There was a problem hiding this comment.
There's a typo in the API reference. The method for submitting multiple results is submitSubAgentResults (plural), not submitSubAgentResult.
| - `mainAgent.submitSubAgentResult(String, List<ToolResultBlock>)` — Submit multiple tool results | |
| - `mainAgent.submitSubAgentResults(String, List<ToolResultBlock>)` — Submit multiple tool results |
docs/zh/multi-agent/agent-as-tool.md
Outdated
| .map(t -> ToolResultBlock.of(t.getId(), t.getName(), | ||
| TextBlock.builder().text("操作已取消").build())) | ||
| .toList(); | ||
| mainAgent.submitSubAgentResult(resultBlock.getId(), cancelResults); |
There was a problem hiding this comment.
There's a typo in the method name in this example. When submitting a list of results, the method should be submitSubAgentResults (plural).
| mainAgent.submitSubAgentResult(resultBlock.getId(), cancelResults); | |
| mainAgent.submitSubAgentResults(resultBlock.getId(), cancelResults); |
docs/zh/multi-agent/agent-as-tool.md
Outdated
|
|
||
| **恢复方法**： | ||
| - `ReActAgent.submitSubAgentResult(String, ToolResultBlock)` — 提交单个工具结果 | ||
| - `ReActAgent.submitSubAgentResult(String, List<ToolResultBlock>)` — 批量提交工具结果 No newline at end of file |
There was a problem hiding this comment.
There's a typo in the API reference. The method for submitting a list of tool results is submitSubAgentResults (plural).
| - `ReActAgent.submitSubAgentResult(String, List<ToolResultBlock>)` — 批量提交工具结果 | |
| - `ReActAgent.submitSubAgentResults(String, List<ToolResultBlock>)` — 批量提交工具结果 |
1. The `remove` method in the `SubAgentPendingStore` class now returns the removed object instead of void type. 2. Correspondingly, the places where this method is called in the `SubAgentContext` class have been updated, directly using the returned result for subsequent processing. 3. At the same time, some unnecessary method definitions, such as `clearToolResult`, have been cleaned up. 4. The relevant documentation and test cases have been updated to match the new API changes. These changes have made the code more concise and easier to maintain.
1 participant
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.8, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Background
In complex AI Agent application scenarios, it is common to use one Agent as a tool to be called by another Agent (via
SubAgentTool). When a sub-agent encounters an operation requiring manual confirmation (Human-in-the-Loop, HITL) during execution, the current framework cannot correctly propagate this suspension state to the main agent. This leads to a break in the entire execution chain.Related Issue Ref: #459
Typical Scenario
User -> Main Agent (Task Planning) -> SubAgent (Data Analysis) -> Tool (Query Database) // Requires manual confirmationIn this scenario, when the database query tool requires user confirmation, the sub-agent is suspended. However, the main agent is unaware of this state, and the user cannot provide the confirmation result to the sub-agent through the main agent.
Goals
Implement full HITL support for
SubAgentToolso that:SubAgentToolcompletes its task.Implementation Plan
1. Suspension State Propagation
When an internal tool of the sub-agent requires user confirmation,
SubAgentToolwill:GenerateReason.TOOL_SUSPENDED,REASONING_STOP_REQUESTED,ACTING_STOP_REQUESTED).ToolResultBlockcontaining suspension metadata:METADATA_SUSPENDED: Marks the suspension state.METADATA_SUBAGENT_SESSION_ID: Thesession_idof the sub-agent.METADATA_GENERATE_REASON: The reason for suspension.ToolUseBlocks pending confirmation within the sub-agent.2. Passing User Confirmation Results
Design Principles:
ReActAgentremains unchanged.ToolResultBlockprovided by the user is intended for the sub-agent's internal tools and should not be added to the main agent's memory.SessionIdmanagement is transparent to the user and handled automatically by the framework.Mechanism:
ReActAgent.registerSubAgentSessionIfNeeded()automatically registers the(toolId, sessionId)pair into theSubAgentContext.SubAgentContext.submitSubAgentResult(toolId, result).SubAgentHookintercepts theSubAgentToolcall during thePreActingEventphase and injects the pending result intoToolUseBlock.metadata.SubAgentToolinvokes theresume()method to continue the sub-agent's execution.3. Sub-Agent Resumption Mechanism
The
callAsync()method inSubAgentToolwill:ToolUseBlock.metadatacontainsPREVIOUS_TOOL_RESULT.session_idandtoolResults, then call theresume()method.resume()method loads the sub-agent's state, injects the tool results, and continues execution.4. Multi-turn Interaction Support
Support for multiple rounds of suspension and resumption is achieved through session mechanisms and state persistence:
session_id.session_idto load the state.SubAgentContextimplements theStateModuleinterface to support state persistence.Component Description
New Components
1. SubAgentHook
Responsibility: Inject pending sub-agent results before tool execution.
Workflow:
PreActingEvent.SubAgentContextbased ontoolId.PREVIOUS_TOOL_RESULTkey ofToolUseBlock.metadata.session_idintoToolUseBlock.input.2. SubAgentContext
Responsibility: Manage the pending states of sub-agent tool calls.
Core Functions:
setSessionId(toolId, sessionId): Register asession_id(must be done before result submission).submitSubAgentResult(toolId, result): Submit tool results.consumePendingResult(toolId): Consume and remove pending results.extractSessionId(result): Static method to extractsession_idfromToolResultBlock.isSubAgentResult(result): Static method to determine if a result belongs to a sub-agent.State Persistence: Implements
StateModuleforsaveTo()andloadFrom().3. SubAgentPendingStore
Responsibility: Thread-safe storage for pending states.
Data Structure:
Map<String, SubAgentPendingContext>(toolId -> context).Design: Uses
ConcurrentHashMapfor thread safety and implementsStatefor serialization.4. SubAgentPendingContext
Responsibility: Encapsulates the complete pending state for a single sub-agent tool.
Modified Components
1. SubAgentTool
New Features:
resume()method: Handles resumption logic.buildSuspendedResult(): Constructs results for suspended states.extractToolResults(): Extracts injected results from metadata.ReActAgent.2. SubAgentConfig
New Configuration:
enableHITL(boolean): Enable/disable HITL support (default:false).3. ReActAgent
New Features:
subAgentContextfield: Manages sub-agent HITL states.enableSubAgentHITL(boolean)Builder method: Enables HITL support.registerSubAgentSessionIfNeeded(): Automatically registerssessionId.configureSubAgentHitl(): Automatically configuresSubAgentContextandSubAgentHook.State Persistence:
StatePersistence.subAgentContextManaged(): Controls whetherSubAgentContextis persisted.Agent Scope
HITL support for
SubAgentToolis currently limited toReActAgent:SubAgentToolconstruction.ReActAgentwith HITL enabled will throw anIllegalArgumentException.Compatibility
enableHITL = false). Existing code requires no changes.SubAgentToolinstances as needed.Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)