SONARJAVA-6828 Add AGENTS.md to provide guidance to agents making changes to rules - #5993
SONARJAVA-6828 Add AGENTS.md to provide guidance to agents making changes to rules#5993romainbrenguier wants to merge 1 commit into
Conversation
hashicorp-vault-sonar-prod
Bot
changed the title
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| The file general structure for a rule implementation and its tests is as follows: | ||
| - 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/files/checks/{RuleId}CheckSample.java` |
There was a problem hiding this comment.
💡 Quality: Test sample path convention contradicts the example
The doc states test samples should live at java-checks-test-sources/default/src/main/files/checks/{RuleId}CheckSample.java, but the AbsOnNegativeCheckTest example immediately below uses the legacy path src/test/files/checks/AbsOnNegative.java (which the Repository Structure section says should be migrated away from). An agent following this example would place new samples in the deprecated location. Align the example's onFile(...) path with the stated java-checks-test-sources convention, or note that the example reflects a legacy test.
Was this helpful? React with 👍 / 👎
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsAdds AGENTS.md to provide clear guidance for agents making changes to rules. Consider correcting the test sample path convention to match the example. 💡 Quality: Test sample path convention contradicts the example📄 AGENTS.md:196 📄 AGENTS.md:211 📄 AGENTS.md:219 📄 AGENTS.md:29 The doc states test samples should live at 🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
There was a problem hiding this comment.
I found three correctness issues in the agent guidance that can directly lead to non-compiling rules or failing tests. I also left a non-blocking suggestion to point the auto-loaded guide at the existing new-rule skill, which contains required metadata-generation and test-dependency guidance absent here.
| - **`reportIssue(tree, message)`** — Report an issue at a specific AST node | ||
| - **`reportIssue(tree, message, secondaries, cost)`** — Report with secondary locations | ||
| - **`Tree.Kind.*`** — AST node type enumeration (e.g., `Tree.Kind.METHOD`, `Tree.Kind.IF_STATEMENT`, `Tree.Kind.METHOD_INVOCATION`) | ||
| - **`TreeUtils.firstAncestorOfKind(tree, Tree.Kind.METHOD)`** — Navigate up the AST |
There was a problem hiding this comment.
TreeUtils.firstAncestorOfKind does not exist in this repository or its compiled API, so following this guidance produces a non-compiling rule. Please replace this with an actual parent-walking pattern, such as ExpressionUtils.getParentOfType(tree, Tree.Kind.METHOD), or remove the example.
|
|
||
| ```java | ||
| @Rule(key = "S2230") | ||
| public class TransactionalMethodVisibilityCheck extends IssuableSubscriptionVisitor implements DependencyVersionAware { |
There was a problem hiding this comment.
This DependencyVersionAware example does not compile: SubscriptionVisitor.nodesToVisit() is abstract and IssuableSubscriptionVisitor does not provide a default implementation. Please add the required override, mirroring TransactionalMethodVisibilityCheck:
@Override
public List<Tree.Kind> nodesToVisit() {
return Collections.singletonList(Tree.Kind.METHOD);
}| The file general structure for a rule implementation and its tests is as follows: | ||
| - 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/files/checks/{RuleId}CheckSample.java` |
There was a problem hiding this comment.
The normal-sample path is invalid. java-checks-test-sources/default/src/main/files/ contains only non-compiling; normal samples live under src/main/java/checks/ and should be loaded with TestUtils.mainCodeSourcesPath("checks/{RuleId}CheckSample.java"). This also contradicts the legacy src/test/files example below. Please update both the documented convention and the example. The same incorrect normal-sample path is duplicated in .claude/skills/new-rule/SKILL.md:75.
| @@ -0,0 +1,255 @@ | |||
| # Introduction | |||
There was a problem hiding this comment.
Non-blocking: since CLAUDE.md symlinks to this auto-loaded guide, please add a pointer to .claude/skills/new-rule/SKILL.md. That skill contains required new-rule guidance not present here, including rule-api metadata generation, output paths, and the prohibition on adding external test dependencies. Without a link, an agent following AGENTS.md alone can implement a rule without its required generated metadata.
2 participants
This is to help being more efficient when creating new rules.