feat: fail fast on implausible commit range in shallow checkout - #49003
Conversation
… path
Add checkImplausibleShallowRange() to git_helpers.cjs that detects when a
shallow clone (fetch-depth:1) causes git rev-list to return the entire local
history as the commit range (e.g. 61k commits for a 1-commit branch). When
the range exceeds SHALLOW_RANGE_MAX_COMMITS (100) and the repo is shallow,
emit an actionable warning/error instead of proceeding to synthesize a
potentially huge/incorrect rewrite commit.
Changes:
- Add SHALLOW_RANGE_MAX_COMMITS = 100 constant
- Add checkImplausibleShallowRange(baseRef, headRef, options) (sync)
that returns { implausible, commitCount } and emits core.warning
- Modify hasMergeCommitsInRange to call the guard first; return false
for implausible ranges to prevent false-positive bundle transport
- Modify linearizeRangeAsCommit to throw before any git state mutation
when range is implausible in a shallow checkout; configurable via
opts.maxCommits; set to Infinity to disable the guard
- Export checkImplausibleShallowRange and SHALLOW_RANGE_MAX_COMMITS
- Add 14 new tests covering both functions and the new guard paths
- Document the fetch-depth workaround in checkout.md and
safe-outputs-pull-requests.md
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage: feat: fail fast on implausible commit range in shallow checkout
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #49003 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Adds safeguards intended to detect implausibly large commit ranges in shallow checkouts before rewriting commits.
Changes:
- Adds shallow-range detection and configurable thresholds.
- Guards commit linearization and merge detection.
- Adds tests and checkout guidance.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/git_helpers.cjs |
Adds range guards and linearization checks. |
actions/setup/js/git_helpers.test.cjs |
Tests shallow-range behavior. |
docs/src/content/docs/reference/checkout.md |
Documents checkout-depth implications. |
docs/src/content/docs/reference/safe-outputs-pull-requests.md |
Documents safe-output behavior and remediation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (3)
actions/setup/js/git_helpers.cjs:359
- This guard is unreachable from its only production caller. In
safe_outputs_handlers.cjs:1212-1242, whenpatchFormatExplicitis false the format defaults tobundle(useBundle === true), and whenuseBundleis false anamformat was explicitly supplied (patchFormatExplicit === true), so!useBundle && !patchFormatExplicitcan never hold. Consequently this new shallow-range check never emits its warning or prevents the default bundle path. Invoke the implausible-range guard independently of that impossible auto-switch branch and abort before artifact generation.
const { implausible } = checkImplausibleShallowRange(baseRef, headRef, options);
if (implausible) return false;
actions/setup/js/git_helpers.cjs:320
- None of the added
checkImplausibleShallowRangetests reaches this positive branch: the “non-shallow large range” test uses a one-commit range below the default threshold, and the shallow/large mocks exercise the separate duplicated logic inlinearizeRangeAsCommit. A regression in this helper's shallow probe, warning, or{ implausible: true }result would therefore pass. Add a direct test using a shallow repository with a range above a loweredmaxCommitsthreshold.
if (isShallow) {
core.warning(
`Shallow checkout produced an implausible commit range of ${commitCount} commits for ${baseRef}..${headRef}. ` +
`This usually means ${baseRef} is not reachable from the shallow grafts. ` +
`Increase fetch-depth in your workflow checkout step (e.g. fetch-depth: 0) to resolve this.`
actions/setup/js/git_helpers.cjs:735
- This check runs only after
create_pull_requesthas generated and transferred the patch/bundle, applied the bundle, and attempted the first signed push (create_pull_request.cjs:1681). It therefore cannot satisfy the linked issue’s requirement to detect the implausible range before payload synthesis; the large artifact work has already happened. Invoke the guard in the create-pull-request safe-output handler beforegenerateGitPatch/generateGitBundle, while retaining this pre-mutation defense.
const { stdout: countOut } = await execApi.getExecOutput("git", ["rev-list", "--count", `${baseRef}..HEAD`], ...execArgs);
rangeCommitCount = parseInt(countOut.trim(), 10) || 0;
} catch {
rangeCommitCount = 0;
}
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Medium
| :::caution[Shallow checkout and large monorepos] | ||
| The merge-commit detection that auto-selects bundle transport inspects the commit range `origin/<branch>..<branch>` in the **agent's** workspace. With the default shallow checkout (`fetch-depth: 1`) `origin/<branch>` has no traversable ancestry, so `git rev-list` cannot exclude any commits and will report the entire local history as the range. On large monorepos this produces a count of tens of thousands of commits, which falsely appears to contain merge commits and can trigger an incorrect rewrite. | ||
|
|
||
| The safe_outputs push job guards against this: if the commit range contains more than 100 commits **and** the repository is shallow, the push is refused with a clear error that includes the commit count. To resolve this, increase `fetch-depth` in your workflow checkout step: |
| Fetch everything the workflow needs at checkout time using `fetch-depth` and [`fetch:`](#fetching-additional-refs), and write changes through safe-output tools such as [`push-to-pull-request-branch`](/gh-aw/reference/safe-outputs-pull-requests/) rather than a direct `git push`. The agent is instructed not to configure credential helpers or run `git credential fill`, because authentication cannot succeed; credential errors are reported as a limitation instead of worked around. | ||
|
|
||
| :::note[Shallow checkout and push-to-pull-request-branch] | ||
| `push-to-pull-request-branch` and `create-pull-request` inspect the commit range `origin/<branch>..<branch>` in the agent's workspace to detect merge commits. With the default shallow clone (`fetch-depth: 1`), `origin/<branch>` has no traversable ancestry, so `git rev-list` reports the entire local history as the range. On large monorepos (thousands of commits) this can falsely trigger bundle or rewrite paths and will be refused with a clear error. Set `fetch-depth: 0` to ensure the correct range is visible. |
| * All git failures are treated as non-fatal: errors cause the function to | ||
| * return `{ implausible: false, commitCount: 0 }` so callers are never blocked. |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (14 tests)
Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 53.1 AIC · ⌖ 4.85 AIC · ⊞ 5.3K
| `This likely means ${baseRef} is not reachable from the shallow grafts. ` + | ||
| `Increase fetch-depth in your workflow checkout step (e.g. fetch-depth: 0) to resolve this.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
The guard in linearizeRangeAsCommit re-implements shallow-range detection inline rather than reusing checkImplausibleShallowRange. This creates two diverging implementations: the shared helper accepts an explicit headRef and uses execGitSync, while this inline block hardcodes HEAD and uses execApi.getExecOutput. A future change to one path will silently miss the other, and the two already differ (e.g. headRef is not surfaced in the thrown message here vs the shared helper).
Consider adapting checkImplausibleShallowRange to accept an optional execApi so both callers can share a single implementation.
@copilot please address this.
| } catch { | ||
| rangeCommitCount = 0; | ||
| } | ||
| if (Number.isFinite(rangeCommitCount) && rangeCommitCount > maxCommits) { |
There was a problem hiding this comment.
parseInt(...) || 0 already coerces NaN to 0, so rangeCommitCount is always a finite integer after that line. The Number.isFinite(rangeCommitCount) guard on line 736 is therefore always true and adds no protection. The same pattern is in checkImplausibleShallowRange (line ~61). Consider removing the redundant isFinite check to simplify the condition to rangeCommitCount > maxCommits.
@copilot please address this.
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42 AIC · ⌖ 6.82 AIC · ⊞ 7K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/git_helpers.cjs:168
[/codebase-design] linearizeRangeAsCommit duplicates the shallow-guard logic inline instead of calling checkImplausibleShallowRange, creating two diverging implementations of the same check.
<details>
<summary>💡 Suggested refactor</summary>
checkImplausibleShallowRange is synchronous and uses execGitSync; linearizeRangeAsCommit needs async execApi.getExecOutput. The cleanest fix is to add an async overload or an internal async helper, then replace the inline block with a sing…
actions/setup/js/git_helpers.cjs:56
[/tdd] parseInt(countOut.trim(), 10) || 0 silently treats non-numeric output (e.g. an unexpected git message) as 0, masking parse failures.
<details>
<summary>💡 Suggested fix</summary>
Use a stricter parse that catches NaN explicitly:
const parsed = parseInt(countOut.trim(), 10);
if (Number.isNaN(parsed)) return { implausible: false, commitCount: 0 };
commitCount = parsed;Or use the same pattern already in linearizeRangeAsCommit (line 147) which also uses || 0 — at m…
actions/setup/js/git_helpers.test.cjs:381
[/tdd] There are no tests for hasMergeCommitsInRange with a shallow + large commit range — the function that uses checkImplausibleShallowRange but lacks coverage for that branch.
<details>
<summary>💡 Suggested test</summary>
it("should return false and not call rev-list --merges when range is implausibly large in a shallow checkout", () => {
// mock execGitSync so rev-list --count returns 500 and --is-shallow-repository returns true
// then call hasMergeCommitsInRange and a…
</details>
There was a problem hiding this comment.
Verdict: COMMENT — solid fix, but the guard has a false-positive risk and a coverage gap worth addressing before merge.
The core fix (refusing to linearize/misdetect on implausible shallow ranges) is correct and well-tested for the primary paths. Left three comments: (1) duplicated implausibility-check logic between checkImplausibleShallowRange and the inline guard in linearizeRangeAsCommit risks drift, (2) the flat 100-commit threshold doesn't account for fetch-depth > 1 shallow clones and could false-positive on legitimately large branches, (3) hasMergeCommitsInRange's new guard behavior — the actual bug this PR targets — has no direct test.
💡 Themes
- Two independent implementations of the same "is this range implausible for a shallow clone" logic (sync execGitSync vs async execApi.getExecOutput) — not a blocking bug today, but a maintainability trap.
- Threshold heuristic is reasonable for
fetch-depth: 1(the stated failure mode) but doesn't distinguish that from deeper shallow clones where 100+ commits could be genuine. - Test suite is thorough for
checkImplausibleShallowRangeandlinearizeRangeAsCommit, but skipshasMergeCommitsInRange, which is the function actually named in the bug's false-positive-bundle-switching scenario.
None of these are correctness-breaking for the stated fetch-depth: 1 scenario, so not blocking, but recommend addressing before merge to avoid a future regression or false-positive report.
🔎 Code quality review by PR Code Quality Reviewer · aut00 · 84.5 AIC · ⌖ 7.44 AIC · ⊞ 7.8K
Comment /review to run again
| * @throws {Error} If the soft reset, staged-changes validation, or recommit fails, or if a | ||
| * shallow checkout produces an implausible commit range. | ||
| */ | ||
| async function linearizeRangeAsCommit(baseRef, commitMessage, execApi, opts = {}) { |
There was a problem hiding this comment.
linearizeRangeAsCommit re-implements the shallow-range implausibility check with its own inline logic instead of reusing checkImplausibleShallowRange, risking behavior drift between the two guards.
💡 Details
Both checkImplausibleShallowRange (line 284) and this inline block (lines 728-753) independently count commits via a rev-list call and probe --is-shallow-repository, but one uses execGitSync (sync spawn) while the other uses execApi.getExecOutput (async exec) with a different error-message format. Any future fix or threshold change in one place silently won't apply to the other — the two implementations already diverge (execGitSync vs async execApi with a distinct ignoreReturnCode option), so drift is already present, not just hypothetical.
Suggested fix: refactor the inline block to call a shared async-capable version of checkImplausibleShallowRange so both callers share one source of truth for the guard logic.
| * Callers can override the threshold via the `maxCommits` option on the | ||
| * functions that accept it. | ||
| */ | ||
| const SHALLOW_RANGE_MAX_COMMITS = 100; |
There was a problem hiding this comment.
The hardcoded threshold of 100 commits can false-positive on legitimate large branches in a shallow-but-deep checkout (e.g. fetch-depth: 200), causing valid work to be blocked or misreported.
💡 Details
The guard only tests --is-shallow-repository, not how deep the clone actually is. A repo with fetch-depth: 200 is still "shallow" per git's definition, and a genuinely active feature branch can easily accumulate more than 100 real commits against its base. In that case:
hasMergeCommitsInRangesilently returnsfalseeven if there truly are merge commits in a 100+ commit range, defeating the bundle-transport detection it exists to protect.linearizeRangeAsCommithard-throws and refuses to linearize valid work, surfacing a confusing error that blames "implausible" history when the range is real.
Consider comparing the reported commit count against git rev-list --count HEAD (the shallow grafted depth) or against the shallow boundary itself, rather than a flat magic number, so the guard only triggers when the count actually exceeds what the clone's real depth could produce.
|
|
||
| // Guard: if the range is implausibly large for a shallow checkout, treat as | ||
| // no merge commits so a false-positive does not force bundle transport. | ||
| const { implausible } = checkImplausibleShallowRange(baseRef, headRef, options); |
There was a problem hiding this comment.
Missing test coverage: no test asserts that hasMergeCommitsInRange actually returns false when checkImplausibleShallowRange reports implausible:true.
💡 Details
The 14 new tests added in git_helpers.test.cjs exercise checkImplausibleShallowRange directly and linearizeRangeAsCommit's guard, but none call hasMergeCommitsInRange with a mocked implausible range to verify it short-circuits to false at line 359 before the --merges rev-list call. This is the change described in the PR body as preventing "false-positive bundle-transport switching" — a regression here would silently re-enable the exact bug this PR is fixing, with no test to catch it.
Suggested addition: a test in a describe("hasMergeCommitsInRange") block that mocks execGitSync to report a huge shallow range and asserts the function returns false without ever calling the merges rev-list.
|
@copilot please run the Recent reviewer feedback to address:
Run: https://github.com/github/gh-aw/actions/runs/30520508562
|
…rage - Fix JSDoc in checkImplausibleShallowRange to accurately document both failure cases: rev-list failure returns commitCount:0; rev-parse failure returns the measured commitCount (not 0) - Fix test to exercise the --is-shallow-repository code path by using maxCommits:0 to force the threshold check on a 1-commit range; test is now environment-aware (works in both shallow and full clones) - Add hasMergeCommitsInRange test coverage: empty refs, non-existent refs, empty range, and environment-aware threshold test - Fix safe-outputs-pull-requests.md to accurately describe behavior: hasMergeCommitsInRange emits warning and returns false (not 'push refused'); only linearizeRangeAsCommit throws on implausible ranges - Fix checkout.md to remove inaccurate mention of create-pull-request inspecting the merge-commit range; only push-to-pull-request-branch uses hasMergeCommitsInRange for transport selection Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all unresolved review feedback in commit
|
Triage Summary
Prevents incorrect commit-range rewrites on large monorepos with shallow checkouts. Has tests+docs. Batch B-git-checkout (with #49002, #49059).
|
|
@copilot add git integration tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…MergeCommitsInRange, and linearizeRangeAsCommit Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added in commit The new
Two implementation notes: |
…in CI shallow clone Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit Root cause: the CI job uses Fix: added an explicit |
In a shallow clone (
fetch-depth: 1),origin/<base>has no traversable ancestry, sogit rev-list base..HEADreturns the entire local history. On large monorepos this produces counts in the tens-of-thousands, which falsely trips merge-commit detection and forceslinearizeRangeAsCommit— producing incorrect rewrites.Changes
git_helpers.cjsSHALLOW_RANGE_MAX_COMMITS = 100constant (override viamaxCommitsoption)checkImplausibleShallowRange(baseRef, headRef, options)— counts commits in range, checks--is-shallow-repository; if both exceed threshold and are shallow, emitscore.warningwith an actionable message; all git failures non-fatalhasMergeCommitsInRange— calls the guard first; returnsfalsefor implausible ranges, preventing false-positive bundle-transport switchinglinearizeRangeAsCommit— adds async guard before any git state mutation (before soft-reset); throws with count, base ref, andfetch-depthfix when implausible; configurable viaopts.maxCommits(setInfinityto disable)git_helpers.test.cjs— 14 new tests covering both functions: empty/invalid refs, git failure paths, non-shallow large ranges, shallow large ranges, custom threshold, rev-list/shallow-probe failure non-fatalityDocs — Added caution blocks in
checkout.mdandsafe-outputs-pull-requests.mddocumenting the shallow-range limitation andfetch-depth: 0workaroundError behavior
When a shallow checkout produces an implausible range,
linearizeRangeAsCommitnow throws before touching the index:And
hasMergeCommitsInRangeemits a warning and returnsfalse(no bundle-force) for the same condition.