Skip to content

fix(skill): skillBox prompt should merge into system prompts #895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LearningGp merged 2 commits into from
Mar 11, 2026
Merged

fix(skill): skillBox prompt should merge into system prompts #895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5558e0
fix: skillBox prompt should merge into system prompts
fang-tech Mar 9, 2026
d37f47c
should deep copy Msg, rather than just content
fang-tech Mar 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 40 additions & 9 deletions agentscope-core/src/main/java/io/agentscope/core/skill/SkillHook.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -38,22 +39,52 @@ public <T extends HookEvent> Mono<T> onEvent(T event) {
if (event instanceof PreReasoningEvent preReasoningEvent) {
String skillPrompt = skillBox.getSkillPrompt();
if (skillPrompt != null && !skillPrompt.isEmpty()) {
List<Msg> 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<Msg> 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<ContentBlock> 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<Msg> newMessages = new ArrayList<>(inputMessages);
newMessages.set(systemIndex, mergedMsg);
preReasoningEvent.setInputMessages(newMessages);
} else {
// No existing system message, add one at the beginning
List<Msg> 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);
}

return Mono.just(event);
}

private int findFirstSystemMessageIndex(List<Msg> 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
Expand Down
93 changes: 79 additions & 14 deletions agentscope-core/src/test/java/io/agentscope/core/skill/SkillHookTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Msg> 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())
Expand All @@ -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);
Expand All @@ -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<Msg> 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 <T extends HookEvent> Mono<T> notifyHooks(T event, List<Hook> hooks) {
Expand Down
Loading