Skip to content

SONARJAVA-7180 Tighten new-rule skill on samples, tests and implementation - #6337

Merged
rombirli merged 3 commits into
masterfrom
rombirli/improve-new-rule-skill
Oct 13, 2026
Merged

rombirli merged 3 commits into
masterfrom
rombirli/improve-new-rule-skill

Conversation

@rombirli

@rombirli rombirli commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #6315, additions to .agents/skills/new-rule/SKILL.md.

Test sample location markers — every primary and secondary location must now be asserted with caret markers, not a bare // Noncompliant. Documents the caret syntax, the </> direction markers for secondaries, multiple locations on one caret line, and limits [[sc=…;ec=…]] to the cases where carets cannot work.

Test class structure — requires extracting the sample path and the check instance into constants so neither is repeated across test methods. The verifier chain stays inline in each test.

Method and type matching — makes MethodMatchers and the type API mandatory, forbids raw string comparisons on FQNs or method names, and prefers explicit addParametersMatcher over withAnyParameters.

Stateless checks — a check class may only declare private static final fields. Sets, maps, counters and lookup tables must be local and passed through the call graph. Carve-outs for the fields the API imposes: @RuleProperty parameters, the JavaFileScannerContext of a BaseTreeVisitor rule, and DependencyVersionAware flags. AGENTS.md is updated so its clearState() section is scoped to existing stateful checks.

@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod Bot changed the title Tighten new-rule skill on samples, tests and implementation SONARJAVA-7180 Tighten new-rule skill on samples, tests and implementation Oct 13, 2026
@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

SONARJAVA-7180

Comment thread .agents/skills/new-rule/SKILL.md
Comment thread .agents/skills/new-rule/SKILL.md
Comment thread .agents/skills/new-rule/SKILL.md
Comment thread .agents/skills/new-rule/SKILL.md
@gitar-bot

gitar-bot Bot commented Oct 13, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 4 closed / 4 findings

🟡 Medium risk · The new-rule skill mandates stateless checks and stricter matcher/test patterns, which could shape future rule implementations; impact is confined to rule-authoring guidance.

Tightens .agents/skills/new-rule/SKILL.md guidance on test sample caret location markers, test class constants, mandatory MethodMatchers and type API usage, and stateless check fields, with AGENTS.md updated to scope its clearState() section to existing stateful checks. Four review findings were resolved: misaligned Arrays.fill caret example realigned, the stateless-check rule reconciled with AGENTS.md, @RuleProperty fields added to the allowed exceptions, and the PR description updated to match the constants approach. No open issues remain.

✅ 4 closed
✅ Bug: Arrays.fill caret example is misaligned; markers miss their tokens

📄 .agents/skills/new-rule/SKILL.md:155-160
The example was copied from ArraysFillIncompatibleTypeCheckSample.java, where the code line is indented by 4 spaces. Here the indentation was removed from the code line but not from the caret line. In the snippet, fill sits at columns 8–11 and textBuffer at 13–22, but the carets start at column 13 (^^^^) and 18–27. So they mark text and uffer, 42). This is the reference example for the new mandatory caret rule, so an agent that copies it learns a wrong alignment. Fix it by restoring the 4-space indent on the code line, or by shifting the caret line 5 columns to the left.

✅ Quality: Stateless-check rule contradicts AGENTS.md per-file state guidance

📄 .agents/skills/new-rule/SKILL.md:222 📄 .agents/skills/new-rule/SKILL.md:249
The new section says a check may only declare private static final fields, and that sets, maps and counters must never be stored in a field. But AGENTS.md's "Per-File State Cleanup" section (which the skill relies on elsewhere) tells authors to keep private final Set<Symbol> seen = new HashSet<>() as a field and reset it in clearState(), and shows a code example that does this. An agent loading both documents gets opposite instructions for the same thing. Fix it by updating AGENTS.md to match, or by saying in the skill that it overrides AGENTS.md for new rules.

✅ Edge Case: Stateless rule forbids @RuleProperty parameter fields

📄 .agents/skills/new-rule/SKILL.md:222 📄 .agents/skills/new-rule/SKILL.md:249
The section says checks may only declare private static final fields, and that the only accepted non-static fields are the JavaFileScannerContext and DependencyVersionAware flags. Rule parameters use the @RuleProperty annotation, which has to go on non-static, non-final instance fields so the platform and tests can inject values. 63 checks in java-checks/src/main/java already use it. An agent that follows this text either cannot build a configurable rule or will hard-code the parameter. Fix it by adding @RuleProperty fields (set before the scan and never changed during it) to the list of allowed exceptions.

✅ Quality: PR description promises createCheckVerifier() factory; doc uses constants

📄 .agents/skills/new-rule/SKILL.md:170-184
The PR description says the skill now requires putting the verifier setup into a single createCheckVerifier() factory, following CollectionsSortCheckTest. The doc instead says to extract SAMPLE and CHECK constants and "keep the verifier chain in the tests", so CheckVerifier.newVerifier().onFile(...).withCheck(...) still appears in every test method. It seems the "Simplify test guidance" commit changed the approach without updating the description. Update the description, or the doc, so they agree.

Verification

Functional validation No results

Rules No rules evaluated

Auto-approval Not enabled

Set up

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

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

@sonarqube-next

Copy link
Copy Markdown
Contributor

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Comment thread .agents/skills/new-rule/SKILL.md
@rombirli
rombirli merged commit 454d7a9 into master Oct 13, 2026
29 checks passed
@rombirli
rombirli deleted the rombirli/improve-new-rule-skill branch October 13, 2026 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants