Add extensive logging and robust fork detection to PR checkout logic#14443
Add extensive logging and robust fork detection to PR checkout logic#14443
Conversation
- Add comprehensive logging for debugging PR checkout issues - Detect fork PRs vs same-repo PRs with clear warnings - Log detailed PR context (head/base refs, repos, SHAs) - Document differences between pull_request and pull_request_target events - Add checkout strategy logging with reasoning - Enhanced error logging with git diagnostics - Update tests to cover new logging scenarios - All 31 tests passing Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Document critical differences between pull_request and pull_request_target - Explain execution contexts and security models - Provide decision logic flowchart and example scenarios - Document logging enhancements for debugging - Add troubleshooting guide for common issues - Include security considerations and best practices - Reference GitHub Actions security documentation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| } | ||
|
|
||
| // Determine if this is a fork PR | ||
| const isFork = pullRequest.head?.repo?.full_name !== pullRequest.base?.repo?.full_name; |
There was a problem hiding this comment.
@copilot look for a better fork detection logic
There was a problem hiding this comment.
Improved fork detection logic in commit 5666aef.
The new approach uses multiple signals:
- GitHub's fork flag (
head.repo.fork === true) - most reliable when available - Deleted fork detection (
!head.repo) - handles null reference edge case - Repository name comparison - fallback for cross-org PRs
Each detection logs the reason (e.g., "head.repo.fork flag is true", "head repository deleted (was likely a fork)", "different repository names") to help with debugging.
This handles edge cases like:
- Deleted forks where
head.repobecomes null - Forks with same name but different owners
- Cross-organization PRs that aren't technically forks but behave similarly
- Use GitHub's head.repo.fork flag as primary signal - Handle deleted fork edge case (null head.repo) - Fall back to repository name comparison - Log fork detection reason for debugging - Add tests for fork flag and deleted fork scenarios - Update documentation with robust detection logic - All 32 tests passing Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| // 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) | ||
| let isFork = false; |
There was a problem hiding this comment.
@copilot refactor fork detection in separate helper, with tests
There was a problem hiding this comment.
Refactored fork detection into separate helper in commit eb2a117.
Created pr_helpers.cjs with detectForkPR function and comprehensive test suite:
New files:
pr_helpers.cjs- Reusable helper with fork detection logicpr_helpers.test.cjs- 8 test cases covering all scenarios
Tests cover:
- GitHub fork flag detection
- Repository name comparison
- Deleted fork scenario (null head.repo)
- Edge cases: null base repo, missing fork flag, priority handling
Benefits:
- Reusable across other scripts
- Easier to test in isolation
- 40 total tests passing (32 checkout + 8 helper)
- Create pr_helpers.cjs with detectForkPR function - Add comprehensive test suite (8 tests) for fork detection - Test all detection signals: fork flag, name comparison, deleted fork - Test edge cases: null repos, missing fork flag, priority handling - Update checkout_pr_branch.cjs to use helper function - All 32 checkout tests passing + 8 new helper tests - Code properly formatted and linted Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✅ Changeset Generator completed successfully! |
Agent Container Tool Check
Result: 10/12 tools available Issues Found:
|
|
PRs: Standardize error wrapping in compiler to preserve error chains (#14435)
|
There was a problem hiding this comment.
Pull request overview
Improves the PR checkout workflow’s observability and reliability by adding grouped logging and introducing a reusable fork-detection helper to select the correct checkout strategy (especially for pull_request_target).
Changes:
- Added grouped, detailed logging for PR context, chosen checkout strategy, and error diagnostics.
- Introduced
detectForkPRhelper module with unit tests, and updated integration tests. - Added documentation explaining event-context differences and checkout strategy rationale.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scratchpad/pr-checkout-logic-explained.md | New guide describing event contexts, fork detection, strategy selection, and troubleshooting. |
| actions/setup/js/pr_helpers.cjs | Adds reusable detectForkPR helper used by checkout logic. |
| actions/setup/js/pr_helpers.test.cjs | Adds unit tests for detectForkPR. |
| actions/setup/js/checkout_pr_branch.cjs | Adds grouped logging, fork detection usage, and enhanced error diagnostics around checkout. |
| actions/setup/js/checkout_pr_branch.test.cjs | Updates tests for new logging, fork detection paths, and error diagnostics. |
| .changeset/patch-improve-pr-checkout-logging.md | Patch changeset entry documenting the behavior/logging improvements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (eventName === "pull_request") { | ||
| // For pull_request events, use the head ref directly | ||
| // For pull_request events, we run in the merge commit context | ||
| // The PR branch is already available, so we can use direct git commands | ||
| const branchName = pullRequest.head.ref; | ||
| core.info(`Checking out PR branch: ${branchName}`); | ||
|
|
||
| logCheckoutStrategy(eventName, "git fetch + checkout", "pull_request event runs in merge commit context with PR branch available"); | ||
|
|
||
| core.info(`Fetching branch: ${branchName} from origin`); | ||
| await exec.exec("git", ["fetch", "origin", branchName]); | ||
|
|
||
| core.info(`Checking out branch: ${branchName}`); | ||
| await exec.exec("git", ["checkout", branchName]); |
There was a problem hiding this comment.
For fork PRs on pull_request events, git fetch origin <head.ref> will typically fail because origin points at the base repo and doesn’t have the fork branch name. Since you already compute isFork, handle that case explicitly: either skip fetch/checkout (stay on the merge commit), or fetch via the PR ref (e.g., refs/pull/<prNumber>/head) instead of <head.ref>.
See below for a potential fix:
const branchName = pullRequest.head.ref;
if (isFork) {
// For fork PRs, origin points at the base repo and does not have the fork branch.
// The workflow is already checked out at the merge commit created by GitHub,
// so stay on that commit instead of trying to fetch/checkout the fork branch.
logCheckoutStrategy(
eventName,
"no-op (stay on merge commit)",
"fork PR on pull_request event; origin does not have head branch, using existing merge commit"
);
core.warning(
"⚠️ Fork PR detected on pull_request event - staying on the GitHub-provided merge commit instead of fetching from origin"
);
} else {
// For PRs from the same repository, the PR branch exists on origin.
// The PR branch is already available, so we can use direct git commands.
logCheckoutStrategy(
eventName,
"git fetch + checkout",
"pull_request event runs in merge commit context with PR branch available on origin"
);
core.info(`Fetching branch: ${branchName} from origin`);
await exec.exec("git", ["fetch", "origin", branchName]);
core.info(`Checking out branch: ${branchName}`);
await exec.exec("git", ["checkout", branchName]);
core.info(`✅ Successfully checked out branch: ${branchName}`);
}
} else {
// For pull_request_target and other PR events, we run in base repository context
// IMPORTANT: For fork PRs, the head branch doesn't exist in the base repo
// We must use `gh pr checkout` which handles fetching from forks
const prNumber = pullRequest.number;
const strategyReason =
eventName === "pull_request_target"
? "pull_request_target runs in base repo context; for fork PRs, head branch doesn't exist in origin"
: `${eventName} event runs in base repo context; must fetch PR branch`;
| import { describe, it, expect } from "vitest"; | ||
|
|
||
| describe("pr_helpers.cjs", () => { | ||
| let detectForkPR; | ||
|
|
||
| // Import the helper before each test | ||
| beforeEach(async () => { | ||
| const helpers = await import("./pr_helpers.cjs"); | ||
| detectForkPR = helpers.detectForkPR; | ||
| }); |
There was a problem hiding this comment.
beforeEach is used but not imported from vitest, which will throw at runtime. Import it (e.g., import { describe, it, expect, beforeEach } from \"vitest\";).
| const pullRequest = { | ||
| head: { | ||
| repo: { | ||
| fork: true, | ||
| full_name: "test-owner/test-repo", | ||
| }, | ||
| }, | ||
| base: { | ||
| repo: { | ||
| full_name: "test-owner/test-repo", | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The test fixture shape is incorrect: base is nested under head here (and similarly in other tests). In the GitHub payload, pull_request.base is a sibling of pull_request.head. With the current nesting, these tests can pass while not actually validating the real-world structure used by detectForkPR.
| // 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, reason }; |
There was a problem hiding this comment.
This integration test re-implements detectForkPR logic inside the mock, which duplicates behavior and can hide regressions (tests may change in lockstep with the production bug). Prefer mocking detectForkPR with explicit return values per scenario (e.g., { isFork: true, reason: \"...\" }) and keep the detection logic verified only in pr_helpers.test.cjs.
| // 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, reason }; | |
| // In this integration test, don't duplicate the real detection logic. | |
| // Allow tests to control behavior explicitly via a special field. | |
| if (pullRequest && pullRequest.__mockDetectForkResult) { | |
| return pullRequest.__mockDetectForkResult; | |
| } | |
| // Default behavior: treat the PR as from the same repository. | |
| return { | |
| isFork: false, | |
| reason: "mocked: treated as same repository", | |
| }; |
| **Checkout Strategy**: Direct git commands | ||
| ```bash | ||
| git fetch origin <branch-name> | ||
| git checkout <branch-name> | ||
| ``` | ||
|
|
||
| **Why it works**: The branch exists in the current repository context because GitHub has already set up the merge commit environment. | ||
|
|
There was a problem hiding this comment.
This implies git fetch origin <branch-name> is generally valid for pull_request, but it’s not true for fork PRs (the base repo’s origin typically won’t have the fork branch name). Consider clarifying that this only works for same-repo PRs, and that for fork PRs you either don’t need to fetch/checkout (already on a merge ref) or must fetch via refs/pull/<id>/head.
| **Checkout Strategy**: Direct git commands | |
| ```bash | |
| git fetch origin <branch-name> | |
| git checkout <branch-name> | |
| ``` | |
| **Why it works**: The branch exists in the current repository context because GitHub has already set up the merge commit environment. | |
| **Checkout Strategy**: | |
| - **Same-repo PRs**: You can use direct git commands if you specifically want to move from the synthetic merge commit to the PR’s head branch: | |
| ```bash | |
| git fetch origin <branch-name> | |
| git checkout <branch-name> |
- Fork PRs: The head branch typically does not exist on the base repo’s
originremote, sogit fetch origin <branch-name>will fail. Forpull_requestevents on fork PRs:- The workflow is already checked out at a merge ref (for example
refs/pull/<id>/merge), which contains the PR changes; in many cases you don’t need to fetch/checkout anything else. - If you really need the PR’s head commit, fetch it via the PR ref instead of by branch name, for example:
git fetch origin pull/<id>/head:pr-<id>-head git checkout pr-<id>-head
- The workflow is already checked out at a merge ref (for example
Why it works:
- For same-repo PRs,
<branch-name>exists on theoriginremote, so a directgit fetch origin <branch-name>succeeds. - For fork PRs, the PR code is available via GitHub’s PR refs (
refs/pull/<id>/merge/refs/pull/<id>/head>), not as a branch onorigin, so you must rely on those refs instead of assuming the fork branch name exists on the base repository.
| // 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) { |
There was a problem hiding this comment.
When pullRequest.base?.repo?.full_name is missing/undefined, the name comparison evaluates to true and sets reason to "different repository names", which is misleading in logs. Consider handling !pullRequest.base?.repo explicitly with a clearer reason (e.g., "base repository information missing") while still returning isFork: true if you want the safer checkout path.
| } else if (pullRequest.head.repo.full_name !== pullRequest.base?.repo?.full_name) { | |
| } else if (!pullRequest.base?.repo) { | |
| // Base repo information is missing - treat as fork for safety | |
| isFork = true; | |
| reason = "base repository information missing"; | |
| } else if (pullRequest.head.repo.full_name !== pullRequest.base.repo.full_name) { |
Smoke Test: Copilot - Run 21790998664PRs Reviewed:
Test Results:
Overall: FAIL @pelikhan
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 Smoke test complete: 8/9 tests passed (Serena MCP unavailable) |
The PR checkout script lacked visibility into execution context and fork detection, making production failures difficult to debug. For
pull_request_targetevents with fork PRs, the head branch doesn't exist in the base repository's origin remote—gh pr checkoutmust be used instead of direct git commands.Changes
Logging Enhancements
core.startGroup/endGroupfor clean outputRobust Fork PR Detection
Refactored into reusable helper function (
pr_helpers.cjs) with comprehensive test coverage. Uses multiple signals for reliable detection and edge case handling:Why multiple signals?
Reusable Helper Module
pr_helpers.cjswithdetectForkPRfunction for reuse across other scriptserror_helpers.cjs)Documentation
pull_requestandpull_request_targetscratchpad/pr-checkout-logic-explained.mdcovering:git fetch origin <branch>fails for fork PRs inpull_request_targetTests
detectForkPRhelper function inpr_helpers.test.cjs:checkout_pr_branch.test.cjsfor integrationKey Insight
The original logic was correct but undocumented. These changes make the behavior observable and debuggable in production, with robust fork detection that handles edge cases like deleted repositories. The refactored helper function enables consistent fork detection across multiple workflows.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Changeset