From 9cdd2511700f0ce0a8389a532b2a70b698a37fd0 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Sun, 11 Oct 2026 23:44:15 +0000 Subject: [PATCH 01/10] perf(source-control): reduce pull request read overhead --- .../src/sourceControl/builtInDrivers.ts | 55 ++- .../server/AzureDevOpsPullRequestCli.test.ts | 5 +- .../server/AzureDevOpsPullRequestProvider.ts | 313 ++++++++++++++---- .../server/BitbucketPullRequestApi.test.ts | 41 +-- .../src/server/BitbucketPullRequestApi.ts | 33 +- .../server/BitbucketPullRequestProvider.ts | 49 +++ .../src/server/ForgejoCli.ts | 83 ++++- .../src/server/ForgejoPullRequestProvider.ts | 46 ++- .../ForgejoSourceControlProvider.test.ts | 223 +++++++++++++ .../src/server/GitCafePullRequestProvider.ts | 207 +++++++++--- .../src/server/GitHubPullRequestApi.test.ts | 170 +++++++++- .../src/server/GitHubPullRequestApi.ts | 251 +++++++++----- .../src/server/gitHubPullRequestJson.ts | 5 +- .../src/server/GitLabPullRequestCli.ts | 19 +- .../src/server/GitLabPullRequestProvider.ts | 132 ++++++-- 15 files changed, 1334 insertions(+), 298 deletions(-) diff --git a/apps/server/src/sourceControl/builtInDrivers.ts b/apps/server/src/sourceControl/builtInDrivers.ts index 1b891ce9259a..56805031ab1c 100644 --- a/apps/server/src/sourceControl/builtInDrivers.ts +++ b/apps/server/src/sourceControl/builtInDrivers.ts @@ -25,7 +25,10 @@ import * as GitLabCli from "@t3tools/source-control-gitlab/server/GitLabCli"; import * as GitLabPullRequestCli from "@t3tools/source-control-gitlab/server/GitLabPullRequestCli"; import * as GitLabDriver from "@t3tools/source-control-gitlab/server/driver"; import type { SourceControlDriver } from "@t3tools/source-control-core/server/driver"; +import * as Undici from "@effect/platform-node/Undici"; +import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; +import { FetchHttpClient } from "effect/http"; import * as ServerSourceControlHost from "./ServerSourceControlHost.ts"; @@ -47,6 +50,56 @@ export const BUILT_IN_SOURCE_CONTROL_DRIVERS: ReadonlyArray< SourceControlDriver > = drivers; +/** + * The client the HTTP drivers read through: fetch, so redirects and headers behave as before, over + * one pool of its own. The server's fetch drops a connection after four idle seconds and opens one + * per concurrent request, so nearly every pull request opened after a pause paid a new TLS + * handshake per read. This pool keeps connections for minutes, or for as long as the host does, + * and carries a panel's parallel reads over one HTTP/2 connection where the host offers it. + * Fresh, since the server builds the same `FetchHttpClient.layer` for itself and a memoized one + * would ignore the `Fetch` given here. + */ +const httpDriverClient = Layer.fresh(FetchHttpClient.layer).pipe( + Layer.provide( + Layer.effect(FetchHttpClient.Fetch)( + Effect.acquireRelease( + Effect.sync(() => { + const options = { + allowH2: true, + keepAliveTimeout: 300_000, + keepAliveMaxTimeout: 300_000, + }; + // Match Node's proxy opt-in, including flags supplied through NODE_OPTIONS. + const proxyFlags = [ + ...(process.env.NODE_OPTIONS?.match(/--(?:no-)?use-env-proxy\b/g) ?? []), + ...process.execArgv, + ]; + const proxyFlag = proxyFlags.findLast( + (flag) => flag === "--use-env-proxy" || flag === "--no-use-env-proxy", + ); + const useProxy = + proxyFlag === undefined + ? process.env.NODE_USE_ENV_PROXY === "1" + : proxyFlag === "--use-env-proxy"; + return useProxy ? new Undici.EnvHttpProxyAgent(options) : new Undici.Agent(options); + }), + (dispatcher) => Effect.promise(() => dispatcher.destroy()), + ).pipe( + // Node's own fetch types and this undici's describe the same init and response, but name + // a different `Dispatcher`, so they meet through `unknown`. + Effect.map( + (dispatcher): typeof globalThis.fetch => + (input, init) => + Undici.fetch(input as Undici.RequestInfo, { + ...(init as unknown as Undici.RequestInit), + dispatcher, + }) as unknown as Promise, + ), + ), + ), + ), +); + /** The services the built-in drivers' packages own, plus the host port they all run against. */ export const layer = Layer.mergeAll( // `GitHubApi.layerWithDependencies` carries the quota reserve and rate-limit pause every GitHub @@ -57,4 +110,4 @@ export const layer = Layer.mergeAll( ForgejoCli.layer, GitCafeApi.layer.pipe(Layer.provideMerge(GitCafeCredentials.layer)), GitLabPullRequestCli.layer.pipe(Layer.provideMerge(GitLabCli.layer)), -).pipe(Layer.provideMerge(ServerSourceControlHost.layer)); +).pipe(Layer.provide(httpDriverClient), Layer.provideMerge(ServerSourceControlHost.layer)); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts index 8ad71188e187..cd39d96c3036 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts @@ -801,8 +801,9 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { yield* ask(); const again = yield* ask(); - assert.strictEqual(mockedExecute.mock.calls.length, 5); - // The second read goes straight to the pushes, and still answers with the head's blob. + assert.strictEqual(mockedExecute.mock.calls.length, 4); + // The second read goes straight to the pushes, finds the same push's changes already held, + // and still answers with the head's blob. expect(argsOfCall(3)).toContain("pullRequestIterations"); expect([...again.revisions]).toEqual([["README.md", "8f80"]]); }), diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index 78d31da360f5..9b94b032980e 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -1,5 +1,8 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; +import * as Cache from "effect/Cache"; +import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; import * as Semaphore from "effect/Semaphore"; import type { PullRequestCapabilities, PullRequestViewerPermissions } from "@t3tools/contracts"; @@ -54,6 +57,13 @@ export const MAX_DIFF_SPAWNS = 2 * DIFF_FILE_CONCURRENCY; /** How many pull requests' repository locations one provider remembers at once. */ export const LOCATION_CACHE_CAPACITY = 128; +/** + * How many pushes' change lists one provider remembers. A list never changes once its push lands, + * but can run to thousands of entries, so this holds a handful of reviews rather than a listing's + * worth. + */ +const ITERATION_CHANGES_CAPACITY = 32; + const CAPABILITIES: PullRequestCapabilities = { // Azure serves no patch of its own, so the one the Code tab reads is built here out of the // files an iteration changed and both sides of each of them. @@ -140,13 +150,42 @@ function toChangeRequest(pullRequest: AzureDevOpsPullRequest): ProviderChangeReq }; } +/** One pull request's pushes, in the terms Azure's iteration routes address them by. */ +interface IterationScope { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; +} + export const make = Effect.gen(function* () { const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; // Made once with the provider, which the registry builds once, so this is the whole build's // allowance rather than one request's. const diffSpawns = yield* Semaphore.make(MAX_DIFF_SPAWNS); + // Diff slices and context expansion read the same immutable blobs. Bound both count and size. + const itemContents = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", project = "", repository = "", commit = "", ...pathParts] = key.split("\n"); + const path = pathParts.join("\n"); + return diffSpawns.withPermits(1)( + cli.readItemContent({ cwd, location: { project, repository }, commit, path }), + ); + }, + { + capacity: 64, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value.contents.length <= 128 * 1024 + ? Duration.infinity + : Duration.zero, + }, + ); const readItemContent = (input: Parameters[0]) => - diffSpawns.withPermits(1)(cli.readItemContent(input)); + Cache.get( + itemContents, + [input.cwd, input.location.project, input.location.repository, input.commit, input.path].join( + "\n", + ), + ); const fail = (operation: string) => (error: AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCliError) => @@ -181,27 +220,142 @@ export const make = Effect.gen(function* () { */ const locations = new Map(); - const locationOf = (input: { readonly cwd: string; readonly number: number }) => { + /** + * Keeps where a read placed a pull request. A listing's rows only take spare room, so walking a + * long listing cannot evict the pull request being read. + */ + const rememberLocation = ( + cwd: string, + pullRequest: AzureDevOpsPullRequest, + options: { readonly evict: boolean }, + ) => { + if (pullRequest.location === null) return; + const key = `${cwd} ${pullRequest.number}`; + if (!locations.has(key) && locations.size >= LOCATION_CACHE_CAPACITY) { + if (!options.evict) return; + const oldest = locations.keys().next().value; + if (oldest !== undefined) locations.delete(oldest); + } + locations.delete(key); + locations.set(key, pullRequest.location); + }; + + const knownLocation = (input: { readonly cwd: string; readonly number: number }) => { const key = `${input.cwd} ${input.number}`; const held = locations.get(key); if (held !== undefined) { locations.delete(key); locations.set(key, held); - return Effect.succeed(held); } - return cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( - Effect.map((pullRequest) => { - const location = pullRequest.location; - if (location === null) return null; - if (locations.size >= LOCATION_CACHE_CAPACITY) { - const oldest = locations.keys().next().value; - if (oldest !== undefined) locations.delete(oldest); - } - locations.set(key, location); - return location; + return held; + }; + + /** + * `az repos pr show`, shared by the reads of one pull request that overlap: a review opening asks + * for its detail, conversation and diff at once, and each of them can start here. Nothing is kept + * once it answers, so a read that starts afterwards asks Azure again. + */ + const pullRequestReads = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", number = ""] = key.split("\n"); + return cli + .getPullRequest({ cwd, number: Number(number) }) + .pipe( + Effect.tap((pullRequest) => + Effect.sync(() => rememberLocation(cwd, pullRequest, { evict: true })), + ), + ); + }, + { capacity: LOCATION_CACHE_CAPACITY, timeToLive: () => Duration.zero }, + ); + + const readPullRequest = (input: { readonly cwd: string; readonly number: number }) => + Cache.get(pullRequestReads, `${input.cwd}\n${input.number}`); + + /** Run after a write, so a read asked for once it lands cannot join one that started before. */ + const forgetRead = (input: { readonly cwd: string; readonly number: number }) => + Cache.invalidate(pullRequestReads, `${input.cwd}\n${input.number}`); + + const locationOf = (input: { readonly cwd: string; readonly number: number }) => { + const held = knownLocation(input); + return held !== undefined + ? Effect.succeed(held) + : readPullRequest(input).pipe(Effect.map((pullRequest) => pullRequest.location)); + }; + + const iterationKey = (scope: IterationScope, iterationId?: number) => + [ + scope.cwd, + scope.location.project, + scope.location.repository, + scope.number, + ...(iterationId === undefined ? [] : [iterationId]), + ].join("\n"); + + const fromIterationKey = (key: string) => { + const [cwd = "", project = "", repository = "", number = "", iterationId = ""] = + key.split("\n"); + return { + cwd, + location: { project, repository }, + number: Number(number), + iterationId: Number(iterationId), + }; + }; + + const changesKey = (scope: IterationScope, iteration: AzureDevOpsIteration) => + [iterationKey(scope, iteration.id), iteration.headCommit, iteration.mergeBaseCommit].join("\n"); + + /** + * The pushes a pull request has had, shared by overlapping reads the way a pull request read is. + * Never kept: the newest push is what the list is read for, and a push adds one. + */ + const iterationReads = yield* Cache.makeWith( + (key: string) => cli.listIterations(fromIterationKey(key)), + { capacity: LOCATION_CACHE_CAPACITY, timeToLive: () => Duration.zero }, + ); + + /** + * One push by its id, which is all a diff cursor names. A push never changes once it lands, so + * the latest one is kept from every listing and a slice carrying on from it lists nothing. + */ + const iterationsById = yield* Cache.makeWith( + (key: string) => { + const scope = fromIterationKey(key); + return Cache.get(iterationReads, iterationKey(scope)).pipe( + Effect.map((iterations) => + iterations.find((candidate) => candidate.id === scope.iterationId), + ), + ); + }, + { + capacity: LOCATION_CACHE_CAPACITY, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value !== undefined ? Duration.infinity : Duration.zero, + }, + ); + + /** + * What one push changed, against the merge base. Never changes once the push lands, so the + * detail's file count, every slice of the diff and the viewed marks share one read of it. + */ + const iterationChanges = yield* Cache.makeWith( + (key: string) => cli.listIterationChanges(fromIterationKey(key)), + { + capacity: ITERATION_CHANGES_CAPACITY, + timeToLive: (exit) => (Exit.isSuccess(exit) ? Duration.infinity : Duration.zero), + }, + ); + + const listIterations = (scope: IterationScope) => + Cache.get(iterationReads, iterationKey(scope)).pipe( + Effect.tap((iterations) => { + const latest = iterations.at(-1); + return latest === undefined + ? Effect.void + : Cache.set(iterationsById, iterationKey(scope, latest.id), latest); }), ); - }; /** * Everything a diff read needs before it can ask for a file: where the repository lives, and @@ -212,7 +366,7 @@ export const make = Effect.gen(function* () { Effect.gen(function* () { const location = yield* locationOf(input); if (location === null) return null; - const iterations = yield* cli.listIterations({ + const iterations = yield* listIterations({ cwd: input.cwd, location, number: input.number, @@ -269,24 +423,16 @@ export const make = Effect.gen(function* () { * reported against the merge base rather than against the push before it, so the newest one is * the whole of the change rather than the last slice of it. */ - const listLatestChanges = (input: { - readonly cwd: string; - readonly location: AzureDevOpsRepositoryLocation; - readonly number: number; - readonly iterations: ReadonlyArray; - }) => { + const listLatestChanges = ( + input: IterationScope & { readonly iterations: ReadonlyArray }, + ) => { const latest = input.iterations.at(-1); return latest === undefined ? Effect.succeed({ changes: [], truncated: false, } as AzureDevOpsPullRequestCli.AzureDevOpsIterationChanges) - : cli.listIterationChanges({ - cwd: input.cwd, - location: input.location, - number: input.number, - iterationId: latest.id, - }); + : Cache.get(iterationChanges, changesKey(input, latest)); }; const provider: PullRequestProviderApi = { @@ -314,6 +460,13 @@ export const make = Effect.gen(function* () { }) .pipe( Effect.mapError(fail("listChangeRequests")), + // A row states where its pull request lives, so opening one from the list needs no + // read of the pull request before its conversation or its pushes can be asked for. + Effect.tap((batch) => + Effect.sync(() => { + for (const row of batch.items) rememberLocation(input.cwd, row, { evict: false }); + }), + ), Effect.map((batch) => ({ items: batch.items.map(toChangeRequest), truncated: batch.truncated, @@ -327,7 +480,7 @@ export const make = Effect.gen(function* () { // The polled path a linked thread's row stays live on: one `az` read, no iterations or // changes behind it, since the file count that would cost is not shown here. getChangeRequestSummary: (input) => - cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( + readPullRequest(input).pipe( Effect.mapError(fail("getChangeRequestSummary")), Effect.map((pullRequest): ProviderChangeRequestSummary => ({ number: pullRequest.number, @@ -345,27 +498,43 @@ export const make = Effect.gen(function* () { })), ), + // A hover card is the pull request read alone, where the detail it would otherwise fall back + // to reads its pushes and their changes behind it for a file count the card never shows. + getChangeRequestPreview: (input) => + readPullRequest(input).pipe( + Effect.mapError(fail("getChangeRequestPreview")), + Effect.map((pullRequest) => ({ + number: pullRequest.number, + title: pullRequest.title, + url: pullRequest.url, + author: pullRequest.author, + state: pullRequest.state, + isDraft: pullRequest.isDraft, + createdAt: pullRequest.createdAt, + })), + ), + getChangeRequest: (input) => Effect.gen(function* () { - const pullRequest = yield* cli.getPullRequest({ cwd: input.cwd, number: input.number }); - const location = pullRequest.location; // The file count is two reads past the pull request itself, and it is the only thing // riding on them, so a failure leaves it unknown rather than losing the whole detail. - const changedFiles = - location === null - ? 0 - : yield* cli.listIterations({ cwd: input.cwd, location, number: input.number }).pipe( - Effect.flatMap((iterations) => - listLatestChanges({ - cwd: input.cwd, - location, - number: input.number, - iterations, - }), - ), - Effect.map((listed) => listed.changes.length), - Effect.orElseSucceed(() => 0), - ); + const countFiles = (location: AzureDevOpsRepositoryLocation) => { + const scope = { cwd: input.cwd, location, number: input.number }; + return listIterations(scope).pipe( + Effect.flatMap((iterations) => listLatestChanges({ ...scope, iterations })), + Effect.map((listed) => listed.changes.length), + Effect.orElseSucceed(() => 0), + ); + }; + // Once anything has placed the pull request, its pushes are read alongside it rather + // than after it. + const known = knownLocation(input); + const [pullRequest, knownCount] = yield* Effect.all( + [readPullRequest(input), known === undefined ? Effect.succeed(null) : countFiles(known)], + { concurrency: 2 }, + ); + const location = pullRequest.location; + const changedFiles = knownCount ?? (location === null ? 0 : yield* countFiles(location)); const detail: ProviderChangeRequestDetail = { ...toChangeRequest(pullRequest), body: pullRequest.body, @@ -385,21 +554,24 @@ export const make = Effect.gen(function* () { }).pipe(Effect.mapError(fail("getChangeRequest"))), getChangeRequestActivity: (input) => - cli.getPullRequest({ cwd: input.cwd, number: input.number }).pipe( - Effect.mapError(fail("getChangeRequestActivity")), - Effect.flatMap((pullRequest) => - (pullRequest.location === null + locationOf(input).pipe( + Effect.flatMap((location) => + (location === null ? Effect.succeed({ comments: [], truncated: true }) - : cli - .listThreads({ - cwd: input.cwd, - location: pullRequest.location, - number: input.number, - }) - .pipe( - Effect.map((comments) => ({ comments, truncated: false })), - Effect.orElseSucceed(() => ({ comments: [], truncated: true })), - ) + : cli.listThreads({ cwd: input.cwd, location, number: input.number }).pipe( + Effect.map((comments) => ({ comments, truncated: false })), + // Only what is this conversation's problem. A signed-out CLI, a rate limit or no + // `az` at all is the host failing, and with a remembered location there is no pull + // request read ahead of this one left to report it. + Effect.catchTags({ + AzureDevOpsPullRequestNotFoundError: () => + Effect.succeed({ comments: [], truncated: true }), + AzureDevOpsCommandFailedError: () => + Effect.succeed({ comments: [], truncated: true }), + AzureDevOpsPullRequestReadError: () => + Effect.succeed({ comments: [], truncated: true }), + }), + ) ).pipe( Effect.map((conversation): ProviderChangeRequestActivity => ({ comments: conversation.comments, @@ -410,6 +582,7 @@ export const make = Effect.gen(function* () { })), ), ), + Effect.mapError(fail("getChangeRequestActivity")), ), // No request at all: Azure has nothing to say about the viewer that a pull request read can @@ -420,22 +593,18 @@ export const make = Effect.gen(function* () { // the Code tab has nothing to scope itself to and always asks for the whole change. getDiff: (input) => Effect.gen(function* () { - const scope = yield* diffScope(input); - if (scope === null) return EMPTY_DIFF_SLICE; + const location = yield* locationOf(input); + if (location === null) return EMPTY_DIFF_SLICE; + const scope = { cwd: input.cwd, location, number: input.number }; const cursor = parseAzureDevOpsDiffCursor(input.cursor); // Reading on stays with the push the first slice was taken against. A push landing // mid-read would otherwise renumber the files and hand the reader one twice, or none. const iteration = cursor === null - ? scope.iterations.at(-1) - : scope.iterations.find((candidate) => candidate.id === cursor.iterationId); + ? (yield* listIterations(scope)).at(-1) + : yield* Cache.get(iterationsById, iterationKey(scope, cursor.iterationId)); if (iteration === undefined) return EMPTY_DIFF_SLICE; - const listed = yield* cli.listIterationChanges({ - cwd: input.cwd, - location: scope.location, - number: input.number, - iterationId: iteration.id, - }); + const listed = yield* Cache.get(iterationChanges, changesKey(scope, iteration)); const changes = listed.changes; const sections: string[] = []; @@ -589,7 +758,7 @@ export const make = Effect.gen(function* () { action: input.action, ...(input.mergeMethod === undefined ? {} : { mergeMethod: input.mergeMethod }), }) - .pipe(Effect.mapError(fail("runAction"))), + .pipe(Effect.ensuring(forgetRead(input)), Effect.mapError(fail("runAction"))), updateChangeRequest: (input) => cli @@ -599,7 +768,7 @@ export const make = Effect.gen(function* () { title: input.title, body: input.body, }) - .pipe(Effect.mapError(fail("updateChangeRequest"))), + .pipe(Effect.ensuring(forgetRead(input)), Effect.mapError(fail("updateChangeRequest"))), // Never called: `capabilities.reviewers.listCandidates` is false, and the service refuses the // list without it. @@ -623,7 +792,7 @@ export const make = Effect.gen(function* () { reviewers: input.reviewers.map((reviewer) => reviewer.id), requested: input.requested, }) - .pipe(Effect.mapError(fail("setReviewerRequest"))), + .pipe(Effect.ensuring(forgetRead(input)), Effect.mapError(fail("setReviewerRequest"))), // Never called: `capabilities.comment` is false, and the service refuses a comment without it. comment: () => unsupported("comment"), diff --git a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts index 8d8feaba8150..5679380ffc64 100644 --- a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts +++ b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.test.ts @@ -1015,6 +1015,25 @@ layer("BitbucketPullRequestApi.layer", (it) => { }), ); + it.effect("still fails the permission read on a failure that is not the removed endpoint", () => + Effect.gen(function* () { + mockedRequest.mockReturnValue( + Effect.fail( + new BitbucketApi.BitbucketResponseError({ + operation: "request", + status: 401, + responseBodyLength: 0, + }), + ), + ); + const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; + + const error = yield* Effect.flip(api.getRepositoryPermission({ repository: "acme/web" })); + + assert.strictEqual(error._tag, "BitbucketResponseError"); + }), + ); + it.effect( "reads a removed permissions endpoint as granted rather than failing the merge on it", () => @@ -1033,28 +1052,12 @@ layer("BitbucketPullRequestApi.layer", (it) => { const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; assert.isTrue(yield* api.getRepositoryPermission({ repository: "acme/web" })); + // Gone for good, so the next check does not ask again. + assert.isTrue(yield* api.getRepositoryPermission({ repository: "acme/web" })); + expect(mockedRequest).toHaveBeenCalledTimes(1); }), ); - it.effect("still fails the permission read on a failure that is not the removed endpoint", () => - Effect.gen(function* () { - mockedRequest.mockReturnValue( - Effect.fail( - new BitbucketApi.BitbucketResponseError({ - operation: "request", - status: 401, - responseBodyLength: 0, - }), - ), - ); - const api = yield* BitbucketPullRequestApi.BitbucketPullRequestApi; - - const error = yield* Effect.flip(api.getRepositoryPermission({ repository: "acme/web" })); - - assert.strictEqual(error._tag, "BitbucketResponseError"); - }), - ); - it.effect("reads the workspace's people and marks whoever is already a reviewer", () => Effect.gen(function* () { mockedRequest diff --git a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts index 6dd04a4a0773..46bd02a73df4 100644 --- a/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts +++ b/packages/source-control-bitbucket/src/server/BitbucketPullRequestApi.ts @@ -606,6 +606,9 @@ export const make = Effect.gen(function* () { }, ); + /** Set by `getRepositoryPermission` once Bitbucket answers that its endpoint is gone. */ + let repositoryPermissionRemoved = false; + return BitbucketPullRequestApi.of({ getViewer: () => bitbucket.request({ method: "GET", url: "/user" }).pipe( @@ -665,17 +668,29 @@ export const make = Effect.gen(function* () { // permission being refused, so it is read the same way an unreachable read already is // elsewhere — as a permission that could not be learned, which grants rather than blocks, and // leaves the actual merge or write to say why if the account may not do it. Any other failure - // (a bad token, a network fault, an unreadable body) still fails as it did before. + // (a bad token, a network fault, an unreadable body) still fails as it did before. Once the + // configured Bitbucket has answered 410 it is not asked again while this server runs. getRepositoryPermission: (input) => withRepository(input.repository, () => - readPage({ - operation: "getRepositoryPermission", - url: `/user/permissions/repositories?q=${encodeURIComponent( - `repository.full_name="${filterLiteral(input.repository.trim())}"`, - )}`, - decode: decodeRepositoryPermissionJson, - }), - ).pipe(Effect.catchIf(isRepositoryPermissionRemovedError, () => Effect.succeed(true))), + Effect.suspend(() => + repositoryPermissionRemoved + ? Effect.succeed(true) + : readPage({ + operation: "getRepositoryPermission", + url: `/user/permissions/repositories?q=${encodeURIComponent( + `repository.full_name="${filterLiteral(input.repository.trim())}"`, + )}`, + decode: decodeRepositoryPermissionJson, + }), + ), + ).pipe( + Effect.catchIf(isRepositoryPermissionRemovedError, () => + Effect.sync(() => { + repositoryPermissionRemoved = true; + return true; + }), + ), + ), getPullRequestDiff: pullRequestDiff, diff --git a/packages/source-control-bitbucket/src/server/BitbucketPullRequestProvider.ts b/packages/source-control-bitbucket/src/server/BitbucketPullRequestProvider.ts index 3d26d5401602..1fa565902672 100644 --- a/packages/source-control-bitbucket/src/server/BitbucketPullRequestProvider.ts +++ b/packages/source-control-bitbucket/src/server/BitbucketPullRequestProvider.ts @@ -9,6 +9,7 @@ import { type ProviderChangeRequest, type ProviderChangeRequestActivity, type ProviderChangeRequestDetail, + type ProviderChangeRequestSummary, type PullRequestProviderApi, } from "@t3tools/source-control-core/server/PullRequestProvider"; import type { BitbucketPullRequest } from "./bitbucketPullRequestJson.ts"; @@ -233,6 +234,54 @@ export const make = Effect.gen(function* () { ); }, + // Everything a hover card shows is on the pull request itself. + getChangeRequestPreview: (input) => + api.getPullRequest({ repository: input.repository, number: input.number }).pipe( + Effect.mapError(fail("getChangeRequestPreview")), + Effect.map((pullRequest) => ({ + number: pullRequest.number, + title: pullRequest.title, + url: pullRequest.url, + author: pullRequest.author, + state: pullRequest.state, + isDraft: pullRequest.isDraft, + createdAt: pullRequest.createdAt, + })), + ), + + // The detail's line counts and conflict state, which a linked thread shows, without its + // checks and permission reads. + getChangeRequestSummary: (input) => { + const target = { repository: input.repository, number: input.number }; + return Effect.all( + [ + api.getPullRequest(target), + api.getDiffStat(target), + recoverRead(api.getMergeability(target), "unknown" as const), + ], + { concurrency: 3 }, + ).pipe( + Effect.mapError(fail("getChangeRequestSummary")), + Effect.map(([pullRequest, diffStat, mergeability]): ProviderChangeRequestSummary => ({ + number: pullRequest.number, + title: pullRequest.title, + url: pullRequest.url, + author: pullRequest.author, + headBranch: pullRequest.headBranch, + baseBranch: pullRequest.baseBranch, + state: pullRequest.state, + isDraft: pullRequest.isDraft, + mergeability, + additions: diffStat.additions, + deletions: diffStat.deletions, + changedFiles: diffStat.changedFiles, + mergedAt: null, + closedAt: null, + updatedAt: pullRequest.updatedAt, + })), + ); + }, + getChangeRequestActivity: (input) => { const target = { repository: input.repository, number: input.number }; return Effect.all( diff --git a/packages/source-control-forgejo/src/server/ForgejoCli.ts b/packages/source-control-forgejo/src/server/ForgejoCli.ts index 90501a4591ca..9c9b2e6936c6 100644 --- a/packages/source-control-forgejo/src/server/ForgejoCli.ts +++ b/packages/source-control-forgejo/src/server/ForgejoCli.ts @@ -1,3 +1,4 @@ +import * as Cache from "effect/Cache"; import * as Context from "effect/Context"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; @@ -109,6 +110,17 @@ export interface ForgejoRepositoryInput { readonly host?: string; } +/** What resolving a target reads from a `ForgejoRepositoryInput`, flat so it can key a cache. */ +interface ForgejoTargetInput { + readonly cwd: string; + readonly reference?: string | undefined; + readonly repository?: string | undefined; + readonly host?: string | undefined; + readonly remoteUrl?: string | undefined; + readonly providerBaseUrl?: string | undefined; + readonly requestedHost?: string | undefined; +} + export interface ForgejoRepository { readonly command?: "fj" | "tea"; readonly login: string; @@ -425,15 +437,24 @@ export const make = Effect.gen(function* () { ), ); - const authenticateFj = Effect.fn("ForgejoCli.authenticateFj")(function* ( + type FjLogin = Pick; + + /** The stored token when fj checked it within the last 30s, read without taking `authLock`. */ + const checkedToken = Effect.fn("ForgejoCli.checkedToken")(function* ( cwd: string, - login: typeof ForgejoLoginSchema.Type, + login: FjLogin, ) { - const keys = yield* readKeys(cwd); - const token = keys.hosts[login.name]?.token; + const token = (yield* readKeys(cwd)).hosts[login.name]?.token; const now = yield* Clock.currentTimeMillis; const cached = authenticated.get(login.url); - if (token && cached?.token === token && now - cached.time < 30_000) return token; + return token && cached?.token === token && now - cached.time < 30_000 ? token : undefined; + }); + + const renewFj = Effect.fn("ForgejoCli.renewFj")(function* (cwd: string, login: FjLogin) { + // Another request may have renewed this login while this one waited for the lock. + const checked = yield* checkedToken(cwd, login); + if (checked) return checked; + const now = yield* Clock.currentTimeMillis; yield* execute({ command: "fj", cwd, args: ["--host", login.url, "whoami"] }); // fj owns OAuth renewal. Re-read the file after it has refreshed an expired token. const refreshed = (yield* readKeys(cwd)).hosts[login.name]?.token; @@ -448,6 +469,12 @@ export const make = Effect.gen(function* () { return refreshed; }, authLock.withPermits(1)); + // Only a renewal serialises; requests with a recently checked token run concurrently. + const authenticateFj = (cwd: string, login: FjLogin) => + checkedToken(cwd, login).pipe( + Effect.flatMap((token) => (token ? Effect.succeed(token) : renewFj(cwd, login))), + ); + const getAccount: NonNullable = Effect.fn( "ForgejoCli.getAccount", )(function* (input) { @@ -481,17 +508,17 @@ export const make = Effect.gen(function* () { }); const resolveTarget = Effect.fn("ForgejoCli.resolveTarget")(function* ( - input: ForgejoRepositoryInput, + input: ForgejoTargetInput, hostOnly = false, ) { const referenceRemote = input.reference ? parseForgejoRemote(input.reference) : null; - let remoteUrl = [input.reference, input.repository, input.context?.remoteUrl].find( + let remoteUrl = [input.reference, input.repository, input.remoteUrl].find( (value) => value && parseForgejoRemote(value), ); let remote = referenceRemote ?? (input.repository ? parseForgejoRemote(input.repository) : null) ?? - (input.context ? parseForgejoRemote(input.context.remoteUrl) : null); + (input.remoteUrl ? parseForgejoRemote(input.remoteUrl) : null); if (!remote && (!input.repository || input.host)) { const result = yield* process .run({ @@ -547,13 +574,13 @@ export const make = Effect.gen(function* () { ssh: false, path: remote?.path ?? "", }; - const schemeRemoteUrl = remote?.ssh ? input.context?.provider.baseUrl : remoteUrl; + const schemeRemoteUrl = remote?.ssh ? input.providerBaseUrl : remoteUrl; const fjLogins = yield* listLogins({ cwd: input.cwd, command: "fj", ...(schemeRemoteUrl ? { remoteUrl: schemeRemoteUrl } : {}), }); - const requestedHost = input.host ?? input.context?.requestedHost; + const requestedHost = input.host ?? input.requestedHost; const matchHostOnly = hostOnly || (!!input.host && !remote?.path); const selectLogin = (logins: ReturnType) => remote @@ -629,11 +656,39 @@ export const make = Effect.gen(function* () { }); return { command, login: login.name, repository, baseUrl: login.url.replace(/\/+$/, "") }; }); - const resolveRepository = (input: ForgejoRepositoryInput) => resolveTarget(input); + // Share pending resolution, but reselect the login after it finishes so account and remote + // changes apply to the next request without an extra identity check. + const targets = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", reference, repository, host, remoteUrl, baseUrl, requestedHost, scope] = key + .split("\n") + .map((value) => value || undefined); + return resolveTarget( + { cwd, reference, repository, host, remoteUrl, providerBaseUrl: baseUrl, requestedHost }, + scope === "host", + ); + }, + { capacity: 64, timeToLive: () => 0 }, + ); + const targetKey = (input: ForgejoRepositoryInput, hostOnly = false) => + [ + input.cwd, + input.reference, + input.repository, + input.host, + input.context?.remoteUrl, + input.context?.provider.baseUrl, + input.context?.requestedHost, + hostOnly ? "host" : "", + ].join("\n"); + const resolveRepository = (input: ForgejoRepositoryInput) => Cache.get(targets, targetKey(input)); const api = Effect.fn("ForgejoCli.api")(function* (input: ForgejoApiInput) { - const repository = yield* resolveTarget( - input, - input.path.replace(/^\/+/, "") === "user" && (!input.method || input.method === "GET"), + const repository = yield* Cache.get( + targets, + targetKey( + input, + input.path.replace(/^\/+/, "") === "user" && (!input.method || input.method === "GET"), + ), ); const stdin = input.body === undefined diff --git a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts index d55c205240dc..ed5fbbff4087 100644 --- a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts +++ b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts @@ -1,4 +1,5 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; +import * as Cache from "effect/Cache"; import * as Effect from "effect/Effect"; import * as Schema from "effect/Schema"; import * as Result from "effect/Result"; @@ -148,24 +149,47 @@ export const make = Effect.gen(function* () { } return { items, truncated: true }; }); - const write = (input: ForgejoCli.ForgejoApiInput) => request(input).pipe(Effect.asVoid); + // Share pending reads only: credentials and repository permissions can change between requests. + const pulls = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", repository = "", host = "", number = ""] = key.split("\n"); + const ref = { cwd, repository, host, number: Number(number) }; + return read({ ...ref, path: pullPath(ref) }, ForgejoPullRequest); + }, + { capacity: 64, timeToLive: () => 0 }, + ); const getPull = (input: ProviderRepositoryRef & { readonly number: number }) => - read( - { - cwd: input.cwd, - repository: input.repository, - host: input.host, - path: pullPath(input), - }, - ForgejoPullRequest, + Cache.get(pulls, [input.cwd, input.repository, input.host, input.number].join("\n")); + const write = (input: ForgejoCli.ForgejoApiInput) => + request(input).pipe( + Effect.onExit(() => Cache.invalidateAll(pulls)), + Effect.asVoid, ); + const repos = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", repository = "", host = ""] = key.split("\n"); + const ref = { cwd, repository, host }; + return read({ ...ref, path: repoPath(ref) }, ForgejoRepository); + }, + { capacity: 32, timeToLive: () => 0 }, + ); + const viewers = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", repository, host] = key.split("\n"); + return read( + { cwd, ...(repository ? { repository } : {}), ...(host ? { host } : {}), path: "user" }, + ForgejoUser, + ).pipe(Effect.map((user) => user.login)); + }, + { capacity: 32, timeToLive: () => 0 }, + ); const getRepo = (input: ProviderRepositoryRef) => - read({ ...input, path: repoPath(input) }, ForgejoRepository); + Cache.get(repos, [input.cwd, input.repository, input.host].join("\n")); const getViewer = (input: { readonly cwd: string; readonly repository?: string; readonly host?: string; - }) => read({ ...input, path: "user" }, ForgejoUser).pipe(Effect.map((user) => user.login)); + }) => Cache.get(viewers, [input.cwd, input.repository, input.host].join("\n")); const permissions = ( repo: typeof ForgejoRepository.Type, pr: typeof ForgejoPullRequest.Type, diff --git a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts index 468d1402eb1d..0985d462a236 100644 --- a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts +++ b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts @@ -1,6 +1,8 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; import { assert, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; +import * as Deferred from "effect/Deferred"; +import * as Fiber from "effect/Fiber"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; @@ -31,6 +33,227 @@ const requestJson = (request: HttpClientRequest.HttpClientRequest): unknown => { }; const encodeJsonEffect = Schema.encodeEffect(Schema.fromJsonString(Schema.Unknown)); +it.effect("refreshes Forgejo viewer permissions immediately after credentials change", () => { + let account = "alice"; + const requests: string[] = []; + const renewals: string[] = []; + return Effect.gen(function* () { + const cli = yield* ForgejoCli.make; + const provider = yield* ForgejoPullRequestProvider.make.pipe( + Effect.provideService(ForgejoCli.ForgejoCli, cli), + ); + const input = { cwd: "/repo", repository: "acme/web", host: "forgejo.test", number: 42 }; + const alice = yield* provider.getViewerPermissions(input); + assert.deepStrictEqual(alice.verdicts, ["comment"]); + assert.include(alice.actions, "merge"); + assert.strictEqual(alice.labels, true); + account = "bob"; + const bob = yield* provider.getViewerPermissions(input); + assert.deepStrictEqual(bob.verdicts, ["comment", "approve", "request-changes"]); + assert.deepStrictEqual(bob.actions, []); + assert.strictEqual(bob.labels, false); + assert.strictEqual(bob.requestReviewers, false); + assert.strictEqual(requests.length, 6); + assert.deepStrictEqual(renewals, ["alice", "bob"]); + }).pipe( + Effect.provideService( + FileSystem.FileSystem, + FileSystem.makeNoop({ + exists: () => Effect.succeed(true), + readFileString: () => + Effect.succeed( + encodeJson({ hosts: { "forgejo.test": { type: "Application", token: account } } }), + ), + }), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make((request) => { + requests.push(request.url); + assert.strictEqual(request.headers.authorization, `token ${account}`); + const response = request.url.endsWith("/user") + ? { login: account } + : request.url.endsWith("/pulls/42") + ? { + number: 42, + title: "Permissions", + body: "", + html_url: "https://forgejo.test/acme/web/pulls/42", + user: { login: "alice" }, + state: "open", + merged: false, + head: { ref: "feature", sha: "head", repo: null }, + base: { ref: "main", sha: "base", repo: null }, + created_at: "2026-09-16T00:00:00Z", + updated_at: "2026-09-16T00:00:00Z", + closed_at: null, + merged_at: null, + labels: [], + } + : { full_name: "acme/web", permissions: { push: account === "alice", admin: false } }; + return Effect.succeed( + HttpClientResponse.fromWeb(request, new Response(encodeJson(response))), + ); + }), + ), + Effect.provide( + TestSourceControlHost.layer({ + process: { + run: (input) => { + if (input.command === "git") + return Effect.succeed( + processOutput("origin\thttps://forgejo.test/acme/web.git (fetch)"), + ); + assert.strictEqual(input.command, "fj"); + assert.deepStrictEqual(input.args, ["--host", "https://forgejo.test", "whoami"]); + renewals.push(account); + return Effect.succeed(processOutput("")); + }, + }, + }), + ), + ); +}); + +it.effect("reselects the current tea login between API requests", () => { + let account = "alice"; + const selected: string[] = []; + return Effect.gen(function* () { + const cli = yield* ForgejoCli.make; + const input = { cwd: "/repo", repository: "https://forgejo.test/acme/web", path: "user" }; + assert.strictEqual((yield* cli.api(input)).stdout, '{"login":"alice"}'); + account = "bob"; + assert.strictEqual((yield* cli.api(input)).stdout, '{"login":"bob"}'); + assert.deepStrictEqual(selected, ["alice", "bob"]); + }).pipe( + Effect.provideService( + FileSystem.FileSystem, + FileSystem.makeNoop({ exists: () => Effect.succeed(false) }), + ), + Effect.provideService( + HttpClient.HttpClient, + HttpClient.make(() => Effect.die("tea owns HTTP")), + ), + Effect.provide( + TestSourceControlHost.layer({ + process: { + run: (input) => { + assert.strictEqual(input.command, "tea"); + if (input.args[0] === "login") + return Effect.succeed( + processOutput( + encodeJson([ + { name: account, url: "https://forgejo.test", user: account, default: "true" }, + ]), + ), + ); + assert.strictEqual(input.args[0], "api"); + assert.strictEqual(input.args[input.args.indexOf("--login") + 1], account); + selected.push(account); + return Effect.succeed( + processOutput(encodeJson({ login: account }), { stderr: "HTTP/1.1 200 OK\n" }), + ); + }, + }, + }), + ), + ); +}); + +it.effect("shares pending Forgejo pull reads and refreshes after writes and failures", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + const paths: string[] = []; + let pullReads = 0; + let state = "open"; + let reject = false; + const provider = yield* ForgejoPullRequestProvider.make.pipe( + Effect.provide( + Layer.mock(ForgejoCli.ForgejoCli)({ + api: (input) => + Effect.gen(function* () { + paths.push(input.path); + if (input.method === "PATCH") { + state = "closed"; + return processOutput("{}"); + } + if (input.path.endsWith("/pulls/42")) { + pullReads++; + if (reject) + return yield* new ForgejoCli.ForgejoCliError({ + command: "tea", + cwd: input.cwd, + reason: "rate-limit", + detail: "limited", + }); + const response = { + number: 42, + title: "Shared pull", + body: "", + html_url: "https://forgejo.test/acme/web/pulls/42", + user: { login: "alice" }, + state, + merged: false, + head: { ref: "feature", sha: state === "open" ? "before" : "after", repo: null }, + base: { ref: "main", sha: "base", repo: null }, + created_at: "2026-09-16T00:00:00Z", + updated_at: "2026-09-16T00:00:00Z", + closed_at: null, + merged_at: null, + labels: [], + }; + if (pullReads === 1) { + yield* Deferred.succeed(started, undefined); + yield* Deferred.await(release); + } + return processOutput(encodeJson(response)); + } + return processOutput( + input.path === "user" + ? '{"login":"bob"}' + : input.path.includes("/statuses/") + ? "[]" + : '{"full_name":"acme/web","permissions":{"push":true,"admin":false}}', + ); + }), + }), + ), + ); + const summary = provider.getChangeRequestSummary; + const checks = provider.getChangeRequestChecks; + if (!summary || !checks) return yield* Effect.die("narrow reads missing"); + const input = { cwd: "/repo", repository: "acme/web", host: "forgejo.test", number: 42 }; + const reads = yield* Effect.all( + [ + summary(input), + checks(input), + provider.getViewerPermissions(input), + provider.getChangeRequest(input), + ], + { concurrency: "unbounded" }, + ).pipe(Effect.forkChild); + yield* Deferred.await(started); + yield* Effect.yieldNow; + assert.strictEqual(pullReads, 1); + yield* provider.runAction({ ...input, action: "close" }); + const updated = yield* checks(input); + assert.strictEqual(updated.state, "closed"); + assert.strictEqual(pullReads, 2); + assert.include(paths, "repos/acme/web/statuses/after?sort=recentupdate&limit=50&page=1"); + yield* Deferred.succeed(release, undefined); + const original = yield* Fiber.join(reads); + assert.strictEqual(original[0].state, "open"); + reject = true; + const failed = yield* summary(input).pipe(Effect.result); + assert.strictEqual(failed._tag, "Failure"); + assert.strictEqual(pullReads, 3); + reject = false; + assert.strictEqual((yield* summary(input)).state, "closed"); + assert.strictEqual(pullReads, 4); + }), +); + it.effect("submits a Forgejo review without sending its summary in the preliminary GET", () => { const methods: string[] = []; const reviewClient = HttpClient.make((request) => { diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts index 4bbd54b73cf9..3d5f5ddbead1 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts @@ -1,5 +1,8 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; +import * as Cache from "effect/Cache"; +import * as Duration from "effect/Duration"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; import * as Option from "effect/Option"; import * as Schema from "effect/Schema"; import { randomUuidV4 } from "@t3tools/provider-core/server/randomUuid"; @@ -55,6 +58,13 @@ const DIFF_FILES_BATCH = 64; const LINE_STATS_MAX_FILES = DIFF_FILES_BATCH * 4; /** Structural `/changes` pages, 500 files each, read to count a pull request's files. */ const MAX_CHANGE_PAGES = 20; +/** + * How long one pull read answers every caller, so a panel's detail, activity, diff and stack + * reads, or a write's permission check and its own read, ask GitCafe once. + */ +const PULL_TTL = Duration.seconds(1); +/** A viewer's actor id is stable, so involvement filters skip `/filter-options` for this long. */ +const ACTOR_TTL = Duration.minutes(10); const chunk = (items: ReadonlyArray, size: number): Array> => Array.from({ length: Math.ceil(items.length / size) }, (_, index) => @@ -136,7 +146,8 @@ const StackOutcome = Schema.Struct({ id: Schema.String, state: Schema.String }); /** Where a stack operation stopped short; anything else is underway or done. */ const FAILED_STACK_STATES = new Set(["failed", "stopped", "cancelled", "reconciliation_required"]); -type PullRef = ProviderRepositoryRef & { readonly number: number }; +type RepositoryRef = Pick; +type PullRef = RepositoryRef & { readonly number: number }; type Paged = { readonly items: ReadonlyArray; readonly next: string | null }; /** The heads a reader saw are still every unmerged layer the action moves. */ @@ -188,7 +199,7 @@ export const make = Effect.gen(function* () { /** The repository's API root, refused before anything is sent for a host GitCafe is not on. */ const repositoryPath = ( operation: string, - input: ProviderRepositoryRef, + input: RepositoryRef, ): Effect.Effect => GitCafeHosts.isGitCafeHost(input.host) && GitCafeHosts.gitCafeRepositoryPath(input.repository) === input.repository @@ -237,11 +248,40 @@ export const make = Effect.gen(function* () { const pullPath = (operation: string, input: PullRef) => repositoryPath(operation, input).pipe(Effect.map((base) => `${base}/pulls/${input.number}`)); + const pullKey = (input: PullRef) => `${input.host}\n${input.repository}\n${input.number}`; + // A failed read is not held, so the next caller asks GitCafe again. + const pulls = yield* Cache.makeWith( + (key: string) => { + const [host = "", repository = "", number = ""] = key.split("\n"); + return read( + "readPull", + { host }, + { path: `/repos/${repository}/pulls/${number}` }, + Json.GitCafePullDetail, + ); + }, + { capacity: 64, timeToLive: (exit) => (Exit.isSuccess(exit) ? PULL_TTL : Duration.zero) }, + ); + const readPull = (operation: string, input: PullRef) => - pullPath(operation, input).pipe( - Effect.flatMap((path) => read(operation, input, { path }, Json.GitCafePullDetail)), + repositoryPath(operation, input).pipe( + Effect.flatMap(() => Cache.get(pulls, pullKey(input))), + // One read can answer several callers, so each failure is reported as its caller's. + Effect.mapError( + (error) => + new PullRequestProviderError({ + provider: error.provider, + operation, + reason: error.reason, + detail: error.detail, + ...(error.cause === undefined ? {} : { cause: error.cause }), + }), + ), ); + /** Drops a pull's shared read once a write on it settles, whatever the outcome. */ + const forgetPull = (input: PullRef) => Cache.invalidate(pulls, pullKey(input)); + /** Every page of a cursor list, up to `MAX_PAGES`; `next` stays set when more remained. */ const readPages = , unknown, never, never>>( operation: string, @@ -278,11 +318,13 @@ export const make = Effect.gen(function* () { /** * The stack a pull request belongs to, with each unmerged layer's head. GitCafe's stack lists no - * heads, and they are what a land or restack is fenced on, so each open layer's pull is read. + * heads, and they are what a land or restack is fenced on, so each open layer's pull is read; + * `fresh` skips the shared pull reads for that fence. */ const readStack = Effect.fn("GitCafePullRequestProvider.readStack")(function* ( operation: string, input: PullRef, + fresh = false, ) { const path = yield* pullPath(operation, input); const { stack } = yield* read( @@ -294,14 +336,16 @@ export const make = Effect.gen(function* () { if (stack === null) return null; const members = yield* Effect.forEach( stack.members, - (member) => - member.state === "merged" - ? Effect.succeed(member) - : readPull(operation, { ...input, number: member.pullRequestNumber }).pipe( - Effect.map((pull) => - pull.headOid == null ? member : { ...member, headOid: pull.headOid }, - ), - ), + (member) => { + if (member.state === "merged") return Effect.succeed(member); + const layer = { ...input, number: member.pullRequestNumber }; + return (fresh ? forgetPull(layer) : Effect.void).pipe( + Effect.andThen(readPull(operation, layer)), + Effect.map((pull) => + pull.headOid == null ? member : { ...member, headOid: pull.headOid }, + ), + ); + }, { concurrency: 4 }, ); return { ...stack, members }; @@ -344,7 +388,7 @@ export const make = Effect.gen(function* () { const operation = land ? "landStackThrough" : "restackStack"; if (!land && input.action !== "update-branch") return yield* failure(operation, `GitCafe cannot ${input.action} a stack.`); - const stack = yield* readStack(operation, input); + const stack = yield* readStack(operation, input, true); if ( stack === null || stack.number !== input.stackNumber || @@ -372,6 +416,12 @@ export const make = Effect.gen(function* () { : { expectedRevision: stack.revision, requestId: yield* randomUuidV4 }, }, StackOutcome, + ).pipe( + Effect.ensuring( + Effect.forEach(stack.members, (member) => + forgetPull({ ...input, number: member.pullRequestNumber }), + ), + ), ); if (FAILED_STACK_STATES.has(outcome.state)) return yield* failure(operation, `GitCafe's stack operation ${outcome.state}.`); @@ -385,6 +435,7 @@ export const make = Effect.gen(function* () { const readLineStats = Effect.fn("GitCafePullRequestProvider.readLineStats")(function* ( input: PullRef, version: number, + headOid: string, ) { const path = yield* pullPath("getChangeRequest", input); const files: Array = []; @@ -399,6 +450,12 @@ export const make = Effect.gen(function* () { { path: `${path}/changes?${query}` }, Changes, ); + if ( + batch.version !== version || + batch.headOid !== headOid || + (revision !== undefined && batch.comparisonBaseOid !== revision.baseOid) + ) + return yield* failure("getChangeRequest", "GitCafe answered from a different revision."); revision ??= { version: batch.version, headOid: batch.headOid, @@ -411,23 +468,54 @@ export const make = Effect.gen(function* () { } // A hunk read that loses a race with a push leaves the counts at zero rather than dropping // the file count with them. - const counted = + const details: Option.Option>> = files.some((file) => file.additions !== undefined) || files.length > LINE_STATS_MAX_FILES || revision === undefined - ? files - : (yield* Effect.forEach( + ? Option.some([files]) + : yield* Effect.forEach( chunk(detailablePaths(files), DIFF_FILES_BATCH), (paths) => readDiffFiles(input, revision, paths), { concurrency: 4 }, - ).pipe(Effect.orElseSucceed(() => [[]]))).flat(); + ).pipe(Effect.option); + const counted: ReadonlyArray = Option.isSome(details) + ? details.value.flat() + : []; return { + revision, + complete: Option.isSome(details) && after === null, changedFiles: files.length, additions: counted.reduce((total, file) => total + (file.additions ?? 0), 0), deletions: counted.reduce((total, file) => total + (file.deletions ?? 0), 0), }; }); + /** + * Line totals held by the revision they count, which never changes, so reopening an unchanged + * pull request skips the `/changes` pages and hunk reads. Only a complete count of the head + * the pull reported is held. + */ + const lineStats = yield* Cache.makeWith( + (key: string) => { + const [host = "", repository = "", number = "", version = "", headOid = ""] = key.split("\n"); + return readLineStats( + { host, repository, number: Number(number) }, + Number(version), + headOid, + ).pipe( + Effect.map(({ revision, complete, ...stats }) => ({ + stats, + held: complete && revision?.version === Number(version) && revision.headOid === headOid, + })), + ); + }, + { + capacity: 64, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value.held ? Duration.infinity : Duration.zero, + }, + ); + const merge = Effect.fn("GitCafePullRequestProvider.merge")(function* ( input: Parameters[0], ) { @@ -470,12 +558,35 @@ export const make = Effect.gen(function* () { }, }, MergeOutcome, - ); + ).pipe(Effect.ensuring(forgetPull(input))); // An accepted merge is GitCafe's to finish; the pull request's state reports when it has. if (outcome.state === "failed") return yield* failure("merge", outcome.reason ?? "GitCafe could not merge the pull request."); }); + /** The viewer's actor on a repository; a viewer GitCafe knows no actor for is asked again. */ + const viewerActors = yield* Cache.makeWith( + (key: string) => { + const [host = "", repository = "", viewer = ""] = key.split("\n"); + return read( + "listChangeRequests", + { host }, + { path: `/repos/${repository}/filter-options` }, + FilterOptions, + ).pipe( + Effect.map( + (options) => + options.actors.find((actor) => actor.handle.toLowerCase() === viewer)?.actorId, + ), + ); + }, + { + capacity: 64, + timeToLive: (exit) => + Exit.isSuccess(exit) && exit.value !== undefined ? ACTOR_TTL : Duration.zero, + }, + ); + const provider: PullRequestProviderApi = { kind: SourceControlProviderKind.make("gitcafe"), capabilities: CAPABILITIES, @@ -495,11 +606,9 @@ export const make = Effect.gen(function* () { // A host narrows as far as it can: when the actor list can't be read, the listing comes // back unnarrowed and the page's own involvement filter does the rest. A refused token // or a rate limit still fails, since the service acts on those for the whole host. - const options = yield* read( - "listChangeRequests", - input, - { path: `${base}/filter-options` }, - FilterOptions, + const actor = yield* Cache.get( + viewerActors, + `${input.host}\n${input.repository}\n${input.viewer.toLowerCase()}`, ).pipe( Effect.map(Option.some), Effect.catchTags({ @@ -509,10 +618,8 @@ export const make = Effect.gen(function* () { : Effect.succeed(Option.none()), }), ); - if (Option.isSome(options)) { - actorId = options.value.actors.find( - (actor) => actor.handle.toLowerCase() === input.viewer.toLowerCase(), - )?.actorId; + if (Option.isSome(actor)) { + actorId = actor.value; // A viewer GitCafe knows no actor for has authored and been asked to review nothing. if (actorId === undefined) return { items: [], truncated: false, continues: false }; } @@ -584,7 +691,20 @@ export const make = Effect.gen(function* () { ), pull.headOid === null ? Effect.succeed(null) - : readLineStats(input, pull.version).pipe(Effect.orElseSucceed(() => null)), + : Cache.get( + lineStats, + [ + input.host, + input.repository, + input.number, + pull.version, + pull.headOid, + pull.observedBaseOid ?? "", + ].join("\n"), + ).pipe( + Effect.map(({ stats }) => stats), + Effect.orElseSucceed(() => null), + ), ], { concurrency: 3 }, ); @@ -634,6 +754,11 @@ export const make = Effect.gen(function* () { : permissions, }; }), + // A hover card needs only the pull, not detail's status, checks and line counts. + getChangeRequestPreview: (input) => + readPull("getChangeRequestPreview", input).pipe( + Effect.map((pull) => Json.toChangeRequest(pull, input)), + ), getChangeRequestStack: (input) => readStack("getChangeRequestStack", input).pipe( Effect.map((stack) => (stack === null ? null : Json.toStack(stack, input))), @@ -668,7 +793,10 @@ export const make = Effect.gen(function* () { }, ), getViewerPermissions: (input) => - readPull("getViewerPermissions", input).pipe(Effect.map(Json.toViewerPermissions)), + forgetPull(input).pipe( + Effect.andThen(readPull("getViewerPermissions", input)), + Effect.map(Json.toViewerPermissions), + ), getDiff: Effect.fn("GitCafePullRequestProvider.getDiff")(function* (input) { if (input.commit !== undefined) return yield* failure("getDiff", "GitCafe commit diffs are not supported yet."); @@ -718,7 +846,7 @@ export const make = Effect.gen(function* () { method: "POST", path: `${path}/${input.action}`, body: { expectedVersion: pull.version }, - }); + }).pipe(Effect.ensuring(forgetPull(input))); return; } default: @@ -742,7 +870,7 @@ export const make = Effect.gen(function* () { }, }, Versioned, - ); + ).pipe(Effect.ensuring(forgetPull(input))); }, ), comment: (input) => @@ -756,6 +884,7 @@ export const make = Effect.gen(function* () { ), ), Effect.asVoid, + Effect.ensuring(forgetPull(input)), ), updateComment: Effect.fn("GitCafePullRequestProvider.updateComment")(function* (input) { if (!isId(input.commentId)) @@ -767,7 +896,7 @@ export const make = Effect.gen(function* () { input, { method: "PATCH", path, body: { body: input.body, expectedVersion: detail.version } }, Written, - ); + ).pipe(Effect.ensuring(forgetPull(input))); }), submitReview: Effect.fn("GitCafePullRequestProvider.submitReview")(function* (input) { if (input.comments.length > 0) @@ -791,7 +920,7 @@ export const make = Effect.gen(function* () { }, }, Written, - ); + ).pipe(Effect.ensuring(forgetPull(input))); }), listReviewerCandidates: () => Effect.fail( @@ -812,7 +941,7 @@ export const make = Effect.gen(function* () { body: { body: input.body }, }, Written, - ); + ).pipe(Effect.ensuring(forgetPull(input))); }), setReaction: Effect.fn("GitCafePullRequestProvider.setReaction")(function* (input) { const path = `${yield* pullPath("setReaction", input)}/reactions/`; @@ -834,13 +963,15 @@ export const make = Effect.gen(function* () { method: "POST", path, body: { subject, emoji: { kind: "unicode", value: Json.REACTION_EMOJI[input.content] } }, - }); + }).pipe(Effect.ensuring(forgetPull(input))); return; } if (mine == null) return; if (!isId(mine)) return yield* failure("setReaction", "GitCafe returned an unreadable reaction id."); - yield* send("setReaction", input, { method: "DELETE", path: `${path}${mine}` }); + yield* send("setReaction", input, { method: "DELETE", path: `${path}${mine}` }).pipe( + Effect.ensuring(forgetPull(input)), + ); }), setThreadResolution: Effect.fn("GitCafePullRequestProvider.setThreadResolution")( function* (input) { @@ -861,7 +992,7 @@ export const make = Effect.gen(function* () { body: { expectedVersion: root.version }, }, Written, - ); + ).pipe(Effect.ensuring(forgetPull(input))); }, ), }; diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 961106c58acc..a799387f6b2b 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -184,12 +184,13 @@ function listedPullRequests( count: number, firstNumber: number, overrides?: (number: number) => Readonly>, + endCursor: string | null = null, ): string { return encodeJson({ data: { repository: { pullRequests: { - pageInfo: { hasNextPage: false }, + pageInfo: { hasNextPage: endCursor !== null, endCursor }, nodes: rows(count, firstNumber, overrides), }, }, @@ -1408,10 +1409,10 @@ layer("GitHubPullRequestApi.layer", (it) => { Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); mockedExecute.mockReturnValueOnce( - Effect.succeed(output(listedPullRequests(3, 1, () => ({ isDraft: true })))), + Effect.succeed(output(listedPullRequests(3, 1, () => ({ isDraft: true }), "c3"))), ); mockedExecute.mockReturnValueOnce( - Effect.succeed(output(listedPullRequests(6, 1, (number) => ({ isDraft: number < 4 })))), + Effect.succeed(output(listedPullRequests(3, 4, () => ({ isDraft: false }), "c6"))), ); mockedStackMemberships.mockReturnValueOnce( Effect.succeed( @@ -1446,6 +1447,7 @@ layer("GitHubPullRequestApi.layer", (it) => { expect(batch.items[1]?.stack).toBeUndefined(); expect(batch.truncated).toBe(true); expect(batch.continues).toBe(false); + expect(mockedExecute).toHaveBeenCalledTimes(3); expect(mockedStackMemberships).toHaveBeenCalledTimes(1); const membership = mockedStackMemberships.mock.calls[0]?.[0]; const numbers = Object.entries( @@ -2048,18 +2050,26 @@ layer("GitHubPullRequestApi.layer", (it) => { Effect.gen(function* () { const unrelated = () => ({ reviewRequests: [{ login: "somebody-else" }] }); mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); - mockedExecute.mockReturnValueOnce( - Effect.succeed(output(listedPullRequests(3, 1, unrelated))), - ); mockedExecute.mockReturnValueOnce( Effect.succeed( output( - listedPullRequests(4, 1, (number) => - number === 4 ? { reviewRequests: [{ login: "bilal" }] } : unrelated(), + listedPullRequests( + 3, + 1, + (number) => ({ + ...unrelated(), + ...(number === 1 ? { number: null } : {}), + }), + "c3", ), ), ), ); + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output(listedPullRequests(1, 4, () => ({ reviewRequests: [{ login: "bilal" }] }))), + ), + ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; const batch = yield* cli.listPullRequests({ @@ -2073,12 +2083,70 @@ layer("GitHubPullRequestApi.layer", (it) => { }); expect(batch.items.map((item) => item.number)).toEqual([4]); + // The malformed row still counts toward the requested range. expect(queryAt(1)).toContain("first: 3,"); - expect(queryAt(2)).toContain("first: 6,"); + expect(queryAt(2)).toContain("first: 3,"); + expect(varsAt(1)["after"]).toBeNull(); + expect(varsAt(2)["after"]).toBe("c3"); + expect(mockedExecute).toHaveBeenCalledTimes(3); assert.isFalse(batch.truncated); }), ); + it.effect("does not restart an exhausted search-free fallback at a full filtered boundary", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(listedPullRequests(3, 1, () => ({ isDraft: true })))), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 2, + filters: { draft: "hide" }, + }); + + expect(mockedExecute).toHaveBeenCalledTimes(2); + expect(batch.items).toEqual([]); + assert.isFalse(batch.truncated); + assert.isFalse(batch.continues); + }), + ); + + it.effect("keeps the repeated-cursor guard while widening a search-free fallback", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(emptySearch())); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(listedPullRequests(3, 1, () => ({ isDraft: true }), "same"))), + ); + mockedExecute.mockReturnValueOnce( + Effect.succeed(output(listedPullRequests(3, 4, () => ({ isDraft: true }), "same"))), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const batch = yield* cli.listPullRequests({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + state: "open", + involvement: "all", + viewer: "bilal", + limit: 2, + filters: { draft: "hide" }, + }); + + expect(mockedExecute).toHaveBeenCalledTimes(3); + expect(varsAt(2)["after"]).toBe("same"); + expect(batch.items).toEqual([]); + assert.isTrue(batch.truncated); + assert.isFalse(batch.continues); + }), + ); + it.effect("bounds a sparse search-free fallback and reports the unread tail", () => Effect.gen(function* () { // A repository with far more pull requests than the bound, none of them for this reader. @@ -2116,10 +2184,17 @@ layer("GitHubPullRequestApi.layer", (it) => { limit: 2, }); - // The last widening reads a thousand rows, a page of a hundred at a time, and stops there. + // Widening continues from the preceding cursor and reads no row a second time. const last = mockedExecute.mock.calls.length - 1; - expect(varsAt(last)["after"]).toBe(`c${listed - 100}`); - expect(listed - (3 + 6 + 12 + 24 + 48 + 96 + 192 + 384 + 768)).toBe(1000); + expect(varsAt(last)["after"]).toBe("c968"); + expect(queryAt(last)).toContain("first: 32,"); + expect(listed).toBe(1000); + expect(mockedExecute).toHaveBeenCalledTimes(17); + expect( + mockedExecute.mock.calls + .slice(2) + .every(([call]) => call.kind === "graphql" && call.variables?.["after"] !== null), + ).toBe(true); assert.strictEqual(batch.items.length, 0); assert.isTrue(batch.truncated); }), @@ -3617,6 +3692,77 @@ layer("GitHubPullRequestApi.layer", (it) => { }), ); + it.effect("starts a fresh detail read after a mutation while an older read is pending", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + let detailReads = 0; + mockedExecute.mockImplementation((call) => + Effect.gen(function* () { + if (call.kind === "graphql" && call.query.trimStart().startsWith("mutation")) { + return output("{}"); + } + if (call.kind === "graphql" && call.query.includes(NODE_ID_QUERY)) { + return output(encodeJson(nodeIdAnswer("PR_after_write"))); + } + const title = ++detailReads === 1 ? "Before mutation" : "After mutation"; + if (detailReads === 1) { + yield* Deferred.succeed(started, undefined); + yield* Deferred.await(release); + } + return output(encodeJson(coreResponse({ title }))); + }), + ); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const input = { + cwd: "/w", + repository: "acme/web", + host: "github.detail-after-write.test", + number: 7, + }; + + const older = yield* cli.getPullRequestDetail(input).pipe(Effect.forkChild); + yield* Deferred.await(started); + yield* cli.updatePullRequest({ ...input, title: "After mutation" }); + const fresh = yield* cli.getPullRequestDetail(input); + + expect(fresh.title).toBe("After mutation"); + expect(detailReads).toBe(2); + expect(variablesOf("updatePullRequest(")).toEqual([ + { pullRequestId: "PR_after_write", title: "After mutation" }, + ]); + yield* Deferred.succeed(release, undefined); + expect((yield* Fiber.join(older)).title).toBe("Before mutation"); + }), + ); + + it.effect("uses the node id from core detail for the first mutation without another lookup", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce( + Effect.succeed(output(encodeJson(coreResponse({ id: "PR_from_core" })))), + ) + .mockReturnValue(Effect.succeed(output("{}"))); + const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; + const input = { + cwd: "/w", + repository: "acme/web", + host: "github.core-node-id.test", + number: 7, + }; + + const detail = yield* cli.getPullRequestDetail(input); + yield* cli.updatePullRequest({ ...input, title: "Uses core id" }); + + expect(detail).not.toHaveProperty("nodeId"); + expect(mockedExecute).toHaveBeenCalledTimes(2); + expect(variablesOf(NODE_ID_QUERY)).toEqual([]); + expect(variablesOf("updatePullRequest(")).toEqual([ + { pullRequestId: "PR_from_core", title: "Uses core id" }, + ]); + }), + ); + it.effect("keeps the core detail read separate from conversation activity", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index b8c7fe227688..313e4705cb4d 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -1,6 +1,7 @@ import { removeAgentCredits } from "@t3tools/source-control-core/server/mergeMessage"; import { KnownWorkflowRuns, makeChecksRevalidator } from "./gitHubConditionalChecks.ts"; import { runGitHubStackAction, type GitHubStackActionError } from "./githubStackActions.ts"; +import * as Cache from "effect/Cache"; import * as Cause from "effect/Cause"; import * as Context from "effect/Context"; import * as Clock from "effect/Clock"; @@ -1104,7 +1105,30 @@ const SIMPLE_ACTION_MUTATIONS = { /** @public Service construction is part of the canonical Effect module API. */ export const make = Effect.gen(function* () { - const api = yield* GitHubApi.GitHubApi; + const transport = yield* GitHubApi.GitHubApi; + // A read started after a write must not join a request that captured the old state. + const forgetReads = Effect.suspend(() => + Effect.all([Cache.invalidateAll(detailsInFlight), Cache.invalidateAll(workflowRunsInFlight)]), + ); + const api = GitHubApi.GitHubApi.of({ + ...transport, + graphql: (input) => + transport + .graphql(input) + .pipe( + Effect.ensuring( + input.query.trimStart().startsWith("mutation") ? forgetReads : Effect.void, + ), + ), + rest: (input) => + transport + .rest(input) + .pipe( + Effect.ensuring( + input.method !== undefined && input.method !== "GET" ? forgetReads : Effect.void, + ), + ), + }); const sourceControlHost = yield* SourceControlHost.SourceControlHost; const fileSystem = yield* FileSystem.FileSystem; const revalidateChecks = yield* makeChecksRevalidator; @@ -1202,6 +1226,15 @@ export const make = Effect.gen(function* () { */ const nodeIds = new Map(); + const rememberNodeId = (key: string, nodeId: string) => { + nodeIds.delete(key); + if (nodeIds.size >= NODE_ID_CACHE_CAPACITY) { + const oldest = nodeIds.keys().next().value; + if (oldest !== undefined) nodeIds.delete(oldest); + } + nodeIds.set(key, nodeId); + }; + const pullRequestNodeId = (input: { readonly cwd: string; readonly repository: string; @@ -1228,17 +1261,7 @@ export const make = Effect.gen(function* () { variables: { owner, name, number: input.number }, query: PULL_REQUEST_NODE_ID_GRAPHQL_QUERY, decode: decodePullRequestNodeIdJson, - }).pipe( - Effect.tap((nodeId) => - Effect.sync(() => { - if (nodeIds.size >= NODE_ID_CACHE_CAPACITY) { - const oldest = nodeIds.keys().next().value; - if (oldest !== undefined) nodeIds.delete(oldest); - } - nodeIds.set(key, nodeId); - }), - ), - ); + }).pipe(Effect.tap((nodeId) => Effect.sync(() => rememberNodeId(key, nodeId)))); }; /** @@ -1542,6 +1565,14 @@ export const make = Effect.gen(function* () { query: pullRequestCoreGraphQlQuery(input.host), decode: decodePullRequestCoreJson, }).pipe( + Effect.tap((core) => { + const nodeId = core.nodeId; + return nodeId === undefined + ? Effect.void + : Effect.sync(() => + rememberNodeId(`${input.host} ${owner}/${name} ${input.number}`, nodeId), + ); + }), // Past a hundred checks the first page would let the rest imply success, so the whole // rollup is walked instead. Effect.filterOrElse( @@ -1564,6 +1595,7 @@ export const make = Effect.gen(function* () { })), ), ), + Effect.map(({ nodeId: _nodeId, ...core }) => core), ), ), ); @@ -1717,6 +1749,52 @@ export const make = Effect.gen(function* () { { concurrency: 2 }, ).pipe(Effect.map(([, runs]) => runs)); + // Opening a pull request reads its detail, its checks and the viewer's permissions at once, and + // each starts from the same detail query, and for a fork's pull request the same workflow runs. + // Reads in flight together under the same credential and quota share one request. Nothing is + // kept once it lands, so a read that starts later asks GitHub again. + const inFlightKey = (...parts: ReadonlyArray) => + Effect.gen(function* () { + const pinned = yield* GitHubApi.PinnedGitHubCredential; + return [ + ...parts, + yield* SourceControlRateLimit.Interactive, + yield* SourceControlRateLimit.CredentialScope, + pinned?.credentialFingerprint ?? "", + ].join("\n"); + }); + const detailsInFlight = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", host = "", repository = "", number = ""] = key.split("\n"); + return getPullRequestDetail({ cwd, host, repository, number: Number(number) }); + }, + { capacity: 64, timeToLive: () => 0 }, + ); + const workflowRunsInFlight = yield* Cache.makeWith( + (key: string) => { + const [ + cwd = "", + host = "", + repository = "", + number = "", + headSha = "", + headBranch = "", + headRepositoryOwner = "", + ] = key.split("\n"); + return listWorkflowRunsRequiringApproval({ + cwd, + host, + repository, + number: Number(number), + headSha, + headBranch, + headRepositoryOwner, + isCrossRepository: true, + }); + }, + { capacity: 64, timeToLive: () => 0 }, + ); + /** One pull request's summary, through the same aliased query the batch uses. */ const viewPullRequestSummary = (input: PullRequestSummaryRead) => { const { owner, name } = parseRepositorySelector(input.repository); @@ -1897,82 +1975,73 @@ export const make = Effect.gen(function* () { }), ); } - /** Pages of up to a hundred until `rows` have arrived or GitHub has no more. */ - const collect = ( - rows: number, - page: ( - after: string | null, - rows: number, - ) => Effect.Effect, - ) => + const read = ( + continues: boolean, + ): Effect.Effect => Effect.gen(function* () { - const countRows = (pages: ReadonlyArray) => - pages.reduce((total, read) => total + read.rawCount, 0); - const { pages, truncated } = yield* readGraphQlPages( - (after, pages: ReadonlyArray) => - page(after, rows - countRows(pages)), + let requestedRows = input.limit + 1; + let rawCount = 0; + const items: GitHubPullRequestListItem[] = []; + const { truncated: cutShort } = yield* readGraphQlPages( + (after): Effect.Effect => { + const rows = requestedRows - rawCount; + const page = continues + ? graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listPullRequests", + // The reader's own words are in the query. + variables: { q: query, after }, + query: pullRequestSearchGraphQlQuery(rows, false, true), + decode: decodePullRequestSearchJson, + }) + : graphqlRead({ + cwd: input.cwd, + host: input.host, + operation: "listPullRequests", + variables: { owner, name, states: LIST_STATES[input.state], after }, + query: pullRequestListGraphQlQuery(rows), + decode: decodePullRequestListJson, + }); + return page.pipe( + Effect.tap((read) => + Effect.sync(() => { + rawCount += read.rawCount; + items.push( + ...(continues + ? read.items + : read.items.filter((item) => matchesUnsortedListing(item, input))), + ); + if ( + !continues && + items.length < input.limit && + rawCount >= requestedRows && + requestedRows < fallbackMaxRows + ) { + // Widen the target while keeping the pages and cursors already read. + requestedRows = Math.min(requestedRows * 2, fallbackMaxRows); + } + }), + ), + ); + }, { nextCursor: (read) => read.endCursor, - until: (pages) => countRows(pages) >= rows, + until: () => rawCount >= requestedRows, }, ); - // `truncated` also covers a cursor GitHub repeated before `rows` arrived. return { - items: pages.flatMap((read) => read.items), - rawCount: countRows(pages), - truncated, + items: items.slice(0, input.limit), + // One row over the page size is the probe for a next page, and it is + // counted before decoding: a skipped malformed row must not end paging. + truncated: + cutShort || + (continues + ? rawCount > input.limit + : items.length > input.limit || rawCount >= requestedRows), + continues, }; }); - const read = ( - continues: boolean, - requestedRows = input.limit + 1, - ): Effect.Effect => - collect(requestedRows, (after, rows) => - continues - ? graphqlRead({ - cwd: input.cwd, - host: input.host, - operation: "listPullRequests", - // The reader's own words are in the query. - variables: { q: query, after }, - query: pullRequestSearchGraphQlQuery(rows, false, true), - decode: decodePullRequestSearchJson, - }) - : graphqlRead({ - cwd: input.cwd, - host: input.host, - operation: "listPullRequests", - variables: { owner, name, states: LIST_STATES[input.state], after }, - query: pullRequestListGraphQlQuery(rows), - decode: decodePullRequestListJson, - }), - ).pipe( - Effect.flatMap(({ items: rawItems, rawCount, truncated: cutShort }) => { - const items = continues - ? rawItems - : rawItems.filter((item) => matchesUnsortedListing(item, input)); - if ( - !continues && - items.length < input.limit && - rawCount >= requestedRows && - requestedRows < fallbackMaxRows - ) { - const nextRows = Math.min(requestedRows * 2, fallbackMaxRows); - if (nextRows > requestedRows) return read(false, nextRows); - } - return Effect.succeed({ - items: items.slice(0, input.limit), - // One row over the page size is the probe for a next page, and it is - // counted before decoding: a skipped malformed row must not end paging. - truncated: - cutShort || - (continues - ? rawCount > input.limit - : items.length > input.limit || rawCount >= requestedRows), - continues, - }); - }), - ); // GitHub does not index every repository for search, and one it will not search answers // with no rows rather than with an error — so an empty listing is read again from the // repository's own list. Those rows come back newest-created first, an order no `updated:` @@ -2109,7 +2178,10 @@ export const make = Effect.gen(function* () { getPullRequestSummary, getPullRequestWatchFingerprint, - getPullRequestDetail, + getPullRequestDetail: (input) => + inFlightKey(input.cwd, input.host, input.repository, input.number).pipe( + Effect.flatMap((key) => Cache.get(detailsInFlight, key)), + ), getPullRequestPreview: (input) => { const { owner, name } = parseRepositorySelector(input.repository); return graphqlRead({ @@ -2121,7 +2193,22 @@ export const make = Effect.gen(function* () { decode: decodePullRequestPreviewJson, }); }, - listWorkflowRunsRequiringApproval, + listWorkflowRunsRequiringApproval: (input) => + Effect.gen(function* () { + // Runs the checks revalidator just confirmed are this caller's alone to filter. + if ((yield* KnownWorkflowRuns) !== null) + return yield* listWorkflowRunsRequiringApproval(input); + const key = yield* inFlightKey( + input.cwd, + input.host, + input.repository, + input.number, + input.headSha, + input.headBranch, + input.headRepositoryOwner, + ); + return yield* Cache.get(workflowRunsInFlight, key); + }), getPullRequestStack: (input) => { const { owner, name } = parseRepositorySelector(input.repository); diff --git a/packages/source-control-github/src/server/gitHubPullRequestJson.ts b/packages/source-control-github/src/server/gitHubPullRequestJson.ts index e3768b04d7bc..68d0a5d43b6b 100644 --- a/packages/source-control-github/src/server/gitHubPullRequestJson.ts +++ b/packages/source-control-github/src/server/gitHubPullRequestJson.ts @@ -708,6 +708,7 @@ const RawCoreSchema = Schema.Struct({ repository: Schema.Struct({ ...RawRepositoryAccessSchema.fields, pullRequest: Schema.Struct({ + id: Schema.optional(Schema.String), ...RawDetailSchema.fields, ...RawViewerFieldsSchema.fields, viewerCanUpdateBranch: Schema.Boolean, @@ -826,7 +827,7 @@ export const pullRequestCoreGraphQlQuery = (host: string) => { repository(owner: $owner, name: $name) { mergeCommitAllowed squashMergeAllowed rebaseMergeAllowed viewerPermission pullRequest(number: $number) { - number title url body state isDraft mergeable reviewDecision + id number title url body state isDraft mergeable reviewDecision additions deletions changedFiles createdAt updatedAt mergedAt closedAt headRefName baseRefName headRefOid isCrossRepository headRepositoryOwner { login } @@ -2302,6 +2303,7 @@ export function decodePullRequestWatchFingerprintsJson( } export interface GitHubPullRequestCore extends GitHubPullRequestDetail { + readonly nodeId?: string; readonly viewerAccess: GitHubViewerAccess & GitHubRepositoryAccess; readonly comparison: GitHubBaseComparison | null; readonly checksTruncated: boolean; @@ -2316,6 +2318,7 @@ export function decodePullRequestCoreJson( const pr = repository.pullRequest; const contexts = pr.commits.nodes[0]?.commit.statusCheckRollup?.contexts; return Result.succeed({ + ...(pr.id === undefined ? {} : { nodeId: pr.id }), ...toDetail({ ...pr, reviewRequests: pr.reviewRequests.nodes.flatMap(({ requestedReviewer }) => diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts index 643d130c85bc..f8507b074e99 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts @@ -309,6 +309,8 @@ export class GitLabPullRequestCli extends Context.Service< readonly cwd: string; readonly repository: string; readonly number: number; + /** The merge request as the caller already holds it, which spares reading it again. */ + readonly mergeRequest?: GitLabMergeRequestDetail | undefined; }) => Effect.Effect; readonly setReviewerRequest: (input: { @@ -317,6 +319,8 @@ export class GitLabPullRequestCli extends Context.Service< readonly number: number; readonly reviewers: ReadonlyArray<{ readonly id: string }>; readonly requested: boolean; + /** The merge request as the caller holds it, whose reviewer set is the one rewritten. */ + readonly mergeRequest?: GitLabMergeRequestDetail | undefined; }) => Effect.Effect; readonly runMergeRequestAction: (input: { @@ -946,6 +950,17 @@ export const make = Effect.gen(function* () { }), ); + /** The merge request a caller handed over, or a read of it where none was. */ + const heldOrRead = (input: { + readonly cwd: string; + readonly repository: string; + readonly number: number; + readonly mergeRequest?: GitLabMergeRequestDetail | undefined; + }): Effect.Effect => + input.mergeRequest === undefined + ? mergeRequestDetail(input) + : Effect.succeed(input.mergeRequest); + /** The people with access to the project, one page deep. */ const projectUsers = (input: { readonly cwd: string; @@ -1319,7 +1334,7 @@ export const make = Effect.gen(function* () { ), listReviewerCandidates: (input) => - Effect.all([mergeRequestDetail(input), projectUsers(input)], { concurrency: 2 }).pipe( + Effect.all([heldOrRead(input), projectUsers(input)], { concurrency: 2 }).pipe( Effect.map(([mergeRequest, users]) => { const author = mergeRequest.author?.login; const requested = new Set(mergeRequest.reviewRequestLogins); @@ -1337,7 +1352,7 @@ export const make = Effect.gen(function* () { ), setReviewerRequest: (input) => - mergeRequestDetail(input).pipe( + heldOrRead(input).pipe( Effect.flatMap((mergeRequest) => { // GitLab has no endpoint that adds or removes one reviewer: `reviewer_ids` replaces the // whole set, so the set that is already there is read first and the change applied to diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index a25d5314ef4d..28cfcfb62e11 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -1,5 +1,8 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; +import * as Cache from "effect/Cache"; import * as Effect from "effect/Effect"; +import * as Exit from "effect/Exit"; +import * as Option from "effect/Option"; import type { PullRequestCapabilities, PullRequestReaction, @@ -99,9 +102,45 @@ export function gitLabProviderFailure( return { reason: "failed" }; } +const mergeRequestKey = (input: { + readonly cwd: string; + readonly repository: string; + readonly number: number; +}) => `${input.cwd}\n${input.repository}\n${input.number}`; + export const make = Effect.gen(function* () { const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + // The detail, its checks and the viewer's permissions are all `merge_requests/:iid`, and a page + // opening asks for them at once. One read answers the ones that land within a second of it; a + // failure is not held, and every write below forgets the merge request it wrote to. + const mergeRequests = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", repository = "", number = ""] = key.split("\n"); + return cli.getMergeRequestDetail({ cwd, repository, number: Number(number) }); + }, + { capacity: 64, timeToLive: (exit) => (Exit.isSuccess(exit) ? "1 second" : 0) }, + ); + const mergeRequest = (input: Parameters[0]) => + Cache.get(mergeRequests, mergeRequestKey(input)); + + // How a project lets its merge requests land is a setting, which moves far more slowly than + // the detail every refresh was reading it alongside. + const mergeSettings = yield* Cache.makeWith( + (key: string) => { + const [cwd = "", repository = ""] = key.split("\n"); + return cli.getProjectMergeCapabilities({ cwd, repository }); + }, + { capacity: 32, timeToLive: (exit) => (Exit.isSuccess(exit) ? "5 minutes" : 0) }, + ); + + // A write can change anything the merge request reports, so the held read is dropped once it + // ends, failed or not: a write that failed part-way may still have changed it. + const writing = + (input: Parameters[0]) => + (write: Effect.Effect) => + write.pipe(Effect.ensuring(Cache.invalidate(mergeRequests, mergeRequestKey(input)))); + const fail = (operation: string) => (error: GitLabPullRequestCli.GitLabPullRequestCliError) => new PullRequestProviderError({ provider: SourceControlProviderKind.make("gitlab"), @@ -138,17 +177,20 @@ export const make = Effect.gen(function* () { ), getChangeRequestChecks: (input) => - cli.getMergeRequestDetail(input).pipe( + mergeRequest(input).pipe( Effect.map(({ state, checks }) => ({ state, checks })), Effect.mapError(fail("getChangeRequestChecks")), ), + // Hover cards and linked rows do not use the project's merge settings. + getChangeRequestPreview: (input) => + mergeRequest(input).pipe(Effect.mapError(fail("getChangeRequestPreview"))), + getChangeRequestSummary: (input) => + mergeRequest(input).pipe(Effect.mapError(fail("getChangeRequestSummary"))), + getChangeRequest: (input) => Effect.all( - [ - cli.getMergeRequestDetail(input), - cli.getProjectMergeCapabilities({ cwd: input.cwd, repository: input.repository }), - ], + [mergeRequest(input), Cache.get(mergeSettings, `${input.cwd}\n${input.repository}`)], { concurrency: 2 }, ).pipe( Effect.mapError(fail("getChangeRequest")), @@ -214,12 +256,17 @@ export const make = Effect.gen(function* () { })), ), - // The same read the detail takes it from, on its own: `user.can_merge` lives on the merge - // request, so there is no cheaper thing to ask GitLab. - getViewerPermissions: (input) => - cli - .getMergeRequestDetail(input) - .pipe(Effect.mapError(fail("getViewerPermissions")), Effect.map(gitLabViewerPermissions)), + // The same read the detail takes it from: `user.can_merge` lives on the merge request, so + // there is no cheaper thing to ask GitLab. The server asks this before every write, so it + // starts a fresh read; the write after it can then reuse what it read. + getViewerPermissions: (input) => { + const key = mergeRequestKey(input); + return Cache.invalidate(mergeRequests, key).pipe( + Effect.andThen(Cache.get(mergeRequests, key)), + Effect.mapError(fail("getViewerPermissions")), + Effect.map(gitLabViewerPermissions), + ); + }, getDiff: (input) => cli.getMergeRequestDiff(input).pipe(Effect.mapError(fail("getDiff"))), @@ -237,25 +284,38 @@ export const make = Effect.gen(function* () { // Users only: GitLab requests a review of a person, and the groups that can stand in for one // appear in approval rules rather than in a merge request's reviewers. + // The merge request the permission check just read, where it is held, rather than a second + // read of it; where it is not, the CLI reads it alongside the project's users. listReviewerCandidates: (input) => - cli - .listReviewerCandidates({ - cwd: input.cwd, - repository: input.repository, - number: input.number, - }) - .pipe(Effect.mapError(fail("listReviewerCandidates"))), + Cache.getSuccess(mergeRequests, mergeRequestKey(input)).pipe( + Effect.flatMap((held) => + cli.listReviewerCandidates({ + cwd: input.cwd, + repository: input.repository, + number: input.number, + mergeRequest: Option.getOrUndefined(held), + }), + ), + Effect.mapError(fail("listReviewerCandidates")), + ), + // The reviewer set is rewritten from the held read, which the permission check before this + // write has just taken and which no write of this provider outlives. setReviewerRequest: (input) => - cli - .setReviewerRequest({ - cwd: input.cwd, - repository: input.repository, - number: input.number, - reviewers: input.reviewers, - requested: input.requested, - }) - .pipe(Effect.mapError(fail("setReviewerRequest"))), + mergeRequest(input).pipe( + Effect.flatMap((held) => + cli.setReviewerRequest({ + cwd: input.cwd, + repository: input.repository, + number: input.number, + reviewers: input.reviewers, + requested: input.requested, + mergeRequest: held, + }), + ), + writing(input), + Effect.mapError(fail("setReviewerRequest")), + ), runAction: (input) => cli @@ -266,7 +326,7 @@ export const make = Effect.gen(function* () { action: input.action, ...(input.mergeMethod === undefined ? {} : { mergeMethod: input.mergeMethod }), }) - .pipe(Effect.mapError(fail("runAction"))), + .pipe(writing(input), Effect.mapError(fail("runAction"))), updateChangeRequest: (input) => cli @@ -277,9 +337,10 @@ export const make = Effect.gen(function* () { ...(input.title === undefined ? {} : { title: input.title }), ...(input.body === undefined ? {} : { description: input.body }), }) - .pipe(Effect.mapError(fail("updateChangeRequest"))), + .pipe(writing(input), Effect.mapError(fail("updateChangeRequest"))), - comment: (input) => cli.commentOnMergeRequest(input).pipe(Effect.mapError(fail("comment"))), + comment: (input) => + cli.commentOnMergeRequest(input).pipe(writing(input), Effect.mapError(fail("comment"))), // The kind is not read: every comment this provider hands out, positioned or not, carries a // plain REST note id, and one endpoint rewrites both. @@ -292,9 +353,10 @@ export const make = Effect.gen(function* () { noteId: input.commentId, body: input.body, }) - .pipe(Effect.mapError(fail("updateComment"))), + .pipe(writing(input), Effect.mapError(fail("updateComment"))), - submitReview: (input) => cli.submitReview(input).pipe(Effect.mapError(fail("submitReview"))), + submitReview: (input) => + cli.submitReview(input).pipe(writing(input), Effect.mapError(fail("submitReview"))), replyToThread: (input) => cli @@ -305,7 +367,7 @@ export const make = Effect.gen(function* () { discussionId: input.threadId, body: input.body, }) - .pipe(Effect.mapError(fail("replyToThread"))), + .pipe(writing(input), Effect.mapError(fail("replyToThread"))), setReaction: (input) => cli @@ -317,7 +379,7 @@ export const make = Effect.gen(function* () { content: input.content, reacted: input.reacted, }) - .pipe(Effect.mapError(fail("setReaction"))), + .pipe(writing(input), Effect.mapError(fail("setReaction"))), setThreadResolution: (input) => cli @@ -328,7 +390,7 @@ export const make = Effect.gen(function* () { discussionId: input.threadId, resolved: input.resolved, }) - .pipe(Effect.mapError(fail("setThreadResolution"))), + .pipe(writing(input), Effect.mapError(fail("setThreadResolution"))), }; return provider; From 6d7c17f6bb8daa450205c1ea6387fc9c45970b56 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Mon, 12 Oct 2026 00:11:37 +0000 Subject: [PATCH 02/10] fix(source-control): preserve fresh reads across account changes --- .../src/sourceControl/builtInDrivers.ts | 2 +- .../server/AzureDevOpsPullRequestCli.test.ts | 8 +- .../server/AzureDevOpsPullRequestProvider.ts | 36 ++- .../src/server/ForgejoCli.ts | 29 ++- .../src/server/ForgejoPullRequestProvider.ts | 24 +- .../ForgejoSourceControlProvider.test.ts | 76 ++++-- .../server/GitCafePullRequestProvider.test.ts | 196 +++++++++++++++ .../src/server/GitCafePullRequestProvider.ts | 37 ++- .../src/server/GitHubPullRequestApi.test.ts | 3 +- .../src/server/GitHubPullRequestApi.ts | 10 +- .../src/server/GitLabPullRequestCli.test.ts | 229 ++++++++++++++++++ .../src/server/GitLabPullRequestProvider.ts | 40 ++- 12 files changed, 616 insertions(+), 74 deletions(-) diff --git a/apps/server/src/sourceControl/builtInDrivers.ts b/apps/server/src/sourceControl/builtInDrivers.ts index 56805031ab1c..74d1d7c80432 100644 --- a/apps/server/src/sourceControl/builtInDrivers.ts +++ b/apps/server/src/sourceControl/builtInDrivers.ts @@ -28,7 +28,7 @@ import type { SourceControlDriver } from "@t3tools/source-control-core/server/dr import * as Undici from "@effect/platform-node/Undici"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; -import { FetchHttpClient } from "effect/http"; +import * as FetchHttpClient from "effect/http/FetchHttpClient"; import * as ServerSourceControlHost from "./ServerSourceControlHost.ts"; diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts index cd39d96c3036..0e12bca31275 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts @@ -1008,7 +1008,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { json({ changeEntries: [ { changeType: "add", item: { path: "/huge.bin", objectId: "8f80" } }, - { changeType: "add", item: { path: "/DEMO.md", objectId: "0ca4" } }, + { changeType: "add", item: { path: "/docs/%scope\nDEMO.md", objectId: "0ca4" } }, ], }), ), @@ -1029,7 +1029,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { const provider = yield* AzureDevOpsPullRequestProvider.make; const slice = yield* provider.getDiff({ - cwd: "/w", + cwd: "/w\n%repo", repository: "web", host: "dev.azure.com", number: 42, @@ -1039,6 +1039,10 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { expect(slice.patch).toContain("diff --git a/huge.bin b/huge.bin"); // And the file behind it still renders, which is the point of giving up on one file. expect(slice.patch).toContain("+hello"); + for (const [call] of mockedExecute.mock.calls) { + expect(call.cwd).toBe("/w\n%repo"); + } + expect(argsOfCall(4)).toContain("path=/docs/%scope\nDEMO.md"); }), ); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index 9b94b032980e..a8017fd47876 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -162,11 +162,14 @@ export const make = Effect.gen(function* () { // Made once with the provider, which the registry builds once, so this is the whole build's // allowance rather than one request's. const diffSpawns = yield* Semaphore.make(MAX_DIFF_SPAWNS); + const cacheKey = (...parts: ReadonlyArray) => + parts.map((part) => encodeURIComponent(String(part))).join("\n"); // Diff slices and context expansion read the same immutable blobs. Bound both count and size. const itemContents = yield* Cache.makeWith( (key: string) => { - const [cwd = "", project = "", repository = "", commit = "", ...pathParts] = key.split("\n"); - const path = pathParts.join("\n"); + const [cwd = "", project = "", repository = "", commit = "", path = ""] = key + .split("\n") + .map(decodeURIComponent); return diffSpawns.withPermits(1)( cli.readItemContent({ cwd, location: { project, repository }, commit, path }), ); @@ -182,8 +185,12 @@ export const make = Effect.gen(function* () { const readItemContent = (input: Parameters[0]) => Cache.get( itemContents, - [input.cwd, input.location.project, input.location.repository, input.commit, input.path].join( - "\n", + cacheKey( + input.cwd, + input.location.project, + input.location.repository, + input.commit, + input.path, ), ); @@ -257,7 +264,7 @@ export const make = Effect.gen(function* () { */ const pullRequestReads = yield* Cache.makeWith( (key: string) => { - const [cwd = "", number = ""] = key.split("\n"); + const [cwd = "", number = ""] = key.split("\n").map(decodeURIComponent); return cli .getPullRequest({ cwd, number: Number(number) }) .pipe( @@ -270,11 +277,11 @@ export const make = Effect.gen(function* () { ); const readPullRequest = (input: { readonly cwd: string; readonly number: number }) => - Cache.get(pullRequestReads, `${input.cwd}\n${input.number}`); + Cache.get(pullRequestReads, cacheKey(input.cwd, input.number)); /** Run after a write, so a read asked for once it lands cannot join one that started before. */ const forgetRead = (input: { readonly cwd: string; readonly number: number }) => - Cache.invalidate(pullRequestReads, `${input.cwd}\n${input.number}`); + Cache.invalidate(pullRequestReads, cacheKey(input.cwd, input.number)); const locationOf = (input: { readonly cwd: string; readonly number: number }) => { const held = knownLocation(input); @@ -284,17 +291,18 @@ export const make = Effect.gen(function* () { }; const iterationKey = (scope: IterationScope, iterationId?: number) => - [ + cacheKey( scope.cwd, scope.location.project, scope.location.repository, scope.number, ...(iterationId === undefined ? [] : [iterationId]), - ].join("\n"); + ); const fromIterationKey = (key: string) => { - const [cwd = "", project = "", repository = "", number = "", iterationId = ""] = - key.split("\n"); + const [cwd = "", project = "", repository = "", number = "", iterationId = ""] = key + .split("\n") + .map(decodeURIComponent); return { cwd, location: { project, repository }, @@ -304,7 +312,11 @@ export const make = Effect.gen(function* () { }; const changesKey = (scope: IterationScope, iteration: AzureDevOpsIteration) => - [iterationKey(scope, iteration.id), iteration.headCommit, iteration.mergeBaseCommit].join("\n"); + [ + iterationKey(scope, iteration.id), + encodeURIComponent(iteration.headCommit), + encodeURIComponent(iteration.mergeBaseCommit), + ].join("\n"); /** * The pushes a pull request has had, shared by overlapping reads the way a pull request read is. diff --git a/packages/source-control-forgejo/src/server/ForgejoCli.ts b/packages/source-control-forgejo/src/server/ForgejoCli.ts index 9c9b2e6936c6..e98fb4d9b99d 100644 --- a/packages/source-control-forgejo/src/server/ForgejoCli.ts +++ b/packages/source-control-forgejo/src/server/ForgejoCli.ts @@ -132,6 +132,7 @@ export interface ForgejoApiInput extends ForgejoRepositoryInput { readonly path: string; readonly method?: "GET" | "POST" | "PUT" | "PATCH" | "DELETE"; readonly body?: unknown; + readonly freshCredentials?: boolean; } export class ForgejoCli extends Context.Service< @@ -662,6 +663,7 @@ export const make = Effect.gen(function* () { (key: string) => { const [cwd = "", reference, repository, host, remoteUrl, baseUrl, requestedHost, scope] = key .split("\n") + .map(decodeURIComponent) .map((value) => value || undefined); return resolveTarget( { cwd, reference, repository, host, remoteUrl, providerBaseUrl: baseUrl, requestedHost }, @@ -680,16 +682,27 @@ export const make = Effect.gen(function* () { input.context?.provider.baseUrl, input.context?.requestedHost, hostOnly ? "host" : "", - ].join("\n"); + ] + .map((part) => encodeURIComponent(part ?? "")) + .join("\n"); const resolveRepository = (input: ForgejoRepositoryInput) => Cache.get(targets, targetKey(input)); const api = Effect.fn("ForgejoCli.api")(function* (input: ForgejoApiInput) { - const repository = yield* Cache.get( - targets, - targetKey( - input, - input.path.replace(/^\/+/, "") === "user" && (!input.method || input.method === "GET"), - ), - ); + const hostOnly = + input.path.replace(/^\/+/, "") === "user" && (!input.method || input.method === "GET"); + const repository = yield* input.freshCredentials + ? resolveTarget( + { + cwd: input.cwd, + reference: input.reference, + repository: input.repository, + host: input.host, + remoteUrl: input.context?.remoteUrl, + providerBaseUrl: input.context?.provider.baseUrl, + requestedHost: input.context?.requestedHost, + }, + hostOnly, + ) + : Cache.get(targets, targetKey(input, hostOnly)); const stdin = input.body === undefined ? undefined diff --git a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts index ed5fbbff4087..78917d4c4e81 100644 --- a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts +++ b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts @@ -150,16 +150,20 @@ export const make = Effect.gen(function* () { return { items, truncated: true }; }); // Share pending reads only: credentials and repository permissions can change between requests. + const cacheKey = (...parts: ReadonlyArray) => + parts.map((part) => encodeURIComponent(String(part ?? ""))).join("\n"); const pulls = yield* Cache.makeWith( (key: string) => { - const [cwd = "", repository = "", host = "", number = ""] = key.split("\n"); + const [cwd = "", repository = "", host = "", number = ""] = key + .split("\n") + .map(decodeURIComponent); const ref = { cwd, repository, host, number: Number(number) }; return read({ ...ref, path: pullPath(ref) }, ForgejoPullRequest); }, { capacity: 64, timeToLive: () => 0 }, ); const getPull = (input: ProviderRepositoryRef & { readonly number: number }) => - Cache.get(pulls, [input.cwd, input.repository, input.host, input.number].join("\n")); + Cache.get(pulls, cacheKey(input.cwd, input.repository, input.host, input.number)); const write = (input: ForgejoCli.ForgejoApiInput) => request(input).pipe( Effect.onExit(() => Cache.invalidateAll(pulls)), @@ -167,7 +171,7 @@ export const make = Effect.gen(function* () { ); const repos = yield* Cache.makeWith( (key: string) => { - const [cwd = "", repository = "", host = ""] = key.split("\n"); + const [cwd = "", repository = "", host = ""] = key.split("\n").map(decodeURIComponent); const ref = { cwd, repository, host }; return read({ ...ref, path: repoPath(ref) }, ForgejoRepository); }, @@ -175,7 +179,7 @@ export const make = Effect.gen(function* () { ); const viewers = yield* Cache.makeWith( (key: string) => { - const [cwd = "", repository, host] = key.split("\n"); + const [cwd = "", repository, host] = key.split("\n").map(decodeURIComponent); return read( { cwd, ...(repository ? { repository } : {}), ...(host ? { host } : {}), path: "user" }, ForgejoUser, @@ -184,12 +188,12 @@ export const make = Effect.gen(function* () { { capacity: 32, timeToLive: () => 0 }, ); const getRepo = (input: ProviderRepositoryRef) => - Cache.get(repos, [input.cwd, input.repository, input.host].join("\n")); + Cache.get(repos, cacheKey(input.cwd, input.repository, input.host)); const getViewer = (input: { readonly cwd: string; readonly repository?: string; readonly host?: string; - }) => Cache.get(viewers, [input.cwd, input.repository, input.host].join("\n")); + }) => Cache.get(viewers, cacheKey(input.cwd, input.repository, input.host)); const permissions = ( repo: typeof ForgejoRepository.Type, pr: typeof ForgejoPullRequest.Type, @@ -220,8 +224,14 @@ export const make = Effect.gen(function* () { const getPermissions = Effect.fn("ForgejoPullRequestProvider.getPermissions")(function* ( input: ProviderRepositoryRef & { readonly number: number }, ) { + // A write must not join a detail read started before the account or access changed. + const fresh = { ...input, freshCredentials: true }; const [repo, pr, viewer] = yield* Effect.all( - [getRepo(input), getPull(input), getViewer(input)], + [ + read({ ...fresh, path: repoPath(input) }, ForgejoRepository), + read({ ...fresh, path: pullPath(input) }, ForgejoPullRequest), + read({ ...fresh, path: "user" }, ForgejoUser).pipe(Effect.map((user) => user.login)), + ], { concurrency: 3 }, ); return permissions(repo, pr, viewer); diff --git a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts index 0985d462a236..68d2945bebb7 100644 --- a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts +++ b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts @@ -37,6 +37,7 @@ it.effect("refreshes Forgejo viewer permissions immediately after credentials ch let account = "alice"; const requests: string[] = []; const renewals: string[] = []; + let holdViewer: Effect.Effect = Effect.void; return Effect.gen(function* () { const cli = yield* ForgejoCli.make; const provider = yield* ForgejoPullRequestProvider.make.pipe( @@ -47,14 +48,22 @@ it.effect("refreshes Forgejo viewer permissions immediately after credentials ch assert.deepStrictEqual(alice.verdicts, ["comment"]); assert.include(alice.actions, "merge"); assert.strictEqual(alice.labels, true); + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + holdViewer = Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))); + const detail = yield* provider.getChangeRequest(input).pipe(Effect.forkChild); + yield* Deferred.await(started); account = "bob"; + holdViewer = Effect.void; const bob = yield* provider.getViewerPermissions(input); assert.deepStrictEqual(bob.verdicts, ["comment", "approve", "request-changes"]); assert.deepStrictEqual(bob.actions, []); assert.strictEqual(bob.labels, false); assert.strictEqual(bob.requestReviewers, false); - assert.strictEqual(requests.length, 6); + assert.strictEqual(requests.length, 9); assert.deepStrictEqual(renewals, ["alice", "bob"]); + yield* Deferred.succeed(release, undefined); + assert.deepStrictEqual((yield* Fiber.join(detail)).viewerPermissions?.verdicts, ["comment"]); }).pipe( Effect.provideService( FileSystem.FileSystem, @@ -90,9 +99,11 @@ it.effect("refreshes Forgejo viewer permissions immediately after credentials ch merged_at: null, labels: [], } - : { full_name: "acme/web", permissions: { push: account === "alice", admin: false } }; - return Effect.succeed( - HttpClientResponse.fromWeb(request, new Response(encodeJson(response))), + : request.url.includes("/statuses/") + ? [] + : { full_name: "acme/web", permissions: { push: account === "alice", admin: false } }; + return (request.url.endsWith("/user") ? holdViewer : Effect.void).pipe( + Effect.as(HttpClientResponse.fromWeb(request, new Response(encodeJson(response)))), ); }), ), @@ -118,13 +129,31 @@ it.effect("refreshes Forgejo viewer permissions immediately after credentials ch it.effect("reselects the current tea login between API requests", () => { let account = "alice"; const selected: string[] = []; + let holdLogin: Effect.Effect = Effect.void; return Effect.gen(function* () { const cli = yield* ForgejoCli.make; - const input = { cwd: "/repo", repository: "https://forgejo.test/acme/web", path: "user" }; + const input = { + cwd: "/repo\n%work", + repository: "https://forgejo.test/acme/web", + path: "user", + }; assert.strictEqual((yield* cli.api(input)).stdout, '{"login":"alice"}'); account = "bob"; assert.strictEqual((yield* cli.api(input)).stdout, '{"login":"bob"}'); - assert.deepStrictEqual(selected, ["alice", "bob"]); + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + holdLogin = Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))); + const pending = yield* cli.api(input).pipe(Effect.forkChild); + yield* Deferred.await(started); + account = "carol"; + holdLogin = Effect.void; + assert.strictEqual( + (yield* cli.api({ ...input, freshCredentials: true })).stdout, + '{"login":"carol"}', + ); + yield* Deferred.succeed(release, undefined); + assert.strictEqual((yield* Fiber.join(pending)).stdout, '{"login":"bob"}'); + assert.deepStrictEqual(selected, ["alice", "bob", "carol", "bob"]); }).pipe( Effect.provideService( FileSystem.FileSystem, @@ -139,19 +168,28 @@ it.effect("reselects the current tea login between API requests", () => { process: { run: (input) => { assert.strictEqual(input.command, "tea"); + assert.strictEqual(input.cwd, "/repo\n%work"); if (input.args[0] === "login") - return Effect.succeed( - processOutput( - encodeJson([ - { name: account, url: "https://forgejo.test", user: account, default: "true" }, - ]), + return holdLogin.pipe( + Effect.as( + processOutput( + encodeJson([ + { + name: account, + url: "https://forgejo.test", + user: account, + default: "true", + }, + ]), + ), ), ); assert.strictEqual(input.args[0], "api"); - assert.strictEqual(input.args[input.args.indexOf("--login") + 1], account); - selected.push(account); + const login = input.args[input.args.indexOf("--login") + 1] ?? ""; + assert.notStrictEqual(login, ""); + selected.push(login); return Effect.succeed( - processOutput(encodeJson({ login: account }), { stderr: "HTTP/1.1 200 OK\n" }), + processOutput(encodeJson({ login }), { stderr: "HTTP/1.1 200 OK\n" }), ); }, }, @@ -173,6 +211,7 @@ it.effect("shares pending Forgejo pull reads and refreshes after writes and fail Layer.mock(ForgejoCli.ForgejoCli)({ api: (input) => Effect.gen(function* () { + assert.strictEqual(input.cwd, "/repo\n%work"); paths.push(input.path); if (input.method === "PATCH") { state = "closed"; @@ -223,14 +262,9 @@ it.effect("shares pending Forgejo pull reads and refreshes after writes and fail const summary = provider.getChangeRequestSummary; const checks = provider.getChangeRequestChecks; if (!summary || !checks) return yield* Effect.die("narrow reads missing"); - const input = { cwd: "/repo", repository: "acme/web", host: "forgejo.test", number: 42 }; + const input = { cwd: "/repo\n%work", repository: "acme/web", host: "forgejo.test", number: 42 }; const reads = yield* Effect.all( - [ - summary(input), - checks(input), - provider.getViewerPermissions(input), - provider.getChangeRequest(input), - ], + [summary(input), checks(input), provider.getChangeRequest(input)], { concurrency: "unbounded" }, ).pipe(Effect.forkChild); yield* Deferred.await(started); diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts index 0603dfec80ec..0e737f509550 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts @@ -19,7 +19,10 @@ import { assert, describe, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as Context from "effect/Context"; +import * as Deferred from "effect/Deferred"; +import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; +import * as Redacted from "effect/Redacted"; import * as Schema from "effect/Schema"; import * as FetchHttpClient from "effect/http/FetchHttpClient"; import * as HttpClient from "effect/http/HttpClient"; @@ -337,6 +340,95 @@ describe("GitCafePullRequestProvider", () => { }).pipe(Effect.provide(server.layer)); }); + it.effect("refreshes permissions while a pull read with old credentials is pending", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + let token = "old-token"; + const authorizations: Array = []; + const client = HttpClient.make((request) => { + const authorization = request.headers.authorization; + authorizations.push(authorization); + const old = authorization === "Bearer old-token"; + return ( + old + ? Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))) + : Effect.void + ).pipe( + Effect.as( + HttpClientResponse.fromWeb( + request, + new Response( + encodeJson({ + ...pull, + title: old ? "Old credentials" : "New credentials", + capabilities: { ...pull.capabilities, edit: !old }, + }), + ), + ), + ), + ); + }); + const provider = yield* GitCafePullRequestProvider.make.pipe( + Effect.provide( + GitCafeApi.layer.pipe( + Layer.provide(Layer.succeed(HttpClient.HttpClient, client)), + Layer.provide( + Layer.succeed(GitCafeCredentials.GitCafeCredentials, { + get: (host) => + Effect.sync(() => ({ + host, + token: Redacted.make(token), + source: "settings" as const, + })), + invalidate: () => Effect.void, + cliEnv: () => Effect.succeed({}), + }), + ), + ), + ), + ); + const pending = yield* provider.getChangeRequestPreview!(target).pipe(Effect.forkChild); + yield* Deferred.await(started); + token = "new-token"; + assert.strictEqual((yield* provider.getViewerPermissions(target)).editChangeRequest, true); + yield* Deferred.succeed(release, undefined); + yield* Fiber.join(pending); + // The late old response must not replace the fresh pull used for permissions. + assert.strictEqual( + (yield* provider.getChangeRequestSummary!(target)).title, + "New credentials", + ); + assert.deepStrictEqual(authorizations, ["Bearer old-token", "Bearer new-token"]); + }), + ); + + it.effect.each([true, false])("invalidates a pull after a write succeeds: %s", (succeeds) => { + let reads = 0; + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": () => ({ + ...pull, + title: reads++ === 0 ? "Before" : "After", + }), + ...(succeeds ? { "POST /repos/owner/repo/pulls/7/close": {} } : {}), + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + assert.strictEqual((yield* provider.getChangeRequestPreview!(target)).title, "Before"); + const result = yield* provider.runAction({ ...target, action: "close" }).pipe(Effect.result); + assert.strictEqual(result._tag, succeeds ? "Success" : "Failure"); + assert.strictEqual((yield* provider.getChangeRequestPreview!(target)).title, "After"); + assert.deepStrictEqual( + server.sent.map((request) => `${request.method} ${request.path}`), + [ + "GET /repos/owner/repo/pulls/7", + "POST /repos/owner/repo/pulls/7/close", + "GET /repos/owner/repo/pulls/7", + ], + ); + }).pipe(Effect.provide(server.layer)); + }); + it.effect("reads the diff from the pull request's changed files", () => { const file = { path: "file.ts", @@ -692,6 +784,110 @@ describe("GitCafePullRequestProvider", () => { }).pipe(Effect.provide(server.layer)); }); + it.effect.each(["changes", "diff-files"] as const)( + "retries line stats after a failed %s read and retains the complete retry", + (failedRead) => { + const snapshot = { version: 4, headOid, comparisonBaseOid: baseOid }; + let recovered = false; + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": pull, + "GET /repos/owner/repo/pulls/7/status": { + merge: { conflicts: "unknown", fastForward: true, strategies: ["merge"] }, + checks: { pending: 0, failing: 0, total: 0 }, + }, + [`GET /repos/owner/repo/commits/${headOid}/checks`]: { items: [], next: null }, + "GET /repos/owner/repo/pulls/7/changes": () => + !recovered && failedRead === "changes" + ? {} + : { ...snapshot, items: [{ path: "file.ts" }], next: null }, + "POST /repos/owner/repo/pulls/7/diff-files": () => + !recovered && failedRead === "diff-files" + ? {} + : { ...snapshot, items: [{ path: "file.ts", additions: 5, deletions: 2 }] }, + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + assert.deepInclude(yield* provider.getChangeRequest(target), { + additions: 0, + changedFiles: failedRead === "changes" ? 0 : 1, + }); + recovered = true; + for (let read = 0; read < 2; read++) { + assert.deepInclude(yield* provider.getChangeRequest(target), { + additions: 5, + deletions: 2, + changedFiles: 1, + }); + } + assert.strictEqual( + server.sent.filter((request) => request.path.endsWith("/changes")).length, + 2, + ); + assert.strictEqual( + server.sent.filter((request) => request.path.endsWith("/diff-files")).length, + failedRead === "changes" ? 1 : 2, + ); + }).pipe(Effect.provide(server.layer)); + }, + ); + + it.effect("retries line stats when a changes page leaves an unread cursor", () => { + const snapshot = { version: 4, headOid, comparisonBaseOid: baseOid }; + let reads = 0; + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": pull, + "GET /repos/owner/repo/pulls/7/status": { + merge: { conflicts: "unknown", fastForward: true, strategies: ["merge"] }, + checks: { pending: 0, failing: 0, total: 0 }, + }, + [`GET /repos/owner/repo/commits/${headOid}/checks`]: { items: [], next: null }, + "GET /repos/owner/repo/pulls/7/changes": () => + reads++ === 0 + ? { ...snapshot, items: [], next: "unread" } + : { ...snapshot, items: [{ path: "file.ts", additions: 5, deletions: 2 }], next: null }, + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + assert.deepInclude(yield* provider.getChangeRequest(target), { + additions: 0, + changedFiles: 0, + }); + assert.deepInclude(yield* provider.getChangeRequest(target), { + additions: 5, + changedFiles: 1, + }); + assert.strictEqual(reads, 2); + }).pipe(Effect.provide(server.layer)); + }); + + it.effect.each([baseOid, null])( + "does not retain line stats for a comparison base different from %s", + (observedBaseOid) => { + let reads = 0; + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": { ...pull, observedBaseOid }, + "GET /repos/owner/repo/pulls/7/status": { + merge: { conflicts: "unknown", fastForward: true, strategies: ["merge"] }, + checks: { pending: 0, failing: 0, total: 0 }, + }, + [`GET /repos/owner/repo/commits/${headOid}/checks`]: { items: [], next: null }, + "GET /repos/owner/repo/pulls/7/changes": () => ({ + version: 4, + headOid, + comparisonBaseOid: "another-base", + items: [{ path: "file.ts", additions: ++reads, deletions: 0 }], + next: null, + }), + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + assert.strictEqual((yield* provider.getChangeRequest(target)).additions, 1); + assert.strictEqual((yield* provider.getChangeRequest(target)).additions, 2); + assert.strictEqual(reads, 2); + }).pipe(Effect.provide(server.layer)); + }, + ); + it.effect("fences a native merge on the target branch's current commit", () => { const branchOid = "fedcba9876543210fedcba9876543210fedcba98"; const server = fakeGitCafe({ diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts index 3d5f5ddbead1..d6e27ca90b9e 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts @@ -248,11 +248,15 @@ export const make = Effect.gen(function* () { const pullPath = (operation: string, input: PullRef) => repositoryPath(operation, input).pipe(Effect.map((base) => `${base}/pulls/${input.number}`)); - const pullKey = (input: PullRef) => `${input.host}\n${input.repository}\n${input.number}`; + let readGeneration = 0; + const pullKey = (input: PullRef) => + [input.host, input.repository, input.number, readGeneration] + .map((value) => encodeURIComponent(String(value ?? ""))) + .join("\n"); // A failed read is not held, so the next caller asks GitCafe again. const pulls = yield* Cache.makeWith( (key: string) => { - const [host = "", repository = "", number = ""] = key.split("\n"); + const [host = "", repository = "", number = ""] = key.split("\n").map(decodeURIComponent); return read( "readPull", { host }, @@ -280,7 +284,13 @@ export const make = Effect.gen(function* () { ); /** Drops a pull's shared read once a write on it settles, whatever the outcome. */ - const forgetPull = (input: PullRef) => Cache.invalidate(pulls, pullKey(input)); + // A late pending read may repopulate an invalidated entry, so abandon its generation too. + const forgetPull = (input: PullRef) => + Effect.suspend(() => { + const key = pullKey(input); + readGeneration++; + return Cache.invalidate(pulls, key); + }); /** Every page of a cursor list, up to `MAX_PAGES`; `next` stays set when more remained. */ const readPages = , unknown, never, never>>( @@ -493,11 +503,12 @@ export const make = Effect.gen(function* () { /** * Line totals held by the revision they count, which never changes, so reopening an unchanged * pull request skips the `/changes` pages and hunk reads. Only a complete count of the head - * the pull reported is held. + * and base the pull reported is held. */ const lineStats = yield* Cache.makeWith( (key: string) => { - const [host = "", repository = "", number = "", version = "", headOid = ""] = key.split("\n"); + const [host = "", repository = "", number = "", version = "", headOid = "", baseOid = ""] = + key.split("\n").map(decodeURIComponent); return readLineStats( { host, repository, number: Number(number) }, Number(version), @@ -505,7 +516,11 @@ export const make = Effect.gen(function* () { ).pipe( Effect.map(({ revision, complete, ...stats }) => ({ stats, - held: complete && revision?.version === Number(version) && revision.headOid === headOid, + held: + complete && + revision?.version === Number(version) && + revision.headOid === headOid && + revision.baseOid === baseOid, })), ); }, @@ -567,7 +582,7 @@ export const make = Effect.gen(function* () { /** The viewer's actor on a repository; a viewer GitCafe knows no actor for is asked again. */ const viewerActors = yield* Cache.makeWith( (key: string) => { - const [host = "", repository = "", viewer = ""] = key.split("\n"); + const [host = "", repository = "", viewer = ""] = key.split("\n").map(decodeURIComponent); return read( "listChangeRequests", { host }, @@ -608,7 +623,9 @@ export const make = Effect.gen(function* () { // or a rate limit still fails, since the service acts on those for the whole host. const actor = yield* Cache.get( viewerActors, - `${input.host}\n${input.repository}\n${input.viewer.toLowerCase()}`, + [input.host, input.repository, input.viewer.toLowerCase()] + .map((value) => encodeURIComponent(String(value ?? ""))) + .join("\n"), ).pipe( Effect.map(Option.some), Effect.catchTags({ @@ -700,7 +717,9 @@ export const make = Effect.gen(function* () { pull.version, pull.headOid, pull.observedBaseOid ?? "", - ].join("\n"), + ] + .map((value) => encodeURIComponent(String(value ?? ""))) + .join("\n"), ).pipe( Effect.map(({ stats }) => stats), Effect.orElseSucceed(() => null), diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index a799387f6b2b..0d2337d32e5a 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -3699,6 +3699,7 @@ layer("GitHubPullRequestApi.layer", (it) => { let detailReads = 0; mockedExecute.mockImplementation((call) => Effect.gen(function* () { + expect(call.host).toBe("github.detail-after-write.test"); if (call.kind === "graphql" && call.query.trimStart().startsWith("mutation")) { return output("{}"); } @@ -3715,7 +3716,7 @@ layer("GitHubPullRequestApi.layer", (it) => { ); const cli = yield* GitHubPullRequestApi.GitHubPullRequestApi; const input = { - cwd: "/w", + cwd: "/w\n%repo", repository: "acme/web", host: "github.detail-after-write.test", number: 7, diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 313e4705cb4d..fa4c66a93390 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -1761,11 +1761,15 @@ export const make = Effect.gen(function* () { yield* SourceControlRateLimit.Interactive, yield* SourceControlRateLimit.CredentialScope, pinned?.credentialFingerprint ?? "", - ].join("\n"); + ] + .map((part) => encodeURIComponent(String(part))) + .join("\n"); }); const detailsInFlight = yield* Cache.makeWith( (key: string) => { - const [cwd = "", host = "", repository = "", number = ""] = key.split("\n"); + const [cwd = "", host = "", repository = "", number = ""] = key + .split("\n") + .map(decodeURIComponent); return getPullRequestDetail({ cwd, host, repository, number: Number(number) }); }, { capacity: 64, timeToLive: () => 0 }, @@ -1780,7 +1784,7 @@ export const make = Effect.gen(function* () { headSha = "", headBranch = "", headRepositoryOwner = "", - ] = key.split("\n"); + ] = key.split("\n").map(decodeURIComponent); return listWorkflowRunsRequiringApproval({ cwd, host, diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts index b4546162f30c..5974c8e38552 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts @@ -1,10 +1,14 @@ import { afterEach, assert, expect, it, vi } from "@effect/vitest"; +import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; +import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; +import * as TestClock from "effect/testing/TestClock"; import { ChildProcessSpawner } from "effect/process"; import * as GitLabCli from "./GitLabCli.ts"; import * as GitLabPullRequestCli from "./GitLabPullRequestCli.ts"; +import * as GitLabPullRequestProvider from "./GitLabPullRequestProvider.ts"; const mockedExecute = vi.fn(); @@ -1533,4 +1537,229 @@ layer("GitLabPullRequestCli.layer", (it) => { assert.strictEqual(mockedExecute.mock.calls.length, 3); }), ); + + it.effect( + "shares concurrent merge request reads for one second without rereading project settings", + () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + let detailReads = 0; + let settingsReads = 0; + mockedExecute.mockImplementation((request) => + Effect.gen(function* () { + if (request.args[1]?.includes("?license=false")) { + settingsReads++; + return output('{"merge_method":"merge"}'); + } + detailReads++; + yield* Deferred.succeed(started, undefined); + yield* Deferred.await(release); + return output(mergeRequestJson({ title: `Read ${detailReads}` })); + }), + ); + const provider = yield* GitLabPullRequestProvider.make; + assert.isDefined(provider.getChangeRequestChecks); + assert.isDefined(provider.getChangeRequestPreview); + assert.isDefined(provider.getChangeRequestSummary); + const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; + const reads = yield* Effect.all( + [ + provider.getChangeRequest(input), + provider.getChangeRequestChecks(input), + provider.getChangeRequestPreview(input), + provider.getChangeRequestSummary(input), + ], + { concurrency: 4 }, + ).pipe(Effect.forkChild); + yield* Deferred.await(started); + yield* Deferred.succeed(release, undefined); + const [detail, checks, preview, summary] = yield* Fiber.join(reads); + + expect([detail.title, preview.title, summary.title]).toEqual([ + "Read 1", + "Read 1", + "Read 1", + ]); + expect(checks.state).toBe("open"); + expect(detailReads).toBe(1); + expect(settingsReads).toBe(1); + yield* TestClock.adjust("900 millis"); + expect((yield* provider.getChangeRequestSummary(input)).title).toBe("Read 1"); + yield* TestClock.adjust("200 millis"); + expect((yield* provider.getChangeRequest(input)).title).toBe("Read 2"); + expect(detailReads).toBe(2); + expect(settingsReads).toBe(1); + }), + ); + + it.effect("rereads permissions before each write and shares that fresh answer with checks", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce( + Effect.succeed(output(mergeRequestJson({ user: { can_merge: true } }))), + ) + .mockReturnValueOnce( + Effect.succeed(output(mergeRequestJson({ user: { can_merge: false } }))), + ) + .mockReturnValueOnce( + Effect.succeed(output(mergeRequestJson({ user: { can_merge: true } }))), + ); + const provider = yield* GitLabPullRequestProvider.make; + assert.isDefined(provider.getChangeRequestChecks); + const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; + + yield* provider.getChangeRequestChecks(input); + expect((yield* provider.getViewerPermissions(input)).actions).not.toContain("merge"); + expect((yield* provider.getViewerPermissions(input)).actions).toContain("merge"); + expect((yield* provider.getChangeRequestChecks(input)).state).toBe("open"); + expect(mockedExecute).toHaveBeenCalledTimes(3); + }), + ); + + it.effect( + "reuses the permission read for reviewer candidates and preserves existing reviewers in a write", + () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) + .mockReturnValueOnce( + Effect.succeed( + output(JSON.stringify([author, reviewer, { id: 9, username: "hubot" }])), + ), + ) + .mockReturnValueOnce(Effect.succeed(output("{}"))) + .mockReturnValueOnce( + Effect.succeed(output(mergeRequestJson({ title: "After review request" }))), + ); + const provider = yield* GitLabPullRequestProvider.make; + assert.isDefined(provider.getChangeRequestSummary); + const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; + + yield* provider.getViewerPermissions(input); + const candidates = yield* provider.listReviewerCandidates(input); + expect( + candidates.candidates.map(({ login, isRequested }) => ({ login, isRequested })), + ).toEqual([ + { login: "octocat", isRequested: true }, + { login: "hubot", isRequested: false }, + ]); + yield* provider.setReviewerRequest({ + ...input, + reviewers: [{ id: "9", kind: "user" }], + requested: true, + }); + + expect(JSON.parse(callAt(2).stdin ?? "")).toEqual({ reviewer_ids: [5, 9] }); + expect((yield* provider.getChangeRequestSummary(input)).title).toBe("After review request"); + expect(mockedExecute).toHaveBeenCalledTimes(4); + }), + ); + + it.effect.each([false, true])( + "starts a fresh read after a write while an old read is pending (failed: %s)", + (failedWrite) => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + let detailReads = 0; + mockedExecute.mockImplementation((request) => { + if (request.args.includes("PUT")) { + return failedWrite + ? Effect.fail( + new GitLabCli.GitLabCliAuthenticationError({ + command: "glab", + operation: "execute", + cwd: request.cwd, + cause: new Error("write refused"), + }), + ) + : Effect.succeed(output("{}")); + } + return Effect.gen(function* () { + const older = ++detailReads === 1; + if (older) { + yield* Deferred.succeed(started, undefined); + yield* Deferred.await(release); + } + return output( + mergeRequestJson({ title: older ? "Before mutation" : "After mutation" }), + ); + }); + }); + const provider = yield* GitLabPullRequestProvider.make; + assert.isDefined(provider.getChangeRequestSummary); + assert.isDefined(provider.updateChangeRequest); + const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; + const older = yield* provider.getChangeRequestSummary(input).pipe(Effect.forkChild); + yield* Deferred.await(started); + const write = provider.updateChangeRequest({ ...input, title: "After mutation" }); + if (failedWrite) { + expect((yield* Effect.flip(write)).reason).toBe("unauthenticated"); + } else { + yield* write; + } + + expect((yield* provider.getChangeRequestSummary(input)).title).toBe("After mutation"); + yield* Deferred.succeed(release, undefined); + expect((yield* Fiber.join(older)).title).toBe("Before mutation"); + expect((yield* provider.getChangeRequestSummary(input)).title).toBe("After mutation"); + expect(detailReads).toBe(2); + }), + ); + + it.effect("retries failed merge request and project settings reads immediately", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce(Effect.succeed(output("invalid json"))) + .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))) + .mockReturnValueOnce(Effect.succeed(output("invalid json"))) + .mockReturnValueOnce(Effect.succeed(output('{"merge_method":"merge"}'))); + const provider = yield* GitLabPullRequestProvider.make; + assert.isDefined(provider.getChangeRequestSummary); + const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; + + expect((yield* Effect.flip(provider.getChangeRequestSummary(input))).reason).toBe("failed"); + expect((yield* provider.getChangeRequestSummary(input)).title).toBe("Merge request 7"); + expect((yield* Effect.flip(provider.getChangeRequest(input))).reason).toBe("failed"); + expect((yield* provider.getChangeRequest(input)).mergeCapabilities).toMatchObject({ + merge: true, + }); + expect(mockedExecute).toHaveBeenCalledTimes(4); + }), + ); + + it.effect( + "preserves newlines and percent signs in cwd for merge request and project settings reads", + () => + Effect.gen(function* () { + mockedExecute.mockImplementation((request) => + Effect.succeed( + output( + request.args[1]?.includes("?license=false") + ? '{"merge_method":"merge"}' + : mergeRequestJson({}), + ), + ), + ); + const provider = yield* GitLabPullRequestProvider.make; + const input = { + cwd: "/w/line\n100% project", + repository: "group/subgroup/web", + host: "gitlab.com", + number: 7, + }; + + expect((yield* provider.getChangeRequest(input)).number).toBe(7); + expect(mockedExecute).toHaveBeenCalledTimes(2); + expect(mockedExecute.mock.calls.map(([request]) => request.cwd)).toEqual([ + input.cwd, + input.cwd, + ]); + expect(mockedExecute.mock.calls.map(([request]) => request.args[1])).toEqual([ + "projects/group%2Fsubgroup%2Fweb/merge_requests/7?include_diverged_commits_count=true", + "projects/group%2Fsubgroup%2Fweb?license=false", + ]); + }), + ); }); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index 28cfcfb62e11..d377cf3ff0ec 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -106,29 +106,42 @@ const mergeRequestKey = (input: { readonly cwd: string; readonly repository: string; readonly number: number; -}) => `${input.cwd}\n${input.repository}\n${input.number}`; +}) => + [input.cwd, input.repository, input.number] + .map((value) => encodeURIComponent(String(value ?? ""))) + .join("\n"); export const make = Effect.gen(function* () { const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + let readGeneration = 0; + const readKey = (input: Parameters[0]) => + `${mergeRequestKey(input)}\n${readGeneration}`; // The detail, its checks and the viewer's permissions are all `merge_requests/:iid`, and a page // opening asks for them at once. One read answers the ones that land within a second of it; a // failure is not held, and every write below forgets the merge request it wrote to. const mergeRequests = yield* Cache.makeWith( (key: string) => { - const [cwd = "", repository = "", number = ""] = key.split("\n"); + const [cwd = "", repository = "", number = ""] = key.split("\n").map(decodeURIComponent); return cli.getMergeRequestDetail({ cwd, repository, number: Number(number) }); }, { capacity: 64, timeToLive: (exit) => (Exit.isSuccess(exit) ? "1 second" : 0) }, ); const mergeRequest = (input: Parameters[0]) => - Cache.get(mergeRequests, mergeRequestKey(input)); + Effect.suspend(() => Cache.get(mergeRequests, readKey(input))); + // An invalidated pending lookup can finish later and repopulate its old cache entry. + const forgetMergeRequest = (input: Parameters[0]) => + Effect.suspend(() => { + const key = readKey(input); + readGeneration++; + return Cache.invalidate(mergeRequests, key); + }); // How a project lets its merge requests land is a setting, which moves far more slowly than // the detail every refresh was reading it alongside. const mergeSettings = yield* Cache.makeWith( (key: string) => { - const [cwd = "", repository = ""] = key.split("\n"); + const [cwd = "", repository = ""] = key.split("\n").map(decodeURIComponent); return cli.getProjectMergeCapabilities({ cwd, repository }); }, { capacity: 32, timeToLive: (exit) => (Exit.isSuccess(exit) ? "5 minutes" : 0) }, @@ -139,7 +152,7 @@ export const make = Effect.gen(function* () { const writing = (input: Parameters[0]) => (write: Effect.Effect) => - write.pipe(Effect.ensuring(Cache.invalidate(mergeRequests, mergeRequestKey(input)))); + write.pipe(Effect.ensuring(forgetMergeRequest(input))); const fail = (operation: string) => (error: GitLabPullRequestCli.GitLabPullRequestCliError) => new PullRequestProviderError({ @@ -190,7 +203,15 @@ export const make = Effect.gen(function* () { getChangeRequest: (input) => Effect.all( - [mergeRequest(input), Cache.get(mergeSettings, `${input.cwd}\n${input.repository}`)], + [ + mergeRequest(input), + Cache.get( + mergeSettings, + [input.cwd, input.repository] + .map((value) => encodeURIComponent(String(value ?? ""))) + .join("\n"), + ), + ], { concurrency: 2 }, ).pipe( Effect.mapError(fail("getChangeRequest")), @@ -260,9 +281,8 @@ export const make = Effect.gen(function* () { // there is no cheaper thing to ask GitLab. The server asks this before every write, so it // starts a fresh read; the write after it can then reuse what it read. getViewerPermissions: (input) => { - const key = mergeRequestKey(input); - return Cache.invalidate(mergeRequests, key).pipe( - Effect.andThen(Cache.get(mergeRequests, key)), + return forgetMergeRequest(input).pipe( + Effect.andThen(Effect.suspend(() => mergeRequest(input))), Effect.mapError(fail("getViewerPermissions")), Effect.map(gitLabViewerPermissions), ); @@ -287,7 +307,7 @@ export const make = Effect.gen(function* () { // The merge request the permission check just read, where it is held, rather than a second // read of it; where it is not, the CLI reads it alongside the project's users. listReviewerCandidates: (input) => - Cache.getSuccess(mergeRequests, mergeRequestKey(input)).pipe( + Effect.suspend(() => Cache.getSuccess(mergeRequests, readKey(input))).pipe( Effect.flatMap((held) => cli.listReviewerCandidates({ cwd: input.cwd, From 396b716321ad6d8581882972d25cbb446f0af687 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Mon, 12 Oct 2026 00:32:19 +0000 Subject: [PATCH 03/10] fix(source-control): reselect forgejo credentials before writes --- .../src/server/ForgejoCli.ts | 6 +- .../src/server/ForgejoPullRequestProvider.ts | 11 ++- .../ForgejoSourceControlProvider.test.ts | 93 ++++++++++++------- 3 files changed, 74 insertions(+), 36 deletions(-) diff --git a/packages/source-control-forgejo/src/server/ForgejoCli.ts b/packages/source-control-forgejo/src/server/ForgejoCli.ts index e98fb4d9b99d..c63f98ede281 100644 --- a/packages/source-control-forgejo/src/server/ForgejoCli.ts +++ b/packages/source-control-forgejo/src/server/ForgejoCli.ts @@ -657,8 +657,8 @@ export const make = Effect.gen(function* () { }); return { command, login: login.name, repository, baseUrl: login.url.replace(/\/+$/, "") }; }); - // Share pending resolution, but reselect the login after it finishes so account and remote - // changes apply to the next request without an extra identity check. + // Share pending read resolution. Writes and permission checks reselect the login so they + // cannot join a target captured before an account change. const targets = yield* Cache.makeWith( (key: string) => { const [cwd = "", reference, repository, host, remoteUrl, baseUrl, requestedHost, scope] = key @@ -689,7 +689,7 @@ export const make = Effect.gen(function* () { const api = Effect.fn("ForgejoCli.api")(function* (input: ForgejoApiInput) { const hostOnly = input.path.replace(/^\/+/, "") === "user" && (!input.method || input.method === "GET"); - const repository = yield* input.freshCredentials + const repository = yield* input.freshCredentials || (input.method && input.method !== "GET") ? resolveTarget( { cwd: input.cwd, diff --git a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts index 78917d4c4e81..55693a5ba82d 100644 --- a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts +++ b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts @@ -515,7 +515,16 @@ export const make = Effect.gen(function* () { body: { body: input.body }, }), submitReview: Effect.fn("ForgejoPullRequestProvider.submitReview")(function* (input) { - const pr = yield* getPull(input); + const pr = yield* read( + { + cwd: input.cwd, + repository: input.repository, + host: input.host, + path: pullPath(input), + freshCredentials: true, + }, + ForgejoPullRequest, + ); yield* write({ ...input, path: `${pullPath(input)}/reviews`, diff --git a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts index 68d2945bebb7..75629486bca5 100644 --- a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts +++ b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts @@ -126,7 +126,7 @@ it.effect("refreshes Forgejo viewer permissions immediately after credentials ch ); }); -it.effect("reselects the current tea login between API requests", () => { +it.effect("reselects the current tea login for writes while an older read is pending", () => { let account = "alice"; const selected: string[] = []; let holdLogin: Effect.Effect = Effect.void; @@ -135,7 +135,7 @@ it.effect("reselects the current tea login between API requests", () => { const input = { cwd: "/repo\n%work", repository: "https://forgejo.test/acme/web", - path: "user", + path: "repos/acme/web/pulls/42", }; assert.strictEqual((yield* cli.api(input)).stdout, '{"login":"alice"}'); account = "bob"; @@ -151,9 +151,14 @@ it.effect("reselects the current tea login between API requests", () => { (yield* cli.api({ ...input, freshCredentials: true })).stdout, '{"login":"carol"}', ); + const write = yield* cli + .api({ ...input, method: "PATCH", body: { state: "closed" } }) + .pipe(Effect.forkChild); + yield* Effect.yieldNow; yield* Deferred.succeed(release, undefined); + assert.strictEqual((yield* Fiber.join(write)).stdout, '{"login":"carol"}'); assert.strictEqual((yield* Fiber.join(pending)).stdout, '{"login":"bob"}'); - assert.deepStrictEqual(selected, ["alice", "bob", "carol", "bob"]); + assert.deepStrictEqual(selected, ["alice", "bob", "carol", "carol", "bob"]); }).pipe( Effect.provideService( FileSystem.FileSystem, @@ -288,35 +293,37 @@ it.effect("shares pending Forgejo pull reads and refreshes after writes and fail }), ); -it.effect("submits a Forgejo review without sending its summary in the preliminary GET", () => { +it.effect("submits a Forgejo review using a fresh head and an empty GET body", () => { const methods: string[] = []; + let head = "old-head"; + let holdPull: Effect.Effect = Effect.void; const reviewClient = HttpClient.make((request) => { methods.push(request.method); if (request.method === "GET") { assert.strictEqual(request.url, "https://forgejo.test/api/v1/repos/maria/project/pulls/42"); - return Effect.succeed( - HttpClientResponse.fromWeb( - request, - new Response( - encodeJson({ - number: 42, - title: "Review target", - body: "", - html_url: "https://forgejo.test/maria/project/pulls/42", - user: { login: "maria" }, - state: "open", - merged: false, - head: { ref: "feature", sha: "head", repo: null }, - base: { ref: "main", sha: "base", repo: null }, - created_at: "2026-09-13T00:00:00Z", - updated_at: "2026-09-13T00:00:00Z", - closed_at: null, - merged_at: null, - labels: [], - }), - ), + const response = HttpClientResponse.fromWeb( + request, + new Response( + encodeJson({ + number: 42, + title: "Review target", + body: "", + html_url: "https://forgejo.test/maria/project/pulls/42", + user: { login: "maria" }, + state: "open", + merged: false, + head: { ref: "feature", sha: head, repo: null }, + base: { ref: "main", sha: "base", repo: null }, + created_at: "2026-09-13T00:00:00Z", + updated_at: "2026-09-13T00:00:00Z", + closed_at: null, + merged_at: null, + labels: [], + }), ), ); + assert.strictEqual(request.body._tag, "Empty"); + return holdPull.pipe(Effect.as(response)); } assert.strictEqual(request.method, "POST"); assert.strictEqual( @@ -338,16 +345,36 @@ it.effect("submits a Forgejo review without sending its summary in the prelimina const provider = yield* ForgejoPullRequestProvider.make.pipe( Effect.provideService(ForgejoCli.ForgejoCli, cli), ); - yield* provider.submitReview({ + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + holdPull = Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))); + const summary = provider.getChangeRequestSummary; + if (!summary) return yield* Effect.die("narrow reads missing"); + const pending = yield* summary({ cwd: "/repo", repository: "maria/project", host: "forgejo.test", number: 42, - verdict: "comment", - body: "Review summary", - comments: [], - }); - assert.deepStrictEqual(methods, ["GET", "POST"]); + }).pipe(Effect.forkChild); + yield* Deferred.await(started); + head = "head"; + holdPull = Effect.void; + const review = yield* provider + .submitReview({ + cwd: "/repo", + repository: "maria/project", + host: "forgejo.test", + number: 42, + verdict: "comment", + body: "Review summary", + comments: [], + }) + .pipe(Effect.forkChild); + yield* Effect.yieldNow; + yield* Deferred.succeed(release, undefined); + yield* Fiber.join(review); + assert.deepStrictEqual(methods, ["GET", "GET", "POST"]); + yield* Fiber.join(pending); }).pipe( Effect.provideService(HttpClient.HttpClient, reviewClient), Effect.provideService( @@ -356,7 +383,9 @@ it.effect("submits a Forgejo review without sending its summary in the prelimina exists: () => Effect.succeed(true), readFileString: () => Effect.succeed( - encodeJson({ hosts: { "forgejo.test": { type: "Application", token: "test-token" } } }), + encodeJson({ + hosts: { "forgejo.test": { type: "Application", token: "test-token" } }, + }), ), }), ), From ea1b4fd9491a10c7fd1e05016188d6565aefe42f Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Mon, 12 Oct 2026 00:46:00 +0000 Subject: [PATCH 04/10] fix(source-control): keep credential reads and incomplete diffs fresh --- .../server/AzureDevOpsPullRequestCli.test.ts | 54 ++++++++++--- .../server/AzureDevOpsPullRequestProvider.ts | 6 +- .../src/server/ForgejoCli.ts | 64 +++++---------- .../src/server/ForgejoPullRequestProvider.ts | 65 +++------------- .../ForgejoSourceControlProvider.test.ts | 23 +++--- .../src/server/GitHubPullRequestApi.test.ts | 78 +++++++++++++++++++ .../src/server/GitHubPullRequestApi.ts | 27 +++++-- 7 files changed, 187 insertions(+), 130 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts index 0e12bca31275..aeb63a4ad5cc 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts @@ -1153,7 +1153,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ); - it.effect("stops following pages when one of them does not move the cursor on", () => + it.effect("retries truncated changes without a push and retains complete results", () => Effect.gen(function* () { mockedExecute .mockReturnValueOnce( @@ -1206,17 +1206,35 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { }), ), ), - ); + ) + .mockReturnValueOnce(Effect.succeed(output(json(oneIteration)))) + .mockReturnValueOnce( + Effect.succeed( + output( + json({ + changeEntries: [ + { changeType: "edit", item: { path: "/a.ts", objectId: "8f80" } }, + { changeType: "edit", item: { path: "/b.ts", objectId: "0ca4" } }, + { changeType: "add", item: { path: "/unlisted.ts", objectId: "cafe" } }, + ], + }), + ), + ), + ) + .mockReturnValueOnce(Effect.succeed(output(json(oneIteration)))); const provider = yield* AzureDevOpsPullRequestProvider.make; - assert.isDefined(provider.getFileRevisions); + const read = provider.getFileRevisions; + assert.isDefined(read); - const answer = yield* provider.getFileRevisions({ - cwd: "/w", - repository: "web", - host: "dev.azure.com", - number: 42, - paths: ["a.ts", "b.ts", "unlisted.ts"], - }); + const ask = () => + read({ + cwd: "/w", + repository: "web", + host: "dev.azure.com", + number: 42, + paths: ["a.ts", "b.ts", "unlisted.ts"], + }); + const answer = yield* ask(); // Four reads and no more: a page pointing at where it already is would be read forever. assert.strictEqual(mockedExecute.mock.calls.length, 4); @@ -1226,6 +1244,22 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { ["a.ts", "8f80"], ["b.ts", "0ca4"], ]); + + const recovered = yield* ask(); + assert.strictEqual(mockedExecute.mock.calls.length, 6); + expect(argsOfCall(4)).toContain("pullRequestIterations"); + expect(argsOfCall(5)).toContain("pullRequestIterationChanges"); + expect(argsOfCall(5)).toContain("iterationId=1"); + expect([...recovered.revisions]).toEqual([ + ["a.ts", "8f80"], + ["b.ts", "0ca4"], + ["unlisted.ts", "cafe"], + ]); + + const cached = yield* ask(); + assert.strictEqual(mockedExecute.mock.calls.length, 7); + expect(argsOfCall(6)).toContain("pullRequestIterations"); + expect([...cached.revisions]).toEqual([...recovered.revisions]); }), ); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index a8017fd47876..40d622374324 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -349,13 +349,15 @@ export const make = Effect.gen(function* () { /** * What one push changed, against the merge base. Never changes once the push lands, so the - * detail's file count, every slice of the diff and the viewed marks share one read of it. + * detail's file count, every slice of the diff and the viewed marks share one complete read. + * Incomplete reads are retried rather than keeping missing files for the life of the push. */ const iterationChanges = yield* Cache.makeWith( (key: string) => cli.listIterationChanges(fromIterationKey(key)), { capacity: ITERATION_CHANGES_CAPACITY, - timeToLive: (exit) => (Exit.isSuccess(exit) ? Duration.infinity : Duration.zero), + timeToLive: (exit) => + Exit.isSuccess(exit) && !exit.value.truncated ? Duration.infinity : Duration.zero, }, ); diff --git a/packages/source-control-forgejo/src/server/ForgejoCli.ts b/packages/source-control-forgejo/src/server/ForgejoCli.ts index c63f98ede281..3404b1d5900e 100644 --- a/packages/source-control-forgejo/src/server/ForgejoCli.ts +++ b/packages/source-control-forgejo/src/server/ForgejoCli.ts @@ -1,4 +1,3 @@ -import * as Cache from "effect/Cache"; import * as Context from "effect/Context"; import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; @@ -110,7 +109,7 @@ export interface ForgejoRepositoryInput { readonly host?: string; } -/** What resolving a target reads from a `ForgejoRepositoryInput`, flat so it can key a cache. */ +/** What resolving a target reads from a `ForgejoRepositoryInput`. */ interface ForgejoTargetInput { readonly cwd: string; readonly reference?: string | undefined; @@ -132,7 +131,6 @@ export interface ForgejoApiInput extends ForgejoRepositoryInput { readonly path: string; readonly method?: "GET" | "POST" | "PUT" | "PATCH" | "DELETE"; readonly body?: unknown; - readonly freshCredentials?: boolean; } export class ForgejoCli extends Context.Service< @@ -657,52 +655,26 @@ export const make = Effect.gen(function* () { }); return { command, login: login.name, repository, baseUrl: login.url.replace(/\/+$/, "") }; }); - // Share pending read resolution. Writes and permission checks reselect the login so they - // cannot join a target captured before an account change. - const targets = yield* Cache.makeWith( - (key: string) => { - const [cwd = "", reference, repository, host, remoteUrl, baseUrl, requestedHost, scope] = key - .split("\n") - .map(decodeURIComponent) - .map((value) => value || undefined); - return resolveTarget( - { cwd, reference, repository, host, remoteUrl, providerBaseUrl: baseUrl, requestedHost }, - scope === "host", - ); - }, - { capacity: 64, timeToLive: () => 0 }, - ); - const targetKey = (input: ForgejoRepositoryInput, hostOnly = false) => - [ - input.cwd, - input.reference, - input.repository, - input.host, - input.context?.remoteUrl, - input.context?.provider.baseUrl, - input.context?.requestedHost, - hostOnly ? "host" : "", - ] - .map((part) => encodeURIComponent(part ?? "")) - .join("\n"); - const resolveRepository = (input: ForgejoRepositoryInput) => Cache.get(targets, targetKey(input)); + // Select credentials for every request; a pending selection can belong to another account. + const resolveRepository = (input: ForgejoRepositoryInput) => + resolveTarget({ + ...input, + remoteUrl: input.context?.remoteUrl, + providerBaseUrl: input.context?.provider.baseUrl, + requestedHost: input.context?.requestedHost, + }); const api = Effect.fn("ForgejoCli.api")(function* (input: ForgejoApiInput) { const hostOnly = input.path.replace(/^\/+/, "") === "user" && (!input.method || input.method === "GET"); - const repository = yield* input.freshCredentials || (input.method && input.method !== "GET") - ? resolveTarget( - { - cwd: input.cwd, - reference: input.reference, - repository: input.repository, - host: input.host, - remoteUrl: input.context?.remoteUrl, - providerBaseUrl: input.context?.provider.baseUrl, - requestedHost: input.context?.requestedHost, - }, - hostOnly, - ) - : Cache.get(targets, targetKey(input, hostOnly)); + const repository = yield* resolveTarget( + { + ...input, + remoteUrl: input.context?.remoteUrl, + providerBaseUrl: input.context?.provider.baseUrl, + requestedHost: input.context?.requestedHost, + }, + hostOnly, + ); const stdin = input.body === undefined ? undefined diff --git a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts index 55693a5ba82d..a0e2776be0d7 100644 --- a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts +++ b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts @@ -1,5 +1,4 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; -import * as Cache from "effect/Cache"; import * as Effect from "effect/Effect"; import * as Schema from "effect/Schema"; import * as Result from "effect/Result"; @@ -149,51 +148,20 @@ export const make = Effect.gen(function* () { } return { items, truncated: true }; }); - // Share pending reads only: credentials and repository permissions can change between requests. - const cacheKey = (...parts: ReadonlyArray) => - parts.map((part) => encodeURIComponent(String(part ?? ""))).join("\n"); - const pulls = yield* Cache.makeWith( - (key: string) => { - const [cwd = "", repository = "", host = "", number = ""] = key - .split("\n") - .map(decodeURIComponent); - const ref = { cwd, repository, host, number: Number(number) }; - return read({ ...ref, path: pullPath(ref) }, ForgejoPullRequest); - }, - { capacity: 64, timeToLive: () => 0 }, - ); + // These reads select the current account independently of older pending requests. const getPull = (input: ProviderRepositoryRef & { readonly number: number }) => - Cache.get(pulls, cacheKey(input.cwd, input.repository, input.host, input.number)); - const write = (input: ForgejoCli.ForgejoApiInput) => - request(input).pipe( - Effect.onExit(() => Cache.invalidateAll(pulls)), - Effect.asVoid, + read( + { cwd: input.cwd, repository: input.repository, host: input.host, path: pullPath(input) }, + ForgejoPullRequest, ); - const repos = yield* Cache.makeWith( - (key: string) => { - const [cwd = "", repository = "", host = ""] = key.split("\n").map(decodeURIComponent); - const ref = { cwd, repository, host }; - return read({ ...ref, path: repoPath(ref) }, ForgejoRepository); - }, - { capacity: 32, timeToLive: () => 0 }, - ); - const viewers = yield* Cache.makeWith( - (key: string) => { - const [cwd = "", repository, host] = key.split("\n").map(decodeURIComponent); - return read( - { cwd, ...(repository ? { repository } : {}), ...(host ? { host } : {}), path: "user" }, - ForgejoUser, - ).pipe(Effect.map((user) => user.login)); - }, - { capacity: 32, timeToLive: () => 0 }, - ); + const write = (input: ForgejoCli.ForgejoApiInput) => request(input).pipe(Effect.asVoid); const getRepo = (input: ProviderRepositoryRef) => - Cache.get(repos, cacheKey(input.cwd, input.repository, input.host)); + read({ ...input, path: repoPath(input) }, ForgejoRepository); const getViewer = (input: { readonly cwd: string; readonly repository?: string; readonly host?: string; - }) => Cache.get(viewers, cacheKey(input.cwd, input.repository, input.host)); + }) => read({ ...input, path: "user" }, ForgejoUser).pipe(Effect.map((user) => user.login)); const permissions = ( repo: typeof ForgejoRepository.Type, pr: typeof ForgejoPullRequest.Type, @@ -224,14 +192,8 @@ export const make = Effect.gen(function* () { const getPermissions = Effect.fn("ForgejoPullRequestProvider.getPermissions")(function* ( input: ProviderRepositoryRef & { readonly number: number }, ) { - // A write must not join a detail read started before the account or access changed. - const fresh = { ...input, freshCredentials: true }; const [repo, pr, viewer] = yield* Effect.all( - [ - read({ ...fresh, path: repoPath(input) }, ForgejoRepository), - read({ ...fresh, path: pullPath(input) }, ForgejoPullRequest), - read({ ...fresh, path: "user" }, ForgejoUser).pipe(Effect.map((user) => user.login)), - ], + [getRepo(input), getPull(input), getViewer(input)], { concurrency: 3 }, ); return permissions(repo, pr, viewer); @@ -515,16 +477,7 @@ export const make = Effect.gen(function* () { body: { body: input.body }, }), submitReview: Effect.fn("ForgejoPullRequestProvider.submitReview")(function* (input) { - const pr = yield* read( - { - cwd: input.cwd, - repository: input.repository, - host: input.host, - path: pullPath(input), - freshCredentials: true, - }, - ForgejoPullRequest, - ); + const pr = yield* getPull(input); yield* write({ ...input, path: `${pullPath(input)}/reviews`, diff --git a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts index 75629486bca5..41b18f531c05 100644 --- a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts +++ b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts @@ -62,8 +62,16 @@ it.effect("refreshes Forgejo viewer permissions immediately after credentials ch assert.strictEqual(bob.requestReviewers, false); assert.strictEqual(requests.length, 9); assert.deepStrictEqual(renewals, ["alice", "bob"]); + const freshDetail = yield* provider.getChangeRequest(input).pipe(Effect.forkChild); + yield* Effect.yieldNow; yield* Deferred.succeed(release, undefined); + assert.deepStrictEqual((yield* Fiber.join(freshDetail)).viewerPermissions?.verdicts, [ + "comment", + "approve", + "request-changes", + ]); assert.deepStrictEqual((yield* Fiber.join(detail)).viewerPermissions?.verdicts, ["comment"]); + assert.strictEqual(requests.length, 14); }).pipe( Effect.provideService( FileSystem.FileSystem, @@ -147,10 +155,7 @@ it.effect("reselects the current tea login for writes while an older read is pen yield* Deferred.await(started); account = "carol"; holdLogin = Effect.void; - assert.strictEqual( - (yield* cli.api({ ...input, freshCredentials: true })).stdout, - '{"login":"carol"}', - ); + assert.strictEqual((yield* cli.api(input)).stdout, '{"login":"carol"}'); const write = yield* cli .api({ ...input, method: "PATCH", body: { state: "closed" } }) .pipe(Effect.forkChild); @@ -203,7 +208,7 @@ it.effect("reselects the current tea login for writes while an older read is pen ); }); -it.effect("shares pending Forgejo pull reads and refreshes after writes and failures", () => +it.effect("keeps Forgejo pull reads fresh during pending reads, writes and failures", () => Effect.gen(function* () { const started = yield* Deferred.make(); const release = yield* Deferred.make(); @@ -274,11 +279,11 @@ it.effect("shares pending Forgejo pull reads and refreshes after writes and fail ).pipe(Effect.forkChild); yield* Deferred.await(started); yield* Effect.yieldNow; - assert.strictEqual(pullReads, 1); + assert.strictEqual(pullReads, 3); yield* provider.runAction({ ...input, action: "close" }); const updated = yield* checks(input); assert.strictEqual(updated.state, "closed"); - assert.strictEqual(pullReads, 2); + assert.strictEqual(pullReads, 4); assert.include(paths, "repos/acme/web/statuses/after?sort=recentupdate&limit=50&page=1"); yield* Deferred.succeed(release, undefined); const original = yield* Fiber.join(reads); @@ -286,10 +291,10 @@ it.effect("shares pending Forgejo pull reads and refreshes after writes and fail reject = true; const failed = yield* summary(input).pipe(Effect.result); assert.strictEqual(failed._tag, "Failure"); - assert.strictEqual(pullReads, 3); + assert.strictEqual(pullReads, 5); reject = false; assert.strictEqual((yield* summary(input)).state, "closed"); - assert.strictEqual(pullReads, 4); + assert.strictEqual(pullReads, 6); }), ); diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 0d2337d32e5a..2d1d44c7386a 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -552,6 +552,84 @@ it.effect( }), ); +it.effect("separates overlapping unpinned permission reads after a saved account switch", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + let activeToken = "restricted-credential"; + const requests: Array<{ readonly url: string; readonly authorization: string | undefined }> = []; + const credentials = Layer.succeed( + GitHubCredentials.GitHubCredentials, + GitHubCredentials.GitHubCredentials.of({ + get: (host) => + Effect.sync(() => ({ + host, + token: Redacted.make(activeToken), + source: "settings" as const, + fingerprint: `${host}:${activeToken.length}${activeToken.at(-1)}`, + })), + invalidate: () => Effect.void, + }), + ); + const http = Layer.succeed( + HttpClient.HttpClient, + HttpClient.make((request) => + Effect.gen(function* () { + const authorization = request.headers["authorization"]; + requests.push({ url: request.url, authorization }); + yield* Deferred.succeed(started, undefined); + yield* Deferred.await(release); + return HttpClientResponse.fromWeb( + request, + new Response( + encodeJson( + coreResponse({ + viewerCanUpdateBranch: authorization === "Bearer broad-credential", + }), + ), + ), + ); + }), + ), + ); + const cli = yield* GitHubPullRequestApi.make.pipe( + Effect.provide( + GitHubApi.layer.pipe( + Layer.provide(Layer.mergeAll(credentials, http)), + Layer.provide(GitHubQuota.layer), + Layer.provide(SourceControlRateLimit.layer), + Layer.merge(TestSourceControlHost.layer()), + Layer.provideMerge(NodeServices.layer), + ), + ), + ); + const input = { cwd: "/repo", host: "github.com", repository: "acme/web", number: 7 }; + // The single-environment permission path is interactive but does not pin an account. + const read = cli + .getPullRequestDetail(input) + .pipe(Effect.provideService(SourceControlRateLimit.Interactive, true)); + const older = yield* read.pipe(Effect.forkChild); + yield* Deferred.await(started); + activeToken = "broad-credential"; + const fresh = yield* Effect.all([read, read], { concurrency: "unbounded" }).pipe(Effect.forkChild); + // Let both readers reach the pending request before answering either account. + yield* Effect.yieldNow; + yield* Deferred.succeed(release, undefined); + + expect((yield* Fiber.join(older)).comparison?.viewerCanUpdate).toBe(false); + expect((yield* Fiber.join(fresh)).map((detail) => detail.comparison?.viewerCanUpdate)).toEqual([ + true, + true, + ]); + // Two accounts need two reads; the new account's panel readers still share one. + expect(requests.map((request) => request.authorization)).toEqual([ + "Bearer restricted-credential", + "Bearer broad-credential", + ]); + expect(requests.every((request) => request.url.endsWith("/graphql"))).toBe(true); + }), +); + layer("GitHubPullRequestApi.layer", (it) => { it.effect("holds background previews once an answer reports the reserve, until it resets", () => Effect.gen(function* () { diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index fa4c66a93390..23d047f09fa6 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -1753,17 +1753,24 @@ export const make = Effect.gen(function* () { // each starts from the same detail query, and for a fork's pull request the same workflow runs. // Reads in flight together under the same credential and quota share one request. Nothing is // kept once it lands, so a read that starts later asks GitHub again. - const inFlightKey = (...parts: ReadonlyArray) => + const inFlightKey = (cwd: string, host: string, ...parts: ReadonlyArray) => Effect.gen(function* () { - const pinned = yield* GitHubApi.PinnedGitHubCredential; - return [ + // Resolve locally even for unpinned clients, then keep the read on that credential. + const { token, fingerprint } = yield* api.credential(host); + const key = [ + cwd, + host, ...parts, yield* SourceControlRateLimit.Interactive, yield* SourceControlRateLimit.CredentialScope, - pinned?.credentialFingerprint ?? "", + fingerprint, ] .map((part) => encodeURIComponent(String(part))) .join("\n"); + return { + key, + credential: { host: host.trim().toLowerCase(), token, credentialFingerprint: fingerprint }, + }; }); const detailsInFlight = yield* Cache.makeWith( (key: string) => { @@ -2184,7 +2191,11 @@ export const make = Effect.gen(function* () { getPullRequestDetail: (input) => inFlightKey(input.cwd, input.host, input.repository, input.number).pipe( - Effect.flatMap((key) => Cache.get(detailsInFlight, key)), + Effect.flatMap(({ key, credential }) => + Cache.get(detailsInFlight, key).pipe( + Effect.provideService(GitHubApi.PinnedGitHubCredential, credential), + ), + ), ), getPullRequestPreview: (input) => { const { owner, name } = parseRepositorySelector(input.repository); @@ -2202,7 +2213,7 @@ export const make = Effect.gen(function* () { // Runs the checks revalidator just confirmed are this caller's alone to filter. if ((yield* KnownWorkflowRuns) !== null) return yield* listWorkflowRunsRequiringApproval(input); - const key = yield* inFlightKey( + const { key, credential } = yield* inFlightKey( input.cwd, input.host, input.repository, @@ -2211,7 +2222,9 @@ export const make = Effect.gen(function* () { input.headBranch, input.headRepositoryOwner, ); - return yield* Cache.get(workflowRunsInFlight, key); + return yield* Cache.get(workflowRunsInFlight, key).pipe( + Effect.provideService(GitHubApi.PinnedGitHubCredential, credential), + ); }), getPullRequestStack: (input) => { From 87fafdcebb663e6c11f5d344c2511600b7a5151c Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Mon, 12 Oct 2026 01:43:17 +0000 Subject: [PATCH 05/10] fix(source-control): use shared transport and fresh authenticated reads --- .../src/sourceControl/builtInDrivers.ts | 55 +---- .../AzureDevOpsPullRequestProvider.test.ts | 125 +++++++++- .../server/AzureDevOpsPullRequestProvider.ts | 87 ++----- .../server/GitCafePullRequestProvider.test.ts | 220 +++++++++++------- .../src/server/GitCafePullRequestProvider.ts | 89 ++----- .../src/server/GitHubPullRequestApi.test.ts | 7 +- .../src/server/GitLabPullRequestCli.test.ts | 204 ++++++++-------- .../src/server/GitLabPullRequestProvider.ts | 114 ++------- 8 files changed, 425 insertions(+), 476 deletions(-) diff --git a/apps/server/src/sourceControl/builtInDrivers.ts b/apps/server/src/sourceControl/builtInDrivers.ts index 74d1d7c80432..1b891ce9259a 100644 --- a/apps/server/src/sourceControl/builtInDrivers.ts +++ b/apps/server/src/sourceControl/builtInDrivers.ts @@ -25,10 +25,7 @@ import * as GitLabCli from "@t3tools/source-control-gitlab/server/GitLabCli"; import * as GitLabPullRequestCli from "@t3tools/source-control-gitlab/server/GitLabPullRequestCli"; import * as GitLabDriver from "@t3tools/source-control-gitlab/server/driver"; import type { SourceControlDriver } from "@t3tools/source-control-core/server/driver"; -import * as Undici from "@effect/platform-node/Undici"; -import * as Effect from "effect/Effect"; import * as Layer from "effect/Layer"; -import * as FetchHttpClient from "effect/http/FetchHttpClient"; import * as ServerSourceControlHost from "./ServerSourceControlHost.ts"; @@ -50,56 +47,6 @@ export const BUILT_IN_SOURCE_CONTROL_DRIVERS: ReadonlyArray< SourceControlDriver > = drivers; -/** - * The client the HTTP drivers read through: fetch, so redirects and headers behave as before, over - * one pool of its own. The server's fetch drops a connection after four idle seconds and opens one - * per concurrent request, so nearly every pull request opened after a pause paid a new TLS - * handshake per read. This pool keeps connections for minutes, or for as long as the host does, - * and carries a panel's parallel reads over one HTTP/2 connection where the host offers it. - * Fresh, since the server builds the same `FetchHttpClient.layer` for itself and a memoized one - * would ignore the `Fetch` given here. - */ -const httpDriverClient = Layer.fresh(FetchHttpClient.layer).pipe( - Layer.provide( - Layer.effect(FetchHttpClient.Fetch)( - Effect.acquireRelease( - Effect.sync(() => { - const options = { - allowH2: true, - keepAliveTimeout: 300_000, - keepAliveMaxTimeout: 300_000, - }; - // Match Node's proxy opt-in, including flags supplied through NODE_OPTIONS. - const proxyFlags = [ - ...(process.env.NODE_OPTIONS?.match(/--(?:no-)?use-env-proxy\b/g) ?? []), - ...process.execArgv, - ]; - const proxyFlag = proxyFlags.findLast( - (flag) => flag === "--use-env-proxy" || flag === "--no-use-env-proxy", - ); - const useProxy = - proxyFlag === undefined - ? process.env.NODE_USE_ENV_PROXY === "1" - : proxyFlag === "--use-env-proxy"; - return useProxy ? new Undici.EnvHttpProxyAgent(options) : new Undici.Agent(options); - }), - (dispatcher) => Effect.promise(() => dispatcher.destroy()), - ).pipe( - // Node's own fetch types and this undici's describe the same init and response, but name - // a different `Dispatcher`, so they meet through `unknown`. - Effect.map( - (dispatcher): typeof globalThis.fetch => - (input, init) => - Undici.fetch(input as Undici.RequestInfo, { - ...(init as unknown as Undici.RequestInit), - dispatcher, - }) as unknown as Promise, - ), - ), - ), - ), -); - /** The services the built-in drivers' packages own, plus the host port they all run against. */ export const layer = Layer.mergeAll( // `GitHubApi.layerWithDependencies` carries the quota reserve and rate-limit pause every GitHub @@ -110,4 +57,4 @@ export const layer = Layer.mergeAll( ForgejoCli.layer, GitCafeApi.layer.pipe(Layer.provideMerge(GitCafeCredentials.layer)), GitLabPullRequestCli.layer.pipe(Layer.provideMerge(GitLabCli.layer)), -).pipe(Layer.provide(httpDriverClient), Layer.provideMerge(ServerSourceControlHost.layer)); +).pipe(Layer.provideMerge(ServerSourceControlHost.layer)); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts index 1b96984cf76b..1edc1c02e5bd 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -1,7 +1,10 @@ import { describe, expect, it } from "@effect/vitest"; +import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; +import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; +import * as AzureDevOpsCli from "./AzureDevOpsCli.ts"; import * as AzureDevOpsPullRequestCli from "./AzureDevOpsPullRequestCli.ts"; import { LOCATION_CACHE_CAPACITY, @@ -368,15 +371,119 @@ describe("what one diff slice spends", () => { it.effect("carries on from where the last slice stopped", () => Effect.gen(function* () { const paths = ["a.ts", "b.ts", "c.ts", "d.ts", "e.ts", "f.ts"]; - const read = yield* readSlice({ - paths, - lines: 100, - width: 900, - cursor: `${ITERATION.id}:2`, - }); - - expect(patchedPaths(read.slice.patch)).toEqual(["c.ts", "d.ts"]); - expect(read.slice.nextCursor).toBe(`${ITERATION.id}:4`); + let signedOut = false; + let pushed = false; + let holdIterations = false; + let iterationReads = 0; + const started = yield* Deferred.make(); + const previewStarted = yield* Deferred.make(); + const release = yield* Deferred.make(); + const contentReads: string[] = []; + const provider = yield* make.pipe( + Effect.provide( + Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ + getPullRequest: () => + Effect.gen(function* () { + if (signedOut) { + return yield* new AzureDevOpsCli.AzureDevOpsCliAuthenticationError({ + operation: "execute", + command: "az", + cwd: "/w", + argumentCount: 1, + cause: "signed out", + }); + } + if (holdIterations) { + yield* Deferred.succeed(previewStarted, undefined); + yield* Deferred.await(release); + } + return PULL_REQUEST; + }), + listIterations: () => + Effect.gen(function* () { + iterationReads += 1; + if (signedOut) { + return yield* new AzureDevOpsCli.AzureDevOpsCliAuthenticationError({ + operation: "execute", + command: "az", + cwd: "/w", + argumentCount: 1, + cause: "signed out", + }); + } + if (holdIterations) { + yield* Deferred.succeed(started, undefined); + yield* Deferred.await(release); + } + return pushed + ? [ITERATION, { ...ITERATION, id: ITERATION.id + 1, headCommit: "new-head" }] + : [ITERATION]; + }), + listIterationChanges: () => + Effect.succeed({ changes: paths.map((path) => change(path)), truncated: false }), + readItemContent: (item) => + Effect.sync(() => { + contentReads.push(item.commit); + return { + contents: side(item.commit === ITERATION.headCommit ? "new" : "old", 100, 900), + isBinary: false, + }; + }), + }), + ), + ); + const input = { cwd: "/w", repository: "acme/web", host: "dev.azure.com", number: 7 }; + const first = yield* provider.getDiff(input); + expect(first.nextCursor).toBe(`${ITERATION.id}:2`); + const continuation = { ...input, cursor: first.nextCursor ?? undefined }; + const slice = yield* provider.getDiff(continuation); + + expect(patchedPaths(slice.patch)).toEqual(["c.ts", "d.ts"]); + expect(slice.nextCursor).toBe(`${ITERATION.id}:4`); + + pushed = true; + const warmedReads = contentReads.length; + const again = yield* provider.getDiff(continuation); + expect(again).toEqual(slice); + expect(contentReads).toHaveLength(warmedReads); + + const fileInput = { + ...input, + oldPath: "c.ts", + newPath: "c.ts", + changeType: "change" as const, + }; + const revisionsInput = { ...input, paths: ["c.ts"] }; + const contents = yield* provider.getDiffFileContents!(fileInput); + yield* provider.getFileRevisions!(revisionsInput); + const warmedContentsReads = contentReads.length; + + holdIterations = true; + const pending = yield* Effect.forkChild(provider.getDiffFileContents!(fileInput)); + yield* Deferred.await(started); + const pendingPreview = yield* Effect.forkChild(provider.getChangeRequestPreview!(input)); + yield* Deferred.await(previewStarted); + signedOut = true; + const signedOutReads = yield* Effect.all( + [ + provider.getDiff(continuation).pipe(Effect.asVoid, Effect.result), + provider.getDiffFileContents!(fileInput).pipe(Effect.asVoid, Effect.result), + provider.getFileRevisions!(revisionsInput).pipe(Effect.asVoid, Effect.result), + provider.getChangeRequestSummary!(input).pipe(Effect.asVoid, Effect.result), + provider.getChangeRequestPreview!(input).pipe(Effect.asVoid, Effect.result), + provider.getChangeRequest(input).pipe(Effect.asVoid, Effect.result), + ].map((read) => Effect.forkChild(read, { startImmediately: true })), + ); + yield* Deferred.succeed(release, undefined); + expect(yield* Fiber.join(pending)).toEqual(contents); + expect((yield* Fiber.join(pendingPreview)).title).toBe(PULL_REQUEST.title); + for (const result of yield* Effect.all(signedOutReads.map(Fiber.join))) { + expect(result._tag).toBe("Failure"); + const error = yield* Effect.flip(Effect.fromResult(result)); + expect(error.reason).toBe("unauthenticated"); + } + expect(iterationReads).toBe(9); + expect(contentReads).toHaveLength(warmedContentsReads); }), ); it.effect("keeps the pull request being read, not the one looked up first", () => diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index 40d622374324..254548ac322d 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -257,31 +257,15 @@ export const make = Effect.gen(function* () { return held; }; - /** - * `az repos pr show`, shared by the reads of one pull request that overlap: a review opening asks - * for its detail, conversation and diff at once, and each of them can start here. Nothing is kept - * once it answers, so a read that starts afterwards asks Azure again. - */ - const pullRequestReads = yield* Cache.makeWith( - (key: string) => { - const [cwd = "", number = ""] = key.split("\n").map(decodeURIComponent); - return cli - .getPullRequest({ cwd, number: Number(number) }) - .pipe( - Effect.tap((pullRequest) => - Effect.sync(() => rememberLocation(cwd, pullRequest, { evict: true })), - ), - ); - }, - { capacity: LOCATION_CACHE_CAPACITY, timeToLive: () => Duration.zero }, - ); - + // Each read uses the current CLI sign-in, including reads overlapping an account switch. const readPullRequest = (input: { readonly cwd: string; readonly number: number }) => - Cache.get(pullRequestReads, cacheKey(input.cwd, input.number)); - - /** Run after a write, so a read asked for once it lands cannot join one that started before. */ - const forgetRead = (input: { readonly cwd: string; readonly number: number }) => - Cache.invalidate(pullRequestReads, cacheKey(input.cwd, input.number)); + cli + .getPullRequest(input) + .pipe( + Effect.tap((pullRequest) => + Effect.sync(() => rememberLocation(input.cwd, pullRequest, { evict: true })), + ), + ); const locationOf = (input: { readonly cwd: string; readonly number: number }) => { const held = knownLocation(input); @@ -318,35 +302,6 @@ export const make = Effect.gen(function* () { encodeURIComponent(iteration.mergeBaseCommit), ].join("\n"); - /** - * The pushes a pull request has had, shared by overlapping reads the way a pull request read is. - * Never kept: the newest push is what the list is read for, and a push adds one. - */ - const iterationReads = yield* Cache.makeWith( - (key: string) => cli.listIterations(fromIterationKey(key)), - { capacity: LOCATION_CACHE_CAPACITY, timeToLive: () => Duration.zero }, - ); - - /** - * One push by its id, which is all a diff cursor names. A push never changes once it lands, so - * the latest one is kept from every listing and a slice carrying on from it lists nothing. - */ - const iterationsById = yield* Cache.makeWith( - (key: string) => { - const scope = fromIterationKey(key); - return Cache.get(iterationReads, iterationKey(scope)).pipe( - Effect.map((iterations) => - iterations.find((candidate) => candidate.id === scope.iterationId), - ), - ); - }, - { - capacity: LOCATION_CACHE_CAPACITY, - timeToLive: (exit) => - Exit.isSuccess(exit) && exit.value !== undefined ? Duration.infinity : Duration.zero, - }, - ); - /** * What one push changed, against the merge base. Never changes once the push lands, so the * detail's file count, every slice of the diff and the viewed marks share one complete read. @@ -361,16 +316,6 @@ export const make = Effect.gen(function* () { }, ); - const listIterations = (scope: IterationScope) => - Cache.get(iterationReads, iterationKey(scope)).pipe( - Effect.tap((iterations) => { - const latest = iterations.at(-1); - return latest === undefined - ? Effect.void - : Cache.set(iterationsById, iterationKey(scope, latest.id), latest); - }), - ); - /** * Everything a diff read needs before it can ask for a file: where the repository lives, and * which pushes the pull request has had. A client names neither, and the iterations are read @@ -380,7 +325,7 @@ export const make = Effect.gen(function* () { Effect.gen(function* () { const location = yield* locationOf(input); if (location === null) return null; - const iterations = yield* listIterations({ + const iterations = yield* cli.listIterations({ cwd: input.cwd, location, number: input.number, @@ -534,7 +479,7 @@ export const make = Effect.gen(function* () { // riding on them, so a failure leaves it unknown rather than losing the whole detail. const countFiles = (location: AzureDevOpsRepositoryLocation) => { const scope = { cwd: input.cwd, location, number: input.number }; - return listIterations(scope).pipe( + return cli.listIterations(scope).pipe( Effect.flatMap((iterations) => listLatestChanges({ ...scope, iterations })), Effect.map((listed) => listed.changes.length), Effect.orElseSucceed(() => 0), @@ -611,12 +556,14 @@ export const make = Effect.gen(function* () { if (location === null) return EMPTY_DIFF_SLICE; const scope = { cwd: input.cwd, location, number: input.number }; const cursor = parseAzureDevOpsDiffCursor(input.cursor); + // Even a fully cached continuation must check the current sign-in can read this review. + const iterations = yield* cli.listIterations(scope); // Reading on stays with the push the first slice was taken against. A push landing // mid-read would otherwise renumber the files and hand the reader one twice, or none. const iteration = cursor === null - ? (yield* listIterations(scope)).at(-1) - : yield* Cache.get(iterationsById, iterationKey(scope, cursor.iterationId)); + ? iterations.at(-1) + : iterations.find((candidate) => candidate.id === cursor.iterationId); if (iteration === undefined) return EMPTY_DIFF_SLICE; const listed = yield* Cache.get(iterationChanges, changesKey(scope, iteration)); const changes = listed.changes; @@ -772,7 +719,7 @@ export const make = Effect.gen(function* () { action: input.action, ...(input.mergeMethod === undefined ? {} : { mergeMethod: input.mergeMethod }), }) - .pipe(Effect.ensuring(forgetRead(input)), Effect.mapError(fail("runAction"))), + .pipe(Effect.mapError(fail("runAction"))), updateChangeRequest: (input) => cli @@ -782,7 +729,7 @@ export const make = Effect.gen(function* () { title: input.title, body: input.body, }) - .pipe(Effect.ensuring(forgetRead(input)), Effect.mapError(fail("updateChangeRequest"))), + .pipe(Effect.mapError(fail("updateChangeRequest"))), // Never called: `capabilities.reviewers.listCandidates` is false, and the service refuses the // list without it. @@ -806,7 +753,7 @@ export const make = Effect.gen(function* () { reviewers: input.reviewers.map((reviewer) => reviewer.id), requested: input.requested, }) - .pipe(Effect.ensuring(forgetRead(input)), Effect.mapError(fail("setReviewerRequest"))), + .pipe(Effect.mapError(fail("setReviewerRequest"))), // Never called: `capabilities.comment` is false, and the service refuses a comment without it. comment: () => unsupported("comment"), diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts index 0e737f509550..bc46022e0420 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts @@ -340,94 +340,152 @@ describe("GitCafePullRequestProvider", () => { }).pipe(Effect.provide(server.layer)); }); - it.effect("refreshes permissions while a pull read with old credentials is pending", () => - Effect.gen(function* () { - const started = yield* Deferred.make(); - const release = yield* Deferred.make(); - let token = "old-token"; - const authorizations: Array = []; - const client = HttpClient.make((request) => { - const authorization = request.headers.authorization; - authorizations.push(authorization); - const old = authorization === "Bearer old-token"; - return ( - old - ? Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))) - : Effect.void - ).pipe( - Effect.as( - HttpClientResponse.fromWeb( - request, - new Response( - encodeJson({ - ...pull, - title: old ? "Old credentials" : "New credentials", - capabilities: { ...pull.capabilities, edit: !old }, + it.effect( + "refreshes preview, summary, detail and permissions during a pending authorized read", + () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + let token: string | null = "old-token"; + const authorizations: Array = []; + const client = HttpClient.make((request) => { + const authorization = request.headers.authorization; + authorizations.push(authorization); + const old = authorization === "Bearer old-token"; + const revoked = authorization === "Bearer revoked-token"; + return ( + old + ? Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))) + : Effect.void + ).pipe( + Effect.as( + HttpClientResponse.fromWeb( + request, + new Response( + encodeJson( + revoked + ? { title: "Unauthorized" } + : new URL(request.url).pathname.endsWith("/status") + ? { + merge: { + conflicts: "unknown", + fastForward: true, + strategies: ["merge"], + }, + checks: { pending: 0, failing: 0, total: 0 }, + } + : { + ...pull, + headOid: null, + title: old ? "Old credentials" : "New credentials", + capabilities: { ...pull.capabilities, edit: !old }, + }, + ), + { status: revoked ? 401 : 200 }, + ), + ), + ), + ); + }); + const provider = yield* GitCafePullRequestProvider.make.pipe( + Effect.provide( + GitCafeApi.layer.pipe( + Layer.provide(Layer.succeed(HttpClient.HttpClient, client)), + Layer.provide( + Layer.succeed(GitCafeCredentials.GitCafeCredentials, { + get: (host) => + Effect.suspend(() => + token === null + ? Effect.fail(new GitCafeCredentials.GitCafeNotSignedInError({ host })) + : Effect.succeed({ + host, + token: Redacted.make(token), + source: "settings" as const, + }), + ), + invalidate: () => Effect.void, + cliEnv: () => Effect.succeed({}), }), ), ), ), ); - }); - const provider = yield* GitCafePullRequestProvider.make.pipe( - Effect.provide( - GitCafeApi.layer.pipe( - Layer.provide(Layer.succeed(HttpClient.HttpClient, client)), - Layer.provide( - Layer.succeed(GitCafeCredentials.GitCafeCredentials, { - get: (host) => - Effect.sync(() => ({ - host, - token: Redacted.make(token), - source: "settings" as const, - })), - invalidate: () => Effect.void, - cliEnv: () => Effect.succeed({}), - }), - ), - ), - ), - ); - const pending = yield* provider.getChangeRequestPreview!(target).pipe(Effect.forkChild); - yield* Deferred.await(started); - token = "new-token"; - assert.strictEqual((yield* provider.getViewerPermissions(target)).editChangeRequest, true); - yield* Deferred.succeed(release, undefined); - yield* Fiber.join(pending); - // The late old response must not replace the fresh pull used for permissions. - assert.strictEqual( - (yield* provider.getChangeRequestSummary!(target)).title, - "New credentials", - ); - assert.deepStrictEqual(authorizations, ["Bearer old-token", "Bearer new-token"]); - }), + const pending = yield* provider.getChangeRequestPreview!(target).pipe(Effect.forkChild); + yield* Deferred.await(started); + token = "new-token"; + const refreshed = [ + yield* provider.getChangeRequestPreview!(target), + yield* provider.getChangeRequestSummary!(target), + yield* provider.getChangeRequest(target), + ]; + assert.deepStrictEqual( + refreshed.map((item) => item.title), + ["New credentials", "New credentials", "New credentials"], + ); + assert.strictEqual((yield* provider.getViewerPermissions(target)).editChangeRequest, true); + for (let phase = 0; phase < 2; phase++) { + token = phase === 0 ? null : "revoked-token"; + const rejected = [ + yield* provider.getChangeRequestPreview!(target).pipe(Effect.flip), + yield* provider.getChangeRequestSummary!(target).pipe(Effect.flip), + yield* provider.getChangeRequest(target).pipe(Effect.flip), + yield* provider.getViewerPermissions(target).pipe(Effect.flip), + ]; + assert.deepStrictEqual( + rejected.map((error) => [error.operation, error.reason]), + [ + ["getChangeRequestPreview", "unauthenticated"], + ["getChangeRequestSummary", "unauthenticated"], + ["getChangeRequest", "unauthenticated"], + ["getViewerPermissions", "unauthenticated"], + ], + ); + if (phase === 0) { + yield* Deferred.succeed(release, undefined); + assert.strictEqual((yield* Fiber.join(pending)).title, "Old credentials"); + } + } + assert.deepStrictEqual(authorizations, [ + "Bearer old-token", + ...Array.from({ length: 5 }, () => "Bearer new-token"), + ...Array.from({ length: 4 }, () => "Bearer revoked-token"), + ]); + }), ); - it.effect.each([true, false])("invalidates a pull after a write succeeds: %s", (succeeds) => { - let reads = 0; - const server = fakeGitCafe({ - "GET /repos/owner/repo/pulls/7": () => ({ - ...pull, - title: reads++ === 0 ? "Before" : "After", - }), - ...(succeeds ? { "POST /repos/owner/repo/pulls/7/close": {} } : {}), - }); - return Effect.gen(function* () { - const provider = yield* GitCafePullRequestProvider.make; - assert.strictEqual((yield* provider.getChangeRequestPreview!(target)).title, "Before"); - const result = yield* provider.runAction({ ...target, action: "close" }).pipe(Effect.result); - assert.strictEqual(result._tag, succeeds ? "Success" : "Failure"); - assert.strictEqual((yield* provider.getChangeRequestPreview!(target)).title, "After"); - assert.deepStrictEqual( - server.sent.map((request) => `${request.method} ${request.path}`), - [ - "GET /repos/owner/repo/pulls/7", - "POST /repos/owner/repo/pulls/7/close", - "GET /repos/owner/repo/pulls/7", - ], - ); - }).pipe(Effect.provide(server.layer)); - }); + it.effect.each([true, false])( + "reads a fresh pull before and after a write succeeds: %s", + (succeeds) => { + let reads = 0; + const server = fakeGitCafe({ + "GET /repos/owner/repo/pulls/7": () => ({ + ...pull, + version: ++reads, + }), + ...(succeeds ? { "POST /repos/owner/repo/pulls/7/close": {} } : {}), + }); + return Effect.gen(function* () { + const provider = yield* GitCafePullRequestProvider.make; + yield* provider.getChangeRequestPreview!(target); + const result = yield* provider + .runAction({ ...target, action: "close" }) + .pipe(Effect.result); + assert.strictEqual(result._tag, succeeds ? "Success" : "Failure"); + assert.deepStrictEqual(server.sent.at(-1)?.body, { expectedVersion: 2 }); + yield* provider.getChangeRequestPreview!(target); + assert.strictEqual(reads, 3); + assert.deepStrictEqual( + server.sent.map((request) => `${request.method} ${request.path}`), + [ + "GET /repos/owner/repo/pulls/7", + "GET /repos/owner/repo/pulls/7", + "POST /repos/owner/repo/pulls/7/close", + "GET /repos/owner/repo/pulls/7", + ], + ); + }).pipe(Effect.provide(server.layer)); + }, + ); it.effect("reads the diff from the pull request's changed files", () => { const file = { diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts index d6e27ca90b9e..21f6fdf4da15 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts @@ -58,11 +58,6 @@ const DIFF_FILES_BATCH = 64; const LINE_STATS_MAX_FILES = DIFF_FILES_BATCH * 4; /** Structural `/changes` pages, 500 files each, read to count a pull request's files. */ const MAX_CHANGE_PAGES = 20; -/** - * How long one pull read answers every caller, so a panel's detail, activity, diff and stack - * reads, or a write's permission check and its own read, ask GitCafe once. - */ -const PULL_TTL = Duration.seconds(1); /** A viewer's actor id is stable, so involvement filters skip `/filter-options` for this long. */ const ACTOR_TTL = Duration.minutes(10); @@ -248,50 +243,11 @@ export const make = Effect.gen(function* () { const pullPath = (operation: string, input: PullRef) => repositoryPath(operation, input).pipe(Effect.map((base) => `${base}/pulls/${input.number}`)); - let readGeneration = 0; - const pullKey = (input: PullRef) => - [input.host, input.repository, input.number, readGeneration] - .map((value) => encodeURIComponent(String(value ?? ""))) - .join("\n"); - // A failed read is not held, so the next caller asks GitCafe again. - const pulls = yield* Cache.makeWith( - (key: string) => { - const [host = "", repository = "", number = ""] = key.split("\n").map(decodeURIComponent); - return read( - "readPull", - { host }, - { path: `/repos/${repository}/pulls/${number}` }, - Json.GitCafePullDetail, - ); - }, - { capacity: 64, timeToLive: (exit) => (Exit.isSuccess(exit) ? PULL_TTL : Duration.zero) }, - ); - const readPull = (operation: string, input: PullRef) => - repositoryPath(operation, input).pipe( - Effect.flatMap(() => Cache.get(pulls, pullKey(input))), - // One read can answer several callers, so each failure is reported as its caller's. - Effect.mapError( - (error) => - new PullRequestProviderError({ - provider: error.provider, - operation, - reason: error.reason, - detail: error.detail, - ...(error.cause === undefined ? {} : { cause: error.cause }), - }), - ), + pullPath(operation, input).pipe( + Effect.flatMap((path) => read(operation, input, { path }, Json.GitCafePullDetail)), ); - /** Drops a pull's shared read once a write on it settles, whatever the outcome. */ - // A late pending read may repopulate an invalidated entry, so abandon its generation too. - const forgetPull = (input: PullRef) => - Effect.suspend(() => { - const key = pullKey(input); - readGeneration++; - return Cache.invalidate(pulls, key); - }); - /** Every page of a cursor list, up to `MAX_PAGES`; `next` stays set when more remained. */ const readPages = , unknown, never, never>>( operation: string, @@ -328,13 +284,11 @@ export const make = Effect.gen(function* () { /** * The stack a pull request belongs to, with each unmerged layer's head. GitCafe's stack lists no - * heads, and they are what a land or restack is fenced on, so each open layer's pull is read; - * `fresh` skips the shared pull reads for that fence. + * heads, and they are what a land or restack is fenced on, so each open layer's pull is read. */ const readStack = Effect.fn("GitCafePullRequestProvider.readStack")(function* ( operation: string, input: PullRef, - fresh = false, ) { const path = yield* pullPath(operation, input); const { stack } = yield* read( @@ -349,8 +303,7 @@ export const make = Effect.gen(function* () { (member) => { if (member.state === "merged") return Effect.succeed(member); const layer = { ...input, number: member.pullRequestNumber }; - return (fresh ? forgetPull(layer) : Effect.void).pipe( - Effect.andThen(readPull(operation, layer)), + return readPull(operation, layer).pipe( Effect.map((pull) => pull.headOid == null ? member : { ...member, headOid: pull.headOid }, ), @@ -398,7 +351,7 @@ export const make = Effect.gen(function* () { const operation = land ? "landStackThrough" : "restackStack"; if (!land && input.action !== "update-branch") return yield* failure(operation, `GitCafe cannot ${input.action} a stack.`); - const stack = yield* readStack(operation, input, true); + const stack = yield* readStack(operation, input); if ( stack === null || stack.number !== input.stackNumber || @@ -426,12 +379,6 @@ export const make = Effect.gen(function* () { : { expectedRevision: stack.revision, requestId: yield* randomUuidV4 }, }, StackOutcome, - ).pipe( - Effect.ensuring( - Effect.forEach(stack.members, (member) => - forgetPull({ ...input, number: member.pullRequestNumber }), - ), - ), ); if (FAILED_STACK_STATES.has(outcome.state)) return yield* failure(operation, `GitCafe's stack operation ${outcome.state}.`); @@ -573,7 +520,7 @@ export const make = Effect.gen(function* () { }, }, MergeOutcome, - ).pipe(Effect.ensuring(forgetPull(input))); + ); // An accepted merge is GitCafe's to finish; the pull request's state reports when it has. if (outcome.state === "failed") return yield* failure("merge", outcome.reason ?? "GitCafe could not merge the pull request."); @@ -812,10 +759,7 @@ export const make = Effect.gen(function* () { }, ), getViewerPermissions: (input) => - forgetPull(input).pipe( - Effect.andThen(readPull("getViewerPermissions", input)), - Effect.map(Json.toViewerPermissions), - ), + readPull("getViewerPermissions", input).pipe(Effect.map(Json.toViewerPermissions)), getDiff: Effect.fn("GitCafePullRequestProvider.getDiff")(function* (input) { if (input.commit !== undefined) return yield* failure("getDiff", "GitCafe commit diffs are not supported yet."); @@ -865,7 +809,7 @@ export const make = Effect.gen(function* () { method: "POST", path: `${path}/${input.action}`, body: { expectedVersion: pull.version }, - }).pipe(Effect.ensuring(forgetPull(input))); + }); return; } default: @@ -889,7 +833,7 @@ export const make = Effect.gen(function* () { }, }, Versioned, - ).pipe(Effect.ensuring(forgetPull(input))); + ); }, ), comment: (input) => @@ -903,7 +847,6 @@ export const make = Effect.gen(function* () { ), ), Effect.asVoid, - Effect.ensuring(forgetPull(input)), ), updateComment: Effect.fn("GitCafePullRequestProvider.updateComment")(function* (input) { if (!isId(input.commentId)) @@ -915,7 +858,7 @@ export const make = Effect.gen(function* () { input, { method: "PATCH", path, body: { body: input.body, expectedVersion: detail.version } }, Written, - ).pipe(Effect.ensuring(forgetPull(input))); + ); }), submitReview: Effect.fn("GitCafePullRequestProvider.submitReview")(function* (input) { if (input.comments.length > 0) @@ -939,7 +882,7 @@ export const make = Effect.gen(function* () { }, }, Written, - ).pipe(Effect.ensuring(forgetPull(input))); + ); }), listReviewerCandidates: () => Effect.fail( @@ -960,7 +903,7 @@ export const make = Effect.gen(function* () { body: { body: input.body }, }, Written, - ).pipe(Effect.ensuring(forgetPull(input))); + ); }), setReaction: Effect.fn("GitCafePullRequestProvider.setReaction")(function* (input) { const path = `${yield* pullPath("setReaction", input)}/reactions/`; @@ -982,15 +925,13 @@ export const make = Effect.gen(function* () { method: "POST", path, body: { subject, emoji: { kind: "unicode", value: Json.REACTION_EMOJI[input.content] } }, - }).pipe(Effect.ensuring(forgetPull(input))); + }); return; } if (mine == null) return; if (!isId(mine)) return yield* failure("setReaction", "GitCafe returned an unreadable reaction id."); - yield* send("setReaction", input, { method: "DELETE", path: `${path}${mine}` }).pipe( - Effect.ensuring(forgetPull(input)), - ); + yield* send("setReaction", input, { method: "DELETE", path: `${path}${mine}` }); }), setThreadResolution: Effect.fn("GitCafePullRequestProvider.setThreadResolution")( function* (input) { @@ -1011,7 +952,7 @@ export const make = Effect.gen(function* () { body: { expectedVersion: root.version }, }, Written, - ).pipe(Effect.ensuring(forgetPull(input))); + ); }, ), }; diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index 2d1d44c7386a..c59d9aae15bd 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -557,7 +557,8 @@ it.effect("separates overlapping unpinned permission reads after a saved account const started = yield* Deferred.make(); const release = yield* Deferred.make(); let activeToken = "restricted-credential"; - const requests: Array<{ readonly url: string; readonly authorization: string | undefined }> = []; + const requests: Array<{ readonly url: string; readonly authorization: string | undefined }> = + []; const credentials = Layer.succeed( GitHubCredentials.GitHubCredentials, GitHubCredentials.GitHubCredentials.of({ @@ -611,7 +612,9 @@ it.effect("separates overlapping unpinned permission reads after a saved account const older = yield* read.pipe(Effect.forkChild); yield* Deferred.await(started); activeToken = "broad-credential"; - const fresh = yield* Effect.all([read, read], { concurrency: "unbounded" }).pipe(Effect.forkChild); + const fresh = yield* Effect.all([read, read], { concurrency: "unbounded" }).pipe( + Effect.forkChild, + ); // Let both readers reach the pending request before answering either account. yield* Effect.yieldNow; yield* Deferred.succeed(release, undefined); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts index 5974c8e38552..65a17bcd2a0f 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts @@ -3,7 +3,6 @@ import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; -import * as TestClock from "effect/testing/TestClock"; import { ChildProcessSpawner } from "effect/process"; import * as GitLabCli from "./GitLabCli.ts"; @@ -1197,7 +1196,7 @@ layer("GitLabPullRequestCli.layer", (it) => { }), ); - it.effect("reads who has access to the project and who is already on the merge request", () => + it.effect("reuses a freshly read merge request when reading reviewer candidates", () => Effect.gen(function* () { mockedExecute .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) @@ -1205,11 +1204,17 @@ layer("GitLabPullRequestCli.layer", (it) => { Effect.succeed(output(JSON.stringify([author, reviewer, { id: 9, username: "hubot" }]))), ); const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + const mergeRequest = yield* cli.getMergeRequestDetail({ + cwd: "/w", + repository: "acme/web", + number: 7, + }); const list = yield* cli.listReviewerCandidates({ cwd: "/w", repository: "acme/web", number: 7, + mergeRequest, }); expect(argsOfCall(1)[1]).toBe("projects/acme%2Fweb/users?per_page=100"); @@ -1219,6 +1224,7 @@ layer("GitLabPullRequestCli.layer", (it) => { ["9", false], ]); assert.isFalse(list.truncated); + expect(mockedExecute).toHaveBeenCalledTimes(2); }), ); @@ -1228,6 +1234,11 @@ layer("GitLabPullRequestCli.layer", (it) => { .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) .mockReturnValueOnce(Effect.succeed(output("{}"))); const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + const mergeRequest = yield* cli.getMergeRequestDetail({ + cwd: "/w", + repository: "acme/web", + number: 7, + }); yield* cli.setReviewerRequest({ cwd: "/w", @@ -1235,12 +1246,14 @@ layer("GitLabPullRequestCli.layer", (it) => { number: 7, reviewers: [{ id: "9" }], requested: true, + mergeRequest, }); // GitLab replaces the whole set, so the reviewer already on the merge request has to be // sent back with the new one or the request would take them off it. expect(argsOfCall(1)).toContain("PUT"); expect(JSON.parse(callAt(1).stdin ?? "")).toEqual({ reviewer_ids: [5, 9] }); + expect(mockedExecute).toHaveBeenCalledTimes(2); }), ); @@ -1538,62 +1551,66 @@ layer("GitLabPullRequestCli.layer", (it) => { }), ); - it.effect( - "shares concurrent merge request reads for one second without rereading project settings", - () => - Effect.gen(function* () { - const started = yield* Deferred.make(); - const release = yield* Deferred.make(); - let detailReads = 0; - let settingsReads = 0; - mockedExecute.mockImplementation((request) => - Effect.gen(function* () { - if (request.args[1]?.includes("?license=false")) { - settingsReads++; - return output('{"merge_method":"merge"}'); - } - detailReads++; + it.effect("reauthorizes every read after logout while an older authorized read is pending", () => + Effect.gen(function* () { + const started = yield* Deferred.make(); + const release = yield* Deferred.make(); + let loggedOut = false; + let holdRead = false; + let settingsReads = 0; + mockedExecute.mockImplementation((request) => { + if (request.args[1]?.includes("?license=false")) { + settingsReads++; + return Effect.succeed(output('{"merge_method":"merge"}')); + } + if (loggedOut) { + return Effect.fail( + new GitLabCli.GitLabCliAuthenticationError({ + command: "glab", + operation: "execute", + cwd: request.cwd, + cause: new Error("logged out"), + }), + ); + } + return Effect.gen(function* () { + if (holdRead) { yield* Deferred.succeed(started, undefined); yield* Deferred.await(release); - return output(mergeRequestJson({ title: `Read ${detailReads}` })); - }), - ); - const provider = yield* GitLabPullRequestProvider.make; - assert.isDefined(provider.getChangeRequestChecks); - assert.isDefined(provider.getChangeRequestPreview); - assert.isDefined(provider.getChangeRequestSummary); - const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; - const reads = yield* Effect.all( - [ - provider.getChangeRequest(input), - provider.getChangeRequestChecks(input), - provider.getChangeRequestPreview(input), - provider.getChangeRequestSummary(input), - ], - { concurrency: 4 }, - ).pipe(Effect.forkChild); - yield* Deferred.await(started); - yield* Deferred.succeed(release, undefined); - const [detail, checks, preview, summary] = yield* Fiber.join(reads); - - expect([detail.title, preview.title, summary.title]).toEqual([ - "Read 1", - "Read 1", - "Read 1", - ]); - expect(checks.state).toBe("open"); - expect(detailReads).toBe(1); - expect(settingsReads).toBe(1); - yield* TestClock.adjust("900 millis"); - expect((yield* provider.getChangeRequestSummary(input)).title).toBe("Read 1"); - yield* TestClock.adjust("200 millis"); - expect((yield* provider.getChangeRequest(input)).title).toBe("Read 2"); - expect(detailReads).toBe(2); - expect(settingsReads).toBe(1); - }), + } + return output(mergeRequestJson({ title: "Private merge request" })); + }); + }); + const provider = yield* GitLabPullRequestProvider.make; + assert.isDefined(provider.getChangeRequestChecks); + assert.isDefined(provider.getChangeRequestPreview); + assert.isDefined(provider.getChangeRequestSummary); + const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; + expect((yield* provider.getChangeRequest(input)).title).toBe("Private merge request"); + holdRead = true; + const older = yield* provider.getChangeRequestSummary(input).pipe(Effect.forkChild); + yield* Deferred.await(started); + loggedOut = true; + + for (const read of [ + provider.getChangeRequest(input).pipe(Effect.asVoid), + provider.getChangeRequestChecks(input).pipe(Effect.asVoid), + provider.getChangeRequestPreview(input).pipe(Effect.asVoid), + provider.getChangeRequestSummary(input).pipe(Effect.asVoid), + provider.getViewerPermissions(input).pipe(Effect.asVoid), + ]) { + expect((yield* Effect.flip(read)).reason).toBe("unauthenticated"); + } + expect(settingsReads).toBe(1); + yield* Deferred.succeed(release, undefined); + expect((yield* Fiber.join(older)).title).toBe("Private merge request"); + expect((yield* Effect.flip(provider.getChangeRequestSummary(input))).reason).toBe( + "unauthenticated", + ); + }), ); - it.effect("rereads permissions before each write and shares that fresh answer with checks", () => + it.effect("rereads permissions and checks with the current account", () => Effect.gen(function* () { mockedExecute .mockReturnValueOnce( @@ -1604,7 +1621,8 @@ layer("GitLabPullRequestCli.layer", (it) => { ) .mockReturnValueOnce( Effect.succeed(output(mergeRequestJson({ user: { can_merge: true } }))), - ); + ) + .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))); const provider = yield* GitLabPullRequestProvider.make; assert.isDefined(provider.getChangeRequestChecks); const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; @@ -1613,47 +1631,45 @@ layer("GitLabPullRequestCli.layer", (it) => { expect((yield* provider.getViewerPermissions(input)).actions).not.toContain("merge"); expect((yield* provider.getViewerPermissions(input)).actions).toContain("merge"); expect((yield* provider.getChangeRequestChecks(input)).state).toBe("open"); - expect(mockedExecute).toHaveBeenCalledTimes(3); + expect(mockedExecute).toHaveBeenCalledTimes(4); }), ); - it.effect( - "reuses the permission read for reviewer candidates and preserves existing reviewers in a write", - () => - Effect.gen(function* () { - mockedExecute - .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) - .mockReturnValueOnce( - Effect.succeed( - output(JSON.stringify([author, reviewer, { id: 9, username: "hubot" }])), - ), - ) - .mockReturnValueOnce(Effect.succeed(output("{}"))) - .mockReturnValueOnce( - Effect.succeed(output(mergeRequestJson({ title: "After review request" }))), - ); - const provider = yield* GitLabPullRequestProvider.make; - assert.isDefined(provider.getChangeRequestSummary); - const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; + it.effect("reads fresh reviewer candidates and preserves the current reviewers in a write", () => + Effect.gen(function* () { + mockedExecute + .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))) + .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) + .mockReturnValueOnce( + Effect.succeed(output(JSON.stringify([author, reviewer, { id: 9, username: "hubot" }]))), + ) + .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) + .mockReturnValueOnce(Effect.succeed(output("{}"))) + .mockReturnValueOnce( + Effect.succeed(output(mergeRequestJson({ title: "After review request" }))), + ); + const provider = yield* GitLabPullRequestProvider.make; + assert.isDefined(provider.getChangeRequestSummary); + const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; - yield* provider.getViewerPermissions(input); - const candidates = yield* provider.listReviewerCandidates(input); - expect( - candidates.candidates.map(({ login, isRequested }) => ({ login, isRequested })), - ).toEqual([ - { login: "octocat", isRequested: true }, - { login: "hubot", isRequested: false }, - ]); - yield* provider.setReviewerRequest({ - ...input, - reviewers: [{ id: "9", kind: "user" }], - requested: true, - }); + yield* provider.getViewerPermissions(input); + const candidates = yield* provider.listReviewerCandidates(input); + expect( + candidates.candidates.map(({ login, isRequested }) => ({ login, isRequested })), + ).toEqual([ + { login: "octocat", isRequested: true }, + { login: "hubot", isRequested: false }, + ]); + yield* provider.setReviewerRequest({ + ...input, + reviewers: [{ id: "9", kind: "user" }], + requested: true, + }); - expect(JSON.parse(callAt(2).stdin ?? "")).toEqual({ reviewer_ids: [5, 9] }); - expect((yield* provider.getChangeRequestSummary(input)).title).toBe("After review request"); - expect(mockedExecute).toHaveBeenCalledTimes(4); - }), + expect(JSON.parse(callAt(4).stdin ?? "")).toEqual({ reviewer_ids: [5, 9] }); + expect((yield* provider.getChangeRequestSummary(input)).title).toBe("After review request"); + expect(mockedExecute).toHaveBeenCalledTimes(6); + }), ); it.effect.each([false, true])( @@ -1704,7 +1720,7 @@ layer("GitLabPullRequestCli.layer", (it) => { yield* Deferred.succeed(release, undefined); expect((yield* Fiber.join(older)).title).toBe("Before mutation"); expect((yield* provider.getChangeRequestSummary(input)).title).toBe("After mutation"); - expect(detailReads).toBe(2); + expect(detailReads).toBe(3); }), ); @@ -1713,7 +1729,9 @@ layer("GitLabPullRequestCli.layer", (it) => { mockedExecute .mockReturnValueOnce(Effect.succeed(output("invalid json"))) .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))) + .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))) .mockReturnValueOnce(Effect.succeed(output("invalid json"))) + .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))) .mockReturnValueOnce(Effect.succeed(output('{"merge_method":"merge"}'))); const provider = yield* GitLabPullRequestProvider.make; assert.isDefined(provider.getChangeRequestSummary); @@ -1725,7 +1743,7 @@ layer("GitLabPullRequestCli.layer", (it) => { expect((yield* provider.getChangeRequest(input)).mergeCapabilities).toMatchObject({ merge: true, }); - expect(mockedExecute).toHaveBeenCalledTimes(4); + expect(mockedExecute).toHaveBeenCalledTimes(6); }), ); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index d377cf3ff0ec..2945223f9b38 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -2,7 +2,6 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; import * as Cache from "effect/Cache"; import * as Effect from "effect/Effect"; import * as Exit from "effect/Exit"; -import * as Option from "effect/Option"; import type { PullRequestCapabilities, PullRequestReaction, @@ -102,43 +101,12 @@ export function gitLabProviderFailure( return { reason: "failed" }; } -const mergeRequestKey = (input: { - readonly cwd: string; - readonly repository: string; - readonly number: number; -}) => - [input.cwd, input.repository, input.number] - .map((value) => encodeURIComponent(String(value ?? ""))) - .join("\n"); - export const make = Effect.gen(function* () { const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; - let readGeneration = 0; - const readKey = (input: Parameters[0]) => - `${mergeRequestKey(input)}\n${readGeneration}`; - - // The detail, its checks and the viewer's permissions are all `merge_requests/:iid`, and a page - // opening asks for them at once. One read answers the ones that land within a second of it; a - // failure is not held, and every write below forgets the merge request it wrote to. - const mergeRequests = yield* Cache.makeWith( - (key: string) => { - const [cwd = "", repository = "", number = ""] = key.split("\n").map(decodeURIComponent); - return cli.getMergeRequestDetail({ cwd, repository, number: Number(number) }); - }, - { capacity: 64, timeToLive: (exit) => (Exit.isSuccess(exit) ? "1 second" : 0) }, - ); - const mergeRequest = (input: Parameters[0]) => - Effect.suspend(() => Cache.get(mergeRequests, readKey(input))); - // An invalidated pending lookup can finish later and repopulate its old cache entry. - const forgetMergeRequest = (input: Parameters[0]) => - Effect.suspend(() => { - const key = readKey(input); - readGeneration++; - return Cache.invalidate(mergeRequests, key); - }); // How a project lets its merge requests land is a setting, which moves far more slowly than - // the detail every refresh was reading it alongside. + // the detail every refresh was reading it alongside. It is returned only with a fresh, + // authorized merge request read; account-specific details are never held between calls. const mergeSettings = yield* Cache.makeWith( (key: string) => { const [cwd = "", repository = ""] = key.split("\n").map(decodeURIComponent); @@ -147,13 +115,6 @@ export const make = Effect.gen(function* () { { capacity: 32, timeToLive: (exit) => (Exit.isSuccess(exit) ? "5 minutes" : 0) }, ); - // A write can change anything the merge request reports, so the held read is dropped once it - // ends, failed or not: a write that failed part-way may still have changed it. - const writing = - (input: Parameters[0]) => - (write: Effect.Effect) => - write.pipe(Effect.ensuring(forgetMergeRequest(input))); - const fail = (operation: string) => (error: GitLabPullRequestCli.GitLabPullRequestCliError) => new PullRequestProviderError({ provider: SourceControlProviderKind.make("gitlab"), @@ -190,21 +151,21 @@ export const make = Effect.gen(function* () { ), getChangeRequestChecks: (input) => - mergeRequest(input).pipe( + cli.getMergeRequestDetail(input).pipe( Effect.map(({ state, checks }) => ({ state, checks })), Effect.mapError(fail("getChangeRequestChecks")), ), // Hover cards and linked rows do not use the project's merge settings. getChangeRequestPreview: (input) => - mergeRequest(input).pipe(Effect.mapError(fail("getChangeRequestPreview"))), + cli.getMergeRequestDetail(input).pipe(Effect.mapError(fail("getChangeRequestPreview"))), getChangeRequestSummary: (input) => - mergeRequest(input).pipe(Effect.mapError(fail("getChangeRequestSummary"))), + cli.getMergeRequestDetail(input).pipe(Effect.mapError(fail("getChangeRequestSummary"))), getChangeRequest: (input) => Effect.all( [ - mergeRequest(input), + cli.getMergeRequestDetail(input), Cache.get( mergeSettings, [input.cwd, input.repository] @@ -278,15 +239,11 @@ export const make = Effect.gen(function* () { ), // The same read the detail takes it from: `user.can_merge` lives on the merge request, so - // there is no cheaper thing to ask GitLab. The server asks this before every write, so it - // starts a fresh read; the write after it can then reuse what it read. - getViewerPermissions: (input) => { - return forgetMergeRequest(input).pipe( - Effect.andThen(Effect.suspend(() => mergeRequest(input))), - Effect.mapError(fail("getViewerPermissions")), - Effect.map(gitLabViewerPermissions), - ); - }, + // there is no cheaper thing to ask GitLab. Every call uses the account signed in now. + getViewerPermissions: (input) => + cli + .getMergeRequestDetail(input) + .pipe(Effect.mapError(fail("getViewerPermissions")), Effect.map(gitLabViewerPermissions)), getDiff: (input) => cli.getMergeRequestDiff(input).pipe(Effect.mapError(fail("getDiff"))), @@ -304,38 +261,11 @@ export const make = Effect.gen(function* () { // Users only: GitLab requests a review of a person, and the groups that can stand in for one // appear in approval rules rather than in a merge request's reviewers. - // The merge request the permission check just read, where it is held, rather than a second - // read of it; where it is not, the CLI reads it alongside the project's users. listReviewerCandidates: (input) => - Effect.suspend(() => Cache.getSuccess(mergeRequests, readKey(input))).pipe( - Effect.flatMap((held) => - cli.listReviewerCandidates({ - cwd: input.cwd, - repository: input.repository, - number: input.number, - mergeRequest: Option.getOrUndefined(held), - }), - ), - Effect.mapError(fail("listReviewerCandidates")), - ), + cli.listReviewerCandidates(input).pipe(Effect.mapError(fail("listReviewerCandidates"))), - // The reviewer set is rewritten from the held read, which the permission check before this - // write has just taken and which no write of this provider outlives. setReviewerRequest: (input) => - mergeRequest(input).pipe( - Effect.flatMap((held) => - cli.setReviewerRequest({ - cwd: input.cwd, - repository: input.repository, - number: input.number, - reviewers: input.reviewers, - requested: input.requested, - mergeRequest: held, - }), - ), - writing(input), - Effect.mapError(fail("setReviewerRequest")), - ), + cli.setReviewerRequest(input).pipe(Effect.mapError(fail("setReviewerRequest"))), runAction: (input) => cli @@ -346,7 +276,7 @@ export const make = Effect.gen(function* () { action: input.action, ...(input.mergeMethod === undefined ? {} : { mergeMethod: input.mergeMethod }), }) - .pipe(writing(input), Effect.mapError(fail("runAction"))), + .pipe(Effect.mapError(fail("runAction"))), updateChangeRequest: (input) => cli @@ -357,10 +287,9 @@ export const make = Effect.gen(function* () { ...(input.title === undefined ? {} : { title: input.title }), ...(input.body === undefined ? {} : { description: input.body }), }) - .pipe(writing(input), Effect.mapError(fail("updateChangeRequest"))), + .pipe(Effect.mapError(fail("updateChangeRequest"))), - comment: (input) => - cli.commentOnMergeRequest(input).pipe(writing(input), Effect.mapError(fail("comment"))), + comment: (input) => cli.commentOnMergeRequest(input).pipe(Effect.mapError(fail("comment"))), // The kind is not read: every comment this provider hands out, positioned or not, carries a // plain REST note id, and one endpoint rewrites both. @@ -373,10 +302,9 @@ export const make = Effect.gen(function* () { noteId: input.commentId, body: input.body, }) - .pipe(writing(input), Effect.mapError(fail("updateComment"))), + .pipe(Effect.mapError(fail("updateComment"))), - submitReview: (input) => - cli.submitReview(input).pipe(writing(input), Effect.mapError(fail("submitReview"))), + submitReview: (input) => cli.submitReview(input).pipe(Effect.mapError(fail("submitReview"))), replyToThread: (input) => cli @@ -387,7 +315,7 @@ export const make = Effect.gen(function* () { discussionId: input.threadId, body: input.body, }) - .pipe(writing(input), Effect.mapError(fail("replyToThread"))), + .pipe(Effect.mapError(fail("replyToThread"))), setReaction: (input) => cli @@ -399,7 +327,7 @@ export const make = Effect.gen(function* () { content: input.content, reacted: input.reacted, }) - .pipe(writing(input), Effect.mapError(fail("setReaction"))), + .pipe(Effect.mapError(fail("setReaction"))), setThreadResolution: (input) => cli @@ -410,7 +338,7 @@ export const make = Effect.gen(function* () { discussionId: input.threadId, resolved: input.resolved, }) - .pipe(writing(input), Effect.mapError(fail("setThreadResolution"))), + .pipe(Effect.mapError(fail("setThreadResolution"))), }; return provider; From 06467418a44f447d307b6f565e4c00eb7986b9a0 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Mon, 12 Oct 2026 02:06:02 +0000 Subject: [PATCH 06/10] test(source-control): synchronize github credential overlap --- .../src/server/GitHubPullRequestApi.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index c59d9aae15bd..b8f2dfc66e97 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -555,6 +555,7 @@ it.effect( it.effect("separates overlapping unpinned permission reads after a saved account switch", () => Effect.gen(function* () { const started = yield* Deferred.make(); + const broadStarted = yield* Deferred.make(); const release = yield* Deferred.make(); let activeToken = "restricted-credential"; const requests: Array<{ readonly url: string; readonly authorization: string | undefined }> = @@ -578,6 +579,9 @@ it.effect("separates overlapping unpinned permission reads after a saved account Effect.gen(function* () { const authorization = request.headers["authorization"]; requests.push({ url: request.url, authorization }); + if (authorization === "Bearer broad-credential") { + yield* Deferred.succeed(broadStarted, undefined); + } yield* Deferred.succeed(started, undefined); yield* Deferred.await(release); return HttpClientResponse.fromWeb( @@ -615,8 +619,8 @@ it.effect("separates overlapping unpinned permission reads after a saved account const fresh = yield* Effect.all([read, read], { concurrency: "unbounded" }).pipe( Effect.forkChild, ); - // Let both readers reach the pending request before answering either account. - yield* Effect.yieldNow; + // Keep both accounts' HTTP requests pending before answering either one. + yield* Deferred.await(broadStarted); yield* Deferred.succeed(release, undefined); expect((yield* Fiber.join(older)).comparison?.viewerCanUpdate).toBe(false); From 61f831d8773038e3576978eeb78c3268676b5c59 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Mon, 12 Oct 2026 02:16:24 +0000 Subject: [PATCH 07/10] fix(source-control): retry incomplete gitcafe line counts --- .../server/GitCafePullRequestProvider.test.ts | 47 +++++++++++++++---- .../src/server/GitCafePullRequestProvider.ts | 7 ++- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts index bc46022e0420..70be0f351275 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts @@ -842,8 +842,8 @@ describe("GitCafePullRequestProvider", () => { }).pipe(Effect.provide(server.layer)); }); - it.effect.each(["changes", "diff-files"] as const)( - "retries line stats after a failed %s read and retains the complete retry", + it.effect.each(["changes", "diff-files", "partial-changes", "partial-diff-files"] as const)( + "retries line stats after a failed or incomplete %s read and retains the complete retry", (failedRead) => { const snapshot = { version: 4, headOid, comparisonBaseOid: baseOid }; let recovered = false; @@ -857,25 +857,56 @@ describe("GitCafePullRequestProvider", () => { "GET /repos/owner/repo/pulls/7/changes": () => !recovered && failedRead === "changes" ? {} - : { ...snapshot, items: [{ path: "file.ts" }], next: null }, + : { + ...snapshot, + items: [ + failedRead === "partial-changes" + ? { path: "file.ts", additions: 5, ...(recovered ? { deletions: 2 } : {}) } + : { path: "file.ts" }, + ...(failedRead === "partial-changes" + ? [{ path: "image.png", binary: true }] + : []), + ], + next: null, + }, "POST /repos/owner/repo/pulls/7/diff-files": () => !recovered && failedRead === "diff-files" ? {} - : { ...snapshot, items: [{ path: "file.ts", additions: 5, deletions: 2 }] }, + : { + ...snapshot, + items: [ + { + path: "file.ts", + additions: 5, + ...(!recovered && failedRead === "partial-diff-files" ? {} : { deletions: 2 }), + }, + ], + }, }); return Effect.gen(function* () { const provider = yield* GitCafePullRequestProvider.make; + const firstReadStart = server.sent.length; assert.deepInclude(yield* provider.getChangeRequest(target), { - additions: 0, - changedFiles: failedRead === "changes" ? 0 : 1, + additions: failedRead.startsWith("partial-") ? 5 : 0, + deletions: 0, + changedFiles: failedRead === "changes" ? 0 : failedRead === "partial-changes" ? 2 : 1, }); + assert.strictEqual( + server.sent.length - firstReadStart, + failedRead === "changes" || failedRead === "partial-changes" ? 4 : 5, + ); recovered = true; for (let read = 0; read < 2; read++) { + const readStart = server.sent.length; assert.deepInclude(yield* provider.getChangeRequest(target), { additions: 5, deletions: 2, - changedFiles: 1, + changedFiles: failedRead === "partial-changes" ? 2 : 1, }); + assert.strictEqual( + server.sent.length - readStart, + read === 1 ? 3 : failedRead === "partial-changes" ? 4 : 5, + ); } assert.strictEqual( server.sent.filter((request) => request.path.endsWith("/changes")).length, @@ -883,7 +914,7 @@ describe("GitCafePullRequestProvider", () => { ); assert.strictEqual( server.sent.filter((request) => request.path.endsWith("/diff-files")).length, - failedRead === "changes" ? 1 : 2, + failedRead === "partial-changes" ? 0 : failedRead === "changes" ? 1 : 2, ); }).pipe(Effect.provide(server.layer)); }, diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts index 21f6fdf4da15..9648ab3ac616 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts @@ -440,7 +440,12 @@ export const make = Effect.gen(function* () { : []; return { revision, - complete: Option.isSome(details) && after === null, + complete: + Option.isSome(details) && + after === null && + counted.every( + (file) => file.binary || (file.additions !== undefined && file.deletions !== undefined), + ), changedFiles: files.length, additions: counted.reduce((total, file) => total + (file.additions ?? 0), 0), deletions: counted.reduce((total, file) => total + (file.deletions ?? 0), 0), From 4884e291e4d3c71e4a226cdb7644488dd1055945 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Mon, 12 Oct 2026 02:35:50 +0000 Subject: [PATCH 08/10] fix(source-control): retain gitcafe totals only for complete file coverage --- .../server/GitCafePullRequestProvider.test.ts | 45 ++++++++++++++----- .../src/server/GitCafePullRequestProvider.ts | 2 +- 2 files changed, 34 insertions(+), 13 deletions(-) diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts index 70be0f351275..c375cdfcfd05 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts @@ -842,7 +842,14 @@ describe("GitCafePullRequestProvider", () => { }).pipe(Effect.provide(server.layer)); }); - it.effect.each(["changes", "diff-files", "partial-changes", "partial-diff-files"] as const)( + it.effect.each([ + "changes", + "diff-files", + "partial-changes", + "partial-diff-files", + "omitted-diff-files", + "oversized-file", + ] as const)( "retries line stats after a failed or incomplete %s read and retains the complete retry", (failedRead) => { const snapshot = { version: 4, headOid, comparisonBaseOid: baseOid }; @@ -866,6 +873,9 @@ describe("GitCafePullRequestProvider", () => { ...(failedRead === "partial-changes" ? [{ path: "image.png", binary: true }] : []), + ...(failedRead === "oversized-file" + ? [{ path: "large.ts", ...(recovered ? { binary: true } : { tooLarge: true }) }] + : []), ], next: null, }, @@ -874,22 +884,32 @@ describe("GitCafePullRequestProvider", () => { ? {} : { ...snapshot, - items: [ - { - path: "file.ts", - additions: 5, - ...(!recovered && failedRead === "partial-diff-files" ? {} : { deletions: 2 }), - }, - ], + items: + !recovered && failedRead === "omitted-diff-files" + ? [] + : [ + { + path: "file.ts", + additions: 5, + ...(!recovered && failedRead === "partial-diff-files" + ? {} + : { deletions: 2 }), + }, + ], }, }); return Effect.gen(function* () { const provider = yield* GitCafePullRequestProvider.make; const firstReadStart = server.sent.length; assert.deepInclude(yield* provider.getChangeRequest(target), { - additions: failedRead.startsWith("partial-") ? 5 : 0, - deletions: 0, - changedFiles: failedRead === "changes" ? 0 : failedRead === "partial-changes" ? 2 : 1, + additions: failedRead.startsWith("partial-") || failedRead === "oversized-file" ? 5 : 0, + deletions: failedRead === "oversized-file" ? 2 : 0, + changedFiles: + failedRead === "changes" + ? 0 + : failedRead === "partial-changes" || failedRead === "oversized-file" + ? 2 + : 1, }); assert.strictEqual( server.sent.length - firstReadStart, @@ -901,7 +921,8 @@ describe("GitCafePullRequestProvider", () => { assert.deepInclude(yield* provider.getChangeRequest(target), { additions: 5, deletions: 2, - changedFiles: failedRead === "partial-changes" ? 2 : 1, + changedFiles: + failedRead === "partial-changes" || failedRead === "oversized-file" ? 2 : 1, }); assert.strictEqual( server.sent.length - readStart, diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts index 9648ab3ac616..0598b4d96be2 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts @@ -443,7 +443,7 @@ export const make = Effect.gen(function* () { complete: Option.isSome(details) && after === null && - counted.every( + withDetails(files, counted).every( (file) => file.binary || (file.additions !== undefined && file.deletions !== undefined), ), changedFiles: files.length, From 8621df128ff53112575205b783a73623ef3e6e22 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Mon, 12 Oct 2026 03:08:03 +0000 Subject: [PATCH 09/10] fix(source-control): invalidate github reads after stack pushes --- .../src/server/GitHubPullRequestApi.test.ts | 152 ++++++++++++++++++ .../src/server/GitHubPullRequestApi.ts | 1 + 2 files changed, 153 insertions(+) diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts index b8f2dfc66e97..4403c997f9ea 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.test.ts @@ -5,7 +5,9 @@ import * as Effect from "effect/Effect"; import * as Clock from "effect/Clock"; import * as Deferred from "effect/Deferred"; import * as Fiber from "effect/Fiber"; +import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; +import * as ChildProcessSpawner from "effect/process/ChildProcessSpawner"; import * as Schema from "effect/Schema"; import * as TestClock from "effect/testing/TestClock"; import * as Redacted from "effect/Redacted"; @@ -637,6 +639,156 @@ it.effect("separates overlapping unpinned permission reads after a saved account }), ); +it.effect.each([false, true])( + "starts fresh reads after a stack rebase (partial failure: %s)", + (partialFailure) => + Effect.gen(function* () { + const detailStarted = yield* Deferred.make(); + const runsStarted = yield* Deferred.make(); + const freshRunsStarted = yield* Deferred.make(); + const release = yield* Deferred.make(); + let pushed = false; + let rebases = 0; + let pushes = 0; + const input = { cwd: "/w", repository: "acme/web", host: "github.com", number: 7 }; + mockedExecute.mockImplementation((call) => + Effect.gen(function* () { + if (call.kind === "rest" && call.path.includes("/stacks?")) { + return output( + encodeJson([ + { + number: 50, + url: "https://api.github.com/repos/acme/web/stacks/50", + base: { ref: "main" }, + pull_requests: [ + { number: 7, head: { ref: "feature", sha: "abc123" }, state: "open" }, + { number: 8, head: { ref: "top", sha: "def456" }, state: "open" }, + ], + }, + ]), + ); + } + if (call.operation === "runGitHubStackAction") { + return output( + encodeJson({ + data: { + repository: { + pr7: { + headRepository: { viewerPermission: "WRITE" }, + maintainerCanModify: false, + }, + pr8: { + headRepository: { viewerPermission: "WRITE" }, + maintainerCanModify: false, + }, + }, + }, + }), + ); + } + if (call.kind === "graphql" && call.query.includes("headRefName: $head")) { + if (pushed) yield* Deferred.await(freshRunsStarted); + return output(encodeJson(heads(pushed ? [] : [7]))); + } + const beforePush = !pushed; + if (beforePush) { + yield* Deferred.succeed(call.kind === "rest" ? runsStarted : detailStarted, undefined); + yield* Deferred.await(release); + } + if (call.kind === "rest" && !beforePush) { + yield* Deferred.succeed(freshRunsStarted, undefined); + } + return output( + encodeJson( + call.kind === "rest" + ? workflowRuns(beforePush ? [10] : []) + : coreResponse({ headRefOid: beforePush ? "abc123" : "new-sha" }), + ), + ); + }), + ); + const cli = yield* GitHubPullRequestApi.make.pipe( + Effect.provide( + Layer.mergeAll( + mockApi.pipe(Layer.provide(GitHubQuota.layer), Layer.provide(NodeCrypto.layer)), + FileSystem.layerNoop({ + makeTempDirectoryScoped: () => Effect.succeed("/scratch"), + }), + TestSourceControlHost.layer({ + process: { + run: (request) => + Effect.sync(() => { + expect(request.command).toBe("git"); + expect(request.operation).toBe("cascadeRebaseStack"); + if (request.args[0] === "push") { + pushed = true; + pushes++; + } + const conflict = + request.args[0] === "rebase" && + request.args[1] !== "--abort" && + ++rebases === 2 && + partialFailure; + return TestSourceControlHost.processOutput( + ["rev-parse", "merge-base"].includes(request.args[0]!) ? "new-sha\n" : "", + { exitCode: ChildProcessSpawner.ExitCode(conflict ? 1 : 0) }, + ); + }), + }, + }), + ), + ), + ); + const workflowInput = { + ...input, + headSha: "abc123", + headBranch: "feat/page", + headRepositoryOwner: "octocat", + isCrossRepository: true as const, + }; + const olderDetail = yield* cli.getPullRequestDetail(input).pipe(Effect.forkChild); + const olderRuns = yield* cli + .listWorkflowRunsRequiringApproval(workflowInput) + .pipe(Effect.forkChild); + yield* Deferred.await(detailStarted); + yield* Deferred.await(runsStarted); + const action = yield* cli + .runPullRequestAction({ + ...input, + number: 8, + stackNumber: 50, + action: "update-branch", + expectedStackHeads: [ + { number: 7, headSha: "abc123" }, + { number: 8, headSha: "def456" }, + ], + }) + .pipe(Effect.result); + expect(action._tag).toBe(partialFailure ? "Failure" : "Success"); + if (partialFailure) { + expect(action).toMatchObject({ + failure: { _tag: "GitHubStackRebaseFailedError", completed: 1 }, + }); + } + expect(pushes).toBe(partialFailure ? 1 : 2); + expect((yield* cli.getPullRequestDetail(input)).headSha).toBe("new-sha"); + expect( + yield* cli.listWorkflowRunsRequiringApproval(workflowInput).pipe(Effect.result), + ).toMatchObject({ + _tag: "Failure", + failure: { _tag: "GitHubWorkflowApprovalRefusedError", reason: "head-not-unique" }, + }); + // The stack action still needs only its membership and permission reads. + expect( + mockedExecute.mock.calls.filter(([call]) => call.operation === "runGitHubStackAction"), + ).toHaveLength(2); + expect(mockedExecute.mock.calls).toHaveLength(8); + yield* Deferred.succeed(release, undefined); + expect((yield* Fiber.join(olderDetail)).headSha).toBe("abc123"); + expect((yield* Fiber.join(olderRuns)).map((run) => run.id)).toEqual([10]); + }), +); + layer("GitHubPullRequestApi.layer", (it) => { it.effect("holds background previews once an answer reports the reserve, until it resets", () => Effect.gen(function* () { diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 23d047f09fa6..12a2e50f45eb 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -2600,6 +2600,7 @@ export const make = Effect.gen(function* () { runPullRequestAction: (input) => { if (input.stackNumber !== undefined) return runGitHubStackAction({ ...input, stackNumber: input.stackNumber }).pipe( + Effect.ensuring(forgetReads), Effect.provideService(GitHubApi.GitHubApi, api), Effect.provideService(SourceControlHost.SourceControlHost, sourceControlHost), Effect.provideService(FileSystem.FileSystem, fileSystem), From 05241eee1b9c53e0843a7174422a803368dccb85 Mon Sep 17 00:00:00 2001 From: maria-rcks <254055478+maria-rcks@users.noreply.github.com> Date: Tue, 13 Oct 2026 02:56:03 +0000 Subject: [PATCH 10/10] refactor(source-control): recreate pr read savings with less code Share any identical in-flight GitHub read in one wrapper instead of per-method caches, drop the Forgejo changes that saved no requests, remove GitLab's unused held-read path, and key caches on NUL-joined strings instead of URI-encoding every part. Request counts per action are unchanged. --- .../server/AzureDevOpsPullRequestCli.test.ts | 8 +- .../AzureDevOpsPullRequestProvider.test.ts | 125 +------ .../server/AzureDevOpsPullRequestProvider.ts | 137 +++---- .../src/server/ForgejoCli.ts | 66 +--- .../src/server/ForgejoPullRequestProvider.ts | 10 +- .../ForgejoSourceControlProvider.test.ts | 349 ++---------------- .../server/GitCafePullRequestProvider.test.ts | 150 -------- .../src/server/GitCafePullRequestProvider.ts | 36 +- .../src/server/GitHubPullRequestApi.ts | 160 +++----- .../src/server/GitLabPullRequestCli.test.ts | 254 +------------ .../src/server/GitLabPullRequestCli.ts | 19 +- .../src/server/GitLabPullRequestProvider.ts | 34 +- 12 files changed, 232 insertions(+), 1116 deletions(-) diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts index aeb63a4ad5cc..c8ac87257422 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestCli.test.ts @@ -1008,7 +1008,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { json({ changeEntries: [ { changeType: "add", item: { path: "/huge.bin", objectId: "8f80" } }, - { changeType: "add", item: { path: "/docs/%scope\nDEMO.md", objectId: "0ca4" } }, + { changeType: "add", item: { path: "/DEMO.md", objectId: "0ca4" } }, ], }), ), @@ -1029,7 +1029,7 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { const provider = yield* AzureDevOpsPullRequestProvider.make; const slice = yield* provider.getDiff({ - cwd: "/w\n%repo", + cwd: "/w", repository: "web", host: "dev.azure.com", number: 42, @@ -1039,10 +1039,6 @@ layer("AzureDevOpsPullRequestCli.layer", (it) => { expect(slice.patch).toContain("diff --git a/huge.bin b/huge.bin"); // And the file behind it still renders, which is the point of giving up on one file. expect(slice.patch).toContain("+hello"); - for (const [call] of mockedExecute.mock.calls) { - expect(call.cwd).toBe("/w\n%repo"); - } - expect(argsOfCall(4)).toContain("path=/docs/%scope\nDEMO.md"); }), ); diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts index 1edc1c02e5bd..1b96984cf76b 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.test.ts @@ -1,10 +1,7 @@ import { describe, expect, it } from "@effect/vitest"; -import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; -import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; -import * as AzureDevOpsCli from "./AzureDevOpsCli.ts"; import * as AzureDevOpsPullRequestCli from "./AzureDevOpsPullRequestCli.ts"; import { LOCATION_CACHE_CAPACITY, @@ -371,119 +368,15 @@ describe("what one diff slice spends", () => { it.effect("carries on from where the last slice stopped", () => Effect.gen(function* () { const paths = ["a.ts", "b.ts", "c.ts", "d.ts", "e.ts", "f.ts"]; - let signedOut = false; - let pushed = false; - let holdIterations = false; - let iterationReads = 0; - const started = yield* Deferred.make(); - const previewStarted = yield* Deferred.make(); - const release = yield* Deferred.make(); - const contentReads: string[] = []; - const provider = yield* make.pipe( - Effect.provide( - Layer.mock(AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli)({ - getPullRequest: () => - Effect.gen(function* () { - if (signedOut) { - return yield* new AzureDevOpsCli.AzureDevOpsCliAuthenticationError({ - operation: "execute", - command: "az", - cwd: "/w", - argumentCount: 1, - cause: "signed out", - }); - } - if (holdIterations) { - yield* Deferred.succeed(previewStarted, undefined); - yield* Deferred.await(release); - } - return PULL_REQUEST; - }), - listIterations: () => - Effect.gen(function* () { - iterationReads += 1; - if (signedOut) { - return yield* new AzureDevOpsCli.AzureDevOpsCliAuthenticationError({ - operation: "execute", - command: "az", - cwd: "/w", - argumentCount: 1, - cause: "signed out", - }); - } - if (holdIterations) { - yield* Deferred.succeed(started, undefined); - yield* Deferred.await(release); - } - return pushed - ? [ITERATION, { ...ITERATION, id: ITERATION.id + 1, headCommit: "new-head" }] - : [ITERATION]; - }), - listIterationChanges: () => - Effect.succeed({ changes: paths.map((path) => change(path)), truncated: false }), - readItemContent: (item) => - Effect.sync(() => { - contentReads.push(item.commit); - return { - contents: side(item.commit === ITERATION.headCommit ? "new" : "old", 100, 900), - isBinary: false, - }; - }), - }), - ), - ); - const input = { cwd: "/w", repository: "acme/web", host: "dev.azure.com", number: 7 }; - const first = yield* provider.getDiff(input); - expect(first.nextCursor).toBe(`${ITERATION.id}:2`); - const continuation = { ...input, cursor: first.nextCursor ?? undefined }; - const slice = yield* provider.getDiff(continuation); - - expect(patchedPaths(slice.patch)).toEqual(["c.ts", "d.ts"]); - expect(slice.nextCursor).toBe(`${ITERATION.id}:4`); - - pushed = true; - const warmedReads = contentReads.length; - const again = yield* provider.getDiff(continuation); - expect(again).toEqual(slice); - expect(contentReads).toHaveLength(warmedReads); - - const fileInput = { - ...input, - oldPath: "c.ts", - newPath: "c.ts", - changeType: "change" as const, - }; - const revisionsInput = { ...input, paths: ["c.ts"] }; - const contents = yield* provider.getDiffFileContents!(fileInput); - yield* provider.getFileRevisions!(revisionsInput); - const warmedContentsReads = contentReads.length; - - holdIterations = true; - const pending = yield* Effect.forkChild(provider.getDiffFileContents!(fileInput)); - yield* Deferred.await(started); - const pendingPreview = yield* Effect.forkChild(provider.getChangeRequestPreview!(input)); - yield* Deferred.await(previewStarted); - signedOut = true; - const signedOutReads = yield* Effect.all( - [ - provider.getDiff(continuation).pipe(Effect.asVoid, Effect.result), - provider.getDiffFileContents!(fileInput).pipe(Effect.asVoid, Effect.result), - provider.getFileRevisions!(revisionsInput).pipe(Effect.asVoid, Effect.result), - provider.getChangeRequestSummary!(input).pipe(Effect.asVoid, Effect.result), - provider.getChangeRequestPreview!(input).pipe(Effect.asVoid, Effect.result), - provider.getChangeRequest(input).pipe(Effect.asVoid, Effect.result), - ].map((read) => Effect.forkChild(read, { startImmediately: true })), - ); - yield* Deferred.succeed(release, undefined); - expect(yield* Fiber.join(pending)).toEqual(contents); - expect((yield* Fiber.join(pendingPreview)).title).toBe(PULL_REQUEST.title); - for (const result of yield* Effect.all(signedOutReads.map(Fiber.join))) { - expect(result._tag).toBe("Failure"); - const error = yield* Effect.flip(Effect.fromResult(result)); - expect(error.reason).toBe("unauthenticated"); - } - expect(iterationReads).toBe(9); - expect(contentReads).toHaveLength(warmedContentsReads); + const read = yield* readSlice({ + paths, + lines: 100, + width: 900, + cursor: `${ITERATION.id}:2`, + }); + + expect(patchedPaths(read.slice.patch)).toEqual(["c.ts", "d.ts"]); + expect(read.slice.nextCursor).toBe(`${ITERATION.id}:4`); }), ); it.effect("keeps the pull request being read, not the one looked up first", () => diff --git a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts index 254548ac322d..793624a316df 100644 --- a/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts +++ b/packages/source-control-azure-devops/src/server/AzureDevOpsPullRequestProvider.ts @@ -150,26 +150,15 @@ function toChangeRequest(pullRequest: AzureDevOpsPullRequest): ProviderChangeReq }; } -/** One pull request's pushes, in the terms Azure's iteration routes address them by. */ -interface IterationScope { - readonly cwd: string; - readonly location: AzureDevOpsRepositoryLocation; - readonly number: number; -} - export const make = Effect.gen(function* () { const cli = yield* AzureDevOpsPullRequestCli.AzureDevOpsPullRequestCli; // Made once with the provider, which the registry builds once, so this is the whole build's // allowance rather than one request's. const diffSpawns = yield* Semaphore.make(MAX_DIFF_SPAWNS); - const cacheKey = (...parts: ReadonlyArray) => - parts.map((part) => encodeURIComponent(String(part))).join("\n"); // Diff slices and context expansion read the same immutable blobs. Bound both count and size. const itemContents = yield* Cache.makeWith( (key: string) => { - const [cwd = "", project = "", repository = "", commit = "", path = ""] = key - .split("\n") - .map(decodeURIComponent); + const [cwd = "", project = "", repository = "", commit = "", path = ""] = key.split("\0"); return diffSpawns.withPermits(1)( cli.readItemContent({ cwd, location: { project, repository }, commit, path }), ); @@ -185,12 +174,8 @@ export const make = Effect.gen(function* () { const readItemContent = (input: Parameters[0]) => Cache.get( itemContents, - cacheKey( - input.cwd, - input.location.project, - input.location.repository, - input.commit, - input.path, + [input.cwd, input.location.project, input.location.repository, input.commit, input.path].join( + "\0", ), ); @@ -219,31 +204,22 @@ export const make = Effect.gen(function* () { const EMPTY_DIFF_SLICE: ProviderDiffSlice = { patch: "", truncated: false, nextCursor: null }; /** - * Where a pull request's repository lives, which is the route every other read of it needs and - * the one thing only the pull request itself states. A pull request cannot move between + * Where a pull request's repository lives, which is the route every other read of it needs. A + * pull request read and a listing row both state it, and a pull request cannot move between * repositories, so it is remembered rather than re-read: the marks alone would otherwise pay for * a whole pull request read every time they checked whether a file had been pushed to. Least - * recently used, so a listing walking cold pull requests cannot evict the one being read. + * recently used, so the pull requests being read stay. */ const locations = new Map(); - /** - * Keeps where a read placed a pull request. A listing's rows only take spare room, so walking a - * long listing cannot evict the pull request being read. - */ - const rememberLocation = ( - cwd: string, - pullRequest: AzureDevOpsPullRequest, - options: { readonly evict: boolean }, - ) => { + const rememberLocation = (cwd: string, pullRequest: AzureDevOpsPullRequest) => { if (pullRequest.location === null) return; const key = `${cwd} ${pullRequest.number}`; - if (!locations.has(key) && locations.size >= LOCATION_CACHE_CAPACITY) { - if (!options.evict) return; + locations.delete(key); + if (locations.size >= LOCATION_CACHE_CAPACITY) { const oldest = locations.keys().next().value; if (oldest !== undefined) locations.delete(oldest); } - locations.delete(key); locations.set(key, pullRequest.location); }; @@ -257,14 +233,11 @@ export const make = Effect.gen(function* () { return held; }; - // Each read uses the current CLI sign-in, including reads overlapping an account switch. const readPullRequest = (input: { readonly cwd: string; readonly number: number }) => cli .getPullRequest(input) .pipe( - Effect.tap((pullRequest) => - Effect.sync(() => rememberLocation(input.cwd, pullRequest, { evict: true })), - ), + Effect.tap((pullRequest) => Effect.sync(() => rememberLocation(input.cwd, pullRequest))), ); const locationOf = (input: { readonly cwd: string; readonly number: number }) => { @@ -274,41 +247,22 @@ export const make = Effect.gen(function* () { : readPullRequest(input).pipe(Effect.map((pullRequest) => pullRequest.location)); }; - const iterationKey = (scope: IterationScope, iterationId?: number) => - cacheKey( - scope.cwd, - scope.location.project, - scope.location.repository, - scope.number, - ...(iterationId === undefined ? [] : [iterationId]), - ); - - const fromIterationKey = (key: string) => { - const [cwd = "", project = "", repository = "", number = "", iterationId = ""] = key - .split("\n") - .map(decodeURIComponent); - return { - cwd, - location: { project, repository }, - number: Number(number), - iterationId: Number(iterationId), - }; - }; - - const changesKey = (scope: IterationScope, iteration: AzureDevOpsIteration) => - [ - iterationKey(scope, iteration.id), - encodeURIComponent(iteration.headCommit), - encodeURIComponent(iteration.mergeBaseCommit), - ].join("\n"); - /** * What one push changed, against the merge base. Never changes once the push lands, so the * detail's file count, every slice of the diff and the viewed marks share one complete read. * Incomplete reads are retried rather than keeping missing files for the life of the push. */ const iterationChanges = yield* Cache.makeWith( - (key: string) => cli.listIterationChanges(fromIterationKey(key)), + (key: string) => { + const [cwd = "", project = "", repository = "", number = "", iterationId = ""] = + key.split("\0"); + return cli.listIterationChanges({ + cwd, + location: { project, repository }, + number: Number(number), + iterationId: Number(iterationId), + }); + }, { capacity: ITERATION_CHANGES_CAPACITY, timeToLive: (exit) => @@ -382,16 +336,41 @@ export const make = Effect.gen(function* () { * reported against the merge base rather than against the push before it, so the newest one is * the whole of the change rather than the last slice of it. */ - const listLatestChanges = ( - input: IterationScope & { readonly iterations: ReadonlyArray }, - ) => { + const changesOf = ( + input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; + }, + iteration: AzureDevOpsIteration, + ) => + Cache.get( + iterationChanges, + // The commits make the key immutable; the rest is what the read is addressed by. + [ + input.cwd, + input.location.project, + input.location.repository, + input.number, + iteration.id, + iteration.headCommit, + iteration.mergeBaseCommit, + ].join("\0"), + ); + + const listLatestChanges = (input: { + readonly cwd: string; + readonly location: AzureDevOpsRepositoryLocation; + readonly number: number; + readonly iterations: ReadonlyArray; + }) => { const latest = input.iterations.at(-1); return latest === undefined ? Effect.succeed({ changes: [], truncated: false, } as AzureDevOpsPullRequestCli.AzureDevOpsIterationChanges) - : Cache.get(iterationChanges, changesKey(input, latest)); + : changesOf(input, latest); }; const provider: PullRequestProviderApi = { @@ -423,7 +402,7 @@ export const make = Effect.gen(function* () { // read of the pull request before its conversation or its pushes can be asked for. Effect.tap((batch) => Effect.sync(() => { - for (const row of batch.items) rememberLocation(input.cwd, row, { evict: false }); + for (const row of batch.items) rememberLocation(input.cwd, row); }), ), Effect.map((batch) => ({ @@ -552,20 +531,20 @@ export const make = Effect.gen(function* () { // the Code tab has nothing to scope itself to and always asks for the whole change. getDiff: (input) => Effect.gen(function* () { - const location = yield* locationOf(input); - if (location === null) return EMPTY_DIFF_SLICE; - const scope = { cwd: input.cwd, location, number: input.number }; + const scope = yield* diffScope(input); + if (scope === null) return EMPTY_DIFF_SLICE; const cursor = parseAzureDevOpsDiffCursor(input.cursor); - // Even a fully cached continuation must check the current sign-in can read this review. - const iterations = yield* cli.listIterations(scope); // Reading on stays with the push the first slice was taken against. A push landing // mid-read would otherwise renumber the files and hand the reader one twice, or none. const iteration = cursor === null - ? iterations.at(-1) - : iterations.find((candidate) => candidate.id === cursor.iterationId); + ? scope.iterations.at(-1) + : scope.iterations.find((candidate) => candidate.id === cursor.iterationId); if (iteration === undefined) return EMPTY_DIFF_SLICE; - const listed = yield* Cache.get(iterationChanges, changesKey(scope, iteration)); + const listed = yield* changesOf( + { cwd: input.cwd, location: scope.location, number: input.number }, + iteration, + ); const changes = listed.changes; const sections: string[] = []; diff --git a/packages/source-control-forgejo/src/server/ForgejoCli.ts b/packages/source-control-forgejo/src/server/ForgejoCli.ts index 3404b1d5900e..90501a4591ca 100644 --- a/packages/source-control-forgejo/src/server/ForgejoCli.ts +++ b/packages/source-control-forgejo/src/server/ForgejoCli.ts @@ -109,17 +109,6 @@ export interface ForgejoRepositoryInput { readonly host?: string; } -/** What resolving a target reads from a `ForgejoRepositoryInput`. */ -interface ForgejoTargetInput { - readonly cwd: string; - readonly reference?: string | undefined; - readonly repository?: string | undefined; - readonly host?: string | undefined; - readonly remoteUrl?: string | undefined; - readonly providerBaseUrl?: string | undefined; - readonly requestedHost?: string | undefined; -} - export interface ForgejoRepository { readonly command?: "fj" | "tea"; readonly login: string; @@ -436,24 +425,15 @@ export const make = Effect.gen(function* () { ), ); - type FjLogin = Pick; - - /** The stored token when fj checked it within the last 30s, read without taking `authLock`. */ - const checkedToken = Effect.fn("ForgejoCli.checkedToken")(function* ( + const authenticateFj = Effect.fn("ForgejoCli.authenticateFj")(function* ( cwd: string, - login: FjLogin, + login: typeof ForgejoLoginSchema.Type, ) { - const token = (yield* readKeys(cwd)).hosts[login.name]?.token; + const keys = yield* readKeys(cwd); + const token = keys.hosts[login.name]?.token; const now = yield* Clock.currentTimeMillis; const cached = authenticated.get(login.url); - return token && cached?.token === token && now - cached.time < 30_000 ? token : undefined; - }); - - const renewFj = Effect.fn("ForgejoCli.renewFj")(function* (cwd: string, login: FjLogin) { - // Another request may have renewed this login while this one waited for the lock. - const checked = yield* checkedToken(cwd, login); - if (checked) return checked; - const now = yield* Clock.currentTimeMillis; + if (token && cached?.token === token && now - cached.time < 30_000) return token; yield* execute({ command: "fj", cwd, args: ["--host", login.url, "whoami"] }); // fj owns OAuth renewal. Re-read the file after it has refreshed an expired token. const refreshed = (yield* readKeys(cwd)).hosts[login.name]?.token; @@ -468,12 +448,6 @@ export const make = Effect.gen(function* () { return refreshed; }, authLock.withPermits(1)); - // Only a renewal serialises; requests with a recently checked token run concurrently. - const authenticateFj = (cwd: string, login: FjLogin) => - checkedToken(cwd, login).pipe( - Effect.flatMap((token) => (token ? Effect.succeed(token) : renewFj(cwd, login))), - ); - const getAccount: NonNullable = Effect.fn( "ForgejoCli.getAccount", )(function* (input) { @@ -507,17 +481,17 @@ export const make = Effect.gen(function* () { }); const resolveTarget = Effect.fn("ForgejoCli.resolveTarget")(function* ( - input: ForgejoTargetInput, + input: ForgejoRepositoryInput, hostOnly = false, ) { const referenceRemote = input.reference ? parseForgejoRemote(input.reference) : null; - let remoteUrl = [input.reference, input.repository, input.remoteUrl].find( + let remoteUrl = [input.reference, input.repository, input.context?.remoteUrl].find( (value) => value && parseForgejoRemote(value), ); let remote = referenceRemote ?? (input.repository ? parseForgejoRemote(input.repository) : null) ?? - (input.remoteUrl ? parseForgejoRemote(input.remoteUrl) : null); + (input.context ? parseForgejoRemote(input.context.remoteUrl) : null); if (!remote && (!input.repository || input.host)) { const result = yield* process .run({ @@ -573,13 +547,13 @@ export const make = Effect.gen(function* () { ssh: false, path: remote?.path ?? "", }; - const schemeRemoteUrl = remote?.ssh ? input.providerBaseUrl : remoteUrl; + const schemeRemoteUrl = remote?.ssh ? input.context?.provider.baseUrl : remoteUrl; const fjLogins = yield* listLogins({ cwd: input.cwd, command: "fj", ...(schemeRemoteUrl ? { remoteUrl: schemeRemoteUrl } : {}), }); - const requestedHost = input.host ?? input.requestedHost; + const requestedHost = input.host ?? input.context?.requestedHost; const matchHostOnly = hostOnly || (!!input.host && !remote?.path); const selectLogin = (logins: ReturnType) => remote @@ -655,25 +629,11 @@ export const make = Effect.gen(function* () { }); return { command, login: login.name, repository, baseUrl: login.url.replace(/\/+$/, "") }; }); - // Select credentials for every request; a pending selection can belong to another account. - const resolveRepository = (input: ForgejoRepositoryInput) => - resolveTarget({ - ...input, - remoteUrl: input.context?.remoteUrl, - providerBaseUrl: input.context?.provider.baseUrl, - requestedHost: input.context?.requestedHost, - }); + const resolveRepository = (input: ForgejoRepositoryInput) => resolveTarget(input); const api = Effect.fn("ForgejoCli.api")(function* (input: ForgejoApiInput) { - const hostOnly = - input.path.replace(/^\/+/, "") === "user" && (!input.method || input.method === "GET"); const repository = yield* resolveTarget( - { - ...input, - remoteUrl: input.context?.remoteUrl, - providerBaseUrl: input.context?.provider.baseUrl, - requestedHost: input.context?.requestedHost, - }, - hostOnly, + input, + input.path.replace(/^\/+/, "") === "user" && (!input.method || input.method === "GET"), ); const stdin = input.body === undefined diff --git a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts index a0e2776be0d7..d55c205240dc 100644 --- a/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts +++ b/packages/source-control-forgejo/src/server/ForgejoPullRequestProvider.ts @@ -148,13 +148,17 @@ export const make = Effect.gen(function* () { } return { items, truncated: true }; }); - // These reads select the current account independently of older pending requests. + const write = (input: ForgejoCli.ForgejoApiInput) => request(input).pipe(Effect.asVoid); const getPull = (input: ProviderRepositoryRef & { readonly number: number }) => read( - { cwd: input.cwd, repository: input.repository, host: input.host, path: pullPath(input) }, + { + cwd: input.cwd, + repository: input.repository, + host: input.host, + path: pullPath(input), + }, ForgejoPullRequest, ); - const write = (input: ForgejoCli.ForgejoApiInput) => request(input).pipe(Effect.asVoid); const getRepo = (input: ProviderRepositoryRef) => read({ ...input, path: repoPath(input) }, ForgejoRepository); const getViewer = (input: { diff --git a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts index 41b18f531c05..468d1402eb1d 100644 --- a/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts +++ b/packages/source-control-forgejo/src/server/ForgejoSourceControlProvider.test.ts @@ -1,8 +1,6 @@ import { SourceControlProviderKind } from "@t3tools/contracts"; import { assert, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; -import * as Deferred from "effect/Deferred"; -import * as Fiber from "effect/Fiber"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; @@ -33,302 +31,35 @@ const requestJson = (request: HttpClientRequest.HttpClientRequest): unknown => { }; const encodeJsonEffect = Schema.encodeEffect(Schema.fromJsonString(Schema.Unknown)); -it.effect("refreshes Forgejo viewer permissions immediately after credentials change", () => { - let account = "alice"; - const requests: string[] = []; - const renewals: string[] = []; - let holdViewer: Effect.Effect = Effect.void; - return Effect.gen(function* () { - const cli = yield* ForgejoCli.make; - const provider = yield* ForgejoPullRequestProvider.make.pipe( - Effect.provideService(ForgejoCli.ForgejoCli, cli), - ); - const input = { cwd: "/repo", repository: "acme/web", host: "forgejo.test", number: 42 }; - const alice = yield* provider.getViewerPermissions(input); - assert.deepStrictEqual(alice.verdicts, ["comment"]); - assert.include(alice.actions, "merge"); - assert.strictEqual(alice.labels, true); - const started = yield* Deferred.make(); - const release = yield* Deferred.make(); - holdViewer = Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))); - const detail = yield* provider.getChangeRequest(input).pipe(Effect.forkChild); - yield* Deferred.await(started); - account = "bob"; - holdViewer = Effect.void; - const bob = yield* provider.getViewerPermissions(input); - assert.deepStrictEqual(bob.verdicts, ["comment", "approve", "request-changes"]); - assert.deepStrictEqual(bob.actions, []); - assert.strictEqual(bob.labels, false); - assert.strictEqual(bob.requestReviewers, false); - assert.strictEqual(requests.length, 9); - assert.deepStrictEqual(renewals, ["alice", "bob"]); - const freshDetail = yield* provider.getChangeRequest(input).pipe(Effect.forkChild); - yield* Effect.yieldNow; - yield* Deferred.succeed(release, undefined); - assert.deepStrictEqual((yield* Fiber.join(freshDetail)).viewerPermissions?.verdicts, [ - "comment", - "approve", - "request-changes", - ]); - assert.deepStrictEqual((yield* Fiber.join(detail)).viewerPermissions?.verdicts, ["comment"]); - assert.strictEqual(requests.length, 14); - }).pipe( - Effect.provideService( - FileSystem.FileSystem, - FileSystem.makeNoop({ - exists: () => Effect.succeed(true), - readFileString: () => - Effect.succeed( - encodeJson({ hosts: { "forgejo.test": { type: "Application", token: account } } }), - ), - }), - ), - Effect.provideService( - HttpClient.HttpClient, - HttpClient.make((request) => { - requests.push(request.url); - assert.strictEqual(request.headers.authorization, `token ${account}`); - const response = request.url.endsWith("/user") - ? { login: account } - : request.url.endsWith("/pulls/42") - ? { - number: 42, - title: "Permissions", - body: "", - html_url: "https://forgejo.test/acme/web/pulls/42", - user: { login: "alice" }, - state: "open", - merged: false, - head: { ref: "feature", sha: "head", repo: null }, - base: { ref: "main", sha: "base", repo: null }, - created_at: "2026-09-16T00:00:00Z", - updated_at: "2026-09-16T00:00:00Z", - closed_at: null, - merged_at: null, - labels: [], - } - : request.url.includes("/statuses/") - ? [] - : { full_name: "acme/web", permissions: { push: account === "alice", admin: false } }; - return (request.url.endsWith("/user") ? holdViewer : Effect.void).pipe( - Effect.as(HttpClientResponse.fromWeb(request, new Response(encodeJson(response)))), - ); - }), - ), - Effect.provide( - TestSourceControlHost.layer({ - process: { - run: (input) => { - if (input.command === "git") - return Effect.succeed( - processOutput("origin\thttps://forgejo.test/acme/web.git (fetch)"), - ); - assert.strictEqual(input.command, "fj"); - assert.deepStrictEqual(input.args, ["--host", "https://forgejo.test", "whoami"]); - renewals.push(account); - return Effect.succeed(processOutput("")); - }, - }, - }), - ), - ); -}); - -it.effect("reselects the current tea login for writes while an older read is pending", () => { - let account = "alice"; - const selected: string[] = []; - let holdLogin: Effect.Effect = Effect.void; - return Effect.gen(function* () { - const cli = yield* ForgejoCli.make; - const input = { - cwd: "/repo\n%work", - repository: "https://forgejo.test/acme/web", - path: "repos/acme/web/pulls/42", - }; - assert.strictEqual((yield* cli.api(input)).stdout, '{"login":"alice"}'); - account = "bob"; - assert.strictEqual((yield* cli.api(input)).stdout, '{"login":"bob"}'); - const started = yield* Deferred.make(); - const release = yield* Deferred.make(); - holdLogin = Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))); - const pending = yield* cli.api(input).pipe(Effect.forkChild); - yield* Deferred.await(started); - account = "carol"; - holdLogin = Effect.void; - assert.strictEqual((yield* cli.api(input)).stdout, '{"login":"carol"}'); - const write = yield* cli - .api({ ...input, method: "PATCH", body: { state: "closed" } }) - .pipe(Effect.forkChild); - yield* Effect.yieldNow; - yield* Deferred.succeed(release, undefined); - assert.strictEqual((yield* Fiber.join(write)).stdout, '{"login":"carol"}'); - assert.strictEqual((yield* Fiber.join(pending)).stdout, '{"login":"bob"}'); - assert.deepStrictEqual(selected, ["alice", "bob", "carol", "carol", "bob"]); - }).pipe( - Effect.provideService( - FileSystem.FileSystem, - FileSystem.makeNoop({ exists: () => Effect.succeed(false) }), - ), - Effect.provideService( - HttpClient.HttpClient, - HttpClient.make(() => Effect.die("tea owns HTTP")), - ), - Effect.provide( - TestSourceControlHost.layer({ - process: { - run: (input) => { - assert.strictEqual(input.command, "tea"); - assert.strictEqual(input.cwd, "/repo\n%work"); - if (input.args[0] === "login") - return holdLogin.pipe( - Effect.as( - processOutput( - encodeJson([ - { - name: account, - url: "https://forgejo.test", - user: account, - default: "true", - }, - ]), - ), - ), - ); - assert.strictEqual(input.args[0], "api"); - const login = input.args[input.args.indexOf("--login") + 1] ?? ""; - assert.notStrictEqual(login, ""); - selected.push(login); - return Effect.succeed( - processOutput(encodeJson({ login }), { stderr: "HTTP/1.1 200 OK\n" }), - ); - }, - }, - }), - ), - ); -}); - -it.effect("keeps Forgejo pull reads fresh during pending reads, writes and failures", () => - Effect.gen(function* () { - const started = yield* Deferred.make(); - const release = yield* Deferred.make(); - const paths: string[] = []; - let pullReads = 0; - let state = "open"; - let reject = false; - const provider = yield* ForgejoPullRequestProvider.make.pipe( - Effect.provide( - Layer.mock(ForgejoCli.ForgejoCli)({ - api: (input) => - Effect.gen(function* () { - assert.strictEqual(input.cwd, "/repo\n%work"); - paths.push(input.path); - if (input.method === "PATCH") { - state = "closed"; - return processOutput("{}"); - } - if (input.path.endsWith("/pulls/42")) { - pullReads++; - if (reject) - return yield* new ForgejoCli.ForgejoCliError({ - command: "tea", - cwd: input.cwd, - reason: "rate-limit", - detail: "limited", - }); - const response = { - number: 42, - title: "Shared pull", - body: "", - html_url: "https://forgejo.test/acme/web/pulls/42", - user: { login: "alice" }, - state, - merged: false, - head: { ref: "feature", sha: state === "open" ? "before" : "after", repo: null }, - base: { ref: "main", sha: "base", repo: null }, - created_at: "2026-09-16T00:00:00Z", - updated_at: "2026-09-16T00:00:00Z", - closed_at: null, - merged_at: null, - labels: [], - }; - if (pullReads === 1) { - yield* Deferred.succeed(started, undefined); - yield* Deferred.await(release); - } - return processOutput(encodeJson(response)); - } - return processOutput( - input.path === "user" - ? '{"login":"bob"}' - : input.path.includes("/statuses/") - ? "[]" - : '{"full_name":"acme/web","permissions":{"push":true,"admin":false}}', - ); - }), - }), - ), - ); - const summary = provider.getChangeRequestSummary; - const checks = provider.getChangeRequestChecks; - if (!summary || !checks) return yield* Effect.die("narrow reads missing"); - const input = { cwd: "/repo\n%work", repository: "acme/web", host: "forgejo.test", number: 42 }; - const reads = yield* Effect.all( - [summary(input), checks(input), provider.getChangeRequest(input)], - { concurrency: "unbounded" }, - ).pipe(Effect.forkChild); - yield* Deferred.await(started); - yield* Effect.yieldNow; - assert.strictEqual(pullReads, 3); - yield* provider.runAction({ ...input, action: "close" }); - const updated = yield* checks(input); - assert.strictEqual(updated.state, "closed"); - assert.strictEqual(pullReads, 4); - assert.include(paths, "repos/acme/web/statuses/after?sort=recentupdate&limit=50&page=1"); - yield* Deferred.succeed(release, undefined); - const original = yield* Fiber.join(reads); - assert.strictEqual(original[0].state, "open"); - reject = true; - const failed = yield* summary(input).pipe(Effect.result); - assert.strictEqual(failed._tag, "Failure"); - assert.strictEqual(pullReads, 5); - reject = false; - assert.strictEqual((yield* summary(input)).state, "closed"); - assert.strictEqual(pullReads, 6); - }), -); - -it.effect("submits a Forgejo review using a fresh head and an empty GET body", () => { +it.effect("submits a Forgejo review without sending its summary in the preliminary GET", () => { const methods: string[] = []; - let head = "old-head"; - let holdPull: Effect.Effect = Effect.void; const reviewClient = HttpClient.make((request) => { methods.push(request.method); if (request.method === "GET") { assert.strictEqual(request.url, "https://forgejo.test/api/v1/repos/maria/project/pulls/42"); - const response = HttpClientResponse.fromWeb( - request, - new Response( - encodeJson({ - number: 42, - title: "Review target", - body: "", - html_url: "https://forgejo.test/maria/project/pulls/42", - user: { login: "maria" }, - state: "open", - merged: false, - head: { ref: "feature", sha: head, repo: null }, - base: { ref: "main", sha: "base", repo: null }, - created_at: "2026-09-13T00:00:00Z", - updated_at: "2026-09-13T00:00:00Z", - closed_at: null, - merged_at: null, - labels: [], - }), + return Effect.succeed( + HttpClientResponse.fromWeb( + request, + new Response( + encodeJson({ + number: 42, + title: "Review target", + body: "", + html_url: "https://forgejo.test/maria/project/pulls/42", + user: { login: "maria" }, + state: "open", + merged: false, + head: { ref: "feature", sha: "head", repo: null }, + base: { ref: "main", sha: "base", repo: null }, + created_at: "2026-09-13T00:00:00Z", + updated_at: "2026-09-13T00:00:00Z", + closed_at: null, + merged_at: null, + labels: [], + }), + ), ), ); - assert.strictEqual(request.body._tag, "Empty"); - return holdPull.pipe(Effect.as(response)); } assert.strictEqual(request.method, "POST"); assert.strictEqual( @@ -350,36 +81,16 @@ it.effect("submits a Forgejo review using a fresh head and an empty GET body", ( const provider = yield* ForgejoPullRequestProvider.make.pipe( Effect.provideService(ForgejoCli.ForgejoCli, cli), ); - const started = yield* Deferred.make(); - const release = yield* Deferred.make(); - holdPull = Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))); - const summary = provider.getChangeRequestSummary; - if (!summary) return yield* Effect.die("narrow reads missing"); - const pending = yield* summary({ + yield* provider.submitReview({ cwd: "/repo", repository: "maria/project", host: "forgejo.test", number: 42, - }).pipe(Effect.forkChild); - yield* Deferred.await(started); - head = "head"; - holdPull = Effect.void; - const review = yield* provider - .submitReview({ - cwd: "/repo", - repository: "maria/project", - host: "forgejo.test", - number: 42, - verdict: "comment", - body: "Review summary", - comments: [], - }) - .pipe(Effect.forkChild); - yield* Effect.yieldNow; - yield* Deferred.succeed(release, undefined); - yield* Fiber.join(review); - assert.deepStrictEqual(methods, ["GET", "GET", "POST"]); - yield* Fiber.join(pending); + verdict: "comment", + body: "Review summary", + comments: [], + }); + assert.deepStrictEqual(methods, ["GET", "POST"]); }).pipe( Effect.provideService(HttpClient.HttpClient, reviewClient), Effect.provideService( @@ -388,9 +99,7 @@ it.effect("submits a Forgejo review using a fresh head and an empty GET body", ( exists: () => Effect.succeed(true), readFileString: () => Effect.succeed( - encodeJson({ - hosts: { "forgejo.test": { type: "Application", token: "test-token" } }, - }), + encodeJson({ hosts: { "forgejo.test": { type: "Application", token: "test-token" } } }), ), }), ), diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts index c375cdfcfd05..cd01e519196f 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.test.ts @@ -19,10 +19,7 @@ import { assert, describe, it } from "@effect/vitest"; import * as Effect from "effect/Effect"; import * as Context from "effect/Context"; -import * as Deferred from "effect/Deferred"; -import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; -import * as Redacted from "effect/Redacted"; import * as Schema from "effect/Schema"; import * as FetchHttpClient from "effect/http/FetchHttpClient"; import * as HttpClient from "effect/http/HttpClient"; @@ -340,153 +337,6 @@ describe("GitCafePullRequestProvider", () => { }).pipe(Effect.provide(server.layer)); }); - it.effect( - "refreshes preview, summary, detail and permissions during a pending authorized read", - () => - Effect.gen(function* () { - const started = yield* Deferred.make(); - const release = yield* Deferred.make(); - let token: string | null = "old-token"; - const authorizations: Array = []; - const client = HttpClient.make((request) => { - const authorization = request.headers.authorization; - authorizations.push(authorization); - const old = authorization === "Bearer old-token"; - const revoked = authorization === "Bearer revoked-token"; - return ( - old - ? Deferred.succeed(started, undefined).pipe(Effect.andThen(Deferred.await(release))) - : Effect.void - ).pipe( - Effect.as( - HttpClientResponse.fromWeb( - request, - new Response( - encodeJson( - revoked - ? { title: "Unauthorized" } - : new URL(request.url).pathname.endsWith("/status") - ? { - merge: { - conflicts: "unknown", - fastForward: true, - strategies: ["merge"], - }, - checks: { pending: 0, failing: 0, total: 0 }, - } - : { - ...pull, - headOid: null, - title: old ? "Old credentials" : "New credentials", - capabilities: { ...pull.capabilities, edit: !old }, - }, - ), - { status: revoked ? 401 : 200 }, - ), - ), - ), - ); - }); - const provider = yield* GitCafePullRequestProvider.make.pipe( - Effect.provide( - GitCafeApi.layer.pipe( - Layer.provide(Layer.succeed(HttpClient.HttpClient, client)), - Layer.provide( - Layer.succeed(GitCafeCredentials.GitCafeCredentials, { - get: (host) => - Effect.suspend(() => - token === null - ? Effect.fail(new GitCafeCredentials.GitCafeNotSignedInError({ host })) - : Effect.succeed({ - host, - token: Redacted.make(token), - source: "settings" as const, - }), - ), - invalidate: () => Effect.void, - cliEnv: () => Effect.succeed({}), - }), - ), - ), - ), - ); - const pending = yield* provider.getChangeRequestPreview!(target).pipe(Effect.forkChild); - yield* Deferred.await(started); - token = "new-token"; - const refreshed = [ - yield* provider.getChangeRequestPreview!(target), - yield* provider.getChangeRequestSummary!(target), - yield* provider.getChangeRequest(target), - ]; - assert.deepStrictEqual( - refreshed.map((item) => item.title), - ["New credentials", "New credentials", "New credentials"], - ); - assert.strictEqual((yield* provider.getViewerPermissions(target)).editChangeRequest, true); - for (let phase = 0; phase < 2; phase++) { - token = phase === 0 ? null : "revoked-token"; - const rejected = [ - yield* provider.getChangeRequestPreview!(target).pipe(Effect.flip), - yield* provider.getChangeRequestSummary!(target).pipe(Effect.flip), - yield* provider.getChangeRequest(target).pipe(Effect.flip), - yield* provider.getViewerPermissions(target).pipe(Effect.flip), - ]; - assert.deepStrictEqual( - rejected.map((error) => [error.operation, error.reason]), - [ - ["getChangeRequestPreview", "unauthenticated"], - ["getChangeRequestSummary", "unauthenticated"], - ["getChangeRequest", "unauthenticated"], - ["getViewerPermissions", "unauthenticated"], - ], - ); - if (phase === 0) { - yield* Deferred.succeed(release, undefined); - assert.strictEqual((yield* Fiber.join(pending)).title, "Old credentials"); - } - } - assert.deepStrictEqual(authorizations, [ - "Bearer old-token", - ...Array.from({ length: 5 }, () => "Bearer new-token"), - ...Array.from({ length: 4 }, () => "Bearer revoked-token"), - ]); - }), - ); - - it.effect.each([true, false])( - "reads a fresh pull before and after a write succeeds: %s", - (succeeds) => { - let reads = 0; - const server = fakeGitCafe({ - "GET /repos/owner/repo/pulls/7": () => ({ - ...pull, - version: ++reads, - }), - ...(succeeds ? { "POST /repos/owner/repo/pulls/7/close": {} } : {}), - }); - return Effect.gen(function* () { - const provider = yield* GitCafePullRequestProvider.make; - yield* provider.getChangeRequestPreview!(target); - const result = yield* provider - .runAction({ ...target, action: "close" }) - .pipe(Effect.result); - assert.strictEqual(result._tag, succeeds ? "Success" : "Failure"); - assert.deepStrictEqual(server.sent.at(-1)?.body, { expectedVersion: 2 }); - yield* provider.getChangeRequestPreview!(target); - assert.strictEqual(reads, 3); - assert.deepStrictEqual( - server.sent.map((request) => `${request.method} ${request.path}`), - [ - "GET /repos/owner/repo/pulls/7", - "GET /repos/owner/repo/pulls/7", - "POST /repos/owner/repo/pulls/7/close", - "GET /repos/owner/repo/pulls/7", - ], - ); - }).pipe(Effect.provide(server.layer)); - }, - ); - it.effect("reads the diff from the pull request's changed files", () => { const file = { path: "file.ts", diff --git a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts index 0598b4d96be2..de331b519afd 100644 --- a/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts +++ b/packages/source-control-gitcafe/src/server/GitCafePullRequestProvider.ts @@ -300,15 +300,14 @@ export const make = Effect.gen(function* () { if (stack === null) return null; const members = yield* Effect.forEach( stack.members, - (member) => { - if (member.state === "merged") return Effect.succeed(member); - const layer = { ...input, number: member.pullRequestNumber }; - return readPull(operation, layer).pipe( - Effect.map((pull) => - pull.headOid == null ? member : { ...member, headOid: pull.headOid }, - ), - ); - }, + (member) => + member.state === "merged" + ? Effect.succeed(member) + : readPull(operation, { ...input, number: member.pullRequestNumber }).pipe( + Effect.map((pull) => + pull.headOid == null ? member : { ...member, headOid: pull.headOid }, + ), + ), { concurrency: 4 }, ); return { ...stack, members }; @@ -460,7 +459,7 @@ export const make = Effect.gen(function* () { const lineStats = yield* Cache.makeWith( (key: string) => { const [host = "", repository = "", number = "", version = "", headOid = "", baseOid = ""] = - key.split("\n").map(decodeURIComponent); + key.split("\0"); return readLineStats( { host, repository, number: Number(number) }, Number(version), @@ -468,11 +467,8 @@ export const make = Effect.gen(function* () { ).pipe( Effect.map(({ revision, complete, ...stats }) => ({ stats, - held: - complete && - revision?.version === Number(version) && - revision.headOid === headOid && - revision.baseOid === baseOid, + // The pages already agree with the version and head asked for; the base is the pull's. + held: complete && revision?.baseOid === baseOid, })), ); }, @@ -534,7 +530,7 @@ export const make = Effect.gen(function* () { /** The viewer's actor on a repository; a viewer GitCafe knows no actor for is asked again. */ const viewerActors = yield* Cache.makeWith( (key: string) => { - const [host = "", repository = "", viewer = ""] = key.split("\n").map(decodeURIComponent); + const [host = "", repository = "", viewer = ""] = key.split("\0"); return read( "listChangeRequests", { host }, @@ -575,9 +571,7 @@ export const make = Effect.gen(function* () { // or a rate limit still fails, since the service acts on those for the whole host. const actor = yield* Cache.get( viewerActors, - [input.host, input.repository, input.viewer.toLowerCase()] - .map((value) => encodeURIComponent(String(value ?? ""))) - .join("\n"), + `${input.host}\0${input.repository}\0${input.viewer.toLowerCase()}`, ).pipe( Effect.map(Option.some), Effect.catchTags({ @@ -669,9 +663,7 @@ export const make = Effect.gen(function* () { pull.version, pull.headOid, pull.observedBaseOid ?? "", - ] - .map((value) => encodeURIComponent(String(value ?? ""))) - .join("\n"), + ].join("\0"), ).pipe( Effect.map(({ stats }) => stats), Effect.orElseSucceed(() => null), diff --git a/packages/source-control-github/src/server/GitHubPullRequestApi.ts b/packages/source-control-github/src/server/GitHubPullRequestApi.ts index 12a2e50f45eb..b6447ca3b35e 100644 --- a/packages/source-control-github/src/server/GitHubPullRequestApi.ts +++ b/packages/source-control-github/src/server/GitHubPullRequestApi.ts @@ -1,7 +1,6 @@ import { removeAgentCredits } from "@t3tools/source-control-core/server/mergeMessage"; import { KnownWorkflowRuns, makeChecksRevalidator } from "./gitHubConditionalChecks.ts"; import { runGitHubStackAction, type GitHubStackActionError } from "./githubStackActions.ts"; -import * as Cache from "effect/Cache"; import * as Cause from "effect/Cause"; import * as Context from "effect/Context"; import * as Clock from "effect/Clock"; @@ -10,6 +9,7 @@ import * as Effect from "effect/Effect"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Exit from "effect/Exit"; +import * as Fiber from "effect/Fiber"; import * as Option from "effect/Option"; import * as Request from "effect/Request"; import * as RequestResolver from "effect/RequestResolver"; @@ -1106,28 +1106,65 @@ const SIMPLE_ACTION_MUTATIONS = { /** @public Service construction is part of the canonical Effect module API. */ export const make = Effect.gen(function* () { const transport = yield* GitHubApi.GitHubApi; - // A read started after a write must not join a request that captured the old state. - const forgetReads = Effect.suspend(() => - Effect.all([Cache.invalidateAll(detailsInFlight), Cache.invalidateAll(workflowRunsInFlight)]), - ); + // Opening a pull request asks GitHub the same questions from its detail, its checks and the + // viewer's permissions at once. A read that starts while an identical one is in flight under the + // same credential takes its answer; nothing is kept once it lands. A write, or a stack push, + // drops what is in flight, so a read started after it asks GitHub again. + const restReads = new Map< + string, + Fiber.Fiber + >(); + const graphqlReads = new Map>(); + const forgetReads = Effect.sync(() => { + restReads.clear(); + graphqlReads.clear(); + }); + const encodeReadKey = Schema.encodeEffect(Schema.fromJsonString(Schema.Unknown)); + const shareRead = ( + reads: Map>, + send: (input: I) => Effect.Effect, + input: I, + ) => + Effect.gen(function* () { + const host = input.host.trim().toLowerCase(); + const { token, fingerprint } = yield* transport.credential(host); + // Every read input is plain data, so it always encodes. + const key = yield* encodeReadKey([ + fingerprint, + yield* SourceControlRateLimit.CredentialScope, + yield* SourceControlRateLimit.Interactive, + input, + ]).pipe(Effect.orDie); + const held = reads.get(key); + if (held !== undefined) return yield* Fiber.join(held); + // Pinned, so whoever joins gets the answer of the credential the key names. + const fiber = yield* send(input).pipe( + Effect.provideService(GitHubApi.PinnedGitHubCredential, { + host, + token, + credentialFingerprint: fingerprint, + }), + Effect.forkDetach, + ); + reads.set(key, fiber); + return yield* Fiber.join(fiber).pipe( + Effect.ensuring( + Effect.sync(() => { + if (reads.get(key) === fiber) reads.delete(key); + }), + ), + ); + }); const api = GitHubApi.GitHubApi.of({ ...transport, graphql: (input) => - transport - .graphql(input) - .pipe( - Effect.ensuring( - input.query.trimStart().startsWith("mutation") ? forgetReads : Effect.void, - ), - ), + input.query.trimStart().startsWith("mutation") + ? transport.graphql(input).pipe(Effect.ensuring(forgetReads)) + : shareRead(graphqlReads, transport.graphql, input), rest: (input) => - transport - .rest(input) - .pipe( - Effect.ensuring( - input.method !== undefined && input.method !== "GET" ? forgetReads : Effect.void, - ), - ), + input.method === undefined || input.method === "GET" + ? shareRead(restReads, transport.rest, input) + : transport.rest(input).pipe(Effect.ensuring(forgetReads)), }); const sourceControlHost = yield* SourceControlHost.SourceControlHost; const fileSystem = yield* FileSystem.FileSystem; @@ -1749,63 +1786,6 @@ export const make = Effect.gen(function* () { { concurrency: 2 }, ).pipe(Effect.map(([, runs]) => runs)); - // Opening a pull request reads its detail, its checks and the viewer's permissions at once, and - // each starts from the same detail query, and for a fork's pull request the same workflow runs. - // Reads in flight together under the same credential and quota share one request. Nothing is - // kept once it lands, so a read that starts later asks GitHub again. - const inFlightKey = (cwd: string, host: string, ...parts: ReadonlyArray) => - Effect.gen(function* () { - // Resolve locally even for unpinned clients, then keep the read on that credential. - const { token, fingerprint } = yield* api.credential(host); - const key = [ - cwd, - host, - ...parts, - yield* SourceControlRateLimit.Interactive, - yield* SourceControlRateLimit.CredentialScope, - fingerprint, - ] - .map((part) => encodeURIComponent(String(part))) - .join("\n"); - return { - key, - credential: { host: host.trim().toLowerCase(), token, credentialFingerprint: fingerprint }, - }; - }); - const detailsInFlight = yield* Cache.makeWith( - (key: string) => { - const [cwd = "", host = "", repository = "", number = ""] = key - .split("\n") - .map(decodeURIComponent); - return getPullRequestDetail({ cwd, host, repository, number: Number(number) }); - }, - { capacity: 64, timeToLive: () => 0 }, - ); - const workflowRunsInFlight = yield* Cache.makeWith( - (key: string) => { - const [ - cwd = "", - host = "", - repository = "", - number = "", - headSha = "", - headBranch = "", - headRepositoryOwner = "", - ] = key.split("\n").map(decodeURIComponent); - return listWorkflowRunsRequiringApproval({ - cwd, - host, - repository, - number: Number(number), - headSha, - headBranch, - headRepositoryOwner, - isCrossRepository: true, - }); - }, - { capacity: 64, timeToLive: () => 0 }, - ); - /** One pull request's summary, through the same aliased query the batch uses. */ const viewPullRequestSummary = (input: PullRequestSummaryRead) => { const { owner, name } = parseRepositorySelector(input.repository); @@ -2189,14 +2169,7 @@ export const make = Effect.gen(function* () { getPullRequestSummary, getPullRequestWatchFingerprint, - getPullRequestDetail: (input) => - inFlightKey(input.cwd, input.host, input.repository, input.number).pipe( - Effect.flatMap(({ key, credential }) => - Cache.get(detailsInFlight, key).pipe( - Effect.provideService(GitHubApi.PinnedGitHubCredential, credential), - ), - ), - ), + getPullRequestDetail, getPullRequestPreview: (input) => { const { owner, name } = parseRepositorySelector(input.repository); return graphqlRead({ @@ -2208,24 +2181,7 @@ export const make = Effect.gen(function* () { decode: decodePullRequestPreviewJson, }); }, - listWorkflowRunsRequiringApproval: (input) => - Effect.gen(function* () { - // Runs the checks revalidator just confirmed are this caller's alone to filter. - if ((yield* KnownWorkflowRuns) !== null) - return yield* listWorkflowRunsRequiringApproval(input); - const { key, credential } = yield* inFlightKey( - input.cwd, - input.host, - input.repository, - input.number, - input.headSha, - input.headBranch, - input.headRepositoryOwner, - ); - return yield* Cache.get(workflowRunsInFlight, key).pipe( - Effect.provideService(GitHubApi.PinnedGitHubCredential, credential), - ); - }), + listWorkflowRunsRequiringApproval, getPullRequestStack: (input) => { const { owner, name } = parseRepositorySelector(input.repository); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts index 65a17bcd2a0f..c41ab18c6aac 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.test.ts @@ -1,7 +1,5 @@ import { afterEach, assert, expect, it, vi } from "@effect/vitest"; -import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; -import * as Fiber from "effect/Fiber"; import * as Layer from "effect/Layer"; import { ChildProcessSpawner } from "effect/process"; @@ -1196,7 +1194,7 @@ layer("GitLabPullRequestCli.layer", (it) => { }), ); - it.effect("reuses a freshly read merge request when reading reviewer candidates", () => + it.effect("reads who has access to the project and who is already on the merge request", () => Effect.gen(function* () { mockedExecute .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) @@ -1204,17 +1202,11 @@ layer("GitLabPullRequestCli.layer", (it) => { Effect.succeed(output(JSON.stringify([author, reviewer, { id: 9, username: "hubot" }]))), ); const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; - const mergeRequest = yield* cli.getMergeRequestDetail({ - cwd: "/w", - repository: "acme/web", - number: 7, - }); const list = yield* cli.listReviewerCandidates({ cwd: "/w", repository: "acme/web", number: 7, - mergeRequest, }); expect(argsOfCall(1)[1]).toBe("projects/acme%2Fweb/users?per_page=100"); @@ -1224,7 +1216,6 @@ layer("GitLabPullRequestCli.layer", (it) => { ["9", false], ]); assert.isFalse(list.truncated); - expect(mockedExecute).toHaveBeenCalledTimes(2); }), ); @@ -1234,11 +1225,6 @@ layer("GitLabPullRequestCli.layer", (it) => { .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) .mockReturnValueOnce(Effect.succeed(output("{}"))); const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; - const mergeRequest = yield* cli.getMergeRequestDetail({ - cwd: "/w", - repository: "acme/web", - number: 7, - }); yield* cli.setReviewerRequest({ cwd: "/w", @@ -1246,14 +1232,12 @@ layer("GitLabPullRequestCli.layer", (it) => { number: 7, reviewers: [{ id: "9" }], requested: true, - mergeRequest, }); // GitLab replaces the whole set, so the reviewer already on the merge request has to be // sent back with the new one or the request would take them off it. expect(argsOfCall(1)).toContain("PUT"); expect(JSON.parse(callAt(1).stdin ?? "")).toEqual({ reviewer_ids: [5, 9] }); - expect(mockedExecute).toHaveBeenCalledTimes(2); }), ); @@ -1551,233 +1535,33 @@ layer("GitLabPullRequestCli.layer", (it) => { }), ); - it.effect("reauthorizes every read after logout while an older authorized read is pending", () => - Effect.gen(function* () { - const started = yield* Deferred.make(); - const release = yield* Deferred.make(); - let loggedOut = false; - let holdRead = false; - let settingsReads = 0; - mockedExecute.mockImplementation((request) => { - if (request.args[1]?.includes("?license=false")) { - settingsReads++; - return Effect.succeed(output('{"merge_method":"merge"}')); - } - if (loggedOut) { - return Effect.fail( - new GitLabCli.GitLabCliAuthenticationError({ - command: "glab", - operation: "execute", - cwd: request.cwd, - cause: new Error("logged out"), - }), - ); - } - return Effect.gen(function* () { - if (holdRead) { - yield* Deferred.succeed(started, undefined); - yield* Deferred.await(release); - } - return output(mergeRequestJson({ title: "Private merge request" })); - }); - }); - const provider = yield* GitLabPullRequestProvider.make; - assert.isDefined(provider.getChangeRequestChecks); - assert.isDefined(provider.getChangeRequestPreview); - assert.isDefined(provider.getChangeRequestSummary); - const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; - expect((yield* provider.getChangeRequest(input)).title).toBe("Private merge request"); - holdRead = true; - const older = yield* provider.getChangeRequestSummary(input).pipe(Effect.forkChild); - yield* Deferred.await(started); - loggedOut = true; - - for (const read of [ - provider.getChangeRequest(input).pipe(Effect.asVoid), - provider.getChangeRequestChecks(input).pipe(Effect.asVoid), - provider.getChangeRequestPreview(input).pipe(Effect.asVoid), - provider.getChangeRequestSummary(input).pipe(Effect.asVoid), - provider.getViewerPermissions(input).pipe(Effect.asVoid), - ]) { - expect((yield* Effect.flip(read)).reason).toBe("unauthenticated"); - } - expect(settingsReads).toBe(1); - yield* Deferred.succeed(release, undefined); - expect((yield* Fiber.join(older)).title).toBe("Private merge request"); - expect((yield* Effect.flip(provider.getChangeRequestSummary(input))).reason).toBe( - "unauthenticated", - ); - }), - ); - - it.effect("rereads permissions and checks with the current account", () => - Effect.gen(function* () { - mockedExecute - .mockReturnValueOnce( - Effect.succeed(output(mergeRequestJson({ user: { can_merge: true } }))), - ) - .mockReturnValueOnce( - Effect.succeed(output(mergeRequestJson({ user: { can_merge: false } }))), - ) - .mockReturnValueOnce( - Effect.succeed(output(mergeRequestJson({ user: { can_merge: true } }))), - ) - .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))); - const provider = yield* GitLabPullRequestProvider.make; - assert.isDefined(provider.getChangeRequestChecks); - const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; - - yield* provider.getChangeRequestChecks(input); - expect((yield* provider.getViewerPermissions(input)).actions).not.toContain("merge"); - expect((yield* provider.getViewerPermissions(input)).actions).toContain("merge"); - expect((yield* provider.getChangeRequestChecks(input)).state).toBe("open"); - expect(mockedExecute).toHaveBeenCalledTimes(4); - }), - ); - - it.effect("reads fresh reviewer candidates and preserves the current reviewers in a write", () => - Effect.gen(function* () { - mockedExecute - .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))) - .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) - .mockReturnValueOnce( - Effect.succeed(output(JSON.stringify([author, reviewer, { id: 9, username: "hubot" }]))), - ) - .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({ reviewers: [reviewer] })))) - .mockReturnValueOnce(Effect.succeed(output("{}"))) - .mockReturnValueOnce( - Effect.succeed(output(mergeRequestJson({ title: "After review request" }))), - ); - const provider = yield* GitLabPullRequestProvider.make; - assert.isDefined(provider.getChangeRequestSummary); - const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; - - yield* provider.getViewerPermissions(input); - const candidates = yield* provider.listReviewerCandidates(input); - expect( - candidates.candidates.map(({ login, isRequested }) => ({ login, isRequested })), - ).toEqual([ - { login: "octocat", isRequested: true }, - { login: "hubot", isRequested: false }, - ]); - yield* provider.setReviewerRequest({ - ...input, - reviewers: [{ id: "9", kind: "user" }], - requested: true, - }); - - expect(JSON.parse(callAt(4).stdin ?? "")).toEqual({ reviewer_ids: [5, 9] }); - expect((yield* provider.getChangeRequestSummary(input)).title).toBe("After review request"); - expect(mockedExecute).toHaveBeenCalledTimes(6); - }), - ); - - it.effect.each([false, true])( - "starts a fresh read after a write while an old read is pending (failed: %s)", - (failedWrite) => + it.effect( + "reads a project's merge settings once while it holds them, and never for a summary", + () => Effect.gen(function* () { - const started = yield* Deferred.make(); - const release = yield* Deferred.make(); - let detailReads = 0; + let settingsReads = 0; mockedExecute.mockImplementation((request) => { - if (request.args.includes("PUT")) { - return failedWrite - ? Effect.fail( - new GitLabCli.GitLabCliAuthenticationError({ - command: "glab", - operation: "execute", - cwd: request.cwd, - cause: new Error("write refused"), - }), - ) - : Effect.succeed(output("{}")); + if (!request.args[1]?.includes("?license=false")) { + return Effect.succeed(output(mergeRequestJson({}))); } - return Effect.gen(function* () { - const older = ++detailReads === 1; - if (older) { - yield* Deferred.succeed(started, undefined); - yield* Deferred.await(release); - } - return output( - mergeRequestJson({ title: older ? "Before mutation" : "After mutation" }), - ); - }); + settingsReads++; + // A failed read is not held, so the next detail asks again. + return Effect.succeed( + output(settingsReads === 1 ? "invalid json" : '{"merge_method":"merge"}'), + ); }); const provider = yield* GitLabPullRequestProvider.make; assert.isDefined(provider.getChangeRequestSummary); - assert.isDefined(provider.updateChangeRequest); const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; - const older = yield* provider.getChangeRequestSummary(input).pipe(Effect.forkChild); - yield* Deferred.await(started); - const write = provider.updateChangeRequest({ ...input, title: "After mutation" }); - if (failedWrite) { - expect((yield* Effect.flip(write)).reason).toBe("unauthenticated"); - } else { - yield* write; - } - - expect((yield* provider.getChangeRequestSummary(input)).title).toBe("After mutation"); - yield* Deferred.succeed(release, undefined); - expect((yield* Fiber.join(older)).title).toBe("Before mutation"); - expect((yield* provider.getChangeRequestSummary(input)).title).toBe("After mutation"); - expect(detailReads).toBe(3); - }), - ); - it.effect("retries failed merge request and project settings reads immediately", () => - Effect.gen(function* () { - mockedExecute - .mockReturnValueOnce(Effect.succeed(output("invalid json"))) - .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))) - .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))) - .mockReturnValueOnce(Effect.succeed(output("invalid json"))) - .mockReturnValueOnce(Effect.succeed(output(mergeRequestJson({})))) - .mockReturnValueOnce(Effect.succeed(output('{"merge_method":"merge"}'))); - const provider = yield* GitLabPullRequestProvider.make; - assert.isDefined(provider.getChangeRequestSummary); - const input = { cwd: "/w", repository: "acme/web", host: "gitlab.com", number: 7 }; - - expect((yield* Effect.flip(provider.getChangeRequestSummary(input))).reason).toBe("failed"); - expect((yield* provider.getChangeRequestSummary(input)).title).toBe("Merge request 7"); - expect((yield* Effect.flip(provider.getChangeRequest(input))).reason).toBe("failed"); - expect((yield* provider.getChangeRequest(input)).mergeCapabilities).toMatchObject({ - merge: true, - }); - expect(mockedExecute).toHaveBeenCalledTimes(6); - }), - ); - - it.effect( - "preserves newlines and percent signs in cwd for merge request and project settings reads", - () => - Effect.gen(function* () { - mockedExecute.mockImplementation((request) => - Effect.succeed( - output( - request.args[1]?.includes("?license=false") - ? '{"merge_method":"merge"}' - : mergeRequestJson({}), - ), - ), - ); - const provider = yield* GitLabPullRequestProvider.make; - const input = { - cwd: "/w/line\n100% project", - repository: "group/subgroup/web", - host: "gitlab.com", - number: 7, - }; + expect((yield* provider.getChangeRequestSummary(input)).title).toBe("Merge request 7"); + expect(settingsReads).toBe(0); + expect((yield* Effect.flip(provider.getChangeRequest(input))).reason).toBe("failed"); + yield* provider.getChangeRequest(input); + yield* provider.getChangeRequest(input); - expect((yield* provider.getChangeRequest(input)).number).toBe(7); - expect(mockedExecute).toHaveBeenCalledTimes(2); - expect(mockedExecute.mock.calls.map(([request]) => request.cwd)).toEqual([ - input.cwd, - input.cwd, - ]); - expect(mockedExecute.mock.calls.map(([request]) => request.args[1])).toEqual([ - "projects/group%2Fsubgroup%2Fweb/merge_requests/7?include_diverged_commits_count=true", - "projects/group%2Fsubgroup%2Fweb?license=false", - ]); + expect(settingsReads).toBe(2); + expect(mockedExecute).toHaveBeenCalledTimes(6); }), ); }); diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts index f8507b074e99..643d130c85bc 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestCli.ts @@ -309,8 +309,6 @@ export class GitLabPullRequestCli extends Context.Service< readonly cwd: string; readonly repository: string; readonly number: number; - /** The merge request as the caller already holds it, which spares reading it again. */ - readonly mergeRequest?: GitLabMergeRequestDetail | undefined; }) => Effect.Effect; readonly setReviewerRequest: (input: { @@ -319,8 +317,6 @@ export class GitLabPullRequestCli extends Context.Service< readonly number: number; readonly reviewers: ReadonlyArray<{ readonly id: string }>; readonly requested: boolean; - /** The merge request as the caller holds it, whose reviewer set is the one rewritten. */ - readonly mergeRequest?: GitLabMergeRequestDetail | undefined; }) => Effect.Effect; readonly runMergeRequestAction: (input: { @@ -950,17 +946,6 @@ export const make = Effect.gen(function* () { }), ); - /** The merge request a caller handed over, or a read of it where none was. */ - const heldOrRead = (input: { - readonly cwd: string; - readonly repository: string; - readonly number: number; - readonly mergeRequest?: GitLabMergeRequestDetail | undefined; - }): Effect.Effect => - input.mergeRequest === undefined - ? mergeRequestDetail(input) - : Effect.succeed(input.mergeRequest); - /** The people with access to the project, one page deep. */ const projectUsers = (input: { readonly cwd: string; @@ -1334,7 +1319,7 @@ export const make = Effect.gen(function* () { ), listReviewerCandidates: (input) => - Effect.all([heldOrRead(input), projectUsers(input)], { concurrency: 2 }).pipe( + Effect.all([mergeRequestDetail(input), projectUsers(input)], { concurrency: 2 }).pipe( Effect.map(([mergeRequest, users]) => { const author = mergeRequest.author?.login; const requested = new Set(mergeRequest.reviewRequestLogins); @@ -1352,7 +1337,7 @@ export const make = Effect.gen(function* () { ), setReviewerRequest: (input) => - heldOrRead(input).pipe( + mergeRequestDetail(input).pipe( Effect.flatMap((mergeRequest) => { // GitLab has no endpoint that adds or removes one reviewer: `reviewer_ids` replaces the // whole set, so the set that is already there is read first and the change applied to diff --git a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts index 2945223f9b38..011a332cfdde 100644 --- a/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts +++ b/packages/source-control-gitlab/src/server/GitLabPullRequestProvider.ts @@ -105,11 +105,10 @@ export const make = Effect.gen(function* () { const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; // How a project lets its merge requests land is a setting, which moves far more slowly than - // the detail every refresh was reading it alongside. It is returned only with a fresh, - // authorized merge request read; account-specific details are never held between calls. + // the detail every refresh was reading it alongside, and is only returned with that fresh read. const mergeSettings = yield* Cache.makeWith( (key: string) => { - const [cwd = "", repository = ""] = key.split("\n").map(decodeURIComponent); + const [cwd = "", repository = ""] = key.split("\0"); return cli.getProjectMergeCapabilities({ cwd, repository }); }, { capacity: 32, timeToLive: (exit) => (Exit.isSuccess(exit) ? "5 minutes" : 0) }, @@ -166,12 +165,7 @@ export const make = Effect.gen(function* () { Effect.all( [ cli.getMergeRequestDetail(input), - Cache.get( - mergeSettings, - [input.cwd, input.repository] - .map((value) => encodeURIComponent(String(value ?? ""))) - .join("\n"), - ), + Cache.get(mergeSettings, `${input.cwd}\0${input.repository}`), ], { concurrency: 2 }, ).pipe( @@ -238,8 +232,8 @@ export const make = Effect.gen(function* () { })), ), - // The same read the detail takes it from: `user.can_merge` lives on the merge request, so - // there is no cheaper thing to ask GitLab. Every call uses the account signed in now. + // The same read the detail takes it from, on its own: `user.can_merge` lives on the merge + // request, so there is no cheaper thing to ask GitLab. getViewerPermissions: (input) => cli .getMergeRequestDetail(input) @@ -262,10 +256,24 @@ export const make = Effect.gen(function* () { // Users only: GitLab requests a review of a person, and the groups that can stand in for one // appear in approval rules rather than in a merge request's reviewers. listReviewerCandidates: (input) => - cli.listReviewerCandidates(input).pipe(Effect.mapError(fail("listReviewerCandidates"))), + cli + .listReviewerCandidates({ + cwd: input.cwd, + repository: input.repository, + number: input.number, + }) + .pipe(Effect.mapError(fail("listReviewerCandidates"))), setReviewerRequest: (input) => - cli.setReviewerRequest(input).pipe(Effect.mapError(fail("setReviewerRequest"))), + cli + .setReviewerRequest({ + cwd: input.cwd, + repository: input.repository, + number: input.number, + reviewers: input.reviewers, + requested: input.requested, + }) + .pipe(Effect.mapError(fail("setReviewerRequest"))), runAction: (input) => cli