Repository navigation
SONARJAVA-7146 Add new rule workflow from sonar-python to the new-rule skill #6315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
c1ea35b
Add mandatory delivery workflow to new-rule skill
rombirli d2231d4
Add cursor instructions
rombirli 26f90d1
Address gitar's comment: Non-negotiable rule forbids opening the PR t…
rombirli a253ea0
Address Romain's comment: do not reask per rule
rombirli 27c2fcd
Address Romain's comment: bugbot -> gitar
rombirli 55dc5c7
fix: clarify ruling sample allocation
gitar-bot b5fd95e
docs: remove ruling Tests section redundant with babysit step
gitar-bot d120e3e
Reuse rspec jira ticket in PR title
rombirli a06cb96
Clarify false positive retry loop
rombirli 23e6c2e
Move agent skills to .agents and symlink .claude
rombirli 475334e
Clarify commit history rules
rombirli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
File renamed without changes.
File renamed without changes.
File renamed without changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,191 @@ | ||
| --- | ||
| name: new-rule | ||
| description: >- | ||
| Implement Java analyzer rules using the mandatory delivery workflow: | ||
| metadata → failing reproducer → implementation → PR babysit → TP/FP > 90%. | ||
| Use when creating or shipping any new/changed rule. | ||
| --- | ||
|
|
||
| # New Rule Implementation Skill | ||
|
|
||
| This skill provides sonar-java-specific guidelines for implementing new rules. | ||
|
|
||
| **Non-negotiable:** every rule PR must follow the [Mandatory delivery workflow](#mandatory-delivery-workflow). Do not end before a PR that is not fully green, has open review-bot comments, messy history, or a TP ratio of 90% or below. | ||
|
|
||
| ## Mandatory delivery workflow | ||
|
|
||
| Execute **in order**. Never skip. Never combine steps into one commit. If precision fails, loop again. | ||
|
|
||
| ### Step 0 — Pre-flight (once, before any rule work) | ||
|
|
||
| **Run once** before Step 1. When implementing multiple rules in parallel (subagents), collect answers **here only** once at the beginning, then start each rule with the answers already known — do not re-ask later. | ||
|
|
||
| Verify / ask as needed: | ||
|
|
||
| - [ ] Access to the RSpec repository and a local `rule-api` jar (see [Metadata Files](#metadata-files)) | ||
| - [ ] Rule id(s) to implement | ||
| - [ ] RSpec status per rule: merged on `master` **or** branch name — look up branch/PR **only** in https://github.com/SonarSource/rspec (never in this repo) | ||
| - [ ] RSpec Jira ticket key (from the RSpec PR title/branch) — reused as the implementation ticket and as the PR title prefix (Step 4) | ||
| - [ ] Rule title (for the PR title format in Step 4) | ||
| - [ ] Any path/env overrides needed | ||
|
|
||
| Stop and resolve gaps before Step 1. | ||
|
|
||
| ### Commit rules (all steps) | ||
|
|
||
| - Aim for **one commit per step** below. While working, extra commits are fine: fixes, retries and ruling updates will be folded back into one commit per step during [Step 7](#step-7--clean-history). The rule is an invariant on the **final** history, not a constraint on how you work. | ||
| - Never squash everything into a single commit either: the final history keeps one commit per step. | ||
| - Message: single line, few words, clear. **No rule id** (it is in the branch). **No "java"** (it is the repo). **No Jira key** (never in commits). | ||
| - Examples: `Add rule metadata`, `Add failing reproducer`, `Implement rule`, `Fix review comments`, `Update ruling expectations` | ||
|
|
||
| ### Step 1 — Metadata → commit | ||
|
|
||
| 1. Generate metadata with `rule-api` (see [Metadata Files](#metadata-files)). Never hand-write `.html`/`.json`. | ||
| 2. Stage **only** rule-relevant files: `SXXXX.html`, `SXXXX.json`, and the profile files under `sonar-java-plugin/src/main/resources/profiles/`. | ||
| 3. Commit. | ||
|
|
||
| ### Step 2 — Failing reproducer → commit | ||
|
|
||
| 1. Write the test sample (`java-checks-test-sources/default/src/main/java/checks/{RuleId}CheckSample.java`) covering **every** edge case, noncompliant and compliant, with precise location markers (see the test conventions in `AGENTS.md`). | ||
| 2. Write `{RuleId}CheckTest.java`, with both a semantic and a `withoutSemantic()` test. | ||
| 3. Add an **empty** check class (`@Rule(key = "SXXXX")`, minimal `nodesToVisit`/`visitNode`). | ||
| 4. Run the test — it **must fail**. | ||
| 5. Commit (reproducer + empty check + test). | ||
|
|
||
| ### Step 3 — Implementation → commit | ||
|
|
||
| 1. Implement the check until the unit test **passes**. | ||
| 2. Commit. | ||
|
|
||
| ### Step 4 — Push & PR | ||
|
|
||
| Push the branch, named with your GitHub username convention (e.g. `<username>/short-kebab-description`), and open the PR. | ||
|
|
||
| **Single Jira ticket (mandatory):** the RSpec and the rule implementation share the **same** SONARJAVA ticket. Do **not** create a second ticket, and do **not** put the Jira key in commits. | ||
|
|
||
| 1. Open the analyzer PR with title exactly: `SONARJAVA-XXXX Implement rule SXXXX - <rule title>`, reusing the RSpec ticket key as prefix (rule id and title from RSpec). | ||
| 2. Do not invent a ticket key — take it from the RSpec PR or branch. | ||
|
|
||
| Do not stop here. | ||
|
|
||
| ### Step 5 — Babysit until merge-ready | ||
|
|
||
| Loop until **all** are true: | ||
|
|
||
| - Every CI check is green | ||
| - **No** unresolved or open review-bot comments (address or validly refute each one) | ||
| - No merge blockers | ||
|
|
||
| While babysitting: | ||
|
|
||
| - **Rebase:** automatically rebase on `master` and fix conflicts (rebase, not merge). | ||
| - **Comments:** only unresolved threads; read body and location only, not huge JSON dumps. Treat gitar review bot carefully — fix only if valid; explain when you disagree. | ||
|
|
||
| If **ruling** fails (likely for a new rule), a PR updating the expectations is created automatically on CI failure. Review it, merge it into the branch if the new findings are legitimate, otherwise fix the implementation and the unit tests. Push and re-babysit. The resulting merge commit is acceptable while babysitting; Step 7 folds the expectation update into a single `Update ruling expectations` commit. | ||
|
|
||
| ### Step 6 — TP/FP analysis (only when Step 5 is fully green) | ||
|
|
||
| 1. Collect the **new** findings from the ruling expectation diff: `its/ruling/src/test/resources/expected/<project>/java-<RULE_ID>.json`, where each entry is `"group:artifact:path/to/File.java": [line, …]`. Sources are under `its/sources/<project>/`. | ||
| 2. **Sample** new findings before classifying (do not analyze unbounded volumes): | ||
| - Let `N` be the sample size (default **100**). | ||
| - If total new findings ≤ `N` → analyze **all**. | ||
| - Else sample up to `N` across projects (water-filling, i.e. equal shares capped by each project's count): | ||
| 1. Give every project with findings its fair equal share of the remaining budget among projects that still have unsampled issues. | ||
| 2. Projects with fewer findings than their share contribute **all** of theirs; leftover budget is redistributed evenly to projects that still have unsampled issues. | ||
| 3. Repeat until the budget is exhausted. | ||
| - Examples: A=1000, B=10_000_000 → about 50 each of 100; A=1, B=100, C=3 → take 1/96/3. | ||
| 3. For **every** sampled finding: classify **TP** (correct) or **FP** (incorrect). | ||
| 4. Compute `TP% = 100 * TPs / (TPs + FPs)` on the sample. | ||
| 5. **Must be > 90%.** If 90% or below: add reproducers for the FPs as new test cases and confirm the test fails (Step 2 commit), then introduce heuristics in the implementation to reduce the noise until it passes (Step 3 commit), and re-run Steps 5 and 6. Do not restart from Step 1: metadata and the check class already exist. | ||
|
|
||
| ### Step 7 — Clean history | ||
|
|
||
| This is where the one-commit-per-step rule is enforced. Whatever the branch looks like after babysitting and precision loops, the final history must show **exactly one commit per workflow step**: | ||
|
|
||
| - one `Add rule metadata` commit | ||
| - one `Add failing reproducer` commit, including the FP cases added in Step 6 | ||
| - one `Implement rule` commit, including the heuristics added in Step 6 | ||
| - one `Update ruling expectations` commit, folding in the auto-generated ruling PR and dropping its merge commit | ||
|
|
||
| Rewrite history with interactive `rebase` (never a merge), then force-push and confirm Steps 5 and 6 still hold. | ||
|
|
||
| ### Definition of done | ||
|
|
||
| The PR is done **only** when: | ||
|
|
||
| 1. History is clean (one commit per step) | ||
| 2. All checks are green | ||
| 3. No open review-bot comments | ||
| 4. TP/FP analysis is done and **TP% > 90%** | ||
|
|
||
| ## What to Do | ||
|
|
||
| ### Metadata Files | ||
| - **DO** generate rule metadata using rule-api tool from the RSPEC repository: | ||
| - Ensure your local rspec repository is up-to-date with the rule branch | ||
| - Use rule-api jar (check Maven local repository for available versions) | ||
| - Command: `java -jar <rule-api.jar> generate -branch rule/add-RSPEC-S{RULE_ID} -rule S{RULE_ID}` | ||
| - This generates HTML and JSON files and updates the Sonar way profile automatically | ||
| - Generated files will be placed in: | ||
| - `sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S{RULE_ID}.html` | ||
| - `sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S{RULE_ID}.json` | ||
| - `sonar-java-plugin/src/main/resources/profiles/{Sonar_way|Sonar_agentic_AI}/S{RULE_ID}` | ||
|
|
||
| ### MethodMatchers | ||
|
|
||
| Use `MethodMatchers` to match method calls by type, name, and signature: | ||
|
|
||
| ```java | ||
| private static final MethodMatchers MY_MATCHER = MethodMatchers.create() | ||
| .ofTypes("java.util.List") | ||
| .names("add") | ||
| .withAnyParameters() | ||
| .build(); | ||
|
|
||
| // In visitNode: | ||
| if (MY_MATCHER.matches(methodInvocationTree)) { | ||
| reportIssue(methodInvocationTree, "Message."); | ||
| } | ||
| ``` | ||
|
|
||
| ### External Dependencies in Tests | ||
| - **DO NOT** add external library dependencies for test samples | ||
| - **DO** create mock-ups or use non-compiling tests when external libraries are needed | ||
| - **SEE** Spring examples in `java-checks-test-sources/*/src/main/files/non-compiling/checks/` | ||
|
|
||
| ## What NOT to Do | ||
|
|
||
| ### 1. Version Control | ||
| - **DO NOT** commit log files (e.g., `*.log`, build logs) | ||
| - **DO NOT** commit temporary or generated files | ||
|
|
||
| ### 2. Build Configuration | ||
| - **DO NOT** modify `pom.xml` files | ||
| - Build configuration is managed centrally | ||
| - Only modify if explicitly required by the rule implementation | ||
|
|
||
| ### 3. Architecture | ||
| - **DO NOT** make architectural changes to the codebase | ||
| - New rules should fit within the existing architecture | ||
| - Follow the patterns used by similar existing rules | ||
|
|
||
| ### 4. Test Dependencies | ||
| - **DO NOT** add real external dependencies to test projects | ||
| - Use mocks, stubs, or non-compiling tests instead | ||
| - Keep test projects lightweight and self-contained | ||
|
|
||
| ## File Structure Reference | ||
|
|
||
| ### Rule Implementation | ||
| - Rule class: `java-checks/src/main/java/org/sonar/java/checks/{RuleId}Check.java` | ||
| - Test class: `java-checks/src/test/java/org/sonar/java/checks/{RuleId}CheckTest.java` | ||
| - Test samples: `java-checks-test-sources/default/src/main/java/checks/{RuleId}CheckSample.java` | ||
|
|
||
| ### Metadata | ||
| - HTML description: `sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/{RULE_ID}.html` | ||
| - JSON metadata: `sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/{RULE_ID}.json` | ||
| - Profile: `sonar-java-plugin/src/main/resources/profiles/{Sonar_way|Sonar_agentic_AI}/S{RULE_ID}` | ||
|
|
||
| ### Non-Compiling Tests (when needed) | ||
| - Location: `java-checks-test-sources/default/src/main/files/non-compiling/checks/` | ||
| - Use for: Code that requires external libraries (Spring, AWS SDK, etc.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| .agents |
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| description: Guide for implementing Java analyzer rules (checks). Delegates to the rule-implementation Claude skill — the single source of truth. | ||
| globs: **/*checks*/**/*.java | ||
| alwaysApply: false | ||
| --- | ||
| # Rule implementation | ||
|
|
||
| Do **not** follow outdated guidance that may appear elsewhere. The single source of truth is: | ||
|
|
||
| `.agents/skills/new-rule/SKILL.md` | ||
|
|
||
| Read that skill and follow it whenever implementing, placing, or wiring a new or existing Java check. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should move the skill to
.agentsinstead of.claudeand make.claudea symbolic link instead, so that these skills are picked-up by all agentsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, skills and commands moved to .agents, .claude is now a symlink.