Repository navigation
Conversation
hashicorp-vault-sonar-prod
Bot
changed the title
rombirli
changed the title
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| - 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: extend the reproducer with cases for those FPs, then **restart from Step 1** and repeat the full workflow (new commits per step; clean history again at the end). |
There was a problem hiding this comment.
⚠️ Bug: Precision-retry loop contradicts Steps 1–2 and the one-commit rule
When TP% is 90% or below, Step 6 (line 99) says to "restart from Step 1 and repeat the full workflow (new commits per step)". On a second pass that can't be done as written. Step 1 regenerates metadata with rule-api, which hasn't changed, so there is nothing to commit, yet line 36 requires exactly one commit per step. Step 2 says to add an empty check class and requires the test to fail, which would throw away the existing implementation. Line 36 and Step 7 (line 103) also require exactly one commit per step, but a restart creates a second set of metadata/reproducer/implementation commits. An agent following this literally will either stall or wipe its implementation. Suggested fix: make the FP loop say "extend the reproducer with the FP cases (commit), confirm the test fails, fix the implementation (commit), then go back to Step 5".
Re-enter at the reproducer/implementation steps instead of Step 1:
5. **Must be > 90%.** If 90% or below: add the FP cases to the reproducer and confirm the test fails (commit `Add false positive cases`), fix the implementation until it passes (commit `Fix false positives`), then return to Step 5 and repeat Step 6. Do not regenerate metadata or recreate the check class.
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
No if step 6 fails, the agent must introduce heuristics to reduce the noise, it must start with reproducers of these FPs (step 2, unit tests)
There was a problem hiding this comment.
Understood. The intended flow is that when TP% is 90% or below, the agent first writes reproducers for the FPs (Step 2 unit tests) and then adds heuristics to reduce the noise.
Line 99 doesn't say that yet. It says to extend the reproducer and "restart from Step 1" with the full workflow. That restart is what conflicts with the "one commit per step" rule and with the metadata step, which has nothing to redo here.
Do you want me to reword line 99 along these lines?
Must be > 90%. If 90% or below: add reproducers for the FPs as new unit-test cases (Step 2 commit), then introduce heuristics in the implementation to reduce the noise (Step 3 commit), and re-run Steps 5 and 6. Do not restart from Step 1.
If you want a different wording, tell me and I'll apply that instead.
| - **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 Bugbot 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. |
There was a problem hiding this comment.
💡 Quality: Merging the ruling PR conflicts with the rebase-only clean-history rule
Step 5 (line 84) and the existing Tests section (lines 128-129) say to merge the automatically created ruling-expectations PR into the branch. That produces a merge commit. Line 81 and Step 7 (line 103) require rebase rather than merge, and a history of exactly one commit per step. The skill never says how to reconcile the two, e.g. squash or rebase the ruling update into a single Update ruling expectations commit. An agent will either leave a merge commit, which breaks the Definition of done, or not know what to do. Suggested fix: spell out how to fold the ruling PR into one linear commit.
Integrate the ruling PR without a merge commit:
If **ruling** fails (likely for a new rule), a PR updating the expectations is created automatically on CI failure. Review it; if the new findings are legitimate, bring its changes onto the branch as a single linear `Update ruling expectations` commit (cherry-pick/squash, no merge commit), otherwise fix the implementation and the unit tests. Push and re-babysit.
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
There was a problem hiding this comment.
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
|
|
||
| **Separate Jira tickets (mandatory):** RSpec and rule implementation use **different** SONARJAVA tickets. Do **not** put a Jira key in the initial PR title or in commits. | ||
|
|
||
| 1. Open the analyzer PR with title exactly: `Implement rule SXXXX - <rule title>` (rule id and title from RSpec; no `SONARJAVA-…` prefix). | ||
| 2. In the **PR body**, include a line `Part of <EPIC-KEY>`, the **epic parent** of the RSpec ticket. Jira automation creates the implementation ticket in that epic. | ||
| 3. After the ticket exists: change its issue type from the automation default (**Maintenance**) to **Feature** (new rules are features). Do not invent an epic or ticket key. |
There was a problem hiding this comment.
That not the workflow I'm used to. RIS creates the RSPEC PR and the Jira ticket at the same time. So these instructions are not compatible.
There was a problem hiding this comment.
Gitar, fix this comment,
instead of using Part of in description, the agent must reuse the same jira ticket "SONARJAVA-..." as PR prefix (same jira ticket for implementation and for rspec)
There was a problem hiding this comment.
Updated .claude/skills/new-rule/SKILL.md. Step 4 now says RSpec and implementation share the same SONARJAVA-… ticket, and the PR title is SONARJAVA-XXXX Implement rule SXXXX - <rule title>. The Part of <EPIC-KEY> line and the "different tickets" wording are gone. I also changed the prerequisites checklist item so it asks for the RSpec ticket key itself rather than its epic parent.
There was a problem hiding this comment.
where is the commit?
There was a problem hiding this comment.
Done, PR title now reuses the same SONARJAVA key, dropped the Part of line.
There was a problem hiding this comment.
This is now redundant with the babysit step
There was a problem hiding this comment.
gitar, i agreee with this comment, fix it
There was a problem hiding this comment.
Removed the redundant "Tests" section; the ruling expectations workflow is already covered in Step 5 (Babysit until merge-ready).
…hat Step 4 requires
Co-authored-by: rombirli <56340680+rombirli@users.noreply.github.com>
Co-authored-by: rombirli <56340680+rombirli@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
This branch has not been deployed
3 participants
Ports the mandatory delivery workflow from the sonar-python-enterprise
rule-implementationskill into the sonar-javanew-ruleskill.Adds an ordered, non-negotiable workflow: pre-flight checks, one commit per step (metadata, failing reproducer, implementation), PR creation conventions, CI babysitting, TP/FP sampling analysis with a >90% precision bar, and history cleanup.
Adapted to Java conventions: rule-api metadata paths,
CheckVerifiertests with and without semantic, ruling expectations underits/ruling/src/test/resources/expected/, and SONARJAVA Jira tickets.