mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-25 20:05:46 -06:00
fix(skills): report execution-directory skill precedence collisions (#125865)
loadMergedWorkspaceSkills dropped execution-directory skills whose name collided with an agent-workspace skill without any log, diagnostic, or record, while the sibling loadSkillEntries merge warned on every collision with winner/loser source and path. An operator's execution-directory skill simply did not exist and nothing said why. Extract the existing warning into warnSkillPrecedenceCollision and call it from both merge paths. Precedence is unchanged: the agent workspace still wins.
This commit is contained in:
committed by
GitHub
parent
005a63f252
commit
9bbb33bd2e
@@ -1,4 +1,5 @@
|
||||
// Workspace precedence tests cover precedence between workspace, plugin, and bundled skills.
|
||||
import fs from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest";
|
||||
import { resetLogger, setLoggerOverride } from "../../logging/logger.js";
|
||||
@@ -8,6 +9,7 @@ import { createFixtureSuite } from "../../test-utils/fixture-suite.js";
|
||||
import { writeSkill } from "../test-support/e2e-test-helpers.js";
|
||||
import type { OpenClawSkillMetadata, SkillEntry } from "../types.js";
|
||||
import { createSyntheticSourceInfo } from "./skill-contract.js";
|
||||
import { loadMergedWorkspaceSkills } from "./workspace-skill-loader.js";
|
||||
import { buildSkillSnapshot } from "./workspace-skill-prompt.js";
|
||||
|
||||
const buildWorkspaceSkillsPrompt = (
|
||||
@@ -47,6 +49,18 @@ function captureWarningLogger() {
|
||||
return warn;
|
||||
}
|
||||
|
||||
function captureJsonWarningLogger() {
|
||||
setLoggerOverride({ level: "silent", consoleLevel: "warn", consoleStyle: "json" });
|
||||
const warn = vi.fn();
|
||||
loggingState.rawConsole = {
|
||||
log: vi.fn(),
|
||||
info: vi.fn(),
|
||||
warn,
|
||||
error: vi.fn(),
|
||||
};
|
||||
return warn;
|
||||
}
|
||||
|
||||
function createSkillEntry(params: {
|
||||
name: string;
|
||||
description?: string;
|
||||
@@ -142,6 +156,72 @@ describe("buildWorkspaceSkillsPrompt", () => {
|
||||
expect(warningText).toContain("winner=openclaw-bundled:~/.bundled/demo-skill/SKILL.md");
|
||||
expect(warningText).toContain("loser=openclaw-extra:~/.extra/demo-skill/SKILL.md");
|
||||
});
|
||||
|
||||
it("reports execution-directory collisions while keeping workspace precedence", async () => {
|
||||
const agentWorkspaceDir = await fixtureSuite.createCaseDir("agent-workspace-collision");
|
||||
const executionWorkspaceDir = await fixtureSuite.createCaseDir("execution-workspace-collision");
|
||||
const workspaceSkillFile = path.join(agentWorkspaceDir, "skills", "demo-skill", "SKILL.md");
|
||||
const executionSkillFile = path.join(executionWorkspaceDir, "skills", "demo-skill", "SKILL.md");
|
||||
await writeSkill({
|
||||
dir: path.dirname(workspaceSkillFile),
|
||||
name: "demo-skill",
|
||||
description: "Workspace version",
|
||||
});
|
||||
await writeSkill({
|
||||
dir: path.dirname(executionSkillFile),
|
||||
name: "demo-skill",
|
||||
description: "Execution version",
|
||||
});
|
||||
const warn = captureJsonWarningLogger();
|
||||
|
||||
const entries = loadMergedWorkspaceSkills({
|
||||
agentWorkspaceDir,
|
||||
executionSkillsDir: path.join(executionWorkspaceDir, "skills"),
|
||||
managedSkillsDir: path.join(agentWorkspaceDir, ".managed"),
|
||||
bundledSkillsDir: "",
|
||||
pluginSkillsDir: path.join(agentWorkspaceDir, ".plugin-skills"),
|
||||
});
|
||||
const warning = JSON.parse(String(warn.mock.calls[0]?.[0])) as Record<string, unknown>;
|
||||
|
||||
expect(entries.find((entry) => entry.skill.name === "demo-skill")?.skill.description).toBe(
|
||||
"Workspace version",
|
||||
);
|
||||
expect(warning).toMatchObject({
|
||||
message: "Skill precedence collision resolved.",
|
||||
skill: "demo-skill",
|
||||
winnerPath: workspaceSkillFile,
|
||||
loserPath: executionSkillFile,
|
||||
});
|
||||
});
|
||||
|
||||
it("does not report execution-directory collisions for the same canonical skill file", async () => {
|
||||
const agentWorkspaceDir = await fixtureSuite.createCaseDir("agent-workspace-symlink");
|
||||
const executionWorkspaceDir = await fixtureSuite.createCaseDir("execution-workspace-symlink");
|
||||
const workspaceSkillsDir = path.join(agentWorkspaceDir, "skills");
|
||||
await writeSkill({
|
||||
dir: path.join(workspaceSkillsDir, "demo-skill"),
|
||||
name: "demo-skill",
|
||||
description: "Workspace version",
|
||||
});
|
||||
const executionSkillsDir = path.join(executionWorkspaceDir, "skills");
|
||||
await fs.symlink(
|
||||
workspaceSkillsDir,
|
||||
executionSkillsDir,
|
||||
process.platform === "win32" ? "junction" : "dir",
|
||||
);
|
||||
const warn = captureWarningLogger();
|
||||
|
||||
const entries = loadMergedWorkspaceSkills({
|
||||
agentWorkspaceDir,
|
||||
executionSkillsDir,
|
||||
managedSkillsDir: path.join(agentWorkspaceDir, ".managed"),
|
||||
bundledSkillsDir: "",
|
||||
pluginSkillsDir: path.join(agentWorkspaceDir, ".plugin-skills"),
|
||||
});
|
||||
|
||||
expect(entries.filter((entry) => entry.skill.name === "demo-skill")).toHaveLength(1);
|
||||
expect(warn).not.toHaveBeenCalled();
|
||||
});
|
||||
it("gates by bins, config, and always", async () => {
|
||||
const workspaceDir = await fixtureSuite.createCaseDir("workspace");
|
||||
const entries = [
|
||||
|
||||
@@ -97,6 +97,29 @@ function warnInvalidSkill(source: string, diagnostic: LocalSkillLoadDiagnostic):
|
||||
});
|
||||
}
|
||||
|
||||
// Shared by both merge paths so a dropped skill is never silent: the by-name merge in
|
||||
// loadSkillEntries and the execution-directory filter in loadMergedWorkspaceSkills.
|
||||
function warnSkillPrecedenceCollision(winner: Skill, loser: Skill): void {
|
||||
// One file reachable through two roots is not a collision. normalizeWorkspaceSkillRoots only
|
||||
// rejects the literal <agentWorkspaceDir>/skills path, so a symlinked execution dir still
|
||||
// arrives here with both sides naming the same skill.
|
||||
if (canonicalizePath(winner.filePath) === canonicalizePath(loser.filePath)) {
|
||||
return;
|
||||
}
|
||||
const collisionName = winner.name.slice(0, 128);
|
||||
skillsLogger.warn("Skill precedence collision resolved.", {
|
||||
skill: collisionName,
|
||||
winnerSource: winner.source,
|
||||
loserSource: loser.source,
|
||||
winnerPath: winner.filePath,
|
||||
loserPath: loser.filePath,
|
||||
consoleMessage:
|
||||
`Skill precedence collision: skill="${collisionName}" ` +
|
||||
`winner=${winner.source}:${compactSkillPath(winner.filePath)} ` +
|
||||
`loser=${loser.source}:${compactSkillPath(loser.filePath)}`,
|
||||
});
|
||||
}
|
||||
|
||||
function filterSkillEntries(
|
||||
entries: SkillEntry[],
|
||||
config?: OpenClawConfig,
|
||||
@@ -370,22 +393,8 @@ function loadSkillEntries(
|
||||
return;
|
||||
}
|
||||
const replaced = merged.get(record.skill.name);
|
||||
if (
|
||||
replaced &&
|
||||
canonicalizePath(replaced.skill.filePath) !== canonicalizePath(record.skill.filePath)
|
||||
) {
|
||||
const collisionName = record.skill.name.slice(0, 128);
|
||||
skillsLogger.warn("Skill precedence collision resolved.", {
|
||||
skill: collisionName,
|
||||
winnerSource: record.skill.source,
|
||||
loserSource: replaced.skill.source,
|
||||
winnerPath: record.skill.filePath,
|
||||
loserPath: replaced.skill.filePath,
|
||||
consoleMessage:
|
||||
`Skill precedence collision: skill="${collisionName}" ` +
|
||||
`winner=${record.skill.source}:${compactSkillPath(record.skill.filePath)} ` +
|
||||
`loser=${replaced.skill.source}:${compactSkillPath(replaced.skill.filePath)}`,
|
||||
});
|
||||
if (replaced) {
|
||||
warnSkillPrecedenceCollision(record.skill, replaced.skill);
|
||||
}
|
||||
merged.set(record.skill.name, record);
|
||||
};
|
||||
@@ -531,12 +540,19 @@ export function loadMergedWorkspaceSkills(
|
||||
canExec: params.eligibility?.nodeSkills?.canExec,
|
||||
node: params.eligibility?.nodeSkills?.node,
|
||||
});
|
||||
const agentNames = new Set(agentEntries.map((entry) => entry.skill.name));
|
||||
const agentEntriesByName = new Map(agentEntries.map((entry) => [entry.skill.name, entry]));
|
||||
const executionEntries = loadSkillEntries(agentWorkspaceDir, {
|
||||
...params,
|
||||
workspaceOnly: true,
|
||||
workspaceSkillsDir: executionSkillsDir,
|
||||
}).filter((entry) => !agentNames.has(entry.skill.name));
|
||||
}).filter((entry) => {
|
||||
const agentEntry = agentEntriesByName.get(entry.skill.name);
|
||||
if (!agentEntry) {
|
||||
return true;
|
||||
}
|
||||
warnSkillPrecedenceCollision(agentEntry.skill, entry.skill);
|
||||
return false;
|
||||
});
|
||||
const effectiveSkillFilter = resolveEffectiveWorkspaceSkillFilter(params);
|
||||
return filterSkillEntries(
|
||||
[...agentEntries, ...executionEntries],
|
||||
|
||||
Reference in New Issue
Block a user