Repository navigation
fix(harness): isolate subagents from parent frontend tools - #3357
wyi184246-creator wants to merge 4 commits into
Conversation
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @wyi184246-creator 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 Automated check by github-manager-bot |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Good, well-scoped fix for #3308: local subagent invocations no longer inherit the parent's per-call ToolRequestConfig (client/AG-UI frontend tools stay with the parent's client), and a suspended child reply is now surfaced as an explicit error instead of a silently successful empty reply. I verified on the PR head that all AgentSpawnTool result paths converge on the new guard (execLocalSync's three paths → execWithTimeoutPromotion's .map(requireNonSuspendedReply) / textOf(msg), promoted tasks via bridge.thenApply(textOf), and the three background LocalTaskRunSpec lambdas via textOf(reply)), and that RuntimeContext.Builder stores toolRequestConfig as a plain field so the explicit .toolRequestConfig(null) genuinely clears a value copied from the parent. Tests cover sync/stream/background/promoted × MERGE_EXTERNAL_PRIORITY/EXTERNAL_ONLY — thorough.
Findings are one compatibility call-out and two non-blocking suggestions (see inline comments).
license/cla check is still pending (CLA not signed). The code itself looks ready to merge once the CLA is signed and the maintainers are happy with the compatibility note.
Automated review by github-manager-bot
| // Frontend tools belong to the parent run's client. | ||
| .toolRequestConfig(null) |
There was a problem hiding this comment.
[Warning] Compatibility: with this change, any existing deployment that relied on a subagent reaching the parent client's frontend tools (parent ToolRequestConfig inherited via AG-UI / per-call config) will now fail with an IllegalStateException instead 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 in docs/v2/*/harness/subagent.md.
There was a problem hiding this comment.
@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.
| ? e.getMessage() | ||
| : e.getClass().getSimpleName()); | ||
| } | ||
| return textOf(reply); |
There was a problem hiding this comment.
[Info] The three new return textOf(reply); call sites sit after a catch (RuntimeException) that only stringifies e.getMessage(). The new IllegalStateException from requireNonSuspendedReply therefore loses its type and stack when rendered as "Error: " + message (and in the background-task path, when TaskRunSpec results are persisted). Pre-existing pattern, but since this PR deliberately introduces a structured failure mode here, wrapping with the exception class name (or keeping the cause) would make "suspended" distinguishable from other runtime failures downstream.
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.
| 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( |
There was a problem hiding this comment.
[Info] Coverage gap: AgentSpawnToolSuspensionTest asserts on the captured TaskRunSpec (execution().get() / future().get()), which validates the guard, but does not assert what TaskRepository/task-status consumers ultimately observe for a suspended background task. If any consumer catches and stores the exception string asynchronously, a suspended child could still surface as a blank/"" result there. A small test that drives the task-status read path (not just the spec) would close the loop on the issue #3308 report end-to-end.
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.
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Re-review on dcf3f51d. The follow-up commit resolves every point from my previous pass, and the change now reads as complete:
- [Warning] compatibility → addressed: EN + ZH release notes under Unreleased spell out that local subagents no longer inherit the parent's per-call
ToolRequestConfig, plus a migration note pointing at the subagent tool-execution-boundary section. - [Info] error-type flattening at
AgentSpawnTool:490→ accepted as designed: keepingrequireNonSuspendedReplyoutside the adjacentcatch (RuntimeException)is what lets theIllegalStateExceptionreach the live task future intact, and the new tests assert on that concrete type rather than a stringified message. - [Info] coverage gap at
AgentSpawnTool:1144→ addressed:suspendedTaskPersistsFailureVisibleToTaskOutputis exactly what was missing. Driving a realWorkspaceTaskRepositoryover spawn/send/reuse × direct/timeout-promoted, assertingTaskStatus.FAILEDwith a null result and the pending-tool diagnostic on the live task, the persistedTaskRecord, and the recovered view from a second repository, means a suspended child can no longer be reported as a successful empty reply anywhere a task consumer looks (TaskTool.taskOutputincluded).
license/cla is now green, so approving. One residual note below.
Findings
- [Info]
AgentSpawnToolSuspensionTest.java:131— the new test threads the failure throughlistTasks/getTask/readTaskRecord, which covers what the issue reports. The non-persisting paths (the earliersuspendedBackgroundChildFailsTask/suspendedPromotedChildFailsFuturecases) still assert only on the capturedTaskRunSpec, so if a future refactor changes what the mock repository records, those two stay green while behavior drifts. Not blocking — just worth knowing which assertions are load-bearing.
Verification
- CI on
dcf3f51d:build (ubuntu-latest),Check License,Check Module Sync,validate, codecov patch all pass;build (windows-latest)still pending at review time — worth a glance before merge since this test uses@TempDirand path-relative workspace files.
LGTM from my side.
Automated review by github-manager-bot
This branch has not been deployed
3 participants
AgentScope-Java Version
2.0.4-SNAPSHOT
Description
Fixes #3308.
Local subagents inherited the parent's per-call frontend tools even though they have no client to execute them. Calling one could suspend the child and report an empty successful reply to the parent.
Clear the inherited ToolRequestConfig when creating child invocation contexts, preserving identity and run context. Reject TOOL_SUSPENDED replies with a diagnostic naming the pending tools, including background tasks and calls promoted after a synchronous timeout. Update the English and Chinese subagent documentation.
Validation
Checklist