Skip to content

[Misc] Correct the java:S6213 denylist entry, and record the multi-line Javadoc convention - #77

Open
claude[bot] wants to merge 6 commits into
masterfrom
claude/cool-tesla-6a1n0j
Open

[Misc] Correct the java:S6213 denylist entry, and record the multi-line Javadoc convention#77
claude[bot] wants to merge 6 commits into
masterfrom
claude/cool-tesla-6a1n0j

Conversation

@claude

@claude claude Bot commented Aug 25, 2026

Copy link
Copy Markdown

What this corrects

okf/sonarqube/index.md denylists java:S6213 ("Restricted Identifiers should not be used as
Identifiers") with:

a rename of a public method or field is an API change, and the XWiki pool sits on record(…)
methods of the *QuestionRecorder classes. Not a cleanup.

That is true of the rule's method half and false of its variable half, which is most of the
pool: 16 of xwiki-commons' 18 open issues and 4 of xwiki-platform's 6 are
"Rename this variable to not match a restricted identifier" on a parameter or a local. Renaming
one changes no signature, breaks no caller, and the compiler is the whole verification — the same
partial-pool shape as the S1172 correction in #71.

The message classifies every site with no source read, so the split costs one issues/search.

Changes

  • index.md — the denylist entry now covers the method half only and points at the family file
    for the other half; S6213 added to the syntax-rules row of the rule map.
  • syntax-rules.md — a short entry for the variable half: the message-based classifier, the two
    mechanics that make it scriptable (a look-behind that spares x.record(…) and the method
    declarations; running per line so the rename does not mangle Javadoc prose, @param tags
    excepted), and the note that it belongs in its own PR rather than in a mechanical batch.
  • Version bumped 1.1.8 → 1.1.9 in the four manifests; node scripts/validate.mjs and
    claude plugin validate ./xwiki both pass.

Validated in production

The rescued half shipped as xwiki/xwiki-commons#1924 (16 issues) and has now been merged.
Without this correction the next run reads the entry, skips the rule, and leaves those 16 plus
platform's 4 untouched — which is what happened on every previous sweep.


Second commit: a comment convention enforced in review today

Added while the sweep this PR unblocks was being reviewed (xwiki/xwiki-platform#6221 and #6222).
It is not stated on the dev wiki's
CodeStyle page — I checked
before writing them — so a session has no way to comply without being told:

  • conventions/code-style.md — a Javadoc comment is always multi-line, never the one-line
    /** text */ form, and a file still using the one-line form gets converted when it is touched.
    It cost a review round today; it is cheap to state and would have prevented it.

Removed on request: the empty-catch // TODO: rule this commit originally also carried. The
OKF should hold only rules that don't yield false positives, and that one does — a catch of a
domain "not found" exception used as a signal is the design, not a defect to be TODO-ed, which is
precisely the exception that came up while reviewing xwiki/xwiki-platform#6221.


Generated by Claude Code

… an API change

* The entry rejects the whole rule because "a rename of a public method or
  field is an API change", but that is only true of the "Rename this method"
  half. "Rename this variable" is a parameter/local rename that changes no
  signature, and the message alone classifies each site.
* Documented the variable half in syntax-rules with the two mechanics that
  make it scriptable (the look-behind that spares method calls, and skipping
  comment lines so the rename doesn't mangle prose).

Co-Authored-By: Vincent Massol <vincent@massol.net>
@claude claude Bot added the llm-agent label Aug 25, 2026
@claude claude Bot assigned vmassol Aug 25, 2026
@claude claude Bot locked as resolved and limited conversation to collaborators Aug 25, 2026
* An empty catch needs a "// TODO:" asking to log a warning or to change the
  logic, not just a rationale comment: a catch that neither rethrows nor logs
  is a bug to fix later, not a decision to document.
* A Javadoc comment is always multi-line, never the one-line form, and a file
  still using the one-line form gets converted when it is touched. Neither
  rule is on the dev wiki CodeStyle page.

Co-Authored-By: Vincent Massol <vincent@massol.net>
@claude claude Bot changed the title [Misc] Correct the java:S6213 denylist entry: only the method half is an API change [Misc] Correct the java:S6213 denylist entry, and record two comment conventions Aug 25, 2026
…y block

Co-Authored-By: Vincent Massol <vincent@massol.net>
@vmassol

vmassol commented Aug 25, 2026

Copy link
Copy Markdown
Member

Remove the part about an empty catch needing a // TODO:, there can be false positives and we need only rules that don't yield false positives.

* Requested in review: the OKF should hold only rules that don't yield false
  positives, and this one does — a catch of a domain "not found" exception
  used as a signal is the design, not a defect to be TODO-ed.
* The Javadoc multi-line rule stays; it has no such exception.

Co-Authored-By: Vincent Massol <vincent@massol.net>
@claude claude Bot changed the title [Misc] Correct the java:S6213 denylist entry, and record two comment conventions [Misc] Correct the java:S6213 denylist entry, and record the multi-line Javadoc convention Aug 25, 2026
@claude claude Bot unlocked this conversation Aug 25, 2026
@claude

claude Bot commented Aug 25, 2026

Copy link
Copy Markdown
Author

Removed — the section is gone from conventions/code-comments.md, along with the pointer to it that
code-style.md carried. The multi-line Javadoc rule stays; it has no exception I know of.

You're right about the false positive, and this PR is where it showed up: the rule as I stated it
would have told a future session to put a // TODO: on the 12 catch (XWikiRightNotFoundException)
blocks of XWikiRightServiceImpl — which is exactly what you had me undo on #6221, since there the
exception is a domain signal and catching it is the design.

I've kept the nuanced version in my own routine notes (where it is conditional and carries that
counter-example) rather than as an OKF rule, so the next Sonar run still knows what the review asked
for on #6221 without the plugin asserting something that isn't always true.

Title and description updated to match; node scripts/validate.mjs and claude plugin validate ./xwiki still pass, version bump unchanged at 1.1.9.


Generated by Claude Code

@claude claude Bot locked as resolved and limited conversation to collaborators Aug 25, 2026
vmassol and others added 2 commits September 1, 2026 16:27
* Five manifests carry one version, so every concurrent pull request conflicted with every other one
  on those same five lines, over something that was never part of either change. The release is now
  cut on master after the merge by scripts/release.mjs, and scripts/validate.mjs fails a branch that
  touches a version field.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* The branch was failing `validate` because it ran its own stale copy of
  scripts/validate.mjs, which still required a bump above master's tip; master
  now requires the opposite — a pull request must not touch the version at
  all, since the release is cut on master.
* No version bump here: the manifests take master's 1.5.1 unchanged.

Co-Authored-By: Vincent Massol <vincent@massol.net>
@claude claude Bot unlocked this conversation Sep 1, 2026
@claude

claude Bot commented Sep 1, 2026

Copy link
Copy Markdown
Author

validate was failing for a reason that is now fixed by a master merge, not by a version change.

The check ran the copy of scripts/validate.mjs on this branch, which still enforced the old rule
("bump above master's tip") — so after you dropped the bump it reported 1.1.8 is not greater than 1.5.1. Meanwhile master had inverted that rule: a pull request must not touch the version at all,
since the release is cut on master. The branch content was already right under the new policy; only
its validator was stale.

So I merged current master (8b7f693) into the branch. The manifests now carry master's 1.5.1
unchanged, the merge base advances to master's tip, and both checks pass locally:

validate.mjs: OK (22 skills, Claude + Kimi + opencode versions in sync at 1.5.1, OKF map complete)
claude plugin validate ./xwiki: Validation passed

The merge was clean — index.md and syntax-rules.md auto-merged with the week of changes on
master. What this PR contributes is unchanged and is now exactly three files:

  • sonarqube/index.md — the S6213 denylist entry narrowed to the method half, and S6213 added
    to the syntax-rules row.
  • sonarqube/syntax-rules.md — the entry for the variable half.
  • conventions/code-style.md — the multi-line Javadoc rule.

Thanks for the merge-base change, by the way: the stale-bump conflict it removes is the single most
common reason these OKF PRs got closed unreviewed.


Generated by Claude Code

@claude claude Bot locked as resolved and limited conversation to collaborators Sep 1, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant