* test(native): add 14 suites for routing, persistence, parsing and identity gaps
Coverage audit of the native test tree; adds the highest-value untested
logic as 11 new suites and extends 3 existing ones (200 test functions).
New: test_stream_framing, test_nodedb_boot_recovery,
test_nodedb_legacy_migration, test_nodedb_v25_roundtrip,
test_nodedb_identity_hygiene, test_channel_keys, test_reliable_ack_matrix,
test_hop_start_policy, test_routing_response_hops,
test_phone_api_config_dump, test_observer.
Extended: test_rtc, test_mqtt, test_xmodem.
Two source changes the audit produced:
- StreamAPI::handleRecStream copied stream->read()'s `cInt < 0` EOF check
into the buffer-fed path, where there is no EOF sentinel; with signed
char any byte >= 0x80 (START1 is 0x94) aborted the parse. Read the byte
as uint8_t directly. Latent on develop (no callers), pinned by
test_stream_framing.
- Extract the post-decode pre-hop predicate from Router::handleReceived
into shouldSkipHandleForPostDecodeHop() (NodeDB.h) so
test_hop_start_policy drives the exact expression the router calls.
No behavior change.
test/state-manifest.tsv declares the suites that construct a NodeDB.
Full 68-suite Docker coverage run matches the pre-change baseline.
* test(native): address review - harden observer dispatch, trim comments
Review follow-ups on the coverage-audit suites:
- Observable::notifyObservers() erased list nodes while holding an iterator
into them, so an observer that unobserves itself from onNotify corrupted the
dispatch. Today the only self-detacher (PhoneAPI::onNotify ->
checkConnectionTimeout -> close -> unobserve) survives solely because it
returns -1 and aborts the chain before the increment; that unwritten contract
is now gone. Removal during a dispatch nulls the entry and the outermost
notify sweeps afterwards, which keeps self-detach, next-detach and
destruction-during-notify all safe without an allocation. Hoisting the next
iterator instead would have inverted the hazard and broken the existing
next-detach case. Two regression tests added.
- Correct the documented caller of shouldSkipHandleForPostDecodeHop: the call
is in Router::dispatchReceived, not handleReceived.
- Cast hop fields to unsigned at the %u call site in test_hop_start_policy.
- Trim the new suites' file headers to the one-or-two-line rule in AGENTS.md.
- Rename eight test functions whose names were exactly `test_` + 35 chars:
that is the shape of a Lob API key, so trufflehog flagged them as secrets
and failed the Trunk CI check.
Full 68-suite Docker coverage run matches the pre-change baseline.
* test(native): revert the observer dispatch change, keep the contract test
Backs out the notifyObservers() deferred-removal hardening from the previous
commit. It was reviewer-driven scope creep: nothing in the coverage audit
needed it, no test required it, and it changes dispatch semantics in a header
with ~76 observe() call sites on native verification alone.
The hazard it addressed is not reachable today. The only observer that
unobserves itself from onNotify is PhoneAPI (onNotify ->
checkConnectionTimeout -> close -> unobserve), and it returns -1, which aborts
the chain before the iterator is advanced past the erased node.
test_self_detach_with_abort_during_notify stays: it passes against the
unmodified dispatch and pins that the -1 is load-bearing, so a later cleanup
that "simplifies" it away goes red. The unsafe variant (self-detach returning
0) is documented in a comment rather than tested, since asserting it would be
asserting UB.
* fix(serial): recover the frame behind a stray framing marker
A byte that failed the START2 check was discarded rather than re-tested as
a possible START1, so 0x94 0x94 0xc3 ... lost the real frame: one corrupted
byte on a noisy UART silently dropped the frame behind it. Re-test the byte
in place instead.
Applied to both copies of the receive state machine. readStream() is the one
that matters in the field - it is the serial path every phone client uses -
while handleRecStream() still has no callers on develop.
Strictly widens what the parser accepts; no frame that parsed before parses
differently. test_stream_framing covers it on both receive paths, plus a run
of stray markers and a START1-then-unrelated-byte resync.
This was originally documented as a known gap in the framing suite. Fixing it
instead was NomDeTom's call on review: a passing test asserting the bad
behavior is what makes it hard to change later, and it is the same defect
shape as the signedness fix three functions away.
Also: use Throttle::deadlinePassed() in test_reliable_ack_matrix rather than
a bare millis() compare, matching the house deadline rule.
* test(native): cover the stray-marker resync on the buffer path too
The stray-marker fix went into both copies of the receive state machine, but
only test_stray_start1_before_frame_still_delivers drove both. The repeated-
marker and unrelated-byte cases drove readStream() alone, so a regression in
handleRecStream() would have gone unnoticed by two of the three.
Verified load-bearing: reverting only the handleRecStream() half of the fix
turns test_repeated_stray_start1_before_frame_still_delivers red on the new
assertion. test_start1_then_unrelated_byte_resyncs stays green under that
mutation by design - its failing byte is 0x00, where both branches reset to 0 -
and covers the other half of the ternary.
Also drops the stale header on test_stray_start1_before_frame_still_delivers,
which still described the gap as pinned-as-is after the fix landed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(native): make the hop-start truth table assert the rows it prints
test_truth_table_summary was six TEST_MESSAGE lines and no assertion, so it
reported as a case that could not fail - the anti-pattern #11517 names in its
unfinished assertion-presence lint, and the one exception to NomDeTom's "no
RUN_TEST without an assertion" pass over this PR.
The printed row and the checked expectation now come from one struct, so the
summary cannot narrate a table the predicates no longer implement. It also
covers the consequence columns the per-row tests do not assert together:
classifyHopStart, shouldDropPacketForPreHop and shouldSkipHandleForPostDecodeHop
for the same packet, with the expectations gated on MESHTASTIC_PREHOP_DROP.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Ben MeadorsandClaude Opus 5
83fd62b756