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

[jamesarich]

Why

Review rounds were outrunning commits. PR #6499 took 8 CodeRabbit review rounds over 6 commits; #6498 took 5 over 4, #6497 5 over 3. The cause was mechanical, not qualitative: auto_incremental_review defaults to true, so every single-commit fix push reopened a full review pass, which surfaced new findings mid-stream and multiplied the loop.

The findings themselves are worth keeping. Across the last 13 non-bot PRs CodeRabbit produced ~18 findings — 1 Critical, 9 Major, 7 Minor, zero style nitpicks — and each one I checked was confirmed valid rather than argued down. They catch semantic bugs CI structurally cannot: presence-vs-zero, read-modify-write races across a suspend boundary, tests that pass for the wrong reason.

So this shortens the loop and stops the same classes recurring, rather than dialing the review down.

🛠️ Changes

Batch the reviews. auto_review.auto_incremental_review: false — one review when the PR goes ready, then on demand via @coderabbitai review once fixes are batched into a single push. #6499 would have been ~2 rounds instead of 8.

Document the recurring classes. Four classes account for most Major findings, and none were written down: presence-vs-sentinel-zero, read–decide–write across a suspend boundary, tests that assert the end state instead of the path, and Room schema bumps without a migration test. They now lead .skills/code-review/SKILL.md. Because knowledge_base.code_guidelines.filePatterns already includes .skills/**/SKILL.md, this single edit tightens both the authoring and the reviewing end.

Enforce class A mechanically. Prose alone did not hold it — an audit found 9 live sites still in the tree, including (env.temperature ?: 0f) != 0f in NodeItemCompact.kt that the NodeItem.kt fix missed, so a node reporting exactly 0 °C shows no temperature in the compact row. tools.ast-grep.rule_dirs now loads two Kotlin rules from .coderabbit/ast-grep-rules/.

Two pre_merge_checks for what an AST rule cannot see, because it is about what is absent from a diff: an unfixed sibling call site, and a test that would still pass if the fix were reverted. Both mode: warning, never error — these are judgment calls and a false positive must not block a merge.

🧹 Cleanups

• Dropped three no-op tools entries. All 57 CodeRabbit tools default to enabled: true, so listing gitleaks/shellcheck/actionlint as enabled: true did nothing; only detekt: false was load-bearing. presidio (PII), zizmor, buf, semgrep and trivy were already running.
• Pinned knowledge_base.learnings.scope: local — auto already resolves to local for public repos, but being explicit stops it shifting if visibility changes.
• No pre_merge_checks.title: CI already lints PR titles in pull-request-target.yml, and duplicating CI is what created the noise this config removed.

Reviewer notes

Scope: config and documentation only. No Kotlin changed. The nine flagged code sites — 6 float guards plus 2 RSSI display paths and 1 BLE advertisement default — are tracked as separate work, since one is a user-facing defect that deserves its own PR with regression tests.

The ast-grep rules are validated, not guessed. Run against the tree with ast-grep scan -r <rule> core/ feature/ androidApp/ desktopApp/: 6 and 3 matches respectively, with these deliberate exclusions —

Excluded	Why
relative_humidity, co2_humidity	0 %RH is not physically reachable; the guard is intentional and EnvironmentMetricsForGraphingTest.humidity_zeroFilteredOut asserts it
rx_snr	Still Float = 0f upstream with no proto presence — genuinely unfixable app-side, so flagging it would be pure noise
replyId, packet.from	0 there means "unset", not a measurement of zero

Two ast-grep gotchas are captured in the rule files: patterns containing ?: must be quoted (YAML reads : as a mapping separator), and Rust's regex has no lookahead, so field names are anchored — co2 otherwise matched co2_humidity.

Presence here is nullability, not hasX(). The models are Wire-generated, so there is no presence accessor; EnvironmentMetrics.temperature/voltage/current/soil_temperature are already Float? = null, which is why the zero-guards are discarding information the proto faithfully preserved. Note packet.rx_rssi is still Int = 0 on the pinned 2.7.26.130 — nullability arrives with 2.7.26.138 in #6498, so the signal rule currently guards app-level nullable RSSI and becomes a proto regression guard once that lands.

Testing Performed

No tests changed — spotless targets Kotlin and Kotlin-Gradle sources only, and no Kotlin or Gradle files are touched, so the baseline gradle run does not apply to this diff.

• .coderabbit.yaml validated against schema.v2.json: no unknown keys, no bad enums, nested array objects valid, custom_checks instructions within the 10,000-char limit.
• Both ast-grep rule files parse and were scanned across core/, feature/, androidApp/, desktopApp/ — match counts and exclusions as tabled above.
• Reviewed locally with the CodeRabbit CLI (coderabbit review --agent --include-untracked --base main) before pushing, which is the loop this PR sets up. It returned 2 Major findings on this very change, both applied: the signal rule's (rssi|snr)$ constraint contradicted its own note that rx_snr is unfixable, and the checklist described == null as a presence check when it tests absence.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added new automated code analysis rules to detect sentinel-value misuse in Kotlin (distinguishing valid zero readings from missing data for measurements and RSSI).
  • Strengthened pre-merge guidance with additional warning-level checks for correct absence/presence behavior and ensuring added/changed tests validate intended side effects safely.

• Chores

  • Updated automated review settings to stop incremental re-reviews on every push, refreshed enabled tool settings, and set review learnings to be local.
  • Added custom AST-grep rule configuration for the new detection patterns.

[coderabbitai]

Warning

Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4533ce4c-0641-48d4-8add-c9c9f642b165

📥 Commits

Reviewing files that changed from the base of the PR and between de77b2c and ee1bef9.

📒 Files selected for processing (3)

• .coderabbit.yaml
• .coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml
• .skills/code-review/SKILL.md

🚧 Files skipped from review as they are similar to previous changes (3)

• .coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml
• .skills/code-review/SKILL.md
• .coderabbit.yaml

---

📝 Walkthrough

Walkthrough

CodeRabbit configuration now controls incremental reviews, AST-grep rule loading, custom warning checks, and local learnings. Kotlin presence-semantics rules and recurring defect checklists were added to review guidance.

Changes

Review automation and defect guidance

Layer / File(s)	Summary
Recurring defect review guidance .skills/code-review/SKILL.md	Adds checks for nullable numeric presence semantics, atomic preference updates, path-specific test assertions, and Room migration tests.
Presence-semantics AST rules .coderabbit/ast-grep-rules/*	Adds Kotlin warning rules for sentinel-zero float expressions and RSSI presence checks.
CodeRabbit review configuration .coderabbit.yaml	Disables incremental reviews, configures AST-grep rules, adjusts tool settings, adds warning-level pre-merge checks, and sets learning scope to local.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• meshtastic/Meshtastic-Android#6016: Both update .coderabbit.yaml, including detekt configuration.
• meshtastic/Meshtastic-Android#6021: Both modify CodeRabbit automatic review behavior.
• meshtastic/Meshtastic-Android#6179: Both tune CodeRabbit review and pre-merge configuration.

Suggested labels: chore

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects reducing CodeRabbit review loops and adding recurring defect-class checks.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

.coderabbit.yaml (1)

117-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align the SNR guidance with the rx_snr exception.

The signal rule explicitly excludes rx_snr because the pinned protobuf lacks presence, but this custom check instructs reviewers to flag SNR zero defaults without that exception. Narrow the instruction to app-level fields with presence, or explicitly exclude rx_snr, to avoid recreating the false positive this PR is removing.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.coderabbit.yaml around lines 117 - 120, Update the SNR portion of the
custom review guidance in the configuration to exclude rx_snr, matching the
existing signal-rule exception, or restrict the check to app-level SNR fields
that support presence. Keep the zero-default warnings for the other listed
physical measurements and preserve the humidity exception.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml:
- Around line 3-6: Update the presence-vs-sentinel-zero-signal rule to detect
Float zero literals in nullable RSSI patterns, including takeIf comparisons and
nullable fallback expressions, while preserving existing Int coverage and RSSI
exclusions in the separate float rule. Add regression fixtures covering both Int
and Float sentinel forms.

In @.skills/code-review/SKILL.md:
- Around line 16-17: Update the “Aggregates over empty sets” guidance in
SKILL.md to describe a zero fallback as introducing comparator-dependent bias,
rather than universally outranking real values. For RSSI specifically, state the
applicable ordering and explain the resulting ranking effect; preserve the
nullable aggregate and explicit missing-value comparator guidance.

---

Nitpick comments:
In @.coderabbit.yaml:
- Around line 117-120: Update the SNR portion of the custom review guidance in
the configuration to exclude rx_snr, matching the existing signal-rule
exception, or restrict the check to app-level SNR fields that support presence.
Keep the zero-default warnings for the other listed physical measurements and
preserve the humidity exception.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 59deeeb9-b279-4ce0-a3d5-ce70fccbf786

📥 Commits

Reviewing files that changed from the base of the PR and between 3913939 and de77b2c.

📒 Files selected for processing (4)

• .coderabbit.yaml
• .coderabbit/ast-grep-rules/presence-vs-sentinel-zero-float.yml
• .coderabbit/ast-grep-rules/presence-vs-sentinel-zero-signal.yml
• .skills/code-review/SKILL.md