Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions actions/setup/js/check_workflow_timestamp_api.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand All @@ -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();
});
Expand Down Expand Up @@ -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"));
});
});

Expand Down Expand Up @@ -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 () => {
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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"));
});
});
});
52 changes: 23 additions & 29 deletions actions/setup/js/checkout_pr_branch.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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" };
},
};
}
Expand Down Expand Up @@ -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();
});
Expand Down Expand Up @@ -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 () => {
Expand Down
36 changes: 18 additions & 18 deletions actions/setup/js/pr_helpers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +11 to +15
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
* `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" };
}

/**
Expand Down
19 changes: 10 additions & 9 deletions actions/setup/js/pr_helpers.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -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", () => {
Expand Down Expand Up @@ -114,27 +116,26 @@ 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",
},
},
};

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", () => {
Expand Down
12 changes: 6 additions & 6 deletions actions/setup/js/push_to_pull_request_branch.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -452,7 +454,7 @@ index 0000000..abc1234
full_name: "test-owner/test-repo",
},
},
title: "Fork PR",
title: "Same-repo PR in forked repo",
labels: [],
},
});
Expand All @@ -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 () => {
Expand Down
12 changes: 11 additions & 1 deletion actions/setup/sh/configure_gh_for_ghe.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading