Repository navigation
fix(harness): isolate subagents from parent frontend tools #3357
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,9 @@ | |
| import io.agentscope.core.event.AgentEventEmitter; | ||
| import io.agentscope.core.event.AgentStartEvent; | ||
| import io.agentscope.core.event.SubagentExposedEvent; | ||
| import io.agentscope.core.message.GenerateReason; | ||
| import io.agentscope.core.message.Msg; | ||
| import io.agentscope.core.message.ToolUseBlock; | ||
| import io.agentscope.core.permission.PermissionContextState; | ||
| import io.agentscope.core.permission.PermissionMode; | ||
| import io.agentscope.core.permission.PermissionRule; | ||
|
|
@@ -469,22 +471,23 @@ public Mono<String> agentSpawn( | |
| spec = | ||
| new TaskRunSpec.LocalTaskRunSpec( | ||
| () -> { | ||
| Msg reply; | ||
| try { | ||
| reply = | ||
| manager.invokeAgent( | ||
| agent, | ||
| sessionId, | ||
| currentUserId, | ||
| capturedTask, | ||
| runtimeContext) | ||
| .block(); | ||
| } catch (RuntimeException e) { | ||
| return "Error: " | ||
| + (e.getMessage() != null | ||
| ? e.getMessage() | ||
| : e.getClass().getSimpleName()); | ||
| } | ||
| return textOf(reply); | ||
|
Collaborator
There was a problem hiding this comment. [Info] The three new
Author
There was a problem hiding this comment. @oss-maintainer, the guard is deliberately outside the preceding catch, so a suspended reply propagates as an IllegalStateException through the live task future. The added repository tests verify the exception type, persisted FAILED status and diagnostic, and task_output after reconstructing the repository. The existing persisted error-message format is unchanged; the suspended-tool diagnostic remains visible to consumers. |
||
| }); | ||
| } | ||
| taskRepository.putTask(runtimeContext, taskId, agentId, parentSessionId, spec); | ||
|
|
@@ -646,22 +649,23 @@ public Mono<String> agentSend( | |
| spec = | ||
| new TaskRunSpec.LocalTaskRunSpec( | ||
| () -> { | ||
| Msg reply; | ||
| try { | ||
| reply = | ||
| manager.invokeAgent( | ||
| spawned.agent(), | ||
| spawned.sessionId(), | ||
| currentUserId, | ||
| capturedMessage, | ||
| runtimeContext) | ||
| .block(); | ||
| } catch (RuntimeException e) { | ||
| return "Error: " | ||
| + (e.getMessage() != null | ||
| ? e.getMessage() | ||
| : e.getClass().getSimpleName()); | ||
| } | ||
| return textOf(reply); | ||
| }); | ||
| } | ||
| taskRepository.putTask( | ||
|
|
@@ -904,6 +908,7 @@ private Mono<String> execWithTimeoutPromotion( | |
| task, | ||
| spawned, | ||
| runtimeContext) | ||
| .map(AgentSpawnTool::requireNonSuspendedReply) | ||
| .contextWrite( | ||
| c -> | ||
| reactor.util.context.Context.of( | ||
|
|
@@ -1118,9 +1123,23 @@ private SpawnedAgent tryRestoreFromState( | |
| } | ||
|
|
||
| private static String textOf(Msg msg) { | ||
| requireNonSuspendedReply(msg); | ||
| return msg != null ? msg.getTextContent() : ""; | ||
| } | ||
|
|
||
| private static Msg requireNonSuspendedReply(Msg msg) { | ||
| if (msg != null && msg.getGenerateReason() == GenerateReason.TOOL_SUSPENDED) { | ||
| List<String> tools = | ||
| msg.getContentBlocks(ToolUseBlock.class).stream() | ||
| .map(ToolUseBlock::getName) | ||
| .distinct() | ||
| .toList(); | ||
| throw new IllegalStateException( | ||
| "Subagent suspended awaiting external tool execution: " + tools); | ||
| } | ||
| return msg; | ||
| } | ||
|
|
||
| private static String formatTimeoutPromoted(String taskId, long timeoutMs) { | ||
| return String.format( | ||
|
Comment on lines
+1126
to
1144
Collaborator
There was a problem hiding this comment. [Info] Coverage gap:
Author
There was a problem hiding this comment. @oss-maintainer, added six cases using a real WorkspaceTaskRepository, covering spawn/send/persistent reuse in background and timeout-promoted execution. They assert FAILED with no success result, the pending-tool diagnostic, and task_output through both the live repository and a fresh instance. All six fail against the original implementation and pass with the fix; the related 46-test suite also passes. The follow-up is in dcf3f51 and is ready for another review. |
||
| """ | ||
|
|
@@ -1646,22 +1665,23 @@ private Mono<String> execSpawnTask( | |
| spec = | ||
| new TaskRunSpec.LocalTaskRunSpec( | ||
| () -> { | ||
| Msg reply; | ||
| try { | ||
| reply = | ||
| manager.invokeAgent( | ||
| spawned.agent(), | ||
| spawned.sessionId(), | ||
| currentUserId, | ||
| capturedTask, | ||
| runtimeContext) | ||
| .block(); | ||
| } catch (RuntimeException e) { | ||
| return "Error: " | ||
| + (e.getMessage() != null | ||
| ? e.getMessage() | ||
| : e.getClass().getSimpleName()); | ||
| } | ||
| return textOf(reply); | ||
| }); | ||
| } | ||
| taskRepository.putTask( | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,221 @@ | ||
| /* | ||
| * Copyright 2024-2026 the original author or authors. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package io.agentscope.harness.agent.subagent; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
| import static org.junit.jupiter.api.Assertions.assertSame; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.mockito.ArgumentMatchers.any; | ||
| import static org.mockito.Mockito.mock; | ||
| import static org.mockito.Mockito.verify; | ||
| import static org.mockito.Mockito.when; | ||
|
|
||
| import io.agentscope.core.ReActAgent; | ||
| import io.agentscope.core.agent.Agent; | ||
| import io.agentscope.core.agent.EventType; | ||
| import io.agentscope.core.agent.RuntimeContext; | ||
| import io.agentscope.core.agent.StreamOptions; | ||
| import io.agentscope.core.agent.test.MockModel; | ||
| import io.agentscope.core.message.Msg; | ||
| import io.agentscope.core.message.TextBlock; | ||
| import io.agentscope.core.message.ToolResultBlock; | ||
| import io.agentscope.core.message.ToolResultState; | ||
| import io.agentscope.core.message.ToolUseBlock; | ||
| import io.agentscope.core.middleware.MiddlewareBase; | ||
| import io.agentscope.core.model.ChatResponse; | ||
| import io.agentscope.core.model.ToolSchema; | ||
| import io.agentscope.core.state.InMemoryAgentStateStore; | ||
| import io.agentscope.core.tool.AgentTool; | ||
| import io.agentscope.core.tool.SchemaOnlyTool; | ||
| import io.agentscope.core.tool.ToolMergeMode; | ||
| import io.agentscope.core.tool.ToolRequestConfig; | ||
| import io.agentscope.core.tool.Toolkit; | ||
| import io.agentscope.harness.agent.HarnessAgent; | ||
| import io.agentscope.harness.agent.memory.MemoryConfig; | ||
| import java.nio.file.Path; | ||
| import java.time.Duration; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.stream.Stream; | ||
| import org.junit.jupiter.api.io.TempDir; | ||
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.Arguments; | ||
| import org.junit.jupiter.params.provider.MethodSource; | ||
| import reactor.core.publisher.Mono; | ||
|
|
||
| class DefaultAgentManagerToolRequestConfigTest { | ||
|
|
||
| @TempDir Path workspace; | ||
|
|
||
| static Stream<Arguments> invocationModes() { | ||
| return Stream.of(ToolMergeMode.MERGE_EXTERNAL_PRIORITY, ToolMergeMode.EXTERNAL_ONLY) | ||
| .flatMap( | ||
| mode -> | ||
| Stream.of(false, true) | ||
| .flatMap( | ||
| streaming -> | ||
| Stream.of(false, true) | ||
| .map( | ||
| harness -> | ||
| Arguments.of( | ||
| mode, | ||
| streaming, | ||
| harness)))); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("invocationModes") | ||
| void childUsesItsOwnTools(ToolMergeMode mode, boolean streaming, boolean harness) { | ||
| Map<String, Object> parameters = Map.of("type", "object", "properties", Map.of()); | ||
| ToolRequestConfig config = | ||
| new ToolRequestConfig( | ||
| Map.of( | ||
| "choose_x", | ||
| new SchemaOnlyTool("choose_x", "Client choice", parameters), | ||
| "server_tool", | ||
| new SchemaOnlyTool("server_tool", "Client override", parameters)), | ||
| mode); | ||
| RuntimeContext parent = | ||
| RuntimeContext.builder() | ||
| .sessionId("parent") | ||
| .userId("user") | ||
| .runId("parent-run") | ||
| .toolRequestConfig(config) | ||
| .build(); | ||
| Toolkit toolkit = new Toolkit(); | ||
| AgentTool serverTool = mock(AgentTool.class); | ||
| when(serverTool.getName()).thenReturn("server_tool"); | ||
| when(serverTool.getDescription()).thenReturn("Server tool"); | ||
| when(serverTool.getParameters()).thenReturn(parameters); | ||
| when(serverTool.callAsync(any())) | ||
| .thenReturn( | ||
| Mono.just( | ||
| ToolResultBlock.of( | ||
| TextBlock.builder().text("server result").build()))); | ||
| toolkit.registerTool(serverTool); | ||
| AtomicInteger turns = new AtomicInteger(); | ||
| MockModel model = | ||
| new MockModel( | ||
| messages -> { | ||
| int turn = turns.getAndIncrement(); | ||
| if (turn < 2) { | ||
| String name = turn == 0 ? "server_tool" : "choose_x"; | ||
| return List.of( | ||
| ChatResponse.builder() | ||
| .content( | ||
| List.of( | ||
| ToolUseBlock.builder() | ||
| .id(name) | ||
| .name(name) | ||
| .input(Map.of()) | ||
| .content("{}") | ||
| .build())) | ||
| .build()); | ||
| } | ||
| return List.of( | ||
| ChatResponse.builder() | ||
| .content( | ||
| List.of( | ||
| TextBlock.builder() | ||
| .text("child done") | ||
| .build())) | ||
| .build()); | ||
| }); | ||
| AtomicReference<RuntimeContext> observed = new AtomicReference<>(); | ||
| MiddlewareBase recorder = | ||
| new MiddlewareBase() { | ||
| @Override | ||
| public Mono<String> onSystemPrompt( | ||
| Agent agent, RuntimeContext ctx, String prompt) { | ||
| observed.set(ctx); | ||
| return Mono.just(prompt); | ||
| } | ||
| }; | ||
| Agent child = | ||
| harness | ||
| ? HarnessAgent.builder() | ||
| .name("child") | ||
| .model(model) | ||
| .toolkit(toolkit) | ||
| .workspace(workspace) | ||
| .stateStore(new InMemoryAgentStateStore()) | ||
| .memory( | ||
| MemoryConfig.builder() | ||
| .flushTrigger(MemoryConfig.FlushTrigger.never()) | ||
| .build()) | ||
| .middlewares(List.of(recorder)) | ||
| .build() | ||
| : ReActAgent.builder() | ||
| .name("child") | ||
| .model(model) | ||
| .toolkit(toolkit) | ||
| .middlewares(List.of(recorder)) | ||
| .build(); | ||
| DefaultAgentManager manager = new DefaultAgentManager(List.of(), null); | ||
| try { | ||
| Msg reply = | ||
| streaming | ||
| ? manager.invokeAgentStream( | ||
| child, | ||
| "child-session", | ||
| "user", | ||
| "go", | ||
| null, | ||
| StreamOptions.defaults(), | ||
| parent) | ||
| .filter( | ||
| event -> | ||
| event.isLast() | ||
| && event.getType() | ||
| == EventType.AGENT_RESULT) | ||
| .map(event -> event.getMessage()) | ||
| .last() | ||
| .block(Duration.ofSeconds(10)) | ||
| : manager.invokeAgent(child, "child-session", "user", "go", parent) | ||
| .block(Duration.ofSeconds(10)); | ||
| assertNotNull(reply); | ||
| assertEquals("child done", reply.getTextContent()); | ||
| assertEquals(3, model.getCallCount()); | ||
| List<String> names = model.getLastTools().stream().map(ToolSchema::getName).toList(); | ||
| assertTrue(names.contains("server_tool")); | ||
| assertFalse(names.contains("choose_x")); | ||
| verify(serverTool).callAsync(any()); | ||
| assertTrue( | ||
| model.getLastMessages().stream() | ||
| .flatMap(msg -> msg.getContentBlocks(ToolResultBlock.class).stream()) | ||
| .anyMatch( | ||
| result -> | ||
| "choose_x".equals(result.getId()) | ||
| && result.getState() == ToolResultState.ERROR)); | ||
| assertNotNull(observed.get()); | ||
| assertNull(observed.get().getToolRequestConfig()); | ||
| assertEquals("child-session", observed.get().getSessionId()); | ||
| assertEquals("user", observed.get().getUserId()); | ||
| assertEquals("parent-run", observed.get().getRunId()); | ||
| assertSame(config, parent.getToolRequestConfig()); | ||
| assertEquals("parent", parent.getSessionId()); | ||
| } finally { | ||
| if (child instanceof HarnessAgent harnessAgent) { | ||
| harnessAgent.close(); | ||
| } | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Warning] Compatibility: with this change, any existing deployment that relied on a subagent reaching the parent client's frontend tools (parent
ToolRequestConfiginherited via AG-UI / per-call config) will now fail with anIllegalStateExceptioninstead of silently returning an empty reply. The direction is right (child must not consume parent-client tools), but consider calling this out in the release notes / migration doc as a behavior change, not only indocs/v2/*/harness/subagent.md.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oss-maintainer, thanks for the review. Added English and Chinese compatibility and migration notes under Unreleased in dcf3f51. They distinguish removing inherited client tools from rejecting replies that still carry TOOL_SUSPENDED, and explain that client interactions should stay in the parent or be replaced with backend-executable child tools. Please take another look at the migration wording.