Repository navigation
feat(harness): let custom subagent factories carry a declaration - #3442
guslegend0510 wants to merge 4 commits into
Conversation
A subagent registered with subagentFactory(name, description, factory) is stored without a SubagentDeclaration, so AgentSpawnTool never sees options such as persistSession. A declared subagent, on the other hand, is always built by the framework and cannot use private middleware, custom tools or an explicit Model. Users had to pick one of the two. Add HarnessAgent.Builder#subagentFactory(SubagentDeclaration, SubagentFactory): the factory builds the instance (and receives the parent RuntimeContext), while the declaration supplies the name, description and spawn-time options (persistSession, mode, hidden, exposeToUser, inheritParentPermissions). Remote declarations are rejected because they are invoked over HTTP and would bypass the factory. Fixes agentscope-ai#3440
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| (rc) -> custom.factory().apply(custom.name()), | ||
| null)); | ||
| custom.factory(), | ||
| custom.declaration())); |
There was a problem hiding this comment.
AgentSpawnTool.agentSpawn() derives the key from (parentSessionId, agentId, label) without userId. When a shared parent agent handles (alice, s1) and (bob, s1) with the same agent ID and label, the second spawn retrieves Alice’s cached child, even if the factory created a child configured for Bob. Passing Bob’s RuntimeContext does not replace tools, credentials, or workspace resources already bound to Alice’s instance.
Please include user identity in the persistent cache key and add a regression test using the same AgentSpawnTool for two users with the same parent session ID. It should verify separate child instances across users and reuse within the same user.
| // SubagentEntry directly with a B-0 SubagentFactory lambda. | ||
| (rc) -> custom.factory().apply(custom.name()), | ||
| null)); | ||
| custom.factory(), |
There was a problem hiding this comment.
With this custom factory registered alongside a persistSession=true declaration, it now enters the persistent reuse path in AgentSpawnTool.
manager.createAgentIfPresent() invokes the factory before agentsByKey.get(key). On a cache hit, the newly created child is discarded without being used or closed. A custom factory may allocate resources such as MCP clients or workspace indexes, so repeated persistent spawns can leave those resources unreleased.
Please check for an existing persistent child before creating a new instance. Extend declaredFactory_persistSessionReusesCustomHarnessAgent to assert that the factory is called once for two sequential spawns with the same persistent identity. Its current createdAgents cleanup closes every constructed child in the test, so it does not detect the discarded instance in production.
| .buildSubagentEntries(workspace); | ||
| AgentSpawnTool tool = new AgentSpawnTool(new DefaultAgentManager(entries, null), null, 0); | ||
| RuntimeContext ctx = | ||
| RuntimeContext.builder().userId("user").sessionId("parent-session").build(); |
There was a problem hiding this comment.
Could we add a regression test using the same AgentSpawnTool for two users with the same parent session ID, agent ID, and label? Have the factory bind a user marker to each child and verify that each user executes their own child, while repeated spawns by the same user reuse that child. The current test only uses one user, so it cannot detect a persistent cache key that omits userId.
| assertEquals( | ||
| lineValue(first, "agent_key: "), | ||
| lineValue(second, "agent_key: "), | ||
| "persistSession must reuse the deterministic key"); |
There was a problem hiding this comment.
Could we also assert that the factory is invoked exactly once for these two sequential persistent spawns, for example with an AtomicInteger incremented inside the factory? The matching agent_key and preserved conversation still pass if the second spawn constructs an extra child and discards it. The createdAgents cleanup closes that extra instance after the test, so the lifecycle regression remains undetected.
There was a problem hiding this comment.
Thanks for catching both issues. I've fixed them in d12469a:
- The persistent cache key now includes userId, and label lookup is scoped by user and parent session.
- Agent creation now uses computeIfAbsent, so persistent cache hits no longer invoke the factory.
Regression tests use the same AgentSpawnTool for Alice and Bob with matching session IDs, agent IDs, and labels. They verify separate user-bound children, reuse within each user, and exactly one factory invocation for both sequential and concurrent reuse.
The branch is also synced with main. Local mvn -B -T1 clean verify passed across all 97 modules, and the documentation checks passed.
Could you take another look?
There was a problem hiding this comment.
Took another look. Both issues are fixed, and I don't see any other blockers. LGTM!
This branch has not been deployed
3 participants
AgentScope-Java Version
2.1.0-SNAPSHOT (validated with main @ 65a35d4)
Description
Fixes #3440
Custom subagent factories previously could not carry a declaration, so they could neither enable persistent sessions nor receive the parent's RuntimeContext through the builder API.
This adds
HarnessAgent.Builder#subagentFactory(SubagentDeclaration, SubagentFactory). The factory builds a fully custom agent and receives the parent RuntimeContext when a new instance is needed. The declaration supplies the name, description, and spawn-time options (persistSession,mode,hidden,exposeToUser, andinheritParentPermissions). Instance-building fields remain the factory's responsibility. Remote declarations are rejected because their HTTP invocation bypasses a local factory. Existing factory overloads retain their behavior.Persistent custom factories also exposed two defects in AgentSpawnTool: its cache identity omitted userId, and it invoked the factory before checking the cache. Different users sharing a parent session ID could execute the same user-bound child, while repeated spawns constructed and discarded unused children.
The persistent identity now includes
(userId, parentSessionId, agentId, label), with unambiguous encoding for null values and separators. Cache lookup and creation usecomputeIfAbsentso cache hits do not invoke the factory, including concurrent spawns. Label lookup is scoped by user and parent session, and both newly created and reused children are registered in the current parent's state before returning. Permission propagation remains in both paths.English/Chinese documentation and Javadoc describe factory invocation, identity isolation, and key compatibility. The branch is synchronized with main @ 65a35d4, preserving its ToolResultBlock success/error/interruption contract.
Validation
mvn -B -T1 clean verify: all 97 reactor modules succeeded.SubagentFactoryDeclarationTest,AgentSpawnToolKeyTest,AgentSpawnToolPlanModeTest,AgentSpawnToolResultStateTestafter synchronizing with main: 29 tests, 0 failures/errors. The full-reactor run also reruns these classes.npm test(13 tests),npm run validate, andnpm run broken-linksall passed.git diff --checkpassed.Compatibility
The builder API is additive. SpawnEntry serialization and event/response formats are unchanged. Persistent key values now include user identity; newly spawned sessions get different keys from older versions. Saved entries remain addressable through
agent_sendusing their originalagent_key.Checklist