From 4e2cfbac3621fa54f6996ff6f0fc4b3978e3eebf Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Tue, 15 Jul 2025 17:10:23 -0700 Subject: [PATCH] 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 018533dc9a6714e53169a043c494a54fc637d45c. * fix branch names --- src/entrypoints/prepare.ts | 20 +-- src/entrypoints/update-comment-link.ts | 2 +- src/github/operations/branch-cleanup.ts | 29 ++++- src/github/operations/branch.ts | 41 +++--- src/mcp/github-file-ops-server.ts | 161 ++++++++++++++++++------ src/mcp/install-mcp-server.ts | 3 + test/branch-cleanup.test.ts | 38 +++++- test/comment-logic.test.ts | 27 +++- test/install-mcp-server.test.ts | 22 ++++ 9 files changed, 271 insertions(+), 72 deletions(-) diff --git a/src/entrypoints/prepare.ts b/src/entrypoints/prepare.ts index 257d7f8..d5e968f 100644 --- 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, diff --git a/src/entrypoints/update-comment-link.ts b/src/entrypoints/update-comment-link.ts index 4664691..85b2455 100644 --- 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, }; diff --git a/src/github/operations/branch-cleanup.ts b/src/github/operations/branch-cleanup.ts index 9ac2cef..88de6de 100644 --- 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})`; } diff --git a/src/github/operations/branch.ts b/src/github/operations/branch.ts index 6f5cd6e..68e8b0e 100644 --- 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); } } diff --git a/src/mcp/github-file-ops-server.ts b/src/mcp/github-file-ops-server.ts index 4b477d2..e3da6f4 100644 --- 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 { + // 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}`, - ); - } - - const refData = (await refResponse.json()) as GitHubRef; - const baseSha = refData.object.sha; + // 1. Get the branch reference (create if doesn't exist) + const baseSha = await getOrCreateBranchRef( + owner, + repo, + branch, + githubToken, + ); // 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}`, - ); - } - - const refData = (await refResponse.json()) as GitHubRef; - const baseSha = refData.object.sha; + // 1. Get the branch reference (create if doesn't exist) + const baseSha = await getOrCreateBranchRef( + owner, + repo, + branch, + githubToken, + ); // 2. Get the base commit const commitUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/commits/${baseSha}`; diff --git a/src/mcp/install-mcp-server.ts b/src/mcp/install-mcp-server.ts index 10b0669..31c57dd 100644 --- 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", diff --git a/test/branch-cleanup.test.ts b/test/branch-cleanup.test.ts index 07b5731..2837432 100644 --- 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", + ); + }); }); diff --git a/test/comment-logic.test.ts b/test/comment-logic.test.ts index a61c235..f1b3754 100644 --- 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"); + }); }); }); diff --git a/test/install-mcp-server.test.ts b/test/install-mcp-server.test.ts index 7c63fb2..3f14a6e 100644 --- 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, });