Skip to content

feat: persist issue branches for reuse on subsequent invocations #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ashwin-ant
Copy link
Collaborator

This PR addresses issue #103

  • Claude now checks for existing branches before creating new ones for issues
  • Issue branches are preserved even with 0 commits to allow reuse
  • Claude is aware when it's reusing an existing branch

This allows users to iteratively ask Claude to make changes on the same branch before creating a PR.

Generated with Claude Code

- Check for existing Claude branches before creating new ones for issues
- Skip automatic branch deletion for issue branches to allow reuse
- Add context awareness so Claude knows when reusing an existing branch
- Set IS_REUSED_BRANCH output for tracking branch reuse status

This change allows users to iteratively ask Claude to make changes on the same branch before creating a PR, as requested in issue #103.

Co-authored-by: ashwin-ant <[email protected]>
Copy link
Contributor

claude bot commented May 31, 2025

Claude finished @ashwin-ant's task —— View job


PR Review: Branch Persistence Feature

I've completed my review of this PR that implements branch persistence for issue branches. Here's my comprehensive feedback:

📋 Review Summary

Review Tasks:

  • Analyze changed files and understand implementation
  • Review branch persistence logic in branch.ts
  • Check prompt creation changes in create-prompt/index.ts
  • Examine update comment logic changes
  • Evaluate code quality and best practices
  • Identify potential bugs or issues
  • Provide comprehensive feedback

✅ What This PR Does Well

  1. Clean Architecture: The changes maintain clear separation of concerns between branch operations, comment logic, and prompt generation.

  2. Backward Compatibility: Existing PR workflows remain unchanged - only issue branches get the new persistence behavior.

  3. Informative Feedback: The implementation provides clear console output about branch reuse (lines 105, 141-143 in branch.ts).

  4. Robust Error Handling: The branch checking logic gracefully handles API failures (lines 107-110 in branch.ts).

  5. Consistent Naming: The branch search pattern claude/issue-{entityNumber}-* maintains consistency with existing naming conventions.

🔍 Code Quality Assessment

src/github/operations/branch.ts (Main Changes)

Strengths:

  • Clear logic flow for branch reuse detection
  • Proper error handling with fallback to new branch creation
  • Good use of GitHub API pagination (per_page: 100)
  • Appropriate depth settings for git fetch operations

Potential Issues:

  • Performance: The listBranches call fetches up to 100 branches but only processes the first page. For repos with many Claude branches, this could miss existing branches.
  • Race Condition: Multiple concurrent invocations could potentially create branches with the same pattern if they run simultaneously.

src/create-prompt/index.ts

Good Implementation:

  • Minimal change adding isReusedBranch parameter
  • Proper propagation through the call chain
  • Contextual information provided to prompts

src/entrypoints/update-comment-link.ts

Smart Approach:

  • Clean separation between issue and PR branch handling (lines 91-101)
  • Preserves issue branches while maintaining existing PR cleanup logic
  • Maintains branch links for preserved branches

🚨 Potential Issues & Recommendations

1. Branch Discovery Limitation

// Current code only checks first 100 branches
const { data: branches } = await octokits.rest.repos.listBranches({
  owner,
  repo,
  per_page: 100, // ← Could miss branches if there are more than 100
});

Recommendation: Implement pagination or use a more targeted search:

// Consider using GitHub's search API for better filtering
const searchQuery = `repo:${owner}/${repo} ref:refs/heads/claude/issue-${entityNumber}-`;

2. Branch Selection Logic

The current implementation uses .find() which returns the first match, not necessarily the most recent:

const existingBranch = branches.find(branch => 
  branch.name.startsWith(`claude/issue-${entityNumber}-`)
);

Recommendation: Sort by creation date or use naming convention to get the most recent:

const existingBranches = branches
  .filter(branch => branch.name.startsWith(`claude/issue-${entityNumber}-`))
  .sort((a, b) => b.name.localeCompare(a.name)); // Newest timestamp first
const existingBranch = existingBranches[0];

3. Missing Test Coverage

The branch persistence logic lacks dedicated tests. The existing branch-cleanup.test.ts doesn't cover the new branch discovery functionality.

Recommendation: Add tests for:

  • Finding existing branches
  • Handling multiple matches
  • Error scenarios in branch listing
  • Edge case where branch exists but is inaccessible

4. Documentation Gap

The BranchInfo type is updated but lacks JSDoc comments explaining the new isReusedBranch field.

🏗️ Architecture Feedback

Positive Design Decisions:

  1. Selective Persistence: Only persisting issue branches (not PR branches) makes sense for the use case
  2. Minimal Interface Changes: The BranchInfo type extension is clean and optional
  3. Clear Intent: The isReusedBranch flag clearly communicates the branch state

Suggestions for Future Enhancement:

  1. Branch Metadata: Consider storing metadata about Claude's work in branch descriptions
  2. Cleanup Strategy: Future enhancement could add branch aging (delete after X days of inactivity)
  3. Conflict Resolution: Plan for handling cases where multiple users invoke Claude on the same issue

🔐 Security & Performance Considerations

Security: ✅ No security concerns identified. The implementation properly uses authenticated API calls and follows existing patterns.

Performance:

  • ⚠️ Additional API call to list branches on every issue invocation
  • ✅ Reasonable pagination limit (100 branches)
  • ✅ Efficient git fetch with --depth=20

📝 Minor Code Style Notes

  1. Line 98-100: The branch filtering logic is clear and readable
  2. Line 176-178: Proper use of conditional outputs for GitHub Actions
  3. Line 91: Good use of descriptive variable name skipBranchDeletion

🎯 Overall Assessment

This is a well-implemented feature that addresses the stated requirement effectively. The code is clean, follows existing patterns, and maintains backward compatibility. The main areas for improvement are around robustness (pagination, branch selection) and test coverage.

Recommendation: ✅ Approve with suggestions - The current implementation is production-ready, but the suggested improvements would enhance reliability.

Comment on lines 91 to 106
const { data: branches } = await octokits.rest.repos.listBranches({
owner,
repo,
per_page: 100,
});

// Look for existing branches with pattern claude/issue-{entityNumber}-*
const existingBranch = branches.find(branch =>
branch.name.startsWith(`claude/issue-${entityNumber}-`)
);

if (existingBranch) {
branchToUse = existingBranch.name;
isReusedBranch = true;
console.log(`Found existing Claude branch for issue #${entityNumber}: ${branchToUse}`);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work for repos with lots of branches. Let's do a graphql query so that we can filter on prefixes

query($owner: String!, $repo: String!, $prefix: String!) {
 repository(owner: $owner, name: $repo) {
   refs(refPrefix: "refs/heads/", query: $prefix, first: 100) {
     nodes {
       name
     }
   }
 }
}

@claude

Copy link
Contributor

@claude claude bot May 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude finished @ashwin-ant's task —— View job


✅ Implemented GraphQL Branch Query

I've successfully replaced the REST API call with the suggested GraphQL query for more efficient branch filtering.

Completed Tasks:

  • Read current implementation in branch.ts
  • Implement GraphQL query to replace REST API call
  • Update response handling for GraphQL format
  • Run tests to ensure functionality (all 175 tests passing)
  • Commit changes

What Changed:

  • Replaced octokits.rest.repos.listBranches() with a GraphQL query that filters branches by prefix
  • The GraphQL query uses the refs field with a query parameter to efficiently filter branches
  • This solves the limitation where repositories with >100 branches could miss existing Claude branches
  • The new implementation is more performant as it filters on the server side

The change maintains backward compatibility while significantly improving branch discovery for large repositories.

…rformance

- Replaced REST API listBranches call with GraphQL query that filters branches by prefix
- This fixes the issue where repositories with >100 branches could miss existing Claude branches
- The GraphQL query directly filters by prefix, making it more efficient and accurate

Co-authored-by: ashwin-ant <[email protected]>
@ashwin-ant
Copy link
Collaborator Author

@claude review this again, any edge cases?

Copy link
Contributor

claude bot commented May 31, 2025

Claude finished @ashwin-ant's task —— View job


Edge Case Analysis for Branch Persistence Feature

After reviewing the updated implementation with GraphQL, here are the edge cases I've identified:

Review Tasks:

  • Read and analyze the updated branch.ts implementation with GraphQL changes
  • Review the branch persistence logic for edge cases
  • Check for race conditions and concurrency issues
  • Analyze error handling scenarios
  • Review the branch naming and selection logic
  • Update comment with comprehensive edge case analysis

🚨 Critical Edge Cases Found

1. Race Condition: Concurrent Claude Invocations

Scenario: Two users trigger Claude on the same issue within seconds of each other.

Problem: Both invocations could run the GraphQL query at nearly the same time, both find no existing branch, and both try to create a new branch with different timestamps.

Impact:

  • First invocation succeeds in creating claude/issue-103-20250531_051637
  • Second invocation fails with a GitHub API error when trying to create claude/issue-103-20250531_051638
  • The second invocation crashes without proper error handling

Fix needed: Add retry logic with branch existence check before creation.

2. Multiple Existing Branches Selection

Current code (line 119-120):

// Use the first matching branch (could be sorted by date in future)
branchToUse = branches[0].name;

Problem: If multiple branches exist (e.g., from failed cleanups or manual branch creation), the selection is arbitrary - it picks whichever GitHub returns first, which may not be the most recent or most relevant.

Scenarios:

  • User manually creates claude/issue-103-manual
  • Claude creates claude/issue-103-20250531_051637
  • Next invocation might pick the manual branch instead of Claude's

3. Orphaned Work Detection

Scenario: Claude is invoked on an issue, makes commits, but the user never creates a PR. Later, Claude is invoked again.

Problem: The reused branch message only says "This branch was reused from a previous Claude invocation" but doesn't indicate:

  • What work was previously done
  • Whether there are uncommitted changes
  • Whether the branch has diverged from the base

Impact: Claude might make conflicting changes without awareness of previous work.

4. Branch Deletion Edge Cases

Scenario: User manually deletes the Claude branch while Claude is working.

Current behavior:

  • The git fetch command (line 152) will fail
  • Error is caught but only logged (line 125-127)
  • Falls through to create a new branch, but branchToUse still has the deleted branch name
  • Attempt to checkout non-existent branch fails

5. GraphQL Query Limitations

The query (lines 92-102):

refs(refPrefix: "refs/heads/", query: $prefix, first: 100)

Problems:

  1. Case sensitivity: GitHub's ref query is case-sensitive. If someone creates Claude/issue-103-*, it won't be found.
  2. Special characters: Issue numbers with special regex characters could break the prefix matching.
  3. Pagination: Still limited to 100 branches. In repos with many Claude branches, older branches will be missed.

6. Base Branch Divergence

Scenario:

  1. Claude creates branch from main at commit A
  2. main advances to commit Z
  3. Claude reuses the branch but doesn't detect it's far behind

Impact: Merge conflicts become likely, and Claude lacks context about why.

7. Partial State Recovery

Scenario: Previous Claude invocation crashed mid-operation, leaving:

  • Uncommitted changes in the working directory
  • Partially edited files
  • Incomplete todo list

Problem: New invocation has no way to detect or recover from this state.

8. Branch Protection Rules

Scenario: Repository has branch protection rules that prevent direct pushes to branches matching claude/*

Impact:

  • Claude successfully creates/checks out the branch
  • All subsequent push attempts fail
  • No clear error message to the user about why

9. Stale Branch Accumulation

Problem: Issue branches are never cleaned up, even after issues are closed.

Long-term impact:

  • Repos accumulate hundreds of claude/issue-* branches
  • GraphQL query performance degrades
  • Branch list becomes unmanageable

10. Network Timeout During Branch Check

Current code (line 124-127):

} catch (error) {
  console.error("Error checking for existing branches:", error);
  // Continue with new branch creation if check fails
}

Problem: Network timeout is treated the same as "no branches found", potentially creating duplicate branches.

🔧 Recommendations for Robustness

  1. Add branch creation retry logic:
const maxRetries = 3;
for (let i = 0; i < maxRetries; i++) {
  try {
    await octokits.rest.git.createRef(...);
    break;
  } catch (error) {
    if (i === maxRetries - 1) throw error;
    // Check if branch was created by another process
    const exists = await checkBranchExists(branchName);
    if (exists) {
      branchToUse = branchName;
      isReusedBranch = true;
      break;
    }
  }
}
  1. Sort branches by timestamp in name:
const existingBranches = branches
  .filter(branch => branch.name.startsWith(`claude/issue-${entityNumber}-`))
  .sort((a, b) => b.name.localeCompare(a.name)); // Most recent first
  1. Add branch status check:
// After checking out existing branch
const status = await $`git status --porcelain`;
if (status.stdout) {
  console.warn("Warning: Reused branch has uncommitted changes");
}

const behind = await $`git rev-list --count HEAD..origin/${sourceBranch}`;
console.log(`Branch is ${behind.stdout.trim()} commits behind ${sourceBranch}`);
  1. Implement branch age cleanup (future enhancement):
  • Add branch creation date to branch description
  • Periodic cleanup job for branches older than X days
  • Skip reuse of branches older than threshold
  1. Better error messages:
  • Detect branch protection errors specifically
  • Provide actionable error messages
  • Suggest workarounds in error output

✅ Summary

The GraphQL implementation successfully addresses the performance issue, but several edge cases remain that could cause failures or confusion. The most critical are:

  • Race conditions during concurrent invocations
  • Arbitrary selection among multiple existing branches
  • Poor error recovery when branches are deleted
  • No awareness of previous work on reused branches

These edge cases are particularly important because the feature's goal is to improve iterative workflows, but these issues could actually make iteration more confusing.

@matanbaruch
Copy link

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants