Code Issues Pull Requests Actions Security InsightsSettings

fix(api): ?state=all returned an empty list instead of every issue #5570

MergedXSccantynz wants to mergefix/api-v2-state-all→mainopened 2d ago

2 changed files+155−4

Addedsrc/__tests__/api-v2-state-filter.test.ts+103−0View fileUnified Split

@@ -0,0 +1,103 @@

1/**

2 * `?state=all` returned an empty list.

3 *

4 * Reported 2026-08-29 from the Vapron instance, measured against a repo with

5 * 13 open issues:

6 *

7 * (default) -> 13 correct

8 * ?state=open -> 13 correct

9 * ?state=all -> 0 WRONG

10 * ?state=closed -> 0 correct, none were closed

11 *

12 * "all" was never special-cased, so it went into `eq(issues.state, "all")` and

13 * matched nothing. The reporting session nearly told its owner that 12 issue

14 * creations had failed — the POSTs had returned real ids, and only a direct

15 * Postgres query proved the rows existed.

16 *

17 * The one-liner is not the interesting part. The failure MODE is: a documented,

18 * valid-looking parameter produced a SUCCESSFUL EMPTY RESULT, indistinguishable

19 * from "this repo has no issues". So these also pin the 400 for unknown values.

20 *

21 * Credential-free by construction: validation runs before repo resolution, and

22 * the clause assertions call the helper directly. No DB, so this stays in the

23 * CI-safe suite rather than the excluded set.

24 */

26import { describe, it, expect } from "bun:test";

27import app from "../app";

28import {

29 stateFilter,

30 ISSUE_STATES,

31 PR_STATES,

32} from "../routes/api-v2";

33import { issues, pullRequests } from "../db/schema";

34import { resolve } from "path";

36/** Read from cwd (the repo root under both `bun test` and the CI runner) —

37 * `new URL(..).pathname` yields "/C:/..." on Windows and fails to open. */

38const API_V2_SRC = resolve("src/routes/api-v2.ts");

40describe("stateFilter — the clause itself", () => {

41 it('"all" produces NO state predicate (the reported bug)', () => {

42 const r = stateFilter("all", issues.state, ISSUE_STATES);

43 expect(r.ok).toBe(true);

44 // undefined clause == "do not filter by state". Anything else here means

45 // "all" is being compared against a column value again.

46 expect(r.ok && r.clause).toBeUndefined();

47 });

49 it("a missing state defaults to open, and DOES filter", () => {

50 const r = stateFilter(undefined, issues.state, ISSUE_STATES);

51 expect(r.ok).toBe(true);

52 expect(r.ok && r.clause).toBeDefined();

53 });

55 it("each schema state is accepted and filters", () => {

56 for (const st of ISSUE_STATES) {

57 const r = stateFilter(st, issues.state, ISSUE_STATES);

58 expect(r.ok).toBe(true);

59 expect(r.ok && r.clause).toBeDefined();

60 }

61 // "merged" is a PR state, not an issue state — the lists are not shared.

62 expect(stateFilter("merged", pullRequests.state, PR_STATES).ok).toBe(true);

63 expect(stateFilter("merged", issues.state, ISSUE_STATES).ok).toBe(false);

64 });

66 it("an unknown value is refused, and the message names what is accepted", () => {

67 const r = stateFilter("banana", issues.state, ISSUE_STATES);

68 expect(r.ok).toBe(false);

69 if (!r.ok) {

70 expect(r.error).toContain("banana");

71 expect(r.error).toContain("open|closed|all");

72 }

73 });

74});

76describe("both list endpoints actually use the helper", () => {

77 /**

78 * The HTTP path is deliberately NOT asserted here. These routes sit behind

79 * middleware that needs a database, so without credentials every request is

80 * a 500 before the handler runs — the existing api-v2 suite tolerates

81 * "404 or 500" for exactly that reason. Asserting a 400 there would mean

82 * excluding this file from CI, and a guard that does not run in CI is not a

83 * guard. So the behaviour is pinned on the helper above, and the wiring is

84 * pinned here.

85 */

86 it("issues and pulls both route their state through stateFilter", async () => {

87 const src = await Bun.file(API_V2_SRC).text();

88 expect(src).toContain(

89 'stateFilter(c.req.query("state"), issues.state, ISSUE_STATES)'

90 );

91 expect(src).toContain(

92 'stateFilter(c.req.query("state"), pullRequests.state, PR_STATES)'

93 );

94 });

96 it("no list endpoint compares a raw query value against a state column", async () => {

97 // The original defect, in the shape it took: a query string dropped

98 // straight into an equality. If this pattern comes back anywhere in the

99 // file, "all" silently returns nothing again.

100 const src = await Bun.file(API_V2_SRC).text();

101 expect(src).not.toMatch(/eq$(?:issues|pullRequests)\.state,\s*state$/);

102 });

103});

Modifiedsrc/routes/api-v2.ts+52−4View fileUnified Split

@@ -11,6 +11,8 @@ import { parseIdNumber } from "../lib/route-params";

1111import { normalizeRepoName, REPO_NAME_PATTERN } from "../lib/repo-name";

1212import { join } from "path";

1313import { eq, and, desc, asc, sql, like, or, gte, lte, gt } from "drizzle-orm";

14import type { SQL } from "drizzle-orm";

15import type { AnyPgColumn } from "drizzle-orm/pg-core";

1416import { deflateRawSync } from "node:zlib";

1517import { db } from "../db";

1618import {

@@ -1144,11 +1146,55 @@ apiv2.post(

11441146

11451147// ─── Issues ─────────────────────────────────────────────────────────────────

11461148

1149/** Issue states the schema actually stores (src/db/schema.ts: "open, closed"). */

1150export const ISSUE_STATES = ["open", "closed"] as const;

1151/** PRs add "merged" (src/db/schema.ts: "open, closed, merged"). */

1152export const PR_STATES = ["open", "closed", "merged"] as const;

1153

1154/**

1155 * Resolve a `?state=` query into a where-clause, or a 400.

1156 *

1157 * REPORTED 2026-08-29 by the Vapron instance: `?state=all` returned `[]` on a

1158 * repo with 13 open issues. "all" was never special-cased, so it went straight

1159 * into `eq(issues.state, "all")` and matched nothing. The reporter nearly told

1160 * their owner that 12 issue creations had failed — the POSTs had returned real

1161 * ids, and only a direct Postgres query showed the rows were there.

1162 *

1163 * The bug is the one-liner. The class is the failure MODE: a documented,

1164 * valid-looking parameter produced a successful empty result, which is

1165 * indistinguishable from "this repo has no issues". So unknown values now get

1166 * a 400 that names what is accepted, rather than a silent lie. `?state=banana`

1167 * used to answer `[]` too.

1168 *

1169 * Shared by both list endpoints because they had already drifted: this exact

1170 * contract was implemented correctly in gluecron_list_prs (src/lib/mcp-tools.ts)

1171 * — enum-checked, "all" special-cased — while the REST surface serving the same

1172 * data did neither. One helper, so the next state value cannot land in one and

1173 * not the other.

1174 */

1175export function stateFilter(

1176 raw: string | undefined,

1177 column: AnyPgColumn,

1178 allowed: readonly string[]

1179): { ok: true; clause: SQL | undefined } | { ok: false; error: string } {

1180 const state = raw || "open";

1181 if (state === "all") return { ok: true, clause: undefined };

1182 if (!allowed.includes(state)) {

1183 return {

1184 ok: false,

1185 error: `state must be one of ${[...allowed, "all"].join("|")} (got "${state}")`,

1186 };

1187 }

1188 return { ok: true, clause: eq(column, state) };

1189}

1190

11471191apiv2.get("/repos/:owner/:repo/issues", async (c) => {

11481192 const { owner, repo } = c.req.param();

1149 const state = c.req.query("state") || "open";

11501193 const limit = Math.min(parseInt(c.req.query("limit") || "30"), 100);

11511194

1195 const filter = stateFilter(c.req.query("state"), issues.state, ISSUE_STATES);

1196 if (!filter.ok) return c.json({ error: filter.error }, 400);

1197

11521198 const resolved = await resolveRepo(owner, repo);

11531199 if (!resolved) return c.json({ error: "Not found" }, 404);

11541200

@@ -1159,7 +1205,7 @@ apiv2.get("/repos/:owner/:repo/issues", async (c) => {

11591205 })

11601206 .from(issues)

11611207 .innerJoin(users, eq(issues.authorId, users.id))

1162 .where(and(eq(issues.repositoryId, (resolved.repo as any).id), eq(issues.state, state)))

1208 .where(and(eq(issues.repositoryId, (resolved.repo as any).id), filter.clause))

11631209 .orderBy(desc(issues.createdAt))

11641210 .limit(limit);

11651211

@@ -1294,13 +1340,15 @@ apiv2.post("/repos/:owner/:repo/issues/:number/comments", requireApiAuth, requir

12941340

12951341apiv2.get("/repos/:owner/:repo/pulls", async (c) => {

12961342 const { owner, repo } = c.req.param();

1297 const state = c.req.query("state") || "open";

12981343 // Match the issue-list pagination contract: default 30, max 100,

12991344 // 0-indexed offset for cursor-style scrolling. Bounded so a buggy

13001345 // client can't accidentally pull the whole table.

13011346 const limit = Math.min(100, Math.max(1, Number(c.req.query("limit")) || 30));

13021347 const offset = Math.max(0, Number(c.req.query("offset")) || 0);

13031348

1349 const filter = stateFilter(c.req.query("state"), pullRequests.state, PR_STATES);

1350 if (!filter.ok) return c.json({ error: filter.error }, 400);

1351

13041352 const resolved = await resolveRepo(owner, repo);

13051353 if (!resolved) return c.json({ error: "Not found" }, 404);

13061354

@@ -1311,7 +1359,7 @@ apiv2.get("/repos/:owner/:repo/pulls", async (c) => {

13111359 })

13121360 .from(pullRequests)

13131361 .innerJoin(users, eq(pullRequests.authorId, users.id))

1314 .where(and(eq(pullRequests.repositoryId, (resolved.repo as any).id), eq(pullRequests.state, state)))

1362 .where(and(eq(pullRequests.repositoryId, (resolved.repo as any).id), filter.clause))

13151363 .orderBy(desc(pullRequests.createdAt))

13161364 .limit(limit)

13171365 .offset(offset);

13181366

c comment · e edit title · m merge · a approve · r request changes · ? shortcuts