fix(skill-runtime): use skill name instead of composite ID for SkillFilter matching …issues/1768#1771
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a clean, focused bug fix that corrects the SkillFilter matching logic in SkillPromptBuilder.render(). The root cause is accurately identified: getSkillId() returns a composite name_source string, while SkillFilter was designed to accept bare skill names (as confirmed by its field name skillNames, the Javadoc example SkillFilter.only("skill_a", "skill_b"), and the HarnessAgent.Builder.enableSkills(String... skillNames) API). The 1-line fix (getSkillId() → getName()) is correct, minimal, and consistent with the intended contract. The variable rename from id to skillName improves readability. Test coverage is solid — the existing test is updated to match the correct behavior, and the new test filterWithBareNameMatchesCompositeId directly reproduces the reported scenario with realistic data. The rendered <skill-id> in the prompt still uses the composite ID (for load_skill_through_path), so there's no downstream breakage. All filter modes (WHITELIST, BLACKLIST, OVERLAY_ENABLE, OVERLAY_DISABLE) benefit from this single-point fix.
AgentScopeJavaBot
left a comment
There was a problem hiding this comment.
🤖 AI Review
This is a clean, focused bug fix that corrects the SkillFilter matching logic in SkillPromptBuilder.render(). The root cause is accurately identified: getSkillId() returns a composite name_source string, while SkillFilter was designed to accept bare skill names (as confirmed by its field name skillNames, the Javadoc example SkillFilter.only("skill_a", "skill_b"), and the HarnessAgent.Builder.enableSkills(String... skillNames) API). The 1-line fix (getSkillId() → getName()) is correct, minimal, and consistent with the intended contract. The variable rename from id to skillName improves readability. Test coverage is solid — the existing test is updated to match the correct behavior, and the new test filterWithBareNameMatchesCompositeId directly reproduces the reported scenario with realistic data. The rendered <skill-id> in the prompt still uses the composite ID (for load_skill_through_path), so there's no downstream breakage. All filter modes (WHITELIST, BLACKLIST, OVERLAY_ENABLE, OVERLAY_DISABLE) benefit from this single-point fix.
…issues/1768
AgentScope-Java Version
2.0.0-SNAPSHOT
Description
Background
SkillFilter.only("host-forensics-client")does not work as expected when used withHarnessAgent. The whitelisted skill is silently filtered out, and the rendered skill prompt is empty.Root Cause
In
SkillPromptBuilder#render(SkillCatalog, SkillFilter), the code passesentry.skill().getSkillId()toSkillFilter#isAllowed(). However,getSkillId()returns a composite ID in the formatname_source(e.g.host-forensics-client_filesystem-agentscope_skills), while users naturally pass bare skill names (e.g.host-forensics-client) toSkillFilter.only(...). SinceisAllowed()performs an exactcontains()check, the match always fails, making WHITELIST / BLACKLIST / OVERLAY_ENABLE / OVERLAY_DISABLE modes effectively unusable.Fix
Replace
entry.skill().getSkillId()withentry.skill().getName()inSkillPromptBuilder#render()so the filter key matches the bare skill name that users configure.Changed files:
SkillPromptBuilder.java— 1-line fix:getSkillId()→getName()SkillRuntimeTest.java— updated existing test to use bare name; added new testfilterWithBareNameMatchesCompositeIdthat reproduces the reported scenario