fix(issues): accept array-form ?status= filter and stop crashing on repeated keys (#4628) (#4890)

## 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 <devin@paperclip.ing>
This commit is contained in:
Lempkey
2026-06-12 05:52:43 +01:00
committed by GitHub
parent 70357b961f
commit e1e2cef928
5 changed files with 239 additions and 24 deletions
@@ -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: [] });
});
});
@@ -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 () => {
@@ -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"]);
});
});
+2 -2
View File
@@ -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,
+35 -22
View File
@@ -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<number>`count(*)` })