From d80ac21db8dee34e82a694ace2ba78aa5d0c9cf8 Mon Sep 17 00:00:00 2001 From: nullStack65 Date: Fri, 2 Oct 2026 02:12:45 -0400 Subject: [PATCH 1/6] fix(server): disable formal PR review mutations --- .../pullRequest/PullRequestService.test.ts | 114 ++++++++++++------ .../src/pullRequest/PullRequestService.ts | 56 ++++----- 2 files changed, 98 insertions(+), 72 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index a0d111bd710f..5ed3a90a1a66 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -2447,44 +2447,68 @@ it.effect("refuses line comments on a host that takes only a summary", () => }), ); -it.effect( - "refuses a review with neither a summary nor a comment, but lets an approval through", - () => - Effect.gen(function* () { - let approved = false; - const service = yield* makeService({ - projects: [ - project({ - id: "p1", - title: "t3code", - workspaceRoot: "/a", - repository: "pingdotgg/t3code", - }), - ], - providers: [ - fakeProvider("github", { - submitReview: () => { - approved = true; - return Effect.void; - }, - }), - ], - }); - const reference = { - projectId: "p1" as ProjectId, - repository: "pingdotgg/t3code", - number: 1, - }; +it.effect("allows non-voting comments but fails closed for formal review decisions", () => + Effect.gen(function* () { + let permissionReads = 0; + let submissions = 0; + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "t3code", + workspaceRoot: "/a", + repository: "pingdotgg/t3code", + }), + ], + providers: [ + fakeProvider("github", { + getViewerPermissions: () => { + permissionReads += 1; + return Effect.succeed({ + actions: ["merge", "ready", "draft", "close", "reopen"], + comment: true, + resolve: true, + verdicts: ["comment", "approve", "request-changes"], + requestReviewers: true, + }); + }, + submitReview: () => { + submissions += 1; + return Effect.void; + }, + }), + ], + }); + const reference = { + projectId: "p1" as ProjectId, + repository: "pingdotgg/t3code", + number: 1, + }; - const error = yield* Effect.flip( - service.submitReview({ ...reference, verdict: "comment", body: " ", comments: [] }), + const error = yield* Effect.flip( + service.submitReview({ ...reference, verdict: "comment", body: " ", comments: [] }), + ); + assert.strictEqual(error._tag, "PullRequestOperationError"); + + for (const verdict of ["approve", "request-changes"] as const) { + const formalError = yield* Effect.flip( + service.submitReview({ ...reference, verdict, body: "", comments: [] }), ); - assert.strictEqual(error._tag, "PullRequestOperationError"); + assert.strictEqual(formalError._tag, "PullRequestOperationError"); + assert.include(formalError.message, "unavailable through T3"); + } + assert.strictEqual(permissionReads, 0); + assert.strictEqual(submissions, 0); - // An approval is a verdict in itself, so it needs no words. - yield* service.submitReview({ ...reference, verdict: "approve", body: "", comments: [] }); - assert.isTrue(approved); - }), + yield* service.submitReview({ + ...reference, + verdict: "comment", + body: "Looks good to discuss", + comments: [], + }); + assert.strictEqual(permissionReads, 1); + assert.strictEqual(submissions, 1); + }), ); it.effect("refuses to resolve a conversation on a host that cannot", () => @@ -2927,7 +2951,23 @@ it.effect("refuses to ask for a review on a host that cannot, before any call is ); assert.strictEqual(error._tag, "PullRequestOperationError"); - assert.include(error.message, "cannot ask somebody for a review."); + assert.include(error.message, "Reviewer requests and removals are unavailable through T3."); + assert.isFalse(asked); + + const removalError = yield* Effect.flip( + service.requestReviewers({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "octocat", kind: "user" }], + requested: false, + }), + ); + assert.strictEqual(removalError._tag, "PullRequestOperationError"); + assert.include( + removalError.message, + "Reviewer requests and removals are unavailable through T3.", + ); assert.isFalse(asked); }), ); @@ -3002,7 +3042,7 @@ it.effect("refuses a review request this viewer may not make, and says what acce ); assert.strictEqual(error._tag, "PullRequestOperationError"); - assert.include(error.message, "You need write access on this repository to ask for a review."); + assert.include(error.message, "Reviewer requests and removals are unavailable through T3."); assert.isFalse(sent); }), ); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 6b4b2bd8bfd5..87beec4ea14a 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -305,6 +305,10 @@ const ACTION_ACCESS_REFUSALS: Record = { */ const REVIEWER_REQUEST_REFUSAL = "You need write access on this repository to ask for a review."; const LABEL_CHANGE_REFUSAL = "You need triage access on this repository to change its labels."; +const OWNER_ONLY_REVIEW_REFUSAL = + "Formal review decisions are unavailable through T3. Review manually in the forge UI or with the ordinary human CLI."; +const OWNER_ONLY_REVIEWER_REQUEST_REFUSAL = + "Reviewer requests and removals are unavailable through T3. Manage reviewers manually in the forge UI or with the ordinary human CLI."; /** A project this page can read: its remote is on a host with an implementation. */ export interface SupportedProject { @@ -2107,8 +2111,16 @@ export const make = Effect.gen(function* () { }), ); - const submitReview: PullRequestService["Service"]["submitReview"] = (input) => - requireProject(input).pipe( + const submitReview: PullRequestService["Service"]["submitReview"] = (input) => { + if (input.verdict !== "comment") { + return Effect.fail( + new PullRequestOperationError({ + operation: "submitReview", + detail: OWNER_ONLY_REVIEW_REFUSAL, + }), + ); + } + return requireProject(input).pipe( Effect.flatMap((project): Effect.Effect => { const review = project.api.capabilities.review; const refuse = (detail: string) => @@ -2159,6 +2171,7 @@ export const make = Effect.gen(function* () { ); }), ); + }; const replyToThread: PullRequestService["Service"]["replyToThread"] = (input) => (input.body.trim().length === 0 @@ -2313,41 +2326,14 @@ export const make = Effect.gen(function* () { ), ); - const requestReviewers: PullRequestService["Service"]["requestReviewers"] = (input) => - requireProject(input).pipe( - Effect.flatMap((project): Effect.Effect => { - if (!project.api.capabilities.reviewers.request) { - return Effect.fail( - new PullRequestOperationError({ - operation: "requestReviewers", - detail: "This host cannot ask somebody for a review.", - }), - ); - } - return viewerPermissionsOf(project, input, "requestReviewers").pipe( - Effect.flatMap((viewer): Effect.Effect => { - if (!viewer.requestReviewers) { - return Effect.fail( - new PullRequestOperationError({ - operation: "requestReviewers", - detail: REVIEWER_REQUEST_REFUSAL, - }), - ); - } - return project.api - .setReviewerRequest({ - cwd: project.project.workspaceRoot, - repository: project.repository, - host: project.host, - number: input.number, - reviewers: input.reviewers, - requested: input.requested, - }) - .pipe(Effect.mapError(toPullRequestError("requestReviewers"))); - }), - ); + const requestReviewers: PullRequestService["Service"]["requestReviewers"] = (input) => { + return Effect.fail( + new PullRequestOperationError({ + operation: "requestReviewers", + detail: OWNER_ONLY_REVIEWER_REQUEST_REFUSAL, }), ); + }; /** * The labels, like the reviewer candidates, are wanted only by somebody about to change them, From 42a6e33472deb277f9a90c81d46f355d166bd60c Mon Sep 17 00:00:00 2001 From: nullStack65 Date: Fri, 2 Oct 2026 02:24:59 -0400 Subject: [PATCH 2/6] fix(server): silence reviewer denial lint warning --- apps/server/src/pullRequest/PullRequestService.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 87beec4ea14a..e395fdafb6bc 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2326,7 +2326,7 @@ export const make = Effect.gen(function* () { ), ); - const requestReviewers: PullRequestService["Service"]["requestReviewers"] = (input) => { + const requestReviewers: PullRequestService["Service"]["requestReviewers"] = (_input) => { return Effect.fail( new PullRequestOperationError({ operation: "requestReviewers", From ac0fe68d3bf83e80a0ecd48f938f60e346f8b07d Mon Sep 17 00:00:00 2001 From: nullStack65 Date: Fri, 2 Oct 2026 03:29:18 -0400 Subject: [PATCH 3/6] fix(server): route reviewer requests to the verified owner --- .../pullRequest/PullRequestService.test.ts | 168 ++++++++++++------ .../src/pullRequest/PullRequestService.ts | 81 ++++++--- 2 files changed, 175 insertions(+), 74 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 5ed3a90a1a66..8b64faa68c3f 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -2447,7 +2447,7 @@ it.effect("refuses line comments on a host that takes only a summary", () => }), ); -it.effect("allows non-voting comments but fails closed for formal review decisions", () => +it.effect("allows comments and formal review decisions through the provider", () => Effect.gen(function* () { let permissionReads = 0; let submissions = 0; @@ -2491,14 +2491,15 @@ it.effect("allows non-voting comments but fails closed for formal review decisio assert.strictEqual(error._tag, "PullRequestOperationError"); for (const verdict of ["approve", "request-changes"] as const) { - const formalError = yield* Effect.flip( - service.submitReview({ ...reference, verdict, body: "", comments: [] }), - ); - assert.strictEqual(formalError._tag, "PullRequestOperationError"); - assert.include(formalError.message, "unavailable through T3"); + yield* service.submitReview({ + ...reference, + verdict, + body: verdict === "approve" ? "" : "Please address these findings.", + comments: [], + }); } - assert.strictEqual(permissionReads, 0); - assert.strictEqual(submissions, 0); + assert.strictEqual(permissionReads, 2); + assert.strictEqual(submissions, 2); yield* service.submitReview({ ...reference, @@ -2506,8 +2507,8 @@ it.effect("allows non-voting comments but fails closed for formal review decisio body: "Looks good to discuss", comments: [], }); - assert.strictEqual(permissionReads, 1); - assert.strictEqual(submissions, 1); + assert.strictEqual(permissionReads, 3); + assert.strictEqual(submissions, 3); }), ); @@ -2914,61 +2915,126 @@ it.effect("asks another checkout who is signed in when the first one cannot answ }), ); -it.effect("refuses to ask for a review on a host that cannot, before any call is made", () => +it.effect( + "refuses human, team, and ambiguous automation reviewer mutations before the adapter", + () => + Effect.gen(function* () { + let asked = false; + const service = yield* makeService({ + projects: [ + project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" }), + ], + providers: [ + fakeProvider("github", { + capabilities: { + diff: true, + comment: true, + actions: ["merge"], + mergeMethods: ["merge"], + search: true, + reactions: true, + review: FULL_REVIEW, + reviewers: { request: false, listCandidates: false }, + }, + getViewerPermissions: () => { + asked = true; + return Effect.die("must not be called"); + }, + setReviewerRequest: () => Effect.die("must not be called"), + }), + ], + }); + + const error = yield* Effect.flip( + service.requestReviewers({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "octocat", kind: "user" }], + requested: true, + }), + ); + + assert.strictEqual(error._tag, "PullRequestOperationError"); + assert.include(error.message, "only request or remove the verified owner nullStack65"); + assert.isFalse(asked); + + const removalError = yield* Effect.flip( + service.requestReviewers({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "reviewers", kind: "team" }], + expectedAccountId: "112618179", + requested: false, + }), + ); + assert.strictEqual(removalError._tag, "PullRequestOperationError"); + assert.include(removalError.message, "only request or remove the verified owner nullStack65"); + assert.isFalse(asked); + + const botError = yield* Effect.flip( + service.requestReviewers({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "dependabot[bot]", kind: "user" }], + expectedAccountId: "112618179", + requested: true, + }), + ); + assert.strictEqual(botError._tag, "PullRequestOperationError"); + assert.include(botError.message, "ambiguous automation"); + assert.isFalse(asked); + }), +); + +it.effect("allows a verified owner reviewer mutation through the existing provider path", () => Effect.gen(function* () { - let asked = false; + let sent: ReadonlyArray<{ id: string; kind: "user" | "team" }> = []; const service = yield* makeService({ projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], providers: [ fakeProvider("github", { - capabilities: { - diff: true, - comment: true, - actions: ["merge"], - mergeMethods: ["merge"], - search: true, - reactions: true, - review: FULL_REVIEW, - reviewers: { request: false, listCandidates: false }, - }, - getViewerPermissions: () => { - asked = true; - return Effect.die("must not be called"); + withVerifiedCredential: (_input, use) => + use({ + accountId: "112618179", + viewer: "nullStack65", + credentialFingerprint: "owner-credential", + }), + getViewerPermissions: () => + Effect.succeed({ + actions: ["ready", "draft", "close", "reopen"], + comment: true, + resolve: true, + verdicts: ["comment", "approve", "request-changes"], + requestReviewers: true, + }), + setReviewerRequest: (input) => { + sent = input.reviewers; + return Effect.void; }, - setReviewerRequest: () => Effect.die("must not be called"), }), ], }); + const reference = { + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + host: "github.com", + expectedAccountId: "112618179", + }; - const error = yield* Effect.flip( + yield* service.withRoutingCredential( + reference, service.requestReviewers({ - projectId: "p1" as ProjectId, - repository: "acme/web", - number: 1, - reviewers: [{ id: "octocat", kind: "user" }], + ...reference, + reviewers: [{ id: "nullStack65", kind: "user" }], requested: true, }), ); - assert.strictEqual(error._tag, "PullRequestOperationError"); - assert.include(error.message, "Reviewer requests and removals are unavailable through T3."); - assert.isFalse(asked); - - const removalError = yield* Effect.flip( - service.requestReviewers({ - projectId: "p1" as ProjectId, - repository: "acme/web", - number: 1, - reviewers: [{ id: "octocat", kind: "user" }], - requested: false, - }), - ); - assert.strictEqual(removalError._tag, "PullRequestOperationError"); - assert.include( - removalError.message, - "Reviewer requests and removals are unavailable through T3.", - ); - assert.isFalse(asked); + assert.deepStrictEqual(sent, [{ id: "nullStack65", kind: "user" }]); }), ); @@ -3042,7 +3108,7 @@ it.effect("refuses a review request this viewer may not make, and says what acce ); assert.strictEqual(error._tag, "PullRequestOperationError"); - assert.include(error.message, "Reviewer requests and removals are unavailable through T3."); + assert.include(error.message, "only request or remove the verified owner nullStack65"); assert.isFalse(sent); }), ); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index e395fdafb6bc..3f0880a5c948 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -161,6 +161,7 @@ const VIEWER_CACHE_CAPACITY = 32; export type PullRequestError = PullRequestUnavailableError | PullRequestOperationError; const routingCredential = Context.Reference<{ + readonly accountId: string; readonly credentialFingerprint: string; readonly viewer: string; } | null>("t3/PullRequestService/routingCredential", { defaultValue: () => null }); @@ -305,10 +306,10 @@ const ACTION_ACCESS_REFUSALS: Record = { */ const REVIEWER_REQUEST_REFUSAL = "You need write access on this repository to ask for a review."; const LABEL_CHANGE_REFUSAL = "You need triage access on this repository to change its labels."; -const OWNER_ONLY_REVIEW_REFUSAL = - "Formal review decisions are unavailable through T3. Review manually in the forge UI or with the ordinary human CLI."; -const OWNER_ONLY_REVIEWER_REQUEST_REFUSAL = - "Reviewer requests and removals are unavailable through T3. Manage reviewers manually in the forge UI or with the ordinary human CLI."; +const OWNER_GITHUB_ACCOUNT_ID = "112618179"; +const OWNER_GITHUB_LOGIN = "nullstack65"; +const AMBIGUOUS_REVIEWER_REQUEST_REFUSAL = + "T3 can only request or remove the verified owner nullStack65 as a reviewer. Other users, teams, and ambiguous automation must be managed manually in the forge UI or with the ordinary human CLI."; /** A project this page can read: its remote is on a host with an implementation. */ export interface SupportedProject { @@ -2111,16 +2112,8 @@ export const make = Effect.gen(function* () { }), ); - const submitReview: PullRequestService["Service"]["submitReview"] = (input) => { - if (input.verdict !== "comment") { - return Effect.fail( - new PullRequestOperationError({ - operation: "submitReview", - detail: OWNER_ONLY_REVIEW_REFUSAL, - }), - ); - } - return requireProject(input).pipe( + const submitReview: PullRequestService["Service"]["submitReview"] = (input) => + requireProject(input).pipe( Effect.flatMap((project): Effect.Effect => { const review = project.api.capabilities.review; const refuse = (detail: string) => @@ -2171,7 +2164,6 @@ export const make = Effect.gen(function* () { ); }), ); - }; const replyToThread: PullRequestService["Service"]["replyToThread"] = (input) => (input.body.trim().length === 0 @@ -2326,14 +2318,57 @@ export const make = Effect.gen(function* () { ), ); - const requestReviewers: PullRequestService["Service"]["requestReviewers"] = (_input) => { - return Effect.fail( - new PullRequestOperationError({ - operation: "requestReviewers", - detail: OWNER_ONLY_REVIEWER_REQUEST_REFUSAL, - }), - ); - }; + const requestReviewers: PullRequestService["Service"]["requestReviewers"] = (input) => + Effect.gen(function* () { + const reviewer = input.reviewers.length === 1 ? input.reviewers[0] : undefined; + if ( + input.expectedAccountId !== OWNER_GITHUB_ACCOUNT_ID || + reviewer?.kind !== "user" || + reviewer.id.toLowerCase() !== OWNER_GITHUB_LOGIN + ) { + return yield* new PullRequestOperationError({ + operation: "requestReviewers", + detail: AMBIGUOUS_REVIEWER_REQUEST_REFUSAL, + }); + } + const credential = yield* routingCredential; + if (credential?.accountId !== OWNER_GITHUB_ACCOUNT_ID) { + return yield* new PullRequestOperationError({ + operation: "requestReviewers", + detail: AMBIGUOUS_REVIEWER_REQUEST_REFUSAL, + }); + } + const project = yield* requireProject(input); + if (project.api.kind !== "github") { + return yield* new PullRequestOperationError({ + operation: "requestReviewers", + detail: AMBIGUOUS_REVIEWER_REQUEST_REFUSAL, + }); + } + if (!project.api.capabilities.reviewers.request) { + return yield* new PullRequestOperationError({ + operation: "requestReviewers", + detail: "This host cannot ask somebody for a review.", + }); + } + const viewer = yield* viewerPermissionsOf(project, input, "requestReviewers"); + if (!viewer.requestReviewers) { + return yield* new PullRequestOperationError({ + operation: "requestReviewers", + detail: REVIEWER_REQUEST_REFUSAL, + }); + } + yield* project.api + .setReviewerRequest({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + reviewers: input.reviewers, + requested: input.requested, + }) + .pipe(Effect.mapError(toPullRequestError("requestReviewers"))); + }); /** * The labels, like the reviewer candidates, are wanted only by somebody about to change them, From c4e5a7621a70baa07da2ca0357b529d7ce21f077 Mon Sep 17 00:00:00 2001 From: nullStack65 Date: Wed, 7 Oct 2026 07:01:43 -0400 Subject: [PATCH 4/6] fix(server): route reviewer requests through verified owner --- .../pullRequest/PullRequestService.test.ts | 37 ++++--- .../src/pullRequest/PullRequestService.ts | 102 +++++++++++------- 2 files changed, 86 insertions(+), 53 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 8b64faa68c3f..114f6c48fe76 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -2991,7 +2991,10 @@ it.effect( it.effect("allows a verified owner reviewer mutation through the existing provider path", () => Effect.gen(function* () { - let sent: ReadonlyArray<{ id: string; kind: "user" | "team" }> = []; + const sent: Array<{ + reviewers: ReadonlyArray<{ id: string; kind: "user" | "team" }>; + requested: boolean; + }> = []; const service = yield* makeService({ projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], providers: [ @@ -3011,7 +3014,7 @@ it.effect("allows a verified owner reviewer mutation through the existing provid requestReviewers: true, }), setReviewerRequest: (input) => { - sent = input.reviewers; + sent.push({ reviewers: input.reviewers, requested: input.requested }); return Effect.void; }, }), @@ -3021,20 +3024,28 @@ it.effect("allows a verified owner reviewer mutation through the existing provid projectId: "p1" as ProjectId, repository: "acme/web", number: 1, - host: "github.com", - expectedAccountId: "112618179", }; - yield* service.withRoutingCredential( - reference, - service.requestReviewers({ - ...reference, - reviewers: [{ id: "nullStack65", kind: "user" }], - requested: true, - }), - ); + yield* service.requestReviewers({ + ...reference, + reviewers: [{ id: "nullStack65", kind: "user" }], + requested: true, + }); + + yield* service.requestReviewers({ + ...reference, + expectedAccountId: "112618179", + reviewers: [{ id: "octocat", kind: "user" }, { id: "reviewers", kind: "team" }], + requested: false, + }); - assert.deepStrictEqual(sent, [{ id: "nullStack65", kind: "user" }]); + assert.deepStrictEqual(sent, [ + { reviewers: [{ id: "nullStack65", kind: "user" }], requested: true }, + { + reviewers: [{ id: "octocat", kind: "user" }, { id: "reviewers", kind: "team" }], + requested: false, + }, + ]); }), ); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 3f0880a5c948..73ac9b784b13 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2320,54 +2320,76 @@ export const make = Effect.gen(function* () { const requestReviewers: PullRequestService["Service"]["requestReviewers"] = (input) => Effect.gen(function* () { - const reviewer = input.reviewers.length === 1 ? input.reviewers[0] : undefined; - if ( - input.expectedAccountId !== OWNER_GITHUB_ACCOUNT_ID || - reviewer?.kind !== "user" || - reviewer.id.toLowerCase() !== OWNER_GITHUB_LOGIN - ) { - return yield* new PullRequestOperationError({ + const invalid = () => + new PullRequestOperationError({ operation: "requestReviewers", detail: AMBIGUOUS_REVIEWER_REQUEST_REFUSAL, }); + const reviewer = input.reviewers.length === 1 ? input.reviewers[0] : undefined; + if ( + input.reviewers.length === 0 || + (input.requested && + (reviewer?.kind !== "user" || reviewer.id.toLowerCase() !== OWNER_GITHUB_LOGIN)) + ) { + return yield* invalid(); } - const credential = yield* routingCredential; - if (credential?.accountId !== OWNER_GITHUB_ACCOUNT_ID) { - return yield* new PullRequestOperationError({ - operation: "requestReviewers", - detail: AMBIGUOUS_REVIEWER_REQUEST_REFUSAL, - }); + if ( + input.expectedAccountId !== undefined && + input.expectedAccountId !== OWNER_GITHUB_ACCOUNT_ID + ) { + return yield* invalid(); } + const project = yield* requireProject(input); - if (project.api.kind !== "github") { - return yield* new PullRequestOperationError({ - operation: "requestReviewers", - detail: AMBIGUOUS_REVIEWER_REQUEST_REFUSAL, - }); + if (project.api.kind !== "github" || project.host.toLowerCase() !== "github.com") { + return yield* invalid(); } - if (!project.api.capabilities.reviewers.request) { - return yield* new PullRequestOperationError({ - operation: "requestReviewers", - detail: "This host cannot ask somebody for a review.", - }); - } - const viewer = yield* viewerPermissionsOf(project, input, "requestReviewers"); - if (!viewer.requestReviewers) { - return yield* new PullRequestOperationError({ - operation: "requestReviewers", - detail: REVIEWER_REQUEST_REFUSAL, - }); + const api = registry.get("github"); + if (api?.withVerifiedCredential === undefined) return yield* invalid(); + + const perform = Effect.gen(function* () { + if (!project.api.capabilities.reviewers.request) { + return yield* new PullRequestOperationError({ + operation: "requestReviewers", + detail: "This host cannot ask somebody for a review.", + }); + } + const viewer = yield* viewerPermissionsOf(project, input, "requestReviewers"); + if (!viewer.requestReviewers) { + return yield* new PullRequestOperationError({ + operation: "requestReviewers", + detail: REVIEWER_REQUEST_REFUSAL, + }); + } + yield* project.api + .setReviewerRequest({ + cwd: project.project.workspaceRoot, + repository: project.repository, + host: project.host, + number: input.number, + reviewers: input.reviewers, + requested: input.requested, + }) + .pipe(Effect.mapError(toPullRequestError("requestReviewers"))); + }); + + const credential = yield* routingCredential; + if (credential !== null) { + if (credential.accountId !== OWNER_GITHUB_ACCOUNT_ID) return yield* invalid(); + return yield* perform; } - yield* project.api - .setReviewerRequest({ - cwd: project.project.workspaceRoot, - repository: project.repository, - host: project.host, - number: input.number, - reviewers: input.reviewers, - requested: input.requested, - }) - .pipe(Effect.mapError(toPullRequestError("requestReviewers"))); + const result = yield* api + .withVerifiedCredential( + { cwd: project.project.workspaceRoot, host: project.host }, + (identity) => + identity.accountId === OWNER_GITHUB_ACCOUNT_ID && + (input.expectedAccountId === undefined || + identity.accountId === input.expectedAccountId) + ? perform.pipe(Effect.provideService(routingCredential, identity), Effect.result) + : Effect.fail(invalid()), + ) + .pipe(Effect.catchTag("PullRequestProviderError", () => Effect.fail(invalid()))); + return yield* Effect.fromResult(result); }); /** From 7bd9af431cb9a2501ffd71d515f890af4004bddf Mon Sep 17 00:00:00 2001 From: nullStack65 Date: Wed, 7 Oct 2026 07:43:26 -0400 Subject: [PATCH 5/6] test(server): cover reviewer routing authorization boundaries Adopts Luna test commit 3eaf0a1460d5d302bd3b59356b949300afd29404 and corrects the independent review finding about removal guidance. --- .../pullRequest/PullRequestService.test.ts | 215 +++++++++++++++++- .../src/pullRequest/PullRequestService.ts | 2 +- 2 files changed, 210 insertions(+), 7 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 114f6c48fe76..77d98e6cff6f 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -2956,7 +2956,7 @@ it.effect( ); assert.strictEqual(error._tag, "PullRequestOperationError"); - assert.include(error.message, "only request or remove the verified owner nullStack65"); + assert.include(error.message, "can request only nullStack65"); assert.isFalse(asked); const removalError = yield* Effect.flip( @@ -2970,7 +2970,7 @@ it.effect( }), ); assert.strictEqual(removalError._tag, "PullRequestOperationError"); - assert.include(removalError.message, "only request or remove the verified owner nullStack65"); + assert.include(removalError.message, "can request only nullStack65"); assert.isFalse(asked); const botError = yield* Effect.flip( @@ -2984,7 +2984,7 @@ it.effect( }), ); assert.strictEqual(botError._tag, "PullRequestOperationError"); - assert.include(botError.message, "ambiguous automation"); + assert.include(botError.message, "Other reviewer requests"); assert.isFalse(asked); }), ); @@ -3035,20 +3035,223 @@ it.effect("allows a verified owner reviewer mutation through the existing provid yield* service.requestReviewers({ ...reference, expectedAccountId: "112618179", - reviewers: [{ id: "octocat", kind: "user" }, { id: "reviewers", kind: "team" }], + reviewers: [ + { id: "octocat", kind: "user" }, + { id: "reviewers", kind: "team" }, + ], requested: false, }); + yield* service.withRoutingCredential( + { ...reference, host: "github.com", expectedAccountId: "112618179" }, + service.requestReviewers({ + ...reference, + host: "github.com", + expectedAccountId: "112618179", + reviewers: [{ id: "nullStack65", kind: "user" }], + requested: true, + }), + ); + assert.deepStrictEqual(sent, [ { reviewers: [{ id: "nullStack65", kind: "user" }], requested: true }, { - reviewers: [{ id: "octocat", kind: "user" }, { id: "reviewers", kind: "team" }], + reviewers: [ + { id: "octocat", kind: "user" }, + { id: "reviewers", kind: "team" }, + ], requested: false, }, + { reviewers: [{ id: "nullStack65", kind: "user" }], requested: true }, ]); }), ); +it.effect("refuses reviewer routing without the verified owner identity", () => + Effect.gen(function* () { + let mutations = 0; + let permissionReads = 0; + const base = { + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "nullStack65", kind: "user" as const }], + requested: true, + }; + const wrongAccount = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + withVerifiedCredential: (_input, use) => + use({ accountId: "7", viewer: "someone-else", credentialFingerprint: "other" }), + getViewerPermissions: () => { + permissionReads++; + return Effect.succeed({ + actions: [], + comment: true, + resolve: true, + verdicts: ["comment", "approve", "request-changes"], + requestReviewers: true, + }); + }, + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const wrongAccountError = yield* Effect.flip(wrongAccount.requestReviewers(base)); + assert.strictEqual(wrongAccountError._tag, "PullRequestOperationError"); + assert.strictEqual(permissionReads, 0); + assert.strictEqual(mutations, 0); + + const noVerifier = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const noVerifierError = yield* Effect.flip(noVerifier.requestReviewers(base)); + assert.strictEqual(noVerifierError._tag, "PullRequestOperationError"); + assert.strictEqual(mutations, 0); + + const explicitMismatch = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + withVerifiedCredential: (_input, use) => + use({ accountId: "112618179", viewer: "nullStack65", credentialFingerprint: "owner" }), + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const mismatchError = yield* Effect.flip( + explicitMismatch.requestReviewers({ ...base, expectedAccountId: "7" }), + ); + assert.strictEqual(mismatchError._tag, "PullRequestOperationError"); + assert.strictEqual(mutations, 0); + }), +); + +it.effect("does not treat the owner's numeric GitHub ID as authority on enterprise hosts", () => + Effect.gen(function* () { + let verified = 0; + let mutations = 0; + const service = yield* makeService({ + projects: [ + project({ + id: "p1", + title: "enterprise", + workspaceRoot: "/a", + repository: "acme/web", + host: "github.enterprise.test", + }), + ], + providers: [ + fakeProvider("github", { + withVerifiedCredential: (_input, use) => { + verified++; + return use({ + accountId: "112618179", + viewer: "nullStack65", + credentialFingerprint: "owner", + }); + }, + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const error = yield* Effect.flip( + service.requestReviewers({ + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "nullStack65", kind: "user" }], + requested: true, + }), + ); + assert.strictEqual(error._tag, "PullRequestOperationError"); + assert.strictEqual(verified, 0); + assert.strictEqual(mutations, 0); + }), +); + +it.effect("preserves write permission and host capability denials for verified owner routing", () => + Effect.gen(function* () { + let mutations = 0; + const deniedPermission = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + withVerifiedCredential: (_input, use) => + use({ accountId: "112618179", viewer: "nullStack65", credentialFingerprint: "owner" }), + getViewerPermissions: () => + Effect.succeed({ + actions: [], + comment: true, + resolve: true, + verdicts: ["comment", "approve", "request-changes"], + requestReviewers: false, + }), + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const reference = { + projectId: "p1" as ProjectId, + repository: "acme/web", + number: 1, + reviewers: [{ id: "nullStack65", kind: "user" as const }], + requested: true, + }; + const permissionError = yield* Effect.flip(deniedPermission.requestReviewers(reference)); + assert.include(permissionError.message, "write access"); + assert.strictEqual(mutations, 0); + + const deniedCapability = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + capabilities: { + diff: true, + comment: true, + actions: ["merge"], + mergeMethods: ["merge"], + search: true, + reactions: true, + review: FULL_REVIEW, + reviewers: { request: false, listCandidates: false }, + }, + withVerifiedCredential: (_input, use) => + use({ accountId: "112618179", viewer: "nullStack65", credentialFingerprint: "owner" }), + setReviewerRequest: () => { + mutations++; + return Effect.void; + }, + }), + ], + }); + const capabilityError = yield* Effect.flip(deniedCapability.requestReviewers(reference)); + assert.include(capabilityError.message, "This host cannot ask somebody"); + assert.strictEqual(mutations, 0); + }), +); + it.effect("refuses the candidate list on a host that has no such list to give", () => Effect.gen(function* () { const service = yield* makeService({ @@ -3119,7 +3322,7 @@ it.effect("refuses a review request this viewer may not make, and says what acce ); assert.strictEqual(error._tag, "PullRequestOperationError"); - assert.include(error.message, "only request or remove the verified owner nullStack65"); + assert.include(error.message, "can request only nullStack65"); assert.isFalse(sent); }), ); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 73ac9b784b13..8d2c778d2c89 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -309,7 +309,7 @@ const LABEL_CHANGE_REFUSAL = "You need triage access on this repository to chang const OWNER_GITHUB_ACCOUNT_ID = "112618179"; const OWNER_GITHUB_LOGIN = "nullstack65"; const AMBIGUOUS_REVIEWER_REQUEST_REFUSAL = - "T3 can only request or remove the verified owner nullStack65 as a reviewer. Other users, teams, and ambiguous automation must be managed manually in the forge UI or with the ordinary human CLI."; + "T3 can request only nullStack65 as a reviewer. The verified owner can remove pending reviewer assignments. Other reviewer requests must be managed manually in the forge UI or with the ordinary human CLI."; /** A project this page can read: its remote is on a host with an implementation. */ export interface SupportedProject { From fedd03077bd2128e4ddecf4e709b8114a35d881e Mon Sep 17 00:00:00 2001 From: nullStack65 Date: Wed, 7 Oct 2026 08:14:11 -0400 Subject: [PATCH 6/6] fix(server): avoid manual fallback for other reviewer requests --- apps/server/src/pullRequest/PullRequestService.test.ts | 2 +- apps/server/src/pullRequest/PullRequestService.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 77d98e6cff6f..c51e6c992da1 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -2984,7 +2984,7 @@ it.effect( }), ); assert.strictEqual(botError._tag, "PullRequestOperationError"); - assert.include(botError.message, "Other reviewer requests"); + assert.include(botError.message, "can request only nullStack65"); assert.isFalse(asked); }), ); diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 8d2c778d2c89..137c7ccffa5e 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -309,7 +309,7 @@ const LABEL_CHANGE_REFUSAL = "You need triage access on this repository to chang const OWNER_GITHUB_ACCOUNT_ID = "112618179"; const OWNER_GITHUB_LOGIN = "nullstack65"; const AMBIGUOUS_REVIEWER_REQUEST_REFUSAL = - "T3 can request only nullStack65 as a reviewer. The verified owner can remove pending reviewer assignments. Other reviewer requests must be managed manually in the forge UI or with the ordinary human CLI."; + "T3 can request only nullStack65 as a reviewer. The verified owner can remove pending reviewer assignments."; /** A project this page can read: its remote is on a host with an implementation. */ export interface SupportedProject {