diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 76bba01..ce7806d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -14,36 +14,19 @@ concurrency: cancel-in-progress: ${{ github.ref_name != github.event.repository.default_branch }} jobs: - build: - # TODO: Choose the appropriate runner for your repository - # For guidance on runner selection, visit: - # https://xtranet-sonarsource.atlassian.net/wiki/spaces/Platform/pages/3694231566/GitHub+Actions+Runner+-+GitHub - runs-on: github-ubuntu-latest-s # Required for public repositories - name: Build - permissions: - id-token: write - contents: read + test-ruling-diff-comment: + runs-on: ubuntu-latest + name: Test ruling-diff-comment action steps: - name: Checkout uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 #TODO: update to the last version - # SECURITY BEST PRACTICES: - # - Pin all external actions to specific SHA commits (not tags) for security - # - All secrets MUST come from Vault using SonarSource/vault-action-wrapper - # - Never use GitHub Secrets for sensitive data + - name: Install uv + uses: astral-sh/setup-uv@e58605a9b6da7c637471fab8847a5e5a6b8df081 # v5 + with: + enable-cache: true - # TODO: Add your build steps here - # - # For ready-to-use SonarSource GitHub Actions with real-world examples - # and production-ready implementations, visit: - # https://github.com/SonarSource/ci-github-actions - # - # For caching, use SonarSource/gh-action_cache@v1 - # It automatically chooses GitHub Actions cache (public) or S3 cache (private) - # - # Real-world reference repositories: - # - sonar-dummy: Private Java/Maven - # - sonar-dummy-gradle-oss: Public Java/Gradle - # - sonar-dummy-python-oss: Public Python/Poetry - # - sonar-dummy-js: Public NodeJS/Yarn - # + - name: Run unit tests + run: | + cd ruling-diff-comment + uv run python -m unittest discover -v -s . -p "test_ruling_diff.py" diff --git a/.gitignore b/.gitignore index 88dbff1..33f35f9 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,12 @@ .vs/ + +# Python +__pycache__/ +*.py[cod] +*$py.class +*.so +.Python +.venv +venv/ +ENV/ +env/ diff --git a/README.md b/README.md index deaba15..46284bf 100644 --- a/README.md +++ b/README.md @@ -1,3 +1,9 @@ # core-languages-tooling-public Development tooling for Core Languages & Parsers squad — For artifacts accessible from public repos + +## Contents + +### GitHub Actions + +- **[ruling-diff-comment](ruling-diff-comment)** - Analyzes ruling file changes and posts human-readable summaries on PRs diff --git a/ruling-diff-comment/README.md b/ruling-diff-comment/README.md new file mode 100644 index 0000000..7663115 --- /dev/null +++ b/ruling-diff-comment/README.md @@ -0,0 +1,137 @@ +# Ruling Diff Comment Action + +This GitHub Action analyzes changes in ruling JSON files and posts a human-readable summary as a PR comment. + +## Purpose + +When ruling test files are modified in a pull request, this action automatically: +1. Detects changed ruling JSON files +2. Compares before/after versions to identify issue differences +3. Generates code snippets showing where new issues appear or old issues were fixed +4. Posts or updates a comment on the PR with a formatted summary + +This helps reviewers understand the impact of code changes on static analysis results. + +## Usage + +Add this action to your workflow: + +```yaml +- name: Post ruling diff comment + uses: ./.github/actions/ruling-diff-comment + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + with: + pr-number: ${{ github.event.pull_request.number }} + repository: ${{ github.repository }} + base-sha: ${{ github.event.pull_request.base.sha }} + head-sha: ${{ github.event.pull_request.head.sha }} + # Optional: customize for different analyzers + # ruling-root: 'its/ruling/src/test/resources' # For Java + # sources-root: 'its/sources' # For Java +``` + +## Inputs + +| Input | Description | Required | Default | +|-------|-------------|----------|---------| +| `pr-number` | Pull request number | Yes | - | +| `repository` | Repository in `owner/repo` format | Yes | - | +| `base-sha` | Base commit SHA for comparison | Yes | - | +| `head-sha` | Head commit SHA for comparison | Yes | - | +| `ruling-root` | Path to ruling directory | No | `private/its-enterprise/ruling/src/test/resources/expected_ruling` | +| `sources-root` | Path to sources directory | No | `private/its-enterprise/sources_ruling` | + +## Requirements + +- `uv` must be installed in the workflow environment +- `gh` CLI must be configured with appropriate permissions +- Repository must contain ruling files in the expected structure + +## Environment Variables + +- `GH_TOKEN`: GitHub token for posting comments (required) +- `RUNNER_DEBUG`: Set to `1` to enable debug logging (optional) + +## Configuration + +### Ruling File Location + +By default, the action expects ruling files at: +``` +private/its-enterprise/ruling/src/test/resources/expected_ruling/ +``` + +Override with the `ruling-root` input parameter for different analyzers. + +### Source File Resolution + +Source files for code snippets are resolved based on project name. The default looks for sources in: +``` +private/its-enterprise/sources_ruling/{project}/ +``` + +Override with the `sources-root` input parameter for different analyzers. + +### Ruling JSON Key Formats + +The action supports different ruling JSON key formats: + +**Python format** (2-part): +``` +"airflow:airflow/cli/cli_parser.py": [42] +``` +Extracts: `airflow/cli/cli_parser.py` + +**Java Maven format** (3-part): +``` +"commons-beanutils:commons-beanutils:src/main/java/...": [42] +"org.eclipse.jetty:jetty-project:jetty-http/src/main/...": [42] +``` +Extracts: `src/main/java/...` or `jetty-http/src/main/...` + +The action automatically detects and handles both formats. + +### Adapting for Different Analyzers + +When using this action in a different analyzer repository: + +1. **Set `ruling-root` parameter** to match your ruling directory + - Java: `its/ruling/src/test/resources` + - Python: `private/its-enterprise/ruling/src/test/resources/expected_ruling` + +2. **Set `sources-root` parameter** to match your sources directory + - Java: `its/sources` + - Python: `private/its-enterprise/sources_ruling` + +3. **Ensure sources are available** at the specified path + +4. **Configure workflow trigger paths** to match your ruling file locations + +See `example-workflow.yml` for a reference implementation. + +## Development + +### Running Tests + +```bash +cd .github/actions/ruling-diff-comment +uv run python -m unittest discover -v -s . -p "test_ruling_diff.py" +``` + +### Project Structure + +- `action.yml` - Action metadata and workflow steps +- `ruling_diff.py` - Main entry point +- `ruling_diff_io.py` - Git and GitHub API interactions +- `ruling_diff_core.py` - Core module exports +- `ruling_diff_core_lib/` - Core logic modules + - `models_and_constants.py` - Data models and configuration + - `ruling_diff_logic.py` - Diff computation logic + - `snippet_generation.py` - Code snippet rendering + - `comment_rendering.py` - Markdown comment formatting +- `test_ruling_diff.py` - Unit tests + +## License + +See the LICENSE file in the repository root. diff --git a/ruling-diff-comment/action.yml b/ruling-diff-comment/action.yml new file mode 100644 index 0000000..9aed3b0 --- /dev/null +++ b/ruling-diff-comment/action.yml @@ -0,0 +1,46 @@ +name: 'Ruling Diff Comment' +description: 'Posts a human-readable summary of ruling file changes on PRs' + +inputs: + pr-number: + description: 'Pull request number' + required: true + repository: + description: 'owner/repo' + required: true + base-sha: + description: 'Base commit SHA for diff' + required: true + head-sha: + description: 'Head commit SHA for diff' + required: true + ruling-root: + description: 'Path to ruling directory (e.g., its/ruling/src/test/resources for Java)' + required: false + default: 'private/its-enterprise/ruling/src/test/resources/expected_ruling' + sources-root: + description: 'Path to sources directory (e.g., its/sources for Java)' + required: false + default: 'private/its-enterprise/sources_ruling' + +runs: + using: 'composite' + steps: + - name: Generate and post ruling diff comment + shell: bash + env: + GH_TOKEN: ${{ env.GH_TOKEN }} + PR_NUMBER: ${{ inputs.pr-number }} + REPOSITORY: ${{ inputs.repository }} + BASE_SHA: ${{ inputs.base-sha }} + HEAD_SHA: ${{ inputs.head-sha }} + RULING_ROOT: ${{ inputs.ruling-root }} + SOURCES_ROOT: ${{ inputs.sources-root }} + run: | + uv run --project "${{ github.action_path }}" python "${{ github.action_path }}/ruling_diff.py" \ + --pr-number "$PR_NUMBER" \ + --repository "$REPOSITORY" \ + --base-sha "$BASE_SHA" \ + --head-sha "$HEAD_SHA" \ + --ruling-root "$RULING_ROOT" \ + --sources-root "$SOURCES_ROOT" diff --git a/ruling-diff-comment/example-workflow.yml b/ruling-diff-comment/example-workflow.yml new file mode 100644 index 0000000..476d9dc --- /dev/null +++ b/ruling-diff-comment/example-workflow.yml @@ -0,0 +1,71 @@ +# Example workflow for using ruling-diff-comment action +# This is a reference implementation that can be adapted for different analyzers + +name: Ruling Diff Comment + +on: + pull_request: + types: [opened, synchronize, reopened] + paths: + # Adjust this path to match your analyzer's ruling directory structure + - 'private/its-enterprise/ruling/src/test/resources/expected_ruling/**' + # Include action changes to test modifications + - '.github/actions/ruling-diff-comment/**' + - '.github/workflows/ruling-diff-comment.yml' + workflow_dispatch: + inputs: + pr-number: + description: 'Pull request number' + required: true + type: string + base-sha: + description: 'Base commit SHA for diff' + required: true + type: string + head-sha: + description: 'Head commit SHA for diff' + required: true + type: string + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +permissions: + contents: read + pull-requests: write + +jobs: + ruling-diff-comment: + runs-on: ubuntu-latest # Adjust runner type as needed + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 + + # If your analyzer uses submodules for ruling sources, initialize them here + # - name: Checkout ruling sources submodule + # run: git submodule update --init --recursive --depth 1 private/its-enterprise/sources_ruling + + # Install required tools (uv and gh CLI) + - name: Install uv + uses: astral-sh/setup-uv@v5 + with: + enable-cache: true + + # gh CLI is typically pre-installed on GitHub runners + # If not, add installation step here + + # Use the action from the public repository + - name: Post ruling diff comment + uses: SonarSource/core-languages-tooling-public/ruling-diff-comment@master + with: + pr-number: ${{ inputs.pr-number || github.event.pull_request.number }} + repository: ${{ github.repository }} + base-sha: ${{ inputs.base-sha || github.event.pull_request.base.sha }} + head-sha: ${{ inputs.head-sha || github.event.pull_request.head.sha }} + # For Java analyzer, use: + # ruling-root: 'its/ruling/src/test/resources' + # sources-root: 'its/sources' + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/ruling-diff-comment/pyproject.toml b/ruling-diff-comment/pyproject.toml new file mode 100644 index 0000000..89b0ba6 --- /dev/null +++ b/ruling-diff-comment/pyproject.toml @@ -0,0 +1,9 @@ +[project] +name = "ruling-diff-comment" +version = "0.1.0" +description = "GitHub Action helper for ruling diff comments" +requires-python = ">=3.10" +dependencies = [] + +[tool.uv] +package = false diff --git a/ruling-diff-comment/ruling_diff.py b/ruling-diff-comment/ruling_diff.py new file mode 100644 index 0000000..33cdd7a --- /dev/null +++ b/ruling-diff-comment/ruling_diff.py @@ -0,0 +1,86 @@ +from __future__ import annotations + +import argparse +import logging +import os +import sys + +from ruling_diff_core import build_rule_diffs, format_comment +from ruling_diff_io import ( + GitHubActionIO, + get_changed_ruling_files, + post_or_update_comment, +) + + +def configure_logging() -> None: + level = logging.DEBUG if os.environ.get("RUNNER_DEBUG") else logging.INFO + logging.basicConfig(level=level, format="%(asctime)s %(levelname)s %(message)s") + + +def parse_args() -> argparse.Namespace: + parser = argparse.ArgumentParser( + description="Generate and post ruling diff comment" + ) + parser.add_argument("--pr-number", required=True) + parser.add_argument("--repository", required=True) + parser.add_argument("--base-sha", required=True) + parser.add_argument("--head-sha", required=True) + parser.add_argument( + "--ruling-root", + default="private/its-enterprise/ruling/src/test/resources/expected_ruling", + help="Path to ruling directory" + ) + parser.add_argument( + "--sources-root", + default="private/its-enterprise/sources_ruling", + help="Path to sources directory" + ) + args = parser.parse_args() + if "/" not in args.repository: + raise ValueError("--repository must be in owner/repo format") + return args + + +def has_required_context(args: argparse.Namespace) -> bool: + return bool( + args.pr_number.strip() and args.base_sha.strip() and args.head_sha.strip() + ) + + +def main() -> None: + configure_logging() + args = parse_args() + if not has_required_context(args): + logging.info("Missing pr/base/head arguments. Skipping ruling diff comment.") + return + + changed_files = get_changed_ruling_files( + args.base_sha, args.head_sha, args.ruling_root + ) + if not changed_files: + logging.info("No changed ruling json files found. Nothing to do.") + return + + logging.info("Found %d changed ruling json files", len(changed_files)) + io = GitHubActionIO(args.ruling_root, args.sources_root) + rule_diffs = build_rule_diffs( + changed_files, + args.base_sha, + args.head_sha, + io, + ) + if not rule_diffs: + logging.info("Changed files have no issue deltas. No comment will be posted.") + return + + comment = format_comment(rule_diffs) + post_or_update_comment(args.pr_number, args.repository, comment) + + +if __name__ == "__main__": + try: + main() + except Exception: + logging.exception("Failed to generate ruling diff comment") + sys.exit(1) diff --git a/ruling-diff-comment/ruling_diff_core.py b/ruling-diff-comment/ruling_diff_core.py new file mode 100644 index 0000000..c8e633e --- /dev/null +++ b/ruling-diff-comment/ruling_diff_core.py @@ -0,0 +1,41 @@ +from ruling_diff_core_lib.comment_rendering import format_comment +from ruling_diff_core_lib.models_and_constants import ( + COMMENT_MARKER, + COMMENT_SOFT_LIMIT, + EXPECTED_RULING_ROOT, + IssueDiff, + RuleDiff, + Snippet, +) +from ruling_diff_core_lib.ruling_diff_logic import ( + build_rule_diffs, + diff_ruling_jsons, + parse_ruling_path, + parse_ruling_relative_path, + parse_rule_filename, + strip_project_key, +) +from ruling_diff_core_lib.snippet_generation import ( + render_file_level_snippet, + render_line_snippet, + render_snippet, +) + +__all__ = [ + "COMMENT_MARKER", + "COMMENT_SOFT_LIMIT", + "EXPECTED_RULING_ROOT", + "IssueDiff", + "RuleDiff", + "Snippet", + "build_rule_diffs", + "diff_ruling_jsons", + "format_comment", + "parse_ruling_path", + "parse_ruling_relative_path", + "parse_rule_filename", + "render_file_level_snippet", + "render_line_snippet", + "render_snippet", + "strip_project_key", +] diff --git a/ruling-diff-comment/ruling_diff_core_lib/__init__.py b/ruling-diff-comment/ruling_diff_core_lib/__init__.py new file mode 100644 index 0000000..c8e633e --- /dev/null +++ b/ruling-diff-comment/ruling_diff_core_lib/__init__.py @@ -0,0 +1,41 @@ +from ruling_diff_core_lib.comment_rendering import format_comment +from ruling_diff_core_lib.models_and_constants import ( + COMMENT_MARKER, + COMMENT_SOFT_LIMIT, + EXPECTED_RULING_ROOT, + IssueDiff, + RuleDiff, + Snippet, +) +from ruling_diff_core_lib.ruling_diff_logic import ( + build_rule_diffs, + diff_ruling_jsons, + parse_ruling_path, + parse_ruling_relative_path, + parse_rule_filename, + strip_project_key, +) +from ruling_diff_core_lib.snippet_generation import ( + render_file_level_snippet, + render_line_snippet, + render_snippet, +) + +__all__ = [ + "COMMENT_MARKER", + "COMMENT_SOFT_LIMIT", + "EXPECTED_RULING_ROOT", + "IssueDiff", + "RuleDiff", + "Snippet", + "build_rule_diffs", + "diff_ruling_jsons", + "format_comment", + "parse_ruling_path", + "parse_ruling_relative_path", + "parse_rule_filename", + "render_file_level_snippet", + "render_line_snippet", + "render_snippet", + "strip_project_key", +] diff --git a/ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py b/ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py new file mode 100644 index 0000000..db7db6a --- /dev/null +++ b/ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py @@ -0,0 +1,129 @@ +from __future__ import annotations + +from pathlib import PurePosixPath + +from ruling_diff_core_lib.models_and_constants import ( + COMMENT_MARKER, + COMMENT_SOFT_LIMIT, + RuleDiff, + Snippet, +) + + +def format_comment( + rule_diffs: list[RuleDiff], soft_limit: int = COMMENT_SOFT_LIMIT +) -> str: + if not rule_diffs: + return "\n".join( + [ + COMMENT_MARKER, + "## Ruling Diff Summary", + "", + "No issue deltas detected." + ] + ) + comment = format_comment_header(rule_diffs) + return append_rule_sections_with_soft_limit(comment, rule_diffs, soft_limit) + + +def format_comment_header(rule_diffs: list[RuleDiff]) -> str: + added = sum(len(diff.added_lines) for rule in rule_diffs for diff in rule.file_diffs) + removed = sum( + len(diff.removed_lines) for rule in rule_diffs for diff in rule.file_diffs + ) + return "\n".join( + [ + COMMENT_MARKER, + "## Ruling Diff Summary", + "", + f"Detected changes in {len(rule_diffs)} rule files: {removed} issues removed, {added} issues added.", + "", + ] + ) + + +def append_rule_sections_with_soft_limit( + comment: str, rule_diffs: list[RuleDiff], soft_limit: int +) -> str: + sections = [format_rule_section(rule_diff) for rule_diff in rule_diffs] + accepted_sections: list[str] = [] + truncated_count = 0 + for index, section in enumerate(sections): + candidate = comment + "\n\n".join(accepted_sections + [section]) + if len(candidate) > soft_limit: + truncated_count = len(sections) - index + break + accepted_sections.append(section) + if accepted_sections: + comment += "\n\n".join(accepted_sections) + if truncated_count: + comment = append_truncation_notice(comment, truncated_count, bool(accepted_sections)) + return comment + + +def append_truncation_notice(comment: str, count: int, has_sections: bool) -> str: + separator = "\n\n" if has_sections else "" + return ( + comment + + separator + + f"... and {count} more rules with changes (diff too large to display fully)" + ) + + +def format_rule_section(rule_diff: RuleDiff) -> str: + lines = ["
", f"{format_rule_summary(rule_diff)}", ""] + if not rule_diff.snippets: + lines.append("No source snippets available for this rule.") + else: + for snippet in rule_diff.snippets: + lines.append(format_snippet_block(snippet)) + lines.append("") + lines.append("
") + return "\n".join(lines) + + +def format_rule_summary(rule_diff: RuleDiff) -> str: + removed = sum(len(diff.removed_lines) for diff in rule_diff.file_diffs) + added = sum(len(diff.added_lines) for diff in rule_diff.file_diffs) + summary_parts = [ + f"{rule_diff.rule_key} ({rule_diff.repo}) on {rule_diff.project}", + f"{removed} issues removed, {added} issues added", + ] + if rule_diff.is_new_file: + summary_parts.append("new ruling file") + if rule_diff.is_deleted_file: + summary_parts.append("deleted ruling file") + return " - ".join(summary_parts) + + +_FENCE_BY_EXT = { + ".py": "python", + ".java": "java", + ".cs": "csharp", + ".js": "javascript", + ".ts": "typescript", + ".cpp": "cpp", + ".c": "c", + ".h": "cpp", + ".go": "go", + ".rb": "ruby", + ".php": "php", + ".kt": "kotlin", + ".scala": "scala", +} + + +def _fence_language(file_path: str) -> str: + """Get the appropriate markdown fence language for a file based on its extension.""" + return _FENCE_BY_EXT.get(PurePosixPath(file_path).suffix, "") + + +def format_snippet_block(snippet: Snippet) -> str: + lang = _fence_language(snippet.file_path) + return "\n".join([format_snippet_header(snippet), f"```{lang}", snippet.body, "```"]) + + +def format_snippet_header(snippet: Snippet) -> str: + label = "Added" if snippet.change_kind == "added" else "Removed" + location = "file-level" if snippet.line_number == 0 else f"line {snippet.line_number}" + return f"**{label}** `{snippet.file_path}` ({location})" diff --git a/ruling-diff-comment/ruling_diff_core_lib/models_and_constants.py b/ruling-diff-comment/ruling_diff_core_lib/models_and_constants.py new file mode 100644 index 0000000..03ae2c8 --- /dev/null +++ b/ruling-diff-comment/ruling_diff_core_lib/models_and_constants.py @@ -0,0 +1,74 @@ +from __future__ import annotations + +from dataclasses import dataclass +from typing import Protocol + +RulingJson = dict[str, list[int]] +OptionalRulingJson = RulingJson | None +SourceLines = list[str] +OptionalSourceLines = SourceLines | None +SourceCache = dict[tuple[str, str], OptionalSourceLines] + + +class RulingDiffIO(Protocol): + ruling_root: str + sources_root: str + + def load_json_at_ref(self, path: str, ref: str) -> OptionalRulingJson: + ... + + def load_text_at_ref(self, path: str, ref: str) -> str | None: + ... + + def resolve_source_path(self, project: str, file_path: str) -> str: + ... + +EXPECTED_RULING_ROOT = ( + "private/its-enterprise/ruling/src/test/resources/expected_ruling" +) +COMMENT_MARKER = "" +COMMENT_SOFT_LIMIT = 60000 +SNIPPET_CONTEXT = 5 +MAX_SNIPPETS_PER_FILE = 3 +MAX_SNIPPETS_PER_RULE = 15 + +PROJECT_SOURCE_OVERRIDES = { + "buildbot": "private/its-enterprise/sources_ruling/buildbot-0.8.6p1", + "buildbot-slave": "private/its-enterprise/sources_ruling/buildbot-slave-0.8.6p1", + "django": "private/its-enterprise/sources_ruling/django-2.2.3", + "django-cms": "private/its-enterprise/sources_ruling/django-cms-3.7.1", + "docker-compose": "private/its-enterprise/sources_ruling/docker-compose-1.24.1", + "mypy": "private/its-enterprise/sources_ruling/mypy-0.782", + "numpy": "private/its-enterprise/sources_ruling/numpy-1.16.4", + "tornado": "private/its-enterprise/sources_ruling/tornado-2.3", + "twisted": "private/its-enterprise/sources_ruling/twisted-12.1.0", + "sources_internal_ruling": "private/its-enterprise/sources_internal_ruling", + "namespace_basic": "private/its-enterprise/sources_internal_namespace_ruling/basic_namespace", + "namespace_mixed": "private/its-enterprise/sources_internal_namespace_ruling/mixed_namespace", +} + + +@dataclass(frozen=True) +class IssueDiff: + file_path: str + added_lines: list[int] + removed_lines: list[int] + + +@dataclass(frozen=True) +class Snippet: + file_path: str + line_number: int + change_kind: str + body: str + + +@dataclass(frozen=True) +class RuleDiff: + project: str + repo: str + rule_key: str + file_diffs: list[IssueDiff] + snippets: list[Snippet] + is_new_file: bool + is_deleted_file: bool diff --git a/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py b/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py new file mode 100644 index 0000000..3ac832c --- /dev/null +++ b/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py @@ -0,0 +1,178 @@ +from __future__ import annotations + +import logging +from collections import Counter +from pathlib import PurePosixPath + +from ruling_diff_core_lib.models_and_constants import ( + EXPECTED_RULING_ROOT, + IssueDiff, + OptionalRulingJson, + RulingDiffIO, + RuleDiff, + RulingJson, + SourceCache, + Snippet, +) +from ruling_diff_core_lib.snippet_generation import build_snippets_for_rule + + +def parse_ruling_path(path: str, ruling_root: str = EXPECTED_RULING_ROOT) -> tuple[str, str, str]: + prefix = f"{ruling_root}/" + if not path.startswith(prefix): + raise ValueError(f"Unexpected ruling path outside expected root: {path}") + relative_path = path[len(prefix) :] + project, filename = parse_ruling_relative_path(relative_path) + repository, rule_key = parse_rule_filename(filename) + return project, repository, rule_key + + +def parse_ruling_relative_path(relative_path: str) -> tuple[str, str]: + parts = PurePosixPath(relative_path).parts + if len(parts) != 2: + raise ValueError( + f"Expected '/-.json' path, got: {relative_path}" + ) + return parts[0], parts[1] + + +def parse_rule_filename(filename: str) -> tuple[str, str]: + if not filename.endswith(".json"): + raise ValueError(f"Expected json filename, got: {filename}") + basename = filename[:-5] + if "-" not in basename: + raise ValueError(f"Expected '-.json', got: {filename}") + repository, rule_key = basename.rsplit("-", 1) + if not repository: + raise ValueError(f"Missing repo in filename: {filename}") + if not rule_key: + raise ValueError(f"Missing rule key in filename: {filename}") + return repository, rule_key + + +def strip_project_key(key: str) -> str: + """ + Extract file path from ruling JSON key. + + Handles different formats: + - Python: "airflow:airflow/cli/cli_parser.py" -> "airflow/cli/cli_parser.py" + - Java Maven: "commons-beanutils:commons-beanutils:src/main/..." -> "src/main/..." + - Java Maven: "org.eclipse.jetty:jetty-project:jetty-http/src/main/..." -> "jetty-http/src/main/..." + + Strategy: + - If there are 3+ colon-separated parts (Java Maven format), return everything after the 2nd colon + - Otherwise (Python format), return everything after the 1st colon + """ + if ":" not in key: + return key + + parts = key.split(":", 2) # Split into at most 3 parts + + if len(parts) == 3: + # Java Maven format: groupId:artifactId:path + # Return just the path (3rd part) + return parts[2] + elif len(parts) == 2: + # Python format: project:path + # Return just the path (2nd part) + return parts[1] + else: + # No colon, return as-is + return key + + +def diff_ruling_jsons( + old: OptionalRulingJson, new: OptionalRulingJson +) -> list[IssueDiff]: + old_map = old or {} + new_map = new or {} + return [ + issue_diff + for issue_diff in ( + diff_single_file_key(key, old_map, new_map) + for key in sorted(set(old_map) | set(new_map)) + ) + if issue_diff is not None + ] + + +def diff_single_file_key( + key: str, old_map: RulingJson, new_map: RulingJson +) -> IssueDiff | None: + old_counter = Counter(old_map.get(key, [])) + new_counter = Counter(new_map.get(key, [])) + added_lines: list[int] = expand_line_counter(new_counter - old_counter) + removed_lines: list[int] = expand_line_counter(old_counter - new_counter) + if not added_lines and not removed_lines: + return None + return IssueDiff( + file_path=strip_project_key(key), + added_lines=added_lines, + removed_lines=removed_lines, + ) + + +def expand_line_counter(counter: Counter[int]) -> list[int]: + line_numbers: list[int] = [] + for line_number in sorted(counter): + line_numbers.extend([line_number] * counter[line_number]) + return line_numbers + + +def build_rule_diffs( + changed_files: list[str], + base_sha: str, + head_sha: str, + io: RulingDiffIO, +) -> list[RuleDiff]: + source_cache: SourceCache = {} + diffs = [ + build_rule_diff_for_file( + path, + base_sha, + head_sha, + source_cache, + io, + ) + for path in sorted(changed_files) + ] + return sorted( + [rule_diff for rule_diff in diffs if rule_diff is not None], + key=lambda diff: (diff.project, diff.repo, diff.rule_key), + ) + + +def build_rule_diff_for_file( + path: str, + base_sha: str, + head_sha: str, + source_cache: SourceCache, + io: RulingDiffIO, +) -> RuleDiff | None: + try: + project, repository, rule_key = parse_ruling_path(path, io.ruling_root) + except ValueError as exc: + logging.warning("Skipping unrecognized ruling path %s: %s", path, exc) + return None + old_json: OptionalRulingJson = io.load_json_at_ref(path, base_sha) + new_json: OptionalRulingJson = io.load_json_at_ref(path, head_sha) + file_diffs: list[IssueDiff] = diff_ruling_jsons(old_json, new_json) + if not file_diffs: + return None + snippets: list[Snippet] = build_snippets_for_rule( + project, + file_diffs, + source_cache, + base_sha, + head_sha, + io, + ) + return RuleDiff( + project=project, + repo=repository, + rule_key=rule_key, + file_diffs=file_diffs, + snippets=snippets, + is_new_file=old_json is None, + is_deleted_file=new_json is None, + ) diff --git a/ruling-diff-comment/ruling_diff_core_lib/snippet_generation.py b/ruling-diff-comment/ruling_diff_core_lib/snippet_generation.py new file mode 100644 index 0000000..40e3a1f --- /dev/null +++ b/ruling-diff-comment/ruling_diff_core_lib/snippet_generation.py @@ -0,0 +1,153 @@ +from __future__ import annotations + +from ruling_diff_core_lib.models_and_constants import ( + IssueDiff, + MAX_SNIPPETS_PER_FILE, + MAX_SNIPPETS_PER_RULE, + SNIPPET_CONTEXT, + OptionalSourceLines, + RulingDiffIO, + Snippet, + SourceCache, + SourceLines, +) + + +def unique_line_numbers_preserving_order(lines: list[int]) -> list[int]: + return list(dict.fromkeys(lines)) + + +def render_snippet( + lines: OptionalSourceLines, issue_line: int, context: int = SNIPPET_CONTEXT +) -> str: + if lines is None: + return "(source file not found at this revision)" + if issue_line == 0: + return render_file_level_snippet(lines, context) + return render_line_snippet(lines, issue_line, context) + + +def render_file_level_snippet(lines: SourceLines, context: int) -> str: + if not lines: + return ">>> FILE-LEVEL ISSUE\n(empty file)" + end = min(len(lines), 1 + (2 * context)) + content = [f" {index:>6} | {lines[index - 1]}" for index in range(1, end + 1)] + return "\n".join([">>> FILE-LEVEL ISSUE", *content]) + + +def render_line_snippet(lines: SourceLines, issue_line: int, context: int) -> str: + if not lines: + return f">>> ISSUE HERE (line {issue_line})\n(empty file)" + clamped_line = max(1, min(issue_line, len(lines))) + prefix = [] + if issue_line != clamped_line: + prefix.append( + f"(requested line {issue_line} not present, showing closest line {clamped_line})" + ) + body = render_line_window(lines, clamped_line, context) + return "\n".join(prefix + body) + + +def render_line_window(lines: SourceLines, center_line: int, context: int) -> list[str]: + start = max(1, center_line - context) + end = min(len(lines), center_line + context) + rendered: list[str] = [] + for number in range(start, end + 1): + marker = ">>>" if number == center_line else " " + rendered.append(f"{marker} {number:>6} | {lines[number - 1]}") + return rendered + + +def build_snippets_for_rule( + project: str, + file_diffs: list[IssueDiff], + source_cache: SourceCache, + base_sha: str, + head_sha: str, + io: RulingDiffIO, +) -> list[Snippet]: + snippets: list[Snippet] = [] + for file_diff in file_diffs: + snippets.extend( + collect_snippets_for_file( + project=project, + file_diff=file_diff, + source_cache=source_cache, + base_sha=base_sha, + head_sha=head_sha, + io=io, + ) + ) + if len(snippets) >= MAX_SNIPPETS_PER_RULE: + break + return snippets[:MAX_SNIPPETS_PER_RULE] + + +def collect_snippets_for_file( + *, + project: str, + file_diff: IssueDiff, + source_cache: SourceCache, + base_sha: str, + head_sha: str, + io: RulingDiffIO, +) -> list[Snippet]: + snippets: list[Snippet] = [] + for change_kind, lines in ( + ("removed", unique_line_numbers_preserving_order(file_diff.removed_lines)), + ("added", unique_line_numbers_preserving_order(file_diff.added_lines)), + ): + for line_number in lines[:MAX_SNIPPETS_PER_FILE]: + snippets.append( + create_issue_snippet( + project=project, + file_path=file_diff.file_path, + line_number=line_number, + change_kind=change_kind, + source_cache=source_cache, + base_sha=base_sha, + head_sha=head_sha, + io=io, + ) + ) + return snippets + + +def create_issue_snippet( + *, + project: str, + file_path: str, + line_number: int, + change_kind: str, + source_cache: SourceCache, + base_sha: str, + head_sha: str, + io: RulingDiffIO, +) -> Snippet: + ref = head_sha if change_kind == "added" else base_sha + source_path = io.resolve_source_path(project, file_path) + lines = load_source_lines_with_cache(source_cache, ref, source_path, io) + body = ( + f"(source file not found at this revision: {file_path})" + if lines is None + else render_snippet(lines, line_number) + ) + return Snippet( + file_path=file_path, + line_number=line_number, + change_kind=change_kind, + body=body, + ) + + +def load_source_lines_with_cache( + cache: SourceCache, + ref: str, + path: str, + io: RulingDiffIO, +) -> OptionalSourceLines: + key = (ref, path) + if key not in cache: + content = io.load_text_at_ref(path, ref) + cache[key] = None if content is None else content.splitlines() + return cache[key] diff --git a/ruling-diff-comment/ruling_diff_io.py b/ruling-diff-comment/ruling_diff_io.py new file mode 100644 index 0000000..c3f1483 --- /dev/null +++ b/ruling-diff-comment/ruling_diff_io.py @@ -0,0 +1,326 @@ +from __future__ import annotations + +import json +import logging +import subprocess +from pathlib import Path + +from ruling_diff_core_lib.models_and_constants import ( + COMMENT_MARKER, + EXPECTED_RULING_ROOT, + PROJECT_SOURCE_OVERRIDES, +) + +RULING_SOURCES_SUBMODULE = "private/its-enterprise/sources_ruling" +SOURCES_INTERNAL_RULING_ROOT = "private/its-enterprise/sources_internal_ruling" +SOURCES_INTERNAL_NAMESPACE_RULING_ROOT = ( + "private/its-enterprise/sources_internal_namespace_ruling" +) + + +class CommandError(RuntimeError): + pass + + +class GitHubActionIO: + def __init__( + self, + ruling_root: str = EXPECTED_RULING_ROOT, + sources_root: str = RULING_SOURCES_SUBMODULE, + ): + self.ruling_root = ruling_root + self.sources_root = sources_root + + def load_json_at_ref(self, path: str, ref: str) -> dict[str, list[int]] | None: + return load_json_at_ref(path, ref) + + def load_text_at_ref(self, path: str, ref: str) -> str | None: + return load_text_at_ref(path, ref, self.sources_root) + + def resolve_source_path(self, project: str, file_path: str) -> str: + if project == "project": + return self._resolve_project_source_path(file_path) + source_root = PROJECT_SOURCE_OVERRIDES.get( + project, f"{self.sources_root}/{project}" + ) + return f"{source_root}/{file_path.lstrip('/')}" + + def _resolve_project_source_path(self, file_path: str) -> str: + clean_path = file_path.lstrip("/") + primary_candidate = f"{self.sources_root}/{clean_path}" + candidates = [primary_candidate] + candidates.extend( + self._with_direct_children_prefixes(self.sources_root, clean_path) + ) + candidates.append(f"{SOURCES_INTERNAL_RULING_ROOT}/{clean_path}") + candidates.append(f"{SOURCES_INTERNAL_NAMESPACE_RULING_ROOT}/{clean_path}") + candidates.extend( + self._with_direct_children_prefixes( + SOURCES_INTERNAL_NAMESPACE_RULING_ROOT, clean_path + ) + ) + for candidate in candidates: + if Path(candidate).is_file(): + return candidate + return primary_candidate + + def _with_direct_children_prefixes(self, root: str, file_path: str) -> list[str]: + root_path = Path(root) + if not root_path.is_dir(): + return [] + return [ + f"{root}/{child.name}/{file_path}" + for child in sorted(root_path.iterdir(), key=lambda path: path.name) + if child.is_dir() and not child.name.startswith(".") + ] + + +def run_command(command: list[str]) -> str: + result = subprocess.run(command, capture_output=True, text=True) + if result.returncode != 0: + raise CommandError( + format_command_failure( + command, result.stdout, result.stderr, result.returncode + ) + ) + return result.stdout + + +def format_command_failure( + command: list[str], stdout: str, stderr: str, returncode: int +) -> str: + return ( + f"Command failed with exit code {returncode}: {' '.join(command)}\n" + f"stdout: {stdout}\n" + f"stderr: {stderr}" + ) + + +def run_gh_json(command: list[str]) -> dict | list: + output = run_command(["gh", *command]) + try: + return json.loads(output) + except json.JSONDecodeError as exc: + raise CommandError(f"Could not parse JSON from gh output: {exc}") from exc + + +def run_gh_paginated_items(endpoint: str) -> list[dict]: + output = run_command(["gh", "api", "--paginate", endpoint]) + docs = parse_json_documents(output) + items: list[dict] = [] + for doc in docs: + if not isinstance(doc, list): + raise CommandError("Unexpected response type while listing paginated items") + for item in doc: + if isinstance(item, dict): + items.append(item) + return items + + +def parse_json_documents(content: str) -> list[object]: + decoder = json.JSONDecoder() + index = 0 + documents: list[object] = [] + while index < len(content): + while index < len(content) and content[index].isspace(): + index += 1 + if index >= len(content): + break + document, next_index = decoder.raw_decode(content, index) + documents.append(document) + index = next_index + return documents + + +def get_changed_ruling_files(base_sha: str, head_sha: str, ruling_root: str = EXPECTED_RULING_ROOT) -> list[str]: + output = run_command( + [ + "git", + "diff", + "--name-only", + f"{base_sha}...{head_sha}", + "--", + f"{ruling_root}/", + ] + ) + changed = [ + path + for path in (line.strip() for line in output.splitlines()) + if is_ruling_json(path, ruling_root) + ] + return sorted(set(changed)) + + +def is_ruling_json(path: str, ruling_root: str = EXPECTED_RULING_ROOT) -> bool: + return ( + bool(path) + and path.endswith(".json") + and path.startswith(f"{ruling_root}/") + ) + + +def _is_missing_at_ref(stderr: str) -> bool: + return any( + marker in stderr + for marker in ("exists on disk, but not in", "does not exist in") + ) + + +def load_json_at_ref(path: str, ref: str) -> dict[str, list[int]] | None: + result = subprocess.run( + ["git", "show", f"{ref}:{path}"], capture_output=True, text=True + ) + if result.returncode != 0: + if _is_missing_at_ref(result.stderr): + return None + raise CommandError( + f"Failed to read file at ref: git show {ref}:{path}\nstdout: {result.stdout}\nstderr: {result.stderr}" + ) + return parse_ruling_json(result.stdout, path, ref) + + +def parse_ruling_json(content: str, path: str, ref: str) -> dict[str, list[int]]: + try: + data = json.loads(content) + except json.JSONDecodeError as exc: + raise ValueError(f"Malformed JSON in {path} at {ref}: {exc}") from exc + if not isinstance(data, dict): + raise ValueError(f"Ruling file {path} at {ref} must be a JSON object") + return normalize_ruling_json(data, path, ref) + + +def normalize_ruling_json(data: dict, path: str, ref: str) -> dict[str, list[int]]: + normalized: dict[str, list[int]] = {} + for key, value in data.items(): + if not isinstance(key, str): + raise ValueError(f"Ruling file {path} at {ref} has non-string key") + if not isinstance(value, list) or not all(isinstance(v, int) for v in value): + raise ValueError( + f"Ruling file {path} at {ref} has non-integer line list for key {key}" + ) + normalized[key] = value + return normalized + + +def load_text_at_ref(path: str, ref: str, sources_root: str = RULING_SOURCES_SUBMODULE) -> str | None: + if is_ruling_source_path(path, sources_root): + return load_submodule_text_at_ref(path, ref, sources_root) + + result = subprocess.run( + ["git", "show", f"{ref}:{path}"], capture_output=True, text=True + ) + if result.returncode == 0: + return result.stdout + if _is_missing_at_ref(result.stderr): + logging.warning("Source file '%s' not found at %s", path, ref) + return None + raise CommandError( + f"Failed to read source file at ref: git show {ref}:{path}\nstdout: {result.stdout}\nstderr: {result.stderr}" + ) + + +def is_ruling_source_path(path: str, sources_root: str = RULING_SOURCES_SUBMODULE) -> bool: + return path.startswith(f"{sources_root}/") + + +def load_submodule_text_at_ref(path: str, ref: str, sources_root: str = RULING_SOURCES_SUBMODULE) -> str | None: + submodule_commit = get_submodule_commit_for_ref(ref, sources_root) + if submodule_commit is None: + logging.warning("Source file '%s' not found at %s", path, ref) + return None + + submodule_relative_path = path[len(f"{sources_root}/") :] + content = read_submodule_file_at_commit(submodule_commit, submodule_relative_path, sources_root) + if content is not None: + return content + + fetch_submodule_commit(submodule_commit, sources_root) + content = read_submodule_file_at_commit(submodule_commit, submodule_relative_path, sources_root) + if content is not None: + return content + + logging.warning( + "Source file '%s' not found in submodule commit %s for %s", + path, + submodule_commit, + ref, + ) + return None + + +def get_submodule_commit_for_ref(ref: str, sources_root: str = RULING_SOURCES_SUBMODULE) -> str | None: + result = subprocess.run( + ["git", "rev-parse", f"{ref}:{sources_root}"], + capture_output=True, + text=True, + ) + if result.returncode != 0: + logging.warning( + "Could not resolve ruling sources submodule commit for %s: %s", + ref, + result.stderr.strip(), + ) + return None + return result.stdout.strip() + + +def read_submodule_file_at_commit(commit: str, relative_path: str, sources_root: str = RULING_SOURCES_SUBMODULE) -> str | None: + result = subprocess.run( + ["git", "-C", sources_root, "show", f"{commit}:{relative_path}"], + capture_output=True, + text=True, + ) + if result.returncode == 0: + return result.stdout + return None + + +def fetch_submodule_commit(commit: str, sources_root: str = RULING_SOURCES_SUBMODULE) -> None: + subprocess.run( + [ + "git", + "-C", + sources_root, + "fetch", + "--depth", + "1", + "origin", + commit, + ], + capture_output=True, + text=True, + ) + + +def get_existing_comment_id(pr_number: str, repository: str) -> str | None: + comments = run_gh_paginated_items( + f"repos/{repository}/issues/{pr_number}/comments?per_page=100" + ) + for comment in comments: + if COMMENT_MARKER in comment.get("body", ""): + return str(comment["id"]) + return None + + +def post_or_update_comment(pr_number: str, repository: str, body: str) -> None: + comment_id = get_existing_comment_id(pr_number, repository) + if comment_id is None: + logging.info("Posting new ruling diff comment on PR #%s", pr_number) + run_command( + ["gh", "pr", "comment", pr_number, "--repo", repository, "--body", body] + ) + return + logging.info( + "Updating existing ruling diff comment %s on PR #%s", comment_id, pr_number + ) + run_command( + [ + "gh", + "api", + "--method", + "PATCH", + f"repos/{repository}/issues/comments/{comment_id}", + "-f", + f"body={body}", + ] + ) diff --git a/ruling-diff-comment/test_ruling_diff.py b/ruling-diff-comment/test_ruling_diff.py new file mode 100644 index 0000000..bbf60f6 --- /dev/null +++ b/ruling-diff-comment/test_ruling_diff.py @@ -0,0 +1,525 @@ +import pathlib +import subprocess +import sys +import tempfile +import unittest +from unittest.mock import patch + + +MODULE_DIR = pathlib.Path(__file__).parent +if str(MODULE_DIR) not in sys.path: + sys.path.insert(0, str(MODULE_DIR)) + +import ruling_diff_core as core +import ruling_diff_io as io + + +IssueDiff = core.IssueDiff +RuleDiff = core.RuleDiff +Snippet = core.Snippet + + +class FakeRulingDiffIO: + def __init__( + self, + json_by_ref_path: dict[tuple[str, str], dict[str, list[int]] | None], + text_by_ref_path: dict[tuple[str, str], str | None], + ruling_root: str = "private/its-enterprise/ruling/src/test/resources/expected_ruling", + sources_root: str = "private/its-enterprise/sources_ruling", + ) -> None: + self.json_by_ref_path = json_by_ref_path + self.text_by_ref_path = text_by_ref_path + self.ruling_root = ruling_root + self.sources_root = sources_root + self.load_json_calls: list[tuple[str, str]] = [] + self.load_text_calls: list[tuple[str, str]] = [] + self.resolve_calls: list[tuple[str, str]] = [] + + def load_json_at_ref(self, path: str, ref: str) -> dict[str, list[int]] | None: + self.load_json_calls.append((path, ref)) + return self.json_by_ref_path.get((path, ref)) + + def load_text_at_ref(self, path: str, ref: str) -> str | None: + self.load_text_calls.append((path, ref)) + return self.text_by_ref_path.get((path, ref)) + + def resolve_source_path(self, project: str, file_path: str) -> str: + self.resolve_calls.append((project, file_path)) + return f"sources/{project}/{file_path.lstrip('/')}" + + +class ParsePathTest(unittest.TestCase): + def test_parse_ruling_path(self) -> None: + path = "private/its-enterprise/ruling/src/test/resources/expected_ruling/airflow/python-S1066.json" + self.assertEqual(("airflow", "python", "S1066"), core.parse_ruling_path(path)) + + def test_parse_ruling_path_with_pythonenterprise(self) -> None: + path = "private/its-enterprise/ruling/src/test/resources/expected_ruling/specific-rules/pythonenterprise-S7471.json" + self.assertEqual( + ("specific-rules", "pythonenterprise", "S7471"), + core.parse_ruling_path(path), + ) + + def test_parse_ruling_path_with_legacy_key(self) -> None: + path = "private/its-enterprise/ruling/src/test/resources/expected_ruling/scikit-learn/python-LineLength.json" + self.assertEqual( + ("scikit-learn", "python", "LineLength"), + core.parse_ruling_path(path), + ) + + def test_parse_rule_filename_rejects_empty_rule_key(self) -> None: + with self.assertRaises(ValueError): + core.parse_rule_filename("python-.json") + + def test_parse_rule_filename_rejects_empty_repository(self) -> None: + with self.assertRaises(ValueError): + core.parse_rule_filename("-S1066.json") + + def test_strip_project_key_python_format(self) -> None: + # Python format: project:path + self.assertEqual( + "airflow/cli/cli_parser.py", + core.strip_project_key("airflow:airflow/cli/cli_parser.py") + ) + + def test_strip_project_key_java_maven_format_commons_beanutils(self) -> None: + # Java Maven format: groupId:artifactId:path + self.assertEqual( + "src/main/java/org/apache/commons/beanutils2/LazyDynaList.java", + core.strip_project_key("commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/LazyDynaList.java") + ) + + def test_strip_project_key_java_maven_format_eclipse_jetty(self) -> None: + # Java Maven format with module path: groupId:artifactId:module/path + self.assertEqual( + "jetty-http/src/main/java/org/eclipse/jetty/http/QuotedCSVParser.java", + core.strip_project_key("org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/QuotedCSVParser.java") + ) + + def test_strip_project_key_java_maven_format_test_file(self) -> None: + # Java Maven format with test file + self.assertEqual( + "jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java", + core.strip_project_key("org.eclipse.jetty:jetty-project:jetty-io/src/test/java/org/eclipse/jetty/io/IOTest.java") + ) + + def test_strip_project_key_no_colon(self) -> None: + # Edge case: no colon in key + self.assertEqual( + "simple-path/file.py", + core.strip_project_key("simple-path/file.py") + ) + + def test_strip_project_key_no_colon_simple_file(self) -> None: + # Edge case: simple filename with no path separator + self.assertEqual( + "file.py", + core.strip_project_key("file.py") + ) + + +class DiffLogicTest(unittest.TestCase): + def test_diff_ruling_jsons_added_issues(self) -> None: + diffs = core.diff_ruling_jsons({"proj:a.py": [1, 2]}, {"proj:a.py": [1, 2, 3]}) + self.assertEqual(1, len(diffs)) + self.assertEqual("a.py", diffs[0].file_path) + self.assertEqual([3], diffs[0].added_lines) + + def test_diff_ruling_jsons_removed_issues(self) -> None: + diffs = core.diff_ruling_jsons({"proj:a.py": [1, 2, 3]}, {"proj:a.py": [1]}) + self.assertEqual([2, 3], diffs[0].removed_lines) + + def test_diff_ruling_jsons_new_file_entry(self) -> None: + diffs = core.diff_ruling_jsons( + {"proj:a.py": [1]}, + {"proj:a.py": [1], "proj:b.py": [5]}, + ) + self.assertEqual("b.py", diffs[0].file_path) + self.assertEqual([5], diffs[0].added_lines) + + def test_diff_ruling_jsons_removed_file_entry(self) -> None: + diffs = core.diff_ruling_jsons( + {"proj:a.py": [1], "proj:b.py": [5]}, + {"proj:a.py": [1]}, + ) + self.assertEqual("b.py", diffs[0].file_path) + self.assertEqual([5], diffs[0].removed_lines) + + def test_diff_ruling_jsons_new_ruling_file(self) -> None: + diffs = core.diff_ruling_jsons(None, {"proj:a.py": [10]}) + self.assertEqual([10], diffs[0].added_lines) + + def test_diff_ruling_jsons_deleted_ruling_file(self) -> None: + diffs = core.diff_ruling_jsons({"proj:a.py": [10]}, None) + self.assertEqual([10], diffs[0].removed_lines) + + def test_diff_ruling_jsons_no_changes(self) -> None: + self.assertEqual( + [], + core.diff_ruling_jsons( + {"proj:a.py": [10], "proj:b.py": [11, 12]}, + {"proj:a.py": [10], "proj:b.py": [11, 12]}, + ), + ) + + def test_duplicate_line_numbers_preserved(self) -> None: + diffs = core.diff_ruling_jsons({"proj:a.py": [297]}, {"proj:a.py": [297, 297]}) + self.assertEqual([297], diffs[0].added_lines) + + +class FormattingTest(unittest.TestCase): + def test_format_comment_single_rule(self) -> None: + rule_diff = RuleDiff( + project="airflow", + repo="python", + rule_key="S107", + file_diffs=[IssueDiff("airflow/hooks/a.py", [10, 11], [8])], + snippets=[ + Snippet( + file_path="airflow/hooks/a.py", + line_number=10, + change_kind="added", + body=">>> 10 | x = 1", + ) + ], + is_new_file=False, + is_deleted_file=False, + ) + comment = core.format_comment([rule_diff]) + self.assertIn("## Ruling Diff Summary", comment) + self.assertIn("
", comment) + self.assertIn("**Added** `airflow/hooks/a.py` (line 10)", comment) + self.assertIn(">>> 10 | x = 1", comment) + self.assertIn("```python", comment) + + def test_format_comment_multiple_rules(self) -> None: + rule_diffs = [ + RuleDiff( + project="airflow", + repo="python", + rule_key="S107", + file_diffs=[IssueDiff("airflow/hooks/a.py", [10], [])], + snippets=[ + Snippet("airflow/hooks/a.py", 10, "added", ">>> 10 | return 1") + ], + is_new_file=False, + is_deleted_file=False, + ), + RuleDiff( + project="django", + repo="python", + rule_key="S3699", + file_diffs=[IssueDiff("django/core/b.py", [20], [15])], + snippets=[ + Snippet( + "django/core/b.py", 15, "removed", ">>> 15 | return None" + ) + ], + is_new_file=False, + is_deleted_file=False, + ), + ] + comment = core.format_comment(rule_diffs) + self.assertIn("Detected changes in 2 rule files", comment) + self.assertIn("S107", comment) + self.assertIn("S3699", comment) + + def test_format_comment_respects_collapse(self) -> None: + rule_diff = RuleDiff( + project="airflow", + repo="python", + rule_key="S107", + file_diffs=[IssueDiff("airflow/hooks/a.py", [10], [])], + snippets=[ + Snippet("airflow/hooks/a.py", 10, "added", ">>> 10 | return 1") + ], + is_new_file=False, + is_deleted_file=False, + ) + comment = core.format_comment([rule_diff]) + self.assertIn("
", comment) + self.assertIn("
", comment) + + def test_strip_project_key_from_path(self) -> None: + self.assertEqual( + "airflow/foo.py", core.strip_project_key("airflow:airflow/foo.py") + ) + + def test_line_zero_displayed_as_file_level(self) -> None: + rule_diff = RuleDiff( + project="specific-rules", + repo="python", + rule_key="S1451", + file_diffs=[IssueDiff("S1716.py", [0], [0])], + snippets=[Snippet("S1716.py", 0, "added", ">>> FILE-LEVEL ISSUE")], + is_new_file=False, + is_deleted_file=False, + ) + comment = core.format_comment([rule_diff]) + self.assertIn("file-level", comment) + self.assertIn(">>> FILE-LEVEL ISSUE", comment) + + def test_format_comment_truncates_when_limit_reached(self) -> None: + rule_diffs = [ + RuleDiff( + project=f"project-{index}", + repo="python", + rule_key=f"S{1000 + index}", + file_diffs=[IssueDiff("a.py", [1], [2])], + snippets=[ + Snippet( + "a.py", 1, "added", "\n".join([f"line {i}" for i in range(50)]) + ) + ], + is_new_file=False, + is_deleted_file=False, + ) + for index in range(5) + ] + comment = core.format_comment(rule_diffs, soft_limit=500) + self.assertIn("diff too large to display fully", comment) + + +class SnippetRenderingTest(unittest.TestCase): + def test_render_line_snippet_uses_plus_minus_five_lines(self) -> None: + lines = [f"line {i}" for i in range(1, 21)] + rendered = core.render_line_snippet(lines, issue_line=10, context=5) + self.assertIn(" 5 | line 5", rendered) + self.assertIn(">>> 10 | line 10", rendered) + self.assertIn(" 15 | line 15", rendered) + + def test_render_line_snippet_handles_out_of_range_line(self) -> None: + rendered = core.render_line_snippet(["alpha", "beta"], issue_line=99, context=5) + self.assertIn("requested line 99 not present", rendered) + self.assertIn(">>> 2 | beta", rendered) + + def test_render_file_level_snippet_marker(self) -> None: + rendered = core.render_file_level_snippet(["a", "b", "c"], context=5) + self.assertIn(">>> FILE-LEVEL ISSUE", rendered) + + def test_render_snippet_missing_source_placeholder(self) -> None: + rendered = core.render_snippet(None, issue_line=12, context=5) + self.assertEqual("(source file not found at this revision)", rendered) + + +class BuildRuleDiffsWithIOTest(unittest.TestCase): + def test_build_rule_diffs_uses_io_object_and_respects_refs(self) -> None: + changed_file = ( + "private/its-enterprise/ruling/src/test/resources/expected_ruling/" + "airflow/python-S107.json" + ) + io_impl = FakeRulingDiffIO( + json_by_ref_path={ + (changed_file, "base-sha"): {"airflow:a.py": [2]}, + (changed_file, "head-sha"): {"airflow:a.py": [2, 7]}, + }, + text_by_ref_path={ + ("sources/airflow/a.py", "head-sha"): "\n".join( + [f"line {index}" for index in range(1, 12)] + ), + }, + ) + + diffs = core.build_rule_diffs([changed_file], "base-sha", "head-sha", io_impl) + + self.assertEqual(1, len(diffs)) + self.assertEqual("airflow", diffs[0].project) + self.assertEqual("python", diffs[0].repo) + self.assertEqual("S107", diffs[0].rule_key) + self.assertEqual([7], diffs[0].file_diffs[0].added_lines) + self.assertEqual([], diffs[0].file_diffs[0].removed_lines) + self.assertIn((changed_file, "base-sha"), io_impl.load_json_calls) + self.assertIn((changed_file, "head-sha"), io_impl.load_json_calls) + self.assertEqual([("airflow", "a.py")], io_impl.resolve_calls) + self.assertEqual([("sources/airflow/a.py", "head-sha")], io_impl.load_text_calls) + + def test_build_rule_diffs_caches_source_loads_per_ref_and_path(self) -> None: + changed_file = ( + "private/its-enterprise/ruling/src/test/resources/expected_ruling/" + "airflow/python-S107.json" + ) + io_impl = FakeRulingDiffIO( + json_by_ref_path={ + (changed_file, "base-sha"): {"airflow:a.py": [1]}, + (changed_file, "head-sha"): {"airflow:a.py": [2, 2]}, + }, + text_by_ref_path={ + ("sources/airflow/a.py", "base-sha"): "base\ncontent\n", + ("sources/airflow/a.py", "head-sha"): "head\ncontent\n", + }, + ) + + core.build_rule_diffs([changed_file], "base-sha", "head-sha", io_impl) + + self.assertEqual(1, io_impl.load_text_calls.count(("sources/airflow/a.py", "base-sha"))) + self.assertEqual(1, io_impl.load_text_calls.count(("sources/airflow/a.py", "head-sha"))) + + def test_build_rule_diffs_missing_source_produces_placeholder_snippet(self) -> None: + changed_file = ( + "private/its-enterprise/ruling/src/test/resources/expected_ruling/" + "airflow/python-S107.json" + ) + io_impl = FakeRulingDiffIO( + json_by_ref_path={ + (changed_file, "base-sha"): {"airflow:a.py": [1]}, + (changed_file, "head-sha"): {"airflow:a.py": [1, 3]}, + }, + text_by_ref_path={("sources/airflow/a.py", "head-sha"): None}, + ) + + diffs = core.build_rule_diffs([changed_file], "base-sha", "head-sha", io_impl) + + self.assertEqual(1, len(diffs[0].snippets)) + self.assertEqual( + "(source file not found at this revision: a.py)", + diffs[0].snippets[0].body, + ) + + +class SourceLoadingTest(unittest.TestCase): + @patch("ruling_diff_io.subprocess.run") + def test_load_text_at_ref_reads_from_submodule_commit(self, mocked_run) -> None: + mocked_run.side_effect = [ + subprocess.CompletedProcess( + args=["git", "rev-parse"], + returncode=0, + stdout="subsha123\n", + stderr="", + ), + subprocess.CompletedProcess( + args=["git", "-C", "sources", "show"], + returncode=0, + stdout="print('from submodule')\n", + stderr="", + ), + ] + + content = io.load_text_at_ref( + "private/its-enterprise/sources_ruling/project/foo.py", "deadbeef" + ) + + self.assertEqual("print('from submodule')\n", content) + @patch("ruling_diff_io.subprocess.run") + def test_load_text_at_ref_warns_when_source_missing(self, mocked_run) -> None: + mocked_run.return_value = subprocess.CompletedProcess( + args=["git"], + returncode=128, + stdout="", + stderr="fatal: path 'missing.py' exists on disk, but not in 'deadbeef'", + ) + with self.assertLogs(level="WARNING") as logs: + content = io.load_text_at_ref("missing.py", "deadbeef") + self.assertIsNone(content) + self.assertTrue(any("not found at deadbeef" in log for log in logs.output)) + + +class GitHubCommentLookupTest(unittest.TestCase): + @patch("ruling_diff_io.run_command") + def test_get_existing_comment_id_reads_all_pages(self, mocked_run_command) -> None: + mocked_run_command.return_value = ( + '[{"id": 1, "body": "first"}]\n' + '[{"id": 2, "body": "text "}]\n' + ) + + comment_id = io.get_existing_comment_id( + "895", "SonarSource/sonar-python-enterprise" + ) + + self.assertEqual("2", comment_id) + + def test_parse_json_documents_handles_multiple_arrays(self) -> None: + documents = io.parse_json_documents('[{"a":1}]\n[{"b":2}]') + self.assertEqual(2, len(documents)) + + +class GitHubActionIOTest(unittest.TestCase): + def test_resolve_source_path_for_project_rulings_uses_path_directly(self) -> None: + io_impl = io.GitHubActionIO() + self.assertEqual( + "private/its-enterprise/sources_ruling/biopython/Bio/Nexus/Nexus.py", + io_impl.resolve_source_path("project", "biopython/Bio/Nexus/Nexus.py"), + ) + + def test_resolve_source_path_for_project_rulings_falls_back_to_sources_internal(self) -> None: + with tempfile.TemporaryDirectory() as tmp_dir: + sources_ruling = f"{tmp_dir}/sources_ruling" + sources_internal = f"{tmp_dir}/sources_internal_ruling" + sources_namespace = f"{tmp_dir}/sources_internal_namespace_ruling" + pathlib.Path(sources_ruling).mkdir(parents=True, exist_ok=True) + pathlib.Path(sources_internal).mkdir(parents=True, exist_ok=True) + pathlib.Path(sources_namespace).mkdir(parents=True, exist_ok=True) + target = f"{sources_internal}/foo.py" + pathlib.Path(target).write_text("x\n", encoding="utf-8") + with ( + patch.object(io, "RULING_SOURCES_SUBMODULE", sources_ruling), + patch.object(io, "SOURCES_INTERNAL_RULING_ROOT", sources_internal), + patch.object( + io, + "SOURCES_INTERNAL_NAMESPACE_RULING_ROOT", + sources_namespace, + ), + ): + io_impl = io.GitHubActionIO(sources_root=sources_ruling) + self.assertEqual(target, io_impl.resolve_source_path("project", "foo.py")) + + def test_resolve_source_path_for_project_rulings_falls_back_to_namespace_child(self) -> None: + with tempfile.TemporaryDirectory() as tmp_dir: + sources_ruling = f"{tmp_dir}/sources_ruling" + sources_internal = f"{tmp_dir}/sources_internal_ruling" + sources_namespace = f"{tmp_dir}/sources_internal_namespace_ruling" + namespace_child = f"{sources_namespace}/basic_namespace" + pathlib.Path(sources_ruling).mkdir(parents=True, exist_ok=True) + pathlib.Path(sources_internal).mkdir(parents=True, exist_ok=True) + pathlib.Path(namespace_child).mkdir(parents=True, exist_ok=True) + target = f"{namespace_child}/foo.py" + pathlib.Path(target).write_text("x\n", encoding="utf-8") + with ( + patch.object(io, "RULING_SOURCES_SUBMODULE", sources_ruling), + patch.object(io, "SOURCES_INTERNAL_RULING_ROOT", sources_internal), + patch.object( + io, + "SOURCES_INTERNAL_NAMESPACE_RULING_ROOT", + sources_namespace, + ), + ): + io_impl = io.GitHubActionIO(sources_root=sources_ruling) + self.assertEqual(target, io_impl.resolve_source_path("project", "foo.py")) + + def test_resolve_source_path_for_project_rulings_returns_primary_on_miss(self) -> None: + with tempfile.TemporaryDirectory() as tmp_dir: + sources_ruling = f"{tmp_dir}/sources_ruling" + sources_internal = f"{tmp_dir}/sources_internal_ruling" + sources_namespace = f"{tmp_dir}/sources_internal_namespace_ruling" + pathlib.Path(sources_ruling).mkdir(parents=True, exist_ok=True) + pathlib.Path(sources_internal).mkdir(parents=True, exist_ok=True) + pathlib.Path(sources_namespace).mkdir(parents=True, exist_ok=True) + primary = f"{sources_ruling}/missing.py" + with ( + patch.object(io, "RULING_SOURCES_SUBMODULE", sources_ruling), + patch.object(io, "SOURCES_INTERNAL_RULING_ROOT", sources_internal), + patch.object( + io, + "SOURCES_INTERNAL_NAMESPACE_RULING_ROOT", + sources_namespace, + ), + ): + io_impl = io.GitHubActionIO(sources_root=sources_ruling) + self.assertEqual(primary, io_impl.resolve_source_path("project", "missing.py")) + + def test_resolve_source_path_uses_project_overrides(self) -> None: + io_impl = io.GitHubActionIO() + self.assertEqual( + "private/its-enterprise/sources_ruling/mypy-0.782/pkg/file.py", + io_impl.resolve_source_path("mypy", "pkg/file.py"), + ) + + def test_resolve_source_path_uses_default_project_root(self) -> None: + io_impl = io.GitHubActionIO() + self.assertEqual( + "private/its-enterprise/sources_ruling/custom-project/pkg/file.py", + io_impl.resolve_source_path("custom-project", "/pkg/file.py"), + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/ruling-diff-comment/uv.lock b/ruling-diff-comment/uv.lock new file mode 100644 index 0000000..a3e2f30 --- /dev/null +++ b/ruling-diff-comment/uv.lock @@ -0,0 +1,8 @@ +version = 1 +revision = 3 +requires-python = ">=3.10" + +[[package]] +name = "ruling-diff-comment" +version = "0.1.0" +source = { virtual = "." }