authorgravatar for julius0216@outlook.comJulius Marminge <julius0216@outlook.com> 2026-10-09 16:22:36-07:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-10-09 16:22:36-07:00
log2ce3e4ae74deb63bda7aaf059949a12aaaefed56
tree5292f030d633c2e15474f629e79ee9ae464809b1
parent49f849ecce3667abab7ce9e6aaa42dfd2e3c45d7
signature Signed by PGP key B5690EEEBB952194

refactor(source-control): PullRequestService reads GitHub resolvers, not its kind (#17619)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

11 files changed, 49 insertions(+), 31 deletions(-)

apps/server/src/pullRequest/PullRequestService.test.ts+2
......@@ -463,6 +463,8 @@ function fakeProvider(
463463 setReaction: () => Effect.void,
464464 listReviewerCandidates: () => Effect.succeed({ candidates: [], truncated: false }),
465465 setReviewerRequest: () => Effect.void,
466 // GitHub's own merge message rewrite, which the service reads instead of the kind.
467 ...(kind === "github" ? { mergeMessageRewrite: (message: string) => message } : {}),
466468 ...overrides,
467469 };
468470}
apps/server/src/pullRequest/PullRequestService.ts+12-7
......@@ -77,7 +77,6 @@ import {
7777import { resolveProjectSettings } from "@t3tools/shared/projectSettings";
7878import { detectSourceControlProviderFromRemoteUrl } from "@t3tools/shared/sourceControl";
7979
80import { AllowGitHubReserve } from "@t3tools/source-control-github/server/GitHubApi";
8180import * as ProjectService from "../project/ProjectService.ts";
8281import * as ServerSettings from "../serverSettings.ts";
8382import * as PullRequestFilesViewed from "../persistence/PullRequestFilesViewed.ts";
......@@ -557,7 +556,7 @@ function withRateLimitBackoff(
557556 ),
558557 Effect.flatMap((lease) =>
559558 effect.pipe(
560 Effect.provideService(AllowGitHubReserve, allowPaused),
559 Effect.provideService(SourceControlRateLimit.Interactive, allowPaused),
561560 Effect.tap(() => limits.recordSuccess({ ...key, lease })),
562561 Effect.tapError((error) =>
563562 error.reason === "rate-limited"
......@@ -587,6 +586,9 @@ function withRateLimitBackoff(
587586 const wrapped = {
588587 kind: api.kind,
589588 capabilities: api.capabilities,
589 ...(api.mergeMessageRewrite === undefined
590 ? {}
591 : { mergeMessageRewrite: api.mergeMessageRewrite }),
590592 // Refused during a pause like any other read, except for the caller that asks for the
591593 // bypass: a lookup that failed is not held, so letting every background read through would
592594 // spawn this host's CLI on each of them and re-extend the pause it was already in.
......@@ -1558,8 +1560,10 @@ export const make = Effect.gen(function* () {
15581560 )(function* (input) {
15591561 const host = input.host.toLowerCase();
15601562 const { supported } = yield* listWorkspaceProjects({ host });
1561 const project = supported.find((candidate) => candidate.api.kind === "github");
1562 const api = registry.get("github");
1563 const project = supported.find(
1564 (candidate) => registry.get(candidate.api.kind)?.getRoutingIdentity !== undefined,
1565 );
1566 const api = project === undefined ? null : registry.get(project.api.kind);
15631567 if (project === undefined || api?.getRoutingIdentity === undefined) {
15641568 return yield* new PullRequestUnavailableError({ reason: "provider-unsupported" });
15651569 }
......@@ -1569,6 +1573,7 @@ export const make = Effect.gen(function* () {
15691573 host,
15701574 })
15711575 .pipe(Effect.mapError(toPullRequestError("routeIdentity")));
1576 // Only GitHub reports a routing identity, and the contract names it.
15721577 return { ...identity, host, provider: "github" as const };
15731578 });
15741579
......@@ -1584,7 +1589,7 @@ export const make = Effect.gen(function* () {
15841589 detail: "The GitHub account could not be verified before starting the operation.",
15851590 });
15861591 const project = yield* requireProject(input).pipe(Effect.mapError(rejected));
1587 const api = project.api.kind === "github" ? registry.get("github") : null;
1592 const api = registry.get(project.api.kind);
15881593 if (
15891594 api?.withVerifiedCredential === undefined ||
15901595 input.host?.toLowerCase() !== project.host.toLowerCase()
......@@ -1605,7 +1610,7 @@ export const make = Effect.gen(function* () {
16051610
16061611 const routing = Effect.fn("PullRequestService.routing")(function* (input: PullRequestRef) {
16071612 const project = yield* requireProject(input);
1608 const api = project.api.kind === "github" ? registry.get("github") : null;
1613 const api = registry.get(project.api.kind);
16091614 if (api?.getRoutingIdentity === undefined) {
16101615 return yield* new PullRequestUnavailableError({ reason: "provider-unsupported" });
16111616 }
......@@ -2047,7 +2052,7 @@ export const make = Effect.gen(function* () {
20472052 );
20482053 }
20492054 const mergeSettings =
2050 project.api.kind === "github" &&
2055 project.api.mergeMessageRewrite !== undefined &&
20512056 input.stackNumber === undefined &&
20522057 (input.action === "merge" || input.action === "enable-auto-merge")
20532058 ? serverSettings.getSettings.pipe(
packages/source-control-core/src/server/PullRequestProvider.ts+8-1
......@@ -345,6 +345,13 @@ export interface PullRequestProviderApi {
345345 >;
346346 readonly kind: SourceControlProviderKind;
347347 readonly capabilities: PullRequestCapabilities;
348 /**
349 * Rewrites the message a merge will use, for a host that lets the merge carry custom text.
350 * Absent means the host always writes its own message, so nothing is read to decide on one.
351 * Today's only rewrite strips agent credits (`mergeMessage.removeAgentCredits`) when the
352 * project asks for it.
353 */
354 readonly mergeMessageRewrite?: (message: string) => string;
348355
349356 /** The signed-in account, which is what involvement filtering compares against. */
350357 readonly getViewer: (input: {
......@@ -561,7 +568,7 @@ export interface PullRequestProviderApi {
561568 readonly action: PullRequestAction;
562569 readonly stackNumber?: number;
563570 readonly expectedStackHeads?: ReadonlyArray<PullRequestStackHead>;
564 /** GitHub merge message cleanup; ignored by hosts without support. */
571 /** Apply `mergeMessageRewrite` to the merge message; never sent to a host without one. */
565572 readonly removeAgentCreditsOnMerge?: boolean;
566573 /** Meaningful for `merge` and `enable-auto-merge`; absent takes the host's own default. */
567574 readonly mergeMethod?: PullRequestMergeMethod;
packages/source-control-core/src/server/SourceControlRateLimit.ts+10
......@@ -20,6 +20,16 @@ export const CredentialScope = Context.Reference<string>(
2020 },
2121);
2222
23/**
24 * Set by interactive callers (a user's read or write, not a background sweep). A host may let
25 * requests made under it spend a reserved quota and go through a rate-limit pause: a user acting
26 * on a change request should not be refused because a background read exhausted the quota.
27 */
28export const Interactive = Context.Reference<boolean>(
29 "@t3tools/source-control-core/server/SourceControlRateLimit/Interactive",
30 { defaultValue: () => false },
31);
32
2333interface RateLimitKey {
2434 readonly provider: SourceControlProviderKind;
2535 readonly host: string;
packages/source-control-github/src/server/GitHubApi.test.ts+2-2
......@@ -275,7 +275,7 @@ describe("GitHubApi", () => {
275275 retryAt: reset * 1000,
276276 });
277277 // A user's own request may spend the reserve, and GraphQL has a quota of its own.
278 yield* api.rest(read).pipe(Effect.provideService(GitHubApi.AllowGitHubReserve, true));
278 yield* api.rest(read).pipe(Effect.provideService(SourceControlRateLimit.Interactive, true));
279279 yield* api.graphql({ host: "github.com", operation: "x", query: "query { viewer { id } }" });
280280 expect(requests).toHaveLength(3);
281281 // The reset gives the background its quota back.
......@@ -344,7 +344,7 @@ describe("GitHubApi", () => {
344344 method: "PUT",
345345 path: "repos/acme/web/pulls/7/merge",
346346 })
347 .pipe(Effect.provideService(GitHubApi.AllowGitHubReserve, true));
347 .pipe(Effect.provideService(SourceControlRateLimit.Interactive, true));
348348 expect(merged.status).toBe(200);
349349 expect(requests).toHaveLength(2);
350350 }).pipe(Effect.provide(layer));
packages/source-control-github/src/server/GitHubApi.ts+3-13
......@@ -34,16 +34,6 @@ export const PinnedGitHubCredential = Context.Reference<{
3434 defaultValue: () => null,
3535});
3636
37/**
38 * Set by interactive callers (a user's read or write, not a background sweep). Requests made
39 * under it may spend the GraphQL reserve and go through a rate-limit pause: a user acting on a
40 * pull request should not be refused because a background read exhausted the quota.
41 */
42export const AllowGitHubReserve = Context.Reference<boolean>(
43 "@t3tools/source-control-github/server/GitHubApi/AllowGitHubReserve",
44 { defaultValue: () => false },
45);
46
4737export class GitHubApiRequestError extends Schema.TaggedError<GitHubApiRequestError>()(
4838 "GitHubApiRequestError",
4939 { host: Schema.String, operation: Schema.String, cause: Schema.Defect() },
......@@ -137,7 +127,7 @@ export interface GitHubRestInput {
137127 readonly maxResponseBytes?: number;
138128 /** Defaults to 30 seconds; a whole pull request's patch may need longer. */
139129 readonly timeout?: Duration.Input;
140 /** Overrides `AllowGitHubReserve` for this one request. */
130 /** Overrides `SourceControlRateLimit.Interactive` for this one request. */
141131 readonly allowReserve?: boolean;
142132}
143133
......@@ -525,7 +515,7 @@ export const make = Effect.gen(function* () {
525515 input.body === undefined
526516 ? withEtag
527517 : withEtag.pipe(HttpClientRequest.bodyJsonUnsafe(input.body));
528 return AllowGitHubReserve.pipe(
518 return SourceControlRateLimit.Interactive.pipe(
529519 Effect.flatMap((interactive) =>
530520 send({
531521 host: input.host,
......@@ -545,7 +535,7 @@ export const make = Effect.gen(function* () {
545535 const host = normalizeHost(input.host);
546536 const { fingerprint } = yield* credential(host);
547537 const scope = (yield* SourceControlRateLimit.CredentialScope) || fingerprint;
548 const allowReserve = input.allowReserve ?? (yield* AllowGitHubReserve);
538 const allowReserve = input.allowReserve ?? (yield* SourceControlRateLimit.Interactive);
549539 // The document, never its variables: user text (bodies, search terms) travels as variables.
550540 yield* Effect.annotateCurrentSpan({
551541 "github.operation": input.operation,
packages/source-control-github/src/server/GitHubPullRequestApi.test.ts+4-3
......@@ -12,7 +12,6 @@ import * as Redacted from "effect/Redacted";
1212
1313import { HttpClient, HttpClientResponse } from "effect/http";
1414
15import { AllowGitHubReserve } from "./GitHubApi.ts";
1615import * as GitHubApi from "./GitHubApi.ts";
1716import * as GitHubCredentials from "./GitHubCredentials.ts";
1817import * as GitHubQuota from "./GitHubQuota.ts";
......@@ -78,7 +77,7 @@ const mockApi = Layer.effect(
7877 const quota = yield* GitHubQuota.GitHubQuota;
7978 return GitHubApi.GitHubApi.of({
8079 graphql: (input) =>
81 GitHubApi.AllowGitHubReserve.pipe(
80 SourceControlRateLimit.Interactive.pipe(
8281 Effect.flatMap((interactive) =>
8382 quota.admit(input.host, "graphql", {
8483 allowReserve: input.allowReserve ?? interactive,
......@@ -3864,7 +3863,9 @@ layer("GitHubPullRequestApi.layer", (it) => {
38643863 const error = yield* Effect.flip(cli.getPullRequestDetail(input));
38653864 expect(error._tag).toBe("SourceControlRateLimitPausedError");
38663865 expect(mockedExecute).toHaveBeenCalledOnce();
3867 yield* cli.getPullRequestDetail(input).pipe(Effect.provideService(AllowGitHubReserve, true));
3866 yield* cli
3867 .getPullRequestDetail(input)
3868 .pipe(Effect.provideService(SourceControlRateLimit.Interactive, true));
38683869 expect(mockedExecute).toHaveBeenCalledTimes(2);
38693870 }),
38703871 );
packages/source-control-github/src/server/GitHubPullRequestApi.ts+1-1
......@@ -1526,7 +1526,7 @@ export const make = Effect.gen(function* () {
15261526
15271527 const getPullRequestDetail: GitHubPullRequestApi["Service"]["getPullRequestDetail"] = (input) => {
15281528 const { owner, name } = parseRepositorySelector(input.repository);
1529 return GitHubApi.AllowGitHubReserve.pipe(
1529 return SourceControlRateLimit.Interactive.pipe(
15301530 Effect.flatMap((allowReserve) =>
15311531 graphqlRead({
15321532 allowReserve,
packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts+2-1
......@@ -7,6 +7,7 @@ import type { PullRequestReaction } from "@t3tools/contracts";
77
88import { decodePullRequestDetailJson } from "./gitHubPullRequestJson.ts";
99import * as GitHubApi from "./GitHubApi.ts";
10import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit";
1011import * as GitHubPullRequestApi from "./GitHubPullRequestApi.ts";
1112import type { GitHubPullRequestCore } from "./gitHubPullRequestJson.ts";
1213import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullRequestProvider.ts";
......@@ -725,7 +726,7 @@ describe("getViewerPermissions", () => {
725726 Layer.mock(GitHubPullRequestApi.GitHubPullRequestApi)({
726727 revalidateChecks: (_input, read) => read,
727728 getPullRequestDetail: () =>
728 GitHubApi.AllowGitHubReserve.pipe(
729 SourceControlRateLimit.Interactive.pipe(
729730 Effect.tap((allowReserve) => Effect.sync(() => onDetail(allowReserve))),
730731 Effect.flatMap(() => detail),
731732 ),
packages/source-control-github/src/server/GitHubPullRequestProvider.ts+4-2
......@@ -1,3 +1,4 @@
1import { removeAgentCredits } from "@t3tools/source-control-core/server/mergeMessage";
12import * as Effect from "effect/Effect";
23import type {
34 PullRequestActor,
......@@ -7,7 +8,7 @@ import type {
78 PullRequestViewerPermissions,
89} from "@t3tools/contracts";
910
10import * as GitHubApi from "./GitHubApi.ts";
11import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit";
1112import * as GitHubPullRequestApi from "./GitHubPullRequestApi.ts";
1213import {
1314 PullRequestProviderError,
......@@ -262,6 +263,7 @@ export const make = Effect.gen(function* () {
262263 const provider: PullRequestProviderApi = {
263264 kind: "github",
264265 capabilities: CAPABILITIES,
266 mergeMessageRewrite: removeAgentCredits,
265267 getRoutingIdentity: (input) =>
266268 cli.getRoutingIdentity(input).pipe(Effect.mapError(fail("routeIdentity"))),
267269 withVerifiedCredential: (input, use) =>
......@@ -526,7 +528,7 @@ export const make = Effect.gen(function* () {
526528 // comparison, so one read usually answers what used to take three. When that heavier read
527529 // fails, the light access read still answers, withholding only update-branch.
528530 return cli.getPullRequestDetail(input).pipe(
529 Effect.provideService(GitHubApi.AllowGitHubReserve, true),
531 Effect.provideService(SourceControlRateLimit.Interactive, true),
530532 Effect.map((pullRequest) =>
531533 gitHubViewerPermissions({
532534 ...pullRequest.viewerAccess,
packages/source-control-github/src/server/GitHubSourceControlProvider.ts+1-1
......@@ -963,7 +963,7 @@ export const make = Effect.gen(function* () {
963963 },
964964 listChangeRequests: (input) =>
965965 // An open lookup is a user waiting on a status; the rest may be a background sweep.
966 (input.state === "open" ? Effect.succeed(true) : GitHubApi.AllowGitHubReserve).pipe(
966 (input.state === "open" ? Effect.succeed(true) : SourceControlRateLimit.Interactive).pipe(
967967 Effect.flatMap((allowReserve) =>
968968 listByHead({
969969 cwd: input.cwd,