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

Co-authored-by: QuantumGhost <obelisk.reg+git@gmail.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This commit is contained in:
Evan 2026-07-08 14:22:59 +08:00 committed by GitHub
parent af4538e942
commit 98f67e9c82
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 196 additions and 100 deletions

View File

@ -53,6 +53,10 @@ jobs:
filters: |
api:
- 'api/**'
- 'scripts/check_no_new_getattr.py'
- 'scripts/ast_grep_rules/no_new_getattr.yml'
- '.github/workflows/style.yml'
- '.github/workflows/main-ci.yml'
- '.github/workflows/api-tests.yml'
- 'docker/.env.example'
- 'docker/envs/middleware.env.example'
@ -380,6 +384,8 @@ jobs:
needs: pre_job
if: needs.pre_job.outputs.should_skip != 'true'
uses: ./.github/workflows/style.yml
with:
base-rev: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }}
vdb-tests-run:
name: Run VDB Tests

View File

@ -2,6 +2,10 @@ name: Style check
on:
workflow_call:
inputs:
base-rev:
required: true
type: string
concurrency:
group: style-${{ github.head_ref || github.run_id }}
@ -33,6 +37,7 @@ jobs:
scripts/check_no_new_getattr.py
scripts/ast_grep_rules/no_new_getattr.yml
.github/workflows/style.yml
.github/workflows/main-ci.yml
- name: Setup UV and Python
if: steps.changed-files.outputs.any_changed == 'true'
@ -54,17 +59,9 @@ jobs:
if: steps.changed-files.outputs.any_changed == 'true'
run: uv run --project api --dev python api/dev/lint_response_contracts.py --fail-on-mismatch
- name: Fetch merge target ref for getattr guard
if: steps.changed-files.outputs.any_changed == 'true'
run: git fetch --no-tags --depth=1 origin +refs/heads/main:refs/remotes/origin/main
- name: Bind merge target branch for getattr guard
if: steps.changed-files.outputs.any_changed == 'true'
run: git show-ref --verify --quiet refs/heads/main || git branch main origin/main
- name: Run No New Getattr Guard
if: steps.changed-files.outputs.any_changed == 'true'
run: uv run --project api python scripts/check_no_new_getattr.py --mode ci --merge-target main
run: uv run --project api python scripts/check_no_new_getattr.py --base-rev "${{ inputs.base-rev }}"
- name: Run Type Checks
if: steps.changed-files.outputs.any_changed == 'true'

View File

@ -77,6 +77,10 @@ def assert_has_actionable_violation(stderr: str, path: str) -> None:
assert "no-new-getattr" in stderr
def main_branch_rev(repo: Path) -> str:
return git(repo, "rev-parse", "main")
def test_resolve_ast_grep_command_prefers_ast_grep(monkeypatch: pytest.MonkeyPatch) -> None:
module = load_guard_module()
monkeypatch.setattr(
@ -130,8 +134,46 @@ def test_resolve_ast_grep_command_raises_without_explicit_binary(monkeypatch: py
module.resolve_ast_grep_command()
def test_cli_requires_explicit_diff_source(tmp_path: Path) -> None:
result = run_script(tmp_path)
assert result.returncode == 2
assert "one of the arguments --staged --base-rev is required" in result.stderr
def test_cli_rejects_mixed_diff_sources(tmp_path: Path) -> None:
result = run_script(tmp_path, "--staged", "--base-rev", "deadbeef")
assert result.returncode == 2
assert "not allowed with argument" in result.stderr
def test_cli_help_exposes_only_new_diff_source_flags(tmp_path: Path) -> None:
help_result = run_script(tmp_path, "--help")
assert help_result.returncode == 0
assert "--staged" in help_result.stdout
assert "--base-rev" in help_result.stdout
assert "--mode" not in help_result.stdout
assert "--merge-target" not in help_result.stdout
result = run_script(tmp_path, "--staged", "--mode", "ci")
assert result.returncode == 2
assert "unrecognized arguments: --mode ci" in result.stderr
result = run_script(tmp_path, "--base-rev", "deadbeef", "--merge-target", "main")
assert result.returncode == 2
assert "unrecognized arguments: --merge-target main" in result.stderr
def test_style_workflow_wires_no_new_getattr_guard() -> None:
workflow = (REPO_ROOT / ".github" / "workflows" / "style.yml").read_text(encoding="utf-8")
assert re.search(
r"(?ms)^on:\n workflow_call:\n inputs:\n base-rev:\n required: true\n type: string\n",
workflow,
)
python_style_job = re.search(
r"(?ms)^ python-style:\n(?P<job>.*?)(?=^ [a-z0-9-]+:\n|\Z)",
workflow,
@ -157,8 +199,9 @@ def test_style_workflow_wires_no_new_getattr_guard() -> None:
assert "scripts/check_no_new_getattr.py\n" in files_block
assert "scripts/ast_grep_rules/no_new_getattr.yml\n" in files_block
assert ".github/workflows/style.yml\n" in files_block
assert ".github/workflows/main-ci.yml\n" in files_block
guard_command = "scripts/check_no_new_getattr.py --mode ci --merge-target main"
guard_command = 'scripts/check_no_new_getattr.py --base-rev "${{ inputs.base-rev }}"'
assert guard_command in job_text
guard_step = re.search(
@ -168,55 +211,35 @@ def test_style_workflow_wires_no_new_getattr_guard() -> None:
)
assert guard_step is not None
pre_guard_text = job_text[: guard_step.start()]
step_pattern = r"(?ms)^ - name: [^\n]*\n(?P<step>.*?)(?=^ - name: |\Z)"
fetch_step_text = next(
(
match.group("step")
for match in re.finditer(step_pattern, pre_guard_text)
if any(
re.search(pattern, line)
for line in match.group("step").splitlines()
for pattern in (
r"git fetch .*refs/heads/main:refs/remotes/origin/main",
r"git fetch .*main:refs/remotes/origin/main",
r"git fetch .*refs/remotes/origin/main",
)
)
),
"",
)
assert fetch_step_text
assert "git fetch" in fetch_step_text
assert "origin" in fetch_step_text
assert any(
re.search(pattern, line)
for line in fetch_step_text.splitlines()
for pattern in (
r"git fetch .*refs/heads/main:refs/remotes/origin/main",
r"git fetch .*main:refs/remotes/origin/main",
r"git fetch .*refs/remotes/origin/main",
)
)
bind_step = re.search(
r"(?ms)^ - name: Bind merge target branch for getattr guard\n(?P<step>.*?)(?=^ - name: |\Z)",
pre_guard_text,
)
assert bind_step is not None
bind_step_text = bind_step.group("step")
assert any(
command in bind_step_text
for command in (
"git branch main origin/main",
"git checkout -B main origin/main",
"git switch -C main origin/main",
"git update-ref refs/heads/main refs/remotes/origin/main",
)
)
assert "GITHUB_BASE_SHA" not in guard_step.group("step")
def test_ci_mode_passes_when_only_legacy_getattr_exists(tmp_path: Path) -> None:
def test_main_ci_passes_style_base_rev_input() -> None:
workflow = (REPO_ROOT / ".github" / "workflows" / "main-ci.yml").read_text(encoding="utf-8")
style_job = re.search(
r"(?ms)^ style-check:\n(?P<job>.*?)(?=^ [a-z0-9-]+:\n|\Z)",
workflow,
)
assert style_job is not None
assert "uses: ./.github/workflows/style.yml" in style_job.group("job")
assert (
"base-rev: ${{ github.event.pull_request.base.sha || github.event.merge_group.base_sha }}"
in style_job.group("job")
)
api_filter = re.search(
r"(?ms)^ api:\n(?P<filter>(?:^ - '[^']+'\n)+)",
workflow,
)
assert api_filter is not None
filter_text = api_filter.group("filter")
assert "scripts/check_no_new_getattr.py" in filter_text
assert "scripts/ast_grep_rules/no_new_getattr.yml" in filter_text
assert ".github/workflows/style.yml" in filter_text
assert ".github/workflows/main-ci.yml" in filter_text
def test_base_rev_mode_passes_when_only_legacy_getattr_exists(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -239,12 +262,12 @@ def test_ci_mode_passes_when_only_legacy_getattr_exists(tmp_path: Path) -> None:
)
commit_all(tmp_path, "unrelated change")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", main_branch_rev(tmp_path))
assert result.returncode == 0, result.stderr
def test_ci_mode_fails_for_new_file_with_getattr(tmp_path: Path) -> None:
def test_base_rev_mode_fails_for_new_file_with_getattr(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -267,13 +290,13 @@ def test_ci_mode_fails_for_new_file_with_getattr(tmp_path: Path) -> None:
)
commit_all(tmp_path, "add new getattr usage")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", main_branch_rev(tmp_path))
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/new_usage.py")
def test_ci_mode_fails_for_new_file_with_two_arg_getattr(tmp_path: Path) -> None:
def test_base_rev_mode_fails_for_new_file_with_two_arg_getattr(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -296,13 +319,13 @@ def test_ci_mode_fails_for_new_file_with_two_arg_getattr(tmp_path: Path) -> None
)
commit_all(tmp_path, "add new two-arg getattr usage")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", main_branch_rev(tmp_path))
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/new_usage.py")
def test_ci_mode_fails_for_new_file_with_builtins_getattr(tmp_path: Path) -> None:
def test_base_rev_mode_fails_for_new_file_with_builtins_getattr(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -328,13 +351,13 @@ def test_ci_mode_fails_for_new_file_with_builtins_getattr(tmp_path: Path) -> Non
)
commit_all(tmp_path, "add new builtins getattr usage")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", main_branch_rev(tmp_path))
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/new_usage.py")
def test_ci_mode_fails_for_new_file_with_two_arg_builtins_getattr(tmp_path: Path) -> None:
def test_base_rev_mode_fails_for_new_file_with_two_arg_builtins_getattr(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -360,13 +383,13 @@ def test_ci_mode_fails_for_new_file_with_two_arg_builtins_getattr(tmp_path: Path
)
commit_all(tmp_path, "add new two-arg builtins getattr usage")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", main_branch_rev(tmp_path))
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/new_usage.py")
def test_ci_mode_fails_for_new_file_with_dunder_builtins_getattr(tmp_path: Path) -> None:
def test_base_rev_mode_fails_for_new_file_with_dunder_builtins_getattr(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -389,13 +412,13 @@ def test_ci_mode_fails_for_new_file_with_dunder_builtins_getattr(tmp_path: Path)
)
commit_all(tmp_path, "add new dunder builtins getattr usage")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", main_branch_rev(tmp_path))
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/new_usage.py")
def test_ci_mode_fails_for_new_file_with_two_arg_dunder_builtins_getattr(tmp_path: Path) -> None:
def test_base_rev_mode_fails_for_new_file_with_two_arg_dunder_builtins_getattr(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -418,13 +441,13 @@ def test_ci_mode_fails_for_new_file_with_two_arg_dunder_builtins_getattr(tmp_pat
)
commit_all(tmp_path, "add new two-arg dunder builtins getattr usage")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", main_branch_rev(tmp_path))
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/new_usage.py")
def test_ci_mode_uses_merge_base_against_main_not_just_head_parent(tmp_path: Path) -> None:
def test_base_rev_mode_uses_provided_base_revision_not_head_parent(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -435,6 +458,7 @@ def test_ci_mode_uses_merge_base_against_main_not_just_head_parent(tmp_path: Pat
""",
)
commit_all(tmp_path, "baseline")
base_rev = main_branch_rev(tmp_path)
checkout_feature_branch(tmp_path)
write_repo_file(
@ -457,13 +481,75 @@ def test_ci_mode_uses_merge_base_against_main_not_just_head_parent(tmp_path: Pat
)
commit_all(tmp_path, "later feature commit does not touch violating file")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", base_rev)
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/introduced_earlier.py")
def test_pre_commit_mode_reads_staged_content_only(tmp_path: Path) -> None:
def test_base_rev_mode_works_without_local_main_branch(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
"pkg/existing.py",
"""
def stable() -> str:
return "ok"
""",
)
commit_all(tmp_path, "baseline")
base_rev = main_branch_rev(tmp_path)
checkout_feature_branch(tmp_path)
write_repo_file(
tmp_path,
"pkg/other.py",
"""
def meaning() -> int:
return 42
""",
)
commit_all(tmp_path, "feature change")
git(tmp_path, "checkout", "--detach", "HEAD")
git(tmp_path, "branch", "-D", "main")
result = run_script(tmp_path, "--base-rev", base_rev)
assert result.returncode == 0, result.stderr
def test_base_rev_mode_ignores_github_base_sha_environment(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
"pkg/existing.py",
"""
def stable() -> str:
return "ok"
""",
)
commit_all(tmp_path, "baseline")
base_rev = main_branch_rev(tmp_path)
checkout_feature_branch(tmp_path)
write_repo_file(
tmp_path,
"pkg/other.py",
"""
def meaning() -> int:
return 42
""",
)
commit_all(tmp_path, "feature change")
monkeypatch.setenv("GITHUB_BASE_SHA", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef")
result = run_script(tmp_path, "--base-rev", base_rev)
assert result.returncode == 0, result.stderr
def test_staged_mode_reads_staged_content_only(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -494,12 +580,12 @@ def test_pre_commit_mode_reads_staged_content_only(tmp_path: Path) -> None:
""",
)
result = run_script(tmp_path, "--mode", "pre-commit")
result = run_script(tmp_path, "--staged")
assert result.returncode == 0, result.stderr
def test_pre_commit_mode_fails_for_staged_two_arg_getattr(tmp_path: Path) -> None:
def test_staged_mode_fails_for_staged_two_arg_getattr(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -521,13 +607,13 @@ def test_pre_commit_mode_fails_for_staged_two_arg_getattr(tmp_path: Path) -> Non
)
git(tmp_path, "add", "pkg/module.py")
result = run_script(tmp_path, "--mode", "pre-commit")
result = run_script(tmp_path, "--staged")
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/module.py")
def test_pre_commit_mode_fails_for_staged_builtins_getattr(tmp_path: Path) -> None:
def test_staged_mode_fails_for_staged_builtins_getattr(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -552,13 +638,13 @@ def test_pre_commit_mode_fails_for_staged_builtins_getattr(tmp_path: Path) -> No
)
git(tmp_path, "add", "pkg/module.py")
result = run_script(tmp_path, "--mode", "pre-commit")
result = run_script(tmp_path, "--staged")
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/module.py")
def test_pre_commit_mode_fails_for_staged_two_arg_builtins_getattr(tmp_path: Path) -> None:
def test_staged_mode_fails_for_staged_two_arg_builtins_getattr(tmp_path: Path) -> None:
init_repo(tmp_path)
write_repo_file(
tmp_path,
@ -583,7 +669,7 @@ def test_pre_commit_mode_fails_for_staged_two_arg_builtins_getattr(tmp_path: Pat
)
git(tmp_path, "add", "pkg/module.py")
result = run_script(tmp_path, "--mode", "pre-commit")
result = run_script(tmp_path, "--staged")
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/module.py")
@ -603,6 +689,7 @@ def test_modified_hunk_with_same_getattr_count_is_allowed(tmp_path: Path) -> Non
""",
)
commit_all(tmp_path, "baseline")
base_rev = main_branch_rev(tmp_path)
checkout_feature_branch(tmp_path)
write_repo_file(
@ -618,7 +705,7 @@ def test_modified_hunk_with_same_getattr_count_is_allowed(tmp_path: Path) -> Non
)
commit_all(tmp_path, "touch legacy getattr hunk")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", base_rev)
assert result.returncode == 0, result.stderr
@ -635,6 +722,7 @@ def test_modified_hunk_with_decreased_getattr_count_is_allowed(tmp_path: Path) -
""",
)
commit_all(tmp_path, "baseline")
base_rev = main_branch_rev(tmp_path)
checkout_feature_branch(tmp_path)
write_repo_file(
@ -647,7 +735,7 @@ def test_modified_hunk_with_decreased_getattr_count_is_allowed(tmp_path: Path) -
)
commit_all(tmp_path, "remove one legacy getattr")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", base_rev)
assert result.returncode == 0, result.stderr
@ -663,6 +751,7 @@ def test_modified_hunk_with_increased_getattr_count_fails(tmp_path: Path) -> Non
""",
)
commit_all(tmp_path, "baseline")
base_rev = main_branch_rev(tmp_path)
checkout_feature_branch(tmp_path)
write_repo_file(
@ -676,7 +765,7 @@ def test_modified_hunk_with_increased_getattr_count_fails(tmp_path: Path) -> Non
)
commit_all(tmp_path, "add one more getattr")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", base_rev)
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/sample.py")
@ -694,6 +783,7 @@ def test_inline_noqa_suppression_with_explanatory_text_skips_added_getattr(tmp_p
""",
)
commit_all(tmp_path, "baseline")
base_rev = main_branch_rev(tmp_path)
checkout_feature_branch(tmp_path)
write_repo_file(
@ -706,7 +796,7 @@ def test_inline_noqa_suppression_with_explanatory_text_skips_added_getattr(tmp_p
)
commit_all(tmp_path, "add suppressed getattr")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", base_rev)
assert "no-new-getattr needed for plugin-defined attributes" in (tmp_path / "pkg/existing.py").read_text(
encoding="utf-8"
@ -725,6 +815,7 @@ def test_inline_noqa_without_explanatory_text_is_not_sufficient(tmp_path: Path)
""",
)
commit_all(tmp_path, "baseline")
base_rev = main_branch_rev(tmp_path)
checkout_feature_branch(tmp_path)
write_repo_file(
@ -737,7 +828,7 @@ def test_inline_noqa_without_explanatory_text_is_not_sufficient(tmp_path: Path)
)
commit_all(tmp_path, "add bare noqa getattr")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", base_rev)
assert result.returncode == 1
assert_has_actionable_violation(result.stderr, "pkg/existing.py")
@ -753,6 +844,7 @@ def test_non_python_file_with_getattr_text_does_not_fail_guard(tmp_path: Path) -
""",
)
commit_all(tmp_path, "baseline")
base_rev = main_branch_rev(tmp_path)
checkout_feature_branch(tmp_path)
write_repo_file(
@ -764,6 +856,6 @@ def test_non_python_file_with_getattr_text_does_not_fail_guard(tmp_path: Path) -
)
commit_all(tmp_path, "document getattr example")
result = run_script(tmp_path, "--mode", "ci", "--merge-target", "main")
result = run_script(tmp_path, "--base-rev", base_rev)
assert result.returncode == 0, result.stderr

View File

@ -1,5 +1,5 @@
#!/usr/bin/env python3
"""Block net-new getattr() usage in changed Python hunks."""
"""Block net-new getattr() usage in staged changes or against an explicit base revision."""
from __future__ import annotations
@ -58,8 +58,16 @@ class Violation:
def parse_args() -> argparse.Namespace:
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("--mode", choices=("pre-commit", "ci"), required=True)
parser.add_argument("--merge-target", default="main")
diff_source_group = parser.add_mutually_exclusive_group(required=True)
diff_source_group.add_argument(
"--staged",
action="store_true",
help="Inspect staged changes against HEAD, for pre-commit style usage.",
)
diff_source_group.add_argument(
"--base-rev",
help="Inspect changes between the provided git revision and HEAD.",
)
return parser.parse_args()
@ -86,21 +94,15 @@ def git_output(*args: str, allow_missing: bool = False) -> str:
raise RuntimeError(completed.stderr.strip() or completed.stdout.strip() or "git command failed")
def resolve_ci_base(merge_target: str) -> str:
merge_base = git_output("merge-base", merge_target, "HEAD", allow_missing=True).strip()
return merge_base or merge_target
def collect_diff_text(args: argparse.Namespace) -> str:
if args.mode == "pre-commit":
if args.staged:
return git_output("diff", "--cached", "--unified=0", "--diff-filter=AM", "--no-ext-diff")
compare_base = resolve_ci_base(args.merge_target)
return git_output(
"diff",
"--unified=0",
"--diff-filter=AM",
"--no-ext-diff",
f"{compare_base}..HEAD",
f"{args.base_rev}..HEAD",
)
@ -145,15 +147,14 @@ def is_python_source_path(path: str) -> bool:
def load_file_versions(path: str, args: argparse.Namespace) -> tuple[str, str]:
if args.mode == "pre-commit":
if args.staged:
return (
git_output("show", f"HEAD:{path}", allow_missing=True),
git_output("show", f":{path}"),
)
compare_base = resolve_ci_base(args.merge_target)
return (
git_output("show", f"{compare_base}:{path}", allow_missing=True),
git_output("show", f"{args.base_rev}:{path}", allow_missing=True),
git_output("show", f"HEAD:{path}"),
)