Fix hardcoded main branch in gh aw trial --host-repo#43405
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
main branch in gh aw trial --host-repo
🤖 PR Triage — Run §28715668077
Fixes hardcoded main branch in gh aw trial --host-repo. +8/-5 Go, +26/-4 Go, +55/-1 test. Low risk.
|
| // getLocalBranch returns the current branch name of the git repository in dir. | ||
| // Falls back to "main" when the directory is not a git repository, the branch | ||
| // cannot be determined, or the repo is in detached HEAD state. | ||
| func getLocalBranch(dir string) string { |
There was a problem hiding this comment.
@copilot look for existing helper, avoid creating a new one here
There was a problem hiding this comment.
Done — removed getLocalBranch and extended the existing getCurrentBranch() in git.go with a getCurrentBranchIn(dir string) sibling that accepts a directory. Both commitAndPushWorkflow and copyTrialResultsToHostRepo now use getCurrentBranchIn(tempDir) with a "main" fallback on error. Addressed in bedf6c5→latest.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Fixes gh aw trial --host-repo=. failing in repositories whose default branch is not main by detecting the checked-out branch and using it for git pull/push operations within the trial temp clone.
Changes:
- Use
getCurrentBranchIn(tempDir)to determine the branch to pull/push in trial flows instead of hardcodingmain. - Ensure
git pull/git pushrun in the intended working directory by settingcmd.Dir = tempDirwhere missing/inconsistent. - Add unit tests for
getCurrentBranchInacross common branch names and non-git directories.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/trial_repository.go | Uses detected branch for pull/push when committing trial workflow changes; fixes cmd.Dir inconsistency. |
| pkg/cli/trial_helpers.go | Uses detected branch and sets cmd.Dir for pull/push when copying trial results into host repo. |
| pkg/cli/trial_repository_test.go | Adds coverage for getCurrentBranchIn behavior across branch names and error cases. |
| pkg/cli/git.go | Refactors branch detection into getCurrentBranchIn(dir) and routes getCurrentBranch() through it. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Low
| fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Pulling latest changes from "+branch+" branch")) | ||
| } | ||
| cmd = exec.Command("git", "pull", "origin", "main") | ||
| cmd = exec.Command("git", "pull", "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)) | ||
| } |
| // Pull latest changes before pushing to avoid conflicts | ||
| 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", "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)) | ||
| } |
| 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") | ||
| } | ||
| }) |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (105 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics (4 tests)
|
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 46.7 AIC · ⌖ 6.2 AIC · ⊞ 4.9K
| } | ||
| } | ||
|
|
||
| func TestGetCurrentBranchIn(t *testing.T) { |
There was a problem hiding this comment.
The TestGetCurrentBranchIn test exercises getCurrentBranchIn, which is defined in pkg/cli/git.go. The existing TestGetCurrentBranch / TestGetCurrentBranchNotInRepo tests for the same function family already live in pkg/cli/git_test.go.
Consider moving this test there to keep all getCurrentBranch* coverage in one discoverable place and align with the established convention.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking issues found; a few improvements suggested.
📋 Key Themes & Highlights
Key Themes
- Silent fallback to
main: BothcommitAndPushWorkflowandcopyTrialResultsToHostReposilently fall back tomainwhen branch detection fails. In the very scenario this PR fixes (non-maindefault branch), that silent fallback could cause a confusing push to the wrong branch. Consider propagating the error or at least emitting a visible warning. - Inconsistent
cmd.Dircoverage: The PR correctly addscmd.Dir = tempDirto pull and push intrial_helpers.go, but the precedinggit addandgit commitcommands in that same function still rely onos.Chdir. For defensive consistency, addcmd.Dirto those as well. - Detached HEAD error message:
getCurrentBranchInreturns a generic"could not determine current branch"error for detached HEAD; a more specific message would significantly improve debuggability for CI users. - Missing test case: No test for the detached HEAD state in
TestGetCurrentBranchIn.
Positive Highlights
- ✅ Root cause cleanly identified and fixed in both call sites
- ✅ The
getCurrentBranchInrefactor is well-structured:getCurrentBranch()delegates to it, maintaining backward compatibility - ✅ Good test coverage across
main,master, custom branch name, and non-git-directory error fallback - ✅ Pre-existing missing
cmd.Dirongit pullwas caught and fixed as part of this PR - ✅ PR description is thorough and includes the helper implementation inline
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 65.3 AIC · ⌖ 8.08 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/trial_helpers.go:370
[/diagnosing-bugs] Silent fallback to "main" can produce a confusing downstream error in non-main repos if branch detection ever fails — the real cause is masked by the subsequent push failure.
<details>
<summary>💡 Consider propagating the error instead</summary>
Since tempDir is always a fresh clone, getCurrentBranchIn failing here is anomalous. Propagating the error gives the user an actionable message rather than a silent wrong-branch push:
branch, err := getCurrentBran…
</details>
<details><summary>pkg/cli/trial_repository.go:528</summary>
**[/diagnosing-bugs]** Same silent-fallback issue as in `trial_helpers.go` — `getCurrentBranchIn` failure silently falls through to pushing to `main`, which may silently corrupt the wrong branch in non-`main` repos.
<details>
<summary>💡 Suggestion</summary>
For consistency and correctness, apply the same error-propagation approach in both callers. If a fallback is intentional, at least emit a visible `fmt.Fprintln(os.Stderr, ...)` warning so users aren't debugging a mysterious push to `main`…
</details>
<details><summary>pkg/cli/trial_helpers.go:362</summary>
**[/diagnosing-bugs]** The `git commit` command at line 362 runs without `cmd.Dir = tempDir`, even though the function already calls `os.Chdir(tempDir)` — this is a latent correctness risk. If the `chdir` is ever removed, the commit will silently target the wrong repo.
<details>
<summary>💡 Add cmd.Dir defensively</summary>
```go
cmd = exec.Command("git", "commit", "-m", commitMsg)
cmd.Dir = tempDirThe PR already adds cmd.Dir to pull and push in this function; applying the same patter…
pkg/cli/trial_repository_test.go:83
[/tdd] git symbolic-ref HEAD refs/heads/<branch> sets the branch but does NOT create a commit. On some Git versions, git branch --show-current may return empty for an initial state with no commits yet.
<details>
<summary>💡 Consider adding an initial commit before asserting branch name</summary>
The test does call git commit --allow-empty -m init which should be sufficient, but note the ordering: symbolic-ref sets the branch name, then commit creates the first commit. This shoul…
pkg/cli/git.go:468
[/diagnosing-bugs] git branch --show-current returns an empty string on detached HEAD (not an error). The current implementation converts that to an error via the branch == "" check — but the error message "could not determine current branch" doesn't mention detached HEAD, which will confuse users during git bisect or CI checkout scenarios.
<details>
<summary>💡 Improve the error message</summary>
if branch == "" {
return "", errors.New("could not determine current branc…
</details>There was a problem hiding this comment.
REQUEST_CHANGES — The core mechanism works correctly (good refactor of getLocalBranch into getCurrentBranchIn reusing the existing helper), but both callers have a silent-fallback-to-"main" pattern that reintroduces the bug this PR is fixing when branch detection fails.
Blocking themes
Silent "main" fallback in both callers (high)
Both commitAndPushWorkflow (trial_repository.go:528) and copyTrialResultsToHostRepo (trial_helpers.go:370) swallow any error from getCurrentBranchIn and proceed with branch = "main". This means any failure in branch detection — including the detached HEAD path where git branch --show-current exits 0 with empty output — will silently push to origin main, logging a green success for the user while targeting the wrong branch. The original bug is only fixed when detection succeeds; when it fails, the regression is silent and indistinguishable from a correct run.
Fix: propagate the error instead of swallowing it.
Missing detached HEAD test case (medium)
The test covers the non-git-directory exit-code-nonzero path but not the detached HEAD path where git branch --show-current exits 0 with empty output. That empty-string check is a separate code path inside getCurrentBranchIn and is the most operationally relevant failure mode for the caller fallback chain.
🔎 Code quality review by PR Code Quality Reviewer · 120.1 AIC · ⌖ 7.88 AIC · ⊞ 5.4K
Comment /review to run again
| branch, err := getCurrentBranchIn(tempDir) | ||
| if err != nil { | ||
| trialLog.Printf("Failed to detect branch in %s: %v, falling back to main", tempDir, err) | ||
| branch = "main" |
There was a problem hiding this comment.
Silent "main" fallback reintroduces the exact bug this PR is fixing when branch detection fails.
💡 Details and suggested fix
If getCurrentBranchIn returns an error — including the detached HEAD path where git branch --show-current exits 0 with empty output — execution silently falls through to branch = "main". The error is only logged at debug level; the function then proceeds to git push origin main, silently pushing to the wrong branch in non-main-default repos.
The tempDir is 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 misleading SUCCESS log for the user.
Suggested approach: propagate the error rather than swallowing it:
branch, err := getCurrentBranchIn(tempDir)
if err != nil {
return fmt.Errorf("failed to detect branch for push in %s: %w", tempDir, err)
}If a graceful fallback is truly desired, at minimum surface it to the user via console.FormatWarningMessage rather than a silent debug log, so operators know the wrong branch may be in use.
| branch, err := getCurrentBranchIn(tempDir) | ||
| if err != nil { | ||
| trialRepoLog.Printf("Failed to detect branch in %s: %v, falling back to main", tempDir, err) | ||
| branch = "main" |
There was a problem hiding this comment.
Same silent "main" fallback here — identical correctness risk as in trial_helpers.go.
💡 Details and suggested fix
When getCurrentBranchIn fails (including an empty-output detached HEAD result), branch is silently set to "main" and git push origin main proceeds without any user-visible warning. On a repo whose default branch is trunk or develop, this pushes workflow files to the wrong branch and logs a green success message.
Because tempDir is a freshly cloned host repo, detached HEAD is unlikely in the happy path, but the silent recovery behavior makes failures invisible. The caller of commitAndPushWorkflow propagates its error, so failing loudly here is the right call:
branch, err := getCurrentBranchIn(tempDir)
if err != nil {
return fmt.Errorf("failed to detect branch for push in %s: %w", tempDir, err)
}| dir := t.TempDir() | ||
| _, err := getCurrentBranchIn(dir) | ||
| if err == nil { | ||
| t.Fatal("getCurrentBranchIn() expected an error for non-git directory, got nil") |
There was a problem hiding this comment.
Test covers non-git-directory error but not detached HEAD, which is the primary trigger for the branch="main" fallback in both callers.
💡 Suggested additional test case
git branch --show-current returns exit code 0 with empty output in a detached HEAD state. getCurrentBranchIn converts that to an errors.New("could not determine current branch") — the separate empty-string check — but this path is completely untested.
This matters because both commitAndPushWorkflow and copyTrialResultsToHostRepo fall back to "main" on any error, and the detached HEAD path is the most operationally relevant way that error arises. Without this test, the test suite gives false confidence that error handling for branch detection is complete.
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")
}
})|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Addressed all outstanding reviewer feedback in b37a1d1:
Local |
|
|
|
🎉 This pull request is included in a new release. Release: |
gh aw trial --host-repo=.fails immediately in repos whose default branch is notmain, because bothcommitAndPushWorkflowandcopyTrialResultsToHostRepounconditionally rungit pull/push origin main.Changes
pkg/cli/trial_repository.go: AddsgetLocalBranch(dir string) stringhelper that reads the actual checked-out branch viagit rev-parse --abbrev-ref HEAD, falling back to"main"with debug logging on failure or detached HEAD. UpdatescommitAndPushWorkflowto use it; also adds the missingcmd.Dir = tempDiron thegit pullcall (pre-existing inconsistency).pkg/cli/trial_helpers.go: UpdatescopyTrialResultsToHostRepoto usegetLocalBranch; adds explicitcmd.Dir = tempDirto both pull and push commands.pkg/cli/trial_repository_test.go: AddsTestGetLocalBranchcoveringmain,master, a custom branch name, and the non-git-directory fallback.Since
tempDiris always a fresh clone of the host repo,HEADreliably reflects the repo's default branch.