From 018533dc9a6714e53169a043c494a54fc637d45c Mon Sep 17 00:00:00 2001 From: Ashwin Bhat Date: Tue, 15 Jul 2025 16:05:30 -0700 Subject: [PATCH] Revert "feat: defer remote branch creation until first commit (#244)" (#278) This reverts commit cefe963a6b4ae0e511c59b9d6cb6b7b5923714a1. --- src/entrypoints/prepare.ts | 17 ++- 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 | 165 ++++++------------------ src/mcp/install-mcp-server.ts | 1 - test/branch-cleanup.test.ts | 38 +----- test/comment-logic.test.ts | 27 +--- 8 files changed, 71 insertions(+), 249 deletions(-) diff --git a/src/entrypoints/prepare.ts b/src/entrypoints/prepare.ts index 3af5c6b..257d7f8 100644 --- a/src/entrypoints/prepare.ts +++ b/src/entrypoints/prepare.ts @@ -12,6 +12,7 @@ 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"; @@ -66,7 +67,17 @@ async function run() { // Step 8: Setup branch const branchInfo = await setupBranch(octokit, githubData, context); - // Step 9: Configure git authentication if not using commit signing + // 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 if (!context.inputs.useCommitSigning) { try { await configureGitAuth(githubToken, context, commentData.user); @@ -76,7 +87,7 @@ async function run() { } } - // Step 10: Create prompt file + // Step 11: Create prompt file await createPrompt( commentId, branchInfo.baseBranch, @@ -85,7 +96,7 @@ async function run() { context, ); - // Step 11: Get MCP configuration + // Step 12: Get MCP configuration const additionalMcpConfig = process.env.MCP_CONFIG || ""; const mcpConfig = await prepareMcpConfig({ githubToken, diff --git a/src/entrypoints/update-comment-link.ts b/src/entrypoints/update-comment-link.ts index 85b2455..4664691 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 || !branchLink ? undefined : claudeBranch, + branchName: shouldDeleteBranch ? undefined : claudeBranch, triggerUsername, errorDetails, }; diff --git a/src/github/operations/branch-cleanup.ts b/src/github/operations/branch-cleanup.ts index 88de6de..9ac2cef 100644 --- a/src/github/operations/branch-cleanup.ts +++ b/src/github/operations/branch-cleanup.ts @@ -14,31 +14,6 @@ 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 } = @@ -106,8 +81,8 @@ export async function checkAndCommitOrDeleteBranch( branchLink = `\n[View branch](${branchUrl})`; } } catch (error) { - console.error("Error comparing commits on Claude branch:", error); - // If we can't compare but the branch exists remotely, include the branch link + console.error("Error checking for commits on Claude branch:", error); + // If we can't check, assume the branch has commits to be safe 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 68e8b0e..6f5cd6e 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; } - // Generate branch name for either an issue or closed/merged PR + // Creating a new branch 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 to verify it exists + // Get the SHA of the source branch const sourceBranchRef = await octokits.rest.git.getRef({ owner, repo, @@ -108,34 +108,23 @@ export async function setupBranch( }); const currentSHA = sourceBranchRef.data.object.sha; - console.log(`Source branch 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)`, - ); + console.log(`Current 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 - }; - } + // Create branch using GitHub API + await octokits.rest.git.createRef({ + owner, + repo, + ref: `refs/heads/${newBranch}`, + sha: currentSHA, + }); - // 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}`; + // Checkout the new branch (shallow fetch for performance) + await $`git fetch origin --depth=1 ${newBranch}`; + await $`git checkout ${newBranch}`; console.log( - `Successfully created and checked out local branch: ${newBranch}`, + `Successfully created and checked out new branch: ${newBranch}`, ); // Set outputs for GitHub Actions @@ -147,7 +136,7 @@ export async function setupBranch( currentBranch: newBranch, }; } catch (error) { - console.error("Error in branch setup:", error); + console.error("Error creating branch:", error); process.exit(1); } } diff --git a/src/mcp/github-file-ops-server.ts b/src/mcp/github-file-ops-server.ts index f71abd2..4b477d2 100644 --- a/src/mcp/github-file-ops-server.ts +++ b/src/mcp/github-file-ops-server.ts @@ -52,120 +52,6 @@ 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}`); - } - - // Branch doesn't exist, need to create it - console.log(`Branch ${branch} does not exist, creating it...`); - - // Get base branch from environment or determine it - const baseBranch = process.env.BASE_BRANCH || "main"; - - // 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 - 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", @@ -195,13 +81,24 @@ server.tool( return filePath; }); - // 1. Get the branch reference (create if doesn't exist) - const baseSha = await getOrCreateBranchRef( - owner, - repo, - branch, - githubToken, - ); + // 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; // 2. Get the base commit const commitUrl = `${GITHUB_API_URL}/repos/${owner}/${repo}/git/commits/${baseSha}`; @@ -363,6 +260,7 @@ 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; } @@ -455,13 +353,24 @@ server.tool( return filePath; }); - // 1. Get the branch reference (create if doesn't exist) - const baseSha = await getOrCreateBranchRef( - owner, - repo, - branch, - githubToken, - ); + // 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; // 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 30482af..10b0669 100644 --- a/src/mcp/install-mcp-server.ts +++ b/src/mcp/install-mcp-server.ts @@ -100,7 +100,6 @@ export async function prepareMcpConfig( REPO_OWNER: owner, REPO_NAME: repo, BRANCH_NAME: branch, - BASE_BRANCH: process.env.BASE_BRANCH || "", 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 2837432..07b5731 100644 --- a/test/branch-cleanup.test.ts +++ b/test/branch-cleanup.test.ts @@ -21,7 +21,6 @@ describe("checkAndCommitOrDeleteBranch", () => { const createMockOctokit = ( compareResponse?: any, deleteRefError?: Error, - branchExists: boolean = true, ): Octokits => { return { rest: { @@ -29,14 +28,6 @@ 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 () => { @@ -111,7 +102,6 @@ describe("checkAndCommitOrDeleteBranch", () => { compareCommitsWithBasehead: async () => { throw new Error("API error"); }, - getBranch: async () => ({ data: {} }), // Branch exists }, git: { deleteRef: async () => ({ data: {} }), @@ -133,7 +123,7 @@ describe("checkAndCommitOrDeleteBranch", () => { `\n[View branch](${GITHUB_SERVER_URL}/owner/repo/tree/claude/issue-123-20240101-1234)`, ); expect(consoleErrorSpy).toHaveBeenCalledWith( - "Error comparing commits on Claude branch:", + "Error checking for commits on Claude branch:", expect.any(Error), ); }); @@ -158,30 +148,4 @@ 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 f1b3754..a61c235 100644 --- a/test/comment-logic.test.ts +++ b/test/comment-logic.test.ts @@ -1,8 +1,5 @@ import { describe, it, expect } from "bun:test"; -import { - updateCommentBody, - type CommentUpdateInput, -} from "../src/github/operations/comment-logic"; +import { updateCommentBody } from "../src/github/operations/comment-logic"; describe("updateCommentBody", () => { const baseInput = { @@ -420,27 +417,5 @@ 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"); - }); }); });