diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index 83f362fa20a0..04d4c879a656 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -463,6 +463,8 @@ function fakeProvider( setReaction: () => Effect.void, listReviewerCandidates: () => Effect.succeed({ candidates: [], truncated: false }), setReviewerRequest: () => Effect.void, + // GitHub's own merge message rewrite, which the service reads instead of the kind. + ...(kind === "github" ? { mergeMessageRewrite: (message: string) => message } : {}), ...overrides, }; } diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 0295673350b7..3644fefe985a 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -77,7 +77,6 @@ import { import { resolveProjectSettings } from "@t3tools/shared/projectSettings"; import { detectSourceControlProviderFromRemoteUrl } from "@t3tools/shared/sourceControl"; -import { AllowGitHubReserve } from "@t3tools/source-control-github/server/GitHubApi"; import * as ProjectService from "../project/ProjectService.ts"; import * as ServerSettings from "../serverSettings.ts"; import * as PullRequestFilesViewed from "../persistence/PullRequestFilesViewed.ts"; @@ -557,7 +556,7 @@ function withRateLimitBackoff( ), Effect.flatMap((lease) => effect.pipe( - Effect.provideService(AllowGitHubReserve, allowPaused), + Effect.provideService(SourceControlRateLimit.Interactive, allowPaused), Effect.tap(() => limits.recordSuccess({ ...key, lease })), Effect.tapError((error) => error.reason === "rate-limited" @@ -587,6 +586,9 @@ function withRateLimitBackoff( const wrapped = { kind: api.kind, capabilities: api.capabilities, + ...(api.mergeMessageRewrite === undefined + ? {} + : { mergeMessageRewrite: api.mergeMessageRewrite }), // Refused during a pause like any other read, except for the caller that asks for the // bypass: a lookup that failed is not held, so letting every background read through would // 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* () { )(function* (input) { const host = input.host.toLowerCase(); const { supported } = yield* listWorkspaceProjects({ host }); - const project = supported.find((candidate) => candidate.api.kind === "github"); - const api = registry.get("github"); + const project = supported.find( + (candidate) => registry.get(candidate.api.kind)?.getRoutingIdentity !== undefined, + ); + const api = project === undefined ? null : registry.get(project.api.kind); if (project === undefined || api?.getRoutingIdentity === undefined) { return yield* new PullRequestUnavailableError({ reason: "provider-unsupported" }); } @@ -1569,6 +1573,7 @@ export const make = Effect.gen(function* () { host, }) .pipe(Effect.mapError(toPullRequestError("routeIdentity"))); + // Only GitHub reports a routing identity, and the contract names it. return { ...identity, host, provider: "github" as const }; }); @@ -1584,7 +1589,7 @@ export const make = Effect.gen(function* () { detail: "The GitHub account could not be verified before starting the operation.", }); const project = yield* requireProject(input).pipe(Effect.mapError(rejected)); - const api = project.api.kind === "github" ? registry.get("github") : null; + const api = registry.get(project.api.kind); if ( api?.withVerifiedCredential === undefined || input.host?.toLowerCase() !== project.host.toLowerCase() @@ -1605,7 +1610,7 @@ export const make = Effect.gen(function* () { const routing = Effect.fn("PullRequestService.routing")(function* (input: PullRequestRef) { const project = yield* requireProject(input); - const api = project.api.kind === "github" ? registry.get("github") : null; + const api = registry.get(project.api.kind); if (api?.getRoutingIdentity === undefined) { return yield* new PullRequestUnavailableError({ reason: "provider-unsupported" }); } @@ -2047,7 +2052,7 @@ export const make = Effect.gen(function* () { ); } const mergeSettings = - project.api.kind === "github" && + project.api.mergeMessageRewrite !== undefined && input.stackNumber === undefined && (input.action === "merge" || input.action === "enable-auto-merge") ? serverSettings.getSettings.pipe( diff --git a/packages/source-control-core/src/server/PullRequestProvider.ts b/packages/source-control-core/src/server/PullRequestProvider.ts index c3f588a62277..2500c8fae62a 100644 --- a/packages/source-control-core/src/server/PullRequestProvider.ts +++ b/packages/source-control-core/src/server/PullRequestProvider.ts @@ -345,6 +345,13 @@ export interface PullRequestProviderApi { >; readonly kind: SourceControlProviderKind; readonly capabilities: PullRequestCapabilities; + /** + * Rewrites the message a merge will use, for a host that lets the merge carry custom text. + * Absent means the host always writes its own message, so nothing is read to decide on one. + * Today's only rewrite strips agent credits (`mergeMessage.removeAgentCredits`) when the + * project asks for it. + */ + readonly mergeMessageRewrite?: (message: string) => string; /** The signed-in account, which is what involvement filtering compares against. */ readonly getViewer: (input: { @@ -561,7 +568,7 @@ export interface PullRequestProviderApi { readonly action: PullRequestAction; readonly stackNumber?: number; readonly expectedStackHeads?: ReadonlyArray; - /** GitHub merge message cleanup; ignored by hosts without support. */ + /** Apply `mergeMessageRewrite` to the merge message; never sent to a host without one. */ readonly removeAgentCreditsOnMerge?: boolean; /** Meaningful for `merge` and `enable-auto-merge`; absent takes the host's own default. */ readonly mergeMethod?: PullRequestMergeMethod; diff --git a/packages/source-control-core/src/server/SourceControlRateLimit.ts b/packages/source-control-core/src/server/SourceControlRateLimit.ts index f2ef07dd2529..262a975eacf5 100644 --- a/packages/source-control-core/src/server/SourceControlRateLimit.ts +++ b/packages/source-control-core/src/server/SourceControlRateLimit.ts @@ -20,6 +20,16 @@ export const CredentialScope = Context.Reference( }, ); +/** + * Set by interactive callers (a user's read or write, not a background sweep). A host may let + * requests made under it spend a reserved quota and go through a rate-limit pause: a user acting + * on a change request should not be refused because a background read exhausted the quota. + */ +export const Interactive = Context.Reference( + "@t3tools/source-control-core/server/SourceControlRateLimit/Interactive", + { defaultValue: () => false }, +); + interface RateLimitKey { readonly provider: SourceControlProviderKind; readonly host: string; diff --git a/packages/source-control-github/src/server/GitHubApi.test.ts b/packages/source-control-github/src/server/GitHubApi.test.ts index 3287ebb8e1a1..e77f10ec9370 100644 --- a/packages/source-control-github/src/server/GitHubApi.test.ts +++ b/packages/source-control-github/src/server/GitHubApi.test.ts @@ -275,7 +275,7 @@ describe("GitHubApi", () => { retryAt: reset * 1000, }); // A user's own request may spend the reserve, and GraphQL has a quota of its own. - yield* api.rest(read).pipe(Effect.provideService(GitHubApi.AllowGitHubReserve, true)); + yield* api.rest(read).pipe(Effect.provideService(SourceControlRateLimit.Interactive, true)); yield* api.graphql({ host: "github.com", operation: "x", query: "query { viewer { id } }" }); expect(requests).toHaveLength(3); // The reset gives the background its quota back. @@ -344,7 +344,7 @@ describe("GitHubApi", () => { method: "PUT", path: "repos/acme/web/pulls/7/merge", }) - .pipe(Effect.provideService(GitHubApi.AllowGitHubReserve, true)); + .pipe(Effect.provideService(SourceControlRateLimit.Interactive, true)); expect(merged.status).toBe(200); expect(requests).toHaveLength(2); }).pipe(Effect.provide(layer)); diff --git a/packages/source-control-github/src/server/GitHubApi.ts b/packages/source-control-github/src/server/GitHubApi.ts index e734e5ff07a5..111281a0484e 100644 --- a/packages/source-control-github/src/server/GitHubApi.ts +++ b/packages/source-control-github/src/server/GitHubApi.ts @@ -34,16 +34,6 @@ export const PinnedGitHubCredential = Context.Reference<{ defaultValue: () => null, }); -/** - * Set by interactive callers (a user's read or write, not a background sweep). Requests made - * under it may spend the GraphQL reserve and go through a rate-limit pause: a user acting on a - * pull request should not be refused because a background read exhausted the quota. - */ -export const AllowGitHubReserve = Context.Reference( - "@t3tools/source-control-github/server/GitHubApi/AllowGitHubReserve", - { defaultValue: () => false }, -); - export class GitHubApiRequestError extends Schema.TaggedError()( "GitHubApiRequestError", { host: Schema.String, operation: Schema.String, cause: Schema.Defect() }, @@ -137,7 +127,7 @@ export interface GitHubRestInput { readonly maxResponseBytes?: number; /** Defaults to 30 seconds; a whole pull request's patch may need longer. */ readonly timeout?: Duration.Input; - /** Overrides `AllowGitHubReserve` for this one request. */ + /** Overrides `SourceControlRateLimit.Interactive` for this one request. */ readonly allowReserve?: boolean; } @@ -525,7 +515,7 @@ export const make = Effect.gen(function* () { input.body === undefined ? withEtag : withEtag.pipe(HttpClientRequest.bodyJsonUnsafe(input.body)); - return AllowGitHubReserve.pipe( + return SourceControlRateLimit.Interactive.pipe( Effect.flatMap((interactive) => send({ host: input.host, @@ -545,7 +535,7 @@ export const make = Effect.gen(function* () { const host = normalizeHost(input.host); const { fingerprint } = yield* credential(host); const scope = (yield* SourceControlRateLimit.CredentialScope) || fingerprint; - const allowReserve = input.allowReserve ?? (yield* AllowGitHubReserve); + const allowReserve = input.allowReserve ?? (yield* SourceControlRateLimit.Interactive); // The document, never its variables: user text (bodies, search terms) travels as variables. yield* Effect.annotateCurrentSpan({ "github.operation": input.operation, diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index badb79677585..961106c58acc 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -12,7 +12,6 @@ import * as Redacted from "effect/Redacted"; import { HttpClient, HttpClientResponse } from "effect/http"; -import { AllowGitHubReserve } from "./GitHubApi.ts"; import * as GitHubApi from "./GitHubApi.ts"; import * as GitHubCredentials from "./GitHubCredentials.ts"; import * as GitHubQuota from "./GitHubQuota.ts"; @@ -78,7 +77,7 @@ const mockApi = Layer.effect( const quota = yield* GitHubQuota.GitHubQuota; return GitHubApi.GitHubApi.of({ graphql: (input) => - GitHubApi.AllowGitHubReserve.pipe( + SourceControlRateLimit.Interactive.pipe( Effect.flatMap((interactive) => quota.admit(input.host, "graphql", { allowReserve: input.allowReserve ?? interactive, @@ -3864,7 +3863,9 @@ layer("GitHubPullRequestApi.layer", (it) => { const error = yield* Effect.flip(cli.getPullRequestDetail(input)); expect(error._tag).toBe("SourceControlRateLimitPausedError"); expect(mockedExecute).toHaveBeenCalledOnce(); - yield* cli.getPullRequestDetail(input).pipe(Effect.provideService(AllowGitHubReserve, true)); + yield* cli + .getPullRequestDetail(input) + .pipe(Effect.provideService(SourceControlRateLimit.Interactive, true)); expect(mockedExecute).toHaveBeenCalledTimes(2); }), ); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index ceff9c6c0b5c..b8c7fe227688 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -1526,7 +1526,7 @@ export const make = Effect.gen(function* () { const getPullRequestDetail: GitHubPullRequestApi["Service"]["getPullRequestDetail"] = (input) => { const { owner, name } = parseRepositorySelector(input.repository); - return GitHubApi.AllowGitHubReserve.pipe( + return SourceControlRateLimit.Interactive.pipe( Effect.flatMap((allowReserve) => graphqlRead({ allowReserve, diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts index 98e8aeb5c15b..781d07f0d496 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.test.ts @@ -7,6 +7,7 @@ import type { PullRequestReaction } from "@t3tools/contracts"; import { decodePullRequestDetailJson } from "./gitHubPullRequestJson.ts"; import * as GitHubApi from "./GitHubApi.ts"; +import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; import * as GitHubPullRequestApi from "./GitHubPullRequestApi.ts"; import type { GitHubPullRequestCore } from "./gitHubPullRequestJson.ts"; import { gitHubViewerPermissions, loginAvatarUrl, make } from "./GitHubPullRequestProvider.ts"; @@ -725,7 +726,7 @@ describe("getViewerPermissions", () => { Layer.mock(GitHubPullRequestApi.GitHubPullRequestApi)({ revalidateChecks: (_input, read) => read, getPullRequestDetail: () => - GitHubApi.AllowGitHubReserve.pipe( + SourceControlRateLimit.Interactive.pipe( Effect.tap((allowReserve) => Effect.sync(() => onDetail(allowReserve))), Effect.flatMap(() => detail), ), diff --git a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts index a3030f6c86ad..7b74bd368b81 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestProvider.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestProvider.ts @@ -1,3 +1,4 @@ +import { removeAgentCredits } from "@t3tools/source-control-core/server/mergeMessage"; import * as Effect from "effect/Effect"; import type { PullRequestActor, @@ -7,7 +8,7 @@ import type { PullRequestViewerPermissions, } from "@t3tools/contracts"; -import * as GitHubApi from "./GitHubApi.ts"; +import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; import * as GitHubPullRequestApi from "./GitHubPullRequestApi.ts"; import { PullRequestProviderError, @@ -262,6 +263,7 @@ export const make = Effect.gen(function* () { const provider: PullRequestProviderApi = { kind: "github", capabilities: CAPABILITIES, + mergeMessageRewrite: removeAgentCredits, getRoutingIdentity: (input) => cli.getRoutingIdentity(input).pipe(Effect.mapError(fail("routeIdentity"))), withVerifiedCredential: (input, use) => @@ -526,7 +528,7 @@ export const make = Effect.gen(function* () { // comparison, so one read usually answers what used to take three. When that heavier read // fails, the light access read still answers, withholding only update-branch. return cli.getPullRequestDetail(input).pipe( - Effect.provideService(GitHubApi.AllowGitHubReserve, true), + Effect.provideService(SourceControlRateLimit.Interactive, true), Effect.map((pullRequest) => gitHubViewerPermissions({ ...pullRequest.viewerAccess, diff --git a/packages/source-control-github/src/server/GitHubSourceControlProvider.ts b/packages/source-control-github/src/server/GitHubSourceControlProvider.ts index f906f3e629d4..8edf9423c7c9 100644 --- a/packages/source-control-github/src/server/GitHubSourceControlProvider.ts +++ b/packages/source-control-github/src/server/GitHubSourceControlProvider.ts @@ -963,7 +963,7 @@ export const make = Effect.gen(function* () { }, listChangeRequests: (input) => // An open lookup is a user waiting on a status; the rest may be a background sweep. - (input.state === "open" ? Effect.succeed(true) : GitHubApi.AllowGitHubReserve).pipe( + (input.state === "open" ? Effect.succeed(true) : SourceControlRateLimit.Interactive).pipe( Effect.flatMap((allowReserve) => listByHead({ cwd: input.cwd,