From 2022287ea9e577fb4210b608e6f012add3c12273 Mon Sep 17 00:00:00 2001 From: tris203 Date: Sun, 11 Oct 2026 19:59:00 +0100 Subject: [PATCH 1/2] fix(server): review guides in a fork checkout cover only the pull request's change In a checkout with both `origin` (a fork) and `upstream`, the project's repository identity is read from `upstream`, but the guide fetched the base branch from the checkout's primary remote, `origin`. When the fork's base branch was behind upstream's, the guide was measured from that stale tip and covered every upstream commit since, not the pull request's own change. The guide now fetches from the remote whose URL is the one the identity was read from, looked up among the checkout's current remotes so a cached remote name that has since been renamed still resolves. --- .../PullRequestReviewGuideService.test.ts | 60 ++++++++++++++++++- .../PullRequestReviewGuideService.ts | 51 ++++++++++++++-- 2 files changed, 104 insertions(+), 7 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts b/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts index 403424cdc5970..3085e6b71e0cc 100644 --- a/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts +++ b/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts @@ -88,7 +88,7 @@ const makeRepositories = Effect.gen(function* () { yield* git(remote, ["checkout", "--quiet", "main"]); yield* commit("unrelated.txt", "main moved on"); yield* git(root, ["clone", "--quiet", remote, workspace]); - return { root, workspace, headSha, lowerSha, upperSha }; + return { root, remote, workspace, headSha, lowerSha, upperSha }; }); const detail = (workspaceRoot: string, headSha: string): PullRequestDetail => @@ -147,6 +147,8 @@ const stack = (repositories: { const makeLayer = (input: { readonly root: string; + /** The repository the workspace was cloned from, which the project's identity points at. */ + readonly remote: string; readonly workspace: string; readonly headSha: string; readonly lowerSha: string; @@ -191,7 +193,10 @@ const makeLayer = (input: { Effect.succeedSome({ id: projectId, workspaceRoot: input.workspace, - repositoryIdentity: input.identity ?? ACME_APP, + repositoryIdentity: { + ...(input.identity ?? ACME_APP), + locator: { ...ACME_APP.locator, remoteUrl: input.remote }, + }, }), } as unknown as ProjectService.ProjectService["Service"]), ), @@ -655,6 +660,57 @@ it.layer(layerBase, { excludeTestServices: true })("PullRequestReviewGuideServic ), ); + it.effect("measures a fork checkout's change from the upstream base, not the fork's", () => + Effect.scoped( + Effect.gen(function* () { + const repositories = yield* makeRepositories; + // The checkout becomes a fork's: `origin` is a copy whose base branch never took the + // commit the change is built on, and the pull request's repository is `upstream`. The + // project's identity still names it `origin`, as a cached one does after the rename. + const fork = `${repositories.root}/fork`; + yield* git(repositories.root, ["clone", "--quiet", "--bare", repositories.remote, fork]); + yield* git(fork, ["update-ref", "refs/heads/stack-base", "refs/heads/main~1"]); + yield* git(repositories.workspace, ["remote", "rename", "origin", "upstream"]); + yield* git(repositories.workspace, ["remote", "add", "origin", fork]); + const changed = yield* Deferred.make(); + const driver = yield* GitVcsDriver.GitVcsDriver; + const stored = yield* Deferred.make(); + const context = yield* Layer.build( + makeLayer({ + ...repositories, + stored, + generate: (input) => + git(input.cwd, ["diff", "--name-only", `${input.baseCommit}...HEAD`]).pipe( + Effect.provideService(GitVcsDriver.GitVcsDriver, driver), + Effect.orDie, + Effect.flatMap((names) => Deferred.succeed(changed, names)), + Effect.as({ stops: [] }), + ), + }), + ); + const service = Context.get( + context, + PullRequestReviewGuideService.PullRequestReviewGuideService, + ); + yield* service.generate(reference); + const failedRun = service.get(reference).pipe( + Effect.flatMap((result) => + result.run?._tag === "Failed" + ? Effect.die(`run failed: ${result.run.detail}`) + : Effect.fail("still running" as const), + ), + Effect.retry(Schedule.spaced("20 millis")), + ); + // Measured from the fork's stale base this would also name lower.txt. + assert.strictEqual( + yield* Effect.raceFirst(Deferred.await(changed), failedRun), + "feature.txt", + ); + yield* Effect.raceFirst(Deferred.await(stored), failedRun); + }), + ), + ); + it.effect("refuses to write a guide while the server has review guides off", () => Effect.scoped( Effect.gen(function* () { diff --git a/apps/server/src/pullRequest/PullRequestReviewGuideService.ts b/apps/server/src/pullRequest/PullRequestReviewGuideService.ts index de3ee38c7aa2a..f6bf6cf8a1d27 100644 --- a/apps/server/src/pullRequest/PullRequestReviewGuideService.ts +++ b/apps/server/src/pullRequest/PullRequestReviewGuideService.ts @@ -108,13 +108,19 @@ const make = Effect.gen(function* () { }, identity, ); - if (own === null || own.host !== key.host || own.repository !== key.repository) { + if ( + identity == null || + own === null || + own.host !== key.host || + own.repository !== key.repository + ) { return yield* new PullRequestOperationError({ operation: "generateReviewGuide", detail: "Guides are written from this project's checkout, and this pull request is in another repository. Write it from a project that is a checkout of that repository.", }); } + return identity.locator; }, ); @@ -148,17 +154,50 @@ const make = Effect.gen(function* () { const checkoutHead = Effect.fn("PullRequestReviewGuideService.checkoutHead")(function* ( workspaceRoot: string, + locator: RepositoryIdentity["locator"], detail: PullRequestDetail, headSha: string, stack: PullRequestStack | null, runId: string, ) { - const remote = yield* git.resolvePrimaryRemoteName(workspaceRoot); + // The remote for the pull request's own repository, not the checkout's primary one: in a + // fork's checkout that is `upstream`, and the fork's `origin` may have a base branch far + // behind the one the pull request targets. Found by URL among the remotes as they are now, + // since the identity is cached and its remote may have been renamed since. `ls-remote + // --get-url` reports the URL git would fetch from, as the identity's was read, and unlike + // `remote get-url` it also answers for a remote configured outside the repository. + const names = yield* git + .execute({ + operation: "PullRequestReviewGuideService.listRemotes", + cwd: workspaceRoot, + args: ["remote"], + }) + .pipe(Effect.map((result) => result.stdout.split("\n").filter((name) => name.length > 0))); + const remote = yield* Effect.findFirst( + [locator.remoteName, ...names.filter((name) => name !== locator.remoteName)], + (name) => + git + .execute({ + operation: "PullRequestReviewGuideService.remoteUrl", + cwd: workspaceRoot, + args: ["ls-remote", "--get-url", "--", name], + }) + .pipe( + Effect.map((result) => result.stdout.trim() === locator.remoteUrl), + Effect.orElseSucceed(() => false), + ), + ).pipe(Effect.map(Option.getOrUndefined)); + if (remote === undefined) { + return yield* new PullRequestOperationError({ + operation: "generateReviewGuide", + detail: "This project's checkout no longer has a remote for the pull request's repository.", + }); + } const fetch = (args: ReadonlyArray) => git.execute({ operation: "PullRequestReviewGuideService.fetch", cwd: workspaceRoot, - args: ["fetch", "--quiet", "--no-tags", remote, ...args], + args: ["fetch", "--quiet", "--no-tags", "--", remote, ...args], timeoutMs: 5 * 60_000, }); // The host names the base branch. A name git rejects, or one that would change the refspec @@ -327,6 +366,7 @@ const make = Effect.gen(function* () { input: PullRequestRef, key: ThreadPullRequestKey, workspaceRoot: string, + locator: RepositoryIdentity["locator"], detail: PullRequestDetail, headSha: string, runId: string, @@ -349,6 +389,7 @@ const make = Effect.gen(function* () { ); const { generated, change } = yield* checkoutHead( workspaceRoot, + locator, detail, headSha, stack, @@ -412,7 +453,7 @@ const make = Effect.gen(function* () { }); } const { key, workspaceRoot, identity } = yield* keyFor(input); - yield* requireOwnRepository(key, identity); + const locator = yield* requireOwnRepository(key, identity); const runKey = keyString(key); yield* pullRequests.invalidate({ reference: input }); const [detail, stored] = yield* Effect.all([pullRequests.detail(input), readStored(key)], { @@ -465,7 +506,7 @@ const make = Effect.gen(function* () { return updated; }); const runId = `${runCounter++}`; - yield* write(input, key, workspaceRoot, detail, currentHeadSha, runId, owns).pipe( + yield* write(input, key, workspaceRoot, locator, detail, currentHeadSha, runId, owns).pipe( Effect.andThen(finish(null)), Effect.catchCause((cause) => Cause.hasInterruptsOnly(cause) From cbd34583df68fb7f2ff9933ccd921a4005cb8155 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 19 Oct 2026 05:27:17 +0000 Subject: [PATCH 2/2] fix(server): use the verified remote for review guides --- .../PullRequestReviewGuideService.test.ts | 81 ++++++++++++++----- .../PullRequestReviewGuideService.ts | 9 +-- 2 files changed, 65 insertions(+), 25 deletions(-) diff --git a/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts b/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts index 3085e6b71e0cc..66efa337ecd19 100644 --- a/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts +++ b/apps/server/src/pullRequest/PullRequestReviewGuideService.test.ts @@ -3,6 +3,7 @@ import { assert, it } from "@effect/vitest"; import * as Context from "effect/Context"; import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; +import * as Fiber from "effect/Fiber"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Ref from "effect/Ref"; @@ -147,7 +148,6 @@ const stack = (repositories: { const makeLayer = (input: { readonly root: string; - /** The repository the workspace was cloned from, which the project's identity points at. */ readonly remote: string; readonly workspace: string; readonly headSha: string; @@ -660,18 +660,22 @@ it.layer(layerBase, { excludeTestServices: true })("PullRequestReviewGuideServic ), ); - it.effect("measures a fork checkout's change from the upstream base, not the fork's", () => + it.effect("measures only the PR change using the renamed verified remote's fetch URL", () => Effect.scoped( Effect.gen(function* () { const repositories = yield* makeRepositories; - // The checkout becomes a fork's: `origin` is a copy whose base branch never took the - // commit the change is built on, and the pull request's repository is `upstream`. The - // project's identity still names it `origin`, as a cached one does after the rename. const fork = `${repositories.root}/fork`; - yield* git(repositories.root, ["clone", "--quiet", "--bare", repositories.remote, fork]); - yield* git(fork, ["update-ref", "refs/heads/stack-base", "refs/heads/main~1"]); + yield* git(repositories.root, ["clone", "--quiet", repositories.remote, fork]); + yield* git(fork, ["branch", "stack-base", "main~1"]); + // The cached identity still names origin, which now belongs to the stale fork. yield* git(repositories.workspace, ["remote", "rename", "origin", "upstream"]); yield* git(repositories.workspace, ["remote", "add", "origin", fork]); + yield* git(repositories.workspace, [ + "config", + `url.${repositories.remote}.insteadOf`, + "review-upstream:", + ]); + yield* git(repositories.workspace, ["remote", "set-url", "upstream", "review-upstream:"]); const changed = yield* Deferred.make(); const driver = yield* GitVcsDriver.GitVcsDriver; const stored = yield* Deferred.make(); @@ -693,20 +697,61 @@ it.layer(layerBase, { excludeTestServices: true })("PullRequestReviewGuideServic PullRequestReviewGuideService.PullRequestReviewGuideService, ); yield* service.generate(reference); - const failedRun = service.get(reference).pipe( - Effect.flatMap((result) => - result.run?._tag === "Failed" - ? Effect.die(`run failed: ${result.run.detail}`) - : Effect.fail("still running" as const), + assert.strictEqual(yield* Deferred.await(changed), "feature.txt"); + yield* Deferred.await(stored); + const result = yield* service.get(reference); + assert.strictEqual(result.guide?.headSha, repositories.headSha); + assert.deepStrictEqual(result.guide?.stops[0]?.files, [{ path: "feature.txt" }]); + }), + ), + ); + + it.effect("refuses generation when the verified remote disappeared despite a usable origin", () => + Effect.scoped( + Effect.gen(function* () { + const repositories = yield* makeRepositories; + const fork = `${repositories.root}/fork`; + yield* git(repositories.root, ["clone", "--quiet", repositories.remote, fork]); + yield* git(fork, ["branch", "stack-base", repositories.lowerSha]); + yield* git(repositories.workspace, ["remote", "set-url", "origin", fork]); + const started = yield* Deferred.make>(); + const stored = yield* Deferred.make(); + const generated = yield* Ref.make(false); + const driver = yield* GitVcsDriver.GitVcsDriver; + const context = yield* Layer.build( + makeLayer({ + ...repositories, + stored, + generate: () => Ref.set(generated, true).pipe(Effect.as({ stops: [] })), + }).pipe( + Layer.provide( + Layer.succeed(GitVcsDriver.GitVcsDriver, { + ...driver, + execute: (call) => + Effect.withFiber((fiber) => + Deferred.succeed(started, fiber).pipe(Effect.andThen(driver.execute(call))), + ), + }), + ), ), - Effect.retry(Schedule.spaced("20 millis")), ); - // Measured from the fork's stale base this would also name lower.txt. - assert.strictEqual( - yield* Effect.raceFirst(Deferred.await(changed), failedRun), - "feature.txt", + const service = Context.get( + context, + PullRequestReviewGuideService.PullRequestReviewGuideService, + ); + const before = yield* service.get(reference); + yield* service.generate(reference); + // Join the background run so its failure is visible without polling service state. + yield* Fiber.await(yield* Deferred.await(started)); + const result = yield* service.get(reference); + assert.strictEqual(result.run?._tag, "Failed"); + assert.include( + result.run?._tag === "Failed" ? result.run.detail : "", + "no longer has a remote for the pull request's repository", ); - yield* Effect.raceFirst(Deferred.await(stored), failedRun); + assert.deepStrictEqual(result.guide, before.guide); + assert.isFalse(yield* Deferred.isDone(stored)); + assert.isFalse(yield* Ref.get(generated)); }), ), ); diff --git a/apps/server/src/pullRequest/PullRequestReviewGuideService.ts b/apps/server/src/pullRequest/PullRequestReviewGuideService.ts index f6bf6cf8a1d27..edd5dd2c72865 100644 --- a/apps/server/src/pullRequest/PullRequestReviewGuideService.ts +++ b/apps/server/src/pullRequest/PullRequestReviewGuideService.ts @@ -160,12 +160,7 @@ const make = Effect.gen(function* () { stack: PullRequestStack | null, runId: string, ) { - // The remote for the pull request's own repository, not the checkout's primary one: in a - // fork's checkout that is `upstream`, and the fork's `origin` may have a base branch far - // behind the one the pull request targets. Found by URL among the remotes as they are now, - // since the identity is cached and its remote may have been renamed since. `ls-remote - // --get-url` reports the URL git would fetch from, as the identity's was read, and unlike - // `remote get-url` it also answers for a remote configured outside the repository. + // Identity names can be stale; match effective fetch URLs, including insteadOf rewrites. const names = yield* git .execute({ operation: "PullRequestReviewGuideService.listRemotes", @@ -180,7 +175,7 @@ const make = Effect.gen(function* () { .execute({ operation: "PullRequestReviewGuideService.remoteUrl", cwd: workspaceRoot, - args: ["ls-remote", "--get-url", "--", name], + args: ["remote", "get-url", "--", name], }) .pipe( Effect.map((result) => result.stdout.trim() === locator.remoteUrl),