test: enforce architecture rules with Konsist (no java/android imports in commonMain, public-API allowlist, etc.)

[jamesarich]

Observation

Your AGENTS.md <rules> block lists a bunch of strict invariants — no java.*/android.*/platform APIs in commonMain, an explicit ~13-type allowlist for public declarations (MqttClient, MqttConfig, MqttMessage, PublishProperties, MqttEndpoint, QoS, ConnectionState, ReasonCode, WillConfig, MqttLogger, MqttLogLevel, RetainHandling, AuthChallenge + the named convenience extensions), MqttTransport/MqttProperties internal, zero deps beyond Ktor + coroutines + kotlinx-io, etc.

You already have a great foundation: library/src/jvmTest/kotlin/org/meshtastic/mqtt/architecture/ArchitectureTest.kt covers 5 of these via Konsist (no java./javax./android./platform. imports in commonMain, MqttTransport internal, MqttProperties internal). Nice — that already catches the most common drift class that apiCheck can't see.

This issue is just to suggest closing the remaining gap between the prose rules and the mechanical ones, and a couple of small structural tweaks.

Proposal

1. Move (or duplicate) the suite into commonTest

Konsist's Konsist.scopeFromProduction() reads source files, not compiled classes, so it works fine from commonTest on a KMP project. Putting the suite in commonTest means it runs as part of allTests on every CI matrix entry, not only jvmTest. That's mostly belt-and-braces — the rules are source-level — but it makes the architectural contract a property of the library rather than of the JVM target.

(If there's a reason it's intentionally jvmTest-only — e.g. CI cost — totally fair, ignore this part.)

2. Additional rules from AGENTS.md that translate cleanly

Each of these maps to a Konsist one-liner against scopeFromProduction() — see the Konsist snippet inspiration page for close analogues:

• Public-API allowlist. Every top-level public declaration in commonMain must be in the named set above (plus the messagesForTopic / messagesMatching / use extensions and the MqttClient(clientId) {} factory). Today this is enforced by apiCheck at the symbol level — but apiCheck only fires once a leak ships in the dump. A Konsist test can fail on the introducing PR with a clearer error message ("FooHelper was made public; either add it to the allowlist in ArchitectureTest or mark it internal").
• MqttPacket and all its subtypes are internal. The sealed interface plus all 15 packet implementations.
• All packet classes are data class (or data object). Catches accidental class / object declarations under the packet sealed hierarchy.
• All data class packet fields are val. Konsist's properties().assertTrue { it.hasValModifier } filtered to the packet hierarchy.
• (Optional) No println / System.err.println in commonMain or nonWebMain production sources — the repo already calls this out implicitly via the MqttLogger abstraction.

3. Implementation notes

• Konsist (com.lemonappdev:konsist:0.17.3) is already in your version catalog and on the jvmTest classpath, so the only build change to move to commonTest is shifting the testImplementation(libs.konsist) dependency from the jvm test source set to commonTest.
• Single suite typically runs in 1–3s; not a CI cost concern.
• For the public-API allowlist test, keeping the allowlist as a private val ALLOWED = setOf("MqttClient", …) inside the test file means contributors get a one-line diff to review when surface intentionally grows.
• Konsist works on KMP projects today and operates on the entire source tree from commonTest. Their docs cover the KMP setup directly: https://docs.konsist.lemonappdev.com.

4. Offer

Happy to send a starter PR with the four highest-value rules (public-API allowlist, MqttPacket internal, packet data class-ness, packet-field immutability) if it'd help. Just say the word and I'll target a fork of main.

[jamesarich]

Complementary suggestion: detekt ForbiddenImport for the import-ban subset

Wanted to flag a related angle since you already run detekt in CI (./gradlew detekt in ci.yml): the no java.* / android.* / javax.* / platform.* in commonMain subset of the rules above can also be expressed as a detekt ForbiddenImport rule, which fires at lint time rather than test time and gives an editor squiggle in IntelliJ. We went through this exact tradeoff recently and ended up using detekt for the import-ban category and skipping Konsist entirely — the rationale (and the trap to watch out for) is worth sharing.

What detekt ForbiddenImport looks like

In config/detekt/detekt.yml:

style:
  ForbiddenImport:
    active: true
    imports:
      - value: 'java.**'
        reason: 'commonMain must not depend on JVM stdlib'
      - value: 'javax.**'
        reason: 'commonMain must not depend on JVM stdlib'
      - value: 'android.**'
        reason: 'commonMain must not depend on Android'
      - value: 'kotlinx.coroutines.sync.Mutex'
        reason: 'engine paths use single-writer actor; no Mutex'

Scope it to commonMain only by adding an includes/excludes clause on the rule (or by gating the detekt task itself to that source set). The official rule docs cover the syntax.

Where each tool wins

Check	Better tool	Why
Banned imports (java.*, android.*, Mutex, etc.) in commonMain	detekt ForbiddenImport	Editor squiggle, fires on every save, no test infra
MqttPacket + subtypes are internal	Konsist	detekt can't reason about sealed hierarchy membership
All packet classes are data class	Konsist	structural / declaration-shape check
Packet val-only fields	Konsist	property modifier inspection
Public-API allowlist	Konsist	needs the symbol set, detekt can't allowlist

So the suggestion isn't "use detekt instead" — it's: the four structural checks I listed in the original issue genuinely need Konsist, but the import bans don't, and detekt gives faster feedback for that subset.

The trap we hit

We had Konsist in the version catalog and ./gradlew :core:konsistTest referenced from AGENTS.md for two release cycles, but never actually wrote a single rule class. It became a dangling promise that confused contributors. If you go ahead with Konsist (which makes sense for the structural rules — your existing suite is already proof), the cheapest insurance is a CI job that fails if the Konsist source set is empty, so a stale reference can't survive a refactor.

Happy to amend the offered starter PR to set up the import-ban category in detekt and the structural rules in Konsist as a side-by-side, if that's the direction you'd take.

[jamesarich]

Addressed in #60.

The Konsist suite now enforces, in addition to the original import bans: a public-API allowlist for :core, MqttPacket internality + data class/val invariants, and (as a config-time Gradle guard) that :core declares no dependency on a transport module — the "build-graph guard" this issue and #27 both wanted.

Two parts of the proposal were intentionally not followed:

• Moving the suite to commonTest isn't viable — Konsist is a JVM-only library with no KMP artifact, so it can't be a commonTest dependency for a project with native/wasm targets. It stays in :core jvmTest.
• The Mutex ForbiddenImport example in the follow-up comment was skipped — it bans kotlinx.coroutines.sync.Mutex, which this repo mandates (a copy-paste artifact from another project).

Also fixed the ArchitectureTest KDoc that falsely claimed a no-println rule; that rule was dropped rather than added, since MqttLogger.println() legitimately uses println.