chore(review): cut CodeRabbit round-trips and enforce the repeat defect classes

.coderabbit.yaml

@@ -13,6 +13,10 @@ reviews:
   auto_review:
     enabled: true
     drafts: false
+    # Review once when the PR goes ready, then on demand via `@coderabbitai review`.
+    # Re-reviewing every push turned 6-commit PRs into 8 review rounds, because each
+    # fix commit reopened a full pass. Batch the fixes, push, then ask for one re-review.
+    auto_incremental_review: false
     ignore_usernames:
       - renovate
       - renovate[bot]
@@ -69,18 +73,20 @@ reviews:
     - path: feature/car/
       instructions: Run unit tests with `./gradlew :feature:car:testGoogleDebugUnitTest`, and keep Robolectric pinned to SDK 36 for this module.
 
-  # CI is the source of truth for detekt + spotless (Zero Lint Tolerance gate),
-  # so disable detekt here to avoid duplicate comments. Keep the security/CI/shell
-  # scanners CI doesn't run inline.
+  # Every CodeRabbit tool is enabled by default, so this block only ever needs to
+  # turn things OFF or configure them. detekt is off because CI owns it (Zero Lint
+  # Tolerance gate) and duplicate comments were the noise we removed. The scanners
+  # CI doesn't run — gitleaks, shellcheck, actionlint, zizmor, semgrep, trivy,
+  # presidio (PII), buf (protobuf) — are already on by default; don't re-list them.
   tools:
     detekt:
       enabled: false
-    gitleaks:
-      enabled: true
-    shellcheck:
-      enabled: true
-    actionlint:
-      enabled: true
+    # Custom AST rules mechanically enforce the recurring defect classes that prose
+    # can't. See .coderabbit/ast-grep-rules/ and .skills/code-review/SKILL.md.
+    # essential_rules stays on (default) — these are additive.
+    ast-grep:
+      rule_dirs:
+        - ".coderabbit/ast-grep-rules"
 
   # Auto-generated docstrings/tests/autofix are noisy for a repo with strict
   # human-authored KDoc and KMP-aware tests; leave finishing touches off.
@@ -91,12 +97,45 @@ reviews:
       enabled: false
 
   # No KDoc-coverage mandate in this repo; the default warning-at-80% check
-  # would nag every PR.
+  # would nag every PR. PR titles are already linted by CI
+  # (.github/workflows/pull-request-target.yml), so no title check here either.
   pre_merge_checks:
     docstrings:
       mode: "off"
+    # The two defect classes that survive review-by-prose because they are about
+    # what's ABSENT from a diff — a sibling call site left unfixed, or a test that
+    # would still pass with the fix reverted. Warning, not error: these are
+    # judgment calls and a false positive must not block a merge.
+    custom_checks:
+      - name: "Sibling call sites and presence semantics"
+        mode: "warning"
+        instructions: >-
+          When a diff changes how an absent value is represented — making a field nullable,
+          removing a zero-guard, or adding a presence check — verify EVERY call site of that
+          field was updated, not just the one the bug was reported against. Ambient temperature
+          was fixed in NodeItem.kt while its sibling NodeItemCompact.kt kept the zero-guard.
+          Name any unfixed sibling explicitly. Also flag a new field defaulting to 0 where 0 is
+          a physically reachable value on that scale (RSSI, temperature, current, voltage,
+          particulate concentration). Two exceptions, do NOT flag either: humidity, where 0 %RH
+          is unreachable and the guard is intentional and tested; and the proto `rx_snr`, which
+          has no presence upstream, so its 0f ambiguity cannot be fixed app-side. An app-level
+          SNR field that IS nullable is still in scope.
+      - name: "Tests prove the path, not the end state"
+        mode: "warning"
+        instructions: >-
+          For each added or changed test, decide whether it would still pass if the production
+          code it covers were reverted. Flag tests that seed a fake's backing store and then
+          assert the value comes back, tests that assert only a collection's size rather than
+          which items survived, and tests asserting emission ORDER under Dispatchers.Unconfined
+          (not a stable contract). A test must assert the side effect only the intended path
+          produces — a call counter, a request issued, a cache written.
 
 knowledge_base:
+  # Learnings are how a confirmed finding stops recurring on the next PR. Pin the
+  # scope to this repo: the default `auto` already resolves to `local` for public
+  # repos, but being explicit keeps it from shifting if visibility ever changes.
+  learnings:
+    scope: local
   # Feed CodeRabbit the same guidance human/AI contributors follow, including
   # the repo-specific .skills/ modules and Copilot path instructions it
   # wouldn't pick up by default.

.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-float.yml

@@ -0,0 +1,19 @@
+# Class A of "Recurring Defect Classes" in .skills/code-review/SKILL.md, enforced mechanically.
+# 0 is a real reading on these scales, so a zero-guard silently drops a genuine measurement.
+# Humidity is deliberately excluded: 0 %RH is not physically reachable, and
+# EnvironmentMetricsForGraphingTest.humidity_zeroFilteredOut asserts that guard on purpose.
+id: presence-vs-sentinel-zero-float
+language: kotlin
+severity: warning
+message: >-
+  Zero-guard on a zero-inclusive scale drops real readings. 0 is valid for temperature,
+  current, voltage and soil moisture, so `(x ?: 0f) != 0f` hides a genuine measurement and
+  conflates it with "not reported". Use a null check — `x?.let { }` — as NodeItem.kt does for
+  ambient temperature. Humidity is the one legitimate exception and is excluded from this rule.
+note: >-
+  See .skills/code-review/SKILL.md, "Recurring Defect Classes", class A.
+rule:
+  pattern: "($FIELD ?: 0f) != 0f"
+constraints:
+  FIELD:
+    regex: "\\.(temperature|soil_temperature|co2_temperature|current|voltage|soil_moisture)$"

.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml

@@ -0,0 +1,33 @@
+# Class A of "Recurring Defect Classes" in .skills/code-review/SKILL.md, enforced mechanically.
+# Presence in this repo is nullability: the models are Wire-generated, so there is no hasX()
+# accessor — an optional field is simply a nullable type. The proto `rx_rssi` only becomes `Int?`
+# in protobufs 2.7.26.138 (PR #6498, still open at time of writing); on the pinned 2.7.26.130 it
+# is `Int = 0`. So this rule currently guards APP-level nullable rssi (parameters, state, BLE
+# advertisement values) and becomes a proto regression guard once #6498 lands.
+#
+# INT LITERALS ONLY, deliberately. RSSI is an integer dBm value everywhere in this repo —
+# proto `rx_rssi: Int`, `Node.rssi: Int`, `MetricFormatter.rssi(value: Int)` — and there is no
+# Float-typed rssi declaration anywhere. Adding `?: 0f` / `takeIf { it != 0f }` variants here
+# would be dead patterns. Float sentinels on genuinely-Float metrics are the float rule's job.
+# RSSI only, deliberately NOT snr: rx_snr still has no proto presence upstream, so a 0f guard there
+# is genuinely ambiguous and unfixable app-side — flagging it would be pure noise on code nobody can
+# correct. Also excludes `replyId ?: 0` and `packet.from.takeIf { it != 0 }`, which are legitimate
+# because 0 there means "unset", not "a measurement of zero".
+id: presence-vs-sentinel-zero-signal
+language: kotlin
+severity: warning
+message: >-
+  Zero-default on a signal metric conflates "not reported" with a real reading. 0 dBm is a
+  valid RSSI (SX126x reports exactly 0, SX127x can go positive) and on a signal-strength scale
+  it renders as the STRONGEST value — so an unknown signal displays as excellent. Keep the type
+  nullable and branch on null, rendering absence explicitly rather than substituting a number.
+note: >-
+  See .skills/code-review/SKILL.md, "Recurring Defect Classes", class A. rx_snr still has no
+  proto presence upstream, so its 0f ambiguity cannot be fixed app-side — do not re-raise that.
+rule:
+  any:
+    - pattern: "$RECV.takeIf { it != 0 }"
+    - pattern: "$RECV ?: 0"
+constraints:
+  RECV:
+    regex: "(?i)rssi$"

.skills/code-review/SKILL.md

@@ -3,6 +3,50 @@
 ## Description
 Perform comprehensive code reviews for `Meshtastic-Android`, ensuring changes adhere to KMP architecture, Kotlin Multiplatform conventions, MAD standards, and CMP best practices.
 
+## Recurring Defect Classes (check these first)
+
+These four classes account for most of the Major findings raised on recent PRs, and they recur because the compiler, `detekt`, and `spotless` cannot see any of them. Check them while *writing* the code, not only while reviewing it.
+
+### A. Presence vs. sentinel zero
+**A numeric field whose absence matters must be nullable. Never let `0` stand in for "not reported".**
+
+`0` is a legitimate reading for RSSI (0 dBm), SNR, temperature, and air-quality concentration, so a `0` default silently merges "no data" with a real measurement. Both directions are bugs: an absent value gets persisted and displayed as a real one, and a genuine `0` gets discarded by a `takeIf { it != 0 }` guard.
+
+- [ ] **Accumulators and defaults:** a field collected over time defaults to `null`, not `0`. Absence checks read `== null` and presence checks `!= null` — never `== 0` for either.
+- [ ] **Aggregates over empty sets:** median/mean/min helpers return `T?` and propagate `null` for an empty input. A `0` fallback biases the result in whichever direction the comparator sorts — under the higher-is-better RSSI ordering used for ranking, an empty set's `0` outranks a real `-80 dBm`; under a plain `min` it would instead win as the smallest. Either way the missing value competes as if measured.
+- [ ] **Comparators:** sort missing values explicitly last; do not let them fall through to a numeric default.
+- [ ] **No zero-guards on real scales:** `takeIf { it != 0 }` is only valid where `0` is genuinely impossible. On any signed or zero-inclusive scale it destroys data.
+- [ ] **Proto presence:** when a proto field gains explicit presence, adopt the nullable accessor everywhere rather than keeping a `0` comparison. Fields with *no* presence cannot be fixed app-side — say so rather than faking it.
+- [ ] **Tests:** a nullable numeric needs **both** a null case and a zero-value case. One without the other does not pin the distinction.
+
+### B. Read–decide–write across a suspend boundary
+**Read the current value, decide, and write inside a single `dataStore.edit { }` block.**
+
+Reading a `StateFlow` (or a prior `suspend` getter), branching on it, and then issuing separate writes leaves a window where a concurrent change interleaves — so a guard can fire against state that no longer exists and clobber a user preference. `NotificationPrefsImpl.setGeofenceAlertOptIn` (`core/prefs/…/notification/NotificationPrefsImpl.kt`) is the reference example: it parses, mutates, caps, and writes in one `edit`.
+
+- [ ] Any "if the flag is X, set Y and Z" transition happens inside one `edit`/`updateData` lambda.
+- [ ] The decision reads the block's own `prefs`/`current` snapshot, **not** a cached `StateFlow` value from outside.
+- [ ] Multi-key transitions are one `edit` call, not several `scope.launch` writes.
+- [ ] Radio-side config mutations go through a single `editSettings { }` transaction (see `AdminController.editSettings`).
+
+### C. Tests that pass for the wrong reason
+**Assert that the intended code path produced the result — not merely that the result exists.**
+
+Seeding a fake's backing store and then asserting the value comes back passes even if the production code under test is deleted. The test must fail when the path breaks.
+
+- [ ] **Prove the path ran:** assert the side effect that only the intended path produces (a call counter incremented, a request issued, a cache written) alongside the observed value.
+- [ ] **Don't pre-seed the answer:** drive the value in through the path being tested (a gated fake response) instead of injecting it directly into the cache.
+- [ ] **Isolate the variable:** a test for one field must not let a second differing field explain the assertion.
+- [ ] **Assert survivors, not just counts:** for dedup/merge logic, assert the identities that remain, not the size.
+- [ ] **No ordering assertions under `Dispatchers.Unconfined`:** emission order is not a stable contract there — assert final state.
+
+### D. Room schema bump without a migration test
+**Every schema-version increment ships a migration test that proves existing rows survive.**
+
+- [ ] A new `core/database/schemas/<n>.json` is accompanied by an `(n-1)→n` test in `core/database/src/androidHostTest/.../*MigrationTest.kt`.
+- [ ] The test inserts rows at the old version, migrates, and asserts **row count and column values** are preserved — not merely that the migration runs.
+- [ ] Columns going nullable assert that pre-existing values are retained and that the new `NULL` state is reachable (this is class A at the storage layer).
+
 ## Code Review Checklist
 
 When reviewing code, meticulously verify the following categories. Flag any deviations and propose the canonical project pattern as a fix.