diff --git a/src/skills/loading/workspace-precedence.test.ts b/src/skills/loading/workspace-precedence.test.ts index ac7ea330d70b..31736a838bb3 100644 --- a/src/skills/loading/workspace-precedence.test.ts +++ b/src/skills/loading/workspace-precedence.test.ts @@ -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; + + 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 = [ diff --git a/src/skills/loading/workspace-skill-loader.ts b/src/skills/loading/workspace-skill-loader.ts index df585dc78b7e..64634d3e6487 100644 --- a/src/skills/loading/workspace-skill-loader.ts +++ b/src/skills/loading/workspace-skill-loader.ts @@ -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 /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],