Skip to content

Fix the PMD violations reported by the Maven site (#122) - #126

Merged
bertysentry merged 3 commits into
MetricsHub:mainfrom
anuragpaul602-netizen:122-fix-pmd-violations
Jul 27, 2026
Merged

Fix the PMD violations reported by the Maven site (#122)#126
bertysentry merged 3 commits into
MetricsHub:mainfrom
anuragpaul602-netizen:122-fix-pmd-violations

Conversation

@anuragpaul602-netizen

@anuragpaul602-netizen anuragpaul602-netizen commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Closes #122.

The PMD report is now clean: 0 violations (PMD 7.7.0, ruleset pmd.xml), down from 89 in the issue (91 on my checkout — see the note on UnusedPrivateMethod below).

Breakdown

Count Rule Fix
68 UselessParentheses Dropped the redundant outer parentheses in the MD4 rounds and helpers, the Envelopes SOAP builders, and the boolean/bitwise returns of WsmanClient, WinRMEndpoint, NTLMMessage, Type1Message, WmiHelper and ShellFileCopy.
11 UnnecessaryModifier Removed public on interface methods and final on try-with-resources variables.
6 EmptyCatchBlock Renamed the caught exception to ignored, the name PMD accepts for a deliberately ignored exception (allowExceptionNameRegex), keeping the existing explanatory comments. Type3Message's genuinely empty block also gained a comment: a missing SHA1PRNG leaves RND_GEN null and the constructor reports it.
2 AvoidBranchingStatementAsLastInLoop NtlmCrypto.Cursor.skipUntil now loops on a new matchesAt(...) predicate instead of a labelled continue/return; WsmanClient.request accumulates into a Decoded sentinel and returns after the retry loop.
1 EmptyControlStatement HttpTransport.readChunked reads the next trailer line in the loop body rather than in the condition.
1 UnusedFormalParameter Removed the unused target parameter from the CipherGen constructor and its single call site.
2 UnusedPrivateMethod Not in the issue's list — these two only surface when PMD's type resolution is degraded, but they are in the report on my checkout, and both are false positives on used methods. Renamed the private collectItems overload to collectRows so PMD resolves the call, and annotated RemoteDigest.matches, which PMD cannot see through Optional#get(), with a justified @SuppressWarnings.

The issue left open whether the light/* NTLM/crypto ports should get a targeted suppression instead. I went with the mechanical fix: those files are already reformatted to the project's style, so they are not byte-comparable with upstream anyway, and removing the parentheses is provably semantics-preserving. Happy to switch them to a class-level suppression if you would rather keep them close to the fork.

Tests

Everything above is behavior-preserving, and the NTLM paths (MD4, NtlmCrypto, WsmanClient.request, Envelopes) are covered end to end by the existing WsmanProtocolTest against FakeWsmanServer.

The one gap was the trailer-draining path rewritten in readChunked, which had no coverage. FakeWsmanServer gained a chunked-response mode (several chunks, a chunk extension, trailer fields after the terminating chunk) and WsmanProtocolTest a test that pages a WQL enumeration over it — a client that leaves the trailers in the socket desyncs the kept-alive NTLM connection and fails on the following request. I checked the test's sensitivity by deliberately truncating the drain, which makes it fail.

Verification

  • PMD 7.7.0: 0 violations. Checkstyle: 0.
  • SpotBugs: 36, down from 37 on main (the Type3Message empty catch), nothing new. Run with plugin 4.9.8.0 because the pinned version cannot read Java 25 bytecode on my machine.
  • mvn test: 83 tests (was 82), all passing except WinRmCliTest.decodesCommandOutputUsingTheRemoteWindowsCodePage and WinRMWqlExecutorTest.testExecute, which fail identically on unmodified main here — Mockito's inline mock maker refuses to mock under JDK 25.
  • Heads-up on the "mvn verify site still green" criterion: I could not confirm it locally, because SpotBugs and the site skin both break under JDK 25 (Unsupported class file major version 69, sun.misc.Unsafe.ensureClassInitialized) before and after these changes. It needs a CI run on a supported JDK.

CipherGen is a public class, so dropping a constructor parameter is technically a signature change; it is internal NTLM plumbing with a single caller, and I noted it in CHANGELOG.md under the in-progress 2.0.0 section. No README change: nothing here is visible to users of the documented API.

pmd:check is enabled in this PR (see the comment below), so the report cannot silently regress.

The PMD report flagged 89 violations; they were report-only, so the build
stayed green while the findings piled up. The report is now clean.

- UselessParentheses (68): drop the redundant outer parentheses in the MD4
  rounds and helpers, the Envelopes SOAP builders, and the boolean/bitwise
  returns of WsmanClient, WinRMEndpoint, NTLMMessage, Type1Message,
  WmiHelper and ShellFileCopy.
- UnnecessaryModifier (11): remove `public` on interface methods and `final`
  on try-with-resources variables.
- EmptyCatchBlock (6): name the caught exception `ignored` (the name PMD
  accepts for a deliberately ignored exception); Type3Message's empty block
  also gets a comment explaining why a missing SHA1PRNG is left to the
  constructor to report.
- AvoidBranchingStatementAsLastInLoop (2): NtlmCrypto.Cursor.skipUntil loops
  on a new matchesAt() predicate instead of a labelled continue/return, and
  WsmanClient.request accumulates into a Decoded sentinel and returns after
  the retry loop.
- EmptyControlStatement (1): HttpTransport.readChunked reads the next trailer
  line in the loop body rather than in the condition.
- UnusedFormalParameter (1): remove the unused `target` parameter from the
  CipherGen constructor and its single call site.
- UnusedPrivateMethod (2, both false positives on used methods): rename the
  private collectItems overload to collectRows so PMD resolves the call, and
  annotate RemoteDigest.matches, which PMD cannot see through Optional#get().

All of it is behavior-preserving. The trailer-draining path rewritten in
readChunked had no coverage, so FakeWsmanServer gained a chunked-response
mode (several chunks, a chunk extension, trailer fields) and
WsmanProtocolTest a test that pages a WQL enumeration over it — a client
that leaves the trailers in the socket desyncs the kept-alive connection and
fails on the following request.
The PMD findings were report-only: nothing stopped a new violation from
landing. Bind pmd:check to the verify phase, with the same pmd.xml ruleset
the report uses, so `mvn verify` fails on any violation.

The plugin version is pinned to 3.26.0 — the version the parent manages for
the site report — so the gate and the report always run the same PMD (7.7.0)
rather than drifting to whatever the build resolves.
@anuragpaul602-netizen

Copy link
Copy Markdown
Contributor Author

Pushed ce08ad0: pmd:check is now enabled, so the report cannot silently regress.

  • Bound to the verify phase with the same pmd.xml ruleset the report uses, and printFailingErrors on so a failure names the rule and line.
  • The plugin version is pinned to 3.26.0 — the version the parent manages for the site report — so the gate and the report always run the same PMD (7.7.0). Left unversioned, the build resolved 3.28.0 (PMD 7.17.0) and Maven warned about the missing version; the two would then drift apart on every plugin release.

Verified both directions: mvn verify passes on the branch, and re-introducing one UselessParentheses in MD4 fails it with PMD Failure: org.metricshub.winrm.light.MD4:203 Rule:UselessParentheses.

CPD is deliberately left alone — cpd-check would fail today, and that is a separate cleanup.

@bertysentry

Copy link
Copy Markdown
Contributor

@codex please review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: 04fd407ad3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@bertysentry
bertysentry merged commit 20a8b61 into MetricsHub:main Jul 27, 2026
1 check failed
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.

Fix the 89 PMD violations reported by the Maven site

2 participants