Skip to content

fix(ci): restore merge base for three-dot diffs broken by shallow clones - #3865

Merged
cixzhang merged 1 commit into
facebook:mainfrom
Han5991:fix/ci-shallow-merge-base
Jul 19, 2026
Merged

fix(ci): restore merge base for three-dot diffs broken by shallow clones#3865
cixzhang merged 1 commit into
facebook:mainfrom
Han5991:fix/ci-shallow-merge-base

Conversation

@Han5991

@Han5991 Han5991 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Summary

The shallow-clone optimization in #1379 (5cf9420) left check-components and build on the default fetch-depth: 1 plus a --depth=1 base fetch, while their change detection still uses three-dot diffs (origin/main...HEAD) — which need the merge base to exist locally. On a depth-1 clone the diff dies with fatal: no merge base, and both consumers swallow the error:

  • check-components: the failed diff piped through grep | head yields an empty string, so has_components=false — and pr-a11y is skipped. Measured: in the 15 most recent PR CI runs, pr-a11y executed zero times — skipped in all 12 completed runs (the remaining 3 are currently queued/in-progress or awaiting fork approval). The accessibility audit has been silently disabled since ci: optimize PR workflow — deduplicate build, shallow clones #1379 landed (~3 months).
  • build: analyze-pr.js catches the diff error and returns [], so the PR Analysis Report comment posts "New components: none / Modified components: none" on every PR, regardless of what actually changed.

The fix reuses the pattern already proven by check-scope in the same file (fetch-depth: 50 + base fetch --depth=50), which has been working correctly all along.

Note: once this lands, the first PRs touching components will run the a11y audit again after ~3 months of it being silently off — pre-existing violations that accumulated in that window may surface as new failures.

Changes

  • check-components: fetch-depth: 50 on checkout + base fetch --depth=50 (was depth 1/1). Added an explicit merge-base guard that fails open — if the clone is ever too shallow again it sets has_components=true with a ::warning so the audit runs instead of silently skipping.
  • build: same depth alignment (fetch-depth: 50 + base fetch --depth=50) so analyze-pr.js's three-dot diff can resolve the merge base.
  • analyze-pr.js: a failed git diff base...head now exits non-zero with a ::error annotation instead of being reported as an empty (legitimate-looking) analysis. Header @output updated accordingly.

Test plan

Verified with a local bare-repo simulation that replicates the actions/checkout fetch pattern (git init + remote add + fetch --depth=N, not git clone --depth which implies --single-branch): a pr branch forking from main~3 with 30+ commits of history, touching packages/core/src/Button/.

  • Repro at depth 1: git diff origin/main...HEAD fails with fatal: origin/main...HEAD: no merge base (exit 128); the current check-components pipeline swallows it and emits has_components=false even though the PR modified Button
  • Fixed at depth 50: merge base resolves, diff lists packages/core/src/Button/Button.tsx, has_components=true
  • analyze-pr.js at depth 1 now exits 1 with ::error::git diff origin/main...HEAD failed: … instead of writing an empty analysis
  • analyze-pr.js at depth 50 exits 0 and reports {"newComponents":[],"modifiedComponents":["Button"]}
  • ci.yml parses as valid YAML; all three PR jobs (check-scope, check-components, build) now consistently use fetch-depth: 50 + base fetch --depth=50 (grep-verified)
  • node --check .github/scripts/analyze-pr.js passes

The shallow-clone optimization in facebook#1379 (5cf9420) set the default
fetch-depth of 1 on the check-components and build jobs while their
change detection still uses three-dot diffs (origin/main...HEAD), which
require the merge base to exist locally. On a depth-1 clone the diff
fails with "fatal: no merge base" and both consumers swallowed it:

- check-components: the failed diff piped through grep/head yields an
  empty string, so has_components=false and pr-a11y is silently skipped
  on every PR.
- build: analyze-pr.js caught the error and returned [], so the PR
  Analysis Report always posted "New components: none / Modified
  components: none".

Fix, following the depth-50 pattern already proven by check-scope in
the same file:

- check-components + build: fetch-depth: 50 on checkout and --depth=50
  on the base fetch.
- check-components: explicit merge-base guard that fails open
  (has_components=true, with a ::warning) instead of silently skipping
  the a11y audit if the clone is ever too shallow again.
- analyze-pr.js: a failed diff now exits non-zero with ::error instead
  of being reported as "no changes".

Verified with a local bare-repo simulation replicating the
actions/checkout fetch pattern: depth 1 reproduces "fatal: no merge
base" -> has_components=false; depth 50 finds the merge base and
detects the changed component.
@vercel

vercel Bot commented Jul 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
astryx Ready Ready Preview, Comment Jul 12, 2026 6:14am

Request Review

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Jul 12, 2026
@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge labels Jul 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR Analysis Report

No new or modified components detected.

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.6KB 0B

Accessibility Audit

Status: No accessibility violations detected.


Generated by PR Enrichment workflow | View full report

@github-actions github-actions Bot removed the needs:code-review High-risk change (new package/component/API) — needs human code review before merge label Jul 12, 2026
@Han5991

Han5991 commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@cixzhang Hi — #3865 has been approved but isn't merged yet. Is there a reason it's still open? Thanks!

@cixzhang

Copy link
Copy Markdown
Contributor

Hmm I can't seem to merge it.
Debugging

@cixzhang
cixzhang merged commit 5cc03ea into facebook:main Jul 19, 2026
18 checks passed
@Han5991
Han5991 deleted the fix/ci-shallow-merge-base branch July 19, 2026 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team) type:fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants