chore/quality tool #21

Merged

NotEvil merged 9 commits from chore/quality into develop

2026-07-01 21:06:47 +00:00

NotEvil commented

2026-07-01 21:06:37 +00:00

Owner

No description provided.

NotEvil added 9 commits

2026-07-01 21:06:37 +00:00

CHORE : add Tier A code-quality tooling (SpotBugs, PMD, CPD, JaCoCo) 1badefedfb

Report-only static analysis, run on demand with `go-task docker:quality`
inside the pinned JDK container. Gated behind -PrunQuality so it never slows
down docker:build, and ignoreFailures = true so a finding never breaks the
build — the point is the reports under build/reports/, not a gate.

- SpotBugs + FindSecBugs (bytecode + security patterns)
- PMD with a focused errorprone/multithreading/performance ruleset
  (classpath = files() to avoid OOM on the deobf-MC auxclasspath)
- CPD copy-paste detection over src/main/java
- JaCoCo coverage on the unit-test suite

Starter exclude filters drop the two dominant noise sources (SpotBugs
EI_EXPOSE_REP on Minecraft objects, PMD RedundantFieldInitializer) so the
first report is actually readable.

FIX : three correctness bugs surfaced by SpotBugs triage 57eaa45353

- FurnitureEntityRenderer.getTextureLocation returned null under the
  @MethodsReturnNonnullByDefault package; return the block atlas like the
  sibling KidnapBombRenderer (it is never actually bound — the GLB pipeline
  manages its own textures — but the contract must hold).
- KidnapperCaptiveManager.abandonCaptive dereferenced this.currentCaptive
  after untie()/free(), which reenter via PlayerBindState -> PlayerCaptivity
  -> removeCaptive() and null the field mid-method, so the following
  this.currentCaptive.free(true) NPEs on a player captive with keep-binds
  off. Hoist the captive into a local first, same defence the class already
  uses in onPutBindOn()/onDie().
- DynamicTextureManager built a ResourceLocation path from
  Math.abs(key.hashCode()); Math.abs(Integer.MIN_VALUE) is still negative,
  putting a '-' in the path. Mask the sign bit instead.

CHORE : suppress verified SpotBugs false-positives (correctness/security) b86e5f602a

Triaged every CORRECTNESS / MT_CORRECTNESS / SECURITY finding and
independently verified each verdict. What is suppressed here is genuinely
not a defect: vendored EpicFight idioms (int->double bit-smuggling,
angle-wrap float loop, pre-assigned catch var), LazyOptional.orElse(null)
capability sentinels, Void reload-listener prepare() returning null,
applyForTests test seams, @Nullable fields read only behind a guard
SpotBugs can't see through, gameplay RNG, and the client-only whitelisted
texture fetch. Every <Match> carries its justification.

Real bugs found in the same pass were fixed in code, not hidden here.
This clears CORRECTNESS / MT_CORRECTNESS / SECURITY to zero.

FIX : PMD triage — host-whitelist case compare + annotate false-positives 9aff0db3d9

One real fix: DynamicTextureManager.isHostWhitelisted compared an already-
lowercased host with allowed.toLowerCase() through equals(); switch to
equalsIgnoreCase (same result for hostnames, drops the default-locale
Turkish-I case trap).

The rest are // NOPMD annotations on findings verified false-positive in the
triage pass: client-main-thread-only lazy singletons (DynamicTextureManager,
GltfMeshRenderer), a per-UUID multiton mistaken for a singleton
(PlayerBindState), an intentional identity check on the selected GUI row
(RigPreviewScreen), all-caps self-tests (EmotionalContext, GagTalkManager),
nullable-operand equals guards (ItemOwnerTarget, KidnapperTargetSelector),
and the render-tick ConcurrentModificationException crash-guard
(RigAnimationTickHandler).

CHORE : focus SpotBugs/PMD on real-defect rules and make them blocking 87f2b0667f

Turn the quality tooling into a clean, blocking pre-MR bug gate:
- SpotBugs reports only CORRECTNESS / SECURITY / MT_CORRECTNESS (drop STYLE,
  BAD_PRACTICE, PERFORMANCE, I18N via the exclude filter); ignoreFailures=false.
- PMD trimmed to a curated bug ruleset: drop the whole performance category and
  the noisy AvoidBranchingStatementAsLastInLoop rule, and exclude 7 vendored
  EpicFight files from linting; ignoreFailures=false.
- CPD (duplication) and JaCoCo (coverage) stay informational.

Both gates are triaged to zero, so `go-task quality` is green now and any new
finding is a real regression. Verified: clean build + test + quality all green
in the pinned container.

REFACTOR : dedup 8 cross-file duplication clusters (CPD) 9e47290c7e

Extract the code CPD flagged as real cross-file duplication, matching the
codebase's existing dedup idiom (AbstractHuntMonstersGoal, AbstractRetaliateGoal,
component classes). Behavior-preserving — every per-file difference stays a hook
or parameter; verified by clean build + full test suite. Designs were produced
and adversarially verified via a workflow before applying.

- AbstractThrowableGadgetItem: shared throw logic for the SmokeBomb / Noisemaker
  / CausticFlask gadgets (projectile via an abstract factory hook)
- AbstractBondageHost: 9 byte-identical host methods for the damsel/kidnapper hosts
- AbstractMaidCampTickGoal: template-method base for MaidAssignTask/MaidInitPrisoner
- GroundScanHelper.findSafeGroundY: shared by the guard/master follow + dogwalk goals
- MeshVisibility.propagate: shared mesh-hide for the player/damsel renderers
- JsonParseHelper: shared primitive JSON getters (RoomTheme/Furniture/DataDrivenItem)
- GlbParserUtils: two shared GLB helpers (GlbParser/FurnitureGlbParser)
- GuiRenderUtil: tab-width + tab-background helpers (SectionTabBar/RegionTabBar)

CPD 47 -> 30 (the 8 targeted cross-file clusters removed). The remainder is
intra-file boilerplate + a few lower-value pairs, kept as an informational
backlog (CPD stays report-only, not a blocking gate).

CHORE : exclude vendored EpicFight files from CPD 96d3585a4e

CPD stays informational (report-only). Scope it to our own code by excluding the
three vendored EF ports it flagged (JsonAssetLoader, MoveCoordFunctions,
ClothColliderPresets) — same policy as the PMD vendored-exclude list.

CHORE : make docker:quality a zero-false-positive pre-MR bug gate 429e488d78

- Split CPD out of `docker:quality` into its own `docker:cpd` (informational,
  gated on -PrunCpd). `docker:quality` is now tests + SpotBugs + PMD only:
  GREEN = clean, RED = a real regression, with no duplication/coverage noise.
- Broaden NP_NONNULL_PARAM_VIOLATION to pattern-global: it was 100% false
  positives (Forge's over-broad @Nonnull on LazyOptional.orElse(null),
  ShapedRecipe.getResultItem(null), applyForTests seams) and would recur on new
  code. Real null derefs are still caught by NP_NULL_PARAM_DEREF /
  NP_NULL_ON_SOME_PATH. NP_NONNULL_RETURN_VIOLATION stays scoped to prepare()
  since it caught a real bug (FurnitureEntityRenderer).

FEAT : CPD baseline diff — flag only NEW copy-paste af89263572

Make CPD a reliable dev-aid instead of a static backlog report: hash each
duplication by its normalised content (stable when code moves) and diff against
a committed baseline. `go-task docker:cpd` now FAILS only on duplication that is
not already accepted — i.e. copy-paste you just introduced — while the existing
backlog never nags. `go-task docker:cpd:accept` re-freezes the baseline when a
duplication is intentional.

- Gradle tasks cpdBaselineCheck / cpdBaselineAccept (content SHA-1 diff).
- config/cpd/baseline.txt: 28 accepted duplications; vendored EF Layer +
  StateSpectrum now also excluded from CPD.
- Verified end-to-end: green on the baseline; a removed/added duplication is
  reported with its file pair and fails the run.

NotEvil merged commit 9469da1ccb into develop

2026-07-01 21:06:47 +00:00

NotEvil referenced this pull request from a commit

2026-07-01 21:06:48 +00:00

Merge pull request 'chore/quality tool' (#21) from chore/quality into develop