Skip to content

fix(subagent): dynamic subagent FAILED/CANCELLED payloads#2187

Open
BG0527 wants to merge 3 commits into
agentscope-ai:mainfrom
BG0527:fix-subagent-payloads
Open

fix(subagent): dynamic subagent FAILED/CANCELLED payloads#2187
BG0527 wants to merge 3 commits into
agentscope-ai:mainfrom
BG0527:fix-subagent-payloads

Conversation

@BG0527

@BG0527 BG0527 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

关联 issue

AgentScope-Java Version

2.0.1-SNAPSHOT

Description

Background

TaskCompletionCallback 原本是一个包含多个裸参数(rc, taskId, subAgentId, sessionId, result)的函数式接口,无法携带任务的终态状态(COMPLETED / FAILED / CANCELLED)和失败原因,消费者需要重新读取持久化的 TaskRecord 才能区分任务的最终状态。此外,WorkspaceTaskRepository 缺少对终态状态的本地追踪和孤儿任务清理机制。

Root Cause

回调签名中缺少 TaskStatuserrorMessage 字段,导致 HarnessAgentSubagentsMiddleware 在收到回调时无法可靠地区分任务是正常完成、失败还是被取消,只能依赖 result 是否为 null 进行粗略判断。

Fix

引入不可变的 TaskCompletionEvent record 替代原有的裸参数回调签名,新增 statuserrorMessage 字段,使消费者无需重新读取 TaskRecord 即可获取任务的权威终态信息。同步更新 WorkspaceTaskRepository 增加终态状态的本地追踪、心跳过终态任务、以及孤儿扫描中标记过期运行任务为 FAILED 的能力。

Changed files:

1. TaskCompletionEvent.java(新增)

新增不可变 record,封装 rc, taskId, subAgentId, sessionId, status, result, errorMessage,替代原来 TaskCompletionCallback 的裸参数列表。

2. SubagentsMiddleware.java

将回调签名从 (rc, taskId, subAgentId, sessionId, result) 更新为 (event) -> { ... },通过 event.status() 区分终态类型,构造不同的 HintItem 内容。

3. HarnessAgent.java

setCompletionCallback 的回调从裸参数改为接收 TaskCompletionEvent,使用 switch (event.status()) 为 COMPLETED / FAILED / CANCELLED 三种终态分别构造不同的通知消息格式。

4. WorkspaceTaskRepository.java

  • TaskCompletionCallback 接口签名更新为接收 TaskCompletionEvent
  • 新增 localTaskStatuses map 追踪本地任务的终态状态;
  • 心跳任务跳过已处于终态的任务;
  • 孤儿扫描中标记超时的 RUNNING 任务为 FAILED 并触发回调;
  • 所有回调触发点统一构造 TaskCompletionEvent

5. WorkspaceTaskRepositoryTest.java(新增,约 508 行)

覆盖 putTaskgetTasklistTaskscancelTaskheartbeatsweepOrphanedTasks 等场景的单元测试,包括会话隔离、终态不被覆盖、跨节点过滤、孤儿标记等边界条件。

Checklist

  • Code has been formatted with mvn spotless:apply
  • All tests are passing (mvn test)
  • Javadoc comments are complete and follow project conventions
  • Related documentation has been updated (e.g. links, examples, etc.)
  • Code is ready for review

@BG0527 BG0527 requested a review from a team July 14, 2026 02:29
@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

@oss-maintainer oss-maintainer left a comment

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.

Summary

Introduces TaskCompletionEvent record to replace bare-parameter callback, adding status and errorMessage fields so consumers can distinguish COMPLETED/FAILED/CANCELLED without re-reading TaskRecord.

Findings

  • [Info] TaskCompletionEvent record encapsulates all terminal state. Clean API design.
  • [Info] WorkspaceTaskRepository gains localTaskStatuses map for terminal state tracking. Good.
  • [Info] Heartbeat tasks skip terminal-state tasks. Correct optimization.
  • [Info] Orphan sweep marks timed-out RUNNING tasks as FAILED. Good cleanup.
  • [Info] 508-line test file covers all scenarios. Excellent coverage.

Verdict

Well-structured fix with thorough tests. LGTM.


Automated review by github-manager

@AgentScopeJavaBot AgentScopeJavaBot added bug Something isn't working area/harness agentscope-harness (test/runtime support) labels Jul 14, 2026

@AgentScopeJavaBot AgentScopeJavaBot left a comment

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.

🤖 AI Review

Solid refactoring that addresses issue #2159 by introducing an immutable TaskCompletionEvent record to replace bare callback parameters. The design correctly separates COMPLETED/FAILED/CANCELLED terminal states, allowing HarnessAgent and SubagentsMiddleware to construct distinct notification messages without re-reading TaskRecord.

The WorkspaceTaskRepository enhancements (terminal state tracking, heartbeat skip for terminated tasks, orphan sweep marking FAILED) are well-implemented. Test coverage spans all three terminal states with both callback-level and middleware-level tests.

Minor nits: (1) TaskCompletionEvent.status should have a @NonNull semantic constraint or Objects.requireNonNull guard. (2) terminatedTaskIds set grows unboundedly — consider a bounded cache for long-running services. (3) SWEEP_MARKER_PATH writes empty files that provide no info during ops troubleshooting — consider writing {"ts": ...} JSON. (4) The orphan scanner callback trigger path lacks dedicated unit test coverage.

* @param taskId task identifier
* @param subAgentId which subagent type executed this task
* @param sessionId parent session scope
* @param status terminal status — one of COMPLETED / FAILED / CANCELLED

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] The status field should carry a non-null semantic constraint. Consider adding Objects.requireNonNull(status) in a compact constructor, or documenting @code non-null in the Javadoc, since null status would break downstream switch(event.status()) logic.

@AgentScopeJavaBot AgentScopeJavaBot left a comment

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.

🤖 AI Review

Solid refactoring that addresses issue #2159 by introducing an immutable TaskCompletionEvent record to replace bare callback parameters. The design correctly separates COMPLETED/FAILED/CANCELLED terminal states, allowing HarnessAgent and SubagentsMiddleware to construct distinct notification messages without re-reading TaskRecord.

The WorkspaceTaskRepository enhancements (terminal state tracking, heartbeat skip for terminated tasks, orphan sweep marking FAILED) are well-implemented. Test coverage spans all three terminal states with both callback-level and middleware-level tests.

Minor nits: (1) TaskCompletionEvent.status should have a @NonNull semantic constraint or Objects.requireNonNull guard. (2) terminatedTaskIds set grows unboundedly — consider a bounded cache for long-running services. (3) SWEEP_MARKER_PATH writes empty files that provide no info during ops troubleshooting — consider writing {"ts": ...} JSON. (4) The orphan scanner callback trigger path lacks dedicated unit test coverage.

* @param taskId task identifier
* @param subAgentId which subagent type executed this task
* @param sessionId parent session scope
* @param status terminal status — one of COMPLETED / FAILED / CANCELLED

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] The status field should carry a non-null semantic constraint. Consider adding Objects.requireNonNull(status) in a compact constructor, or documenting @code non-null in the Javadoc, since null status would break downstream switch(event.status()) logic.

@BG0527 BG0527 force-pushed the fix-subagent-payloads branch from 926b625 to 4253e3a Compare July 15, 2026 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/harness agentscope-harness (test/runtime support) bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Dynamic subagent notifications drop FAILED/CANCELLED payloads

3 participants