Feat/oob sync - #79
Closed
marcinpsk wants to merge 120 commits into
Closed
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds OOB controller detection and linkage handling, server-key-scoped sync and migration flows, updated validation/UI behavior, bulk collision detection, and matching documentation and regression coverage. ChangesOOB import, sync, and migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_librenms_plugin/utils.py (1)
875-908:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer host-ID matches before OOB-ID matches.
This query now treats
librenms_id[server_key].id == Xandlibrenms_id[server_key].oob.id == Xas the same lookup and then returns.first(). If one NetBox row owns host ID42and a different row stores OOB ID42, the selected object is DB-order dependent.Suggested fix
- q = Q(**{f"custom_field_data__librenms_id__{server_key}": librenms_id}) - # Also match when the namespaced value was stored as a string (e.g. {"production": "42"}). - q |= Q(**{f"custom_field_data__librenms_id__{server_key}": str(librenms_id)}) - # Match when value stored as {"id": librenms_id, "oob": {...}} — new dict-with-id form. - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": librenms_id}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": str(librenms_id)}) - # Match when librenms_id is the OOB device id — so re-import recognises merged device. - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": librenms_id}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": str(librenms_id)}) + host_q = Q(**{f"custom_field_data__librenms_id__{server_key}": librenms_id}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}": str(librenms_id)}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": librenms_id}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": str(librenms_id)}) @@ - q |= Q(**{f"custom_field_data__librenms_id__{server_key}": canonical_str}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}": int_value}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": canonical_str}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": int_value}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": canonical_str}) - q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": int_value}) - q |= Q(custom_field_data__librenms_id=canonical_str) - q |= Q(custom_field_data__librenms_id=int_value) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}": canonical_str}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}": int_value}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": canonical_str}) + host_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__id": int_value}) + host_q |= Q(custom_field_data__librenms_id=canonical_str) + host_q |= Q(custom_field_data__librenms_id=int_value) - return model.objects.filter(q).first() + host_match = model.objects.filter(host_q).first() + if host_match: + return host_match + + oob_q = Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": librenms_id}) + oob_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": str(librenms_id)}) + if isinstance(librenms_id, str): + cleaned = librenms_id.strip() + try: + int_value = int(cleaned) + if int_value > 0: + canonical_str = str(int_value) + oob_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": canonical_str}) + oob_q |= Q(**{f"custom_field_data__librenms_id__{server_key}__oob__id": int_value}) + except ValueError: + pass + return model.objects.filter(oob_q).first()🤖 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 `@netbox_librenms_plugin/utils.py` around lines 875 - 908, The current combined Q (variable q) treats host-ID (__id) and OOB-ID (__oob__id) matches equally and returns model.objects.filter(q).first(), making the result DB-order dependent when different rows match host vs OOB; split the logic into two prioritized queries: build a host_q containing all host-ID forms (namespaced __{server_key} and legacy bare values, including the canonicalized numeric/string variants) and an oob_q containing only the OOB-ID forms, then return model.objects.filter(host_q).first() if present, otherwise model.objects.filter(oob_q).first(); keep the existing canonicalization and legacy fallbacks but move legacy bare-ID checks into host_q so host-ID matches are always preferred over OOB matches.
🤖 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 `@docs/usage_tips/module_sync.md`:
- Line 38: The docs use two different labels for the same carrier-action —
"Suggest Carrier" here versus "Install Carrier" elsewhere — which is confusing;
pick one canonical label (e.g., "Install Carrier") and update this line in
module_sync.md along with any references to the CarrierAutoInstallRule, the
mapping rules guide, and any UI/button label text so all occurrences use the
chosen term consistently.
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 257-264: Add a default "serial_role_choice_available": False entry
to the base result dict (the same dict that contains "serial_action",
"serial_confirmed", "serial_duplicate", etc.) so the function
(device_operations.py where that result is built/returned) always returns a
stable schema; ensure this key is initialized alongside the other defaults and
not only set inside the single serial-match branch.
- Around line 43-77: _describe_existing_librenms_link currently only accepts
literal ints for "id" and nested oob "id", so dicts like
{"id":"42","oob":{"id":"7"}} are treated as unlinked; change the strict
isinstance(int) checks to normalize using the existing coerce_librenms_id helper
(the same normalisation used by find_by_librenms_id/get_librenms_device_id) —
call coerce_librenms_id(entry.get("id")) and coerce_librenms_id(oob.get("id"))
and if the result is not None (and not a bool) assign
info["host_id"]/info["oob_id"] accordingly while leaving the oob_type string
check as-is.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js`:
- Around line 354-357: The client is sending the preference key
'auto_create_ipam' from librenms_import.js via savePref(...) but the backend
rejects it because SaveUserPrefView.ALLOWED_PREFS does not include that key;
update SaveUserPrefView.ALLOWED_PREFS to include 'auto_create_ipam' (or
alternatively change the client to an existing allowed key) so the POST from the
handler tied to document.getElementById('auto-create-ipam-toggle') (and the
savePref call) will be accepted and persist.
In `@netbox_librenms_plugin/tables/device_status.py`:
- Around line 520-534: The code casts paired_oob_id to int directly when
building btn_title (in the branch where match_type == "librenms_id") which can
raise and break rendering; change this to safely coerce/validate paired_oob_id
before interpolating—e.g., check isdigit()/numeric or wrap int(paired_oob_id) in
a try/except and fall back to a safe placeholder or the original escaped value
on error, then use that safe value when constructing btn_title (leave
escape(paired_oob_type or '') as-is).
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html`:
- Line 343: The hx-vals for the move action is using a fallback literal
"default" which can choose the wrong server context; change the hx-vals to
always post the migration marker's server_key (use "{{ interface_sync.server_key
}}") so the view can use CacheMixin.get_cache_key() to load server-scoped cache
correctly for the move action; update the template fragment containing
hx-vals='{"server_key": "{{ interface_sync.server_key|default:"default" }}"}' to
stop using the "default" fallback and send the actual interface_sync.server_key
value.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html`:
- Around line 12-15: The HTMX form posting to the add_device_type_mapping view
omits the auto-create IPAM toggle from its hx-include list, so include the
element id "`#auto-create-ipam-toggle`" in the hx-include attribute of the form
(the form with hx-post="{% url
'plugins:netbox_librenms_plugin:add_device_type_mapping'
device_id=libre_device.device_id %}" and class "mt-1") so the current
auto-create IPAM preference is sent with role_{{ libre_device.device_id }},
rack_{{ libre_device.device_id }}, cluster_{{ libre_device.device_id }},
`#use-sysname-toggle` and `#strip-domain-toggle` during mapping revalidation/update
flows.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html`:
- Around line 10-13: The HTMX form that posts to the
'plugins:netbox_librenms_plugin:add_platform_mapping' endpoint is missing the
auto-create IPAM toggle in its hx-include list; update the form element (the
<form> with hx-post and hx-include currently listing [name=role_{{
libre_device.device_id }}], [name=rack_{{ libre_device.device_id }}],
[name=cluster_{{ libre_device.device_id }}], `#use-sysname-toggle`,
`#strip-domain-toggle`) to also include the selector `#auto-create-ipam-toggle` so
the current import preference is submitted with platform-mapping updates.
In `@netbox_librenms_plugin/tests/test_import_utils.py`:
- Around line 1760-1763: The test currently asserts result["serial_action"],
result["existing_match_type"], and that result.get("oob_candidate") is not None
but misses the promised second payload; update the test to also assert that the
role-choice payload "promote_to_host" is present (e.g., assert
result.get("promote_to_host") is not None) so the contract exposing both
oob_candidate and promote_to_host is enforced (locate the assertion block that
references result in the failing test in tests/test_import_utils.py).
In `@netbox_librenms_plugin/utils.py`:
- Around line 658-675: coerce_librenms_id currently accepts 0 and negative
integers (and string-digit equivalents) as valid IDs which conflicts with
callers like _normalize_librenms_id that treat <= 0 as invalid; update
coerce_librenms_id so that after handling bools it only returns a positive int
(> 0): for an int return it only if value > 0 else None, and for a str convert
to int and return it only if the parsed value > 0, otherwise return None (keep
existing bool-rejection behavior intact).
- Around line 971-995: Validate oob_device_id before storing by ensuring it is
an integer (not a bool) and a positive value consistent with the main device-ID
rules; in the block that constructs oob = {"id": int(oob_device_id), "type":
normalized_type} (and where entry["oob"] is set), first check
isinstance(oob_device_id, int) and not isinstance(oob_device_id, bool) and
oob_device_id > 0 (or coerce safely then validate), and if the check fails log a
warning via logger and skip setting entry["oob"] (or raise ValueError) so we
don't persist boolean/zero/negative IDs that later match real devices.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 300-307: The loop that applies _OOB_OFFSET to entPhysicalIndex and
entPhysicalContainedIn should coerce those fields to integers before doing
arithmetic and before comparing to 0: in the for-loop over oob_inventory (the
block referencing _OOB_OFFSET, item["entPhysicalIndex"], and
item["entPhysicalContainedIn"]), parse item.get("entPhysicalIndex") and
item.get("entPhysicalContainedIn") with int() (or a safe int conversion), handle
non-numeric or missing values by skipping the offset or leaving the original
value, and then replace the fields with the adjusted integer values so that idx
+ _OOB_OFFSET and parent + _OOB_OFFSET never attempt string concatenation or
compare a string to 0.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 571-578: The collision branch currently returns HTMX modal HTML
with status=409 which breaks the project's 200-based HTMX pattern enforced by
_htmx_error_response; update the branch that calls detect_bulk_collisions and
render(...) (the block that assigns collisions and returns the
"netbox_librenms_plugin/htmx/bulk_import_collision.html" view) to return a 2xx
response (remove or change the status argument to 200) so the collision modal is
delivered as a normal HTMX swap instead of triggering error handling.
In `@netbox_librenms_plugin/views/sync/migrate.py`:
- Around line 136-149: The precondition checks (e.g., the
Interface.objects.filter(...).exists() winner-name check and similar
winner/donor guards used in TransferDeviceIPView) must be executed after
acquiring row locks to avoid races: move those checks inside the with
transaction.atomic() block immediately after the select_for_update() call that
locks both Device rows (the block that uses ordered = sorted({donor.pk,
winner.pk}) and Device.objects.select_for_update()), then re-run the same
existence/winner checks and abort with the same _fail response if they now fail
before performing interface.device = winner and interface.save(); apply the same
change to the other similar blocks referenced (the blocks around the other
ranges noted).
---
Outside diff comments:
In `@netbox_librenms_plugin/utils.py`:
- Around line 875-908: The current combined Q (variable q) treats host-ID (__id)
and OOB-ID (__oob__id) matches equally and returns
model.objects.filter(q).first(), making the result DB-order dependent when
different rows match host vs OOB; split the logic into two prioritized queries:
build a host_q containing all host-ID forms (namespaced __{server_key} and
legacy bare values, including the canonicalized numeric/string variants) and an
oob_q containing only the OOB-ID forms, then return
model.objects.filter(host_q).first() if present, otherwise
model.objects.filter(oob_q).first(); keep the existing canonicalization and
legacy fallbacks but move legacy bare-ID checks into host_q so host-ID matches
are always preferred over OOB matches.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 566bbda4-aa1d-478a-b3b9-6f7617d591eb
⛔ Files ignored due to path filters (13)
docs/img/Netbox-librenms-plugin-device-sync-fields.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-import-page.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-module-sync-tab.pngis excluded by!**/*.pngdocs/img/carrier_auto_install_rules/list.pngis excluded by!**/*.pngdocs/img/device_type_mappings/list.pngis excluded by!**/*.pngdocs/img/inventory_ignore_rules/list.pngis excluded by!**/*.pngdocs/img/module_bay_mappings/list.pngis excluded by!**/*.pngdocs/img/module_type_mappings/add.pngis excluded by!**/*.pngdocs/img/module_type_mappings/list.pngis excluded by!**/*.pngdocs/img/normalization_rules/add.pngis excluded by!**/*.pngdocs/img/normalization_rules/list.pngis excluded by!**/*.pngdocs/img/platform_mappings/add.pngis excluded by!**/*.pngdocs/img/platform_mappings/list.pngis excluded by!**/*.png
📒 Files selected for processing (49)
docs/README.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/README.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/migrate.py
marcinpsk
added a commit
that referenced
this pull request
May 22, 2026
docs: fix carrier button label 'Suggest Carrier' -> 'Install Carrier'
The tables/modules.py and carrierautoinstallrule_list.html both use
'Install Carrier'; the module_sync.md usage tip was inconsistent.
fix(device_operations): normalize string-digit IDs in dict-form CF values
_describe_existing_librenms_link() used strict isinstance(int) checks
for the new dict-form {"id": ...} path, so string-digit values like
{"id": "42"} were silently treated as unlinked. Switch to
coerce_librenms_id() which handles str->int, plus reject <= 0.
fix(device_operations): add serial_role_choice_available default to result schema
fix(actions): add auto_create_ipam to SaveUserPrefView.ALLOWED_PREFS
JS savePref('auto_create_ipam', ...) always returned 400 because the
backend whitelist didn't include the key.
fix(device_status): guard paired_oob_id int() coerce against malformed data
fix(template): use migrated_to_marker.server_key for move-to-winner action
fix(templates): propagate auto-create-ipam toggle in mapping form HTMX posts
fix(utils): reject non-positive IDs in coerce_librenms_id()
fix(utils): validate oob_device_id before storing in set_librenms_oob_device_id
test: fix docstring for test_serial_match_diff_hostname_offers_role_choice
There was a problem hiding this comment.
Pull request overview
This PR expands the NetBox LibreNMS plugin’s import and sync workflows to better handle “host + out-of-band (OOB) controller” scenarios, adds bulk-import collision detection, and introduces optional IPAM auto-creation for LibreNMS-known IPs. It also adds Stage 2b “migrated donor” actions for incrementally moving interfaces/IPs to the merge winner, plus supporting UI, settings, and documentation updates.
Changes:
- Add OOB linking + detection across import, sync (interfaces/cables/modules), and status tables, including UI badges and role-toggle flows.
- Add Stage 2 device merge flow enhancements: bulk collision blocking and Stage 2b “move-to-winner / transfer IP” endpoints + migrated-mode UI.
- Add IPAM auto-create option (settings + per-import toggle) and supporting import utilities/tests/docs.
Reviewed changes
Copilot reviewed 49 out of 62 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| netbox_librenms_plugin/views/sync/migrate.py | New Stage 2b endpoints to move interfaces/IPs and transfer device IP FKs from donor to winner. |
| netbox_librenms_plugin/views/imports/list.py | Preserves the auto-create IPAM toggle state when loading cached background-job results. |
| netbox_librenms_plugin/views/base/modules_view.py | Merges OOB controller inventory into module sync and tags rows with _source. |
| netbox_librenms_plugin/views/base/librenms_sync_view.py | Adds migrated-donor context (migrated_to_marker/migrated_to_winner) to sync pages. |
| netbox_librenms_plugin/views/base/interfaces_view.py | Merges OOB ports into interface sync, tags _source, and flags shared-LOM MAC conflicts. |
| netbox_librenms_plugin/views/base/cables_view.py | Merges OOB LLDP links into cable sync and tags _source. |
| netbox_librenms_plugin/views/init.py | Re-exports newly added import/sync views. |
| netbox_librenms_plugin/utils.py | Adds OOB helpers, migrated marker helpers, dict-with-id parsing, collision-resistant find, and auto-create IPAM resolver. |
| netbox_librenms_plugin/urls.py | Adds routes for OOB attach/promote/merge and Stage 2b move/transfer endpoints. |
| netbox_librenms_plugin/tests/test_migrate_views.py | New tests for migrated marker + Stage 2b per-row move/transfer endpoints. |
| netbox_librenms_plugin/tests/test_librenms_id.py | Extends librenms_id tests for dict-with-id/OOB and merge/migrate helpers. |
| netbox_librenms_plugin/tests/test_ip_helpers.py | New tests for IPAM auto-create helper behavior. |
| netbox_librenms_plugin/tests/test_import_utils.py | Updates serial-match behavior expectations to the new host/OOB role choice flow. |
| netbox_librenms_plugin/tests/test_coverage_list.py | Updates coverage tests for _load_job_results(..., request=...) signature. |
| netbox_librenms_plugin/tests/test_coverage_device_operations.py | Adds coverage for OOB detection + merge-candidate detection paths. |
| netbox_librenms_plugin/tests/test_coverage_actions.py | Updates action tests for new error statuses and adds collision + sentinel regression tests. |
| netbox_librenms_plugin/tests/test_collisions.py | New unit tests for bulk collision grouping logic. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/settings.html | Adds plugin setting toggle for auto_create_ipam_default. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html | Adds migrated-donor warning banner and transfer-IP buttons. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.html | Adds import-page “Auto-create IPAM” toggle and includes it in bulk actions. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html | Major import modal updates: OOB/host role toggle, merge UI, mapping shortcuts, and new fragments. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.html | Removes message-container wipe; relies on view-level OOB message attachment. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.html | New modal fragment to block bulk import on NetBox-device collisions. |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html | New reusable platform-mapping typeahead form (used in validation modal). |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html | New reusable device-type-mapping typeahead form (used in validation modal). |
| netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html | Adds per-row “Move to winner” action column in migrated-donor mode. |
| netbox_librenms_plugin/tables/modules.py | Adds OOB badge rendering in module inventory rows. |
| netbox_librenms_plugin/tables/interfaces.py | Adds OOB + Shared LOM badges to interface names. |
| netbox_librenms_plugin/tables/device_status.py | Refines action button rendering for OOB-linked/paired host rows and OOB candidates. |
| netbox_librenms_plugin/tables/cables.py | Adds OOB badge rendering for local port rows. |
| netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js | Persists auto-create IPAM preference and supports in-place validation modal refresh + nested modal handling. |
| netbox_librenms_plugin/models.py | Adds auto_create_ipam_default setting field. |
| netbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.py | Migration for the new auto_create_ipam_default setting. |
| netbox_librenms_plugin/librenms_api.py | Reuses shared coerce_librenms_id() for ID normalization. |
| netbox_librenms_plugin/import_utils/vm_operations.py | Adds optional IPAM auto-create during VM import (best-effort, no primary assignment). |
| netbox_librenms_plugin/import_utils/ip_helpers.py | New helper for get-or-create of global-scope /32 or /128 IPs with opt-in. |
| netbox_librenms_plugin/import_utils/device_operations.py | Adds OOB detection/role selection + merge-candidate detection and existing link description. |
| netbox_librenms_plugin/import_utils/collisions.py | New collision grouping logic for bulk import confirmation. |
| netbox_librenms_plugin/import_utils/bulk_import.py | Propagates created_ips into bulk import success results. |
| netbox_librenms_plugin/import_utils/init.py | Re-exports IP helpers and collision detector. |
| netbox_librenms_plugin/forms.py | Adds auto_create_ipam_default form field to settings UI. |
| netbox_librenms_plugin/constants.py | Adds OOB detection constants and normalize_oob_type(). |
| docs/usage_tips/README.md | Links new mapping/module sync docs. |
| docs/usage_tips/module_sync.md | New user guide for module/inventory sync. |
| docs/usage_tips/mapping_rules.md | New mapping rules guide (platform/device type/module types/bays/etc.). |
| docs/README.md | Highlights module sync and platform mapping documentation. |
| docs/librenms_import/validation.md | Updates validation docs to reference mapping rules and platform mapping behavior. |
| docs/feature_list.md | Expands feature list to include mapping rules + module sync capabilities. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
netbox_librenms_plugin/views/sync/migrate.py (2)
151-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLock and narrow the
Interfacewrite.Only the
Devicerows are locked here; theInterfacerow is still a stale in-memory object, andinterface.save()writes every column. A concurrent rename or field edit can be silently overwritten even though this action only intends to changedevice_id.🔧 Suggested fix
with transaction.atomic(): # Lock both devices in pk order to avoid cross-merge deadlocks. ordered = sorted({donor.pk, winner.pk}) - list(Device.objects.select_for_update().filter(pk__in=ordered).order_by("pk")) + locked = { + d.pk: d for d in Device.objects.select_for_update().filter(pk__in=ordered).order_by("pk") + } + donor = locked[donor.pk] + winner = locked[winner.pk] + interface = Interface.objects.select_for_update().get(pk=interface.pk) # Re-check under the lock to close the TOCTOU window. if Interface.objects.filter(device=winner, name=interface.name).exists(): return self._fail( request, f"Winner device '{winner.name}' already has an interface named '{interface.name}'. " "Rename or remove the existing interface first.", status=409, ) interface.device = winner - interface.save() + interface.save(update_fields=["device"])🤖 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 `@netbox_librenms_plugin/views/sync/migrate.py` around lines 151 - 164, You're only locking Device rows but then calling interface.save() which will overwrite all Interface columns from a stale in-memory instance; instead, re-fetch and lock the specific Interface row (e.g., with Interface.objects.select_for_update().get(pk=interface.pk) inside the same transaction) or perform an atomic queryset update to only change device_id (e.g., Interface.objects.filter(pk=interface.pk).update(device=winner)) so concurrent edits to other fields aren't clobbered; keep the existence check (Interface.objects.filter(device=winner, name=interface.name).exists()) under the same transaction/lock and return via _fail as before.
55-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't hard-code
"default"as the fallback server key.If the POST omits
server_key, all three actions resolve_migrated_toin the"default"namespace, so migrations started from a non-default LibreNMS server can incorrectly fail with “Donor device is not marked as migrated.” Fall back to the active API server key instead of a literal default.🔧 Suggested fix
-def _server_key_from_request(request, default="default"): +def _server_key_from_request(request, default): """Extract the LibreNMS server key from the POST body (form field).""" sk = request.POST.get("server_key") or default return sk if isinstance(sk, str) and sk else default- server_key = _server_key_from_request(request) + server_key = _server_key_from_request(request, self.librenms_api.server_key)Based on learnings: "In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key ..."
🤖 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 `@netbox_librenms_plugin/views/sync/migrate.py` around lines 55 - 58, The helper _server_key_from_request currently hard-codes "default" as the fallback which breaks namespace scoping; change _server_key_from_request to take a default_server_key (or default=None) parameter instead of the literal "default", use that provided default when request.POST lacks a non-empty server_key, and update callers (e.g. the migration views/actions that call _server_key_from_request) to pass self.librenms_api.server_key as the default_server_key so the active API server key is used for cache namespace scoping.
🤖 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 `@netbox_librenms_plugin/constants.py`:
- Around line 11-12: OOB_TYPES contains "oob" but OOB_TYPE_PATTERN won't match
it, so normalize_oob_type() can never return "oob"; update the regex
OOB_TYPE_PATTERN to include "oob" (e.g., add "oob" to the alternation) so that
inputs matching "oob" are normalized, or alternatively remove "oob" from
OOB_TYPES if you deliberately don't want to normalize that value; adjust
OOB_TYPE_PATTERN and verify normalize_oob_type() returns values from OOB_TYPES
accordingly.
In `@netbox_librenms_plugin/utils.py`:
- Around line 1198-1209: The code only sets winner_entry["oob"] when
OOB_TYPE_PATTERN matches donor.name, dropping donor_id when no vendor-specific
type is found; update the logic in the block that calls coerce_librenms_id
(symbols: coerce_librenms_id, OOB_TYPE_PATTERN, winner_entry["oob"], donor.name)
so that if OOB_TYPE_PATTERN.search(donor.name or "") returns no match you
fallback to inferred_type = "oob" (lowercase) and still create demoted = {"id":
_coerced_donor, "type": inferred_type} and assign winner_entry["oob"] = demoted,
preserving the donor LibreNMS id as a generic oob link.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 317-322: The current computation of main_max_idx/_OOB_OFFSET uses
only inventory_data but _merge_transceiver_data() may append synthetic rows with
entity_physical_index values, causing OOB remapped indices to collide with those
synthetic main indices; to fix, compute the max index after merging by calling
_merge_transceiver_data(inventory_data, ...) (or otherwise obtaining the merged
list) and then compute main_max_idx = max(... for item in merged_inventory) and
set _OOB_OFFSET from that value so the OOB namespace is always above both real
and synthetic main transceiver indices (references: main_max_idx, _OOB_OFFSET,
_merge_transceiver_data, inventory_data, entity_physical_index, index_map).
In `@netbox_librenms_plugin/views/sync/migrate.py`:
- Around line 228-241: The IPAddress row (`ip`) must be re-locked and
re-validated inside the existing transaction to avoid overwriting concurrent
updates: inside the transaction.atomic() block, re-fetch the IPAddress using
select_for_update() (e.g. filter(pk=ip.pk).select_for_update().first()), confirm
its current assigned_object still refers to the donor/interface (compare to the
donor's Interface or `assigned`), and if it changed return self._fail(...) with
an appropriate 409; if still valid, update only the assignment columns (set
assigned_object to `winner_iface`) and call save(update_fields=[...]) so other
fields are not clobbered. Ensure you reference `ip`, `IPAddress`, `Interface`,
`assigned`, `donor`, `winner`, and `_fail` when locating and modifying the code.
---
Outside diff comments:
In `@netbox_librenms_plugin/views/sync/migrate.py`:
- Around line 151-164: You're only locking Device rows but then calling
interface.save() which will overwrite all Interface columns from a stale
in-memory instance; instead, re-fetch and lock the specific Interface row (e.g.,
with Interface.objects.select_for_update().get(pk=interface.pk) inside the same
transaction) or perform an atomic queryset update to only change device_id
(e.g., Interface.objects.filter(pk=interface.pk).update(device=winner)) so
concurrent edits to other fields aren't clobbered; keep the existence check
(Interface.objects.filter(device=winner, name=interface.name).exists()) under
the same transaction/lock and return via _fail as before.
- Around line 55-58: The helper _server_key_from_request currently hard-codes
"default" as the fallback which breaks namespace scoping; change
_server_key_from_request to take a default_server_key (or default=None)
parameter instead of the literal "default", use that provided default when
request.POST lacks a non-empty server_key, and update callers (e.g. the
migration views/actions that call _server_key_from_request) to pass
self.librenms_api.server_key as the default_server_key so the active API server
key is used for cache namespace scoping.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e393373-a59a-49c0-bbc9-b9bbba6c23b1
📒 Files selected for processing (18)
docs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/sync/migrate.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (python)
- GitHub Check: test-netbox (3.12)
- GitHub Check: test-netbox (3.14)
- GitHub Check: test-netbox (3.13)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.py
netbox_librenms_plugin/templates/**/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/templates/**/*.html: HTMX 2.x is the primary async layer. Table row updates should return<tr hx-swap-oob="true">.
AvoidouterHTMLswaps in HTMX; use OOB or targetedinnerHTMLswaps to keep table layout intact.
Do not reintroducedata-bs-toggleor duplicate modal IDs in modal implementation.
Keep<select class="device-role-select">markup stable to preserve JavaScript hook-up for TomSelect decorators.
Do not re-addtable-responsivewrappers as their removal was deliberate to prevent dropdown clipping.
Templates live intemplates/netbox_librenms_plugin/; reuse and includes go underinc/subdirectory.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/**/*.{html,css}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
Styling assumes Tabler defaults for the netbox_librenms_plugin frontend.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/*.html
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
HTMX fragments live in
templates/netbox_librenms_plugin/htmx/including:device_import_row.html,device_validation_details.html,device_vc_details.html,bulk_import_confirm.html.
Files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/modules_view.py
**/tables/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Table classes in
tables/must useToggleColumn(attrs={'input': {'name': 'select'}})for selection, accept contextual parameters in constructors (e.g.,device,interface_name_field,vlan_groups), setself.tabandself.prefixfor multi-table pagination, includedata-*attributes in row attrs, and VLAN columns must userender_vlans()with hidden inputs and JSON data.
Files:
netbox_librenms_plugin/tables/device_status.py
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/migrate.py
**/import_utils/device_operations.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
device_operations.pymust export:validate_device_for_import(device, ...)andbulk_import_devices_shared(devices, user, ...)
Files:
netbox_librenms_plugin/import_utils/device_operations.py
**/views/imports/**
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/**: NetBox's/api/core/background-tasks/endpoint requires superuser (IsSuperuserinBaseRQViewSet). Non-superuser users cannot poll job status (403 Forbidden). Plugin automatically falls back to synchronous mode for non-superusers viashould_use_background_job()inlist.pyandactions.py
Import page filter fields:librenms_location,librenms_type,librenms_os,librenms_hostname,librenms_sysname,librenms_hardware,enable_vc_detection,show_disabled,exclude_existing
Files:
netbox_librenms_plugin/views/imports/actions.py
**/views/imports/actions.py
📄 CodeRabbit inference engine (.github/instructions/background-jobs.instructions.md)
**/views/imports/actions.py:DeviceImportHelperMixinprovidesget_validated_device_with_selections()andrender_device_row()for HTMX row rendering, shared by update views
BulkImportConfirmView(POST) — renders confirmation modal with selected device list viahtmx/bulk_import_confirm.html
BulkImportDevicesView(POST) — executes import. Background mode enqueuesImportDevicesJob; sync mode callsbulk_import_devices()+bulk_import_vms()and returns OOB row swaps withHX-Trigger: closeModal
DeviceValidationDetailsView(GET) — renders expandable validation details viahtmx/device_validation_details.html
DeviceVCDetailsView(GET) — renders VC member details viahtmx/device_vc_details.html
DeviceRoleUpdateView,DeviceClusterUpdateView,DeviceRackUpdateView(POST) — per-device dropdown updates. Apply selection to validation state and return re-rendered row viarender_device_row()
Files:
netbox_librenms_plugin/views/imports/actions.py
🧠 Learnings (23)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-13T11:16:36.294Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:136-137
Timestamp: 2026-03-13T11:16:36.294Z
Learning: In Django templates under netbox_librenms_plugin/templates/**/*.html, do not suggest adding explicit parentheses to {% if %} expressions for readability. The project favors compact expressions using implicit operator precedence (and binds tighter than or). Treat parentheses as cosmetic and avoid guidance to insert them for style reasons.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-05-01T08:25:06.260Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html:245-246
Timestamp: 2026-05-01T08:25:06.260Z
Learning: In netbox_librenms_plugin template HTML/HTMX code, only require an X-CSRFToken header for state-changing requests made via fetch() or HTMX (POST, PUT, PATCH, DELETE). Do not require X-CSRFToken on read-only fetch() GET calls (e.g., autocomplete/lookup endpoints like dcim-api:devicetype-list); Django/DRF exempt GET requests from CSRF validation. Therefore, code reviews should not flag missing CSRF headers on GET fetch() calls used for lookups/autocomplete.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-03-13T20:03:16.435Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api2.py:319-355
Timestamp: 2026-03-13T20:03:16.435Z
Learning: Do not propose replacing server_info with module_sync.server_key in the templates located under netbox_librenms_plugin/templates/netbox_librenms_plugin (specifically _module_sync.html and inc/_module_sync.html). These templates rely on server_info being present in the parent template context (librenms_sync_base.html) and server_key may be absent on initial load. Treat the correct usage of server_info for populating the value as the intended pattern; only flag issues if server_key is incorrectly used in these templates. This guideline applies to all files under the templates path for this plugin.
Applied to files:
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html
📚 Learning: 2026-03-08T14:17:28.826Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/import_utils/virtual_chassis.py:73-80
Timestamp: 2026-03-08T14:17:28.826Z
Learning: In Python code, when a lookup returns None (including API failures), implement negative caching by storing an empty result with a configurable TTL (default 5 minutes). Document the TTL and ensure a force_refresh=True bypasses the cache for manual re-fetch actions. Do not treat caching None/empty results on API failure as a bug if this mirrors existing patterns (e.g., get_device_with_server caching None on not-found). Apply this guidance to files within netbox_librenms_plugin/import_utils where similar inventory/API lookups occur.
Applied to files:
netbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-03-09T20:10:48.502Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/import_utils/device_operations.py:496-575
Timestamp: 2026-03-09T20:10:48.502Z
Learning: In netbox_librenms_plugin/import_utils/device_operations.py, in validate_device_for_import(), ensure both the cluster-required blocker (VM path) and the device_role-required blocker (device path) are guarded with if not result.get('existing_device') to ensure create-time prerequisites are only appended for new imports, not for link/update flows; keep available_roles and available_clusters populated for both new and existing-device cases so UI dropdowns function correctly on update views.
Applied to files:
netbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-05-22T19:37:46.167Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_import_utils.py:1763-1766
Timestamp: 2026-05-22T19:37:46.167Z
Learning: In `validate_device_for_import` (device operations / serial-diff name-resolution logic), preserve the following result contract so the UI/test expectations remain stable:
- If `serial_action` is determined via a serial match AND the existing device has NO OOB/LibreNMS link, set `serial_action` to `"oob_candidate"` and ensure `promote_to_host` is NOT present in the returned dict.
- Populate `promote_to_host` only when the existing device already has an OOB/LibreNMS link (i.e., a host id is available to inherit from); otherwise omit the key.
- Always include `serial_role_choice_available` in the returned dict, defaulting to `False` (baseline) even when other resolution outcomes do not enable it.
Applied to files:
netbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_librenms_id.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_librenms_id.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_librenms_id.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/imports/actions.py
📚 Learning: 2026-03-08T14:23:14.395Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tables/modules.py:0-0
Timestamp: 2026-03-08T14:23:14.395Z
Learning: In Python/Django code, avoid wrapping a list already containing SafeString values (produced by format_html) with format_html("{}", mark_safe(...)). This is redundant and can raise Django 6.0 deprecation warnings. Instead, concatenate the strings directly and wrap once, e.g. use mark_safe("".join(str(b) for b in buttons)) and avoid nested format_html calls. Apply this pattern to files under netbox_librenms_plugin/tables/ (any .py files) to ensure SafeString handling remains explicit and compatible with Django 6.0.
Applied to files:
netbox_librenms_plugin/tables/device_status.py
📚 Learning: 2026-03-07T10:32:06.242Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:107-117
Timestamp: 2026-03-07T10:32:06.242Z
Learning: In netbox_librenms_plugin/utils.py, keep the Priority 1 loop to guard against None and bool values when accessing raw_cf.get(server_key). Do not replace the Priority 1 condition with a full get_librenms_device_id call. The two-pass design is intentional: Priority 1 performs quick sanity checks, while Priority 2 handles string normalization and full validation by calling get_librenms_device_id(member, server_key, auto_save=False).
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-07T22:38:43.110Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:552-584
Timestamp: 2026-03-07T22:38:43.110Z
Learning: In netbox_librenms_plugin/utils.py, do not propose replacing 'obj.custom_field_data.get("librenms_id") or {}' with a None check. The code intentionally uses 'or {}' to handle falsey values; downstream type guards treat them equivalently since LibreNMS IDs start at 1, making 0 equivalent to 'not set'. Do not modify this logic; keep the existing behavior for all falsey values.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-08T08:55:46.317Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/utils.py:532-595
Timestamp: 2026-03-08T08:55:46.317Z
Learning: In netbox_librenms_plugin/utils.py, do not modify set_librenms_device_id to call obj.save(). It is mutator-only and should only update in-memory obj.custom_field_data[...] without persisting. Ensure callers perform persistence: after mutation, run full_clean() and then save() (as seen in device_operations.py around lines ~864-866) or explicit obj.save() after set_librenms_device_id (as in librenms_api.py around lines ~261-262). This pattern prevents coupling mutation with persistence and preserves validation in between.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-09T19:15:13.104Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/utils.py:249-267
Timestamp: 2026-03-09T19:15:13.104Z
Learning: In netbox_librenms_plugin/utils.py, ensure match_librenms_hardware_to_device_type returns None when DeviceTypeMapping.MultipleObjectsReturned is raised (fail-closed per inline comment). Callers must guard for result is None separately from the normal result check (e.g., if result is None: handle; elif result.get('matched'): ... ). Note that the success path uses match_type='mapping' (not 'exact'), distinguishing it from standard part_number/model exact lookups. Consider adding a unit test that asserts None is returned on MultipleObjectsReturned and that callers properly handle both None and dict results.
Applied to files:
netbox_librenms_plugin/utils.py
📚 Learning: 2026-03-09T10:39:37.846Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:1039-1047
Timestamp: 2026-03-09T10:39:37.846Z
Learning: In netbox_librenms_plugin/tests/test_coverage_device_operations.py, fix test_no_hostname_adds_issue in both TestValidateDeviceForImportEdgeCases and TestValidateDeviceMoreEdgeCases. Do not patch _determine_device_name. Instead, call validate_device_for_import with sysName="" and hostname="" and assert that "no hostname" is NOT present in result.get("issues", []) because the fallback name (device-{id}) is used when both inputs are empty. Ensure the test directly verifies the absence of the blocker when inputs are empty, relying on the actual fallback behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_device_operations.py
📚 Learning: 2026-03-07T13:12:59.182Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/device_fields.py:445-448
Timestamp: 2026-03-07T13:12:59.182Z
Learning: Across netbox_librenms_plugin/views/sync/*.py, verify redirects and URL rewrites do not append ?server_key=. The active server context is determined from the global setting (settings.selected_server) via BaseLibreNMSSyncView.get(), so request.GET server_key is not used to preserve context. Ensure RemoveServerMappingView and ConvertLegacyLibreNMSIdView (and similar views) rely on the global setting instead of propagating server_key in redirects.
Applied to files:
netbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-03-08T09:30:45.499Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/views/imports/actions.py:1031-1035
Timestamp: 2026-03-08T09:30:45.499Z
Learning: In netbox_librenms_plugin/views/imports/actions.py, ensure that DeviceConflictActionView.post() explicitly rejects boolean values for librenms_id via isinstance(librenms_id, bool) before coercing to int, returning HTTP 400. Do not remove or consolidate this pre-coercion boolean check. This guard is intentional and consistent with the similar bool-guard pattern used in set_librenms_device_id, get_librenms_device_id, and find_by_librenms_id; preserve this behavior to avoid ambiguity and misinterpretation of truthy/falsey booleans.
Applied to files:
netbox_librenms_plugin/views/imports/actions.py
🔇 Additional comments (19)
netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.html (1)
12-12: LGTM!Also applies to: 34-38, 45-45, 53-53, 62-65, 81-81
docs/usage_tips/module_sync.md (1)
38-38: LGTM!netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.html (1)
14-14: LGTM!Also applies to: 36-40, 47-47, 55-55, 64-67, 83-83
netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html (1)
343-343: LGTM!netbox_librenms_plugin/import_utils/collisions.py (1)
84-84: LGTM!Also applies to: 89-94
netbox_librenms_plugin/tests/test_import_utils.py (1)
1733-1738: LGTM!netbox_librenms_plugin/tests/test_coverage_actions.py (1)
4191-4320: LGTM!netbox_librenms_plugin/tests/test_collisions.py (1)
6-184: LGTM!netbox_librenms_plugin/tests/test_migrate_views.py (1)
261-266: LGTM!Also applies to: 323-327
netbox_librenms_plugin/views/base/modules_view.py (1)
50-60: LGTM!netbox_librenms_plugin/tables/device_status.py (1)
532-538: LGTM!netbox_librenms_plugin/utils.py (1)
253-301: LGTM!Also applies to: 658-675, 975-989, 1005-1005, 1128-1197
netbox_librenms_plugin/tests/test_coverage_device_operations.py (1)
1956-1961: LGTM!netbox_librenms_plugin/views/sync/migrate.py (1)
30-52: LGTM!Also applies to: 306-327
netbox_librenms_plugin/tests/test_librenms_id.py (1)
564-595: LGTM!Also applies to: 662-675, 731-751
netbox_librenms_plugin/import_utils/device_operations.py (1)
63-74: LGTM!Also applies to: 261-265, 575-579
netbox_librenms_plugin/views/imports/actions.py (3)
573-582: LGTM!
2305-2328: LGTM!
2574-2574: LGTM!
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
netbox_librenms_plugin/import_utils/device_operations.py (1)
382-385:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize the stored OOB id before refining to
librenms_oob.This branch only recognizes an OOB match when the stored nested id has the same runtime type as
librenms_id. A value like{"oob": {"id": "42"}}will still match viafind_by_librenms_id()and then fall through as"librenms_id"here, which renders the wrong UI path.🔧 Minimal fix
- if _existing_oob and _existing_oob.get("id") == librenms_id: + if _existing_oob and coerce_librenms_id(_existing_oob.get("id")) == coerce_librenms_id(librenms_id): result["existing_match_type"] = "librenms_oob"🤖 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 `@netbox_librenms_plugin/import_utils/device_operations.py` around lines 382 - 385, The existing check in device_operations.py using get_librenms_oob(existing_device, server_key=server_key) only matches when the stored nested id and librenms_id share the same runtime type; normalize both IDs (e.g., cast to string or perform a safe int cast) before comparison so values like {"oob": {"id": "42"}} correctly match; update the conditional around _existing_oob.get("id") == librenms_id (used to set result["existing_match_type"] = "librenms_oob") to compare normalized values (reference symbols: get_librenms_oob, existing_device, server_key, librenms_id, result["existing_match_type"]) and ensure the normalization handles None/invalid values safely.
🤖 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 `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 263-265: The response payload currently always includes the
"promote_to_host" key set to None which changes the validation shape; change the
logic that builds the result dict in device_operations.py so that
"promote_to_host" is only added when an existing LibreNMS host link can be
inherited (i.e., when the existing device's link info contains a host id — the
same condition you use to detect an existing_librenms_link / existing_libre_id);
otherwise do not include the "promote_to_host" key at all and leave
"oob_candidate" and "existing_librenms_link" handling unchanged.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.html`:
- Line 119: The HTMX form omits unchecked checkboxes (so auto_create_ipam is
never sent when off); to fix, add an explicit hidden input with the same name
and a false/off value immediately before the corresponding checkbox(s) so a
value is always submitted (e.g. add <input type="hidden" name="auto_create_ipam"
value="off"> immediately before the checkbox with id "auto-create-ipam-toggle");
apply the same pattern for the other toggles referenced (use-sysname-toggle,
strip-domain-toggle, etc.) or alternatively normalize missing keys in the view,
but the preferred fix is the hidden-input-before-checkbox change in the template
so HTMX posts an explicit false when unchecked.
In `@netbox_librenms_plugin/utils.py`:
- Around line 996-1003: The code drops numeric-string host IDs (e.g., "42") when
preparing the CF entry; detect and preserve numeric strings by treating them
like ints: add a branch that if entry is a str and entry.isdigit() then set
entry = {"id": int(entry)}, keep the existing dict branch (entry = dict(entry))
so shapes like {"default": "42"} are not reset to {}, and only fall back to {}
for truly non-numeric/non-dict values; update the logic around the
cf_value.get(server_key) handling (the variable entry and the surrounding
branch) so get_librenms_device_id, find_by_librenms_id and set_librenms_oob see
preserved numeric IDs.
- Around line 1168-1180: The normalization misses server IDs represented as
string or as dicts like {"default":"99"} so those get treated as empty; update
the handling of winner_entry and donor_entry (lookups from winner_cf/donor_cf
using server_key) to also normalize string numeric values and dicts with a
"default" key by converting them into {"id": int(value)} (or {"id": value} if
non-numeric string is expected) before the existing dict/int branches, and apply
the same normalization logic for both winner_entry and donor_entry so merges do
not lose valid IDs.
In `@netbox_librenms_plugin/views/__init__.py`:
- Around line 111-117: The F401 noqa is on the closing parenthesis so flake8
still flags the re-exports; move the "# noqa: F401" to the actual import
statement line (for example immediately after "from .imports.actions import (")
or add "# noqa: F401" on each imported name line (AddAsOOBView,
AddDeviceTypeMappingView, AddPlatformMappingView, MergeNetBoxDevicesView,
PromoteToHostView) so the unused-import ignore applies to those re-exported
symbols in netbox_librenms_plugin/views/__init__.py.
In `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 113-133: The merged LLDP entries appended to links_data include a
"_source" key (see links_data entries added after get_librenms_oob and
self.librenms_api.get_device_links) but later cache re-enrichment/sanitization
strips unknown keys and drops "_source"; update the cache
re-enrichment/sanitization path to preserve or reapply the "_source" field for
cached links (or whitelist "_source" in the sanitization logic) so that entries
coming from OOB vs main remain source-aware after cache round-trips.
In `@netbox_librenms_plugin/views/imports/actions.py`:
- Around line 1930-1945: existing_mapping is checked before the transaction but
get_or_create() may return an object created concurrently, so after
get_or_create() you must re-check the change permission before mutating an
existing record: after mapping, created =
PlatformMapping.objects.get_or_create(...) and before assigning
mapping.netbox_platform or calling mapping.save(), call the permission check
(via self.require_object_permissions("POST") or an explicit check for ("change",
PlatformMapping)) and abort/return the error if present so callers who only
passed the initial "add" gate cannot update an existing mapping.
In `@netbox_librenms_plugin/views/sync/migrate.py`:
- Around line 265-270: The code assumes Device.objects.select_for_update()
returns both donor and winner and indexes locked[...] directly; instead, after
building locked = {d.pk: d ...}, verify that both donor.pk and winner.pk are
present (e.g. if not set(ordered) <= set(locked.keys()) or by using
locked.get(pk) and checking for None) and handle the missing case by returning
the same HTTP 410/409 response flow used elsewhere rather than letting a
KeyError propagate; apply the same presence-check + response logic to the other
similar block around the second select_for_update() call (the block related to
_resolve_winner_for_donor and the ip =
IPAddress.objects.select_for_update().get(...) sequence).
- Around line 192-207: The Interface instance was loaded before the transaction
so re-fetch it under the same lock and validate ownership before updating:
inside the transaction.atomic() block, after locking devices
(Device.objects.select_for_update()) call
Interface.objects.select_for_update().filter(pk=interface.pk) to re-retrieve the
row, check that the re-fetched interface exists and its device == donor (handle
missing or different owner by returning the same failure response), and only
then perform the update (e.g. update(device=winner)) so you don’t move a
stale/deleted/reattached row.
---
Duplicate comments:
In `@netbox_librenms_plugin/import_utils/device_operations.py`:
- Around line 382-385: The existing check in device_operations.py using
get_librenms_oob(existing_device, server_key=server_key) only matches when the
stored nested id and librenms_id share the same runtime type; normalize both IDs
(e.g., cast to string or perform a safe int cast) before comparison so values
like {"oob": {"id": "42"}} correctly match; update the conditional around
_existing_oob.get("id") == librenms_id (used to set
result["existing_match_type"] = "librenms_oob") to compare normalized values
(reference symbols: get_librenms_oob, existing_device, server_key, librenms_id,
result["existing_match_type"]) and ensure the normalization handles None/invalid
values safely.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3e234ae2-4712-4c4a-b353-03652fcf6e1f
⛔ Files ignored due to path filters (13)
docs/img/Netbox-librenms-plugin-device-sync-fields.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-import-page.pngis excluded by!**/*.pngdocs/img/Netbox-librenms-plugin-module-sync-tab.pngis excluded by!**/*.pngdocs/img/carrier_auto_install_rules/list.pngis excluded by!**/*.pngdocs/img/device_type_mappings/list.pngis excluded by!**/*.pngdocs/img/inventory_ignore_rules/list.pngis excluded by!**/*.pngdocs/img/module_bay_mappings/list.pngis excluded by!**/*.pngdocs/img/module_type_mappings/add.pngis excluded by!**/*.pngdocs/img/module_type_mappings/list.pngis excluded by!**/*.pngdocs/img/normalization_rules/add.pngis excluded by!**/*.pngdocs/img/normalization_rules/list.pngis excluded by!**/*.pngdocs/img/platform_mappings/add.pngis excluded by!**/*.pngdocs/img/platform_mappings/list.pngis excluded by!**/*.png
📒 Files selected for processing (49)
docs/README.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/README.mddocs/usage_tips/mapping_rules.mddocs/usage_tips/module_sync.mdnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/forms.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_utils/ip_helpers.pynetbox_librenms_plugin/import_utils/vm_operations.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/migrations/0011_librenmssettings_auto_create_ipam_default.pynetbox_librenms_plugin/models.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_import.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/settings.htmlnetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_list.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_ip_helpers.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/imports/list.pynetbox_librenms_plugin/views/sync/migrate.py
marcinpsk
commented
May 23, 2026
marcinpsk
added a commit
that referenced
this pull request
May 23, 2026
…ixes Phase 3 — AddAsOOBView, PromoteToHostView, MergeNetBoxDevicesView actions and the full UI (import validation modal, OOB badge, promote button). Phase 4 — Stage-2 'Move to winner' views: MoveInterfaceToWinnerView, MoveIPAddressToWinnerView, TransferDeviceIPView (migrate.py). Utilities — set_librenms_oob, clear_librenms_oob, get_librenms_oob, merge_librenms_links, mark_librenms_migrated, coerce_librenms_id, get_migrated_to_marker, detect_bulk_collisions, validate_device_for_import OOB/promote/host-serial path. Bug fixes addressed in PR #79 review (batches 1-5): - Accept generic 'oob' sentinel in set_librenms_oob - String-ID branches in set_librenms_oob and merge_librenms_links - Conditional filter(device=donor) guard on interface queryset update - locked.get() guards in MoveIPAddressToWinnerView and TransferDeviceIPView - _fail() unified OOB toast (HTTP 200 + HX-Reswap:none) across all views - _server_key_from_request default_server_key parameter rename - promote_to_host key omitted from default result dict when not applicable - noqa:F401 moved to from-line in views/__init__.py - _source preserved in cables_view._raw_keys on both strip paths - AddPlatformMappingView concurrent get_or_create permission re-check - Toggle checkboxes wrapped in span containers with hidden off inputs - TOCTOU race guards in conflict checks before atomic writes - entPhysicalIndex OOB offset changed from hardcoded 1_000_000 to dynamic - Bulk-import collision modal rendered at 200 not 409 - Various test fixes for new mock patterns and db access markers
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
…directs Two module-sync handlers compute server_key = POST or self.librenms_api.server_key but called _modules_redirect_response(request, sync_url) without it on several return sites, so the helper fell back to POST/GET only and dropped the active-server context when the POST field was absent. Pass the computed server_key at every return site in those handlers.
…in a nested modal Bootstrap can strip .show from a nested modal before the keydown listener runs, so the '.modal.show' guard missed and Escape tore down the outer validation modal. Also gate on the event origin (event.target.closest of a nested modal), which is unaffected by that timing.
The test used oob_type='idrac' (no chars needing escaping), so it would pass even if render_actions() emitted the value unescaped. Use '<idrac>' and assert the escaped form is present and the raw value absent.
…failure An OOB-only mapping has no host librenms_id, so the host get_device_links() call always records _links_fetch_error even though no host fetch was meaningfully attempted. When the OOB controller validly returned no links, the empty-result guard mislabeled that as a failure and returned None, so _prepare_context() skipped caching the empty snapshot and stale OOB cable rows lingered after a genuine empty refresh. Return [] when the only reason for the recorded error is the absent host mapping on an OOB-scoped device.
…s to existing
When a previously-unmatched bulk-import row later resolves to an existing object,
its create-time blockers ("Device role must be..." / "Cluster must be...") are
stale — validate_device_for_import() only adds them when there's no existing_device.
The VM path cleared neither (both branches were gated on `not actual_is_vm`), and a
cross-model match can carry the other model's blocker, so a stale message lingered
in the UI. Clear both unconditionally once existing_device is set, before recalc.
Also docs: list `idrac` alongside `drac` in the OOB-type list (constants.OOB_TYPES
treats them as distinct canonical tokens).
Test: test_fresh_lookup_vm_clears_stale_cluster_blocker (real-DB) — a VM row resolves
by name match (actual_is_vm=True) and the cluster blocker is gone; verified red→green.
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 (1)
netbox_librenms_plugin/views/base/cables_view.py (1)
221-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly cache an empty OOB-only refresh after the OOB fetch succeeded.
Line 272 treats any configured OOB ID as a valid empty refresh. If the OOB controller fetch fails, or Line 229 returns early for malformed OOB
links,_prepare_context()caches[]and clears stale cable rows even though no OOB snapshot was successfully fetched.Prevent failed OOB-only refreshes from overwriting the cache
oob = get_librenms_oob(lookup_device, server_key=server_key) + oob_links_valid = False if oob and oob.get("id"): oob_success, oob_data = self.librenms_api.get_device_links(oob["id"]) @@ oob_links = oob_data.get("links") if not isinstance(oob_links, list): self._oob_links_fetch_failed = True + if self.librenms_id is None: + self._links_fetch_error = "OOB links fetch returned a malformed payload." logger.warning( "OOB links fetch returned a malformed payload for device %s (OOB id %s): %s", self.librenms_id, oob["id"], oob_data, ) - return links_data + oob_links = [] + else: + oob_links_valid = True for link in oob_links: @@ else: # Don't silently drop OOB cable rows on a fetch failure — flag it so # post() can warn the user (this method has no request to message on). self._oob_links_fetch_failed = True + oob_error = oob_data.get("message") if isinstance(oob_data, dict) else oob_data + if self.librenms_id is None and oob_error: + self._links_fetch_error = str(oob_error) logger.warning( "OOB links fetch failed for device %s (OOB id %s): %s", self.librenms_id, oob["id"], - oob_data.get("message") if isinstance(oob_data, dict) else oob_data, + oob_error, ) @@ - host_mapping_absent_but_oob_scoped = self.librenms_id is None and bool(oob and oob.get("id")) + host_mapping_absent_but_oob_scoped = ( + self.librenms_id is None and bool(oob and oob.get("id")) and oob_links_valid + )Also applies to: 272-273
🤖 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 `@netbox_librenms_plugin/views/base/cables_view.py` around lines 221 - 229, The caching logic at lines 272-273 treats any configured OOB ID as a valid empty refresh without checking whether the OOB fetch actually succeeded. When the OOB fetch fails or returns malformed data (as detected when setting self._oob_links_fetch_failed to True at line 222), the code at line 229 returns early, but the cache is still updated with empty data at lines 272-273. You need to guard the cache update at lines 272-273 to only proceed if the OOB fetch succeeded by checking that self._oob_links_fetch_failed is False before caching the empty OOB-only refresh result. This ensures stale cable rows are only cleared when a successful OOB snapshot was actually fetched.
🤖 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 `@netbox_librenms_plugin/views/base/cables_view.py`:
- Around line 221-229: The caching logic at lines 272-273 treats any configured
OOB ID as a valid empty refresh without checking whether the OOB fetch actually
succeeded. When the OOB fetch fails or returns malformed data (as detected when
setting self._oob_links_fetch_failed to True at line 222), the code at line 229
returns early, but the cache is still updated with empty data at lines 272-273.
You need to guard the cache update at lines 272-273 to only proceed if the OOB
fetch succeeded by checking that self._oob_links_fetch_failed is False before
caching the empty OOB-only refresh result. This ensures stale cable rows are
only cleared when a successful OOB snapshot was actually fetched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 411d6547-f5f4-41f9-9c11-f977c8fee8a5
📒 Files selected for processing (13)
docs/usage_tips/oob_management.mdnetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test-netbox (3.13)
- GitHub Check: test-netbox (3.14)
- GitHub Check: Analyze (python)
- GitHub Check: test-netbox (3.12)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building
HttpResponsefrom Django-template-rendered HTML in views, useformat_html()to compose the envelope andmark_safe()on the inner HTML to clear CodeQLpy/reflected-xssfalse positives. Example:format_html('<div id="target" hx-swap-oob="innerHTML">{}</div>', mark_safe(modal_html))
Files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
**/views/sync/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
Sync action views must follow the pattern: check permissions with
LibreNMSPermissionMixinandNetBoxObjectPermissionMixin, read selected items fromrequest.POST.getlist('select'), load cached data usingCacheMixin.get_cache_key(), apply changes insidetransaction.atomic(), and redirect to the sync tab with?tab=<resource>.
Files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.py
netbox_librenms_plugin/**/*.{html,js}
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
All HTMX requests and
fetch()calls must include a CSRF token. Prefer extracting from hidden form input viadocument.querySelector('[name=csrfmiddlewaretoken]').valuerather than cookie-based approach.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
netbox_librenms_plugin/static/**/*.js
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/static/**/*.js: Modals should try Bootstrap 5 native (bootstrap.Modal) first, falling back to manual DOM manipulation if unavailable. UseshowModal()/hideModal()helper functions.
UseModalManagerclass reference andfilterModalManagerinstance in fetch callbacks; do not use undefinedmodalInstancevariables.
Bind dismiss handlers (backdrop click,data-bs-dismissbuttons) once per element to prevent stacking on repeatedshowModal()calls.
Always checkresponse.okbefore processing fetch responses to catch HTTP errors.
In fetch catch blocks, showerror.messagefor debugging rather than generic messages.
The import filter form uses fetch withAccept: application/json, text/html—JSON for background jobs, HTML for synchronous mode.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
netbox_librenms_plugin/static/**/librenms_import.js
📄 CodeRabbit inference engine (.github/instructions/frontend.instructions.md)
netbox_librenms_plugin/static/**/librenms_import.js:librenms_import.jsshould be wrapped in an IIFE withwindow.LibreNMSImportInitializedguard to prevent re-initialization during HTMX swaps.
ImplementModalManagerclass wrapping Bootstrap 5 modal show/hide with fallback in import page JavaScript.
ImplementpollJobStatus()function that polls/api/core/background-tasks/{jobId}/every 2s, updates progress messages, handles cancel button, and redirects on completion.
ImplementcaptureSelectionState()andrestoreSelectionState()functions to preserve checkbox state across HTMX content swaps.
ImplementcreateCacheCountdown()as a generic countdown timer for cache expiration display.
ImplementinitializeFilterForm()to intercept form submit, detect JSON response (background job), and start polling.
Files:
netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js
**/views/base/**/*.py
📄 CodeRabbit inference engine (.github/instructions/sync.instructions.md)
**/views/base/**/*.py: Base view classes (BaseLibreNMSSyncView,BaseInterfaceTableView,BaseCableTableView,BaseIPAddressTableView,BaseVLANTableView) must implement the data pipeline pattern: fetch data from LibreNMS API, cache results withCacheMixinkeys likelibrenms_{data_type}_{model_name}_{pk}, compare against NetBox objects, and render a django-tables2 table in a partial template.
Base table view classes must implement resource-specific comparison logic: interface matching by name, IP matching by address/mask, VLAN matching by VID+group, and cables by matching remote devices and checking cable status.
VlanAssignmentMixinmust resolve VLAN group scope in order: Rack → Location → Site → SiteGroup → Region → Global, and must provide auto-selection of the most-specific VLAN group and lookup map building for interface and VLAN sync.
Files:
netbox_librenms_plugin/views/base/cables_view.py
🧠 Learnings (19)
📚 Learning: 2026-03-07T22:46:57.537Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 21
File: netbox_librenms_plugin/tests/test_librenms_id.py:184-184
Timestamp: 2026-03-07T22:46:57.537Z
Learning: Do not flag or request style-only changes (such as removing redundant imports or cosmetic cleanup) in Python code reviews. Focus on issues that affect correctness, functionality, or security. This guideline applies to Python files across the repository, including tests (e.g., netbox_librenms_plugin/tests/test_librenms_id.py).
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-03-08T13:09:49.031Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/forms.py:75-79
Timestamp: 2026-03-08T13:09:49.031Z
Learning: In multi-server caching within the codebase, ensure poller group choices and similar cache discriminators use api.server_key as the cache key component (e.g., cache_key = f"librenms_poller_group_choices_{api.server_key}") rather than api.librenms_url. This aligns with the fixed approach seen in commit bf37f07 and with other modules. Apply this pattern consistently to Python files under netbox_librenms_plugin (and similar multi-server cache keys) to maintain correct cross-server caching behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-05-05T09:51:15.707Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/views/base/modules_view.py:311-313
Timestamp: 2026-05-05T09:51:15.707Z
Learning: In this repository, if you see `timezone.timedelta` used from `django.utils.timezone`, do not request a diff to replace it with `datetime.timedelta` solely on style grounds. This usage is functionally correct because Django exposes `timedelta` via `django.utils.timezone`; treat this as intentional and avoid churn unless there is evidence of changed/incorrect behavior (e.g., `timezone` is not Django’s module or `timedelta` is unavailable).
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-06-01T13:35:47.228Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/migrate.py:177-181
Timestamp: 2026-06-01T13:35:47.228Z
Learning: When reviewing this plugin’s permission checks, note that `check_object_permissions` / `NetBoxObjectPermissionMixin` enforce only **model-level** permissions: they call `request.user.has_perm(perm)` without any object/row instance, and the plugin does not currently implement per-object (row-level) permission scoping. Therefore, do **not** flag “missing winner-side/per-object object-permission checks” in sync/migrate views (or elsewhere in the plugin) as a defect; per-object permission scoping is an intentional plugin-wide design gap to be addressed in a dedicated future PR.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-03-27T02:04:22.276Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/tests/test_coverage_api.py:893-939
Timestamp: 2026-03-27T02:04:22.276Z
Learning: For unit tests in this repo (e.g., coverage API tests), when testing a happy-path call like `add_device()`, assert both the success flag and the expected success message (e.g., `assert ok is True` and `assert msg == "Device added successfully."`). This ensures the test fails if `add_device()` returns `(False, ...)`. If a related assertion is explicitly tracked as a known deferred follow-up for a prior PR, do not treat the missing `ok is True` assertion as a new review finding in subsequent reviews.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-06-02T11:11:56.131Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_actions.py:4773-4776
Timestamp: 2026-06-02T11:11:56.131Z
Learning: When application code performs a function-local import inside a method body (e.g., `from utilities.permissions import get_permission_for_model`), unit tests should patch the original source attribute (`utilities.permissions.get_permission_for_model`). Do not patch the consumer module’s name (e.g., `netbox_librenms_plugin.views.imports.actions.get_permission_for_model`) unless the function is imported at module scope and exposed as a module attribute—local imports re-resolve the attribute at call time.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-06-02T20:43:51.604Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_coverage_device_operations.py:2466-2478
Timestamp: 2026-06-02T20:43:51.604Z
Learning: When reviewing tests under netbox_librenms_plugin/tests, don’t treat intentional stubs/mocks of lower-layer helper functions as a “coverage hole” if the test’s goal is to isolate and verify only the validate-layer (or another single unit of behavior). If the stubbed helper’s actual logic is exercised in dedicated tests at the helper/service layer (e.g., test_*_helper* / test_librenms_id.py), it’s acceptable for the validate-layer test to control helper outputs (via side_effect/return values) and assert the validate-layer mapping/selection logic only. Flag only when the stub hides untested logic that should belong to the unit under test (i.e., the test asserts behavior from the helper without actually verifying the unit’s own responsibility).
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-04-01T15:55:42.180Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/tests/test_coverage_actions.py:88-171
Timestamp: 2026-04-01T15:55:42.180Z
Learning: When unit/integration testing actions that indirectly use a function imported at module import time, patch the function where it is *used* (the consumer’s import path), e.g. `netbox_librenms_plugin.views.imports.actions.resolve_naming_preferences`, rather than its original definition. For tests that target the function itself directly, patch the original dependency/definition (e.g. `netbox_librenms_plugin.utils.get_user_pref` or patch `resolve_naming_preferences` at `netbox_librenms_plugin.utils`) so the function under test sees the mocked behavior.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-05-05T09:46:17.700Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 58
File: netbox_librenms_plugin/tests/test_vm_operations.py:46-46
Timestamp: 2026-05-05T09:46:17.700Z
Learning: When the code under test performs *lazy imports* inside function bodies (i.e., the imported symbol is not bound at the module scope), mock/patch the *source module path that the function imports from*, not the consumer module path. The correct patch target is where the imported name is resolved at runtime (e.g., `virtualization.models.VirtualMachine`), because patching `netbox_librenms_plugin.import_utils.vm_operations.VirtualMachine` can fail with `AttributeError` since `VirtualMachine` is never a `vm_operations` module attribute.
Applied to files:
netbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.py
📚 Learning: 2026-03-07T10:40:38.106Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/interfaces.py:241-244
Timestamp: 2026-03-07T10:40:38.106Z
Learning: In netbox_librenms_plugin/views/sync/interfaces.py, ensure that set_librenms_device_id does not apply the legacy bare-integer guard to Interface/VMInterface objects. The guard is only relevant for Device/VM objects with pre-existing bare integers from before multi-server support. Interfaces/VMInterfaces have librenms_id starting empty and their port_id is always written from the LibreNMS API JSON response, so there is no migration concern. Do not treat the warning-log path as a silent no-op for interfaces; keep appropriate logging/alerts active. Add or adjust tests to verify that interfaces paths write port_id correctly and do not trigger the legacy-bare-int logic, and document this distinction in code comments.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.py
📚 Learning: 2026-03-07T17:17:04.217Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/sync/cables.py:160-165
Timestamp: 2026-03-07T17:17:04.217Z
Learning: In Python views under netbox_librenms_plugin/views/sync, when obtaining a server_key for cache namespace scoping, read it from request.POST with a fallback to self.librenms_api.server_key (e.g., server_key = request.POST.get("server_key") or self.librenms_api.server_key) and assign it to an attribute (e.g., self._post_server_key) used by get_cached_links_data to build the cache key. Do not flag or remove this POST-read pattern, as it ensures consistent, future-proof cache namespace scoping for link data lookups. Apply this guidance to similar Sync views in the same module where server_key-based cache scoping is used.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-04-15T12:38:49.280Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 50
File: netbox_librenms_plugin/views/base/modules_view.py:133-141
Timestamp: 2026-04-15T12:38:49.280Z
Learning: Do not require or flag an explicit `permission_required = PERM_VIEW_PLUGIN` on views that inherit from `LibreNMSPermissionMixin` (e.g., those ultimately including `BaseModuleTableView` in `views/base/modules_view.py`). `LibreNMSPermissionMixin` is the authoritative place where `permission_required` is set to `PERM_VIEW_PLUGIN`; inherited values are already enforced via the mixin, making a redundant override unnecessary.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-05-17T11:32:40.631Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 76
File: netbox_librenms_plugin/views/object_sync/devices.py:178-180
Timestamp: 2026-05-17T11:32:40.631Z
Learning: In this plugin’s JSON endpoint views (e.g., NetBox view classes/functions under netbox_librenms_plugin/views/**), do not require a try/except JSONDecodeError around `json.loads(request.body)` by default if the endpoint is already protected by CSRF + session authentication and has object-level permission gates. Treat missing JSONDecodeError handling as acceptable when the only expected downside is a noisy log line. If you identify any additional security or availability impact from invalid JSON (e.g., unhandled exceptions leading to user-visible 500s, potential DoS amplification, or lack of appropriate throttling/validation), then flag it and recommend adding guarded parsing/validation.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-06-01T15:12:26.824Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/sync/ip_addresses.py:94-103
Timestamp: 2026-06-01T15:12:26.824Z
Learning: For any redirect/tab URL building in netbox_librenms_plugin/views/sync, views/base, and views/object_sync, propagate the active multi-server `server_key` as a `?server_key=<key>` query parameter so users return to the same server’s tab after POST actions. When handling POST requests, read the POST-scoped `server_key` from `request.POST` and store it (e.g., `self._post_server_key`) with a fallback to `self.librenms_api.server_key`; use this POST-scoped key for both cache-key scoping and for constructing the redirect/tab URLs. Treat this as the intentional codebase-wide convention—do not flag the presence/usage of the `server_key` query parameter (or the corresponding POST-scoped `_post_server_key` pattern) in these views as an error.
Applied to files:
netbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/sync/modules.pynetbox_librenms_plugin/views/sync/migrate.py
📚 Learning: 2026-03-07T09:14:06.791Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 20
File: netbox_librenms_plugin/views/base/cables_view.py:324-331
Timestamp: 2026-03-07T09:14:06.791Z
Learning: In netbox_librenms_plugin/views/base/cables_view.py, do not treat cache.ttl() usage as a portability issue. NetBox requires Redis as the cache backend (since NetBox v2.6), so django-redis cache.ttl() and cache.pttl() extensions are available. Consider this as a project-specific guideline: cache.ttl() is intentional/safe in this codebase.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-06-05T07:19:49.079Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/views/base/interfaces_view.py:158-165
Timestamp: 2026-06-05T07:19:49.079Z
Learning: When building OOB relationships from interface/device view code, call get_librenms_oob() using the resolved sync device (e.g., `lookup_device = get_librenms_sync_device(obj, server_key=...) or obj; oob = get_librenms_oob(lookup_device, ...)`) rather than calling get_librenms_oob(obj, ... ) directly. For VC members, OOB data (including shared-LOM markers) is stored on the resolved sync device, so resolving first is required to avoid dropping OOB rows.
Applied to files:
netbox_librenms_plugin/views/base/cables_view.py
📚 Learning: 2026-03-08T14:17:28.826Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 22
File: netbox_librenms_plugin/import_utils/virtual_chassis.py:73-80
Timestamp: 2026-03-08T14:17:28.826Z
Learning: In Python code, when a lookup returns None (including API failures), implement negative caching by storing an empty result with a configurable TTL (default 5 minutes). Document the TTL and ensure a force_refresh=True bypasses the cache for manual re-fetch actions. Do not treat caching None/empty results on API failure as a bug if this mirrors existing patterns (e.g., get_device_with_server caching None on not-found). Apply this guidance to files within netbox_librenms_plugin/import_utils where similar inventory/API lookups occur.
Applied to files:
netbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-03-09T20:10:48.502Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 25
File: netbox_librenms_plugin/import_utils/device_operations.py:496-575
Timestamp: 2026-03-09T20:10:48.502Z
Learning: In netbox_librenms_plugin/import_utils/device_operations.py, in validate_device_for_import(), ensure both the cluster-required blocker (VM path) and the device_role-required blocker (device path) are guarded with if not result.get('existing_device') to ensure create-time prerequisites are only appended for new imports, not for link/update flows; keep available_roles and available_clusters populated for both new and existing-device cases so UI dropdowns function correctly on update views.
Applied to files:
netbox_librenms_plugin/import_utils/bulk_import.py
📚 Learning: 2026-05-22T19:37:46.167Z
Learnt from: marcinpsk
Repo: marcinpsk/netbox-librenms-plugin PR: 79
File: netbox_librenms_plugin/tests/test_import_utils.py:1763-1766
Timestamp: 2026-05-22T19:37:46.167Z
Learning: In `validate_device_for_import` (device operations / serial-diff name-resolution logic), preserve the following result contract so the UI/test expectations remain stable:
- If `serial_action` is determined via a serial match AND the existing device has NO OOB/LibreNMS link, set `serial_action` to `"oob_candidate"` and ensure `promote_to_host` is NOT present in the returned dict.
- Populate `promote_to_host` only when the existing device already has an OOB/LibreNMS link (i.e., a host id is available to inherit from); otherwise omit the key.
- Always include `serial_role_choice_available` in the returned dict, defaulting to `False` (baseline) even when other resolution outcomes do not enable it.
Applied to files:
netbox_librenms_plugin/import_utils/bulk_import.py
🔇 Additional comments (19)
netbox_librenms_plugin/tests/test_coverage_tables.py (1)
1371-1373: LGTM!Also applies to: 1386-1388
docs/usage_tips/oob_management.md (1)
22-22: LGTM!netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py (1)
299-320: LGTM!netbox_librenms_plugin/tests/test_coverage_base_views2.py (1)
23-37: LGTM!Also applies to: 111-123, 296-351, 694-736, 1244-1371
netbox_librenms_plugin/views/sync/interfaces.py (1)
161-167: LGTM!netbox_librenms_plugin/views/base/cables_view.py (1)
600-614: LGTM!netbox_librenms_plugin/tests/test_coverage_bulk_import.py (1)
1089-1114: LGTM!netbox_librenms_plugin/tests/test_migrate_views.py (1)
566-582: LGTM!netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js (1)
1293-1298: LGTM!netbox_librenms_plugin/tests/test_sync_modules.py (1)
5667-5702: LGTM!netbox_librenms_plugin/import_utils/bulk_import.py (1)
641-649: LGTM!netbox_librenms_plugin/views/sync/modules.py (7)
1276-1282: LGTM!
1652-1652: LGTM!
1665-1665: LGTM!
1673-1673: LGTM!
1688-1688: LGTM!
1705-1705: LGTM!
1815-1815: LGTM!netbox_librenms_plugin/views/sync/migrate.py (1)
610-615: LGTM!
…in cache refresh; require OOB fetch success before caching empty OOB-only links - _refresh_existing_device now mirrors validate_device_for_import's cross-model collision check: raise AmbiguousLibreNMSIdError when the same (server_key, librenms_id) resolves on both a Device and a VirtualMachine, instead of binding to whichever model is queried first. - get_links_data OOB-only exemption now also requires the OOB fetch to have succeeded; a failed/malformed OOB fetch returns None so a transient failure doesn't overwrite the cache with an empty snapshot and drop stale OOB rows. - Harden the open-redirect-barrier test: pin get_librenms_sync_device so lookup_device is deterministic (matches sibling post() tests).
marcinpsk
commented
Jun 15, 2026
…move-to-winner swap and test contracts - validate_device_for_import merge block: re-validate the CURRENT side (hostname/ serial) with the same unique [:2] guard as the peer, so a duplicate-name/serial .first() match can't pair the user with an arbitrary merge target; warn + skip instead. - _interface_sync_content.html: add hx-swap="none" to the move-to-winner button so an OOB-only/empty move response can't clear the action button. - test_librenms_id _qs_returning: enforce the [:2] slice contract so a regression to [:1] fails loudly. - test_vlan_sync: encode server_key in the cache-key mocks so the eviction asserts actually prove the POSTed 'prod' scope (not a constant that passes regardless).
…etBox-only link copy - _cable_sync_content.html: migrated donor mode rendered a bare <div> dropping the CSRF token and server_key, but handleCableChange()'s verify-cable fetch reads both from the DOM — emit standalone hidden inputs (mirrors _interface_sync_content.html) so those JS requests don't hit a null token (TypeError/403) or the wrong server. - _interface_sync_content.html: the NetBox-only modal is transfer-only in migrated mode, so branch the trigger's title copy to 'view and move' (vs 'view and delete') to match.
…vice; harden device-field tests - import_single_device: an ambiguous librenms_id has existing_device=None, so the existing-device guard didn't catch it and a manual_mappings import could still create a duplicate Device under the ambiguous id. Add an explicit ambiguous_librenms_id guard that blocks the create. Real-DB test asserts no Device is created. - test_coverage_device_fields: pin three real behaviours the mocks otherwise let slip — full_clean() before save in the reused-platform mapping path, >=2 nested atomic() calls in the IntegrityError retry, and that the write-site add_platformmapping permission check ran.
…-mode block
- _refresh_existing_device: the vanished-link and deleted-device branches recomputed
readiness but deferred re-asserting the create-time role/cluster blocker to the fresh
lookup, which early-returns when libre_device is None (or its except swallows) — so a
dropped-match row could stay importable with no role/cluster. Re-assert the blocker in
both drop branches before recompute. Real-DB regression test.
- apply_merge_candidates: set is_ready=False alongside can_import=False so a stale
is_ready=True (from hostname-first processing) can't leave contradictory state.
- test_coverage_base_views: seed POST server_key + assert rebind_api_for_server('prod') so
the open-redirect regression proves post() actually reads the submitted key.
…pstream review)
- cables_view: malformed OOB links no longer early-return links_data (which bypassed the
final None classification and cleared cached rows on an OOB-only device); fall through.
- interfaces_view: validate the main ports payload (dict + list of dict rows) before
enrichment, mirroring the OOB branch — a malformed 200 now fails closed, not 500.
- modules_view: run the transceiver type-check before the emptiness check so an empty
non-list ({}) is treated as malformed, not a successful 'no transceivers' response.
- librenms_api: legacy single-server mode now rejects an EXPLICIT non-default server_key
(fail closed) instead of using the default URL/token under a bogus cache/CF scope.
- device_operations: route _describe_existing_librenms_link host-id through the canonical
get_librenms_device_id accessor (read-only), keeping only OOB-subobject parsing local.
- device_status: use coerce_librenms_id (rejects bool/float) for paired host/OOB IDs.
- tests: malformed-payload + legacy-key red/green coverage, boolean vrf_id rejection,
sync_views2 docstring correction.
…ols; test hardening - device_status: the OOB-linked title used a raw int() for the paired host id, bypassing the strict _coerce_pair_id() the host-half branch uses — a bool/float id could render a bogus 'LibreNMS #1'. Reuse _coerce_pair_id for a single id contract. - librenms_import.js: the no-Bootstrap modal fallback called preventDefault() on every nested dismiss control, cancelling submit/hx-* actions on dismiss buttons that also act. Only suppress default for inert dismiss controls. - tests: whitespace-tolerant full-sync server_key assertion; pin request.htmx explicitly in the migrate request builders so a truthy MagicMock can't mis-route the non-HTMX path.
_build_interface_maps hand-rolled obj.virtual_chassis.members.values_list(...). Add a shared get_virtual_chassis_members(device) helper (returns all member Devices, or [device] when not in a VC) and use it, so the IP-sync member set can't drift from the rest of the plugin's VC handling. Behavior-preserving: added real-VC characterization tests (cross-member port-id/name resolution, cross-member name ambiguity, non-VC isolation) that pass identically before and after the refactor, plus a direct helper test. (interfaces.py's two member-expansion sites are develop-owned and left for a develop-side pass to route through the same helper.)
…de CSRF/transfer gating - collisions.detect_bulk_collisions: skip non-dict rows / non-dict validation instead of crashing the whole bulk-confirm flow on .get(). - device_status actions render: type-check existing_librenms_link before reading pairing keys so a malformed payload can't break the table render. - _vlan_sync_content.html: render CSRF + server_key in migrated mode too (the VLAN verify JS reads csrfmiddlewaretoken and posts server_key); only the form-submit action input stays gated. - librenms_sync_base.html: gate the donor transfer-IP buttons to Device pages (object|meta model_name == device) so a VM can't drive device_transfer_ip on a same-pk Device. - tests: red->green coverage for each; add non-dict main-ports row case; pin OOB-only fixture librenms_id=None; drop brittle '?tab=' >=5 sanity threshold.
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
netbox_librenms_plugin/librenms_api.py (1)
75-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip malformed entries when choosing the auto-fallback server.
get_available_servers()now omits non-mapping configs, but the default fallback still picks the first raw key. With{"bad": None, "prod": {...}},LibreNMSAPI()falls back to"bad"and raises before reaching the valid server.Suggested fix
- first_key = next(iter(servers_config), None) + first_key = next( + (key for key, config in servers_config.items() if isinstance(config, dict)), + None, + ) if first_key: logger.info( "Server '%s' not found in config, falling back to '%s'",🤖 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 `@netbox_librenms_plugin/librenms_api.py` around lines 75 - 82, The fallback server selection logic in the code around `next(iter(servers_config), None)` picks the first raw key from servers_config without validating that it corresponds to a valid server configuration. Since `get_available_servers()` filters out non-mapping configs, the fallback should do the same by iterating through servers_config to find the first key that maps to a valid (non-None, non-malformed) configuration entry instead of blindly taking the first raw key. Update the logic to skip over invalid entries and select the first valid server configuration to prevent falling back to malformed entries like ones with None values.netbox_librenms_plugin/views/base/ip_addresses_view.py (1)
191-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard the
get_port_by_id()payload before indexing it.A truthy
successwithport_data=None,{"port": {}}, or{"port": ["bad"]}still raises here or later atport_info.get(...), bypassing the malformed-payload fail-closed behavior added around the IP fetch path.🛡️ Proposed guard
if port_id not in port_data_cache: success, port_data = self.librenms_api.get_port_by_id(port_id) - if success and "port" in port_data and port_data["port"]: - port_data_cache[port_id] = port_data["port"][0] - else: - port_data_cache[port_id] = None + ports = port_data.get("port") if success and isinstance(port_data, dict) else None + first_port = ports[0] if isinstance(ports, list) and ports else None + port_data_cache[port_id] = first_port if isinstance(first_port, dict) else None🤖 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 `@netbox_librenms_plugin/views/base/ip_addresses_view.py` around lines 191 - 194, The guard clause before indexing port_data["port"][0] is insufficient and does not properly validate the structure of the port_data payload. Even when success is True, port_data could be None, contain an empty dict for "port", or have a non-list value. Strengthen the guard condition in the get_port_by_id() call block to additionally verify that port_data is a non-None dict, that port_data["port"] is a list (not just truthy), and that the list contains at least one element before attempting to access the first element with [0]. This ensures malformed payloads are properly rejected and prevents IndexError or type errors from occurring in subsequent port_info.get(...) calls.netbox_librenms_plugin/views/base/modules_view.py (1)
489-506:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate cached inventory shape before
_build_context().
post()now rejects malformed API payloads before caching, but the read path still accepts any cached"inventory"value. A stale pre-fix cache entry like{"inventory": [None], ...}reaches_build_context()and crashes onitem.get(...); mirror the list-of-dicts guard here before using the payload.🛡️ Proposed fix
cached_payload = cache.get(cache_key) if not isinstance(cached_payload, dict) or "inventory" not in cached_payload: cache.delete(cache_key) return {"table": None, "object": obj, "cache_expiry": None, "server_key": self.librenms_api.server_key} + cached_inventory = cached_payload.get("inventory") + if not isinstance(cached_inventory, list) or any(not isinstance(item, dict) for item in cached_inventory): + cache.delete(cache_key) + return {"table": None, "object": obj, "cache_expiry": None, "server_key": self.librenms_api.server_key} # Validate that the cached inventory was built for the same LibreNMS device. # If the object has been remapped to a different device, discard stale inventory. current_librenms_id = self.librenms_api.get_librenms_id(sync_device) @@ - return self._build_context(request, obj, cached_payload["inventory"]) + return self._build_context(request, obj, cached_inventory)🤖 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 `@netbox_librenms_plugin/views/base/modules_view.py` around lines 489 - 506, The code validates that the cached inventory exists and that device/OOB mappings are current, but does not validate the structure of the inventory data itself before passing it to _build_context(). A stale cache entry with malformed inventory (such as a list containing None instead of a list of dictionaries) will crash when _build_context() tries to call get() on the items. Add a validation check for the cached_payload["inventory"] to ensure it is a list of dictionaries before the final return statement that calls _build_context(); if validation fails, delete the cache and return the early-exit response matching the pattern used for the librenms_id and oob_librenms_id checks.
🤖 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 `@netbox_librenms_plugin/import_utils/bulk_import.py`:
- Around line 671-679: When a previously unmatched row resolves to an existing
object, the code currently only removes "role" and "cluster" validation blockers
using remove_validation_issue(). However, other stale create-time blockers such
as site and device type selection issues can persist, leaving validation
inconsistent. After the existing calls to remove_validation_issue() for "role"
and "cluster", add additional remove_validation_issue() calls to clear all other
stale create-time blockers (such as "site" and "device_type" or their equivalent
validation issue names) that may have been accumulated from the previous
new-import validation attempt.
- Around line 365-377: The current logic treats a missing scanned_id (None) the
same as a scanned_id that no longer matches, incorrectly clearing valid cached
matches. Modify the conditional structure to only clear the
validation["existing_match_type"] to None when scanned_id is actually not None
but fails to match both the oob_id and link["host_id"]. If scanned_id is None,
the code should not modify the existing match type, leaving the prior cached
match unchanged until a current LibreNMS ID can be compared.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.html`:
- Around line 28-31: The multiline comment block at lines 28-31 currently uses
the inline comment syntax {# ... #}, which is causing a CI failure. Replace this
entire comment block (which discusses CSRF and server_key rendering in both
modes) with Django's multiline comment tag syntax by using {% comment %} at the
start and {% endcomment %} at the end, preserving all the comment text content
inside.
In
`@netbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.html`:
- Around line 61-62: The multiline short template comment using {# #} syntax on
lines 61-62 is failing the pipeline check. Replace this multiline {# #} comment
block with either a {% comment %}...{% endcomment %} block to wrap the
multi-line comment, or if the comment is short enough, convert it to a
single-line {# ... #} syntax. Use the {% comment %} block approach to preserve
the full comment text about device_transfer_ip resolving a Device by pk and the
reason for gating transfer buttons.
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Around line 573-585: The current validation for object_id and vrf_id allows
float values to be silently coerced to integers (e.g., int(1.9) becomes 1),
which could map to unintended objects instead of returning a 400 error. Before
attempting int() conversion on both object_id and vrf_id, add validation to
reject float inputs by checking if the value is an instance of float and
returning a 400 error response with an appropriate message. This check must
occur before the try-except blocks that call int() on object_id and int() on
vrf_id in the else clause, ensuring that fractional values are explicitly
rejected rather than silently truncated.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 803-806: To avoid rebuilding a failed default API client during
graceful server rebind failures, you need to fix two locations. In
netbox_librenms_plugin/views/sync/device_fields.py lines 803-806, replace the
fallback access through the self.librenms_api property with direct access to the
private _librenms_api attribute using getattr(getattr(self, "_librenms_api",
None), "server_key", ""), or alternatively pass an explicit resolved fallback
parameter into _sync_url() instead of invoking the property. In
netbox_librenms_plugin/views/sync/ip_addresses.py lines 98-101, after a None
rebind, either redirect to the base IP tab without calling a helper that would
re-instantiate self.librenms_api, or update the get_ip_tab_url() helper to only
use _post_server_key and _librenms_api when an actual server was resolved.
---
Outside diff comments:
In `@netbox_librenms_plugin/librenms_api.py`:
- Around line 75-82: The fallback server selection logic in the code around
`next(iter(servers_config), None)` picks the first raw key from servers_config
without validating that it corresponds to a valid server configuration. Since
`get_available_servers()` filters out non-mapping configs, the fallback should
do the same by iterating through servers_config to find the first key that maps
to a valid (non-None, non-malformed) configuration entry instead of blindly
taking the first raw key. Update the logic to skip over invalid entries and
select the first valid server configuration to prevent falling back to malformed
entries like ones with None values.
In `@netbox_librenms_plugin/views/base/ip_addresses_view.py`:
- Around line 191-194: The guard clause before indexing port_data["port"][0] is
insufficient and does not properly validate the structure of the port_data
payload. Even when success is True, port_data could be None, contain an empty
dict for "port", or have a non-list value. Strengthen the guard condition in the
get_port_by_id() call block to additionally verify that port_data is a non-None
dict, that port_data["port"] is a list (not just truthy), and that the list
contains at least one element before attempting to access the first element with
[0]. This ensures malformed payloads are properly rejected and prevents
IndexError or type errors from occurring in subsequent port_info.get(...) calls.
In `@netbox_librenms_plugin/views/base/modules_view.py`:
- Around line 489-506: The code validates that the cached inventory exists and
that device/OOB mappings are current, but does not validate the structure of the
inventory data itself before passing it to _build_context(). A stale cache entry
with malformed inventory (such as a list containing None instead of a list of
dictionaries) will crash when _build_context() tries to call get() on the items.
Add a validation check for the cached_payload["inventory"] to ensure it is a
list of dictionaries before the final return statement that calls
_build_context(); if validation fails, delete the cache and return the
early-exit response matching the pattern used for the librenms_id and
oob_librenms_id checks.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 35ab631b-2ee0-44ec-b3c6-736bf3aef035
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.gitignoredocs/SUMMARY.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/custom_field.mddocs/usage_tips/oob_management.mdmkdocs.ymlnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_oob_interface_select.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/create_platform_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_cable_sync_content_template.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
marcinpsk
commented
Jun 16, 2026
… migrated-mode CSRF
- bulk_import: a missing scanned device_id no longer clears a still-live cached
librenms_id match; only drop it when the DB linkage is genuinely gone.
- bulk_import: clear stale site/device-type create-time blockers (not just
role/cluster) when a row resolves to an existing match.
- ip verify: reject JSON float device_id/vrf_id before int() truncation.
- sync redirects: on the no-server_key fallback, prefer the already-bound
_librenms_api; only when nothing is bound (failed rebind) resolve the default
via the librenms_api property so the redirect still carries the resolved
server_key, guarded so a misconfigured default degrades gracefully instead of
500ing.
- modules read path: mirror the post-side fail-closed inventory-shape guard so a
stale malformed cache entry can't crash _build_context.
- templates: convert multiline {# #} comments to {% comment %} (CI lint); emit a
standalone CSRF + server_key in migrated mode for the module fragment so the
verify-module JS POST keeps working.
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@netbox_librenms_plugin/librenms_api.py`:
- Around line 83-96: Remove the entire elif block that raises KeyError when an
explicit non-default server_key is requested in legacy single-server mode. The
stricter unknown-key contract documented in the PR is only for multi-server
mode, so legacy single-server mode should tolerate explicit keys gracefully.
Delete the condition checking explicit_server_key, servers_config, and
server_key != "default" along with the KeyError raise, allowing the fallback
behavior to handle the request instead of forcing build_librenms_api() to return
None.
In `@netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.js`:
- Around line 1307-1308: The code calls `.closest()` on event.target without
checking if it's an Element first. Since the keydown listener on document can
receive document as the event target when no element has focus, and Document
lacks the .closest() method, this causes a TypeError. Add an instanceof Element
guard before calling .closest() on event.target in the line where
eventStartedInNestedModal is assigned, ensuring that the .closest() method is
only called if event.target is actually an Element instance, otherwise assign
eventStartedInNestedModal to null or false.
In `@netbox_librenms_plugin/views/sync/device_fields.py`:
- Around line 800-823: The fallback logic at lines 803-817 only executes when
`requested` is empty, but if a stale POST value is present for `server_key`, it
bypasses this fallback even if that stale value is not configured. Restructure
the validation to first check if the `requested` value exists in the configured
servers (using the logic currently at lines 820-822 with
LibreNMSAPI.get_available_servers()), and only if it is not found in the
available servers should the fallback logic at lines 803-817 execute to retrieve
the bound `_librenms_api` or default server. This ensures the fallback is used
both when no POST value is provided AND when the POST value is unconfigured,
preventing stale values from bypassing the fallback path.
In `@netbox_librenms_plugin/views/sync/ip_addresses.py`:
- Around line 313-315: The _set_primary_ip method call at this location can
incorrectly assign a device's primary IP to an interface belonging to a sibling
VC member device, since _build_interface_maps() intentionally indexes interfaces
from all VC members. Add a guard condition before the _set_primary_ip call to
verify that the interface object (ip_obj) belongs to the same device as obj by
checking their owner devices match, ensuring the primary IP assignment only
happens for same-owner interfaces and not for sibling VC member interfaces.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: c34db59e-2eae-43f8-902e-f68f7c1f672f
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (86)
.gitignoredocs/SUMMARY.mddocs/feature_list.mddocs/librenms_import/validation.mddocs/usage_tips/custom_field.mddocs/usage_tips/oob_management.mdmkdocs.ymlnetbox_librenms_plugin/constants.pynetbox_librenms_plugin/import_utils/__init__.pynetbox_librenms_plugin/import_utils/bulk_import.pynetbox_librenms_plugin/import_utils/collisions.pynetbox_librenms_plugin/import_utils/device_operations.pynetbox_librenms_plugin/import_validation_helpers.pynetbox_librenms_plugin/librenms_api.pynetbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_import.jsnetbox_librenms_plugin/tables/cables.pynetbox_librenms_plugin/tables/device_status.pynetbox_librenms_plugin/tables/interfaces.pynetbox_librenms_plugin/tables/ipaddresses.pynetbox_librenms_plugin/tables/modules.pynetbox_librenms_plugin/templates/netbox_librenms_plugin/_cable_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_ipaddress_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_module_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/_vlan_sync_content.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_dt_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_oob_interface_select.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/_platform_mapping_form.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/bulk_import_collision.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/create_platform_modal.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_import_row.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/htmx/device_validation_details.htmlnetbox_librenms_plugin/templates/netbox_librenms_plugin/librenms_sync_base.htmlnetbox_librenms_plugin/tests/conftest.pynetbox_librenms_plugin/tests/test_cable_sync_content_template.pynetbox_librenms_plugin/tests/test_collisions.pynetbox_librenms_plugin/tests/test_coverage_actions.pynetbox_librenms_plugin/tests/test_coverage_base_views.pynetbox_librenms_plugin/tests/test_coverage_base_views2.pynetbox_librenms_plugin/tests/test_coverage_bulk_import.pynetbox_librenms_plugin/tests/test_coverage_device_fields.pynetbox_librenms_plugin/tests/test_coverage_device_operations.pynetbox_librenms_plugin/tests/test_coverage_devices.pynetbox_librenms_plugin/tests/test_coverage_mixins.pynetbox_librenms_plugin/tests/test_coverage_sync_interfaces.pynetbox_librenms_plugin/tests/test_coverage_sync_view.pynetbox_librenms_plugin/tests/test_coverage_sync_views.pynetbox_librenms_plugin/tests/test_coverage_sync_views2.pynetbox_librenms_plugin/tests/test_coverage_tables.pynetbox_librenms_plugin/tests/test_coverage_utils.pynetbox_librenms_plugin/tests/test_import_utils.pynetbox_librenms_plugin/tests/test_import_validation_helpers.pynetbox_librenms_plugin/tests/test_interface_sync_content_template.pynetbox_librenms_plugin/tests/test_ip_verify.pynetbox_librenms_plugin/tests/test_librenms_api.pynetbox_librenms_plugin/tests/test_librenms_id.pynetbox_librenms_plugin/tests/test_migrate_views.pynetbox_librenms_plugin/tests/test_module_sync_content_template.pynetbox_librenms_plugin/tests/test_modules_view.pynetbox_librenms_plugin/tests/test_permissions.pynetbox_librenms_plugin/tests/test_reviewer_fixes.pynetbox_librenms_plugin/tests/test_server_key_in_redirects.pynetbox_librenms_plugin/tests/test_sync_devices.pynetbox_librenms_plugin/tests/test_sync_modules.pynetbox_librenms_plugin/tests/test_sync_view_mismatch.pynetbox_librenms_plugin/tests/test_tables_modules.pynetbox_librenms_plugin/tests/test_utils.pynetbox_librenms_plugin/tests/test_vlan_sync.pynetbox_librenms_plugin/urls.pynetbox_librenms_plugin/utils.pynetbox_librenms_plugin/views/__init__.pynetbox_librenms_plugin/views/base/cables_view.pynetbox_librenms_plugin/views/base/interfaces_view.pynetbox_librenms_plugin/views/base/ip_addresses_view.pynetbox_librenms_plugin/views/base/librenms_sync_view.pynetbox_librenms_plugin/views/base/modules_view.pynetbox_librenms_plugin/views/base/vlan_table_view.pynetbox_librenms_plugin/views/imports/actions.pynetbox_librenms_plugin/views/mixins.pynetbox_librenms_plugin/views/object_sync/devices.pynetbox_librenms_plugin/views/sync/device_fields.pynetbox_librenms_plugin/views/sync/interfaces.pynetbox_librenms_plugin/views/sync/ip_addresses.pynetbox_librenms_plugin/views/sync/migrate.pynetbox_librenms_plugin/views/sync/modules.py
… fallback, VC primary-IP guard - librenms_api: drop the legacy single-server-mode KeyError for an explicit non-default key. The strict unknown-key contract is multi-server-only; legacy mode binds the single configured server (view-layer validation handles cache/CF scoping). Unbreaks build_librenms_api() in legacy mode. - device_fields._sync_url: resolve the requested server_key against the trusted config first; a stale/unconfigured POST key no longer short-circuits the active/default fallback, so the redirect keeps its server_key (re-validated through the allowlist, open-redirect safe). - ip_addresses.process_ip_sync: don't set obj.primary_ip from an address bound to a sibling VC member's interface (the interface maps span all members); record it as primary_no_interface instead of persisting an invalid primary. - librenms_import.js: guard event.target with instanceof Element before .closest() (document keydown target has no .closest()). - test(modules): pin the librenms_id/OOB cache fingerprint in the malformed- inventory guard test so it actually exercises the inventory check (was passing via an id-mismatch invalidation; now a true red->green guard).
The migrated-mode Move button emitted hx-vals='{"server_key": "..."}'
unconditionally, so a marker without a server_key POSTed an empty
{"server_key": ""} — overriding the active/default server with a blank
key on non-default installs. Wrap it in {% if migrated_to_marker.server_key %}
to match the form-mode guard (the conditional server_key hidden input).
Tests render the real template: with a key the Move button carries
hx-vals; without one it omits the payload entirely (red->green).
…aces
NetBox's ComponentModel.clean() hard-blocks any device change on an existing
component ("Components cannot be moved to a different device"), keyed on the
device_id cached into _original_device at load time. MoveInterfaceToWinnerView's
full_clean()+save() therefore rejected EVERY interface move with a 409 — the
feature could never move an interface on NetBox 4.2+.
Re-seed _original_device to the winner so only that one blanket guard is
defeated; the real cross-device parent/lag/bridge checks, name uniqueness, and
save()'s _site/_location/_rack denormalization all still run (a bare
.update(device=...) would skip them and leave stale denormalized location).
Convert the migrate view tests from MagicMock queryset-chain stubs to real-DB
end-to-end (donor/winner devices, interfaces, IPs via the conftest builders +
mark_librenms_migrated): the move, the cross-device LAG rejection, the
unique-constraint FK transfer ordering, and the reject paths now exercise the
real ORM. The mock suite masked this bug entirely — its no-op full_clean()
'moved' the interface in-memory and asserted success. Concurrency-only paths
(marker repointed under lock, save IntegrityError race) and pure guards stay
mock-based with a note.
Device primary_ip4/primary_ip6/oob_ip are persisted via save(update_fields=...) in the OOB-link, merge, and move-to-winner flows to avoid full_clean() rejecting the write over unrelated pre-existing inconsistencies (e.g. face without rack). That also skips the NetBox invariant that the address must be assigned to one of the device's OWN interfaces — so the ownership check lived by convention at each call site, and a future site that forgot it could silently persist an off-device FK. Add utils.set_device_ip_fk(device, field, ip, *, save=True) as the single guarded chokepoint: it raises ValueError for a non-None address not assigned to an interface on the device, and (by default) saves only that column. Route all four write sites through it — migrate.py _reconcile_donor_device_ip_fks + TransferDeviceIPView (save=True), actions.py OOB-link set + merge OOB transfer (save=False, batched into the existing update_fields). Callers still order release-before-claim for the UNIQUE FK. Test: TestSetDeviceIpFk (real DB) — sets/clears on an owned address, refuses an off-device address without persisting, save=False defers the write, bad field rejected. Red->green verified.
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
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
Briefly describe what this PR does in plain English, and provide as much of the following information as possible.
Motivation / Problem
What issue does this solve?
Link any related issues if applicable.
Scope of Change
Delete items that don’t apply:
How Was This Tested?
Delete items that don’t apply and describe briefly.
Manual Test Steps (if applicable)
Risk Assessment
Explain briefly.
Backwards Compatibility
Other Notes
Anything the maintainer(s) should pay particular attention to?
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
librenms_idOOB JSON format details.