Skip large files in PR summaries (#692)
Co-authored-by: UltralyticsAssistant <[email protected]>
This commit is contained in:
Glenn Jocher
co-authored by
UltralyticsAssistant
UltralyticsAssistant
parent
58cff3386f
commit
669f1245fd
+1
-1
@@ -27,4 +27,4 @@
|
||||
# ├── test_summarize_pr.py
|
||||
# └── ...
|
||||
|
||||
__version__ = "0.2.9"
|
||||
__version__ = "0.2.10"
|
||||
|
||||
@@ -7,7 +7,15 @@ import time
|
||||
|
||||
from . import review_pr
|
||||
from .summarize_pr import SUMMARY_MARKER
|
||||
from .utils import ACTIONS_CREDIT, Action, filter_labels, get_pr_open_response, get_response, remove_html_comments
|
||||
from .utils import (
|
||||
ACTIONS_CREDIT,
|
||||
Action,
|
||||
filter_labels,
|
||||
format_skipped_files_dropdown,
|
||||
get_pr_open_response,
|
||||
get_response,
|
||||
remove_html_comments,
|
||||
)
|
||||
|
||||
BLOCK_USER = os.getenv("BLOCK_USER", "false").lower() == "true"
|
||||
AUTO_PR_REVIEW = os.getenv("REVIEW", "true").lower() == "true"
|
||||
@@ -192,7 +200,8 @@ def main(*args, **kwargs):
|
||||
|
||||
if summary := response.get("summary"):
|
||||
print("Updating PR description with summary...")
|
||||
event.update_pr_description(number, f"{SUMMARY_MARKER}\n\n{ACTIONS_CREDIT}\n\n{summary}")
|
||||
skipped_dropdown = format_skipped_files_dropdown(response.get("skipped_files", []))
|
||||
event.update_pr_description(number, f"{SUMMARY_MARKER}\n\n{ACTIONS_CREDIT}\n\n{summary}{skipped_dropdown}")
|
||||
else:
|
||||
summary = body
|
||||
|
||||
|
||||
+3
-38
@@ -11,50 +11,21 @@ from .utils import (
|
||||
GITHUB_API_URL,
|
||||
MAX_PROMPT_CHARS,
|
||||
Action,
|
||||
format_skipped_files_dropdown,
|
||||
get_response,
|
||||
remove_html_comments,
|
||||
sanitize_ai_text,
|
||||
should_skip_file,
|
||||
)
|
||||
|
||||
REVIEW_MARKER = "## 🔍 PR Review"
|
||||
ERROR_MARKER = "⚠️ Review generation encountered an error"
|
||||
EMOJI_MAP = {"CRITICAL": "❗", "HIGH": "⚠️", "MEDIUM": "💡", "LOW": "📝", "SUGGESTION": "💭"}
|
||||
SKIP_PATTERN_STRINGS = [
|
||||
r"\.lock$", # Lock files
|
||||
r"-lock\.(json|yaml|yml)$",
|
||||
r"\.min\.(js|css)$", # Minified
|
||||
r"\.bundle\.(js|css)$",
|
||||
r"(^|/)dist/", # Generated/vendored directories
|
||||
r"(^|/)build/",
|
||||
r"(^|/)vendor/",
|
||||
r"(^|/)node_modules/",
|
||||
r"(^|/)coverage/", # Coverage reports
|
||||
r"\.pb\.py$", # Proto generated
|
||||
r"_pb2\.py$",
|
||||
r"_pb2_grpc\.py$",
|
||||
r"^package-lock\.json$", # Package locks
|
||||
r"^yarn\.lock$",
|
||||
r"^poetry\.lock$",
|
||||
r"^Pipfile\.lock$",
|
||||
r"\.(svg|png|jpe?g|gif|ico|webp)$", # Images
|
||||
r"\.(woff2?|ttf|eot|otf)$", # Fonts
|
||||
r"\.(mp4|webm|mov|avi|mkv)$", # Videos
|
||||
r"\.(pdf|doc|docx|xls|xlsx)$", # Documents
|
||||
r"\.generated\.", # Common generated file pattern
|
||||
]
|
||||
SKIP_PATTERNS = tuple(re.compile(pattern) for pattern in SKIP_PATTERN_STRINGS)
|
||||
MAX_CONTEXT_FILE_CHARS = 5000
|
||||
MAX_REVIEW_COMMENTS = 8
|
||||
SEVERITY_RANK = {"CRITICAL": 0, "HIGH": 1, "MEDIUM": 2, "LOW": 3, "SUGGESTION": 4, None: 5}
|
||||
|
||||
|
||||
def should_skip_file(path: str) -> bool:
|
||||
"""Return True if file path matches a generated/minified skip pattern."""
|
||||
normalized = Path(path).as_posix().lstrip("./")
|
||||
filename = normalized.rsplit("/", 1)[-1]
|
||||
return any(pattern.search(candidate) for pattern in SKIP_PATTERNS for candidate in (normalized, filename))
|
||||
|
||||
|
||||
def parse_diff_files(diff_text: str) -> tuple[dict, str]:
|
||||
"""Parse diff and return file mapping with line numbers AND augmented diff with explicit line numbers.
|
||||
|
||||
@@ -416,13 +387,7 @@ def post_review_summary(event: Action, review_data: dict, review_number: int) ->
|
||||
if review_data.get("diff_truncated"):
|
||||
body += "\n⚠️ **Large PR**: Review focused on critical issues. Some details may not be covered.\n"
|
||||
|
||||
if skipped := review_data.get("skipped_files"):
|
||||
count = len(skipped)
|
||||
skip_str = f"📋 Skipped {count} file{'s' if count != 1 else ''} (lock files, minified, images, etc.)"
|
||||
file_list = "\n".join(f"- `{f}`" for f in sorted(skipped[:100]))
|
||||
if count > 100:
|
||||
file_list += f"\n{count - 100} more..."
|
||||
body += f"\n<details><summary>{skip_str}</summary>\n\n{file_list}\n</details>\n"
|
||||
body += format_skipped_files_dropdown(review_data.get("skipped_files", []))
|
||||
|
||||
# Build inline comments for the review
|
||||
review_comments = []
|
||||
|
||||
+13
-3
@@ -2,7 +2,14 @@
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from .utils import ACTIONS_CREDIT, GITHUB_API_URL, Action, get_pr_summary_prompt, get_response
|
||||
from .utils import (
|
||||
ACTIONS_CREDIT,
|
||||
GITHUB_API_URL,
|
||||
Action,
|
||||
format_skipped_files_dropdown,
|
||||
get_pr_summary_prompt,
|
||||
get_response,
|
||||
)
|
||||
|
||||
SUMMARY_MARKER = "## 🛠️ PR Summary"
|
||||
|
||||
@@ -59,7 +66,7 @@ def generate_issue_comment(pr_url, pr_summary, pr_credit, pr_title=""):
|
||||
|
||||
def generate_pr_summary(repository, diff_text):
|
||||
"""Generates a concise, professional summary of a PR using OpenAI's API."""
|
||||
prompt, is_large = get_pr_summary_prompt(repository, diff_text)
|
||||
prompt, is_large, skipped_files = get_pr_summary_prompt(repository, diff_text)
|
||||
|
||||
messages = [
|
||||
{
|
||||
@@ -72,7 +79,10 @@ def generate_pr_summary(repository, diff_text):
|
||||
if is_large:
|
||||
reply = "**WARNING ⚠️** this PR is very large, summary may not cover all changes.\n\n" + reply
|
||||
|
||||
return f"{SUMMARY_MARKER}\n\n{ACTIONS_CREDIT}\n\n{reply}"
|
||||
# Add skipped files dropdown if any files were filtered
|
||||
skipped_dropdown = format_skipped_files_dropdown(skipped_files)
|
||||
|
||||
return f"{SUMMARY_MARKER}\n\n{ACTIONS_CREDIT}\n\n{reply}{skipped_dropdown}"
|
||||
|
||||
|
||||
def label_fixed_issues(event, pr_summary):
|
||||
|
||||
@@ -5,9 +5,14 @@ from .common_utils import (
|
||||
REDIRECT_END_IGNORE_LIST,
|
||||
REDIRECT_START_IGNORE_LIST,
|
||||
REQUESTS_HEADERS,
|
||||
SKIP_PATTERNS,
|
||||
URL_IGNORE_LIST,
|
||||
allow_redirect,
|
||||
filter_diff_text,
|
||||
format_skipped_files_dropdown,
|
||||
format_skipped_files_note,
|
||||
remove_html_comments,
|
||||
should_skip_file,
|
||||
)
|
||||
from .github_utils import GITHUB_API_URL, GITHUB_GRAPHQL_URL, Action, ultralytics_actions_info
|
||||
from .openai_utils import (
|
||||
@@ -29,17 +34,22 @@ __all__ = (
|
||||
"REDIRECT_END_IGNORE_LIST",
|
||||
"REDIRECT_START_IGNORE_LIST",
|
||||
"REQUESTS_HEADERS",
|
||||
"SKIP_PATTERNS",
|
||||
"URL_IGNORE_LIST",
|
||||
"Action",
|
||||
"allow_redirect",
|
||||
"check_pubdev_version",
|
||||
"check_pypi_version",
|
||||
"filter_diff_text",
|
||||
"filter_labels",
|
||||
"format_skipped_files_dropdown",
|
||||
"format_skipped_files_note",
|
||||
"get_pr_open_response",
|
||||
"get_pr_summary_guidelines",
|
||||
"get_pr_summary_prompt",
|
||||
"get_response",
|
||||
"remove_html_comments",
|
||||
"sanitize_ai_text",
|
||||
"should_skip_file",
|
||||
"ultralytics_actions_info",
|
||||
)
|
||||
|
||||
@@ -6,10 +6,38 @@ import os
|
||||
import re
|
||||
import time
|
||||
from concurrent.futures import ThreadPoolExecutor
|
||||
from pathlib import Path
|
||||
from urllib import parse
|
||||
|
||||
import requests
|
||||
|
||||
# Patterns for files that should be skipped in PR summaries and reviews (lock files, generated, minified, etc.)
|
||||
SKIP_PATTERN_STRINGS = [
|
||||
r"\.lock$", # Lock files
|
||||
r"-lock\.(json|yaml|yml)$",
|
||||
r"\.min\.(js|css)$", # Minified
|
||||
r"\.bundle\.(js|css)$",
|
||||
r"(^|/)dist/", # Generated/vendored directories
|
||||
r"(^|/)build/",
|
||||
r"(^|/)vendor/",
|
||||
r"(^|/)node_modules/",
|
||||
r"(^|/)coverage/", # Coverage reports
|
||||
r"\.pb\.py$", # Proto generated
|
||||
r"_pb2\.py$",
|
||||
r"_pb2_grpc\.py$",
|
||||
r"^package-lock\.json$", # Package locks
|
||||
r"^yarn\.lock$",
|
||||
r"^poetry\.lock$",
|
||||
r"^Pipfile\.lock$",
|
||||
r"^uv\.lock$",
|
||||
r"\.(svg|png|jpe?g|gif|ico|webp)$", # Images
|
||||
r"\.(woff2?|ttf|eot|otf)$", # Fonts
|
||||
r"\.(mp4|webm|mov|avi|mkv)$", # Videos
|
||||
r"\.(pdf|doc|docx|xls|xlsx)$", # Documents
|
||||
r"\.generated\.", # Common generated file pattern
|
||||
]
|
||||
SKIP_PATTERNS = tuple(re.compile(pattern) for pattern in SKIP_PATTERN_STRINGS)
|
||||
|
||||
REQUESTS_HEADERS = {
|
||||
"User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/132.0.0.0 Safari/537.36",
|
||||
"Accept": "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7",
|
||||
@@ -137,6 +165,72 @@ def remove_html_comments(body: str) -> str:
|
||||
return re.sub(r"<!--.*?-->", "", body, flags=re.DOTALL).strip() if body else ""
|
||||
|
||||
|
||||
def should_skip_file(path: str) -> bool:
|
||||
"""Return True if file path matches a generated/minified skip pattern (lock files, images, etc.)."""
|
||||
normalized = Path(path).as_posix().removeprefix("./")
|
||||
filename = normalized.rsplit("/", 1)[-1]
|
||||
return any(pattern.search(candidate) for pattern in SKIP_PATTERNS for candidate in (normalized, filename))
|
||||
|
||||
|
||||
def filter_diff_text(diff_text: str) -> tuple[str, list[str]]:
|
||||
"""Filter diff text to exclude lock files and other generated files.
|
||||
|
||||
Returns:
|
||||
tuple: (filtered_diff_text, list of skipped file paths)
|
||||
"""
|
||||
if not diff_text or diff_text.startswith("ERROR"):
|
||||
return diff_text, []
|
||||
|
||||
filtered_lines = []
|
||||
skipped_files = set()
|
||||
current_file = None
|
||||
skip_current = False
|
||||
|
||||
for line in diff_text.split("\n"):
|
||||
if line.startswith("diff --git"):
|
||||
# Extract file path from diff header, handling quoted paths (spaces/renames)
|
||||
# Formats: "diff --git a/path b/path" or 'diff --git "a/path" "b/path"'
|
||||
if match := re.search(r' "?b/(.+?)"?$', line):
|
||||
current_file = match.group(1).rstrip('"')
|
||||
else:
|
||||
current_file = None
|
||||
skip_current = current_file and should_skip_file(current_file)
|
||||
|
||||
if skip_current and current_file:
|
||||
skipped_files.add(current_file)
|
||||
else:
|
||||
filtered_lines.append(line)
|
||||
elif skip_current:
|
||||
continue
|
||||
else:
|
||||
filtered_lines.append(line)
|
||||
|
||||
return "\n".join(filtered_lines), sorted(skipped_files)
|
||||
|
||||
|
||||
def format_skipped_files_dropdown(skipped_files: list[str], max_files: int = 100) -> str:
|
||||
"""Format skipped files as a collapsible HTML details dropdown for GitHub markdown."""
|
||||
if not skipped_files:
|
||||
return ""
|
||||
count = len(skipped_files)
|
||||
summary = f"📋 Skipped {count} file{'s' if count != 1 else ''} (lock files, generated, images, etc.)"
|
||||
file_list = "\n".join(f"- `{f}`" for f in sorted(skipped_files)[:max_files])
|
||||
if count > max_files:
|
||||
file_list += f"\n- ... and {count - max_files} more"
|
||||
return f"\n<details><summary>{summary}</summary>\n\n{file_list}\n</details>\n"
|
||||
|
||||
|
||||
def format_skipped_files_note(skipped_files: list[str], max_files: int = 10) -> str:
|
||||
"""Format skipped files as a brief inline note for AI prompts."""
|
||||
if not skipped_files:
|
||||
return ""
|
||||
note = "\n\nNote: The following auto-generated/lock files were also modified but diff details omitted: "
|
||||
note += ", ".join(f"`{f}`" for f in skipped_files[:max_files])
|
||||
if len(skipped_files) > max_files:
|
||||
note += f" and {len(skipped_files) - max_files} more"
|
||||
return note
|
||||
|
||||
|
||||
def clean_url(url):
|
||||
"""Remove extra characters from URL strings."""
|
||||
url = str(url).strip('"').strip("'").rstrip(".,:;!?`\\").replace(".git@main", "").replace("git+", "")
|
||||
|
||||
@@ -9,7 +9,7 @@ import time
|
||||
|
||||
import requests
|
||||
|
||||
from actions.utils.common_utils import check_links_in_string
|
||||
from actions.utils.common_utils import check_links_in_string, filter_diff_text, format_skipped_files_note
|
||||
|
||||
OPENAI_API_KEY = os.getenv("OPENAI_API_KEY")
|
||||
OPENAI_MODEL = os.getenv("OPENAI_MODEL", "gpt-5.2-2025-12-11")
|
||||
@@ -99,10 +99,12 @@ def get_pr_summary_guidelines() -> str:
|
||||
- (bullet points explaining benefits and potential impact to users)"""
|
||||
|
||||
|
||||
def get_pr_summary_prompt(repository: str, diff_text: str) -> tuple[str, bool]:
|
||||
"""Returns the complete PR summary generation prompt with diff (used by PR update/merge)."""
|
||||
prompt = f"{get_pr_summary_guidelines()}\n\nRepository: '{repository}'\n\nHere's the PR diff:\n\n{diff_text[:MAX_PROMPT_CHARS]}"
|
||||
return prompt, len(diff_text) > MAX_PROMPT_CHARS
|
||||
def get_pr_summary_prompt(repository: str, diff_text: str) -> tuple[str, bool, list[str]]:
|
||||
"""Returns the complete PR summary generation prompt with filtered diff (used by PR update/merge)."""
|
||||
filtered_diff, skipped_files = filter_diff_text(diff_text)
|
||||
prompt = f"{get_pr_summary_guidelines()}\n\nRepository: '{repository}'\n\nHere's the PR diff:\n\n{filtered_diff[:MAX_PROMPT_CHARS]}"
|
||||
prompt += format_skipped_files_note(skipped_files)
|
||||
return prompt, len(filtered_diff) > MAX_PROMPT_CHARS, skipped_files
|
||||
|
||||
|
||||
def get_pr_first_comment_template(repository: str, username: str) -> str:
|
||||
@@ -219,7 +221,8 @@ def get_response(
|
||||
|
||||
def get_pr_open_response(repository: str, diff_text: str, title: str, username: str, available_labels: dict) -> dict:
|
||||
"""Generates unified PR response with summary, labels, and first comment in a single API call."""
|
||||
is_large = len(diff_text) > MAX_PROMPT_CHARS
|
||||
filtered_diff, skipped_files = filter_diff_text(diff_text)
|
||||
is_large = len(filtered_diff) > MAX_PROMPT_CHARS
|
||||
|
||||
filtered_labels = filter_labels(available_labels, is_pr=True)
|
||||
labels_str = "\n".join(f"- {name}: {description}" for name, description in filtered_labels.items())
|
||||
@@ -227,7 +230,7 @@ def get_pr_open_response(repository: str, diff_text: str, title: str, username:
|
||||
prompt = f"""You are processing a new GitHub PR by @{username} for the {repository} repository.
|
||||
|
||||
Generate 3 outputs in a single JSON response for the PR titled '{title}' with the following diff:
|
||||
{diff_text[:MAX_PROMPT_CHARS]}
|
||||
{filtered_diff[:MAX_PROMPT_CHARS]}{format_skipped_files_note(skipped_files)}
|
||||
|
||||
|
||||
--- FIRST JSON OUTPUT (PR SUMMARY) ---
|
||||
@@ -274,6 +277,7 @@ Example comment template (adapt as needed, keep all links):
|
||||
result["summary"] = (
|
||||
"**WARNING ⚠️** this PR is very large, summary may not cover all changes.\n\n" + result["summary"]
|
||||
)
|
||||
result["skipped_files"] = skipped_files
|
||||
return result
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user