From e1e2cef9284dbfedb686a6179be068ab0b1e0d01 Mon Sep 17 00:00:00 2001 From: Lempkey Date: Fri, 12 Jun 2026 05:52:43 +0100 Subject: [PATCH] fix(issues): accept array-form ?status= filter and stop crashing on repeated keys (#4628) (#4890) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - Boards, agents, and the public REST API all read issue lists via `GET /api/companies/:cid/issues`, with `?status=` as the most-common filter > - Express's default `qs` parser binds repeated keys to a `string[]` — the conventional URL form `?status=todo&status=in_progress` is therefore valid input > - The service layer treated `filters.status` as a string and called `.split(",")` unconditionally, returning HTTP 500 with `TypeError: filters.status.split is not a function`. The same buggy pattern lived at a second call site in the same file > - This PR adds a small `parseStatusFilter` helper that normalizes all four shapes the route can receive, routes both service-layer call sites through it, and widens the `IssueFilters.status` type so the contract stops lying about runtime reality > - The benefit is a passive 500 disappears for any client (curl, board, agent code) that builds `?status=` with array-style binding, and the type system now forces every future caller to handle both shapes correctly ## Linked Issues or Issue Description - Refs #4628 - Closes #4084 - Related earlier attempt: #1964 ## What Changed - **`server/src/services/issues.ts`** — Added exported helper `parseStatusFilter(input: string | readonly string[] | undefined): string[]` that normalizes single strings, CSV (`?status=todo,in_progress`), array (`?status=todo&status=in_progress`), and mixed array+CSV; trims and filters empties. Widened `IssueFilters.status` from `string` to `string | readonly string[]`. Replaced inline `.split` call sites in `list()`, blocked-count filtering, `count()`, and `countUnreadTouchedByUser()` with helper-driven branching. - **`server/src/routes/issues.ts`** — Replaced dishonest `req.query.status` casts with `string | string[] | undefined` at both issue-list and blocked-count entry points so the route contract matches Express `qs` runtime behavior. - **`server/src/__tests__/parse-status-filter.test.ts`** (new) — 10 unit cases: undefined, empty string, single, CSV, array, mixed array+CSV, whitespace trim, trailing/extra commas, no-mutation guarantee, hostile non-string entry guard. - **`server/src/__tests__/issues-list-query-parsing.test.ts`** (new) — 5 supertest cases against a minimal Express app whose handler mirrors the route cast/forwarding pattern: single, CSV, repeated-key array, mixed array+CSV, and no `?status` param. - **`server/src/__tests__/issues-service.test.ts`** — Added embedded-Postgres service coverage for array-form status filters through `list`, `count`, and `countUnreadTouchedByUser` on current master. **Why service-layer, not route-layer:** the bug is the service contract. Fixing only at the route would leave other service-layer call sites latent, keep `IssueFilters.status` inaccurate, and let future internal callers reintroduce the same crash. Widening the type is the forcing function that prevents recurrence. **Why `parseStatusFilter` is exported, not file-local:** the helper has direct unit coverage and keeps the normalization logic colocated with its only current call sites. ## Coordination with prior work - Supersedes **#4084** (thanks to @adlai88 for the original report-and-fix). This PR additionally fixes the extra current-master service call sites, widens `IssueFilters.status` so the type contract is honest, replaces the incorrect route casts, and ships direct regression coverage. - **#1964** bundles unrelated route/service changes; this PR keeps scope tight per CONTRIBUTING.md's one-PR-one-change guidance. ## Out-of-scope finding While verifying all query-string status parsing sites, I found a sibling bug in `server/src/services/execution-workspaces.ts:409` reachable from `routes/execution-workspaces.ts:48`, where repeated `?status=` keys can still hit the same `.split(",")` assumption. I left that out of this PR to keep the review surface small. ## Verification ```bash pnpm --filter @paperclipai/server exec vitest run \ src/__tests__/parse-status-filter.test.ts \ src/__tests__/issues-list-query-parsing.test.ts \ src/__tests__/issues-service.test.ts \ --testNamePattern='parseStatusFilter|issue list status query parsing|accepts array-form status filters in list and count|excludes plugin operation issues from unread inbox counts' ``` Result on the rebased head: `3` files passed, `17` tests passed, `72` skipped. GitHub CI on PR `#4890` is green on the rebased head `805731d3270783d0b80b33ee1dccdc6771febef6`, including `verify`, `Typecheck + Release Registry`, `Build`, `e2e`, general tests, serialized suites, Socket checks, Snyk, and `Greptile Review`. Local workspace typecheck commands still encounter unrelated current-master baseline errors under `packages/plugins/sdk` and `server/src/services/company-skills.ts`; no failures were produced from the `issues` files changed in this PR. ## Risks - **Type widening blast radius:** `IssueFilters.status` widens from `string` to `string | readonly string[]`. Any direct caller that still assumes `.split()` on the input now gets a useful typecheck failure instead of a latent runtime crash. - **Behavior change:** `?status=todo,in_progress&status=done` previously returned HTTP 500; it now returns HTTP 200 with the union of matching statuses. Single-string and CSV behavior remain unchanged. - **No migration. No breaking API changes. No new deps. No UI changes.** > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. — Confirmed: this is a bug fix, not a feature. ROADMAP.md grep showed no overlap. ## Model Used - **Claude Opus 4.7** (Anthropic), `claude-opus-4-7`, 1M context, extended thinking. Used for problem scoping, implementation, and test authoring; the final rebasing, PR prep, and verification updates were handled in the maintainer workflow. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge ## Cross-references and status (maintainer) - Closes #4084 --------- Co-authored-by: Devin Foley --- .../issues-list-query-parsing.test.ts | 74 +++++++++++++++++ server/src/__tests__/issues-service.test.ts | 45 ++++++++++ .../src/__tests__/parse-status-filter.test.ts | 83 +++++++++++++++++++ server/src/routes/issues.ts | 4 +- server/src/services/issues.ts | 57 ++++++++----- 5 files changed, 239 insertions(+), 24 deletions(-) create mode 100644 server/src/__tests__/issues-list-query-parsing.test.ts create mode 100644 server/src/__tests__/parse-status-filter.test.ts 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(*)` })