mirror of
https://gitee.com/mateos/mateclaw.git
synced 2026-09-13 03:13:41 +08:00
fix(skill): stop the LLM from calling skill names as tools (issue #46)
When a user-installed skill (e.g. RedisOps) was bound to an agent, the model frequently called the skill name directly as a tool, hit "Tool not found: RedisOps", and either gave up or fell back to shell guessing. Two compounding causes: 1. The system prompt block injected by SkillRuntimeService listed each skill as `- **RedisOps** — desc`, which is the same format used for tool catalogs and primed the model to call the names directly. The "how to use" instructions referenced `read_skill_file` / `run_skill_script` — names that don't exist in the tool registry, so even a compliant LLM couldn't follow them. 2. ToolExecutionExecutor's `callback == null` branches returned a bare "Tool not found: <name>" string. The model had no recovery signal and no hint that the name it called was actually a skill. Fix is two-layered: - Prompt rewrite (SkillRuntimeService.buildSkillPromptEnhancement): lead with an explicit warning that skills are NOT directly callable, use the correct camelCase tool names (readSkillFile / runSkillScript), include a concrete worked example anchored to the first enabled skill, and render the listing as a markdown table so it stops looking like a callable tool list. listAvailableSkills tool description and output follow the same pattern. - Runtime safety net (ToolExecutionExecutor): when toolCallbackMap.get misses, check if the requested name (case-insensitive) matches an active skill. If so, return a precise hint telling the LLM the right invocation pattern instead of the bare error. Wired through both the main execute path and the pre-approved replay path. SkillRuntimeService is attached via a setter from AgentGraphBuilder so the executor's many legacy constructors stay untouched, and it's nullable so isolated tests still work. Adds 5 unit tests covering: skill match -> hint, case-insensitive match, no-match -> bare error, no SkillRuntimeService wired -> bare error, pre-approved replay path -> hint. Reported and reproduced by @pipima9950-glitch in issue #46.
This commit is contained in:
matevip
parent
d20b440ce5
commit
101aa3209e
@ -310,6 +310,10 @@ public class AgentGraphBuilder {
|
||||
primaryModelConfig != null ? primaryModelConfig.getProvider() : null,
|
||||
providerPool);
|
||||
ToolExecutionExecutor executor = new ToolExecutionExecutor(toolSet, toolGuardService, approvalService, streamTracker, toolTimeoutProperties, toolResultStorage, toolConcurrencyRegistry);
|
||||
// Issue #46: enable skill-aware "Tool not found" hint so when the
|
||||
// LLM mis-calls a skill name as a tool, the response tells it
|
||||
// the right invocation pattern instead of a dead-end error.
|
||||
executor.setSkillRuntimeService(skillRuntimeService);
|
||||
PlanGenerationNode planGenerationNode = new PlanGenerationNode(chatModel, planningService, streamingHelper, conversationWindowManager, toolSet);
|
||||
StepExecutionNode stepExecutionNode = new StepExecutionNode(chatModel, toolSet, executor, planningService, streamTracker, reasoningEffort, streamingHelper, conversationWindowManager);
|
||||
PlanSummaryNode planSummaryNode = new PlanSummaryNode(chatModel, planningService, streamingHelper);
|
||||
@ -437,6 +441,10 @@ public class AgentGraphBuilder {
|
||||
primaryModelConfig != null ? primaryModelConfig.getProvider() : null,
|
||||
providerPool);
|
||||
ToolExecutionExecutor executor = new ToolExecutionExecutor(toolSet, toolGuardService, approvalService, streamTracker, toolTimeoutProperties, toolResultStorage, toolConcurrencyRegistry);
|
||||
// Issue #46: enable skill-aware "Tool not found" hint so when the
|
||||
// LLM mis-calls a skill name as a tool, the response tells it
|
||||
// the right invocation pattern instead of a dead-end error.
|
||||
executor.setSkillRuntimeService(skillRuntimeService);
|
||||
// PR-1.2 (RFC-049 L1-B): propagate the bound model's capability so ReasoningNode
|
||||
// can gate the ThinkingLevelHolder override explicitly, rather than inferring
|
||||
// capability from reasoningEffort == null.
|
||||
|
||||
@ -228,17 +228,36 @@ public abstract class BaseAgent {
|
||||
}
|
||||
}
|
||||
|
||||
// Tail guard: a few providers reject prompts whose history ends with an
|
||||
// assistant message. Anthropic Claude returns 400 "does not support
|
||||
// assistant message prefill"; DeepSeek thinking mode requires the
|
||||
// last assistant turn's reasoning_content (which we may not have).
|
||||
// The trailing-user-dedup above can already produce an assistant tail
|
||||
// when the immediately-prior turn was an error / placeholder that got
|
||||
// dropped by stage 1 / 1.5 of sanitizeForLlm. Strip remaining assistant
|
||||
// tails defensively — the current user message is fed in separately as
|
||||
// the final prompt by the caller, so dropping these assistant entries
|
||||
// never loses information the LLM needs.
|
||||
while (!messages.isEmpty() && messages.get(messages.size() - 1) instanceof AssistantMessage) {
|
||||
// Tail guard — orphan-user strip (issue #47).
|
||||
//
|
||||
// Invariant: every caller of buildConversationHistory appends the
|
||||
// current user message AFTER this history (BaseAgent.buildClient
|
||||
// via .user(), StateGraphReActAgent / StateGraphPlanExecuteAgent
|
||||
// via messages.add(buildCurrentUserMessage)). So the final prompt is
|
||||
// [system, ...history, current_user]
|
||||
// and is *always* terminated by a user message. That means a trailing
|
||||
// assistant in history is FINE for every provider we support — it
|
||||
// produces the correct [..., user, assistant, current_user] alternation
|
||||
// (OpenAI, Anthropic, DeepSeek regular/thinking, Gemini, Qwen, …).
|
||||
//
|
||||
// The actual hazard is the opposite: a trailing USER in history.
|
||||
// That happens when the immediately-prior turn's assistant message
|
||||
// was dropped by Stage 1 (approval placeholder) or Stage 1.5 (errored
|
||||
// turn / "[错误] " row), or never persisted at all (turn interrupted
|
||||
// before doOnComplete saved the assistant). In that case the history
|
||||
// ends with an orphan unanswered user, and appending the current user
|
||||
// produces TWO consecutive user messages. Most providers concatenate
|
||||
// those and answer both — leaking the orphan question's answer
|
||||
// alongside the current answer. (This was the symptom reported in
|
||||
// issue #47, originally caused by a tail guard that stripped trailing
|
||||
// ASSISTANT messages instead of trailing USER ones — a direction-
|
||||
// reversed version of this loop.)
|
||||
//
|
||||
// Stripping orphan users is safe: the user re-asked or asked a new
|
||||
// question; the orphan turn produced no answer the model can build
|
||||
// on. We lose a small amount of conversational context in exchange
|
||||
// for clean alternation across every provider.
|
||||
while (!messages.isEmpty() && messages.get(messages.size() - 1) instanceof UserMessage) {
|
||||
messages.remove(messages.size() - 1);
|
||||
}
|
||||
return messages;
|
||||
|
||||
@ -123,6 +123,18 @@ public class ToolExecutionExecutor {
|
||||
private final ToolResultStorage resultStorage;
|
||||
/** RFC-008 Phase 4 metadata-driven concurrency classifier; nullable for legacy constructors. */
|
||||
private final vip.mate.tool.ToolConcurrencyRegistry concurrencyRegistry;
|
||||
/**
|
||||
* Issue #46: when {@code toolCallbackMap} misses a name that the LLM
|
||||
* called, we check whether it matches an active skill so we can
|
||||
* return a precise hint instead of bare "Tool not found". Nullable —
|
||||
* legacy constructors and tests may leave this unset, in which case
|
||||
* the safety net falls through to the original error string.
|
||||
*/
|
||||
private vip.mate.skill.runtime.SkillRuntimeService skillRuntimeService;
|
||||
|
||||
public void setSkillRuntimeService(vip.mate.skill.runtime.SkillRuntimeService s) {
|
||||
this.skillRuntimeService = s;
|
||||
}
|
||||
|
||||
public ToolExecutionExecutor(AgentToolSet toolSet, ToolGuardService toolGuardService,
|
||||
ApprovalWorkflowService approvalService, ChatStreamTracker streamTracker) {
|
||||
@ -326,10 +338,11 @@ public class ToolExecutionExecutor {
|
||||
}
|
||||
ToolCallback callback = toolCallbackMap.get(toolName);
|
||||
if (callback == null) {
|
||||
log.warn("[ToolExecutor] Tool not found: {}", toolName);
|
||||
events.add(GraphEventPublisher.toolComplete(toolCall.id(), toolName, "Tool not found: " + toolName, false));
|
||||
String msg = skillAwareNotFoundMessage(toolName);
|
||||
log.warn("[ToolExecutor] {}", msg);
|
||||
events.add(GraphEventPublisher.toolComplete(toolCall.id(), toolName, msg, false));
|
||||
allResponses.add(new ToolResponseMessage.ToolResponse(
|
||||
toolCall.id(), toolName, "Tool not found: " + toolName));
|
||||
toolCall.id(), toolName, msg));
|
||||
continue;
|
||||
}
|
||||
|
||||
@ -402,9 +415,10 @@ public class ToolExecutionExecutor {
|
||||
|
||||
ToolCallback callback = toolCallbackMap.get(toolName);
|
||||
if (callback == null) {
|
||||
log.warn("[ToolExecutor] Pre-approved tool not found: {}", toolName);
|
||||
events.add(GraphEventPublisher.toolComplete(toolCall.id(), toolName, "Tool not found: " + toolName, false));
|
||||
return new ToolResponseMessage.ToolResponse(toolCall.id(), toolName, "Tool not found: " + toolName);
|
||||
String msg = skillAwareNotFoundMessage(toolName);
|
||||
log.warn("[ToolExecutor] Pre-approved {}", msg);
|
||||
events.add(GraphEventPublisher.toolComplete(toolCall.id(), toolName, msg, false));
|
||||
return new ToolResponseMessage.ToolResponse(toolCall.id(), toolName, msg);
|
||||
}
|
||||
|
||||
try {
|
||||
@ -817,6 +831,38 @@ public class ToolExecutionExecutor {
|
||||
return approvalResponse;
|
||||
}
|
||||
|
||||
/**
|
||||
* Issue #46 — when a tool callback miss happens, check whether the
|
||||
* unrecognized name actually matches an active skill. If it does, return
|
||||
* a precise hint telling the LLM the right invocation pattern instead
|
||||
* of bare "Tool not found: X". Without this, an LLM that called e.g.
|
||||
* {@code RedisOps} as a tool gets no recovery signal and either gives
|
||||
* up or falls back to shell guessing.
|
||||
*
|
||||
* <p>Case-insensitive match because LLMs sometimes change the case of
|
||||
* skill names mid-conversation.
|
||||
*/
|
||||
private String skillAwareNotFoundMessage(String toolName) {
|
||||
if (skillRuntimeService != null && toolName != null && !toolName.isBlank()) {
|
||||
try {
|
||||
boolean isSkill = skillRuntimeService.getActiveSkills().stream()
|
||||
.anyMatch(s -> s.getName() != null && s.getName().equalsIgnoreCase(toolName));
|
||||
if (isSkill) {
|
||||
return String.format(
|
||||
"'%s' is a Skill, not a Tool — calling it as a tool fails. "
|
||||
+ "To use it, FIRST call readSkillFile(skillName=\"%s\", filePath=\"SKILL.md\") "
|
||||
+ "to read its instructions, THEN follow what SKILL.md tells you "
|
||||
+ "(typically runSkillScript with a scripts/<file> path).",
|
||||
toolName, toolName);
|
||||
}
|
||||
} catch (Exception e) {
|
||||
// Don't let a hint-side failure mask the original error.
|
||||
log.debug("[ToolExecutor] skill-aware hint check failed: {}", e.getMessage());
|
||||
}
|
||||
}
|
||||
return "Tool not found: " + toolName;
|
||||
}
|
||||
|
||||
// ==================== 内部数据类 ====================
|
||||
|
||||
private record PreparedToolCall(
|
||||
|
||||
@ -162,30 +162,62 @@ public class SkillRuntimeService {
|
||||
return "";
|
||||
}
|
||||
|
||||
// Issue #46 prompt rewrite. Three deliberate choices vs. the old
|
||||
// version, all driven by observed LLM mis-calls (e.g. calling
|
||||
// "RedisOps" directly as a tool):
|
||||
// 1. Lead with an explicit warning that skills are NOT callable.
|
||||
// Primacy bias — putting it first is what makes it stick.
|
||||
// 2. Use the actual camelCase tool names (readSkillFile /
|
||||
// runSkillScript). The old text said `read_skill_file` /
|
||||
// `run_skill_script`, which don't exist in the tool registry,
|
||||
// so even an LLM trying to comply couldn't find them.
|
||||
// 3. Concrete worked example: "to use RedisOps, START with
|
||||
// readSkillFile(...)". Abstract instructions reliably lose
|
||||
// to a worked example in tool-use prompting.
|
||||
// 4. Render the listing as a markdown table with the call pattern
|
||||
// explicit, instead of `- **Name** — desc` which looks
|
||||
// identical to a tool list and primes the model to call
|
||||
// the name directly.
|
||||
StringBuilder sb = new StringBuilder();
|
||||
sb.append("\n\n## Available Skills\n");
|
||||
sb.append("以下技能已启用，你可以通过 skill runtime tools 使用它们：\n\n");
|
||||
sb.append("\n\n## Available Skills\n\n");
|
||||
sb.append("⚠️ **Skills are documentation packages, NOT directly callable tools.**\n");
|
||||
sb.append("Calling a skill name as a tool (e.g. tool_use{name=\"RedisOps\"}) will fail with \"Tool not found\". ");
|
||||
sb.append("To use a skill, follow this two-step pattern:\n\n");
|
||||
sb.append("1. Read its SKILL.md to learn how it works:\n");
|
||||
sb.append(" `readSkillFile(skillName=\"<name>\", filePath=\"SKILL.md\")`\n");
|
||||
sb.append("2. Follow what SKILL.md tells you — usually that means calling:\n");
|
||||
sb.append(" `runSkillScript(skillName=\"<name>\", scriptPath=\"scripts/<file>\")` or\n");
|
||||
sb.append(" `readSkillFile(skillName=\"<name>\", filePath=\"references/<file>\")`\n\n");
|
||||
|
||||
// Concrete example anchored to the first enabled skill so the LLM
|
||||
// sees a real name it just read in the listing below.
|
||||
String exampleName = activeSkills.get(0).getName();
|
||||
sb.append("Concrete example — to use the `").append(exampleName).append("` skill, START with:\n");
|
||||
sb.append(" `readSkillFile(skillName=\"").append(exampleName).append("\", filePath=\"SKILL.md\")`\n\n");
|
||||
|
||||
sb.append("### Enabled skills\n");
|
||||
sb.append("Pass these names as the `skillName=` argument to `readSkillFile` / `runSkillScript`. ");
|
||||
sb.append("Do **not** call them as tools.\n\n");
|
||||
sb.append("| Skill name | Description |\n");
|
||||
sb.append("|------------|-------------|\n");
|
||||
for (ResolvedSkill skill : activeSkills) {
|
||||
sb.append("- **").append(skill.getName()).append("**");
|
||||
sb.append("| `").append(skill.getName()).append("`");
|
||||
if (skill.getIcon() != null && !skill.getIcon().isBlank()) {
|
||||
sb.append(" ").append(skill.getIcon());
|
||||
}
|
||||
sb.append(" | ");
|
||||
if (skill.getDescription() != null && !skill.getDescription().isBlank()) {
|
||||
String desc = skill.getDescription();
|
||||
if (desc.length() > 200) {
|
||||
desc = desc.substring(0, 200) + "...";
|
||||
}
|
||||
sb.append(" — ").append(desc);
|
||||
// Escape pipe and newline so a multi-line description doesn't
|
||||
// break the table layout.
|
||||
sb.append(desc.replace("|", "\\|").replace("\n", " "));
|
||||
}
|
||||
sb.append("\n");
|
||||
sb.append(" |\n");
|
||||
}
|
||||
|
||||
sb.append("\n### 如何使用技能\n");
|
||||
sb.append("1. 使用 `read_skill_file` 工具读取技能内部文件（SKILL.md / references / scripts）\n");
|
||||
sb.append("2. 使用 `run_skill_script` 工具执行技能脚本\n");
|
||||
sb.append("3. 所有路径相对技能根目录解析，必须以 references/ 或 scripts/ 开头\n");
|
||||
|
||||
return sb.toString();
|
||||
}
|
||||
}
|
||||
|
||||
@ -148,9 +148,16 @@ public class SkillFileTool {
|
||||
}
|
||||
|
||||
@Tool(description = """
|
||||
列出所有当前可用的技能（Skills），包括名称、图标和描述。
|
||||
使用此工具查看系统中有哪些已启用且可运行的技能。
|
||||
注意：这里列出的是技能（Skills），不是 Agent。如需列出可用 Agent，请使用 listAvailableAgents。
|
||||
List all currently available Skills (documentation packages).
|
||||
|
||||
IMPORTANT: Skills are NOT directly callable as tools. Each name
|
||||
returned here is a `skillName` argument, not a tool name. To use
|
||||
a skill, call `readSkillFile(skillName="<name>", filePath="SKILL.md")`
|
||||
first to read its instructions, then follow what SKILL.md tells you.
|
||||
Calling a skill name as a tool will fail with "Tool not found".
|
||||
|
||||
Note: this returns Skills (vendor-installable docs), not Agents.
|
||||
For Agents, use `listAvailableAgents`.
|
||||
|
||||
Returns: A formatted list of active skills with name, icon, and description.
|
||||
""")
|
||||
@ -160,27 +167,35 @@ public class SkillFileTool {
|
||||
List<ResolvedSkill> activeSkills = runtimeService.getActiveSkills();
|
||||
|
||||
if (activeSkills.isEmpty()) {
|
||||
return "当前没有可用的技能（Skills）。";
|
||||
return "No skills are currently available.";
|
||||
}
|
||||
|
||||
StringBuilder sb = new StringBuilder("可用技能（Skills）列表：\n\n");
|
||||
// Issue #46: render as a table with the call pattern stated up front,
|
||||
// instead of a `- **Name** — desc` list that primes the LLM to call
|
||||
// the names directly as tools.
|
||||
StringBuilder sb = new StringBuilder();
|
||||
sb.append("⚠️ These are Skills (documentation packages), NOT directly callable tools.\n");
|
||||
sb.append("To use any of them, call:\n");
|
||||
sb.append(" readSkillFile(skillName=\"<name from below>\", filePath=\"SKILL.md\")\n");
|
||||
sb.append("then follow what SKILL.md tells you (typically `runSkillScript`).\n\n");
|
||||
sb.append("| Skill name | Description |\n");
|
||||
sb.append("|------------|-------------|\n");
|
||||
for (ResolvedSkill skill : activeSkills) {
|
||||
sb.append("- **").append(skill.getName()).append("**");
|
||||
sb.append("| `").append(skill.getName()).append("`");
|
||||
if (skill.getIcon() != null && !skill.getIcon().isBlank()) {
|
||||
sb.append(" ").append(skill.getIcon());
|
||||
}
|
||||
sb.append(" | ");
|
||||
if (skill.getDescription() != null && !skill.getDescription().isBlank()) {
|
||||
String desc = skill.getDescription();
|
||||
if (desc.length() > 200) {
|
||||
desc = desc.substring(0, 200) + "...";
|
||||
}
|
||||
sb.append(" — ").append(desc);
|
||||
sb.append(desc.replace("|", "\\|").replace("\n", " "));
|
||||
}
|
||||
sb.append("\n");
|
||||
sb.append(" |\n");
|
||||
}
|
||||
|
||||
sb.append("\n共 ").append(activeSkills.size()).append(" 个可用技能。");
|
||||
sb.append("\n\n使用 `readSkillFile` 读取技能详情，使用 `runSkillScript` 执行技能脚本。");
|
||||
sb.append("\nTotal: ").append(activeSkills.size()).append(" skill(s).");
|
||||
return sb.toString();
|
||||
}
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user