Fix: Pass correct branch names to MCP file ops server (#279)

* Reapply "feat: defer remote branch creation until first commit (#244)" (#278) This reverts commit 018533dc9a. * fix branch names
2026-01-22 14:24:13 +08:00 · 2025-07-15 17:10:23 -07:00
parent 018533dc9a
commit 4e2cfbac36
9 changed files with 271 additions and 72 deletions
--- a/src/entrypoints/prepare.ts
+++ b/src/entrypoints/prepare.ts
@@ -12,7 +12,6 @@ import { checkHumanActor } from "../github/validation/actor";
 import { checkWritePermissions } from "../github/validation/permissions";
 import { createInitialComment } from "../github/operations/comments/create-initial";
 import { setupBranch } from "../github/operations/branch";
-import { updateTrackingComment } from "../github/operations/comments/update-with-branch";
 import { configureGitAuth } from "../github/operations/git-config";
 import { prepareMcpConfig } from "../mcp/install-mcp-server";
 import { createPrompt } from "../create-prompt";
@@ -67,17 +66,7 @@ async function run() {
    // Step 8: Setup branch
    const branchInfo = await setupBranch(octokit, githubData, context);

-    // Step 9: Update initial comment with branch link (only for issues that created a new branch)
-    if (branchInfo.claudeBranch) {
-      await updateTrackingComment(
-        octokit,
-        context,
-        commentId,
-        branchInfo.claudeBranch,
-      );
-    }
-
-    // Step 10: Configure git authentication if not using commit signing
+    // Step 9: Configure git authentication if not using commit signing
    if (!context.inputs.useCommitSigning) {
      try {
        await configureGitAuth(githubToken, context, commentData.user);
@@ -87,7 +76,7 @@ async function run() {
      }
    }

-    // Step 11: Create prompt file
+    // Step 10: Create prompt file
    await createPrompt(
      commentId,
      branchInfo.baseBranch,
@@ -96,13 +85,14 @@ async function run() {
      context,
    );

-    // Step 12: Get MCP configuration
+    // Step 11: Get MCP configuration
    const additionalMcpConfig = process.env.MCP_CONFIG || "";
    const mcpConfig = await prepareMcpConfig({
      githubToken,
      owner: context.repository.owner,
      repo: context.repository.repo,
-      branch: branchInfo.currentBranch,
+      branch: branchInfo.claudeBranch || branchInfo.currentBranch,
+      baseBranch: branchInfo.baseBranch,
      additionalMcpConfig,
      claudeCommentId: commentId.toString(),
      allowedTools: context.inputs.allowedTools,
--- a/src/entrypoints/update-comment-link.ts
+++ b/src/entrypoints/update-comment-link.ts
@@ -201,7 +201,7 @@ async function run() {
      jobUrl,
      branchLink,
      prLink,
-      branchName: shouldDeleteBranch ? undefined : claudeBranch,
+      branchName: shouldDeleteBranch || !branchLink ? undefined : claudeBranch,
      triggerUsername,
      errorDetails,
    };
--- a/src/github/operations/branch-cleanup.ts
+++ b/src/github/operations/branch-cleanup.ts
@@ -14,6 +14,31 @@ export async function checkAndCommitOrDeleteBranch(
  let shouldDeleteBranch = false;

  if (claudeBranch) {
+    // First check if the branch exists remotely
+    let branchExistsRemotely = false;
+    try {
+      await octokit.rest.repos.getBranch({
+        owner,
+        repo,
+        branch: claudeBranch,
+      });
+      branchExistsRemotely = true;
+    } catch (error: any) {
+      if (error.status === 404) {
+        console.log(`Branch ${claudeBranch} does not exist remotely`);
+      } else {
+        console.error("Error checking if branch exists:", error);
+      }
+    }
+
+    // Only proceed if branch exists remotely
+    if (!branchExistsRemotely) {
+      console.log(
+        `Branch ${claudeBranch} does not exist remotely, no branch link will be added`,
+      );
+      return { shouldDeleteBranch: false, branchLink: "" };
+    }
+
    // Check if Claude made any commits to the branch
    try {
      const { data: comparison } =
@@ -81,8 +106,8 @@ export async function checkAndCommitOrDeleteBranch(
        branchLink = `\n[View branch](${branchUrl})`;
      }
    } catch (error) {
-      console.error("Error checking for commits on Claude branch:", error);
-      // If we can't check, assume the branch has commits to be safe
+      console.error("Error comparing commits on Claude branch:", error);
+      // If we can't compare but the branch exists remotely, include the branch link
      const branchUrl = `${GITHUB_SERVER_URL}/${owner}/${repo}/tree/${claudeBranch}`;
      branchLink = `\n[View branch](${branchUrl})`;
    }
--- a/src/github/operations/branch.ts
+++ b/src/github/operations/branch.ts
@@ -84,7 +84,7 @@ export async function setupBranch(
    sourceBranch = repoResponse.data.default_branch;
  }

-  // Creating a new branch for either an issue or closed/merged PR
+  // Generate branch name for either an issue or closed/merged PR
  const entityType = isPR ? "pr" : "issue";

  // Create Kubernetes-compatible timestamp: lowercase, hyphens only, shorter format
@@ -100,7 +100,7 @@ export async function setupBranch(
  const newBranch = branchName.toLowerCase().substring(0, 50);

  try {
-    // Get the SHA of the source branch
+    // Get the SHA of the source branch to verify it exists
    const sourceBranchRef = await octokits.rest.git.getRef({
      owner,
      repo,
@@ -108,23 +108,34 @@ export async function setupBranch(
    });

    const currentSHA = sourceBranchRef.data.object.sha;
+    console.log(`Source branch SHA: ${currentSHA}`);

-    console.log(`Current SHA: ${currentSHA}`);
+    // For commit signing, defer branch creation to the file ops server
+    if (context.inputs.useCommitSigning) {
+      console.log(
+        `Branch name generated: ${newBranch} (will be created by file ops server on first commit)`,
+      );

-    // Create branch using GitHub API
-    await octokits.rest.git.createRef({
-      owner,
-      repo,
-      ref: `refs/heads/${newBranch}`,
-      sha: currentSHA,
-    });
+      // Set outputs for GitHub Actions
+      core.setOutput("CLAUDE_BRANCH", newBranch);
+      core.setOutput("BASE_BRANCH", sourceBranch);
+      return {
+        baseBranch: sourceBranch,
+        claudeBranch: newBranch,
+        currentBranch: sourceBranch, // Stay on source branch for now
+      };
+    }

-    // Checkout the new branch (shallow fetch for performance)
-    await $`git fetch origin --depth=1 ${newBranch}`;
-    await $`git checkout ${newBranch}`;
+    // For non-signing case, create and checkout the branch locally only
+    console.log(
+      `Creating local branch ${newBranch} for ${entityType} #${entityNumber} from source branch: ${sourceBranch}...`,
+    );
+
+    // Create and checkout the new branch locally
+    await $`git checkout -b ${newBranch}`;

    console.log(
-      `Successfully created and checked out new branch: ${newBranch}`,
+      `Successfully created and checked out local branch: ${newBranch}`,
    );

    // Set outputs for GitHub Actions
@@ -136,7 +147,7 @@ export async function setupBranch(
      currentBranch: newBranch,
    };
  } catch (error) {
-    console.error("Error creating branch:", error);
+    console.error("Error in branch setup:", error);
    process.exit(1);
  }
 }
--- a/src/mcp/github-file-ops-server.ts
+++ b/src/mcp/github-file-ops-server.ts
@@ -52,6 +52,116 @@ const server = new McpServer({
  version: "0.0.1",
 });

+// Helper function to get or create branch reference
+async function getOrCreateBranchRef(
+  owner: string,
+  repo: string,
+  branch: string,
+  githubToken: string,
+): Promise<string> {
+  // Try to get the branch reference
+  const refUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/refs/heads/${branch}`;
+  const refResponse = await fetch(refUrl, {
+    headers: {
+      Accept: "application/vnd.github+json",
+      Authorization: `Bearer ${githubToken}`,
+      "X-GitHub-Api-Version": "2022-11-28",
+    },
+  });
+
+  if (refResponse.ok) {
+    const refData = (await refResponse.json()) as GitHubRef;
+    return refData.object.sha;
+  }
+
+  if (refResponse.status !== 404) {
+    throw new Error(`Failed to get branch reference: ${refResponse.status}`);
+  }
+
+  const baseBranch = process.env.BASE_BRANCH!;
+
+  // Get the SHA of the base branch
+  const baseRefUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/refs/heads/${baseBranch}`;
+  const baseRefResponse = await fetch(baseRefUrl, {
+    headers: {
+      Accept: "application/vnd.github+json",
+      Authorization: `Bearer ${githubToken}`,
+      "X-GitHub-Api-Version": "2022-11-28",
+    },
+  });
+
+  let baseSha: string;
+
+  if (!baseRefResponse.ok) {
+    // If base branch doesn't exist, try default branch
+    const repoUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}`;
+    const repoResponse = await fetch(repoUrl, {
+      headers: {
+        Accept: "application/vnd.github+json",
+        Authorization: `Bearer ${githubToken}`,
+        "X-GitHub-Api-Version": "2022-11-28",
+      },
+    });
+
+    if (!repoResponse.ok) {
+      throw new Error(`Failed to get repository info: ${repoResponse.status}`);
+    }
+
+    const repoData = (await repoResponse.json()) as {
+      default_branch: string;
+    };
+    const defaultBranch = repoData.default_branch;
+
+    // Try default branch
+    const defaultRefUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/refs/heads/${defaultBranch}`;
+    const defaultRefResponse = await fetch(defaultRefUrl, {
+      headers: {
+        Accept: "application/vnd.github+json",
+        Authorization: `Bearer ${githubToken}`,
+        "X-GitHub-Api-Version": "2022-11-28",
+      },
+    });
+
+    if (!defaultRefResponse.ok) {
+      throw new Error(
+        `Failed to get default branch reference: ${defaultRefResponse.status}`,
+      );
+    }
+
+    const defaultRefData = (await defaultRefResponse.json()) as GitHubRef;
+    baseSha = defaultRefData.object.sha;
+  } else {
+    const baseRefData = (await baseRefResponse.json()) as GitHubRef;
+    baseSha = baseRefData.object.sha;
+  }
+
+  // Create the new branch using the same pattern as octokit
+  const createRefUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/refs`;
+  const createRefResponse = await fetch(createRefUrl, {
+    method: "POST",
+    headers: {
+      Accept: "application/vnd.github+json",
+      Authorization: `Bearer ${githubToken}`,
+      "X-GitHub-Api-Version": "2022-11-28",
+      "Content-Type": "application/json",
+    },
+    body: JSON.stringify({
+      ref: `refs/heads/${branch}`,
+      sha: baseSha,
+    }),
+  });
+
+  if (!createRefResponse.ok) {
+    const errorText = await createRefResponse.text();
+    throw new Error(
+      `Failed to create branch: ${createRefResponse.status} - ${errorText}`,
+    );
+  }
+
+  console.log(`Successfully created branch ${branch}`);
+  return baseSha;
+}
+
 // Commit files tool
 server.tool(
  "commit_files",
@@ -81,24 +191,13 @@ server.tool(
        return filePath;
      });

-      // 1. Get the branch reference
-      const refUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/refs/heads/${branch}`;
-      const refResponse = await fetch(refUrl, {
-        headers: {
-          Accept: "application/vnd.github+json",
-          Authorization: `Bearer ${githubToken}`,
-          "X-GitHub-Api-Version": "2022-11-28",
-        },
-      });
-
-      if (!refResponse.ok) {
-        throw new Error(
-          `Failed to get branch reference: ${refResponse.status}`,
+      // 1. Get the branch reference (create if doesn't exist)
+      const baseSha = await getOrCreateBranchRef(
+        owner,
+        repo,
+        branch,
+        githubToken,
      );
-      }
-
-      const refData = (await refResponse.json()) as GitHubRef;
-      const baseSha = refData.object.sha;

      // 2. Get the base commit
      const commitUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/commits/${baseSha}`;
@@ -260,7 +359,6 @@ server.tool(

            // Only retry on 403 errors - these are the intermittent failures we're targeting
            if (updateRefResponse.status === 403) {
-              console.log("Received 403 error, will retry...");
              throw error;
            }

@@ -353,24 +451,13 @@ server.tool(
        return filePath;
      });

-      // 1. Get the branch reference
-      const refUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/refs/heads/${branch}`;
-      const refResponse = await fetch(refUrl, {
-        headers: {
-          Accept: "application/vnd.github+json",
-          Authorization: `Bearer ${githubToken}`,
-          "X-GitHub-Api-Version": "2022-11-28",
-        },
-      });
-
-      if (!refResponse.ok) {
-        throw new Error(
-          `Failed to get branch reference: ${refResponse.status}`,
+      // 1. Get the branch reference (create if doesn't exist)
+      const baseSha = await getOrCreateBranchRef(
+        owner,
+        repo,
+        branch,
+        githubToken,
      );
-      }
-
-      const refData = (await refResponse.json()) as GitHubRef;
-      const baseSha = refData.object.sha;

      // 2. Get the base commit
      const commitUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/commits/${baseSha}`;
--- a/src/mcp/install-mcp-server.ts
+++ b/src/mcp/install-mcp-server.ts
@@ -8,6 +8,7 @@ type PrepareConfigParams = {
  owner: string;
  repo: string;
  branch: string;
+  baseBranch: string;
  additionalMcpConfig?: string;
  claudeCommentId?: string;
  allowedTools: string[];
@@ -54,6 +55,7 @@ export async function prepareMcpConfig(
    owner,
    repo,
    branch,
+    baseBranch,
    additionalMcpConfig,
    claudeCommentId,
    allowedTools,
@@ -100,6 +102,7 @@ export async function prepareMcpConfig(
          REPO_OWNER: owner,
          REPO_NAME: repo,
          BRANCH_NAME: branch,
+          BASE_BRANCH: baseBranch,
          REPO_DIR: process.env.GITHUB_WORKSPACE || process.cwd(),
          GITHUB_EVENT_NAME: process.env.GITHUB_EVENT_NAME || "",
          IS_PR: process.env.IS_PR || "false",
--- a/test/branch-cleanup.test.ts
+++ b/test/branch-cleanup.test.ts
@@ -21,6 +21,7 @@ describe("checkAndCommitOrDeleteBranch", () => {
  const createMockOctokit = (
    compareResponse?: any,
    deleteRefError?: Error,
+    branchExists: boolean = true,
  ): Octokits => {
    return {
      rest: {
@@ -28,6 +29,14 @@ describe("checkAndCommitOrDeleteBranch", () => {
          compareCommitsWithBasehead: async () => ({
            data: compareResponse || { total_commits: 0 },
          }),
+          getBranch: async () => {
+            if (!branchExists) {
+              const error: any = new Error("Not Found");
+              error.status = 404;
+              throw error;
+            }
+            return { data: {} };
+          },
        },
        git: {
          deleteRef: async () => {
@@ -102,6 +111,7 @@ describe("checkAndCommitOrDeleteBranch", () => {
          compareCommitsWithBasehead: async () => {
            throw new Error("API error");
          },
+          getBranch: async () => ({ data: {} }), // Branch exists
        },
        git: {
          deleteRef: async () => ({ data: {} }),
@@ -123,7 +133,7 @@ describe("checkAndCommitOrDeleteBranch", () => {
      `\n[View branch](${GITHUB_SERVER_URL}/owner/repo/tree/claude/issue-123-20240101-1234)`,
    );
    expect(consoleErrorSpy).toHaveBeenCalledWith(
-      "Error checking for commits on Claude branch:",
+      "Error comparing commits on Claude branch:",
      expect.any(Error),
    );
  });
@@ -148,4 +158,30 @@ describe("checkAndCommitOrDeleteBranch", () => {
      deleteError,
    );
  });
+
+  test("should return no branch link when branch doesn't exist remotely", async () => {
+    const mockOctokit = createMockOctokit(
+      { total_commits: 0 },
+      undefined,
+      false, // branch doesn't exist
+    );
+
+    const result = await checkAndCommitOrDeleteBranch(
+      mockOctokit,
+      "owner",
+      "repo",
+      "claude/issue-123-20240101-1234",
+      "main",
+      false,
+    );
+
+    expect(result.shouldDeleteBranch).toBe(false);
+    expect(result.branchLink).toBe("");
+    expect(consoleLogSpy).toHaveBeenCalledWith(
+      "Branch claude/issue-123-20240101-1234 does not exist remotely",
+    );
+    expect(consoleLogSpy).toHaveBeenCalledWith(
+      "Branch claude/issue-123-20240101-1234 does not exist remotely, no branch link will be added",
+    );
+  });
 });
--- a/test/comment-logic.test.ts
+++ b/test/comment-logic.test.ts
@@ -1,5 +1,8 @@
 import { describe, it, expect } from "bun:test";
-import { updateCommentBody } from "../src/github/operations/comment-logic";
+import {
+  updateCommentBody,
+  type CommentUpdateInput,
+} from "../src/github/operations/comment-logic";

 describe("updateCommentBody", () => {
  const baseInput = {
@@ -417,5 +420,27 @@ describe("updateCommentBody", () => {
        "• [Create PR ➔](https://github.com/owner/repo/compare/main...claude/issue-123-20240101-1200)",
      );
    });
+
+    it("should not show branch name when branch doesn't exist remotely", () => {
+      const input: CommentUpdateInput = {
+        currentBody: "@claude can you help with this?",
+        actionFailed: false,
+        executionDetails: { duration_ms: 90000 },
+        jobUrl: "https://github.com/owner/repo/actions/runs/123",
+        branchLink: "", // Empty branch link means branch doesn't exist remotely
+        branchName: undefined, // Should be undefined when branchLink is empty
+        triggerUsername: "claude",
+        prLink: "",
+      };
+
+      const result = updateCommentBody(input);
+
+      expect(result).toContain("Claude finished @claude's task in 1m 30s");
+      expect(result).toContain(
+        "[View job](https://github.com/owner/repo/actions/runs/123)",
+      );
+      expect(result).not.toContain("claude/issue-123");
+      expect(result).not.toContain("tree/claude/issue-123");
+    });
  });
 });
--- a/test/install-mcp-server.test.ts
+++ b/test/install-mcp-server.test.ts
@@ -88,6 +88,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [],
      context: mockContext,
    });
@@ -118,6 +119,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [],
      context: contextWithSigning,
    });
@@ -143,6 +145,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [
        "mcp__github__create_issue",
        "mcp__github_file_ops__commit_files",
@@ -174,6 +177,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [
        "mcp__github_file_ops__commit_files",
        "mcp__github_file_ops__update_claude_comment",
@@ -193,6 +197,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: ["Edit", "Read", "Write"],
      context: mockContext,
    });
@@ -210,6 +215,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: "",
      allowedTools: [],
      context: mockContext,
@@ -228,6 +234,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: "   \n\t  ",
      allowedTools: [],
      context: mockContext,
@@ -258,6 +265,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: additionalConfig,
      allowedTools: [
        "mcp__github__create_issue",
@@ -296,6 +304,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: additionalConfig,
      allowedTools: [
        "mcp__github__create_issue",
@@ -337,6 +346,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: additionalConfig,
      allowedTools: [],
      context: mockContextWithSigning,
@@ -357,6 +367,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: invalidJson,
      allowedTools: [],
      context: mockContextWithSigning,
@@ -378,6 +389,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: nonObjectJson,
      allowedTools: [],
      context: mockContextWithSigning,
@@ -402,6 +414,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: nullJson,
      allowedTools: [],
      context: mockContextWithSigning,
@@ -426,6 +439,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: arrayJson,
      allowedTools: [],
      context: mockContextWithSigning,
@@ -473,6 +487,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      additionalMcpConfig: additionalConfig,
      allowedTools: [],
      context: mockContextWithSigning,
@@ -496,6 +511,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [],
      context: mockContextWithSigning,
    });
@@ -517,6 +533,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [],
      context: mockContextWithSigning,
    });
@@ -545,6 +562,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [],
      context: contextWithPermissions,
    });
@@ -564,6 +582,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [],
      context: mockContextWithSigning,
    });
@@ -582,6 +601,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [],
      context: mockPRContextWithSigning,
    });
@@ -613,6 +633,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [],
      context: contextWithPermissions,
    });
@@ -641,6 +662,7 @@ describe("prepareMcpConfig", () => {
      owner: "test-owner",
      repo: "test-repo",
      branch: "test-branch",
+      baseBranch: "main",
      allowedTools: [],
      context: contextWithPermissions,
    });