fix: stop retrying deterministic ref updates (#1551)

2026-08-22 03:18:54 +08:00 · 2026-08-07 07:58:19 -07:00
parent 5dd098c551
commit 7764306e92
3 changed files with 195 additions and 104 deletions
@@ -8,8 +8,8 @@ import { resolve } from "path";
 import { constants } from "fs";
 import fetch from "node-fetch";
 import { GITHUB_API_URL } from "../github/api/config";
-import { retryWithBackoff } from "../utils/retry";
 import { validatePathWithinRepo } from "./path-validation";
+import { updateGitReference } from "./update-git-reference";

 type GitHubRef = {
  object: {
@@ -365,57 +365,13 @@ server.tool(
      const newCommitData = (await newCommitResponse.json()) as GitHubNewCommit;

      // 6. Update the reference to point to the new commit
-      const updateRefUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/refs/heads/${branch}`;
-
-      // We're seeing intermittent 403 "Resource not accessible by integration" errors
-      // on certain repos when updating git references. These appear to be transient
-      // GitHub API issues that succeed on retry.
-      await retryWithBackoff(
-        async () => {
-          const updateRefResponse = await fetch(updateRefUrl, {
-            method: "PATCH",
-            headers: {
-              Accept: "application/vnd.github+json",
-              Authorization: `Bearer ${githubToken}`,
-              "X-GitHub-Api-Version": "2022-11-28",
-              "Content-Type": "application/json",
-            },
-            body: JSON.stringify({
-              sha: newCommitData.sha,
-              force: false,
-            }),
-          });
-
-          if (!updateRefResponse.ok) {
-            const errorText = await updateRefResponse.text();
-
-            // Provide a more helpful error message for 403 permission errors
-            if (updateRefResponse.status === 403) {
-              const permissionError = new Error(
-                `Permission denied: Unable to push commits to branch '${branch}'. ` +
-                  `Please rebase your branch from the main/master branch to allow Claude to commit.\n\n` +
-                  `Original error: ${errorText}`,
-              );
-              throw permissionError;
-            }
-
-            // For other errors, use the original message
-            const error = new Error(
-              `Failed to update reference: ${updateRefResponse.status} - ${errorText}`,
-            );
-
-            // For non-403 errors, fail immediately without retry
-            console.error("Non-retryable error:", updateRefResponse.status);
-            throw error;
-          }
-        },
-        {
-          maxAttempts: 3,
-          initialDelayMs: 1000, // Start with 1 second delay
-          maxDelayMs: 5000, // Max 5 seconds delay
-          backoffFactor: 2, // Double the delay each time
-        },
-      );
+      await updateGitReference({
+        owner,
+        repo,
+        branch,
+        sha: newCommitData.sha,
+        githubToken,
+      });

      const simplifiedResult = {
        commit: {
@@ -580,58 +536,13 @@ server.tool(
      const newCommitData = (await newCommitResponse.json()) as GitHubNewCommit;

      // 6. Update the reference to point to the new commit
-      const updateRefUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/refs/heads/${branch}`;
-
-      // We're seeing intermittent 403 "Resource not accessible by integration" errors
-      // on certain repos when updating git references. These appear to be transient
-      // GitHub API issues that succeed on retry.
-      await retryWithBackoff(
-        async () => {
-          const updateRefResponse = await fetch(updateRefUrl, {
-            method: "PATCH",
-            headers: {
-              Accept: "application/vnd.github+json",
-              Authorization: `Bearer ${githubToken}`,
-              "X-GitHub-Api-Version": "2022-11-28",
-              "Content-Type": "application/json",
-            },
-            body: JSON.stringify({
-              sha: newCommitData.sha,
-              force: false,
-            }),
-          });
-
-          if (!updateRefResponse.ok) {
-            const errorText = await updateRefResponse.text();
-
-            // Provide a more helpful error message for 403 permission errors
-            if (updateRefResponse.status === 403) {
-              console.log("Received 403 error, will retry...");
-              const permissionError = new Error(
-                `Permission denied: Unable to push commits to branch '${branch}'. ` +
-                  `Please rebase your branch from the main/master branch to allow Claude to commit.\n\n` +
-                  `Original error: ${errorText}`,
-              );
-              throw permissionError;
-            }
-
-            // For other errors, use the original message
-            const error = new Error(
-              `Failed to update reference: ${updateRefResponse.status} - ${errorText}`,
-            );
-
-            // For non-403 errors, fail immediately without retry
-            console.error("Non-retryable error:", updateRefResponse.status);
-            throw error;
-          }
-        },
-        {
-          maxAttempts: 3,
-          initialDelayMs: 1000, // Start with 1 second delay
-          maxDelayMs: 5000, // Max 5 seconds delay
-          backoffFactor: 2, // Double the delay each time
-        },
-      );
+      await updateGitReference({
+        owner,
+        repo,
+        branch,
+        sha: newCommitData.sha,
+        githubToken,
+      });

      const simplifiedResult = {
        commit: {
@@ -0,0 +1,90 @@
+import fetch, { type RequestInit, type Response } from "node-fetch";
+import { GITHUB_API_URL } from "../github/api/config";
+import { retryWithBackoff, type RetryOptions } from "../utils/retry";
+
+type GitHubFetch = (
+  url: string,
+  init: RequestInit,
+) => Promise<Pick<Response, "ok" | "status" | "text">>;
+
+type UpdateGitReferenceOptions = {
+  owner: string;
+  repo: string;
+  branch: string;
+  sha: string;
+  githubToken: string;
+  fetchFn?: GitHubFetch;
+  retryOptions?: Omit<RetryOptions, "shouldRetry">;
+};
+
+class GitReferenceUpdateError extends Error {
+  constructor(
+    readonly status: number,
+    message: string,
+  ) {
+    super(message);
+    this.name = "GitReferenceUpdateError";
+  }
+}
+
+function shouldRetryGitReferenceUpdate(error: Error): boolean {
+  if (!(error instanceof GitReferenceUpdateError)) {
+    return true;
+  }
+
+  return error.status === 403 || error.status === 429 || error.status >= 500;
+}
+
+export async function updateGitReference({
+  owner,
+  repo,
+  branch,
+  sha,
+  githubToken,
+  fetchFn = fetch,
+  retryOptions,
+}: UpdateGitReferenceOptions): Promise<void> {
+  const updateRefUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/refs/heads/${branch}`;
+
+  await retryWithBackoff(
+    async () => {
+      const response = await fetchFn(updateRefUrl, {
+        method: "PATCH",
+        headers: {
+          Accept: "application/vnd.github+json",
+          Authorization: `Bearer ${githubToken}`,
+          "X-GitHub-Api-Version": "2022-11-28",
+          "Content-Type": "application/json",
+        },
+        body: JSON.stringify({ sha, force: false }),
+      });
+
+      if (response.ok) {
+        return;
+      }
+
+      const errorText = await response.text();
+      if (response.status === 403) {
+        throw new GitReferenceUpdateError(
+          response.status,
+          `Permission denied: Unable to push commits to branch '${branch}'. ` +
+            `Please rebase your branch from the main/master branch to allow Claude to commit.\n\n` +
+            `Original error: ${errorText}`,
+        );
+      }
+
+      throw new GitReferenceUpdateError(
+        response.status,
+        `Failed to update reference: ${response.status} - ${errorText}`,
+      );
+    },
+    {
+      maxAttempts: 3,
+      initialDelayMs: 1000,
+      maxDelayMs: 5000,
+      backoffFactor: 2,
+      ...retryOptions,
+      shouldRetry: shouldRetryGitReferenceUpdate,
+    },
+  );
+}
@@ -0,0 +1,90 @@
+import { describe, expect, it } from "bun:test";
+import { GITHUB_API_URL } from "../src/github/api/config";
+import { updateGitReference } from "../src/mcp/update-git-reference";
+
+const reference = {
+  owner: "owner",
+  repo: "repo",
+  branch: "feature",
+  sha: "abc123",
+  githubToken: "token",
+};
+
+function response(status: number) {
+  return {
+    ok: status >= 200 && status < 300,
+    status,
+    text: async () => "response body",
+  };
+}
+
+describe("updateGitReference", () => {
+  it("should patch the branch reference", async () => {
+    await updateGitReference({
+      ...reference,
+      fetchFn: async (url, init) => {
+        expect(url).toBe(
+          `${GITHUB_API_URL}/repos/owner/repo/git/refs/heads/feature`,
+        );
+        expect(init.method).toBe("PATCH");
+        expect(init.headers).toMatchObject({ Authorization: "Bearer token" });
+        expect(JSON.parse(String(init.body))).toEqual({
+          sha: "abc123",
+          force: false,
+        });
+        return response(200);
+      },
+    });
+  });
+
+  it("should not retry deterministic client errors", async () => {
+    for (const status of [400, 404, 409, 422]) {
+      let attempts = 0;
+
+      await expect(
+        updateGitReference({
+          ...reference,
+          fetchFn: async () => {
+            attempts++;
+            return response(status);
+          },
+          retryOptions: { initialDelayMs: 1 },
+        }),
+      ).rejects.toThrow(`Failed to update reference: ${status}`);
+
+      expect(attempts).toBe(1);
+    }
+  });
+
+  it("should retry transient HTTP errors", async () => {
+    for (const status of [403, 429, 500]) {
+      let attempts = 0;
+
+      await updateGitReference({
+        ...reference,
+        fetchFn: async () => response(attempts++ === 0 ? status : 200),
+        retryOptions: { initialDelayMs: 1 },
+      });
+
+      expect(attempts).toBe(2);
+    }
+  });
+
+  it("should retry network errors", async () => {
+    let attempts = 0;
+
+    await updateGitReference({
+      ...reference,
+      fetchFn: async () => {
+        attempts++;
+        if (attempts === 1) {
+          throw new Error("network error");
+        }
+        return response(200);
+      },
+      retryOptions: { initialDelayMs: 1 },
+    });
+
+    expect(attempts).toBe(2);
+  });
+});