diff --git a/github.py b/github.py index 1883400..f9c105f 100644 --- a/github.py +++ b/github.py @@ -13,6 +13,7 @@ from tenacity import Retrying, before_sleep_log, stop_after_attempt, wait_exponential from config import get_github_orgs +from implementation_lines import count_implementation_additions from time_window import TimeWindow load_dotenv() @@ -33,6 +34,7 @@ ) GITHUB_GRAPHQL_EXECUTE_TIMEOUT_SECONDS = 30 CURSOR_AGENT_LOGIN = "cursoragent" +REVIEW_REMINDER_MAX_IMPLEMENTATION_ADDITIONS = 200 _cursor_pr_cache_lock = threading.Lock() @@ -153,6 +155,15 @@ def get_prs(repo_id, pr_states, repo_name=None): closedAt isDraft additions + files(first: 100) { + pageInfo { + hasNextPage + } + nodes { + path + additions + } + } reviews( first: 10, states: [APPROVED, CHANGES_REQUESTED] @@ -667,16 +678,19 @@ def get_prs_waiting_for_review_by_reviewer(): Includes pull requests with an open review request or active requested-changes reviewer that has been waiting more than 24 hours. Approved PRs are excluded - even if GitHub still has leftover review requests. Only includes PRs with - fewer than 200 lines added. + even if GitHub still has leftover review requests. """ all_prs = _get_all_prs(["OPEN"]) stuck_prs = {} threshold = datetime.now(timezone.utc) - timedelta(hours=24) for pr in all_prs: - additions = pr.get("additions") - if additions is None or additions >= 200: + implementation_additions = count_implementation_additions(pr) + if ( + implementation_additions is None + or implementation_additions >= REVIEW_REMINDER_MAX_IMPLEMENTATION_ADDITIONS + ): continue + pr["implementation_additions"] = implementation_additions if has_known_merge_conflicts(pr): continue if has_required_approval(pr): diff --git a/implementation_lines.py b/implementation_lines.py new file mode 100644 index 0000000..ea39462 --- /dev/null +++ b/implementation_lines.py @@ -0,0 +1,108 @@ +from __future__ import annotations + +import fnmatch +import posixpath +from typing import Any + +EXCLUDED_DIRECTORIES = frozenset( + { + "__tests__", + "__test__", + "test", + "tests", + "spec", + "specs", + "__mocks__", + "__fixtures__", + "fixtures", + "__snapshots__", + "e2e", + "cypress", + "docs", + "doc", + "__generated__", + "generated", + "locales", + "locale", + "i18n", + "translations", + ".storybook", + ".cursor", + ".claude", + } +) + +EXCLUDED_FILENAME_PATTERNS = ( + "*.test.*", + "*.tests.*", + "*.spec.*", + "*_test.py", + "test_*.py", + "conftest.py", + "*_test.go", + "*_spec.rb", + "*.snap", + "*.stories.*", + "*.md", + "*.mdx", + "*.rst", + "license*", + "changelog*", + "codeowners", + ".cursorrules", + "*.lock", + "package-lock.json", + "pnpm-lock.yaml", + "*.generated.*", + "*.min.js", + "*.min.css", + "*.pb.go", + "*_pb2.py", + "*_pb2_grpc.py", + "*.po", + "*.pot", + "*.png", + "*.jpg", + "*.jpeg", + "*.gif", + "*.svg", + "*.webp", + "*.ico", + "*.pdf", + "*.ttf", + "*.otf", + "*.woff", + "*.woff2", + "*.mp3", + "*.mp4", + "*.mov", +) + + +def is_implementation_path(path: str) -> bool: + normalized = path.lower() + if not normalized: + return False + directory, filename = posixpath.split(normalized) + if any(segment in EXCLUDED_DIRECTORIES for segment in directory.split("/")): + return False + return not any(fnmatch.fnmatchcase(filename, pattern) for pattern in EXCLUDED_FILENAME_PATTERNS) + + +def count_implementation_additions(pr: dict[str, Any]) -> int | None: + """Sum additions across a PR's implementation files. + + Falls back to the PR's total additions when GitHub did not return the file + list or truncated it, so an unclassifiable PR is never under-counted. + """ + files = pr.get("files") or {} + nodes = files.get("nodes") + truncated = (files.get("pageInfo") or {}).get("hasNextPage", False) + if nodes is None or truncated: + total = pr.get("additions") + return int(total) if total is not None else None + return sum( + int(node.get("additions") or 0) + for node in nodes + if is_implementation_path(node.get("path") or "") + ) diff --git a/jobs.py b/jobs.py index db4afd3..4b154f0 100644 --- a/jobs.py +++ b/jobs.py @@ -15,6 +15,7 @@ from constants import ENGINEERING_TEAM_SLUG, PRIORITY_TO_SCORE from fleet_health_cache import refresh_fleet_health_cache, should_use_redis_cache from github import ( + REVIEW_REMINDER_MAX_IMPLEMENTATION_ADDITIONS, GitHubDataError, get_merged_pr_activity, get_merged_pr_counts_for_user, @@ -646,7 +647,12 @@ def post_stale(): filtered[reviewer] = pr_list prs = filtered if prs: - markdown += "*PRs - Checks Passing, Waiting for Review (+24h, <200 lines added)*\n" + markdown += ( + "*PRs - Checks Passing, Waiting for Review " + f"(+24h, <{REVIEW_REMINDER_MAX_IMPLEMENTATION_ADDITIONS} implementation lines added)*\n" + "_Implementation lines exclude tests, fixtures, snapshots, docs, lockfiles, " + "generated code, localization files and assets._\n" + ) for reviewer, pr_list in prs.items(): if not pr_list: continue @@ -676,7 +682,10 @@ def post_stale(): pr_days.append((days_waiting, pr)) for days_waiting, pr in sorted(pr_days, key=lambda x: x[0], reverse=True): - markdown += f"- <{pr['url']}|{pr['title']}> (+{days_waiting}d)\n" + markdown += ( + f"- <{pr['url']}|{pr['title']}> " + f"(+{days_waiting}d, {pr['implementation_additions']} impl lines)\n" + ) markdown += "\n\n" filtered_stale_issues = { diff --git a/tests/test_gql_client_requests.py b/tests/test_gql_client_requests.py index f2a6830..770970d 100644 --- a/tests/test_gql_client_requests.py +++ b/tests/test_gql_client_requests.py @@ -317,6 +317,7 @@ def test_get_prs_retries_only_failed_page_and_keeps_complete_results(self): sleep.assert_called_once_with(1) query = print_ast(execute.call_args.args[0].document) self.assertIn("first: 20", query) + self.assertIn("files(first: 100)", query) def test_get_prs_raises_when_repo_fetch_fails(self): with ( @@ -404,6 +405,50 @@ def now(cls, tz=None): ) self.assertEqual(waiting, expected) + def test_waiting_for_review_gates_on_implementation_additions(self): + def stuck_pr(number, additions, files): + return { + "number": number, + "url": f"https://github.com/example/repo/pull/{number}", + "additions": additions, + "files": {"pageInfo": {"hasNextPage": False}, "nodes": files}, + "mergeable": "MERGEABLE", + "reviewDecision": "REVIEW_REQUIRED", + "reviewRequests": {"nodes": [{"requestedReviewer": {"login": "darrylyip"}}]}, + "reviews": {"nodes": []}, + "timelineItems": { + "nodes": [ + { + "createdAt": "2020-01-01T00:00:00Z", + "requestedReviewer": {"login": "darrylyip"}, + } + ] + }, + "statusCheckRollup": {"state": "SUCCESS"}, + } + + mostly_tests = stuck_pr( + 1, + 309, + [ + {"path": "src/core/schema.js", "additions": 17}, + {"path": "src/data/prayers/__tests__/resolver.tests.js", "additions": 191}, + {"path": "src/data/prayers/dataSource.js", "additions": 93}, + {"path": "src/data/prayers/resolver.js", "additions": 8}, + ], + ) + large_implementation = stuck_pr( + 2, 250, [{"path": "src/data/giving/dataSource.js", "additions": 250}] + ) + + with patch.object( + github, "_get_all_prs", return_value=[mostly_tests, large_implementation] + ): + waiting = github.get_prs_waiting_for_review_by_reviewer() + + self.assertEqual(waiting, {"darrylyip": [mostly_tests]}) + self.assertEqual(mostly_tests["implementation_additions"], 118) + def test_waiting_for_review_allows_unknown_mergeability(self): class FixedDateTime(datetime): @classmethod diff --git a/tests/test_implementation_lines.py b/tests/test_implementation_lines.py new file mode 100644 index 0000000..8067554 --- /dev/null +++ b/tests/test_implementation_lines.py @@ -0,0 +1,84 @@ +import unittest + +from implementation_lines import count_implementation_additions, is_implementation_path + + +class IsImplementationPathTest(unittest.TestCase): + def test_classifies_paths(self): + cases = { + "src/data/prayers/dataSource.js": True, + ".github/workflows/ci.yml": True, + "src/data/prayers/__tests__/resolver.tests.js": False, + "src/data/prayers/__tests__/helpers.js": False, + "tests/helpers.py": False, + "e2e/setup.ts": False, + "docs/index.html": False, + "src/components/__snapshots__/notes.json": False, + "src/Foo.Test.js": False, + "src/utils/format.test.ts": False, + "e2e/giving.spec.ts": False, + "tests/test_regressions.py": False, + "shovel/conftest.py": False, + "pkg/sync/sync_test.go": False, + "src/components/__snapshots__/Card.test.tsx.snap": False, + "src/components/Card.stories.tsx": False, + "README.md": False, + "docs/setup.mdx": False, + ".cursor/rules/style.mdc": False, + "yarn.lock": False, + "package-lock.json": False, + "src/__generated__/graphql.ts": False, + "src/api/types.generated.ts": False, + "src/locales/en.json": False, + "assets/logo.svg": False, + "fonts/Inter.woff2": False, + "": False, + } + for path, expected in cases.items(): + with self.subTest(path=path): + self.assertEqual(is_implementation_path(path), expected) + + +class CountImplementationAdditionsTest(unittest.TestCase): + def test_sums_only_implementation_files(self): + pr = { + "additions": 309, + "files": { + "pageInfo": {"hasNextPage": False}, + "nodes": [ + {"path": "src/core/schema.js", "additions": 17}, + {"path": "src/data/prayers/__tests__/resolver.tests.js", "additions": 191}, + {"path": "src/data/prayers/dataSource.js", "additions": 93}, + {"path": "src/data/prayers/resolver.js", "additions": 8}, + ], + }, + } + self.assertEqual(count_implementation_additions(pr), 118) + + def test_falls_back_to_total_when_file_list_is_truncated(self): + pr = { + "additions": 49995, + "files": { + "pageInfo": {"hasNextPage": True}, + "nodes": [{"path": "src/a.js", "additions": 1}], + }, + } + self.assertEqual(count_implementation_additions(pr), 49995) + + def test_falls_back_to_total_when_files_are_missing(self): + self.assertEqual(count_implementation_additions({"additions": 12}), 12) + self.assertEqual(count_implementation_additions({"additions": 12, "files": None}), 12) + + def test_returns_none_without_any_size_data(self): + self.assertIsNone(count_implementation_additions({})) + + def test_tolerates_missing_file_fields(self): + pr = { + "additions": 5, + "files": {"nodes": [{"path": None, "additions": 3}, {"path": "src/a.py"}]}, + } + self.assertEqual(count_implementation_additions(pr), 0) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_jobs.py b/tests/test_jobs.py index ff2609c..1decfa6 100644 --- a/tests/test_jobs.py +++ b/tests/test_jobs.py @@ -53,6 +53,7 @@ def _install_import_shims() -> None: github_module = cast(Any, types.ModuleType("github")) github_module.GitHubDataError = type("GitHubDataError", (RuntimeError,), {}) + github_module.REVIEW_REMINDER_MAX_IMPLEMENTATION_ADDITIONS = 200 github_module.get_prs_waiting_for_review_by_reviewer = lambda *args, **kwargs: {} github_module.get_merged_pr_activity = lambda *args, **kwargs: ({}, {}) github_module.get_merged_pr_counts_for_user = lambda *args, **kwargs: (0, 0) @@ -361,6 +362,36 @@ def fake_get_stale_issues(issues, days): self.assertIn("APO-7555", message) self.assertIn("(74d)", message) + def _post_stale_message(self, team_members, prs_by_reviewer): + with ( + patch.object(jobs_module, "get_team_members", return_value=team_members), + patch.object( + jobs_module, + "get_prs_waiting_for_review_by_reviewer", + return_value=prs_by_reviewer, + ), + patch.object(jobs_module, "get_open_stale_issues", return_value=[]), + patch.object(jobs_module, "get_stale_issues_by_assignee", return_value={}), + patch.dict(jobs_module.os.environ, {"APP_URL": "https://bug-board.example"}), + patch.object(jobs_module, "post_to_slack") as post, + ): + jobs_module.post_stale() + return post.call_args.args[0] + + def test_review_reminders_show_implementation_lines(self): + pr = { + "title": "Group prayer list via forwarded-community consent", + "url": "https://github.com/ApollosProject/apollos-cluster/pull/4678", + "implementation_additions": 118, + "timelineItems": {"nodes": []}, + } + message = self._post_stale_message( + {"vincent": {"github_username": "vincentwilson", "slack_id": "U0VW"}}, + {"vincentwilson": [pr]}, + ) + self.assertIn("<200 implementation lines added", message) + self.assertIn("(+0d, 118 impl lines)", message) + def test_continues_with_linear_stale_issues_when_github_pr_fetch_fails(self): open_issues = [{"id": "APO-7555"}]