diff --git a/server/src/__tests__/issues-list-query-parsing.test.ts b/server/src/__tests__/issues-list-query-parsing.test.ts new file mode 100644 index 00000000..f601aa8f --- /dev/null +++ b/server/src/__tests__/issues-list-query-parsing.test.ts @@ -0,0 +1,74 @@ +import express from "express"; +import request from "supertest"; +import { describe, expect, it } from "vitest"; +import { parseStatusFilter } from "../services/issues.ts"; + +/** + * Regression test for https://github.com/paperclipai/paperclip/issues/4628 + * + * Stands up a minimal Express app whose handler mirrors the parsing path + * `server/src/routes/issues.ts:957-958` uses to forward `req.query.status` + * into `issueService.list({ status })`. Verifies the four shapes Express's + * default `qs` parser can produce all normalize correctly and none crash: + * + * 1. Single value — `?status=todo` + * 2. Comma-separated — `?status=todo,in_progress` (legacy) + * 3. Repeated key (array) — `?status=todo&status=in_progress` (the bug) + * 4. Mixed array + CSV — `?status=todo,in_progress&status=done` + * + * Pre-fix, case 3 returned HTTP 500 with `TypeError: filters.status.split is + * not a function`. We don't spin embedded-postgres here; the helper itself is + * unit-tested in `parse-status-filter.test.ts`, and the route→helper contract + * is what regresses if anyone reverts the fix. + */ + +function buildApp() { + const app = express(); + app.use(express.json()); + app.get("/api/companies/:companyId/issues", (req, res) => { + // Mirror the cast at routes/issues.ts:958 exactly. Pre-fix this said + // `as string | undefined` and the cast was a lie when qs returned an array. + const statusInput = req.query.status as string | string[] | undefined; + const statuses = parseStatusFilter(statusInput); + res.status(200).json({ statuses }); + }); + return app; +} + +describe("issue list status query parsing", () => { + it("accepts a single ?status=todo and returns one normalized status", async () => { + const res = await request(buildApp()).get("/api/companies/c1/issues?status=todo"); + expect(res.status).toBe(200); + expect(res.body).toEqual({ statuses: ["todo"] }); + }); + + it("accepts comma-separated ?status=todo,in_progress (preserves legacy CSV)", async () => { + const res = await request(buildApp()).get( + "/api/companies/c1/issues?status=todo,in_progress", + ); + expect(res.status).toBe(200); + expect(res.body).toEqual({ statuses: ["todo", "in_progress"] }); + }); + + it("accepts repeated ?status=todo&status=in_progress without crashing (the bug fix)", async () => { + const res = await request(buildApp()).get( + "/api/companies/c1/issues?status=todo&status=in_progress", + ); + expect(res.status).toBe(200); + expect(res.body).toEqual({ statuses: ["todo", "in_progress"] }); + }); + + it("accepts mixed array + CSV ?status=todo,in_progress&status=done", async () => { + const res = await request(buildApp()).get( + "/api/companies/c1/issues?status=todo,in_progress&status=done", + ); + expect(res.status).toBe(200); + expect(res.body).toEqual({ statuses: ["todo", "in_progress", "done"] }); + }); + + it("accepts no ?status param and returns an empty status filter", async () => { + const res = await request(buildApp()).get("/api/companies/c1/issues"); + expect(res.status).toBe(200); + expect(res.body).toEqual({ statuses: [] }); + }); +}); diff --git a/server/src/__tests__/issues-service.test.ts b/server/src/__tests__/issues-service.test.ts index 9b0d7a83..ba6efe59 100644 --- a/server/src/__tests__/issues-service.test.ts +++ b/server/src/__tests__/issues-service.test.ts @@ -1227,6 +1227,51 @@ describeEmbeddedPostgres("issueService.list participantAgentId", () => { ]); await expect(svc.countUnreadTouchedByUser(companyId, userId, "todo")).resolves.toBe(1); + await expect(svc.countUnreadTouchedByUser(companyId, userId, ["todo", "in_progress"])).resolves.toBe(1); + }); + + it("accepts array-form status filters in list and count", async () => { + const companyId = randomUUID(); + const todoIssueId = randomUUID(); + const inProgressIssueId = randomUUID(); + const doneIssueId = randomUUID(); + + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + await db.insert(issues).values([ + { + id: todoIssueId, + companyId, + title: "Todo issue", + status: "todo", + priority: "medium", + }, + { + id: inProgressIssueId, + companyId, + title: "In-progress issue", + status: "in_progress", + priority: "medium", + }, + { + id: doneIssueId, + companyId, + title: "Done issue", + status: "done", + priority: "medium", + }, + ]); + + const resultIds = (await svc.list(companyId, { status: ["todo", "in_progress"] })) + .map((issue) => issue.id); + + expect(resultIds).toEqual(expect.arrayContaining([todoIssueId, inProgressIssueId])); + expect(resultIds).not.toContain(doneIssueId); + await expect(svc.count(companyId, { status: ["todo", "in_progress"] })).resolves.toBe(2); }); it("hides archived inbox issues until new external activity arrives", async () => { diff --git a/server/src/__tests__/parse-status-filter.test.ts b/server/src/__tests__/parse-status-filter.test.ts new file mode 100644 index 00000000..5eccc9a9 --- /dev/null +++ b/server/src/__tests__/parse-status-filter.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it } from "vitest"; +import { parseStatusFilter } from "../services/issues.ts"; + +/** + * Unit tests for the helper introduced in https://github.com/paperclipai/paperclip/issues/4628 + * + * Express's default `qs` parser binds repeated query keys (`?status=todo&status=in_progress`) + * to a `string[]`, but `services/issues.ts` previously called `.split(",")` + * unconditionally and crashed with a 500. `parseStatusFilter` normalizes all + * shapes the route can legitimately receive: + * - `undefined` + * - single string (`?status=todo`) + * - comma-separated string (`?status=todo,in_progress`) + * - string array (`?status=todo&status=in_progress`) + * - mixed array+CSV (`?status=todo,in_progress&status=done`) + */ + +describe("parseStatusFilter", () => { + it("returns [] for undefined", () => { + expect(parseStatusFilter(undefined)).toEqual([]); + }); + + it("returns [] for empty string", () => { + expect(parseStatusFilter("")).toEqual([]); + }); + + it("returns single-element array for a single status", () => { + expect(parseStatusFilter("todo")).toEqual(["todo"]); + }); + + it("splits comma-separated values (preserves prior CSV behavior)", () => { + expect(parseStatusFilter("todo,in_progress,done")).toEqual([ + "todo", + "in_progress", + "done", + ]); + }); + + it("accepts string[] from repeated query keys (the bug fix)", () => { + expect(parseStatusFilter(["todo", "in_progress"])).toEqual([ + "todo", + "in_progress", + ]); + }); + + it("flattens mixed array + CSV (?status=todo,in_progress&status=done)", () => { + expect(parseStatusFilter(["todo,in_progress", "done"])).toEqual([ + "todo", + "in_progress", + "done", + ]); + }); + + it("trims surrounding whitespace", () => { + expect(parseStatusFilter(" todo ")).toEqual(["todo"]); + expect(parseStatusFilter(" todo , in_progress ")).toEqual(["todo", "in_progress"]); + }); + + it("filters out empty entries from trailing/extra commas", () => { + expect(parseStatusFilter("todo,")).toEqual(["todo"]); + expect(parseStatusFilter(",")).toEqual([]); + expect(parseStatusFilter(",,todo,,")).toEqual(["todo"]); + }); + + it("does not mutate caller-supplied arrays", () => { + const input: readonly string[] = ["todo", "in_progress"]; + const out = parseStatusFilter(input); + expect(out).not.toBe(input); + expect(input).toEqual(["todo", "in_progress"]); + }); + + it("handles hostile non-string entries inside an array via type-defensive guard", () => { + // The TypeScript signature forbids this, but Express casts hide a lot at + // runtime — confirm we don't throw if a stray non-string slips in. + const out = parseStatusFilter([ + "todo", + // @ts-expect-error intentional: simulate a route-layer cast slipping a non-string through + 42, + "done", + ]); + expect(out).toEqual(["todo", "done"]); + }); +}); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 81c91469..9336508a 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -2457,7 +2457,7 @@ export function issueRoutes( const rawResult = await svc.list(companyId, { attention: attention === "blocked" ? "blocked" : undefined, - status: req.query.status as string | undefined, + status: req.query.status as string | string[] | undefined, assigneeAgentId: req.query.assigneeAgentId as string | undefined, participantAgentId: req.query.participantAgentId as string | undefined, assigneeUserId, @@ -2537,7 +2537,7 @@ export function issueRoutes( const blockedCountFilters = { attention: "blocked", - status: req.query.status as string | undefined, + status: req.query.status as string | string[] | undefined, assigneeAgentId: req.query.assigneeAgentId as string | undefined, participantAgentId: req.query.participantAgentId as string | undefined, assigneeUserId: req.query.assigneeUserId as string | undefined, diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index d41f27ce..f77744d2 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -222,9 +222,22 @@ export function deriveIssueCommentRunLogAttribution( return derivedByCommentId; } +// Express's default `qs` parser binds repeated query keys to a `string[]`, +// so a request like `?status=todo&status=in_progress` arrives here as an +// array. Single-key + comma-separated forms remain valid too; normalize the +// supported shapes once so the service contract matches runtime reality. +export function parseStatusFilter(input: string | readonly string[] | undefined): string[] { + if (input === undefined || input === null) return []; + const entries = Array.isArray(input) ? input : typeof input === "string" ? [input] : []; + return entries + .flatMap((entry) => (typeof entry === "string" ? entry.split(",") : [])) + .map((status) => status.trim()) + .filter(Boolean); +} + export interface IssueFilters { attention?: "blocked"; - status?: string; + status?: string | readonly string[]; assigneeAgentId?: string; participantAgentId?: string; assigneeUserId?: string; @@ -2877,11 +2890,9 @@ async function blockedInboxIssueConditions( } const lowTrustCondition = lowTrustBoundaryIssueCondition(companyId, filters?.lowTrustBoundary); if (lowTrustCondition) conditions.push(lowTrustCondition); - if (filters?.status) { - const statuses = filters.status.split(",").map((status) => status.trim()).filter(Boolean); - if (statuses.length > 0) { - conditions.push(statuses.length === 1 ? eq(issues.status, statuses[0]!) : inArray(issues.status, statuses)); - } + const statuses = parseStatusFilter(filters?.status); + if (statuses.length > 0) { + conditions.push(statuses.length === 1 ? eq(issues.status, statuses[0]!) : inArray(issues.status, statuses)); } if (filters?.assigneeAgentId) conditions.push(eq(issues.assigneeAgentId, filters.assigneeAgentId)); if (filters?.participantAgentId) conditions.push(participatedByAgentCondition(companyId, filters.participantAgentId)); @@ -3890,9 +3901,11 @@ export function issueService(db: Db) { } const lowTrustCondition = lowTrustBoundaryIssueCondition(companyId, filters?.lowTrustBoundary); if (lowTrustCondition) conditions.push(lowTrustCondition); - if (filters?.status) { - const statuses = filters.status.split(",").map((s) => s.trim()); - conditions.push(statuses.length === 1 ? eq(issues.status, statuses[0]) : inArray(issues.status, statuses)); + const statuses = parseStatusFilter(filters?.status); + if (statuses.length === 1) { + conditions.push(eq(issues.status, statuses[0])); + } else if (statuses.length > 1) { + conditions.push(inArray(issues.status, statuses)); } if (filters?.assigneeAgentId) { conditions.push(eq(issues.assigneeAgentId, filters.assigneeAgentId)); @@ -4073,11 +4086,9 @@ export function issueService(db: Db) { } const conditions = [eq(issues.companyId, companyId), isNull(issues.hiddenAt)]; - if (filters?.status) { - const statuses = filters.status.split(",").map((status) => status.trim()).filter(Boolean); - if (statuses.length === 1) conditions.push(eq(issues.status, statuses[0]!)); - else if (statuses.length > 1) conditions.push(inArray(issues.status, statuses)); - } + const statuses = parseStatusFilter(filters?.status); + if (statuses.length === 1) conditions.push(eq(issues.status, statuses[0]!)); + else if (statuses.length > 1) conditions.push(inArray(issues.status, statuses)); if (filters?.assigneeAgentId) conditions.push(eq(issues.assigneeAgentId, filters.assigneeAgentId)); if (filters?.assigneeUserId) conditions.push(eq(issues.assigneeUserId, filters.assigneeUserId)); if (filters?.projectId) conditions.push(eq(issues.projectId, filters.projectId)); @@ -4103,20 +4114,22 @@ export function issueService(db: Db) { return Number(row?.count ?? 0); }, - countUnreadTouchedByUser: async (companyId: string, userId: string, status?: string) => { + countUnreadTouchedByUser: async ( + companyId: string, + userId: string, + status?: string | readonly string[], + ) => { const conditions = [ eq(issues.companyId, companyId), isNull(issues.hiddenAt), nonPluginOperationIssueCondition(), unreadForUserCondition(companyId, userId), ]; - if (status) { - const statuses = status.split(",").map((s) => s.trim()).filter(Boolean); - if (statuses.length === 1) { - conditions.push(eq(issues.status, statuses[0])); - } else if (statuses.length > 1) { - conditions.push(inArray(issues.status, statuses)); - } + const statuses = parseStatusFilter(status); + if (statuses.length === 1) { + conditions.push(eq(issues.status, statuses[0])); + } else if (statuses.length > 1) { + conditions.push(inArray(issues.status, statuses)); } const [row] = await db .select({ count: sql`count(*)` })