feat(archunit-rules): combined NoBlanketCatch evaluate() single-scan API + fix javadoc -1.1 warning (#332 W1) #17
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/archunit-noblanketcatch-combined-eval"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
#332 W1 follow-up —
archunit-rulestwo small improvementsProject im2be-platform-libs • Module archunit-rules (+ parent pom) • Base
main• #332 W1 follow-up • 2026-05-30TL;DR
CONDITIONAL_APPROVE— additive combinedevaluate()single-scan API (resolves the long-standing "double evaluation" minor flagged every reviewer round) + pins maven-javadoc-plugin to 3.11.2 to drop the spurious-1.1 switchwarning (a 3.11.3 regression). Zero warnings; no API broken; no version bump.Summary
Two independent improvements, one PR, two commits. (1) Adds
NoBlanketCatchArchTest.evaluate(JavaClasses) → WarnResult(count, report)that scans the rule ONCE and returns both the ratchet count and the warn-mode report — the clean fix every PR-reviewer round suggested for the double evaluation in warn-mode tests. The legacyevaluateForWarning+violationCountare kept unchanged (the 7 merged services depend on them; purely additive). (2) Fixes the pre-existing[WARNING] Javadoc 1.4+ doesn't support the -1.1 switch anymoreemitted onmvn install(javadoc:jar is bound to package). Reviewed commits:1b8bbf0(API) +7e48163(javadoc pin).Findings (addressed)
NoBlanketCatchArchTest.javarecord WarnResult(int count, String report)+evaluate()single-scan method; Javadoc steers new callers to it; legacy methods retained for back-compatpom.xml(parent, maven-javadoc-plugin)-1.1 switchwarning breaks the zero-warning gate (rule 62)3.11.2— last release before the 3.11.3 regression; no suppression knob exists in 3.11.3Task-1 self-test:
NoBlanketCatchSelfTest.evaluateReturnsCombinedCountAndReport— negative package →count==3+ report contains"catches Exception"; positive package →count==0+ reportisEmpty(). Both branches pinned via.contain(...)to guard a vacuous pass.Task-2 root cause (rule 61, doc-grounded): maven-javadoc-plugin 3.11.3 (resolved from the spring-boot-starter-parent 3.5.14 BOM) unconditionally logs the
-1.1 switchwarning during argument construction even thoughold=falseand<release>17>is set — confirmed a regression (absent in 3.11.2) per the Maven users mailing-list thread. No config knob suppresses it in 3.11.3 (tested<release>,<detectJavaApiLink>false>,<detectOfflineLinks>false>,<javadocVersion>— none work); the plugin main-branch fix is unreleased. 3.11.2 is the last clean, behaviour-compatible release. The pin carries a remove-once-fixed note.Verification
Pre-existing unrelated log lines in OTHER modules (ApicurioIT skipped; surefire fork-kill in redis-outbox-backend IT) are present on
mainand out of scope.Verdict
CONDITIONAL_APPROVE— additive + behaviour-preserving.evaluateForWarning+violationCountunchanged.Footer
im2be-platform-libs • archunit-rules + parent pom • #332 W1 follow-up • resolved=2 / new=0 / carried=0 • commits
1b8bbf0+7e48163• 2026-05-30Show previous round
hib-pr-reviewer review — PR #17 (affinity-intelligence-rework/im2be-platform-libs)
Round 1 — head
7e481630499d, basemain, triggeropenedTL;DR: CONDITIONAL_APPROVE — A=1 kept (verified double-getFailureReport call), B=2 kept (class-level Javadoc mismatch at L19 + WarnResult missing invariant guards); no blocking issues.
Summary
Arbitration — Round 1
No prior run history for PR #17 (first arbitration). No reusable submodule patterns in Memora. All three findings verified against HEAD at
/var/lib/pr-reviewer/repos/affinity-intelligence-rework/im2be-platform-libs/1300.A-1 (unique-to-A, lines 113–115): CONFIRMED. The new
evaluate()method at lines 107–116 makes two separate calls toresult.getFailureReport()— once for.getDetails().size()(line 114) and once for.toString()(line 115). The companion methodsevaluateForWarning(line 131) andviolationCount(line 150) each call it once. A's analysis is correct: two allocations per call and no guarantee of identity. KEPT.B-1 (unique-to-B, cited line 107): CONFIRMED with corrected line. The real issue is at line 19 of the class-level Javadoc, which still reads
NoBlanketCatchArchTest.evaluateForWarning(classes)despiteevaluate()(line 107) being documented as the preferred single-scan entry point. B's cited line 107 is whereevaluate()opens — the actual offending text is at line 19 — but the finding is grounded and the concern is real. KEPT with corrected line citation.B-2 (unique-to-B, line 68): CONFIRMED.
public record WarnResult(int count, String report) { }has no compact constructor. As a public library type, external callers can construct it withcount < 0orreport == null; the latter causes NPE on theresult.report().isEmpty()pattern shown in the method Javadoc. KEPT.All findings are minor. Run summary persisted to Memora (id 421).
Blast Radius
The diff touches a single archunit-rules library module plus a pom.xml version pin. The changed surface is the public API of
NoBlanketCatchArchTest— newevaluate()method andWarnResultrecord — which will be consumed downstream but is additive-only (legacy methods retained). Impact is limited to library consumers that adopt the new entry point.BLAST_SCORE: 2/10
CI status (head
7e481630499d)Overall: ✓ success
2 checks: 2 pending
Findings (2)
[MINOR]
evaluate()callsresult.getFailureReport()twice — two allocations, no shared referencearchunit-rules/src/main/java/com/aim2be/platform/archunit/NoBlanketCatchArchTest.java:113
In the violation branch of
evaluate()(lines 113–115),result.getFailureReport()is invoked twice:In ArchUnit 1.3.0,
EvaluationResult.getFailureReport()constructs a newFailureReportfrom the underlyingConditionEventson every call — two objects, two traversals. More critically, no guarantee exists that both calls return an equivalent snapshot; if ArchUnit ever lazily re-evaluates,countandreportin the returnedWarnResultcould silently diverge. The sibling methodsevaluateForWarning(line 131) andviolationCount(line 150) each call it once. Apply the same discipline:This eliminates the double allocation and makes
countandreportprovably consistent (sameFailureReportinstance).[MINOR] Public
WarnResultrecord lacks compact-constructor invariant enforcementarchunit-rules/src/main/java/com/aim2be/platform/archunit/NoBlanketCatchArchTest.java:68
WarnResultispublicwith no guards:countcan be negative andreportcan be null, both contradicting the Javadoc contract. Callers following the method Javadoc's pattern (result.report().isEmpty()) will throw NPE ifreportis null. Add a compact constructor:This is cheap, idiomatic for Java 17+ public-library records, and makes the contract machine-checked rather than documentation-only.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 1 • 2 findings (2m) • 2026-05-30T21:53:50.789Z → 2026-05-30T21:55:11.763Z • posted-as: pr-reviewer-bot • model: auto
R1 verdict CONDITIONAL_APPROVE; 3 minor findings (kept): (1) MINOR evaluate() called result.getFailureReport() twice (two allocations; count + report could diverge if ArchUnit ever re-evaluates). Now retrieved once into a local FailureReport → fewer allocations + provably-consistent snapshot. (2) MINOR WarnResult record had no invariant enforcement. Added a compact constructor: count >= 0 (IllegalArgumentException) + report non-null (NPE). Matches its Javadoc. (3) MINOR class-level Javadoc WARN-mode snippet still showed the old evaluateForWarning pattern; updated to the preferred single-scan evaluate() (consistent with the method Javadoc). Added NoBlanketCatchSelfTest.warnResultGuardsInvariants (rule 6 — the new guard). Verification: - mvn -pl archunit-rules test → Tests run: 10, Failures: 0, Errors: 0 (BUILD SUCCESS, zero warnings); NoBlanketCatchSelfTest 7 → 8.hib-pr-reviewer review — PR #17 (affinity-intelligence-rework/im2be-platform-libs)
Round 2 — head
fb61ab3a6da9, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 unique-to-B findings (both verified via Read): Javadoc misattribution in test (minor) and fully-qualified Objects.requireNonNull without import (info); both R1 blocking findings confirmed resolved.
Summary
Reconciliation: A=0 findings, B=2 findings (both unique-to-B, both verified).
No agreed findings (A was empty). Both B findings were verified via
Readagainst HEAD:Javadoc misattribution (minor, kept):
NoBlanketCatchSelfTest.javalines 175-178 contain a/** Imports the negative package, pins the three flagged fixtures… */block that was originally Javadoc forevaluateNegative(). After the PR insertedwarnResultGuardsInvariants()immediately after that comment, the Javadoc now precedeswarnResultGuardsInvariants()— Java associates a/** */block with the immediately following declaration.evaluateNegative()(line 190) now has no Javadoc. Confirmed by Read at lines 175-190.Fully-qualified
Objects.requireNonNullwithout import (info, kept):NoBlanketCatchArchTest.javaline 76 usesjava.util.Objects.requireNonNull(report, "report")while the file's import block (lines 1-5) only imports threecom.tngtech.archunitclasses andFailureReport— noimport java.util.Objects;. Valid Java but inconsistent with the file's single-type-import style. Confirmed by Read.Prior R1 findings both resolved per both reviewers; not re-raised. Memora persisted as new memory ID 422 (create-new path,
review_count=2).Blast Radius
The diff touches 4 files but the substantive changes are confined to
archunit-rules— an internal utility library. The newWarnResultrecord andevaluate()method are exported public API consumed by downstream service test suites, so callers will need to migrate from the dual-accessor pattern to use the single-scan entry point. README and pom.xml changes are documentation/dependency-only.BLAST_SCORE: 4/10
CI status (head
fb61ab3a6da9)Overall: ✓ success
2 checks: 2 pending
Findings (2)
[MINOR] Pre-existing Javadoc for
evaluateNegative()is now misattributed towarnResultGuardsInvariants()archunit-rules/src/test/java/com/aim2be/platform/archunit/NoBlanketCatchSelfTest.java:179
The
/** Imports the negative package, pins the three flagged fixtures so the evaluation can never silently skip one, and returns a violating result. */block (lines 175-178) was written as Javadoc for theevaluateNegative()private helper. The PR insertedwarnResultGuardsInvariants()(lines 179-188) immediately after that comment block, so Java now associates the Javadoc withwarnResultGuardsInvariants()instead.evaluateNegative()(line 190) is left without any Javadoc.Concrete fix — move
warnResultGuardsInvariants(lines 179-188) to before line 175 so the Javadoc immediately precedesevaluateNegative()again:Impact is limited to test sources (private method, no generated Javadoc published), but the mismatch is a readability hazard for future contributors.
[INFO]
java.util.Objects.requireNonNullused with fully-qualified name — no importarchunit-rules/src/main/java/com/aim2be/platform/archunit/NoBlanketCatchArchTest.java:76
Line 76 uses
java.util.Objects.requireNonNull(report, "report")in the compact constructor ofWarnResult. The file's import block (lines 1-5) imports only threecom.tngtech.archunitclasses; there is noimport java.util.Objects;. The code compiles and behaves correctly, but it is inconsistent with the single-type-import convention used for every other dependency in the file.Concrete fix — add
import java.util.Objects;after line 5 and shorten the call:No behaviour change; purely style alignment.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 2 • 2 findings (1m/1i) • 2026-05-30T22:01:39.494Z → 2026-05-30T22:03:10.842Z • posted-as: pr-reviewer-bot • model: auto