diff --git a/agentscope-core/src/main/java/io/agentscope/core/skill/SkillHook.java b/agentscope-core/src/main/java/io/agentscope/core/skill/SkillHook.java index 9762c19f2..e548c1c1a 100644 --- a/agentscope-core/src/main/java/io/agentscope/core/skill/SkillHook.java +++ b/agentscope-core/src/main/java/io/agentscope/core/skill/SkillHook.java @@ -18,6 +18,7 @@ import io.agentscope.core.hook.Hook; import io.agentscope.core.hook.HookEvent; import io.agentscope.core.hook.PreReasoningEvent; +import io.agentscope.core.message.ContentBlock; import io.agentscope.core.message.Msg; import io.agentscope.core.message.MsgRole; import io.agentscope.core.message.TextBlock; @@ -38,15 +39,36 @@ public Mono onEvent(T event) { if (event instanceof PreReasoningEvent preReasoningEvent) { String skillPrompt = skillBox.getSkillPrompt(); if (skillPrompt != null && !skillPrompt.isEmpty()) { - List inputMessages = - new ArrayList<>(preReasoningEvent.getInputMessages().size() + 1); - inputMessages.add( - Msg.builder() - .role(MsgRole.SYSTEM) - .content(TextBlock.builder().text(skillPrompt).build()) - .build()); - inputMessages.addAll(preReasoningEvent.getInputMessages()); - preReasoningEvent.setInputMessages(inputMessages); + List inputMessages = preReasoningEvent.getInputMessages(); + int systemIndex = findFirstSystemMessageIndex(inputMessages); + if (systemIndex >= 0) { + // Merge skill prompt into existing system message in-place (structural) + Msg existingSystem = inputMessages.get(systemIndex); + List mergedContent = new ArrayList<>(existingSystem.getContent()); + mergedContent.add(TextBlock.builder().text(skillPrompt).build()); + Msg mergedMsg = + Msg.builder() + .id(existingSystem.getId()) + .role(MsgRole.SYSTEM) + .name(existingSystem.getName()) + .content(mergedContent) + .metadata(existingSystem.getMetadata()) + .timestamp(existingSystem.getTimestamp()) + .build(); + List newMessages = new ArrayList<>(inputMessages); + newMessages.set(systemIndex, mergedMsg); + preReasoningEvent.setInputMessages(newMessages); + } else { + // No existing system message, add one at the beginning + List newMessages = new ArrayList<>(inputMessages.size() + 1); + newMessages.add( + Msg.builder() + .role(MsgRole.SYSTEM) + .content(TextBlock.builder().text(skillPrompt).build()) + .build()); + newMessages.addAll(inputMessages); + preReasoningEvent.setInputMessages(newMessages); + } } return Mono.just(event); } @@ -54,6 +76,15 @@ public Mono onEvent(T event) { return Mono.just(event); } + private int findFirstSystemMessageIndex(List messages) { + for (int i = 0; i < messages.size(); i++) { + if (messages.get(i).getRole() == MsgRole.SYSTEM) { + return i; + } + } + return -1; + } + @Override public int priority() { // High priority (55) to ensure skills system prompt is added early diff --git a/agentscope-core/src/test/java/io/agentscope/core/skill/SkillHookTest.java b/agentscope-core/src/test/java/io/agentscope/core/skill/SkillHookTest.java index 9b09cdb3d..6e7a1ceca 100644 --- a/agentscope-core/src/test/java/io/agentscope/core/skill/SkillHookTest.java +++ b/agentscope-core/src/test/java/io/agentscope/core/skill/SkillHookTest.java @@ -254,13 +254,9 @@ void testInjectSkillPromptAtFirst() { skillBox.registerSkill(skill); activateSkill(skill.getSkillId()); - // Create PreReasoningEvent with multiple messages + // Create PreReasoningEvent with multiple messages (no existing SYSTEM message) List messages = List.of( - Msg.builder() - .role(MsgRole.SYSTEM) - .content(TextBlock.builder().text("System instruction").build()) - .build(), Msg.builder() .role(MsgRole.USER) .content(TextBlock.builder().text("User query").build()) @@ -279,7 +275,7 @@ void testInjectSkillPromptAtFirst() { // Assert: Skill prompt should be injected at the FIRST position assertNotNull(result, "Event should be processed"); - assertEquals(4, result.getInputMessages().size(), "Should add skill prompt message"); + assertEquals(3, result.getInputMessages().size(), "Should add skill prompt message"); // Verify the first message is the skill prompt (SYSTEM role) Msg firstMsg = result.getInputMessages().get(0); @@ -288,22 +284,91 @@ void testInjectSkillPromptAtFirst() { firstMsg.getRole(), "First message should be SYSTEM message with skill prompt"); assertTrue( - firstMsg.getContent().toString().contains("test_skill"), + firstMsg.getTextContent().contains("test_skill"), "First message should contain skill information"); // Verify original messages are preserved in order after skill prompt assertEquals( - "System instruction", + "User query", result.getInputMessages().get(1).getTextContent(), - "Second message should be original system instruction"); + "Second message should be original user query"); assertEquals( - "User query", + "Assistant response", result.getInputMessages().get(2).getTextContent(), - "Third message should be original user query"); + "Third message should be original assistant response"); + } + + @Test + @DisplayName( + "[ISSUE#845] should merge skill prompt into existing system message instead of adding a" + + " second one") + void testMergeSkillPromptIntoExistingSystemMessage() { + // Arrange: Register and activate a skill + AgentSkill skill = new AgentSkill("test_skill", "Test Skill", "# Test Content", null); + skillBox.registerSkill(skill); + activateSkill(skill.getSkillId()); + + // Create PreReasoningEvent with an existing SYSTEM message + List messages = + List.of( + Msg.builder() + .role(MsgRole.SYSTEM) + .content(TextBlock.builder().text("System instruction").build()) + .build(), + Msg.builder() + .role(MsgRole.USER) + .content(TextBlock.builder().text("User query").build()) + .build(), + Msg.builder() + .role(MsgRole.ASSISTANT) + .content(TextBlock.builder().text("Assistant response").build()) + .build()); + + PreReasoningEvent event = + new PreReasoningEvent( + testAgent, "test-model", GenerateOptions.builder().build(), messages); + + // Act: Process event through hook + PreReasoningEvent result = skillHook.onEvent(event).block(); + + // Assert: Should still have exactly 3 messages (merged, not added) + assertNotNull(result, "Event should be processed"); assertEquals( - "Assistant response", - result.getInputMessages().get(3).getTextContent(), - "Fourth message should be original assistant response"); + 3, + result.getInputMessages().size(), + "Should merge into existing SYSTEM message, not add a new one"); + + // Verify there is exactly one SYSTEM message + long systemCount = + result.getInputMessages().stream() + .filter(m -> m.getRole() == MsgRole.SYSTEM) + .count(); + assertEquals(1, systemCount, "There should be exactly one SYSTEM message"); + + // Verify the merged SYSTEM message is at index 0 + Msg systemMsg = result.getInputMessages().get(0); + assertEquals(MsgRole.SYSTEM, systemMsg.getRole()); + + // Verify structural merge: content blocks are preserved, not flattened + // First content block should be the original system instruction TextBlock, + // second should be the skill prompt TextBlock + assertEquals( + 2, + systemMsg.getContent().size(), + "Merged SYSTEM message should have 2 content blocks (structural merge)"); + assertInstanceOf(TextBlock.class, systemMsg.getContent().get(0)); + assertInstanceOf(TextBlock.class, systemMsg.getContent().get(1)); + assertEquals( + "System instruction", + ((TextBlock) systemMsg.getContent().get(0)).getText(), + "First content block should be the original system instruction"); + assertTrue( + ((TextBlock) systemMsg.getContent().get(1)).getText().contains("test_skill"), + "Second content block should be the skill prompt"); + + // Verify other messages are preserved + assertEquals("User query", result.getInputMessages().get(1).getTextContent()); + assertEquals("Assistant response", result.getInputMessages().get(2).getTextContent()); } private Mono notifyHooks(T event, List hooks) {