[WIP] Fix checkout PR branch fetch depth issue - #50378
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50378 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
🟡 Not ready to approve
The new shallow-clone diagnostic is swallowed by fallback handling and does not reliably reach callers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Prevents PR checkout from re-shallowing complete repositories and improves shallow-history diagnostics.
Changes:
- Omits fetch depth for complete clones.
- Adds coverage for both PR fetch paths.
- Adds shallow merge-base error handling.
File summaries
| File | Description |
|---|---|
actions/setup/js/checkout_pr_branch.cjs |
Preserves full history during PR fetches. |
actions/setup/js/checkout_pr_branch.test.cjs |
Tests non-shallow fetch behavior. |
actions/setup/js/generate_git_patch.cjs |
Adds shallow merge-base diagnostics. |
Review details
Suppressed comments (1)
actions/setup/js/checkout_pr_branch.cjs:349
- On the new non-shallow path, the preceding message still claims this refs/pull fetch has
depth: N, even thoughdepthArgsomits the option. Log the resolved fetch mode instead so the checkout diagnostics reflect the command that actually runs.
await exec.exec("git", ["fetch", "origin", `+refs/pull/${prNumber}/head:refs/remotes/origin/pr-head`, ...(await depthArgs(fetchDepth))]);
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| if (fs.existsSync(path.join(cwd || process.cwd(), ".git", "shallow"))) { | ||
| throw new Error( | ||
| `${ERR_SYSTEM}: Could not compute merge-base between ${defaultBranchRef} and ${tipRef} because the repository is a shallow clone (.git/shallow exists). ` + | ||
| "Deepen the clone (checkout.fetch-depth: 0) so the common ancestor is reachable." | ||
| ); | ||
| } | ||
| throw mergeBaseError; |
|
|
||
| core.info(`Fetching branch: ${branchName} from origin (depth: ${fetchDepth} for ${commitCount} PR commit(s))`); | ||
| await exec.exec("git", ["fetch", "origin", branchName, `--depth=${fetchDepth}`]); | ||
| await exec.exec("git", ["fetch", "origin", branchName, ...(await depthArgs(fetchDepth))]); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Fix checkout PR branch fetch depth issue
The approach is correct and well-structured. One reliability concern worth addressing before merging:
isShallowRepository() fallback on Git < 2.15
git rev-parse --is-shallow-repository was added in Git 2.15. When it is not available the command exits non-zero, and isShallowRepository() returns false — silently skipping --depth on shallow clones and recreating the original bug. The existing generate_git_patch.cjs already handles this portably by checking fs.existsSync(".git/shallow"); using the same technique as a fallback here would make the fix robust across all runner git versions.
A memoisation cache is a nice-to-have (two fetches at most per run, each very fast) but the correctness issue is the more pressing concern.
Everything else looks good — test coverage is clear and the generate_git_patch.cjs error message is a genuine usability improvement.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.5 AIC · ⌖ 9.34 AIC · ⊞ 5.4K
| * grafts history, silently undoing an explicit `checkout: fetch-depth: 0`. That | ||
| * breaks later `git merge-base` calls (e.g. patch generation for | ||
| * create_pull_request). We therefore only pass `--depth` when the repository is | ||
| * already shallow; a complete clone already has the objects we need. |
There was a problem hiding this comment.
isShallowRepository() runs on every depthArgs() call with no memoization, and it silently falls back to false when git rev-parse --is-shallow-repository exits non-zero (i.e., Git < 2.15). On a genuinely shallow clone with an older git binary, --depth is therefore omitted, recreating the original bug.
Consider two improvements:
- Cache the result: memoize at module level so the
git rev-parsesubprocess runs at most once. - Portable fallback: when
exitCode !== 0, fall back tofs.existsSync(path.join(process.cwd(), ".git", "shallow"))— purely local, no credentials, and already used for this purpose ingenerate_git_patch.cjs.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness/coverage issues.
📋 Key Themes & Highlights
Key Themes
- Shallow detection inconsistency:
generate_git_patch.cjsusesfs.existsSync('.git/shallow')whilecheckout_pr_branch.cjsusesgit rev-parse --is-shallow-repository. These diverge in git worktrees and after--unshallow. A shared helper ingit_helpers.cjswould make both paths consistent and testable. - Missing regression test for the new
merge-baseerror path ingenerate_git_patch.cjs— the improved error message has no test guard. - Test isolation: second new test mutates
mockContext.eventNamewithout explicit teardown.
Positive Highlights
- ✅ Clean abstraction:
isShallowRepository()anddepthArgs()are well-named and well-documented - ✅ Safe default: both helpers return
false/[]on errors — correct fail-open for complete clones - ✅ Good coverage of both fetch paths (pull_request and pull_request_target events)
- ✅ Informative
core.infolog when depth is omitted
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 40.5 AIC · ⌖ 8.26 AIC · ⊞ 7.1K
Comment /matt to run again
| // otherwise complete clone) can make the merge-base unreachable. | ||
| // Surface that explicitly instead of the misleading "branch does not | ||
| // exist locally" message. | ||
| if (fs.existsSync(path.join(cwd || process.cwd(), ".git", "shallow"))) { |
There was a problem hiding this comment.
[/diagnosing-bugs] Shallow detection here uses fs.existsSync('.git/shallow'), but checkout_pr_branch.cjs uses git rev-parse --is-shallow-repository — these diverge on edge cases: git worktrees have .git as a file (not a directory) so the path.join(cwd, '.git', 'shallow') join silently fails; also git fetch --unshallow does not always delete .git/shallow immediately, giving false positives.
💡 Suggested fix
Extract isShallowRepository() into a shared helper (e.g. git_helpers.cjs) and call it in both files:
const shallow = await isShallowRepository({ cwd });
if (shallow) {
throw new Error(`${ERR_SYSTEM}: Could not compute merge-base ... shallow clone.`);
}
throw mergeBaseError;This is also more testable — no real .git/shallow file needed.
@copilot please address this.
| throw new Error( | ||
| `${ERR_SYSTEM}: Could not compute merge-base between ${defaultBranchRef} and ${tipRef} because the repository is a shallow clone (.git/shallow exists). ` + | ||
| "Deepen the clone (checkout.fetch-depth: 0) so the common ancestor is reachable." | ||
| ); |
There was a problem hiding this comment.
[/tdd] No test covers the new merge-base error path in generate_git_patch.cjs. Without a regression test, future refactors can silently remove the improved error message and nobody will notice.
💡 Suggested test shape
In generate_git_patch.test.cjs, add a test that stubs execGitSync to throw on merge-base and asserts the resulting error message contains 'shallow clone' and ERR_SYSTEM:
it('reports shallow-clone error when merge-base fails and .git/shallow exists', async () => {
mockExecGitSync.mockImplementationOnce((args) => {
if (args[0] === 'merge-base') throw new Error('fatal: no merge base');
});
mockFsExistsSync.mockReturnValue(true); // simulate .git/shallow present
await expect(generateGitPatch('branch', 'main', { mode: 'full' }))
.rejects.toThrow(/shallow clone/);
});@copilot please address this.
| }); | ||
|
|
||
| it("should omit --depth for refs/pull fetch when the repository is not shallow", async () => { | ||
| mockExec.getExecOutput.mockResolvedValue({ stdout: "false\n", stderr: "", exitCode: 0 }); |
There was a problem hiding this comment.
[/tdd] The second test modifies mockContext.eventName but relies on beforeEach/module cache reset (via vi.resetModules) to clean up. If the reset order changes, this mutation leaks into subsequent tests. Explicitly set mockContext.eventName back in the test or in afterEach to make the test self-contained.
💡 Minimal fix
At the top of the pull_request_target test, save and restore:
const originalEvent = mockContext.eventName;
mockContext.eventName = 'pull_request_target';
// ... assertions ...
mockContext.eventName = originalEvent;Or confirm vi.resetModules() in beforeEach fully re-creates mockContext (add a comment stating this intent).
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Quality AnalysisBoth tests verify the core design invariant: conditional --depth flag based on repository shallowness. Test 1: Shallow repo check (line 243)
Test 2: Shallow repo check for refs/pull fetch (line 254)
Production code alignment: Changes in Verdict
|
There was a problem hiding this comment.
Review: no blocking issues found
The fetch-depth fix is sound and well-tested; two minor robustness gaps flagged inline are non-blocking.
💡 Themes
checkout_pr_branch.cjs:isShallowRepository()correctly distinguishes shallow vs. non-shallow clones and preservesfetch-depth: 0semantics as intended; new unit tests cover both branches.- Minor: the non-exception failure path in
isShallowRepository()doesn't log a warning, unlike the exception path — inconsistent diagnostics for similar failure classes (see inline comment). generate_git_patch.cjs: the new shallow-clone error message is a good UX improvement over the previous misleading "branch does not exist locally" error, but the.git/shallowfile-existence check is a proxy that can both false-positive and false-negative relative to the actual cause of amerge-basefailure (see inline comment).- No correctness, concurrency, or security-relevant issues found in the diff.
🔎 Code quality review by PR Code Quality Reviewer · auto · 44.8 AIC · ⌖ 10 AIC · ⊞ 7.9K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/checkout_pr_branch.cjs:26
When git rev-parse exits non-zero (not an exception), the failure is swallowed silently and treated as "not shallow" with no diagnostic — unlike the catch block below which does warn.
<details><summary>💡 Details</summary>
If rev-parse --is-shallow-repository fails for a legitimate reason (e.g. detached worktree quirks, older git version, permissions), the function silently falls back to "non-shallow" behavior (omitting --depth), which changes fetch semantics with zero visibility for wh…
actions/setup/js/generate_git_patch.cjs:129
Detecting shallowness via fs.existsSync(".git/shallow") misdiagnoses the real failure: a merge-base can also be unreachable on a fully-fetched-but-still-shallow-graft repo, or fail for reasons unrelated to shallow history (e.g. missing ref), yet this always attributes it to shallowness when the file happens to exist.
<details><summary>💡 Details</summary>
The new fix in checkout_pr_branch.cjs explicitly tries to avoid ever creating .git/shallow when the repo started non-shallow, but any…
|
@copilot Please address the remaining review feedback on this PR, refresh the branch if GitHub allows it, and then run the Open review feedback to close out (newest first):
|
…logging Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please address the remaining review feedback on this PR, refresh the branch if GitHub allows it, and then run the Open review feedback to close out (newest first):
|
--depth=Nincheckout_pr_branch.cjswhen the repo is not already shallow (preservescheckout: fetch-depth: 0)git merge-basefails ingenerate_git_patch.cjsfull modeRun: https://github.com/github/gh-aw/actions/runs/30949820215