FN-7777: fix skill content/file routes double-decoding skill IDs
Fixes Skills view showing "Skill not found" for every skill, including built-in Fusion skills, by removing a redundant URL decode in the content/file lookup routes. - register-agent-skills-routes.ts: stop calling decodeURIComponent on req.params.id in the /skills/:id/content and /skills/:id/file handlers since Express 5 already decodes route params once; a second decode corrupted encoded source segments so IDs no longer matched computeSkillId's discovery output - Added FNXC:Skills comments documenting the Express 5 single-decode behavior and why re-decoding breaks plugin/npm/path source IDs - routes-skills.test.ts: added regression coverage for exact-ID content/file lookups across skill sources - Added a patch changeset describing the fix for release notes Files changed: .changeset/fn-7777-skills-content-not-found.md | 7 + .../dashboard/src/__tests__/routes-skills.test.ts | 144 ++++++++++++++++++++- .../src/routes/register-agent-skills-routes.ts | 26 ++-- 3 files changed, 159 insertions(+), 18 deletions(-) Fusion-Task-Id: FN-7777 Fusion-Task-Lineage: dc190b9a-ac5c-4f8b-9d65-11bfd5598db2 Co-authored-by: Fusion (runfusion.ai) <noreply@runfusion.ai>
This commit is contained in:
7
.changeset/fn-7777-skills-content-not-found.md
Normal file
7
.changeset/fn-7777-skills-content-not-found.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Fix Skills view showing "Skill not found" when opening any skill's content.
|
||||
category: fix
|
||||
dev: The /skills/:id/content and /skills/:id/file routes double-decoded the URL param (Express 5 already decodes route params once), corrupting the encoded source segment so the id no longer matched computeSkillId's discovery output. Routes now use the once-decoded canonical id (FN-7777).
|
||||
@@ -3,7 +3,7 @@ import { EventEmitter } from "node:events";
|
||||
import type { Task } from "@fusion/core";
|
||||
import { request } from "../test-request.js";
|
||||
import { createServer } from "../server.js";
|
||||
import type { SkillsAdapter } from "../skills-adapter.js";
|
||||
import type { DiscoveredSkill, SkillsAdapter } from "../skills-adapter.js";
|
||||
import { computeSkillId, parseSkillId } from "../skills-adapter.js";
|
||||
|
||||
class MockStore extends EventEmitter {
|
||||
@@ -51,6 +51,60 @@ class MockStore extends EventEmitter {
|
||||
}
|
||||
|
||||
// Mock skills adapter for testing
|
||||
function createExactLookupSkillsAdapter(skills: DiscoveredSkill[]): SkillsAdapter {
|
||||
return createMockSkillsAdapter({
|
||||
discoverSkills: vi.fn().mockResolvedValue(skills),
|
||||
readSkillContent: vi.fn().mockImplementation(async (_rootDir: string, skillId: string) => {
|
||||
if (!parseSkillId(skillId)) {
|
||||
throw new Error(`Invalid skill ID format: ${skillId}`);
|
||||
}
|
||||
const skill = skills.find((entry) => entry.id === skillId);
|
||||
if (!skill) {
|
||||
throw new Error(`Skill not found: ${skillId}`);
|
||||
}
|
||||
return {
|
||||
name: skill.name,
|
||||
skillMd: `# ${skill.name}\n\nCanonical id: ${skill.id}`,
|
||||
files: [{ name: "notes.txt", relativePath: "notes.txt", type: "file" as const }],
|
||||
};
|
||||
}),
|
||||
readSkillFileContent: vi.fn().mockImplementation(async (_rootDir: string, skillId: string, relativePath: string) => {
|
||||
if (!parseSkillId(skillId)) {
|
||||
throw new Error(`Invalid skill ID format: ${skillId}`);
|
||||
}
|
||||
const skill = skills.find((entry) => entry.id === skillId);
|
||||
if (!skill) {
|
||||
throw new Error(`Skill not found: ${skillId}`);
|
||||
}
|
||||
if (relativePath !== "notes.txt") {
|
||||
throw new Error(`Skill file not found: ${relativePath}`);
|
||||
}
|
||||
return {
|
||||
name: "notes.txt",
|
||||
relativePath,
|
||||
content: `Supplement for ${skill.id}`,
|
||||
isText: true,
|
||||
};
|
||||
}),
|
||||
});
|
||||
}
|
||||
|
||||
function createDiscoveredSkill(source: string, relativePath: string): DiscoveredSkill {
|
||||
return {
|
||||
id: computeSkillId(source, relativePath),
|
||||
name: relativePath.replace(/^skills\//, ""),
|
||||
path: `/tmp/fn-skills/${relativePath}`,
|
||||
relativePath,
|
||||
enabled: true,
|
||||
metadata: {
|
||||
source,
|
||||
scope: "project",
|
||||
origin: source === "*" ? "top-level" : "package",
|
||||
baseDir: "/tmp/fn-skills",
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
function createMockSkillsAdapter(overrides?: Partial<SkillsAdapter>): SkillsAdapter {
|
||||
return {
|
||||
discoverSkills: vi.fn().mockResolvedValue([
|
||||
@@ -246,10 +300,32 @@ describe("Skills routes", () => {
|
||||
});
|
||||
expect(mockAdapter.readSkillContent).toHaveBeenCalledWith(
|
||||
"/tmp/fn-skills",
|
||||
"npm:@example/skill::skills/example/SKILL.md",
|
||||
"npm%3A%40example%2Fskill::skills/example/SKILL.md",
|
||||
);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["disk source", createDiscoveredSkill("*", "skills/local/SKILL.md")],
|
||||
["plugin source", createDiscoveredSkill("plugin:foo", "skills/bar/SKILL.md")],
|
||||
["npm scoped source", createDiscoveredSkill("npm:@example/skill", "skills/example/SKILL.md")],
|
||||
])("preserves the canonical ID for %s content lookup", async (_label, skill) => {
|
||||
const mockAdapter = createExactLookupSkillsAdapter([skill]);
|
||||
const store = new MockStore();
|
||||
const app = createServer(store as any, { skillsAdapter: mockAdapter as SkillsAdapter });
|
||||
|
||||
const res = await request(app, "GET", `/api/skills/${encodeURIComponent(skill.id)}/content`);
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({
|
||||
content: {
|
||||
name: skill.name,
|
||||
skillMd: `# ${skill.name}\n\nCanonical id: ${skill.id}`,
|
||||
files: [{ name: "notes.txt", relativePath: "notes.txt", type: "file" }],
|
||||
},
|
||||
});
|
||||
expect(mockAdapter.readSkillContent).toHaveBeenCalledWith("/tmp/fn-skills", skill.id);
|
||||
});
|
||||
|
||||
it("returns 404 when skills adapter is not configured", async () => {
|
||||
const store = new MockStore();
|
||||
const app = createServer(store as any, {});
|
||||
@@ -296,6 +372,70 @@ describe("Skills routes", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("GET /api/skills/:id/file", () => {
|
||||
it.each([
|
||||
["disk source", createDiscoveredSkill("*", "skills/local/SKILL.md")],
|
||||
["plugin source", createDiscoveredSkill("plugin:foo", "skills/bar/SKILL.md")],
|
||||
["npm scoped source", createDiscoveredSkill("npm:@example/skill", "skills/example/SKILL.md")],
|
||||
])("preserves the canonical ID for %s file lookup", async (_label, skill) => {
|
||||
const mockAdapter = createExactLookupSkillsAdapter([skill]);
|
||||
const store = new MockStore();
|
||||
const app = createServer(store as any, { skillsAdapter: mockAdapter as SkillsAdapter });
|
||||
|
||||
const res = await request(app, "GET", `/api/skills/${encodeURIComponent(skill.id)}/file?path=${encodeURIComponent("notes.txt")}`);
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({
|
||||
file: {
|
||||
name: "notes.txt",
|
||||
relativePath: "notes.txt",
|
||||
content: `Supplement for ${skill.id}`,
|
||||
isText: true,
|
||||
},
|
||||
});
|
||||
expect(mockAdapter.readSkillFileContent).toHaveBeenCalledWith("/tmp/fn-skills", skill.id, "notes.txt");
|
||||
});
|
||||
|
||||
it("returns 400 when path is missing", async () => {
|
||||
const skill = createDiscoveredSkill("plugin:foo", "skills/bar/SKILL.md");
|
||||
const mockAdapter = createExactLookupSkillsAdapter([skill]);
|
||||
const store = new MockStore();
|
||||
const app = createServer(store as any, { skillsAdapter: mockAdapter as SkillsAdapter });
|
||||
|
||||
const res = await request(app, "GET", `/api/skills/${encodeURIComponent(skill.id)}/file`);
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body).toEqual({ error: "path is required", code: "invalid_path" });
|
||||
expect(mockAdapter.readSkillFileContent).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns 400 for invalid skill IDs", async () => {
|
||||
const mockAdapter = createExactLookupSkillsAdapter([]);
|
||||
const store = new MockStore();
|
||||
const app = createServer(store as any, { skillsAdapter: mockAdapter as SkillsAdapter });
|
||||
|
||||
const res = await request(app, "GET", `/api/skills/${encodeURIComponent("invalid")}/file?path=notes.txt`);
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body).toEqual({
|
||||
error: "Invalid skill ID format: invalid",
|
||||
code: "invalid_path",
|
||||
});
|
||||
});
|
||||
|
||||
it("returns 404 for non-existent skill files", async () => {
|
||||
const skill = createDiscoveredSkill("plugin:foo", "skills/bar/SKILL.md");
|
||||
const mockAdapter = createExactLookupSkillsAdapter([skill]);
|
||||
const store = new MockStore();
|
||||
const app = createServer(store as any, { skillsAdapter: mockAdapter as SkillsAdapter });
|
||||
|
||||
const res = await request(app, "GET", `/api/skills/${encodeURIComponent(skill.id)}/file?path=${encodeURIComponent("missing.txt")}`);
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
expect(res.body).toEqual({ error: "Skill file not found", code: "skill_file_not_found" });
|
||||
});
|
||||
});
|
||||
|
||||
describe("PATCH /api/skills/execution", () => {
|
||||
it("toggles skill execution successfully", async () => {
|
||||
const mockAdapter = createMockSkillsAdapter();
|
||||
|
||||
@@ -50,14 +50,11 @@ export function registerAgentSkillsRoutes(ctx: ApiRoutesContext): void {
|
||||
return;
|
||||
}
|
||||
|
||||
const encodedSkillId = req.params.id as string;
|
||||
let skillId = encodedSkillId;
|
||||
try {
|
||||
skillId = decodeURIComponent(encodedSkillId);
|
||||
} catch {
|
||||
res.status(400).json({ error: "Invalid skill ID", code: "invalid_skill_id" });
|
||||
return;
|
||||
}
|
||||
/*
|
||||
FNXC:Skills 2026-07-10-00:00:
|
||||
Express 5 decodes route params once before this handler runs. Discovery IDs from computeSkillId intentionally keep only the source segment URL-encoded, so pass req.params.id through unchanged; a second decode corrupts plugin/npm/path sources and breaks exact-ID skill content lookup (FN-7777).
|
||||
*/
|
||||
const skillId = req.params.id as string;
|
||||
|
||||
const rootDir = scopedStore.getRootDir();
|
||||
const content = await skillsAdapter.readSkillContent(rootDir, skillId);
|
||||
@@ -97,14 +94,11 @@ export function registerAgentSkillsRoutes(ctx: ApiRoutesContext): void {
|
||||
return;
|
||||
}
|
||||
|
||||
const encodedSkillId = req.params.id as string;
|
||||
let skillId = encodedSkillId;
|
||||
try {
|
||||
skillId = decodeURIComponent(encodedSkillId);
|
||||
} catch {
|
||||
res.status(400).json({ error: "Invalid skill ID", code: "invalid_skill_id" });
|
||||
return;
|
||||
}
|
||||
/*
|
||||
FNXC:Skills 2026-07-10-00:00:
|
||||
Express 5 decodes route params once before this handler runs. Discovery IDs from computeSkillId intentionally keep only the source segment URL-encoded, so pass req.params.id through unchanged; a second decode corrupts plugin/npm/path sources and breaks exact-ID supplementary file lookup (FN-7777).
|
||||
*/
|
||||
const skillId = req.params.id as string;
|
||||
|
||||
const rawPath = typeof req.query.path === "string" ? req.query.path : "";
|
||||
if (!rawPath.trim()) {
|
||||
|
||||
Reference in New Issue
Block a user