Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .github/workflows/deleted-symbols-gate.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
name: Deleted symbols gate

# Detects PRs that silently delete Python symbols (def/class/test names) that
# landed on the target branch after the PR's merge base. A PR cut before
# hardening commits landed on dev would silently delete them when merged --
# git reports "Automatic merge went well" with no conflict because the PR
# branch simply wins on files dev touched after the branch point.
#
# This check fails such PRs and names the deleted symbols and the commits that
# added them. A "Removes-Intentionally: <symbol>" trailer in the PR body
# waives named symbols, making deliberate deletions a conscious, auditable act.
#
# See scripts/check_deleted_symbols.py for the implementation.

on:
pull_request:
branches: [master, dev]

jobs:
deleted-symbols-gate:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v7
with:
fetch-depth: 0

Comment on lines +23 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

2. Gate checks merge ref 🐞 Bug ≡ Correctness

The workflow likely runs on the pull_request synthetic merge commit (refs/pull/<n>/merge) and does
not explicitly checkout the PR head SHA; in that case git merge-base HEAD origin/<base> becomes
the base tip, making dev_symbols and mb_symbols identical and the signal set always empty. As a
result, the deleted-symbols gate can report "clean" even when the PR would silently delete symbols
added to the base branch after the branch point.
Agent Prompt
## Issue description
`deleted-symbols-gate.yml` checks out the default ref for `pull_request` events, which is typically the synthetic merge commit. The guard script computes `git merge-base HEAD <base>`, and when `HEAD` is that merge commit the merge-base resolves to the base tip, making the signal empty and disabling the gate.

## Issue Context
The guard’s algorithm depends on `HEAD` being the PR branch head (not the merge result). The repo already documents that PR events use `refs/pull/<n>/merge`.

## Fix Focus Areas
- .github/workflows/deleted-symbols-gate.yml[23-39]
  - Update `actions/checkout` to explicitly checkout the PR head commit, e.g.:
    - `with: ref: ${{ github.event.pull_request.head.sha }}` (keep `fetch-depth: 0`).
  - Keep the existing `git fetch origin "$BASE_REF"` step so `origin/${{ github.base_ref }}` is available.

(Alternative fix if you prefer not to change checkout: extend the script to accept a `--head <ref>` argument and compute merge-base against that ref instead of `HEAD`, then pass it from the workflow.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

- uses: actions/setup-python@v7
with:
python-version: "3.12"

Comment on lines +27 to +30

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Workflow uses python 3.12 📜 Skill insight ≡ Correctness

The new deleted-symbols-gate workflow pins python-version: "3.12", which does not enforce the
required minimum runtime target of Python 3.11 for Python code in this repo. This can allow
3.12-only syntax/features to slip in without being caught by CI running on the mandated baseline.
Agent Prompt
## Issue description
The workflow `.github/workflows/deleted-symbols-gate.yml` is configured to run with Python 3.12, but the compliance requirement is to target Python 3.11 as the minimum supported version.

## Issue Context
CI should validate against the minimum supported Python version so that 3.12-only syntax/features do not merge unnoticed.

## Fix Focus Areas
- .github/workflows/deleted-symbols-gate.yml[27-30]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

- name: Fetch base branch
env:
BASE_REF: ${{ github.base_ref }}
run: git fetch origin "$BASE_REF"

- name: Check for silently deleted symbols
env:
PR_BODY: ${{ github.event.pull_request.body }}
run: python scripts/check_deleted_symbols.py --base "origin/${{ github.base_ref }}"
249 changes: 249 additions & 0 deletions scripts/check_deleted_symbols.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,249 @@
#!/usr/bin/env python3
"""Deleted-symbols guard.

Detects PRs that silently delete code added to the target branch after the
merge base. This catches the "clean merge, no conflict" failure mode where a
PR branch cut before hardening commits landed on dev would silently delete
them when merged -- git reports "Automatic merge went well" with no conflict
because the PR branch simply wins on files dev touched after the branch point.

Algorithm:
1. Compute the merge base of HEAD and the target branch.
2. Extract all Python def/class symbols at three points: the target branch
head, HEAD (the PR branch), and the merge base.
3. A symbol is "deleted by the PR" if it exists at the target head but not
at HEAD. It is "added after the merge base" if it does not exist at the
merge base. The intersection is the signal: work that landed on dev after
the branch was cut and is being silently removed.
4. Fail with an explicit list of the deleted symbols and the commits that
added them.
5. A "Removes-Intentionally: <symbol>, ..." trailer in the PR body waives
named symbols, making deliberate deletions a conscious, auditable act.

Narrow by design: Python def/class names only (test functions are defs).
No semantic analysis -- name-level matching catches every instance hit so far.

Usage:
python scripts/check_deleted_symbols.py --base origin/dev
python scripts/check_deleted_symbols.py --base origin/dev --pr-body "..."
python scripts/check_deleted_symbols.py --base origin/dev --waived "path/file.py:func"
"""
from __future__ import annotations

import argparse
import ast
import io
import os
import subprocess
import sys
import tarfile
from dataclasses import dataclass
from pathlib import Path

REPO_ROOT = Path(__file__).resolve().parent.parent
TRAILER = "Removes-Intentionally:"


@dataclass
class Violation:
symbol: str
added_by: str


def _run_git(args: list[str], cwd: str | Path | None = None) -> str:
result = subprocess.run(
["git", *args], cwd=cwd, capture_output=True, text=True, check=True,
)
return result.stdout


def _merge_base(base_ref: str, repo_root: Path = REPO_ROOT) -> str:
return _run_git(["merge-base", "HEAD", base_ref], cwd=repo_root).strip()


def _extract_symbols(source: str, file_path: str) -> dict[str, str]:
"""Extract symbol identifiers from Python source code.

Returns a dict mapping "file_path:qualified_name" to kind ("def" or
"class"). Qualified names use dot notation for nested definitions
(e.g. ClassName.method, Outer.Inner).
"""
symbols: dict[str, str] = {}
try:
tree = ast.parse(source)
except SyntaxError:
return symbols

def visit(node: ast.AST, prefix: str = "") -> None:
for child in ast.iter_child_nodes(node):
if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)):
name = f"{prefix}{child.name}" if prefix else child.name
symbols[f"{file_path}:{name}"] = "def"
visit(child, f"{name}.")
elif isinstance(child, ast.ClassDef):
name = f"{prefix}{child.name}" if prefix else child.name
symbols[f"{file_path}:{name}"] = "class"
visit(child, f"{name}.")
else:
visit(child, prefix)

visit(tree)
return symbols


def _get_symbols_at_ref(ref: str, repo_root: Path = REPO_ROOT) -> dict[str, str]:
"""Get all Python symbols at a given git ref using git archive."""
result = subprocess.run(
["git", "archive", ref],
cwd=repo_root, capture_output=True, check=True,
)
symbols: dict[str, str] = {}
with tarfile.open(fileobj=io.BytesIO(result.stdout)) as tar:
for member in tar.getmembers():
if not member.name.endswith(".py"):
continue
f = tar.extractfile(member)
if f is None:
continue
source = f.read().decode("utf-8", errors="ignore")
symbols.update(_extract_symbols(source, member.name))
return symbols


def _find_adding_commit(
file_path: str,
name: str,
kind: str,
merge_base: str,
target: str,
repo_root: Path = REPO_ROOT,
) -> str:
"""Find the commit that added a symbol to the target branch after merge base."""
leaf = name.split(".")[-1]
search = f"{kind} {leaf}("
out = _run_git(
["log", "--oneline", "--reverse", "-S", search, f"{merge_base}..{target}", "--", file_path],
cwd=repo_root,
)
lines = out.splitlines()
if lines:
return lines[0]
# Fallback: try without the opening paren (for classes without bases,
# e.g. `class Foo:` rather than `class Foo(Bar):`).
search = f"{kind} {leaf}"
out = _run_git(
["log", "--oneline", "--reverse", "-S", search, f"{merge_base}..{target}", "--", file_path],
cwd=repo_root,
)
lines = out.splitlines()
if lines:
return lines[0]
return "unknown"
Comment on lines +131 to +141

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Fallback class-name search can match a longer name sharing the same prefix.

The fallback pattern f"{kind} {leaf}" (no trailing ( or :) is a raw substring match. For class Foo: it also matches class FooBar: or class FooExtended(Base):, so _find_adding_commit can attribute a deleted Foo class to the commit that actually added an unrelated FooBar/FooExtended class. This only corrupts the "added by" message (the pass/fail signal itself is computed elsewhere via exact symbol keys), but that message is the tool's main auditability payoff.

🐛 Suggested fix: anchor the fallback to a word boundary
     # Fallback: try without the opening paren (for classes without bases,
     # e.g. `class Foo:` rather than `class Foo(Bar):`).
-    search = f"{kind} {leaf}"
+    search = f"{kind} {leaf}:"
     out = _run_git(
         ["log", "--oneline", "--reverse", "-S", search, f"{merge_base}..{target}", "--", file_path],
         cwd=repo_root,
     )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Fallback: try without the opening paren (for classes without bases,
# e.g. `class Foo:` rather than `class Foo(Bar):`).
search = f"{kind} {leaf}"
out = _run_git(
["log", "--oneline", "--reverse", "-S", search, f"{merge_base}..{target}", "--", file_path],
cwd=repo_root,
)
lines = out.splitlines()
if lines:
return lines[0]
return "unknown"
# Fallback: try without the opening paren (for classes without bases,
# e.g. `class Foo:` rather than `class Foo(Bar):`).
search = f"{kind} {leaf}:"
out = _run_git(
["log", "--oneline", "--reverse", "-S", search, f"{merge_base}..{target}", "--", file_path],
cwd=repo_root,
)
lines = out.splitlines()
if lines:
return lines[0]
return "unknown"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/check_deleted_symbols.py` around lines 131 - 141, Update the fallback
search pattern in _find_adding_commit to require a word boundary after leaf,
preventing names such as FooBar or FooExtended from matching a requested Foo
symbol. Preserve the existing git search flow and unknown fallback while
ensuring the exact class or symbol name is matched.



def parse_waived_symbols(pr_body: str | None) -> set[str]:
"""Parse Removes-Intentionally trailer from PR body text."""
waived: set[str] = set()
if not pr_body:
return waived
for line in pr_body.splitlines():
line = line.strip()
if line.startswith(TRAILER):
symbols_str = line[len(TRAILER):].strip()
for sym in symbols_str.split(","):
sym = sym.strip()
if sym:
waived.add(sym)
return waived


def find_signal_symbols(
dev_symbols: dict[str, str],
head_symbols: dict[str, str],
mb_symbols: dict[str, str],
) -> dict[str, str]:
"""Find symbols at dev that are not at HEAD and not at merge base."""
dev_keys = set(dev_symbols.keys())
head_keys = set(head_symbols.keys())
mb_keys = set(mb_symbols.keys())
signal_keys = dev_keys - head_keys - mb_keys
return {k: dev_symbols[k] for k in signal_keys}


def check_deleted_symbols(
base_ref: str,
repo_root: Path = REPO_ROOT,
pr_body: str | None = None,
waived: set[str] | None = None,
) -> tuple[list[Violation], set[str]]:
"""Main check function.

Returns (violations, waived_symbols) where violations is a list of
Violation objects for non-waived signal symbols, and waived_symbols is
the set of signal symbols that were waived.
"""
merge_base = _merge_base(base_ref, repo_root)

dev_symbols = _get_symbols_at_ref(base_ref, repo_root)
head_symbols = _get_symbols_at_ref("HEAD", repo_root)
mb_symbols = _get_symbols_at_ref(merge_base, repo_root)

signal = find_signal_symbols(dev_symbols, head_symbols, mb_symbols)

waived_set: set[str] = set()
if waived:
waived_set.update(waived)
waived_set.update(parse_waived_symbols(pr_body))

violations: list[Violation] = []
waived_in_signal: set[str] = set()
for symbol, kind in signal.items():
if symbol in waived_set:
waived_in_signal.add(symbol)
continue
file_path, name = symbol.rsplit(":", 1)
added_by = _find_adding_commit(file_path, name, kind, merge_base, base_ref, repo_root)
violations.append(Violation(symbol=symbol, added_by=added_by))

return violations, waived_in_signal


def main(argv: list[str] | None = None) -> int:
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("--base", required=True, help="Target branch ref (e.g. origin/dev)")
parser.add_argument("--pr-body", default=None, help="PR body text (for Removes-Intentionally trailer)")
parser.add_argument("--waived", default=None, help="Comma-separated list of waived symbols")
args = parser.parse_args(argv)

pr_body = args.pr_body
if pr_body is None:
pr_body = os.environ.get("PR_BODY")

waived: set[str] = set()
if args.waived:
for sym in args.waived.split(","):
sym = sym.strip()
if sym:
waived.add(sym)

violations, waived_symbols = check_deleted_symbols(args.base, REPO_ROOT, pr_body, waived)

if waived_symbols:
for sym in sorted(waived_symbols):
print(f"deleted-symbols-guard: waived via Removes-Intentionally: {sym}")

if violations:
print(
f"DELETED-SYMBOLS FAIL: this PR deletes {len(violations)} symbol(s) that "
f"landed on dev after your branch was cut:"
)
for v in violations:
print(f" - {v.symbol} (added by {v.added_by})")
return 1

print("deleted-symbols-guard: clean")
return 0


if __name__ == "__main__":
sys.exit(main())
Loading
Loading