fix(server): review guides in a fork checkout cover only the pull request's change

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";
@@ -88,7 +89,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 +148,7 @@ const stack = (repositories: {
 
 const makeLayer = (input: {
   readonly root: string;
+  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,102 @@ it.layer(layerBase, { excludeTestServices: true })("PullRequestReviewGuideServic
     ),
   );
 
+  it.effect("measures only the PR change using the renamed verified remote's fetch URL", () =>
+    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", "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<string>();
+        const driver = yield* GitVcsDriver.GitVcsDriver;
+        const stored = yield* Deferred.make<void>();
+        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);
+        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<Fiber.Fiber<unknown, unknown>>();
+        const stored = yield* Deferred.make<void>();
+        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))),
+                  ),
+              }),
+            ),
+          ),
+        );
+        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",
+        );
+        assert.deepStrictEqual(result.guide, before.guide);
+        assert.isFalse(yield* Deferred.isDone(stored));
+        assert.isFalse(yield* Ref.get(generated));
+      }),
+    ),
+  );
+
   it.effect("refuses to write a guide while the server has review guides off", () =>
     Effect.scoped(
       Effect.gen(function* () {

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,12 +154,40 @@ 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);
+    // Identity names can be stale; match effective fetch URLs, including insteadOf rewrites.
+    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: ["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<string>) =>
       git.execute({
         operation: "PullRequestReviewGuideService.fetch",
@@ -327,6 +361,7 @@ const make = Effect.gen(function* () {
     input: PullRequestRef,
     key: ThreadPullRequestKey,
     workspaceRoot: string,
+    locator: RepositoryIdentity["locator"],
     detail: PullRequestDetail,
     headSha: string,
     runId: string,
@@ -349,6 +384,7 @@ const make = Effect.gen(function* () {
     );
     const { generated, change } = yield* checkoutHead(
       workspaceRoot,
+      locator,
       detail,
       headSha,
       stack,
@@ -412,7 +448,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 +501,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)