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")); }); }); }); 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 () => { 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/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 () => { 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