CLP-179 Add ruling-diff-comment GitHub Action for cross-analyzer use

[romainbrenguier]

Part of Core Languages & Parsers shared tooling

Summary

This PR adds a reusable GitHub Action (ruling-diff-comment) that analyzes changes in ruling JSON files and posts human-readable summaries on pull requests. This helps reviewers understand the impact of code changes on static analysis results.

The action is now analyzer-agnostic and can be used across Python, Java, C#, and other analyzers.

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
• ✅ Supports both Python and Java Maven ruling key formats
• ✅ Configurable paths for different analyzer structures
• ✅ Comprehensive unit tests (44 tests)
• ✅ CI integration to ensure action stays functional

Configuration

The action accepts optional parameters for different analyzers:

Python (default):

uses: SonarSource/core-languages-tooling-public/ruling-diff-comment@master
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 }}

Java:

uses: SonarSource/core-languages-tooling-public/ruling-diff-comment@master
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 }}
  ruling-root: 'its/ruling/src/test/resources'
  sources-root: 'its/sources'

Ruling Key Format Support

Automatically handles different ruling JSON key formats:

Python (2-part):

{"airflow:airflow/cli/cli_parser.py": [42]}

Java Maven (3-part):

{
  "commons-beanutils:commons-beanutils:src/main/java/...": [42],
  "org.eclipse.jetty:jetty-project:jetty-http/src/main/...": [42]
}

Testing

✅ Tested on sonar-java: SonarSource/sonar-java#5761 (comment)

The action successfully:

• Detected changed ruling files in Java format
• Parsed Java Maven coordinates correctly
• Resolved source files and generated snippets
• Posted formatted PR comments

Commits

1. Add ruling-diff-comment GitHub Action - Initial implementation with comprehensive tests and documentation
2. Add unit tests to CI - Ensures action stays functional on every change
3. Make action configurable - Add ruling-root and sources-root parameters for different analyzers
4. Fix Java Maven coordinate handling - Support 3-part groupId:artifactId:path format with unit tests

Migration Path

For analyzer repositories (Python, Java, C#, etc.):

1. Add workflow file referencing this action
2. Set ruling-root and sources-root parameters for your analyzer
3. Configure trigger paths to match your ruling file locations

See ruling-diff-comment/example-workflow.yml for a complete reference implementation.

---

@rombirli - Please review when you have a chance!

---

Summary by Gitar

• Refactored testing infrastructure:
  • Removed obsolete workspace fallback test from test_ruling_diff.py.
• Resolved technical issues:
  • Fixed strip_project_key to correctly process Java Maven coordinates by handling 3-part formats.
  • Initialized GitHubActionIO with mocked paths in unit tests to prevent failures.
  • Fixed SonarQube issues in core logic to ensure cleaner code quality.
  • Updated FakeRulingDiffIO to include mandatory ruling_root and sources_root parameters.

_{This will update automatically on new commits.}

[hashicorp-vault-sonar-prod]

CLP-179

[rombirli]

LGTM, as long as it works

[rombirli]

Source snippets can show the wrong code

When the action can't load a source file at base_sha / head_sha (common if the sources submodule isn't initialized or the clone is shallow), it falls back to the file on disk — i.e. the current checkout (PR HEAD).

For removed issues, snippets should show code at base_sha, but they may show HEAD instead. The PR comment won't say that; it's only a warning in the Actions log.

Suggestion: drop the workspace fallback and show "(source file not found at this revision)" when git show {ref}:path fails.

ruling_diff_io.py → load_text_with_workspace_fallback()

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues
0 New dependency risks

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[gitar-bot]

Code Review ✅ Approved 7 resolved / 7 findings

Adds a reusable analyzer-agnostic GitHub Action to post ruling-diff summaries, addressing multiple issues including script injection risks, hardcoded language fences, and git-related failure handling. All previous findings have been resolved.

✅ 7 resolved

✅ Quality: Snippet code fence hardcoded to ```python for all analyzers

📄 ruling-diff-comment/ruling_diff_core_lib/comment_rendering.py:97-98
format_snippet_block in comment_rendering.py always emits a ```python fenced block (line 98), but the PR's headline feature is making the action analyzer-agnostic (Java, C#, etc.). Java/C# snippets will be rendered with Python syntax highlighting in the PR comment, which is misleading. Consider deriving the fence language from the source file extension (e.g. .java -> `java`, `.cs` -> `csharp`, `.py` -> `python`) and passing it through the Snippet, defaulting to no language when unknown.

✅ Edge Case: One non-conforming ruling path aborts the entire comment

📄 ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py:19-33 📄 ruling-diff-comment/ruling_diff_core_lib/ruling_diff_logic.py:144-151 📄 ruling-diff-comment/ruling_diff.py:82-86
build_rule_diff_for_file calls parse_ruling_path, which raises ValueError for any changed .json file that does not match the strict <project>/<repo>-<rule>.json (exactly 2 relative parts) shape. That exception propagates through build_rule_diffs -> main, where the top-level handler in ruling_diff.py logs and sys.exit(1). As a result, a single unexpected ruling file (deeper nesting, a non-ruling JSON accidentally under the ruling root, or a differently-structured layout in another analyzer) causes the whole action to fail and post no comment, even for the files that were parseable. Since the action is now used across analyzers with differing layouts, consider skipping (log a warning and continue) files that fail to parse rather than aborting the entire run.

✅ Bug: _is_missing_at_ref may swallow genuine git errors

📄 ruling-diff-comment/ruling_diff_io.py:162-166 📄 ruling-diff-comment/ruling_diff_io.py:169-179
_is_missing_at_ref treats stderr as a 'file missing at ref' signal if it contains any of the markers, including the very broad substring "path '". Many unrelated git failures include path '...' in their message, so a real error (e.g. an unexpected git failure while reading an existing file) could be silently reclassified as 'file absent', returning None and producing a misleading '(source file not found)' snippet or an incorrect new/deleted-file classification (is_new_file/is_deleted_file in RuleDiff derive from these None results). Consider tightening the match to the specific git phrases (does not exist in, exists on disk, but not in) and dropping the bare path ' marker.

✅ Security: Script injection via untrusted refs/inputs in run blocks

📄 ruling-update-and-notify/action.yml:66-80 📄 ruling-update-and-notify/action.yml:138-140 📄 ruling-update-and-notify/action.yml:190-193 📄 ruling-update-and-notify/action.yml:283
In ruling-update-and-notify/action.yml many values are interpolated directly into shell: bash run: blocks using ${{ ... }}, including attacker-controllable data. Line 79 does TARGET_REF="${{ github.head_ref || github.ref_name }}" — github.head_ref is the PR source branch name, which an external contributor fully controls and which may contain shell metacharacters. Because the expression is expanded by the Actions runner into the script before bash executes it, a branch name such as $(malicious) or ";curl … # results in arbitrary command execution in a job that has contents: write, pull-requests: write and GH_TOKEN. The same pattern applies to sync-command (line 140) and other interpolated inputs (base-ref, target-ref, pr-number, fix-pr-branch-prefix). GitHub's hardening guidance is to never interpolate untrusted ${{ }} values into run scripts; instead pass them through env: and reference "$VAR". Recommend binding all these values to env: entries and using the shell variables in the script body.

✅ Bug: git stash pop fails when sync produced no working-tree changes

📄 ruling-update-and-notify/action.yml:207-221
In the 'Create fix PR' step (action.yml ~lines 208-249) the script runs git stash push -m ... -- "${{ inputs.ruling-root }}/" and later unconditionally git stash pop. git stash push does NOT create a stash entry when there are no local changes (it prints 'No local changes to save' and exits 0). In that case the subsequent git stash pop fails with 'No stash entries found', aborting the step with a non-zero exit. This step runs whenever has-differences == 'true' and ruling-failed == 'true', but has-differences is derived from the base↔head commit diff (see below), not from the actual synced working-tree changes, so it is entirely possible to reach git stash pop with an empty stash. Guard the pop, e.g. only pop if a stash was actually created (git stash list | grep -q ruling-sync-changes) or capture the result of git stash push and branch on it.

...and 2 more resolved from earlier reviews

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}