-
Notifications
You must be signed in to change notification settings - Fork 471
Fix hardcoded main branch in gh aw trial --host-repo
#43405
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
Changes from all commits
8aee340
e6ed69e
980047c
bedf6c5
18342aa
d9331c0
c586c11
b37a1d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| # ADR-43405: Dynamic Default-Branch Detection for `gh aw trial --host-repo` | ||
|
|
||
| **Date**: 2026-07-05 | ||
| **Status**: Draft | ||
| **Deciders**: pelikhan, copilot-swe-agent | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| `gh aw trial --host-repo=.` fails immediately when the host repository's default branch is not `main`. The two internal helpers `commitAndPushWorkflow` and `copyTrialResultsToHostRepo` unconditionally ran `git pull origin main` and `git push origin main`, which do not exist in repositories that use `master`, `trunk`, or any other default branch name. Additionally, neither helper set `cmd.Dir` on the `git pull` command, which was a pre-existing bug that could silently operate on the wrong directory. The tool needs to work correctly across all repositories regardless of their default branch name. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will introduce `getCurrentBranchIn(dir string)` — a directory-aware variant of the existing `getCurrentBranch()` helper — and use it in both `commitAndPushWorkflow` and `copyTrialResultsToHostRepo` to read the actual default branch from the locally-checked-out clone (`tempDir`) at runtime. When detection fails (non-git directory, detached HEAD, command error), the implementation falls back to `"main"` with debug logging. Since `tempDir` is always a fresh clone of the host repo, `HEAD` reliably reflects the remote default branch. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Explicit `--default-branch` CLI flag | ||
|
|
||
| Users could pass the default branch name as a flag to `gh aw trial`. This avoids any dynamic detection and makes the branch explicit. | ||
|
|
||
| This was not chosen because it places an unnecessary burden on every user to know their repo's default branch name. The local clone already carries this information, making auto-detection both simpler and more reliable. A flag would also break automation scripts that work across repos with different default branches. | ||
|
|
||
| #### Alternative 2: Query the remote for the default branch | ||
|
|
||
| The default branch could be queried from the remote via `git remote show origin` (parsing `HEAD branch:`) or the GitHub API. This would work regardless of local checkout state. | ||
|
|
||
| This was not chosen because `tempDir` is always a fresh clone of the host repo with a checked-out `HEAD`, so reading `HEAD` locally is equivalent to reading the remote default branch — while being faster, simpler, and not requiring network access or API credentials at that point in the workflow. | ||
|
|
||
| #### Alternative 3: Read default branch from GitHub API / repository metadata | ||
|
|
||
| The `gh` CLI or GitHub API could provide the default branch before cloning. This is authoritative but adds an API call and couples the implementation to the GitHub API. | ||
|
|
||
| This was not chosen for the same reason as Alternative 2: the local clone already reflects the remote's `HEAD`, and the added complexity is not justified for a simple bug fix. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - `gh aw trial --host-repo=.` now works correctly for repositories using `master`, `trunk`, or any other default branch name. | ||
| - Reuses the existing `getCurrentBranch()` infrastructure, keeping the codebase DRY; the new `getCurrentBranchIn(dir)` function is the single source of truth for branch detection with or without a directory argument. | ||
| - Adds comprehensive test coverage (`TestGetCurrentBranchIn`) for `main`, `master`, custom branch names, and the non-git-directory error path. | ||
| - Fixes the pre-existing missing `cmd.Dir` on `git pull` calls, preventing potential silent operation on the wrong working directory. | ||
|
|
||
| #### Negative | ||
| - If `tempDir` is somehow in detached HEAD state (edge case: shallow clone, CI runner behavior), the fallback to `"main"` will be used silently (only debug-logged), which could cause a push failure or push to an unintended branch without a user-visible error message. | ||
| - Relies on local checkout state (`HEAD`) rather than an authoritative remote query; if the remote default branch changes between clone and push (extremely unlikely in normal usage), the push could target a stale branch name. | ||
|
|
||
| #### Neutral | ||
| - The `getCurrentBranch()` function (no-dir variant) is refactored to delegate to `getCurrentBranchIn("")`, maintaining backward compatibility with all existing callers. | ||
| - The fallback branch name remains `"main"` for compatibility with the majority of GitHub repositories. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -522,16 +522,22 @@ func commitAndPushWorkflow(tempDir, workflowName string, verbose bool) error { | |
| return fmt.Errorf("failed to commit changes: %w (output: %s)", err, string(output)) | ||
| } | ||
|
|
||
| branch, err := getCurrentBranchIn(tempDir) | ||
| if err != nil { | ||
| trialRepoLog.Printf("Failed to detect branch in %s: %v, falling back to main", tempDir, err) | ||
| branch = "main" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same silent 💡 Details and suggested fixWhen Because branch, err := getCurrentBranchIn(tempDir)
if err != nil {
return fmt.Errorf("failed to detect branch for push in %s: %w", tempDir, err)
} |
||
| } | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Pulling latest changes from main branch")) | ||
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Pulling latest changes from "+branch+" branch")) | ||
| } | ||
| cmd = exec.Command("git", "pull", "origin", "main") | ||
| cmd = exec.Command("git", "pull", "--rebase", "origin", branch) | ||
| cmd.Dir = tempDir | ||
| if output, err := cmd.CombinedOutput(); err != nil { | ||
| return fmt.Errorf("failed to pull latest changes: %w (output: %s)", err, string(output)) | ||
| } | ||
|
Comment on lines
+531
to
537
|
||
|
|
||
| // Push to main | ||
| cmd = exec.Command("git", "push", "origin", "main") | ||
| // Push to current branch | ||
| cmd = exec.Command("git", "push", "origin", branch) | ||
| cmd.Dir = tempDir | ||
| if output, err := cmd.CombinedOutput(); err != nil { | ||
| return fmt.Errorf("failed to push changes: %w (output: %s)", err, string(output)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,11 @@ | |
|
|
||
| package cli | ||
|
|
||
| import "testing" | ||
| import ( | ||
| "os/exec" | ||
| "strings" | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestTrialRepositoryURLHelpers(t *testing.T) { | ||
| tests := []struct { | ||
|
|
@@ -61,3 +65,89 @@ func TestTrialRepositoryURLHelpers(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestGetCurrentBranchIn(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Consider moving this test there to keep all @copilot please address this. |
||
| // initRepo creates a minimal git repo in dir with the given branch name. | ||
| initRepo := func(t *testing.T, dir, branch string) { | ||
| t.Helper() | ||
| run := func(args ...string) { | ||
| t.Helper() | ||
| cmd := exec.Command(args[0], args[1:]...) | ||
| cmd.Dir = dir | ||
| if out, err := cmd.CombinedOutput(); err != nil { | ||
| t.Fatalf("command %v failed: %v (output: %s)", args, err, out) | ||
| } | ||
| } | ||
| run("git", "init") | ||
| run("git", "config", "user.email", "test@example.com") | ||
| run("git", "config", "user.name", "Test") | ||
| run("git", "symbolic-ref", "HEAD", "refs/heads/"+branch) | ||
| run("git", "commit", "--allow-empty", "-m", "init") | ||
| } | ||
|
|
||
| t.Run("returns main for a repo using main", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
| initRepo(t, dir, "main") | ||
| got, err := getCurrentBranchIn(dir) | ||
| if err != nil { | ||
| t.Fatalf("getCurrentBranchIn() unexpected error: %v", err) | ||
| } | ||
| if got != "main" { | ||
| t.Fatalf("getCurrentBranchIn() = %q, want %q", got, "main") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("returns master for a repo using master", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
| initRepo(t, dir, "master") | ||
| got, err := getCurrentBranchIn(dir) | ||
| if err != nil { | ||
| t.Fatalf("getCurrentBranchIn() unexpected error: %v", err) | ||
| } | ||
| if got != "master" { | ||
| t.Fatalf("getCurrentBranchIn() = %q, want %q", got, "master") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("returns custom branch name", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
| initRepo(t, dir, "trunk") | ||
| got, err := getCurrentBranchIn(dir) | ||
| if err != nil { | ||
| t.Fatalf("getCurrentBranchIn() unexpected error: %v", err) | ||
| } | ||
| if got != "trunk" { | ||
| t.Fatalf("getCurrentBranchIn() = %q, want %q", got, "trunk") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("returns error for non-git directory", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
| _, err := getCurrentBranchIn(dir) | ||
| if err == nil { | ||
| t.Fatal("getCurrentBranchIn() expected an error for non-git directory, got nil") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test covers non-git-directory error but not detached HEAD, which is the primary trigger for the 💡 Suggested additional test case
This matters because both t.Run("returns error for detached HEAD", func(t *testing.T) {
dir := t.TempDir()
initRepo(t, dir, "main")
// detach HEAD by checking out the commit SHA directly
run := func(args ...string) {
t.Helper()
cmd := exec.Command(args[0], args[1:]...)
cmd.Dir = dir
if out, err := cmd.CombinedOutput(); err != nil {
t.Fatalf("command %v failed: %v (output: %s)", args, err, out)
}
}
// Get current HEAD SHA and detach
sha, _ := exec.Command("git", "-C", dir, "rev-parse", "HEAD").Output()
run("git", "checkout", strings.TrimSpace(string(sha)))
_, err := getCurrentBranchIn(dir)
if err == nil {
t.Fatal("getCurrentBranchIn() expected an error for detached HEAD, got nil")
}
}) |
||
| } | ||
| }) | ||
|
Comment on lines
+112
to
+130
|
||
|
|
||
| t.Run("returns error in detached HEAD state", func(t *testing.T) { | ||
| dir := t.TempDir() | ||
| initRepo(t, dir, "main") | ||
| // Detach HEAD by checking out the commit hash directly. | ||
| cmd := exec.Command("git", "rev-parse", "HEAD") | ||
| cmd.Dir = dir | ||
| out, err := cmd.Output() | ||
| if err != nil { | ||
| t.Fatalf("git rev-parse HEAD failed: %v", err) | ||
| } | ||
| hash := strings.TrimSpace(string(out)) | ||
| detach := exec.Command("git", "checkout", hash) | ||
| detach.Dir = dir | ||
| if out, err := detach.CombinedOutput(); err != nil { | ||
| t.Fatalf("git checkout %s failed: %v (output: %s)", hash, err, out) | ||
| } | ||
| _, err = getCurrentBranchIn(dir) | ||
| if err == nil { | ||
| t.Fatal("getCurrentBranchIn() expected an error in detached HEAD state, got nil") | ||
| } | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent
"main"fallback reintroduces the exact bug this PR is fixing when branch detection fails.💡 Details and suggested fix
If
getCurrentBranchInreturns an error — including the detached HEAD path wheregit branch --show-currentexits 0 with empty output — execution silently falls through tobranch = "main". The error is only logged at debug level; the function then proceeds togit push origin main, silently pushing to the wrong branch in non-main-default repos.The
tempDiris a fresh clone, making detached HEAD uncommon in practice, but any transient git failure (filesystem permissions, corrupt index) will also trigger this path and produce a misleadingSUCCESSlog for the user.Suggested approach: propagate the error rather than swallowing it:
If a graceful fallback is truly desired, at minimum surface it to the user via
console.FormatWarningMessagerather than a silent debug log, so operators know the wrong branch may be in use.