diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 35d9616c92..1c43c99424 100644 --- a/.coderabbit.yaml +++ b/.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. diff --git a/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-float.yml b/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-float.yml new file mode 100644 index 0000000000..f11268adab --- /dev/null +++ b/.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)$" diff --git a/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml b/.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml new file mode 100644 index 0000000000..758a783ae4 --- /dev/null +++ b/.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$" diff --git a/.skills/code-review/SKILL.md b/.skills/code-review/SKILL.md index e58934add9..0be540ed14 100644 --- a/.skills/code-review/SKILL.md +++ b/.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/.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.