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) <noreply@anthropic.com>
This commit is contained in:
@@ -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);
|
||||
});
|
||||
}
|
||||
});
|
||||
@@ -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<typeof vi.fn>).mockResolvedValue({
|
||||
...INSTALLED_PLUGIN,
|
||||
path: dirPath,
|
||||
});
|
||||
(pluginStore.updatePlugin as ReturnType<typeof vi.fn>).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<typeof vi.fn>).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"));
|
||||
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user