swadhin / opencodehub public

Tighten PR state transitions and document approval endpoints

swadhinbiswas committed 8 months ago
a14e474
1 parent 34e0385
commit a14e474
3 files changed
+324 -1
Showing 3 changed files+324-1
src/lib/openapi.tssrc/pages/api/repos/[owner]/[repo]/pulls/[number]/index.tstests/integration/pr-update-state-route.test.ts
▼src/lib/openapi.ts
+116-0
@@ -122,6 +122,30 @@ export const openApiSpec = {
122
122
},
123
123
required: ["reviewId"],
124
124
},
125
+ SuggestionApplyRequest: {
126
+ type: "object",
127
+ properties: {
128
+ commentIds: {
129
+ type: "array",
130
+ minItems: 1,
131
+ maxItems: 50,
132
+ items: { type: "string" },
133
+ },
134
+ },
135
+ required: ["commentIds"],
136
+ },
137
+ StackApprovalRequest: {
138
+ type: "object",
139
+ properties: {
140
+ reviewers: {
141
+ type: "array",
142
+ minItems: 1,
143
+ maxItems: 50,
144
+ items: { type: "string", description: "Reviewer username" },
145
+ },
146
+ },
147
+ required: ["reviewers"],
148
+ },
125
149
},
126
150
},
127
151
paths: {
@@ -324,6 +348,27 @@ export const openApiSpec = {
324
348
{ name: "owner", in: "path", required: true, schema: { type: "string" } },
325
349
{ name: "repo", in: "path", required: true, schema: { type: "string" } },
326
350
{ name: "number", in: "path", required: true, schema: { type: "integer" } },
351
+ {
352
+ name: "x-opencodehub-timestamp",
353
+ in: "header",
354
+ required: false,
355
+ schema: { type: "string" },
356
+ description: "Unix timestamp (seconds or milliseconds) for signed callbacks.",
357
+ },
358
+ {
359
+ name: "x-opencodehub-signature",
360
+ in: "header",
361
+ required: false,
362
+ schema: { type: "string" },
363
+ description: "HMAC signature in format sha256=<hex> over '<timestamp>.<rawBody>'.",
364
+ },
365
+ {
366
+ name: "x-opencodehub-event-id",
367
+ in: "header",
368
+ required: false,
369
+ schema: { type: "string" },
370
+ description: "Unique event id used for replay protection.",
371
+ },
327
372
],
328
373
requestBody: {
329
374
required: true,
@@ -360,6 +405,77 @@ export const openApiSpec = {
360
405
},
361
406
},
362
407
},
408
+ "/repos/{owner}/{repo}/pulls/{number}/suggestions/apply": {
409
+ post: {
410
+ tags: ["Pull Requests"],
411
+ summary: "Apply one or more review suggestions",
412
+ description: "Applies suggestion comments to the PR branch after repository and path-scoped permission checks.",
413
+ security: [{ bearerAuth: [] }],
414
+ parameters: [
415
+ { name: "owner", in: "path", required: true, schema: { type: "string" } },
416
+ { name: "repo", in: "path", required: true, schema: { type: "string" } },
417
+ { name: "number", in: "path", required: true, schema: { type: "integer" } },
418
+ ],
419
+ requestBody: {
420
+ required: true,
421
+ content: {
422
+ "application/json": {
423
+ schema: { $ref: "#/components/schemas/SuggestionApplyRequest" },
424
+ },
425
+ },
426
+ },
427
+ responses: {
428
+ 200: { description: "Suggestion(s) applied" },
429
+ 400: { description: "Invalid comment ids or payload", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
430
+ 401: { description: "Unauthorized", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
431
+ 403: { description: "Forbidden", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
432
+ 404: { description: "Repository or pull request not found", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
433
+ },
434
+ },
435
+ },
436
+ "/repos/{owner}/{repo}/stacks/{stackId}/approvals": {
437
+ get: {
438
+ tags: ["Stacks"],
439
+ summary: "Get stack approval status",
440
+ security: [{ bearerAuth: [] }],
441
+ parameters: [
442
+ { name: "owner", in: "path", required: true, schema: { type: "string" } },
443
+ { name: "repo", in: "path", required: true, schema: { type: "string" } },
444
+ { name: "stackId", in: "path", required: true, schema: { type: "string" } },
445
+ ],
446
+ responses: {
447
+ 200: { description: "Stack approval status returned" },
448
+ 401: { description: "Unauthorized", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
449
+ 404: { description: "Repository or stack not found", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
450
+ },
451
+ },
452
+ post: {
453
+ tags: ["Stacks"],
454
+ summary: "Request approvals across all PRs in a stack",
455
+ description: "Requests stack approvals for eligible reviewers. Reviewers without repository access are skipped.",
456
+ security: [{ bearerAuth: [] }],
457
+ parameters: [
458
+ { name: "owner", in: "path", required: true, schema: { type: "string" } },
459
+ { name: "repo", in: "path", required: true, schema: { type: "string" } },
460
+ { name: "stackId", in: "path", required: true, schema: { type: "string" } },
461
+ ],
462
+ requestBody: {
463
+ required: true,
464
+ content: {
465
+ "application/json": {
466
+ schema: { $ref: "#/components/schemas/StackApprovalRequest" },
467
+ },
468
+ },
469
+ },
470
+ responses: {
471
+ 200: { description: "Approval requests created for eligible reviewers" },
472
+ 400: { description: "Invalid payload or no eligible reviewers", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
473
+ 401: { description: "Unauthorized", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
474
+ 403: { description: "Forbidden", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
475
+ 404: { description: "Repository or stack not found", content: { "application/json": { schema: { $ref: "#/components/schemas/Error" } } } },
476
+ },
477
+ },
478
+ },
363
479
"/user/ai-config": {
364
480
get: {
365
481
tags: ["AI Review"],
366
482
▼src/pages/api/repos/[owner]/[repo]/pulls/[number]/index.ts
+28-1
@@ -8,6 +8,8 @@ import { withErrorHandler } from "@/lib/errors";
8
8
import { logger } from "@/lib/logger";
9
9
import { unauthorized, badRequest, notFound, success, forbidden } from "@/lib/api";
10
10
import { autoLinkPR } from "@/lib/pr-issue-linking";
11
+import { canMerge } from "@/lib/merge-queue";
12
+import { checkCodeOwnerApprovalsForPR } from "@/lib/pr-codeowner";
11
13
12
14
// GET: Get PR details
13
15
export const GET: APIRoute = withErrorHandler(async ({ params, locals }) => {
@@ -93,7 +95,10 @@ export const PATCH: APIRoute = withErrorHandler(async ({ params, request, locals
93
95
94
96
if (!pr) return notFound("Pull request not found");
95
97
96
- const body = await request.json();
98
+ const body = await request.json().catch(() => null);
99
+ if (!body || typeof body !== "object") {
100
+ return badRequest("Invalid request body");
101
+ }
97
102
const { title, body: description, state } = body;
98
103
99
104
const updateData: any = { updatedAt: new Date() };
@@ -170,6 +175,20 @@ export const PATCH: APIRoute = withErrorHandler(async ({ params, request, locals
170
175
}
171
176
}
172
177
178
+ if (customState.requireCodeOwner) {
179
+ const codeOwnerResult = await checkCodeOwnerApprovalsForPR(db, pr.id);
180
+ if (!codeOwnerResult.ok) {
181
+ return badRequest(codeOwnerResult.reason || "Code owner approval required");
182
+ }
183
+ }
184
+
185
+ if (customState.allowMerge) {
186
+ const readiness = await canMerge(pr.id);
187
+ if (!readiness.canMerge) {
188
+ return badRequest(readiness.reason || "Merge requirements not met");
189
+ }
190
+ }
191
+
173
192
updateData.stateId = customState.id;
174
193
updateData.customStateChangedAt = new Date();
175
194
// Custom states are generally 'open' in the high-level sense
@@ -181,14 +200,22 @@ export const PATCH: APIRoute = withErrorHandler(async ({ params, request, locals
181
200
} else if (state === "closed") {
182
201
updateData.state = "closed";
183
202
updateData.closedAt = new Date();
203
+ updateData.closedById = user.id;
204
+ updateData.stateId = null;
205
+ updateData.customStateChangedAt = new Date();
184
206
stateChanged = true;
185
207
} else if (state === "open") {
186
208
updateData.state = "open";
187
209
updateData.closedAt = null;
210
+ updateData.closedById = null;
211
+ updateData.stateId = null;
212
+ updateData.customStateChangedAt = new Date();
188
213
if (pr.isMerged) {
189
214
return badRequest("Cannot re-open a merged pull request");
190
215
}
191
216
stateChanged = true;
217
+ } else {
218
+ return badRequest("Invalid pull request state");
192
219
}
193
220
}
194
221
195
222
▼tests/integration/pr-update-state-route.test.ts
+180-0
@@ -0,0 +1,180 @@
1
+import { beforeEach, describe, expect, it, vi } from "vitest";
2
+
3
+const {
4
+ canWriteRepoMock,
5
+ autoLinkPRMock,
6
+ triggerAutomationMock,
7
+ checkCodeOwnerApprovalsForPRMock,
8
+ canMergeMock,
9
+ fakeSchema,
10
+} = vi.hoisted(() => ({
11
+ canWriteRepoMock: vi.fn(async () => true),
12
+ autoLinkPRMock: vi.fn(async () => {}),
13
+ triggerAutomationMock: vi.fn(async () => {}),
14
+ checkCodeOwnerApprovalsForPRMock: vi.fn(async () => ({ ok: true })),
15
+ canMergeMock: vi.fn(async () => ({ canMerge: true })),
16
+ fakeSchema: {
17
+ users: { username: {}, id: {} },
18
+ repositories: { ownerId: {}, name: {}, id: {} },
19
+ pullRequests: { repositoryId: {}, number: {}, id: {} },
20
+ prStateDefinitions: { repositoryId: {}, name: {} },
21
+ } as any,
22
+}));
23
+
24
+let mockDb: any;
25
+
26
+vi.mock("@/db", () => ({
27
+ getDatabase: () => mockDb,
28
+ schema: fakeSchema,
29
+}));
30
+
31
+vi.mock("@/lib/permissions", () => ({
32
+ canWriteRepo: canWriteRepoMock,
33
+ canReadRepo: vi.fn(async () => true),
34
+}));
35
+
36
+vi.mock("@/lib/pr-issue-linking", () => ({
37
+ autoLinkPR: autoLinkPRMock,
38
+}));
39
+
40
+vi.mock("@/lib/automations", () => ({
41
+ triggerAutomation: triggerAutomationMock,
42
+}));
43
+
44
+vi.mock("@/lib/pr-codeowner", () => ({
45
+ checkCodeOwnerApprovalsForPR: checkCodeOwnerApprovalsForPRMock,
46
+}));
47
+
48
+vi.mock("@/lib/merge-queue", () => ({
49
+ canMerge: canMergeMock,
50
+}));
51
+
52
+import { PATCH as patchPr } from "@/pages/api/repos/[owner]/[repo]/pulls/[number]/index";
53
+
54
+function makeDb() {
55
+ const owner = { id: "owner-1", username: "owner-1" };
56
+ const repo = { id: "repo-1", ownerId: "owner-1", name: "demo" };
57
+ const pr = {
58
+ id: "pr-1",
59
+ repositoryId: "repo-1",
60
+ state: "open",
61
+ stateId: "custom-state-1",
62
+ isMerged: false,
63
+ };
64
+
65
+ const updateCalls: unknown[] = [];
66
+
67
+ return {
68
+ query: {
69
+ users: {
70
+ findFirst: vi.fn(async () => owner),
71
+ },
72
+ repositories: {
73
+ findFirst: vi.fn(async () => repo),
74
+ },
75
+ pullRequests: {
76
+ findFirst: vi.fn(async () => pr),
77
+ },
78
+ prStateDefinitions: {
79
+ findFirst: vi.fn(async () => null),
80
+ },
81
+ prStateReviewers: {
82
+ findMany: vi.fn(async () => []),
83
+ },
84
+ pullRequestReviews: {
85
+ findMany: vi.fn(async () => []),
86
+ },
87
+ teams: {
88
+ findFirst: vi.fn(async () => null),
89
+ },
90
+ teamMembers: {
91
+ findMany: vi.fn(async () => []),
92
+ },
93
+ users2: {
94
+ findFirst: vi.fn(async () => null),
95
+ },
96
+ },
97
+ update: vi.fn(() => ({
98
+ set: vi.fn((value: unknown) => ({
99
+ where: vi.fn(async () => {
100
+ updateCalls.push(value);
101
+ }),
102
+ })),
103
+ })),
104
+ __state: {
105
+ updateCalls,
106
+ },
107
+ };
108
+}
109
+
110
+async function readJson(response: Response): Promise<any> {
111
+ return response.json();
112
+}
113
+
114
+describe("PR update state route", () => {
115
+ beforeEach(() => {
116
+ mockDb = makeDb();
117
+ canWriteRepoMock.mockResolvedValue(true);
118
+ triggerAutomationMock.mockResolvedValue(undefined);
119
+ });
120
+
121
+ it("rejects unknown state values", async () => {
122
+ const response = await patchPr({
123
+ params: { owner: "owner-1", repo: "demo", number: "42" },
124
+ locals: { user: { id: "actor-1" } },
125
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42", {
126
+ method: "PATCH",
127
+ headers: { "content-type": "application/json" },
128
+ body: JSON.stringify({ state: "ready_to_ship" }),
129
+ }),
130
+ } as any);
131
+
132
+ const body = await readJson(response);
133
+ expect(response.status).toBe(400);
134
+ expect(body?.error?.code).toBe("BAD_REQUEST");
135
+ });
136
+
137
+ it("clears custom state reference when closing PR", async () => {
138
+ const response = await patchPr({
139
+ params: { owner: "owner-1", repo: "demo", number: "42" },
140
+ locals: { user: { id: "actor-1" } },
141
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42", {
142
+ method: "PATCH",
143
+ headers: { "content-type": "application/json" },
144
+ body: JSON.stringify({ state: "closed" }),
145
+ }),
146
+ } as any);
147
+
148
+ expect(response.status).toBe(200);
149
+ const updateData = mockDb.__state.updateCalls[0] as Record<string, unknown>;
150
+ expect(updateData.state).toBe("closed");
151
+ expect(updateData.stateId).toBeNull();
152
+ expect(updateData.closedById).toBe("actor-1");
153
+ });
154
+
155
+ it("clears custom state reference when reopening PR", async () => {
156
+ mockDb.query.pullRequests.findFirst.mockResolvedValueOnce({
157
+ id: "pr-1",
158
+ repositoryId: "repo-1",
159
+ state: "closed",
160
+ stateId: "custom-state-1",
161
+ isMerged: false,
162
+ });
163
+
164
+ const response = await patchPr({
165
+ params: { owner: "owner-1", repo: "demo", number: "42" },
166
+ locals: { user: { id: "actor-1" } },
167
+ request: new Request("http://localhost/api/repos/owner-1/demo/pulls/42", {
168
+ method: "PATCH",
169
+ headers: { "content-type": "application/json" },
170
+ body: JSON.stringify({ state: "open" }),
171
+ }),
172
+ } as any);
173
+
174
+ expect(response.status).toBe(200);
175
+ const updateData = mockDb.__state.updateCalls[0] as Record<string, unknown>;
176
+ expect(updateData.state).toBe("open");
177
+ expect(updateData.stateId).toBeNull();
178
+ expect(updateData.closedAt).toBeNull();
179
+ });
180
+});
0
181