From 7dde43a63b0321961eceda35ad2abe714929d41b Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 4 Jun 2026 22:10:20 -0700 Subject: [PATCH] Address PR review feedback (#1428) - Add heal block to createPluginRouter's enable handler so it matches routes.ts (directory-path registrations re-pointed at entry files) - Test the 400 "no loadable entry file" install branch - Add a real-fs drift-guard test asserting the CLI and @fusion/core copies of resolvePluginEntryPath resolve identically Co-Authored-By: Claude Opus 4.8 (1M context) --- .../resolve-plugin-entry-path-sync.test.ts | 57 +++++++++++++++++++ .../src/__tests__/plugin-routes.test.ts | 43 ++++++++++++++ packages/dashboard/src/plugin-routes.ts | 14 +++++ 3 files changed, 114 insertions(+) create mode 100644 packages/cli/src/plugins/__tests__/resolve-plugin-entry-path-sync.test.ts diff --git a/packages/cli/src/plugins/__tests__/resolve-plugin-entry-path-sync.test.ts b/packages/cli/src/plugins/__tests__/resolve-plugin-entry-path-sync.test.ts new file mode 100644 index 0000000000..0fa5e80258 --- /dev/null +++ b/packages/cli/src/plugins/__tests__/resolve-plugin-entry-path-sync.test.ts @@ -0,0 +1,57 @@ +/** + * Drift guard for the intentionally duplicated resolvePluginEntryPath. + * + * The CLI keeps a local copy in bundled-plugin-install.ts (so its fs mocks + * work in tests) while @fusion/core owns the copy used by the dashboard + * install/enable routes. This test runs both against real on-disk layouts and + * asserts identical results, so a candidate-list change applied to one copy + * but not the other fails CI instead of silently diverging. + * + * No fs mocks here on purpose — vitest module mocks don't reach the + * externalized @fusion/core import, so real temp directories are the only + * seam that exercises both implementations equally. + */ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { resolvePluginEntryPath as cliResolve } from "../bundled-plugin-install.js"; +import { resolvePluginEntryPath as coreResolve } from "@fusion/core"; + +describe("resolvePluginEntryPath: CLI copy stays in sync with @fusion/core", () => { + let dir: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "entry-path-sync-")); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + function touch(relative: string) { + const full = join(dir, relative); + mkdirSync(join(full, ".."), { recursive: true }); + writeFileSync(full, "// entry\n"); + } + + const layouts: Array<{ name: string; files: string[]; expected: string | null }> = [ + { name: "bundled.js only", files: ["bundled.js"], expected: "bundled.js" }, + { name: "dist/index.js only", files: ["dist/index.js"], expected: "dist/index.js" }, + { name: "src/index.ts only", files: ["src/index.ts"], expected: "src/index.ts" }, + { name: "bundled.js preferred over src", files: ["bundled.js", "src/index.ts"], expected: "bundled.js" }, + { name: "dist preferred over src", files: ["dist/index.js", "src/index.ts"], expected: "dist/index.js" }, + { name: "all three → bundled.js", files: ["bundled.js", "dist/index.js", "src/index.ts"], expected: "bundled.js" }, + { name: "no entry files", files: ["README.md"], expected: null }, + ]; + + for (const layout of layouts) { + it(`resolves identically for: ${layout.name}`, () => { + for (const f of layout.files) touch(f); + const expected = layout.expected === null ? null : join(dir, layout.expected); + + expect(cliResolve(dir)).toBe(expected); + expect(coreResolve(dir)).toBe(expected); + }); + } +}); diff --git a/packages/dashboard/src/__tests__/plugin-routes.test.ts b/packages/dashboard/src/__tests__/plugin-routes.test.ts index b6e7f54eb2..6d09caf506 100644 --- a/packages/dashboard/src/__tests__/plugin-routes.test.ts +++ b/packages/dashboard/src/__tests__/plugin-routes.test.ts @@ -705,6 +705,29 @@ describe("POST /api/plugins/:id/enable — legacy directory path heal", () => { expect(pluginLoader.loadPlugin).toHaveBeenCalledWith("my-plugin"); }); + it("heals directory paths in createPluginRouter's enable handler too", async () => { + const dirPath = "/home/user/plugins/my-plugin"; + mockStat.mockResolvedValue({ isDirectory: () => true }); + mockExistsSync.mockImplementation((p: string) => p === `${dirPath}/bundled.js`); + (pluginStore.enablePlugin as ReturnType).mockResolvedValue({ + ...INSTALLED_PLUGIN, + path: dirPath, + }); + (pluginStore.updatePlugin as ReturnType).mockResolvedValue({ + ...INSTALLED_PLUGIN, + path: `${dirPath}/bundled.js`, + }); + + const app = express(); + app.use(express.json()); + app.use("/api/plugins", createPluginRouter(pluginStore, pluginLoader)); + const res = await REQUEST(app, "POST", "/api/plugins/my-plugin/enable", {}); + + expect(res.status).toBe(200); + expect(pluginStore.updatePlugin).toHaveBeenCalledWith("my-plugin", { path: `${dirPath}/bundled.js` }); + expect(pluginLoader.loadPlugin).toHaveBeenCalledWith("my-plugin"); + }); + it("leaves file paths untouched on enable", async () => { mockStatSync.mockReturnValue({ isDirectory: () => false }); (pluginStore.enablePlugin as ReturnType).mockResolvedValue({ @@ -744,6 +767,26 @@ describe("POST /api/plugins mode:install — negative paths", () => { return app; } + it("returns 400 when the package has no loadable entry file", async () => { + const pkgRoot = "/home/user/plugins/my-plugin"; + mockAccess.mockImplementation((p: string) => { + if (p === pkgRoot || p === `${pkgRoot}/manifest.json`) return Promise.resolve(); + return Promise.reject(new Error("not found")); + }); + mockReadFile.mockResolvedValue(JSON.stringify(VALID_MANIFEST)); + // Manifest resolves, but no bundled.js / dist/index.js / src/index.ts exists. + mockExistsSync.mockReturnValue(false); + + const res = await REQUEST(buildApp(), "POST", "/api/plugins", { + mode: "install", + path: pkgRoot, + }); + + expect(res.status).toBe(400); + expect(res.body.error).toContain("no loadable entry file"); + expect(pluginStore.registerPlugin).not.toHaveBeenCalled(); + }); + it("returns 404 when path does not exist", async () => { mockAccess.mockRejectedValue(new Error("not found")); diff --git a/packages/dashboard/src/plugin-routes.ts b/packages/dashboard/src/plugin-routes.ts index 883498d78a..9720c9bf84 100644 --- a/packages/dashboard/src/plugin-routes.ts +++ b/packages/dashboard/src/plugin-routes.ts @@ -307,6 +307,20 @@ export function createPluginRouter( // Enable in store let plugin = await pluginStore.enablePlugin(id); + // Heal legacy registrations that stored the package directory instead of + // a loadable entry file (Node ESM cannot import directories). Mirrors the + // heal in routes.ts's enable handler and the CLI's startup heal. + try { + if ((await stat(plugin.path)).isDirectory()) { + const entryPath = resolvePluginEntryPath(plugin.path); + if (entryPath) { + plugin = await pluginStore.updatePlugin(id, { path: entryPath }); + } + } + } catch { + // Path missing or unreadable — let loadPlugin surface the real error. + } + // Start the plugin try { await pluginLoader.loadPlugin(id);