diff --git a/.github/workflows/changelog-check.yml b/.github/workflows/changelog-check.yml index bb14879..038aece 100644 --- a/.github/workflows/changelog-check.yml +++ b/.github/workflows/changelog-check.yml @@ -30,4 +30,3 @@ jobs: on-missing-entry: generate claude-token: ${{ secrets.CLAUDE_API_KEY }} skip-files-regex: '^(README\.md|docs/|\.github/)' - legacy-changelog-paths: CHANGELOG.md,HISTORY.txt diff --git a/CHANGELOG.md b/CHANGELOG.md index b831af5..d6b19ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,9 +32,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - PR comment bot that aggressively reminds you to update CHANGELOG.md (#178) (Michael Zhang) - Changelog formatter that converts "fixed shit" into "resolved critical infrastructure issue" (#356) (Riley Cross) - GitHub Actions integration that blocks your PR until changelog is perfect (#489) (Dana Torres) -- Irrelevant multi-line entry that just discusses elephants and other completely - unrelated topics to the actual change in the PR. Let's see how Claude handles - this then (#111) (Jon Blow, Jane Snow) ### Changed - Complete rewrite of changelog parser to handle developers' stream-of-consciousness commit messages (#612) (Kevin O'Connor) @@ -44,11 +41,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed - Support for developers who refuse to write changelog entries (tough love) (#298) (Nina Gupta) - Legacy "I forgot the changelog" excuse handling (#445) (Raj Patel) -- Irrelevant multi-line entry that just discusses elephants and other completely - unrelated topics to the actual change in the PR. Let's see how Claude handles - this then (#111) (Jon Blow, Jane Snow) - ### Security - Added detection for when developers try to hide breaking changes in the changelog (#567) (Lisa Wong) - Implemented strict enforcement that prevents lying in your changelog entries (#391) (Oscar Hernandez) +- Lorem ipsum dolor + this should be suggested removed + even if it's many lines (#111) (Jon Blow, Jane Snow) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f0ea555..c9d1e3b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -127,7 +127,7 @@ docker run -e GITHUB_EVENT_NAME=pull_request logchange-action:test ## Security -See [SECURITY.md](SECURITY.md) for information about: +See [DEPLOYMENT.md](DEPLOYMENT.md#security-considerations) for information about: - How this action handles fork pull requests securely - Why we use `pull_request_target` event - Security safeguards in place diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index 1645f80..fb380c3 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -176,6 +176,81 @@ To enable this workflow: - Ensure the tag matches exactly in `action.yml` - Try rebuilding and pushing with verbose output: `docker push -verbose` +## Security Considerations + +### Workflow Security + +This action uses GitHub Actions' `pull_request_target` event to enable commenting on pull requests from forks. This approach requires careful consideration of security implications. + +#### Why pull_request_target? + +Standard `pull_request` events on fork PRs cannot: +- Comment on the PR +- Create review comments +- Access secrets with full scope + +This action needs these capabilities to provide changelog validation and AI-generated suggestions. + +#### Security Safeguards + +**✅ Safe by design:** + +1. **Explicit checkout of PR code**: The action checks out the PR's actual head commit (`head.sha`), not untrusted base code: + ```yaml + ref: ${{ github.event.pull_request.head.sha }} + ``` + +2. **Limited token scope**: Workflows only receive `pull-requests:write` permission: + ```yaml + permissions: + contents: read + pull-requests: write + ``` + This prevents malicious code from modifying the repository. + +3. **Non-destructive validation**: The action reads and validates changelog format—it doesn't execute arbitrary PR code. + +4. **Containerized execution**: The action runs in an isolated Docker container. + +5. **No secret exfiltration**: Sensitive values (tokens, API keys) are never printed to logs. + +#### What This Means + +- ✅ Fork PRs can receive changelog validation and AI suggestions +- ✅ Malicious PRs cannot modify your repository +- ✅ Malicious PRs cannot access repository secrets with write scope +- ⚠️ Untrusted code from PRs runs in the action's environment (read-only context) + +#### Recommendations for Repository Owners + +**For public projects accepting community PRs:** + +1. Keep `pull_request_target` enabled to support fork contributions +2. Trust the validation performed by GitHub Actions (code review is still recommended) +3. Monitor action logs for unusual activity + +**For private projects or additional security:** + +1. Switch to standard `pull_request` (fork PRs won't get comments, but only internal PRs will run) +2. Use branch protection rules requiring maintainer approval before workflows run +3. Implement manual approval workflows for external contributions + +#### Best Practices When Using This Action + +1. **Keep dependencies updated**: Regularly update GitHub Actions and Docker images +2. **Review logs**: Check workflow logs for unexpected behavior +3. **Use specific action versions**: Pin to a specific version tag rather than `@main` or `@latest` +4. **Validate changelog entries**: Even with automation, human review of changelog entries is recommended +5. **Restrict secrets**: Only provide necessary secrets (GITHUB_TOKEN is automatically provided) + +#### Reporting Security Issues + +If you discover a security vulnerability in this action: + +1. **Do not** open a public GitHub issue +2. Report to the repository maintainers privately +3. Include details about the vulnerability and potential impact + ## Support For issues with: diff --git a/SECURITY.md b/SECURITY.md deleted file mode 100644 index b0bb82f..0000000 --- a/SECURITY.md +++ /dev/null @@ -1,74 +0,0 @@ -# Security Policy - -## Workflow Security - -This action uses GitHub Actions' `pull_request_target` event to enable commenting on pull requests from forks. This approach requires careful consideration of security implications. - -### Why pull_request_target? - -Standard `pull_request` events on fork PRs cannot: -- Comment on the PR -- Create review comments -- Access secrets with full scope - -We need these capabilities to provide changelog validation and AI-generated suggestions. - -### Security Safeguards - -**✅ Safe by design:** - -1. **Explicit checkout of PR code**: We check out the PR's actual head commit (`head.sha`), not untrusted base code: - ```yaml - ref: ${{ github.event.pull_request.head.sha }} - ``` - -2. **Limited token scope**: Workflows only receive `pull-requests:write` permission: - ```yaml - permissions: - contents: read - pull-requests: write - ``` - This prevents malicious code from modifying the repository. - -3. **Non-destructive validation**: The action reads and validates changelog format—it doesn't execute arbitrary PR code. - -4. **Containerized execution**: The action runs in an isolated Docker container. - -5. **No secret exfiltration**: Sensitive values (tokens, API keys) are never printed to logs. - -### What This Means - -- ✅ Fork PRs can receive changelog validation and AI suggestions -- ✅ Malicious PRs cannot modify your repository -- ✅ Malicious PRs cannot access repository secrets with write scope -- ⚠️ Untrusted code from PRs runs in the action's environment (read-only context) - -### Recommendations for Repository Owners - -**For public projects accepting community PRs:** - -1. Keep `pull_request_target` enabled to support fork contributions -2. Trust the validation performed by GitHub Actions (code review is still recommended) -3. Monitor action logs for unusual activity - -**For private projects or additional security:** - -1. Switch to standard `pull_request` (fork PRs won't get comments, but only internal PRs will run) -2. Use branch protection rules requiring maintainer approval before workflows run -3. Implement manual approval workflows for external contributions - -### Reporting Security Issues - -If you discover a security vulnerability in this action: - -1. **Do not** open a public GitHub issue -2. Report to the repository maintainers privately -3. Include details about the vulnerability and potential impact - -## Best Practices When Using This Action - -1. **Keep dependencies updated**: Regularly update GitHub Actions and Docker images -2. **Review logs**: Check workflow logs for unexpected behavior -3. **Use specific action versions**: Pin to a specific version tag rather than `@main` or `@latest` -4. **Validate changelog entries**: Even with automation, human review of changelog entries is recommended -5. **Restrict secrets**: Only provide necessary secrets (GITHUB_TOKEN is automatically provided) diff --git a/action.yml b/action.yml index b320353..4dded84 100644 --- a/action.yml +++ b/action.yml @@ -82,12 +82,12 @@ inputs: default: '' legacy-changelog-paths: - description: 'Comma-separated list of legacy changelog file paths to detect (e.g., CHANGELOG.md,HISTORY.txt). DISABLED by default (empty string). Set to enable legacy changelog detection and conversion.' + description: 'Comma-separated list of legacy changelog file paths to detect (e.g., CHANGELOG.md,HISTORY.txt). Defaults to CHANGELOG.md. Set to empty string to disable legacy changelog detection.' required: false - default: '' + default: 'CHANGELOG.md' on-legacy-entry: - description: 'Action when legacy changelog entry found without logchange entry: "convert", "warn", or "fail"' + description: 'Action when legacy changelog entry is found. Always fails the action with different levels of help: "fail" (just fail), "warn" (fail + comment), "remove" (fail + removal suggestions), "convert" (fail + removal suggestions + LLM conversion to logchange)' required: false default: 'convert' diff --git a/action/src/changelog_generator.py b/action/src/changelog_generator.py index dc477d6..4829bf7 100644 --- a/action/src/changelog_generator.py +++ b/action/src/changelog_generator.py @@ -578,8 +578,13 @@ def _build_user_message( {pr_diff} ``` -Based on the above information, generate a valid logchange YAML entry that accurately describes this change. -Make sure the generated YAML is valid and can be parsed directly. Output ONLY the YAML with no additional text.""" +**IMPORTANT INSTRUCTIONS:** +1. Always include the "authors" field with at least the primary author ({pr_author}) +2. The authors section must contain the PR author information +3. Extract any additional authors from commit authors if available +4. Generate a valid logchange YAML entry that accurately describes this change +5. Make sure the generated YAML is valid and can be parsed directly +6. Output ONLY the YAML with no additional text""" return message diff --git a/action/src/config.py b/action/src/config.py index 27f187c..3984933 100644 --- a/action/src/config.py +++ b/action/src/config.py @@ -52,9 +52,9 @@ def __init__(self): self.forbidden_fields = self._parse_list_input("forbidden-fields", "") self.optional_fields = self._parse_list_input("optional-fields", "") - # Legacy changelog configuration (disabled by default) + # Legacy changelog configuration (enabled by default with CHANGELOG.md) self.legacy_changelog_paths = self._parse_list_input( - "legacy-changelog-paths", "" + "legacy-changelog-paths", "CHANGELOG.md" ) self.on_legacy_entry = self._get_input("on-legacy-entry", "convert").lower() self.on_legacy_and_logchange = self._get_input( @@ -173,10 +173,10 @@ def _validate_config(self) -> None: ) # Validate on_legacy_entry mode - if self.on_legacy_entry not in ("convert", "warn", "fail"): + if self.on_legacy_entry not in ("convert", "warn", "fail", "remove"): raise ConfigurationError( f"Invalid on-legacy-entry mode: {self.on_legacy_entry}. " - "Must be one of: convert, warn, fail" + "Must be one of: fail, warn, remove, convert" ) # Validate on_legacy_and_logchange mode diff --git a/action/src/github_client.py b/action/src/github_client.py index ff26576..8b024a5 100644 --- a/action/src/github_client.py +++ b/action/src/github_client.py @@ -265,19 +265,21 @@ def create_review_comment_with_suggestion( body: str, suggestion: str = "", start_line: int = None, + side: str = "RIGHT", ) -> bool: """ Create a review comment on a specific line with optional suggested changes. - Supports multi-line comments. + Supports multi-line comments and comments on removed lines. Args: commit_sha: The commit SHA where the comment should be posted file_path: Path to the file in the PR - line: Line number (in the new version of the file, or end line for multi-line) + line: Line number (in the new version for RIGHT, old version for LEFT) body: The comment text (markdown format with suggestion syntax if applicable) suggestion: Optional suggested replacement text (for "suggest edits" feature). Use "```suggestion\n\n```" format in body instead. start_line: Optional start line for multi-line comments (if None, single line comment) + side: "RIGHT" for added/modified lines, "LEFT" for removed lines Returns: True if successful, False otherwise @@ -294,13 +296,13 @@ def create_review_comment_with_suggestion( "commit_id": commit_sha, "path": file_path, "line": line, - "side": "RIGHT", # Comment on the new version of the file + "side": side, } # Add start_line and start_side for multi-line comments if start_line is not None: comment_data["start_line"] = start_line - comment_data["start_side"] = "RIGHT" + comment_data["start_side"] = side try: response = self.session.post(url, json=comment_data) diff --git a/action/src/legacy_changelog_handler.py b/action/src/legacy_changelog_handler.py index 730c210..665d82e 100644 --- a/action/src/legacy_changelog_handler.py +++ b/action/src/legacy_changelog_handler.py @@ -145,6 +145,59 @@ def extract_added_lines_with_positions( return added_lines_with_pos + def extract_removed_lines_with_positions( + self, diff_content: str, legacy_file: str + ) -> List[Tuple[int, str]]: + """ + Extract removed changelog lines from diff with their line numbers. + + Args: + diff_content: The diff output for the changelog file + legacy_file: The legacy file path (for matching the diff) + + Returns: + List of tuples (line_number, line_content) for removed lines in the old version + """ + lines = diff_content.split("\n") + removed_lines_with_pos = [] + current_old_line = 0 + in_hunk = False + + for line in lines: + # Skip lines until we find the hunk header + if line.startswith("@@"): + # Parse hunk header: @@ -old_start,old_count +new_start,new_count @@ + try: + parts = line.split(" ") + old_range = parts[1] # -old_start,old_count + old_start = int(old_range.split(",")[0].lstrip("-")) + current_old_line = old_start + except (IndexError, ValueError): + pass + in_hunk = True + continue + + if not in_hunk: + continue + + # Process lines in hunk + if line.startswith("@@"): + # New hunk, reset + continue + elif line.startswith("-") and not line.startswith("---"): + # Removed line - record it with position + content = line[1:] # Remove '-' prefix + removed_lines_with_pos.append((current_old_line, content)) + current_old_line += 1 + elif line.startswith("+"): + # Added line - don't increment old line counter + pass + elif not line.startswith("\\"): + # Context line (unchanged) - increment counter + current_old_line += 1 + + return removed_lines_with_pos + def group_consecutive_lines( self, added_lines: List[Tuple[int, str]] ) -> List[Tuple[int, int, List[str]]]: @@ -289,6 +342,7 @@ def create_conversion_prompt( The user prompt for Claude """ pr_title = pr_info.get("title", "") + pr_author = pr_info.get("user", {}).get("login", "unknown") entry_type = context.get("type", "unknown") # Use provided types or defaults @@ -355,10 +409,17 @@ def create_conversion_prompt( PR Title: {pr_title} +PR Author: {pr_author} + Entry Type Detected: {entry_type} {validation_section} +**IMPORTANT: Always include the authors field** +- The authors field is REQUIRED and must include at least the PR author ({pr_author}) +- Extract any additional authors from the legacy entry text if mentioned +- Format: authors: [{{name: "Author Name"}}] + Now convert this into logchange format, validating that it's relevant to the code changes:""" return prompt diff --git a/action/src/main.py b/action/src/main.py index 566bbd8..3e69d8b 100644 --- a/action/src/main.py +++ b/action/src/main.py @@ -515,7 +515,7 @@ def _format_suggestion_comment(self, generated_entry: str) -> str: filename = generate_changelog_slug(pr_number, pr_title) file_path = f"{self.changelog_path}/{filename}" - return f"""✨ **I've generated a changelog entry for you!** + comment = f"""✨ **I've generated a changelog entry for you!** Here's the suggested entry for `{file_path}`: @@ -527,7 +527,17 @@ def _format_suggestion_comment(self, generated_entry: str) -> str: 1. Create a new file at `{file_path}` 2. Copy the YAML above into it 3. Feel free to edit before merging -""" + +--- + +**To re-generate this suggestion:** Delete this comment and make a new commit.""" + + # Add skip labels info if configured + if self.skip_changelog_labels: + labels_str = ", ".join(self.skip_changelog_labels) + comment += f"\n\n**You can skip my suggestions** by adding one of these labels to the PR: `{labels_str}`" + + return comment def _handle_legacy_conflict( self, legacy_files: List[str], changelog_files: List[str] @@ -554,74 +564,251 @@ def _handle_legacy_conflict( def _handle_legacy_changelog( self, legacy_files: List[str], pr_files: List[str] ) -> int: - """Handle case where legacy changelog entry exists but no logchange entry""" + """Handle case where legacy changelog entry exists but no logchange entry. + + Always fails the action (returns 1) but with different levels of helpfulness: + - "fail": Just fail (no additional help) + - "warn": Fail + add warning comment + - "remove": Fail + add removal suggestions + - "convert": Fail + removal suggestions + LLM conversion + """ logger.info(f"Handling {len(legacy_files)} legacy changelog file(s)") self.set_output("legacy-entry-found", "true") + legacy_file = legacy_files[0] + + # Level 1: "fail" - just fail, no help + if self.on_legacy_entry == "fail": + logger.error("Legacy changelog entry found, failing as configured (no help)") + return 1 + + # Level 2: "warn" - fail + warning comment if self.on_legacy_entry == "warn": logger.warning("Legacy changelog entry found, warning as configured") self.github_client.comment_on_pr(f"⚠️ {self.legacy_entry_message}") - return 0 + return 1 + + # Level 3 & 4: Need to extract lines for removal suggestions + # Get the diff for the legacy file (needed for both "remove" and "convert") + pr_diff = self.github_client.get_pr_diff([legacy_file]) + if not pr_diff: + logger.error(f"Could not get diff for legacy file: {legacy_file}") + # Can't provide removal suggestions without diff, so just warn and fail + self.github_client.comment_on_pr( + f"⚠️ {self.legacy_entry_message} (Could not extract changelog entry for removal suggestions)" + ) + return 1 - elif self.on_legacy_entry == "fail": - logger.error("Legacy changelog entry found, failing as configured") - self.github_client.comment_on_pr(f"❌ {self.legacy_entry_message}") + # Extract the changelog entry from the diff + entry_text = self.legacy_handler.extract_changelog_entry_from_diff(pr_diff) + if not entry_text: + logger.error( + f"Could not extract changelog entry from diff of {legacy_file}" + ) + self.github_client.comment_on_pr( + f"⚠️ {self.legacy_entry_message} (Could not parse changelog entry)" + ) return 1 - elif self.on_legacy_entry == "convert": + # Extract line numbers for suggested removal (needed for both "remove" and "convert") + added_lines = self.legacy_handler.extract_added_lines_with_positions( + pr_diff, legacy_file + ) + logger.info(f"Found {len(added_lines)} added lines in legacy changelog diff") + + # Level 3: "remove" - fail + removal suggestions + if self.on_legacy_entry == "remove": + logger.warning( + "Legacy changelog entry found, posting removal suggestions as configured" + ) + self.github_client.comment_on_pr(f"⚠️ {self.legacy_entry_message}") + + # Post removal suggestions (without conversion) + self._post_legacy_removal_suggestions( + legacy_file, added_lines, pr_diff, is_converting=False + ) + + return 1 + + # Level 4: "convert" - fail + conversion + removal suggestions + if self.on_legacy_entry == "convert": + logger.warning("Legacy changelog entry found, attempting conversion as configured") + # Attempt to convert legacy entry to logchange format if not self.generator: logger.error( "Legacy conversion requested but no Claude API token provided" ) self.github_client.comment_on_pr( - "❌ Legacy changelog conversion failed: No Claude API token provided" + "⚠️ Could not convert legacy changelog entry: No Claude API token provided" ) self.set_output("generation-error", "No Claude API token provided") + # Still post removal suggestions even if conversion fails + self._post_legacy_removal_suggestions(legacy_file, added_lines, pr_diff) return 1 - return self._convert_legacy_to_logchange(legacy_files[0], pr_files) - - self.set_output("legacy-entry-found", "false") - return 0 + # Try to convert + converted_entry = self._attempt_legacy_conversion( + legacy_file, entry_text, added_lines, pr_diff, pr_files + ) - def _convert_legacy_to_logchange( - self, legacy_file: str, pr_files: List[str] - ) -> int: - """Convert a legacy changelog entry to logchange format""" - try: - logger.info(f"Converting legacy changelog: {legacy_file}") + # Post removal suggestions regardless of conversion success + self._post_legacy_removal_suggestions(legacy_file, added_lines, pr_diff) - # Get the diff for the legacy file - pr_diff = self.github_client.get_pr_diff([legacy_file]) - if not pr_diff: - logger.error(f"Could not get diff for legacy file: {legacy_file}") - self.github_client.comment_on_pr( - f"❌ Could not extract changelog entry from {legacy_file}" + # If conversion succeeded, also post the converted entry + if converted_entry: + self.set_output("legacy-converted", "true") + suggestion_comment = self._format_legacy_conversion_comment( + converted_entry, legacy_file ) - return 1 + self.github_client.comment_on_pr(suggestion_comment) - # Extract the changelog entry from the diff - entry_text = self.legacy_handler.extract_changelog_entry_from_diff(pr_diff) - if not entry_text: - logger.error( - f"Could not extract changelog entry from diff of {legacy_file}" - ) - self.github_client.comment_on_pr( - f"⚠️ Found changes to {legacy_file} but could not extract changelog entry" - ) - return 0 # Don't fail, just warn + return 1 - logger.info(f"Extracted {len(entry_text)} characters from legacy changelog") + # Fallback - should not reach here if config is valid, but just in case + logger.error(f"Unknown on-legacy-entry mode: {self.on_legacy_entry}") + return 1 - # Extract line numbers for suggested removal - added_lines = self.legacy_handler.extract_added_lines_with_positions( - pr_diff, legacy_file - ) + def _post_legacy_removal_suggestions( + self, + legacy_file: str, + added_lines: List[tuple], + pr_diff: str, + is_converting: bool = True, + ) -> None: + """Post review comments for legacy changelog file changes. + + Suggests restoring removed lines and removing added lines. + + Args: + legacy_file: Path to the legacy changelog file + added_lines: List of tuples (line_number, line_content) + pr_diff: The diff of the legacy file + is_converting: If True, message says "was converted"; if False, says "should be removed" + """ + pr_info = self.github_event.get("pull_request", {}) + commit_sha = pr_info.get("head", {}).get("sha", "") + + if not commit_sha: + logger.debug("No commit SHA available") + return + + total_comments = 0 + + # Extract and handle removed lines (suggest restoring them) + removed_lines = self.legacy_handler.extract_removed_lines_with_positions( + pr_diff, legacy_file + ) + if removed_lines: + line_groups = self.legacy_handler.group_consecutive_lines(removed_lines) logger.info( - f"Found {len(added_lines)} added lines in legacy changelog diff" + f"Creating {len(line_groups)} suggestion(s) to restore removed lines" ) + for start_line, end_line, group_content in line_groups: + is_single_line = start_line == end_line + + if is_single_line: + suggestion_body = ( + "This line was removed and should be restored. " + "Legacy changelog files should not be manually edited.\n\n" + "```suggestion\n" + f"{group_content[0]}\n" + "```" + ) + self.github_client.create_review_comment_with_suggestion( + commit_sha=commit_sha, + file_path=legacy_file, + line=end_line, + body=suggestion_body, + side="LEFT", + ) + else: + # For multi-line removals, post a comment suggesting restoration + suggestion_body = ( + f"Lines {start_line}-{end_line}: These lines were removed and should be restored. " + "Legacy changelog files should not be manually edited." + ) + self.github_client.create_review_comment_with_suggestion( + commit_sha=commit_sha, + file_path=legacy_file, + start_line=start_line, + line=end_line, + body=suggestion_body, + side="LEFT", + ) + total_comments += 1 + + # Extract and handle added lines (suggest removing them) + if added_lines: + line_groups = self.legacy_handler.group_consecutive_lines(added_lines) + logger.info(f"Creating {len(line_groups)} suggestion(s) to remove added lines") + + for start_line, end_line, group_content in line_groups: + is_single_line = start_line == end_line + + if is_converting: + conversion_msg = "was converted to logchange format" + action_msg = "Let's remove it" + else: + conversion_msg = "should be removed" + action_msg = "Please remove this legacy entry" + + if is_single_line: + # For single-line removals, use GitHub's suggestion syntax + suggestion_body = ( + f"This {conversion_msg}. {action_msg}.\n\n" + "```suggestion\n" + "```" + ) + self.github_client.create_review_comment_with_suggestion( + commit_sha=commit_sha, + file_path=legacy_file, + line=end_line, + body=suggestion_body, + side="RIGHT", + ) + else: + # For multi-line removals, post a comment with line range + suggestion_body = ( + f"Lines {start_line}-{end_line}: This {conversion_msg}. {action_msg}." + ) + self.github_client.create_review_comment_with_suggestion( + commit_sha=commit_sha, + file_path=legacy_file, + start_line=start_line, + line=end_line, + body=suggestion_body, + side="RIGHT", + ) + total_comments += 1 + + if total_comments > 0: + logger.info(f"Created {total_comments} total comments on legacy file changes") + + def _attempt_legacy_conversion( + self, + legacy_file: str, + entry_text: str, + added_lines: List[tuple], + pr_diff: str, + pr_files: List[str], + ) -> Optional[str]: + """Attempt to convert a legacy changelog entry to logchange format. + + Args: + legacy_file: Path to the legacy changelog file + entry_text: The extracted changelog entry text + added_lines: List of tuples (line_number, line_content) + pr_diff: The diff of the legacy file + pr_files: All files modified in the PR + + Returns: + The generated logchange YAML entry, or None if conversion failed + """ + try: + logger.info(f"Converting legacy changelog: {legacy_file}") + # Build context about the legacy entry context = self.legacy_handler.build_legacy_context(entry_text) logger.info(f"Legacy entry context: {context}") @@ -646,7 +833,8 @@ def _convert_legacy_to_logchange( if not generated_entry: # Conversion failed - helper already posted error comment - return 0 + logger.warning("Legacy changelog conversion failed") + return None # Check if Claude detected the entry as irrelevant if "IRRELEVANT_ENTRY" in generated_entry: @@ -657,65 +845,29 @@ def _convert_legacy_to_logchange( "ℹ️ Found changes to changelog but the entry appears unrelated to the code changes in this PR. " "Skipping conversion. Please review the entry in the changelog manually if you think this is incorrect." ) - self.set_output("legacy-entry-found", "true") self.set_output("legacy-converted", "false") - return 0 - - # Post review comments with suggested removal of legacy changelog lines - commit_sha = pr_info.get("head", {}).get("sha", "") - if added_lines and commit_sha: - # Group consecutive lines for multi-line suggestions - line_groups = self.legacy_handler.group_consecutive_lines(added_lines) - logger.info( - f"Creating {len(line_groups)} review comment(s) for removal suggestions" - ) - - for start_line, end_line, group_content in line_groups: - is_single_line = start_line == end_line - - if is_single_line: - # For single-line removals, use GitHub's suggestion syntax - suggestion_body = ( - "This was converted to logchange format. Let's remove it.\n\n" - "```suggestion\n" - "```" - ) - self.github_client.create_review_comment_with_suggestion( - commit_sha=commit_sha, - file_path=legacy_file, - line=end_line, - body=suggestion_body, - ) - else: - # For multi-line removals, post a comment with line range - suggestion_body = ( - f"Lines {start_line}-{end_line}: This was converted to logchange format. " - "Please remove these lines." - ) - self.github_client.create_review_comment_with_suggestion( - commit_sha=commit_sha, - file_path=legacy_file, - start_line=start_line, - line=end_line, - body=suggestion_body, - ) - - # Post the converted entry as a regular comment - suggestion_comment = self._format_legacy_conversion_comment( - generated_entry, legacy_file - ) - self.github_client.comment_on_pr(suggestion_comment) + return None - self.set_output("legacy-converted", "true") logger.info("Legacy changelog successfully converted to logchange format") - return 0 + return generated_entry except Exception as e: logger.error(f"Legacy conversion failed: {e}", exc_info=True) - error_msg = f"Legacy changelog conversion failed: {str(e)}" - self.github_client.comment_on_pr(f"❌ {error_msg}") + error_msg = f"Could not convert legacy changelog entry: {str(e)}" + self.github_client.comment_on_pr(f"⚠️ {error_msg}") self.set_output("generation-error", str(e)) - return 1 + return None + + def _convert_legacy_to_logchange( + self, legacy_file: str, pr_files: List[str] + ) -> int: + """Convert a legacy changelog entry to logchange format (legacy method, deprecated)""" + # This method is deprecated and kept for backward compatibility + # New code should use _handle_legacy_changelog with proper on_legacy_entry handling + logger.warning( + "_convert_legacy_to_logchange() is deprecated, use _handle_legacy_changelog() instead" + ) + return 1 def _format_legacy_conversion_comment( self, generated_entry: str, legacy_file: str @@ -730,7 +882,7 @@ def _format_legacy_conversion_comment( filename = generate_changelog_slug(pr_number, pr_title) file_path = f"{self.changelog_path}/{filename}" - return f"""🔄 **I've converted the changelog entry to logchange format!** + comment = f"""🔄 **I've converted the changelog entry to logchange format!** I detected changes to `{legacy_file}` and converted them to the logchange format below. @@ -757,7 +909,17 @@ def _format_legacy_conversion_comment( **Why?** This project uses logchange format for changelog entries. Using logchange ensures consistency and better tooling support across all changelog entries. -""" + +--- + +**To re-generate this suggestion:** Delete this comment and make a new commit.""" + + # Add skip labels info if configured + if self.skip_changelog_labels: + labels_str = ", ".join(self.skip_changelog_labels) + comment += f"\n\n**You can skip my suggestions** by adding one of these labels to the PR: `{labels_str}`" + + return comment def main():