Skip to content

fix(ci): make no-new-getattr guard stable in shallow PR checkouts#38480

Merged
crazywoola merged 3 commits into
langgenius:mainfrom
EvanYao826:fix/shallow-getattr-ci-guard
Jul 8, 2026
Merged

fix(ci): make no-new-getattr guard stable in shallow PR checkouts#38480
crazywoola merged 3 commits into
langgenius:mainfrom
EvanYao826:fix/shallow-getattr-ci-guard

Conversation

@EvanYao826

Copy link
Copy Markdown
Contributor

Summary

The no-new-getattr style guard could fail in pull request CI when the workflow checked out a shallow synthetic PR merge commit, then fetched a newer main tip with --depth=1. git merge-base main HEAD failed because the PR base commit wasn't reachable from the shallow-fetched main ref.

Root Cause

The workflow fetched origin/main with --depth=1 and created a local main branch pointing to it. When main had advanced past the PR's base commit, git merge-base main HEAD returned an error because Git had no common ancestor — the shallow-fetched main tip was newer than the PR's base and didn't share history.

Fix

  • scripts/check_no_new_getattr.py: resolve_ci_base() now checks the GITHUB_BASE_SHA environment variable first. GitHub Actions automatically sets this to the stable base commit SHA of the pull request, so it's always in Git history regardless of how shallow the checkout is.
  • .github/workflows/style.yml: Removed the now-unnecessary git fetch --depth=1 origin/main and git branch main origin/main steps. Added GITHUB_BASE_SHA env to the guard step.
  • Tests: Added unit tests for resolve_ci_base with and without GITHUB_BASE_SHA set.

Fixes #38325

The check_no_new_getattr guard could fail when CI checked out a shallow
synthetic PR merge commit, then fetched a newer main tip with --depth=1.
git merge-base failed because the PR base commit wasn't reachable from
the shallow-fetched main ref.

Fix:
- Use GITHUB_BASE_SHA (set by GitHub Actions for PR events) as the stable
  base commit reference instead of relying on git merge-base against a
  moving, shallow-fetched main branch.
- Remove the now-unnecessary git fetch/bind steps from style.yml.
- Add unit tests for the GITHUB_BASE_SHA fallback in resolve_ci_base.

Fixes langgenius#38325
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jul 6, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Pyrefly Type Coverage

Metric Base PR Delta
Type coverage 52.39% 52.40% +0.01%
Strict coverage 51.91% 51.92% +0.01%
Typed symbols 32,141 32,155 +14
Untyped symbols 29,478 29,478 0
Modules 2976 2976 0

QuantumGhost
QuantumGhost previously approved these changes Jul 7, 2026
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jul 7, 2026
@QuantumGhost QuantumGhost added this pull request to the merge queue Jul 7, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 7, 2026
@QuantumGhost QuantumGhost marked this pull request as draft July 7, 2026 01:56
@QuantumGhost

Copy link
Copy Markdown
Contributor

The merge queue check failed.

Besides, I believe the better approach would be inject the base commit directly through the script command line arguments, instead of accessing GITHUB_BASE_SHA implicitly in the script.

@QuantumGhost

Copy link
Copy Markdown
Contributor

Hi, @EvanYao826

I have updated the branch and making base commit passing explicitly.

@QuantumGhost QuantumGhost marked this pull request as ready for review July 7, 2026 03:21
@dosubot dosubot Bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jul 7, 2026
@crazywoola crazywoola added this pull request to the merge queue Jul 7, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 7, 2026
@crazywoola crazywoola added this pull request to the merge queue Jul 8, 2026
Merged via the queue into langgenius:main with commit 98f67e9 Jul 8, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Refactor/Chore] Make no-new-getattr guard stable in shallow PR checkouts

3 participants