fix(FN-1909): reorder insights routes to prevent /runs being shadowed by /:id

- Reorder route definitions in insights-routes.ts to place /runs before /:id - Fix bug where /runs endpoint was shadowed by the catch-all /:id parameterized route - Add comprehensive regression tests for insights route ordering covering /runs, /stats, /agents, and dynamic ID matching - Document route ordering pitfall in memory.md for future reference
2026-04-16 08:36:11 -07:00
parent b1c9f29880
commit ad09d3b582
3 changed files with 264 additions and 54 deletions
--- a/.fusion/memory.md
+++ b/.fusion/memory.md
@@ -269,7 +269,11 @@ Dashboard SSE (`/api/events`) streams plugin lifecycle events as normalized `plu
 ## Pitfalls

 - When adding props to a React component interface that were previously declared but not destructured in the function body, remember to add them to the destructuring list too. TypeScript won't warn about unused interface fields, so `onOpenScripts` in `MobileNavBarProps` compiled fine but caused `ReferenceError: onOpenScripts is not defined` at runtime.
- **Express wildcard route ordering (FN-1492)**: When defining Express routes with wildcard patterns like `{*filepath}`, ALWAYS define more specific routes BEFORE the generic wildcard route. Express matches routes in order, so `POST /files/{*filepath}` would shadow `POST /files/{*filepath}/delete` if defined first. The fix is to define operation routes (`/copy`, `/move`, `/delete`, `/rename`, `/download`, etc.) BEFORE the generic write route. See `packages/dashboard/src/routes.ts` for the correct ordering pattern.
+- **Express wildcard route ordering (FN-1492/FN-1909)**: When defining Express routes, ALWAYS define more specific routes BEFORE generic parameterized or wildcard routes. Express matches routes in order, so:
+  - `{*filepath}` patterns: `POST /files/{*filepath}` shadows `POST /files/{*filepath}/delete` if defined first
+  - `/:id` patterns (FN-1909): `GET /:id` shadows `GET /runs` and `GET /runs/:id` if defined before them
+  - The fix is to define operation routes (`/runs`, `/run`, `/runs/:id`, `/copy`, `/move`, `/delete`, etc.) BEFORE the generic write/catch-all route
+  - See `packages/dashboard/src/routes.ts` and `packages/dashboard/src/insights-routes.ts` for the correct ordering patterns
 - **Webhook HMAC testing**: The `REQUEST` test utility in `test-request.ts` doesn't handle stream-based middleware like `express.raw()` well. For webhook routes requiring HMAC verification (e.g., GitHub webhooks, routine webhooks), test the `verifyWebhookSignature` function directly using `await import()` rather than trying to set up raw body middleware through Express. See the routine webhook tests in `routes.test.ts` for the pattern.

 - `vi.fn<Parameters<SomeType>, ReturnType<SomeType>>()` works in Vitest runtime but causes TypeScript build errors (`TS2558: Expected 0-1 type arguments, but got 2`). Always use the cast pattern instead.
--- a/packages/dashboard/src/tests/insights-routes.test.ts
+++ b/packages/dashboard/src/tests/insights-routes.test.ts
@@ -0,0 +1,206 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { EventEmitter } from "node:events";
+import { request } from "../test-request.js";
+import { createServer } from "../server.js";
+
+const mockListRuns = vi.fn().mockReturnValue([]);
+const mockGetRun = vi.fn();
+const mockCreateRun = vi.fn();
+const mockListInsights = vi.fn().mockReturnValue([]);
+const mockCountInsights = vi.fn().mockReturnValue(0);
+const mockGetInsight = vi.fn();
+const mockUpdateInsight = vi.fn();
+const mockDeleteInsight = vi.fn();
+
+const mockInsightStore = {
+  listRuns: mockListRuns,
+  getRun: mockGetRun,
+  createRun: mockCreateRun,
+  listInsights: mockListInsights,
+  countInsights: mockCountInsights,
+  getInsight: mockGetInsight,
+  updateInsight: mockUpdateInsight,
+  deleteInsight: mockDeleteInsight,
+};
+
+class MockStore extends EventEmitter {
+  getRootDir(): string {
+    return "/tmp/fn-1909";
+  }
+
+  getFusionDir(): string {
+    return "/tmp/fn-1909/.fusion";
+  }
+
+  getDatabase() {
+    return {
+      exec: vi.fn(),
+      prepare: vi.fn().mockReturnValue({ run: vi.fn().mockReturnValue({ changes: 0 }), get: vi.fn(), all: vi.fn().mockReturnValue([]) }),
+    };
+  }
+
+  getInsightStore() {
+    return mockInsightStore;
+  }
+}
+
+describe("Insights routes", () => {
+  const app = createServer(new MockStore() as any);
+
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockListRuns.mockReturnValue([]);
+    mockGetRun.mockReturnValue(null);
+    mockCreateRun.mockReturnValue({ id: "IR-test-1", trigger: "manual", status: "running", projectId: "proj", createdAt: "2026-04-16T00:00:00.000Z" });
+    mockListInsights.mockReturnValue([]);
+    mockCountInsights.mockReturnValue(0);
+    mockGetInsight.mockReturnValue(null);
+    mockUpdateInsight.mockReturnValue(null);
+    mockDeleteInsight.mockReturnValue(false);
+  });
+
+  // ── Route ordering regression: static routes must not be shadowed by /:id ──
+
+  it("GET /api/insights/runs returns 200 with runs list (not 404 shadowed by /:id)", async () => {
+    mockListRuns.mockReturnValue([
+      { id: "IR-run-1", trigger: "manual", status: "completed", projectId: "proj", createdAt: "2026-04-16T00:00:00.000Z", completedAt: "2026-04-16T00:01:00.000Z" },
+      { id: "IR-run-2", trigger: "schedule", status: "running", projectId: "proj", createdAt: "2026-04-16T00:02:00.000Z" },
+    ]);
+
+    const res = await request(app, "GET", "/api/insights/runs");
+
+    expect(res.status).toBe(200);
+    expect(res.body).toEqual({
+      runs: [
+        { id: "IR-run-1", trigger: "manual", status: "completed", projectId: "proj", createdAt: "2026-04-16T00:00:00.000Z", completedAt: "2026-04-16T00:01:00.000Z" },
+        { id: "IR-run-2", trigger: "schedule", status: "running", projectId: "proj", createdAt: "2026-04-16T00:02:00.000Z" },
+      ],
+    });
+  });
+
+  it("POST /api/insights/run creates a run successfully (not shadowed by /:id)", async () => {
+    mockCreateRun.mockReturnValue({ id: "IR-run-new", trigger: "manual", status: "running", projectId: "proj", createdAt: "2026-04-16T00:00:00.000Z" });
+
+    const res = await request(
+      app,
+      "POST",
+      "/api/insights/run",
+      JSON.stringify({ trigger: "manual" }),
+      { "Content-Type": "application/json" },
+    );
+
+    expect(res.status).toBe(201);
+    expect(res.body).toEqual({ id: "IR-run-new", trigger: "manual", status: "running", projectId: "proj", createdAt: "2026-04-16T00:00:00.000Z" });
+  });
+
+  it("GET /api/insights/runs/:id returns run by id", async () => {
+    mockGetRun.mockReturnValue({ id: "IR-run-1", trigger: "manual", status: "completed", projectId: "proj", createdAt: "2026-04-16T00:00:00.000Z", completedAt: "2026-04-16T00:01:00.000Z" });
+
+    const res = await request(app, "GET", "/api/insights/runs/IR-run-1");
+
+    expect(res.status).toBe(200);
+    expect(res.body).toEqual({ id: "IR-run-1", trigger: "manual", status: "completed", projectId: "proj", createdAt: "2026-04-16T00:00:00.000Z", completedAt: "2026-04-16T00:01:00.000Z" });
+  });
+
+  it("GET /api/insights/runs/:id returns 404 when run not found", async () => {
+    mockGetRun.mockReturnValue(null);
+
+    const res = await request(app, "GET", "/api/insights/runs/IR-notfound");
+
+    expect(res.status).toBe(404);
+    expect((res.body as { error?: string }).error).toMatch(/Run not found: IR-notfound/);
+  });
+
+  // ── Insight CRUD routes ──────────────────────────────────────────────────
+
+  it("GET /api/insights returns list of insights", async () => {
+    mockListInsights.mockReturnValue([
+      { id: "INS-1", title: "High priority", category: "quality", status: "generated", projectId: "proj" },
+    ]);
+    mockCountInsights.mockReturnValue(1);
+
+    const res = await request(app, "GET", "/api/insights");
+
+    expect(res.status).toBe(200);
+    expect(res.body).toEqual({
+      insights: [{ id: "INS-1", title: "High priority", category: "quality", status: "generated", projectId: "proj" }],
+      count: 1,
+    });
+  });
+
+  it("GET /api/insights/:id returns insight by id", async () => {
+    mockGetInsight.mockReturnValue({ id: "INS-1", title: "High priority", category: "quality", status: "generated", projectId: "proj" });
+
+    const res = await request(app, "GET", "/api/insights/INS-1");
+
+    expect(res.status).toBe(200);
+    expect(res.body).toEqual({ id: "INS-1", title: "High priority", category: "quality", status: "generated", projectId: "proj" });
+  });
+
+  it("GET /api/insights/:id returns 404 when insight not found", async () => {
+    mockGetInsight.mockReturnValue(null);
+
+    const res = await request(app, "GET", "/api/insights/INS-notfound");
+
+    expect(res.status).toBe(404);
+    expect((res.body as { error?: string }).error).toMatch(/Insight not found: INS-notfound/);
+  });
+
+  it("PATCH /api/insights/:id updates insight status", async () => {
+    mockUpdateInsight.mockReturnValue({ id: "INS-1", title: "High priority", category: "quality", status: "confirmed", projectId: "proj" });
+
+    const res = await request(
+      app,
+      "PATCH",
+      "/api/insights/INS-1",
+      JSON.stringify({ status: "confirmed" }),
+      { "Content-Type": "application/json" },
+    );
+
+    expect(res.status).toBe(200);
+    expect(mockUpdateInsight).toHaveBeenCalledWith("INS-1", { status: "confirmed" });
+  });
+
+  it("DELETE /api/insights/:id deletes insight", async () => {
+    mockDeleteInsight.mockReturnValue(true);
+
+    const res = await request(app, "DELETE", "/api/insights/INS-1");
+
+    expect(res.status).toBe(204);
+  });
+
+  it("POST /api/insights/:id/dismiss sets insight status to dismissed", async () => {
+    mockUpdateInsight.mockReturnValue({ id: "INS-1", title: "High priority", status: "dismissed", projectId: "proj" });
+
+    const res = await request(
+      app,
+      "POST",
+      "/api/insights/INS-1/dismiss",
+      JSON.stringify({}),
+      { "Content-Type": "application/json" },
+    );
+
+    expect(res.status).toBe(200);
+    expect(mockUpdateInsight).toHaveBeenCalledWith("INS-1", { status: "dismissed" });
+  });
+
+  it("POST /api/insights/:id/create-task returns insight data for task creation", async () => {
+    mockGetInsight.mockReturnValue({ id: "INS-1", title: "Refactor X", content: "Consider refactoring module X", projectId: "proj" });
+
+    const res = await request(
+      app,
+      "POST",
+      "/api/insights/INS-1/create-task",
+      JSON.stringify({}),
+      { "Content-Type": "application/json" },
+    );
+
+    expect(res.status).toBe(200);
+    expect(res.body).toEqual({
+      success: true,
+      insight: { id: "INS-1", title: "Refactor X", content: "Consider refactoring module X", projectId: "proj" },
+      suggestedTitle: "Refactor X",
+      suggestedDescription: "Consider refactoring module X",
+    });
+  });
+});
--- a/packages/dashboard/src/insights-routes.ts
+++ b/packages/dashboard/src/insights-routes.ts
@@ -165,6 +165,58 @@ export function createInsightsRouter(store: TaskStore): Router {
    }
  });

+  // ── Trigger Insight Run ───────────────────────────────────────────────
+
+  router.post("/run", (req: Request, res: Response) => {
+    try {
+      const projectId = getProjectId(req) ?? "";
+      const store = getInsightStore();
+      const trigger: InsightRunTrigger = (req.body.trigger as InsightRunTrigger) ?? "manual";
+
+      if (!VALID_TRIGGERS.includes(trigger)) {
+        throw badRequest(`Invalid trigger: ${trigger}`);
+      }
+
+      const input: InsightRunCreateInput = {
+        trigger,
+        inputMetadata: req.body.inputMetadata,
+      };
+
+      const run = store.createRun(projectId, input);
+      res.status(201).json(run);
+    } catch (error) {
+      rethrowAsApiError(error, "Failed to create insight run");
+    }
+  });
+
+  // ── List Runs ──────────────────────────────────────────────────────────
+
+  router.get("/runs", (req: Request, res: Response) => {
+    try {
+      const store = getInsightStore();
+      const runs = store.listRuns({});
+      res.json({ runs });
+    } catch (error) {
+      rethrowAsApiError(error, "Failed to list runs");
+    }
+  });
+
+  // ── Get Run ────────────────────────────────────────────────────────────
+
+  router.get("/runs/:id", (req: Request, res: Response) => {
+    try {
+      const id = String(req.params.id);
+      const store = getInsightStore();
+      const run = store.getRun(id);
+      if (!run) {
+        throw notFound(`Run not found: ${id}`);
+      }
+      res.json(run);
+    } catch (error) {
+      rethrowAsApiError(error, "Failed to get run");
+    }
+  });
+
  // ── Get Insight ────────────────────────────────────────────────────────

  router.get("/:id", (req: Request, res: Response) => {
@@ -181,7 +233,7 @@ export function createInsightsRouter(store: TaskStore): Router {
    }
  });

-  // ── Update Insight ──────────────────────────────────────────────────────
+  // ── Update Insight ─────────────────────────────────────────────────────

  router.patch("/:id", (req: Request, res: Response) => {
    try {
@@ -250,58 +302,6 @@ export function createInsightsRouter(store: TaskStore): Router {
    }
  });

-  // ── Trigger Insight Run ────────────────────────────────────────────────
-
-  router.post("/run", (req: Request, res: Response) => {
-    try {
-      const projectId = getProjectId(req) ?? "";
-      const store = getInsightStore();
-      const trigger: InsightRunTrigger = (req.body.trigger as InsightRunTrigger) ?? "manual";
-
-      if (!VALID_TRIGGERS.includes(trigger)) {
-        throw badRequest(`Invalid trigger: ${trigger}`);
-      }
-
-      const input: InsightRunCreateInput = {
-        trigger,
-        inputMetadata: req.body.inputMetadata,
-      };
-
-      const run = store.createRun(projectId, input);
-      res.status(201).json(run);
-    } catch (error) {
-      rethrowAsApiError(error, "Failed to create insight run");
-    }
-  });
-
-  // ── List Runs ──────────────────────────────────────────────────────────
-
-  router.get("/runs", (req: Request, res: Response) => {
-    try {
-      const store = getInsightStore();
-      const runs = store.listRuns({});
-      res.json({ runs });
-    } catch (error) {
-      rethrowAsApiError(error, "Failed to list runs");
-    }
-  });
-
-  // ── Get Run ────────────────────────────────────────────────────────────
-
-  router.get("/runs/:id", (req: Request, res: Response) => {
-    try {
-      const id = String(req.params.id);
-      const store = getInsightStore();
-      const run = store.getRun(id);
-      if (!run) {
-        throw notFound(`Run not found: ${id}`);
-      }
-      res.json(run);
-    } catch (error) {
-      rethrowAsApiError(error, "Failed to get run");
-    }
-  });
-
  // ── Create Task from Insight ────────────────────────────────────────────

  router.post("/:id/create-task", (req: Request, res: Response) => {