feat(skill): add model configuration and SubAgent context sharing support#841
feat(skill): add model configuration and SubAgent context sharing support#841xun404 wants to merge 5 commits intoagentscope-ai:mainfrom
Conversation
|
|
Summary of ChangesHello, 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 advanced capabilities for skill management and sub-agent interaction. It provides granular control over which AI models execute specific skills and offers flexible options for how sub-agents share or isolate their conversational memory with parent agents. These changes aim to improve the modularity, reusability, and contextual awareness of agents within the system. 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 enhancements to the skill system, adding support for custom model configurations and sub-agent context sharing. The changes are extensive, including new interfaces for model providers, context-aware sub-agent creation, and memory forking capabilities. The implementation is well-tested and documented. My review focuses on a few key areas: I've identified some code duplication that could be refactored for better maintainability, a discrepancy between the documented and implemented behavior of the SHARED context sharing mode, a potential issue with shallow copying in the fork() method of AutoContextMemory, and a minor error in the documentation examples. Addressing these points will improve the robustness and clarity of this new feature.
| // Create sub-agent tool if skill has model configured | ||
| createSubAgentForSkill(skill, skillId); |
There was a problem hiding this comment.
This call to createSubAgentForSkill is redundant and can lead to incorrect behavior. The SkillRegistration.apply() method, which is the primary way to register skills, already calls createSubAgentIfHasModel() to handle sub-agent creation. This duplication means the sub-agent tool might be created twice. If runtimeModel is used, the first creation by this method is incorrect and immediately overwritten. To avoid this, this call and the createSubAgentForSkill method should be removed, centralizing the logic within the SkillRegistration flow.
| case SHARED: | ||
| // Fork parent's memory and remove pending tool calls | ||
| // Note: We cannot directly share parent's memory because it contains | ||
| // the pending tool call to this sub-agent, which would cause | ||
| // validation errors when the sub-agent tries to add new messages. | ||
| // We use a forked copy with pending tool calls removed. | ||
| Memory sharedMemory = parentMemory.fork(); | ||
| removePendingToolCalls(sharedMemory); | ||
| logger.debug( | ||
| "Sub-agent will use SHARED (forked with pending calls removed) memory from" | ||
| + " parent ({} messages)", | ||
| sharedMemory.getMessages().size()); | ||
| return sharedMemory; | ||
|
|
There was a problem hiding this comment.
The implementation for SHARED mode is identical to FORK mode, as both fork the parent's memory. This contradicts the documentation in ContextSharingMode, which states that SHARED mode uses the same memory instance. With the current implementation, any changes the sub-agent makes to its memory will not be reflected in the parent's memory, which is not what "shared" implies.
Please either update the implementation to achieve true memory sharing (which might be complex due to the pending tool call issue you've noted in the comments) or update the documentation for ContextSharingMode.SHARED to clarify that it provides a one-way context copy, similar to FORK. The current discrepancy is misleading.
| forked.originalMemoryStorage = new ArrayList<>(this.originalMemoryStorage); | ||
|
|
||
| // Copy offload context | ||
| forked.offloadContext = new HashMap<>(this.offloadContext); |
There was a problem hiding this comment.
The copy of offloadContext is a shallow copy. This means both the original and the forked memory will share the same List<Msg> instances as values in the map. If the forked memory modifies one of these lists (e.g., by calling reload() and then modifying the returned list), it will affect the original memory, which defeats the purpose of forking. You should perform a deep copy of the map's values to ensure true isolation.
| forked.offloadContext = new HashMap<>(this.offloadContext); | |
| forked.offloadContext = new HashMap<>(); | |
| for (Map.Entry<String, List<Msg>> entry : this.offloadContext.entrySet()) { | |
| forked.offloadContext.put(entry.getKey(), new ArrayList<>(entry.getValue())); | |
| } |
| /** | ||
| * Create a SubAgentTool if the skill has a model configured. | ||
| * | ||
| * <p>This method automatically creates a sub-agent tool that can execute the skill using the | ||
| * configured model. The tool name follows the pattern "call_{skillName}". | ||
| * | ||
| * <p>If no model is configured, the model provider is missing, or the model is not found, | ||
| * this method does nothing (graceful degradation). | ||
| */ | ||
| private void createSubAgentIfHasModel() { | ||
| String effectiveModel = resolveEffectiveModel(); | ||
| logger.debug( | ||
| "createSubAgentIfHasModel called for skill '{}', effectiveModel='{}', " | ||
| + "toolkit={}, modelProvider={}", | ||
| skill.getName(), | ||
| effectiveModel, | ||
| skillBox.toolkit != null ? "present" : "null", | ||
| skillBox.modelProvider != null ? "present" : "null"); | ||
|
|
||
| if (effectiveModel == null || effectiveModel.isBlank()) { | ||
| logger.debug( | ||
| "Skill '{}' has no model configured, skipping sub-agent creation", | ||
| skill.getName()); | ||
| return; // No model specified | ||
| } | ||
|
|
||
| if (skillBox.toolkit == null) { | ||
| logger.warn( | ||
| "No toolkit configured for skill '{}', cannot create sub-agent with model" | ||
| + " '{}'", | ||
| skill.getName(), | ||
| effectiveModel); | ||
| return; | ||
| } | ||
|
|
||
| if (skillBox.modelProvider == null) { | ||
| logger.warn( | ||
| "No SkillModelProvider configured for skill '{}', " | ||
| + "cannot create sub-agent with model '{}'", | ||
| skill.getName(), | ||
| effectiveModel); | ||
| return; | ||
| } | ||
|
|
||
| Model model = skillBox.modelProvider.getModel(effectiveModel); | ||
| if (model == null) { | ||
| logger.warn( | ||
| "Model '{}' not found for skill '{}', skipping sub-agent creation", | ||
| effectiveModel, | ||
| skill.getName()); | ||
| return; | ||
| } | ||
|
|
||
| // Use the same toolkit reference as skillBox | ||
| Toolkit effectiveToolkit = toolkit != null ? toolkit : skillBox.toolkit; | ||
|
|
||
| // Create SubAgentProvider with structured system prompt | ||
| final Model resolvedModel = model; | ||
| final Toolkit toolkitCopy = effectiveToolkit.copy(); | ||
| final String systemPrompt = | ||
| SkillSubagentPromptBuilder.builder() | ||
| .skill(skill) | ||
| .modelName(resolvedModel.getModelName()) | ||
| .build(); | ||
|
|
||
| // Parse context sharing mode from skill | ||
| final ContextSharingMode contextMode = parseContextSharingMode(skill.getContext()); | ||
|
|
||
| // Create SubAgentProvider - context-aware for memory sharing | ||
| SubAgentProvider<ReActAgent> provider = | ||
| new SubAgentProvider<>() { | ||
| @Override | ||
| public ReActAgent provideWithContext(SubAgentContext context) { | ||
| ReActAgent.Builder agentBuilder = | ||
| ReActAgent.builder() | ||
| .name(skill.getName() + "_agent") | ||
| .description(skill.getDescription()) | ||
| .model(resolvedModel) | ||
| .toolkit(toolkitCopy); | ||
|
|
||
| // Check if context provides a memory to use (SHARED or FORK mode) | ||
| Memory memoryToUse = context.getMemoryToUse(); | ||
| if (memoryToUse != null) { | ||
| // Use the provided memory (shared or forked from parent) | ||
| agentBuilder.memory(memoryToUse); | ||
| } else { | ||
| // No memory provided - use independent memory with our system | ||
| // prompt | ||
| agentBuilder.sysPrompt(systemPrompt).memory(new InMemoryMemory()); | ||
| } | ||
|
|
||
| return agentBuilder.build(); | ||
| } | ||
| }; | ||
|
|
||
| // Create tool group if needed | ||
| String skillToolGroup = skill.getSkillId() + "_skill_tools"; | ||
| if (effectiveToolkit.getToolGroup(skillToolGroup) == null) { | ||
| effectiveToolkit.createToolGroup(skillToolGroup, skillToolGroup, false); | ||
| } | ||
|
|
||
| // Register sub-agent tool | ||
| effectiveToolkit | ||
| .registration() | ||
| .group(skillToolGroup) | ||
| .subAgent( | ||
| provider, | ||
| SubAgentConfig.builder() | ||
| .toolName("call_" + skill.getName()) | ||
| .description( | ||
| "Execute " | ||
| + skill.getName() | ||
| + " skill task using configured model") | ||
| .contextSharingMode(contextMode) | ||
| .build()) | ||
| .apply(); | ||
|
|
||
| logger.info( | ||
| "Created sub-agent tool 'call_{}' for skill '{}' with model '{}'", | ||
| skill.getName(), | ||
| skill.getName(), | ||
| model.getModelName()); | ||
| } |
There was a problem hiding this comment.
This method createSubAgentIfHasModel shares a significant amount of code with SkillToolFactory.createSubAgentIfHasModel. This duplication makes the code harder to maintain. Consider refactoring the common logic into a shared private helper method to reduce redundancy. For instance, a method could be created in SkillBox that takes all necessary parameters (like the effective model, skill, and toolkit) and handles the sub-agent creation, which can then be called from both SkillRegistration and SkillToolFactory.
| private static ContextSharingMode parseContextSharingMode(String context) { | ||
| if (context == null || context.isEmpty() || "shared".equalsIgnoreCase(context)) { | ||
| return ContextSharingMode.SHARED; | ||
| } else if ("fork".equalsIgnoreCase(context)) { | ||
| return ContextSharingMode.FORK; | ||
| } else if ("new".equalsIgnoreCase(context)) { | ||
| return ContextSharingMode.NEW; | ||
| } else { | ||
| logger.warn( | ||
| "Unknown context mode '{}', defaulting to SHARED. " | ||
| + "Supported values: shared, fork, new", | ||
| context); | ||
| return ContextSharingMode.SHARED; | ||
| } | ||
| } |
There was a problem hiding this comment.
The parseContextSharingMode method is duplicated in SkillToolFactory. To adhere to the DRY (Don't Repeat Yourself) principle, this logic should be centralized. You could make this method package-private (static ContextSharingMode parseContextSharingMode(...)) and call it from SkillToolFactory, or move it to a shared utility class.
docs/en/task/agent-skill.md
Outdated
| import com.alibaba.agentscope.skill.model.SkillModelProvider; | ||
| import com.alibaba.agentscope.skill.model.MapBasedSkillModelProvider; |
There was a problem hiding this comment.
The import paths in this example are incorrect. The package name should be io.agentscope.core.skill instead of com.alibaba.agentscope.skill.model.
| import com.alibaba.agentscope.skill.model.SkillModelProvider; | |
| import com.alibaba.agentscope.skill.model.MapBasedSkillModelProvider; | |
| import io.agentscope.core.skill.SkillModelProvider; | |
| import io.agentscope.core.skill.MapBasedSkillModelProvider; |
docs/zh/task/agent-skill.md
Outdated
| import com.alibaba.agentscope.skill.model.SkillModelProvider; | ||
| import com.alibaba.agentscope.skill.model.MapBasedSkillModelProvider; |
There was a problem hiding this comment.
The import paths in this example are incorrect. The package name should be io.agentscope.core.skill instead of com.alibaba.agentscope.skill.model.
| import com.alibaba.agentscope.skill.model.SkillModelProvider; | |
| import com.alibaba.agentscope.skill.model.MapBasedSkillModelProvider; | |
| import io.agentscope.core.skill.SkillModelProvider; | |
| import io.agentscope.core.skill.MapBasedSkillModelProvider; |
xun404
force-pushed
the
skill-support-model
branch
from
eb9e6f9 to
900681a
Compare
- Remove redundant createSubAgentForSkill call in registerSkill() to prevent duplicate sub-agent creation with SkillRegistration.apply() - Add ContextSharingMode.fromString() factory method and consolidate parsing logic from SkillBox and SkillToolFactory (DRY principle) - Update SHARED mode documentation to accurately reflect fork-based implementation due to pending tool call validation constraints - Fix shallow copy of offloadContext in AutoContextMemory.fork() to use deep copy for proper memory isolation - Refactor duplicate createSubAgentIfHasModel code into shared helper method SkillBox.createSubAgentToolForSkill() to reduce ~200 lines of code duplication Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…verage improvements
- Add assertNotSame import to AutoContextMemoryTest Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Clarify why SHARED and FORK have identical implementations - Explain technical limitation: cannot directly share memory due to pending tool calls - Document rationale for keeping both modes despite identical behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
AgentScope-Java Version
1.0.10-SNAPSHOT
Description
Background
The Skill system needs to support custom model configuration, and SubAgent requires better context sharing management.
Key Changes
1. Skill Model Configuration Support
SkillModelProviderinterface for model lookup by name or aliasMapBasedSkillModelProviderimplementation with Map-based model mappingSkillBoxandSkillToolFactorysupport model configuration viamodelProviderAgentSkillparsesmodelfield from SKILL.md2. SubAgent Context Sharing Mechanism
ContextSharingModeenum:SHARED,FORK,NEWSubAgentContextclass to carry parent agent info and pre-computed memorySubAgentProviderinterface for context-aware agent creationSubAgentToolsupports memory sharing/isolation based on mode:3. Other Improvements
fork()method toMemoryinterface, all implementations support creating independent copiesSubAgentToolinherits session_id from parent context (SHARED/FORK mode)SubAgentToolforwards images from user messages (SHARED/FORK mode)session_idJSON Schema validation (allow null type)How to Test
mvn test -pl agentscope-core -Dtest="SkillTest,SubAgentToolTest"
Checklist