From 51fc34f5854dc5ff30e54b4ae7958db60e8b4aeb Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 21 Jul 2026 19:17:20 -0700 Subject: [PATCH] FN-8465: align skill toggles with canonical paths Align session skill filtering with Skills-view path identities. - Match session inclusions, exclusions, and diagnostics by skills-relative body path. - Ignore stale flat toggle keys for categorized skills and cover the display/session invariant. - Document the behavior and add a patch changeset. Files changed: .../fn-8465-legacy-skill-toggle-path-match.md | 7 ++ docs/dashboard-guide.md | 2 +- .../legacy-flat-skill-toggle-session-divergence.md | 41 ++++++++ .../engine/src/__tests__/skill-resolver.test.ts | 73 ++++++++++++++ packages/engine/src/skill-resolver.ts | 105 ++++++++++++++------- 5 files changed, 191 insertions(+), 37 deletions(-) Fusion-Task-Id: FN-8465 Fusion-Task-Lineage: 042fc7f4-08a3-42f4-9a6f-52f066d585e8 Co-authored-by: Fusion (runfusion.ai) --- .../fn-8465-legacy-skill-toggle-path-match.md | 7 ++ docs/dashboard-guide.md | 2 +- ...cy-flat-skill-toggle-session-divergence.md | 41 +++++++ .../src/__tests__/skill-resolver.test.ts | 73 ++++++++++++ packages/engine/src/skill-resolver.ts | 105 ++++++++++++------ 5 files changed, 191 insertions(+), 37 deletions(-) create mode 100644 .changeset/fn-8465-legacy-skill-toggle-path-match.md create mode 100644 docs/solutions/logic-errors/legacy-flat-skill-toggle-session-divergence.md diff --git a/.changeset/fn-8465-legacy-skill-toggle-path-match.md b/.changeset/fn-8465-legacy-skill-toggle-path-match.md new file mode 100644 index 0000000000..6da788cf77 --- /dev/null +++ b/.changeset/fn-8465-legacy-skill-toggle-path-match.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Ignore stale flat skill-toggle keys so session skills match the Skills view after category layouts. +category: fix +dev: Session skillsOverride matches +/- patterns by skills/-relative path (not bareSkillName alone); legacy flat disables no longer suppress nested skillFiles bodies (GitHub #2385 / FN-8465). diff --git a/docs/dashboard-guide.md b/docs/dashboard-guide.md index 40936bb092..9a4b2c0a95 100644 --- a/docs/dashboard-guide.md +++ b/docs/dashboard-guide.md @@ -1620,7 +1620,7 @@ For setup prerequisites, security caveats for tokenized URLs/QR links, and troub The Skills view now supports the full browse-and-install loop for skills.sh entries: use **Skills Catalog** to search the catalog, click **Install** on any card with a source repository, and the dashboard will run the same installer as the CLI (`npx skills add -y -a pi`, with `--skill ` when applicable). On success, the view refreshes **Discovered Skills** immediately so the newly installed skill appears without a page reload. -The Skills API provides endpoints for managing execution skills. Skills are toggled via project-scoped settings in `.fusion/settings.json`. +The Skills API provides endpoints for managing execution skills. Skills are toggled via project-scoped settings in `.fusion/settings.json`. Toggle entries match the skill body’s relative path beneath `skills/` (for example, `api/api-versioning/SKILL.md`), not just its displayed name. Stale flat-layout entries such as `-api-versioning/SKILL.md` are ignored for skills that now use a categorized body path, keeping the Skills view and agent-session manifest aligned. ![Skills view](./screenshots/skills-view.png) diff --git a/docs/solutions/logic-errors/legacy-flat-skill-toggle-session-divergence.md b/docs/solutions/logic-errors/legacy-flat-skill-toggle-session-divergence.md new file mode 100644 index 0000000000..bd61c5a425 --- /dev/null +++ b/docs/solutions/logic-errors/legacy-flat-skill-toggle-session-divergence.md @@ -0,0 +1,41 @@ +--- +title: "Legacy flat skill-toggle keys silently suppressed categorized session skills" +date: 2026-07-21 +category: docs/solutions/logic-errors +module: engine (session skill resolver) +problem_type: logic_error +component: path_identity +symptoms: + - "The Skills view showed a categorized plugin skill enabled while agent sessions omitted it." + - "Deleting a stale flat `-name/SKILL.md` settings entry restored the skill to the session manifest." +root_cause: logic_error +resolution_type: code_fix +severity: medium +related_components: + - skill-resolver + - skill-settings + - skills-adapter +tags: + - skills + - settings + - plugin-skills + - path-identity + - session-manifest + - legacy-settings +--- + +# Legacy flat skill-toggle keys silently suppressed categorized session skills + +## Problem + +Project skill toggles are persisted as signed paths such as `-api/api-versioning/SKILL.md`. The Skills view resolves those paths relative to a skill body beneath `skills/`, but the session override resolver also accepted a bare skill-name match. After a plugin moved a body from `skills/api-versioning/SKILL.md` to `skills/api/api-versioning/SKILL.md`, an old `-api-versioning/SKILL.md` entry no longer matched the view yet still excluded the session skill because pi exposed `skill.name === "api-versioning"`. + +## Solution + +Session allow, exclusion, and disabled-diagnostic matching now use the same canonical identity as the Skills view: the complete body-relative path beneath the final `skills/` segment. Absolute patterns continue to match exact file paths. A flat `name/SKILL.md` pattern remains compatible only when the discovered body path is itself flat; it cannot match a categorized `category/name/SKILL.md` body merely by sharing the final directory name. + +## Prevention + +- Treat stored setting keys as identities, not labels: compare the complete normalized path at every reader. +- When a layout changes, test every reader of the persisted key, including session assembly and diagnostics, not only the UI display. +- Preserve legacy compatibility only where the legacy key still identifies the current on-disk path; otherwise ignore it consistently rather than allowing one reader to honor it. diff --git a/packages/engine/src/__tests__/skill-resolver.test.ts b/packages/engine/src/__tests__/skill-resolver.test.ts index 3aacf6191f..dea3fb4fb7 100644 --- a/packages/engine/src/__tests__/skill-resolver.test.ts +++ b/packages/engine/src/__tests__/skill-resolver.test.ts @@ -2,6 +2,7 @@ * Unit tests for skill resolver. */ +import { resolvePluginSkillEnabled } from "@fusion/core"; import { beforeEach, describe, expect, it, vi } from "vitest"; /* @@ -1152,6 +1153,78 @@ describe("createSkillsOverrideFromSelection", () => { expect(notFoundInfoDiagnostics).toHaveLength(0); }); + it("keeps nested skills available when legacy flat exclusions are ignored by the Skills view", () => { + const settings = { skills: ["-api-versioning/SKILL.md"] }; + const dir = createMockProjectDir(settings); + const selection = resolveSessionSkills({ projectRootDir: dir }); + const result = createSkillsOverrideFromSelection(selection, { + sessionPurpose: "executor", + })({ + skills: [ + { name: "api-versioning", filePath: "/plugins/foo/skills/api/api-versioning/SKILL.md", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false }, + { name: "unrelated", filePath: "/plugins/foo/skills/other/unrelated/SKILL.md", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false }, + ], + diagnostics: [], + }); + + expect(resolvePluginSkillEnabled( + settings, + "foo", + "api-versioning", + true, + "skills/api/api-versioning/SKILL.md", + )).toBe(true); + expect(result.skills.map((skill) => skill.name)).toEqual(["api-versioning", "unrelated"]); + expect(result.diagnostics.some((diagnostic) => + diagnostic.message.includes("api-versioning/SKILL.md") && diagnostic.message.includes("disabled"), + )).toBe(false); + }); + + it("does not let a legacy flat enable create an empty allow-list for nested skills", () => { + const settings = { skills: ["+api-versioning/SKILL.md"] }; + const dir = createMockProjectDir(settings); + const selection = resolveSessionSkills({ projectRootDir: dir }); + const result = createSkillsOverrideFromSelection(selection)({ + skills: [ + { name: "api-versioning", filePath: "/plugins/foo/skills/api/api-versioning/SKILL.md", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false }, + { name: "unrelated", filePath: "/plugins/foo/skills/other/unrelated/SKILL.md", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false }, + ], + diagnostics: [], + }); + + expect(resolvePluginSkillEnabled( + settings, + "foo", + "api-versioning", + true, + "skills/api/api-versioning/SKILL.md", + )).toBe(true); + expect(result.skills.map((skill) => skill.name)).toEqual(["api-versioning", "unrelated"]); + expect(result.diagnostics.some((diagnostic) => + diagnostic.message.includes("api-versioning/SKILL.md") && diagnostic.message.includes("not found"), + )).toBe(false); + }); + + it.each([ + "-api/api-versioning/SKILL.md", + "-skills/api/api-versioning/SKILL.md", + ])("excludes nested skills for the canonical path %s", (pattern) => { + const dir = createMockProjectDir({ skills: [pattern] }); + const selection = resolveSessionSkills({ projectRootDir: dir }); + const result = createSkillsOverrideFromSelection(selection)({ + skills: [ + { name: "api-versioning", filePath: "/plugins/foo/skills/api/api-versioning/SKILL.md", description: "", baseDir: "", sourceInfo: {} as any, disableModelInvocation: false }, + ], + diagnostics: [], + }); + + expect(result.skills).toEqual([]); + expect(result.diagnostics).toContainEqual(expect.objectContaining({ + type: "warning", + message: expect.stringContaining("disabled by project execution settings"), + })); + }); + it("exclusion patterns with /SKILL.md suffix correctly exclude pi-discovered skills", () => { const dir = createMockProjectDir({ skills: ["+web-research/SKILL.md", "-paperclip/SKILL.md"], diff --git a/packages/engine/src/skill-resolver.ts b/packages/engine/src/skill-resolver.ts index 14d5efb263..1da7df736d 100644 --- a/packages/engine/src/skill-resolver.ts +++ b/packages/engine/src/skill-resolver.ts @@ -12,7 +12,7 @@ import { existsSync, readFileSync } from "node:fs"; import { dirname, join, resolve } from "node:path"; import type { ResourceDiagnostic, Skill } from "@earendil-works/pi-coding-agent"; -import { getProjectRootFromWorktree } from "@fusion/core"; +import { getProjectRootFromWorktree, normalizeStoredSkillPath } from "@fusion/core"; import { piLog } from "./logger.js"; // ── Project Root Resolution ────────────────────────────────────────────────── @@ -173,21 +173,59 @@ function isExclusionPattern(pattern: string): boolean { } /** - * Extract the bare skill name for matching purposes. - * - * Fusion conventions use two-segment names like "web-research/SKILL.md" (from - * extractSkillName and normalizeAgentSkills), but pi-coding-agent sets Skill.name - * to just the parent directory (e.g. "web-research"). This helper strips common - * suffixes so both sides can be compared: - * - * "web-research/SKILL.md" → "web-research" - * "skills/web-research/SKILL.md" → "web-research" - * "web-research" → "web-research" - * "/abs/path/skills/web-research/SKILL.md" → left unchanged (absolute paths - * are matched by filePath comparison, not by this helper) + * Return the canonical body path beneath a discovered skill's `skills/` root. + * A path without that root is only eligible for exact filePath matching. */ -function bareSkillName(name: string): string { - return name.replace(/\/SKILL\.md$/i, ""); +function skillBodyRelativePath(filePath: string): string | undefined { + const normalizedFilePath = filePath.replaceAll("\\", "/"); + const lowerCasePath = normalizedFilePath.toLowerCase(); + const segmentIndex = lowerCasePath.lastIndexOf("/skills/"); + + if (segmentIndex >= 0) { + return normalizedFilePath.slice(segmentIndex + "/skills/".length); + } + if (lowerCasePath.startsWith("skills/")) { + return normalizedFilePath.slice("skills/".length); + } + return undefined; +} + +/** + * FNXC:SkillResolution 2026-07-21-00:00: + * GitHub #2385 / FN-8465 requires session filtering to use the same skills-relative body-path identity as the Skills view. Legacy `-name/SKILL.md` entries must not suppress a re-categorized `skills/category/name/SKILL.md` body merely because pi exposes the same bare Skill.name. + */ +function skillMatchesExecutionPattern(skill: Skill, pattern: string): boolean { + if (skill.filePath === pattern) { + return true; + } + + const bodyRelativePath = skillBodyRelativePath(skill.filePath); + return bodyRelativePath !== undefined + && normalizeStoredSkillPath(bodyRelativePath).toLowerCase() + === normalizeStoredSkillPath(pattern).toLowerCase(); +} + +/** + * FNXC:SkillResolution 2026-07-21-00:00: + * Legacy flat `+name/SKILL.md` keys must be ignored as well as legacy disables. + * Otherwise an unmatched stale allow key activates the session allow-list and + * suppresses re-categorized skills even though the Skills view shows them enabled. + */ +function isLegacyFlatPatternForNestedSkill(skill: Skill, pattern: string): boolean { + const bodyRelativePath = skillBodyRelativePath(skill.filePath); + if (!bodyRelativePath) return false; + + const bodySegments = normalizeStoredSkillPath(bodyRelativePath) + .toLowerCase() + .split("/"); + const patternSegments = normalizeStoredSkillPath(pattern) + .toLowerCase() + .split("/"); + + return bodySegments.length > 2 + && patternSegments.length === 2 + && bodySegments.at(-2) === patternSegments[0] + && bodySegments.at(-1) === patternSegments[1]; } /** @@ -398,34 +436,31 @@ export function createSkillsOverrideFromSelection( // Determine the effective filter criteria // When requestedSkillNames is provided without patterns, filter by name // When patterns are provided, filter by file path - const hasPatterns = allowedSkillPaths.size > 0; const hasRequestedNames = Boolean(requestedSkillNames && requestedSkillNames.length > 0); + // A stale flat enable must not turn into an empty allow-list after a body + // moved to a category. Keep truly missing configured patterns intact so + // their existing missing-pattern diagnostic and filtering semantics remain. + const effectiveAllowedSkillPaths = new Set( + [...allowedSkillPaths].filter((pattern) => !base.skills.some( + (skill) => isLegacyFlatPatternForNestedSkill(skill, pattern), + )), + ); + const hasPatterns = effectiveAllowedSkillPaths.size > 0; + // Filter skills // Skills must match the inclusion criteria AND not be in the exclusion list const hasExcluded = excludedSkillPaths.size > 0; let filteredSkills: Skill[]; - // Build a name-based lookup for pattern/exclusion matching. - // Settings patterns are relative (e.g. "web-research/SKILL.md") but - // skill.filePath is absolute. Match against skill.name instead so - // that patterns written by toggleExecutionSkill() actually resolve. - // - // pi-coding-agent sets Skill.name to the parent directory name - // (e.g. "web-research") while Fusion uses two-segment names - // (e.g. "web-research/SKILL.md"). bareSkillName() normalizes - // both sides so the comparison succeeds. - const skillNameMatches = (skill: Skill, pattern: string): boolean => - bareSkillName(skill.name).toLowerCase() === bareSkillName(pattern).toLowerCase() - || skill.filePath === pattern; const isExcluded = (skill: Skill): boolean => { - for (const ep of excludedSkillPaths) { - if (skillNameMatches(skill, ep)) return true; + for (const excludedPath of excludedSkillPaths) { + if (skillMatchesExecutionPattern(skill, excludedPath)) return true; } return false; }; const isAllowed = (skill: Skill): boolean => { - for (const ap of allowedSkillPaths) { - if (skillNameMatches(skill, ap)) return true; + for (const allowedPath of effectiveAllowedSkillPaths) { + if (skillMatchesExecutionPattern(skill, allowedPath)) return true; } return false; }; @@ -454,10 +489,8 @@ export function createSkillsOverrideFromSelection( // Check for excluded paths that DO match a discovered skill (disabled) const purpose = sessionPurpose ? ` [${sessionPurpose}]` : ""; - const discoveredBareNames = new Set(base.skills.map((s) => bareSkillName(s.name).toLowerCase())); - const discoveredFilePaths = new Set(base.skills.map((s) => s.filePath)); const hasDiscoveredMatch = (pattern: string): boolean => - discoveredBareNames.has(bareSkillName(pattern).toLowerCase()) || discoveredFilePaths.has(pattern); + base.skills.some((skill) => skillMatchesExecutionPattern(skill, pattern)); for (const excludedPath of excludedSkillPaths) { if (hasDiscoveredMatch(excludedPath)) { @@ -470,7 +503,7 @@ export function createSkillsOverrideFromSelection( } // Check for configured patterns (allowed paths) that don't match any discovered skill - for (const allowedPath of allowedSkillPaths) { + for (const allowedPath of effectiveAllowedSkillPaths) { if (!hasDiscoveredMatch(allowedPath)) { newDiagnostics.push({ type: "info" as ResourceDiagnostic["type"],