fix(agent): deny skill-discovery tools when skillsDisabled (#184 follow-up)

This commit is contained in:
matevip 2026-05-26 22:16:51 +08:00
parent 0ac325a337
commit c8b25e1bfb
3 changed files with 103 additions and 0 deletions

View File

@ -206,6 +206,17 @@ public class AgentGraphBuilder {
Set<String> boundTools = agentBindingService.getEffectiveToolNames(entity.getId());
toolSet = toolSet.withAllowedToolsOnly(boundTools); // null = 全局默认
// Issue #184 follow-up: an agent that opted out of skills must not be
// able to circle back and discover/load them via the meta tools. Strip
// the skill-discovery surface (listAvailableSkills / load_skill /
// readSkillFile / runSkillScript / listSkillFiles) here. This runs as a
// separate deny layer so the allowlist matrix in getEffectiveToolNames
// stays untouched in particular, the (skillsDisabled, !toolsDisabled,
// no tool bindings) cell still returns null so non-skill global tools
// continue to flow through.
toolSet = toolSet.withDeniedToolsFiltered(
agentBindingService.getSkillDiscoveryDeniedTools(entity.getId()));
// Escape hatch: drop the load_skill meta tool entirely when disabled, so
// it isn't advertised regardless of binding (the catalog guidance falls
// back to readSkillFile see SkillRuntimeService).

View File

@ -583,6 +583,40 @@ public class AgentBindingService implements AgentBindingResolver {
}
}
/**
* Skill-discovery meta tools that let the LLM enumerate, load, read, or
* execute the workspace's skill catalog. Normally these live in
* {@link #SYSTEM_LEVEL_TOOLS} because every agent needs them but when
* an agent has opted out of skills (issue #184), keeping them callable
* defeats the opt-out: the LLM can simply call {@code listAvailableSkills}
* to enumerate the catalog and {@code load_skill} to pull a SKILL.md
* into the conversation, even though the SKILL.md catalog itself was
* suppressed from the system prompt.
*
* <p>Resolved via {@link #getSkillDiscoveryDeniedTools} as a separate
* deny layer chained after the main allowlist, so the four-state matrix
* in {@link #getEffectiveToolNames} stays untouched.
*/
private static final Set<String> SKILL_DISCOVERY_TOOLS = Set.of(
"listAvailableSkills",
"load_skill",
"readSkillFile",
"runSkillScript",
"listSkillFiles"
);
/**
* Tools the agent must NOT see when {@link AgentEntity#getSkillsDisabled()}
* is {@code true}. Empty otherwise. Chained on top of the allowlist by
* {@code AgentGraphBuilder} via {@code withDeniedToolsFiltered}.
*/
public Set<String> getSkillDiscoveryDeniedTools(Long agentId) {
if (isSkillsDisabled(agentId)) {
return SKILL_DISCOVERY_TOOLS;
}
return Set.of();
}
/**
* Tools that exist outside the skill scope and must survive any
* agent-level skill binding restriction.

View File

@ -645,4 +645,62 @@ class AgentBindingServiceTest {
assertFalse(readToolsDisabledFlag(),
"单条 bindTool 也算明确的承诺,应当自动清掉 flag");
}
// ==================== Issue #184 follow-up: skill-discovery deny ====================
@Test
@DisplayName("issue #184 follow-up: skills_disabled=true → 屏蔽 listAvailableSkills / load_skill / readSkillFile / runSkillScript / listSkillFiles")
void skillsDisabledDeniesAllSkillDiscoveryTools() {
// Verified during smoke test: even with skillsDisabled=true the LLM
// could call listAvailableSkills and discover the full catalog. The
// deny layer below subtracts the 5 skill-discovery tools so the opt-out
// is honored end-to-end, not just in the SKILL.md catalog injection.
setSkillsDisabledFlag(true);
Set<String> denied = bindingService.getSkillDiscoveryDeniedTools(agentId);
assertEquals(5, denied.size(), "应当返回 5 个 skill-discovery 工具名");
assertTrue(denied.contains("listAvailableSkills"));
assertTrue(denied.contains("load_skill"));
assertTrue(denied.contains("readSkillFile"));
assertTrue(denied.contains("runSkillScript"));
assertTrue(denied.contains("listSkillFiles"));
}
@Test
@DisplayName("issue #184 follow-up: skills_disabled=false → 不屏蔽任何工具(保留向后兼容)")
void skillsEnabledReturnsEmptyDenySet() {
// Default state no flag, no rows. The deny layer must be a no-op
// so a legacy agent keeps every skill-discovery tool it had before.
Set<String> denied = bindingService.getSkillDiscoveryDeniedTools(agentId);
assertNotNull(denied);
assertTrue(denied.isEmpty(),
"skillsDisabled=false 时 deny 集必须为空,否则会误伤未禁用技能的 agent");
}
@Test
@DisplayName("issue #184 follow-up: setSkillBindings 非空时 auto-clear flag → deny 也回到空集")
void skillDiscoveryDenyTracksAutoClearOfFlag() {
// Auto-clear contract from the main PR: writing a non-empty binding
// clears skills_disabled. The deny set must follow it reads the
// same flag at call time, so after auto-clear it should be empty.
long skillId = 7_777_901L;
seedSkill(skillId);
setSkillsDisabledFlag(true);
assertEquals(5, bindingService.getSkillDiscoveryDeniedTools(agentId).size(),
"前置flag 开启时 deny 应为 5 个");
bindingService.setSkillBindings(agentId, List.of(skillId));
assertTrue(bindingService.getSkillDiscoveryDeniedTools(agentId).isEmpty(),
"auto-clear 后 deny 必须回到空集,否则用户重新绑定技能后仍然看不到 listAvailableSkills");
}
@Test
@DisplayName("issue #184 follow-up: missing agent → deny 空集(防御性)")
void skillDiscoveryDenyHandlesMissingAgent() {
Set<String> denied = bindingService.getSkillDiscoveryDeniedTools(999_999_999L);
assertNotNull(denied);
assertTrue(denied.isEmpty(),
"agent 不存在时 deny 集应为空 —— 严格但不抛错,与 isSkillsDisabled 的契约一致");
}
}