SYNC plugin improvements - skipping non-reachable nodes, SYNC_BEHAVIOR#1655
Open
jokob-sk wants to merge 5 commits into
Open
SYNC plugin improvements - skipping non-reachable nodes, SYNC_BEHAVIOR#1655jokob-sk wants to merge 5 commits into
jokob-sk wants to merge 5 commits into
Conversation
…s for PUSH and PULL modes
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds configurable hub SYNC_BEHAVIOR (copy-new, carbon-copy, hub-defaults), updates hub sync code and test helper to honor behavior including event pre-inserts and upserts, tightens filename PUSH/PULL detection test logic, and updates frontend config and documentation. ChangesFilename Detection Logic Refinement
Hub Sync Behavior Implementation and Tests
Documentation and Frontend Config
Sequence Diagram(s)sequenceDiagram
participant HubSync
participant EventsTable
participant DevicesTable
HubSync->>EventsTable: INSERT New Device rows for genuinely new MACs (non-hub-defaults)
HubSync->>DevicesTable: INSERT OR IGNORE (copy-new)
HubSync->>DevicesTable: INSERT ... ON CONFLICT(devMac) DO UPDATE (carbon-copy)
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… update related documentation #1652
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/API_SYNC.md (1)
94-115: ⚡ Quick winDeduplicate repeated processing-condition note.
The same bullet appears at Line 94 and Line 115; keep one copy to avoid future inconsistency.
🤖 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 `@docs/API_SYNC.md` around lines 94 - 115, Remove the duplicated sentence "The data is only processed if the relevant plugins are enabled and run on the target server." so it appears only once (keep the occurrence under Key Notes) by deleting the second copy at the end of the "Storage Details" section; ensure surrounding bullets/spacing remain consistent and run a quick spell/format check to avoid leaving an extra blank line.
🤖 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 `@front/plugins/sync/README.md`:
- Line 88: Fix the typo "fileds" -> "fields" in the README sentence that
describes hub field behavior for user-customized values; locate the sentence
mentioning "source set to `USER` or `LOCKED`" and replace "fileds" with "fields"
so the line reads "...Fields customized by a user on the hub (fields with source
set to `USER` or `LOCKED`) are never overwritten."
- Around line 90-118: Update the three fenced code blocks in
front/plugins/sync/README.md to include an explicit language tag (use "text") so
markdown linting passes; specifically, change the first block containing "First
sync: INSERT with node's full config" to start with ```text, the second block
containing "First sync: UPSERT with node config" to start with ```text, and the
third block containing "First sync: NEWDEV defaults applied" to start with
```text.
In `@front/plugins/sync/sync.py`:
- Around line 269-286: The comment describing the "carbon-copy" SYNC_BEHAVIOR is
incorrect; update the comment in front/plugins/sync/sync.py so it accurately
states that "carbon-copy" UPSERTs overwrite all hub fields on every sync,
including USER/LOCKED-sourced fields, and remove the claim that enforcement is
handled by update_devices_data_from_scan (since that pipeline is not invoked for
these writes); reference the SYNC_BEHAVIOR "carbon-copy" mode and the
update_devices_data_from_scan pipeline in the updated comment so readers can
find the related implementation and README contract.
In `@test/plugins/test_sync_protocol.py`:
- Line 575: Replace the unicode EN DASH characters in the comment containing
"SYNC_BEHAVIOR — three hub device-write modes (Mode 3 – RECEIVE)" with an ASCII
hyphen-minus '-' (e.g., "SYNC_BEHAVIOR - three hub device-write modes (Mode 3 -
RECEIVE)"); update both occurrences of the EN DASH so Ruff RUF003 no longer
flags them, leaving the comment text otherwise unchanged.
- Around line 768-778: The final assertion is calling cur.fetchone()["cnt"]
after already consuming the first row from the SELECT * query, causing
fetchone() to return None; update the test (in test_sync_protocol.py around the
block using cur and row) to either 1) run a separate COUNT query such as SELECT
COUNT(*) AS cnt FROM Events WHERE eveEventType='New Device' AND eveMac=? and
assert cur.fetchone()["cnt"] == 0/expected, or 2) replace the initial SELECT *
with cur.execute(...) then rows = cur.fetchall() and assert len(rows) and
inspect rows[0] (use the variable names cur and row as appropriate) so you don't
call fetchone() on an exhausted cursor.
---
Nitpick comments:
In `@docs/API_SYNC.md`:
- Around line 94-115: Remove the duplicated sentence "The data is only processed
if the relevant plugins are enabled and run on the target server." so it appears
only once (keep the occurrence under Key Notes) by deleting the second copy at
the end of the "Storage Details" section; ensure surrounding bullets/spacing
remain consistent and run a quick spell/format check to avoid leaving an extra
blank line.
🪄 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
Run ID: 0c5f1742-3b9a-4957-8ac0-53a5d088a774
📒 Files selected for processing (9)
docs/ADVISORY_MULTI_NETWORK.mddocs/API_SYNC.mddocs/NOTIFICATIONS.mddocs/REMOTE_NETWORKS.mdfront/plugins/sync/README.mdfront/plugins/sync/config.jsonfront/plugins/sync/sync.pytest/db_test_helpers.pytest/plugins/test_sync_protocol.py
💤 Files with no reviewable changes (1)
- docs/NOTIFICATIONS.md
✅ Files skipped from review due to trivial changes (1)
- docs/REMOTE_NETWORKS.md
Comment on lines
+90
to
+118
| ``` | ||
| First sync: INSERT with node's full config | ||
| Next syncs: empty fields updated only (name, vendor) via scan pipeline | ||
| User edits: protected — never overwritten | ||
| ``` | ||
|
|
||
| #### `carbon-copy` | ||
|
|
||
| All received devices are upserted on every sync. The node is treated as fully authoritative: its values overwrite **all** hub fields on every sync cycle, including fields with `USER` or `LOCKED` source. | ||
|
|
||
| > ⚠️ Do not customize devices on the hub when using `carbon-copy`. Any hub-side changes will be overwritten on the next sync. | ||
|
|
||
| ``` | ||
| First sync: UPSERT with node config | ||
| Next syncs: UPSERT — node values win (all fields, no exceptions) | ||
| User edits: overwritten on next sync | ||
| ``` | ||
|
|
||
| #### `hub-defaults` | ||
|
|
||
| **The hub is the source of truth.** Nodes contribute only presence data (MAC, IP, vendor from scans). All device configuration — name, alerts, notes, group — should be set on the hub. Node-side values for those fields are ignored. | ||
|
|
||
| Use this mode when you want the hub to behave as a fully independent instance — it receives presence data from nodes but manages its own device configuration. | ||
|
|
||
| ``` | ||
| First sync: NEWDEV defaults applied | ||
| Next syncs: empty fields updated only via scan pipeline | ||
| User edits: set and maintained on the hub — never overwritten | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language tags to fenced code blocks.
The three example blocks should use an explicit language (e.g., text) to satisfy markdown lint rules.
Suggested patch
-```
+```text
First sync: INSERT with node's full config
Next syncs: empty fields updated only (name, vendor) via scan pipeline
User edits: protected — never overwritten@@
- +text
First sync: UPSERT with node config
Next syncs: UPSERT — node values win (all fields, no exceptions)
User edits: overwritten on next sync
@@
-```
+```text
First sync: NEWDEV defaults applied
Next syncs: empty fields updated only via scan pipeline
User edits: set and maintained on the hub — never overwritten
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 90-90: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
[warning] 102-102: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
[warning] 114-114: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @front/plugins/sync/README.md around lines 90 - 118, Update the three fenced
code blocks in front/plugins/sync/README.md to include an explicit language tag
(use "text") so markdown linting passes; specifically, change the first block
containing "First sync: INSERT with node's full config" to start with ```text,
the second block containing "First sync: UPSERT with node config" to start with
to start with ```text.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
front/plugins/sync/README.md (1)
157-182: ⚡ Quick winAdd the standard plugin
### Other infofooter section.This README is missing the standard metadata footer used across plugin docs.
📄 Proposed patch
--- #### How to Set It Up @@ 7. Check **Sync Devices**. +### Other info + +- Version: 0.1 +- Author: [jokob-sk](https://github.com/jokob-sk) +- Release Date: 26-May-2026Based on learnings: For each
front/plugins/**/README.md, include a### Other infosection with standardized fields and labels.🤖 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 `@front/plugins/sync/README.md` around lines 157 - 182, The README's missing the standard plugin metadata footer—append a "### Other info" section at the end of the document (after the "How to Set It Up" content) and populate it with the standardized fields used across plugin docs: e.g., "Supported versions", "Maintainers", "License", "Configuration keys", and "Troubleshooting" (use the exact heading "### Other info" and consistent labels for each field so it matches other front/plugins/**/README.md files).
🤖 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.
Nitpick comments:
In `@front/plugins/sync/README.md`:
- Around line 157-182: The README's missing the standard plugin metadata
footer—append a "### Other info" section at the end of the document (after the
"How to Set It Up" content) and populate it with the standardized fields used
across plugin docs: e.g., "Supported versions", "Maintainers", "License",
"Configuration keys", and "Troubleshooting" (use the exact heading "### Other
info" and consistent labels for each field so it matches other
front/plugins/**/README.md files).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f2641fb-d40d-415a-b0a2-92cb284027f7
📒 Files selected for processing (3)
front/plugins/sync/README.mdfront/plugins/sync/sync.pytest/plugins/test_sync_protocol.py
jokob-sk
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
front/plugins/sync/sync.py (2)
294-337:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
SYNC_BEHAVIORbefore branching.Any value other than
copy-new,carbon-copy, orhub-defaultscurrently falls into an undocumented path: Line 314 processes all devices, then Lines 323-337 useINSERT OR IGNORE. That silently creates a fourth behavior instead of honoring the config contract or falling back to the default.Suggested fix
sync_behavior = get_setting_value('SYNC_BEHAVIOR') or 'copy-new' + valid_behaviors = {'copy-new', 'carbon-copy', 'hub-defaults'} + if sync_behavior not in valid_behaviors: + mylog('none', [f'[{pluginName}] Invalid SYNC_BEHAVIOR "{sync_behavior}", defaulting to "copy-new"']) + sync_behavior = 'copy-new' mylog('verbose', [f'[{pluginName}] SYNC_BEHAVIOR: "{sync_behavior}"'])🤖 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 `@front/plugins/sync/sync.py` around lines 294 - 337, sync_behavior returned by get_setting_value can be any string and unrecognized values silently fall into the "else" path (treated like copy-new) causing an undocumented fourth behavior; before the existing branching (around the use of sync_behavior, devices_to_write, and the carbon-copy/insert logic), validate sync_behavior against the allowed set {'copy-new','carbon-copy','hub-defaults'} and if it's not one of those, log a warning via mylog (include pluginName and the bad value) and set sync_behavior = 'copy-new' (or otherwise explicitly enforce a known default) so the subsequent branches (hub-defaults check, carbon-copy handling, and INSERT OR IGNORE) only execute for known modes.
137-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
base64.b64decodein hub PULL so one bad node can’t abort the loop.In
front/plugins/sync/sync.py,decoded_data = base64.b64decode(data_base64)is unguarded; invalid/incorrectly padded (or non-string/None)data_base64will raise and stop processing for remainingpull_nodes. Decode errors should be caught and that node skipped.Suggested fix
- # Extract node_name and base64 data - node_name = response_json.get('node_name', 'unknown_node') - data_base64 = response_json.get('data_base64', '') - - # Decode base64 data - decoded_data = base64.b64decode(data_base64) + # Extract node_name and base64 data + node_name = response_json.get('node_name', 'unknown_node') + data_base64 = response_json.get('data_base64', '') + + try: + decoded_data = base64.b64decode(data_base64) + except Exception: + mylog('none', [f'[{pluginName}] Skipping node "{node_url}" due to invalid payload encoding']) + continue🤖 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 `@front/plugins/sync/sync.py` around lines 137 - 149, Wrap the base64 decoding in a try/except around base64.b64decode(data_base64) (inside the code handling the hub PULL loop that processes each node) so a malformed or non-string data_base64 for a given node_name does not raise and abort the loop; catch binascii.Error, ValueError and TypeError, log the failure (including node_name and the problematic data_base64) and skip writing the log_file_name to LOG_PATH for that node, allowing the loop to continue to the next pull node.
🤖 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.
Outside diff comments:
In `@front/plugins/sync/sync.py`:
- Around line 294-337: sync_behavior returned by get_setting_value can be any
string and unrecognized values silently fall into the "else" path (treated like
copy-new) causing an undocumented fourth behavior; before the existing branching
(around the use of sync_behavior, devices_to_write, and the carbon-copy/insert
logic), validate sync_behavior against the allowed set
{'copy-new','carbon-copy','hub-defaults'} and if it's not one of those, log a
warning via mylog (include pluginName and the bad value) and set sync_behavior =
'copy-new' (or otherwise explicitly enforce a known default) so the subsequent
branches (hub-defaults check, carbon-copy handling, and INSERT OR IGNORE) only
execute for known modes.
- Around line 137-149: Wrap the base64 decoding in a try/except around
base64.b64decode(data_base64) (inside the code handling the hub PULL loop that
processes each node) so a malformed or non-string data_base64 for a given
node_name does not raise and abort the loop; catch binascii.Error, ValueError
and TypeError, log the failure (including node_name and the problematic
data_base64) and skip writing the log_file_name to LOG_PATH for that node,
allowing the loop to continue to the next pull node.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff23f107-5698-465a-9f8c-b2872782269a
📒 Files selected for processing (1)
front/plugins/sync/sync.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Tests