fix: Clear stale GH_HOST and remove false fork PR detection (#24208)#24221
Merged
fix: Clear stale GH_HOST and remove false fork PR detection (#24208)#24221
Conversation
Two bugs interacted to cause PR checkout failures on github.com repos: 1. configure_gh_for_ghe.sh: For github.com, the script returned early without clearing GH_HOST. A stale value (e.g., localhost:18443 from a prior DIFC proxy run) persisted, causing 'gh pr checkout' to fail with 'none of the git remotes correspond to GH_HOST'. Now explicitly unsets stale GH_HOST values on the github.com path. 2. pr_helpers.cjs: detectForkPR() checked head.repo.fork to detect fork PRs, but this flag indicates whether the *repository itself* is a fork (common in OSS), not whether the PR is cross-repo. This caused false positives that forced the slow 'gh pr checkout' path (which depends on GH_HOST) instead of fast 'git fetch' (which doesn't). Removed the fork flag check; now only uses full_name comparison. Fixes #24208 Closes #24217 Closes #24218 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes PR checkout failures caused by stale GH_HOST values and incorrect “fork PR” detection logic that could force the gh pr checkout path.
Changes:
- Update
configure_gh_for_ghe.shto clear staleGH_HOSTwhen running againstgithub.com, and persistGH_HOST=github.comviaGITHUB_ENV. - Simplify
detectForkPR()to rely onhead.repopresence andhead.repo.full_namevsbase.repo.full_name, removing the incorrecthead.repo.forksignal. - Adjust/add unit tests in
pr_helpers.test.cjsto validate the updated fork detection behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| actions/setup/sh/configure_gh_for_ghe.sh | Clears stale GH_HOST when targeting github.com to prevent gh host/remote mismatch errors. |
| actions/setup/js/pr_helpers.cjs | Removes incorrect fork-flag logic; fork PR detection now uses cross-repo full_name comparison. |
| actions/setup/js/pr_helpers.test.cjs | Updates tests to reflect the new fork PR detection semantics and adds a cross-repo case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+11
to
+15
| * NOTE: We intentionally do NOT check head.repo.fork. That flag indicates | ||
| * whether the repository *itself* is a fork of another repo, not whether | ||
| * the PR is cross-repo. A same-repo PR in a forked repository (common in | ||
| * OSS) would have fork=true but is NOT a cross-repo fork PR. Using that | ||
| * flag caused false positives that forced `gh pr checkout` instead of fast |
There was a problem hiding this comment.
detectForkPR() no longer uses head.repo.fork and will never return the reason "head.repo.fork flag is true", but actions/setup/js/checkout_pr_branch.test.cjs currently mocks ./pr_helpers.cjs with the old fork-flag-based logic and asserts against that reason. This makes the checkout tests inconsistent with the production implementation and reduces coverage for the bug this PR is fixing. Update those tests to use the real detectForkPR (preferred) or at least mirror the new full_name-comparison logic and expected reasons.
The test 'should detect fork PR via fork flag and fail early' assumed that head.repo.fork=true with same full_name meant a cross-repo fork PR. After fixing detectForkPR() to only use full_name comparison (#24208), same-repo PRs in forked repositories are correctly treated as non-fork, allowing push to proceed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The mock detectForkPR() in checkout_pr_branch.test.cjs replicated the old fork-flag-based logic, making tests inconsistent with production. Updated the mock to match the corrected full_name-only comparison and updated assertions to verify that same-repo PRs in forked repositories use the fast git fetch path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests asserted 'integrity check failed' but the error message was changed to 'is outdated or unverifiable' in #24198. Updated all 6 assertions to match the current production error format. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the PR checkout failure reported in #24208 by addressing two interacting bugs.
Changes
1. Clear stale GH_HOST on github.com (
configure_gh_for_ghe.sh)The script returned early for github.com without touching
GH_HOST. If a stale value was present (e.g.,localhost:18443from a prior DIFC proxy), it persisted and causedgh pr checkoutto fail with:Fix: Explicitly unset
GH_HOSTwhen it has a stale non-github.com value, and writeGH_HOST=github.comtoGITHUB_ENVfor downstream steps.2. Remove false fork PR detection (
pr_helpers.cjs)detectForkPR()checkedhead.repo.fork === trueto detect fork PRs. But this flag indicates whether the repository itself is a fork of another repo — not whether the PR is cross-repo. For same-repo PRs in forked repositories (common in OSS likefsprojects/FSharp.Control.AsyncSeq), this produced a false positive that forced thegh pr checkoutpath (sensitive to GH_HOST) instead of the fastgit fetchpath (not sensitive).Fix: Removed the
head.repo.forkcheck. Fork PR detection now relies solely on comparinghead.repo.full_namevsbase.repo.full_name, which correctly identifies cross-repo PRs.Why both bugs were needed
GH_HOSTthat breaksgh pr checkoutgh pr checkout(which depends onGH_HOST) instead ofgit fetch(which doesn't)Fixing either independently prevents the failure.
Testing
pr_helpers.test.cjs— all 24 tests passshould NOT treat same-repo PR as fork even when repo has fork flagvalidates the core fixshould detect cross-repo fork PR by different full_nameensures real fork PRs still detectedFixes #24208
Closes #24217
Closes #24218