feat(archunit-rules): NoBlanketCatch rule (W1 OTEL sweep, ADR-0013 §3) #15
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/archunit-no-blanket-catch"
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?
NoBlanketCatch ArchUnit rule — W1 OTEL sweep (ADR-0013 §3)
TL;DR: shared rule forbidding
catch (Exception|Throwable)outside the@RestControllerAdvice/@ControllerAdviceboundary, so domain failures stay typed (each service'sXxxError+OtelErrorEvents). Introduced in warn mode (~115 pre-existing blanket catches across 8 services, audit Memora #3620). Sibling toEntityVersionParity.How (the interesting part)
ArchUnit's high-level DSL can't inspect
catch-clause types. The rule usesJavaCodeUnit.getTryCatchBlocks()→TryCatchBlock.getCaughtThrowables()(archunit 1.3.0, verified viajavap) and flags any block catchingjava.lang.Exception/Throwable. The boundary is excluded by annotation FQN string (areNotAnnotatedWith("...RestControllerAdvice")), so this module needs no spring-web compile dep (only the test fixture does → test scope).Warn semantics
ArchUnit has no native warn.
NoBlanketCatchArchTest.evaluateForWarning(classes)returns the failure-report text (or""); the per-service test logs it without asserting → build stays green until catches are typed, then flip to a hard@ArchTest(mirrors the EntityVersionParity sample-in-main convention).Self-test
NoBlanketCatchSelfTest+ 3 fixtures: negative blanket-catch (flagged); positive specific-catch (IOException, allowed); positive@RestControllerAdvicecatchingException(allowed — exercises the FQN exclusion). Positive + negative packages imported separately (no cross-pollution; vacuous-pass guarded).Verification
mvn -pl archunit-rules test→ 4/4 green (2 new), BUILD SUCCESS, zero warnings (parentfailOnWarning).Footer. im2be-platform-libs • feat/archunit-no-blanket-catch → main • #332 W1 • verified mvn test.
Show previous round
hib-pr-reviewer review — PR #15 (affinity-intelligence-rework/im2be-platform-libs)
Round 1 — head
2ee144000eff, basemain, triggeropenedTL;DR: CONDITIONAL_APPROVE — 1 agreed finding (Throwable fixture gap), 2 unique-to-A verified (RuntimeException scope + meta-annotation false positives), 2 unique-to-B verified (@ControllerAdvice fixture gap + Javadoc naming collision); all minor/info, no blocking issues.
Summary
Arbitration — NoBlanketCatch ArchUnit rule (PR #15, round 1)
First run for this PR (no prior Memora history). Read
NoBlanketCatch.java,NoBlanketCatchSelfTest.java,NoBlanketCatchArchTest.java,BlanketCatchService.java, andBoundaryAdvice.javaat HEAD.Agreed (A-2 ∩ B-2): Both reviewers independently flagged missing
catch (Throwable)negative fixture — kept as one finding (used B's line 53, the@Teststart; A cited line 55 which is the@DisplayName).Unique-to-A, verified: A-1 (
RuntimeExceptionabsent fromGENERIC_THROWABLES, no explanatory comment) confirmed at line 56–57 — kept. A-3 (areNotAnnotatedWith(String)checks only directly-present annotations, not meta-annotations) confirmed at lines 78–79 — kept; this is a real ArchUnit behaviour gap that will produce false positives for any consumer using a composed@GlobalExceptionHandler-style annotation.Unique-to-B, verified: B-1 (no positive fixture exercising the
@ControllerAdviceFQN constant; drift or typo would be undetected) confirmed by reading SelfTest lines 32–50 and BoundaryAdvice.java (uses only@RestControllerAdvice) — kept. B-3 (Javadoc block-mode template names consumer classNoBlanketCatchArchTest, identical to the library class itself) confirmed at NoBlanketCatchArchTest.java lines 32–35 — kept as info.All five findings hold under verification. No blocking issues. Verdict: CONDITIONAL_APPROVE.
Memora run summary persisted (id 397).
Blast Radius
All changes are contained within the
archunit-rulessubmodule, adding new rule infrastructure (one rule class, one helper, one self-test, two fixture classes). The rule is opt-in for downstream consumers. The meta-annotation gap (finding 3) is the widest concern — it will produce false positives for any consumer service that already uses a composed boundary annotation — but this surfaces as CI noise rather than a build break during the current warn-mode sweep.BLAST_SCORE: 3/10
CI status (head
2ee144000eff)Overall: ✓ success
2 checks: 2 pending
Findings (5)
[MINOR] Agreed: no
catch (Throwable)negative fixture — Throwable branch ofGENERIC_THROWABLESuntestedarchunit-rules/src/test/java/com/aim2be/platform/archunit/NoBlanketCatchSelfTest.java:53
Both reviewers A and B independently flagged this.
GENERIC_THROWABLEScontains both"java.lang.Exception"and"java.lang.Throwable"(NoBlanketCatch.java line 57), butflagsBlanketCatchOutsideBoundary()(lines 53–70) only exercisescatch (Exception)viaBlanketCatchService. A regression or typo in the"java.lang.Throwable"entry — wrong package, wrong capitalisation — would go undetected by the self-test.Fix: add a
BlanketThrowableCatchServicefixture tofixtures/negative/withcatch (Throwable t), add anegative.contain(BlanketThrowableCatchService.class)guard, and assertresult.getFailureReport().toString().contains("Throwable")alongside the existingExceptionassertion (or add a parallel@Test).Verified:
BlanketCatchService.javaline 18 iscatch (Exception e)only; nocatch (Throwable …)anywhere in the fixture tree.[MINOR]
GENERIC_THROWABLESsilently omitsjava.lang.RuntimeException— undocumented scope boundaryarchunit-rules/src/main/java/com/aim2be/platform/archunit/NoBlanketCatch.java:57
GENERIC_THROWABLES = Set.of("java.lang.Exception", "java.lang.Throwable")does not include"java.lang.RuntimeException". A developer migratingcatch (Exception e)→catch (RuntimeException e)satisfies the rule letter but defeats the ADR-0013 §3 structured-error.eventgoal just as thoroughly, becauseRuntimeExceptionis equally a blanket catch erasing typed error signals.With no Javadoc explaining the intentional scope, every future reviewer (and every PR author who spots the bypass) will have the same question.
Fix (either):
"java.lang.RuntimeException"to the set and add aBlanketCatchRuntimeExceptionServicenegative fixture.Verified: line 57 is
Set.of("java.lang.Exception", "java.lang.Throwable").[MINOR]
areNotAnnotatedWith(String)checks only directly-present annotations — false positives for composed@GlobalExceptionHandler-style boundariesarchunit-rules/src/main/java/com/aim2be/platform/archunit/NoBlanketCatch.java:78
ArchUnit's
areNotAnnotatedWith(String)(the FQN-string overload) tests only for annotations directly present on a class. A consuming service that introduces a composed annotation:will have its
@GlobalExceptionHandlerclasses not excluded — the rule will flag them as violations every CI run, burning trust with teams you need to adopt it.Fix: replace the two
areNotAnnotatedWithpredicates with aDescribedPredicatethat usesJavaClass.isMetaAnnotatedWith:Also add a meta-annotated fixture to
passesForCompliantFixtures()so the path is tested.Verified: lines 78–79 are
areNotAnnotatedWith(REST_CONTROLLER_ADVICE)/areNotAnnotatedWith(CONTROLLER_ADVICE).[MINOR] No positive fixture exercises the
@ControllerAdviceexclusion branch — FQN constant untestedarchunit-rules/src/test/java/com/aim2be/platform/archunit/NoBlanketCatchSelfTest.java:32
passesForCompliantFixtures()(lines 32–50) pins onlyBoundaryAdvice(@RestControllerAdvice) andSpecificCatchService. The second exclusion constantCONTROLLER_ADVICE = "org.springframework.web.bind.annotation.ControllerAdvice"(NoBlanketCatch.java line 52–53) has zero fixture coverage. A typo in that FQN string, or a Spring package relocation, would never be caught by the self-test.Fix: add a
ControllerAdviceWithBlanketCatchfixture tofixtures/positive/annotated@ControllerAdviceand catchingException. Add apositive.contain(ControllerAdviceWithBlanketCatch.class)guard and assert the result has no violation, parallel to the existingBoundaryAdviceblock.Verified:
BoundaryAdvice.javauses@RestControllerAdviceonly; no@ControllerAdvicefixture exists anywhere in the fixture tree.[INFO] Block-mode Javadoc template names consumer class
NoBlanketCatchArchTest— identical to this library classarchunit-rules/src/main/java/com/aim2be/platform/archunit/NoBlanketCatchArchTest.java:33
The BLOCK-mode template in lines 30–35 of the Javadoc recommends:
A consumer who copies this verbatim creates a class named
NoBlanketCatchArchTestin their own package, shadowing the library helper in IDE auto-complete and confusing readers who expect the library'sNoBlanketCatchArchTestto be the warn-mode helper (not a consumer test class).Fix: rename the template class in the Javadoc to
NoBlanketCatchTestorNoBlanketCatchHardBlockTest, e.g.:Verified: lines 32–35 of NoBlanketCatchArchTest.java show the template class is literally
class NoBlanketCatchArchTest.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 1 • 5 findings (4m/1i) • 2026-05-30T17:14:41.449Z → 2026-05-30T17:17:07.444Z • posted-as: pr-reviewer-bot • model: auto
R1 verdict CONDITIONAL_APPROVE; 5 minor/info findings (kept=5): (1) MINOR NoBlanketCatchSelfTest — Throwable branch of GENERIC_THROWABLES untested (only catch(Exception) exercised). Added BlanketThrowableCatchService negative fixture + flagsBlanketThrowableCatch test asserting violation-specific "catches Throwable" text. (2) MINOR NoBlanketCatch:57 — RuntimeException silently omitted, undocumented scope bypass. The per-service typed XxxError domain types are unchecked (RuntimeException subclasses), so catch(RuntimeException) erases the typed signal exactly as catch(Exception) does. Added "java.lang.RuntimeException" to GENERIC_THROWABLES + BlanketRuntimeCatchService fixture + Javadoc rationale. (3) MINOR NoBlanketCatch:78 — areNotAnnotatedWith(String) matches only directly -present annotations → composed @GlobalExceptionHandler stereotypes (meta-annotated with @RestControllerAdvice) would be false-flagged every CI run. Replaced the two areNotAnnotatedWith predicates with an IS_BOUNDARY DescribedPredicate that checks isAnnotatedWith||isMetaAnnotatedWith for both FQNs. Added GlobalExceptionHandler composed-annotation fixture + ComposedBoundaryAdvice consumer (regression guard). (4) MINOR self-test — no plain @ControllerAdvice positive fixture (only @RestControllerAdvice). Added ControllerAdviceBoundary fixture. (5) INFO NoBlanketCatchArchTest — *ArchTest suffix implies a JUnit test. Defended (mirrors sibling EntityVersionParityArchTest, rule 10); added a Javadoc note clarifying it is a src/main consumer-facing helper with no @Test/@ArchTest member, never auto-discovered. Ripple (rule 63): IS_BOUNDARY collapses the two-clause that() into one predicate; a single direct-or-meta check on each Spring FQN covers both annotations and any composed stereotype (because @RestControllerAdvice is itself meta-annotated with @ControllerAdvice). README module-map line updated to the widened scope. No consumer wiring exists yet (rule gated on merge+deploy) so no downstream ripple. Verification: - mvn -pl archunit-rules test → Tests run: 6, Failures: 0, Errors: 0 (BUILD SUCCESS) - NoBlanketCatchSelfTest 2 → 4 tests; composed-boundary test passing empirically confirms the meta-annotation exclusion works - zero [WARNING]/[ERROR] lines (parent failOnWarning enforced)Show previous round
hib-pr-reviewer review — PR #15 (affinity-intelligence-rework/im2be-platform-libs)
Round 2 — head
0e4e5e74417b, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept all 3 unique-to-one findings after direct code verification; no blocking issues; 0 dropped.
Summary
Arbitration — Round 2
All three findings are unique-to-one. Each was verified with
Readagainst the HEAD checkout before a keep/drop decision.A-1 — KEEP (verified)
NoBlanketCatch.java:100shows@AnalyzeClasses(packagesOf = ApplicationMainClass.class)with noimportOptions. The canonical BLOCK-mode template inNoBlanketCatchArchTest.java:30-31correctly includesimportOptions = ImportOption.DoNotIncludeTests.class. A developer copying the Javadoc snippet from the rule class itself (the first natural reference point) would inadvertently scan test-scoped helpers, producing false positives. Confirmed present, confirmed divergent from canonical template.A-2 — KEEP (verified)
NoBlanketCatchArchTest.java:32showsclass NoBlanketCatchArchTestas the BLOCK-mode template class name — identical to the library helper class. The Javadoc note on lines 38-46 explains the naming rationale but does not remove the footgun: a consumer team that adds the BLOCK-mode class before removing their WARN-mode test file (same package) will shadow the library import and break compilation of anyevaluateForWarning()callsite. Confirmed in file at HEAD. Keeping with A'sdeferannotation (safe follow-on).B-1 — KEEP (verified)
NoBlanketCatchSelfTest.javadoes not importNoBlanketCatchArchTestat all; every evaluation path callsNoBlanketCatch.noBlanketCatchOutsideBoundary().evaluate()directly. TheevaluateForWarning()wrapper (lines 64-67 ofNoBlanketCatchArchTest.java) — specifically itsresult.hasViolation() ? result.getFailureReport().toString() : ""branch — is entirely untested. An accidental inversion of the condition would go undetected by the current suite. Confirmed no coverage at HEAD.Dropped: 0 findings. Kept: 3 (all minor). No blocking issues found.
Blast Radius
The diff adds a new shared ArchUnit rule to platform-libs that every service in the mono will onboard during the L0-T0 #7 OTEL sweep. The rule logic and predicate are self-contained, but the public
evaluateForWarning()wrapper — the primary consumer-facing API — ships with an untested branch. The two documentation gaps (missingimportOptions, shadowing template name) could silently misdirect consuming teams during the rollout.BLAST_SCORE: 4/10
Risk Indicators
NoBlanketCatchArchTest.evaluateForWarning,NoBlanketCatch.noBlanketCatchOutsideBoundaryCI status (head
0e4e5e74417b)Overall: ✓ success
2 checks: 2 pending
Findings (3)
[MINOR] Block-mode Javadoc snippet omits
importOptions = ImportOption.DoNotIncludeTests.classarchunit-rules/src/main/java/com/aim2be/platform/archunit/NoBlanketCatch.java:100
Line 100 shows:
The canonical BLOCK-mode template in
NoBlanketCatchArchTest.java:30-31correctly includes:A developer copying the snippet from the rule's own Javadoc (the most natural first reference point) would inadvertently scan test-utility and fixture classes. Because the self-test fixtures include three negative services with blanket catches, this would produce false positives when a consumer's production package happens to be on the same classpath scan. Fix: add
importOptions = ImportOption.DoNotIncludeTests.classto the snippet at line 100 to match the canonical template.[MINOR] BLOCK-mode template class name
NoBlanketCatchArchTestis identical to the library helper — shadowing riskarchunit-rules/src/main/java/com/aim2be/platform/archunit/NoBlanketCatchArchTest.java:32
The BLOCK-mode Javadoc snippet (lines 29-35) names the consumer class
NoBlanketCatchArchTest— the same identifier as the library helper being imported forevaluateForWarning()in WARN mode. The Javadoc note at lines 38-46 explains the naming rationale but does not eliminate the footgun: if a consuming service's WARN-mode test file (FooTest.java) importscom.aim2be.platform.archunit.NoBlanketCatchArchTestand that file lives in the same package as the new BLOCK-mode class, the unqualified reference resolves to the consumer-defined class, which has noevaluateForWarningmethod, breaking compilation. Teams routinely add before they remove.Suggested mitigation (does not require renaming the library class): rename the template class in the snippet to
ApplicationNoBlanketCatchArchTestor another service-namespaced identifier to make it unambiguously a new consumer artifact. The same change should be applied to the parallel snippet inNoBlanketCatch.java:101.Deferred: safe to address in a follow-on PR once the W1→W2 sweep is underway and consuming services begin adding the BLOCK-mode class.
[MINOR]
NoBlanketCatchArchTest.evaluateForWarning()— public consumer-facing API is never exercised by the self-testarchunit-rules/src/test/java/com/aim2be/platform/archunit/NoBlanketCatchSelfTest.java:98
NoBlanketCatchArchTestis not imported anywhere inNoBlanketCatchSelfTest.java. Every evaluation path in the self-test callsNoBlanketCatch.noBlanketCatchOutsideBoundary().evaluate()directly, bypassing the public wrapper entirely. The wrapper's branch logic —result.hasViolation() ? result.getFailureReport().toString() : ""(lines 64-67 ofNoBlanketCatchArchTest.java) — is the entry point documented in the README and class Javadoc for consuming services operating in warn mode. If that branch were accidentally inverted or the method returned early, the self-test would still pass.Add two assertions alongside the existing structure:
This exercises the wrapper branch that consuming services actually invoke.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 2 • 3 findings (3m) • 2026-05-30T17:29:15.614Z → 2026-05-30T17:31:32.662Z • posted-as: pr-reviewer-bot • model: auto
R2 verdict CONDITIONAL_APPROVE; 3 minor findings (kept=3, dropped=0): (1) MINOR NoBlanketCatch.java:100 — BLOCK-mode Javadoc snippet omitted importOptions=ImportOption.DoNotIncludeTests.class, diverging from the canonical template; a developer copying the rule-class snippet would scan test fixtures (which legitimately carry blanket catches) → false positives. Added importOptions to the snippet + a sentence explaining why it is required. (2) MINOR NoBlanketCatchArchTest.java:32 — the BLOCK-mode template class in the Javadoc was named NoBlanketCatchArchTest, identical to this module's helper; a consumer adding that class in the same package before removing their WARN-mode test would shadow the library import and break evaluateForWarning() callsites. Renamed the snippet class to NoBlanketCatchGateTest in both snippets (rule class + helper) + added a note on the naming requirement. (3) MINOR NoBlanketCatchSelfTest — evaluateForWarning() (the consumer-facing public API) had an untested branch (the result.hasViolation() ? report : "" wrapper). Added evaluateForWarningReportsViolationsAndEmptyWhenClean exercising both branches: negative package → non-empty report containing "catches Exception"; positive package → empty string. Ripple (rule 63): findings (1)+(2) are documentation-only; (3) is additive test coverage. No production logic changed, no consumer wiring exists yet → no downstream ripple. Verification: - mvn -pl archunit-rules test → Tests run: 7, Failures: 0, Errors: 0 (BUILD SUCCESS) - NoBlanketCatchSelfTest 4 → 5 tests - zero [WARNING]/[ERROR] lines (parent failOnWarning enforced)hib-pr-reviewer review — PR #15 (affinity-intelligence-rework/im2be-platform-libs)
Round 3 — head
70630c1cc14c, basemain, triggersynchronizeTL;DR: NO_NEW_FINDINGS — No new findings this round.
Summary
[quorum-converged] A=0 = B=0. Round 3 resolution: all 8 prior-round findings are closed.
catch (Throwable)negative fixture absentBlanketThrowableCatchServiceadded;flagsBlanketThrowableCatchpins and asserts itGENERIC_THROWABLESomittedRuntimeExceptionSet.of(…)at line 74areNotAnnotatedWithwas direct-onlyIS_BOUNDARYnow uses bothisAnnotatedWithandisMetaAnnotatedWith(lines 87-90)@ControllerAdvicepositive fixtureControllerAdviceBoundaryadded; pinned atNoBlanketCatchSelfTest.java:52-54NoBlanketCatchArchTest(shadowing)NoBlanketCatchGateTest; explicit warning added in JavadocimportOptions = DoNotIncludeTestsNoBlanketCatch.javalines 100-101 now show it in both snippetsNoBlanketCatchGateTestrename + the Javadoc paragraph inNoBlanketCatchArchTest(lines 38-42)evaluateForWarningpublic API never exercisedevaluateForWarningReportsViolationsAndEmptyWhenClean(lines 94-110) exercises both the violation and compliant branchesAll three negative fixtures (
BlanketCatchService,BlanketThrowableCatchService,BlanketRuntimeCatchService) are pinned and independently asserted. All four positive fixtures (SpecificCatchService,BoundaryAdvice,ControllerAdviceBoundary,ComposedBoundaryAdvice) are pinned. TheGlobalExceptionHandlercomposed annotation is correctly defined with@RestControllerAdviceas meta-annotation (line 19 ofGlobalExceptionHandler.java). No new actionable issues found.CI status (head
70630c1cc14c)Overall: ✓ success
2 checks: 2 pending
Findings
No new findings this round.
Quorum converged on empty findings (A + B both returned 0).
Verdict
NO_NEW_FINDINGS
hib-pr-reviewer • round 3 • 0 findings • 2026-05-30T17:33:35.675Z → 2026-05-30T17:37:23.858Z • posted-as: pr-reviewer-bot • model: auto • [bookkeeping fallback]