Skip to content
Merged
File renamed without changes.
191 changes: 191 additions & 0 deletions .agents/skills/new-rule/SKILL.md

Copy link
Copy Markdown
Contributor

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 .agents instead of .claude and make .claude a symbolic link instead, so that these skills are picked-up by all agents

Copy link
Copy Markdown
Contributor Author

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.

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.)
1 change: 1 addition & 0 deletions .claude
84 changes: 0 additions & 84 deletions .claude/skills/new-rule/SKILL.md

This file was deleted.

12 changes: 12 additions & 0 deletions .cursor/rules/rules-implementation.mdc
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.
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ SonarJava is a static code analyzer for Java.

The rules are implemented in **Java** using the SonarJava API. Test resources are **Java** files annotated with special comment markers.

For step-by-step guidance on implementing a new rule (including metadata generation), see `.claude/skills/new-rule/SKILL.md`.
For step-by-step guidance on implementing a new rule (including metadata generation), see `.agents/skills/new-rule/SKILL.md`.

---

Expand Down
Loading