anthropics / anthropics/claude-code-action

Design discussion: Fork PR support

Abierto
#939 3 comentarios 4 reacciones 0 asignados Ver en GitHub
dev-experience feature-request p2
Lenguaje dominante
TypeScript
Estrellas
8.9k
Forks
2.1k
Merge medio
3 d 9 h
PR fusionados (30 d)
10

Descripción

## Background

PR #937 reverted fork PR support (#851) and unique branch naming (#931) to fix a P1 bug where commits were pushed to wrong branches. This blocks major OSS adoption (e.g., PyTorch where 90%+ PRs are from forks).

> **Opening this discussion to align on design approach before implementation.**

**Related:** #851, #930, #931, #936, #937

## Problem

Three conflicting requirements:
1. Local branch name must match PR's head ref for `git push` to work (as raised in #936)
2. Unique local naming (`pr-{number}`) needed to avoid conflicts (the concern in #930)
3. Fork PRs require fetching from `pull/{number}/head` refs (the issue noted in #851)

## Design Decisions Needed

### 1. Environment Setup Philosophy

Should `claude-code-action` remain self-contained or delegate to user setup?

**Option A: Self-Contained**
- Action handles all git setup automatically
- Pros: Zero config, consistent behavior, better UX
- Cons: More complex implementation

**Option B: Delegated Setup**
- Users run `actions/checkout`
- Pros: Simpler action code
- Cons: Higher user burden, inconsistent setups

---

### 2. Fork PR Support Implementation

Assuming self-contained approach, propose the following solution:

## Proposed Solution

**Explicit Push Target + Conditional Handling**

```typescript
// Key changes in setupBranch()
if (isForkPR) {
const localBranch = `pr-${entityNumber}`;
execGit(["fetch", "origin", `pull/${entityNumber}/head:${localBranch}`]);
execGit(["checkout", localBranch, "--"]);
return { currentBranch: localBranch, originalBranchRef: headRefName };
} else {
// Same-repo: keep current behavior
execGit(["fetch", "origin", `--depth=${fetchDepth}`, headRefName]);
execGit(["checkout", headRefName, "--"]);
return { currentBranch: headRefName };
}

// In push logic
const target = originalBranchRef || currentBranch;
execGit(["push", "origin", `HEAD:${target}`]);
```

**Changes needed:**
1. Add `originalBranchRef?: string` to `BranchInfo` type
2. Implement fork detection: `headRepository.id !== baseRepository.id`
3. Update push commands to use explicit target
4. Add GraphQL fields for repository IDs

**Why it works:**
- Fork PRs: unique local name (`pr-123`) + explicit push target
- Same-repo PRs: no change (keeps current behavior)
- Avoids branch conflicts while fixing push destination

## Discussion Points

**Feedback needed on these key decisions:**

1. **Environment setup**: Should we keep the self-contained approach or allow delegated setup?
2. **Fork detection**: Is `headRepository.id !== baseRepository.id` sufficient, or do we need additional checks?
3. **Fork support enablement**: Should fork PR support be opt-out (default enabled) or opt-in?

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.