diff --git a/tools/ods/cmd/run-ci.go b/tools/ods/cmd/run-ci.go index 6687c5f373..815d393bf9 100644 --- a/tools/ods/cmd/run-ci.go +++ b/tools/ods/cmd/run-ci.go @@ -109,6 +109,7 @@ type PRInfo struct { Title string `json:"title"` Body string `json:"body"` HeadRefName string `json:"headRefName"` + HeadRefOid string `json:"headRefOid"` HeadRepository struct { Name string `json:"name"` } `json:"headRepository"` @@ -293,7 +294,7 @@ func runCI(prNumber string, opts *RunCIOptions) error { if existingPRURL != "" { action = "Update existing CI branch" } - if !prompt.Confirm(fmt.Sprintf("%s for PR #%s? (yes/no): ", action, prNumber)) { + if !prompt.Confirm(fmt.Sprintf("%s for PR #%s (%s:%s @ %s)? (yes/no): ", action, prNumber, forkRepo, prInfo.HeadRefName, prInfo.HeadRefOid)) { log.Info("Exiting...") return nil } @@ -309,7 +310,15 @@ func runCI(prNumber string, opts *RunCIOptions) error { return fatalErrorf("Failed to fetch fork branch: %w", err) } - // Create or update the CI branch from FETCH_HEAD + fetchedSHA, err := git.ResolveCommit("FETCH_HEAD") + if err != nil { + return fatalErrorf("Failed to resolve FETCH_HEAD: %w", err) + } + if fetchedSHA != prInfo.HeadRefOid { + return fatalErrorf("Fork branch moved since PR #%s was read: fetched %s, expected %s. Re-run to review the new head", prNumber, fetchedSHA, prInfo.HeadRefOid) + } + + // Create or update the CI branch from the confirmed commit if originalBranch == ciBranch { // Already on the CI branch - stash any uncommitted changes before resetting stashResult, err := git.StashChanges() @@ -317,7 +326,7 @@ func runCI(prNumber string, opts *RunCIOptions) error { return fatalErrorf("Failed to stash changes: %w", err) } log.Infof("Already on %s, resetting to fork's HEAD", ciBranch) - if err := git.RunCommand("reset", "--hard", "FETCH_HEAD"); err != nil { + if err := git.RunCommand("reset", "--hard", fetchedSHA); err != nil { return fatalErrorf("Failed to reset branch to fork's HEAD: %w", err) } git.RestoreStash(stashResult) @@ -330,7 +339,7 @@ func runCI(prNumber string, opts *RunCIOptions) error { } } log.Infof("Creating CI branch: %s", ciBranch) - if err := git.RunCommand("checkout", "--quiet", "-b", ciBranch, "FETCH_HEAD"); err != nil { + if err := git.RunCommand("checkout", "--quiet", "-b", ciBranch, fetchedSHA); err != nil { return fatalErrorf("Failed to create CI branch: %w", err) } } @@ -360,7 +369,7 @@ func runCI(prNumber string, opts *RunCIOptions) error { } else { pushArgs = append(pushArgs, "-f") } - pushArgs = append(pushArgs, "--quiet", "-u", "origin", ciBranch) + pushArgs = append(pushArgs, "--quiet", "origin", fetchedSHA+":refs/heads/"+ciBranch) if err := pushWithHookHint(opts.NoVerify, func() error { return git.RunCommand(pushArgs...) }); err != nil { // Switch back to original branch before exiting if switchErr := git.RunCommand("switch", "--quiet", originalBranch); switchErr != nil { @@ -369,6 +378,15 @@ func runCI(prNumber string, opts *RunCIOptions) error { return fatalErrorf("Failed to push CI branch: %w", err) } + for _, setting := range [][2]string{{"remote", "origin"}, {"merge", "refs/heads/" + ciBranch}} { + if err := git.RunCommand("config", "branch."+ciBranch+"."+setting[0], setting[1]); err != nil { + if switchErr := git.RunCommand("switch", "--quiet", originalBranch); switchErr != nil { + log.Warnf("Failed to switch back to original branch: %v", switchErr) + } + return fatalErrorf("Failed to set CI branch upstream %s: %w", setting[0], err) + } + } + if existingPRURL != "" { // PR already exists - force push is enough to re-trigger CI log.Infof("Switching back to original branch: %s", originalBranch) @@ -409,7 +427,7 @@ func runCI(prNumber string, opts *RunCIOptions) error { // getPRInfo fetches PR information using the GitHub CLI func getPRInfo(prNumber string) (*PRInfo, error) { cmd := exec.Command("gh", "pr", "view", prNumber, - "--json", "number,title,body,headRefName,headRepository,headRepositoryOwner,baseRefName,isCrossRepository") + "--json", "number,title,body,headRefName,headRefOid,headRepository,headRepositoryOwner,baseRefName,isCrossRepository") output, err := cmd.Output() if err != nil { if exitErr, ok := err.(*exec.ExitError); ok { diff --git a/tools/ods/cmd/run-ci_test.go b/tools/ods/cmd/run-ci_test.go index 7936e1076a..be118bc2a5 100644 --- a/tools/ods/cmd/run-ci_test.go +++ b/tools/ods/cmd/run-ci_test.go @@ -14,6 +14,7 @@ package cmd // R6 gh, fork, or push failures -> errors, back on the original TestRunCI_failures import ( + "fmt" "os" "path/filepath" "strings" @@ -24,8 +25,8 @@ import ( const ( gitrelEditableBranch = "chore/feature-pr-7353-edits" - gitrelForkPRJSON = `{"number":7353,"title":"feat: from a fork","body":"PR body","headRefName":"feature","headRepository":{"name":"onyx"},"headRepositoryOwner":{"login":"alice"},"baseRefName":"main","isCrossRepository":true}` - gitrelPRViewArm = `"pr view 7353 --json number,title,body,headRefName,headRepository,headRepositoryOwner,baseRefName,isCrossRepository") echo '` + gitrelForkPRJSON + `' ;;` + gitrelForkPRJSON = `{"number":7353,"title":"feat: from a fork","body":"PR body","headRefName":"feature","headRefOid":"'"$GITREL_FORK_SHA"'","headRepository":{"name":"onyx"},"headRepositoryOwner":{"login":"alice"},"baseRefName":"main","isCrossRepository":true}` + gitrelPRViewArm = `"pr view 7353 --json number,title,body,headRefName,headRefOid,headRepository,headRepositoryOwner,baseRefName,isCrossRepository") echo '` + gitrelForkPRJSON + `' ;;` gitrelNoCIPRArm = `"pr list --head run-ci/7353 --state open --limit 1 --json url") echo '[]' ;;` gitrelCIPRArm = `"pr list --head run-ci/7353 --state open --limit 1 --json url") echo '[{"url":"https://github.com/onyx-dot-app/onyx/pull/8000"}]' ;;` gitrelNoEditablePRArm = `"pr list --head ` + gitrelEditableBranch + ` --state all --limit 1 --json url") echo '[]' ;;` @@ -60,6 +61,7 @@ func gitrelSetupRunCIRepo(t *testing.T) gitrelRunCIRepo { // Refuse any transport but local paths, should the rewrite ever miss. t.Setenv("GIT_ALLOW_PROTOCOL", "file") t.Chdir(work) + t.Setenv("GITREL_FORK_SHA", forkSHA) return gitrelRunCIRepo{Origin: origin, Work: work, ForkSHA: forkSHA} } @@ -78,6 +80,12 @@ func TestRunCI_createsBranchAndPR(t *testing.T) { if tip := gittest.Git(t, repo.Origin, "rev-parse", "refs/heads/run-ci/7353"); tip != repo.ForkSHA { t.Errorf("expected origin run-ci/7353 at the fork head %s, got %s", repo.ForkSHA, tip) } + if remote := gittest.Git(t, repo.Work, "config", "branch.run-ci/7353.remote"); remote != "origin" { + t.Errorf("expected CI branch remote origin, got %q", remote) + } + if merge := gittest.Git(t, repo.Work, "config", "branch.run-ci/7353.merge"); merge != "refs/heads/run-ci/7353" { + t.Errorf("expected CI branch merge ref, got %q", merge) + } creates := gitrelCallsWithPrefix(calls(), "pr", "create") if len(creates) != 1 { t.Fatalf("expected one gh pr create call, got %q", creates) @@ -316,6 +324,15 @@ func TestRunCI_failures(t *testing.T) { }, wantErr: "Failed to push CI branch: exit status 1", }, + { + name: "upstream config write fails", + ghArms: gitrelPRViewArm + "\n" + gitrelNoCIPRArm + "\n" + gitrelCIPRCreate, + setup: func(t *testing.T, repo gitrelRunCIRepo) { + // A held config lock makes the post-push tracking write fail. + gitrelHooks(t, repo.Work, map[string]string{"pre-push": "#!/bin/sh\ntouch \"$(git rev-parse --git-dir)/config.lock\"\n"}) + }, + wantErr: "Failed to set CI branch upstream remote: ", + }, { name: "PR creation fails", ghArms: gitrelPRViewArm + "\n" + gitrelNoCIPRArm + "\n" + `"pr create "*) echo 'base branch not found' >&2; exit 1 ;;`, @@ -344,3 +361,39 @@ func TestRunCI_failures(t *testing.T) { }) } } + +func TestRunCI_rejectsChangedForkHead(t *testing.T) { + for _, onCI := range []bool{false, true} { + t.Run(fmt.Sprintf("onCI=%v", onCI), func(t *testing.T) { + repo := gitrelSetupRunCIRepo(t) + if onCI { + gittest.Git(t, repo.Work, "checkout", "--quiet", "-b", "run-ci/7353") + } + before := gittest.Git(t, repo.Work, "rev-parse", "HEAD") + t.Setenv("GITREL_FORK_SHA", before) + gitrelFakeGH(t, gitrelPRViewArm+"\n"+gitrelNoCIPRArm+"\n"+gitrelCIPRCreate) + err := runCI("7353", &RunCIOptions{Yes: true}) + if err == nil || !strings.Contains(err.Error(), "Fork branch moved") { + t.Fatalf("expected changed-head rejection, got %v", err) + } + if head := gittest.Git(t, repo.Work, "rev-parse", "HEAD"); head != before { + t.Fatalf("local head changed: %s", head) + } + if gitrelRefExists(repo.Origin, "refs/heads/run-ci/7353") { + t.Fatal("unconfirmed commit was pushed") + } + }) + } +} + +func TestRunCI_pushesConfirmedCommitAfterLocalBranchMoves(t *testing.T) { + repo := gitrelSetupRunCIRepo(t) + gitrelFakeGH(t, gitrelPRViewArm+"\n"+gitrelNoCIPRArm+"\n"+gitrelCIPRCreate) + gitrelHooks(t, repo.Work, map[string]string{"post-checkout": "#!/bin/sh\nif [ \"$(git branch --show-current)\" = run-ci/7353 ]; then git reset --hard main; fi\n"}) + if err := runCI("7353", &RunCIOptions{Yes: true}); err != nil { + t.Fatal(err) + } + if tip := gittest.Git(t, repo.Origin, "rev-parse", "refs/heads/run-ci/7353"); tip != repo.ForkSHA { + t.Fatalf("pushed mutable branch instead of confirmed commit: got %s, want %s", tip, repo.ForkSHA) + } +}