From 2ebc236b089d77bb1ac48f19d06b35a44fa3ff05 Mon Sep 17 00:00:00 2001 From: scokeepa Date: Wed, 5 Aug 2026 10:45:42 +0900 Subject: [PATCH] fix(server): accept parentIssueId alias in GET /issues (#4032) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents coordinate through the server API. They find sub-tasks by filtering the company issues list by parent. > - `GET /api/companies/:companyId/issues` accepts `?parentId=`. Many callers send `?parentIssueId=` instead, which the handler never read. > - The mismatch is silent. The filter is dropped and the full company list comes back, so agents fetch everything and filter client-side. Issue #3846 reports this. > - `parentIssueId` is not an arbitrary spelling. It is the field name the wakeup payloads in this same route file already use, so callers expect it. > - This pull request accepts `parentIssueId` as an alias for `parentId` at the route boundary, on both the issues list and `issues/count`. > - The benefit is that parent filtering works for both spellings, and the list and its count cannot disagree. ## Linked Issues or Issue Description Fixes #3846 Related: #3870 proposes the same alias for the list route. ## What Changed - `server/src/routes/issues.ts`: `listFilters.parentId` in `GET /companies/:companyId/issues` now reads `req.query.parentId ?? req.query.parentIssueId`. - `server/src/routes/issues.ts`: `blockedCountFilters.parentId` in `GET /companies/:companyId/issues/count` reads the same alias, so the list and its count agree. - `server/src/__tests__/issues-parent-id-alias.test.ts`: new regression test for alias resolution, precedence, and absence. ## Verification - Run `pnpm run test:run -- server/src/__tests__/issues-parent-id-alias.test.ts`. - The test covers four query shapes: `?parentId=`, `?parentIssueId=`, both present (short form wins), and neither present (filter unset). - Existing callers are unaffected. The UI client `ui/src/api/issues.ts` only sets `parentId`. Nullish coalescing falls back only when the primary key is absent. - The service layer applies the filter with `if (filters?.parentId)` in `server/src/services/issues.ts`. This pull request does not change it. ## Risks - Low risk. The change only widens accepted query input. Both spellings resolve, and the short form still wins. - `?parentId=` with an empty value stays falsy and unfiltered, exactly as before. - This route has no validation middleware, and these list filters are not in the published OpenAPI surface. No contract needs an update. ## Model Used - Claude Opus 5 (`claude-opus-5`), extended thinking with tool use, run by the maintainer's triage agent. It rebased the original commit onto current `master`, extended the alias to `issues/count`, and wrote the regression test. @scokeepa authored the original one-line route change. ## 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 not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: josangmun Co-authored-by: Andrew Aymeloglu --- .../__tests__/helpers/route-test-harness.ts | 141 ++++++++++++++++++ .../issues-list-query-parsing.test.ts | 103 ++++++------- .../__tests__/issues-parent-id-alias.test.ts | 116 ++++++++++++++ server/src/routes/issues.ts | 4 +- 4 files changed, 303 insertions(+), 61 deletions(-) create mode 100644 server/src/__tests__/helpers/route-test-harness.ts create mode 100644 server/src/__tests__/issues-parent-id-alias.test.ts diff --git a/server/src/__tests__/helpers/route-test-harness.ts b/server/src/__tests__/helpers/route-test-harness.ts new file mode 100644 index 0000000000..6b704caf6a --- /dev/null +++ b/server/src/__tests__/helpers/route-test-harness.ts @@ -0,0 +1,141 @@ +import { randomUUID } from "node:crypto"; +import express, { type Router } from "express"; +import { isNotNull } from "drizzle-orm"; +import { + companies, + companyMemberships, + createDb, + issues, + principalPermissionGrants, +} from "@paperclipai/db"; +import { afterAll, afterEach, beforeAll, describe } from "vitest"; +import { errorHandler } from "../../middleware/index.js"; +import { ensureHumanRoleDefaultGrants } from "../../services/principal-access-compatibility.js"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./embedded-postgres.js"; + +type Db = ReturnType; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); + +/** `describe` on hosts that can run embedded Postgres, `describe.skip` elsewhere. */ +export const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +export type EmbeddedPostgresContext = { readonly db: Db }; + +/** + * Registers the embedded Postgres lifecycle for the enclosing `describe`. Read + * `.db` from inside a test; it is unavailable until `beforeAll` has run. + */ +export function useEmbeddedPostgres( + prefix: string, + opts: { resetEach?: (db: Db) => Promise } = {}, +): EmbeddedPostgresContext { + let db: Db | null = null; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase(prefix); + db = createDb(tempDb.connectionString); + }, 20_000); + + if (opts.resetEach) { + const resetEach = opts.resetEach; + afterEach(async () => { + if (db) await resetEach(db); + }); + } + + afterAll(async () => { + await tempDb?.cleanup(); + tempDb = null; + db = null; + }); + + return { + get db() { + if (!db) throw new Error("embedded Postgres is not started yet — read .db inside a test"); + return db; + }, + }; +} + +export type BoardActor = { + type: "board"; + source: string; + userId: string; + companyIds: string[]; + memberships: { companyId: string; membershipRole: string; status: string }[]; + isInstanceAdmin: boolean; +}; + +export type SeededCompany = { companyId: string; userId: string; actor: BoardActor }; + +/** Creates a company whose board user holds company-scoped read and write grants. */ +export async function seedCompanyWithBoardAccess(db: Db, name: string): Promise { + const companyId = randomUUID(); + const userId = `user-${randomUUID()}`; + await db.insert(companies).values({ + id: companyId, + name: `${name} ${companyId}`, + issuePrefix: `T${companyId.replaceAll("-", "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + await db.insert(companyMemberships).values({ + companyId, + principalType: "user", + principalId: userId, + status: "active", + membershipRole: "owner", + updatedAt: new Date(), + }); + await ensureHumanRoleDefaultGrants(db, { + companyId, + principalId: userId, + membershipRole: "owner", + grantedByUserId: null, + }); + return { + companyId, + userId, + actor: { + type: "board", + source: "session", + userId, + companyIds: [companyId], + memberships: [{ companyId, membershipRole: "owner", status: "active" }], + isInstanceAdmin: false, + }, + }; +} + +type RouterFactory = (db: Db, storage: never) => Router; + +/** Mounts route modules under `/api` behind a fixed actor, with the real error handler. */ +export function routeApp(db: Db, actor: BoardActor, ...routerFactories: RouterFactory[]) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + (req as any).actor = actor; + next(); + }); + for (const factory of routerFactories) { + app.use("/api", factory(db, {} as never)); + } + app.use(errorHandler); + return app; +} + +/** + * Clears the company and issue fixtures seeded by `seedCompanyWithBoardAccess`. + * Suites that seed other tables need their own reset. + */ +export async function resetCompanyIssueFixtures(db: Db) { + await db.delete(issues).where(isNotNull(issues.parentId)); + await db.delete(issues); + await db.delete(principalPermissionGrants); + await db.delete(companyMemberships); + await db.delete(companies); +} diff --git a/server/src/__tests__/issues-list-query-parsing.test.ts b/server/src/__tests__/issues-list-query-parsing.test.ts index f601aa8fc2..91024b93d0 100644 --- a/server/src/__tests__/issues-list-query-parsing.test.ts +++ b/server/src/__tests__/issues-list-query-parsing.test.ts @@ -1,74 +1,59 @@ -import express from "express"; +import { randomUUID } from "node:crypto"; import request from "supertest"; -import { describe, expect, it } from "vitest"; -import { parseStatusFilter } from "../services/issues.ts"; +import { expect, it } from "vitest"; +import { issues } from "@paperclipai/db"; +import { issueRoutes } from "../routes/issues.js"; +import { + describeEmbeddedPostgres, + resetCompanyIssueFixtures, + routeApp, + seedCompanyWithBoardAccess, + useEmbeddedPostgres, +} from "./helpers/route-test-harness.js"; /** - * 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. + * Regression coverage for https://github.com/paperclipai/paperclip/issues/4628. + * Express's `qs` parser hands the list route either a string or an array for + * `?status=`, and the route normalizes both shapes. */ -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"] }); +describeEmbeddedPostgres("issue list status query parsing", () => { + const ctx = useEmbeddedPostgres("paperclip-issues-list-query-parsing-", { + resetEach: resetCompanyIssueFixtures, }); - 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"] }); + async function listStatuses(query: string) { + const company = await seedCompanyWithBoardAccess(ctx.db, "Status parsing"); + const companyId = company.companyId; + await ctx.db.insert(issues).values([ + { id: randomUUID(), companyId, title: "Todo", status: "todo", priority: "medium" }, + { id: randomUUID(), companyId, title: "In progress", status: "in_progress", priority: "medium" }, + { id: randomUUID(), companyId, title: "Done", status: "done", priority: "medium" }, + ]); + const res = await request(routeApp(ctx.db, company.actor, issueRoutes)) + .get(`/api/companies/${companyId}/issues${query}`) + .expect(200); + return (res.body as { status: string }[]).map((issue) => issue.status).sort(); + } + + it("accepts a single ?status=todo", async () => { + expect(await listStatuses("?status=todo")).toEqual(["todo"]); }); - 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 comma-separated ?status=todo,in_progress", async () => { + expect(await listStatuses("?status=todo,in_progress")).toEqual(["in_progress", "todo"]); }); - 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 repeated ?status=todo&status=in_progress", async () => { + expect(await listStatuses("?status=todo&status=in_progress")).toEqual(["in_progress", "todo"]); }); - 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: [] }); + it("accepts mixed array and CSV ?status=todo,in_progress&status=done", async () => { + expect(await listStatuses("?status=todo,in_progress&status=done")) + .toEqual(["done", "in_progress", "todo"]); + }); + + it("returns every status when ?status is absent", async () => { + expect(await listStatuses("")).toEqual(["done", "in_progress", "todo"]); }); }); diff --git a/server/src/__tests__/issues-parent-id-alias.test.ts b/server/src/__tests__/issues-parent-id-alias.test.ts new file mode 100644 index 0000000000..5d1be26202 --- /dev/null +++ b/server/src/__tests__/issues-parent-id-alias.test.ts @@ -0,0 +1,116 @@ +import { randomUUID } from "node:crypto"; +import request from "supertest"; +import { expect, it } from "vitest"; +import { issues } from "@paperclipai/db"; +import { issueRoutes } from "../routes/issues.js"; +import { + describeEmbeddedPostgres, + resetCompanyIssueFixtures, + routeApp, + seedCompanyWithBoardAccess, + useEmbeddedPostgres, +} from "./helpers/route-test-harness.js"; + +describeEmbeddedPostgres("issue list parentIssueId query alias", () => { + const ctx = useEmbeddedPostgres("paperclip-issues-parent-id-alias-", { + resetEach: resetCompanyIssueFixtures, + }); + + async function seed() { + const company = await seedCompanyWithBoardAccess(ctx.db, "Parent alias"); + const companyId = company.companyId; + const parentId = randomUUID(); + const otherParentId = randomUUID(); + const childId = randomUUID(); + const blockedChildId = randomUUID(); + + await ctx.db.insert(issues).values([ + { id: parentId, companyId, title: "Parent", status: "todo", priority: "medium" }, + { id: otherParentId, companyId, title: "Other parent", status: "todo", priority: "medium" }, + ]); + await ctx.db.insert(issues).values([ + { id: childId, companyId, title: "Child", status: "todo", priority: "medium", parentId }, + { id: blockedChildId, companyId, title: "Blocked child", status: "blocked", priority: "medium", parentId }, + { + id: randomUUID(), + companyId, + title: "Other child", + status: "todo", + priority: "medium", + parentId: otherParentId, + }, + { + id: randomUUID(), + companyId, + title: "Blocked other child", + status: "blocked", + priority: "medium", + parentId: otherParentId, + }, + ]); + + return { ...company, parentId, otherParentId, childId, blockedChildId }; + } + + type Seeded = Awaited>; + + function appFor(seeded: Seeded) { + return routeApp(ctx.db, seeded.actor, issueRoutes); + } + + async function listIds(seeded: Seeded, query: Record) { + const res = await request(appFor(seeded)) + .get(`/api/companies/${seeded.companyId}/issues`) + .query(query) + .expect(200); + return (res.body as { id: string }[]).map((issue) => issue.id).sort(); + } + + async function blockedCount(seeded: Seeded, query: Record) { + const res = await request(appFor(seeded)) + .get(`/api/companies/${seeded.companyId}/issues/count`) + .query({ attention: "blocked", ...query }) + .expect(200); + return res.body as { count: number }; + } + + function childrenOfParent(seeded: Seeded) { + return [seeded.childId, seeded.blockedChildId].sort(); + } + + it("filters children by ?parentId=", async () => { + const seeded = await seed(); + expect(await listIds(seeded, { parentId: seeded.parentId })).toEqual(childrenOfParent(seeded)); + }); + + it("filters children by ?parentIssueId=", async () => { + const seeded = await seed(); + expect(await listIds(seeded, { parentIssueId: seeded.parentId })).toEqual(childrenOfParent(seeded)); + }); + + it("prefers ?parentId= when both spellings are present", async () => { + const seeded = await seed(); + const ids = await listIds(seeded, { + parentId: seeded.parentId, + parentIssueId: seeded.otherParentId, + }); + expect(ids).toEqual(childrenOfParent(seeded)); + }); + + it("returns the whole company list when neither spelling is present", async () => { + const seeded = await seed(); + expect(await listIds(seeded, {})).toHaveLength(6); + }); + + it("filters the blocked count by ?parentIssueId=", async () => { + const seeded = await seed(); + expect(await blockedCount(seeded, { parentIssueId: seeded.parentId })).toEqual({ count: 1 }); + }); + + it("returns the same blocked count for both spellings", async () => { + const seeded = await seed(); + const byShortForm = await blockedCount(seeded, { parentId: seeded.parentId }); + const byAlias = await blockedCount(seeded, { parentIssueId: seeded.parentId }); + expect(byAlias).toEqual(byShortForm); + }); +}); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 8477bbdd1a..811efef137 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -5210,7 +5210,7 @@ export function issueRoutes( projectId: req.query.projectId as string | undefined, workspaceId: req.query.workspaceId as string | undefined, executionWorkspaceId: req.query.executionWorkspaceId as string | undefined, - parentId: req.query.parentId as string | undefined, + parentId: (req.query.parentId ?? req.query.parentIssueId) as string | undefined, descendantOf: req.query.descendantOf as string | undefined, labelId: req.query.labelId as string | undefined, originKind: req.query.originKind as string | undefined, @@ -5402,7 +5402,7 @@ export function issueRoutes( projectId: req.query.projectId as string | undefined, workspaceId: req.query.workspaceId as string | undefined, executionWorkspaceId: req.query.executionWorkspaceId as string | undefined, - parentId: req.query.parentId as string | undefined, + parentId: (req.query.parentId ?? req.query.parentIssueId) as string | undefined, descendantOf: req.query.descendantOf as string | undefined, labelId: req.query.labelId as string | undefined, originKind: req.query.originKind as string | undefined,