From da4e460102d65a21571fa9b4df5077d20eeebd6c Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 2 Apr 2026 20:28:47 -0700 Subject: [PATCH 1/4] fix: clear stale GH_HOST and remove false fork PR detection 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> --- actions/setup/js/pr_helpers.cjs | 36 ++++++++++++------------ actions/setup/js/pr_helpers.test.cjs | 19 +++++++------ actions/setup/sh/configure_gh_for_ghe.sh | 12 +++++++- 3 files changed, 39 insertions(+), 28 deletions(-) diff --git a/actions/setup/js/pr_helpers.cjs b/actions/setup/js/pr_helpers.cjs index 039b5bf97f3..3b48792d76c 100644 --- a/actions/setup/js/pr_helpers.cjs +++ b/actions/setup/js/pr_helpers.cjs @@ -3,33 +3,33 @@ /** * Detect if a pull request is from a fork repository. * - * Uses multiple signals for robust detection: - * 1. Check if head.repo.fork is explicitly true (GitHub's fork flag) - * 2. Compare repository full names if both repos exist - * 3. Handle deleted fork case (head.repo is null) + * A "fork PR" means the head and base are in *different* repositories + * (cross-repo PR). Detection uses two signals: + * 1. Handle deleted fork case (head.repo is null) + * 2. Compare repository full names — different names mean cross-repo + * + * 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 + * `git fetch`, which then failed due to stale GH_HOST values. See #24208. * * @param {object} pullRequest - The pull request object from GitHub context * @returns {{isFork: boolean, reason: string}} Fork detection result with reason */ function detectForkPR(pullRequest) { - let isFork = false; - let reason = "same repository"; - if (!pullRequest.head?.repo) { // Head repo is null - likely a deleted fork - isFork = true; - reason = "head repository deleted (was likely a fork)"; - } else if (pullRequest.head.repo.fork === true) { - // GitHub's explicit fork flag - isFork = true; - reason = "head.repo.fork flag is true"; - } else if (pullRequest.head.repo.full_name !== pullRequest.base?.repo?.full_name) { - // Different repository names - isFork = true; - reason = "different repository names"; + return { isFork: true, reason: "head repository deleted (was likely a fork)" }; + } + + if (pullRequest.head.repo.full_name !== pullRequest.base?.repo?.full_name) { + // Different repository names — this is a cross-repo (fork) PR + return { isFork: true, reason: "different repository names" }; } - return { isFork, reason }; + return { isFork: false, reason: "same repository" }; } /** diff --git a/actions/setup/js/pr_helpers.test.cjs b/actions/setup/js/pr_helpers.test.cjs index 05548a79db5..352958c4a72 100644 --- a/actions/setup/js/pr_helpers.test.cjs +++ b/actions/setup/js/pr_helpers.test.cjs @@ -12,7 +12,9 @@ describe("pr_helpers.cjs", () => { }); describe("detectForkPR", () => { - it("should detect fork using GitHub's fork flag", () => { + it("should NOT treat same-repo PR as fork even when repo has fork flag", () => { + // A repository that is itself a fork of another repo has fork=true, + // but a same-repo PR within it is NOT a cross-repo fork PR (#24208) const pullRequest = { head: { repo: { @@ -29,8 +31,8 @@ describe("pr_helpers.cjs", () => { const result = detectForkPR(pullRequest); - expect(result.isFork).toBe(true); - expect(result.reason).toBe("head.repo.fork flag is true"); + expect(result.isFork).toBe(false); + expect(result.reason).toBe("same repository"); }); it("should detect fork using different repository names", () => { @@ -114,19 +116,18 @@ describe("pr_helpers.cjs", () => { expect(result.reason).toBe("same repository"); }); - it("should prioritize fork flag over repository name comparison", () => { - // Edge case: fork flag is true even though names match - // This could happen if a user forks and keeps the same name + it("should detect cross-repo fork PR by different full_name", () => { + // A real fork PR: head is in a different repo than base const pullRequest = { head: { repo: { fork: true, - full_name: "test-owner/test-repo", + full_name: "contributor/test-repo", }, }, base: { repo: { - full_name: "test-owner/test-repo", + full_name: "upstream/test-repo", }, }, }; @@ -134,7 +135,7 @@ describe("pr_helpers.cjs", () => { const result = detectForkPR(pullRequest); expect(result.isFork).toBe(true); - expect(result.reason).toBe("head.repo.fork flag is true"); + expect(result.reason).toBe("different repository names"); }); it("should handle null base repo gracefully", () => { diff --git a/actions/setup/sh/configure_gh_for_ghe.sh b/actions/setup/sh/configure_gh_for_ghe.sh index 88e90b6e954..ef42f8c477e 100755 --- a/actions/setup/sh/configure_gh_for_ghe.sh +++ b/actions/setup/sh/configure_gh_for_ghe.sh @@ -62,8 +62,18 @@ main() { local github_host github_host=$(detect_github_host) - # If the host is github.com, no configuration is needed + # If the host is github.com, no GHE configuration is needed. + # However, we must clear any stale GH_HOST value to prevent gh CLI + # from targeting the wrong host (e.g., a leftover localhost:18443 + # from a prior DIFC proxy run). See #24208. if [ "$github_host" = "github.com" ]; then + if [ -n "${GH_HOST:-}" ] && [ "${GH_HOST}" != "github.com" ]; then + echo "Clearing stale GH_HOST=${GH_HOST} (expected github.com)" + unset GH_HOST + if [ -n "${GITHUB_ENV:-}" ]; then + echo "GH_HOST=github.com" >> "${GITHUB_ENV}" + fi + fi echo "Using public GitHub (github.com) - no additional gh configuration needed" return 0 fi From 22123934dd6b12b94732bcd3fb7b5827c761c3b7 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 2 Apr 2026 20:37:31 -0700 Subject: [PATCH 2/4] test: update push_to_pr_branch test for corrected fork detection 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> --- .../setup/js/push_to_pull_request_branch.test.cjs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/actions/setup/js/push_to_pull_request_branch.test.cjs b/actions/setup/js/push_to_pull_request_branch.test.cjs index de63c54b8a8..0af76a13327 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -435,7 +435,9 @@ index 0000000..abc1234 expect(mockExec.exec).not.toHaveBeenCalled(); }); - it("should detect fork PR via fork flag and fail early", async () => { + it("should NOT treat same-repo PR as fork even when repo has fork flag", async () => { + // A repository that is itself a fork of another repo has fork=true, + // but a same-repo PR within it is NOT a cross-repo fork PR (#24208) mockContext.payload.pull_request.head.repo.fork = true; mockGithub.rest.pulls.get.mockResolvedValue({ @@ -452,7 +454,7 @@ index 0000000..abc1234 full_name: "test-owner/test-repo", }, }, - title: "Fork PR", + title: "Same-repo PR in forked repo", labels: [], }, }); @@ -463,10 +465,8 @@ index 0000000..abc1234 const handler = await module.main({ target: "triggering" }); const result = await handler({ patch_path: patchPath }, {}); - // Fork PRs should fail early with a clear error - expect(result.success).toBe(false); - expect(result.error).toContain("fork"); - expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("Cannot push to fork PR")); + // Same full_name means same repo — push should proceed, not fail + expect(result.success).toBe(true); }); it("should handle deleted head repo (likely a fork) and fail early", async () => { From 0b497291189726952b02a60ca7b21c528977fef0 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 2 Apr 2026 20:41:31 -0700 Subject: [PATCH 3/4] test: update checkout_pr_branch tests for corrected fork detection 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> --- actions/setup/js/checkout_pr_branch.test.cjs | 52 +++++++++----------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/actions/setup/js/checkout_pr_branch.test.cjs b/actions/setup/js/checkout_pr_branch.test.cjs index 7899e786b5a..87f13b4455f 100644 --- a/actions/setup/js/checkout_pr_branch.test.cjs +++ b/actions/setup/js/checkout_pr_branch.test.cjs @@ -106,21 +106,13 @@ describe("checkout_pr_branch.cjs", () => { return { detectForkPR: pullRequest => { // Replicate the actual logic for testing - let isFork = false; - let reason = "same repository"; - if (!pullRequest.head?.repo) { - isFork = true; - reason = "head repository deleted (was likely a fork)"; - } else if (pullRequest.head.repo.fork === true) { - isFork = true; - reason = "head.repo.fork flag is true"; - } else if (pullRequest.head.repo.full_name !== pullRequest.base?.repo?.full_name) { - isFork = true; - reason = "different repository names"; + return { isFork: true, reason: "head repository deleted (was likely a fork)" }; } - - return { isFork, reason }; + if (pullRequest.head.repo.full_name !== pullRequest.base?.repo?.full_name) { + return { isFork: true, reason: "different repository names" }; + } + return { isFork: false, reason: "same repository" }; }, }; } @@ -255,21 +247,20 @@ If the pull request is still open, verify that: expect(mockCore.setFailed).not.toHaveBeenCalled(); }); - it("should use gh pr checkout for fork PR with fork flag set", async () => { - // Set up fork PR via the fork flag + it("should use git fetch for same-repo PR even when repo has fork flag", async () => { + // A repo that is itself a fork has fork=true, but same-repo PRs + // should still use fast git fetch, not gh pr checkout (#24208) mockContext.payload.pull_request.head.repo.fork = true; await runScript(); - // Verify fork is detected via fork flag - expect(mockCore.info).toHaveBeenCalledWith("Is fork PR: true (head.repo.fork flag is true)"); - - // Verify correct strategy reason for fork PR - expect(mockCore.info).toHaveBeenCalledWith("Reason: pull_request event from fork repository; head branch exists only in fork, not in origin"); + // Verify NOT detected as fork (same full_name) + expect(mockCore.info).toHaveBeenCalledWith("Is fork PR: false (same repository)"); - // Verify gh pr checkout is used - expect(mockExec.exec).toHaveBeenCalledWith("gh", ["pr", "checkout", "123"]); - expect(mockExec.exec).not.toHaveBeenCalledWith("git", ["fetch", "origin", "feature-branch", expect.anything()]); + // Verify git fetch + checkout is used (fast path) + expect(mockExec.exec).toHaveBeenCalledWith("git", ["fetch", "origin", "feature-branch", "--depth=2"]); + expect(mockExec.exec).toHaveBeenCalledWith("git", ["checkout", "feature-branch"]); + expect(mockExec.exec).not.toHaveBeenCalledWith("gh", ["pr", "checkout", "123"]); expect(mockCore.setFailed).not.toHaveBeenCalled(); }); @@ -511,17 +502,20 @@ If the pull request is still open, verify that: expect(mockExec.exec).toHaveBeenCalledWith("gh", ["pr", "checkout", "123"]); }); - it("should detect fork using GitHub's fork flag", async () => { + it("should NOT detect fork when repo has fork flag but same full_name", async () => { mockContext.eventName = "pull_request_target"; - // Set fork flag explicitly + // A repo that is itself a fork has fork=true, but same full_name + // means it's NOT a cross-repo fork PR (#24208) mockContext.payload.pull_request.head.repo.fork = true; - mockContext.payload.pull_request.head.repo.full_name = "test-owner/test-repo"; // Same name but forked + mockContext.payload.pull_request.head.repo.full_name = "test-owner/test-repo"; await runScript(); - // Verify fork detection using fork flag - expect(mockCore.info).toHaveBeenCalledWith("Is fork PR: true (head.repo.fork flag is true)"); - expect(mockCore.warning).toHaveBeenCalledWith("⚠️ Fork PR detected - gh pr checkout will fetch from fork repository"); + // Same full_name = not a fork PR + expect(mockCore.info).toHaveBeenCalledWith("Is fork PR: false (same repository)"); + expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("Fork PR detected")); + // Still uses gh pr checkout because pull_request_target always does + expect(mockExec.exec).toHaveBeenCalledWith("gh", ["pr", "checkout", "123"]); }); it("should detect non-fork PRs in pull_request_target events", async () => { From 5482ca21fa9e7fe1f665c41f71b12d69d8ef7ec1 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 2 Apr 2026 20:46:34 -0700 Subject: [PATCH 4/4] test: fix check_workflow_timestamp_api tests for updated error messages 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> --- .../setup/js/check_workflow_timestamp_api.test.cjs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/actions/setup/js/check_workflow_timestamp_api.test.cjs b/actions/setup/js/check_workflow_timestamp_api.test.cjs index 7a4d227e0a4..f9a1e50ca37 100644 --- a/actions/setup/js/check_workflow_timestamp_api.test.cjs +++ b/actions/setup/js/check_workflow_timestamp_api.test.cjs @@ -234,7 +234,7 @@ jobs: await main(); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not compare frontmatter hashes")); - expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("integrity check failed")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("is outdated")); expect(mockCore.summary.addRaw).toHaveBeenCalled(); expect(mockCore.summary.write).toHaveBeenCalled(); }); @@ -245,7 +245,7 @@ jobs: await main(); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not compare frontmatter hashes")); - expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("integrity check failed")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("is outdated")); expect(mockCore.summary.addRaw).toHaveBeenCalled(); expect(mockCore.summary.write).toHaveBeenCalled(); }); @@ -347,7 +347,7 @@ engine: claude expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Unable to fetch lock file content")); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not compare frontmatter hashes")); - expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("integrity check failed")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("is outdated")); }); }); @@ -699,7 +699,7 @@ engine: copilot expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Unable to fetch lock file content for hash comparison via API")); expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("GITHUB_WORKSPACE not available")); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not compare frontmatter hashes")); - expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("integrity check failed")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("is outdated")); }); it("should fail when API fails and local lock file is missing", async () => { @@ -711,7 +711,7 @@ engine: copilot expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Local lock file not found")); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not compare frontmatter hashes")); - expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("integrity check failed")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("is outdated")); }); it("should use API if available even in cross-repo scenario (API preferred over local files)", async () => { @@ -784,7 +784,7 @@ engine: copilot // The path traversal is rejected before any file read expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("escapes workspace")); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not compare frontmatter hashes")); - expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("integrity check failed")); + expect(mockCore.setFailed).toHaveBeenCalledWith(expect.stringContaining("is outdated")); }); }); });