fix(api): one definition of `state`, and a gate that fails when two drift #5571
6 changed files+258−57
Modifiedsrc/__tests__/api-v2-state-filter.test.ts+1−1View fileUnifiedSplit
@@ -29,7 +29,7 @@ import {
2929 stateFilter,
3030 ISSUE_STATES,
3131 PR_STATES,
32} from "../routes/api-v2";
32} from "../lib/state-filters";
3333import { issues, pullRequests } from "../db/schema";
3434import { resolve } from "path";
3535
Addedsrc/__tests__/state-filter-parity.test.ts+135−0View fileUnifiedSplit
@@ -0,0 +1,135 @@
1/**
2 * The MCP tool and the REST endpoint must accept the same state vocabulary.
3 *
4 * They did not. `gluecron_list_prs` enum-checked its `state` and special-cased
5 * "all"; `GET /api/v2/repos/:owner/:repo/pulls`, serving the same rows one
6 * directory away, did neither — so `?state=all` returned an empty list while
7 * the MCP tool returned every PR. Two correct-looking implementations of one
8 * contract, drifting, with nothing comparing them.
9 *
10 * Suggested by the session that reported the bug (cross-instance, 2026-08-29),
11 * and it is the right shape: a gate that fails when two things that must agree
12 * don't. The repo already has this pattern for money columns and env-var reads.
13 *
14 * BEHAVIOURAL, deliberately. The cheap version would assert both files import
15 * the same symbol — but that passes the instant someone re-inlines a list,
16 * which is exactly how these two drifted in the first place. So this drives
17 * each surface with real values and compares what they ACCEPT.
18 *
19 * Credential-free: `gluecron_list_prs` validates `state` before its auth
20 * check, so a rejected state is observable without a database or a token.
21 * That keeps this in the CI suite rather than the excluded set — a parity gate
22 * that does not run in CI is not a gate.
23 */
24
25import { describe, it, expect } from "bun:test";
26import { defaultTools } from "../lib/mcp-tools";
27import { stateFilter, PR_STATES, ISSUE_STATES } from "../lib/state-filters";
28import { pullRequests, issues } from "../db/schema";
29import type { McpContext } from "../lib/mcp";
30
31const tools = defaultTools();
32
33/** Values worth probing: every real state, the sentinel, and clear rubbish. */
34const PROBES = [
35 "open",
36 "closed",
37 "merged",
38 "all",
39 "banana",
40 "",
41 "OPEN",
42 "any",
43] as const;
44
45/**
46 * Does the MCP tool refuse this state?
47 *
48 * Anything past the state gate (auth, DB, missing repo) counts as ACCEPTED —
49 * the gate let us through, which is what is being measured.
50 */
51async function mcpRejects(state: string): Promise<boolean> {
52 const handler = tools["gluecron_list_prs"];
53 if (!handler) throw new Error("gluecron_list_prs is not registered");
54 try {
55 await handler.run(
56 { owner: "someone", repo: "something", state },
57 { userId: "user-1", scopes: ["repo"] } as unknown as McpContext
58 );
59 return false;
60 } catch (err) {
61 const msg = err instanceof Error ? err.message : String(err);
62 return /state must be one of/.test(msg);
63 }
64}
65
66/** Does the REST helper refuse this state? */
67function restRejects(state: string): boolean {
68 return !stateFilter(state, pullRequests.state, PR_STATES).ok;
69}
70
71describe("state vocabulary parity — MCP tool vs REST endpoint", () => {
72 it("accepts and refuses exactly the same values", async () => {
73 const disagreements: string[] = [];
74 for (const state of PROBES) {
75 const [mcp, rest] = [await mcpRejects(state), restRejects(state)];
76 if (mcp !== rest) {
77 disagreements.push(
78 `"${state}": MCP ${mcp ? "refuses" : "accepts"}, REST ${rest ? "refuses" : "accepts"}`
79 );
80 }
81 }
82 // Naming the disagreement beats "expected true, got false" — the failure
83 // this guards is precisely "which one is wrong, and about what value".
84 expect(disagreements).toEqual([]);
85 });
86
87 it("both accept every PR state plus the no-filter sentinel", async () => {
88 for (const state of [...PR_STATES, "all"]) {
89 expect(await mcpRejects(state)).toBe(false);
90 expect(restRejects(state)).toBe(false);
91 }
92 });
93
94 it("both refuse a value no row can hold", async () => {
95 expect(await mcpRejects("banana")).toBe(true);
96 expect(restRejects("banana")).toBe(true);
97 });
98
99 it("the refusal message is identical, not merely similar", async () => {
100 // A caller debugging one surface must not get a different story from the
101 // other. Both derive it from stateError(), so this pins that they do.
102 const handler = tools["gluecron_list_prs"]!;
103 let mcpMsg = "";
104 try {
105 await handler.run(
106 { owner: "someone", repo: "something", state: "banana" },
107 { userId: "user-1", scopes: ["repo"] } as unknown as McpContext
108 );
109 } catch (err) {
110 mcpMsg = err instanceof Error ? err.message : String(err);
111 }
112 const rest = stateFilter("banana", pullRequests.state, PR_STATES);
113 expect(rest.ok).toBe(false);
114 if (!rest.ok) expect(mcpMsg).toContain(rest.error);
115 });
116});
117
118describe("the state lists stay honest about the schema", () => {
119 it('"all" is never a member of a state list', () => {
120 // It is the absence of a filter, not a state a row can be in. Adding it to
121 // an enum reintroduces the original bug in a tidier form: it would pass
122 // validation and then be compared against a column no row matches.
123 expect(ISSUE_STATES as readonly string[]).not.toContain("all");
124 expect(PR_STATES as readonly string[]).not.toContain("all");
125 });
126
127 it("issues and PRs do not share a vocabulary by accident", () => {
128 // PRs have "merged"; issues do not. If these ever become the same list,
129 // one of the two endpoints has started accepting a state its table cannot
130 // hold — which returns an empty list, the failure mode this all began with.
131 expect(PR_STATES as readonly string[]).toContain("merged");
132 expect(ISSUE_STATES as readonly string[]).not.toContain("merged");
133 expect(stateFilter("merged", issues.state, ISSUE_STATES).ok).toBe(false);
134 });
135});
Modifiedsrc/lib/mcp-tools.ts+14−12View fileUnifiedSplit
@@ -38,6 +38,7 @@ import { isAiReviewEnabled, triggerAiReview } from "./ai-review";
3838import { triggerPrTriage } from "./pr-triage";
3939import { performGatedMerge } from "./pr-merge-gated";
4040import { expandedTools } from "./mcp-tools-expanded";
41import { stateFilter, PR_STATES } from "./state-filters";
4142
4243/**
4344 * MCP spec ToolAnnotations — behavioural hints surfaced to clients via
@@ -1145,23 +1146,24 @@ const listPrs: McpToolHandler = {
11451146 const owner = argString(args, "owner");
11461147 const repo = argString(args, "repo");
11471148 const state = argString(args, "state", "open");
1148 if (!["open", "closed", "merged", "all"].includes(state)) {
1149 throw new McpError(
1150 ERR_INVALID_PARAMS,
1151 `state must be one of open|closed|merged|all (got "${state}")`
1152 );
1149 // Shared with the REST list endpoints (src/lib/state-filters.ts). This
1150 // tool had the contract right while GET /api/v2/.../pulls did not, and
1151 // nothing compared them — see that file's header.
1152 const prFilter = stateFilter(state, pullRequests.state, PR_STATES);
1153 if (!prFilter.ok) {
1154 throw new McpError(ERR_INVALID_PARAMS, prFilter.error);
11531155 }
11541156
11551157 requireAuthedCtx(ctx, "gluecron_list_prs");
11561158 const info = await resolveAccessibleRepo(owner, repo, ctx.userId);
11571159
1158 const whereClause =
1159 state === "all"
1160 ? eq(pullRequests.repositoryId, info.repoId)
1161 : and(
1162 eq(pullRequests.repositoryId, info.repoId),
1163 eq(pullRequests.state, state)
1164 );
1160 // `and()` drops an undefined operand, which is how "all" becomes
1161 // "no state predicate" rather than a comparison against a value no row
1162 // holds — the original bug, in the surface that never had it.
1163 const whereClause = and(
1164 eq(pullRequests.repositoryId, info.repoId),
1165 prFilter.clause
1166 );
11651167
11661168 const rows = await db
11671169 .select({
Addedsrc/lib/state-filters.ts+87−0View fileUnifiedSplit
@@ -0,0 +1,87 @@
1/**
2 * One definition of what `state` means, for every surface that filters by it.
3 *
4 * WHY THIS FILE EXISTS. On 2026-08-29 the Vapron instance reported that
5 * `GET /api/v2/repos/:owner/:repo/issues?state=all` returned `[]` against a
6 * repo with 13 open issues: "all" was never special-cased, so it went into
7 * `eq(issues.state, "all")` and matched nothing. The reporting session nearly
8 * told its owner that 12 issue creations had failed.
9 *
10 * The bug was a one-liner. The interesting part was that the SAME contract was
11 * already implemented correctly one directory away — `gluecron_list_prs` in
12 * mcp-tools.ts enum-checked the value and special-cased "all" — while the REST
13 * endpoint serving the same rows did neither. Two implementations of one idea,
14 * drifting, with nothing comparing them. That is the shape behind most of the
15 * defects found this week.
16 *
17 * So the fix is not "fix both". It is "make them the same code". Every surface
18 * that accepts a `state` filter derives its accepted set, its error message and
19 * its where-clause from here, and src/__tests__/state-filter-parity.test.ts
20 * exercises the surfaces BEHAVIOURALLY — not by checking they reference this
21 * symbol, which would pass the moment someone re-inlines a list, which is
22 * exactly how they diverged the first time.
23 *
24 * "all" is deliberately NOT a member of the state lists: it is not a state a
25 * row can be in, it is the absence of a filter. Adding it to an enum would
26 * reintroduce the original bug in a tidier form.
27 */
28
29import { eq } from "drizzle-orm";
30import type { SQL } from "drizzle-orm";
31import type { AnyPgColumn } from "drizzle-orm/pg-core";
32
33/** Issue states the schema stores (src/db/schema.ts: "open, closed"). */
34export const ISSUE_STATES = ["open", "closed"] as const;
35
36/** PRs add "merged" (src/db/schema.ts: "open, closed, merged"). */
37export const PR_STATES = ["open", "closed", "merged"] as const;
38
39/** Everything a caller may legally pass, including the no-filter sentinel. */
40export function acceptedStates(allowed: readonly string[]): string[] {
41 return [...allowed, "all"];
42}
43
44/** Is this a value we can answer? */
45export function isAcceptedState(
46 state: string,
47 allowed: readonly string[]
48): boolean {
49 return state === "all" || allowed.includes(state);
50}
51
52/**
53 * The refusal message. Shared so REST and MCP cannot describe the same
54 * contract differently — a caller debugging one should not get a different
55 * story from the other.
56 */
57export function stateError(state: string, allowed: readonly string[]): string {
58 return `state must be one of ${acceptedStates(allowed).join("|")} (got "${state}")`;
59}
60
61export type StateFilterResult =
62 | { ok: true; clause: SQL | undefined }
63 | { ok: false; error: string };
64
65/**
66 * Resolve a caller-supplied `state` into a where-clause, or a refusal.
67 *
68 * A `clause` of `undefined` means "do not filter by state" — `and()` drops
69 * undefined operands, so callers can pass it through unconditionally.
70 *
71 * Unknown values are REFUSED rather than filtered on. The original defect
72 * returned a successful empty result for `?state=banana` too, and an empty
73 * list is indistinguishable from "there is nothing here" — the endpoint was
74 * confidently answering a question it had not understood.
75 */
76export function stateFilter(
77 raw: string | undefined,
78 column: AnyPgColumn,
79 allowed: readonly string[]
80): StateFilterResult {
81 const state = raw || "open";
82 if (state === "all") return { ok: true, clause: undefined };
83 if (!allowed.includes(state)) {
84 return { ok: false, error: stateError(state, allowed) };
85 }
86 return { ok: true, clause: eq(column, state) };
87}
Modifiedsrc/routes/api-v2.ts+5−44View fileUnifiedSplit
@@ -11,8 +11,11 @@ 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";
14import {
15 stateFilter,
16 ISSUE_STATES,
17 PR_STATES,
18} from "../lib/state-filters";
1619import { deflateRawSync } from "node:zlib";
1720import { db } from "../db";
1821import {
@@ -1146,48 +1149,6 @@ apiv2.post(
11461149
11471150// ─── Issues ─────────────────────────────────────────────────────────────────
11481151
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
11911152apiv2.get("/repos/:owner/:repo/issues", async (c) => {
11921153 const { owner, repo } = c.req.param();
11931154 const limit = Math.min(parseInt(c.req.query("limit") || "30"), 100);
Modifiedsrc/routes/github-compat.ts+16−0View fileUnifiedSplit
@@ -633,6 +633,22 @@ ghc.get("/repos/:owner/:repo/issues", async (c) => {
633633 .map((s) => s.trim())
634634 .filter(Boolean);
635635
636 // KNOWN DIVERGENCE from GitHub, deliberately kept (2026-08-29).
637 //
638 // "all" is handled correctly here — this endpoint never had the bug that
639 // GET /api/v2/.../issues did. But a MISTYPED state (?state=oepn) still
640 // yields an empty list, where GitHub answers 422. The v2 API was changed to
641 // 400 on unknown values, because a successful empty result is
642 // indistinguishable from "there is nothing here" — an instance reported
643 // exactly that confusion and nearly concluded 12 issue creations had failed.
644 //
645 // This surface is NOT changed to match, on the reasoning that decided it:
646 // it exists to be mistaken for GitHub by clients we have no inventory of,
647 // and a client that has quietly coped with an empty list for months would
648 // start receiving an error it has no handler for. "More correct in
649 // isolation" and "safe to change under live clients" are different
650 // questions, and only the first is answerable from the code. Revisit with a
651 // client inventory, not with a code review.
636652 const conds = [eq(issues.repositoryId, resolved.repo.id)];
637653 if (state !== "all") conds.push(eq(issues.state, state));
638654
639655
c comment · e edit title · m merge · a approve · r request changes · ? shortcuts