Conversation
…ties Persist client /get have-ids, abort mid-transfer cancels, keep Host enabled for local settle, demote absurd hop ghosts, and skip known RNCP control LXMF.
Add a sidecar-persisted Auto blacklist with Network UI Ignore/Allow controls so poorly behaved nodes are skipped without blocking Manual.
Silent Host /get no longer clears a newer user Sync latch; local-prop and Auto soft-defer paths stop reporting false success; stamp validation, atomic have-ids, locale/docs, and behavioral tests cover the audit findings.
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR adds persistent Auto-mode propagation blacklists, blacklist-aware target selection, stricter hop and path handling, cancellable client retrieval, stale-sync protection, explicit local deposit outcomes, RNCP retry controls, API updates, UI controls, and supporting documentation and validation. ChangesReticulum propagation and runtime updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UI
participant PropagationStore
participant SidecarAPI
participant PropagationBridge
participant ReticulumNode
UI->>PropagationStore: select target and start sync
PropagationStore->>SidecarAPI: POST /api/v1/propagation/sync
SidecarAPI->>PropagationBridge: start client /get
PropagationBridge->>ReticulumNode: validate path and retrieve messages
ReticulumNode-->>PropagationBridge: progress or error
PropagationBridge-->>PropagationStore: completion, deferred, or failure
PropagationStore-->>UI: update sync state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
…blacklist CI coverage failed because the reconnect-hardening contract still expected the pre-blacklist call shape for shouldApplyLinkDeliveryTimeoutFailureBridge.
Build & Test failed solely on scripts/install-actionlint.mjs network fetch; no product code change. Co-authored-by: Joey Stanford <rinchen@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
reticulum-sidecar/src/stack/propagation_bridge.rs (1)
562-592: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not mark undecodable blobs as "have" — this permanently loses them.
poll_client_download'sCompletebranch computestidsfrom every rawblobinblobs(line 568-571) and immediately callsclient.add_local_messageand, below,merge_persist_client_have_idsfor all of them — beforemessagesis built from the decode-filtered subset (line 583-586). A blob that fails to decode (corrupted transfer, unexpected content) is still marked "have" and persisted.Because have-ids gate re-download across every PN ("Auto cascading across PNs reports haves instead of re-downloading"), an undecodable blob can never be retried from this PN or any other PN once its transient ID is recorded here. Since it was also never decoded into a message, it never reaches Chat. The message is lost silently and permanently.
drain_local_inbox(lines 671-680) handles the equivalent local case correctly: it derives have/purge IDs only from the successfully decodedmessages, with an explicit comment that undecodable blobs must stay for a later retry.poll_client_downloadshould follow the same ordering.As per path instructions (AGENTS.md): "preserve state on failures and log meaningful failure points" and "ensure competing or stale operations cannot overwrite newer state."
🛠️ Proposed fix: derive have-ids from decoded messages, not raw blobs
PropagationClientState::Complete => { let listed = client.available_messages().len(); let downloaded = client.received_count(); let blobs = client.take_received_messages(); - // Remember retrieved tids as haves (survives acknowledge/cleanup) - // so the next `/get` (same or other PN) purges instead of re-serving. - let tids: Vec<PropagationTransientId> = blobs - .iter() - .map(|blob| LxMessage::compute_propagation_transient_id(blob)) - .collect(); + let messages = blobs + .iter() + .filter_map(|blob| decode_downloaded_propagated_blob(&self.identity, blob)) + .collect::<Vec<_>>(); + // Only remember successfully-decoded tids as haves. An undecodable + // blob must stay re-fetchable (from this or another PN) — never + // decoded, never delivered, so it must not be silently dropped. + let tids: Vec<PropagationTransientId> = + messages.iter().filter_map(|msg| msg.transient_id).collect(); for tid in &tids { client.add_local_message(*tid); } let have_added = tids.len(); // Consume the terminal snapshot → Idle so the next // start_client_download can proceed without a cancel first. let _ = client.acknowledge_transfer(); drop(client); if have_added > 0 { merge_persist_client_have_ids(&self.client_have_path, &tids); } - let messages = blobs - .iter() - .filter_map(|blob| decode_downloaded_propagated_blob(&self.identity, blob)) - .collect::<Vec<_>>(); ClientDownloadPoll::Complete { messages, listed, downloaded, } }🤖 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 `@reticulum-sidecar/src/stack/propagation_bridge.rs` around lines 562 - 592, Update the PropagationClientState::Complete branch in poll_client_download so have IDs are derived only from successfully decoded messages, matching drain_local_inbox behavior. Decode blobs before calling client.add_local_message or merge_persist_client_have_ids, retain undecodable blobs for retry, and preserve the existing listed/downloaded reporting and completion flow.Source: Path instructions
src/renderer/lib/reticulum/reticulumPropagationMode.ts (1)
159-174: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNormalize implausible configured-node hop counts before sorting.
node.hopsvalues above 32 still sort before unknown hops in this function. The sidecar maps these values to unknown withhops_rank. Auto Sync can therefore select a different configured node than outbound Auto deposit.Map non-finite or out-of-range configured hops to
Number.POSITIVE_INFINITYbeforesortByHopsThenKey. Add coverage for a configured node with hops above 32.Proposed fix
const rows: { id: string; hops: number; sortKey: string }[] = []; for (const node of nodes) { if (node.id === 'local-prop' || !node.enabled) continue; if (isPropagationHashAutoBlacklisted(node.destination_hash, autoBlacklist)) continue; + const hops = node.hops ?? Number.POSITIVE_INFINITY; rows.push({ id: node.id, - hops: node.hops ?? Number.POSITIVE_INFINITY, + hops: hasFinitePropagationHops(hops) ? hops : Number.POSITIVE_INFINITY, sortKey: node.name, }); }🤖 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 `@src/renderer/lib/reticulum/reticulumPropagationMode.ts` around lines 159 - 174, Update listConfiguredRemotePropagationIds to normalize each node’s hops before sorting: convert non-finite values and values outside the valid 0–32 range to Number.POSITIVE_INFINITY, while preserving valid hop counts. Add coverage for a configured node with hops above 32 and verify it sorts as an unknown-hop node.
🟡 Other comments (1)
reticulum-sidecar/src/stack/persistence.rs-425-436 (1)
425-436: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTighten Auto-blacklist hash validation to reject, not strip, invalid characters.
normalize_propagation_auto_blacklist_hashfilters out every character that is not an ASCII hex digit, then checks the remaining length. A hash string with stray characters (a typo, a copy-paste artifact, anything other than the expected 32 hex digits) can be silently reduced to a 32-character value that passes validation, instead of being rejected.Other hash-normalization helpers in this file reject malformed input instead of stripping it:
add_propagation_noderequireshash.chars().all(is_ascii_hexdigit)aftertrim().to_lowercase(), and the RRC helpers strip only the known:separator withreplace(':', ""). This helper should follow the same pattern so a corrupted destination hash cannot silently pass as valid and blacklist (or un-blacklist) the wrong node.This helper backs both
add_propagation_auto_blacklist/remove_propagation_auto_blacklistand the deserialization path (line 992), so the leniency affects every entry point for this field.As per path instructions (AGENTS.md): "Validate external hashes and inputs, use standardized error/result handling."
🛡️ Proposed fix: reject non-hex input instead of stripping it
pub fn normalize_propagation_auto_blacklist_hash(raw: &str) -> Result<String, String> { - let clean: String = raw - .chars() - .filter(char::is_ascii_hexdigit) - .collect::<String>() - .to_lowercase(); - if clean.len() != 32 { + let clean = raw.trim().to_lowercase(); + if clean.len() != 32 || !clean.chars().all(|c| c.is_ascii_hexdigit()) { return Err("destination_hash must be 32 hex characters".into()); } Ok(clean) }🤖 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 `@reticulum-sidecar/src/stack/persistence.rs` around lines 425 - 436, Update normalize_propagation_auto_blacklist_hash to trim and lowercase the input, then reject it unless all characters are ASCII hexadecimal and the normalized value is exactly 32 characters; do not filter or strip arbitrary characters. Preserve the existing Result error behavior so add_propagation_auto_blacklist, remove_propagation_auto_blacklist, and deserialization consistently reject malformed hashes.Source: Path instructions
🧹 Nitpick comments (2)
reticulum-sidecar/src/stack/persistence.rs (1)
1181-1202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for the 256-entry cap.
The existing test covers normalization, idempotent re-add, and invalid-hash rejection, but not the
"propagation Auto blacklist is full"error path whenPROPAGATION_AUTO_BLACKLIST_CAPis reached. A short test filling the list to the cap and asserting the next add fails would cover that boundary.🤖 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 `@reticulum-sidecar/src/stack/persistence.rs` around lines 1181 - 1202, Extend propagation_auto_blacklist_add_remove_normalizes_hash or add a focused test for add_propagation_auto_blacklist that inserts PROPAGATION_AUTO_BLACKLIST_CAP distinct valid hashes, verifies the list reaches the cap, and asserts the next valid add returns an error for the full-list boundary.reticulum-sidecar/src/stack/propagation_bridge.rs (1)
737-778: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSerialze persisted have-ids writes across concurrent drains.
merge_persist_client_have_idsreads the current have-ids file, merges IDs, and writes back. It can run frompoll_client_downloadand fromdrain_local_inboxinside a spawned drain task, while the client-download driver is still active. If both paths start with the same on-disk state, the later write can drop the earlier new IDs. Keep aMutex<()>forclient_have_pathand hold it across the load-merge-atomic-write sequence.🤖 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 `@reticulum-sidecar/src/stack/propagation_bridge.rs` around lines 737 - 778, The merge_persist_client_have_ids flow must serialize concurrent persisted have-ID updates. Add or reuse a Mutex<()> associated with client_have_path, acquire its guard at the start of merge_persist_client_have_ids, and hold it through load_client_have_ids, merging, and the atomic temporary-file write/rename sequence so concurrent poll_client_download and drain_local_inbox calls cannot overwrite each other’s IDs.Source: Path instructions
🤖 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 `@reticulum-sidecar/src/stack/live.rs`:
- Around line 3783-3795: In the path-gate failure branch of
start_propagation_sync, only clear propagation_sync_target when the current
target still belongs to this attempt, matching the ownership checks used later
in the same function. Compare driver.propagation_sync_target() with this
attempt’s destination hash before calling set_propagation_sync_target(None),
while preserving the existing logging and error return.
In `@reticulum-sidecar/src/stack/lxmf_outbound.rs`:
- Around line 677-700: Move the `mark_propagated_delivery_attempt` call and its
maximum-attempts check out of the pre-branch flow and place them immediately
before `start_packed_delivery`, so `InProcessDepositOutcome::Busy` requeues
without consuming an attempt. Add a behavioral regression test that holds the
local-node mutex across multiple ticks, then releases it and verifies the local
deposit succeeds.
- Around line 606-628: Update the PropagationNode try_lock handling in the
outbound delivery paths around local cost calculation and the related lines near
782 and 810 to distinguish WouldBlock from Poisoned. Keep WouldBlock on the
existing busy/requeue path, but for Poisoned log the mutex failure, return
Failed without treating it as Busy, and advance to the next PN cascade.
In `@src/renderer/lib/reticulum/reticulumPropagationAutoApply.ts`:
- Around line 63-66: Update the sync error propagation around startSync,
attemptSync, and finishWith so soft-deferred failures do not overwrite the last
failed attempt’s lastSyncError. Preserve the prior error when handling
PROPAGATION_STACK_NOT_LIVE, RNS stack not live, or PROPAGATION_RETRIEVE_BUSY,
and ensure finishWith clears to the last active sync attempt’s preserved error.
In `@src/renderer/stores/reticulumPropagationStore.ts`:
- Around line 429-435: Update the propagation sync request flow around
proxyPost() and its catch handler to verify stillCurrent() immediately after
completion and before clearing the shared stall watchdog; stale successful
responses must return 'deferred' rather than 'accepted', and stale failures or
rejections must not clear the newer attempt’s watchdog. Add coverage in
reticulumPropagationStore.test.ts for stale remote-success and stale rejection
cases.
---
Outside diff comments:
In `@reticulum-sidecar/src/stack/propagation_bridge.rs`:
- Around line 562-592: Update the PropagationClientState::Complete branch in
poll_client_download so have IDs are derived only from successfully decoded
messages, matching drain_local_inbox behavior. Decode blobs before calling
client.add_local_message or merge_persist_client_have_ids, retain undecodable
blobs for retry, and preserve the existing listed/downloaded reporting and
completion flow.
In `@src/renderer/lib/reticulum/reticulumPropagationMode.ts`:
- Around line 159-174: Update listConfiguredRemotePropagationIds to normalize
each node’s hops before sorting: convert non-finite values and values outside
the valid 0–32 range to Number.POSITIVE_INFINITY, while preserving valid hop
counts. Add coverage for a configured node with hops above 32 and verify it
sorts as an unknown-hop node.
---
Other comments:
In `@reticulum-sidecar/src/stack/persistence.rs`:
- Around line 425-436: Update normalize_propagation_auto_blacklist_hash to trim
and lowercase the input, then reject it unless all characters are ASCII
hexadecimal and the normalized value is exactly 32 characters; do not filter or
strip arbitrary characters. Preserve the existing Result error behavior so
add_propagation_auto_blacklist, remove_propagation_auto_blacklist, and
deserialization consistently reject malformed hashes.
---
Nitpick comments:
In `@reticulum-sidecar/src/stack/persistence.rs`:
- Around line 1181-1202: Extend
propagation_auto_blacklist_add_remove_normalizes_hash or add a focused test for
add_propagation_auto_blacklist that inserts PROPAGATION_AUTO_BLACKLIST_CAP
distinct valid hashes, verifies the list reaches the cap, and asserts the next
valid add returns an error for the full-list boundary.
In `@reticulum-sidecar/src/stack/propagation_bridge.rs`:
- Around line 737-778: The merge_persist_client_have_ids flow must serialize
concurrent persisted have-ID updates. Add or reuse a Mutex<()> associated with
client_have_path, acquire its guard at the start of
merge_persist_client_have_ids, and hold it through load_client_have_ids,
merging, and the atomic temporary-file write/rename sequence so concurrent
poll_client_download and drain_local_inbox calls cannot overwrite each other’s
IDs.
🪄 Autofix
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: QUIET
Plan: Pro Plus
Run ID: 6e448464-5b61-433b-8035-7f72729d9a3d
⛔ Files ignored due to path filters (18)
reticulum-sidecar/patches/README.mdis excluded by!reticulum-sidecar/patches/**reticulum-sidecar/patches/rsLXMF-propagation-client-abort-transfer.patchis excluded by!reticulum-sidecar/patches/**src/renderer/locales/cs/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/de/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/en/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/es/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/fr/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/id/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/it/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/ja/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/ko/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/nl/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/pl/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/pt-BR/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/ru/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/tr/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/uk/translation.jsonis excluded by!src/renderer/locales/**src/renderer/locales/zh/translation.jsonis excluded by!src/renderer/locales/**
📒 Files selected for processing (41)
docs/agents/common-issues.mddocs/agents/reticulum.mddocs/meshcore-meshtastic-parity.mddocs/reticulum-sidecar-ipc.mddocs/reticulum.mddocs/troubleshooting.mdreticulum-sidecar/src/api/mod.rsreticulum-sidecar/src/api/propagation.rsreticulum-sidecar/src/stack/live.rsreticulum-sidecar/src/stack/lxmf_outbound.rsreticulum-sidecar/src/stack/mod.rsreticulum-sidecar/src/stack/persistence.rsreticulum-sidecar/src/stack/pn_cascade.rsreticulum-sidecar/src/stack/propagation_bridge.rsreticulum-sidecar/src/stack/propagation_mode.rsscripts/apply-rsLXMF-propagation-client-abort-transfer.shscripts/check-i18n-quality.mjsscripts/lib/ratspeak-overlay-apply-list.shscripts/update.shsrc/renderer/components/NodeListPanel.test.tsxsrc/renderer/components/ReticulumPropagationNotice.tsxsrc/renderer/components/ReticulumPropagationSection.test.tsxsrc/renderer/components/ReticulumPropagationSection.tsxsrc/renderer/lib/reticulum/reticulumDiagnosticSnapshot.tssrc/renderer/lib/reticulum/reticulumOutboundFailureBridge.tssrc/renderer/lib/reticulum/reticulumPropagationAutoApply.test.tssrc/renderer/lib/reticulum/reticulumPropagationAutoApply.tssrc/renderer/lib/reticulum/reticulumPropagationEffective.test.tssrc/renderer/lib/reticulum/reticulumPropagationEffective.tssrc/renderer/lib/reticulum/reticulumPropagationMode.test.tssrc/renderer/lib/reticulum/reticulumPropagationMode.tssrc/renderer/lib/reticulum/reticulumPropagationSync.test.tssrc/renderer/lib/reticulum/reticulumPropagationSync.tssrc/renderer/lib/reticulum/useReticulumPropagationAutoSync.tssrc/renderer/lib/rncpLxmfControlSideEffectDedup.test.tssrc/renderer/lib/rncpLxmfControlSideEffectDedup.tssrc/renderer/runtime/useReticulumRuntime.reconnect-hardening.test.tssrc/renderer/runtime/useReticulumRuntime.rncp-receive-dest.test.tssrc/renderer/runtime/useReticulumRuntime.tssrc/renderer/stores/reticulumPropagationStore.test.tssrc/renderer/stores/reticulumPropagationStore.ts
Guard path-gate latch clear by attempt ownership; do not burn delivery attempts on local-prop Busy; distinguish poisoned mutex from WouldBlock; persist have-ids only for decoded mail under a serialize lock; harden blacklist hash validation; preserve cascade errors across soft-defer; and rank absurd configured hops as unknown.
2 participants
Problem
Propagation Node (PN) Sync/Auto was unreliable in several ways that looked like “empty inbox” or a stuck UI even when the stack was busy or racey:
/get/ permanent busy — Cancel mid-retrieve left the client download in a non-Idle state (RETRIEVE_BUSY), so later Sync attempts failed until restart. Client have-ids were not persisted atomically, so a crash mid-write could wipe haves and trigger re-fetch storms.lastPropagationSyncAtand suppressing retries.local-propSync could return Ok/100% when the RNS stack was not live. Latelocal-propresponses could clear a newer Sync attempt’s error/idle state./getracing user Sync — Finishing Host peer/periodic retrieve clearedpropagation_sync_targetunconditionally, wiping a newer user Sync latch and breaking progress/cancel ownership.min_cost = 0and could self-Link on lock miss; absurd hop counts ranked ghosts ahead of real peers; docs still described Sync as/offer-primary while code is/get-primary.How this fixes it
RETRIEVE_BUSY, outbound busy, stack not live) clear error without backoff success; Auto does not advance success timestamps when remotes only deferred and only local settled.local-propreturnsPROPAGATION_STACK_NOT_LIVEwhen live is missing.startSyncignores superseded attempt tokens./getterminal clear only ifpropagation_sync_targetstill equals that peer (same guard on user Sync busy/terminal paths).min_stamp_cost; defer instead of self-Link on lock miss; demote absurd hops (aligned Rust/TS); rewrite Sync//get-primary docs; locale refresh for stale EN + false friends; RNCP cold-start hydrate skip with upsert_failed retry token.Test plan
/getis finishing — Sync latch and progress stay on the user targetlocal-propbefore sidecar live attach — expect soft-defer / stack-not-live, not 100% successlastPropagationSyncAt; UI shows retrieve-busy/getthen Sync again — second start succeeds (not permanentRETRIEVE_BUSY)pnpm run reticulum:sidecar:testand related Vitest (propagation store/Auto/effective/Section/RNCP)Summary by CodeRabbit
New Features
Bug Fixes
Documentation