Fix commit-aware PR resolution across forges

2026-07-18 14:21:10 +02:00
parent ffe76a7e57
commit c97f823236
16 changed files with 879 additions and 122 deletions
@@ -48,7 +48,11 @@ import { startGitCommandMetrics, stopGitCommandMetrics } from "./run-git-command
 import { createForgeResolver } from "../services/forge-resolver.js";
 import { GitHubCommandError, GitHubCliMissingError } from "../services/github-service.js";
 import type { CurrentPullRequestStatus, ForgeService } from "../services/forge-service.js";
-import { TeaAuthenticationError, TeaCliMissingError } from "../services/gitea-service.js";
+import {
+  TeaAuthenticationError,
+  TeaCliMissingError,
+  TeaCommandError,
+} from "../services/gitea-service.js";
 import {
  createWorktree as createWorktreePrimitive,
  type CreateWorktreeOptions,
@@ -161,6 +165,7 @@ function createPullRequestStatus(overrides?: Partial<CurrentPullRequestStatus>)

 interface RequestedPullRequestTarget {
  headRef: string;
+  headSha?: string;
  headRepositoryOwner?: string;
 }

@@ -176,6 +181,7 @@ function createGitHubServiceRecordingPullRequestTargets(
  github.getCurrentPullRequestStatus = async (request) => {
    options.requestedTargets.push({
      headRef: request.headRef,
+      ...(request.headSha ? { headSha: request.headSha } : {}),
      ...(request.headRepositoryOwner ? { headRepositoryOwner: request.headRepositoryOwner } : {}),
    });
    return createPullRequestStatus({
@@ -2341,7 +2347,8 @@ const x = 1;

    const lookupTarget = await readPullRequestLookupTargetFromFacts(repoDir, paseoHome);

-    expect(lookupTarget).toEqual({ headRef: "refactor/workspace-scripts" });
+    expect(lookupTarget).toMatchObject({ headRef: "refactor/workspace-scripts" });
+    expect(lookupTarget?.headSha).toMatch(/^[0-9a-f]{40}$/);
  });

  it("keeps the local branch lookup when origin tracking uses the same head name", async () => {
@@ -2356,7 +2363,8 @@ const x = 1;

    const lookupTarget = await readPullRequestLookupTargetFromFacts(repoDir, paseoHome);

-    expect(lookupTarget).toEqual({ headRef: "feature" });
+    expect(lookupTarget).toMatchObject({ headRef: "feature" });
+    expect(lookupTarget?.headSha).toMatch(/^[0-9a-f]{40}$/);
  });

  it("does not attach an owner when the tracked remote is the same GitHub repository", async () => {
@@ -2378,7 +2386,8 @@ const x = 1;

    const lookupTarget = await readPullRequestLookupTargetFromFacts(repoDir, paseoHome);

-    expect(lookupTarget).toEqual({ headRef: "refactor/workspace-scripts" });
+    expect(lookupTarget).toMatchObject({ headRef: "refactor/workspace-scripts" });
+    expect(lookupTarget?.headSha).toMatch(/^[0-9a-f]{40}$/);
  });

  it("keeps the fork owner when same-repo comparison is indeterminate", async () => {
@@ -2396,7 +2405,8 @@ const x = 1;

    const lookupTarget = await readPullRequestLookupTargetFromFacts(repoDir, paseoHome);

-    expect(lookupTarget).toEqual({ headRef: "main", headRepositoryOwner: "chethanuk" });
+    expect(lookupTarget).toMatchObject({ headRef: "main", headRepositoryOwner: "chethanuk" });
+    expect(lookupTarget?.headSha).toMatch(/^[0-9a-f]{40}$/);
  });

  it("uses the configured push remote for fork PR lookup when upstream is absent", async () => {
@@ -2425,8 +2435,11 @@ const x = 1;
    );

    expect(getBranchUpstream(repoDir)).toBeNull();
-    expect(factsTarget).toEqual({ headRef: "main", headRepositoryOwner: "chethanuk" });
-    expect(requestedTargets).toEqual([{ headRef: "main", headRepositoryOwner: "chethanuk" }]);
+    expect(factsTarget).toMatchObject({ headRef: "main", headRepositoryOwner: "chethanuk" });
+    expect(factsTarget?.headSha).toMatch(/^[0-9a-f]{40}$/);
+    expect(requestedTargets).toEqual([
+      expect.objectContaining({ headRef: "main", headRepositoryOwner: "chethanuk" }),
+    ]);
  });

  it("keeps the local branch lookup when same-repo tracking points at the base branch", async () => {
@@ -2441,7 +2454,8 @@ const x = 1;

    const lookupTarget = await readPullRequestLookupTargetFromFacts(repoDir, paseoHome);

-    expect(lookupTarget).toEqual({ headRef: "tender-parrot" });
+    expect(lookupTarget).toMatchObject({ headRef: "tender-parrot" });
+    expect(lookupTarget?.headSha).toMatch(/^[0-9a-f]{40}$/);
  });

  it("derives the same origin tracked head for on-demand PR status reads", async () => {
@@ -2495,7 +2509,9 @@ const x = 1;

    const status = await getPullRequestStatus(repoDir, github);

-    expect(requestedTargets).toEqual([{ headRef: "main", headRepositoryOwner: "chethanuk" }]);
+    expect(requestedTargets).toEqual([
+      expect.objectContaining({ headRef: "main", headRepositoryOwner: "chethanuk" }),
+    ]);
    expect(status.status?.number).toBe(345);
    expect(status.status?.headRefName).toBe("main");
  });
@@ -2544,6 +2560,33 @@ const x = 1;
    expect(callCount).toBe(1);
  });

+  it("does not reuse a PR status cache entry after HEAD changes on the same branch", async () => {
+    execFileSync("git", ["checkout", "-b", "feature"], { cwd: repoDir });
+    execFileSync("git", ["remote", "add", "origin", "https://github.com/getpaseo/paseo.git"], {
+      cwd: repoDir,
+    });
+
+    const requestedShas: string[] = [];
+    const github = createGitHubServiceForStatus(null);
+    github.getCurrentPullRequestStatus = async (options) => {
+      if (options.headSha) requestedShas.push(options.headSha);
+      return createPullRequestStatus({
+        url: `https://github.com/getpaseo/paseo/pull/${requestedShas.length}`,
+      });
+    };
+
+    const first = await getPullRequestStatus(repoDir, github);
+    writeFileSync(join(repoDir, "next.txt"), "next\n");
+    execFileSync("git", ["add", "next.txt"], { cwd: repoDir });
+    execFileSync("git", ["commit", "-m", "next commit"], { cwd: repoDir });
+    const second = await getPullRequestStatus(repoDir, github);
+
+    expect(first.status?.url).toContain("/pull/1");
+    expect(second.status?.url).toContain("/pull/2");
+    expect(requestedShas).toHaveLength(2);
+    expect(requestedShas[0]).not.toBe(requestedShas[1]);
+  });
+
  it("passes forced PR status reads through to the GitHub service", async () => {
    execFileSync("git", ["checkout", "-b", "feature"], { cwd: repoDir });
    execFileSync("git", ["remote", "add", "origin", "https://github.com/getpaseo/paseo.git"], {
@@ -2637,6 +2680,41 @@ const x = 1;
    }
  });

+  it("keeps stale PR status when a Gitea-family refresh hits a transient command error", async () => {
+    execFileSync("git", ["checkout", "-b", "feature"], { cwd: repoDir });
+    execFileSync("git", ["remote", "add", "origin", "https://gitea.example.com/acme/repo.git"], {
+      cwd: repoDir,
+    });
+
+    __setPullRequestStatusCacheTtlForTests(50);
+    try {
+      let callCount = 0;
+      const service = createGitHubServiceForStatus(null);
+      service.getCurrentPullRequestStatus = async () => {
+        callCount += 1;
+        if (callCount === 1) {
+          return createPullRequestStatus({ url: "https://gitea.example.com/acme/repo/pulls/7" });
+        }
+        throw new TeaCommandError({
+          args: ["pr", "list"],
+          cwd: repoDir,
+          exitCode: 1,
+          stderr: "request timed out",
+        });
+      };
+
+      const fresh = await getPullRequestStatus(repoDir, service);
+      await sleep(80);
+      const stale = await getPullRequestStatus(repoDir, service);
+
+      expect(stale).toEqual(fresh);
+      expect(stale.status?.url).toContain("/pulls/7");
+      expect(callCount).toBe(2);
+    } finally {
+      __resetPullRequestStatusCacheForTests();
+    }
+  });
+
  it("does not use stale PR status fallback for forced GitHub errors", async () => {
    execFileSync("git", ["checkout", "-b", "feature"], { cwd: repoDir });
    execFileSync("git", ["remote", "add", "origin", "https://github.com/getpaseo/paseo.git"], {
@@ -7,7 +7,7 @@ import type { Logger } from "pino";
 import type { ParsedDiffFile } from "../server/utils/diff-highlighter.js";
 import { parseAndHighlightDiff } from "../server/utils/diff-highlighter.js";
 import { parseGitHubRepoFromRemote } from "../server/workspace-git-metadata.js";
-import { GitHubCommandError, createGitHubService } from "../services/github-service.js";
+import { createGitHubService } from "../services/github-service.js";
 import type {
  CurrentPullRequestStatus,
  ForgeAuthState,
@@ -15,7 +15,11 @@ import type {
  ForgeSpecificStatusFacts,
  PullRequestMergeable,
 } from "../services/forge-service.js";
-import { ForgeAuthenticationError, ForgeCliMissingError } from "../services/forge-cli-command.js";
+import {
+  ForgeAuthenticationError,
+  ForgeCliMissingError,
+  ForgeCommandError,
+} from "../services/forge-cli-command.js";
 import { parseGitRevParsePath, resolveGitRevParsePath } from "./git-rev-parse-path.js";
 import { runGitCommand } from "./run-git-command.js";
 import { isPaseoOwnedWorktreeCwd, resolvePaseoWorktreesBaseRoot } from "./worktree.js";
@@ -67,6 +71,7 @@ interface CheckoutReadCacheOptions {

 interface PullRequestStatusLookupTarget {
  headRef: string;
+  headSha?: string;
  headRepositoryOwner?: string;
 }

@@ -116,8 +121,8 @@ function createShortstatCache(ttlMs: number) {
  });
 }

-function getPullRequestStatusCacheKey(cwd: string): string {
-  return resolve(cwd);
+function getPullRequestStatusCacheKey(cwd: string, headSha: string | null): string {
+  return `${resolve(cwd)}\u0000${headSha ?? ""}`;
 }

 function rememberPullRequestStatus(cacheKey: string, status: PullRequestStatusResult): void {
@@ -829,6 +834,38 @@ export async function getCurrentBranch(cwd: string): Promise<string | null> {
  }
 }

+async function getCurrentHeadSha(cwd: string, context?: CheckoutContext): Promise<string | null> {
+  const knownSha = context?.facts?.isGit
+    ? context.facts.pullRequestLookupTarget?.headSha
+    : undefined;
+  if (knownSha) {
+    return knownSha;
+  }
+  try {
+    const { stdout } = await runGitCommand(["rev-parse", "HEAD"], {
+      cwd,
+      envOverlay: READ_ONLY_GIT_ENV,
+      logger: context?.logger,
+    });
+    const sha = stdout.trim();
+    return sha.length > 0 ? sha : null;
+  } catch {
+    return null;
+  }
+}
+
+async function addHeadShaToPullRequestLookupTarget(
+  cwd: string,
+  target: PullRequestStatusLookupTarget | null,
+  context?: CheckoutContext,
+): Promise<PullRequestStatusLookupTarget | null> {
+  if (!target) {
+    return null;
+  }
+  const headSha = await getCurrentHeadSha(cwd, context);
+  return headSha ? { ...target, headSha } : target;
+}
+
 async function getRebaseHeadBranch(cwd: string): Promise<string | null> {
  const paths = ["rebase-merge/head-name", "rebase-apply/head-name"];
  const results = await Promise.all(
@@ -1761,6 +1798,11 @@ export async function getCheckoutSnapshotFacts(
        context,
      )) ?? pullRequestLookupTarget;
  }
+  pullRequestLookupTarget = await addHeadShaToPullRequestLookupTarget(
+    cwd,
+    pullRequestLookupTarget,
+    context,
+  );

  return {
    isGit: true,
@@ -3402,7 +3444,8 @@ export async function getPullRequestStatus(
  options?: CheckoutReadCacheOptions,
  context?: CheckoutContext,
 ): Promise<PullRequestStatusResult> {
-  const cacheKey = getPullRequestStatusCacheKey(cwd);
+  const headSha = await getCurrentHeadSha(cwd, context);
+  const cacheKey = getPullRequestStatusCacheKey(cwd, headSha);
  if (!options?.force) {
    const cached = pullRequestStatusCache.get(cacheKey);
    if (cached) {
@@ -3415,14 +3458,14 @@ export async function getPullRequestStatus(
    }
  }

-  const lookup = getPullRequestStatusUncached(cwd, forgeService, options, context)
+  const lookup = getPullRequestStatusUncached(cwd, forgeService, options, context, headSha)
    .then((status) => {
      pullRequestStatusCache.set(cacheKey, status);
      rememberPullRequestStatus(cacheKey, status);
      return status;
    })
    .catch((error) => {
-      if (!options?.force && error instanceof GitHubCommandError) {
+      if (!options?.force && error instanceof ForgeCommandError) {
        const stale = lastSuccessfulPullRequestStatus.get(cacheKey);
        if (stale) {
          return stale;
@@ -3443,6 +3486,7 @@ async function getPullRequestStatusUncached(
  forgeService: ForgeService,
  options?: CheckoutReadCacheOptions,
  context?: CheckoutContext,
+  headSha?: string | null,
 ): Promise<PullRequestStatusResult> {
  if (context?.facts?.isGit === false) {
    return buildPullRequestStatusResult(null, "no_remote");
@@ -3455,7 +3499,11 @@ async function getPullRequestStatusUncached(
    return buildPullRequestStatusResult(null, "no_remote");
  }
  try {
-    const lookupTarget = await resolvePullRequestStatusLookupTarget(cwd, head, context);
+    const resolvedLookupTarget = await resolvePullRequestStatusLookupTarget(cwd, head, context);
+    const lookupTarget =
+      headSha && !resolvedLookupTarget.headSha
+        ? { ...resolvedLookupTarget, headSha }
+        : resolvedLookupTarget;
    let status: CurrentPullRequestStatus | null;
    if (options?.force) {
      const reason = options.reason;