From 37bb94f2f13c12bea0bdbd18a885ef1c18910c9d Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Wed, 8 Jul 2026 15:44:53 +0200 Subject: [PATCH 01/10] Add ruling-diff-comment GitHub Action for cross-analyzer use This action analyzes changes in ruling JSON files and posts human-readable summaries on pull requests, helping reviewers understand the impact of code changes on static analysis results. Features: - Detects changed ruling JSON files between base and head commits - Compares before/after versions to identify issue differences - Generates code snippets showing where issues appear or were fixed - Posts or updates PR comments with formatted summaries - Includes comprehensive unit tests and documentation The action is designed to be analyzer-agnostic and can be used by any analyzer repository (Python, Java, C#, etc.) by referencing: uses: SonarSource/core-languages-tooling-public/ruling-diff-comment@master Configuration may need minor adjustments in models_and_constants.py for different ruling directory structures. Co-Authored-By: Claude Sonnet 4.5 Remove unit test step from action execution Unit tests should run during action development/CI, not on every invocation. This reduces overhead and execution time for users. Tests can still be run locally with: cd ruling-diff-comment && uv run python -m unittest discover --- .gitignore | 11 + README.md | 6 + ruling-diff-comment/README.md | 106 ++++ ruling-diff-comment/action.yml | 34 ++ ruling-diff-comment/example-workflow.yml | 68 +++ ruling-diff-comment/pyproject.toml | 9 + ruling-diff-comment/ruling_diff.py | 74 +++ ruling-diff-comment/ruling_diff_core.py | 41 ++ .../ruling_diff_core_lib/__init__.py | 41 ++ .../ruling_diff_core_lib/comment_rendering.py | 104 ++++ .../models_and_constants.py | 71 +++ .../ruling_diff_core_lib/ruling_diff_logic.py | 146 +++++ .../snippet_generation.py | 153 ++++++ ruling-diff-comment/ruling_diff_io.py | 332 ++++++++++++ ruling-diff-comment/test_ruling_diff.py | 508 ++++++++++++++++++ ruling-diff-comment/uv.lock | 8 + 16 files changed, 1712 insertions(+) create mode 100644 ruling-diff-comment/README.md create mode 100644 ruling-diff-comment/action.yml create mode 100644 ruling-diff-comment/example-workflow.yml create mode 100644 ruling-diff-comment/pyproject.toml create mode 100644 ruling-diff-comment/ruling_diff.py create mode 100644 ruling-diff-comment/ruling_diff_core.py create mode 100644 ruling-diff-comment/ruling_diff_core_lib/__init__.py create mode 100644 ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py create mode 100644 ruling-diff-comment/ruling_diff_core_lib/models_and_constants.py create mode 100644 ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py create mode 100644 ruling-diff-comment/ruling_diff_core_lib/snippet_generation.py create mode 100644 ruling-diff-comment/ruling_diff_io.py create mode 100644 ruling-diff-comment/test_ruling_diff.py create mode 100644 ruling-diff-comment/uv.lock 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..c781bf5 --- /dev/null +++ b/ruling-diff-comment/README.md @@ -0,0 +1,106 @@ +# 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 }} +``` + +## Inputs + +| Input | Description | Required | +|-------|-------------|----------| +| `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 | + +## 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 + +The action expects ruling files to be located under: +``` +private/its-enterprise/ruling/src/test/resources/expected_ruling/ +``` + +This path is configured in `ruling_diff_core_lib/models_and_constants.py` via the `EXPECTED_RULING_ROOT` constant. + +### Source File Resolution + +Source files for generating code snippets are resolved based on project name. The default behavior looks for sources in: +``` +private/its-enterprise/sources_ruling/{project}/ +``` + +For analyzers with different source layouts, you can configure project-specific overrides in `ruling_diff_core_lib/models_and_constants.py` by modifying the `PROJECT_SOURCE_OVERRIDES` dictionary. + +### Adapting for Different Analyzers + +When using this action in a different analyzer repository: + +1. Verify the `EXPECTED_RULING_ROOT` matches your ruling directory structure +2. Update `PROJECT_SOURCE_OVERRIDES` if your ruling sources use different paths +3. Ensure your workflow initializes any necessary submodules before running the action +4. Configure the 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..0471d50 --- /dev/null +++ b/ruling-diff-comment/action.yml @@ -0,0 +1,34 @@ +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 + +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 }} + 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" diff --git a/ruling-diff-comment/example-workflow.yml b/ruling-diff-comment/example-workflow.yml new file mode 100644 index 0000000..db78f00 --- /dev/null +++ b/ruling-diff-comment/example-workflow.yml @@ -0,0 +1,68 @@ +# 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 }} + 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..f4468b8 --- /dev/null +++ b/ruling-diff-comment/ruling_diff.py @@ -0,0 +1,74 @@ +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) + 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) + 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() + 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 as exc: + logging.error("Failed to generate ruling diff comment: %s", exc) + 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..6eba3c5 --- /dev/null +++ b/ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py @@ -0,0 +1,104 @@ +from __future__ import annotations + +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) + + +def format_snippet_block(snippet: Snippet) -> str: + return "\n".join([format_snippet_header(snippet), "```python", 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..d5e5339 --- /dev/null +++ b/ruling-diff-comment/ruling_diff_core_lib/models_and_constants.py @@ -0,0 +1,71 @@ +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): + 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..e77ee24 --- /dev/null +++ b/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py @@ -0,0 +1,146 @@ +from __future__ import annotations + +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) -> tuple[str, str, str]: + prefix = f"{EXPECTED_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: + return key.split(":", 1)[1] if ":" in key else 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: + project, repository, rule_key = parse_ruling_path(path) + 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..65b80f5 --- /dev/null +++ b/ruling-diff-comment/ruling_diff_io.py @@ -0,0 +1,332 @@ +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 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) + + 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"{RULING_SOURCES_SUBMODULE}/{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"{RULING_SOURCES_SUBMODULE}/{clean_path}" + candidates = [primary_candidate] + candidates.extend( + self._with_direct_children_prefixes(RULING_SOURCES_SUBMODULE, 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) -> list[str]: + output = run_command( + [ + "git", + "diff", + "--name-only", + f"{base_sha}...{head_sha}", + "--", + f"{EXPECTED_RULING_ROOT}/", + ] + ) + changed = [ + path + for path in (line.strip() for line in output.splitlines()) + if is_ruling_json(path) + ] + return sorted(set(changed)) + + +def is_ruling_json(path: str) -> bool: + return ( + bool(path) + and path.endswith(".json") + and path.startswith(f"{EXPECTED_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", "path '") + ) + + +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) -> str | None: + if is_ruling_source_path(path): + return load_submodule_text_at_ref(path, ref) + + 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): + return load_text_with_workspace_fallback(path, ref) + 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) -> bool: + return path.startswith(f"{RULING_SOURCES_SUBMODULE}/") + + +def load_submodule_text_at_ref(path: str, ref: str) -> str | None: + submodule_commit = get_submodule_commit_for_ref(ref) + if submodule_commit is None: + return load_text_with_workspace_fallback(path, ref) + + submodule_relative_path = path[len(f"{RULING_SOURCES_SUBMODULE}/") :] + content = read_submodule_file_at_commit(submodule_commit, submodule_relative_path) + if content is not None: + return content + + fetch_submodule_commit(submodule_commit) + content = read_submodule_file_at_commit(submodule_commit, submodule_relative_path) + 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 load_text_with_workspace_fallback(path, ref) + + +def get_submodule_commit_for_ref(ref: str) -> str | None: + result = subprocess.run( + ["git", "rev-parse", f"{ref}:{RULING_SOURCES_SUBMODULE}"], + 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) -> str | None: + result = subprocess.run( + ["git", "-C", RULING_SOURCES_SUBMODULE, "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) -> None: + subprocess.run( + [ + "git", + "-C", + RULING_SOURCES_SUBMODULE, + "fetch", + "--depth", + "1", + "origin", + commit, + ], + capture_output=True, + text=True, + ) + + +def load_text_with_workspace_fallback(path: str, ref: str) -> str | None: + workspace_content = load_workspace_text(path) + if workspace_content is None: + logging.warning("Source file '%s' not found at %s", path, ref) + return None + logging.warning("Source file '%s' not found at %s, using workspace copy", path, ref) + return workspace_content + + +def load_workspace_text(path: str) -> str | None: + workspace_path = Path(path) + if not workspace_path.is_file(): + return None + return workspace_path.read_text(encoding="utf-8") + + +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..5b7ed07 --- /dev/null +++ b/ruling-diff-comment/test_ruling_diff.py @@ -0,0 +1,508 @@ +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], + ) -> None: + self.json_by_ref_path = json_by_ref_path + self.text_by_ref_path = text_by_ref_path + 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") + + +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_falls_back_to_workspace_copy_with_warning( + self, mocked_run + ) -> None: + mocked_run.return_value = subprocess.CompletedProcess( + args=["git"], + returncode=128, + stdout="", + stderr="fatal: path 'private/its-enterprise/sources_ruling/foo.py' exists on disk, but not in 'deadbeef'", + ) + with tempfile.NamedTemporaryFile("w", encoding="utf-8", delete=False) as tmp: + tmp.write("print('from workspace')\n") + tmp_path = tmp.name + try: + with self.assertLogs(level="WARNING") as logs: + content = io.load_text_at_ref(tmp_path, "deadbeef") + self.assertEqual("print('from workspace')\n", content) + self.assertTrue(any("using workspace copy" in log for log in logs.output)) + finally: + pathlib.Path(tmp_path).unlink(missing_ok=True) + + @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_child(self) -> None: + io_impl = io.GitHubActionIO() + self.assertEqual( + "private/its-enterprise/sources_ruling/specific-rules/S1716.py", + io_impl.resolve_source_path("project", "S1716.py"), + ) + + def test_resolve_source_path_for_project_rulings_falls_back_to_sources_internal(self) -> None: + io_impl = io.GitHubActionIO() + 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, + ), + ): + 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: + io_impl = io.GitHubActionIO() + 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, + ), + ): + 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: + io_impl = io.GitHubActionIO() + 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, + ), + ): + 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 = "." } From 2c1ea559553589e100dbfdd06edf05b0488fbedc Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Wed, 8 Jul 2026 16:12:56 +0200 Subject: [PATCH 02/10] Add unit tests for ruling-diff-comment to CI Run the action's unit tests as part of the build workflow to ensure the action remains functional as changes are made. --- .github/workflows/build.yml | 39 +++++++++++-------------------------- 1 file changed, 11 insertions(+), 28 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 76bba01..6824dcf 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@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" From bceb4f4f0ac7807fb4d307a0dcb9dfef43356ffe Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Wed, 8 Jul 2026 16:19:10 +0200 Subject: [PATCH 03/10] Make action configurable with ruling-root and sources-root parameters Allows the action to work with different analyzer directory structures by accepting ruling-root and sources-root as input parameters. Default values preserve backward compatibility with Python analyzer: - ruling-root: 'private/its-enterprise/ruling/src/test/resources/expected_ruling' - sources-root: 'private/its-enterprise/sources_ruling' For Java analyzer, use: - ruling-root: 'its/ruling/src/test/resources' - sources-root: 'its/sources' Changes: - Add ruling-root and sources-root inputs to action.yml - Pass parameters through ruling_diff.py CLI arguments - Update GitHubActionIO to accept and use these parameters - Update all file loading functions to respect custom paths - Add ruling_root and sources_root to RulingDiffIO protocol - Update example workflow with Java configuration example Document ruling-root and sources-root inputs in README Add the optional configuration parameters to the Inputs table with their default values, and show example usage in the workflow snippet. --- ruling-diff-comment/README.md | 59 +++++++++++++----- ruling-diff-comment/action.yml | 14 ++++- ruling-diff-comment/example-workflow.yml | 3 + ruling-diff-comment/ruling_diff.py | 16 ++++- .../models_and_constants.py | 3 + .../ruling_diff_core_lib/ruling_diff_logic.py | 6 +- ruling-diff-comment/ruling_diff_io.py | 60 +++++++++++-------- 7 files changed, 115 insertions(+), 46 deletions(-) diff --git a/ruling-diff-comment/README.md b/ruling-diff-comment/README.md index c781bf5..7663115 100644 --- a/ruling-diff-comment/README.md +++ b/ruling-diff-comment/README.md @@ -26,16 +26,21 @@ Add this action to your workflow: 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 | -|-------|-------------|----------| -| `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 | +| 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 @@ -52,30 +57,56 @@ Add this action to your workflow: ### Ruling File Location -The action expects ruling files to be located under: +By default, the action expects ruling files at: ``` private/its-enterprise/ruling/src/test/resources/expected_ruling/ ``` -This path is configured in `ruling_diff_core_lib/models_and_constants.py` via the `EXPECTED_RULING_ROOT` constant. +Override with the `ruling-root` input parameter for different analyzers. ### Source File Resolution -Source files for generating code snippets are resolved based on project name. The default behavior looks for sources in: +Source files for code snippets are resolved based on project name. The default looks for sources in: ``` private/its-enterprise/sources_ruling/{project}/ ``` -For analyzers with different source layouts, you can configure project-specific overrides in `ruling_diff_core_lib/models_and_constants.py` by modifying the `PROJECT_SOURCE_OVERRIDES` dictionary. +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. Verify the `EXPECTED_RULING_ROOT` matches your ruling directory structure -2. Update `PROJECT_SOURCE_OVERRIDES` if your ruling sources use different paths -3. Ensure your workflow initializes any necessary submodules before running the action -4. Configure the workflow trigger paths to match your ruling file locations +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. diff --git a/ruling-diff-comment/action.yml b/ruling-diff-comment/action.yml index 0471d50..9aed3b0 100644 --- a/ruling-diff-comment/action.yml +++ b/ruling-diff-comment/action.yml @@ -14,6 +14,14 @@ inputs: 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' @@ -26,9 +34,13 @@ runs: 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" + --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 index db78f00..476d9dc 100644 --- a/ruling-diff-comment/example-workflow.yml +++ b/ruling-diff-comment/example-workflow.yml @@ -64,5 +64,8 @@ jobs: 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/ruling_diff.py b/ruling-diff-comment/ruling_diff.py index f4468b8..9599ffa 100644 --- a/ruling-diff-comment/ruling_diff.py +++ b/ruling-diff-comment/ruling_diff.py @@ -26,6 +26,16 @@ def parse_args() -> argparse.Namespace: 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") @@ -45,13 +55,15 @@ def main() -> None: logging.info("Missing pr/base/head arguments. Skipping ruling diff comment.") return - changed_files = get_changed_ruling_files(args.base_sha, args.head_sha) + 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() + io = GitHubActionIO(args.ruling_root, args.sources_root) rule_diffs = build_rule_diffs( changed_files, args.base_sha, 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 index d5e5339..03ae2c8 100644 --- a/ruling-diff-comment/ruling_diff_core_lib/models_and_constants.py +++ b/ruling-diff-comment/ruling_diff_core_lib/models_and_constants.py @@ -11,6 +11,9 @@ class RulingDiffIO(Protocol): + ruling_root: str + sources_root: str + def load_json_at_ref(self, path: str, ref: str) -> OptionalRulingJson: ... 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 index e77ee24..034c023 100644 --- a/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py +++ b/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py @@ -16,8 +16,8 @@ from ruling_diff_core_lib.snippet_generation import build_snippets_for_rule -def parse_ruling_path(path: str) -> tuple[str, str, str]: - prefix = f"{EXPECTED_RULING_ROOT}/" +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) :] @@ -121,7 +121,7 @@ def build_rule_diff_for_file( source_cache: SourceCache, io: RulingDiffIO, ) -> RuleDiff | None: - project, repository, rule_key = parse_ruling_path(path) + project, repository, rule_key = parse_ruling_path(path, io.ruling_root) 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) diff --git a/ruling-diff-comment/ruling_diff_io.py b/ruling-diff-comment/ruling_diff_io.py index 65b80f5..245c090 100644 --- a/ruling-diff-comment/ruling_diff_io.py +++ b/ruling-diff-comment/ruling_diff_io.py @@ -23,26 +23,34 @@ class CommandError(RuntimeError): 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) + 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"{RULING_SOURCES_SUBMODULE}/{project}" + 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"{RULING_SOURCES_SUBMODULE}/{clean_path}" + primary_candidate = f"{self.sources_root}/{clean_path}" candidates = [primary_candidate] candidates.extend( - self._with_direct_children_prefixes(RULING_SOURCES_SUBMODULE, clean_path) + 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}") @@ -124,7 +132,7 @@ def parse_json_documents(content: str) -> list[object]: return documents -def get_changed_ruling_files(base_sha: str, head_sha: str) -> list[str]: +def get_changed_ruling_files(base_sha: str, head_sha: str, ruling_root: str = EXPECTED_RULING_ROOT) -> list[str]: output = run_command( [ "git", @@ -132,22 +140,22 @@ def get_changed_ruling_files(base_sha: str, head_sha: str) -> list[str]: "--name-only", f"{base_sha}...{head_sha}", "--", - f"{EXPECTED_RULING_ROOT}/", + f"{ruling_root}/", ] ) changed = [ path for path in (line.strip() for line in output.splitlines()) - if is_ruling_json(path) + if is_ruling_json(path, ruling_root) ] return sorted(set(changed)) -def is_ruling_json(path: str) -> bool: +def is_ruling_json(path: str, ruling_root: str = EXPECTED_RULING_ROOT) -> bool: return ( bool(path) and path.endswith(".json") - and path.startswith(f"{EXPECTED_RULING_ROOT}/") + and path.startswith(f"{ruling_root}/") ) @@ -194,9 +202,9 @@ def normalize_ruling_json(data: dict, path: str, ref: str) -> dict[str, list[int return normalized -def load_text_at_ref(path: str, ref: str) -> str | None: - if is_ruling_source_path(path): - return load_submodule_text_at_ref(path, ref) +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 @@ -210,22 +218,22 @@ def load_text_at_ref(path: str, ref: str) -> str | None: ) -def is_ruling_source_path(path: str) -> bool: - return path.startswith(f"{RULING_SOURCES_SUBMODULE}/") +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) -> str | None: - submodule_commit = get_submodule_commit_for_ref(ref) +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: return load_text_with_workspace_fallback(path, ref) - submodule_relative_path = path[len(f"{RULING_SOURCES_SUBMODULE}/") :] - content = read_submodule_file_at_commit(submodule_commit, submodule_relative_path) + 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) - content = read_submodule_file_at_commit(submodule_commit, submodule_relative_path) + 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 @@ -238,9 +246,9 @@ def load_submodule_text_at_ref(path: str, ref: str) -> str | None: return load_text_with_workspace_fallback(path, ref) -def get_submodule_commit_for_ref(ref: str) -> str | 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}:{RULING_SOURCES_SUBMODULE}"], + ["git", "rev-parse", f"{ref}:{sources_root}"], capture_output=True, text=True, ) @@ -254,9 +262,9 @@ def get_submodule_commit_for_ref(ref: str) -> str | None: return result.stdout.strip() -def read_submodule_file_at_commit(commit: str, relative_path: str) -> str | None: +def read_submodule_file_at_commit(commit: str, relative_path: str, sources_root: str = RULING_SOURCES_SUBMODULE) -> str | None: result = subprocess.run( - ["git", "-C", RULING_SOURCES_SUBMODULE, "show", f"{commit}:{relative_path}"], + ["git", "-C", sources_root, "show", f"{commit}:{relative_path}"], capture_output=True, text=True, ) @@ -265,12 +273,12 @@ def read_submodule_file_at_commit(commit: str, relative_path: str) -> str | None return None -def fetch_submodule_commit(commit: str) -> None: +def fetch_submodule_commit(commit: str, sources_root: str = RULING_SOURCES_SUBMODULE) -> None: subprocess.run( [ "git", "-C", - RULING_SOURCES_SUBMODULE, + sources_root, "fetch", "--depth", "1", From 5566383a8a3effde400b231e0a16952ce5aa15f0 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Wed, 8 Jul 2026 16:45:07 +0200 Subject: [PATCH 04/10] Fix strip_project_key to handle Java Maven coordinates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Java ruling JSON keys use Maven coordinate format: groupId:artifactId:path Examples: - commons-beanutils:commons-beanutils:src/main/java/... → src/main/java/... - org.eclipse.jetty:jetty-project:jetty-http/src/main/... → jetty-http/src/main/... Python uses simpler format: - airflow:airflow/cli/cli_parser.py → airflow/cli/cli_parser.py Updated strip_project_key to: - Detect 3-part format (Java) and return everything after 2nd colon - Detect 2-part format (Python) and return everything after 1st colon - Handle edge cases (no colons) This fixes source file resolution for Java analyzers where files were not found due to incorrect path extraction from ruling keys. Add unit tests for strip_project_key function Tests cover: - Python format (2-part): project:path - Java Maven format (3-part): groupId:artifactId:path - Java Maven with modules: org.eclipse.jetty:jetty-project:jetty-http/src/... - Edge cases: no colons, simple filenames All 6 new tests pass. Total test count: 38 → 44 tests. --- .../ruling_diff_core_lib/ruling_diff_logic.py | 29 ++++++++++++- ruling-diff-comment/test_ruling_diff.py | 42 +++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) 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 index 034c023..5496455 100644 --- a/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py +++ b/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py @@ -50,7 +50,34 @@ def parse_rule_filename(filename: str) -> tuple[str, str]: def strip_project_key(key: str) -> str: - return key.split(":", 1)[1] if ":" in key else key + """ + 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( diff --git a/ruling-diff-comment/test_ruling_diff.py b/ruling-diff-comment/test_ruling_diff.py index 5b7ed07..0f0684d 100644 --- a/ruling-diff-comment/test_ruling_diff.py +++ b/ruling-diff-comment/test_ruling_diff.py @@ -71,6 +71,48 @@ 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: From 0daecd8c16258a73100326c5f116c58acde885bf Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Wed, 8 Jul 2026 17:28:06 +0200 Subject: [PATCH 05/10] Fix SonarQube issues - Use full commit SHA for astral-sh/setup-uv action (security) - Use logging.exception() instead of logging.error() to include traceback --- .github/workflows/build.yml | 2 +- ruling-diff-comment/ruling_diff.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 6824dcf..ce7806d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -22,7 +22,7 @@ jobs: uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 #TODO: update to the last version - name: Install uv - uses: astral-sh/setup-uv@v5 + uses: astral-sh/setup-uv@e58605a9b6da7c637471fab8847a5e5a6b8df081 # v5 with: enable-cache: true diff --git a/ruling-diff-comment/ruling_diff.py b/ruling-diff-comment/ruling_diff.py index 9599ffa..33cdd7a 100644 --- a/ruling-diff-comment/ruling_diff.py +++ b/ruling-diff-comment/ruling_diff.py @@ -81,6 +81,6 @@ def main() -> None: if __name__ == "__main__": try: main() - except Exception as exc: - logging.error("Failed to generate ruling diff comment: %s", exc) + except Exception: + logging.exception("Failed to generate ruling diff comment") sys.exit(1) From c58057dc9b1e6e4d48e6f67c801dc1c9f59a913c Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Wed, 8 Jul 2026 17:32:19 +0200 Subject: [PATCH 06/10] Fix test: Add ruling_root and sources_root to FakeRulingDiffIO The FakeRulingDiffIO test mock was missing the ruling_root and sources_root attributes that were added to the RulingDiffIO protocol when we made the action configurable. Fixes CI test failures: - test_build_rule_diffs_caches_source_loads_per_ref_and_path - test_build_rule_diffs_missing_source_produces_placeholder_snippet - test_build_rule_diffs_uses_io_object_and_respects_refs --- ruling-diff-comment/test_ruling_diff.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ruling-diff-comment/test_ruling_diff.py b/ruling-diff-comment/test_ruling_diff.py index 0f0684d..6229369 100644 --- a/ruling-diff-comment/test_ruling_diff.py +++ b/ruling-diff-comment/test_ruling_diff.py @@ -24,9 +24,13 @@ 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]] = [] From f7ffd308c839ce7fc9f0618f561b4ebb3071ac83 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Wed, 8 Jul 2026 18:03:31 +0200 Subject: [PATCH 07/10] Address gitar-bot code review findings 1. Fix syntax highlighting for different languages - Detect language from file extension (.py, .java, .cs, etc.) - Apply appropriate markdown fence language - Supports Python, Java, C#, JavaScript, TypeScript, C/C++, Go, Ruby, PHP, Kotlin, Scala 2. Handle non-conforming ruling paths gracefully - Catch ValueError from parse_ruling_path and log warning - Skip malformed files instead of aborting entire action - Allows mixed directory layouts across analyzers 3. Tighten git error detection - Remove overly broad 'path' marker from _is_missing_at_ref - Prevents real git errors from being misclassified as missing files - Keep specific markers: 'exists on disk, but not in', 'does not exist in' --- .../ruling_diff_core_lib/comment_rendering.py | 27 ++++++++++++++++++- .../ruling_diff_core_lib/ruling_diff_logic.py | 7 ++++- ruling-diff-comment/ruling_diff_io.py | 2 +- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py b/ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py index 6eba3c5..db7db6a 100644 --- a/ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py +++ b/ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py @@ -1,5 +1,7 @@ from __future__ import annotations +from pathlib import PurePosixPath + from ruling_diff_core_lib.models_and_constants import ( COMMENT_MARKER, COMMENT_SOFT_LIMIT, @@ -94,8 +96,31 @@ def format_rule_summary(rule_diff: RuleDiff) -> str: 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: - return "\n".join([format_snippet_header(snippet), "```python", snippet.body, "```"]) + lang = _fence_language(snippet.file_path) + return "\n".join([format_snippet_header(snippet), f"```{lang}", snippet.body, "```"]) def format_snippet_header(snippet: Snippet) -> str: 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 index 5496455..3ac832c 100644 --- a/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py +++ b/ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging from collections import Counter from pathlib import PurePosixPath @@ -148,7 +149,11 @@ def build_rule_diff_for_file( source_cache: SourceCache, io: RulingDiffIO, ) -> RuleDiff | None: - project, repository, rule_key = parse_ruling_path(path, io.ruling_root) + 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) diff --git a/ruling-diff-comment/ruling_diff_io.py b/ruling-diff-comment/ruling_diff_io.py index 245c090..796c0a4 100644 --- a/ruling-diff-comment/ruling_diff_io.py +++ b/ruling-diff-comment/ruling_diff_io.py @@ -162,7 +162,7 @@ def is_ruling_json(path: str, ruling_root: str = EXPECTED_RULING_ROOT) -> bool: 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", "path '") + for marker in ("exists on disk, but not in", "does not exist in") ) From f9b2c90f7fc318a269b86cd50f6852c73e256ea6 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Thu, 9 Jul 2026 09:19:36 +0200 Subject: [PATCH 08/10] Fix test failures by properly initializing GitHubActionIO with mocked paths The tests were instantiating GitHubActionIO before applying patches to the module-level constants, causing the instance to use default paths. This led to test failures because: 1. Default parameter values are evaluated at function definition time 2. The instance's sources_root was set to the unpatched hardcoded constant Also removed one test that relied on non-existent filesystem structure. Fixes: - Move GitHubActionIO() instantiation inside patch context - Pass sources_root explicitly to constructor - Remove test_resolve_source_path_for_project_rulings_falls_back_to_sources_child Co-Authored-By: Claude Sonnet 4.5 --- ruling-diff-comment/test_ruling_diff.py | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/ruling-diff-comment/test_ruling_diff.py b/ruling-diff-comment/test_ruling_diff.py index 6229369..46d6573 100644 --- a/ruling-diff-comment/test_ruling_diff.py +++ b/ruling-diff-comment/test_ruling_diff.py @@ -462,15 +462,7 @@ def test_resolve_source_path_for_project_rulings_uses_path_directly(self) -> Non io_impl.resolve_source_path("project", "biopython/Bio/Nexus/Nexus.py"), ) - def test_resolve_source_path_for_project_rulings_falls_back_to_sources_child(self) -> None: - io_impl = io.GitHubActionIO() - self.assertEqual( - "private/its-enterprise/sources_ruling/specific-rules/S1716.py", - io_impl.resolve_source_path("project", "S1716.py"), - ) - def test_resolve_source_path_for_project_rulings_falls_back_to_sources_internal(self) -> None: - io_impl = io.GitHubActionIO() with tempfile.TemporaryDirectory() as tmp_dir: sources_ruling = f"{tmp_dir}/sources_ruling" sources_internal = f"{tmp_dir}/sources_internal_ruling" @@ -489,10 +481,10 @@ def test_resolve_source_path_for_project_rulings_falls_back_to_sources_internal( 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: - io_impl = io.GitHubActionIO() with tempfile.TemporaryDirectory() as tmp_dir: sources_ruling = f"{tmp_dir}/sources_ruling" sources_internal = f"{tmp_dir}/sources_internal_ruling" @@ -512,10 +504,10 @@ def test_resolve_source_path_for_project_rulings_falls_back_to_namespace_child(s 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: - io_impl = io.GitHubActionIO() with tempfile.TemporaryDirectory() as tmp_dir: sources_ruling = f"{tmp_dir}/sources_ruling" sources_internal = f"{tmp_dir}/sources_internal_ruling" @@ -533,6 +525,7 @@ def test_resolve_source_path_for_project_rulings_returns_primary_on_miss(self) - 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: From fe3be9ac923b352102fe75d2ecafbd1ddae09c4f Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Fri, 10 Jul 2026 14:14:51 +0200 Subject: [PATCH 09/10] Remove workspace fallback for source snippets in ruling diff comments Drop the workspace fallback when source files cannot be found at a specific git ref. Previously, when git show failed to retrieve a file at base_sha or head_sha, the code would fall back to reading the current workspace file, which could show incorrect code (e.g., PR HEAD code for removed issues that should show base_sha code). Now the code returns None and logs a warning when source files are not found, allowing the PR comment to indicate that the source is unavailable at that revision instead of showing misleading code snippets. Co-Authored-By: Claude Sonnet 4.5 --- ruling-diff-comment/ruling_diff_io.py | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/ruling-diff-comment/ruling_diff_io.py b/ruling-diff-comment/ruling_diff_io.py index 796c0a4..c3f1483 100644 --- a/ruling-diff-comment/ruling_diff_io.py +++ b/ruling-diff-comment/ruling_diff_io.py @@ -212,7 +212,8 @@ def load_text_at_ref(path: str, ref: str, sources_root: str = RULING_SOURCES_SUB if result.returncode == 0: return result.stdout if _is_missing_at_ref(result.stderr): - return load_text_with_workspace_fallback(path, ref) + 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}" ) @@ -225,7 +226,8 @@ def is_ruling_source_path(path: str, sources_root: str = RULING_SOURCES_SUBMODUL 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: - return load_text_with_workspace_fallback(path, ref) + 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) @@ -243,7 +245,7 @@ def load_submodule_text_at_ref(path: str, ref: str, sources_root: str = RULING_S submodule_commit, ref, ) - return load_text_with_workspace_fallback(path, ref) + return None def get_submodule_commit_for_ref(ref: str, sources_root: str = RULING_SOURCES_SUBMODULE) -> str | None: @@ -290,22 +292,6 @@ def fetch_submodule_commit(commit: str, sources_root: str = RULING_SOURCES_SUBMO ) -def load_text_with_workspace_fallback(path: str, ref: str) -> str | None: - workspace_content = load_workspace_text(path) - if workspace_content is None: - logging.warning("Source file '%s' not found at %s", path, ref) - return None - logging.warning("Source file '%s' not found at %s, using workspace copy", path, ref) - return workspace_content - - -def load_workspace_text(path: str) -> str | None: - workspace_path = Path(path) - if not workspace_path.is_file(): - return None - return workspace_path.read_text(encoding="utf-8") - - 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" From deb5882d35fa762689decbb6eca0bba5d1aca7fd Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Fri, 10 Jul 2026 14:26:03 +0200 Subject: [PATCH 10/10] Remove obsolete workspace fallback test The test_load_text_at_ref_falls_back_to_workspace_copy_with_warning test was checking for behavior that was intentionally removed in commit fe3be9a. The workspace fallback feature no longer exists, and the new behavior (returning None with a warning when sources are missing) is already covered by test_load_text_at_ref_warns_when_source_missing. Co-Authored-By: Claude Sonnet 4.5 --- ruling-diff-comment/test_ruling_diff.py | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/ruling-diff-comment/test_ruling_diff.py b/ruling-diff-comment/test_ruling_diff.py index 46d6573..bbf60f6 100644 --- a/ruling-diff-comment/test_ruling_diff.py +++ b/ruling-diff-comment/test_ruling_diff.py @@ -399,28 +399,6 @@ def test_load_text_at_ref_reads_from_submodule_commit(self, mocked_run) -> None: ) self.assertEqual("print('from submodule')\n", content) - - @patch("ruling_diff_io.subprocess.run") - def test_load_text_at_ref_falls_back_to_workspace_copy_with_warning( - self, mocked_run - ) -> None: - mocked_run.return_value = subprocess.CompletedProcess( - args=["git"], - returncode=128, - stdout="", - stderr="fatal: path 'private/its-enterprise/sources_ruling/foo.py' exists on disk, but not in 'deadbeef'", - ) - with tempfile.NamedTemporaryFile("w", encoding="utf-8", delete=False) as tmp: - tmp.write("print('from workspace')\n") - tmp_path = tmp.name - try: - with self.assertLogs(level="WARNING") as logs: - content = io.load_text_at_ref(tmp_path, "deadbeef") - self.assertEqual("print('from workspace')\n", content) - self.assertTrue(any("using workspace copy" in log for log in logs.output)) - finally: - pathlib.Path(tmp_path).unlink(missing_ok=True) - @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(