feat: add adguard_export plugin#1649
Merged
Merged
Conversation
Adds a new NetAlertX plugin that syncs known devices from the NetAlertX database to AdGuard Home as persistent clients, keeping names, MACs, IP addresses, and device-type tags in sync. Also fixes the adguard_import config.json description placeholder and a minor indentation inconsistency in that file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
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)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a new AdGuard Home device export plugin: configuration and README, Python implementation that syncs NetAlertX Devices to AdGuard Home persistent clients with managed-name state and optional deletions, and a comprehensive pytest suite; plus a minor description update to the AdGuard import plugin. ChangesAdGuard Export Plugin
Import Plugin Documentation Update
Sequence Diagram(s): sequenceDiagram
participant Main as main()
participant DB as get_netalertx_devices()
participant Builder as build_agrd_client()
participant Sync as sync_to_adguard()
participant AdGuard as AdGuardClient
participant State as managed_names
Main->>DB: query devices with filters
DB-->>Main: list of devices
Main->>Sync: pass devices and flags
Sync->>State: load_managed_names()
State-->>Sync: managed name set
Sync->>AdGuard: get_clients()
AdGuard-->>Sync: existing clients list
Sync->>Builder: build_agrd_client(device)
Builder-->>Sync: client payload (ids,tags,name)
Sync->>AdGuard: add_client()/update_client()/delete_client()
AdGuard-->>Sync: HTTP responses
Sync-->>Main: counts (added, updated, skipped, deleted)
Main->>State: save_managed_names()
Main-->>Main: write result file
Suggested reviewers
🚥 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 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 |
![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: 4
🧹 Nitpick comments (2)
front/plugins/adguard_export/README.md (1)
136-140: ⚡ Quick winStandardize the maintainer metadata block to
### Other infoformat.This README uses
## Notes; plugin READMEs are expected to use the standard### Other infoblock with canonical labels and date format.Suggested patch
-## Notes +### Other info - Version: 1.0.0 - Author: [natecj](https://github.com/natecj) -- Release Date: 2026-05-10 +- Release Date: 10-May-2026Based on learnings: For each NetAlertX plugin README at
front/plugins/**/README.md, include a### Other infosection with the exact standard field labels andDD-Mon-YYYYrelease date format.🤖 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/adguard_export/README.md` around lines 136 - 140, Replace the non-standard "## Notes" block with the canonical "### Other info" metadata block in this README: change the heading to "### Other info", use the exact standard field labels (Version, Maintainer, Release Date) and convert the Release Date to the DD-Mon-YYYY format (e.g., 10-May-2026); update the Author label to "Maintainer" and ensure the maintainer value uses the same notation as other plugins (e.g., [natecj](https://github.com/natecj)).test/plugins/test_adguard_export.py (1)
339-347: ⚡ Quick winAdd a regression test to ensure ID-matched manual clients are not adopted into managed state.
Current tests verify unmanaged clients aren’t deleted when state is empty, but not the case where a manual client is matched by ID during sync and must still remain unmanaged.
Suggested test addition
class TestSyncToAdguard: @@ def test_unmanaged_device_not_deleted(self, tmp_path): # State file is empty — we never added this client state = tmp_path / "state.json" state.write_text(json.dumps({"managed": []})) existing = [{"name": "Manual Client", "ids": ["10.0.0.50"], "tags": []}] agrd = _mock_agrd(existing=existing) with patch("script.STATE_FILE", str(state)): sync_to_adguard(agrd, [], use_mac=True, delete_missing=True) agrd.delete_client.assert_not_called() + + def test_id_matched_manual_client_is_not_marked_managed(self, tmp_path): + state = tmp_path / "state.json" + state.write_text(json.dumps({"managed": []})) + existing = [{"name": "Manual Client", "ids": ["aa:bb:cc:00:00:01", "10.0.0.1"], "tags": ["device_pc"]}] + agrd = _mock_agrd(existing=existing) + with patch("script.STATE_FILE", str(state)): + sync_to_adguard(agrd, [self._device(name="Manual Client")], use_mac=True, delete_missing=False) + managed = load_managed_names() + assert managed == set()🤖 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 `@test/plugins/test_adguard_export.py` around lines 339 - 347, Add a regression test ensuring manual clients matched by ID are not adopted: create a test (e.g. test_unmanaged_device_not_deleted_by_id) that writes an empty managed state to STATE_FILE, mocks existing AdGuard clients with an entry containing the matching ID (use _mock_agrd to return existing), then call sync_to_adguard(agrd, [], use_mac=True, delete_missing=True) and assert agrd.delete_client.assert_not_called(); this mirrors test_unmanaged_device_not_deleted but verifies ID-based matching doesn't convert a manual client into managed state during sync.
🤖 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/adguard_export/config.json`:
- Around line 340-533: The visible column objects in the config (the column
definitions mapped to cur_MAC, cur_IP, cur_Name, etc.) are missing show: true so
getColumnDefinitions() (which filters by colDef.show) hides them; update each
visible column object (e.g., the entries with "column": "Object_PrimaryID",
"Object_SecondaryID", "Watched_Value1", "Watched_Value2", "Watched_Value3",
"Watched_Value4", "Extra", "ForeignKey") to include show: true so they render in
the plugin output table.
In `@front/plugins/adguard_export/README.md`:
- Around line 96-114: Update the three fenced code blocks that show file paths
so they include a language identifier (e.g., "text") to satisfy markdownlint
MD040: change the blocks containing "/app/db/state.ADGUARDEXP.json",
"/tmp/log/plugins/script.ADGUARDEXP.log", and
"/tmp/log/plugins/last_result.ADGUARDEXP.log" to use triple backticks with a
language tag (text) instead of plain fences; keep the content unchanged and only
add the language identifier to each fence in README.md.
- Line 46: Update the README entry for ADGUARDEXP_URL to match the actual config
default: change the documented default URL from http://192.168.11.1:3000 to
http://localhost:3000 so it matches the default set for ADGUARDEXP_URL in the
plugin config (check the default value in config.json where ADGUARDEXP_URL is
defined).
In `@front/plugins/adguard_export/script.py`:
- Around line 320-342: The current logic marks any matched existing AdGuard
clients as plugin-managed by adding to managed_names inside the update and skip
branches (references: managed_names, agrd.update_client, existing, device,
client_data), which may cause deletion of manually-created clients later; change
it so we do NOT add or rename entries in managed_names when we merely match or
update an existing client (remove managed_names.add(old_name) /
managed_names.add(device["name"]) from the update and skip branches), and only
add names to managed_names at the single location where we actually create a new
client (the create_client/create branch) or when we can prove the plugin already
owned it; ensure rename logic still updates names for created-managed clients
but never converts a pre-existing manual client into managed-owned.
---
Nitpick comments:
In `@front/plugins/adguard_export/README.md`:
- Around line 136-140: Replace the non-standard "## Notes" block with the
canonical "### Other info" metadata block in this README: change the heading to
"### Other info", use the exact standard field labels (Version, Maintainer,
Release Date) and convert the Release Date to the DD-Mon-YYYY format (e.g.,
10-May-2026); update the Author label to "Maintainer" and ensure the maintainer
value uses the same notation as other plugins (e.g.,
[natecj](https://github.com/natecj)).
In `@test/plugins/test_adguard_export.py`:
- Around line 339-347: Add a regression test ensuring manual clients matched by
ID are not adopted: create a test (e.g. test_unmanaged_device_not_deleted_by_id)
that writes an empty managed state to STATE_FILE, mocks existing AdGuard clients
with an entry containing the matching ID (use _mock_agrd to return existing),
then call sync_to_adguard(agrd, [], use_mac=True, delete_missing=True) and
assert agrd.delete_client.assert_not_called(); this mirrors
test_unmanaged_device_not_deleted but verifies ID-based matching doesn't convert
a manual client into managed state during sync.
🪄 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: e0fd4190-ac85-4f3d-aa4f-7f0ae59be37c
📒 Files selected for processing (5)
front/plugins/adguard_export/README.mdfront/plugins/adguard_export/config.jsonfront/plugins/adguard_export/script.pyfront/plugins/adguard_import/config.jsontest/plugins/test_adguard_export.py
- config.json: add show:true to all visible column definitions so they render in the plugin output table - script.py: fix managed_names adoption bug — update/skip branches no longer add unowned clients to managed state; rename tracking now scoped to plugin-created clients only - README.md: fix ADGUARDEXP_URL default (localhost:3000, not local IP), add language tags to fenced code blocks, normalise metadata block to Other info / Maintainer / DD-Mon-YYYY format - test_adguard_export.py: add regression test for manual client matched by ID not being adopted into managed state Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Collaborator
|
Hi @natecj - thanks a lot for the PR. Let me know once you think it's ready to go and I'll do a close review. |
jokob-sk
reviewed
May 24, 2026
Replaces the raw sqlite3 query in get_netalertx_devices() with DeviceInstance().getAll() as suggested in code review, applying the archived/offline/new filters in Python. Removes the sqlite3 and fullDbPath imports. Updates tests to mock DeviceInstance.getAll(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
front/plugins/adguard_export/script.py (1)
51-62: ⚡ Quick winSpecify explicit UTF-8 encoding for state file I/O.
Device names may contain non-ASCII characters, and the default encoding depends on system locale (which can vary in container environments). Explicit encoding avoids potential
UnicodeDecodeError/UnicodeEncodeError.Proposed fix
def load_managed_names() -> Set[str]: """Return the set of AdGuard client names we previously added.""" try: - with open(STATE_FILE) as f: + with open(STATE_FILE, encoding="utf-8") as f: return set(json.load(f).get("managed", [])) except (FileNotFoundError, json.JSONDecodeError): return set() def save_managed_names(names: Set[str]) -> None: - with open(STATE_FILE, "w") as f: + with open(STATE_FILE, "w", encoding="utf-8") as f: json.dump({"managed": sorted(names)}, f, indent=2)🤖 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/adguard_export/script.py` around lines 51 - 62, The state file I/O in load_managed_names and save_managed_names should use explicit UTF-8 to avoid locale-dependent decode/encode errors: open STATE_FILE with encoding="utf-8" when reading in load_managed_names and when writing in save_managed_names, and when dumping JSON set ensure_ascii=False so non-ASCII device names are preserved; keep the existing fallback for FileNotFoundError/json.JSONDecodeError in load_managed_names unchanged.
🤖 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/adguard_export/script.py`:
- Around line 51-62: The state file I/O in load_managed_names and
save_managed_names should use explicit UTF-8 to avoid locale-dependent
decode/encode errors: open STATE_FILE with encoding="utf-8" when reading in
load_managed_names and when writing in save_managed_names, and when dumping JSON
set ensure_ascii=False so non-ASCII device names are preserved; keep the
existing fallback for FileNotFoundError/json.JSONDecodeError in
load_managed_names unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40e01db7-76d6-4e9f-a7ea-96f386d8f734
📒 Files selected for processing (2)
front/plugins/adguard_export/script.pytest/plugins/test_adguard_export.py
jokob-sk
reviewed
May 29, 2026
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestGetNetalertxDevices: |
Collaborator
There was a problem hiding this comment.
Do we need these tests? I assume not plugin specific - looks like testing core NAX functionality - if we need tests like these probably not here in a plugin specific test file
Contributor
There was a problem hiding this comment.
@jokob-sk, good question. My take: these tests are appropriately plugin-specific and belong here.
Here's the key distinction: get_netalertx_devices is defined in the plugin's own script.py. The tests patch DeviceInstance.getAll with mock data, so they never exercise the core NAX model — they only validate the plugin's own logic on top of it:
- Filtering by
devIsArchived,devPresentLastScan,devIsNew(plugin-controlled settings) - Normalizing rows into
{mac, name, last_ip, dev_type} - Falling back to MAC as the device name when
devNameis empty - Skipping rows that lack both MAC and IP
None of that is testing core NAX functionality — it's testing the plugin's wrapper behavior. The same reasoning applies to the rest of the test classes (TestDeviceTypeToTag, TestBuildAgrdClient, TestManagedNames, TestSyncToAdguard), which are entirely plugin-scoped.
If this pattern (DeviceInstance().getAll() + filter/normalize) were shared across many plugins and the filtering logic lived in core NAX code, then it would make sense to test it centrally. But since the logic lives in this plugin's script, testing it here is the right call.
🧠 Learnings used
Learnt from: jokob-sk
Repo: netalertx/NetAlertX PR: 1435
File: test/authoritative_fields/test_field_lock_scan_integration.py:1-12
Timestamp: 2026-01-21T00:17:51.986Z
Learning: In NetAlertX, exempt test files from the 500-line code file maximum guideline. This applies to Python test files named test_*.py and any files under the test/ directories; do not apply the 500-line limit to these test files.
Learnt from: jokob-sk
Repo: netalertx/NetAlertX PR: 1621
File: server/auth/ldap_provider.py:97-97
Timestamp: 2026-04-27T00:51:56.385Z
Learning: In the NetAlertX server-side Python code, treat logging text inside `mylog(...)` as developer/debug-only: keep it in English and do not add those strings to `front/php/templates/language/en_us.json`. By contrast, any string that is user-facing (e.g., error messages returned from API responses that are rendered in the login page/front-end) must be added as a translation key to `front/php/templates/language/en_us.json` so it can be translated via Weblate.
Learnt from: jokob-sk
Repo: netalertx/NetAlertX PR: 1621
File: server/auth/ldap_provider.py:165-165
Timestamp: 2026-04-27T00:52:02.534Z
Learning: In the NetAlertX codebase, do not use inline/dynamic imports in Python (e.g., `import ...` inside functions/methods). Require all imports to be at module top level so they are auditable and missing dependencies fail at import/load time rather than later at runtime. If an optional dependency must be handled, use a top-level `try/except ImportError` block (e.g., for `ldap3`) instead of a deferred inline import.
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
2 participants
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
adguard_exportplugin that syncs known devices from the NetAlertX database to AdGuard Home as persistent clients, keeping names, MAC addresses, IP addresses, and device-type tags in syncadguard_importconfig.json description placeholder and a minor indentation inconsistencyNew files
front/plugins/adguard_export/script.py— main sync scriptfront/plugins/adguard_export/config.json— plugin metadata and settingsfront/plugins/adguard_export/README.md— documentationtest/plugins/test_adguard_export.py— 31 pytest tests covering device-type tag mapping, client building, state file round-trips, SQLite queries, and the full sync logicTest plan
pytest test/plugins/test_adguard_export.py -vpasses (31 tests)ADGUARDEXP_DELETE, remove a device from NetAlertX, re-run, verify only that client is removed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests